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

On Mon, Mar 26, 2018 at 04:34:42AM -0400, Jeff King wrote:

quoted

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d..d9e389a33 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt

@@ -233,8 +233,10 @@ See also <<FILES>>.
 	using `--file`, `--global`, etc) and `on` when searching all
 	config files.

-CONFIGURATION
--------------
+--default value::
+  When using `--get`, behave as if value were the value assigned to the given
+  slot.
+
 `pager.config` is only respected when listing configuration, i.e., when
 using `--list` or any of the `--get-*` which may return multiple results.
 The default is to use a pager.

Hmm, what's going on with the CONFIGURATION header here?

My mistake, I have correct this erroneous diff in v3. Thanks for
pointing it out!

For the description of "--default" itself, do we want to say something
like:

  When using `--get` and the requested slot is not found, behave as
  if...

That is hopefully a given from the name "--default", but it seems like
an important point to mention.

Ditto.

quoted

+test_expect_success 'marshals default value as int' '
+	echo 810 >expect &&
+	git config --default 810 --int core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'marshals default value as int (storage unit)' '
+	echo 1048576 >expect &&
+	git config --default 1M --int core.foo >actual &&
+	test_cmp expect actual
+'

I'm not sure what we're really testing in the first one. The storage
unit conversion is the interesting bit.

TBH, I'm not sure we need to separately test each possible type
conversion here. If we know that the type conversions are tested
elsewhere (and I would not be surprised if they're not, but let's assume
for a moment...) and we check that type conversions are applied to
"--default" values for some types, I don't think there's any reason to
assume that the others would not work.

Agreed. I don't think that this test (and the ones that your comment
below concerns) are testing anything meaningful, or that would catch any
interesting future regressions. I have removed them from this series in
v3.


Thanks,
Taylor