cherry-pick / pre-commit hook?

Is there a good reason that git cherry-pick (without --no-commit)
doesn't run my pre-commit hook?  Is there a hook that cherry-pick
/will/ run instead?

Thanks!

-- 
Dave Abrahams
BoostPro Computing
http://www.boostpro.com

Dave Abrahams wrote:

Is there a good reason that git cherry-pick (without --no-commit)
doesn't run my pre-commit hook?

Interesting question.

 $ git grep -F -e '"cherry-pick"'
 [...]
 git.c:          { "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
 $ git grep -F -e cmd_cherry_pick
 builtin.h:extern int cmd_cherry_pick(int argc, const char **argv, const char *prefix);
 builtin/revert.c:int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 git.c:          { "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },

cherry-pick is implemented in builtin/revert.c.  How does it invoke
the "git commit" machinery?  Explicitly, as luck would have it.

 $ git grep --show-function -F -h -C5 -e '"commit"' -- builtin/revert.c
 static int run_git_commit(const char *defmsg)
 {
         /* 6 is max possible length of our args array including NULL */
         const char *args[6];
         int i = 0;
 
         args[i++] = "commit";
         args[i++] = "-n";
         if (signoff)
                 args[i++] = "-s";
 [...]

So cherry-pick deliberately uses -n (= --no-verify) when it calls "git commit".
Why, though?

 $ git log --oneline --follow -S'"-n"' -- builtin/revert.c
 cfd9c27 Allow cherry-pick (and revert) to add signoff line
 f810379 Make builtin-revert.c use parse_options.
 9509af6 Make git-revert & git-cherry-pick a builtin

The '-n' was copied from the old git-revert.sh script when cherry-pick
was made builtin.  Not to let the trail grow cold:

 $ git log --oneline --follow -S-n -- git-revert.sh
 9509af6 Make git-revert & git-cherry-pick a builtin
 abd6970 cherry-pick: make -r the default
 674b280 Add documentation for git-revert and git-cherry-pick.
 8bf14d6 Document the --(no-)edit switch of git-revert and git-cherry-pick
 b788498 git-revert: make --edit default.
 e2f5f6e Do not require clean tree when reverting and cherry-picking.
 9fa4db5 Do not verify reverted/cherry-picked/rebased patches.
 [...]
 $ git show -s 9fa4db5
 commit 9fa4db544e2e4d6c931f6adabc5270daec041536
 Author: Junio C Hamano [off-list ref]
 Date:   Mon Aug 29 21:19:04 2005 -0700

     Do not verify reverted/cherry-picked/rebased patches.
    
     The original committer may have used validation criteria that is less
     stricter than yours.  You do not want to lose the changes even if they
     are done in substandard way from your 'commit -v' verifier's point of
     view.
    
     Signed-off-by: Junio C Hamano [off-list ref]
 $

At last, an answer.  The main purpose of the pre-commit hook (and
builtin checks that preceded it) is to avoid introducing regressions
in whitespace style, encoding, and so forth; but it would make
cherry-picking unnecessarily difficult, without preventing
regressions, to apply the same standards to existing code.

Is there a hook that cherry-pick
/will/ run instead?

"git log --grep=pre-commit" seems to suggest that the commit-msg and
post-commit hooks will be run.  But first, what are you trying to
accomplish?  Maybe there is a simpler way, or maybe with that use
case in mind we can make changes to support it better.

Hope that helps,
Jonathan

Hi Jonathan,

At Wed, 8 Dec 2010 11:53:24 -0600,
Jonathan Nieder wrote:

 $ git show -s 9fa4db5
 commit 9fa4db544e2e4d6c931f6adabc5270daec041536
 Author: Junio C Hamano [off-list ref]
 Date:   Mon Aug 29 21:19:04 2005 -0700

     Do not verify reverted/cherry-picked/rebased patches.
    
     The original committer may have used validation criteria that is less
     stricter than yours.  You do not want to lose the changes even if they
     are done in substandard way from your 'commit -v' verifier's point of
     view.
    
     Signed-off-by: Junio C Hamano [off-list ref]
 $

At last, an answer.  The main purpose of the pre-commit hook (and
builtin checks that preceded it) is to avoid introducing regressions
in whitespace style, encoding, and so forth; but it would make
cherry-picking unnecessarily difficult, without preventing
regressions, to apply the same standards to existing code.

I suspected as much.

quoted

Is there a hook that cherry-pick
/will/ run instead?

"git log --grep=pre-commit" seems to suggest that the commit-msg and
post-commit hooks will be run.  But first, what are you trying to
accomplish?  

You're going to love this: I had sent a pull request upstream and the
maintainer of the project rejected my changes because I didn't follow
some formatting convention he didn't tell me about ;-).  So I set up a
commit hook that would prevent me from making the same mistake again,
and cherry-picked the changes one-by-one.  So it was exactly the same
scenario, except I am the author of the original changes.  

I wonder whether this would have gone better had I used rebase.

Maybe there is a simpler way, or maybe with that use
case in mind we can make changes to support it better.

Looking forward to hearing more.

Thanks,

-- 
Dave Abrahams
BoostPro Computing
http://www.boostpro.com

Dave Abrahams wrote:

You're going to love this: I had sent a pull request upstream and the
maintainer of the project rejected my changes because I didn't follow
some formatting convention he didn't tell me about ;-).  So I set up a
commit hook that would prevent me from making the same mistake again,
and cherry-picked the changes one-by-one.

Funny.  Maybe "cherry-pick --no-commit" followed by ordinary commit
would be appropriate?  That way, when the checks fail, you are in a
position to clean them up.

If the conventions were whitespace related, "git rebase --whitespace=fix"
might be even more useful.

Just for kicks, here is the cherry-pick --verify for picky
cherry-pickers.

-- 8< --
Subject: cherry-pick/revert: learn --verify to run pre-commit and commit-msg hooks

The main purpose of the pre-commit and commit-msg hooks is to avoid
introducing regressions in whitespace style, encoding, and so forth;
and it would make cherry-picking unnecessarily difficult, without
preventing regressions, to unconditionally apply the same standards to
existing code.  For this reason, in v0.99.6~51 (2005-08-29), git
learned to skip the usual hooks when cherry-picking or reverting an
existing commit.

But sometimes the checks are wanted anyway.  For example, with this
patch applied, you can safely fetch some new contributor's code:

	$ git cherry-pick -s --verify HEAD..FETCH_HEAD

while allowing the pre-commit and commit-msg hooks to run their usual
checks so the result can error out if the patches are not clean.

Signed-off-by: Jonathan Nieder <redacted>
---
Untested.  Please feel free to add some documentation and tests and
submit it for real if this looks like a good idea. :)

 builtin/revert.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index bb6e9e8..511b2ea 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -36,7 +36,7 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static int edit, no_replay, no_commit, mainline, signoff, allow_ff;
+static int edit, no_replay, no_commit, verify, mainline, signoff, allow_ff;
 static enum { REVERT, CHERRY_PICK } action;
 static struct commit *commit;
 static int commit_argc;

@@ -67,6 +67,7 @@ static void parse_args(int argc, const char **argv)
 		OPT_INTEGER('m', "mainline", &mainline, "parent number"),
 		OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
 		OPT_STRING(0, "strategy", &strategy, "strategy", "merge strategy"),
+		OPT_BOOLEAN(0, "verify", &verify, "let hooks intervene before commiting"),
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),

@@ -375,7 +376,8 @@ static int run_git_commit(const char *defmsg)
 	int i = 0;
 
 	args[i++] = "commit";
-	args[i++] = "-n";
+	if (!verify)
+		args[i++] = "-n";
 	if (signoff)
 		args[i++] = "-s";
 	if (!edit) {

-- 
1.7.2.4

At Wed, 8 Dec 2010 16:05:14 -0600,
Jonathan Nieder wrote:

The main purpose of the pre-commit and commit-msg hooks is to avoid
introducing regressions in whitespace style, encoding, and so forth;
and it would make cherry-picking unnecessarily difficult, without
preventing regressions, to unconditionally apply the same standards to
existing code.  For this reason, in v0.99.6~51 (2005-08-29), git
learned to skip the usual hooks when cherry-picking or reverting an
existing commit.

But sometimes the checks are wanted anyway.  For example, with this
patch applied, you can safely fetch some new contributor's code:

	$ git cherry-pick -s --verify HEAD..FETCH_HEAD

while allowing the pre-commit and commit-msg hooks to run their usual
checks so the result can error out if the patches are not clean.

Signed-off-by: Jonathan Nieder <redacted>
---
Untested.  Please feel free to add some documentation and tests and
submit it for real if this looks like a good idea. :)

Well, thanks, but sadly I can only invest enough time to file this bug
report right now: if you're going to have a "pre-commit hook" concept,
but not run that hook for some kinds of commits, then that fact needs
to be documented.

-- 
Dave Abrahams
BoostPro Computing
http://www.boostpro.com

Dave Abrahams wrote:

if you're going to have a "pre-commit hook" concept,
but not run that hook for some kinds of commits, then that fact needs
to be documented.

True, and thanks for a reminder.  Suggested wording?

The current githooks(5) says

 pre-commit
	This hook is invoked by git commit, and can be bypassed with
	--no-verify option.

and leaves the question of whether it is invoked by git cherry-pick
unanswered.