threads / patch / 41759

patchparse-options: Add a new nousage opt

Subject: [PATCH/GSoC] parse-options: Add a new nousage opt

## tl;dr

5 messages between Mar 20, 2016 and Mar 24, 2016. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Chirayu Desai· Mar 20, 2016, 06:46 UTC · lore
* To show only error text on an error instead of the full usage
* Currently used only by commands with options "--with" or "--contains",
  such as 'tag', 'branch', 'for-each-ref'.
It now prints only
  $ git tag --contains qq
  error: malformed object name qq
instead of the full usage text after the error text.
TODO: Add tests
---
 parse-options-cb.c | 12 ++++++++----
 parse-options.c    |  5 +++++
 parse-options.h    |  6 ++++--
 3 files changed, 17 insertions(+), 6 deletions(-)
Show changes to 3 files +17 −6

parse-options-cb.c, parse-options.c, parse-options.h

diff --git a/parse-options-cb.c b/parse-options-cb.c
index 239898d946..ac2ea4d674 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -85,11 +85,15 @@ int parse_opt_commits(const struct option *opt, const char *arg, int unset)
 
 	if (!arg)
 		return -1;
-	if (get_sha1(arg, sha1))
-		return error("malformed object name %s", arg);
+	if (get_sha1(arg, sha1)) {
+		error("malformed object name %s", arg);
+		return -3;
+	}
 	commit = lookup_commit_reference(sha1);
-	if (!commit)
-		return error("no such commit %s", arg);
+	if (!commit) {
+		error("no such commit %s", arg);
+		return -3;
+	}
 	commit_list_insert(commit, opt->value);
 	return 0;
 }
diff --git a/parse-options.c b/parse-options.c
index 47a9192060..d136c1afd0 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -158,6 +158,9 @@ static int get_value(struct parse_opt_ctx_t *p,
 			return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
 		if (get_arg(p, opt, flags, &arg))
 			return -1;
+		if (opt->flags & PARSE_OPT_NOUSAGE) {
+			return (*opt->callback)(opt, arg, 0);
+		}
 		return (*opt->callback)(opt, arg, 0) ? (-1) : 0;
 
 	case OPTION_INTEGER:
@@ -504,6 +507,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			goto show_usage_error;
 		case -2:
 			goto unknown;
+		case -3:
+			return PARSE_OPT_DONE;
 		}
 		continue;
 unknown:
diff --git a/parse-options.h b/parse-options.h
index ea4af92a51..628e34c5af 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -38,7 +38,8 @@ enum parse_opt_option_flags {
 	PARSE_OPT_LASTARG_DEFAULT = 16,
 	PARSE_OPT_NODASH = 32,
 	PARSE_OPT_LITERAL_ARGHELP = 64,
-	PARSE_OPT_SHELL_EVAL = 256
+	PARSE_OPT_SHELL_EVAL = 256,
+	PARSE_OPT_NOUSAGE = 512
 };
 
 struct option;
@@ -89,6 +90,7 @@ typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
  *   PARSE_OPT_LITERAL_ARGHELP: says that argh shouldn't be enclosed in brackets
  *				(i.e. '<argh>') in the help message.
  *				Useful for options with multiple parameters.
+ *   PARSE_OPT_NOUSAGE: do not print usage / help on error.
  *
  * `callback`::
  *   pointer to the callback to use for OPTION_CALLBACK or
@@ -254,7 +256,7 @@ extern int parse_opt_passthru_argv(const struct option *, const char *, int);
 	{ OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), parse_opt_passthru_argv }
 #define _OPT_CONTAINS_OR_WITH(name, variable, help, flag) \
 	{ OPTION_CALLBACK, 0, name, (variable), N_("commit"), (help), \
-	  PARSE_OPT_LASTARG_DEFAULT | flag, \
+	  PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NOUSAGE | flag, \
 	  parse_opt_commits, (intptr_t) "HEAD" \
 	}
 #define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("contains", v, h, 0)

-- 
2.7.4
Chirayu Desai· Mar 20, 2016, 06:52 UTC · re: Chirayu Desai · lore

Re: [PATCH/GSoC] parse-options: Add a new nousage opt

This is being discussed in the "Re: "git tag --contains <id>" is too chatty, if <id> is invalid" thread. $gmane/289312

