-
Notifications
You must be signed in to change notification settings - Fork 280
Properly handle submodule-only changes #149
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.
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
Conversation
|
This looks good to me, but @sds has more context and might have better thoughts. Can you write a test to go along with this? |
|
Oh, also thanks for the speedy pull request! |
Ensure that when only a submodule change is staged, the change is not affected by #setup_environment and #cleanup_environment. Change-Id: Idbfd394c156369d9f86355e808097f2548f4e8a8
Change-Id: I5fd7db9a291aa87fcdf3fed860b18e7a9ac6a450
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make more sense to check that the submodule change was committed, rather than checking that the diff was empty? It's possible that we simply lost all changes, in which case the diff would still be empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But setup_environment and cleanup_environment run before the commit. Are you suggesting something like this?
`git commit -m "Submodule update"`
`git show`.should match(/-Subproject commit[\s\S]*\+Subproject commit/)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, poor terminology on my part. We should check that git diff --cached has the submodule change in it (i.e. that the changes are still staged). Make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, I'll make the change.
Change-Id: Ibe489e9d606761f4001c6749a199522a6a591803
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sds How's this?
|
Thanks! Merged in 9172dd3. |
Don't clear the working tree or apply the last stash if only submodule changes are staged.
Fixes #148