{"thread":{"id":"42932","subject":"[PATCH] merge: Run commit-msg hook","startedAt":"2016-07-26T15:33:16Z","lastAt":"2016-07-26T15:33:16Z","messageCount":1,"participants":["Orgad Shaneh"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"292215","messageId":"CAGHpTBKoLeAe9paTOLRuQM3JSv5GAq60LP64+kLS2JYuJEOcoQ@mail.gmail.com","threadId":"42932","inReplyTo":null,"subject":"[PATCH] merge: Run commit-msg hook","fromName":"Orgad Shaneh","fromEmail":"orgads@gmail.com","sentAt":"2016-07-26T15:32:54Z","receivedAt":"2016-07-26T15:33:16Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"Hi again,\n\nOn Tue, Jul 26, 2016 at 5:54 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>\n> Hi Orgad,\n>\n> On Tue, 26 Jul 2016, Orgad Shaneh wrote:\n>\n> > On Tue, Jul 26, 2016 at 4:02 PM, Johannes Schindelin\n> > <Johannes.Schindelin@gmx.de> wrote:\n> > >\n> > > On Tue, 26 Jul 2016, Orgad Shaneh wrote:\n> > >\n> > >> commit-msg is needed to either validate the commit message or edit it.\n> > >> Gerrit for instance uses this hook to append its Change-Id footer.\n> > >>\n> > >> This is relevant to merge commit just like any other commit.\n> > >\n> > > Hmm. This is not very convincing to me, as\n> > >\n> > > - if you call commit-msg in `git merge` now, why not `prepare-commit-msg`?\n> >\n> > prepare-commit-msg is already called, a few lines above this addition.\n>\n> Oh. That would have made a heck of a convincing argument in the commit\n> message. Pity it was not mentioned. (Yes, please read that as a strong\n> suggestion to fix that in the next patch iteration.)\n\n\nDone.\n\n>\n>\n> FWIW I dug out the original submission:\n> http://thread.gmane.org/gmane.comp.version-control.git/151297/focus=151435\n>\n> It seems that there was no discussion about the commit-msg. Which makes me\n> wonder why nobody thought of this.\n>\n> Now, you could make a case that you want to run the commit-msg hook in the\n> same spirit as prepare-commit-msg (and mention that apparently nobody\n> thought of that when the patch was accepted into builtin/merge.c).\n>\n> But then I wonder what your argument would be for *not* running the\n> pre-commit and the post-commit hooks in `git merge` as well?\n>\n> Seems like a big inconsistency to me, one that would not be helped by a\n> piecemeal patch that does only half the job of resolving the inconsistency.\n>\n> There was actually a question why the post-commit hook was not run in `git\n> merge`:\n> http://thread.gmane.org/gmane.comp.version-control.git/168716/focus=168773\n> (but it seems nobody cared all that much).\n\n\npre-commit seems to have been rejected, though I must admit I don't\nfully understand why.\npost-commit might make sense, but I wouldn't include it in the same\npatch. These are\ndifferent issues.\n\n>\n> > > - a merge is a different beast from a simple commit. That is why we\n> > > have two different commands for them. A hook to edit the merge message\n> > > may need to know the *second* parent commit, too, for example to\n> > > generate a diffstat, or to add information about changes in an \"evil\n> > > commit\".\n> >\n> > That is correct for a post-merge hook. Why should *message validation*\n> > differ between simple and merge commit?\n>\n> You yourself do not use the hook for validation. You use it to *edit* the\n> message. My examples do the very same thing. Why should they wait for a\n> *post-merge* hook to amend the message?\n\n\nBecause commit-msg doesn't know anything about the commit. It only refers\nto the message. The commit might even not exist when it is called. If you\nneed data from the commit, post-merge is the right place.\n\n>\n> Otherwise, why wouldn't you use the post-merge hook to add the Change-Id:\n> in your use case, too...\n>\n> > > - if Gerrit is the intended user, would it not make more sense to\n> > >   introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`), as you\n> > >   have to teach Gerrit a new trick anyway?\n> >\n> > Why is that new? Every commit in gerrit has a Change-Id footer, which is\n> > generated by commit-msg hook.\n>\n> So it already works for Gerrit? Why is this patch needed, then? This is\n> confusing.\n\n\nGerrit is a server, the user installs a commit-msg hook provided by\nGerrit on the local machine.\nThis hook currently works only for simple commits and not for\n(trivial) merge commits.\nThat's where this patch comes to the rescue.\n\n>\n> > What I currently do for merges that succeed without conflicts is\n> > unconditional commit --amend --no-edit just to run the hook.\n>\n> So you do that manually? Or you taught Gerrit to do that? Please clarify.\n\n\nGerrit can't do anything on my machine. It's a web/ssh server. I have my own\npost-merge hook that runs commit --amend\n\n> > > - if Gerrit is the intended user, why does it not simply edit the merge\n> > >   message itself? After all, it executes it, and probably crafts a merge\n> > >   message mentioning that this is an automatic merge, anyway, so why not\n> > >   add the Change-Id *then*?\n> >\n> > Most Gerrit setups require Change-Id in the commit message that the user\n> > pushes. It is possible to disable this setting, and then you don't need the\n> > Change-Id at all, but then you can't push a new patch set for the change\n> > (unless you copy the Change-Id from gerrit to your commit message).\n> > That's a real pain, I'm not aware of any public gerrit server that\n> > disables this :)\n>\n> Forgive me, I never used Gerrit, and neither do I intend to do so.\n\nToo bad, great system :)\n\n> I would appreciate a self-contained commit message that explains just\n> enough about Gerrit to understand what the patch tries to solve, and in a\n> manner such that even developers who decided to ignore Gerrit understand.\n\n\nI hope it's better now. Thanks for your comments.\n\n- Orgad\n"}]}