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

[Jeff King <peff@peff.net>]

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