From c7685003d8fb11e39234866fd43b82b7ba883a2f Mon Sep 17 00:00:00 2001 From: Adithya Chakilam Date: Fri, 29 Jan 2021 19:35:04 -0600 Subject: [PATCH] Fix DateRevQueue tie breaks with more than 2 elements DateRevQueue is expected to give out the commits that have higher commit time. But in case of tie(same commit time), it should give the commit that is inserted first. This is inferred from the testInsertTie test case written for DateRevQueue. Also that test case, right now uses just two commits which caused it not to fail with the current implementation, so added another commit to make the test more robust. By fixing the DateRevQueue, we would also match the behaviour of LogCommand.addRange(c1,c2) with git log c1..c2. A test case for the same is added to show that current behaviour is not the expected one. By fixing addRange(), the order in which commits are applied during a rebase is altered. Rebase logic should have never depended upon LogCommand.addRange() since the intended order of addRange() is not the order a rebase should use. So, modify the RebaseCommand to use RevWalk directly with TopoNonIntermixSortGenerator. Add a new LogCommandTest.addRangeWithMerge() test case which creates commits in the following order: A - B - C - M \ / -D- Using git 2.30.0, git log B..M outputs: M C D LogCommand.addRange(B, M) without this fix outputs: M D C LogCommand.addRange(B, M) with this fix outputs: M C D Change-Id: I30cc3ba6c97f0960f64e9e021df96ff276f63db7 Signed-off-by: Adithya Chakilam --- .../org/eclipse/jgit/api/LogCommandTest.java | 47 +++++++++++++++++++ .../jgit/revwalk/DateRevQueueTest.java | 6 +++ .../org/eclipse/jgit/api/RebaseCommand.java | 21 +++++---- .../eclipse/jgit/revwalk/DateRevQueue.java | 2 +- 4 files changed, 67 insertions(+), 9 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/LogCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/LogCommandTest.java index 6460c7988..c563d5a47 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/LogCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/LogCommandTest.java @@ -232,6 +232,53 @@ public void logNoMergeCommits() throws Exception { assertFalse(i.hasNext()); } + /** + * 
+	 * A - B - C - M
+	 *      \     /
+	 *        -D(side)
+	 *
+ */ + @Test + public void addRangeWithMerge() throws Exception{ + String fileA = "fileA"; + String fileB = "fileB"; + Git git = Git.wrap(db); + + writeTrashFile(fileA, fileA); + git.add().addFilepattern(fileA).call(); + git.commit().setMessage("commit a").call(); + + writeTrashFile(fileA, fileA); + git.add().addFilepattern(fileA).call(); + RevCommit b = git.commit().setMessage("commit b").call(); + + writeTrashFile(fileA, fileA); + git.add().addFilepattern(fileA).call(); + RevCommit c = git.commit().setMessage("commit c").call(); + + createBranch(b, "refs/heads/side"); + checkoutBranch("refs/heads/side"); + + writeTrashFile(fileB, fileB); + git.add().addFilepattern(fileB).call(); + RevCommit d = git.commit().setMessage("commit d").call(); + + checkoutBranch("refs/heads/master"); + MergeResult m = git.merge().include(d.getId()).call(); + assertEquals(MergeResult.MergeStatus.MERGED, m.getMergeStatus()); + + Iterator rangeLog = git.log().addRange(b.getId(), m.getNewHead()).call().iterator(); + + RevCommit commit = rangeLog.next(); + assertEquals(m.getNewHead(), commit.getId()); + commit = rangeLog.next(); + assertEquals(c.getId(), commit.getId()); + commit = rangeLog.next(); + assertEquals(d.getId(), commit.getId()); + assertFalse(rangeLog.hasNext()); + } + private void setCommitsAndMerge() throws Exception { Git git = Git.wrap(db); writeTrashFile("file1", "1\n2\n3\n4\n"); diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/DateRevQueueTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/DateRevQueueTest.java index a9dfe15c9..852d18c35 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/DateRevQueueTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/DateRevQueueTest.java @@ -59,20 +59,26 @@ public void testInsertOutOfOrder() throws Exception { public void testInsertTie() throws Exception { final RevCommit a = parseBody(commit()); final RevCommit b = parseBody(commit(0, a)); + final RevCommit c = parseBody(commit(0, b)); + { q = create(); q.add(a); q.add(b); + q.add(c); assertCommit(a, q.next()); assertCommit(b, q.next()); + assertCommit(c, q.next()); assertNull(q.next()); } { q = create(); + q.add(c); q.add(b); q.add(a); + assertCommit(c, q.next()); assertCommit(b, q.next()); assertCommit(a, q.next()); assertNull(q.next()); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseCommand.java index 6678af163..836175dce 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseCommand.java @@ -67,6 +67,7 @@ import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevSort; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.revwalk.filter.RevFilter; import org.eclipse.jgit.submodule.SubmoduleWalk.IgnoreSubmoduleMode; @@ -1137,15 +1138,19 @@ else if (!isInteractive() && walk.isMergedInto(headCommit, upstream)) { private List calculatePickList(RevCommit headCommit) throws GitAPIException, NoHeadException, IOException { - Iterable commitsToUse; - try (Git git = new Git(repo)) { - LogCommand cmd = git.log().addRange(upstreamCommit, headCommit); - commitsToUse = cmd.call(); - } List cherryPickList = new ArrayList<>(); - for (RevCommit commit : commitsToUse) { - if (preserveMerges || commit.getParentCount() == 1) - cherryPickList.add(commit); + try (RevWalk r = new RevWalk(repo)) { + r.sort(RevSort.TOPO_KEEP_BRANCH_TOGETHER, true); + r.sort(RevSort.COMMIT_TIME_DESC, true); + r.markUninteresting(r.lookupCommit(upstreamCommit)); + r.markStart(r.lookupCommit(headCommit)); + Iterator commitsToUse = r.iterator(); + while (commitsToUse.hasNext()) { + RevCommit commit = commitsToUse.next(); + if (preserveMerges || commit.getParentCount() == 1) { + cherryPickList.add(commit); + } + } } Collections.reverse(cherryPickList); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DateRevQueue.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DateRevQueue.java index b875be927..0cabf0705 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DateRevQueue.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DateRevQueue.java @@ -93,7 +93,7 @@ else if (t > when) head = n; } else { Entry p = q.next; - while (p != null && p.commit.commitTime > when) { + while (p != null && p.commit.commitTime >= when) { q = p; p = q.next; }