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 @@ protected void run() throws Exception { 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 @@ protected void run() throws Exception { 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 @@ protected void run() throws Exception { 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 @@ protected void run() throws Exception { } } + 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.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 @@ private void testCoreAutoCrlf(AutoCRLF modeForCommitting, 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..5b02ce6c9 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; @@ -72,6 +73,7 @@ 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; @@ -121,7 +123,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 +148,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 @@ -422,6 +424,18 @@ public BlameGenerator reverse(AnyObjectId start, 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. * @@ -532,6 +546,7 @@ private Candidate pop() { 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 +554,24 @@ private void push(BlobCandidate toInsert) { } 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,23 +598,21 @@ private boolean processOne(Candidate n) throws IOException { 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)) { - 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) @@ -597,7 +626,7 @@ private boolean processOne(Candidate n) throws IOException { // 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; } @@ -609,6 +638,21 @@ private boolean processOne(Candidate n) throws IOException { 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, @@ -636,26 +680,16 @@ private boolean split(Candidate parent, Candidate source) 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)) { - n.sourceCommit = parent; - push(n); - return false; + return blameEntireRegionOnParent(n, parent); } if (ids == null) ids = new ObjectId[pCnt]; @@ -668,8 +702,6 @@ private boolean processMerge(Candidate n) throws IOException { 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; @@ -689,7 +721,7 @@ private boolean processMerge(Candidate n) throws IOException { // 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; } @@ -702,8 +734,6 @@ private boolean processMerge(Candidate n) throws IOException { 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) { @@ -927,20 +957,17 @@ public void release() { 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) @@ -961,4 +988,26 @@ private static boolean isRename(DiffEntry ent) { 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 95b4cec02..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() { @@ -124,10 +126,18 @@ Candidate getNextCandidate(@SuppressWarnings("unused") int idx) { 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(); } @@ -136,6 +146,11 @@ PersonIdent getAuthor() { return sourceCommit.getAuthorIdent(); } + void setSourcePath(PathFilter path) { + sourcePath = path; + recursivePath = path.shouldBeRecursive(); + } + Candidate create(RevCommit commit, PathFilter path) { return new Candidate(commit, path); } @@ -275,6 +290,42 @@ private Region clearRegionList() { 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 +420,22 @@ Candidate getNextCandidate(int idx) { 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; 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 EditList diff( 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 @@ private void diff(Edit r) { break; case REPLACE: - diffReplace(r); + if (r.getLengthA() == 1 && r.getLengthB() == 1) + edits.add(r); + else + diffReplace(r); break; case EMPTY: