threads / rfc / 37479

RFC patch, 3 partsrevert/cherry-pick --no-verify: Update documentation

Subject: [RFC/PATCH 3/3] revert/cherry-pick --no-verify: Update documentation

## tl;dr

20 messages between Sep 3, 2014 and Sep 8, 2014. Diffs are folded; open one to read it.

replies: 19people: 5as markdown or json

Johan Herland· Sep 3, 2014, 14:03 UTC · lore

[RFC/PATCH 0/3] Teach revert/cherry-pick the --no-verify option

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· Sep 3, 2014, 14:03 UTC · re: Johan Herland · lore

[RFC/PATCH 1/3] t7503/4: Add failing testcases for revert/cherry-pick --no-verify

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 <johan@herland.net>
---
 t/t7503-pre-commit-hook.sh | 24 ++++++++++++++++++++++++
 t/t7504-commit-msg-hook.sh | 24 ++++++++++++++++++++++++
 2 files changed, 48 insertions(+)
Show changes to 2 files +48 −0

t/t7503-pre-commit-hook.sh, t/t7504-commit-msg-hook.sh

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
Junio C Hamano· Sep 3, 2014, 19:28 UTC · re: Johan Herland · lore

Re: [RFC/PATCH 1/3] t7503/4: Add failing testcases for revert/cherry-pick --no-verify

Johan Herland <johan@herland.net> writes:
Show 11 quoted lines
> 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 <johan@herland.net>
> ---
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.
Show 88 quoted lines
>  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 &&
Johan Herland· Sep 3, 2014, 14:03 UTC · re: Johan Herland · lore

[RFC/PATCH 2/3] revert/cherry-pick: Add --no-verify option, and pass it on to commit

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 <johan@herland.net>
---
 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(-)
Show changes to 5 files +13 −4

builtin/revert.c, sequencer.c, sequencer.h, t/t7503-pre-commit-hook.sh, t/t7504-commit-msg-hook.sh

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
Junio C Hamano· Sep 3, 2014, 19:32 UTC · re: Johan Herland · lore

Re: [RFC/PATCH 2/3] revert/cherry-pick: Add --no-verify option, and pass it on to commit

Johan Herland <johan@herland.net> writes:
Show 27 quoted lines
> 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 <johan@herland.net>
> ---
>  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...

Show 86 quoted lines
>  		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· Sep 3, 2014, 19:42 UTC · re: Junio C Hamano · lore

[PATCH] parse-options: detect attempt to add a duplicate short option name

Junio C Hamano <gitster@pobox.com> writes:
Show 15 quoted lines
>> 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 <gitster@pobox.com>
---
Show changes to parse-options.c +5 −0
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· Sep 3, 2014, 20:29 UTC · re: Junio C Hamano · lore

Re: [PATCH] parse-options: detect attempt to add a duplicate short option name

Am 03.09.2014 um 21:42 schrieb Junio C Hamano:
Show 52 quoted lines
> Junio C Hamano <gitster@pobox.com> 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 <gitster@pobox.com>
> ---
> 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(-)
Show changes to t/t1502-rev-parse-parseopt.sh +2 −2
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 <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
@@ -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· Sep 3, 2014, 21:05 UTC · re: René Scharfe · lore

Re: [PATCH] parse-options: detect attempt to add a duplicate short option name

René Scharfe <l.s.r@web.de> 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 <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 parse-options.c               | 15 +++++++++++++++
 t/t1502-rev-parse-parseopt.sh |  4 ++--
 2 files changed, 17 insertions(+), 2 deletions(-)
Show changes to 2 files +17 −2

parse-options.c, t/t1502-rev-parse-parseopt.sh

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 <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
@@ -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· Sep 3, 2014, 21:46 UTC · re: Junio C Hamano · lore

Re: [PATCH] parse-options: detect attempt to add a duplicate short option name