On Sun, Mar 20, 2016 at 12:16 PM, Chirayu Desai <chirayudesai1@gmail.com> wrote:
Show 74 quoted lines
> * To show only error text on an error instead of the full usage
> * Currently used only by commands with options "--with" or "--contains",
>   such as 'tag', 'branch', 'for-each-ref'.
>
> It now prints only
>   $ git tag --contains qq
>   error: malformed object name qq
> instead of the full usage text after the error text.
>
> TODO: Add tests
> ---
>  parse-options-cb.c | 12 ++++++++----
>  parse-options.c    |  5 +++++
>  parse-options.h    |  6 ++++--
>  3 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index 239898d946..ac2ea4d674 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -85,11 +85,15 @@ int parse_opt_commits(const struct option *opt, const char *arg, int unset)
>
>         if (!arg)
>                 return -1;
> -       if (get_sha1(arg, sha1))
> -               return error("malformed object name %s", arg);
> +       if (get_sha1(arg, sha1)) {
> +               error("malformed object name %s", arg);
> +               return -3;
> +       }
>         commit = lookup_commit_reference(sha1);
> -       if (!commit)
> -               return error("no such commit %s", arg);
> +       if (!commit) {
> +               error("no such commit %s", arg);
> +               return -3;
> +       }
>         commit_list_insert(commit, opt->value);
>         return 0;
>  }
> diff --git a/parse-options.c b/parse-options.c
> index 47a9192060..d136c1afd0 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -158,6 +158,9 @@ static int get_value(struct parse_opt_ctx_t *p,
>                         return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
>                 if (get_arg(p, opt, flags, &arg))
>                         return -1;
> +               if (opt->flags & PARSE_OPT_NOUSAGE) {
> +                       return (*opt->callback)(opt, arg, 0);
> +               }
>                 return (*opt->callback)(opt, arg, 0) ? (-1) : 0;
>
>         case OPTION_INTEGER:
> @@ -504,6 +507,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>                         goto show_usage_error;
>                 case -2:
>                         goto unknown;
> +               case -3:
> +                       return PARSE_OPT_DONE;
>                 }
>                 continue;
>  unknown:
> diff --git a/parse-options.h b/parse-options.h
> index ea4af92a51..628e34c5af 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -38,7 +38,8 @@ enum parse_opt_option_flags {
>         PARSE_OPT_LASTARG_DEFAULT = 16,
>         PARSE_OPT_NODASH = 32,
>         PARSE_OPT_LITERAL_ARGHELP = 64,
> -       PARSE_OPT_SHELL_EVAL = 256
> +       PARSE_OPT_SHELL_EVAL = 256,
> +       PARSE_OPT_NOUSAGE = 512
Perhaps a _ON_ERR should be appended at the end to make it clearer?
Show 23 quoted lines
>  };
>
>  struct option;
> @@ -89,6 +90,7 @@ typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
>   *   PARSE_OPT_LITERAL_ARGHELP: says that argh shouldn't be enclosed in brackets
>   *                             (i.e. '<argh>') in the help message.
>   *                             Useful for options with multiple parameters.
> + *   PARSE_OPT_NOUSAGE: do not print usage / help on error.
>   *
>   * `callback`::
>   *   pointer to the callback to use for OPTION_CALLBACK or
> @@ -254,7 +256,7 @@ extern int parse_opt_passthru_argv(const struct option *, const char *, int);
>         { OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), parse_opt_passthru_argv }
>  #define _OPT_CONTAINS_OR_WITH(name, variable, help, flag) \
>         { OPTION_CALLBACK, 0, name, (variable), N_("commit"), (help), \
> -         PARSE_OPT_LASTARG_DEFAULT | flag, \
> +         PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NOUSAGE | flag, \
>           parse_opt_commits, (intptr_t) "HEAD" \
>         }
>  #define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("contains", v, h, 0)
> --
> 2.7.4
>
Jeff King· Mar 23, 2016, 22:31 UTC · re: Chirayu Desai · lore

Re: [PATCH/GSoC] parse-options: Add a new nousage opt

On Sun, Mar 20, 2016 at 12:16:45PM +0530, Chirayu Desai wrote:
Show 14 quoted lines
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index 239898d946..ac2ea4d674 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -85,11 +85,15 @@ int parse_opt_commits(const struct option *opt, const char *arg, int unset)
>  
>  	if (!arg)
>  		return -1;
> -	if (get_sha1(arg, sha1))
> -		return error("malformed object name %s", arg);
> +	if (get_sha1(arg, sha1)) {
> +		error("malformed object name %s", arg);
> +		return -3;
> +	}

Now that we have a few meaningful return values, should we have some enum that gives them human-readable names?

E.g., why don't we allow "-2" here? I think it is because parse_options_step internally uses it for "I don't know about that option". But maybe we should have something like:

  enum PARSE_OPT_ERROR {
          PARSE_OPT_ERR_USAGE = -1,
	  PARSE_OPT_ERR_UNKNOWN_OPTION = -2,
	  PARSE_OPT_ERR_FAIL_QUIETLY = -3,
  }

