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@xxxxxxxx> wrote:
> 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.
>
>> 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.
>
> 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.
>
>> 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
--
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