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

From: Orgad Shaneh <orgads@gmail.com>
Date: Jul 26, 2016, 13:50 UTC
Message-ID: <CAGHpTBLr9q8h-+hVUzsTS1aS1TyZjz9gYM_T_ZBdY=o26JGaHw@mail.gmail.com>
In-Reply-To: <alpine.DEB.2.20.1607261456480.14111@virtualbox>

Hi,

On Tue, Jul 26, 2016 at 4:02 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 17 quoted lines

> Hi Orgad,
>
> On Tue, 26 Jul 2016, Orgad Shaneh wrote:
>
>> From: Orgad Shaneh <orgads@gmail.com>
>
> Again, this is unnecessary if you already send the mail from the same
> address.
>
>> 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.

Show 5 quoted lines

>
> - 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?

> - 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. What I currently do for merges that succeed without conflicts is unconditional commit --amend --no-edit just to run the hook. This doesn't make sense.

> - 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 :)

> Ciao,
> Dscho

- Orgad