Issue #704: Use built in git-describe #705
Conversation
Generate changelog in
|
src/main/java/com/palantir/gradle/gitversion/VersionDetailsImpl.java
Outdated
Show resolved
Hide resolved
| @@ -56,7 +57,7 @@ private boolean isClean() { | |||
| private String description() { | |||
| String rawDescription = expensiveComputeRawDescription(); | |||
| String processedDescription = | |||
| rawDescription == null ? null : rawDescription.replaceFirst("^" + args.getPrefix(), ""); | |||
| Strings.isNullOrEmpty(rawDescription) ? null : rawDescription.replaceFirst("^" + args.getPrefix(), ""); | |||
There was a problem hiding this comment.
nit: Would prefer if we didn't change this. Treating the empty string the same as null can result in subtle bugs.
There was a problem hiding this comment.
This one we need to change because the describe method no longer returns a null (that is, wraps a thrown exception) when the result is an empty string. The previous implementation erroneously threw an exception because it would try to truncate the empty string when git returned no found result.
I think it's still okay because it is at this single chokepoint and passes the null on up so that all the rest of the code that calls this still behaves the same.
We could check for empty string in Git.describe() and return a null instead, but that seems worse than this.
I did do some extra cleanup of removing the unnecessary extra method in VersionDetailsImpl - The expensiveComputeRawDescription method is no longer needed since we are not caching the resulting value anymore (from Yiwen's change a couple of months ago).
There was a problem hiding this comment.
We could check for empty string in Git.describe() and return a null instead, but that seems worse than this.
I think I would prefer this, primarily because it removes ambiguity from the contract of describe and avoids leaking concerns across function boundaries. Null means the operation failed, non-null means the operation succeeds - callers don't have think about the empty string case.
There was a problem hiding this comment.
Changing the return value of Git.describe() to an Optional<String> would be an even better solution, making this contract very clear to callers.
|
Refusing to release a major rev - please do this manually at https://autorelease.general.dmz.palantir.tech/palantir/gradle-git-version |
Before this PR
Addresses Issue #704
After this PR
==COMMIT_MSG==
Use built in git-describe
==COMMIT_MSG==
Possible downsides?
Will require version of git <10 years old. If you consider that a downside.