[GSoC][PATCH v3] Make options that expect object ids less chatty if id is invalid

[GSoC][PATCH v3] Make options that expect object ids less chatty if id is invalid

[Paul-Sebastian Ungureanu]

Usually, the usage should be shown only if the user does not know what
options are available. If the user specifies an invalid value, the user
is already aware of the available options. In this case, there is no
point in displaying the usage anymore.

This patch applies to "git tag --contains", "git branch --contains",
"git branch --points-at", "git for-each-ref --contains" and many more.

Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
---
 builtin/update-index.c        |  2 +
 parse-options.c               | 13 +++--
 parse-options.h               |  1 +
 t/t0040-parse-options.sh      |  9 ++--
 t/t3404-rebase-interactive.sh |  2 +-
 t/t3502-cherry-pick-merge.sh  |  8 +--
 t/tcontains.sh                | 92 +++++++++++++++++++++++++++++++++++
 7 files changed, 111 insertions(+), 16 deletions(-)
 create mode 100755 t/tcontains.sh

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 58d1c2d28..eeee1c170 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1060,6 +1060,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		switch (parseopt_state) {
 		case PARSE_OPT_HELP:
 			exit(129);
+		case PARSE_OPT_ERROR:
+			exit(1);
 		case PARSE_OPT_NON_OPTION:
 		case PARSE_OPT_DONE:
 		{
diff --git a/parse-options.c b/parse-options.c
index d02eb8b01..eee401662 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -434,7 +434,6 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 		       const char * const usagestr[])
 {
 	int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);
-	int err = 0;
 
 	/* we must reset ->opt, unknown short option leave it dangling */
 	ctx->opt = NULL;
@@ -459,7 +458,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			ctx->opt = arg + 1;
 			switch (parse_short_opt(ctx, options)) {
 			case -1:
-				goto show_usage_error;
+				return PARSE_OPT_ERROR;
 			case -2:
 				if (ctx->opt)
 					check_typos(arg + 1, options);
@@ -472,7 +471,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			while (ctx->opt) {
 				switch (parse_short_opt(ctx, options)) {
 				case -1:
-					goto show_usage_error;
+					return PARSE_OPT_ERROR;
 				case -2:
 					if (internal_help && *ctx->opt == 'h')
 						goto show_usage;
@@ -504,7 +503,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			goto show_usage;
 		switch (parse_long_opt(ctx, arg + 2, options)) {
 		case -1:
-			goto show_usage_error;
+			return PARSE_OPT_ERROR;
 		case -2:
 			goto unknown;
 		}
@@ -517,10 +516,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 	}
 	return PARSE_OPT_DONE;
 
- show_usage_error:
-	err = 1;
  show_usage:
-	return usage_with_options_internal(ctx, usagestr, options, 0, err);
+	return usage_with_options_internal(ctx, usagestr, options, 0, 0);
 }
 
 int parse_options_end(struct parse_opt_ctx_t *ctx)
@@ -543,6 +540,8 @@ int parse_options(int argc, const char **argv, const char *prefix,
 	case PARSE_OPT_NON_OPTION:
 	case PARSE_OPT_DONE:
 		break;
+	case PARSE_OPT_ERROR:
+		exit(1);
 	default: /* PARSE_OPT_UNKNOWN */
 		if (ctx.argv[0][1] == '-') {
 			error("unknown option `%s'", ctx.argv[0] + 2);
diff --git a/parse-options.h b/parse-options.h
index af711227a..c77bb3b4f 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -188,6 +188,7 @@ enum {
 	PARSE_OPT_HELP = -1,
 	PARSE_OPT_DONE,
 	PARSE_OPT_NON_OPTION,
+	PARSE_OPT_ERROR,
 	PARSE_OPT_UNKNOWN
 };
 
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 0c2fc81d7..8af12e8a1 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -162,9 +162,9 @@ test_expect_success 'long options' '
 '
 
 test_expect_success 'missing required value' '
-	test_expect_code 129 test-parse-options -s &&
-	test_expect_code 129 test-parse-options --string &&
-	test_expect_code 129 test-parse-options --file
+	test_expect_code 1 test-parse-options -s &&
+	test_expect_code 1 test-parse-options --string &&
+	test_expect_code 1 test-parse-options --file
 '
 
 cat >expect <<\EOF
@@ -214,7 +214,7 @@ test_expect_success 'unambiguously abbreviated option with "="' '
 '
 
 test_expect_success 'ambiguously abbreviated option' '
-	test_expect_code 129 test-parse-options --strin 123
+	test_expect_code 1 test-parse-options --strin 123
 '
 
 test_expect_success 'non ambiguous option (after two options it abbreviates)' '
@@ -291,6 +291,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '
 test_expect_success 'OPT_CALLBACK() and callback errors work' '
 	test_must_fail test-parse-options --no-length >output 2>output.err &&
 	test_i18ncmp expect output &&
+	>expect.err &&
 	test_i18ncmp expect.err output.err
 '
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ef2887bd8..e6a0766f8 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -921,7 +921,7 @@ test_expect_success 'rebase -i --exec without <CMD>' '
 	set_fake_editor &&
 	test_must_fail git rebase -i --exec 2>tmp &&
 	sed -e "1d" tmp >actual &&
-	test_must_fail git rebase -h >expected &&
+	>expected &&
 	test_cmp expected actual &&
 	git checkout master
 '
diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index b1602718f..157cbcdb2 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -34,10 +34,10 @@ test_expect_success setup '
 test_expect_success 'cherry-pick -m complains of bogus numbers' '
 	# expect 129 here to distinguish between cases where
 	# there was nothing to cherry-pick
-	test_expect_code 129 git cherry-pick -m &&
-	test_expect_code 129 git cherry-pick -m foo b &&
-	test_expect_code 129 git cherry-pick -m -1 b &&
-	test_expect_code 129 git cherry-pick -m 0 b
+	test_expect_code 1 git cherry-pick -m &&
+	test_expect_code 1 git cherry-pick -m foo b &&
+	test_expect_code 1 git cherry-pick -m -1 b &&
+	test_expect_code 1 git cherry-pick -m 0 b
 '
 
 test_expect_success 'cherry-pick a non-merge with -m should fail' '
diff --git a/t/tcontains.sh b/t/tcontains.sh
new file mode 100755
index 000000000..4856111ff
--- /dev/null
+++ b/t/tcontains.sh
@@ -0,0 +1,92 @@
+#!/bin/sh
+
+test_description='Test "contains" argument behavior'
+
+. ./test-lib.sh
+
+test_expect_success 'setup ' '
+	git init . &&
+	echo "this is a test" >file &&
+	git add -A &&
+	git commit -am "tag test" &&
+	git tag "v1.0" &&
+	git tag "v1.1"
+'
+
+test_expect_success 'tag --contains <existent_tag>' '
+	git tag --contains "v1.0" >actual &&
+	grep "v1.0" actual &&
+	grep "v1.1" actual
+'
+
+test_expect_success 'tag --contains <inexistent_tag>' '
+	test_must_fail git tag --contains "notag" 2>actual &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success 'tag --no-contains <existent_tag>' '
+	git tag --no-contains "v1.1" >actual &&
+	test_line_count = 0 actual
+'
+
+test_expect_success 'tag --no-contains <inexistent_tag>' '
+	test_must_fail git tag --no-contains "notag" 2>actual &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success 'tag usage error' '
+	test_must_fail git tag --noopt 2>actual &&
+	test_i18ngrep "usage" actual
+'
+
+test_expect_success 'branch --contains <existent_commit>' '
+	git branch --contains "master" >actual &&
+	test_i18ngrep "master" actual
+'
+
+test_expect_success 'branch --contains <inexistent_commit>' '
+	test_must_fail git branch --no-contains "nocommit" 2>actual &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success 'branch --no-contains <existent_commit>' '
+	git branch --no-contains "master" >actual &&
+	test_line_count = 0 actual
+'
+
+test_expect_success 'branch --no-contains <inexistent_commit>' '
+	test_must_fail git branch --no-contains "nocommit" 2>actual &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success 'branch usage error' '
+	test_must_fail git branch --noopt 2>actual &&
+	test_i18ngrep "usage" actual
+'
+
+test_expect_success 'for-each-ref --contains <existent_object>' '
+	git for-each-ref --contains "master" >actual &&
+	test_line_count = 3 actual
+'
+
+test_expect_success 'for-each-ref --contains <inexistent_object>' '
+	test_must_fail git for-each-ref --no-contains "noobject" 2>actual &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success 'for-each-ref --no-contains <existent_object>' '
+	git for-each-ref --no-contains "master" >actual &&
+	test_line_count = 0 actual
+'
+
+test_expect_success 'for-each-ref --no-contains <inexistent_object>' '
+	test_must_fail git for-each-ref --no-contains "noobject" 2>actual &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success 'for-each-ref usage error' '
+	test_must_fail git for-each-ref --noopt 2>actual &&
+	test_i18ngrep "usage" actual
+'
+
+test_done
-- 
2.16.2.346.g22874a30c

Re: [GSoC][PATCH v3] Make options that expect object ids less chatty if id is invalid

[Junio C Hamano]

Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> writes:

> Usually, the usage should be shown only if the user does not know what
> options are available. If the user specifies an invalid value, the user
> is already aware of the available options. In this case, there is no
> point in displaying the usage anymore.
>
> This patch applies to "git tag --contains", "git branch --contains",
> "git branch --points-at", "git for-each-ref --contains" and many more.
>
> Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
> ---

I am guessing that this was sent as a replacement for fcfba373
("ref-filter: make "--contains <id>" less chatty if <id> is
invalid", 2018-02-23) that was merged to 'next' at 9623d681 ("Merge
branch 'ps/contains-id-error-message' into next", 2018-02-27).

In general, we do not drop and replace what is already merged to
'next' with a new version; once a topic is merged to 'next', we go
incremental and further refinements are made on top instead.

I however strongly suspect that the approach taken by this round is
a lot better, and it is sufficiently different that an "incremental"
that applies on top of the previous patches would essentially revert
them and builds what we see in this message afresh.

So I am tempted to revert the previous one out of 'next' and then
treat this one as if it were a new/different topic.


> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 58d1c2d28..eeee1c170 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1060,6 +1060,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>  		switch (parseopt_state) {
>  		case PARSE_OPT_HELP:
>  			exit(129);
> +		case PARSE_OPT_ERROR:
> +			exit(1);

OK, so things like

	$ git update-index --index-version
	$ git update-index --index-version NOT_AN_INTEGER

used to give the full usage (just like your primary and original
focus that was what "tag --contains" etc. did), but with this
change, they just throw an error and stop.  I guess this is a very
good thing ;-)

Also the exit status is changed from 129 to 1.  It is not clear if
that is a desirable change (I am not yet saying we shouldn't change
it, though).  Calling scripts probably only care about non-zero ness
of the exit status, so this change probably does not hurt people in
practice, I guess.

> @@ -504,7 +503,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>  			goto show_usage;
>  		switch (parse_long_opt(ctx, arg + 2, options)) {
>  		case -1:
> -			goto show_usage_error;
> +			return PARSE_OPT_ERROR;

An error return -1 from parse_long_opt() unfortunately includes the
case where the given string is ambiguous.  The test-parse-options
command in t/helpers e.g. has --string and --string2 option, that
take a string argument, so "test-parse-options --stri" says "error:
Ambigous option: stri (could be --string or --string2)", and without
this patch, it goes on to show the usage help.  With the patch,
however, we no longer get the help, and I think that is a regression;
the user likely wants to see the help text that describes these two
potential options to decide between the two.

Of course, "test-parse-options --string" fails with "error: options
`string' requires a value" and stops without the usage help---and
that is definitely an improvement.

Taking these together, I _think_ this patch is moving things in the
right direction, in that it allows callers of parse_options_step()
to tell "user knew what option s/he wanted, and sufficient error
message has already be given" and "user gave us a nonsense option
and would be helped by usage text" cases apart by introducing a new
value PARSE_OPT_ERROR, but in order to be able to correctly give
PARSE_OPT_ERROR back to the caller, parse_long_opt() and
parse_short_opt() (possibly, but I didn't check) would need a bit of
tweak to help their callers in this function.

>  test_expect_success 'non ambiguous option (after two options it abbreviates)' '
> @@ -291,6 +291,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '
>  test_expect_success 'OPT_CALLBACK() and callback errors work' '
>  	test_must_fail test-parse-options --no-length >output 2>output.err &&
>  	test_i18ncmp expect output &&
> +	>expect.err &&
>  	test_i18ncmp expect.err output.err
>  '

The way the existing script sets test vectors up is not that great,
but this "expect.err" file is created by getting the usage output at
the very beginning of the test (into "expect") and a few tests refer
to it, expecting it to have the usage help (see check_unknown_i18n
helper).  We should treat its contents as a constant, and shouldn't
touch it like this.  Instead, perhaps do

	test_must_fail ... 2>actual.err &&
	test_must_be_empty actual.err

here.

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index ef2887bd8..e6a0766f8 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -921,7 +921,7 @@ test_expect_success 'rebase -i --exec without <CMD>' '
>  	set_fake_editor &&
>  	test_must_fail git rebase -i --exec 2>tmp &&
>  	sed -e "1d" tmp >actual &&
> -	test_must_fail git rebase -h >expected &&
> +	>expected &&
>  	test_cmp expected actual &&
>  	git checkout master
>  '

This used to test

 - "git rebase -i --exec" fails (due to lack of <cmd>).

 - its error output is of no interest--expected to be some "error"
   message that talks about the lack of <cmd>.

 - after that first line of the error message, the remainder must be
   the usage help.

With the change, the third point is no longer the case.  I wonder if
we want to change the second point above to more actively test that
we get the error about the lack of <cmd>, e.g. something like

	...
	set_fake_editor &&
	test_must_fail git rebase -i --exec 2>actual &&
	test_i18ngrep "requires a value" actual
	...

> diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
> index b1602718f..157cbcdb2 100755
> --- a/t/t3502-cherry-pick-merge.sh
> +++ b/t/t3502-cherry-pick-merge.sh
> @@ -34,10 +34,10 @@ test_expect_success setup '
>  test_expect_success 'cherry-pick -m complains of bogus numbers' '
>  	# expect 129 here to distinguish between cases where
>  	# there was nothing to cherry-pick
> -	test_expect_code 129 git cherry-pick -m &&
> -	test_expect_code 129 git cherry-pick -m foo b &&
> -	test_expect_code 129 git cherry-pick -m -1 b &&
> -	test_expect_code 129 git cherry-pick -m 0 b
> +	test_expect_code 1 git cherry-pick -m &&
> +	test_expect_code 1 git cherry-pick -m foo b &&
> +	test_expect_code 1 git cherry-pick -m -1 b &&
> +	test_expect_code 1 git cherry-pick -m 0 b

The comment in the pre-context is something to ponder on.  Perhaps
we would want to find a way to do this change without affecting the
exit status code.

Re: [GSoC][PATCH v3] Make options that expect object ids less chatty if id is invalid

[Paul-Sebastian Ungureanu]

Hello Junio,

Thank you for reviewing my code. I appreciate it. I made the changes
here [1].

On Mon, 2018-03-05 at 16:19 -0800, Junio C Hamano wrote:
> Taking these together, I _think_ this patch is moving things in the
> right direction, in that it allows callers of parse_options_step()
> to tell "user knew what option s/he wanted, and sufficient error
> message has already be given" and "user gave us a nonsense option
> and would be helped by usage text" cases apart by introducing a new
> value PARSE_OPT_ERROR, but in order to be able to correctly give
> PARSE_OPT_ERROR back to the caller, parse_long_opt() and
> parse_short_opt() (possibly, but I didn't check) would need a bit of
> tweak to help their callers in this function.

I am not sure I got this right, but I believe this is already done.

- If an error occurs during value parsing in parse_short_opt or
parse_long_opt then -1 is returned and parse_option_step returns
PARSE_OPT_ERROR.

- If parse_short_opt and parse_long_opt encounter an unknown option
then -2 is returned and parse_option_step returns PARSE_OPT_UNKNOWN
(but only if PARSE_OPT_KEEP_UNKNOWN is not specified).

- If usage is shown by calling usage_with_options_internal then
PARSE_OPT_HELP is going to be forwarded and also returned by
parse_options_step.

What I also changed in the new patch [1] is to make parse_long_opt
return -3 when ambiguous option are found, in which case
parse_options_step will handle this return value by showing usage and
returning PARSE_OPT_HELP.

Please correct me if I am wrong.

[1] https://public-inbox.org/git/20180306193116.23876-1-ungureanupaulse
bastian@gmail.com/T/#u

Best regards,
Paul Ungureanu