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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html