From 64f2407f19330d182728feeaf3d2c1a4092a2051 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Sat, 2 Nov 2019 19:26:42 +0100 Subject: [PATCH] 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 --- .../eclipse/jgit/events/ChangeRecorder.java | 11 +++--- .../jgit/api/CherryPickCommandTest.java | 34 +++++++++++++++++++ .../eclipse/jgit/api/RebaseCommandTest.java | 14 +++----- .../eclipse/jgit/api/CherryPickCommand.java | 4 +++ .../org/eclipse/jgit/api/RevertCommand.java | 5 +++ .../jgit/dircache/DirCacheCheckout.java | 30 ++++++++++------ 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 228df35c7..e1f6b1225 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 void assertEvent(String[] expectedModified, 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 adeb8b839..76958f1dd 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 @@ -345,6 +345,40 @@ public void testCherryPickConflictFiresModifiedEvent() throws Exception { } } + @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)) { 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 1b2c85019..7c8ec2384 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.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.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 void testRebaseWithAutoStashAndSubdirs() throws Exception { checkoutBranch("refs/heads/topic"); writeTrashFile("sub/file0", "unstaged modified file0"); - Set 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 void testRebaseWithAutoStashAndSubdirs() throws Exception { + "[sub/file0, mode:100644, content:file0]", indexState(CONTENT)); assertEquals(RepositoryState.SAFE, db.getRepositoryState()); - List 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 9573d2e21..aa63725c5 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 CherryPickResult call() throws GitAPIException, NoMessageException, 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 ddd60b6fa..aa0055fb5 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.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 RevCommit call() throws NoMessageException, UnmergedPathsException, + "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 1334949eb..1faeff2ab 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.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 CheckoutMetadata(EolStreamType eolStreamType, private ArrayList removed = new ArrayList<>(); + private ArrayList kept = new ArrayList<>(); + private ObjectId mergeCommitTree; private DirCache dc; @@ -432,11 +436,11 @@ void processEntry(CanonicalTreeParser m, DirCacheBuildIterator i, 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 boolean checkout() throws IOException { dc.unlock(); } finally { if (performingCheckout) { + Set 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 @@ void processEntry(CanonicalTreeParser h, CanonicalTreeParser m, 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 @@ void processEntry(CanonicalTreeParser h, CanonicalTreeParser m, 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 @@ void processEntry(CanonicalTreeParser h, CanonicalTreeParser m, conflict(name, dce, h, m); break; default: - keep(dce, f); + keep(name, dce, f); } return; } @@ -968,7 +975,7 @@ else if (m == null) if (initialCheckout) update(name, mId, mMode); else - keep(dce, f); + keep(name, dce, f); } else conflict(name, dce, h, m); } @@ -1031,7 +1038,7 @@ else if (m == null) // 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 @@ && isModified_IndexTree(name, iId, iMode, mId, mMode, // 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 @@ private void conflict(String path, DirCacheEntry e, AbstractTreeIterator h, Abst } } - 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()); } }