{"thread":{"id":"37479","subject":"[RFC/PATCH 3/3] revert/cherry-pick --no-verify: Update documentation","startedAt":"2014-09-03T14:03:51Z","lastAt":"2014-09-08T15:13:45Z","messageCount":20,"participants":["Johan Herland","Junio C Hamano","René Scharfe","Jonathan Nieder","Fabian Ruch"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"248786","messageId":"1409753034-9459-1-git-send-email-johan@herland.net","threadId":"37479","inReplyTo":null,"subject":"[RFC/PATCH 0/3] Teach revert/cherry-pick the --no-verify option","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2014-09-03T14:03:51Z","receivedAt":"2014-09-03T14:03:51Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"A colleague of mine noticed that cherry-pick does not accept the\n--no-verify option to skip running the pre-commit/commit-msg hooks.\n\nHere's a first attempt at adding --no-verify to the revert/cherry-pick.\n\nHave fun! :)\n\n...Johan\n\nJohan Herland (3):\n  t7503/4: Add failing testcases for revert/cherry-pick --no-verify\n  revert/cherry-pick: Add --no-verify option, and pass it on to commit\n  revert/cherry-pick --no-verify: Update documentation\n\n Documentation/git-cherry-pick.txt |  4 ++++\n Documentation/git-revert.txt      |  4 ++++\n Documentation/githooks.txt        | 20 ++++++++++----------\n builtin/revert.c                  |  1 +\n sequencer.c                       |  7 +++++++\n sequencer.h                       |  1 +\n t/t7503-pre-commit-hook.sh        | 24 ++++++++++++++++++++++++\n t/t7504-commit-msg-hook.sh        | 24 ++++++++++++++++++++++++\n 8 files changed, 75 insertions(+), 10 deletions(-)\n\n-- \n2.0.0.rc4.501.gdaf83ca\n"},{"id":"248785","messageId":"1409753034-9459-2-git-send-email-johan@herland.net","threadId":"37479","inReplyTo":"1409753034-9459-1-git-send-email-johan@herland.net","subject":"[RFC/PATCH 1/3] t7503/4: Add failing testcases for revert/cherry-pick --no-verify","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2014-09-03T14:03:52Z","receivedAt":"2014-09-03T14:03:52Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"The revert/cherry-pick machinery currently exercises the pre-commit\nand commit-msg hooks. However, where commit accepts a --no-verify\noption to temporarily disable these hooks, the revert and cherry-pick\ncommands have no such option.\n\nThis patch adds some testcases demonstrating how the --no-verify\noption is supposed when it is added to revert and cherry-pick\n(in the next patch).\n\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n t/t7503-pre-commit-hook.sh | 24 ++++++++++++++++++++++++\n t/t7504-commit-msg-hook.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 48 insertions(+)\n\ndiff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh\nindex 984889b..adc892b 100755\n--- a/t/t7503-pre-commit-hook.sh\n+++ b/t/t7503-pre-commit-hook.sh\n@@ -60,6 +60,18 @@ test_expect_success 'with failing hook' '\n \n '\n \n+test_expect_success 'revert with failing hook' '\n+\n+\ttest_must_fail git revert HEAD\n+\n+'\n+\n+test_expect_success 'cherry-pick with failing hook' '\n+\n+\ttest_must_fail git cherry-pick --no-verify HEAD^\n+\n+'\n+\n test_expect_success '--no-verify with failing hook' '\n \n \techo \"stuff\" >> file &&\n@@ -68,6 +80,18 @@ test_expect_success '--no-verify with failing hook' '\n \n '\n \n+test_expect_failure 'revert --no-verify with failing hook' '\n+\n+\tgit revert --no-verify HEAD\n+\n+'\n+\n+test_expect_failure 'cherry-pick --no-verify with failing hook' '\n+\n+\tgit cherry-pick --no-verify HEAD^\n+\n+'\n+\n chmod -x \"$HOOK\"\n test_expect_success POSIXPERM 'with non-executable hook' '\n \ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex 1f53ea8..4f8b9fe 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -109,6 +109,18 @@ test_expect_success 'with failing hook' '\n \n '\n \n+test_expect_success 'revert with failing hook' '\n+\n+\ttest_must_fail git revert HEAD\n+\n+'\n+\n+test_expect_success 'cherry-pick with failing hook' '\n+\n+\ttest_must_fail git cherry-pick --no-verify HEAD^\n+\n+'\n+\n test_expect_success 'with failing hook (editor)' '\n \n \techo \"more another\" >> file &&\n@@ -126,6 +138,18 @@ test_expect_success '--no-verify with failing hook' '\n \n '\n \n+test_expect_failure 'revert --no-verify with failing hook' '\n+\n+\tgit revert --no-verify HEAD\n+\n+'\n+\n+test_expect_failure 'cherry-pick --no-verify with failing hook' '\n+\n+\tgit cherry-pick --no-verify HEAD^\n+\n+'\n+\n test_expect_success '--no-verify with failing hook (editor)' '\n \n \techo \"more stuff\" >> file &&\n-- \n2.0.0.rc4.501.gdaf83ca\n"},{"id":"248784","messageId":"1409753034-9459-3-git-send-email-johan@herland.net","threadId":"37479","inReplyTo":"1409753034-9459-1-git-send-email-johan@herland.net","subject":"[RFC/PATCH 2/3] revert/cherry-pick: Add --no-verify option, and pass it on to commit","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2014-09-03T14:03:53Z","receivedAt":"2014-09-03T14:03:53Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Allow users to temporarily disable the pre-commit and commit-msg hooks\nwhen running \"git revert\" or \"git cherry-pick\", just like they currently\ncan for \"git commit\".\n\nThe --no-verify option is added to the sequencer machinery and handled\nlike the other commit-related options.\n\nThis fixes the failing t7503/t7504 test cases added previously.\n\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n builtin/revert.c           | 1 +\n sequencer.c                | 7 +++++++\n sequencer.h                | 1 +\n t/t7503-pre-commit-hook.sh | 4 ++--\n t/t7504-commit-msg-hook.sh | 4 ++--\n 5 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex f9ed5bd..831c2cd 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -91,6 +91,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\tN_(\"option for merge strategy\"), option_parse_x),\n \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n \t\t  N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n+\t\tOPT_BOOL('n', \"no-verify\", &opts->no_verify, N_(\"bypass pre-commit hook\")),\n \t\tOPT_END(),\n \t\tOPT_END(),\n \t\tOPT_END(),\ndiff --git a/sequencer.c b/sequencer.c\nindex 3c060e0..3d68113 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -378,6 +378,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \tif (opts->allow_empty_message)\n \t\targv_array_push(&array, \"--allow-empty-message\");\n \n+\tif (opts->no_verify)\n+\t\targv_array_push(&array, \"--no-verify\");\n+\n \trc = run_command_v_opt(array.argv, RUN_GIT_CMD);\n \targv_array_clear(&array);\n \treturn rc;\n@@ -773,6 +776,8 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \t\topts->record_origin = git_config_bool_or_int(key, value, &error_flag);\n \telse if (!strcmp(key, \"options.allow-ff\"))\n \t\topts->allow_ff = git_config_bool_or_int(key, value, &error_flag);\n+\telse if (!strcmp(key, \"options.no-verify\"))\n+\t\topts->no_verify = git_config_bool_or_int(key, value, &error_flag);\n \telse if (!strcmp(key, \"options.mainline\"))\n \t\topts->mainline = git_config_int(key, value);\n \telse if (!strcmp(key, \"options.strategy\"))\n@@ -944,6 +949,8 @@ static void save_opts(struct replay_opts *opts)\n \t\tgit_config_set_in_file(opts_file, \"options.record-origin\", \"true\");\n \tif (opts->allow_ff)\n \t\tgit_config_set_in_file(opts_file, \"options.allow-ff\", \"true\");\n+\tif (opts->no_verify)\n+\t\tgit_config_set_in_file(opts_file, \"options.no-verify\", \"true\");\n \tif (opts->mainline) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstrbuf_addf(&buf, \"%d\", opts->mainline);\ndiff --git a/sequencer.h b/sequencer.h\nindex db43e9c..abfadc0 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -34,6 +34,7 @@ struct replay_opts {\n \tint allow_empty;\n \tint allow_empty_message;\n \tint keep_redundant_commits;\n+\tint no_verify;\n \n \tint mainline;\n \ndiff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh\nindex adc892b..b0307f4 100755\n--- a/t/t7503-pre-commit-hook.sh\n+++ b/t/t7503-pre-commit-hook.sh\n@@ -80,13 +80,13 @@ test_expect_success '--no-verify with failing hook' '\n \n '\n \n-test_expect_failure 'revert --no-verify with failing hook' '\n+test_expect_success 'revert --no-verify with failing hook' '\n \n \tgit revert --no-verify HEAD\n \n '\n \n-test_expect_failure 'cherry-pick --no-verify with failing hook' '\n+test_expect_success 'cherry-pick --no-verify with failing hook' '\n \n \tgit cherry-pick --no-verify HEAD^\n \ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex 4f8b9fe..e819c25 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -138,13 +138,13 @@ test_expect_success '--no-verify with failing hook' '\n \n '\n \n-test_expect_failure 'revert --no-verify with failing hook' '\n+test_expect_success 'revert --no-verify with failing hook' '\n \n \tgit revert --no-verify HEAD\n \n '\n \n-test_expect_failure 'cherry-pick --no-verify with failing hook' '\n+test_expect_success 'cherry-pick --no-verify with failing hook' '\n \n \tgit cherry-pick --no-verify HEAD^\n \n-- \n2.0.0.rc4.501.gdaf83ca\n"},{"id":"248783","messageId":"1409753034-9459-4-git-send-email-johan@herland.net","threadId":"37479","inReplyTo":"1409753034-9459-1-git-send-email-johan@herland.net","subject":"[RFC/PATCH 3/3] revert/cherry-pick --no-verify: Update documentation","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2014-09-03T14:03:54Z","receivedAt":"2014-09-03T14:03:54Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Add --no-verify to the revert and cherry-pick man pages. Also mention\nrevert and cherry-pick in the corresponding documentation for the\npre-commit and commit-msg hooks.\n\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n Documentation/git-cherry-pick.txt |  4 ++++\n Documentation/git-revert.txt      |  4 ++++\n Documentation/githooks.txt        | 20 ++++++++++----------\n 3 files changed, 18 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\nindex 1c03c79..f56818f 100644\n--- a/Documentation/git-cherry-pick.txt\n+++ b/Documentation/git-cherry-pick.txt\n@@ -97,6 +97,10 @@ OPTIONS\n This is useful when cherry-picking more than one commits'\n effect to your index in a row.\n \n+--no-verify::\n+\tThis option bypasses the pre-commit and commit-msg hooks.\n+\tSee also linkgit:githooks[5].\n+\n -s::\n --signoff::\n \tAdd Signed-off-by line at the end of the commit message.\ndiff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt\nindex cceb5f2..c9fb148 100644\n--- a/Documentation/git-revert.txt\n+++ b/Documentation/git-revert.txt\n@@ -80,6 +80,10 @@ more details.\n This is useful when reverting more than one commits'\n effect to your index in a row.\n \n+--no-verify::\n+\tThis option bypasses the pre-commit and commit-msg hooks.\n+\tSee also linkgit:githooks[5].\n+\n -S[<key-id>]::\n --gpg-sign[=<key-id>]::\n \tGPG-sign commits.\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex d954bf6..9c3bf6c 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -72,11 +72,11 @@ the outcome of 'git am'.\n pre-commit\n ~~~~~~~~~~\n \n-This hook is invoked by 'git commit', and can be bypassed\n-with `--no-verify` option.  It takes no parameter, and is\n-invoked before obtaining the proposed commit log message and\n-making a commit.  Exiting with non-zero status from this script\n-causes the 'git commit' to abort.\n+This hook is invoked by 'git commit' (including 'git revert' and\n+'git cherry-pick'), and can be bypassed with `--no-verify` option.\n+It takes no parameter, and is invoked before obtaining the proposed\n+commit log message and making a commit.  Exiting with non-zero\n+status from this script causes the 'git commit' to abort.\n \n The default 'pre-commit' hook, when enabled, catches introduction\n of lines with trailing whitespaces and aborts the commit when\n@@ -114,11 +114,11 @@ out the `Conflicts:` part of a merge's commit message.\n commit-msg\n ~~~~~~~~~~\n \n-This hook is invoked by 'git commit', and can be bypassed\n-with `--no-verify` option.  It takes a single parameter, the\n-name of the file that holds the proposed commit log message.\n-Exiting with non-zero status causes the 'git commit' to\n-abort.\n+This hook is invoked by 'git commit' (including 'git revert'\n+and 'git cherry-pick'), and can be bypassed with `--no-verify`\n+option.  It takes a single parameter, the name of the file that\n+holds the proposed commit log message. Exiting with non-zero\n+status causes the 'git commit' to abort.\n \n The hook is allowed to edit the message file in place, and can\n be used to normalize the message into some project standard\n-- \n2.0.0.rc4.501.gdaf83ca\n"},{"id":"248796","messageId":"xmqqk35ky0fg.fsf@gitster.dls.corp.google.com","threadId":"37479","inReplyTo":"1409753034-9459-1-git-send-email-johan@herland.net","subject":"Re: [RFC/PATCH 0/3] Teach revert/cherry-pick the --no-verify option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-03T19:21:55Z","receivedAt":"2014-09-03T19:21:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland <johan@herland.net> writes:\n\n> A colleague of mine noticed that cherry-pick does not accept the\n> --no-verify option to skip running the pre-commit/commit-msg hooks.\n>\n> Here's a first attempt at adding --no-verify to the revert/cherry-pick.\n\nBack when cherry-pick was a single commit operation, lack of the\noption did not matter very much, but it probably makes sense to\nallow telling the command that the entire series of commits is\nexpected to be full of ones that do not verify.  In the same vain,\nwe already support --allow-empty and --allow-empty-message, so in\nthat sense this change probably is an improvement.\n"},{"id":"248797","messageId":"xmqqbnqwy03p.fsf@gitster.dls.corp.google.com","threadId":"37479","inReplyTo":"1409753034-9459-2-git-send-email-johan@herland.net","subject":"Re: [RFC/PATCH 1/3] t7503/4: Add failing testcases for revert/cherry-pick --no-verify","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-03T19:28:58Z","receivedAt":"2014-09-03T19:28:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland <johan@herland.net> writes:\n\n> The revert/cherry-pick machinery currently exercises the pre-commit\n> and commit-msg hooks. However, where commit accepts a --no-verify\n> option to temporarily disable these hooks, the revert and cherry-pick\n> commands have no such option.\n>\n> This patch adds some testcases demonstrating how the --no-verify\n> option is supposed when it is added to revert and cherry-pick\n> (in the next patch).\n>\n> Signed-off-by: Johan Herland <johan@herland.net>\n> ---\n\nThe added test looks OK; will queue.\n\nWe may want to update its style of testing (the shell scripting\nstyle is also bad, but they assume and depend on that the previous\nsteps have all passed to take the history and the repository into a\ncertain state without explicit \"reset --hard\" to allow some previous\nsteps to fail), though.\n\nAlso, do we already test these commands with the --allow-empty\noption and/or the --allow-empty-message option, which I think share\nthe same issue, somewhere in the test suite?  If not, we may want to\nwhile we remember the issue.\n\nThanks.\n\n>  t/t7503-pre-commit-hook.sh | 24 ++++++++++++++++++++++++\n>  t/t7504-commit-msg-hook.sh | 24 ++++++++++++++++++++++++\n>  2 files changed, 48 insertions(+)\n>\n> diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh\n> index 984889b..adc892b 100755\n> --- a/t/t7503-pre-commit-hook.sh\n> +++ b/t/t7503-pre-commit-hook.sh\n> @@ -60,6 +60,18 @@ test_expect_success 'with failing hook' '\n>  \n>  '\n>  \n> +test_expect_success 'revert with failing hook' '\n> +\n> +\ttest_must_fail git revert HEAD\n> +\n> +'\n> +\n> +test_expect_success 'cherry-pick with failing hook' '\n> +\n> +\ttest_must_fail git cherry-pick --no-verify HEAD^\n> +\n> +'\n> +\n>  test_expect_success '--no-verify with failing hook' '\n>  \n>  \techo \"stuff\" >> file &&\n> @@ -68,6 +80,18 @@ test_expect_success '--no-verify with failing hook' '\n>  \n>  '\n>  \n> +test_expect_failure 'revert --no-verify with failing hook' '\n> +\n> +\tgit revert --no-verify HEAD\n> +\n> +'\n> +\n> +test_expect_failure 'cherry-pick --no-verify with failing hook' '\n> +\n> +\tgit cherry-pick --no-verify HEAD^\n> +\n> +'\n> +\n>  chmod -x \"$HOOK\"\n>  test_expect_success POSIXPERM 'with non-executable hook' '\n>  \n> diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\n> index 1f53ea8..4f8b9fe 100755\n> --- a/t/t7504-commit-msg-hook.sh\n> +++ b/t/t7504-commit-msg-hook.sh\n> @@ -109,6 +109,18 @@ test_expect_success 'with failing hook' '\n>  \n>  '\n>  \n> +test_expect_success 'revert with failing hook' '\n> +\n> +\ttest_must_fail git revert HEAD\n> +\n> +'\n> +\n> +test_expect_success 'cherry-pick with failing hook' '\n> +\n> +\ttest_must_fail git cherry-pick --no-verify HEAD^\n> +\n> +'\n> +\n>  test_expect_success 'with failing hook (editor)' '\n>  \n>  \techo \"more another\" >> file &&\n> @@ -126,6 +138,18 @@ test_expect_success '--no-verify with failing hook' '\n>  \n>  '\n>  \n> +test_expect_failure 'revert --no-verify with failing hook' '\n> +\n> +\tgit revert --no-verify HEAD\n> +\n> +'\n> +\n> +test_expect_failure 'cherry-pick --no-verify with failing hook' '\n> +\n> +\tgit cherry-pick --no-verify HEAD^\n> +\n> +'\n> +\n>  test_expect_success '--no-verify with failing hook (editor)' '\n>  \n>  \techo \"more stuff\" >> file &&\n"},{"id":"248799","messageId":"xmqq7g1kxzxi.fsf@gitster.dls.corp.google.com","threadId":"37479","inReplyTo":"1409753034-9459-3-git-send-email-johan@herland.net","subject":"Re: [RFC/PATCH 2/3] revert/cherry-pick: Add --no-verify option, and pass it on to commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-03T19:32:41Z","receivedAt":"2014-09-03T19:32:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland <johan@herland.net> writes:\n\n> Allow users to temporarily disable the pre-commit and commit-msg hooks\n> when running \"git revert\" or \"git cherry-pick\", just like they currently\n> can for \"git commit\".\n>\n> The --no-verify option is added to the sequencer machinery and handled\n> like the other commit-related options.\n>\n> This fixes the failing t7503/t7504 test cases added previously.\n>\n> Signed-off-by: Johan Herland <johan@herland.net>\n> ---\n>  builtin/revert.c           | 1 +\n>  sequencer.c                | 7 +++++++\n>  sequencer.h                | 1 +\n>  t/t7503-pre-commit-hook.sh | 4 ++--\n>  t/t7504-commit-msg-hook.sh | 4 ++--\n>  5 files changed, 13 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index f9ed5bd..831c2cd 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -91,6 +91,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n>  \t\t\tN_(\"option for merge strategy\"), option_parse_x),\n>  \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n>  \t\t  N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n> +\t\tOPT_BOOL('n', \"no-verify\", &opts->no_verify, N_(\"bypass pre-commit hook\")),\n\nI doubt we want this option to squat on '-n'; besides, it is already\ntaken by a more often used \"--no-commit\".\n\nI thought that we added sanity checker for the options[] array to parse-options\nAPI.  I wonder why it did not kick in...\n\n\n\n>  \t\tOPT_END(),\n>  \t\tOPT_END(),\n>  \t\tOPT_END(),\n> diff --git a/sequencer.c b/sequencer.c\n> index 3c060e0..3d68113 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -378,6 +378,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n>  \tif (opts->allow_empty_message)\n>  \t\targv_array_push(&array, \"--allow-empty-message\");\n>  \n> +\tif (opts->no_verify)\n> +\t\targv_array_push(&array, \"--no-verify\");\n> +\n>  \trc = run_command_v_opt(array.argv, RUN_GIT_CMD);\n>  \targv_array_clear(&array);\n>  \treturn rc;\n> @@ -773,6 +776,8 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n>  \t\topts->record_origin = git_config_bool_or_int(key, value, &error_flag);\n>  \telse if (!strcmp(key, \"options.allow-ff\"))\n>  \t\topts->allow_ff = git_config_bool_or_int(key, value, &error_flag);\n> +\telse if (!strcmp(key, \"options.no-verify\"))\n> +\t\topts->no_verify = git_config_bool_or_int(key, value, &error_flag);\n>  \telse if (!strcmp(key, \"options.mainline\"))\n>  \t\topts->mainline = git_config_int(key, value);\n>  \telse if (!strcmp(key, \"options.strategy\"))\n> @@ -944,6 +949,8 @@ static void save_opts(struct replay_opts *opts)\n>  \t\tgit_config_set_in_file(opts_file, \"options.record-origin\", \"true\");\n>  \tif (opts->allow_ff)\n>  \t\tgit_config_set_in_file(opts_file, \"options.allow-ff\", \"true\");\n> +\tif (opts->no_verify)\n> +\t\tgit_config_set_in_file(opts_file, \"options.no-verify\", \"true\");\n>  \tif (opts->mainline) {\n>  \t\tstruct strbuf buf = STRBUF_INIT;\n>  \t\tstrbuf_addf(&buf, \"%d\", opts->mainline);\n> diff --git a/sequencer.h b/sequencer.h\n> index db43e9c..abfadc0 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -34,6 +34,7 @@ struct replay_opts {\n>  \tint allow_empty;\n>  \tint allow_empty_message;\n>  \tint keep_redundant_commits;\n> +\tint no_verify;\n>  \n>  \tint mainline;\n>  \n> diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh\n> index adc892b..b0307f4 100755\n> --- a/t/t7503-pre-commit-hook.sh\n> +++ b/t/t7503-pre-commit-hook.sh\n> @@ -80,13 +80,13 @@ test_expect_success '--no-verify with failing hook' '\n>  \n>  '\n>  \n> -test_expect_failure 'revert --no-verify with failing hook' '\n> +test_expect_success 'revert --no-verify with failing hook' '\n>  \n>  \tgit revert --no-verify HEAD\n>  \n>  '\n>  \n> -test_expect_failure 'cherry-pick --no-verify with failing hook' '\n> +test_expect_success 'cherry-pick --no-verify with failing hook' '\n>  \n>  \tgit cherry-pick --no-verify HEAD^\n>  \n> diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\n> index 4f8b9fe..e819c25 100755\n> --- a/t/t7504-commit-msg-hook.sh\n> +++ b/t/t7504-commit-msg-hook.sh\n> @@ -138,13 +138,13 @@ test_expect_success '--no-verify with failing hook' '\n>  \n>  '\n>  \n> -test_expect_failure 'revert --no-verify with failing hook' '\n> +test_expect_success 'revert --no-verify with failing hook' '\n>  \n>  \tgit revert --no-verify HEAD\n>  \n>  '\n>  \n> -test_expect_failure 'cherry-pick --no-verify with failing hook' '\n> +test_expect_success 'cherry-pick --no-verify with failing hook' '\n>  \n>  \tgit cherry-pick --no-verify HEAD^\n"},{"id":"248800","messageId":"xmqq1trsxzgy.fsf_-_@gitster.dls.corp.google.com","threadId":"37479","inReplyTo":"xmqq7g1kxzxi.fsf@gitster.dls.corp.google.com","subject":"[PATCH] parse-options: detect attempt to add a duplicate short option name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-03T19:42:37Z","receivedAt":"2014-09-03T19:42:37Z","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>> diff --git a/builtin/revert.c b/builtin/revert.c\n>> index f9ed5bd..831c2cd 100644\n>> --- a/builtin/revert.c\n>> +++ b/builtin/revert.c\n>> @@ -91,6 +91,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n>>  \t\t\tN_(\"option for merge strategy\"), option_parse_x),\n>>  \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n>>  \t\t  N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n>> +\t\tOPT_BOOL('n', \"no-verify\", &opts->no_verify, N_(\"bypass pre-commit hook\")),\n>\n> I doubt we want this option to squat on '-n'; besides, it is already\n> taken by a more often used \"--no-commit\".\n>\n> I thought that we added sanity checker for the options[] array to parse-options\n> API.  I wonder why it did not kick in...\n\n... because we didn't, not quite.\n\nPerhaps like this?\n\n-- >8 --\nIt is easy to overlook an already assigned single-letter option name\nand try to use it for a new one.  Help the developer to catch it\nbefore such a mistake escapes the lab.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\ndiff --git a/parse-options.c b/parse-options.c\nindex e7dafa8..b7925c5 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -347,12 +347,17 @@ static void check_typos(const char *arg, const struct option *options)\n static void parse_options_check(const struct option *opts)\n {\n \tint err = 0;\n+\tchar short_opts[128];\n+\n+\tmemset(short_opts, '\\0', sizeof(short_opts));\n \n \tfor (; opts->type != OPTION_END; opts++) {\n \t\tif ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&\n \t\t    (opts->flags & PARSE_OPT_OPTARG))\n \t\t\terr |= optbug(opts, \"uses incompatible flags \"\n \t\t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n+\t\tif (opts->short_name && short_opts[opts->short_name]++)\n+\t\t\terr |= optbug(opts, \"short name already used\");\n \t\tif (opts->flags & PARSE_OPT_NODASH &&\n \t\t    ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t     !(opts->flags & PARSE_OPT_NOARG) ||\n"},{"id":"248802","messageId":"54077A3E.20703@web.de","threadId":"37479","inReplyTo":"xmqq1trsxzgy.fsf_-_@gitster.dls.corp.google.com","subject":"Re: [PATCH] parse-options: detect attempt to add a duplicate short option name","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-09-03T20:29:50Z","receivedAt":"2014-09-03T20:29:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.09.2014 um 21:42 schrieb Junio C Hamano:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>>> diff --git a/builtin/revert.c b/builtin/revert.c\n>>> index f9ed5bd..831c2cd 100644\n>>> --- a/builtin/revert.c\n>>> +++ b/builtin/revert.c\n>>> @@ -91,6 +91,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n>>>   \t\t\tN_(\"option for merge strategy\"), option_parse_x),\n>>>   \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n>>>   \t\t  N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n>>> +\t\tOPT_BOOL('n', \"no-verify\", &opts->no_verify, N_(\"bypass pre-commit hook\")),\n>>\n>> I doubt we want this option to squat on '-n'; besides, it is already\n>> taken by a more often used \"--no-commit\".\n>>\n>> I thought that we added sanity checker for the options[] array to parse-options\n>> API.  I wonder why it did not kick in...\n> \n> ... because we didn't, not quite.\n> \n> Perhaps like this?\n> \n> -- >8 --\n> It is easy to overlook an already assigned single-letter option name\n> and try to use it for a new one.  Help the developer to catch it\n> before such a mistake escapes the lab.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n> diff --git a/parse-options.c b/parse-options.c\n> index e7dafa8..b7925c5 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -347,12 +347,17 @@ static void check_typos(const char *arg, const struct option *options)\n>   static void parse_options_check(const struct option *opts)\n>   {\n>   \tint err = 0;\n> +\tchar short_opts[128];\n> +\n> +\tmemset(short_opts, '\\0', sizeof(short_opts));\n>   \n>   \tfor (; opts->type != OPTION_END; opts++) {\n>   \t\tif ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&\n>   \t\t    (opts->flags & PARSE_OPT_OPTARG))\n>   \t\t\terr |= optbug(opts, \"uses incompatible flags \"\n>   \t\t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n> +\t\tif (opts->short_name && short_opts[opts->short_name]++)\n> +\t\t\terr |= optbug(opts, \"short name already used\");\n>   \t\tif (opts->flags & PARSE_OPT_NODASH &&\n>   \t\t    ((opts->flags & PARSE_OPT_OPTARG) ||\n>   \t\t     !(opts->flags & PARSE_OPT_NOARG) ||\n> \n\nCompact and useful, I like it.\n\nYou might want to squash in something like this, though.  Without it\nt1502 fails because -b is defined twice there.\n\n---\n t/t1502-rev-parse-parseopt.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 922423e..ebe7c3b 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -19,7 +19,7 @@ sed -e 's/^|//' >expect <<\\END_EXPECT\n |    -d, --data[=...]      short and long option with an optional argument\n |\n |Argument hints\n-|    -b <arg>              short option required argument\n+|    -B <arg>              short option required argument\n |    --bar2 <arg>          long option required argument\n |    -e, --fuz <with-space>\n |                          short and long option required argument\n@@ -51,7 +51,7 @@ sed -e 's/^|//' >optionspec <<\\EOF\n |d,data?   short and long option with an optional argument\n |\n | Argument hints\n-|b=arg     short option required argument\n+|B=arg     short option required argument\n |bar2=arg  long option required argument\n |e,fuz=with-space  short and long option required argument\n |s?some    short option optional argument\n-- \n2.1.0\n"},{"id":"248805","messageId":"xmqqoauwwh2c.fsf@gitster.dls.corp.google.com","threadId":"37479","inReplyTo":"54077A3E.20703@web.de","subject":"Re: [PATCH] parse-options: detect attempt to add a duplicate short option name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-03T21:05:31Z","receivedAt":"2014-09-03T21:05:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Compact and useful, I like it.\n>\n> You might want to squash in something like this, though.  Without it\n> t1502 fails because -b is defined twice there.\n\nThanks.  I like it to see that the check automatically propagates\neven to scripts ;-)\n\nIt bugged me enough that we didn't identify which short option\nletter we were complaining about and that opts->short_name is\ndefined as an \"int\", which may cause us to overstep char[128],\nI ended up doing it this way instead, though.  It no longer is so\ncompact, even though it may still have the same usefulness.\n\nWe might want to tighten the type of the short_name member to\nunsigned char, but I didn't go that far yet, at least in this step.\n\n-- >8 --\nSubject: [PATCH] parse-options: detect attempt to add a duplicate short option name\n\nIt is easy to overlook an already assigned single-letter option name\nand try to use it for a new one.  Help the developer to catch it\nbefore such a mistake escapes the lab.\n\nHelped-by: René Scharfe <l.s.r@web.de>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n parse-options.c               | 15 +++++++++++++++\n t/t1502-rev-parse-parseopt.sh |  4 ++--\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex b536896..70227e9 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -345,12 +345,27 @@ static void check_typos(const char *arg, const struct option *options)\n static void parse_options_check(const struct option *opts)\n {\n \tint err = 0;\n+\tchar short_opts[128];\n+\n+\tmemset(short_opts, '\\0', sizeof(short_opts));\n \n \tfor (; opts->type != OPTION_END; opts++) {\n \t\tif ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&\n \t\t    (opts->flags & PARSE_OPT_OPTARG))\n \t\t\terr |= optbug(opts, \"uses incompatible flags \"\n \t\t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n+\t\tif (opts->short_name) {\n+\t\t\tstruct strbuf errmsg = STRBUF_INIT;\n+\t\t\tif (opts->short_name < ' ' || 0x7F <= opts->short_name)\n+\t\t\t\tstrbuf_addf(&errmsg, \"invalid short name (0x%02x)\",\n+\t\t\t\t\t    opts->short_name);\n+\t\t\telse if (short_opts[opts->short_name]++)\n+\t\t\t\tstrbuf_addf(&errmsg, \"short name %c already used\",\n+\t\t\t\t\t    opts->short_name);\n+\t\t\tif (errmsg.len)\n+\t\t\t\terr |= optbug(opts, errmsg.buf);\n+\t\t\tstrbuf_release(&errmsg);\n+\t\t}\n \t\tif (opts->flags & PARSE_OPT_NODASH &&\n \t\t    ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t     !(opts->flags & PARSE_OPT_NOARG) ||\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 922423e..ebe7c3b 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -19,7 +19,7 @@ sed -e 's/^|//' >expect <<\\END_EXPECT\n |    -d, --data[=...]      short and long option with an optional argument\n |\n |Argument hints\n-|    -b <arg>              short option required argument\n+|    -B <arg>              short option required argument\n |    --bar2 <arg>          long option required argument\n |    -e, --fuz <with-space>\n |                          short and long option required argument\n@@ -51,7 +51,7 @@ sed -e 's/^|//' >optionspec <<\\EOF\n |d,data?   short and long option with an optional argument\n |\n | Argument hints\n-|b=arg     short option required argument\n+|B=arg     short option required argument\n |bar2=arg  long option required argument\n |e,fuz=with-space  short and long option required argument\n |s?some    short option optional argument\n-- \n2.1.0-394-g3e31896\n"},{"id":"248812","messageId":"54078C2C.5020503@web.de","threadId":"37479","inReplyTo":"xmqqoauwwh2c.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] parse-options: detect attempt to add a duplicate short option name","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-09-03T21:46:20Z","receivedAt":"2014-09-03T21:46:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.09.2014 um 23:05 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> Compact and useful, I like it.\n>>\n>> You might want to squash in something like this, though.  Without it\n>> t1502 fails because -b is defined twice there.\n>\n> Thanks.  I like it to see that the check automatically propagates\n> even to scripts ;-)\n>\n> It bugged me enough that we didn't identify which short option\n> letter we were complaining about\n\nThe old code did report the short option.  E.g. for t1502 it said:\n\n\terror: BUG: switch 'b' short name already used\n\nYou can leave that to optbug(), no need for the strbuf.\n\n> and that opts->short_name is\n> defined as an \"int\", which may cause us to overstep char[128],\n> I ended up doing it this way instead, though.   It no longer is so\n> compact, even though it may still have the same usefulness.\n\nA range check is an additional feature (increased usefulness).  I guess \nusing invalid characters is not that common a mistake, though.\n\nSpace is allowed as a short option by the code; intentionally?\n\n>\n> We might want to tighten the type of the short_name member to\n> unsigned char, but I didn't go that far yet, at least in this step.\n>\n> -- >8 --\n> Subject: [PATCH] parse-options: detect attempt to add a duplicate short option name\n>\n> It is easy to overlook an already assigned single-letter option name\n> and try to use it for a new one.  Help the developer to catch it\n> before such a mistake escapes the lab.\n>\n> Helped-by: René Scharfe <l.s.r@web.de>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>   parse-options.c               | 15 +++++++++++++++\n>   t/t1502-rev-parse-parseopt.sh |  4 ++--\n>   2 files changed, 17 insertions(+), 2 deletions(-)\n>\n> diff --git a/parse-options.c b/parse-options.c\n> index b536896..70227e9 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -345,12 +345,27 @@ static void check_typos(const char *arg, const struct option *options)\n>   static void parse_options_check(const struct option *opts)\n>   {\n>   \tint err = 0;\n> +\tchar short_opts[128];\n> +\n> +\tmemset(short_opts, '\\0', sizeof(short_opts));\n>\n>   \tfor (; opts->type != OPTION_END; opts++) {\n>   \t\tif ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&\n>   \t\t    (opts->flags & PARSE_OPT_OPTARG))\n>   \t\t\terr |= optbug(opts, \"uses incompatible flags \"\n>   \t\t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n> +\t\tif (opts->short_name) {\n> +\t\t\tstruct strbuf errmsg = STRBUF_INIT;\n> +\t\t\tif (opts->short_name < ' ' || 0x7F <= opts->short_name)\n> +\t\t\t\tstrbuf_addf(&errmsg, \"invalid short name (0x%02x)\",\n> +\t\t\t\t\t    opts->short_name);\n> +\t\t\telse if (short_opts[opts->short_name]++)\n> +\t\t\t\tstrbuf_addf(&errmsg, \"short name %c already used\",\n> +\t\t\t\t\t    opts->short_name);\n> +\t\t\tif (errmsg.len)\n> +\t\t\t\terr |= optbug(opts, errmsg.buf);\n> +\t\t\tstrbuf_release(&errmsg);\n> +\t\t}\n>   \t\tif (opts->flags & PARSE_OPT_NODASH &&\n>   \t\t    ((opts->flags & PARSE_OPT_OPTARG) ||\n>   \t\t     !(opts->flags & PARSE_OPT_NOARG) ||\n> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\n> index 922423e..ebe7c3b 100755\n> --- a/t/t1502-rev-parse-parseopt.sh\n> +++ b/t/t1502-rev-parse-parseopt.sh\n> @@ -19,7 +19,7 @@ sed -e 's/^|//' >expect <<\\END_EXPECT\n>   |    -d, --data[=...]      short and long option with an optional argument\n>   |\n>   |Argument hints\n> -|    -b <arg>              short option required argument\n> +|    -B <arg>              short option required argument\n>   |    --bar2 <arg>          long option required argument\n>   |    -e, --fuz <with-space>\n>   |                          short and long option required argument\n> @@ -51,7 +51,7 @@ sed -e 's/^|//' >optionspec <<\\EOF\n>   |d,data?   short and long option with an optional argument\n>   |\n>   | Argument hints\n> -|b=arg     short option required argument\n> +|B=arg     short option required argument\n>   |bar2=arg  long option required argument\n>   |e,fuz=with-space  short and long option required argument\n>   |s?some    short option optional argument\n>\n"},{"id":"248811","messageId":"20140903214624.GY18279@google.com","threadId":"37479","inReplyTo":"xmqqoauwwh2c.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] parse-options: detect attempt to add a duplicate short option name","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-03T21:46:25Z","receivedAt":"2014-09-03T21:46:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -345,12 +345,27 @@ static void check_typos(const char *arg, const struct option *options)\n>  static void parse_options_check(const struct option *opts)\n>  {\n>  \tint err = 0;\n> +\tchar short_opts[128];\n> +\n> +\tmemset(short_opts, '\\0', sizeof(short_opts));\n>  \n>  \tfor (; opts->type != OPTION_END; opts++) {\n>  \t\tif ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&\n>  \t\t    (opts->flags & PARSE_OPT_OPTARG))\n>  \t\t\terr |= optbug(opts, \"uses incompatible flags \"\n>  \t\t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n> +\t\tif (opts->short_name) {\n> +\t\t\tstruct strbuf errmsg = STRBUF_INIT;\n> +\t\t\tif (opts->short_name < ' ' || 0x7F <= opts->short_name)\n> +\t\t\t\tstrbuf_addf(&errmsg, \"invalid short name (0x%02x)\",\n> +\t\t\t\t\t    opts->short_name);\n> +\t\t\telse if (short_opts[opts->short_name]++)\n\nWhat happens on platforms with a signed char?\n\nWith the following squashed in,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\ndiff --git i/parse-options.c w/parse-options.c\nindex f7f153a..4cc3f3e 100644\n--- i/parse-options.c\n+++ w/parse-options.c\n@@ -361,7 +361,7 @@ static void parse_options_check(const struct option *opts)\n \t\t\tif (opts->short_name < ' ' || 0x7F <= opts->short_name)\n \t\t\t\tstrbuf_addf(&errmsg, \"invalid short name (0x%02x)\",\n \t\t\t\t\t    opts->short_name);\n-\t\t\telse if (short_opts[opts->short_name]++)\n+\t\t\telse if (short_opts[(unsigned char) opts->short_name]++)\n \t\t\t\tstrbuf_addf(&errmsg, \"short name %c already used\",\n \t\t\t\t\t    opts->short_name);\n \t\t\tif (errmsg.len)\n"},{"id":"248814","messageId":"20140903215834.GZ18279@google.com","threadId":"37479","inReplyTo":"20140903214624.GY18279@google.com","subject":"Re: [PATCH] parse-options: detect attempt to add a duplicate short option name","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-03T21:58:34Z","receivedAt":"2014-09-03T21:58:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"On Wed, Sep 03, 2014 at 02:46:25PM -0700, Jonathan Nieder wrote:\n> Junio C Hamano wrote:\n> \n> > --- a/parse-options.c\n> > +++ b/parse-options.c\n> > @@ -345,12 +345,27 @@ static void check_typos(const char *arg, const struct option *options)\n> >  static void parse_options_check(const struct option *opts)\n> >  {\n> >  \tint err = 0;\n> > +\tchar short_opts[128];\n> > +\n> > +\tmemset(short_opts, '\\0', sizeof(short_opts));\n> >  \n> >  \tfor (; opts->type != OPTION_END; opts++) {\n> >  \t\tif ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&\n> >  \t\t    (opts->flags & PARSE_OPT_OPTARG))\n> >  \t\t\terr |= optbug(opts, \"uses incompatible flags \"\n> >  \t\t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n> > +\t\tif (opts->short_name) {\n> > +\t\t\tstruct strbuf errmsg = STRBUF_INIT;\n> > +\t\t\tif (opts->short_name < ' ' || 0x7F <= opts->short_name)\n> > +\t\t\t\tstrbuf_addf(&errmsg, \"invalid short name (0x%02x)\",\n> > +\t\t\t\t\t    opts->short_name);\n> > +\t\t\telse if (short_opts[opts->short_name]++)\n> \n> What happens on platforms with a signed char?\n\nAh, I see now that the \"< ' '\" check would catch that.\n\nWith René's suggestions squashed in, that becomes\n\n parse-options.c               | 9 +++++++++\n t/t1502-rev-parse-parseopt.sh | 4 ++--\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex e7dafa8..6ad7d90 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -347,12 +347,21 @@ static void check_typos(const char *arg, const struct option *options)\n static void parse_options_check(const struct option *opts)\n {\n \tint err = 0;\n+\tchar short_opts[128];\n+\n+\tmemset(short_opts, '\\0', sizeof(short_opts));\n \n \tfor (; opts->type != OPTION_END; opts++) {\n \t\tif ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&\n \t\t    (opts->flags & PARSE_OPT_OPTARG))\n \t\t\terr |= optbug(opts, \"uses incompatible flags \"\n \t\t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n+\t\tif (opts->short_name) {\n+\t\t\tif (opts->short_name <= ' ' || 0x7F <= opts->short_name)\n+\t\t\t\terr |= optbug(opts, \"invalid short name\");\n+\t\t\telse if (short_opts[opts->short_name]++)\n+\t\t\t\terr |= optbug(opts, \"short name already used\");\n+\t\t}\n \t\tif (opts->flags & PARSE_OPT_NODASH &&\n \t\t    ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t     !(opts->flags & PARSE_OPT_NOARG) ||\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 922423e..ebe7c3b 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -19,7 +19,7 @@ sed -e 's/^|//' >expect <<\\END_EXPECT\n |    -d, --data[=...]      short and long option with an optional argument\n |\n |Argument hints\n-|    -b <arg>              short option required argument\n+|    -B <arg>              short option required argument\n |    --bar2 <arg>          long option required argument\n |    -e, --fuz <with-space>\n |                          short and long option required argument\n@@ -51,7 +51,7 @@ sed -e 's/^|//' >optionspec <<\\EOF\n |d,data?   short and long option with an optional argument\n |\n | Argument hints\n-|b=arg     short option required argument\n+|B=arg     short option required argument\n |bar2=arg  long option required argument\n |e,fuz=with-space  short and long option required argument\n |s?some    short option optional argument\n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"248815","messageId":"xmqqbnqwwds2.fsf@gitster.dls.corp.google.com","threadId":"37479","inReplyTo":"54078C2C.5020503@web.de","subject":"Re: [PATCH] parse-options: detect attempt to add a duplicate short option name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-03T22:16:29Z","receivedAt":"2014-09-03T22:16:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>> It bugged me enough that we didn't identify which short option\n>> letter we were complaining about\n>\n> The old code did report the short option.  E.g. for t1502 it said:\n>\n> \terror: BUG: switch 'b' short name already used\n>\n> You can leave that to optbug(), no need for the strbuf.\n\nNot quite, as an opt with long name is reported with the long name\nonly, which is not very nice when the problem we are reporting is\nabout its short variant.\n\n> Space is allowed as a short option by the code; intentionally?\n\nI didn't think of a strong reason to declare either way, so, yes it\nwas deliberate that I didn't tighten to disallow.\n"},{"id":"248829","messageId":"540802F5.1070708@web.de","threadId":"37479","inReplyTo":"xmqqbnqwwds2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] parse-options: detect attempt to add a duplicate short option name","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-09-04T06:13:09Z","receivedAt":"2014-09-04T06:13:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 04.09.2014 um 00:16 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n> \n>>> It bugged me enough that we didn't identify which short option\n>>> letter we were complaining about\n>>\n>> The old code did report the short option.  E.g. for t1502 it said:\n>>\n>> \terror: BUG: switch 'b' short name already used\n>>\n>> You can leave that to optbug(), no need for the strbuf.\n> \n> Not quite, as an opt with long name is reported with the long name\n> only, which is not very nice when the problem we are reporting is\n> about its short variant.\n\nPerhaps something like the patch below helps, here and in general?\n\n>> Space is allowed as a short option by the code; intentionally?\n> \n> I didn't think of a strong reason to declare either way, so, yes it\n> was deliberate that I didn't tighten to disallow.\n\nOK.  I don't think it's easy to come up with a usable way for having\nspace as a short option, but maybe it's possible.\n\n---\n parse-options.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex b7925c5..f1c0b5d 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -14,8 +14,12 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n \n int optbug(const struct option *opt, const char *reason)\n {\n-\tif (opt->long_name)\n+\tif (opt->long_name) {\n+\t\tif (opt->short_name)\n+\t\t\treturn error(\"BUG: switch '%c' (--%s) %s\",\n+\t\t\t\t     opt->short_name, opt->long_name, reason);\n \t\treturn error(\"BUG: option '%s' %s\", opt->long_name, reason);\n+\t}\n \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n }\n \n-- \n2.1.0\n"},{"id":"299211","messageId":"CALKQrgcdEMcFWhT7ZSKYR23QL7=_CKXRKxwZ_nOCSzRzPk0TGg@mail.gmail.com","threadId":"37479","inReplyTo":"xmqq7g1kxzxi.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 2/3] revert/cherry-pick: Add --no-verify option, and pass it on to commit","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2014-09-04T08:34:20Z","receivedAt":"2014-09-04T08:34:20Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Wed, Sep 3, 2014 at 9:32 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Johan Herland <johan@herland.net> writes:\n>> diff --git a/builtin/revert.c b/builtin/revert.c\n>> index f9ed5bd..831c2cd 100644\n>> --- a/builtin/revert.c\n>> +++ b/builtin/revert.c\n>> @@ -91,6 +91,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n>>                       N_(\"option for merge strategy\"), option_parse_x),\n>>               { OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n>>                 N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n>> +             OPT_BOOL('n', \"no-verify\", &opts->no_verify, N_(\"bypass pre-commit hook\")),\n>\n> I doubt we want this option to squat on '-n'; besides, it is already\n> taken by a more often used \"--no-commit\".\n\nOops. I never meant to squat on '-n'. Will fix in re-roll.\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"248844","messageId":"xmqqegvruwmz.fsf@gitster.dls.corp.google.com","threadId":"37479","inReplyTo":"540802F5.1070708@web.de","subject":"Re: [PATCH] parse-options: detect attempt to add a duplicate short option name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-04T17:24:20Z","receivedAt":"2014-09-04T17:24:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>> Not quite, as an opt with long name is reported with the long name\n>> only, which is not very nice when the problem we are reporting is\n>> about its short variant.\n>\n> Perhaps something like the patch below helps, here and in general?\n\nExcellent.  Not just this particular case, but we would show both\nwhen both are available.\n\nThanks; will reroll.\n\n>  parse-options.c | 6 +++++-\n>  1 file changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/parse-options.c b/parse-options.c\n> index b7925c5..f1c0b5d 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -14,8 +14,12 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n>  \n>  int optbug(const struct option *opt, const char *reason)\n>  {\n> -\tif (opt->long_name)\n> +\tif (opt->long_name) {\n> +\t\tif (opt->short_name)\n> +\t\t\treturn error(\"BUG: switch '%c' (--%s) %s\",\n> +\t\t\t\t     opt->short_name, opt->long_name, reason);\n>  \t\treturn error(\"BUG: option '%s' %s\", opt->long_name, reason);\n> +\t}\n>  \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n>  }\n"},{"id":"248847","messageId":"xmqqtx4ntg33.fsf@gitster.dls.corp.google.com","threadId":"37479","inReplyTo":"xmqqegvruwmz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] parse-options: detect attempt to add a duplicate short option name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-04T18:07:11Z","receivedAt":"2014-09-04T18:07:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"It is easy to overlook an already assigned single-letter option name\nand try to use it for a new one.  Help the developer to catch it\nbefore such a mistake escapes the lab.\n\nThis retroactively forbids any short option name (which is defined\nto be of type \"int\") outside the ASCII printable range.  We might\nwant to do one of two things:\n\n - tighten the type of short_name member to 'char', and further\n   update optbug() to protect it against doing \"'%c'\" on a funny\n   value, e.g. negative or above 127.\n\n - drop the check (even the \"duplicate\" check) for an option whose\n   short_name is either negative or above 255, to allow clever folks\n   to take advantage of the fact that such a short_name cannot be\n   parsed from the command line and the member can be used to store\n   some extra information.\n\nHelped-by: René Scharfe <l.s.r@web.de>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n  Junio C Hamano <gitster@pobox.com> writes:\n\n  > René Scharfe <l.s.r@web.de> writes:\n  >\n  >>> Not quite, as an opt with long name is reported with the long name\n  >>> only, which is not very nice when the problem we are reporting is\n  >>> about its short variant.\n  >>\n  >> Perhaps something like the patch below helps, here and in general?\n  >\n  > Excellent.  Not just this particular case, but we would show both\n  > when both are available.\n  >\n  > Thanks; will reroll.\n\n parse-options.c               | 14 +++++++++++++-\n t/t1502-rev-parse-parseopt.sh |  4 ++--\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex b536896..34a15aa 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -14,8 +14,12 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n \n int optbug(const struct option *opt, const char *reason)\n {\n-\tif (opt->long_name)\n+\tif (opt->long_name) {\n+\t\tif (opt->short_name)\n+\t\t\treturn error(\"BUG: switch '%c' (--%s) %s\",\n+\t\t\t\t     opt->short_name, opt->long_name, reason);\n \t\treturn error(\"BUG: option '%s' %s\", opt->long_name, reason);\n+\t}\n \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n }\n \n@@ -345,12 +349,20 @@ static void check_typos(const char *arg, const struct option *options)\n static void parse_options_check(const struct option *opts)\n {\n \tint err = 0;\n+\tchar short_opts[128];\n \n+\tmemset(short_opts, '\\0', sizeof(short_opts));\n \tfor (; opts->type != OPTION_END; opts++) {\n \t\tif ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&\n \t\t    (opts->flags & PARSE_OPT_OPTARG))\n \t\t\terr |= optbug(opts, \"uses incompatible flags \"\n \t\t\t\t\t\"LASTARG_DEFAULT and OPTARG\");\n+\t\tif (opts->short_name) {\n+\t\t\tif (0x7F <= opts->short_name)\n+\t\t\t\terr |= optbug(opts, \"invalid short name\");\n+\t\t\telse if (short_opts[opts->short_name]++)\n+\t\t\t\terr |= optbug(opts, \"short name already used\");\n+\t\t}\n \t\tif (opts->flags & PARSE_OPT_NODASH &&\n \t\t    ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t     !(opts->flags & PARSE_OPT_NOARG) ||\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 922423e..ebe7c3b 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -19,7 +19,7 @@ sed -e 's/^|//' >expect <<\\END_EXPECT\n |    -d, --data[=...]      short and long option with an optional argument\n |\n |Argument hints\n-|    -b <arg>              short option required argument\n+|    -B <arg>              short option required argument\n |    --bar2 <arg>          long option required argument\n |    -e, --fuz <with-space>\n |                          short and long option required argument\n@@ -51,7 +51,7 @@ sed -e 's/^|//' >optionspec <<\\EOF\n |d,data?   short and long option with an optional argument\n |\n | Argument hints\n-|b=arg     short option required argument\n+|B=arg     short option required argument\n |bar2=arg  long option required argument\n |e,fuz=with-space  short and long option required argument\n |s?some    short option optional argument\n-- \n2.1.0-396-ga35a9df\n"},{"id":"248932","messageId":"540A2598.8000101@gmail.com","threadId":"37479","inReplyTo":"1409753034-9459-1-git-send-email-johan@herland.net","subject":"Re: [RFC/PATCH 0/3] Teach revert/cherry-pick the --no-verify option","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-09-05T21:05:28Z","receivedAt":"2014-09-05T21:05:28Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"Hi Johan,\n\nJohan Herland writes:\n> A colleague of mine noticed that cherry-pick does not accept the\n> --no-verify option to skip running the pre-commit/commit-msg hooks.\n\nneither git-cherry-pick nor git-revert execute the pre-commit or\ncommit-msg hooks at the moment. The underlying rationale can be found in\nthe log message of commit 9fa4db5 (\"Do not verify\nreverted/cherry-picked/rebased patches.\"). Indeed, the sequencer uses\ngit-commit internally which executes the two verify hooks by default.\nHowever, the particular command line being used implicitly specifies the\n--no-verify option. This behaviour is implemented in\nsequencer.c#run_git_commit as well, right before the configurable\ngit-commit options are handled. I guess that's easily overlooked since\nthe documentation doesn't mention it and the implementation uses the\nshort version -n of --no-verify.\n\nThe reasons why the new test cases succeed nonetheless are manifold. I\nhope they're still understandable even though I don't put the comments\nnext to the code.\n\nThe \"revert with failing hook\" test case fails if run in isolation,\nwhich can be achieved by using the very cool --run option of test-lib.\nMore specifically, git-revert does not fail because it executes the\nfailing hook but because the preceding test case leaves behind an\nuncommitted index.\n\nIn the \"cherry-pick with failing hook\" test case, git-cherry-pick really\nfails because it doesn't know the --no-verify option yet, which\npresumably ended up there only by accident. This test case is\nmeaningless if run in isolation because it assumes that \"revert with\nfailing hook\" creates a commit (else HEAD^ points nowhere).\n\nI like your patchset for that it makes it explicit in both the\ndocumentation and the tests whether the commits resulting from\ncherry-picks are being verified or not.\n\nKind regards,\n   Fabian\n\n> Here's a first attempt at adding --no-verify to the revert/cherry-pick.\n> \n> Have fun! :)\n> \n> ...Johan\n> \n> Johan Herland (3):\n>   t7503/4: Add failing testcases for revert/cherry-pick --no-verify\n>   revert/cherry-pick: Add --no-verify option, and pass it on to commit\n>   revert/cherry-pick --no-verify: Update documentation\n> \n>  Documentation/git-cherry-pick.txt |  4 ++++\n>  Documentation/git-revert.txt      |  4 ++++\n>  Documentation/githooks.txt        | 20 ++++++++++----------\n>  builtin/revert.c                  |  1 +\n>  sequencer.c                       |  7 +++++++\n>  sequencer.h                       |  1 +\n>  t/t7503-pre-commit-hook.sh        | 24 ++++++++++++++++++++++++\n>  t/t7504-commit-msg-hook.sh        | 24 ++++++++++++++++++++++++\n>  8 files changed, 75 insertions(+), 10 deletions(-)\n"},{"id":"249039","messageId":"CALKQrgewtPOFhqjH_zoY3q922DgzXxHfuUF=sS86on-r4SJYeA@mail.gmail.com","threadId":"37479","inReplyTo":"540A2598.8000101@gmail.com","subject":"Re: [RFC/PATCH 0/3] Teach revert/cherry-pick the --no-verify option","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2014-09-08T15:13:45Z","receivedAt":"2014-09-08T15:13:45Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Fri, Sep 5, 2014 at 11:05 PM, Fabian Ruch <bafain@gmail.com> wrote:\n> neither git-cherry-pick nor git-revert execute the pre-commit or\n> commit-msg hooks at the moment. The underlying rationale can be found in\n> the log message of commit 9fa4db5 (\"Do not verify\n> reverted/cherry-picked/rebased patches.\"). Indeed, the sequencer uses\n> git-commit internally which executes the two verify hooks by default.\n> However, the particular command line being used implicitly specifies the\n> --no-verify option. This behaviour is implemented in\n> sequencer.c#run_git_commit as well, right before the configurable\n> git-commit options are handled. I guess that's easily overlooked since\n> the documentation doesn't mention it and the implementation uses the\n> short version -n of --no-verify.\n\nDamn. You're obviously correct, and my patch series is seriously\nmisguided. Please drop it, Junio, and I'm very sorry for the noise.\nHopefully, I will learn not to blindly follow my assumptions.\n\n...Johan\n\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"}]}