Skip to content

Commit

Permalink
use --no-walk when merely checking if a revision exists
Browse files Browse the repository at this point in the history
When implementing isCommitInRepo() for the CLI git implementation we use
git rev-list, assuming that if this command fails then the commit
doesn't exist in the repository. This is technically correct.

However, git rev-list actually performs a complete revision walk, which
for large repositories can take a long time. For example, on the Linux
repository, git rev-list HEAD took around 30 seconds. This is entirely
due to the command producing an entire list of all revisions leading
back to the beginning of history.

But isCommitInRepo() doesn't even care about that! We can significantly
reduce the cost of this function in a number of ways. One, we could
re-implement this using various other git commands, such as git
cat-file. Or, we could ask rev-list not to actually walk the revisions.
This was added in git 1.5.3 (~2007).

Local testing found there to be minimal difference in "git cat-file -t
<id>" vs "git rev-list --no-walk <id>". Since the latter is mostly
already implemented, add support for "--no-walk" to the
RevListCommand.

This allows us to modify isCommitInRepo() to use the RevList_()
directly, and pass .nowalk(true) in order to avoid the unnecessary
revision walk.

For completeness, also implement the equivalent support in JGit. To do
so, simply use parseCommit() of the refspec provided to the command. If
all was set to true, we'll search all refs and include those as well.

Ultimately, this enables the command line implementation of the
gitClient to have a significantly faster test for whether a commit is in
a repository.

Signed-off-by: Jacob Keller <[email protected]>
  • Loading branch information
jacob-keller committed Nov 2, 2017
1 parent 40e9281 commit d19a04c
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 4 deletions.
28 changes: 24 additions & 4 deletions src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -2416,6 +2416,7 @@ public List<IndexEntry> lsTree(String treeIsh, boolean recursive) throws GitExce
public RevListCommand revList_() {
return new RevListCommand() {
public boolean all;
public boolean nowalk;
public boolean firstParent;
public String refspec;
public List<ObjectId> out;
Expand All @@ -2429,7 +2430,14 @@ public RevListCommand all(boolean all) {
this.all = all;
return this;
}

public RevListCommand nowalk(boolean nowalk) {
// --no-walk wasn't introduced until v1.5.3
if (isAtLeastVersion(1, 5, 3, 0)) {
this.nowalk = nowalk;
}
return this;
}

public RevListCommand firstParent() {
return firstParent(true);
}
Expand Down Expand Up @@ -2461,6 +2469,10 @@ public void execute() throws GitException, InterruptedException {
args.add("--all");
}

if (nowalk) {
args.add("--no-walk");
}

if (refspec != null) {
args.add(refspec);
}
Expand Down Expand Up @@ -2515,9 +2527,17 @@ public boolean isCommitInRepo(ObjectId commit) throws InterruptedException {
return false;
}
try {
List<ObjectId> revs = revList(commit.name());

return revs.size() != 0;
// Use revList_() directly in order to pass .nowalk(true) which
// allows us to bypass the unnecessary revision walk when we
// only care to determine if the commit exists.
List<ObjectId> oidList = new ArrayList<>();
RevListCommand revListCommand = revList_();
revListCommand.reference(commit.name());
revListCommand.to(oidList);
revListCommand.nowalk(true);
revListCommand.execute();

return oidList.size() != 0;
} catch (GitException e) {
return false;
}
Expand Down
19 changes: 19 additions & 0 deletions src/main/java/org/jenkinsci/plugins/gitclient/JGitAPIImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -1898,6 +1898,7 @@ public RevListCommand revList_()
{
return new RevListCommand() {
public boolean all;
public boolean nowalk;
public boolean firstParent;
public String refspec;
public List<ObjectId> out;
Expand All @@ -1912,6 +1913,11 @@ public RevListCommand all(boolean all) {
return this;
}

public RevListCommand nowalk(boolean nowalk) {
this.nowalk = nowalk;
return this;
}

public RevListCommand firstParent() {
return firstParent(true);
}
Expand Down Expand Up @@ -1941,6 +1947,19 @@ public void execute() throws GitException, InterruptedException {
ObjectReader or = repo.newObjectReader();
RevWalk walk = new RevWalk(or)) {

if (nowalk) {
RevCommit c = walk.parseCommit(repo.resolve(refspec));
out.add(c.copy());

if (all) {
for (Ref r : repo.getAllRefs().values()) {
c = walk.parseCommit(r.getObjectId());
out.add(c.copy());
}
}
return;
}

if (all)
{
markAllRefs(walk);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,14 @@ public interface RevListCommand extends GitCommand {
*/
RevListCommand all(boolean all);

/**
* nowalk.
*
* @param nowalk {@code true} to skip revision walk.
* @return a {@link org.jenkinsci.plugins.gitclient.RevListCommand} object.
*/
RevListCommand nowalk(boolean nowalk);

/**
* firstParent.
*
Expand Down
19 changes: 19 additions & 0 deletions src/test/java/org/jenkinsci/plugins/gitclient/GitClientTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ public class GitClientTest {
private final boolean CLI_GIT_SUPPORTS_SUBMODULE_DEINIT;
private final boolean CLI_GIT_SUPPORTS_SUBMODULE_RENAME;
private final boolean CLI_GIT_SUPPORTS_SYMREF;
private final boolean CLI_GIT_SUPPORTS_REV_LIST_NO_WALK;

@Rule
public ExpectedException thrown = ExpectedException.none();
Expand All @@ -114,6 +115,7 @@ public GitClientTest(final String gitImplName) throws IOException, InterruptedEx
CLI_GIT_SUPPORTS_SUBMODULE_DEINIT = cliGitClient.isAtLeastVersion(1, 9, 0, 0);
CLI_GIT_SUPPORTS_SUBMODULE_RENAME = cliGitClient.isAtLeastVersion(1, 9, 0, 0);
CLI_GIT_SUPPORTS_SYMREF = cliGitClient.isAtLeastVersion(2, 8, 0, 0);
CLI_GIT_SUPPORTS_REV_LIST_NO_WALK = cliGitClient.isAtLeastVersion(1, 5, 3, 0);

boolean gitLFSExists;
try {
Expand Down Expand Up @@ -925,6 +927,23 @@ public void testRevList() throws Exception {
assertThat(resultB, contains(commitB, commitA));
}

@Test
public void testRevListNoWalk() throws Exception {
assumeTrue(CLI_GIT_SUPPORTS_REV_LIST_NO_WALK);
ObjectId commitA = commitOneFile();
List<ObjectId> resultA = new ArrayList<>();
gitClient.revList_().to(resultA).reference(commitA.name()).nowalk(true).execute();
assertThat(resultA, contains(commitA));
assertEquals(resultA.size(), 1);

/* Make sure it's correct when there's more than one commit in the history */
ObjectId commitB = commitOneFile();
List<ObjectId> resultB = new ArrayList<>();
gitClient.revList_().to(resultB).reference(commitB.name()).nowalk(true).execute();
assertThat(resultB, contains(commitB));
assertEquals(resultB.size(), 1);
}

// @Test
public void testSubGit() throws Exception {
// Tested in assertSubmoduleContents
Expand Down

0 comments on commit d19a04c

Please sign in to comment.