Re: [PATCH 1/4] builtin/config: introduce `--default`

From: Jeff King <peff@peff.net>
Date: Mar 6, 2018, 06:52 UTC
Message-ID: <20180306065208.GB3522@sigill.intra.peff.net>
In-Reply-To: <20180306021729.45813-2-me@ttaylorr.com>

On Mon, Mar 05, 2018 at 06:17:26PM -0800, Taylor Blau wrote:

Show 10 quoted lines

> In an aim to replace:
> 
>   $ git config --get-color slot [default] [...]
> 
> with:
> 
>   $ git config --default default --color slot [...]
> 
> introduce `--defualt` to behave as if the given default were present and
> assigned to slot in the case that that slot does not exist.

I think this motivation skips over the beginning part of the story, which is why we want "--color --default". :)

IMHO, the reason we want --default is two-fold:

  1. Callers have to handle parsing defaults themselves, like:

       foo=$(git config core.foo || echo 1234)

     For an integer, that's not too bad, since you can write "1048576"
     instead of "1M". For colors, it's abominable, which is why we added
     "--get-color". But as we add more types that are hard to parse
     (like --expiry-date), it would be nice for them to get the same
     defaulting feature without adding --get-expiry-date, etc.

  2. --get-color is a one-off unlike all of the other types. That's bad
     interface design, but the inconsistency also makes it harder to add
     features which treat the types uniformly (like, say, a --stdin
     query mode).

And perhaps minor, but it's also easier to correctly error-check --default, since the "foo" example above would do the wrong thing if git-config encountered a fatal error.

Show 11 quoted lines

> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 14da5fc15..390b49831 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -233,6 +233,10 @@ See also <<FILES>>.
>  	using `--file`, `--global`, etc) and `on` when searching all
>  	config files.
>  
> +--default value::
> +  When using `--get`, `--get-all`, and `--get-regexp`, behave as
> +  if value were the value assigned to the given slot.

I had thought about this in the context of --get, where a single value makes sense.

For --get-all, would we want to be able to specify a list of objects? E.g.:

  git config --default foo --default bar --get-all core.slot

and behave as if we found two entries, "foo" and "bar"?

I'm not really sure what semantics would be most useful.

Ditto for --get-regexp.

This isn't necessarily an objection. I'm just not sure what people would expect. So it might make sense to start more limited and wait for a real use case to pop up. But I'm also open to arguments about plausible use cases. ;)

> diff --git a/builtin/config.c b/builtin/config.c
> index ab5f95476..76edefc07 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c

The general design of the implementation looks good. There's one funny thing:

Show 8 quoted lines

> +	if (!values.nr && default_value) {
> +		struct strbuf *item;
> +
> +		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
> +		item = &values.items[values.nr++];
> +		if (format_config(item, key_, default_value) < 0) {
> +			values.nr = 0;
> +		}

We never initialize the strbuf data here, and we can't count on ALLOC_GROW to even zero it (which I suspect would work, but still isn't how strbufs are meant to be used).

Do you need:

  strbuf_init(item, 0);

here, similar to what collect_config does?

(As an aside, it seems like this whole thing might be simpler with a string_list, but that's certainly not a problem that you're introducing here).

Show 7 quoted lines

> +test_expect_success 'marshals default value as bool-or-int' '
> +	echo "1
> +true" >expect &&
> +	git config --default 1 --bool-or-int core.foo >actual &&
> +	git config --default true --bool-or-int core.foo >>actual &&
> +	test_cmp expect actual
> +'

Funny indentation. Use:

  {
	echo 1 &&
	echo true
  } >expect &&

or

  cat >expect <<-\EOF
  1
  true
  EOF

-Peff