add boolean property: 'files.dirty' if working dir is dirty #142
Conversation
| @@ -59,6 +59,7 @@ | |||
| public static final String BRANCH = "branch"; | |||
| public static final String COMMIT_ID = "commit.id"; | |||
| public static final String COMMIT_ID_ABBREV = "commit.id.abbrev"; | |||
| public static final String FILES_DIRTY = "files.dirty"; | |||
There was a problem hiding this comment.
does not match any naming convention.
please previx with commit.id
There was a problem hiding this comment.
I agree that it doesn't match any naming convention, but it's also not part of the commit id or anything like that. This particular property has more to do with the files on the filesystem than anything else though. It's easy enough for me to change the name, but I don't think that "commit.id.files.dirty" (or anything starting "commit.id") is really intuitive. "commit.files.dirty" maybe?
|
please include a test for this property if you'd like to see this merged (and address review feedback) :) |
|
Thanks for updating the PR (and the test)! |
|
Thinking more, maybe even |
|
I was thinking that it's not really a property of the commit, but of the repository, so |
|
I could use this one (in addition to pull #143) immediately! |
add boolean property: 'files.dirty' if working dir is dirty
|
Merged – I'll do some more cleanups this weekend and cut a release tomorrow night :) |
|
You rock. Thanks for the feedback (i.e. forcing me to write a unit test) and for merging. |
|
Thanks for the kudos, and the PR! This is now available as I reimplemented the native checking as well, so it's faster now. |
| @@ -176,6 +176,12 @@ protected String getAbbrevCommitId() throws MojoExecutionException { | |||
| } | |||
|
|
|||
| @Override | |||
| protected boolean isDirty() throws MojoExecutionException { | |||
| String output = tryToRunGitCommand(canonical, "status --porcelain"); | |||
There was a problem hiding this comment.
this was very expensive, I have reimplemented it to not kill performance on large dirty repositories :-)
This impl has to wait for the entire status to return, while we only want to know if it's more than 0 lines. :)
There was a problem hiding this comment.
Sounds reasonable. But I'm curious if -s is really faster than --porcelain? On my system both produce the same output (the former being colorized, assuming git thinks it is talking to a tty). On the largest repo I could quickly find (linux kernel) both performed identically (~100ms after 10 runs).
There was a problem hiding this comment.
That's not the speed up change.
The speed up change is to not collect the entire output into a string and then check isEmpty() - way too costly. Instead as I described above, we return once there's a line of output. See https://github.com/ktoso/maven-git-commit-id-plugin/blob/master/src/main/java/pl/project13/maven/git/NativeGitProvider.java#L363
There was a problem hiding this comment.
Ahh, I missed that.
- Doesn't
waitFor()wait for the process to exit? So you may not have to sit around reading all of the input in, butgitstill has to run to completion - This won't work on Windows systems
There was a problem hiding this comment.
- it's still way faster than consuming the stream into string, as in - doesn't kill my build
- I don't think the native impl ever worked on windows.
Tested with both JGit and native git. Handles both staged and unstaged commits.