From 0bdf030b26248c77806ababd757dad58f1344e57 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 3 May 2013 11:18:53 -0700 Subject: [PATCH] Require a DiffConfig when creating a FollowFilter The various rename detection options are an inherent part of the filter, similar to the path being followed. This fixes a potential NPE when a RevWalk with a FollowFilter is created without a Repository, since the old code path tried to get the DiffConfig from the RevWalk's possibly-missing repository. Change-Id: Idb273d5a92849b42935ac14eed73b796b80aad50 --- .../eclipse/jgit/pgm/RevWalkTextBuiltin.java | 14 +++++++----- .../jgit/revwalk/RevWalkFollowFilterTest.java | 5 ++++- .../org/eclipse/jgit/diff/DiffFormatter.java | 5 ++++- .../org/eclipse/jgit/diff/RenameDetector.java | 22 +++++++++++++------ .../eclipse/jgit/revwalk/FollowFilter.java | 14 ++++++++---- .../jgit/revwalk/RewriteTreeFilter.java | 15 +++++-------- 6 files changed, 47 insertions(+), 28 deletions(-) diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/RevWalkTextBuiltin.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/RevWalkTextBuiltin.java index 8543dcb91..92a22b00b 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/RevWalkTextBuiltin.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/RevWalkTextBuiltin.java @@ -50,6 +50,7 @@ import org.kohsuke.args4j.Argument; import org.kohsuke.args4j.Option; +import org.eclipse.jgit.diff.DiffConfig; import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; @@ -117,9 +118,7 @@ void enableBoundary(final boolean on) { } @Option(name = "--follow", metaVar = "metaVar_path") - void follow(final String path) { - pathFilter = FollowFilter.create(path); - } + private String followPath; @Argument(index = 0, metaVar = "metaVar_commitish") private final List commits = new ArrayList(); @@ -150,11 +149,14 @@ protected void run() throws Exception { for (final RevSort s : sorting) walk.sort(s, true); - if (pathFilter instanceof FollowFilter) - walk.setTreeFilter(pathFilter); - else if (pathFilter != TreeFilter.ALL) + if (pathFilter == TreeFilter.ALL) { + if (followPath != null) + walk.setTreeFilter(FollowFilter.create(followPath, + db.getConfig().get(DiffConfig.KEY))); + } else if (pathFilter != TreeFilter.ALL) { walk.setTreeFilter(AndTreeFilter.create(pathFilter, TreeFilter.ANY_DIFF)); + } if (revLimiter.size() == 1) walk.setRevFilter(revLimiter.get(0)); diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkFollowFilterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkFollowFilterTest.java index 1f70a71a1..05e552e41 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkFollowFilterTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkFollowFilterTest.java @@ -47,8 +47,10 @@ import java.util.ArrayList; import java.util.List; +import org.eclipse.jgit.diff.DiffConfig; import org.eclipse.jgit.diff.DiffEntry; import org.eclipse.jgit.junit.TestRepository.CommitBuilder; +import org.eclipse.jgit.lib.Config; import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -74,7 +76,8 @@ public void setUp() throws Exception { } protected FollowFilter follow(final String followPath) { - FollowFilter followFilter = FollowFilter.create(followPath); + FollowFilter followFilter = + FollowFilter.create(followPath, new Config().get(DiffConfig.KEY)); followFilter.setRenameCallback(diffCollector); rw.setTreeFilter(followFilter); return followFilter; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/diff/DiffFormatter.java b/org.eclipse.jgit/src/org/eclipse/jgit/diff/DiffFormatter.java index fe0db33d3..11848e2c9 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/diff/DiffFormatter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/diff/DiffFormatter.java @@ -119,6 +119,8 @@ public class DiffFormatter { private ObjectReader reader; + private DiffConfig diffCfg; + private int context = 3; private int abbreviationLength = 7; @@ -173,6 +175,7 @@ public void setRepository(Repository repository) { db = repository; reader = db.newObjectReader(); + diffCfg = db.getConfig().get(DiffConfig.KEY); ContentSource cs = ContentSource.create(reader); source = new ContentSource.Pair(cs, cs); @@ -534,7 +537,7 @@ private List updateFollowFilter(List files) { String oldPath = ((FollowFilter) pathFilter).getPath(); for (DiffEntry ent : files) { if (isRename(ent) && ent.getNewPath().equals(oldPath)) { - pathFilter = FollowFilter.create(ent.getOldPath()); + pathFilter = FollowFilter.create(ent.getOldPath(), diffCfg); return Collections.singletonList(ent); } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/diff/RenameDetector.java b/org.eclipse.jgit/src/org/eclipse/jgit/diff/RenameDetector.java index f229a04a0..7648a48ac 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/diff/RenameDetector.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/diff/RenameDetector.java @@ -111,7 +111,7 @@ private int sortOf(ChangeType changeType) { private boolean done; - private final Repository repo; + private final ObjectReader objectReader; /** Similarity score required to pair an add/delete as a rename. */ private int renameScore = 60; @@ -136,11 +136,20 @@ private int sortOf(ChangeType changeType) { * the repository to use for rename detection */ public RenameDetector(Repository repo) { - this.repo = repo; + this(repo.newObjectReader(), repo.getConfig().get(DiffConfig.KEY)); + } - DiffConfig cfg = repo.getConfig().get(DiffConfig.KEY); + /** + * Create a new rename detector with a specified reader and diff config. + * + * @param reader + * reader to obtain objects from the repository with. + * @param cfg + * diff config specifying rename detection options. + */ + public RenameDetector(ObjectReader reader, DiffConfig cfg) { + objectReader = reader.newReader(); renameLimit = cfg.getRenameLimit(); - reset(); } @@ -310,11 +319,10 @@ public List compute() throws IOException { */ public List compute(ProgressMonitor pm) throws IOException { if (!done) { - ObjectReader reader = repo.newObjectReader(); try { - return compute(reader, pm); + return compute(objectReader, pm); } finally { - reader.release(); + objectReader.release(); } } return Collections.unmodifiableList(entries); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/FollowFilter.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/FollowFilter.java index ab2cf2bcc..9928286dc 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/FollowFilter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/FollowFilter.java @@ -45,6 +45,7 @@ import java.io.IOException; +import org.eclipse.jgit.diff.DiffConfig; import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.treewalk.TreeWalk; @@ -77,20 +78,25 @@ public class FollowFilter extends TreeFilter { * the path to filter on. Must not be the empty string. All * trailing '/' characters will be trimmed before string's length * is checked or is used as part of the constructed filter. + * @param cfg + * diff config specifying rename detection options. * @return a new filter for the requested path. * @throws IllegalArgumentException * the path supplied was the empty string. + * @since 3.0 */ - public static FollowFilter create(String path) { - return new FollowFilter(PathFilter.create(path)); + public static FollowFilter create(String path, DiffConfig cfg) { + return new FollowFilter(PathFilter.create(path), cfg); } private final PathFilter path; + final DiffConfig cfg; private RenameCallback renameCallback; - FollowFilter(final PathFilter path) { + FollowFilter(final PathFilter path, final DiffConfig cfg) { this.path = path; + this.cfg = cfg; } /** @return the path this filter matches. */ @@ -112,7 +118,7 @@ public boolean shouldBeRecursive() { @Override public TreeFilter clone() { - return new FollowFilter(path.clone()); + return new FollowFilter(path.clone(), cfg); } @SuppressWarnings("nls") diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RewriteTreeFilter.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RewriteTreeFilter.java index 9e1f02139..a84e80e65 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RewriteTreeFilter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RewriteTreeFilter.java @@ -46,15 +46,15 @@ import java.io.IOException; import java.util.List; +import org.eclipse.jgit.diff.DiffConfig; import org.eclipse.jgit.diff.DiffEntry; -import org.eclipse.jgit.diff.RenameDetector; import org.eclipse.jgit.diff.DiffEntry.ChangeType; +import org.eclipse.jgit.diff.RenameDetector; import org.eclipse.jgit.errors.CorruptObjectException; import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.errors.StopWalkException; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.filter.RevFilter; import org.eclipse.jgit.treewalk.TreeWalk; import org.eclipse.jgit.treewalk.filter.TreeFilter; @@ -82,10 +82,7 @@ class RewriteTreeFilter extends RevFilter { private final TreeWalk pathFilter; - private final Repository repository; - RewriteTreeFilter(final RevWalk walker, final TreeFilter t) { - repository = walker.repository; pathFilter = new TreeWalk(walker.reader); pathFilter.setFilter(t); pathFilter.setRecursive(t.shouldBeRecursive()); @@ -142,7 +139,7 @@ public boolean include(final RevWalk walker, final RevCommit c) // commit. We need to update our filter to its older // name, if we can discover it. Find out what that is. // - updateFollowFilter(trees); + updateFollowFilter(trees, ((FollowFilter) tw.getFilter()).cfg); } return true; } @@ -235,7 +232,7 @@ public boolean requiresCommitBody() { return false; } - private void updateFollowFilter(ObjectId[] trees) + private void updateFollowFilter(ObjectId[] trees, DiffConfig cfg) throws MissingObjectException, IncorrectObjectTypeException, CorruptObjectException, IOException { TreeWalk tw = pathFilter; @@ -244,14 +241,14 @@ private void updateFollowFilter(ObjectId[] trees) tw.reset(trees); List files = DiffEntry.scan(tw); - RenameDetector rd = new RenameDetector(repository); + RenameDetector rd = new RenameDetector(tw.getObjectReader(), cfg); rd.addAll(files); files = rd.compute(); TreeFilter newFilter = oldFilter; for (DiffEntry ent : files) { if (isRename(ent) && ent.getNewPath().equals(oldFilter.getPath())) { - newFilter = FollowFilter.create(ent.getOldPath()); + newFilter = FollowFilter.create(ent.getOldPath(), cfg); RenameCallback callback = oldFilter.getRenameCallback(); if (callback != null) { callback.renamed(ent);