(I don't quite like the final name, but I couldn't think of anything better).

Show 12 quoted lines
> diff --git a/parse-options.c b/parse-options.c
> index 47a9192060..d136c1afd0 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -158,6 +158,9 @@ static int get_value(struct parse_opt_ctx_t *p,
>  			return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
>  		if (get_arg(p, opt, flags, &arg))
>  			return -1;
> +		if (opt->flags & PARSE_OPT_NOUSAGE) {
> +			return (*opt->callback)(opt, arg, 0);
> +		}
>  		return (*opt->callback)(opt, arg, 0) ? (-1) : 0;

Here you use PARSE_OPT_NOUSAGE to pass the callback's value directly back to the rest of the option-parsing code. But can't we just intercept "-3" always? It's possible that another callback is using it to generically return an error, but it seems like a rather low risk, and the resulting code is much simpler.

Or we could go the opposite direction. If a callback is annotated with PARSE_OPT_NOUSAGE, why do we even need to care about its return value? The callback could continue to return -1, and we would simply suppress the usage message.

Show 10 quoted lines
>  	case OPTION_INTEGER:
> @@ -504,6 +507,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>  			goto show_usage_error;
>  		case -2:
>  			goto unknown;
> +		case -3:
> +			return PARSE_OPT_DONE;
>  		}
>  		continue;
>  unknown:

If I understand correctly, this is now getting the value from the callback directly. What happens if a callback returns "-4" or "4"?

Also, this covers the parse_long_opt() call, but there are two parse_short_opt() calls earlier. Wouldn't they need to learn the same logic?

-Peff
Chirayu Desai· Mar 24, 2016, 17:21 UTC · re: Jeff King · lore

Re: [PATCH/GSoC] parse-options: Add a new nousage opt

Note Before: I have decided not to apply for GSoC with Git this year, as I was already late, and all the remaining time got taken by the proposal I wrote for Debian, and college studies / exams.

I definitely want to work with Git in the future too, it has always piqued my interest being something that I use daily. I want to get this change done as well, if that is okay.

On Thu, Mar 24, 2016 at 4:01 AM, Jeff King <peff@peff.net> wrote:
Show 32 quoted lines
> On Sun, Mar 20, 2016 at 12:16:45PM +0530, Chirayu Desai wrote:
>
>> diff --git a/parse-options-cb.c b/parse-options-cb.c
>> index 239898d946..ac2ea4d674 100644
>> --- a/parse-options-cb.c
>> +++ b/parse-options-cb.c
>> @@ -85,11 +85,15 @@ int parse_opt_commits(const struct option *opt, const char *arg, int unset)
>>
>>       if (!arg)
>>               return -1;
>> -     if (get_sha1(arg, sha1))
>> -             return error("malformed object name %s", arg);
>> +     if (get_sha1(arg, sha1)) {
>> +             error("malformed object name %s", arg);
>> +             return -3;
>> +     }
>
> Now that we have a few meaningful return values, should we have some
> enum that gives them human-readable names?
>
> E.g., why don't we allow "-2" here? I think it is because
> parse_options_step internally uses it for "I don't know about that
> option". But maybe we should have something like:
>
>   enum PARSE_OPT_ERROR {
>           PARSE_OPT_ERR_USAGE = -1,
>           PARSE_OPT_ERR_UNKNOWN_OPTION = -2,
>           PARSE_OPT_ERR_FAIL_QUIETLY = -3,
>   }
>
> (I don't quite like the final name, but I couldn't think of anything
> better).
I agree, this would be much better and clearer than using hard coded values.
Show 19 quoted lines
>
>> diff --git a/parse-options.c b/parse-options.c
>> index 47a9192060..d136c1afd0 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -158,6 +158,9 @@ static int get_value(struct parse_opt_ctx_t *p,
>>                       return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
>>               if (get_arg(p, opt, flags, &arg))
>>                       return -1;
>> +             if (opt->flags & PARSE_OPT_NOUSAGE) {
>> +                     return (*opt->callback)(opt, arg, 0);
>> +             }
>>               return (*opt->callback)(opt, arg, 0) ? (-1) : 0;
>
> Here you use PARSE_OPT_NOUSAGE to pass the callback's value directly
> back to the rest of the option-parsing code. But can't we just intercept
> "-3" always? It's possible that another callback is using it to
> generically return an error, but it seems like a rather low risk, and
> the resulting code is much simpler.

I don't get what you mean by intercepting '-3'. The idea was that other options could use it in the future to return arbitary values.

Show 5 quoted lines
>
> Or we could go the opposite direction. If a callback is annotated with
> PARSE_OPT_NOUSAGE, why do we even need to care about its return value?
> The callback could continue to return -1, and we would simply suppress
> the usage message.
That would also work, but I feel that this is cleaner.
Show 14 quoted lines
>
>>       case OPTION_INTEGER:
>> @@ -504,6 +507,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>>                       goto show_usage_error;
>>               case -2:
>>                       goto unknown;
>> +             case -3:
>> +                     return PARSE_OPT_DONE;
>>               }
>>               continue;
>>  unknown:
>
> If I understand correctly, this is now getting the value from the
> callback directly. What happens if a callback returns "-4" or "4"?

