Hi Orgad, On Tue, 26 Jul 2016, Orgad Shaneh wrote: > On Tue, Jul 26, 2016 at 4:02 PM, Johannes Schindelin > <Johannes.Schindelin@xxxxxx> wrote: > > > > On Tue, 26 Jul 2016, Orgad Shaneh wrote: > > > >> commit-msg is needed to either validate the commit message or edit it. > >> Gerrit for instance uses this hook to append its Change-Id footer. > >> > >> This is relevant to merge commit just like any other commit. > > > > Hmm. This is not very convincing to me, as > > > > - if you call commit-msg in `git merge` now, why not `prepare-commit-msg`? > > prepare-commit-msg is already called, a few lines above this addition. Oh. That would have made a heck of a convincing argument in the commit message. Pity it was not mentioned. (Yes, please read that as a strong suggestion to fix that in the next patch iteration.) FWIW I dug out the original submission: http://thread.gmane.org/gmane.comp.version-control.git/151297/focus=151435 It seems that there was no discussion about the commit-msg. Which makes me wonder why nobody thought of this. Now, you could make a case that you want to run the commit-msg hook in the same spirit as prepare-commit-msg (and mention that apparently nobody thought of that when the patch was accepted into builtin/merge.c). But then I wonder what your argument would be for *not* running the pre-commit and the post-commit hooks in `git merge` as well? Seems like a big inconsistency to me, one that would not be helped by a piecemeal patch that does only half the job of resolving the inconsistency. There was actually a question why the post-commit hook was not run in `git merge`: http://thread.gmane.org/gmane.comp.version-control.git/168716/focus=168773 (but it seems nobody cared all that much). > > - a merge is a different beast from a simple commit. That is why we > > have two different commands for them. A hook to edit the merge message > > may need to know the *second* parent commit, too, for example to > > generate a diffstat, or to add information about changes in an "evil > > commit". > > That is correct for a post-merge hook. Why should *message validation* > differ between simple and merge commit? You yourself do not use the hook for validation. You use it to *edit* the message. My examples do the very same thing. Why should they wait for a *post-merge* hook to amend the message? Otherwise, why wouldn't you use the post-merge hook to add the Change-Id: in your use case, too... > > - if Gerrit is the intended user, would it not make more sense to > > introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`), as you > > have to teach Gerrit a new trick anyway? > > Why is that new? Every commit in gerrit has a Change-Id footer, which is > generated by commit-msg hook. So it already works for Gerrit? Why is this patch needed, then? This is confusing. > What I currently do for merges that succeed without conflicts is > unconditional commit --amend --no-edit just to run the hook. So you do that manually? Or you taught Gerrit to do that? Please clarify. > > - if Gerrit is the intended user, why does it not simply edit the merge > > message itself? After all, it executes it, and probably crafts a merge > > message mentioning that this is an automatic merge, anyway, so why not > > add the Change-Id *then*? > > Most Gerrit setups require Change-Id in the commit message that the user > pushes. It is possible to disable this setting, and then you don't need the > Change-Id at all, but then you can't push a new patch set for the change > (unless you copy the Change-Id from gerrit to your commit message). > That's a real pain, I'm not aware of any public gerrit server that > disables this :) Forgive me, I never used Gerrit, and neither do I intend to do so. I would appreciate a self-contained commit message that explains just enough about Gerrit to understand what the patch tries to solve, and in a manner such that even developers who decided to ignore Gerrit understand. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html