From: Michael Haggerty <mhagger@alum•mit.edu>
To: Michael J Gruber <git@drmicha•warpmail.net>
Cc: git@vger•kernel.org, Junio C Hamano <gitster@pobox•com>
Subject: Re: [PATCH 1/3] git-merge: Honor pre-merge hook
Date: Thu, 06 Sep 2012 12:48:42 +0200 [thread overview]
Message-ID: <50487F8A.4050803@alum.mit.edu> (raw)
In-Reply-To: <50485BD3.5020802@drmicha.warpmail.net>
On 09/06/2012 10:16 AM, Michael J Gruber wrote:
> Michael Haggerty venit, vidit, dixit 05.09.2012 17:30:
>> On 09/05/2012 03:39 PM, Michael J Gruber wrote:
>>> 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.
>> 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/
next prev parent reply other threads:[~2012-09-06 10:48 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-05 13:39 [PATCH 0/3] pre-merge-hook Michael J Gruber
2012-09-05 13:39 ` [PATCH 1/3] git-merge: Honor pre-merge hook Michael J Gruber
2012-09-05 15:30 ` Michael Haggerty
2012-09-06 8:16 ` Michael J Gruber
2012-09-06 10:48 ` Michael Haggerty [this message]
2012-09-06 14:25 ` [PATCHv2 0/4] pre-commit hook for merges Michael J Gruber
2012-09-06 14:25 ` [PATCHv2 1/4] merge: document prepare-commit-msg hook usage Michael J Gruber
2012-09-06 14:25 ` [PATCHv2 2/4] git-merge: Honor pre-commit hook based on config Michael J Gruber
2012-09-06 14:25 ` [PATCHv2 3/4] merge: --no-verify to bypass pre-commit hook Michael J Gruber
2012-09-06 14:25 ` [PATCHv2 4/4] t7503: add tests for pre-commit hook (merge) Michael J Gruber
2012-09-05 13:39 ` [PATCH 2/3] merge: --no-verify to bypass pre-merge hook Michael J Gruber
2012-09-05 13:39 ` [PATCH 3/3] t7503: add tests for pre-merge-hook Michael J Gruber
2012-09-06 5:07 ` [PATCH 0/3] pre-merge-hook Junio C Hamano
2012-09-06 8:16 ` Michael J Gruber
2012-09-06 18:34 ` Junio C Hamano
2012-09-07 10:00 ` Michael J Gruber
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50487F8A.4050803@alum.mit.edu \
--to=mhagger@alum$(echo .)mit.edu \
--cc=git@drmicha$(echo .)warpmail.net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox