Re: [PATCH 1/3] git-merge: Honor pre-merge hook

On 09/06/2012 10:16 AM, Michael J Gruber wrote:

Michael Haggerty venit, vidit, dixit 05.09.2012 17:30:

quoted

On 09/05/2012 03:39 PM, Michael J Gruber wrote:

quoted

git-merge does not honor the pre-commit hook when doing automatic merge
commits, and for compatibility reasons this is going to stay.

Introduce a pre-merge hook which is called for an automatic merge commit
just like pre-commit is called for a non-automatic merge commit (or any
other commit).

What exactly is an "automatic merge commit"?  Is it any merge that
doesn't have a conflict?  A merge that doesn't invoke the editor?  A
merge done as part of another operation (e.g., pull)?  I don't see the
term mentioned in the git-merge or githooks man pages.

I think it would be good if you would define this term in the
documentation files that your patch touched, and perhaps in the githooks
section about "pre-commit" as well.

"git merge" can go three ways:

F: fast forward: no commit is created, only a ref is changed
A: automatic: true merge (non-ff) without conflicts (i.e. chosen
strategy can perform the merge); a new commit is created
C: merge with conflicts: no commit is created but the index is prepared
(partially) for a merge commit

In case F, no commit hook is run (talking only about pre-commit/pre-merge).

In case A, no commit is run so far but my patch proposes pre-merge to be
run.

In case C, pre-commit (!) is run so far and after my patch.

Thanks for the explanation.  I hope you will explain this briefly in the
patch to the docs.

quoted

Secondly, though it is impossible (for backwards compatibility reasons)
for the pre-commit hook to be invoked for automatic merges, no such
considerations prohibit the pre-merge commit from being invoked for
non-automatic merges.  In other words, both hooks, pre-commit *and*
pre-merge, could be invoked for non-automatic merges.  Would this be
preferable?

It depends on what pre-merge scripts are likely to be used for.  If they
will tend to be used for merge-specific actions, then it might be more
convenient for *all* merges to be vetted by them.  On the other hand, if
they tend to do the same actions as pre-commit hooks, then having
non-automatic merge commits go through both hooks would tend to be more
annoying than helpful.  Specifically, one of the scripts would probably
have to check whether the merge is a non-automatic merge, and if so do
nothing (i.e., letting the other script take care of it).  This would
also require an easy way for a script to determine whether a commit is a
non-automatic merge commit.

Have you considered this?

Your second paragraph explains why I did it the way I did. One can
easily have pre-merge call pre-commit, or have them be different. One
can not easily have only pre-merge called for a non-automatic merge
commit, but that is because of backward compatibility. The way *I* would
like it is:

- call pre-merge for any non-ff merge commit (automatic or not)
- call pre-commit for any non-merge commit (#parents <=1)

But that would break compatibility.

So I hope my patch is the best approximation to the above which keeps
compatibility and is simple to handle in most situations.

I can understand your reasoning and won't object.  But before I shut up,
I will point out a third alternative that is arguably closer to your
"ideal":

- For non-merge commits, call pre-commit
- For automatic merge commits, call pre-merge
- For non-automatic merge commits:
  if pre-merge exists, call pre-merge (only)
  else if pre-commit exists, call pre-commit (for backwards comptibility)

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/