Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_val()

[Christian Couder <christian.couder@gmail.com>]

On Sun, Dec 3, 2017 at 7:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> From: Christian Couder <christian.couder@gmail.com>
>>
>> We often accept both a "--key" option and a "--key=<val>" option.
>>
>> These options currently are parsed using something like:
>>
>> if (!strcmp(arg, "--key")) {
>>       /* do something */
>> } else if (skip_prefix(arg, "--key=", &arg)) {
>>       /* do something with arg */
>> }
>>
>> which is a bit cumbersome compared to just:
>>
>> if (skip_to_opt_val(arg, "--key", &arg)) {
>>       /* do something with arg */
>> }
>
> Sounds sensible; especially if there are many existing code that can
> be shortened by using this helper, that is.
>
>> Note that, when using skip_to_opt_val(), it is not possible any more
>> to do something different when the first argument is exactly "--key"
>> than when it is exactly "--key=", but in most cases we already don't
>> make any difference, which is a good thing.
>
> Note that "it is not possible" is misleading.  skip_to_opt_val()
> could be written to allow the caller to tell the difference if you
> chose to, but *you* made it impossible by assigning "" to arg upon
> seeing "--key".  You could assign NULL to arg in that case, and
> "--key" and "--key=" can be differenciated by checking arg; the
> former will give you NULL and the latter "".

Yeah, what I meant was "the design of the function in this patch makes
it impossible..."
That's why I wrote after the diffstat:

"""Another possibility would be to add a "const char *default"
argument to the function, and to do:

      if (!*p) {
              *val = default;
              return 1;
      }

This could make the function more useful in some cases."""

But yeah I could have added the above to the commit message and it
hopefully would have made it clear what I meant.

Anyway there is a design choice to be made. Adding a "const char
*default" argument makes the function more generic, but this might not
be very useful as there are few cases that might benefit. And if we
want to make the command line interface consistent and perhaps avoid
user errors, we might want to have a rule that says that "--key" and
"--key=" should always give the same result. In this case we may also
want to make it easy to implement options that follow the rule.

So my preference is to not add the "const char *default" argument to
the function. But it is not a strong preference.

> Not that I am convinced that it is a bad idea to deliberately lose
> information like this implementation does.  At least not yet.
>
> If there will be no conceivable caller that wants to differenticate
> between the two, the implementation that "loses information" can
> claim to be easier to use, as the callers do not have to be forced
> to write something like:
>
>         if (skip_to_optional_val(arg, "--key", &arg)
>                 do_something(arg ? arg : "");
>
> to treat them the same.
>
> Having said all that, I would expect skip_to_optional_val() (as a
> name of the helper I suspect skip_to_optional_arg() is more
> appropriate, though)

Ok I will rename it skip_to_optional_arg().

> to store NULL in the arg thing if there is no
> optional argument given.  Also, the caller does not have to even do
> the 'arg ? arg : ""' dance if it is so common and natural for the
> application to treat "--key" and "--key=" equivalently, as it is
> expected that the actual code to work on the arg, i.e. do_something()
> in the above example, _should_ be prepared to take NULL and "" and
> treat them the same way (that is the definition of "it is so common
> and natural for the application to treat them the same).

I'd rather add the "const char *default" argument to the function
rather than have the function just set "arg" to NULL. I think setting
"arg" to NULL increases the risk of crashes and makes it too easy to
make "--key" and "--key=" behave differently which I think is not
consistent and not intuitive.

> So, I think you identified interesting pattern that can be cleaned
> up by introducing a helper, but I am not sure if I agree with the
> exact design of the helper.

Ok, maybe the above explains the design a bit better.

>> Note that "opt" in the function name actually means "optional" as
>> the function can be used to parse any "key=value" string where "key"
>> is also considered as valid, not just command line options.
>
> Yup.  This paragraph is a good thing to have in the proposed log
> message, to explain the reason why you force callers of this helper
> to spell out the leading dashes like "--key" (not "key").  I think
> that it is a sane design of the input side of this function---it
> does not limit it to an overly narrow case of command line option
> processing.  For the same reason, I think it is saner design of the
> output side to allow callers to tell between "key=" and "key".
>
> While staring at this helper and writing "does not limit to command
> line option processing", it occurs to me that a helper that allows
> you to look for an optional ':' (instead of '=') may also be useful,
> so the final version might become a pair of functions, perhaps like
> so:
>
>     int skip_to_optional_delim(const char *s,
>                                const char *prefix, char delim,
>                                const char **rest)
>     {
>         const char *p;
>
>         if (!skip_prefix(str, prefix, &p))
>                 return 0;
>         if (!*p)
>                 *rest = NULL;
>         else if (*p != delim)
>                 return 0;
>         else
>                 *rest = p + 1;
>         return 1;
>     }
>
>     int skip_to_optional_arg(const char *s,
>                              const char *prefix,
>                              const char **arg)
>     {
>         return skip_to_optional_delim(s, prefix, '=', arg);
>     }
>
> As the first one would not (by definition) have any callers
> initially after your series, it can be static to a file that
> implements both of them and it is OK to expose only the latter to
> the public.

Yeah, I thought about the above, but I am not very much interested in
implementing it now. I wonder if the callers will always want
skip_to_optional_delim() to take only one delim character or if they
will want more than one delim character.

> I do think it is a premature optimization to inline this function,
> whose initial callers will be (from the look of the remainder of
> your patches) command line parsing that sits farthest from any
> performance critical code.

Ok, I will not inline it.

Thanks!