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

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

[Chirayu Desai]

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

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

[Chirayu Desai]

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:
> * 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?
>  };
>
>  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
>

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

[Jeff King]

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

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

>  	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

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

[Chirayu Desai]

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

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

[Jeff King]

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.

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

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

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