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
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