Am 03.09.2014 um 23:05 schrieb Junio C Hamano:
Show 12 quoted lines
> René Scharfe <l.s.r@web.de> 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?
Show 73 quoted lines
>
> 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 <l.s.r@web.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   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 <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
> @@ -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
>
Junio C Hamano· Sep 3, 2014, 22:16 UTC · re: René Scharfe · lore

Re: [PATCH] parse-options: detect attempt to add a duplicate short option name

René Scharfe <l.s.r@web.de> writes:
Show 8 quoted lines
>> 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· Sep 4, 2014, 06:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] parse-options: detect attempt to add a duplicate short option name

Am 04.09.2014 um 00:16 schrieb Junio C Hamano:
Show 14 quoted lines
> René Scharfe <l.s.r@web.de> 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(-)
Show changes to parse-options.c +5 −1
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
Junio C Hamano· Sep 4, 2014, 17:24 UTC · re: René Scharfe · lore

Re: [PATCH] parse-options: detect attempt to add a duplicate short option name

René Scharfe <l.s.r@web.de> writes:
Show 5 quoted lines
>> 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.
Show 20 quoted lines
>  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· Sep 4, 2014, 18:07 UTC · re: Junio C Hamano · lore

Re: [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.

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 <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
  Junio C Hamano <gitster@pobox.com> writes:
  > René Scharfe <l.s.r@web.de> 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(-)
Show changes to 2 files +15 −3

parse-options.c, t/t1502-rev-parse-parseopt.sh

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 <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
@@ -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
Jonathan Nieder· Sep 3, 2014, 21:46 UTC · re: Junio C Hamano · lore

Re: [PATCH] parse-options: detect attempt to add a duplicate short option name

Junio C Hamano wrote:
Show 21 quoted lines
> --- 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 <jrnieder@gmail.com>
Show changes to diff +1 −1
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· Sep 3, 2014, 21:58 UTC · re: Jonathan Nieder · lore

Re: [PATCH] parse-options: detect attempt to add a duplicate short option name

On Wed, Sep 03, 2014 at 02:46:25PM -0700, Jonathan Nieder wrote:
Show 25 quoted lines
> 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(-)
Show changes to 2 files +11 −2

parse-options.c, t/t1502-rev-parse-parseopt.sh

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 <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
@@ -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
Johan Herland· Sep 4, 2014, 08:34 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH 2/3] revert/cherry-pick: Add --no-verify option, and pass it on to commit

On Wed, Sep 3, 2014 at 9:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
> Johan Herland <johan@herland.net> 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, <johan@herland.net>
www.herland.net
Johan Herland· Sep 3, 2014, 14:03 UTC · re: Johan Herland · lore

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 <johan@herland.net>
---
 Documentation/git-cherry-pick.txt |  4 ++++
 Documentation/git-revert.txt      |  4 ++++
 Documentation/githooks.txt        | 20 ++++++++++----------
 3 files changed, 18 insertions(+), 10 deletions(-)
Show changes to 3 files +18 −10

Documentation/git-cherry-pick.txt, Documentation/git-revert.txt, Documentation/githooks.txt

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[<key-id>]::
 --gpg-sign[=<key-id>]::
 	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· Sep 3, 2014, 19:21 UTC · re: Johan Herland · lore

Re: [RFC/PATCH 0/3] Teach revert/cherry-pick the --no-verify option

Johan Herland <johan@herland.net> 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.

Fabian Ruch· Sep 5, 2014, 21:05 UTC · re: Johan Herland · lore

Re: [RFC/PATCH 0/3] Teach revert/cherry-pick the --no-verify option

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
Show 20 quoted lines
> 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· Sep 8, 2014, 15:13 UTC · re: Fabian Ruch · lore

Re: [RFC/PATCH 0/3] Teach revert/cherry-pick the --no-verify option

On Fri, Sep 5, 2014 at 11:05 PM, Fabian Ruch <bafain@gmail.com> wrote:
Show 11 quoted lines
> 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, <johan@herland.net>
www.herland.net

← back to recent threads