Skip to content

Commit

Permalink
Fix possible NPE while traversing RevWalks
Browse files Browse the repository at this point in the history 
This fixes a problem where describing commits would find old
information from a previous walk that could lead to an NPE inside JGit.

This effectively reverts 657cf3d by
reintroducing try-with-resources for `RevWalk` instances. Instead of
using a field inside the mojo, there are now only local instances where
try-with-resources makes sense.
  • Loading branch information
@koraktor
koraktor committed Feb 12, 2019
1 parent 934453e commit 8731a89
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 65 deletions.
29 changes: 7 additions & 22 deletions src/main/java/com/github/koraktor/mavanagaiata/git/jgit/JGitRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ public class JGitRepository extends AbstractGitRepository {
Repository repository;
RevCommit headCommit;
ObjectId headObject;
RevWalk revWalk;

/**
* Creates a new empty instance
Expand Down Expand Up @@ -173,10 +172,6 @@ public void close() {
repository.close();
repository = null;
}

if (revWalk != null) {
revWalk.close();
}
}

@Override
Expand All @@ -191,8 +186,7 @@ public GitTagDescription describe() throws GitRepositoryException {
return new GitTagDescription(getAbbreviatedCommitId(getHeadCommit()), tag,0);
}

prepareRevWalk();
try {
try (RevWalk revWalk = getRevWalk()) {
revWalk.markStart(start);
revWalk.setRetainBody(false);
revWalk.sort(RevSort.COMMIT_TIME_DESC);
Expand Down Expand Up @@ -345,8 +339,7 @@ public Map<String, GitTag> getTags()
throws GitRepositoryException {
Map<String, GitTag> tags = new HashMap<>();

prepareRevWalk();
try {
try (RevWalk revWalk = getRevWalk()) {
for (Ref tag : repository.getRefDatabase().getRefsByPrefix(R_TAGS)) {
try {
RevTag revTag = revWalk.lookupTag(tag.getObjectId());
Expand Down Expand Up @@ -400,10 +393,9 @@ public void loadTag(GitTag tag) throws GitRepositoryException {
return;
}

assert revWalk != null;
JGitTag jgitTag = (JGitTag) tag;

try {
try (RevWalk revWalk = getRevWalk()) {
revWalk.parseBody(jgitTag.tag);
jgitTag.taggerIdent = jgitTag.tag.getTaggerIdent();
} catch (IOException e) {
Expand All @@ -419,8 +411,7 @@ public <T extends CommitWalkAction> T walkCommits(T action)
action.setRepository(this);
action.prepare();

prepareRevWalk();
try {
try (RevWalk revWalk = getRevWalk()) {
revWalk.markStart(getHeadRevCommit());

for (RevCommit commit : revWalk) {
Expand Down Expand Up @@ -480,16 +471,10 @@ ObjectId getHeadObject() throws GitRepositoryException {
}

/**
* Prepares a JGit {@code RevWalk} instance for this repository
* <p>
* Creates a new instance or resets an existing one.
* @return A new JGit {@code RevWalk} instance for this repository
*/
void prepareRevWalk() {
if (revWalk == null) {
revWalk = new RevWalk(repository);
} else {
revWalk.reset();
}
RevWalk getRevWalk() {
return new RevWalk(repository);
}

}
74 changes: 31 additions & 43 deletions src/test/java/com/github/koraktor/mavanagaiata/git/jgit/JGitRepositoryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,12 +279,9 @@ void testCleanIgnoreUntracked() throws Exception {
@DisplayName("should close the underlying JGit repository and RevWalk")
@Test
void testClose() {
repository.revWalk = mock(RevWalk.class);

this.repository.close();

verify(this.repo).close();
verify(repository.revWalk).close();
}

@DisplayName("should not fail when closing a non-existing repository")
Expand Down Expand Up @@ -332,22 +329,20 @@ void testDescribeTagged() throws Exception {
repository.headObject = mock(ObjectId.class);
repository.headCommit = head;

JGitRepository repo = spy(this.repository);
RevWalk revWalk = mockRevWalk();
when(revWalk.iterator()).
thenReturn(asList(head, head_1, head_2).iterator());
RevFlag seenFlag = RevFlag.UNINTERESTING;
when(revWalk.newFlag("2.0.0")).thenReturn(seenFlag);

Map<String, JGitTag> tags = new HashMap<>();
JGitTag tag = createTag("2.0.0", head.getName());
tags.put(head_2.getName(), tag);
doReturn(tags).when(repo).getTags();
doReturn(tags).when(repository).getTags();

repo.revWalk = mock(RevWalk.class);
when(repo.revWalk.iterator()).
thenReturn(asList(head, head_1, head_2).iterator());
RevFlag seenFlag = RevFlag.UNINTERESTING;
when(repo.revWalk.newFlag("2.0.0")).thenReturn(seenFlag);
when(repo.getObjectDatabase().newReader().abbreviate(head)).thenReturn(abbrevId);

when(this.repo.getObjectDatabase().newReader().abbreviate(head)).thenReturn(abbrevId);

GitTagDescription description = repo.describe();
GitTagDescription description = repository.describe();
assertThat(description.getNextTagName(), is(equalTo("2.0.0")));
assertThat(description.toString(), is(equalTo("2.0.0-2-g" + abbrevId.name())));
}
Expand All @@ -368,25 +363,23 @@ void testDescribeMergeCommitFirstBranch() throws Exception {
repository.headObject = mock(ObjectId.class);
repository.headCommit = head;

JGitRepository repo = spy(this.repository);
RevWalk revWalk = mockRevWalk();
when(revWalk.iterator()).
thenReturn(asList(head, head_a1, head_b1, head_b2).iterator());
RevFlag seenFlag = RevFlag.UNINTERESTING;
when(revWalk.newFlag("a1")).thenReturn(seenFlag);
when(revWalk.newFlag("b2")).thenReturn(seenFlag);

Map<String, JGitTag> tags = new HashMap<>();
JGitTag tagA1 = createTag("a1", head.getName());
JGitTag tagB2 = createTag("b2", head.getName());
tags.put(head_a1.getName(), tagA1);
tags.put(head_b2.getName(), tagB2);
doReturn(tags).when(repo).getTags();
doReturn(tags).when(repository).getTags();

repo.revWalk = mock(RevWalk.class);
when(repo.revWalk.iterator()).
thenReturn(asList(head, head_a1, head_b1, head_b2).iterator());
RevFlag seenFlag = RevFlag.UNINTERESTING;
when(repo.revWalk.newFlag("a1")).thenReturn(seenFlag);
when(repo.revWalk.newFlag("b2")).thenReturn(seenFlag);

when(this.repo.getObjectDatabase().newReader().abbreviate(head)).thenReturn(abbrevId);
when(repo.getObjectDatabase().newReader().abbreviate(head)).thenReturn(abbrevId);

GitTagDescription description = repo.describe();
GitTagDescription description = repository.describe();
assertThat(description.getNextTagName(), is(equalTo("a1")));
assertThat(description.toString(), is(equalTo("a1-3-g" + abbrevId.name())));
}
Expand All @@ -409,25 +402,23 @@ void testDescribeMergeCommitSecondBranch() throws Exception {
repository.headObject = mock(ObjectId.class);
repository.headCommit = head;

JGitRepository repo = spy(this.repository);
RevWalk revWalk = mockRevWalk();
when(revWalk.iterator()).
thenReturn(asList(head, head_a1, head_b1, head_a2, head_b2).iterator());
RevFlag seenFlag = RevFlag.UNINTERESTING;
when(revWalk.newFlag("a2")).thenReturn(seenFlag);
when(revWalk.newFlag("b1")).thenReturn(seenFlag);

Map<String, JGitTag> tags = new HashMap<>();
JGitTag tagA1 = createTag("a2", head.getName());
JGitTag tagB2 = createTag("b1", head.getName());
tags.put(head_a2.getName(), tagA1);
tags.put(head_b1.getName(), tagB2);
doReturn(tags).when(repo).getTags();
doReturn(tags).when(repository).getTags();

repo.revWalk = mock(RevWalk.class);
when(repo.revWalk.iterator()).
thenReturn(asList(head, head_a1, head_b1, head_a2, head_b2).iterator());
RevFlag seenFlag = RevFlag.UNINTERESTING;
when(repo.revWalk.newFlag("a2")).thenReturn(seenFlag);
when(repo.revWalk.newFlag("b1")).thenReturn(seenFlag);

when(this.repo.getObjectDatabase().newReader().abbreviate(head)).thenReturn(abbrevId);
when(repo.getObjectDatabase().newReader().abbreviate(head)).thenReturn(abbrevId);

GitTagDescription description = repo.describe();
GitTagDescription description = repository.describe();
assertThat(description.getNextTagName(), is(equalTo("b1")));
assertThat(description.toString(), is(equalTo("b1-3-g" + abbrevId.name())));
}
Expand All @@ -444,8 +435,8 @@ void testDescribeUntagged() throws Exception {
repository.headObject = mock(ObjectId.class);
repository.headCommit = head;

repository.revWalk = mock(RevWalk.class);
when(repository.revWalk.iterator()).
RevWalk revWalk = mockRevWalk();
when(revWalk.iterator()).
thenReturn(asList(head, head_1, head_2).iterator());

when(this.repo.getObjectDatabase().newReader().abbreviate(head)).thenReturn(abbrevId);
Expand Down Expand Up @@ -678,15 +669,15 @@ void testLoadTag() throws Exception {
"Version 1.0.0\n").getBytes());
Date tagDate = new Date(1275131880000L);

repository.revWalk = mock(RevWalk.class);
RevWalk revWalk = mockRevWalk();
JGitTag tag = new JGitTag(rawTag);

repository.loadTag(tag);

assertThat(tag.getDate(), is(equalTo(tagDate)));
assertThat(tag.getTimeZone(), is(equalTo(TimeZone.getTimeZone("GMT+0200"))));

verify(repository.revWalk).parseBody(rawTag);
verify(revWalk).parseBody(rawTag);
}

@DisplayName("should allow walking the HEAD’s history with a CommitWalkAction")
Expand Down Expand Up @@ -734,10 +725,7 @@ private RevWalk mockRevWalk() {
repository = spy(repository);

RevWalk revWalk = mock(RevWalk.class);
doAnswer(invocation -> {
repository.revWalk = revWalk;
return null;
}).when(repository).prepareRevWalk();
doReturn(revWalk).when(repository).getRevWalk();

return revWalk;
}
Expand Down

0 comments on commit 8731a89

Please sign in to comment.