{"thread":{"id":"46707","subject":"[PATCH] builtin/merge: honor commit-msg hook for merges","startedAt":"2017-09-05T21:01:30Z","lastAt":"2017-09-22T01:58:17Z","messageCount":19,"participants":["Stefan Beller","Junio C Hamano","Kaartic Sivaraam"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"327605","messageId":"20170905210116.26343-1-sbeller@google.com","threadId":"46707","inReplyTo":null,"subject":"[PATCH] builtin/merge: honor commit-msg hook for merges","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-05T21:01:16Z","receivedAt":"2017-09-05T21:01:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Similar to 65969d43d1 (merge: honor prepare-commit-msg hook, 2011-02-14)\nmerge should also honor the commit-msg hook; the reason is the same as\nin that commit: When a merge is stopped due to conflicts or --no-commit,\nthe subsequent commit calls the commit-msg hook.  However, it is not\ncalled after a clean merge. Fix this inconsistency by invoking the hook\nafter clean merges as well.\n\nThis change is motivated by Gerrits commit-msg hook to install a change-id\ntrailer into the commit message. Without such a change id, Gerrit refuses\nto accept (merge) commits by default, such that the inconsistency of\n(not) running commit-msg hook between commit and merge leads to confusion\nand might block people from getting their work done.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/merge.c            |  8 ++++++++\n t/t7504-commit-msg-hook.sh | 45 +++++++++++++++++++++++++++++++++++++++++----\n 2 files changed, 49 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 7df3fe3927..087efd560d 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -73,6 +73,7 @@ static int show_progress = -1;\n static int default_to_upstream = 1;\n static int signoff;\n static const char *sign_commit;\n+static int no_verify;\n \n static struct strategy all_strategy[] = {\n \t{ \"recursive\",  DEFAULT_TWOHEAD | NO_TRIVIAL },\n@@ -236,6 +237,7 @@ static struct option builtin_merge_options[] = {\n \t  N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n \tOPT_BOOL(0, \"overwrite-ignore\", &overwrite_ignore, N_(\"update ignored files (default)\")),\n \tOPT_BOOL(0, \"signoff\", &signoff, N_(\"add Signed-off-by:\")),\n+\tOPT_BOOL(0, \"no-verify\", &no_verify, N_(\"bypass pre-commit and commit-msg hooks\")),\n \tOPT_END()\n };\n \n@@ -780,6 +782,12 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\tif (launch_editor(git_path_merge_msg(), NULL, NULL))\n \t\t\tabort_commit(remoteheads, NULL);\n \t}\n+\n+\tif (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),\n+\t\t\t\t\t  \"commit-msg\",\n+\t\t\t\t\t  git_path_merge_msg(), NULL))\n+\t\tabort_commit(remoteheads, NULL);\n+\n \tread_merge_msg(&msg);\n \tstrbuf_stripspace(&msg, 0 < option_edit);\n \tif (!msg.len)\ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex 88d4cda299..1cd54af3cc 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -101,6 +101,10 @@ cat > \"$HOOK\" <<EOF\n exit 1\n EOF\n \n+commit_msg_is () {\n+\ttest \"$(git log --pretty=format:%s%b -1)\" = \"$1\"\n+}\n+\n test_expect_success 'with failing hook' '\n \n \techo \"another\" >> file &&\n@@ -135,6 +139,32 @@ test_expect_success '--no-verify with failing hook (editor)' '\n \n '\n \n+test_expect_success 'merge fails with failing hook' '\n+\n+\ttest_when_finished \"git branch -D newbranch\" &&\n+\ttest_when_finished \"git checkout -f master\" &&\n+\tgit checkout --orphan newbranch &&\n+\t: >file2 &&\n+\tgit add file2 &&\n+\tgit commit --no-verify file2 -m in-side-branch &&\n+\ttest_must_fail git merge --allow-unrelated-histories master &&\n+\tcommit_msg_is \"in-side-branch\" # HEAD before merge\n+\n+'\n+\n+test_expect_success 'merge bypasses failing hook with --no-verify' '\n+\n+\ttest_when_finished \"git branch -D newbranch\" &&\n+\ttest_when_finished \"git checkout -f master\" &&\n+\tgit checkout --orphan newbranch &&\n+\t: >file2 &&\n+\tgit add file2 &&\n+\tgit commit --no-verify file2 -m in-side-branch &&\n+\tgit merge --no-verify --allow-unrelated-histories master &&\n+\tcommit_msg_is \"Merge branch '\\''master'\\'' into newbranch\"\n+'\n+\n+\n chmod -x \"$HOOK\"\n test_expect_success POSIXPERM 'with non-executable hook' '\n \n@@ -178,10 +208,6 @@ exit 0\n EOF\n chmod +x \"$HOOK\"\n \n-commit_msg_is () {\n-\ttest \"$(git log --pretty=format:%s%b -1)\" = \"$1\"\n-}\n-\n test_expect_success 'hook edits commit message' '\n \n \techo \"additional\" >> file &&\n@@ -217,7 +243,17 @@ test_expect_success \"hook doesn't edit commit message (editor)\" '\n \techo \"more plus\" > FAKE_MSG &&\n \tGIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git commit --no-verify &&\n \tcommit_msg_is \"more plus\"\n+'\n \n+test_expect_success 'hook called in git-merge picks up commit message' '\n+\ttest_when_finished \"git branch -D newbranch\" &&\n+\ttest_when_finished \"git checkout -f master\" &&\n+\tgit checkout --orphan newbranch &&\n+\t: >file2 &&\n+\tgit add file2 &&\n+\tgit commit --no-verify file2 -m in-side-branch &&\n+\tgit merge --allow-unrelated-histories master &&\n+\tcommit_msg_is \"new message\"\n '\n \n # set up fake editor to replace `pick` by `reword`\n@@ -237,4 +273,5 @@ test_expect_success 'hook is called for reword during `rebase -i`' '\n \n '\n \n+\n test_done\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"327608","messageId":"xmqqd174bzco.fsf@gitster.mtv.corp.google.com","threadId":"46707","inReplyTo":"20170905210116.26343-1-sbeller@google.com","subject":"Re: [PATCH] builtin/merge: honor commit-msg hook for merges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-05T21:38:15Z","receivedAt":"2017-09-05T21:38:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Similar to 65969d43d1 (merge: honor prepare-commit-msg hook, 2011-02-14)\n> merge should also honor the commit-msg hook; the reason is the same as\n> in that commit: When a merge is stopped due to conflicts or --no-commit,\n> the subsequent commit calls the commit-msg hook.  However, it is not\n> called after a clean merge. Fix this inconsistency by invoking the hook\n> after clean merges as well.\n\nThe above reads better without \"; the reason is the same as in that\ncommit\"---\"Similar to\", combined with the clean and concise\nexplanation after the colon you have, sufficiently justifies why\nthis is a good change.  \n\nExcellent job spotting the precedent and making it consistent ;-).\n\n> This change is motivated by Gerrits commit-msg hook to install a change-id\n\ns/Gerrits/Gerrit's/ perhaps?\n\n> trailer into the commit message. Without such a change id, Gerrit refuses\n\nI do not live in Gerrit land and I do not know which one is the more\npreferred one, but be consistent between \"change-id\" and \"change\nid\".\n\n> to accept (merge) commits by default, such that the inconsistency of\n> (not) running commit-msg hook between commit and merge leads to confusion\n> and might block people from getting their work done.\n\nYup.  Nicely explained.\n\nI didn't check how \"merge --continue\" is implemented, but we need to\nbehave exactly the same way over there, too.  Making sure that it is\na case in t7504 may be a good idea, in addition to the test you\nadded.\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  builtin/merge.c            |  8 ++++++++\n>  t/t7504-commit-msg-hook.sh | 45 +++++++++++++++++++++++++++++++++++++++++----\n>  2 files changed, 49 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 7df3fe3927..087efd560d 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -73,6 +73,7 @@ static int show_progress = -1;\n>  static int default_to_upstream = 1;\n>  static int signoff;\n>  static const char *sign_commit;\n> +static int no_verify;\n>  \n>  static struct strategy all_strategy[] = {\n>  \t{ \"recursive\",  DEFAULT_TWOHEAD | NO_TRIVIAL },\n> @@ -236,6 +237,7 @@ static struct option builtin_merge_options[] = {\n>  \t  N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n>  \tOPT_BOOL(0, \"overwrite-ignore\", &overwrite_ignore, N_(\"update ignored files (default)\")),\n>  \tOPT_BOOL(0, \"signoff\", &signoff, N_(\"add Signed-off-by:\")),\n> +\tOPT_BOOL(0, \"no-verify\", &no_verify, N_(\"bypass pre-commit and commit-msg hooks\")),\n\nThis allows \"--no-no-verify\", which may want to be eventually\naddressed (either by changing the code not to accept, or declaring\nthat it is an intended behaviour); I do not offhand know for sure but I\nstrong suspect \"commit\" shares the same issue, in which case this\npatch is perfectly fine and addressing \"--no-no-verify\" should be\ndone for both of them in a separate follow-up topic.  #leftoverbits\n\nThanks.  I'll be online starting today, but please expect slow\nresponses for a few days as there is significant backlog.\n\n"},{"id":"327616","messageId":"20170905230845.17108-1-sbeller@google.com","threadId":"46707","inReplyTo":"xmqqd174bzco.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] parse-options: warn developers on negated options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-05T23:08:45Z","receivedAt":"2017-09-05T23:08:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When compiling with -DDEVELOPER set (enabled via the Makefile DEVELOPER\nswitch), inspect options if they are negated and warn about them.\n\n 1. Negated options are usually are problem down the maintenance road:\n    When a new negated option (\"--no-foo\") is introduced, then the option\n    can be negated at run time(\"--no-no-foo\"), which is usually confusing\n    for boolean options.\n 2. The handling of negated values (specifically double negations), usually\n    require more brain power to get it right. In code that we own, we\n    should strive to avoid double negatives (\"!no_foo\").\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n  #leftoverbits\n  \n  I was wondering if such a patch may be of value eventually (after all\n  occurrences of \"no-\" options are fixed).\n  This patch disallows all no- options, but we could be more open and allow\n  --no-options that have the NO_NEG bit set.\n\n Makefile        | 3 ++-\n parse-options.c | 6 ++++++\n parse-options.h | 7 +++++--\n 3 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex aba0f5a0f9..9b87093f99 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -433,7 +433,8 @@ DEVELOPER_CFLAGS = -Werror \\\n \t-Wpointer-arith \\\n \t-Wstrict-prototypes \\\n \t-Wunused \\\n-\t-Wvla\n+\t-Wvla \\\n+\t-DDEVELOPER\n LDFLAGS =\n ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\ndiff --git a/parse-options.c b/parse-options.c\nindex 0dd9fc6a0d..08db926adf 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -403,6 +403,12 @@ static void parse_options_check(const struct option *opts)\n \t\tif (opts->argh &&\n \t\t    strcspn(opts->argh, \" _\") != strlen(opts->argh))\n \t\t\terr |= optbug(opts, \"multi-word argh should use dash to separate words\");\n+#if DEVELOPER\n+\t\tif ((opts->flags & PARSE_OPT_ERR_NEGATED) &&\n+\t\t    !strcmp(\"no-\", opts->long_name))\n+\t\t\tBUG(\"Get %s fixed! double negation possible\",\n+\t\t\t    opts->long_name);\n+#endif\n \t}\n \tif (err)\n \t\texit(128);\ndiff --git a/parse-options.h b/parse-options.h\nindex af711227ae..9dc07fd9bb 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -38,7 +38,8 @@ enum parse_opt_option_flags {\n \tPARSE_OPT_LASTARG_DEFAULT = 16,\n \tPARSE_OPT_NODASH = 32,\n \tPARSE_OPT_LITERAL_ARGHELP = 64,\n-\tPARSE_OPT_SHELL_EVAL = 256\n+\tPARSE_OPT_SHELL_EVAL = 256,\n+\tPARSE_OPT_ERR_NEGATED = 512\n };\n \n struct option;\n@@ -124,7 +125,9 @@ struct option {\n \t\t\t\t      (h), PARSE_OPT_NOARG }\n #define OPT_SET_INT(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \\\n \t\t\t\t      (h), PARSE_OPT_NOARG, NULL, (i) }\n-#define OPT_BOOL(s, l, v, h)        OPT_SET_INT(s, l, v, h, 1)\n+#define OPT_BOOL(s, l, v, h)        { OPTION_SET_INT, (s), (l), (v), NULL, \\\n+\t\t\t\t      (h), PARSE_OPT_NOARG|PARSE_OPT_ERR_NEGATED, \\\n+\t\t\t\t      NULL, 1 }\n #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \\\n \t\t\t\t      (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}\n #define OPT_CMDMODE(s, l, v, h, i)  { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"327617","messageId":"20170905232953.22330-1-sbeller@google.com","threadId":"46707","inReplyTo":"xmqqd174bzco.fsf@gitster.mtv.corp.google.com","subject":"[PATCHv2] builtin/merge: honor commit-msg hook for merges","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-05T23:29:53Z","receivedAt":"2017-09-05T23:30:01Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Similar to 65969d43d1 (merge: honor prepare-commit-msg hook, 2011-02-14)\nmerge should also honor the commit-msg hook: When a merge is stopped due\nto conflicts or --no-commit, the subsequent commit calls the commit-msg\nhook.  However, it is not called after a clean merge. Fix this\ninconsistency by invoking the hook after clean merges as well.\n\nThis change is motivated by Gerrit's commit-msg hook to install a ChangeId\ntrailer into the commit message. Without such a ChangeId, Gerrit refuses\nto accept any commit by default, such that the inconsistency of (not)\nrunning the commit-msg hook between commit and merge leads to confusion\nand might block people from getting their work done.\n\nAs the githooks man page is very vocal about the possibility of skipping\nthe commit-msg hook via the --no-verify option, implement the option\nin merge, too.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\naddressed all but one issues.\n\nJunio writes:\n> I didn't check how \"merge --continue\" is implemented, but we need to\n> behave exactly the same way over there, too.  Making sure that it is\n> a case in t7504 may be a good idea, in addition to the test you\n> added.\n\nAfter inspection of the code I do not think it is a good idea, because\n(a) it clutters the test suite with something \"obvious\" for now,\n    the call to cmd_commit will be the same as git-commit on the\n    command line and\n(b) piping through --[no-]verify would either introduce irregularities\n    (\"Why do we pipe through --no-verify, when --sign-off is more important?\")\n    or miss important options to pipe through: \n\n\tstatic int continue_current_merge;\n...\n\tOPT_BOOL(0, \"continue\", &continue_current_merge,\n\t\tN_(\"continue the current in-progress merge\")),\n...\n\tif (continue_current_merge) {\n\t\tint nargc = 1;\n\t\tconst char *nargv[] = {\"commit\", NULL};\n\n\t\tif (orig_argc != 2)\n\t\t\tusage_msg_opt(_(\"--continue expects no arguments\"),\n\t\t\t      builtin_merge_usage, builtin_merge_options);\n\n\t\t/* Invoke 'git commit' */\n\t\tret = cmd_commit(nargc, nargv, prefix);\n\t\tgoto done;\n\t}\n\nThanks,\nStefan\n\n builtin/merge.c            |  8 ++++++++\n t/t7504-commit-msg-hook.sh | 45 +++++++++++++++++++++++++++++++++++++++++----\n 2 files changed, 49 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 7df3fe3927..780435d7a1 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -73,6 +73,7 @@ static int show_progress = -1;\n static int default_to_upstream = 1;\n static int signoff;\n static const char *sign_commit;\n+static int verify_msg = 1;\n \n static struct strategy all_strategy[] = {\n \t{ \"recursive\",  DEFAULT_TWOHEAD | NO_TRIVIAL },\n@@ -236,6 +237,7 @@ static struct option builtin_merge_options[] = {\n \t  N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n \tOPT_BOOL(0, \"overwrite-ignore\", &overwrite_ignore, N_(\"update ignored files (default)\")),\n \tOPT_BOOL(0, \"signoff\", &signoff, N_(\"add Signed-off-by:\")),\n+\tOPT_BOOL(0, \"verify\", &verify_msg, N_(\"verify commit-msg hook\")),\n \tOPT_END()\n };\n \n@@ -780,6 +782,12 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\tif (launch_editor(git_path_merge_msg(), NULL, NULL))\n \t\t\tabort_commit(remoteheads, NULL);\n \t}\n+\n+\tif (verify_msg && run_commit_hook(0 < option_edit, get_index_file(),\n+\t\t\t\t\t  \"commit-msg\",\n+\t\t\t\t\t  git_path_merge_msg(), NULL))\n+\t\tabort_commit(remoteheads, NULL);\n+\n \tread_merge_msg(&msg);\n \tstrbuf_stripspace(&msg, 0 < option_edit);\n \tif (!msg.len)\ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex 88d4cda299..1cd54af3cc 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -101,6 +101,10 @@ cat > \"$HOOK\" <<EOF\n exit 1\n EOF\n \n+commit_msg_is () {\n+\ttest \"$(git log --pretty=format:%s%b -1)\" = \"$1\"\n+}\n+\n test_expect_success 'with failing hook' '\n \n \techo \"another\" >> file &&\n@@ -135,6 +139,32 @@ test_expect_success '--no-verify with failing hook (editor)' '\n \n '\n \n+test_expect_success 'merge fails with failing hook' '\n+\n+\ttest_when_finished \"git branch -D newbranch\" &&\n+\ttest_when_finished \"git checkout -f master\" &&\n+\tgit checkout --orphan newbranch &&\n+\t: >file2 &&\n+\tgit add file2 &&\n+\tgit commit --no-verify file2 -m in-side-branch &&\n+\ttest_must_fail git merge --allow-unrelated-histories master &&\n+\tcommit_msg_is \"in-side-branch\" # HEAD before merge\n+\n+'\n+\n+test_expect_success 'merge bypasses failing hook with --no-verify' '\n+\n+\ttest_when_finished \"git branch -D newbranch\" &&\n+\ttest_when_finished \"git checkout -f master\" &&\n+\tgit checkout --orphan newbranch &&\n+\t: >file2 &&\n+\tgit add file2 &&\n+\tgit commit --no-verify file2 -m in-side-branch &&\n+\tgit merge --no-verify --allow-unrelated-histories master &&\n+\tcommit_msg_is \"Merge branch '\\''master'\\'' into newbranch\"\n+'\n+\n+\n chmod -x \"$HOOK\"\n test_expect_success POSIXPERM 'with non-executable hook' '\n \n@@ -178,10 +208,6 @@ exit 0\n EOF\n chmod +x \"$HOOK\"\n \n-commit_msg_is () {\n-\ttest \"$(git log --pretty=format:%s%b -1)\" = \"$1\"\n-}\n-\n test_expect_success 'hook edits commit message' '\n \n \techo \"additional\" >> file &&\n@@ -217,7 +243,17 @@ test_expect_success \"hook doesn't edit commit message (editor)\" '\n \techo \"more plus\" > FAKE_MSG &&\n \tGIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git commit --no-verify &&\n \tcommit_msg_is \"more plus\"\n+'\n \n+test_expect_success 'hook called in git-merge picks up commit message' '\n+\ttest_when_finished \"git branch -D newbranch\" &&\n+\ttest_when_finished \"git checkout -f master\" &&\n+\tgit checkout --orphan newbranch &&\n+\t: >file2 &&\n+\tgit add file2 &&\n+\tgit commit --no-verify file2 -m in-side-branch &&\n+\tgit merge --allow-unrelated-histories master &&\n+\tcommit_msg_is \"new message\"\n '\n \n # set up fake editor to replace `pick` by `reword`\n@@ -237,4 +273,5 @@ test_expect_success 'hook is called for reword during `rebase -i`' '\n \n '\n \n+\n test_done\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"327625","messageId":"xmqq8thsa901.fsf@gitster.mtv.corp.google.com","threadId":"46707","inReplyTo":"20170905230845.17108-1-sbeller@google.com","subject":"Re: [PATCH] parse-options: warn developers on negated options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-06T01:52:46Z","receivedAt":"2017-09-06T01:52:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>   This patch disallows all no- options, but we could be more open and allow\n>   --no-options that have the NO_NEG bit set.\n\n\"--no-foo\" that does not take \"--foo\" is perhaps OK so should not\ntrigger an error.\n\nA (\"--no-foo\", \"--foo\") pair is better spelled as (\"--foo\",\n\"--no-foo\") pair whose default is \"--foo\", but making it an error is\nprobably a bit too much.\n\nCompared to that, (\"--no-foo\", \"--no-no-foo\") pair feels nonsense.\n\nHaving said that, because the existing parse_options_check() is all\nabout catching the programming mistake (the end user cannot fix an\nerror from it by tweaking the command line option s/he gives to the\nprogram), I do not think a conditional compilation like you added\nmixes well.  Either make the whole thing, not just your new test,\nconditional to -DDEVELOPER (which would make it possible for you to\nbuild and ship a binary with broken options[] array to the end-users\nthat does not die in this function), which is undesirable, or add a\nnew test that catches a definite error unconditionally.\n\n"},{"id":"327626","messageId":"xmqq4lsga8s7.fsf@gitster.mtv.corp.google.com","threadId":"46707","inReplyTo":"20170905232953.22330-1-sbeller@google.com","subject":"Re: [PATCHv2] builtin/merge: honor commit-msg hook for merges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-06T01:57:28Z","receivedAt":"2017-09-06T01:57:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Junio writes:\n>> I didn't check how \"merge --continue\" is implemented, but we need to\n>> behave exactly the same way over there, too.  Making sure that it is\n>> a case in t7504 may be a good idea, in addition to the test you\n>> added.\n>\n> After inspection of the code I do not think it is a good idea, because\n> (a) it clutters the test suite with something \"obvious\" for now,\n>     the call to cmd_commit will be the same as git-commit on the\n>     command line and\n> (b) piping through --[no-]verify would either introduce irregularities\n>     (\"Why do we pipe through --no-verify, when --sign-off is more important?\")\n>     or miss important options to pipe through: \n>\n> \tstatic int continue_current_merge;\n> ...\n> \tOPT_BOOL(0, \"continue\", &continue_current_merge,\n> \t\tN_(\"continue the current in-progress merge\")),\n> ...\n> \tif (continue_current_merge) {\n> \t\tint nargc = 1;\n> \t\tconst char *nargv[] = {\"commit\", NULL};\n>\n> \t\tif (orig_argc != 2)\n> \t\t\tusage_msg_opt(_(\"--continue expects no arguments\"),\n> \t\t\t      builtin_merge_usage, builtin_merge_options);\n>\n> \t\t/* Invoke 'git commit' */\n> \t\tret = cmd_commit(nargc, nargv, prefix);\n> \t\tgoto done;\n> \t}\n\nThat line of thought is backwards.  'something \"obvious\" for now'\ntalks about the present.  tests are all about future-proofing.\n\nI also thought that we were hunting calls of cmd_foo() from outside\nthe git.c command dispatcher as grave errors and want to clean up\nthe codebase to get rid of them.  So the above is the worst example\nto use when you are trying to convince why it needs no test---the\nabove is a good example of the code that would need to change soon\nwhen we have enough volunteers willing to keep the codebase clean\nand healthy, and we would benefit from future-proofing tests.\n"},{"id":"327627","messageId":"xmqqzia88qkj.fsf@gitster.mtv.corp.google.com","threadId":"46707","inReplyTo":"xmqq8thsa901.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] parse-options: warn developers on negated options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-06T03:16:12Z","receivedAt":"2017-09-06T03:16:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Stefan Beller <sbeller@google.com> writes:\n>\n>>   This patch disallows all no- options, but we could be more open and allow\n>>   --no-options that have the NO_NEG bit set.\n>\n> \"--no-foo\" that does not take \"--foo\" is perhaps OK so should not\n> trigger an error.\n>\n> A (\"--no-foo\", \"--foo\") pair is better spelled as (\"--foo\",\n> \"--no-foo\") pair whose default is \"--foo\", but making it an error is\n> probably a bit too much.\n>\n> Compared to that, (\"--no-foo\", \"--no-no-foo\") pair feels nonsense.\n\nAhh, I was an idiot (call it vacation-induced-brain-disfunction).  I\nforgot about 0f1930c5 (\"parse-options: allow positivation of options\nstarting, with no-\", 2012-02-25), which may have already made your\nnew use of \"--no-verify\" in builtin/merge.c and existing one in\ncommit.c OK long time ago.  A quick check to see how your version of\n\n\tgit merge --verify\n\tgit merge --no-verify\n\nbehaves with respect to the commit-msg hook is veriy much\nappreciated, as my tree is in no shape to apply and try a patch\nwhile trying to absorb the patches sent to the list the past week.\n\nThanks, and sorry for a possible false alarm.\n\n> Having said that, because the existing parse_options_check() is all\n> about catching the programming mistake (the end user cannot fix an\n> error from it by tweaking the command line option s/he gives to the\n> program), I do not think a conditional compilation like you added\n> mixes well.  Either make the whole thing, not just your new test,\n> conditional to -DDEVELOPER (which would make it possible for you to\n> build and ship a binary with broken options[] array to the end-users\n> that does not die in this function), which is undesirable, or add a\n> new test that catches a definite error unconditionally.\n\nThis part still is valid.  If René's work 2 years ago is sufficient\nto address \"--no-foo\" thing, then there is nothing we need to add to\nthis test, but if we later need to add new sanity check, we should\nadd it without -DDEVELOPER, or we should make the whole thing inside\nit.\n\nThanks.\n"},{"id":"327675","messageId":"CAGZ79kaHHXdFMeEMfidWj7=FurR3L8VMysLrCLG3OyMtZPCgDQ@mail.gmail.com","threadId":"46707","inReplyTo":"xmqqzia88qkj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] parse-options: warn developers on negated options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-06T21:36:00Z","receivedAt":"2017-09-06T21:36:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 5, 2017 at 8:16 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>>   This patch disallows all no- options, but we could be more open and allow\n>>>   --no-options that have the NO_NEG bit set.\n>>\n>> \"--no-foo\" that does not take \"--foo\" is perhaps OK so should not\n>> trigger an error.\n>>\n>> A (\"--no-foo\", \"--foo\") pair is better spelled as (\"--foo\",\n>> \"--no-foo\") pair whose default is \"--foo\", but making it an error is\n>> probably a bit too much.\n>>\n>> Compared to that, (\"--no-foo\", \"--no-no-foo\") pair feels nonsense.\n>\n> Ahh, I was an idiot (call it vacation-induced-brain-disfunction).  I\n> forgot about 0f1930c5 (\"parse-options: allow positivation of options\n> starting, with no-\", 2012-02-25), which may have already made your\n> new use of \"--no-verify\" in builtin/merge.c and existing one in\n> commit.c OK long time ago.  A quick check to see how your version of\n>\n>         git merge --verify\n>         git merge --no-verify\n>\n> behaves with respect to the commit-msg hook is veriy much\n> appreciated, as my tree is in no shape to apply and try a patch\n> while trying to absorb the patches sent to the list the past week.\n>\n> Thanks, and sorry for a possible false alarm.\n>\n>> Having said that, because the existing parse_options_check() is all\n>> about catching the programming mistake (the end user cannot fix an\n>> error from it by tweaking the command line option s/he gives to the\n>> program), I do not think a conditional compilation like you added\n>> mixes well.  Either make the whole thing, not just your new test,\n>> conditional to -DDEVELOPER (which would make it possible for you to\n>> build and ship a binary with broken options[] array to the end-users\n>> that does not die in this function), which is undesirable, or add a\n>> new test that catches a definite error unconditionally.\n>\n> This part still is valid.  If René's work 2 years ago is sufficient\n> to address \"--no-foo\" thing, then there is nothing we need to add to\n> this test, but if we later need to add new sanity check, we should\n> add it without -DDEVELOPER, or we should make the whole thing inside\n> it.\n\nAs far as the code is concerned it is only inside the -DDEVELOPER ?\nThe intent of this patch is to have a developers aid to remind them\nthat too many negations might be a sign of trouble.\n\nThanks,\nStefan\n\n>\n> Thanks.\n"},{"id":"327677","messageId":"CAGZ79kauB+AznEOwmNBQLbuz-szD3kYBHAJDmy0OWWEbxny9xQ@mail.gmail.com","threadId":"46707","inReplyTo":"xmqq4lsga8s7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2] builtin/merge: honor commit-msg hook for merges","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-06T22:11:23Z","receivedAt":"2017-09-06T22:11:32Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 5, 2017 at 6:57 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Junio writes:\n>>> I didn't check how \"merge --continue\" is implemented, but we need to\n>>> behave exactly the same way over there, too.  Making sure that it is\n>>> a case in t7504 may be a good idea, in addition to the test you\n>>> added.\n>>\n>> After inspection of the code I do not think it is a good idea, because\n>> (a) it clutters the test suite with something \"obvious\" for now,\n>>     the call to cmd_commit will be the same as git-commit on the\n>>     command line and\n>> (b) piping through --[no-]verify would either introduce irregularities\n>>     (\"Why do we pipe through --no-verify, when --sign-off is more important?\")\n>>     or miss important options to pipe through:\n>>\n>>       static int continue_current_merge;\n>> ...\n>>       OPT_BOOL(0, \"continue\", &continue_current_merge,\n>>               N_(\"continue the current in-progress merge\")),\n>> ...\n>>       if (continue_current_merge) {\n>>               int nargc = 1;\n>>               const char *nargv[] = {\"commit\", NULL};\n>>\n>>               if (orig_argc != 2)\n>>                       usage_msg_opt(_(\"--continue expects no arguments\"),\n>>                             builtin_merge_usage, builtin_merge_options);\n>>\n>>               /* Invoke 'git commit' */\n>>               ret = cmd_commit(nargc, nargv, prefix);\n>>               goto done;\n>>       }\n>\n> That line of thought is backwards.  'something \"obvious\" for now'\n> talks about the present.  tests are all about future-proofing.\n\nI agree, but I did not think a call to cmd_commit would need to\nbe future-proofed as we already test git-commit, and these\nare equal....\n\n>\n> I also thought that we were hunting calls of cmd_foo() from outside\n> the git.c command dispatcher as grave errors and want to clean up\n> the codebase to get rid of them.\n\n... but I did not account for this fact. (I was not aware of these being\ncalled grave errors, but assumed this is a good state. And why change\na good state?)\n\n> So the above is the worst example\n> to use when you are trying to convince why it needs no test---the\n> above is a good example of the code that would need to change soon\n> when we have enough volunteers willing to keep the codebase clean\n> and healthy, and we would benefit from future-proofing tests.\n\nGiven that new fact, I agree with the reasoning to add a new test\nfor future proofing. In the current form\n\n    git merge --continue --no-verify\n\nwould trigger to usage_msg_opt(..), so all I'd offer is a test_must_fail\nfor now?\n"},{"id":"327679","messageId":"xmqqvakv2y5v.fsf@gitster.mtv.corp.google.com","threadId":"46707","inReplyTo":"CAGZ79kaHHXdFMeEMfidWj7=FurR3L8VMysLrCLG3OyMtZPCgDQ@mail.gmail.com","subject":"Re: [PATCH] parse-options: warn developers on negated options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-06T23:41:00Z","receivedAt":"2017-09-06T23:41:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> Ahh, I was an idiot (call it vacation-induced-brain-disfunction).  I\n>> forgot about 0f1930c5 (\"parse-options: allow positivation of options\n>> starting, with no-\", 2012-02-25), which may have already made your\n>> new use of \"--no-verify\" in builtin/merge.c and existing one in\n>> commit.c OK long time ago.  A quick check to see how your version of\n>>\n>>         git merge --verify\n>>         git merge --no-verify\n>>\n>> behaves with respect to the commit-msg hook is veriy much\n>> appreciated, as my tree is in no shape to apply and try a patch\n>> while trying to absorb the patches sent to the list the past week.\n>>\n>> Thanks, and sorry for a possible false alarm.\n>>\n>>> Having said that, because the existing parse_options_check() is all\n>>> about catching the programming mistake (the end user cannot fix an\n>>> error from it by tweaking the command line option s/he gives to the\n>>> program), I do not think a conditional compilation like you added\n>>> mixes well.  Either make the whole thing, not just your new test,\n>>> conditional to -DDEVELOPER (which would make it possible for you to\n>>> build and ship a binary with broken options[] array to the end-users\n>>> that does not die in this function), which is undesirable, or add a\n>>> new test that catches a definite error unconditionally.\n>>\n>> This part still is valid.  If René's work 2 years ago is sufficient\n>> to address \"--no-foo\" thing, then there is nothing we need to add to\n>> this test, but if we later need to add new sanity check, we should\n>> add it without -DDEVELOPER, or we should make the whole thing inside\n>> it.\n>\n> As far as the code is concerned it is only inside the -DDEVELOPER ?\n> The intent of this patch is to have a developers aid to remind them\n> that too many negations might be a sign of trouble.\n\nI understand that.  What I was saying is that there may be no point\n\"reminding\" them with René's \"positivation\" thing in effect and that\nis why I asked you to try the simple two commands out to see if that\nis the case.\n\nI did that myself with \"git commit --[no-]verify\" and they are\nindeed OK, so there is no reason to force developers to do this:\n\n\tint distim = 1; /* default is to distim */\n\tstruct option options[] = {\n\t\t...\n\t\tOPT_BOOL(0, \"distim\", &distim, N_(\"distim\")),\n\t\t...\n\t};\n\t...\n\tif (distim)\n\t\tdo_the_distim_thing();\n\nif/when the following is more natural in the context of the command:\n\n\tint no_distim = 1; /* default is to distim */\n\tstruct option options[] = {\n\t\t...\n\t\tOPT_BOOL(0, \"no-distim\", &distim, N_(\"bypass distimming\")),\n\t\t...\n\t};\n\t...\n\tif (distim)\n\t\tdo_the_distim_thing();\n\nwhether it is inside -DDEVELOPER or not.\n\n\n"},{"id":"327680","messageId":"xmqqr2vj2y1b.fsf@gitster.mtv.corp.google.com","threadId":"46707","inReplyTo":"CAGZ79kauB+AznEOwmNBQLbuz-szD3kYBHAJDmy0OWWEbxny9xQ@mail.gmail.com","subject":"Re: [PATCHv2] builtin/merge: honor commit-msg hook for merges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-06T23:43:44Z","receivedAt":"2017-09-06T23:44:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> I also thought that we were hunting calls of cmd_foo() from outside\n>> the git.c command dispatcher as grave errors and want to clean up\n>> the codebase to get rid of them.\n>\n> ... but I did not account for this fact. (I was not aware of these being\n> called grave errors, but assumed this is a good state. And why change\n> a good state?)\n\nhttps://public-inbox.org/git/20170830053108.g2xsn43rwulnwn3p@sigill.intra.peff.net/\n\ngives a good explanation why it is not a good state.\n"},{"id":"327734","messageId":"20170907220429.31312-1-sbeller@google.com","threadId":"46707","inReplyTo":"xmqqd174bzco.fsf@gitster.mtv.corp.google.com","subject":"[PATCHv3] builtin/merge: honor commit-msg hook for merges","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-07T22:04:29Z","receivedAt":"2017-09-07T22:04:42Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Similar to 65969d43d1 (merge: honor prepare-commit-msg hook, 2011-02-14)\nmerge should also honor the commit-msg hook: When a merge is stopped due\nto conflicts or --no-commit, the subsequent commit calls the commit-msg\nhook.  However, it is not called after a clean merge. Fix this\ninconsistency by invoking the hook after clean merges as well.\n\nThis change is motivated by Gerrit's commit-msg hook to install a ChangeId\ntrailer into the commit message. Without such a ChangeId, Gerrit refuses\nto accept any commit by default, such that the inconsistency of (not)\nrunning the commit-msg hook between commit and merge leads to confusion\nand might block people from getting their work done.\n\nAs the githooks man page is very vocal about the possibility of skipping\nthe commit-msg hook via the --no-verify option, implement the option\nin merge, too.\n\n'git merge --continue' is currently implemented as calling cmd_commit\nwith no further arguments. This works for most other merge related options,\nsuch as demonstrated via the --allow-unrelated-histories flag in the\ntest. The --no-verify option however is not remembered across invocations\nof git-merge. Originally the author assumed an alternative in which the\n'git merge --continue' command accepts the --no-verify flag, but that\nopens up the discussion which flags are allows to the continued merge\ncommand and which must be given in the first invocation.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n> I didn't check how \"merge --continue\" is implemented, but we need to\n> behave exactly the same way over there, too.  Making sure that it is\n> a case in t7504 may be a good idea, in addition to the test you\n> added.\n\nFirst I understood this as if we'd want to support\n'git merge --continue --no-verify' eventually, but by now I think we want\nto 'carry over' the meaning from the first invocation of git-merge.\n\nFor that I added a test.\n\nThanks,\nStefan\n\n builtin/merge.c            |  8 ++++++\n t/t7504-commit-msg-hook.sh | 64 +++++++++++++++++++++++++++++++++++++++++++---\n 2 files changed, 68 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 7df3fe3927..780435d7a1 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -73,6 +73,7 @@ static int show_progress = -1;\n static int default_to_upstream = 1;\n static int signoff;\n static const char *sign_commit;\n+static int verify_msg = 1;\n \n static struct strategy all_strategy[] = {\n \t{ \"recursive\",  DEFAULT_TWOHEAD | NO_TRIVIAL },\n@@ -236,6 +237,7 @@ static struct option builtin_merge_options[] = {\n \t  N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n \tOPT_BOOL(0, \"overwrite-ignore\", &overwrite_ignore, N_(\"update ignored files (default)\")),\n \tOPT_BOOL(0, \"signoff\", &signoff, N_(\"add Signed-off-by:\")),\n+\tOPT_BOOL(0, \"verify\", &verify_msg, N_(\"verify commit-msg hook\")),\n \tOPT_END()\n };\n \n@@ -780,6 +782,12 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\tif (launch_editor(git_path_merge_msg(), NULL, NULL))\n \t\t\tabort_commit(remoteheads, NULL);\n \t}\n+\n+\tif (verify_msg && run_commit_hook(0 < option_edit, get_index_file(),\n+\t\t\t\t\t  \"commit-msg\",\n+\t\t\t\t\t  git_path_merge_msg(), NULL))\n+\t\tabort_commit(remoteheads, NULL);\n+\n \tread_merge_msg(&msg);\n \tstrbuf_stripspace(&msg, 0 < option_edit);\n \tif (!msg.len)\ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex 88d4cda299..302a3a2082 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -101,6 +101,10 @@ cat > \"$HOOK\" <<EOF\n exit 1\n EOF\n \n+commit_msg_is () {\n+\ttest \"$(git log --pretty=format:%s%b -1)\" = \"$1\"\n+}\n+\n test_expect_success 'with failing hook' '\n \n \techo \"another\" >> file &&\n@@ -135,6 +139,32 @@ test_expect_success '--no-verify with failing hook (editor)' '\n \n '\n \n+test_expect_success 'merge fails with failing hook' '\n+\n+\ttest_when_finished \"git branch -D newbranch\" &&\n+\ttest_when_finished \"git checkout -f master\" &&\n+\tgit checkout --orphan newbranch &&\n+\t: >file2 &&\n+\tgit add file2 &&\n+\tgit commit --no-verify file2 -m in-side-branch &&\n+\ttest_must_fail git merge --allow-unrelated-histories master &&\n+\tcommit_msg_is \"in-side-branch\" # HEAD before merge\n+\n+'\n+\n+test_expect_success 'merge bypasses failing hook with --no-verify' '\n+\n+\ttest_when_finished \"git branch -D newbranch\" &&\n+\ttest_when_finished \"git checkout -f master\" &&\n+\tgit checkout --orphan newbranch &&\n+\t: >file2 &&\n+\tgit add file2 &&\n+\tgit commit --no-verify file2 -m in-side-branch &&\n+\tgit merge --no-verify --allow-unrelated-histories master &&\n+\tcommit_msg_is \"Merge branch '\\''master'\\'' into newbranch\"\n+'\n+\n+\n chmod -x \"$HOOK\"\n test_expect_success POSIXPERM 'with non-executable hook' '\n \n@@ -178,10 +208,6 @@ exit 0\n EOF\n chmod +x \"$HOOK\"\n \n-commit_msg_is () {\n-\ttest \"$(git log --pretty=format:%s%b -1)\" = \"$1\"\n-}\n-\n test_expect_success 'hook edits commit message' '\n \n \techo \"additional\" >> file &&\n@@ -217,7 +243,36 @@ test_expect_success \"hook doesn't edit commit message (editor)\" '\n \techo \"more plus\" > FAKE_MSG &&\n \tGIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git commit --no-verify &&\n \tcommit_msg_is \"more plus\"\n+'\n \n+test_expect_success 'hook called in git-merge picks up commit message' '\n+\ttest_when_finished \"git branch -D newbranch\" &&\n+\ttest_when_finished \"git checkout -f master\" &&\n+\tgit checkout --orphan newbranch &&\n+\t: >file2 &&\n+\tgit add file2 &&\n+\tgit commit --no-verify file2 -m in-side-branch &&\n+\tgit merge --allow-unrelated-histories master &&\n+\tcommit_msg_is \"new message\"\n+'\n+\n+test_expect_failure 'merge --continue remembers --no-verify' '\n+\ttest_when_finished \"git branch -D newbranch\" &&\n+\ttest_when_finished \"git checkout -f master\" &&\n+\tgit checkout master &&\n+\techo a >file2 &&\n+\tgit add file2 &&\n+\tgit commit --no-verify -m \"add file2 to master\" &&\n+\tgit checkout -b newbranch master^ &&\n+\techo b >file2 &&\n+\tgit add file2 &&\n+\tgit commit --no-verify file2 -m in-side-branch &&\n+\tgit merge --no-verify -m not-rewritten-by-hook master &&\n+\t# resolve conflict:\n+\techo c >file2 &&\n+\tgit add file2 &&\n+\tgit merge --continue &&\n+\tcommit_msg_is not-rewritten-by-hook\n '\n \n # set up fake editor to replace `pick` by `reword`\n@@ -237,4 +292,5 @@ test_expect_success 'hook is called for reword during `rebase -i`' '\n \n '\n \n+\n test_done\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"327737","messageId":"xmqqr2vi0z7q.fsf@gitster.mtv.corp.google.com","threadId":"46707","inReplyTo":"20170907220429.31312-1-sbeller@google.com","subject":"Re: [PATCHv3] builtin/merge: honor commit-msg hook for merges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-08T01:13:29Z","receivedAt":"2017-09-08T01:13:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> .... The --no-verify option however is not remembered across invocations\n> of git-merge. Originally the author assumed an alternative in which the\n> 'git merge --continue' command accepts the --no-verify flag, but that\n> opens up the discussion which flags are allows to the continued merge\n> command and which must be given in the first invocation.\n\nThis leaves a reader (me) wondering what the final conclusion was,\nafter the author assumed something and thought about alternatives.\nI am guessing that your final decision was not to remember\n\"--no-verify\" so a user who started \"merge --no-verify\" that stopped\nin the middle must say \"merge --continue --no-verify\" or \"commit\n--no-verify\" to conclude the merge?  Or you added some mechanism to\nremember the fact that no-verify was given so that \"merge --continue\"\nwill read from there, ignoring \"merge --continue --verify\" from the\ncommand line?  Not just the above part of the log message confusing,\nbut there is no update to the documentation, and we shouldn't expect\nend-users to find out what ought to happen by reading t7504 X-<.\n\nThe new test in t7504 tells me that you remember --[no-]verify from\nthe initial invocation and use the same when --continue is given; it\nis unclear how that remembered one interacts with --[no-]verify that\nis given when --continue is given.  It is not documented, tested and\nexplained in the log message.  I would expect that the command line \ntrumps what was given in the initial invocation.\n\n\n> +static int verify_msg = 1;\n>  \n>  static struct strategy all_strategy[] = {\n>  \t{ \"recursive\",  DEFAULT_TWOHEAD | NO_TRIVIAL },\n> @@ -236,6 +237,7 @@ static struct option builtin_merge_options[] = {\n>  \t  N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n>  \tOPT_BOOL(0, \"overwrite-ignore\", &overwrite_ignore, N_(\"update ignored files (default)\")),\n>  \tOPT_BOOL(0, \"signoff\", &signoff, N_(\"add Signed-off-by:\")),\n> +\tOPT_BOOL(0, \"verify\", &verify_msg, N_(\"verify commit-msg hook\")),\n>  \tOPT_END()\n>  };\n\nI suspect that the previous iteration gives a much better end-user\nexperience when \"git merge -h\" is used.  This will give the\nimpression that the user MUST say \"merge --verify\" if the user wants\nto verify commit-msg hook (whatever that means), but because the\noption defaults to true, that is not what happens.  The user instead\nmust say \"merge --no-verify\" if the verification is unwanted.\n\n\"git commit -h\" explains \n\n    --no-verify        bypass pre-commit and commit-msg hooks\n\nand I think that is the way how we want to explain this option in\n\"git merge\" too.  Normally it is not bypassed, and the user can ask\nwith \"--no-verify\".  Thanks to René's change in 2012, the option\ndefinition you had in the previous one will make --[no-]verify\naccepted just fine.\n\n> +test_expect_success 'merge fails with failing hook' '\n> + ...\n> +'\n> +\n> +test_expect_success 'merge bypasses failing hook with --no-verify' '\n> + ...\n> +'\n\nBoth look sensible.\n\n> +test_expect_failure 'merge --continue remembers --no-verify' '\n> +\ttest_when_finished \"git branch -D newbranch\" &&\n> +\ttest_when_finished \"git checkout -f master\" &&\n> +\tgit checkout master &&\n> +\techo a >file2 &&\n> +\tgit add file2 &&\n> +\tgit commit --no-verify -m \"add file2 to master\" &&\n> +\tgit checkout -b newbranch master^ &&\n> +\techo b >file2 &&\n> +\tgit add file2 &&\n> +\tgit commit --no-verify file2 -m in-side-branch &&\n> +\tgit merge --no-verify -m not-rewritten-by-hook master &&\n> +\t# resolve conflict:\n> +\techo c >file2 &&\n> +\tgit add file2 &&\n> +\tgit merge --continue &&\n> +\tcommit_msg_is not-rewritten-by-hook\n>  '\n\nOK.  What should happen when the last \"merge --continue\" was given\n\"--verify\" at the same time?  A similar test whose title is\n\"--no-verify remembered by merge --continue can be overriden\" may be\na good thing to follow this one, perhaps?\n\nThanks.\n"},{"id":"327852","messageId":"CAGZ79kZCQq8ts6RAN2Qz5vWtUh7swu0UWO_6LTaRhNF5sDbnDg@mail.gmail.com","threadId":"46707","inReplyTo":"xmqqr2vi0z7q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3] builtin/merge: honor commit-msg hook for merges","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-11T17:12:18Z","receivedAt":"2017-09-11T17:12:26Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 7, 2017 at 6:13 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> .... The --no-verify option however is not remembered across invocations\n>> of git-merge. Originally the author assumed an alternative in which the\n>> 'git merge --continue' command accepts the --no-verify flag, but that\n>> opens up the discussion which flags are allows to the continued merge\n>> command and which must be given in the first invocation.\n>\n> This leaves a reader (me) wondering what the final conclusion was,\n> after the author assumed something and thought about alternatives.\n\nI did not draw a conclusion, I was just saying, that this has to be\nthought about.\nIt looks to me as if --continue currently wants to take no extra arguments,\nbut to remember all flags from the previous invocation of merge.\n\nBut that only looks this way as all the command line flags are related\nfor tree manipulation, not (indirect) commit message manipulation.\n\nSo I think having --no-verify be respected (either by restating, or by\nremembering, or both) by the `git merge --continue` call would be a\nreasonable thing to want.\n\nSo that new patch just added the test as a #needswork for the future,\nnot actually making a decision how to approach it.\n\n> I am guessing that your final decision was not to remember\n> \"--no-verify\" so a user who started \"merge --no-verify\" that stopped\n> in the middle must say \"merge --continue --no-verify\" or \"commit\n> --no-verify\" to conclude the merge?  Or you added some mechanism to\n> remember the fact that no-verify was given so that \"merge --continue\"\n> will read from there, ignoring \"merge --continue --verify\" from the\n> command line?  Not just the above part of the log message confusing,\n> but there is no update to the documentation, and we shouldn't expect\n> end-users to find out what ought to happen by reading t7504 X-<.\n\nInterestingly the documentation that I read to approach the problem\nis already in shape as it would not specify the specific command for the\n'--no-verify' option. I missed that we need to add documentation for\nthe continued merge case.\n\n> The new test in t7504 tells me that you remember --[no-]verify from\n> the initial invocation and use the same when --continue is given; it\n> is unclear how that remembered one interacts with --[no-]verify that\n> is given when --continue is given.  It is not documented, tested and\n> explained in the log message.  I would expect that the command line\n> trumps what was given in the initial invocation.\n\nI would expect that, too.\n\n>\n>\n>> +static int verify_msg = 1;\n>>\n>>  static struct strategy all_strategy[] = {\n>>       { \"recursive\",  DEFAULT_TWOHEAD | NO_TRIVIAL },\n>> @@ -236,6 +237,7 @@ static struct option builtin_merge_options[] = {\n>>         N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n>>       OPT_BOOL(0, \"overwrite-ignore\", &overwrite_ignore, N_(\"update ignored files (default)\")),\n>>       OPT_BOOL(0, \"signoff\", &signoff, N_(\"add Signed-off-by:\")),\n>> +     OPT_BOOL(0, \"verify\", &verify_msg, N_(\"verify commit-msg hook\")),\n>>       OPT_END()\n>>  };\n>\n> I suspect that the previous iteration gives a much better end-user\n> experience when \"git merge -h\" is used.  This will give the\n> impression that the user MUST say \"merge --verify\" if the user wants\n> to verify commit-msg hook (whatever that means), but because the\n> option defaults to true, that is not what happens.  The user instead\n> must say \"merge --no-verify\" if the verification is unwanted.\n\nok, will revert to that in a resend.\n\n>> +test_expect_failure 'merge --continue remembers --no-verify' '\n>> +     test_when_finished \"git branch -D newbranch\" &&\n>> +     test_when_finished \"git checkout -f master\" &&\n>> +     git checkout master &&\n>> +     echo a >file2 &&\n>> +     git add file2 &&\n>> +     git commit --no-verify -m \"add file2 to master\" &&\n>> +     git checkout -b newbranch master^ &&\n>> +     echo b >file2 &&\n>> +     git add file2 &&\n>> +     git commit --no-verify file2 -m in-side-branch &&\n>> +     git merge --no-verify -m not-rewritten-by-hook master &&\n>> +     # resolve conflict:\n>> +     echo c >file2 &&\n>> +     git add file2 &&\n>> +     git merge --continue &&\n>> +     commit_msg_is not-rewritten-by-hook\n>>  '\n>\n> OK.  What should happen when the last \"merge --continue\" was given\n> \"--verify\" at the same time?\n\nCurrently not possible, due to --continue requiring that it is the\nonly argument. In the future where --continue works well with other\narguments, we should override the original.\n\n> A similar test whose title is\n> \"--no-verify remembered by merge --continue can be overriden\" may be\n> a good thing to follow this one, perhaps?\n\nNote that this is already test_expect_failure, which I used to mark\nthat this particular flag is broken across a --continue invocation of git-merge,\nso I would not add yet another test that describes the future yet to be\nimplemented?\n\nThanks.\n"},{"id":"328184","messageId":"1505542931.27598.4.camel@gmail.com","threadId":"46707","inReplyTo":"20170907220429.31312-1-sbeller@google.com","subject":"Re: [PATCHv3] builtin/merge: honor commit-msg hook for merges","fromName":"Kaartic Sivaraam","fromEmail":"kaarticsivaraam91196@gmail.com","sentAt":"2017-09-16T06:22:11Z","receivedAt":"2017-09-16T06:22:21Z","isPatch":false,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"Seems 'Documentation/githooks.txt' needs an update related to this\nchange. Previously it said(note the **s) that 'commit-msg' is invoked\nonly by 'git commit',\n\n    commit-msg\n           This hook is invoked by git commit**, and can be bypassed with the\n           --no-verify option. It takes a single parameter, the name of the file\n           that holds the proposed commit log message. Exiting with a non-zero\n           status causes the git commit** to abort.\n\n---\nKaartic\n"},{"id":"328493","messageId":"CAGZ79kaN4nKwGXq9t+Lv9e4X6qkBUAj4S8gofrNj3VZYw1sv+w@mail.gmail.com","threadId":"46707","inReplyTo":"1505542931.27598.4.camel@gmail.com","subject":"Re: [PATCHv3] builtin/merge: honor commit-msg hook for merges","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-20T19:55:49Z","receivedAt":"2017-09-20T19:55:58Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Sep 15, 2017 at 11:22 PM, Kaartic Sivaraam\n<kaarticsivaraam91196@gmail.com> wrote:\n> Seems 'Documentation/githooks.txt' needs an update related to this\n> change. Previously it said(note the **s) that 'commit-msg' is invoked\n> only by 'git commit',\n>\n>     commit-msg\n>            This hook is invoked by git commit**, and can be bypassed with the\n>            --no-verify option. It takes a single parameter, the name of the file\n>            that holds the proposed commit log message. Exiting with a non-zero\n>            status causes the git commit** to abort.\n\nYes that needs an update. When writing the patch, it read ambivalently[1], such\nthat I decided to not include the update to the man page.\n\n[1] at the time I was reading it as \"when producing a [git] commit\", instead\nof \"the command `git commit`\".\n\nI'll send a patch fixing the docs, though with this thought, maybe we need\nto fix other commands, that produce commits as well?\n(git revert, others?)\n\n\n>\n> ---\n> Kaartic\n"},{"id":"328515","messageId":"xmqqy3p8n9fe.fsf@gitster.mtv.corp.google.com","threadId":"46707","inReplyTo":"CAGZ79kaN4nKwGXq9t+Lv9e4X6qkBUAj4S8gofrNj3VZYw1sv+w@mail.gmail.com","subject":"Re: [PATCHv3] builtin/merge: honor commit-msg hook for merges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-21T01:10:29Z","receivedAt":"2017-09-21T01:10:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> I'll send a patch fixing the docs, though with this thought, maybe we need\n> to fix other commands, that produce commits as well?\n> (git revert, others?)\n\nI do not think \"commands that create commits\" is not a good\ncriteria.  \"git notes\" and \"git stash\" would (internally) create a\ncommit while recording a new change, but you obvously would not want\nthese hooks to kick in.\n\nA command that can stop in the middle and to which running \"git\ncommit\" is how the end-user concludes the operation would be a\ncandidate.  \"git merge X\", \"git cherry-pick A\", and \"git cherry-pick\nA..B\" may be good candidates.\n\nFor \"rebase\" and others that have the \"convenience --continue\"\noption that make a commit before continuing, I would think that we\nshould treat these as invoking \"git commit\".  That is, when these\ncommands stop and you resolve the conflict in the index and in the\nworking tree, the next \"git $cmd --continue\" you type is merely a\nway to let you be lazy.  You'd be typing \"git commit && git $cmd\n--continue\" if you were to refuse the lazy convenience option and\nwant to spell out what you are doing explicitly, and you'd get the\nsame result as you'd get from just \"git $cmd --continue\" if you did\nso.\n\nOn the other hand, \"git am\" is not a candidate.  You never use \"git\ncommit\" to mark that you are done, even if you refuse to use the\nlazy convenience option and spell out what you are doing explicitly.\n"},{"id":"328589","messageId":"20170921202954.7854-1-sbeller@google.com","threadId":"46707","inReplyTo":"1505542931.27598.4.camel@gmail.com","subject":"[PATCH] Documentation/githooks: mention merge in commit-msg hook","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-21T20:29:54Z","receivedAt":"2017-09-21T20:30:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The commit-msg hook is invoked by both commit and merge now.\n\nReported-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/githooks.txt | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 623ed1a138..f4e75b9c90 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -127,11 +127,10 @@ help message found in the commented portion of the commit template.\n commit-msg\n ~~~~~~~~~~\n \n-This hook is invoked by 'git commit', and can be bypassed\n-with the `--no-verify` option.  It takes a single parameter, the\n-name of the file that holds the proposed commit log message.\n-Exiting with a non-zero status causes the 'git commit' to\n-abort.\n+This hook is invoked by 'git commit' and 'git merge', and can be\n+bypassed with the `--no-verify` option.  It takes a single parameter,\n+the name of the file that holds the proposed commit log message.\n+Exiting with a non-zero status causes the command to abort.\n \n The hook is allowed to edit the message file in place, and can be used\n to normalize the message into some project standard format. It\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"328610","messageId":"xmqqr2uzlcjx.fsf@gitster.mtv.corp.google.com","threadId":"46707","inReplyTo":"20170921202954.7854-1-sbeller@google.com","subject":"Re: [PATCH] Documentation/githooks: mention merge in commit-msg hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-22T01:58:10Z","receivedAt":"2017-09-22T01:58:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The commit-msg hook is invoked by both commit and merge now.\n>\n> Reported-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\nThanks for tying the loose end.  Very much appreciated.\n\n>  Documentation/githooks.txt | 9 ++++-----\n>  1 file changed, 4 insertions(+), 5 deletions(-)\n>\n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> index 623ed1a138..f4e75b9c90 100644\n> --- a/Documentation/githooks.txt\n> +++ b/Documentation/githooks.txt\n> @@ -127,11 +127,10 @@ help message found in the commented portion of the commit template.\n>  commit-msg\n>  ~~~~~~~~~~\n>  \n> -This hook is invoked by 'git commit', and can be bypassed\n> -with the `--no-verify` option.  It takes a single parameter, the\n> -name of the file that holds the proposed commit log message.\n> -Exiting with a non-zero status causes the 'git commit' to\n> -abort.\n> +This hook is invoked by 'git commit' and 'git merge', and can be\n> +bypassed with the `--no-verify` option.  It takes a single parameter,\n> +the name of the file that holds the proposed commit log message.\n> +Exiting with a non-zero status causes the command to abort.\n>  \n>  The hook is allowed to edit the message file in place, and can be used\n>  to normalize the message into some project standard format. It\n"}]}