From: Michael J Gruber <git@drmicha•warpmail.net>
To: Junio C Hamano <gitster@pobox•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH 0/3] pre-merge-hook
Date: Fri, 07 Sep 2012 12:00:03 +0200 [thread overview]
Message-ID: <5049C5A3.1000202@drmicha.warpmail.net> (raw)
In-Reply-To: <7vwr072e7a.fsf@alter.siamese.dyndns.org>
Junio C Hamano venit, vidit, dixit 06.09.2012 20:34:
> Michael J Gruber <git@drmicha•warpmail.net> writes:
>
>> 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.
Sure. I meant to make a pun the config option approach being an option.
>> 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.
Hmm. Nothing calls cmd_merge() nor the relevant parts. "git pull" calls
"git merge" and should probably obey that option. All others (am etc.)
call git-merge-${strategy} directly and are not affected by the option.
Am I overlooking something?
> 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.
That is a mess, yes. Part of it is due to the fact that both
builtin/{commit,merge}.c do a lot of "commit stuff" in parallel, often
with copy & pasted code, and "commit" needs to be aware of other states
(in-am, in-rebase) also.
My feeling is that builtin/merge.c should rather get rid of the stuff
that parallels things that builtin/commit.c does, so that all is in one
place. (I think that redundancy is to blame for the inconsistent hook
behaviour for merges even within builtin/merge.c).
> [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.
"git merge --continue" to commit a resolved merge, but code-wise
builtin/commit.c taking over because it's just one more case?
I afraid that restructuring is beyond my available Git capacity and my
self-imposed constraint to avoid anything with backwards compatibility
or migration plan issues. (I took this up because I thought it's within,
to share something I've been using for a while.)
I fully agree that a big-picture-solution is preferable.
Michael
prev parent reply other threads:[~2012-09-07 10:00 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
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 [this message]
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=5049C5A3.1000202@drmicha.warpmail.net \
--to=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