GitAPITestCase is a JUnit 3 test class that provides test implementations that are used in 3 different tests (CliGitAPIImplTest, JGitAPIImplTest, and JGitApacheAPIImplTest). We're working to replace that combination of 4 test classes with JUnit 4 tests that are based on GitClientTest.

I'd suggest that you create a separate test class based on GitClientTest for this specific case. The existing git client plugin test classes feel too large to me. You may even want to make the new test specific to JGit, without the overhead and complexity of a parameterized test.

Since the existing tests did not detect this problem, I think adding a new test (and even a new test class) is quite reasonable.

Understood and will do. To be clear, the log from above is from a brand new test for the current issue. (Before I realized there was already some related coverage elsewhere.)

I'm at a loss as to why it's returning friendly LooseUnpeeled as opposed to the problematic PeeledNonTag for the lightweight tag. This suggests the test won't actually replicate the NPE use case. But I'll take another run at it with the JUnit 4 base. Knock on wood, maybe it will replicate the scenario then.

LooseUnpeeled objects are, naturally, loose refs. Peeled[Non]Tag objects are packed refs.

JGit 4.9.0+ packs all refs upon cloning. 4.8.0 and prior leaves them loose. That explains the variation in types.

I'm close to having a valid test and then the patch. But the upcoming two days are booked; it may be next week before this lands in a PR.

Thanks so much for your efforts brianeray!

Yep. Finally have a test that replicates the scenario available for a sneak peak here in my fork.

[ERROR] Failures:
[ERROR]   JGitClientTest.testGetTags_packedRefs:143
Expected: a collection containing (hasProperty("name", "lightweight_tag") and hasProperty("SHA1", <AnyObjectId[45809c4f5d218192b94cb668964ecb7f0a3aeebc]>))
     but: mismatches were: [hasProperty("name", "lightweight_tag")  property 'name' was "annotated_tag", hasProperty("SHA1", <AnyObjectId[45809c4f5d218192b94cb668964ecb7f0a3aeebc]>)  property 'SHA1' was null]
[INFO]
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0

That was almost as tricky to write as the NPE was to track down. (I wish the tests were Spock specs. ) Fixing the bug might be the easiest part!

Time to rebase, then the actual patch, then the PR ... maybe by the end of the week.

Thanks for doing that. Your experience matches mine when exploring a bug. By the time I've duplicated the bug in a unit test, I've understood it well enough that the bug fix is simple compared to all the learning that preceded it.

brianeray I'm starting the planning so that we can release a new version of the git plugin in the next 1-2 weeks. I'd love to include this fix in a git client plugin release at the same time, if it is ready. Thanks again for your work on it!

There are 3 pull requests to the git plugin that need to be evaluated and merged before the release. Wanted you to know the plans for the release in case that information helps you.

Thanks markewaite. PR is imminent, Monday at the latest but more likely this afternoon.

The full Git Client test suite just finished up here on my local, rebased-to-fresh-master branch. 100% success and and coverage on getTags() via Jacoco (largely in thanks to existing tests to be sure).

After grabbing some lunch I should be able slip the plugin into my test Jenkins and confirm the downstream NPE is squashed.

Expected to release in git client plugin 3.1.2 within the next 2 weeks

Release in git client plugin 3.2.0 March 1, 2020.