Re: [PATCH 0/3] pre-merge-hook
Junio C Hamano venit, vidit, dixit 06.09.2012 20:34:
Michael J Gruber g...@drmicha.warpmail.net writes:
Junio C Hamano venit, vidit, dixit 06.09.2012 07:07:
Michael J Gruber g...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] pre-merge-hook
Michael J Gruber g...@drmicha.warpmail.net writes: Junio C Hamano venit, vidit, dixit 06.09.2012 07:07: Michael J Gruber g...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] pre-merge-hook
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. Michael J Gruber (3): git-merge: Honor pre-merge hook merge: --no-verify to bypass pre-merge hook t7503: add tests for pre-merge-hook Documentation/git-merge.txt | 2 +- Documentation/githooks.txt| 7 + Documentation/merge-options.txt | 4 +++ builtin/merge.c | 15 - t/t7503-pre-commit-hook.sh| 66 ++- templates/hooks--pre-merge.sample | 13 6 files changed, 104 insertions(+), 3 deletions(-) create mode 100755 templates/hooks--pre-merge.sample -- 1.7.12.406.gafd3f81 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] pre-merge-hook
Michael J Gruber g...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
