On Fri, Dec 20, 2013 at 8:04 AM, Jeff King <peff@xxxxxxxx> wrote:
> 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.
I agree too.
> 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);
Yeah, I agree that the function signature you suggest is better, but I
like the "skip_prefix" name better.
Or perhaps "remove_prefix"?
Thanks,
Christian.
--
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