On Mon, Mar 05, 2018 at 06:17:26PM -0800, Taylor Blau wrote:
> 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.
> 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:
> + 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).
> +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
