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.

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. ;)

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