{"thread":{"id":"25995","subject":"cherry-pick / pre-commit hook?","startedAt":"2010-12-08T17:10:33Z","lastAt":"2010-12-29T01:00:48Z","messageCount":11,"participants":["Dave Abrahams","Jonathan Nieder","Junio C Hamano","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"157611","messageId":"m2wrnktcl2.wl%dave@boostpro.com","threadId":"25995","inReplyTo":null,"subject":"cherry-pick / pre-commit hook?","fromName":"Dave Abrahams","fromEmail":"dave@boostpro.com","sentAt":"2010-12-08T17:10:33Z","receivedAt":"2010-12-08T17:10:33Z","isPatch":false,"sender":{"key":"dave@boostpro.com","avatar":"https://gravatar.com/avatar/df0921f05114687777894565de21c052fb137ba7c303a399528b43d08833f065?d=mp&s=160"},"body":"\nIs there a good reason that git cherry-pick (without --no-commit)\ndoesn't run my pre-commit hook?  Is there a hook that cherry-pick\n/will/ run instead?\n\nThanks!\n\n-- \nDave Abrahams\nBoostPro Computing\nhttp://www.boostpro.com\n"},{"id":"157614","messageId":"20101208175324.GB5687@burratino","threadId":"25995","inReplyTo":"m2wrnktcl2.wl%dave@boostpro.com","subject":"Re: cherry-pick / pre-commit hook?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-08T17:53:24Z","receivedAt":"2010-12-08T17:53:24Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Dave Abrahams wrote:\n\n> Is there a good reason that git cherry-pick (without --no-commit)\n> doesn't run my pre-commit hook?\n\nInteresting question.\n\n $ git grep -F -e '\"cherry-pick\"'\n [...]\n git.c:          { \"cherry-pick\", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },\n $ git grep -F -e cmd_cherry_pick\n builtin.h:extern int cmd_cherry_pick(int argc, const char **argv, const char *prefix);\n builtin/revert.c:int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n git.c:          { \"cherry-pick\", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },\n\ncherry-pick is implemented in builtin/revert.c.  How does it invoke\nthe \"git commit\" machinery?  Explicitly, as luck would have it.\n\n $ git grep --show-function -F -h -C5 -e '\"commit\"' -- builtin/revert.c\n static int run_git_commit(const char *defmsg)\n {\n         /* 6 is max possible length of our args array including NULL */\n         const char *args[6];\n         int i = 0;\n \n         args[i++] = \"commit\";\n         args[i++] = \"-n\";\n         if (signoff)\n                 args[i++] = \"-s\";\n [...]\n\nSo cherry-pick deliberately uses -n (= --no-verify) when it calls \"git commit\".\nWhy, though?\n\n $ git log --oneline --follow -S'\"-n\"' -- builtin/revert.c\n cfd9c27 Allow cherry-pick (and revert) to add signoff line\n f810379 Make builtin-revert.c use parse_options.\n 9509af6 Make git-revert & git-cherry-pick a builtin\n\nThe '-n' was copied from the old git-revert.sh script when cherry-pick\nwas made builtin.  Not to let the trail grow cold:\n\n $ git log --oneline --follow -S-n -- git-revert.sh\n 9509af6 Make git-revert & git-cherry-pick a builtin\n abd6970 cherry-pick: make -r the default\n 674b280 Add documentation for git-revert and git-cherry-pick.\n 8bf14d6 Document the --(no-)edit switch of git-revert and git-cherry-pick\n b788498 git-revert: make --edit default.\n e2f5f6e Do not require clean tree when reverting and cherry-picking.\n 9fa4db5 Do not verify reverted/cherry-picked/rebased patches.\n [...]\n $ git show -s 9fa4db5\n commit 9fa4db544e2e4d6c931f6adabc5270daec041536\n Author: Junio C Hamano <junkio@cox.net>\n Date:   Mon Aug 29 21:19:04 2005 -0700\n\n     Do not verify reverted/cherry-picked/rebased patches.\n    \n     The original committer may have used validation criteria that is less\n     stricter than yours.  You do not want to lose the changes even if they\n     are done in substandard way from your 'commit -v' verifier's point of\n     view.\n    \n     Signed-off-by: Junio C Hamano <junkio@cox.net>\n $\n\nAt last, an answer.  The main purpose of the pre-commit hook (and\nbuiltin checks that preceded it) is to avoid introducing regressions\nin whitespace style, encoding, and so forth; but it would make\ncherry-picking unnecessarily difficult, without preventing\nregressions, to apply the same standards to existing code.\n\n> Is there a hook that cherry-pick\n> /will/ run instead?\n\n\"git log --grep=pre-commit\" seems to suggest that the commit-msg and\npost-commit hooks will be run.  But first, what are you trying to\naccomplish?  Maybe there is a simpler way, or maybe with that use\ncase in mind we can make changes to support it better.\n\nHope that helps,\nJonathan\n"},{"id":"157627","messageId":"m2oc8wt0xc.wl%dave@boostpro.com","threadId":"25995","inReplyTo":"20101208175324.GB5687@burratino","subject":"Re: cherry-pick / pre-commit hook?","fromName":"Dave Abrahams","fromEmail":"dave@boostpro.com","sentAt":"2010-12-08T21:22:23Z","receivedAt":"2010-12-08T21:22:23Z","isPatch":false,"sender":{"key":"dave@boostpro.com","avatar":"https://gravatar.com/avatar/df0921f05114687777894565de21c052fb137ba7c303a399528b43d08833f065?d=mp&s=160"},"body":"\nHi Jonathan,\n\nAt Wed, 8 Dec 2010 11:53:24 -0600,\nJonathan Nieder wrote:\n> \n>  $ git show -s 9fa4db5\n>  commit 9fa4db544e2e4d6c931f6adabc5270daec041536\n>  Author: Junio C Hamano <junkio@cox.net>\n>  Date:   Mon Aug 29 21:19:04 2005 -0700\n> \n>      Do not verify reverted/cherry-picked/rebased patches.\n>     \n>      The original committer may have used validation criteria that is less\n>      stricter than yours.  You do not want to lose the changes even if they\n>      are done in substandard way from your 'commit -v' verifier's point of\n>      view.\n>     \n>      Signed-off-by: Junio C Hamano <junkio@cox.net>\n>  $\n> \n> At last, an answer.  The main purpose of the pre-commit hook (and\n> builtin checks that preceded it) is to avoid introducing regressions\n> in whitespace style, encoding, and so forth; but it would make\n> cherry-picking unnecessarily difficult, without preventing\n> regressions, to apply the same standards to existing code.\n\nI suspected as much.\n\n> \n> > Is there a hook that cherry-pick\n> > /will/ run instead?\n> \n> \"git log --grep=pre-commit\" seems to suggest that the commit-msg and\n> post-commit hooks will be run.  But first, what are you trying to\n> accomplish?  \n\nYou're going to love this: I had sent a pull request upstream and the\nmaintainer of the project rejected my changes because I didn't follow\nsome formatting convention he didn't tell me about ;-).  So I set up a\ncommit hook that would prevent me from making the same mistake again,\nand cherry-picked the changes one-by-one.  So it was exactly the same\nscenario, except I am the author of the original changes.  \n\nI wonder whether this would have gone better had I used rebase.\n\n> Maybe there is a simpler way, or maybe with that use\n> case in mind we can make changes to support it better.\n\nLooking forward to hearing more.\n\nThanks,\n\n-- \nDave Abrahams\nBoostPro Computing\nhttp://www.boostpro.com\n"},{"id":"157633","messageId":"20101208220514.GA8865@burratino","threadId":"25995","inReplyTo":"m2oc8wt0xc.wl%dave@boostpro.com","subject":"Re: cherry-pick / pre-commit hook?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-08T22:05:14Z","receivedAt":"2010-12-08T22:05:14Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Dave Abrahams wrote:\n\n> You're going to love this: I had sent a pull request upstream and the\n> maintainer of the project rejected my changes because I didn't follow\n> some formatting convention he didn't tell me about ;-).  So I set up a\n> commit hook that would prevent me from making the same mistake again,\n> and cherry-picked the changes one-by-one.\n\nFunny.  Maybe \"cherry-pick --no-commit\" followed by ordinary commit\nwould be appropriate?  That way, when the checks fail, you are in a\nposition to clean them up.\n\nIf the conventions were whitespace related, \"git rebase --whitespace=fix\"\nmight be even more useful.\n\nJust for kicks, here is the cherry-pick --verify for picky\ncherry-pickers.\n\n-- 8< --\nSubject: cherry-pick/revert: learn --verify to run pre-commit and commit-msg hooks\n\nThe main purpose of the pre-commit and commit-msg hooks is to avoid\nintroducing regressions in whitespace style, encoding, and so forth;\nand it would make cherry-picking unnecessarily difficult, without\npreventing regressions, to unconditionally apply the same standards to\nexisting code.  For this reason, in v0.99.6~51 (2005-08-29), git\nlearned to skip the usual hooks when cherry-picking or reverting an\nexisting commit.\n\nBut sometimes the checks are wanted anyway.  For example, with this\npatch applied, you can safely fetch some new contributor's code:\n\n\t$ git cherry-pick -s --verify HEAD..FETCH_HEAD\n\nwhile allowing the pre-commit and commit-msg hooks to run their usual\nchecks so the result can error out if the patches are not clean.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nUntested.  Please feel free to add some documentation and tests and\nsubmit it for real if this looks like a good idea. :)\n\n builtin/revert.c |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex bb6e9e8..511b2ea 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -36,7 +36,7 @@ static const char * const cherry_pick_usage[] = {\n \tNULL\n };\n \n-static int edit, no_replay, no_commit, mainline, signoff, allow_ff;\n+static int edit, no_replay, no_commit, verify, mainline, signoff, allow_ff;\n static enum { REVERT, CHERRY_PICK } action;\n static struct commit *commit;\n static int commit_argc;\n@@ -67,6 +67,7 @@ static void parse_args(int argc, const char **argv)\n \t\tOPT_INTEGER('m', \"mainline\", &mainline, \"parent number\"),\n \t\tOPT_RERERE_AUTOUPDATE(&allow_rerere_auto),\n \t\tOPT_STRING(0, \"strategy\", &strategy, \"strategy\", \"merge strategy\"),\n+\t\tOPT_BOOLEAN(0, \"verify\", &verify, \"let hooks intervene before commiting\"),\n \t\tOPT_END(),\n \t\tOPT_END(),\n \t\tOPT_END(),\n@@ -375,7 +376,8 @@ static int run_git_commit(const char *defmsg)\n \tint i = 0;\n \n \targs[i++] = \"commit\";\n-\targs[i++] = \"-n\";\n+\tif (!verify)\n+\t\targs[i++] = \"-n\";\n \tif (signoff)\n \t\targs[i++] = \"-s\";\n \tif (!edit) {\n-- \n1.7.2.4\n"},{"id":"158607","messageId":"m2d3oo9cwr.wl%dave@boostpro.com","threadId":"25995","inReplyTo":"20101208220514.GA8865@burratino","subject":"Re: cherry-pick / pre-commit hook?","fromName":"Dave Abrahams","fromEmail":"dave@boostpro.com","sentAt":"2010-12-27T02:18:12Z","receivedAt":"2010-12-27T02:18:12Z","isPatch":false,"sender":{"key":"dave@boostpro.com","avatar":"https://gravatar.com/avatar/df0921f05114687777894565de21c052fb137ba7c303a399528b43d08833f065?d=mp&s=160"},"body":"At Wed, 8 Dec 2010 16:05:14 -0600,\nJonathan Nieder wrote:\n> \n> The main purpose of the pre-commit and commit-msg hooks is to avoid\n> introducing regressions in whitespace style, encoding, and so forth;\n> and it would make cherry-picking unnecessarily difficult, without\n> preventing regressions, to unconditionally apply the same standards to\n> existing code.  For this reason, in v0.99.6~51 (2005-08-29), git\n> learned to skip the usual hooks when cherry-picking or reverting an\n> existing commit.\n> \n> But sometimes the checks are wanted anyway.  For example, with this\n> patch applied, you can safely fetch some new contributor's code:\n> \n> \t$ git cherry-pick -s --verify HEAD..FETCH_HEAD\n> \n> while allowing the pre-commit and commit-msg hooks to run their usual\n> checks so the result can error out if the patches are not clean.\n> \n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> Untested.  Please feel free to add some documentation and tests and\n> submit it for real if this looks like a good idea. :)\n\n\nWell, thanks, but sadly I can only invest enough time to file this bug\nreport right now: if you're going to have a \"pre-commit hook\" concept,\nbut not run that hook for some kinds of commits, then that fact needs\nto be documented.\n\n-- \nDave Abrahams\nBoostPro Computing\nhttp://www.boostpro.com\n"},{"id":"158616","messageId":"20101227093729.GB1201@burratino","threadId":"25995","inReplyTo":"m2d3oo9cwr.wl%dave@boostpro.com","subject":"Re: cherry-pick / pre-commit hook?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-27T09:37:29Z","receivedAt":"2010-12-27T09:37:29Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Dave Abrahams wrote:\n\n> if you're going to have a \"pre-commit hook\" concept,\n> but not run that hook for some kinds of commits, then that fact needs\n> to be documented.\n\nTrue, and thanks for a reminder.  Suggested wording?\n\nThe current githooks(5) says\n\n pre-commit\n\tThis hook is invoked by git commit, and can be bypassed with\n\t--no-verify option.\n\nand leaves the question of whether it is invoked by git cherry-pick\nunanswered.\n"},{"id":"158621","messageId":"7vipyfx79g.fsf@alter.siamese.dyndns.org","threadId":"25995","inReplyTo":"20101227093729.GB1201@burratino","subject":"Re: cherry-pick / pre-commit hook?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-27T20:58:35Z","receivedAt":"2010-12-27T20:58:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Dave Abrahams wrote:\n>\n>> if you're going to have a \"pre-commit hook\" concept,\n>> but not run that hook for some kinds of commits, then that fact needs\n>> to be documented.\n>\n> True, and thanks for a reminder.  Suggested wording?\n>\n> The current githooks(5) says\n>\n>  pre-commit\n> \tThis hook is invoked by git commit, and can be bypassed with\n> \t--no-verify option.\n>\n> and leaves the question of whether it is invoked by git cherry-pick\n> unanswered.\n\nHuh?  Isn't it very clear that \"git commit\" calls it and \"git status\" or\nanything that is not \"git commit\" doesn't?\n"},{"id":"158623","messageId":"AANLkTimhMa+zGp+MNi9fjwNsAaMWz3VG=KERoDo7c0zA@mail.gmail.com","threadId":"25995","inReplyTo":"7vipyfx79g.fsf@alter.siamese.dyndns.org","subject":"Re: cherry-pick / pre-commit hook?","fromName":"Dave Abrahams","fromEmail":"dave@boostpro.com","sentAt":"2010-12-27T21:33:27Z","receivedAt":"2010-12-27T21:33:27Z","isPatch":false,"sender":{"key":"dave@boostpro.com","avatar":"https://gravatar.com/avatar/df0921f05114687777894565de21c052fb137ba7c303a399528b43d08833f065?d=mp&s=160"},"body":"On Mon, Dec 27, 2010 at 11:58 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n>\n>> Dave Abrahams wrote:\n>>\n>>> if you're going to have a \"pre-commit hook\" concept,\n>>> but not run that hook for some kinds of commits, then that fact needs\n>>> to be documented.\n>>\n>> True, and thanks for a reminder.  Suggested wording?\n>>\n>> The current githooks(5) says\n>>\n>>  pre-commit\n>>       This hook is invoked by git commit, and can be bypassed with\n>>       --no-verify option.\n>>\n>> and leaves the question of whether it is invoked by git cherry-pick\n>> unanswered.\n>\n> Huh?  Isn't it very clear that \"git commit\" calls it and \"git status\" or\n> anything that is not \"git commit\" doesn't?\n\nNot to me.  And is it really true?  What about rebase?  What about merge?\n\n-- \nDave Abrahams\nBoostPro Computing\nhttp://www.boostpro.com\n"},{"id":"158683","messageId":"7vy679wynd.fsf@alter.siamese.dyndns.org","threadId":"25995","inReplyTo":"20101227093729.GB1201@burratino","subject":"Re: cherry-pick / pre-commit hook?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-28T18:16:54Z","receivedAt":"2010-12-28T18:16:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Dave Abrahams wrote:\n>\n>> if you're going to have a \"pre-commit hook\" concept,\n>> but not run that hook for some kinds of commits, then that fact needs\n>> to be documented.\n>\n> True, and thanks for a reminder.  Suggested wording?\n>\n> The current githooks(5) says\n>\n>  pre-commit\n> \tThis hook is invoked by git commit, and can be bypassed with\n> \t--no-verify option.\n>\n> and leaves the question of whether it is invoked by git cherry-pick\n> unanswered.\n\nYeah, but do we want to answer it in this section, or in git-cherry-pick's\nmanual page?\n\nAfter all \"pre-commit\" was _not_ about \"before doing anything that creates\na commit\", but was about \"before git-commit creates a commit\".  Changing\nthat definition is fine as long as we have a good way to explain it to the\nusers and more importantly a simple rule that the users can understand,\nand that rule _could_ be \"anything that creates a new commit\".\n\nIn reality \"anything that creates a new commit\" cannot be that simple\nrule, however.  For example, \"git pull\" does attempt to create a new merge\ncommit, but failing it because the work done by the other person you are\npulling from does not conform _your_ standard defined by your hook is not\na sane default.\n\nI think the basic direction could be (I haven't thought things through,\njust a strawman):\n\n - Allow --verify/--no-verify to all commands that possibly create a new\n   commit, and run pre-commit hook where an updated index is about to be\n   made into a commit (for some commands this may not be very easy);\n\n - The guideline of picking the default would probably look like this:\n\n   (1) for existing commands, keep the current behaviour;\n\n   (2) for a new command, --verify should be the default if the command is\n       primarily about letting the user do what s/he would/could/should\n       have done as \"git commit\" in the first place (e.g. cherry-picking\n       one's own commit from a separate branch or rebasing one's own\n       unpublished branch on top of updated upstream), and --no-verify\n       otherwise (i.e. taking other's work and using it in a context\n       different from the original).\n"},{"id":"158689","messageId":"m3vd2djzhp.fsf@localhost.localdomain","threadId":"25995","inReplyTo":"7vy679wynd.fsf@alter.siamese.dyndns.org","subject":"Re: cherry-pick / pre-commit hook?","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-28T22:38:20Z","receivedAt":"2010-12-28T22:38:20Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I think the basic direction could be (I haven't thought things through,\n> just a strawman):\n> \n>  - Allow --verify/--no-verify to all commands that possibly create a new\n>    commit, and run pre-commit hook where an updated index is about to be\n>    made into a commit (for some commands this may not be very easy);\n> \n>  - The guideline of picking the default would probably look like this:\n> \n>    (1) for existing commands, keep the current behaviour;\n> \n>    (2) for a new command, --verify should be the default if the command is\n>        primarily about letting the user do what s/he would/could/should\n>        have done as \"git commit\" in the first place (e.g. cherry-picking\n>        one's own commit from a separate branch or rebasing one's own\n>        unpublished branch on top of updated upstream), and --no-verify\n>        otherwise (i.e. taking other's work and using it in a context\n>        different from the original).\n\nDoes it mean that for now (and perhaps also for later) it means that\n\"git commit\" by default runs pre-commit hook, unless one use\n--no-verify, and that all comands that create a new commit (rebase,\ncherry-pick, revert, merge/pull) can request for pre-commit hook to be\nrun (if they create commit) with --verify?\n\nI think it is a very good idea, though I don't know how difficult it\nwould be to make all commands that can create commit accept --verify..\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"158699","messageId":"m2bp45fl4v.wl%dave@boostpro.com","threadId":"25995","inReplyTo":"m3vd2djzhp.fsf@localhost.localdomain","subject":"Re: cherry-pick / pre-commit hook?","fromName":"Dave Abrahams","fromEmail":"dave@boostpro.com","sentAt":"2010-12-29T01:00:48Z","receivedAt":"2010-12-29T01:00:48Z","isPatch":false,"sender":{"key":"dave@boostpro.com","avatar":"https://gravatar.com/avatar/df0921f05114687777894565de21c052fb137ba7c303a399528b43d08833f065?d=mp&s=160"},"body":"At Tue, 28 Dec 2010 14:38:20 -0800 (PST),\nJakub Narebski wrote:\n> \n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > I think the basic direction could be (I haven't thought things through,\n> > just a strawman):\n> > \n> >  - Allow --verify/--no-verify to all commands that possibly create a new\n> >    commit, and run pre-commit hook where an updated index is about to be\n> >    made into a commit (for some commands this may not be very easy);\n> > \n> >  - The guideline of picking the default would probably look like this:\n> > \n> >    (1) for existing commands, keep the current behaviour;\n> > \n> >    (2) for a new command, --verify should be the default if the command is\n> >        primarily about letting the user do what s/he would/could/should\n> >        have done as \"git commit\" in the first place (e.g. cherry-picking\n> >        one's own commit from a separate branch or rebasing one's own\n> >        unpublished branch on top of updated upstream), and --no-verify\n> >        otherwise (i.e. taking other's work and using it in a context\n> >        different from the original).\n> \n> Does it mean that for now (and perhaps also for later) it means that\n> \"git commit\" by default runs pre-commit hook, unless one use\n> --no-verify, and that all comands that create a new commit (rebase,\n> cherry-pick, revert, merge/pull) can request for pre-commit hook to be\n> run (if they create commit) with --verify?\n> \n> I think it is a very good idea\n\n+1!\n\n-- \nDave Abrahams\nBoostPro Computing\nhttp://www.boostpro.com\n"}]}