{"thread":{"id":"31446","subject":"[PATCH 3/3] t7503: add tests for pre-merge-hook","startedAt":"2012-09-05T13:39:08Z","lastAt":"2012-09-07T10:00:03Z","messageCount":16,"participants":["Michael J Gruber","Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"198382","messageId":"cover.1346851863.git.git@drmicha.warpmail.net","threadId":"31446","inReplyTo":null,"subject":"[PATCH 0/3] pre-merge-hook","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-09-05T13:39:08Z","receivedAt":"2012-09-05T13:39:08Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"The pre-commit hook is often used to ensure certain properties of each\ncomitted tree like formatting or coding standards, validity (lint/make)\nor code quality (make test). But merges introduce new commits unless\nthey are fast forwards, and therefore they can break these properties\nbecause the pre-commit hook is not run by \"git merge\".\n\nIntroduce a pre-merge hook which works for (non ff, automatic) merges\nlike pre-commit does for commits. Typically this will just call the\npre-commit hook (like in the sample hook), but it does not need to.\n\nMichael J Gruber (3):\n git-merge: Honor pre-merge hook\n merge: --no-verify to bypass pre-merge hook\n t7503: add tests for pre-merge-hook\n\n Documentation/git-merge.txt | 2 +-\n Documentation/githooks.txt | 7 +++++\n Documentation/merge-options.txt | 4 +++\n builtin/merge.c | 15 ++++++++-\n t/t7503-pre-commit-hook.sh | 66 ++++++++++++++++++++++++++++++++++++++-\n templates/hooks--pre-merge.sample | 13 ++++++++\n 6 files changed, 104 insertions(+), 3 deletions(-)\n create mode 100755 templates/hooks--pre-merge.sample\n\n-- \n1.7.12.406.gafd3f81\n"},{"id":"198383","messageId":"dc8ebcd7f7b80ff930c04b5a407361ba8f2f077f.1346851863.git.git@drmicha.warpmail.net","threadId":"31446","inReplyTo":"cover.1346851863.git.git@drmicha.warpmail.net","subject":"[PATCH 1/3] git-merge: Honor pre-merge hook","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-09-05T13:39:09Z","receivedAt":"2012-09-05T13:39:09Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"git-merge does not honor the pre-commit hook when doing automatic merge\ncommits, and for compatibility reasons this is going to stay.\n\nIntroduce a pre-merge hook which is called for an automatic merge commit\njust like pre-commit is called for a non-automatic merge commit (or any\nother commit).\n\nSigned-off-by: Michael J Gruber \n---\n Documentation/githooks.txt | 7 +++++++\n builtin/merge.c | 13 ++++++++++++-\n templates/hooks--pre-merge.sample | 13 +++++++++++++\n 3 files changed, 32 insertions(+), 1 deletion(-)\n create mode 100755 templates/hooks--pre-merge.sample\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex b9003fe..3fae643 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -86,6 +86,13 @@ All the 'git commit' hooks are invoked with the environment\n variable `GIT_EDITOR=:` if the command will not bring up an editor\n to modify the commit message.\n \n+pre-merge\n+~~~~~~~~~\n+\n+This hook is invoked by 'git merge' when doing an automatic merge\n+commit; it is equivalent to 'pre-commit' for a non-automatic commit\n+for a merge.\n+\n prepare-commit-msg\n ~~~~~~~~~~~~~~~~~~\n \ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 0ec8f0d..7c09e0b 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -898,12 +898,23 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n {\n \tstruct strbuf msg = STRBUF_INIT;\n \tconst char *comment = _(merge_editor_comment);\n+\tconst char *index_file = get_index_file();\n+\n+\tif (run_hook(index_file, \"pre-merge\", NULL))\n+\t\tabort_commit(remoteheads, NULL);\n+\t/*\n+\t * Re-read the index as pre-merge hook could have updated it,\n+\t * and write it out as a tree. We must do this before we invoke\n+\t * the editor and after we invoke run_status above.\n+\t */\n+\tdiscard_cache();\n+\tread_cache_from(index_file);\n \tstrbuf_addbuf(&msg, &merge_msg);\n \tstrbuf_addch(&msg, '\\n');\n \tif (0 < option_edit)\n \t\tstrbuf_add_lines(&msg, \"# \", comment, strlen(comment));\n \twrite_merge_msg(&msg);\n-\trun_hook(get_index_file(), \"prepare-commit-msg\",\n+\trun_hook(index_file, \"prepare-commit-msg\",\n \t\t git_path(\"MERGE_MSG\"), \"merge\", NULL, NULL);\n \tif (0 < option_edit) {\n \t\tif (launch_editor(git_path(\"MERGE_MSG\"), NULL, NULL))\ndiff --git a/templates/hooks--pre-merge.sample b/templates/hooks--pre-merge.sample\nnew file mode 100755\nindex 0000000..a6313e6\n--- /dev/null\n+++ b/templates/hooks--pre-merge.sample\n@@ -0,0 +1,13 @@\n+#!/bin/sh\n+#\n+# An example hook script to verify what is about to be committed.\n+# Called by \"git merge\" with no arguments. The hook should\n+# exit with non-zero status after issuing an appropriate message if\n+# it wants to stop the commit.\n+#\n+# To enable this hook, rename this file to \"pre-merge\".\n+\n+. git-sh-setup\n+test -x \"$GIT_DIR/hooks/pre-commit\" &&\n+ exec \"$GIT_DIR/hooks/pre-commit\"\n+:\n-- \n1.7.12.406.gafd3f81\n"},{"id":"198384","messageId":"2b6dd795b58f6303e1c1fac242825fe95f150b34.1346851863.git.git@drmicha.warpmail.net","threadId":"31446","inReplyTo":"cover.1346851863.git.git@drmicha.warpmail.net","subject":"[PATCH 2/3] merge: --no-verify to bypass pre-merge hook","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-09-05T13:39:10Z","receivedAt":"2012-09-05T13:39:10Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Analogous to commit, introduce a '--no-verify' option which bypasses the\npre-merge hook. The shorthand '-n' is taken by the (non-existing)\n'--no-stat' already.\n\nSigned-off-by: Michael J Gruber \n---\n Documentation/git-merge.txt | 2 +-\n Documentation/githooks.txt | 2 +-\n Documentation/merge-options.txt | 4 ++++\n builtin/merge.c | 4 +++-\n 4 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex 20f9228..8816865 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]\n-\t[-s ] [-X ]\n+\t[--no-verify] [-s ] [-X ]\n \t[--[no-]rerere-autoupdate] [-m ] [...]\n 'git merge' HEAD ...\n 'git merge' --abort\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 3fae643..e20bfe0 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -91,7 +91,7 @@ pre-merge\n \n This hook is invoked by 'git merge' when doing an automatic merge\n commit; it is equivalent to 'pre-commit' for a non-automatic commit\n-for a merge.\n+for a merge, and can be bypassed with the `\\--no-verify` option. \n \n prepare-commit-msg\n ~~~~~~~~~~~~~~~~~~\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 0bcbe0a..93e2e61 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -70,6 +70,10 @@ merge.\n With --no-squash perform the merge and commit the result. This\n option can be used to override --squash.\n \n+--no-verify::\n+\tThis option bypasses the pre-merge and commit-msg hooks.\n+\tSee also linkgit:githooks[5].\n+\n -s ::\n --strategy=::\n \tUse the given merge strategy; can be supplied more than\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 7c09e0b..4d36a00 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -60,6 +60,7 @@ static const char *branch;\n static char *branch_mergeoptions;\n static int option_renormalize;\n static int verbosity;\n+static int no_verify;\n static int allow_rerere_auto;\n static int abort_current_merge;\n static int show_progress = -1;\n@@ -198,6 +199,7 @@ static struct option builtin_merge_options[] = {\n \t\tN_(\"allow fast-forward (default)\")),\n \tOPT_BOOLEAN(0, \"ff-only\", &fast_forward_only,\n \t\tN_(\"abort if fast-forward is not possible\")),\n+\tOPT_BOOLEAN(0, \"no-verify\", &no_verify, \"bypass pre-merge hook\"),\n \tOPT_RERERE_AUTOUPDATE(&allow_rerere_auto),\n \tOPT_CALLBACK('s', \"strategy\", &use_strategies, N_(\"strategy\"),\n \t\tN_(\"merge strategy to use\"), option_parse_strategy),\n@@ -900,7 +902,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tconst char *comment = _(merge_editor_comment);\n \tconst char *index_file = get_index_file();\n \n-\tif (run_hook(index_file, \"pre-merge\", NULL))\n+\tif (!no_verify && run_hook(index_file, \"pre-merge\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n \t/*\n \t * Re-read the index as pre-merge hook could have updated it,\n-- \n1.7.12.406.gafd3f81\n"},{"id":"198381","messageId":"9794b51187bd02662b84597edcea5911fd5b41f3.1346851864.git.git@drmicha.warpmail.net","threadId":"31446","inReplyTo":"cover.1346851863.git.git@drmicha.warpmail.net","subject":"[PATCH 3/3] t7503: add tests for pre-merge-hook","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-09-05T13:39:11Z","receivedAt":"2012-09-05T13:39:11Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Add tests which make sure that the pre-merge-hook is called when\npresent, allows/disallows merge commits depending on its return value\nand is suppressed by \"--no-verify\".\n\nSigned-off-by: Michael J Gruber \n---\n t/t7503-pre-commit-hook.sh | 66 +++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 65 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh\nindex 984889b..36ae87f 100755\n--- a/t/t7503-pre-commit-hook.sh\n+++ b/t/t7503-pre-commit-hook.sh\n@@ -1,9 +1,22 @@\n #!/bin/sh\n \n-test_description='pre-commit hook'\n+test_description='pre-commit and pre-merge hooks'\n \n . ./test-lib.sh\n \n+test_expect_success 'root commit' '\n+\n+\techo \"root\" > file &&\n+\tgit add file &&\n+\tgit commit -m \"zeroth\" &&\n+\tgit checkout -b side &&\n+\techo \"foo\" > foo &&\n+\tgit add foo &&\n+\tgit commit -m \"make it non-ff\" &&\n+\tgit checkout master\n+\n+'\n+\n test_expect_success 'with no hook' '\n \n \techo \"foo\" > file &&\n@@ -12,6 +25,14 @@ test_expect_success 'with no hook' '\n \n '\n \n+test_expect_success 'with no hook (merge)' '\n+\n+\tgit checkout side &&\n+\tgit merge -m \"merge master\" master &&\n+\tgit checkout master\n+\n+'\n+\n test_expect_success '--no-verify with no hook' '\n \n \techo \"bar\" > file &&\n@@ -20,15 +41,25 @@ test_expect_success '--no-verify with no hook' '\n \n '\n \n+test_expect_success '--no-verify with no hook (merge)' '\n+\n+\tgit checkout side &&\n+\tgit merge --no-verify -m \"merge master\" master &&\n+\tgit checkout master\n+\n+'\n+\n # now install hook that always succeeds\n HOOKDIR=\"$(git rev-parse --git-dir)/hooks\"\n HOOK=\"$HOOKDIR/pre-commit\"\n+MERGEHOOK=\"$HOOKDIR/pre-merge\"\n mkdir -p \"$HOOKDIR\"\n cat > \"$HOOK\" <> file &&\n@@ -46,11 +85,20 @@ test_expect_success '--no-verify with succeeding hook' '\n \n '\n \n+test_expect_success '--no-verify with succeeding hook (merge)' '\n+\n+\tgit checkout side &&\n+\tgit merge --no-verify -m \"merge master\" master &&\n+\tgit checkout master\n+\n+'\n+\n # now a hook that fails\n cat > \"$HOOK\" < git-merge does not honor the pre-commit hook when doing automatic merge\n> commits, and for compatibility reasons this is going to stay.\n> \n> Introduce a pre-merge hook which is called for an automatic merge commit\n> just like pre-commit is called for a non-automatic merge commit (or any\n> other commit).\n\nWhat exactly is an \"automatic merge commit\"? Is it any merge that\ndoesn't have a conflict? A merge that doesn't invoke the editor? A\nmerge done as part of another operation (e.g., pull)? I don't see the\nterm mentioned in the git-merge or githooks man pages.\n\nI think it would be good if you would define this term in the\ndocumentation files that your patch touched, and perhaps in the githooks\nsection about \"pre-commit\" as well.\n\nSecondly, though it is impossible (for backwards compatibility reasons)\nfor the pre-commit hook to be invoked for automatic merges, no such\nconsiderations prohibit the pre-merge commit from being invoked for\nnon-automatic merges. In other words, both hooks, pre-commit *and*\npre-merge, could be invoked for non-automatic merges. Would this be\npreferable?\n\nIt depends on what pre-merge scripts are likely to be used for. If they\nwill tend to be used for merge-specific actions, then it might be more\nconvenient for *all* merges to be vetted by them. On the other hand, if\nthey tend to do the same actions as pre-commit hooks, then having\nnon-automatic merge commits go through both hooks would tend to be more\nannoying than helpful. Specifically, one of the scripts would probably\nhave to check whether the merge is a non-automatic merge, and if so do\nnothing (i.e., letting the other script take care of it). This would\nalso require an easy way for a script to determine whether a commit is a\nnon-automatic merge commit.\n\nHave you considered this?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198418","messageId":"7v8vcn68o9.fsf@alter.siamese.dyndns.org","threadId":"31446","inReplyTo":"cover.1346851863.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 0/3] pre-merge-hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-06T05:07:34Z","receivedAt":"2012-09-06T05:07:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> The pre-commit hook is often used to ensure certain properties of each\n> comitted tree like formatting or coding standards, validity (lint/make)\n> or code quality (make test). But merges introduce new commits unless\n> they are fast forwards, and therefore they can break these properties\n> because the pre-commit hook is not run by \"git merge\".\n>\n> Introduce a pre-merge hook which works for (non ff, automatic) merges\n> like pre-commit does for commits. Typically this will just call the\n> pre-commit hook (like in the sample hook), but it does not need to.\n\nWhen your merge asks for a help from you to resolve conflict, you\nconclude with \"git commit\", and at that point, pre-commit hook will\nhave a chance to reject it, presumably. That means for any project\nthat wants to audit a merge via hook, their pre-commit hook MUST be\nprepared to look at and judge a merge. Given that, is a separate\nhook that \"can just call the pre-commit but does not need to\" really\nneeded and useful?\n\nI admit that I haven't thought things through, but adding a boolean\n\"merge.usePreCommitHook\" smells like a more appropriate approach to\nme.\n\nI dunno.\n"},{"id":"198426","messageId":"50485BCB.20607@drmicha.warpmail.net","threadId":"31446","inReplyTo":"7v8vcn68o9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] pre-merge-hook","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-09-06T08:16:11Z","receivedAt":"2012-09-06T08:16:11Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 06.09.2012 07:07:\n> Michael J Gruber writes:\n> \n>> The pre-commit hook is often used to ensure certain properties of each\n>> comitted tree like formatting or coding standards, validity (lint/make)\n>> or code quality (make test). But merges introduce new commits unless\n>> they are fast forwards, and therefore they can break these properties\n>> because the pre-commit hook is not run by \"git merge\".\n>>\n>> Introduce a pre-merge hook which works for (non ff, automatic) merges\n>> like pre-commit does for commits. Typically this will just call the\n>> pre-commit hook (like in the sample hook), but it does not need to.\n> \n> When your merge asks for a help from you to resolve conflict, you\n> conclude with \"git commit\", and at that point, pre-commit hook will\n> have a chance to reject it, presumably. That means for any project\n> that wants to audit a merge via hook, their pre-commit hook MUST be\n> prepared to look at and judge a merge. Given that, is a separate\n> hook that \"can just call the pre-commit but does not need to\" really\n> needed and useful?\n> \n> I admit that I haven't thought things through, but adding a boolean\n> \"merge.usePreCommitHook\" smells like a more appropriate approach to\n> me.\n> \n> I dunno.\n\nThat would be an option ;)\n\nSeriously, that would make the case where both hooks are the same even\nsimpler, obviously. On the other hand it would make the other case more\ndifficult. But, really, since non-automatic merges call pre-commit now,\nand that is probably to stay, catching those with a different (part of\nthe pre-commit) hook will stay difficult anyways. So the question is\nreally more whether we prefer to bloat the config space or the name\nspace by this.\n\nEither works for me, and if we don't change the current behaviour\n(pre-commit-hook resp. no hook for non-automatic merges resp. automatic\nmerges) the config option is probably less confusing.\n\nMichael\n"},{"id":"198427","messageId":"50485BD3.5020802@drmicha.warpmail.net","threadId":"31446","inReplyTo":"50476FFE.5070602@alum.mit.edu","subject":"Re: [PATCH 1/3] git-merge: Honor pre-merge hook","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-09-06T08:16:19Z","receivedAt":"2012-09-06T08:16:19Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Michael Haggerty venit, vidit, dixit 05.09.2012 17:30:\n> On 09/05/2012 03:39 PM, Michael J Gruber wrote:\n>> git-merge does not honor the pre-commit hook when doing automatic merge\n>> commits, and for compatibility reasons this is going to stay.\n>>\n>> Introduce a pre-merge hook which is called for an automatic merge commit\n>> just like pre-commit is called for a non-automatic merge commit (or any\n>> other commit).\n> \n> What exactly is an \"automatic merge commit\"? Is it any merge that\n> doesn't have a conflict? A merge that doesn't invoke the editor? A\n> merge done as part of another operation (e.g., pull)? I don't see the\n> term mentioned in the git-merge or githooks man pages.\n> \n> I think it would be good if you would define this term in the\n> documentation files that your patch touched, and perhaps in the githooks\n> section about \"pre-commit\" as well.\n\n\"git merge\" can go three ways:\n\nF: fast forward: no commit is created, only a ref is changed\nA: automatic: true merge (non-ff) without conflicts (i.e. chosen\nstrategy can perform the merge); a new commit is created\nC: merge with conflicts: no commit is created but the index is prepared\n(partially) for a merge commit\n\nIn case F, no commit hook is run (talking only about pre-commit/pre-merge).\n\nIn case A, no commit is run so far but my patch proposes pre-merge to be\nrun.\n\nIn case C, pre-commit (!) is run so far and after my patch.\n\n> Secondly, though it is impossible (for backwards compatibility reasons)\n> for the pre-commit hook to be invoked for automatic merges, no such\n> considerations prohibit the pre-merge commit from being invoked for\n> non-automatic merges. In other words, both hooks, pre-commit *and*\n> pre-merge, could be invoked for non-automatic merges. Would this be\n> preferable?\n>\n> It depends on what pre-merge scripts are likely to be used for. If they\n> will tend to be used for merge-specific actions, then it might be more\n> convenient for *all* merges to be vetted by them. On the other hand, if\n> they tend to do the same actions as pre-commit hooks, then having\n> non-automatic merge commits go through both hooks would tend to be more\n> annoying than helpful. Specifically, one of the scripts would probably\n> have to check whether the merge is a non-automatic merge, and if so do\n> nothing (i.e., letting the other script take care of it). This would\n> also require an easy way for a script to determine whether a commit is a\n> non-automatic merge commit.\n> \n> Have you considered this?\n\nYour second paragraph explains why I did it the way I did. One can\neasily have pre-merge call pre-commit, or have them be different. One\ncan not easily have only pre-merge called for a non-automatic merge\ncommit, but that is because of backward compatibility. The way *I* would\nlike it is:\n\n- call pre-merge for any non-ff merge commit (automatic or not)\n- call pre-commit for any non-merge commit (#parents <=1)\n\nBut that would break compatibility.\n\nSo I hope my patch is the best approximation to the above which keeps\ncompatibility and is simple to handle in most situations.\n\nCheers\nMichael\n"},{"id":"198431","messageId":"50487F8A.4050803@alum.mit.edu","threadId":"31446","inReplyTo":"50485BD3.5020802@drmicha.warpmail.net","subject":"Re: [PATCH 1/3] git-merge: Honor pre-merge hook","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-06T10:48:42Z","receivedAt":"2012-09-06T10:48:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/06/2012 10:16 AM, Michael J Gruber wrote:\n> Michael Haggerty venit, vidit, dixit 05.09.2012 17:30:\n>> On 09/05/2012 03:39 PM, Michael J Gruber wrote:\n>>> git-merge does not honor the pre-commit hook when doing automatic merge\n>>> commits, and for compatibility reasons this is going to stay.\n>>>\n>>> Introduce a pre-merge hook which is called for an automatic merge commit\n>>> just like pre-commit is called for a non-automatic merge commit (or any\n>>> other commit).\n>>\n>> What exactly is an \"automatic merge commit\"? Is it any merge that\n>> doesn't have a conflict? A merge that doesn't invoke the editor? A\n>> merge done as part of another operation (e.g., pull)? I don't see the\n>> term mentioned in the git-merge or githooks man pages.\n>>\n>> I think it would be good if you would define this term in the\n>> documentation files that your patch touched, and perhaps in the githooks\n>> section about \"pre-commit\" as well.\n> \n> \"git merge\" can go three ways:\n> \n> F: fast forward: no commit is created, only a ref is changed\n> A: automatic: true merge (non-ff) without conflicts (i.e. chosen\n> strategy can perform the merge); a new commit is created\n> C: merge with conflicts: no commit is created but the index is prepared\n> (partially) for a merge commit\n> \n> In case F, no commit hook is run (talking only about pre-commit/pre-merge).\n> \n> In case A, no commit is run so far but my patch proposes pre-merge to be\n> run.\n> \n> In case C, pre-commit (!) is run so far and after my patch.\n\nThanks for the explanation. I hope you will explain this briefly in the\npatch to the docs.\n\n>> Secondly, though it is impossible (for backwards compatibility reasons)\n>> for the pre-commit hook to be invoked for automatic merges, no such\n>> considerations prohibit the pre-merge commit from being invoked for\n>> non-automatic merges. In other words, both hooks, pre-commit *and*\n>> pre-merge, could be invoked for non-automatic merges. Would this be\n>> preferable?\n>>\n>> It depends on what pre-merge scripts are likely to be used for. If they\n>> will tend to be used for merge-specific actions, then it might be more\n>> convenient for *all* merges to be vetted by them. On the other hand, if\n>> they tend to do the same actions as pre-commit hooks, then having\n>> non-automatic merge commits go through both hooks would tend to be more\n>> annoying than helpful. Specifically, one of the scripts would probably\n>> have to check whether the merge is a non-automatic merge, and if so do\n>> nothing (i.e., letting the other script take care of it). This would\n>> also require an easy way for a script to determine whether a commit is a\n>> non-automatic merge commit.\n>>\n>> Have you considered this?\n> \n> Your second paragraph explains why I did it the way I did. One can\n> easily have pre-merge call pre-commit, or have them be different. One\n> can not easily have only pre-merge called for a non-automatic merge\n> commit, but that is because of backward compatibility. The way *I* would\n> like it is:\n> \n> - call pre-merge for any non-ff merge commit (automatic or not)\n> - call pre-commit for any non-merge commit (#parents <=1)\n> \n> But that would break compatibility.\n> \n> So I hope my patch is the best approximation to the above which keeps\n> compatibility and is simple to handle in most situations.\n\nI can understand your reasoning and won't object. But before I shut up,\nI will point out a third alternative that is arguably closer to your\n\"ideal\":\n\n- For non-merge commits, call pre-commit\n- For automatic merge commits, call pre-merge\n- For non-automatic merge commits:\n if pre-merge exists, call pre-merge (only)\n else if pre-commit exists, call pre-commit (for backwards comptibility)\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198442","messageId":"cover.1346939542.git.git@drmicha.warpmail.net","threadId":"31446","inReplyTo":"50487F8A.4050803@alum.mit.edu","subject":"[PATCHv2 0/4] pre-commit hook for merges","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-09-06T14:25:54Z","receivedAt":"2012-09-06T14:25:54Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"In this second iteration I implement Junio's suggestion: 'git merge' invokes\nthe pre-commit hook when merge.usePreCommitHook is set to true.\n\n1/4 documents that 'git merge' invokes the prepare-commit-msg hook\nunconditionally already.\n\n2-4 are v2 of the previous 1-3.\n\nThis leaves aside the issue of commit-msg and post-commit hooks. If we can live\nwith a bit of incompatibility I would rename the config to merge.useCommitHooks\nand call all of them based on that. This would change the way prepare-commit-msg is\ntreated now.\n\nWe could also introduce 4 config variables, 3 defaulting to false, 1 to true,\nof course...\n\n[I had messed up my alias file when adding mhagger, and it seems that tripped up\nvger; resending, sorry.]\n\nMichael J Gruber (4):\n merge: document prepare-commit-msg hook usage\n git-merge: Honor pre-commit hook based on config\n merge: --no-verify to bypass pre-commit hook\n t7503: add tests for pre-commit hook (merge)\n\n Documentation/git-merge.txt | 7 ++++-\n Documentation/githooks.txt | 6 ++++\n Documentation/merge-config.txt | 5 ++++\n Documentation/merge-options.txt | 4 +++\n builtin/merge.c | 19 ++++++++++++-\n t/t7503-pre-commit-hook.sh | 62 +++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 101 insertions(+), 2 deletions(-)\n\n-- \n1.7.12.406.gafd3f81\n"},{"id":"198443","messageId":"f64d15fe61f1e9d755a7afbe015d1b0dbf5f706d.1346939542.git.git@drmicha.warpmail.net","threadId":"31446","inReplyTo":"50487F8A.4050803@alum.mit.edu","subject":"[PATCHv2 1/4] merge: document prepare-commit-msg hook usage","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-09-06T14:25:55Z","receivedAt":"2012-09-06T14:25:55Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"\nSigned-off-by: Michael J Gruber \n---\n Documentation/git-merge.txt | 5 +++++\n Documentation/githooks.txt | 2 ++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex 20f9228..b3ba8a8 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -308,6 +308,11 @@ branch..mergeoptions::\n \tsupported options are the same as those of 'git merge', but option\n \tvalues containing whitespace characters are currently not supported.\n \n+HOOKS\n+-----\n+This command can run the `prepare-commit-msg` hook.\n+See linkgit:githooks[5] for more information.\n+\n SEE ALSO\n --------\n linkgit:git-fmt-merge-msg[1], linkgit:git-pull[1],\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex b9003fe..b32134c 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -108,6 +108,8 @@ it is not suppressed by the `--no-verify` option. A non-zero exit\n means a failure of the hook and aborts the commit. It should not\n be used as replacement for pre-commit hook.\n \n+This hook is also called by 'git merge' when it creates a new commit.\n+\n The sample `prepare-commit-msg` hook that comes with git comments\n out the `Conflicts:` part of a merge's commit message.\n \n-- \n1.7.12.406.gafd3f81\n"},{"id":"198444","messageId":"882ac21b36f2498492f6ca2be2df6c446b3a290b.1346939542.git.git@drmicha.warpmail.net","threadId":"31446","inReplyTo":"50487F8A.4050803@alum.mit.edu","subject":"[PATCHv2 2/4] git-merge: Honor pre-commit hook based on config","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-09-06T14:25:56Z","receivedAt":"2012-09-06T14:25:56Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"git-merge does not honor the pre-commit hook when doing automatic merge\ncommits, and for compatibility reasons this is going to stay.\n\nIntroduce a merge.usePreCommitHook which controls whether an automatic\nmerge commit invokes pre-commit.\n\nSigned-off-by: Michael J Gruber \n---\n Documentation/git-merge.txt | 2 +-\n Documentation/githooks.txt | 3 +++\n Documentation/merge-config.txt | 5 +++++\n builtin/merge.c | 17 ++++++++++++++++-\n 4 files changed, 25 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex b3ba8a8..3501ae2 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -310,7 +310,7 @@ branch..mergeoptions::\n \n HOOKS\n -----\n-This command can run the `prepare-commit-msg` hook.\n+This command can run the `prepare-commit-msg` and `pre-commit` hooks.\n See linkgit:githooks[5] for more information.\n \n SEE ALSO\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex b32134c..d62e02d 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -86,6 +86,9 @@ All the 'git commit' hooks are invoked with the environment\n variable `GIT_EDITOR=:` if the command will not bring up an editor\n to modify the commit message.\n \n+If the configuration option `merge.usePreCommitHook` is set to `true`\n+then 'git merge' invokes this hook whenever it creates a new commit.\n+\n prepare-commit-msg\n ~~~~~~~~~~~~~~~~~~\n \ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex 861bd6f..727caad 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -59,6 +59,11 @@ merge.tool::\n \tand \"xxdiff\". Any other value is treated is custom merge tool\n \tand there must be a corresponding mergetool..cmd option.\n \n+merge.usePreCommitHook::\n+\tControls whether the pre-commit hook is invoked when 'git merge'\n+\tcreates a new commit (true merge without conflicts). False by\n+\tdefault.\n+\n merge.verbosity::\n \tControls the amount of output shown by the recursive merge\n \tstrategy. Level 0 outputs nothing except a final error\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 0ec8f0d..a2590a9 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -64,6 +64,7 @@ static int allow_rerere_auto;\n static int abort_current_merge;\n static int show_progress = -1;\n static int default_to_upstream;\n+static int use_pre_commit_hook;\n static const char *sign_commit;\n \n static struct strategy all_strategy[] = {\n@@ -575,6 +576,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n \t} else if (!strcmp(k, \"merge.defaulttoupstream\")) {\n \t\tdefault_to_upstream = git_config_bool(k, v);\n \t\treturn 0;\n+\t} else if (!strcmp(k, \"merge.useprecommithook\")) {\n+\t\tuse_pre_commit_hook = git_config_bool(k, v);\n+\t\treturn 0;\n \t}\n \n \tstatus = fmt_merge_msg_config(k, v, cb);\n@@ -898,12 +902,23 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n {\n \tstruct strbuf msg = STRBUF_INIT;\n \tconst char *comment = _(merge_editor_comment);\n+\tconst char *index_file = get_index_file();\n+\n+\tif (use_pre_commit_hook && run_hook(index_file, \"pre-commit\", NULL))\n+\t\tabort_commit(remoteheads, NULL);\n+\t/*\n+\t * Re-read the index as pre-commit hook could have updated it,\n+\t * and write it out as a tree. We must do this before we invoke\n+\t * the editor and after we invoke run_status above.\n+\t */\n+\tdiscard_cache();\n+\tread_cache_from(index_file);\n \tstrbuf_addbuf(&msg, &merge_msg);\n \tstrbuf_addch(&msg, '\\n');\n \tif (0 < option_edit)\n \t\tstrbuf_add_lines(&msg, \"# \", comment, strlen(comment));\n \twrite_merge_msg(&msg);\n-\trun_hook(get_index_file(), \"prepare-commit-msg\",\n+\trun_hook(index_file, \"prepare-commit-msg\",\n \t\t git_path(\"MERGE_MSG\"), \"merge\", NULL, NULL);\n \tif (0 < option_edit) {\n \t\tif (launch_editor(git_path(\"MERGE_MSG\"), NULL, NULL))\n-- \n1.7.12.406.gafd3f81\n"},{"id":"198445","messageId":"526ce72df2e65572cbbbc72d3f93b4a917562ead.1346939542.git.git@drmicha.warpmail.net","threadId":"31446","inReplyTo":"50487F8A.4050803@alum.mit.edu","subject":"[PATCHv2 3/4] merge: --no-verify to bypass pre-commit hook","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-09-06T14:25:57Z","receivedAt":"2012-09-06T14:25:57Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Analogous to commit, introduce a '--no-verify' option which bypasses the\npre-commit hook. The shorthand '-n' is taken by the (non-existing)\n'--no-stat' already.\n\nSigned-off-by: Michael J Gruber \n---\n Documentation/git-merge.txt | 2 +-\n Documentation/githooks.txt | 1 +\n Documentation/merge-options.txt | 4 ++++\n builtin/merge.c | 4 +++-\n 4 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex 3501ae2..363fbea 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]\n-\t[-s ] [-X ]\n+\t[--no-verify] [-s ] [-X ]\n \t[--[no-]rerere-autoupdate] [-m ] [...]\n 'git merge' HEAD ...\n 'git merge' --abort\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex d62e02d..c734e2c 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -88,6 +88,7 @@ to modify the commit message.\n \n If the configuration option `merge.usePreCommitHook` is set to `true`\n then 'git merge' invokes this hook whenever it creates a new commit.\n+It can be bypassed with the `\\--no-verify` option. \n \n prepare-commit-msg\n ~~~~~~~~~~~~~~~~~~\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 0bcbe0a..5695fc6 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -70,6 +70,10 @@ merge.\n With --no-squash perform the merge and commit the result. This\n option can be used to override --squash.\n \n+--no-verify::\n+\tThis option bypasses the pre-commit hook.\n+\tSee also linkgit:githooks[5].\n+\n -s ::\n --strategy=::\n \tUse the given merge strategy; can be supplied more than\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a2590a9..58a848f 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -60,6 +60,7 @@ static const char *branch;\n static char *branch_mergeoptions;\n static int option_renormalize;\n static int verbosity;\n+static int no_verify;\n static int allow_rerere_auto;\n static int abort_current_merge;\n static int show_progress = -1;\n@@ -199,6 +200,7 @@ static struct option builtin_merge_options[] = {\n \t\tN_(\"allow fast-forward (default)\")),\n \tOPT_BOOLEAN(0, \"ff-only\", &fast_forward_only,\n \t\tN_(\"abort if fast-forward is not possible\")),\n+\tOPT_BOOLEAN(0, \"no-verify\", &no_verify, \"bypass pre-merge hook\"),\n \tOPT_RERERE_AUTOUPDATE(&allow_rerere_auto),\n \tOPT_CALLBACK('s', \"strategy\", &use_strategies, N_(\"strategy\"),\n \t\tN_(\"merge strategy to use\"), option_parse_strategy),\n@@ -904,7 +906,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tconst char *comment = _(merge_editor_comment);\n \tconst char *index_file = get_index_file();\n \n-\tif (use_pre_commit_hook && run_hook(index_file, \"pre-commit\", NULL))\n+\tif (use_pre_commit_hook && !no_verify && run_hook(index_file, \"pre-commit\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n \t/*\n \t * Re-read the index as pre-commit hook could have updated it,\n-- \n1.7.12.406.gafd3f81\n"},{"id":"198446","messageId":"084ade976d944576da58c8334ffec5e47b209421.1346939542.git.git@drmicha.warpmail.net","threadId":"31446","inReplyTo":"50487F8A.4050803@alum.mit.edu","subject":"[PATCHv2 4/4] t7503: add tests for pre-commit hook (merge)","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-09-06T14:25:58Z","receivedAt":"2012-09-06T14:25:58Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Add tests which make sure that the pre-commit hook is called by 'git\nmerge' when merge.usePreCommitHook is set, allows/disallows merge\ncommits depending on its return value and is suppressed by\n\"--no-verify\".\n\nSigned-off-by: Michael J Gruber \n---\n t/t7503-pre-commit-hook.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 62 insertions(+)\n\ndiff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh\nindex 984889b..e4d324d 100755\n--- a/t/t7503-pre-commit-hook.sh\n+++ b/t/t7503-pre-commit-hook.sh\n@@ -4,6 +4,19 @@ test_description='pre-commit hook'\n \n . ./test-lib.sh\n \n+test_expect_success 'root commit' '\n+\n+\techo \"root\" > file &&\n+\tgit add file &&\n+\tgit commit -m \"zeroth\" &&\n+\tgit checkout -b side &&\n+\techo \"foo\" > foo &&\n+\tgit add foo &&\n+\tgit commit -m \"make it non-ff\" &&\n+\tgit checkout master\n+\n+'\n+\n test_expect_success 'with no hook' '\n \n \techo \"foo\" > file &&\n@@ -12,6 +25,14 @@ test_expect_success 'with no hook' '\n \n '\n \n+test_expect_success 'with no hook (merge)' '\n+\n+\tgit checkout side &&\n+\tgit merge -m \"merge master\" master &&\n+\tgit checkout master\n+\n+'\n+\n test_expect_success '--no-verify with no hook' '\n \n \techo \"bar\" > file &&\n@@ -20,6 +41,14 @@ test_expect_success '--no-verify with no hook' '\n \n '\n \n+test_expect_success '--no-verify with no hook (merge)' '\n+\n+\tgit checkout side &&\n+\tgit merge --no-verify -m \"merge master\" master &&\n+\tgit checkout master\n+\n+'\n+\n # now install hook that always succeeds\n HOOKDIR=\"$(git rev-parse --git-dir)/hooks\"\n HOOK=\"$HOOKDIR/pre-commit\"\n@@ -29,6 +58,7 @@ cat > \"$HOOK\" <> file &&\n@@ -46,6 +84,14 @@ test_expect_success '--no-verify with succeeding hook' '\n \n '\n \n+test_expect_success '--no-verify with succeeding hook (merge)' '\n+\n+\tgit checkout side &&\n+\tgit merge --no-verify -m \"merge master\" master &&\n+\tgit checkout master\n+\n+'\n+\n # now a hook that fails\n cat > \"$HOOK\" < writes:\n\n> Junio C Hamano venit, vidit, dixit 06.09.2012 07:07:\n>> Michael J Gruber writes:\n>> \n>>> The pre-commit hook is often used to ensure certain properties of each\n>>> comitted tree like formatting or coding standards, validity (lint/make)\n>>> or code quality (make test). But merges introduce new commits unless\n>>> they are fast forwards, and therefore they can break these properties\n>>> because the pre-commit hook is not run by \"git merge\".\n>>>\n>>> Introduce a pre-merge hook which works for (non ff, automatic) merges\n>>> like pre-commit does for commits. Typically this will just call the\n>>> pre-commit hook (like in the sample hook), but it does not need to.\n>> \n>> When your merge asks for a help from you to resolve conflict, you\n>> conclude with \"git commit\", and at that point, pre-commit hook will\n>> have a chance to reject it, presumably. That means for any project\n>> that wants to audit a merge via hook, their pre-commit hook MUST be\n>> prepared to look at and judge a merge. Given that, is a separate\n>> hook that \"can just call the pre-commit but does not need to\" really\n>> needed and useful?\n>> \n>> I admit that I haven't thought things through, but adding a boolean\n>> \"merge.usePreCommitHook\" smells like a more appropriate approach to\n>> me.\n>> \n>> I dunno.\n>\n> That would be an option ;)\n\nI said \"I dunno\" as others may have opinions on the best direction\nto go.\n\n> Either works for me, and if we don't change the current behaviour\n> (pre-commit-hook resp. no hook for non-automatic merges resp. automatic\n> merges) the config option is probably less confusing.\n\nIf we were to go in the \"pre-commit has to be prepared to vet a\nmerge already, so let it handle the automerge case\" direction, I\nhave another suggestion.\n\nBecause you need to support \"merge --no-verify\" to override the hook\nanyway, I think it makes sense to introduce \"merge --verify\" to\nforce it trigger the hook (and it needs to percolate up to \"pull\").\n\nPeople who want it always on may want a boolean merge.verify, but\nthat should come in a separate step. The patch that implements it\nmust make sure all our internal uses of \"merge\" is updated to pass\n\"--no-verify\", unless there is a very good reason.\n\nAnother direction to go would be to deprecate the \"commit is the way\nto conclude a merge that stopped in the middle due to a conflict\nasking for your help\" and introduce \"merge --continue\" [*1*]. That\nwould open a way to a separate \"pre/post-merge\" hook much cleanly.\nAt that point it would be clear that \"pre-commit\" won't be involved\nin \"merge\" (i.e. the user never will use \"git commit\" when merging).\n\nI am fairly negative on the current mess imposed on \"git commit\"\nthat has to know who called it and behave differently (look for\n\"whence\" in builtin/commit.c), and would rather not to see it made\nworse by piling \"call pre-merge if exists and in a merge, or call\npre-commit\" kind of hack on top of it.\n\n\n\n[Footnote]\n\n*1* This has been brought up a few times during discussion on\nsequencer and mergy operations, and I think it makes sense in the\nlonger term. \"am\" and \"rebase\" were first to use \"--continue\",\ninstead of having the user to use \"commit\" to conclude, and later\n\"cherry-pick\" and \"revert\" have been updated to follow suit, so\n\"merge\" may be the only oddball remaining now.\n"},{"id":"198512","messageId":"5049C5A3.1000202@drmicha.warpmail.net","threadId":"31446","inReplyTo":"7vwr072e7a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] pre-merge-hook","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-09-07T10:00:03Z","receivedAt":"2012-09-07T10:00:03Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 06.09.2012 20:34:\n> Michael J Gruber writes:\n> \n>> Junio C Hamano venit, vidit, dixit 06.09.2012 07:07:\n>>> Michael J Gruber writes:\n>>> \n>>>> The pre-commit hook is often used to ensure certain properties\n>>>> of each comitted tree like formatting or coding standards,\n>>>> validity (lint/make) or code quality (make test). But merges\n>>>> introduce new commits unless they are fast forwards, and\n>>>> therefore they can break these properties because the\n>>>> pre-commit hook is not run by \"git merge\".\n>>>> \n>>>> Introduce a pre-merge hook which works for (non ff, automatic)\n>>>> merges like pre-commit does for commits. Typically this will\n>>>> just call the pre-commit hook (like in the sample hook), but it\n>>>> does not need to.\n>>> \n>>> When your merge asks for a help from you to resolve conflict,\n>>> you conclude with \"git commit\", and at that point, pre-commit\n>>> hook will have a chance to reject it, presumably. That means for\n>>> any project that wants to audit a merge via hook, their\n>>> pre-commit hook MUST be prepared to look at and judge a merge.\n>>> Given that, is a separate hook that \"can just call the pre-commit\n>>> but does not need to\" really needed and useful?\n>>> \n>>> I admit that I haven't thought things through, but adding a\n>>> boolean \"merge.usePreCommitHook\" smells like a more appropriate\n>>> approach to me.\n>>> \n>>> I dunno.\n>> \n>> That would be an option ;)\n> \n> I said \"I dunno\" as others may have opinions on the best direction to\n> go.\n\nSure. I meant to make a pun the config option approach being an option.\n\n>> Either works for me, and if we don't change the current behaviour \n>> (pre-commit-hook resp. no hook for non-automatic merges resp.\n>> automatic merges) the config option is probably less confusing.\n> \n> If we were to go in the \"pre-commit has to be prepared to vet a merge\n> already, so let it handle the automerge case\" direction, I have\n> another suggestion.\n> \n> Because you need to support \"merge --no-verify\" to override the hook \n> anyway, I think it makes sense to introduce \"merge --verify\" to force\n> it trigger the hook (and it needs to percolate up to \"pull\").\n> \n> People who want it always on may want a boolean merge.verify, but \n> that should come in a separate step. The patch that implements it \n> must make sure all our internal uses of \"merge\" is updated to pass \n> \"--no-verify\", unless there is a very good reason.\n\nHmm. Nothing calls cmd_merge() nor the relevant parts. \"git pull\" calls\n\"git merge\" and should probably obey that option. All others (am etc.)\ncall git-merge-${strategy} directly and are not affected by the option.\nAm I overlooking something?\n\n> Another direction to go would be to deprecate the \"commit is the way \n> to conclude a merge that stopped in the middle due to a conflict \n> asking for your help\" and introduce \"merge --continue\" [*1*]. That \n> would open a way to a separate \"pre/post-merge\" hook much cleanly. At\n> that point it would be clear that \"pre-commit\" won't be involved in\n> \"merge\" (i.e. the user never will use \"git commit\" when merging).\n> \n> I am fairly negative on the current mess imposed on \"git commit\" that\n> has to know who called it and behave differently (look for \"whence\"\n> in builtin/commit.c), and would rather not to see it made worse by\n> piling \"call pre-merge if exists and in a merge, or call pre-commit\"\n> kind of hack on top of it.\n\nThat is a mess, yes. Part of it is due to the fact that both\nbuiltin/{commit,merge}.c do a lot of \"commit stuff\" in parallel, often\nwith copy & pasted code, and \"commit\" needs to be aware of other states\n(in-am, in-rebase) also.\n\nMy feeling is that builtin/merge.c should rather get rid of the stuff\nthat parallels things that builtin/commit.c does, so that all is in one\nplace. (I think that redundancy is to blame for the inconsistent hook\nbehaviour for merges even within builtin/merge.c).\n\n> [Footnote]\n> \n> *1* This has been brought up a few times during discussion on \n> sequencer and mergy operations, and I think it makes sense in the \n> longer term. \"am\" and \"rebase\" were first to use \"--continue\", \n> instead of having the user to use \"commit\" to conclude, and later \n> \"cherry-pick\" and \"revert\" have been updated to follow suit, so \n> \"merge\" may be the only oddball remaining now.\n\n\"git merge --continue\" to commit a resolved merge, but code-wise\nbuiltin/commit.c taking over because it's just one more case?\n\nI afraid that restructuring is beyond my available Git capacity and my\nself-imposed constraint to avoid anything with backwards compatibility\nor migration plan issues. (I took this up because I thought it's within,\nto share something I've been using for a while.)\n\nI fully agree that a big-picture-solution is preferable.\n\nMichael\n"}]}