# [RFC/PATCH 3/3] revert/cherry-pick --no-verify: Update documentation 20 messages from 2014-09-03 to 2014-09-08. Participants: Johan Herland, Junio C Hamano, René Scharfe, Jonathan Nieder, Fabian Ruch. Thread: https://gitlist.dev/t/37479 ## Johan Herland, 2014-09-03 14:03 Subject: [RFC/PATCH 0/3] Teach revert/cherry-pick the --no-verify option Message-ID: <1409753034-9459-1-git-send-email-johan@herland.net> URL: https://gitlist.dev/e/1409753034-9459-1-git-send-email-johan%40herland.net ``` A colleague of mine noticed that cherry-pick does not accept the --no-verify option to skip running the pre-commit/commit-msg hooks. Here's a first attempt at adding --no-verify to the revert/cherry-pick. Have fun! :) ...Johan Johan Herland (3): t7503/4: Add failing testcases for revert/cherry-pick --no-verify revert/cherry-pick: Add --no-verify option, and pass it on to commit revert/cherry-pick --no-verify: Update documentation Documentation/git-cherry-pick.txt | 4 ++++ Documentation/git-revert.txt | 4 ++++ Documentation/githooks.txt | 20 ++++++++++---------- builtin/revert.c | 1 + sequencer.c | 7 +++++++ sequencer.h | 1 + t/t7503-pre-commit-hook.sh | 24 ++++++++++++++++++++++++ t/t7504-commit-msg-hook.sh | 24 ++++++++++++++++++++++++ 8 files changed, 75 insertions(+), 10 deletions(-) -- 2.0.0.rc4.501.gdaf83ca ``` ## Johan Herland, 2014-09-03 14:03 Subject: [RFC/PATCH 1/3] t7503/4: Add failing testcases for revert/cherry-pick --no-verify Message-ID: <1409753034-9459-2-git-send-email-johan@herland.net> URL: https://gitlist.dev/e/1409753034-9459-2-git-send-email-johan%40herland.net In-Reply-To: <1409753034-9459-1-git-send-email-johan@herland.net> ``` The revert/cherry-pick machinery currently exercises the pre-commit and commit-msg hooks. However, where commit accepts a --no-verify option to temporarily disable these hooks, the revert and cherry-pick commands have no such option. This patch adds some testcases demonstrating how the --no-verify option is supposed when it is added to revert and cherry-pick (in the next patch). Signed-off-by: Johan Herland --- t/t7503-pre-commit-hook.sh | 24 ++++++++++++++++++++++++ t/t7504-commit-msg-hook.sh | 24 ++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh index 984889b..adc892b 100755 --- a/t/t7503-pre-commit-hook.sh +++ b/t/t7503-pre-commit-hook.sh @@ -60,6 +60,18 @@ test_expect_success 'with failing hook' ' ' +test_expect_success 'revert with failing hook' ' + + test_must_fail git revert HEAD + +' + +test_expect_success 'cherry-pick with failing hook' ' + + test_must_fail git cherry-pick --no-verify HEAD^ + +' + test_expect_success '--no-verify with failing hook' ' echo "stuff" >> file && @@ -68,6 +80,18 @@ test_expect_success '--no-verify with failing hook' ' ' +test_expect_failure 'revert --no-verify with failing hook' ' + + git revert --no-verify HEAD + +' + +test_expect_failure 'cherry-pick --no-verify with failing hook' ' + + git cherry-pick --no-verify HEAD^ + +' + chmod -x "$HOOK" test_expect_success POSIXPERM 'with non-executable hook' ' diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh index 1f53ea8..4f8b9fe 100755 --- a/t/t7504-commit-msg-hook.sh +++ b/t/t7504-commit-msg-hook.sh @@ -109,6 +109,18 @@ test_expect_success 'with failing hook' ' ' +test_expect_success 'revert with failing hook' ' + + test_must_fail git revert HEAD + +' + +test_expect_success 'cherry-pick with failing hook' ' + + test_must_fail git cherry-pick --no-verify HEAD^ + +' + test_expect_success 'with failing hook (editor)' ' echo "more another" >> file && @@ -126,6 +138,18 @@ test_expect_success '--no-verify with failing hook' ' ' +test_expect_failure 'revert --no-verify with failing hook' ' + + git revert --no-verify HEAD + +' + +test_expect_failure 'cherry-pick --no-verify with failing hook' ' + + git cherry-pick --no-verify HEAD^ + +' + test_expect_success '--no-verify with failing hook (editor)' ' echo "more stuff" >> file && -- 2.0.0.rc4.501.gdaf83ca ``` ## Johan Herland, 2014-09-03 14:03 Subject: [RFC/PATCH 2/3] revert/cherry-pick: Add --no-verify option, and pass it on to commit Message-ID: <1409753034-9459-3-git-send-email-johan@herland.net> URL: https://gitlist.dev/e/1409753034-9459-3-git-send-email-johan%40herland.net In-Reply-To: <1409753034-9459-1-git-send-email-johan@herland.net> ``` Allow users to temporarily disable the pre-commit and commit-msg hooks when running "git revert" or "git cherry-pick", just like they currently can for "git commit". The --no-verify option is added to the sequencer machinery and handled like the other commit-related options. This fixes the failing t7503/t7504 test cases added previously. Signed-off-by: Johan Herland --- builtin/revert.c | 1 + sequencer.c | 7 +++++++ sequencer.h | 1 + t/t7503-pre-commit-hook.sh | 4 ++-- t/t7504-commit-msg-hook.sh | 4 ++-- 5 files changed, 13 insertions(+), 4 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index f9ed5bd..831c2cd 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -91,6 +91,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) N_("option for merge strategy"), option_parse_x), { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"), N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, + OPT_BOOL('n', "no-verify", &opts->no_verify, N_("bypass pre-commit hook")), OPT_END(), OPT_END(), OPT_END(), diff --git a/sequencer.c b/sequencer.c index 3c060e0..3d68113 100644 --- a/sequencer.c +++ b/sequencer.c @@ -378,6 +378,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts, if (opts->allow_empty_message) argv_array_push(&array, "--allow-empty-message"); + if (opts->no_verify) + argv_array_push(&array, "--no-verify"); + rc = run_command_v_opt(array.argv, RUN_GIT_CMD); argv_array_clear(&array); return rc; @@ -773,6 +776,8 @@ static int populate_opts_cb(const char *key, const char *value, void *data) opts->record_origin = git_config_bool_or_int(key, value, &error_flag); else if (!strcmp(key, "options.allow-ff")) opts->allow_ff = git_config_bool_or_int(key, value, &error_flag); + else if (!strcmp(key, "options.no-verify")) + opts->no_verify = git_config_bool_or_int(key, value, &error_flag); else if (!strcmp(key, "options.mainline")) opts->mainline = git_config_int(key, value); else if (!strcmp(key, "options.strategy")) @@ -944,6 +949,8 @@ static void save_opts(struct replay_opts *opts) git_config_set_in_file(opts_file, "options.record-origin", "true"); if (opts->allow_ff) git_config_set_in_file(opts_file, "options.allow-ff", "true"); + if (opts->no_verify) + git_config_set_in_file(opts_file, "options.no-verify", "true"); if (opts->mainline) { struct strbuf buf = STRBUF_INIT; strbuf_addf(&buf, "%d", opts->mainline); diff --git a/sequencer.h b/sequencer.h index db43e9c..abfadc0 100644 --- a/sequencer.h +++ b/sequencer.h @@ -34,6 +34,7 @@ struct replay_opts { int allow_empty; int allow_empty_message; int keep_redundant_commits; + int no_verify; int mainline; diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh index adc892b..b0307f4 100755 --- a/t/t7503-pre-commit-hook.sh +++ b/t/t7503-pre-commit-hook.sh @@ -80,13 +80,13 @@ test_expect_success '--no-verify with failing hook' ' ' -test_expect_failure 'revert --no-verify with failing hook' ' +test_expect_success 'revert --no-verify with failing hook' ' git revert --no-verify HEAD ' -test_expect_failure 'cherry-pick --no-verify with failing hook' ' +test_expect_success 'cherry-pick --no-verify with failing hook' ' git cherry-pick --no-verify HEAD^ diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh index 4f8b9fe..e819c25 100755 --- a/t/t7504-commit-msg-hook.sh +++ b/t/t7504-commit-msg-hook.sh @@ -138,13 +138,13 @@ test_expect_success '--no-verify with failing hook' ' ' -test_expect_failure 'revert --no-verify with failing hook' ' +test_expect_success 'revert --no-verify with failing hook' ' git revert --no-verify HEAD ' -test_expect_failure 'cherry-pick --no-verify with failing hook' ' +test_expect_success 'cherry-pick --no-verify with failing hook' ' git cherry-pick --no-verify HEAD^ -- 2.0.0.rc4.501.gdaf83ca ``` ## Johan Herland, 2014-09-03 14:03 Subject: [RFC/PATCH 3/3] revert/cherry-pick --no-verify: Update documentation Message-ID: <1409753034-9459-4-git-send-email-johan@herland.net> URL: https://gitlist.dev/e/1409753034-9459-4-git-send-email-johan%40herland.net In-Reply-To: <1409753034-9459-1-git-send-email-johan@herland.net> ``` Add --no-verify to the revert and cherry-pick man pages. Also mention revert and cherry-pick in the corresponding documentation for the pre-commit and commit-msg hooks. Signed-off-by: Johan Herland --- Documentation/git-cherry-pick.txt | 4 ++++ Documentation/git-revert.txt | 4 ++++ Documentation/githooks.txt | 20 ++++++++++---------- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt index 1c03c79..f56818f 100644 --- a/Documentation/git-cherry-pick.txt +++ b/Documentation/git-cherry-pick.txt @@ -97,6 +97,10 @@ OPTIONS This is useful when cherry-picking more than one commits' effect to your index in a row. +--no-verify:: + This option bypasses the pre-commit and commit-msg hooks. + See also linkgit:githooks[5]. + -s:: --signoff:: Add Signed-off-by line at the end of the commit message. diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt index cceb5f2..c9fb148 100644 --- a/Documentation/git-revert.txt +++ b/Documentation/git-revert.txt @@ -80,6 +80,10 @@ more details. This is useful when reverting more than one commits' effect to your index in a row. +--no-verify:: + This option bypasses the pre-commit and commit-msg hooks. + See also linkgit:githooks[5]. + -S[]:: --gpg-sign[=]:: GPG-sign commits. diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index d954bf6..9c3bf6c 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -72,11 +72,11 @@ the outcome of 'git am'. pre-commit ~~~~~~~~~~ -This hook is invoked by 'git commit', and can be bypassed -with `--no-verify` option. It takes no parameter, and is -invoked before obtaining the proposed commit log message and -making a commit. Exiting with non-zero status from this script -causes the 'git commit' to abort. +This hook is invoked by 'git commit' (including 'git revert' and +'git cherry-pick'), and can be bypassed with `--no-verify` option. +It takes no parameter, and is invoked before obtaining the proposed +commit log message and making a commit. Exiting with non-zero +status from this script causes the 'git commit' to abort. The default 'pre-commit' hook, when enabled, catches introduction of lines with trailing whitespaces and aborts the commit when @@ -114,11 +114,11 @@ out the `Conflicts:` part of a merge's commit message. commit-msg ~~~~~~~~~~ -This hook is invoked by 'git commit', and can be bypassed -with `--no-verify` option. It takes a single parameter, the -name of the file that holds the proposed commit log message. -Exiting with non-zero status causes the 'git commit' to -abort. +This hook is invoked by 'git commit' (including 'git revert' +and 'git cherry-pick'), and can be bypassed with `--no-verify` +option. It takes a single parameter, the name of the file that +holds the proposed commit log message. Exiting with non-zero +status causes the 'git commit' to abort. The hook is allowed to edit the message file in place, and can be used to normalize the message into some project standard -- 2.0.0.rc4.501.gdaf83ca ``` ## Junio C Hamano, 2014-09-03 19:21 Subject: Re: [RFC/PATCH 0/3] Teach revert/cherry-pick the --no-verify option Message-ID: URL: https://gitlist.dev/e/xmqqk35ky0fg.fsf%40gitster.dls.corp.google.com In-Reply-To: <1409753034-9459-1-git-send-email-johan@herland.net> ``` Johan Herland writes: > A colleague of mine noticed that cherry-pick does not accept the > --no-verify option to skip running the pre-commit/commit-msg hooks. > > Here's a first attempt at adding --no-verify to the revert/cherry-pick. Back when cherry-pick was a single commit operation, lack of the option did not matter very much, but it probably makes sense to allow telling the command that the entire series of commits is expected to be full of ones that do not verify. In the same vain, we already support --allow-empty and --allow-empty-message, so in that sense this change probably is an improvement. ``` ## Junio C Hamano, 2014-09-03 19:28 Subject: Re: [RFC/PATCH 1/3] t7503/4: Add failing testcases for revert/cherry-pick --no-verify Message-ID: URL: https://gitlist.dev/e/xmqqbnqwy03p.fsf%40gitster.dls.corp.google.com In-Reply-To: <1409753034-9459-2-git-send-email-johan@herland.net> ``` Johan Herland writes: > The revert/cherry-pick machinery currently exercises the pre-commit > and commit-msg hooks. However, where commit accepts a --no-verify > option to temporarily disable these hooks, the revert and cherry-pick > commands have no such option. > > This patch adds some testcases demonstrating how the --no-verify > option is supposed when it is added to revert and cherry-pick > (in the next patch). > > Signed-off-by: Johan Herland > --- The added test looks OK; will queue. We may want to update its style of testing (the shell scripting style is also bad, but they assume and depend on that the previous steps have all passed to take the history and the repository into a certain state without explicit "reset --hard" to allow some previous steps to fail), though. Also, do we already test these commands with the --allow-empty option and/or the --allow-empty-message option, which I think share the same issue, somewhere in the test suite? If not, we may want to while we remember the issue. Thanks. > t/t7503-pre-commit-hook.sh | 24 ++++++++++++++++++++++++ > t/t7504-commit-msg-hook.sh | 24 ++++++++++++++++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh > index 984889b..adc892b 100755 > --- a/t/t7503-pre-commit-hook.sh > +++ b/t/t7503-pre-commit-hook.sh > @@ -60,6 +60,18 @@ test_expect_success 'with failing hook' ' > > ' > > +test_expect_success 'revert with failing hook' ' > + > + test_must_fail git revert HEAD > + > +' > + > +test_expect_success 'cherry-pick with failing hook' ' > + > + test_must_fail git cherry-pick --no-verify HEAD^ > + > +' > + > test_expect_success '--no-verify with failing hook' ' > > echo "stuff" >> file && > @@ -68,6 +80,18 @@ test_expect_success '--no-verify with failing hook' ' > > ' > > +test_expect_failure 'revert --no-verify with failing hook' ' > + > + git revert --no-verify HEAD > + > +' > + > +test_expect_failure 'cherry-pick --no-verify with failing hook' ' > + > + git cherry-pick --no-verify HEAD^ > + > +' > + > chmod -x "$HOOK" > test_expect_success POSIXPERM 'with non-executable hook' ' > > diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh > index 1f53ea8..4f8b9fe 100755 > --- a/t/t7504-commit-msg-hook.sh > +++ b/t/t7504-commit-msg-hook.sh > @@ -109,6 +109,18 @@ test_expect_success 'with failing hook' ' > > ' > > +test_expect_success 'revert with failing hook' ' > + > + test_must_fail git revert HEAD > + > +' > + > +test_expect_success 'cherry-pick with failing hook' ' > + > + test_must_fail git cherry-pick --no-verify HEAD^ > + > +' > + > test_expect_success 'with failing hook (editor)' ' > > echo "more another" >> file && > @@ -126,6 +138,18 @@ test_expect_success '--no-verify with failing hook' ' > > ' > > +test_expect_failure 'revert --no-verify with failing hook' ' > + > + git revert --no-verify HEAD > + > +' > + > +test_expect_failure 'cherry-pick --no-verify with failing hook' ' > + > + git cherry-pick --no-verify HEAD^ > + > +' > + > test_expect_success '--no-verify with failing hook (editor)' ' > > echo "more stuff" >> file && ``` ## Junio C Hamano, 2014-09-03 19:32 Subject: Re: [RFC/PATCH 2/3] revert/cherry-pick: Add --no-verify option, and pass it on to commit Message-ID: URL: https://gitlist.dev/e/xmqq7g1kxzxi.fsf%40gitster.dls.corp.google.com In-Reply-To: <1409753034-9459-3-git-send-email-johan@herland.net> ``` Johan Herland writes: > Allow users to temporarily disable the pre-commit and commit-msg hooks > when running "git revert" or "git cherry-pick", just like they currently > can for "git commit". > > The --no-verify option is added to the sequencer machinery and handled > like the other commit-related options. > > This fixes the failing t7503/t7504 test cases added previously. > > Signed-off-by: Johan Herland > --- > builtin/revert.c | 1 + > sequencer.c | 7 +++++++ > sequencer.h | 1 + > t/t7503-pre-commit-hook.sh | 4 ++-- > t/t7504-commit-msg-hook.sh | 4 ++-- > 5 files changed, 13 insertions(+), 4 deletions(-) > > diff --git a/builtin/revert.c b/builtin/revert.c > index f9ed5bd..831c2cd 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -91,6 +91,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) > N_("option for merge strategy"), option_parse_x), > { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"), > N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, > + OPT_BOOL('n', "no-verify", &opts->no_verify, N_("bypass pre-commit hook")), I doubt we want this option to squat on '-n'; besides, it is already taken by a more often used "--no-commit". I thought that we added sanity checker for the options[] array to parse-options API. I wonder why it did not kick in... > OPT_END(), > OPT_END(), > OPT_END(), > diff --git a/sequencer.c b/sequencer.c > index 3c060e0..3d68113 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -378,6 +378,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts, > if (opts->allow_empty_message) > argv_array_push(&array, "--allow-empty-message"); > > + if (opts->no_verify) > + argv_array_push(&array, "--no-verify"); > + > rc = run_command_v_opt(array.argv, RUN_GIT_CMD); > argv_array_clear(&array); > return rc; > @@ -773,6 +776,8 @@ static int populate_opts_cb(const char *key, const char *value, void *data) > opts->record_origin = git_config_bool_or_int(key, value, &error_flag); > else if (!strcmp(key, "options.allow-ff")) > opts->allow_ff = git_config_bool_or_int(key, value, &error_flag); > + else if (!strcmp(key, "options.no-verify")) > + opts->no_verify = git_config_bool_or_int(key, value, &error_flag); > else if (!strcmp(key, "options.mainline")) > opts->mainline = git_config_int(key, value); > else if (!strcmp(key, "options.strategy")) > @@ -944,6 +949,8 @@ static void save_opts(struct replay_opts *opts) > git_config_set_in_file(opts_file, "options.record-origin", "true"); > if (opts->allow_ff) > git_config_set_in_file(opts_file, "options.allow-ff", "true"); > + if (opts->no_verify) > + git_config_set_in_file(opts_file, "options.no-verify", "true"); > if (opts->mainline) { > struct strbuf buf = STRBUF_INIT; > strbuf_addf(&buf, "%d", opts->mainline); > diff --git a/sequencer.h b/sequencer.h > index db43e9c..abfadc0 100644 > --- a/sequencer.h > +++ b/sequencer.h > @@ -34,6 +34,7 @@ struct replay_opts { > int allow_empty; > int allow_empty_message; > int keep_redundant_commits; > + int no_verify; > > int mainline; > > diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh > index adc892b..b0307f4 100755 > --- a/t/t7503-pre-commit-hook.sh > +++ b/t/t7503-pre-commit-hook.sh > @@ -80,13 +80,13 @@ test_expect_success '--no-verify with failing hook' ' > > ' > > -test_expect_failure 'revert --no-verify with failing hook' ' > +test_expect_success 'revert --no-verify with failing hook' ' > > git revert --no-verify HEAD > > ' > > -test_expect_failure 'cherry-pick --no-verify with failing hook' ' > +test_expect_success 'cherry-pick --no-verify with failing hook' ' > > git cherry-pick --no-verify HEAD^ > > diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh > index 4f8b9fe..e819c25 100755 > --- a/t/t7504-commit-msg-hook.sh > +++ b/t/t7504-commit-msg-hook.sh > @@ -138,13 +138,13 @@ test_expect_success '--no-verify with failing hook' ' > > ' > > -test_expect_failure 'revert --no-verify with failing hook' ' > +test_expect_success 'revert --no-verify with failing hook' ' > > git revert --no-verify HEAD > > ' > > -test_expect_failure 'cherry-pick --no-verify with failing hook' ' > +test_expect_success 'cherry-pick --no-verify with failing hook' ' > > git cherry-pick --no-verify HEAD^ ``` ## Junio C Hamano, 2014-09-03 19:42 Subject: [PATCH] parse-options: detect attempt to add a duplicate short option name Message-ID: URL: https://gitlist.dev/e/xmqq1trsxzgy.fsf_-_%40gitster.dls.corp.google.com In-Reply-To: ``` Junio C Hamano writes: >> diff --git a/builtin/revert.c b/builtin/revert.c >> index f9ed5bd..831c2cd 100644 >> --- a/builtin/revert.c >> +++ b/builtin/revert.c >> @@ -91,6 +91,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) >> N_("option for merge strategy"), option_parse_x), >> { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"), >> N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, >> + OPT_BOOL('n', "no-verify", &opts->no_verify, N_("bypass pre-commit hook")), > > I doubt we want this option to squat on '-n'; besides, it is already > taken by a more often used "--no-commit". > > I thought that we added sanity checker for the options[] array to parse-options > API. I wonder why it did not kick in... ... because we didn't, not quite. Perhaps like this? -- >8 -- It is easy to overlook an already assigned single-letter option name and try to use it for a new one. Help the developer to catch it before such a mistake escapes the lab. Signed-off-by: Junio C Hamano --- diff --git a/parse-options.c b/parse-options.c index e7dafa8..b7925c5 100644 --- a/parse-options.c +++ b/parse-options.c @@ -347,12 +347,17 @@ static void check_typos(const char *arg, const struct option *options) static void parse_options_check(const struct option *opts) { int err = 0; + char short_opts[128]; + + memset(short_opts, '\0', sizeof(short_opts)); for (; opts->type != OPTION_END; opts++) { if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) && (opts->flags & PARSE_OPT_OPTARG)) err |= optbug(opts, "uses incompatible flags " "LASTARG_DEFAULT and OPTARG"); + if (opts->short_name && short_opts[opts->short_name]++) + err |= optbug(opts, "short name already used"); if (opts->flags & PARSE_OPT_NODASH && ((opts->flags & PARSE_OPT_OPTARG) || !(opts->flags & PARSE_OPT_NOARG) || ``` ## René Scharfe, 2014-09-03 20:29 Subject: Re: [PATCH] parse-options: detect attempt to add a duplicate short option name Message-ID: <54077A3E.20703@web.de> URL: https://gitlist.dev/e/54077A3E.20703%40web.de In-Reply-To: ``` Am 03.09.2014 um 21:42 schrieb Junio C Hamano: > Junio C Hamano writes: > >>> diff --git a/builtin/revert.c b/builtin/revert.c >>> index f9ed5bd..831c2cd 100644 >>> --- a/builtin/revert.c >>> +++ b/builtin/revert.c >>> @@ -91,6 +91,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) >>> N_("option for merge strategy"), option_parse_x), >>> { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"), >>> N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, >>> + OPT_BOOL('n', "no-verify", &opts->no_verify, N_("bypass pre-commit hook")), >> >> I doubt we want this option to squat on '-n'; besides, it is already >> taken by a more often used "--no-commit". >> >> I thought that we added sanity checker for the options[] array to parse-options >> API. I wonder why it did not kick in... > > ... because we didn't, not quite. > > Perhaps like this? > > -- >8 -- > It is easy to overlook an already assigned single-letter option name > and try to use it for a new one. Help the developer to catch it > before such a mistake escapes the lab. > > Signed-off-by: Junio C Hamano > --- > diff --git a/parse-options.c b/parse-options.c > index e7dafa8..b7925c5 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -347,12 +347,17 @@ static void check_typos(const char *arg, const struct option *options) > static void parse_options_check(const struct option *opts) > { > int err = 0; > + char short_opts[128]; > + > + memset(short_opts, '\0', sizeof(short_opts)); > > for (; opts->type != OPTION_END; opts++) { > if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) && > (opts->flags & PARSE_OPT_OPTARG)) > err |= optbug(opts, "uses incompatible flags " > "LASTARG_DEFAULT and OPTARG"); > + if (opts->short_name && short_opts[opts->short_name]++) > + err |= optbug(opts, "short name already used"); > if (opts->flags & PARSE_OPT_NODASH && > ((opts->flags & PARSE_OPT_OPTARG) || > !(opts->flags & PARSE_OPT_NOARG) || > Compact and useful, I like it. You might want to squash in something like this, though. Without it t1502 fails because -b is defined twice there. --- t/t1502-rev-parse-parseopt.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh index 922423e..ebe7c3b 100755 --- a/t/t1502-rev-parse-parseopt.sh +++ b/t/t1502-rev-parse-parseopt.sh @@ -19,7 +19,7 @@ sed -e 's/^|//' >expect <<\END_EXPECT | -d, --data[=...] short and long option with an optional argument | |Argument hints -| -b short option required argument +| -B short option required argument | --bar2 long option required argument | -e, --fuz | short and long option required argument @@ -51,7 +51,7 @@ sed -e 's/^|//' >optionspec <<\EOF |d,data? short and long option with an optional argument | | Argument hints -|b=arg short option required argument +|B=arg short option required argument |bar2=arg long option required argument |e,fuz=with-space short and long option required argument |s?some short option optional argument -- 2.1.0 ``` ## Junio C Hamano, 2014-09-03 21:05 Subject: Re: [PATCH] parse-options: detect attempt to add a duplicate short option name Message-ID: URL: https://gitlist.dev/e/xmqqoauwwh2c.fsf%40gitster.dls.corp.google.com In-Reply-To: <54077A3E.20703@web.de> ``` René Scharfe writes: > Compact and useful, I like it. > > You might want to squash in something like this, though. Without it > t1502 fails because -b is defined twice there. Thanks. I like it to see that the check automatically propagates even to scripts ;-) It bugged me enough that we didn't identify which short option letter we were complaining about and that opts->short_name is defined as an "int", which may cause us to overstep char[128], I ended up doing it this way instead, though. It no longer is so compact, even though it may still have the same usefulness. We might want to tighten the type of the short_name member to unsigned char, but I didn't go that far yet, at least in this step. -- >8 -- Subject: [PATCH] parse-options: detect attempt to add a duplicate short option name It is easy to overlook an already assigned single-letter option name and try to use it for a new one. Help the developer to catch it before such a mistake escapes the lab. Helped-by: René Scharfe Signed-off-by: Junio C Hamano --- parse-options.c | 15 +++++++++++++++ t/t1502-rev-parse-parseopt.sh | 4 ++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/parse-options.c b/parse-options.c index b536896..70227e9 100644 --- a/parse-options.c +++ b/parse-options.c @@ -345,12 +345,27 @@ static void check_typos(const char *arg, const struct option *options) static void parse_options_check(const struct option *opts) { int err = 0; + char short_opts[128]; + + memset(short_opts, '\0', sizeof(short_opts)); for (; opts->type != OPTION_END; opts++) { if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) && (opts->flags & PARSE_OPT_OPTARG)) err |= optbug(opts, "uses incompatible flags " "LASTARG_DEFAULT and OPTARG"); + if (opts->short_name) { + struct strbuf errmsg = STRBUF_INIT; + if (opts->short_name < ' ' || 0x7F <= opts->short_name) + strbuf_addf(&errmsg, "invalid short name (0x%02x)", + opts->short_name); + else if (short_opts[opts->short_name]++) + strbuf_addf(&errmsg, "short name %c already used", + opts->short_name); + if (errmsg.len) + err |= optbug(opts, errmsg.buf); + strbuf_release(&errmsg); + } if (opts->flags & PARSE_OPT_NODASH && ((opts->flags & PARSE_OPT_OPTARG) || !(opts->flags & PARSE_OPT_NOARG) || diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh index 922423e..ebe7c3b 100755 --- a/t/t1502-rev-parse-parseopt.sh +++ b/t/t1502-rev-parse-parseopt.sh @@ -19,7 +19,7 @@ sed -e 's/^|//' >expect <<\END_EXPECT | -d, --data[=...] short and long option with an optional argument | |Argument hints -| -b short option required argument +| -B short option required argument | --bar2 long option required argument | -e, --fuz | short and long option required argument @@ -51,7 +51,7 @@ sed -e 's/^|//' >optionspec <<\EOF |d,data? short and long option with an optional argument | | Argument hints -|b=arg short option required argument +|B=arg short option required argument |bar2=arg long option required argument |e,fuz=with-space short and long option required argument |s?some short option optional argument -- 2.1.0-394-g3e31896 ``` ## René Scharfe, 2014-09-03 21:46 Subject: Re: [PATCH] parse-options: detect attempt to add a duplicate short option name Message-ID: <54078C2C.5020503@web.de> URL: https://gitlist.dev/e/54078C2C.5020503%40web.de In-Reply-To: ``` Am 03.09.2014 um 23:05 schrieb Junio C Hamano: > René Scharfe writes: > >> Compact and useful, I like it. >> >> You might want to squash in something like this, though. Without it >> t1502 fails because -b is defined twice there. > > Thanks. I like it to see that the check automatically propagates > even to scripts ;-) > > It bugged me enough that we didn't identify which short option > letter we were complaining about The old code did report the short option. E.g. for t1502 it said: error: BUG: switch 'b' short name already used You can leave that to optbug(), no need for the strbuf. > and that opts->short_name is > defined as an "int", which may cause us to overstep char[128], > I ended up doing it this way instead, though. It no longer is so > compact, even though it may still have the same usefulness. A range check is an additional feature (increased usefulness). I guess using invalid characters is not that common a mistake, though. Space is allowed as a short option by the code; intentionally? > > We might want to tighten the type of the short_name member to > unsigned char, but I didn't go that far yet, at least in this step. > > -- >8 -- > Subject: [PATCH] parse-options: detect attempt to add a duplicate short option name > > It is easy to overlook an already assigned single-letter option name > and try to use it for a new one. Help the developer to catch it > before such a mistake escapes the lab. > > Helped-by: René Scharfe > Signed-off-by: Junio C Hamano > --- > parse-options.c | 15 +++++++++++++++ > t/t1502-rev-parse-parseopt.sh | 4 ++-- > 2 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/parse-options.c b/parse-options.c > index b536896..70227e9 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -345,12 +345,27 @@ static void check_typos(const char *arg, const struct option *options) > static void parse_options_check(const struct option *opts) > { > int err = 0; > + char short_opts[128]; > + > + memset(short_opts, '\0', sizeof(short_opts)); > > for (; opts->type != OPTION_END; opts++) { > if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) && > (opts->flags & PARSE_OPT_OPTARG)) > err |= optbug(opts, "uses incompatible flags " > "LASTARG_DEFAULT and OPTARG"); > + if (opts->short_name) { > + struct strbuf errmsg = STRBUF_INIT; > + if (opts->short_name < ' ' || 0x7F <= opts->short_name) > + strbuf_addf(&errmsg, "invalid short name (0x%02x)", > + opts->short_name); > + else if (short_opts[opts->short_name]++) > + strbuf_addf(&errmsg, "short name %c already used", > + opts->short_name); > + if (errmsg.len) > + err |= optbug(opts, errmsg.buf); > + strbuf_release(&errmsg); > + } > if (opts->flags & PARSE_OPT_NODASH && > ((opts->flags & PARSE_OPT_OPTARG) || > !(opts->flags & PARSE_OPT_NOARG) || > diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh > index 922423e..ebe7c3b 100755 > --- a/t/t1502-rev-parse-parseopt.sh > +++ b/t/t1502-rev-parse-parseopt.sh > @@ -19,7 +19,7 @@ sed -e 's/^|//' >expect <<\END_EXPECT > | -d, --data[=...] short and long option with an optional argument > | > |Argument hints > -| -b short option required argument > +| -B short option required argument > | --bar2 long option required argument > | -e, --fuz > | short and long option required argument > @@ -51,7 +51,7 @@ sed -e 's/^|//' >optionspec <<\EOF > |d,data? short and long option with an optional argument > | > | Argument hints > -|b=arg short option required argument > +|B=arg short option required argument > |bar2=arg long option required argument > |e,fuz=with-space short and long option required argument > |s?some short option optional argument > ``` ## Jonathan Nieder, 2014-09-03 21:46 Subject: Re: [PATCH] parse-options: detect attempt to add a duplicate short option name Message-ID: <20140903214624.GY18279@google.com> URL: https://gitlist.dev/e/20140903214624.GY18279%40google.com In-Reply-To: ``` Junio C Hamano wrote: > --- a/parse-options.c > +++ b/parse-options.c > @@ -345,12 +345,27 @@ static void check_typos(const char *arg, const struct option *options) > static void parse_options_check(const struct option *opts) > { > int err = 0; > + char short_opts[128]; > + > + memset(short_opts, '\0', sizeof(short_opts)); > > for (; opts->type != OPTION_END; opts++) { > if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) && > (opts->flags & PARSE_OPT_OPTARG)) > err |= optbug(opts, "uses incompatible flags " > "LASTARG_DEFAULT and OPTARG"); > + if (opts->short_name) { > + struct strbuf errmsg = STRBUF_INIT; > + if (opts->short_name < ' ' || 0x7F <= opts->short_name) > + strbuf_addf(&errmsg, "invalid short name (0x%02x)", > + opts->short_name); > + else if (short_opts[opts->short_name]++) What happens on platforms with a signed char? With the following squashed in, Reviewed-by: Jonathan Nieder diff --git i/parse-options.c w/parse-options.c index f7f153a..4cc3f3e 100644 --- i/parse-options.c +++ w/parse-options.c @@ -361,7 +361,7 @@ static void parse_options_check(const struct option *opts) if (opts->short_name < ' ' || 0x7F <= opts->short_name) strbuf_addf(&errmsg, "invalid short name (0x%02x)", opts->short_name); - else if (short_opts[opts->short_name]++) + else if (short_opts[(unsigned char) opts->short_name]++) strbuf_addf(&errmsg, "short name %c already used", opts->short_name); if (errmsg.len) ``` ## Jonathan Nieder, 2014-09-03 21:58 Subject: Re: [PATCH] parse-options: detect attempt to add a duplicate short option name Message-ID: <20140903215834.GZ18279@google.com> URL: https://gitlist.dev/e/20140903215834.GZ18279%40google.com In-Reply-To: <20140903214624.GY18279@google.com> ``` On Wed, Sep 03, 2014 at 02:46:25PM -0700, Jonathan Nieder wrote: > Junio C Hamano wrote: > > > --- a/parse-options.c > > +++ b/parse-options.c > > @@ -345,12 +345,27 @@ static void check_typos(const char *arg, const struct option *options) > > static void parse_options_check(const struct option *opts) > > { > > int err = 0; > > + char short_opts[128]; > > + > > + memset(short_opts, '\0', sizeof(short_opts)); > > > > for (; opts->type != OPTION_END; opts++) { > > if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) && > > (opts->flags & PARSE_OPT_OPTARG)) > > err |= optbug(opts, "uses incompatible flags " > > "LASTARG_DEFAULT and OPTARG"); > > + if (opts->short_name) { > > + struct strbuf errmsg = STRBUF_INIT; > > + if (opts->short_name < ' ' || 0x7F <= opts->short_name) > > + strbuf_addf(&errmsg, "invalid short name (0x%02x)", > > + opts->short_name); > > + else if (short_opts[opts->short_name]++) > > What happens on platforms with a signed char? Ah, I see now that the "< ' '" check would catch that. With René's suggestions squashed in, that becomes parse-options.c | 9 +++++++++ t/t1502-rev-parse-parseopt.sh | 4 ++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/parse-options.c b/parse-options.c index e7dafa8..6ad7d90 100644 --- a/parse-options.c +++ b/parse-options.c @@ -347,12 +347,21 @@ static void check_typos(const char *arg, const struct option *options) static void parse_options_check(const struct option *opts) { int err = 0; + char short_opts[128]; + + memset(short_opts, '\0', sizeof(short_opts)); for (; opts->type != OPTION_END; opts++) { if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) && (opts->flags & PARSE_OPT_OPTARG)) err |= optbug(opts, "uses incompatible flags " "LASTARG_DEFAULT and OPTARG"); + if (opts->short_name) { + if (opts->short_name <= ' ' || 0x7F <= opts->short_name) + err |= optbug(opts, "invalid short name"); + else if (short_opts[opts->short_name]++) + err |= optbug(opts, "short name already used"); + } if (opts->flags & PARSE_OPT_NODASH && ((opts->flags & PARSE_OPT_OPTARG) || !(opts->flags & PARSE_OPT_NOARG) || diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh index 922423e..ebe7c3b 100755 --- a/t/t1502-rev-parse-parseopt.sh +++ b/t/t1502-rev-parse-parseopt.sh @@ -19,7 +19,7 @@ sed -e 's/^|//' >expect <<\END_EXPECT | -d, --data[=...] short and long option with an optional argument | |Argument hints -| -b short option required argument +| -B short option required argument | --bar2 long option required argument | -e, --fuz | short and long option required argument @@ -51,7 +51,7 @@ sed -e 's/^|//' >optionspec <<\EOF |d,data? short and long option with an optional argument | | Argument hints -|b=arg short option required argument +|B=arg short option required argument |bar2=arg long option required argument |e,fuz=with-space short and long option required argument |s?some short option optional argument -- 2.1.0.rc2.206.gedb03e5 ``` ## Junio C Hamano, 2014-09-03 22:16 Subject: Re: [PATCH] parse-options: detect attempt to add a duplicate short option name Message-ID: URL: https://gitlist.dev/e/xmqqbnqwwds2.fsf%40gitster.dls.corp.google.com In-Reply-To: <54078C2C.5020503@web.de> ``` René Scharfe writes: >> It bugged me enough that we didn't identify which short option >> letter we were complaining about > > The old code did report the short option. E.g. for t1502 it said: > > error: BUG: switch 'b' short name already used > > You can leave that to optbug(), no need for the strbuf. Not quite, as an opt with long name is reported with the long name only, which is not very nice when the problem we are reporting is about its short variant. > Space is allowed as a short option by the code; intentionally? I didn't think of a strong reason to declare either way, so, yes it was deliberate that I didn't tighten to disallow. ``` ## René Scharfe, 2014-09-04 06:13 Subject: Re: [PATCH] parse-options: detect attempt to add a duplicate short option name Message-ID: <540802F5.1070708@web.de> URL: https://gitlist.dev/e/540802F5.1070708%40web.de In-Reply-To: ``` Am 04.09.2014 um 00:16 schrieb Junio C Hamano: > René Scharfe writes: > >>> It bugged me enough that we didn't identify which short option >>> letter we were complaining about >> >> The old code did report the short option. E.g. for t1502 it said: >> >> error: BUG: switch 'b' short name already used >> >> You can leave that to optbug(), no need for the strbuf. > > Not quite, as an opt with long name is reported with the long name > only, which is not very nice when the problem we are reporting is > about its short variant. Perhaps something like the patch below helps, here and in general? >> Space is allowed as a short option by the code; intentionally? > > I didn't think of a strong reason to declare either way, so, yes it > was deliberate that I didn't tighten to disallow. OK. I don't think it's easy to come up with a usable way for having space as a short option, but maybe it's possible. --- parse-options.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/parse-options.c b/parse-options.c index b7925c5..f1c0b5d 100644 --- a/parse-options.c +++ b/parse-options.c @@ -14,8 +14,12 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx, int optbug(const struct option *opt, const char *reason) { - if (opt->long_name) + if (opt->long_name) { + if (opt->short_name) + return error("BUG: switch '%c' (--%s) %s", + opt->short_name, opt->long_name, reason); return error("BUG: option '%s' %s", opt->long_name, reason); + } return error("BUG: switch '%c' %s", opt->short_name, reason); } -- 2.1.0 ``` ## Johan Herland, 2014-09-04 08:34 Subject: Re: [RFC/PATCH 2/3] revert/cherry-pick: Add --no-verify option, and pass it on to commit Message-ID: URL: https://gitlist.dev/e/CALKQrgcdEMcFWhT7ZSKYR23QL7%3D_CKXRKxwZ_nOCSzRzPk0TGg%40mail.gmail.com In-Reply-To: ``` On Wed, Sep 3, 2014 at 9:32 PM, Junio C Hamano wrote: > Johan Herland writes: >> diff --git a/builtin/revert.c b/builtin/revert.c >> index f9ed5bd..831c2cd 100644 >> --- a/builtin/revert.c >> +++ b/builtin/revert.c >> @@ -91,6 +91,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) >> N_("option for merge strategy"), option_parse_x), >> { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"), >> N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, >> + OPT_BOOL('n', "no-verify", &opts->no_verify, N_("bypass pre-commit hook")), > > I doubt we want this option to squat on '-n'; besides, it is already > taken by a more often used "--no-commit". Oops. I never meant to squat on '-n'. Will fix in re-roll. ...Johan -- Johan Herland, www.herland.net ``` ## Junio C Hamano, 2014-09-04 17:24 Subject: Re: [PATCH] parse-options: detect attempt to add a duplicate short option name Message-ID: URL: https://gitlist.dev/e/xmqqegvruwmz.fsf%40gitster.dls.corp.google.com In-Reply-To: <540802F5.1070708@web.de> ``` René Scharfe writes: >> Not quite, as an opt with long name is reported with the long name >> only, which is not very nice when the problem we are reporting is >> about its short variant. > > Perhaps something like the patch below helps, here and in general? Excellent. Not just this particular case, but we would show both when both are available. Thanks; will reroll. > parse-options.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/parse-options.c b/parse-options.c > index b7925c5..f1c0b5d 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -14,8 +14,12 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx, > > int optbug(const struct option *opt, const char *reason) > { > - if (opt->long_name) > + if (opt->long_name) { > + if (opt->short_name) > + return error("BUG: switch '%c' (--%s) %s", > + opt->short_name, opt->long_name, reason); > return error("BUG: option '%s' %s", opt->long_name, reason); > + } > return error("BUG: switch '%c' %s", opt->short_name, reason); > } ``` ## Junio C Hamano, 2014-09-04 18:07 Subject: Re: [PATCH] parse-options: detect attempt to add a duplicate short option name Message-ID: URL: https://gitlist.dev/e/xmqqtx4ntg33.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` It is easy to overlook an already assigned single-letter option name and try to use it for a new one. Help the developer to catch it before such a mistake escapes the lab. This retroactively forbids any short option name (which is defined to be of type "int") outside the ASCII printable range. We might want to do one of two things: - tighten the type of short_name member to 'char', and further update optbug() to protect it against doing "'%c'" on a funny value, e.g. negative or above 127. - drop the check (even the "duplicate" check) for an option whose short_name is either negative or above 255, to allow clever folks to take advantage of the fact that such a short_name cannot be parsed from the command line and the member can be used to store some extra information. Helped-by: René Scharfe Signed-off-by: Junio C Hamano --- Junio C Hamano writes: > René Scharfe writes: > >>> Not quite, as an opt with long name is reported with the long name >>> only, which is not very nice when the problem we are reporting is >>> about its short variant. >> >> Perhaps something like the patch below helps, here and in general? > > Excellent. Not just this particular case, but we would show both > when both are available. > > Thanks; will reroll. parse-options.c | 14 +++++++++++++- t/t1502-rev-parse-parseopt.sh | 4 ++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/parse-options.c b/parse-options.c index b536896..34a15aa 100644 --- a/parse-options.c +++ b/parse-options.c @@ -14,8 +14,12 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx, int optbug(const struct option *opt, const char *reason) { - if (opt->long_name) + if (opt->long_name) { + if (opt->short_name) + return error("BUG: switch '%c' (--%s) %s", + opt->short_name, opt->long_name, reason); return error("BUG: option '%s' %s", opt->long_name, reason); + } return error("BUG: switch '%c' %s", opt->short_name, reason); } @@ -345,12 +349,20 @@ static void check_typos(const char *arg, const struct option *options) static void parse_options_check(const struct option *opts) { int err = 0; + char short_opts[128]; + memset(short_opts, '\0', sizeof(short_opts)); for (; opts->type != OPTION_END; opts++) { if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) && (opts->flags & PARSE_OPT_OPTARG)) err |= optbug(opts, "uses incompatible flags " "LASTARG_DEFAULT and OPTARG"); + if (opts->short_name) { + if (0x7F <= opts->short_name) + err |= optbug(opts, "invalid short name"); + else if (short_opts[opts->short_name]++) + err |= optbug(opts, "short name already used"); + } if (opts->flags & PARSE_OPT_NODASH && ((opts->flags & PARSE_OPT_OPTARG) || !(opts->flags & PARSE_OPT_NOARG) || diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh index 922423e..ebe7c3b 100755 --- a/t/t1502-rev-parse-parseopt.sh +++ b/t/t1502-rev-parse-parseopt.sh @@ -19,7 +19,7 @@ sed -e 's/^|//' >expect <<\END_EXPECT | -d, --data[=...] short and long option with an optional argument | |Argument hints -| -b short option required argument +| -B short option required argument | --bar2 long option required argument | -e, --fuz | short and long option required argument @@ -51,7 +51,7 @@ sed -e 's/^|//' >optionspec <<\EOF |d,data? short and long option with an optional argument | | Argument hints -|b=arg short option required argument +|B=arg short option required argument |bar2=arg long option required argument |e,fuz=with-space short and long option required argument |s?some short option optional argument -- 2.1.0-396-ga35a9df ``` ## Fabian Ruch, 2014-09-05 21:05 Subject: Re: [RFC/PATCH 0/3] Teach revert/cherry-pick the --no-verify option Message-ID: <540A2598.8000101@gmail.com> URL: https://gitlist.dev/e/540A2598.8000101%40gmail.com In-Reply-To: <1409753034-9459-1-git-send-email-johan@herland.net> ``` Hi Johan, Johan Herland writes: > A colleague of mine noticed that cherry-pick does not accept the > --no-verify option to skip running the pre-commit/commit-msg hooks. neither git-cherry-pick nor git-revert execute the pre-commit or commit-msg hooks at the moment. The underlying rationale can be found in the log message of commit 9fa4db5 ("Do not verify reverted/cherry-picked/rebased patches."). Indeed, the sequencer uses git-commit internally which executes the two verify hooks by default. However, the particular command line being used implicitly specifies the --no-verify option. This behaviour is implemented in sequencer.c#run_git_commit as well, right before the configurable git-commit options are handled. I guess that's easily overlooked since the documentation doesn't mention it and the implementation uses the short version -n of --no-verify. The reasons why the new test cases succeed nonetheless are manifold. I hope they're still understandable even though I don't put the comments next to the code. The "revert with failing hook" test case fails if run in isolation, which can be achieved by using the very cool --run option of test-lib. More specifically, git-revert does not fail because it executes the failing hook but because the preceding test case leaves behind an uncommitted index. In the "cherry-pick with failing hook" test case, git-cherry-pick really fails because it doesn't know the --no-verify option yet, which presumably ended up there only by accident. This test case is meaningless if run in isolation because it assumes that "revert with failing hook" creates a commit (else HEAD^ points nowhere). I like your patchset for that it makes it explicit in both the documentation and the tests whether the commits resulting from cherry-picks are being verified or not. Kind regards, Fabian > Here's a first attempt at adding --no-verify to the revert/cherry-pick. > > Have fun! :) > > ...Johan > > Johan Herland (3): > t7503/4: Add failing testcases for revert/cherry-pick --no-verify > revert/cherry-pick: Add --no-verify option, and pass it on to commit > revert/cherry-pick --no-verify: Update documentation > > Documentation/git-cherry-pick.txt | 4 ++++ > Documentation/git-revert.txt | 4 ++++ > Documentation/githooks.txt | 20 ++++++++++---------- > builtin/revert.c | 1 + > sequencer.c | 7 +++++++ > sequencer.h | 1 + > t/t7503-pre-commit-hook.sh | 24 ++++++++++++++++++++++++ > t/t7504-commit-msg-hook.sh | 24 ++++++++++++++++++++++++ > 8 files changed, 75 insertions(+), 10 deletions(-) ``` ## Johan Herland, 2014-09-08 15:13 Subject: Re: [RFC/PATCH 0/3] Teach revert/cherry-pick the --no-verify option Message-ID: URL: https://gitlist.dev/e/CALKQrgewtPOFhqjH_zoY3q922DgzXxHfuUF%3DsS86on-r4SJYeA%40mail.gmail.com In-Reply-To: <540A2598.8000101@gmail.com> ``` On Fri, Sep 5, 2014 at 11:05 PM, Fabian Ruch wrote: > neither git-cherry-pick nor git-revert execute the pre-commit or > commit-msg hooks at the moment. The underlying rationale can be found in > the log message of commit 9fa4db5 ("Do not verify > reverted/cherry-picked/rebased patches."). Indeed, the sequencer uses > git-commit internally which executes the two verify hooks by default. > However, the particular command line being used implicitly specifies the > --no-verify option. This behaviour is implemented in > sequencer.c#run_git_commit as well, right before the configurable > git-commit options are handled. I guess that's easily overlooked since > the documentation doesn't mention it and the implementation uses the > short version -n of --no-verify. Damn. You're obviously correct, and my patch series is seriously misguided. Please drop it, Junio, and I'm very sorry for the noise. Hopefully, I will learn not to blindly follow my assumptions. ...Johan -- Johan Herland, www.herland.net ```