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

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

[Johan Herland]

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

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

[Johan Herland]

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(+)

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

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

[Johan Herland]

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")),
 		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

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

[Johan Herland]

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(-)

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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

>  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 &&

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

[Junio C Hamano]

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



>  		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^

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

[Junio C Hamano]

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) ||

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

[René Scharfe]

Am 03.09.2014 um 21:42 schrieb Junio C Hamano:
> 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(-)

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

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

[Junio C Hamano]

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(-)

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

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

[René Scharfe]

Am 03.09.2014 um 23:05 schrieb Junio C Hamano:
> 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?

>
> 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
>

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

[Jonathan Nieder]

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 <jrnieder@gmail.com>

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)

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

[Jonathan Nieder]

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 <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

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

[Junio C Hamano]

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.

> 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.

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

[René Scharfe]

Am 04.09.2014 um 00:16 schrieb Junio C Hamano:
> 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(-)

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

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

[Johan Herland]

On Wed, Sep 3, 2014 at 9:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Junio C Hamano]

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 | 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);
>  }

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

[Junio C Hamano]

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(-)

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

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

[Fabian Ruch]

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(-)

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

[Johan Herland]

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