WorkingTreeModifiedEvent: must be fired explicitly after merge

A merge may write files to the working tree. After a successful
merge one must fire a WorkingTreeModifiedEvent explicitly if
getModifiedFiles() is not empty.

Also, any touched files must be reported by the
WorkingTreeModifiedEvent fired by DirCacheCheckout.checkout().

Bug: 552636
Change-Id: I5fab8279ed8be8a4ae34cddfa726836b9277aea6
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

6 files changed, 73 insertions, 25 deletions

diff --git a/org.eclipse.jgit.test/src/org/eclipse/jgit/events/ChangeRecorder.java b/org.eclipse.jgit.test/src/org/eclipse/jgit/events/ChangeRecorder.java
index 228df35c76..e1f6b12258 100644
--- a/org.eclipse.jgit.test/src/org/eclipse/jgit/events/ChangeRecorder.java
+++ b/org.eclipse.jgit.test/src/org/eclipse/jgit/events/ChangeRecorder.java
@@ -43,7 +43,6 @@
 
 package org.eclipse.jgit.events;
 
-import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 
 import java.util.Arrays;
@@ -99,10 +98,12 @@ public class ChangeRecorder implements WorkingTreeModifiedListener {
 		Arrays.sort(expectedModified);
 		Arrays.sort(actuallyDeleted);
 		Arrays.sort(expectedDeleted);
-		assertArrayEquals("Unexpected modifications reported", expectedModified,
-				actuallyModified);
-		assertArrayEquals("Unexpected deletions reported", expectedDeleted,
-				actuallyDeleted);
+		assertEquals("Unexpected modifications reported",
+				Arrays.toString(expectedModified),
+				Arrays.toString(actuallyModified));
+		assertEquals("Unexpected deletions reported",
+				Arrays.toString(expectedDeleted),
+				Arrays.toString(actuallyDeleted));
 		reset();
 	}
 }
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CherryPickCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CherryPickCommandTest.java
index adeb8b839c..76958f1ddf 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CherryPickCommandTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CherryPickCommandTest.java
@@ -346,6 +346,40 @@ public class CherryPickCommandTest extends RepositoryTestCase {
 	}
 
 	@Test
