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

Detect git commit --amend in pre-commit hooks so staged files include everything in commit #146

Closed
sds opened this issue Mar 25, 2015 · 7 comments

Comments

@sds
Copy link
Owner

sds commented Mar 25, 2015

Running pre-commit hooks when amending a commit currently behaves a little strangely in that only the changes being added to the commit (not all changes in the commit) are checked by Overcommit.

This sometimes results in weird behavior where depending on the order you add files and amend them to a commit you can get around certain pre-commit hooks. For example, with Rubocop's MethodLength cop set to 20 lines, if you add a method that is 19 lines long, commit it, then amend the commit to add another couple of lines to the method body, the Rubocop lint message will now become a warning because it is reported on a line that is not currently staged, since it's already in the commit.

Benefits

  • Prevents potential bugs/problems being introduced due to order of changes being amended
  • Assuming we can detect when a pre-commit hook is being executed as part of an "amend", this allows us to write a hook that prevents amending commits on certain branches (e.g. master) to enforce certain workflows

Costs

  • Unclear if there is a definitive way to detect an amend on pre-commit (if it requires a non-reliable hack, this feature may be dead in the water)
  • Significant behavior change, which may be surprising to existing users

Feel free to comment or ask questions in this issue.

@jawshooah
Copy link
Collaborator

Since no new commits have been created when the pre-commit hook runs, and the hook receives no helpful information via args or STDIN, really the only way is to check the command that initiated the commit via ps -ocommand= -p $PPID or similar.

@sds
Copy link
Owner Author

sds commented Apr 7, 2015

What happens if the user invoked git commit --amend via an alias in their .gitconfig? Running a quick test on my system and running ps f it looks like the command will have the alias' name (e.g. for amend = commit --amend):

/bin/zsh -l
\_ git amend
   \_ ruby .git/hooks/pre-commit

Luckily, we can easily see if an alias is defined using git config --get-regexp ^alias\.amend. Even better, git prevents you from having aliases point to other aliases or reference themselves, so we should be able to get away with just one check to see if commit.*--amend matches the alias.

For aliases that invoke shell scripts, those scripts will still end up triggering their own pre-commit hook, e.g. for:

[alias]
amend = "!f() { git commit --amend; }; f"

...running git amend would produce the following output in ps f:

/bin/zsh -l
\_ git amend
   \_ /bin/sh -c f() { git commit --amend; }; f f() { git commit --amend; }; f
      \_ git commit --amend
         \_ ruby .git/hooks/pre-commit

...and so our original code for detecting amends will work just fine.

@jawshooah
Copy link
Collaborator

Looks like shell aliases are expanded in the ps output as well, so this might actually work! 😄

@jawshooah
Copy link
Collaborator

Seems like git aliases are also expanded. Using a dummy repository with the following pre-commit hook:

#!/usr/bin/env bash
ps -ocommand= -p $PPID

I get the following:

$ git config alias.amd 'commit --amend'
$ git amd
/usr/local/bin/git commit --amend
[master 675d5e1] blargh
...

@sds
Copy link
Owner Author

sds commented Apr 7, 2015

Which OS are you running? On both CentOS 7 and Mac OS X 10.10.2 I get the following output using your script:

$ git amend
git amend

I've tested this with both bash 3.2.57(1) and zsh 5.0.5. It's possible there is some other setting that controls the output here?

@jawshooah
Copy link
Collaborator

Whoops, forgot that git was aliased to hub, which probably expands any aliases internally.

$ git amend
/usr/local/bin/git commit --amend

$ /usr/local/bin/git amend
/usr/local/bin/git amend

@sds
Copy link
Owner Author

sds commented Apr 8, 2015

Implemented in #167. Thanks!

@sds sds closed this as completed Apr 8, 2015
listx added a commit to listx/syscfg that referenced this issue Nov 4, 2017
Without this check, we could not amend commits when the index was empty. The
invocation of `ps' is from sds/overcommit#146 (comment).
listx added a commit to listx/syscfg that referenced this issue Aug 10, 2021
Without this check, we could not amend commits when the index was empty. The
invocation of `ps' is from sds/overcommit#146 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants