Optionally warn (once) if repository is shallow/sparse

[dbwiddis]

Fixes #252.

Adds a new parameter skipIfShallow that will have the same effect as skip if executed on a shallow clone.

[hazendaz]

@dbwiddis I'm adding github actions ci in...waiting on abilty to self merge as I'm otherwise waiting. I trust that a bit more than what all this seems to be trying to pull off in travis CI so I'll come back to this afterwards. Hopefully later this weekend. As to the change, I feel like the default may need flipped but false to start with while its being floated around seems like a logic first step. We did something similar on formatter maven plugin and even flipped the flag for our next release after it was proven out. Anyway, this is useful in regards to your other findings ;)

[dbwiddis]

I feel like the default may need flipped

I went back and forth on a few other ideas. The shallow history is only really relevant if you're generating copyright range using ${license.git.copyrightCreationYear} or the new ${license.git.copyrightExistenceYears}, and it could be harmful if defaulting to true otherwise and unnoticed.

I pondered checking for those definitions in the template and defaulting to true only if they were defined, and/or issuing a log warning but figured it would get lost in log spam if defaulting true, and lost in header rewrite spam otherwise, and ultimately increases complexity.

What I really wanted to do is do a regex digit match for the beginning year of the copyright, only when those substitutions were used, and ignore the "newer" year if it was a shallow clone. Once I got to the two branches (strict vs. not) I decided it wasn't worth the effort.

Potentially this setting should be mentioned in the documentation for those two template substitutions, though.

[dbwiddis]

Converting this to a draft because I think this can be done one of two ways without the Runtime.exec() call:

1. It does look like JGIt has functionality to detect a shallow clone (PackStatistics depth > 0)
2. We can also test for the existence of .git/shallow in the filesystem

[mathieucarbou]

Thanks guys for looking into that 👍

[dbwiddis]

OK, new approach! I think this makes a lot more sense, causing a failure when it should...

• The parameter is now failIfShallow and defaults to true.
• Detects shallow clone by testing for existence of .git/shallow file.
• If failIfShallow is true causes an exception to be thrown only when attempting to fetch properties requiring full git history
  • original author/email
  • copyright dates that are not the inception year

[dbwiddis]

Hmm, obviously I messed something up. Will hunt down that NPE.

[dbwiddis]

It's throwing NPE on this line (two places):

if (mojo.failIfShallow && gitLookup.isShallowRepository()) 

After my latest fix it is impossible for gitLookup to be null. Is mojo null in this case? I'm not sure how I can access the value of the failIfShallow property.

[dbwiddis]

Ah, I see the problem, the test is passing null for the mojo:

Map<String, String> actual = provider.getAdditionalProperties(null, props, document);

I'll instantiate a new somethingMojo in the test...

[dbwiddis]

Switched back to draft for 2 discussion points:

1. At present we'd throw an exception for shallow repo for the project.inceptionYear. That doesn't require git history so I'd like to refactor that substitution out to its own provider. Non-issue. This is just a lookup from the existing properties passed.
2. Should I null-check repo in the provider and assume default, e.g., true setting in that case? Is there any real world use case where null is a valid argument?

[mathieucarbou]

2. Should I null-check repo in the provider and assume default, e.g., true setting in that case? Is there any real world use case where null is a valid argument?

I I maybe lacking context (I don't understand fully the question), but if it is regarding injected Maven properties, then we have to stub them in our test. In real life they won't be null.

Or the question is regarding the existence or not of a git repo when this plugin is used ?

[dbwiddis]

I maybe lacking context (I don't understand fully the question), but if it is regarding injected Maven properties, then we have to stub them in our test. In real life they won't be null.

That was essentially the question. The context was that in the test cases (here) we were passing null which wasn't a problem when we never referred to the mojo, but is now. So my "test fix" creating a new mojo was the appropriate way to address this.

I'll mark this as ready again, pending further discussion!

[mathieucarbou]

@dbwiddis I'm sorry I didn't notice all your last activity... I was wondering if your PR is ready to be reviewed again, and also if you could rebase it if it is ? Thanks a lot!

[dbwiddis]

Rebased to master, although I don't think that's necessary when GitHub says "This branch has no conflicts with the base branch."

I think this PR is GTG but please note #300 which is essentially the same problem as I'm trying to solve here, but far less detectable, and I believe is a blocking bug to your next release. I'd suggest at least thinking through the solution to #300 before merging this to see if there are any changes needed.

Also note that I had submitted #286 for comment, which would be a workaround to this. I withdrew it because I think there's room for a much more flexible way of tokenizing the whole copyright line (basically everything after the word "copyright" up to the next newline should be allowed to be kept as is) which would also be a workaround for this.

[mathieucarbou]

Thanks! Will merge.