Skip to content
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

Changes lost which are made attempting a commit #135

Closed
pratik60 opened this Issue Mar 19, 2015 · 11 comments

Comments

Projects
None yet
4 participants
@pratik60
Copy link

pratik60 commented Mar 19, 2015

Let me explain this with an example

I try to commit two files.
git commit -am "test"

Now the overcommit precommit hooks come into place, and it keeps starts going through the various checks I've enabled. It reports an error in the initial check itself, and I promptly go to the file to edit the change while I keep the pre-commit hook running so that i may see the complete list of changes at the end.

But after the pre-commit hook is over, all the changes made during this time are completely lost as it does some kind of reset.

Any way I can fix this? Why is overcommit trying to be smart and restoring file, in case the checks fail....?

Any quick fix?

@pratik60 pratik60 changed the title Changes lost which are made during commiting Changes lost which are made attempting a commit Mar 19, 2015

@pratik60

This comment has been minimized.

Copy link
Author

pratik60 commented Mar 19, 2015

Can almost certainly say the problem is somewhere here (lib/overcommit/hook_context/pre_commit.rb) , can you please tell me how to disable this? I'm assuming it isn't intentional but I really don't understand why overcommit is trying to do a reset hard on my code. :-(

Please reply soon.

@sds sds added the question label Mar 19, 2015

@sds

This comment has been minimized.

Copy link
Collaborator

sds commented Mar 19, 2015

Hey @pratik60,

This is intended behavior, but I understand why it seems confusing, so let me explain.

Overcommit's pre-commit hook needs to stash your unstaged changes so they don't affect the outcome of the hook run (since the unstaged changes aren't going into the commit, they shouldn't be checked by the hooks).

Once the checks have finished running, git requires us to git reset --hard in order to restore your repo to the state it was before the hook ran. This is unavoidable because you can't apply a stash to a dirty working tree.

You may ask why we don't do this in a separate directory with temporary files, and the reason is that it would mess up all the paths. Tools like rubocop use configuration files with paths that would break if we started doing all of this in temporary directories and files, so we stopped doing that in order to make things work.

Hope that explains things. In a nutshell, this is not a bug but Overcommit working as intended. I recommend you not make changes to your files while commit hooks are running.

@sds sds closed this Mar 19, 2015

@pratik60

This comment has been minimized.

Copy link
Author

pratik60 commented Mar 23, 2015

@sds - We are still trying to wrap our heads around this.

"Once the checks have finished running, git requires us to git reset --hard in order to restore your repo to the state it was before the hook ran. This is unavoidable because you can't apply a stash to a dirty working tree."

If the precommit hook has failed, why are you required to restore my repo to the state before hook? Why can't you just let it be?

Anyways our rpsec, and jasmine suite runs as a part of precommit hook, but rubocop runs before it, and throws up error, and keep rspec and jasmine running in background. I naturally want to go and fix it, but then all my changes are lost as you do a git reset --hard after 5 minutes.

