Browse Source

Merge changes I483c40e8,Ib16d684d,I9fb25229,I5f7c4855,I36905959

* changes:
  diff: Optimize single line edits
  blame: Reduce running time ~4.5% by skipping common subtrees
  blame: Micro optimize blob lookup in tree
  blame: Automatically increase commit abbreviation length
  Blame correctly in the presence of conflicting merges
stable-3.4
Shawn Pearce 11 years ago committed by Gerrit Code Review @ Eclipse.org
parent
commit
c8dd915c45
  1. 14
      org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Blame.java
  2. 70
      org.eclipse.jgit.test/tst/org/eclipse/jgit/api/BlameCommandTest.java
  3. 135
      org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java
  4. 62
      org.eclipse.jgit/src/org/eclipse/jgit/blame/Candidate.java
  5. 3
      org.eclipse.jgit/src/org/eclipse/jgit/diff/DiffAlgorithm.java
  6. 3
      org.eclipse.jgit/src/org/eclipse/jgit/diff/HistogramDiff.java

14
org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Blame.java

@ -143,6 +143,7 @@ class Blame extends TextBuiltin {
revision = null; revision = null;
} }
boolean autoAbbrev = abbrev == 0;
if (abbrev == 0) if (abbrev == 0)
abbrev = db.getConfig().getInt("core", "abbrev", 7); //$NON-NLS-1$ //$NON-NLS-2$ abbrev = db.getConfig().getInt("core", "abbrev", 7); //$NON-NLS-1$ //$NON-NLS-2$
if (!showBlankBoundary) if (!showBlankBoundary)
@ -156,6 +157,7 @@ class Blame extends TextBuiltin {
dateFmt = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss ZZZZ"); //$NON-NLS-1$ dateFmt = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss ZZZZ"); //$NON-NLS-1$
BlameGenerator generator = new BlameGenerator(db, file); BlameGenerator generator = new BlameGenerator(db, file);
RevFlag scanned = generator.newFlag("SCANNED"); //$NON-NLS-1$
reader = db.newObjectReader(); reader = db.newObjectReader();
try { try {
generator.setTextComparator(comparator); generator.setTextComparator(comparator);
@ -198,9 +200,17 @@ class Blame extends TextBuiltin {
int pathWidth = 1; int pathWidth = 1;
int maxSourceLine = 1; int maxSourceLine = 1;
for (int line = begin; line < end; line++) { for (int line = begin; line < end; line++) {
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()); authorWidth = Math.max(authorWidth, author(line).length());
dateWidth = Math.max(dateWidth, date(line).length()); dateWidth = Math.max(dateWidth, date(line).length());
pathWidth = Math.max(pathWidth, path(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)); 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() { private void parseLineRangeOption() {
String beginStr, endStr; String beginStr, endStr;
if (rangeString.startsWith("/")) { //$NON-NLS-1$ if (rangeString.startsWith("/")) { //$NON-NLS-1$

70
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(1));
assertEquals(commit, lines.getSourceCommit(2)); 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));
}
} }

135
org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java

@ -44,6 +44,7 @@
package org.eclipse.jgit.blame; package org.eclipse.jgit.blame;
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
import static org.eclipse.jgit.lib.FileMode.TYPE_FILE;
import java.io.IOException; import java.io.IOException;
import java.util.Collection; import java.util.Collection;
@ -72,6 +73,7 @@ import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevFlag; import org.eclipse.jgit.revwalk.RevFlag;
import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.treewalk.TreeWalk; 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.PathFilter;
import org.eclipse.jgit.treewalk.filter.TreeFilter; import org.eclipse.jgit.treewalk.filter.TreeFilter;
@ -121,7 +123,7 @@ public class BlameGenerator {
/** Revision pool used to acquire commits from. */ /** Revision pool used to acquire commits from. */
private RevWalk revPool; 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 RevFlag SEEN;
private ObjectReader reader; private ObjectReader reader;
@ -422,6 +424,18 @@ public class BlameGenerator {
return this; 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. * Execute the generator in a blocking fashion until all data is ready.
* *
@ -532,6 +546,7 @@ public class BlameGenerator {
private void push(BlobCandidate toInsert) { private void push(BlobCandidate toInsert) {
Candidate c = queue; Candidate c = queue;
if (c != null) { if (c != null) {
c.remove(SEEN); // will be pushed by toInsert
c.regionList = null; c.regionList = null;
toInsert.parent = c; toInsert.parent = c;
} }
@ -539,8 +554,24 @@ public class BlameGenerator {
} }
private void push(Candidate toInsert) { private void push(Candidate toInsert) {
// Mark sources to ensure they get discarded (above) if if (toInsert.has(SEEN)) {
// another path to the same commit. // 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); toInsert.add(SEEN);
// Insert into the queue using descending commit time, so // Insert into the queue using descending commit time, so
@ -567,23 +598,21 @@ public class BlameGenerator {
RevCommit parent = n.getParent(0); RevCommit parent = n.getParent(0);
if (parent == null) if (parent == null)
return split(n.getNextCandidate(0), n); return split(n.getNextCandidate(0), n);
if (parent.has(SEEN))
return false;
revPool.parseHeaders(parent); revPool.parseHeaders(parent);
if (find(parent, n.sourcePath)) { if (n.sourceCommit != null && n.recursivePath) {
if (idBuf.equals(n.sourceBlob)) { treeWalk.setFilter(AndTreeFilter.create(n.sourcePath, ID_DIFF));
// The common case of the file not being modified in treeWalk.reset(n.sourceCommit.getTree(), parent.getTree());
// a simple string-of-pearls history. Blame parent. if (!treeWalk.next())
n.sourceCommit = parent; return blameEntireRegionOnParent(n, parent);
push(n); if (isFile(treeWalk.getRawMode(1))) {
return false; treeWalk.getObjectId(idBuf, 1);
return splitBlameWithParent(n, parent);
} }
} else if (find(parent, n.sourcePath)) {
Candidate next = n.create(parent, n.sourcePath); if (idBuf.equals(n.sourceBlob))
next.sourceBlob = idBuf.toObjectId(); return blameEntireRegionOnParent(n, parent);
next.loadText(reader); return splitBlameWithParent(n, parent);
return split(next, n);
} }
if (n.sourceCommit == null) if (n.sourceCommit == null)
@ -597,7 +626,7 @@ public class BlameGenerator {
// A 100% rename without any content change can also // A 100% rename without any content change can also
// skip directly to the parent. // skip directly to the parent.
n.sourceCommit = parent; n.sourceCommit = parent;
n.sourcePath = PathFilter.create(r.getOldPath()); n.setSourcePath(PathFilter.create(r.getOldPath()));
push(n); push(n);
return false; return false;
} }
@ -609,6 +638,21 @@ public class BlameGenerator {
return split(next, n); 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) private boolean split(Candidate parent, Candidate source)
throws IOException { throws IOException {
EditList editList = diffAlgorithm.diff(textComparator, EditList editList = diffAlgorithm.diff(textComparator,
@ -636,26 +680,16 @@ public class BlameGenerator {
private boolean processMerge(Candidate n) throws IOException { private boolean processMerge(Candidate n) throws IOException {
int pCnt = n.getParentCount(); 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 // If any single parent exactly matches the merge, follow only
// that one parent through history. // that one parent through history.
ObjectId[] ids = null; ObjectId[] ids = null;
for (int pIdx = 0; pIdx < pCnt; pIdx++) { for (int pIdx = 0; pIdx < pCnt; pIdx++) {
RevCommit parent = n.getParent(pIdx); RevCommit parent = n.getParent(pIdx);
if (parent.has(SEEN)) revPool.parseHeaders(parent);
continue;
if (!find(parent, n.sourcePath)) if (!find(parent, n.sourcePath))
continue; continue;
if (!(n instanceof ReverseCandidate) && idBuf.equals(n.sourceBlob)) { if (!(n instanceof ReverseCandidate) && idBuf.equals(n.sourceBlob)) {
n.sourceCommit = parent; return blameEntireRegionOnParent(n, parent);
push(n);
return false;
} }
if (ids == null) if (ids == null)
ids = new ObjectId[pCnt]; ids = new ObjectId[pCnt];
@ -668,8 +702,6 @@ public class BlameGenerator {
renames = new DiffEntry[pCnt]; renames = new DiffEntry[pCnt];
for (int pIdx = 0; pIdx < pCnt; pIdx++) { for (int pIdx = 0; pIdx < pCnt; pIdx++) {
RevCommit parent = n.getParent(pIdx); RevCommit parent = n.getParent(pIdx);
if (parent.has(SEEN))
continue;
if (ids != null && ids[pIdx] != null) if (ids != null && ids[pIdx] != null)
continue; continue;
@ -689,7 +721,7 @@ public class BlameGenerator {
// we choose to follow the one parent over trying to do // we choose to follow the one parent over trying to do
// possibly both parents. // possibly both parents.
n.sourceCommit = parent; n.sourceCommit = parent;
n.sourcePath = PathFilter.create(r.getOldPath()); n.setSourcePath(PathFilter.create(r.getOldPath()));
push(n); push(n);
return false; return false;
} }
@ -702,8 +734,6 @@ public class BlameGenerator {
Candidate[] parents = new Candidate[pCnt]; Candidate[] parents = new Candidate[pCnt];
for (int pIdx = 0; pIdx < pCnt; pIdx++) { for (int pIdx = 0; pIdx < pCnt; pIdx++) {
RevCommit parent = n.getParent(pIdx); RevCommit parent = n.getParent(pIdx);
if (parent.has(SEEN))
continue;
Candidate p; Candidate p;
if (renames != null && renames[pIdx] != null) { if (renames != null && renames[pIdx] != null) {
@ -927,20 +957,17 @@ public class BlameGenerator {
private boolean find(RevCommit commit, PathFilter path) throws IOException { private boolean find(RevCommit commit, PathFilter path) throws IOException {
treeWalk.setFilter(path); treeWalk.setFilter(path);
treeWalk.reset(commit.getTree()); treeWalk.reset(commit.getTree());
while (treeWalk.next()) { if (treeWalk.next() && isFile(treeWalk.getRawMode(0))) {
if (path.isDone(treeWalk)) {
if (treeWalk.getFileMode(0).getObjectType() != OBJ_BLOB)
return false;
treeWalk.getObjectId(idBuf, 0); treeWalk.getObjectId(idBuf, 0);
return true; return true;
} }
if (treeWalk.isSubtree())
treeWalk.enterSubtree();
}
return false; return false;
} }
private static final boolean isFile(int rawMode) {
return (rawMode & TYPE_FILE) == TYPE_FILE;
}
private DiffEntry findRename(RevCommit parent, RevCommit commit, private DiffEntry findRename(RevCommit parent, RevCommit commit,
PathFilter path) throws IOException { PathFilter path) throws IOException {
if (renameDetector == null) if (renameDetector == null)
@ -961,4 +988,26 @@ public class BlameGenerator {
return ent.getChangeType() == ChangeType.RENAME return ent.getChangeType() == ChangeType.RENAME
|| ent.getChangeType() == ChangeType.COPY; || 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$
}
};
} }

62
org.eclipse.jgit/src/org/eclipse/jgit/blame/Candidate.java

@ -80,6 +80,7 @@ class Candidate {
/** Path of the candidate file in {@link #sourceCommit}. */ /** Path of the candidate file in {@link #sourceCommit}. */
PathFilter sourcePath; PathFilter sourcePath;
boolean recursivePath;
/** Unique name of the candidate blob in {@link #sourceCommit}. */ /** Unique name of the candidate blob in {@link #sourceCommit}. */
ObjectId sourceBlob; ObjectId sourceBlob;
@ -110,6 +111,7 @@ class Candidate {
Candidate(RevCommit commit, PathFilter path) { Candidate(RevCommit commit, PathFilter path) {
sourceCommit = commit; sourceCommit = commit;
sourcePath = path; sourcePath = path;
recursivePath = path.shouldBeRecursive();
} }
int getParentCount() { int getParentCount() {
@ -124,10 +126,18 @@ class Candidate {
return null; return null;
} }
boolean has(RevFlag flag) {
return sourceCommit.has(flag);
}
void add(RevFlag flag) { void add(RevFlag flag) {
sourceCommit.add(flag); sourceCommit.add(flag);
} }
void remove(RevFlag flag) {
sourceCommit.remove(flag);
}
int getTime() { int getTime() {
return sourceCommit.getCommitTime(); return sourceCommit.getCommitTime();
} }
@ -136,6 +146,11 @@ class Candidate {
return sourceCommit.getAuthorIdent(); return sourceCommit.getAuthorIdent();
} }
void setSourcePath(PathFilter path) {
sourcePath = path;
recursivePath = path.shouldBeRecursive();
}
Candidate create(RevCommit commit, PathFilter path) { Candidate create(RevCommit commit, PathFilter path) {
return new Candidate(commit, path); return new Candidate(commit, path);
} }
@ -275,6 +290,42 @@ class Candidate {
return r; 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") @SuppressWarnings("nls")
@Override @Override
public String toString() { public String toString() {
@ -369,11 +420,22 @@ class Candidate {
return parent; return parent;
} }
@Override
boolean has(RevFlag flag) {
return true; // Pretend flag was added; sourceCommit is null.
}
@Override @Override
void add(RevFlag flag) { void add(RevFlag flag) {
// Do nothing, sourceCommit is null. // Do nothing, sourceCommit is null.
} }
@Override
void remove(RevFlag flag) {
// Do nothing, sourceCommit is null.
}
@Override @Override
int getTime() { int getTime() {
return Integer.MAX_VALUE; return Integer.MAX_VALUE;

3
org.eclipse.jgit/src/org/eclipse/jgit/diff/DiffAlgorithm.java

@ -114,6 +114,9 @@ public abstract class DiffAlgorithm {
return EditList.singleton(region); return EditList.singleton(region);
case REPLACE: { case REPLACE: {
if (region.getLengthA() == 1 && region.getLengthB() == 1)
return EditList.singleton(region);
SubsequenceComparator<S> cs = new SubsequenceComparator<S>(cmp); SubsequenceComparator<S> cs = new SubsequenceComparator<S>(cmp);
Subsequence<S> as = Subsequence.a(a, region); Subsequence<S> as = Subsequence.a(a, region);
Subsequence<S> bs = Subsequence.b(b, region); Subsequence<S> bs = Subsequence.b(b, region);

3
org.eclipse.jgit/src/org/eclipse/jgit/diff/HistogramDiff.java

@ -192,6 +192,9 @@ public class HistogramDiff extends LowLevelDiffAlgorithm {
break; break;
case REPLACE: case REPLACE:
if (r.getLengthA() == 1 && r.getLengthB() == 1)
edits.add(r);
else
diffReplace(r); diffReplace(r);
break; break;

Loading…
Cancel
Save