Re: [PATCH 0/3] pre-merge-hook

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 6, 2012, 18:34 UTC
Message-ID: <7vwr072e7a.fsf@alter.siamese.dyndns.org>
In-Reply-To: <50485BCB.20607@drmicha.warpmail.net>

Michael J Gruber <git@drmicha.warpmail.net> writes:

Show 28 quoted lines

> Junio C Hamano venit, vidit, dixit 06.09.2012 07:07:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>> 
>>> The pre-commit hook is often used to ensure certain properties of each
>>> comitted tree like formatting or coding standards, validity (lint/make)
>>> or code quality (make test). But merges introduce new commits unless
>>> they are fast forwards, and therefore they can break these properties
>>> because the pre-commit hook is not run by "git merge".
>>>
>>> Introduce a pre-merge hook which works for (non ff, automatic) merges
>>> like pre-commit does for commits. Typically this will just call the
>>> pre-commit hook (like in the sample hook), but it does not need to.
>> 
>> When your merge asks for a help from you to resolve conflict, you
>> conclude with "git commit", and at that point, pre-commit hook will
>> have a chance to reject it, presumably.  That means for any project
>> that wants to audit a merge via hook, their pre-commit hook MUST be
>> prepared to look at and judge a merge.  Given that, is a separate
>> hook that "can just call the pre-commit but does not need to" really
>> needed and useful?
>> 
>> I admit that I haven't thought things through, but adding a boolean
>> "merge.usePreCommitHook" smells like a more appropriate approach to
>> me.
>> 
>> I dunno.
>
> That would be an option ;)

I said "I dunno" as others may have opinions on the best direction to go.

> Either works for me, and if we don't change the current behaviour
> (pre-commit-hook resp. no hook for non-automatic merges resp. automatic
> merges) the config option is probably less confusing.

If we were to go in the "pre-commit has to be prepared to vet a merge already, so let it handle the automerge case" direction, I have another suggestion.

Because you need to support "merge --no-verify" to override the hook anyway, I think it makes sense to introduce "merge --verify" to force it trigger the hook (and it needs to percolate up to "pull").

People who want it always on may want a boolean merge.verify, but that should come in a separate step. The patch that implements it must make sure all our internal uses of "merge" is updated to pass "--no-verify", unless there is a very good reason.

Another direction to go would be to deprecate the "commit is the way to conclude a merge that stopped in the middle due to a conflict asking for your help" and introduce "merge --continue" [*1*]. That would open a way to a separate "pre/post-merge" hook much cleanly. At that point it would be clear that "pre-commit" won't be involved in "merge" (i.e. the user never will use "git commit" when merging).

I am fairly negative on the current mess imposed on "git commit" that has to know who called it and behave differently (look for "whence" in builtin/commit.c), and would rather not to see it made worse by piling "call pre-merge if exists and in a merge, or call pre-commit" kind of hack on top of it.

[Footnote]

*1* This has been brought up a few times during discussion on sequencer and mergy operations, and I think it makes sense in the longer term. "am" and "rebase" were first to use "--continue", instead of having the user to use "commit" to conclude, and later "cherry-pick" and "revert" have been updated to follow suit, so "merge" may be the only oddball remaining now.