Re: [PATCH] merge: Run commit-msg hook

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Orgad,

On Tue, 26 Jul 2016, Orgad Shaneh wrote:

> On Tue, Jul 26, 2016 at 4:02 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> 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