{"thread":{"id":"47829","subject":"Why git-revert doesn't invoke the pre-commit and the commit-msg hooks?","startedAt":"2018-02-13T22:27:40Z","lastAt":"2018-02-21T13:34:12Z","messageCount":5,"participants":["Gustavo Chaves","Phillip Wood","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"339236","messageId":"CAPx58qqv84+i0JbdsVzFqWB=bRDecWHxss8frD4=nWOsFj-NPg@mail.gmail.com","threadId":"47829","inReplyTo":null,"subject":"Why git-revert doesn't invoke the pre-commit and the commit-msg hooks?","fromName":"Gustavo Chaves","fromEmail":"gustavo@gnustavo.com","sentAt":"2018-02-13T22:27:13Z","receivedAt":"2018-02-13T22:27:40Z","isPatch":false,"sender":{"key":"gustavo@gnustavo.com","avatar":"https://gravatar.com/avatar/43ef8e2eb692e324332192522639e3980095945b95d611bde4ac8f0e55666fef?d=mp&s=160"},"body":"Using strace I noticed that git-revert invokes only two hooks:\n- prepare-commit-msg\n- post-commit\n\nBut git-commit invoke these four:\n- pre-commit\n- prepare-commit-msg\n- commit-msg\n- post-commit\n\nSince git-revert produces a commit, why doesn't it invoke the same\nhooks as git-commit?\n\nI couldn't find any discussing about this in the list or elsewhere. So\nI'm asking here.\n\nI ended up researching this when I was implementing a hook to detect\nand deny commits which revert merge-commits, since they are\ntroublesome (https://www.kernel.org/pub/software/scm/git/docs/howto/revert-a-faulty-merge.html).\nI tried to implement it as a commit-msg hook to search for the string\n\"This reverts commit SHA-1\" in the commit message. But git-revert\ndoesn't invoke the commit-msg hook.\n\nSo, for now I implemented my check as a pre-receive hook. But I find\nit useful to have all pre-receive checks implemented also as a\npre-commit or a commit-msg hook so that I can detect problems at\ncommit time instead of only at push time.\n\n-- \nGustavo Chaves\n"},{"id":"339617","messageId":"CAPx58qoS-J+yJ_J4QOOnKyG=EOrT5J=UoCrXfXxEijq4Z2Z_3w@mail.gmail.com","threadId":"47829","inReplyTo":"CAPx58qqv84+i0JbdsVzFqWB=bRDecWHxss8frD4=nWOsFj-NPg@mail.gmail.com","subject":"Re: Why git-revert doesn't invoke the pre-commit and the commit-msg hooks?","fromName":"Gustavo Chaves","fromEmail":"gustavo@gnustavo.com","sentAt":"2018-02-19T14:50:00Z","receivedAt":"2018-02-19T14:50:28Z","isPatch":false,"sender":{"key":"gustavo@gnustavo.com","avatar":"https://gravatar.com/avatar/43ef8e2eb692e324332192522639e3980095945b95d611bde4ac8f0e55666fef?d=mp&s=160"},"body":"I asked this question on StackOverflow and got an answer:\nhttps://stackoverflow.com/q/48852925/114983\n\nThe problem is that git-revert invokes git-commit with the -n flag,\nexplicitly avoiding the pre-commit and the commit-msg hooks.\n\nThis was originally introduced on commit 9fa4db544e2e, by Junio\nHamano, in 2005! The rationale in the commit message was the\nfollowing:\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\nI get it, but since by default you are allowed to edit the commit\nmessage during a git-revert I think there's a case to be made to make\nthe pre-commit and the commit-msg being invoked by default. Also,\ngit-revert introduces new lines in the original commit message, and\nthey could be used to trigger specific checks, such as the one I\nwanted to implement, to deny commits reverting merge-commits.\n\nShouldn't git-revert work exactly as git-commit? Instead of disabling\nhooks by default, it could accept the --no-verify flag just like\ngit-commit to disable the hooks if the user wants it.\n\n-- \nGustavo Chaves\n"},{"id":"339696","messageId":"7ff6079d-5834-3bbd-781b-a2fc0659e7e5@talktalk.net","threadId":"47829","inReplyTo":"CAPx58qoS-J+yJ_J4QOOnKyG=EOrT5J=UoCrXfXxEijq4Z2Z_3w@mail.gmail.com","subject":"Re: Why git-revert doesn't invoke the pre-commit and the commit-msg hooks?","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-02-20T10:28:55Z","receivedAt":"2018-02-20T10:29:02Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Gustavo\n\nOn 19/02/18 14:50, Gustavo Chaves wrote:\n> \n> I asked this question on StackOverflow and got an answer:\n> https://stackoverflow.com/q/48852925/114983\n> \n> The problem is that git-revert invokes git-commit with the -n flag,\n> explicitly avoiding the pre-commit and the commit-msg hooks.\n> \n> This was originally introduced on commit 9fa4db544e2e, by Junio\n> Hamano, in 2005! The rationale in the commit message was the\n> following:\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> I get it, but since by default you are allowed to edit the commit\n> message during a git-revert I think there's a case to be made to make\n> the pre-commit and the commit-msg being invoked by default. Also,\n> git-revert introduces new lines in the original commit message, and\n> they could be used to trigger specific checks, such as the one I\n> wanted to implement, to deny commits reverting merge-commits.\n> \n> Shouldn't git-revert work exactly as git-commit? Instead of disabling\n> hooks by default, it could accept the --no-verify flag just like\n> git-commit to disable the hooks if the user wants it.\n> \n\nI think you're right that cherry-pick and revert should not be passing\n--no-verify when the commit message is being edited (I was surprised to\nsee that they were). I wonder if it's worth getting rid of the\nVERIFY_MSG flag completely and just set --no-verify if EDIT_MSG isn't\nset to avoid these problems in the future. I'm worried though that\nsomeone out there is scripting with a non-interactive editor which may\nbreak if we start verifying the message so maybe the default should be\nchanged and a --no-verify option added to cherry-pick and revert\ninstead. They'd have to change their script but at least it would work\nand everyone else would get the behaviour they probably expect.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"339718","messageId":"xmqqfu5v4k4c.fsf@gitster-ct.c.googlers.com","threadId":"47829","inReplyTo":"7ff6079d-5834-3bbd-781b-a2fc0659e7e5@talktalk.net","subject":"Re: Why git-revert doesn't invoke the pre-commit and the commit-msg hooks?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-20T18:00:03Z","receivedAt":"2018-02-20T18:00:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood@talktalk.net> writes:\n\n> ... I'm worried though that\n> someone out there is scripting with a non-interactive editor which may\n> break if we start verifying the message ...\n\nThis is a very valid concern.  Making sure that 'revert' pays\nattention to the --verify option when given from the command line,\nwithout changing the default or anything else, would satisfy\neverybody's wish, I would think.  It won't bother existing users,\nand in addition will now give an option to people to selectively run\npre-whatever hook when they choose to.\n\nI am not sure if we should be running pre-commit/commit-msg for\nrevert.  The message and the effect of revert is quite distinct from\nthe ordinary commit, and it is very likely that people would want to\ntreat them differently.  It would make more sense (if we were to add\nan option to run any hook we currently do not run to the command) to\nrun pre-revert/revert-msg hooks instead, and then people who happen\nto want to do the same thing in these hooks what they do for\nordinary commits can just call their pre-commit/commit-msg hooks\nfrom there, perhaps.\n"},{"id":"339804","messageId":"CAPx58qp4VssKSKWJ=ng9CwHZUqkgnw6=D5iJLFyhYzwMz-zocA@mail.gmail.com","threadId":"47829","inReplyTo":"xmqqfu5v4k4c.fsf@gitster-ct.c.googlers.com","subject":"Re: Why git-revert doesn't invoke the pre-commit and the commit-msg hooks?","fromName":"Gustavo Chaves","fromEmail":"gustavo@gnustavo.com","sentAt":"2018-02-21T13:33:42Z","receivedAt":"2018-02-21T13:34:12Z","isPatch":false,"sender":{"key":"gustavo@gnustavo.com","avatar":"https://gravatar.com/avatar/43ef8e2eb692e324332192522639e3980095945b95d611bde4ac8f0e55666fef?d=mp&s=160"},"body":"2018-02-20 15:00 GMT-03:00 Junio C Hamano <gitster@pobox.com>:\n> It would make more sense (if we were to add\n> an option to run any hook we currently do not run to the command) to\n> run pre-revert/revert-msg hooks instead, and then people who happen\n> to want to do the same thing in these hooks what they do for\n> ordinary commits can just call their pre-commit/commit-msg hooks\n> from there, perhaps.\n\nI like this idea very much as it doesn't break a long standing behaviour and\nsimply introduces a new feature.\n\n-- \nGustavo.\n"}]}