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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html