+	public void testCherryPickNewFileFiresModifiedEvent() throws Exception {
+		ListenerHandle listener = null;
+		try (Git git = new Git(db)) {
+			writeTrashFile("test.txt", "a");
+			git.add().addFilepattern("test.txt").call();
+			git.commit().setMessage("commit1").call();
+			git.checkout().setCreateBranch(true).setName("a").call();
+
+			writeTrashFile("side.txt", "side");
+			git.add().addFilepattern("side.txt").call();
+			RevCommit side = git.commit().setMessage("side").call();
+			assertNotNull(side);
+
+			assertNotNull(git.checkout().setName(Constants.MASTER).call());
+			writeTrashFile("test.txt", "b");
+			assertNotNull(git.add().addFilepattern("test.txt").call());
+			assertNotNull(git.commit().setMessage("commit2").call());
+
+			ChangeRecorder recorder = new ChangeRecorder();
+			listener = db.getListenerList()
+					.addWorkingTreeModifiedListener(recorder);
+			CherryPickResult result = git.cherryPick()
+					.include(side.getId()).call();
+			assertEquals(CherryPickStatus.OK, result.getStatus());
+			recorder.assertEvent(new String[] { "side.txt" },
+					ChangeRecorder.EMPTY);
+		} finally {
+			if (listener != null) {
+				listener.remove();
+			}
+		}
+	}
+
+	@Test
 	public void testCherryPickOurCommitName() throws Exception {
 		try (Git git = new Git(db)) {
 			RevCommit sideCommit = prepareCherryPick(git);
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java
index 1b2c850192..7c8ec23848 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java
@@ -57,12 +57,9 @@ import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStreamReader;
-import java.util.ArrayList;
 import java.util.Collections;
-import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
-import java.util.Set;
 
 import org.eclipse.jgit.api.MergeResult.MergeStatus;
 import org.eclipse.jgit.api.RebaseCommand.InteractiveHandler;
@@ -78,6 +75,7 @@ import org.eclipse.jgit.errors.AmbiguousObjectException;
 import org.eclipse.jgit.errors.IllegalTodoFileModification;
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
+import org.eclipse.jgit.events.ChangeRecorder;
 import org.eclipse.jgit.events.ListenerHandle;
 import org.eclipse.jgit.junit.RepositoryTestCase;
 import org.eclipse.jgit.lib.AbbreviatedObjectId;
@@ -2014,10 +2012,9 @@ public class RebaseCommandTest extends RepositoryTestCase {
 		checkoutBranch("refs/heads/topic");
 		writeTrashFile("sub/file0", "unstaged modified file0");
 
-		Set<String> modifiedFiles = new HashSet<>();
+		ChangeRecorder recorder = new ChangeRecorder();
 		ListenerHandle handle = db.getListenerList()
-				.addWorkingTreeModifiedListener(
-						event -> modifiedFiles.addAll(event.getModified()));
+				.addWorkingTreeModifiedListener(recorder);
 		try {
 			// rebase
 			assertEquals(Status.OK, git.rebase()
@@ -2035,9 +2032,8 @@ public class RebaseCommandTest extends RepositoryTestCase {
 						+ "[sub/file0, mode:100644, content:file0]",
 				indexState(CONTENT));
 		assertEquals(RepositoryState.SAFE, db.getRepositoryState());
-		List<String> modified = new ArrayList<>(modifiedFiles);
-		Collections.sort(modified);
-		assertEquals("[file1, sub/file0]", modified.toString());
+		recorder.assertEvent(new String[] { "file1", "file2", "sub/file0" },
+				new String[0]);
 	}
 
 	@Test
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/CherryPickCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/CherryPickCommand.java
index 9573d2e21b..aa63725c55 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/api/CherryPickCommand.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/CherryPickCommand.java
@@ -160,6 +160,10 @@ public class CherryPickCommand extends GitCommand<CherryPickResult> {
 				merger.setCommitNames(new String[] { "BASE", ourName, //$NON-NLS-1$
 						cherryPickName });
 				if (merger.merge(newHead, srcCommit)) {
+					if (!merger.getModifiedFiles().isEmpty()) {
+						repo.fireEvent(new WorkingTreeModifiedEvent(
+								merger.getModifiedFiles(), null));
+					}
 					if (AnyObjectId.isEqual(newHead.getTree().getId(),
 							merger.getResultTreeId())) {
 						continue;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/RevertCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/RevertCommand.java
index ddd60b6fa2..aa0055fb53 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/api/RevertCommand.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/RevertCommand.java
@@ -58,6 +58,7 @@ import org.eclipse.jgit.api.errors.NoMessageException;
 import org.eclipse.jgit.api.errors.UnmergedPathsException;
 import org.eclipse.jgit.api.errors.WrongRepositoryStateException;
 import org.eclipse.jgit.dircache.DirCacheCheckout;
+import org.eclipse.jgit.events.WorkingTreeModifiedEvent;
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.lib.AnyObjectId;
 import org.eclipse.jgit.lib.Constants;
@@ -175,6 +176,10 @@ public class RevertCommand extends GitCommand<RevCommit> {
 						+ "This reverts commit " + srcCommit.getId().getName() //$NON-NLS-1$
 						+ ".\n"; //$NON-NLS-1$
 				if (merger.merge(headCommit, srcParent)) {
+					if (!merger.getModifiedFiles().isEmpty()) {
+						repo.fireEvent(new WorkingTreeModifiedEvent(
+								merger.getModifiedFiles(), null));
+					}
 					if (AnyObjectId.isEqual(headCommit.getTree().getId(),
 							merger.getResultTreeId()))
 						continue;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java
index 1334949eb4..1faeff2abc 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java
@@ -53,9 +53,11 @@ import java.text.MessageFormat;
 import java.time.Instant;
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import org.eclipse.jgit.api.errors.CanceledException;
 import org.eclipse.jgit.api.errors.FilterFailedException;
@@ -142,6 +144,8 @@ public class DirCacheCheckout {
 
 	private ArrayList<String> removed = new ArrayList<>();
 
+	private ArrayList<String> kept = new ArrayList<>();
+
 	private ObjectId mergeCommitTree;
 
 	private DirCache dc;
@@ -432,11 +436,11 @@ public class DirCacheCheckout {
 					if (mtime == null || mtime.equals(Instant.EPOCH)) {
 						entry.setLastModified(f.getEntryLastModifiedInstant());
 					}
-					keep(entry, f);
+					keep(i.getEntryPathString(), entry, f);
 				}
 			} else
 				// The index contains a folder
-				keep(i.getDirCacheEntry(), f);
+				keep(i.getEntryPathString(), i.getDirCacheEntry(), f);
 		} else {
 			// There is no entry in the merge commit. Means: we want to delete
 			// what's currently in the index and working tree
@@ -496,8 +500,11 @@ public class DirCacheCheckout {
 				dc.unlock();
 			} finally {
 				if (performingCheckout) {
+					Set<String> touched = new HashSet<>(conflicts);
+					touched.addAll(getUpdated().keySet());
+					touched.addAll(kept);
 					WorkingTreeModifiedEvent event = new WorkingTreeModifiedEvent(
-							getUpdated().keySet(), getRemoved());
+							touched, getRemoved());
 					if (!event.isEmpty()) {
 						repo.fireEvent(event);
 					}
@@ -826,14 +833,14 @@ public class DirCacheCheckout {
 
 				break;
 			case 0xDFD: // 3 4
-				keep(dce, f);
+				keep(name, dce, f);
 				break;
 			case 0xF0D: // 18
 				remove(name);
 				break;
 			case 0xDFF: // 5 5b 6 6b
 				if (equalIdAndMode(iId, iMode, mId, mMode))
-					keep(dce, f); // 5 6
+					keep(name, dce, f); // 5 6
 				else
 					conflict(name, dce, h, m); // 5b 6b
 				break;
@@ -863,7 +870,7 @@ public class DirCacheCheckout {
 					conflict(name, dce, h, m); // 9
 				break;
 			case 0xFD0: // keep without a rule
-				keep(dce, f);
+				keep(name, dce, f);
 				break;
 			case 0xFFD: // 12 13 14
 				if (equalIdAndMode(hId, hMode, iId, iMode))
@@ -883,7 +890,7 @@ public class DirCacheCheckout {
 					conflict(name, dce, h, m);
 				break;
 			default:
-				keep(dce, f);
+				keep(name, dce, f);
 			}
 			return;
 		}
@@ -968,7 +975,7 @@ public class DirCacheCheckout {
 					if (initialCheckout)
 						update(name, mId, mMode);
 					else
-						keep(dce, f);
+						keep(name, dce, f);
 				} else
 					conflict(name, dce, h, m);
 			}
@@ -1031,7 +1038,7 @@ public class DirCacheCheckout {
 						// Nothing in Head
 						// Something in Index
 						// -> Merge contains nothing new. Keep the index.
-						keep(dce, f);
+						keep(name, dce, f);
 				} else
 					// Merge contains something and it is not the same as Index
 					// Nothing in Head
@@ -1182,7 +1189,7 @@ public class DirCacheCheckout {
 					// to the other one.
 					// -> In all three cases we don't touch index and file.
 
-					keep(dce, f);
+					keep(name, dce, f);
 				}
 			}
 		}
@@ -1231,12 +1238,13 @@ public class DirCacheCheckout {
 		}
 	}
 
-	private void keep(DirCacheEntry e, WorkingTreeIterator f)
+	private void keep(String path, DirCacheEntry e, WorkingTreeIterator f)
 			throws IOException {
 		if (e != null && !FileMode.TREE.equals(e.getFileMode()))
 			builder.add(e);
 		if (force) {
 			if (f.isModified(e, true, this.walk.getObjectReader())) {
+				kept.add(path);
 				checkoutEntry(repo, e, this.walk.getObjectReader());
 			}
 		}