CHE-3760: Fix git commit not staged specified path with amend #3833
Conversation
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1712/ |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1734/ |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1738/ |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1756/ |
| @@ -564,14 +589,27 @@ public Revision commit(CommitParams params) throws GitException { | |||
| } | |||
|
|
|||
| CommitCommand commitCommand = getGit().commit() | |||
| .setCommitter(committerName, committerEmail).setAuthor(committerName, committerEmail) | |||
| .setAllowEmpty(params.isAmend()) | |||
There was a problem hiding this comment.
I'd move this setting to .setAmend(...) and add comment why is it needed
| if other changes are present, but the list of changed and specified paths is empty, to prevent committing other paths. | ||
| TODO Remove throwing exception when the bug will be fixed. | ||
| */ | ||
| if (!specified.isEmpty() && !(params.isAll() ? changed.isEmpty() : staged.isEmpty()) && specifiedChanged.isEmpty()) { |
There was a problem hiding this comment.
I'd move this check above to other checks before creating commit comand
There was a problem hiding this comment.
Please double check that it is right check. Looks like we should throw exception here in case when specified is not empty and specifiedChanged is empty.
There was a problem hiding this comment.
Moved the check. Added tests for this exception.
| if (!params.isAmend()) { | ||
| // Check that there are staged changes present for commit, or any changes if 'isAll' is enabled | ||
| if (status.isClean()) { | ||
| throw new GitException("Nothing to commit, working directory clean"); |
There was a problem hiding this comment.
Should we throw exception that are related to changes and then about missing commiter's name and email?
Please check behavior of native git and do in the same way.
Any way I think we should check repository state and https://github.com/eclipse/che/pull/3833/files#diff-7f323c0167c366f41270f82a35ed368dR579 and only then throw exception related to changes.
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1763/ |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1780/ |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1868/ |
| if (message == null) { | ||
| throw new GitException("Message wasn't set"); | ||
| // Check repository state | ||
| if (!repository.getRepositoryState().canCommit()) { |
There was a problem hiding this comment.
Is getRepositoryState easy operation? I'd save result to variable and then reuse it
| .setMessage(message) | ||
| .setAll(params.isAll()) | ||
| .setAmend(params.isAmend()); | ||
| .setAmend(params.isAmend()) | ||
| // Allow empty commits with specified paths with amend |
There was a problem hiding this comment.
comment says about commits with specified paths but code in next line doesn't do it. Fix code or comment
| only changed and specified paths. If other changes are present, but the list of changed and specified paths is empty, | ||
| throw exception to prevent committing other paths. TODO Remove this check when the bug will be fixed. | ||
| */ | ||
| } else if (!specified.isEmpty() && !(params.isAll() ? changed.isEmpty() : staged.isEmpty()) && specifiedChanged.isEmpty()) { |
There was a problem hiding this comment.
To make comment clear I'd rewrite it as
if (!params.isAmend()) {
...
} else {
// comment with TODO to remove this block
if (...) {
...
}
}
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1890/ |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1897/ |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1983/ |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2007/ |
|
Build finished. |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2084/ |
vinokurig
added
the
kind/bug
What does this PR do?
Fix throwing exception if working directory is clean or nothing added to commit.
Add a workaround to allow empty commit with amend and specified paths.
What issues does this PR fix or reference?
#3760
Changelog
BugFix: Fix throwing exception on Git commit, if working directory is clean or nothing added to commit.
Release Notes
BugFix N/A
Docs PR
BugFix N/A