Re: [PATCH 02/12] Convert starts_with() to skip_prefix() for option parsing

> On Fri, Dec 20, 2013 at 07:51:00AM +0100, Johannes Sixt wrote:
>
>> >     for (i = 1; i < argc && *argv[i] == '-'; i++) {
>> >             const char *arg = argv[i];
>> > +           const char *optarg;
>> >
>> > -           if (starts_with(arg, "--upload-pack=")) {
>> > -                   args.uploadpack = arg + 14;
>> > +           if ((optarg = skip_prefix(arg, "--upload-pack=")) != NULL) {
>> > +                   args.uploadpack = optarg;
>>
>> Quite frankly, I do not think this is an improvement. The old code is
>> *MUCH* easier to understand because "starts_with" is clearly a predicate
>> that is either true or false, but the code with "skip_prefix" is much
>> heavier on the eye with its extra level of parenthesis. That it removes a
>> hard-coded constant does not count much IMHO because it is very clear
>> where the value comes from.
>
> Yeah, I agree that is unfortunate.

> Maybe we could have the best of both
> worlds, like:
>
>   if (starts_with(arg, "--upload-pack=", &optarg))
>           ... use optarg ...
>
> Probably we do not want to call it just "starts_with", as quite a few
> callers to do not care about what comes next, and would just pass NULL.
> I cannot seem to think of a good name, though, as the "with" means that
> obvious things like "starts_with_value" naturally parse as a single
> (nonsensical) sentence.  Something like "parse_prefix" would work, but
> it is not as clearly a predicate as "starts_with" (but we have at least
> gotten rid of the extra parentheses).
>
> Elsewhere in the thread, the concept was discussed of returning the full
> string to mean "did not match", which makes some other idioms simpler
> (but IMHO makes the simple cases like this even harder to read). My
> proposal splits the "start of string" out parameter from the boolean
> return, so it handles both cases naturally:
>
>   /* here we care if we saw the prefix, as above */
>   if (parse_prefix(foo, prefix, &the_rest))
>       ...
>
>   /*
>    * and here we do not care, and just want to optionally strip the
>    * prefix, and take the full value otherwise; we just have to ignore
>    * the return value in this case.
>    */
>   parse_prefix(foo, prefix, &foo);