I cannot believe this is the long term solution :-(

I want a great developer experience, i love how easy it is to integrate stuff, the features it provides etc. But I can't go and tell all devs, to wait for 5 minutes because any changes made now will be lost.

@jawshooah

This comment has been minimized.

Copy link
Collaborator

jawshooah commented Mar 23, 2015

If the precommit hook has failed, why are you required to restore my repo to the state before hook? Why can't you just let it be?

All unstaged changes are stashed before the hooks run. To restore those changes if a hook fails, we must first run git reset --hard because, as @sds explained, we can't apply the stash to a dirty working tree, which is exactly what you are creating when you change files while the hooks are running.

If we didn't git reset --hard after the hooks run, then we couldn't apply the stash containing your previous unstaged changes, and there would be a ton of bug reports like #129 claiming that file changes were lost.

@jawshooah

This comment has been minimized.

Copy link
Collaborator

jawshooah commented Mar 23, 2015

Besides, and this is just my opinion, you shouldn't be running your entire test suite on each commit. Each push, maybe, but each commit is definitely overkill.

Git encourages us to keep our commits as small and atomic as possible, which means that commits will probably be made fairly often, while pushes should happen only when you are satisfied that your local copy works as intended.

For example, say you make a change to an existing feature that intentionally breaks previously expected behavior. You would probably have separate commits for the change itself and for modifying the tests accordingly. You would only expect the tests to pass after both commits.

To me, this is a strong argument for adding pre-push hook support. I'll get on it when I have a chance.

@sds

This comment has been minimized.

Copy link
Collaborator

sds commented Mar 23, 2015

Hey @pratik60,

As @jawshooah has alluded to, Overcommit isn't intended to be used for running entire test suites before committing. You can use overcommit --run as part of a CI run for that.

Git commit hooks (especially pre-commit hooks) should be fast. They are intended to give you quick feedback before sending your commit through a heavier duty series of tests and code review (which take much longer).

@jawshooah brings up an interesting suggesting of using pre-push to run actual integration tests. I don't feel strongly about this suggestion but I don't think hooks are the right place to run your CI suite, as you technically can't guarantee your developers have the hooks installed.

Circling back to your original complaint about Overcommit stashing/resetting/unstashing changes: this is absolutely necessary so that when Overcommit runs hooks against your files it is only reporting issues for changes that are about to be committed.

If you were to make changes to files during a hook run and Overcommit didn't attempt to isolate, you would potentially see errors/warnings introduced because you made changes during the hook run, even though those changes aren't actually about to be committed. It's essentially a concurrency problem where multiple threads (Overcommit and yourself) are trying to work with a shared resource (the files). To guarantee its usefulness, Overcommit needs full control of the resource during the hook run.

Hope that explains things, and I hope it's clear why you should not be running your test suite as part of a pre-commit hook. Leave that to your CI runs.

@jawshooah

This comment has been minimized.

Copy link
Collaborator

jawshooah commented Mar 23, 2015

@sds Definitely agree that running test suites should primarily be the responsibility of your CI environment. But pushing broken code and waiting for the CI server to complain about it introduces (what I consider to be) unnecessary overhead.

A pre-push hook would help to ensure that you never push broken code in the first place, keeping your CI build history clean and your fellow devs happy 😄

@sds

This comment has been minimized.

Copy link
Collaborator

sds commented Mar 23, 2015

Being able to push broken code is a process problem. You could always get around it by disabling Overcommit, so how useful is it really?

Better to instrument a process where all commits are automatically deployed once they pass tests, so you can remove the human element entirely. However, that's putting a lot of faith in your tests.

If you want to add a pre-push hook, there's no stopping you. :)

@pratik60

This comment has been minimized.

Copy link
Author

pratik60 commented Mar 23, 2015

Very interesting views guys, and I tend to agree with both of you.

The point taken that rspec and jasmine are too long for pre-commit. I also don't want it to be the developer's onus that he runs rspec and jasmine manually himself, or we wait for it before the CI server complains. A pre-push really makes sense that way.

So after git 1.8.2 we do have a git pre push hook. https://github.com/git/git/blob/87c86dd14abe8db7d00b0df5661ef8cf147a72a3/templates/hooks--pre-push.sample

How may we go implementing that, as I don't think overcommit supports it already? Right?

Btw thank you to both of you guys for weighing in on the issue. Your views are helpful and make me understand the problem better.

@jawshooah

This comment has been minimized.

Copy link
Collaborator

jawshooah commented Mar 23, 2015

@sds I envision the use of pre-push hooks (and really pre-commit hooks as well) as being for peace of mind rather than enforcement of policy since, as you say, it's easy to just skip the hooks entirely. Some lazy dev could even uninstall Overcommit and bypass the entire process. There's no way to ensure that everyone is using the hooks properly, so really this entire endeavor operates on the honor system.

If you want to add a pre-push hook, there's no stopping you. :)

Count on it!

@pratik60 Until (unless?) Overcommit implements pre-push support, you can just roll your own. Overcommit won't overwrite any hooks that don't conflict with the ones it supports.

@pirj

This comment has been minimized.

Copy link

pirj commented Feb 16, 2017

you can't apply a stash to a dirty working tree.

Is this still the case?
Just tried this:

  cd stash
  git init
  touch one
  git add one
  git commit -m 'one'
  echo two > two
  git add two
  git commit -m 'two'
  echo two2 > two
  git stash
  echo one1 > one
  echo This stash passed
  git stash pop || exit 1
  cat two
  git stash
  echo two3 > two
  git add two
  echo This stash with fail
  git stash pop || exit 1
  echo Unreachable

You cannot pop a stash when working tree contains dirty files that are also in a stash, but this isn't the case, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.