Re: [PATCH/GSoC] parse-options: Add a new nousage opt
- From
Chirayu Desai <chirayudesai1@gmail.com>
- Date
- Mar 24, 2016, 17:21 UTC
- Message-ID
- <CAJj6+1EYYiK3qjOpZLBZG1d0FfW2r67759dCcLjxEy=+vFN0Dg@mail.gmail.com>
- In-Reply-To
- <20160323223157.GA12531@sigill.intra.peff.net>
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:
Show 32 quoted lines
> 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.
Show 19 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.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.
Show 5 quoted lines
> > 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.
Show 14 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"?
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