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

From: Jeff King <peff@peff.net>
Date: Mar 23, 2016, 22:31 UTC
Message-ID: <20160323223157.GA12531@sigill.intra.peff.net>
In-Reply-To: <1458456405-3519-1-git-send-email-chirayudesai1@gmail.com>

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