That could be handled in the future if somebody decides to use that. Now this makes using the above "return -1 always but don't print usage if flags contain PARSE_OPT_NOUSAGE" option look much better.

>
> Also, this covers the parse_long_opt() call, but there are two
> parse_short_opt() calls earlier. Wouldn't they need to learn the same
> logic?
I didn't add it right now as both "--contains" and "--with" are long opts.
>
> -Peff

Thanks, Chirayu

Jeff King· Mar 24, 2016, 17:34 UTC · re: Chirayu Desai · lore

Re: [PATCH/GSoC] parse-options: Add a new nousage opt

On Thu, Mar 24, 2016 at 10:51:05PM +0530, Chirayu Desai wrote:
> I definitely want to work with Git in the future too, it has always
> piqued my interest being something that I use daily.
> I want to get this change done as well, if that is okay.

Sure, that's great. Part of the point of microprojects is that they're useful changes on their own, outside of GSoC.

Show 21 quoted lines
> >> diff --git a/parse-options.c b/parse-options.c
> >> index 47a9192060..d136c1afd0 100644
> >> --- a/parse-options.c
> >> +++ b/parse-options.c
> >> @@ -158,6 +158,9 @@ static int get_value(struct parse_opt_ctx_t *p,
> >>                       return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
> >>               if (get_arg(p, opt, flags, &arg))
> >>                       return -1;
> >> +             if (opt->flags & PARSE_OPT_NOUSAGE) {
> >> +                     return (*opt->callback)(opt, arg, 0);
> >> +             }
> >>               return (*opt->callback)(opt, arg, 0) ? (-1) : 0;
> >
> > Here you use PARSE_OPT_NOUSAGE to pass the callback's value directly
> > back to the rest of the option-parsing code. But can't we just intercept
> > "-3" always? It's possible that another callback is using it to
> > generically return an error, but it seems like a rather low risk, and
> > the resulting code is much simpler.
> I don't get what you mean by intercepting '-3'.
> The idea was that other options could use it in the future to return
> arbitary values.
I mean could this code just be:
  switch (opt->callback(opt, arg, 0)) {
  case 0: /* ok, no error */
	return 0;
  case -3: /* error, but we were asked not to show usage; relay it */
	return -3;
  default: /* anything else is a normal error */
	return -1;
  }
Do we need PARSE_OPT_NOUSAGE at all?
Show 5 quoted lines
> > Or we could go the opposite direction. If a callback is annotated with
> > PARSE_OPT_NOUSAGE, why do we even need to care about its return value?
> > The callback could continue to return -1, and we would simply suppress
> > the usage message.
> That would also work, but I feel that this is cleaner.

Yeah, I think it comes down to where the decision for "don't show usage" should be made. Is it something that the options-list (that is using the callback) knows, or is it something that the callback knows?

Show 16 quoted lines
> >>       case OPTION_INTEGER:
> >> @@ -504,6 +507,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
> >>                       goto show_usage_error;
> >>               case -2:
> >>                       goto unknown;
> >> +             case -3:
> >> +                     return PARSE_OPT_DONE;
> >>               }
> >>               continue;
> >>  unknown:
> >
> > If I understand correctly, this is now getting the value from the
> > callback directly. What happens if a callback returns "-4" or "4"?
> That could be handled in the future if somebody decides to use that.
> Now this makes using the above "return -1 always but don't print usage if flags
> contain PARSE_OPT_NOUSAGE" option look much better.

What I meant specifically was that I think a callback returning "-4" is an error (with usage) in the current code, but is silently ignored with your patch (if PARSE_OPT_NOUSAGE is set, of course), making it a potential hazard.

> > Also, this covers the parse_long_opt() call, but there are two
> > parse_short_opt() calls earlier. Wouldn't they need to learn the same
> > logic?
> I didn't add it right now as both "--contains" and "--with" are long opts.

Yeah, I agree it is not necessary for those long opts, but it seems like a maintenance hazard for it to work in one case, but not another. IOW, it would be a big surprise for somebody who later adds a short option for "--contains", or who adds PARSE_OPT_NOUSAGE to an existing short option.

-Peff

← back to recent threads