Skip to content

add boolean property: 'files.dirty' if working dir is dirty #142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 6, 2014
Merged

add boolean property: 'files.dirty' if working dir is dirty #142

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
00fae69
add boolean property: 'files.dirty' if working dir is dirty
abatkin Dec 2, 2014
0cc2459
Small cleanups
abatkin Dec 3, 2014
f1108b5
add unit test checking for dirty files
abatkin Dec 3, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/main/java/pl/project13/maven/git/GitCommitIdMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public class GitCommitIdMojo extends AbstractMojo {
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";
Copy link
Collaborator

@ktoso ktoso Dec 2, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not match any naming convention.
please previx with commit.id

Copy link
Contributor Author

@abatkin abatkin Dec 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

public static final String COMMIT_DESCRIBE = "commit.id.describe";
public static final String COMMIT_SHORT_DESCRIBE = "commit.id.describe-short";
public static final String BUILD_AUTHOR_NAME = "build.user.name";
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/pl/project13/maven/git/GitDataProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public abstract class GitDataProvider {
protected abstract String getGitDescribe() throws MojoExecutionException;
protected abstract String getCommitId();
protected abstract String getAbbrevCommitId() throws MojoExecutionException;
protected abstract boolean isDirty() throws MojoExecutionException;
protected abstract String getCommitAuthorName();
protected abstract String getCommitAuthorEmail();
protected abstract String getCommitMessageFull();
Expand Down Expand Up @@ -65,6 +66,8 @@ public void loadGitData(@NotNull Properties properties) throws IOException, Mojo
put(properties, GitCommitIdMojo.COMMIT_ID, getCommitId());
// git.commit.id.abbrev
put(properties, GitCommitIdMojo.COMMIT_ID_ABBREV, getAbbrevCommitId());
// git.files.dirty
put(properties, GitCommitIdMojo.FILES_DIRTY, isDirty()? "true" : "false");
Copy link
Collaborator

@ktoso ktoso Dec 2, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is same as isDirty.toString

Copy link
Contributor Author

@abatkin abatkin Dec 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. It's also autoboxing just to extract the textual value

// git.commit.author.name
put(properties, GitCommitIdMojo.COMMIT_AUTHOR_NAME, getCommitAuthorName());
// git.commit.author.email
Expand Down
10 changes: 10 additions & 0 deletions src/main/java/pl/project13/maven/git/JGitProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,16 @@ protected String getAbbrevCommitId() throws MojoExecutionException {
return abbrevCommitId;
}

@Override
protected boolean isDirty() throws MojoExecutionException {
Git gitObject = Git.wrap(git);
try {
return !gitObject.status().call().isClean();
Copy link
Collaborator

@ktoso ktoso Dec 2, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks ok, though we should probably measure performance impact of wrapping at some point...

} catch (GitAPIException e) {
throw new MojoExecutionException("Failed to get git status: " + e.getMessage(), e);
}
}

@Override
protected String getCommitAuthorName() {
String commitAuthor = headCommit.getAuthorIdent().getName();
Expand Down
6 changes: 6 additions & 0 deletions src/main/java/pl/project13/maven/git/NativeGitProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,12 @@ protected String getAbbrevCommitId() throws MojoExecutionException {
return abbrevCommitId;
}

@Override
protected boolean isDirty() throws MojoExecutionException {
String output = tryToRunGitCommand(canonical, "status --porcelain");
Copy link
Collaborator

@ktoso ktoso Dec 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Contributor Author

@abatkin abatkin Dec 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

@ktoso ktoso Dec 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@abatkin abatkin Dec 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I missed that.

  1. Doesn't waitFor() wait for the process to exit? So you may not have to sit around reading all of the input in, but git still has to run to completion
  2. This won't work on Windows systems

Copy link
Collaborator

@ktoso ktoso Dec 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. it's still way faster than consuming the stream into string, as in - doesn't kill my build
  2. I don't think the native impl ever worked on windows.

return !output.trim().isEmpty();
}

@Override
protected String getCommitAuthorName() {
return tryToRunGitCommand(canonical, "log -1 --pretty=format:\"%cn\"");
Expand Down