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

Hi Orgad,

On Tue, 26 Jul 2016, Orgad Shaneh wrote:

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

quoted

On Tue, 26 Jul 2016, Orgad Shaneh wrote:

quoted

On Tue, Jul 26, 2016 at 4:02 PM, Johannes Schindelin
[off-list ref] wrote:

quoted

On Tue, 26 Jul 2016, Orgad Shaneh wrote:

quoted

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

quoted

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

Done.

Good.

quoted

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

pre-commit seems to have been rejected, though I must admit I don't
fully understand why.  post-commit might make sense, but I wouldn't
include it in the same patch.  These are different issues.

I did not mean to include that change in the patch. I meant to make a case
for, or against, it, in the commit message.

The commit message is an excellent place to demonstrate the usefulness of
the change as well as background information that lets the reader
understand how thorough the patch is. A commit message can increase the
trust in the rigidity of the patch.

That is why I suggested to research further by providing two links with
further information.

Now that I know that pre-commit has been rejected, of course, I want to
know why. Because the same argument might well preclude commit-msg support
from being added to `git merge` (and it might well lead to the
prepare-commit-msg hook to be deprecated in `git merge`).

This is all very useful information. We are slowly getting to the full
picture needed to assess whether to include this patch or not.

quoted

quoted

quoted

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

Because commit-msg doesn't know anything about the commit.

That is incorrect. When that hook is called, `git rev-parse HEAD` refers
to the parent commit, and `GIT_INDEX_FILE` refers to the index that will be
written as a tree object. That is pretty much all the information needed
to know about the future commit.

quoted

Otherwise, why wouldn't you use the post-merge hook to add the Change-Id:
in your use case, too...

quoted

quoted

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

Gerrit is a server, the user installs a commit-msg hook provided by
Gerrit on the local machine.  This hook currently works only for simple
commits and not for (trivial) merge commits.  That's where this patch
comes to the rescue.

Sorry, you really need to explain this better in the commit message.
Gerrit exists for quite some time, so I would assume that other developers
would have had serious problems in similar situations. How do they address
those challenges?

quoted

quoted

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.

Gerrit can't do anything on my machine. It's a web/ssh server. I have my
own post-merge hook that runs commit --amend

Okay, so why exactly do you need the commit-msg?

All the concerns I raised, all the confusion *need* to be addressed by a
commit message that makes a good case for the change. That is what commit
messages are for. So far, the commit message of this patch is not up to
the task.

Ciao,
Johannes