threads / patch / 47992

v4Make options that expect object ids less chatty if id is invalid

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

## tl;dr

5 messages between Mar 6, 2018 and Mar 19, 2018. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Paul-Sebastian Ungureanu· Mar 6, 2018, 19:31 UTC · lore

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/blame.c               |  1 +
 builtin/shortlog.c            |  1 +
 builtin/update-index.c        |  1 +
 parse-options.c               | 20 ++++----
 parse-options.h               |  1 +
 t/t0040-parse-options.sh      |  2 +-
 t/t3404-rebase-interactive.sh |  6 +--
 t/tcontains.sh                | 92 +++++++++++++++++++++++++++++++++++
 8 files changed, 110 insertions(+), 14 deletions(-)
 create mode 100755 t/tcontains.sh
Show changes to 8 files +110 −14

builtin/blame.c, builtin/shortlog.c, builtin/update-index.c, parse-options.c, parse-options.h, t/t0040-parse-options.sh, t/t3404-rebase-interactive.sh, t/tcontains.sh

diff --git a/builtin/blame.c b/builtin/blame.c
index 9dcb367b9..a5fbec8fb 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -728,6 +728,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 			    PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);
 	for (;;) {
 		switch (parse_options_step(&ctx, options, blame_opt_usage)) {
+		case PARSE_OPT_ERROR:
 		case PARSE_OPT_HELP:
 			exit(129);
 		case PARSE_OPT_DONE:
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index e29875b84..0789e2eea 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -282,6 +282,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 
 	for (;;) {
 		switch (parse_options_step(&ctx, options, shortlog_usage)) {
+		case PARSE_OPT_ERROR:
 		case PARSE_OPT_HELP:
 			exit(129);
 		case PARSE_OPT_DONE:
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 58d1c2d28..34adf55a7 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1059,6 +1059,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			break;
 		switch (parseopt_state) {
 		case PARSE_OPT_HELP:
+		case PARSE_OPT_ERROR:
 			exit(129);
 		case PARSE_OPT_NON_OPTION:
 		case PARSE_OPT_DONE:
diff --git a/parse-options.c b/parse-options.c
index d02eb8b01..47c09a82b 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -317,14 +317,16 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
 		return get_value(p, options, all_opts, flags ^ opt_flags);
 	}
 
-	if (ambiguous_option)
-		return error("Ambiguous option: %s "
+	if (ambiguous_option) {
+		error("Ambiguous option: %s "
 			"(could be --%s%s or --%s%s)",
 			arg,
 			(ambiguous_flags & OPT_UNSET) ?  "no-" : "",
 			ambiguous_option->long_name,
 			(abbrev_flags & OPT_UNSET) ?  "no-" : "",
 			abbrev_option->long_name);
+		return -3;
+	}
 	if (abbrev_option)
 		return get_value(p, abbrev_option, all_opts, abbrev_flags);
 	return -2;
@@ -434,7 +436,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 +460,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 +473,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,9 +505,11 @@ 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;
+		case -3:
+			goto show_usage;
 		}
 		continue;
 unknown:
@@ -517,10 +520,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)
@@ -539,6 +540,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
 	parse_options_start(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
+	case PARSE_OPT_ERROR:
 		exit(129);
 	case PARSE_OPT_NON_OPTION:
 	case PARSE_OPT_DONE:
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..04d474c84 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -291,7 +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 &&
-	test_i18ncmp expect.err output.err
+	test_must_be_empty output.err
 '
 
 cat >expect <<\EOF
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ef2887bd8..cac8b2bd8 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -919,10 +919,8 @@ test_expect_success 'rebase --exec works without -i ' '
 test_expect_success 'rebase -i --exec without <CMD>' '
 	git reset --hard execute &&
 	set_fake_editor &&
-	test_must_fail git rebase -i --exec 2>tmp &&
-	sed -e "1d" tmp >actual &&
-	test_must_fail git rebase -h >expected &&
-	test_cmp expected actual &&
+	test_must_fail git rebase -i --exec 2>actual &&
+	test_i18ngrep "requires a value" actual &&
 	git checkout master
 '
 
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.g16307f54f.dirty
Junio C Hamano· Mar 6, 2018, 21:16 UTC · re: Paul-Sebastian Ungureanu · lore

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

Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> writes:
Show 10 quoted lines
> 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 notice that this version changes the way the case where an unbiguous long option is given, compared to the previous one is handled. And I recall that that single case is what I happened to have noticed during my review of the previous one, in which I was not trying to be exhausitive.

I kind of find it surprising that the one single case I happened to have noticed is the only one that needed special treatment. Did you go though all the codepath and made sure that the ones that still return -1 (not -2 and not -3) to parse_options_step() are all OK (in other words, I was just lucky) or does this version change only the "ambiguous" case, simply because that was the only one I noticed in my review (in other words, this may still not be sufficient)?

Just double checking.

The changes to existing tests have become a lot less noisy, compared to the previous one, which is probably a good thing.

Thanks.
Junio C Hamano· Mar 6, 2018, 21:28 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> I kind of find it surprising that the one single case I happened to
> have noticed is the only one that needed special treatment.  Did you
> go though all the codepath and made sure that the ones that still
> return -1 (not -2 and not -3) to parse_options_step() are all OK (in
> other words, I was just lucky) or does this version change only the
> "ambiguous" case, simply because that was the only one I noticed in
> my review (in other words, this may still not be sufficient)?
>
> Just double checking.
>
> The changes to existing tests have become a lot less noisy, compared
> to the previous one, which is probably a good thing.

I guess I should stop reading my inbox in reverse order. In your reply to my v3 review you said you studied all the codepaths from parse_short_opt() and parse_long_opt() and addition of -3 was needed only for the "ambiguous" case, so the answer to my question above is "I was just lucky and happened to have hit the only problematic case" ;-)

Will revert what is in 'next' and queue this one.
Thanks.
Martin Ågren· Mar 8, 2018, 19:27 UTC · re: Paul-Sebastian Ungureanu · lore

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

Hi Paul-Sebastian

On 6 March 2018 at 20:31, Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> wrote:

Show 7 quoted lines
> 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.
Thanks for scratching this itch.
Show 39 quoted lines
>  8 files changed, 110 insertions(+), 14 deletions(-)
>  create mode 100755 t/tcontains.sh
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 9dcb367b9..a5fbec8fb 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -728,6 +728,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>                             PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);
>         for (;;) {
>                 switch (parse_options_step(&ctx, options, blame_opt_usage)) {
> +               case PARSE_OPT_ERROR:
>                 case PARSE_OPT_HELP:
>                         exit(129);
>                 case PARSE_OPT_DONE:
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index e29875b84..0789e2eea 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -282,6 +282,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
>
>         for (;;) {
>                 switch (parse_options_step(&ctx, options, shortlog_usage)) {
> +               case PARSE_OPT_ERROR:
>                 case PARSE_OPT_HELP:
>                         exit(129);
>                 case PARSE_OPT_DONE:
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 58d1c2d28..34adf55a7 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1059,6 +1059,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>                         break;
>                 switch (parseopt_state) {
>                 case PARSE_OPT_HELP:
> +               case PARSE_OPT_ERROR:
>                         exit(129);
>                 case PARSE_OPT_NON_OPTION:
>                 case PARSE_OPT_DONE:
> diff --git a/parse-options.c b/parse-options.c
> index d02eb8b01..47c09a82b 100644
> --- a/parse-options.c
> +++ b/parse-options.c
[...]
Show 9 quoted lines
>  int parse_options_end(struct parse_opt_ctx_t *ctx)
> @@ -539,6 +540,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
>         parse_options_start(&ctx, argc, argv, prefix, options, flags);
>         switch (parse_options_step(&ctx, options, usagestr)) {
>         case PARSE_OPT_HELP:
> +       case PARSE_OPT_ERROR:
>                 exit(129);
>         case PARSE_OPT_NON_OPTION:
>         case PARSE_OPT_DONE:

These are very slightly inconsistent with the ordering of PARSE_OPT_ERROR and PARSE_OPT_HELP. I don't think it matters much. ;-)

Show 5 quoted lines
> diff --git a/t/tcontains.sh b/t/tcontains.sh
> new file mode 100755
> index 000000000..4856111ff
> --- /dev/null
> +++ b/t/tcontains.sh

This filename is not on the usual form, t1234-foo. As a result, it it is not picked up by `make test`, so isn't actually exercised. I believe you selected this name based on the feedback in [1], but I do not think that this ("tcontains.sh") was what Junio had in mind. Running the script manually succeeds though. :-)

> @@ -0,0 +1,92 @@
> +#!/bin/sh
> +
> +test_description='Test "contains" argument behavior'
Ok, that matches the file name.
Show 8 quoted lines
> +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"
> +'

You might find `test_commit` helpful. All in all, this could be a two-liner (this example is white-space mangled though):

test_expect_success 'setup ' '
        test_commit "v1.0" &&
        git tag "v1.1"
'
> +test_expect_success 'tag usage error' '
> +       test_must_fail git tag --noopt 2>actual &&
> +       test_i18ngrep "usage" actual
> +'

Hmm, this one and a couple like it do not match the filename or test description. I wonder if these are needed at all, or if some checks like these are already done elsewhere (maybe not for "git tag", but for some other user of the option-parsing machinery).

I hope you find this useful.
Martin
[1] https://public-inbox.org/git/xmqqinar1bq2.fsf@gitster-ct.c.googlers.com/

← back to recent threads