-
Notifications
You must be signed in to change notification settings - Fork 280
Add failing test case for restoring unstaged changes #158
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
Add test case for #cleanup_environment to check that unstaged changes are restored when no changes were staged. This test is currently failing.
|
Pushed the fix on this branch instead of in a separate PR. |
|
I would rather have the fix for this accompanying the newly introduced tests. However, I can see why you didn't come up with a fix yet, as the problem is rather nuanced. Some facts:
I'm a little confused as to the reasoning behind 2, so I've posted a question to the Git mailing list. I would link to the archive but it appears to be down at the moment. Here is the question I posed to the list:
I'll let you know if I hear anything back. However, it looks like we're going to need to special case submodules so that even though git doesn't stash them we "stash" changes on behalf of the user. |
|
Ah, I see you merged a fix. Great! However, doesn't this still result in submodule changes being lost, since when we |
|
The specs for submodule-only changes still pass, so we know that when only submodule changes are staged, everything works as expected. We should add specs for when submodule changes are staged in addition to file-level changes for completeness. |
|
@sds I've added specs to check that submodule changes are preserved when staged along with file changes. All green. |
|
You could also post your question to the git-users Google group in case the mailing list is down. |
|
Nice! For some reason I thought I had tested running |
Add test case for
Overcommit::HookContext::PreCommit#cleanup_environmentto check that unstaged changes are restored when no changes were staged. This test is currently failing.This is likely a regression introduced by #149. My bad. Fix incoming in a separate PR.