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

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

On Sun, Dec 3, 2017 at 11:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> Anyway there is a design choice to be made. Adding a "const char
>> *default" argument makes the function more generic,...
>
> I didn't suggest anything of that sort, and I do not understand why
> you are repeatedly talking about "default" that you considered and
> rejected, as if it were an alternative viable option.  I agree that
> "default" is not yet a good idea and it is a solution to a problem
> that is not yet shown to exist.
>
> On the other hand, just assigning NULL to *arg when you did not see
> a delimiting '=', on the other hand, is an alternative option that
> is viable.

What I am saying is that I'd rather have a lot of code like:

        if (skip_to_optional_val(arg, "--key", &arg, "") /* the last
argument is the default value */
                do_something(arg);

than a lot of code like this:

        if (skip_to_optional_val(arg, "--key", &arg) /* no default can
be passed, NULL is the default */
                do_something(arg ? arg : "");

because in the former case the `arg ? arg : ""` pattern is captured
inside the function, so the code is simpler.

In the few cases where do_something() accepts NULL and does something
different with it, the former can be changed to:

        if (skip_to_optional_val(arg, "--key", &arg, NULL) /* the last
argument is the default value */
                do_something(arg);

So yeah I rejected it, but my preference is not strong and I never
said or thought that it was not viable. I just think that there are
few cases that might benefit. So the benefits are not big and it might
be better for consistency and simplicity of the UI to nudge people to
make "--key" and "--key=" behave the same. That's why having "" as the
default and no default argument is a little better in my opinion.

>> .... 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 now this is very specific to the need of command line argument
> parsing and is not a generic thing?  You cannot have your cake and
> eat it too, though.

I think that even when we are not parsing options, it is probably good
in general for UI consistency and simplicity that "key" and "key=" are
interpreted in the same way.