mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Christian Couder <>
To: Junio C Hamano <>
Cc: git <>, Christian Couder <>
Subject: Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_val()
Date: Mon, 4 Dec 2017 08:59:12 +0100	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Sun, Dec 3, 2017 at 11:48 PM, Junio C Hamano <> wrote:
> Christian Couder <> 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 */

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 */

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.

  reply	other threads:[~2017-12-04  7:59 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-03 17:04 [PATCH 1/3] git-compat-util: introduce skip_to_opt_val() Christian Couder
2017-12-03 17:04 ` [PATCH 2/3] index-pack: use skip_to_opt_val() Christian Couder
2017-12-03 17:04 ` [PATCH 3/3] diff: " Christian Couder
2017-12-07  0:16   ` Jacob Keller
2017-12-07  0:18     ` Jacob Keller
2017-12-07  6:26       ` Christian Couder
2017-12-03 18:45 ` [PATCH 1/3] git-compat-util: introduce skip_to_opt_val() Junio C Hamano
2017-12-03 20:34   ` Christian Couder
2017-12-03 22:48     ` Junio C Hamano
2017-12-04  7:59       ` Christian Couder [this message]
2017-12-04 13:35         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='' \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).