From 551f3a0fa5460513008e26bd2bb1ac73c185f5f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konrad=20K=C3=BCgler?= Date: Sun, 13 Apr 2014 19:21:06 +0200 Subject: [PATCH 1/5] Blame correctly in the presence of conflicting merges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: The BlameGenerator used the RevFlag SEEN to mark commits it had already looked at (but not necessarily processed), to prevent processing a commit multiple times. If a commit is a conflicting merge that contains lines of the merge base, that have been deleted in its first parent, either these lines or the lines untouched since the merge base would not be blamed properly. This happens for example if a file is modified on a main branch in an earlier commit M and on a side branch in a later commit S. For this example, M deletes some lines relative to the common base commit B, and S modifies a subset of these lines, leaving some other of these lines untouched. Then side is merged into main, creating a conflict for these lines. The merge resolution shall carry over some unmodified lines from B that would otherwise be deleted by M. The route to blame these lines is via S to B. They can't be blamed via M, as they don't exist there anymore. Q |\ | \ | S | | M | | / |/ B Blaming the merged file first blames via S, because that is the most recent commit. Doing so, it also looks at B to blame the unmodified lines of B carried over by S into the merge result. In the course of this, B is submitted for later processing and marked SEEN. Later M is blamed. It notices that its parent commit B has been SEEN and aborts processing for M. B is blamed after that, but only for the lines that survived via S. As a result, only the lines contributed by S or by B via S are blamed. All the other lines that were unchanges by both M and S, which should have been blamed to B via M, are not blamed. Solution: Don't abort processing when encountering a SEEN commit. Rather add the new region list of lines to be blamed to those of the already SEEN and enqueued commit's region list. This way when the B commit of the above example is processed, it will blame both the lines of M and S, yielding a complete blame result. Bug: 374382 Change-Id: I369059597608022948009ea7708cc8190f05a8d3 Signed-off-by: Konrad Kügler Signed-off-by: Shawn Pearce --- .../eclipse/jgit/api/BlameCommandTest.java | 70 +++++++++++++++++++ .../eclipse/jgit/blame/BlameGenerator.java | 41 ++++++----- .../src/org/eclipse/jgit/blame/Candidate.java | 55 +++++++++++++++ 3 files changed, 147 insertions(+), 19 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/BlameCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/BlameCommandTest.java index 20e93f88c..743e16de0 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/BlameCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/BlameCommandTest.java @@ -386,4 +386,74 @@ public class BlameCommandTest extends RepositoryTestCase { assertEquals(commit, lines.getSourceCommit(1)); assertEquals(commit, lines.getSourceCommit(2)); } + + @Test + public void testConflictingMerge1() throws Exception { + Git git = new Git(db); + + RevCommit base = commitFile("file.txt", join("0", "1", "2", "3", "4"), + "master"); + + git.checkout().setName("side").setCreateBranch(true) + .setStartPoint(base).call(); + RevCommit side = commitFile("file.txt", + join("0", "1 side", "2", "3 on side", "4"), "side"); + + commitFile("file.txt", join("0", "1", "2"), "master"); + + checkoutBranch("refs/heads/master"); + git.merge().include(side).call(); + + // The merge results in a conflict, which we resolve using mostly the + // side branch contents. Especially the "4" survives. + RevCommit merge = commitFile("file.txt", + join("0", "1 side", "2", "3 resolved", "4"), "master"); + + BlameCommand command = new BlameCommand(db); + command.setFilePath("file.txt"); + BlameResult lines = command.call(); + + assertEquals(5, lines.getResultContents().size()); + assertEquals(base, lines.getSourceCommit(0)); + assertEquals(side, lines.getSourceCommit(1)); + assertEquals(base, lines.getSourceCommit(2)); + assertEquals(merge, lines.getSourceCommit(3)); + assertEquals(base, lines.getSourceCommit(4)); + } + + // this test inverts the order of the master and side commit and is + // otherwise identical to testConflictingMerge1 + @Test + public void testConflictingMerge2() throws Exception { + Git git = new Git(db); + + RevCommit base = commitFile("file.txt", join("0", "1", "2", "3", "4"), + "master"); + + commitFile("file.txt", join("0", "1", "2"), "master"); + + git.checkout().setName("side").setCreateBranch(true) + .setStartPoint(base).call(); + RevCommit side = commitFile("file.txt", + join("0", "1 side", "2", "3 on side", "4"), "side"); + + checkoutBranch("refs/heads/master"); + git.merge().include(side).call(); + + // The merge results in a conflict, which we resolve using mostly the + // side branch contents. Especially the "4" survives. + RevCommit merge = commitFile("file.txt", + join("0", "1 side", "2", "3 resolved", "4"), "master"); + + BlameCommand command = new BlameCommand(db); + command.setFilePath("file.txt"); + BlameResult lines = command.call(); + + assertEquals(5, lines.getResultContents().size()); + assertEquals(base, lines.getSourceCommit(0)); + assertEquals(side, lines.getSourceCommit(1)); + assertEquals(base, lines.getSourceCommit(2)); + assertEquals(merge, lines.getSourceCommit(3)); + assertEquals(base, lines.getSourceCommit(4)); + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java b/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java index b7ad9d1a1..c198385e8 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java @@ -121,7 +121,7 @@ public class BlameGenerator { /** Revision pool used to acquire commits from. */ private RevWalk revPool; - /** Indicates the commit has already been processed. */ + /** Indicates the commit was put into the queue at least once. */ private RevFlag SEEN; private ObjectReader reader; @@ -146,7 +146,7 @@ public class BlameGenerator { /** * Create a blame generator for the repository and path (relative to * repository) - * + * * @param repository * repository to access revision data from. * @param path @@ -532,6 +532,7 @@ public class BlameGenerator { private void push(BlobCandidate toInsert) { Candidate c = queue; if (c != null) { + c.remove(SEEN); // will be pushed by toInsert c.regionList = null; toInsert.parent = c; } @@ -539,8 +540,24 @@ public class BlameGenerator { } private void push(Candidate toInsert) { - // Mark sources to ensure they get discarded (above) if - // another path to the same commit. + if (toInsert.has(SEEN)) { + // We have already added a Candidate for this commit to the queue, + // this can happen if the commit is a merge base for two or more + // parallel branches that were merged together. + // + // It is likely the candidate was not yet processed. The queue + // sorts descending by commit time and usually descendant commits + // have higher timestamps than the ancestors. + // + // Find the existing candidate and merge the new candidate's + // region list into it. + for (Candidate p = queue; p != null; p = p.queueNext) { + if (p.canMergeRegions(toInsert)) { + p.mergeRegions(toInsert); + return; + } + } + } toInsert.add(SEEN); // Insert into the queue using descending commit time, so @@ -567,8 +584,6 @@ public class BlameGenerator { RevCommit parent = n.getParent(0); if (parent == null) return split(n.getNextCandidate(0), n); - if (parent.has(SEEN)) - return false; revPool.parseHeaders(parent); if (find(parent, n.sourcePath)) { @@ -636,20 +651,12 @@ public class BlameGenerator { private boolean processMerge(Candidate n) throws IOException { int pCnt = n.getParentCount(); - for (int pIdx = 0; pIdx < pCnt; pIdx++) { - RevCommit parent = n.getParent(pIdx); - if (parent.has(SEEN)) - continue; - revPool.parseHeaders(parent); - } - // If any single parent exactly matches the merge, follow only // that one parent through history. ObjectId[] ids = null; for (int pIdx = 0; pIdx < pCnt; pIdx++) { RevCommit parent = n.getParent(pIdx); - if (parent.has(SEEN)) - continue; + revPool.parseHeaders(parent); if (!find(parent, n.sourcePath)) continue; if (!(n instanceof ReverseCandidate) && idBuf.equals(n.sourceBlob)) { @@ -668,8 +675,6 @@ public class BlameGenerator { renames = new DiffEntry[pCnt]; for (int pIdx = 0; pIdx < pCnt; pIdx++) { RevCommit parent = n.getParent(pIdx); - if (parent.has(SEEN)) - continue; if (ids != null && ids[pIdx] != null) continue; @@ -702,8 +707,6 @@ public class BlameGenerator { Candidate[] parents = new Candidate[pCnt]; for (int pIdx = 0; pIdx < pCnt; pIdx++) { RevCommit parent = n.getParent(pIdx); - if (parent.has(SEEN)) - continue; Candidate p; if (renames != null && renames[pIdx] != null) { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/blame/Candidate.java b/org.eclipse.jgit/src/org/eclipse/jgit/blame/Candidate.java index 95b4cec02..cbc5b43a6 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/blame/Candidate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/blame/Candidate.java @@ -124,10 +124,18 @@ class Candidate { return null; } + boolean has(RevFlag flag) { + return sourceCommit.has(flag); + } + void add(RevFlag flag) { sourceCommit.add(flag); } + void remove(RevFlag flag) { + sourceCommit.remove(flag); + } + int getTime() { return sourceCommit.getCommitTime(); } @@ -275,6 +283,42 @@ class Candidate { return r; } + boolean canMergeRegions(Candidate other) { + return sourceCommit == other.sourceCommit + && sourcePath.getPath().equals(other.sourcePath.getPath()); + } + + void mergeRegions(Candidate other) { + // regionList is always sorted by resultStart. Merge join two + // linked lists, preserving the ordering. Combine neighboring + // regions to reduce the number of results seen by callers. + Region a = clearRegionList(); + Region b = other.clearRegionList(); + Region t = null; + + while (a != null && b != null) { + if (a.resultStart < b.resultStart) { + Region n = a.next; + t = add(t, this, a); + a = n; + } else { + Region n = b.next; + t = add(t, this, b); + b = n; + } + } + + if (a != null) { + Region n = a.next; + t = add(t, this, a); + t.next = n; + } else /* b != null */{ + Region n = b.next; + t = add(t, this, b); + t.next = n; + } + } + @SuppressWarnings("nls") @Override public String toString() { @@ -369,11 +413,22 @@ class Candidate { return parent; } + @Override + boolean has(RevFlag flag) { + return true; // Pretend flag was added; sourceCommit is null. + } + @Override void add(RevFlag flag) { // Do nothing, sourceCommit is null. } + @Override + + void remove(RevFlag flag) { + // Do nothing, sourceCommit is null. + } + @Override int getTime() { return Integer.MAX_VALUE; From 5a4e34009be6ab108d3165a436da38cb23fb8a0f Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Thu, 17 Apr 2014 14:19:23 -0700 Subject: [PATCH 2/5] blame: Automatically increase commit abbreviation length Ensure commit object names are unique by extending the default abbreviation as long as necessary. This allows `jgit blame` to more closely match the formatted output of `git blame` on large histories like Gerrit Code Review's ReceiveCommits.java file. Change-Id: I5f7c4855769ee9dcba973389df9e109005dcdb5b --- .../src/org/eclipse/jgit/pgm/Blame.java | 20 ++++++++++++++++--- .../eclipse/jgit/blame/BlameGenerator.java | 12 +++++++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Blame.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Blame.java index 164da3f1d..307d947f3 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Blame.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Blame.java @@ -143,6 +143,7 @@ class Blame extends TextBuiltin { revision = null; } + boolean autoAbbrev = abbrev == 0; if (abbrev == 0) abbrev = db.getConfig().getInt("core", "abbrev", 7); //$NON-NLS-1$ //$NON-NLS-2$ if (!showBlankBoundary) @@ -156,6 +157,7 @@ class Blame extends TextBuiltin { dateFmt = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss ZZZZ"); //$NON-NLS-1$ BlameGenerator generator = new BlameGenerator(db, file); + RevFlag scanned = generator.newFlag("SCANNED"); //$NON-NLS-1$ reader = db.newObjectReader(); try { generator.setTextComparator(comparator); @@ -198,9 +200,17 @@ class Blame extends TextBuiltin { int pathWidth = 1; int maxSourceLine = 1; for (int line = begin; line < end; line++) { - authorWidth = Math.max(authorWidth, author(line).length()); - dateWidth = Math.max(dateWidth, date(line).length()); - pathWidth = Math.max(pathWidth, path(line).length()); + RevCommit c = blame.getSourceCommit(line); + if (c != null && !c.has(scanned)) { + c.add(scanned); + if (autoAbbrev) + abbrev = Math.max(abbrev, uniqueAbbrevLen(c)); + authorWidth = Math.max(authorWidth, author(line).length()); + dateWidth = Math.max(dateWidth, date(line).length()); + pathWidth = Math.max(pathWidth, path(line).length()); + } + while (line + 1 < end && blame.getSourceCommit(line + 1) == c) + line++; maxSourceLine = Math.max(maxSourceLine, blame.getSourceLine(line)); } @@ -232,6 +242,10 @@ class Blame extends TextBuiltin { } } + private int uniqueAbbrevLen(RevCommit commit) throws IOException { + return reader.abbreviate(commit, abbrev).length(); + } + private void parseLineRangeOption() { String beginStr, endStr; if (rangeString.startsWith("/")) { //$NON-NLS-1$ diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java b/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java index c198385e8..63d0fb2cf 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java @@ -422,6 +422,18 @@ public class BlameGenerator { return this; } + /** + * Allocate a new RevFlag for use by the caller. + * + * @param name + * unique name of the flag in the blame context. + * @return the newly allocated flag. + * @since 3.4 + */ + public RevFlag newFlag(String name) { + return revPool.newFlag(name); + } + /** * Execute the generator in a blocking fashion until all data is ready. * From 52500d3264d2557cc8ff272b1df9ab12aba0c43b Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Thu, 17 Apr 2014 15:03:18 -0700 Subject: [PATCH 3/5] blame: Micro optimize blob lookup in tree Avoid converting the raw mode to FileMode. This is an expensive if-else-if sort of test to just check if the thing is a blob. Instead test the bit mask directly, which is at least a few instructions shorter. The TreeWalk is already recursive and will auto-dive into any subtrees found. isSubtree check is unnecessary, as is the loop, as only one result will ever be returned by next(). Change-Id: I9fb25229ebed857469427bfbdf74aedebfddfac8 --- .../org/eclipse/jgit/blame/BlameGenerator.java | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java b/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java index 63d0fb2cf..34e15475c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java @@ -44,6 +44,7 @@ package org.eclipse.jgit.blame; import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; +import static org.eclipse.jgit.lib.FileMode.TYPE_FILE; import java.io.IOException; import java.util.Collection; @@ -942,20 +943,17 @@ public class BlameGenerator { private boolean find(RevCommit commit, PathFilter path) throws IOException { treeWalk.setFilter(path); treeWalk.reset(commit.getTree()); - while (treeWalk.next()) { - if (path.isDone(treeWalk)) { - if (treeWalk.getFileMode(0).getObjectType() != OBJ_BLOB) - return false; - treeWalk.getObjectId(idBuf, 0); - return true; - } - - if (treeWalk.isSubtree()) - treeWalk.enterSubtree(); + if (treeWalk.next() && isFile(treeWalk.getRawMode(0))) { + treeWalk.getObjectId(idBuf, 0); + return true; } return false; } + private static final boolean isFile(int rawMode) { + return (rawMode & TYPE_FILE) == TYPE_FILE; + } + private DiffEntry findRename(RevCommit parent, RevCommit commit, PathFilter path) throws IOException { if (renameDetector == null) From 6de12836d72fe4cba9afc297c8988c6fff851fa9 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Thu, 17 Apr 2014 15:26:04 -0700 Subject: [PATCH 4/5] blame: Reduce running time ~4.5% by skipping common subtrees With this commit running blame on render_view_impl.cc[1] saves about 644 ms over prior versions, reducing the time about 4.5%. Large projects often contain strands of commits where no changes are made to a particular subtree. Blame used to dive recursively into these subtrees to look for the blob and check if its SHA-1 was changed. In chromium/src[1] only 20% of the commits modify the content/renderer subtree relevant for the file. The recursivePath is necessary to check for '/' and remember if common subtree elimination should be attempted. When a file lives within a subtree the extra cost to check for unmodified subtrees saves time. However for files in the root tree the extra work incurred by TreeWalk is not worthwhile and would significantly increase overall running time. Now typical running times from an otherwise idle desktop: real 0m13.387s 0m13.341s 0m13.443s user 0m15.410s 0m15.220s 0m15.350s previously: real 0m14.085s 0m14.049s 0m13.968s user 0m15.730s 0m15.820s 0m15.770s [1] https://chromium.googlesource.com/chromium/src/+show/34d6e5c5b4248b1b199405af7ad00f961921f347/content/renderer/render_view_impl.cc Change-Id: Ib16d684df7ffa034ee28def3fb22c797998d5b7b --- .../eclipse/jgit/blame/BlameGenerator.java | 70 ++++++++++++++----- .../src/org/eclipse/jgit/blame/Candidate.java | 7 ++ 2 files changed, 60 insertions(+), 17 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java b/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java index 34e15475c..5b02ce6c9 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java @@ -73,6 +73,7 @@ import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevFlag; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.treewalk.TreeWalk; +import org.eclipse.jgit.treewalk.filter.AndTreeFilter; import org.eclipse.jgit.treewalk.filter.PathFilter; import org.eclipse.jgit.treewalk.filter.TreeFilter; @@ -599,19 +600,19 @@ public class BlameGenerator { return split(n.getNextCandidate(0), n); revPool.parseHeaders(parent); - if (find(parent, n.sourcePath)) { - if (idBuf.equals(n.sourceBlob)) { - // The common case of the file not being modified in - // a simple string-of-pearls history. Blame parent. - n.sourceCommit = parent; - push(n); - return false; + if (n.sourceCommit != null && n.recursivePath) { + treeWalk.setFilter(AndTreeFilter.create(n.sourcePath, ID_DIFF)); + treeWalk.reset(n.sourceCommit.getTree(), parent.getTree()); + if (!treeWalk.next()) + return blameEntireRegionOnParent(n, parent); + if (isFile(treeWalk.getRawMode(1))) { + treeWalk.getObjectId(idBuf, 1); + return splitBlameWithParent(n, parent); } - - Candidate next = n.create(parent, n.sourcePath); - next.sourceBlob = idBuf.toObjectId(); - next.loadText(reader); - return split(next, n); + } else if (find(parent, n.sourcePath)) { + if (idBuf.equals(n.sourceBlob)) + return blameEntireRegionOnParent(n, parent); + return splitBlameWithParent(n, parent); } if (n.sourceCommit == null) @@ -625,7 +626,7 @@ public class BlameGenerator { // A 100% rename without any content change can also // skip directly to the parent. n.sourceCommit = parent; - n.sourcePath = PathFilter.create(r.getOldPath()); + n.setSourcePath(PathFilter.create(r.getOldPath())); push(n); return false; } @@ -637,6 +638,21 @@ public class BlameGenerator { return split(next, n); } + private boolean blameEntireRegionOnParent(Candidate n, RevCommit parent) { + // File was not modified, blame parent. + n.sourceCommit = parent; + push(n); + return false; + } + + private boolean splitBlameWithParent(Candidate n, RevCommit parent) + throws IOException { + Candidate next = n.create(parent, n.sourcePath); + next.sourceBlob = idBuf.toObjectId(); + next.loadText(reader); + return split(next, n); + } + private boolean split(Candidate parent, Candidate source) throws IOException { EditList editList = diffAlgorithm.diff(textComparator, @@ -673,9 +689,7 @@ public class BlameGenerator { if (!find(parent, n.sourcePath)) continue; if (!(n instanceof ReverseCandidate) && idBuf.equals(n.sourceBlob)) { - n.sourceCommit = parent; - push(n); - return false; + return blameEntireRegionOnParent(n, parent); } if (ids == null) ids = new ObjectId[pCnt]; @@ -707,7 +721,7 @@ public class BlameGenerator { // we choose to follow the one parent over trying to do // possibly both parents. n.sourceCommit = parent; - n.sourcePath = PathFilter.create(r.getOldPath()); + n.setSourcePath(PathFilter.create(r.getOldPath())); push(n); return false; } @@ -974,4 +988,26 @@ public class BlameGenerator { return ent.getChangeType() == ChangeType.RENAME || ent.getChangeType() == ChangeType.COPY; } + + private static final TreeFilter ID_DIFF = new TreeFilter() { + @Override + public boolean include(TreeWalk tw) { + return !tw.idEqual(0, 1); + } + + @Override + public boolean shouldBeRecursive() { + return false; + } + + @Override + public TreeFilter clone() { + return this; + } + + @Override + public String toString() { + return "ID_DIFF"; //$NON-NLS-1$ + } + }; } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/blame/Candidate.java b/org.eclipse.jgit/src/org/eclipse/jgit/blame/Candidate.java index cbc5b43a6..a652278d0 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/blame/Candidate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/blame/Candidate.java @@ -80,6 +80,7 @@ class Candidate { /** Path of the candidate file in {@link #sourceCommit}. */ PathFilter sourcePath; + boolean recursivePath; /** Unique name of the candidate blob in {@link #sourceCommit}. */ ObjectId sourceBlob; @@ -110,6 +111,7 @@ class Candidate { Candidate(RevCommit commit, PathFilter path) { sourceCommit = commit; sourcePath = path; + recursivePath = path.shouldBeRecursive(); } int getParentCount() { @@ -144,6 +146,11 @@ class Candidate { return sourceCommit.getAuthorIdent(); } + void setSourcePath(PathFilter path) { + sourcePath = path; + recursivePath = path.shouldBeRecursive(); + } + Candidate create(RevCommit commit, PathFilter path) { return new Candidate(commit, path); } From cbc7c5c03fa85380b809cbd1bead82f4685d98ca Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Fri, 18 Apr 2014 09:05:05 -0700 Subject: [PATCH 5/5] diff: Optimize single line edits If the header and trailer are identical up to a single line on both sides, return that REPLACE edit as the only result. No algorithm can break down a REPLACE with height of 1. Change-Id: I483c40e8790cc3e8b322ef6dfce2299491fd0ac7 --- .../src/org/eclipse/jgit/diff/DiffAlgorithm.java | 3 +++ .../src/org/eclipse/jgit/diff/HistogramDiff.java | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/diff/DiffAlgorithm.java b/org.eclipse.jgit/src/org/eclipse/jgit/diff/DiffAlgorithm.java index 7545821d1..39421c6de 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/diff/DiffAlgorithm.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/diff/DiffAlgorithm.java @@ -114,6 +114,9 @@ public abstract class DiffAlgorithm { return EditList.singleton(region); case REPLACE: { + if (region.getLengthA() == 1 && region.getLengthB() == 1) + return EditList.singleton(region); + SubsequenceComparator cs = new SubsequenceComparator(cmp); Subsequence as = Subsequence.a(a, region); Subsequence bs = Subsequence.b(b, region); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/diff/HistogramDiff.java b/org.eclipse.jgit/src/org/eclipse/jgit/diff/HistogramDiff.java index 026e63fce..0979db1e7 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/diff/HistogramDiff.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/diff/HistogramDiff.java @@ -192,7 +192,10 @@ public class HistogramDiff extends LowLevelDiffAlgorithm { break; case REPLACE: - diffReplace(r); + if (r.getLengthA() == 1 && r.getLengthB() == 1) + edits.add(r); + else + diffReplace(r); break; case EMPTY: