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

Junio C Hamano venit, vidit, dixit 06.09.2012 20:34:

Michael J Gruber [off-list ref] writes:

quoted

Junio C Hamano venit, vidit, dixit 06.09.2012 07:07:

quoted

Michael J Gruber [off-list ref] writes:

quoted

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.

quoted

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