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

[Taylor Blau]

On Mon, Apr 09, 2018 at 08:18:18AM +0900, Junio C Hamano wrote:
> Jeff King  writes:
>
> > On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote:
> >
> >> @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char 
> >> *regex_)
> >>config_with_options(collect_config, ,
> >>_config_source, _options);
> >>
> >> +  if (!values.nr && default_value) {
> >> +  struct strbuf *item;
> >> +  ALLOC_GROW(values.items, values.nr + 1, values.alloc);
> >> +  item = [values.nr++];
> >> +  strbuf_init(item, 0);
> >> +  if (format_config(item, key_, default_value) < 0) {
> >> +  exit(1);
> >> +  }
> >> +  }
> > ...
> >
> > It's obvious in this toy example, but that config call may be buried
> > deep in a script. It'd probably be nicer for that exit(1) to be
> > something like:
> >
> >   die(_("failed to format default config value"));
>
> Together with key_ and default_value, you mean?
>
> >
> >> +test_expect_success 'does not allow --default without --get' '
> >> +  test_must_fail git config --default quux --unset a >output 2>&1 &&
> >> +  test_i18ngrep "\-\-default is only applicable to" output
> >> +'
> >
> > I think "a" here needs to be "a.section". I get:
> >
> >   error: key does not contain a section: a
>
> "section.var"?  That aside, even with a properly formatted key, I
> seem to get an empty output here, so this may need a bit more work.

This should be fixed in the latest re-roll, v6:

expecting success:
test_must_fail git config 
--default=quux --unset a.section >output 2>&1 &&
test_i18ngrep "\-\-default is only 
applicable to" output

error: --default is only applicable to --get
ok 5 - does not allow --default without --get


Thanks,
Taylor

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

[Junio C Hamano]

Jeff King  writes:

> On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote:
>
>> @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char 
>> *regex_)
>>  config_with_options(collect_config, ,
>>  _config_source, _options);
>>  
>> +if (!values.nr && default_value) {
>> +struct strbuf *item;
>> +ALLOC_GROW(values.items, values.nr + 1, values.alloc);
>> +item = [values.nr++];
>> +strbuf_init(item, 0);
>> +if (format_config(item, key_, default_value) < 0) {
>> +exit(1);
>> +}
>> +}
> ...
>
> It's obvious in this toy example, but that config call may be buried
> deep in a script. It'd probably be nicer for that exit(1) to be
> something like:
>
>   die(_("failed to format default config value"));

Together with key_ and default_value, you mean?

>
>> +test_expect_success 'does not allow --default without --get' '
>> +test_must_fail git config --default quux --unset a >output 2>&1 &&
>> +test_i18ngrep "\-\-default is only applicable to" output
>> +'
>
> I think "a" here needs to be "a.section". I get:
>
>   error: key does not contain a section: a

"section.var"?  That aside, even with a properly formatted key, I
seem to get an empty output here, so this may need a bit more work.

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

[Taylor Blau]

On Thu, Apr 05, 2018 at 06:40:03PM -0400, Eric Sunshine wrote:
> On Wed, Apr 4, 2018 at 10:59 PM, Taylor Blau  wrote:
> > [...]
> > This commit (and those following it in this series) aim to eventually
> > replace `--get-color` with a consistent alternative. By introducing
> > `--default`, we allow the `--get-color` action to be promoted to a
> > `--type=color` type specifier, retaining the "fallback" behavior via the
> > `--default` flag introduced in this commit.
> > [...]
> >
> > Signed-off-by: Taylor Blau 
> > ---
> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > @@ -235,6 +235,10 @@ Valid ``'s include:
> > +--default value::
>
> This should be typeset as: --default 

I see; thank you for letting me know. I have updated this in the
forthcoming re-roll.

> > +  When using `--get`, and the requested slot is not found, behave as if 
> > value
> > +  were the value assigned to the that slot.
>
> And you might use "behaves as if " in the body as well (though
> I think Git documentation isn't terribly consistent about typesetting
> as "" in the body, so I don't insist upon it).

Since I'm here, and re-rolling anyway, I have included this change as
well.


Thanks,
Taylor

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

[Taylor Blau]

On Thu, Apr 05, 2018 at 06:29:49PM -0400, Jeff King wrote:
> On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote:
>
> > @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char 
> > *regex_)
> > config_with_options(collect_config, ,
> > _config_source, _options);
> >
> > +   if (!values.nr && default_value) {
> > +   struct strbuf *item;
> > +   ALLOC_GROW(values.items, values.nr + 1, values.alloc);
> > +   item = [values.nr++];
> > +   strbuf_init(item, 0);
> > +   if (format_config(item, key_, default_value) < 0) {
> > +   exit(1);
> > +   }
> > +   }
>
> Calling exit() explicitly is unusual for our code. Usually we would
> either die() or propagate the error. Most of the types in
> format_config() would die on bogus input, but a few code paths will
> return an error.
>
> What happens if a non-default value has a bogus format? E.g.:
>
>   $ git config foo.bar '~NoSuchUser'
>   $ git config --path foo.bar
>   fatal: failed to expand user dir in: '~NoSuchUser'
>
> Oops. Despite us checking for an error return from
> git_config_pathname(), it will always either return 0 or die(). So
> that's not a good example. ;)
>
> Let's try expiry-date:
>
>   $ git config foo.bar 'the first of octember'
>   $ git config --expiry-date foo.bar
>   error: 'the first of octember' for 'foo.bar' is not a valid timestamp
>   fatal: bad config line 7 in file .git/config
>
> OK. So we call format_config() there from the actual collect_config()
> callback, and the error gets propagated back to the config parser, which
> then gives us an informative die(). What happens with your new code:
>
>   $ ./git config --default 'the first of octember' --type=expiry-date 
> no.such.key
>   error: 'the first of octember' for 'no.such.key' is not a valid timestamp
>
> It's obvious in this toy example, but that config call may be buried
> deep in a script. It'd probably be nicer for that exit(1) to be
> something like:
>
>   die(_("failed to format default config value"));

Aha. Thanks for the explanation :-). I've removed the exit() and changed
it to the die() that you suggested above. The test in t1310 is updated
to grep for the new message, so we know that it is being reported
appropriately.

> > +test_expect_success 'does not allow --default without --get' '
> > +   test_must_fail git config --default quux --unset a >output 2>&1 &&
> > +   test_i18ngrep "\-\-default is only applicable to" output
> > +'
>
> I think "a" here needs to be "a.section". I get:
>
>   error: key does not contain a section: a

Aha, thanks again. I have updated this in the forthcoming re-roll.


Thanks,
Taylor

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

[Eric Sunshine]

On Wed, Apr 4, 2018 at 10:59 PM, Taylor Blau  wrote:
> [...]
> This commit (and those following it in this series) aim to eventually
> replace `--get-color` with a consistent alternative. By introducing
> `--default`, we allow the `--get-color` action to be promoted to a
> `--type=color` type specifier, retaining the "fallback" behavior via the
> `--default` flag introduced in this commit.
> [...]
>
> Signed-off-by: Taylor Blau 
> ---
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> @@ -235,6 +235,10 @@ Valid ``'s include:
> +--default value::

This should be typeset as: --default 

> +  When using `--get`, and the requested slot is not found, behave as if value
> +  were the value assigned to the that slot.

And you might use "behaves as if " in the body as well (though
I think Git documentation isn't terribly consistent about typesetting
as "" in the body, so I don't insist upon it).

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

[Jeff King]

On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote:

> @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char 
> *regex_)
>   config_with_options(collect_config, ,
>   _config_source, _options);
>  
> + if (!values.nr && default_value) {
> + struct strbuf *item;
> + ALLOC_GROW(values.items, values.nr + 1, values.alloc);
> + item = [values.nr++];
> + strbuf_init(item, 0);
> + if (format_config(item, key_, default_value) < 0) {
> + exit(1);
> + }
> + }

Calling exit() explicitly is unusual for our code. Usually we would
either die() or propagate the error. Most of the types in
format_config() would die on bogus input, but a few code paths will
return an error.

What happens if a non-default value has a bogus format? E.g.:

  $ git config foo.bar '~NoSuchUser'
  $ git config --path foo.bar
  fatal: failed to expand user dir in: '~NoSuchUser'

Oops. Despite us checking for an error return from
git_config_pathname(), it will always either return 0 or die(). So
that's not a good example. ;)

Let's try expiry-date:

  $ git config foo.bar 'the first of octember'
  $ git config --expiry-date foo.bar
  error: 'the first of octember' for 'foo.bar' is not a valid timestamp
  fatal: bad config line 7 in file .git/config

OK. So we call format_config() there from the actual collect_config()
callback, and the error gets propagated back to the config parser, which
then gives us an informative die(). What happens with your new code:

  $ ./git config --default 'the first of octember' --type=expiry-date 
no.such.key
  error: 'the first of octember' for 'no.such.key' is not a valid timestamp

It's obvious in this toy example, but that config call may be buried
deep in a script. It'd probably be nicer for that exit(1) to be
something like:

  die(_("failed to format default config value"));

> +test_expect_success 'does not allow --default without --get' '
> + test_must_fail git config --default quux --unset a >output 2>&1 &&
> + test_i18ngrep "\-\-default is only applicable to" output
> +'

I think "a" here needs to be "a.section". I get:

  error: key does not contain a section: a

-Peff

[PATCH v4 1/3] builtin/config: introduce `--default`

[Taylor Blau]

For some use cases, callers of the `git-config(1)` builtin would like to
fallback to default values when the variable asked for does not exist.
In addition, users would like to use existing type specifiers to ensure
that values are parsed correctly when they do exist in the
configuration.

For example, to fetch a value without a type specifier and fallback to
`$fallback`, the following is required:

  $ git config core.foo || echo "$fallback"

This is fine for most values, but can be tricky for difficult-to-express
`$fallback`'s, like ANSI color codes.

This motivates `--get-color`, which is a one-off exception to the normal
type specifier rules wherein a user specifies both the configuration
variable and an optional fallback. Both are formatted according to their
type specifier, which eases the burden on the user to ensure that values
are correctly formatted.

This commit (and those following it in this series) aim to eventually
replace `--get-color` with a consistent alternative. By introducing
`--default`, we allow the `--get-color` action to be promoted to a
`--type=color` type specifier, retaining the "fallback" behavior via the
`--default` flag introduced in this commit.

For example, we aim to replace:

  $ git config --get-color variable [default] [...]

with:

  $ git config --default default --type=color variable [...]

Values filled by `--default` behave exactly as if they were present in
the affected configuration file; they will be parsed by type specifiers
without the knowledge that they are not themselves present in the
configuration.

Specifically, this means that the following will work:

  $ git config --int --default 1M does.not.exist
  1048576

In subsequent commits, we will offer `--type=color`, which (in
conjunction with `--default`) will be sufficient to replace
`--get-color`.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt |  4 
 builtin/config.c | 12 
 t/t1310-config-default.sh| 38 
 3 files changed, 54 insertions(+)
 create mode 100755 t/t1310-config-default.sh

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index c7e56e3fd..620492d1e 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -235,6 +235,10 @@ Valid ``'s include:
using `--file`, `--global`, etc) and `on` when searching all
config files.
 
+--default value::
+  When using `--get`, and the requested slot is not found, behave as if value
+  were the value assigned to the that slot.
+
 CONFIGURATION
 -
 `pager.config` is only respected when listing configuration, i.e., when
diff --git a/builtin/config.c b/builtin/config.c
index 6e1495eac..1328b568b 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -26,6 +26,7 @@ static char term = '\n';
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
 static int actions, type;
+static char *default_value;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -122,6 +123,7 @@ static struct option builtin_config_options[] = {
OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
OPT_BOOL(0, "includes", _includes_opt, N_("respect include 
directives on lookup")),
OPT_BOOL(0, "show-origin", _origin, N_("show origin of config 
(file, standard input, blob, command line)")),
+   OPT_STRING(0, "default", _value, N_("value"), N_("with --get, 
use default value when missing entry")),
OPT_END(),
 };
 
@@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)
config_with_options(collect_config, ,
_config_source, _options);
 
+   if (!values.nr && default_value) {
+   struct strbuf *item;
+   ALLOC_GROW(values.items, values.nr + 1, values.alloc);
+   item = [values.nr++];
+   strbuf_init(item, 0);
+   if (format_config(item, key_, default_value) < 0) {
+   exit(1);
+   }
+   }
+
ret = !values.nr;
 
for (i = 0; i < values.nr; i++) {
diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
new file mode 100755
index 0..ab4e35f06
--- /dev/null
+++ b/t/t1310-config-default.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='Test git config in different settings (with --default)'
+
+. ./test-lib.sh
+
+test_expect_success 'uses --default when missing entry' '
+   echo quux >expect &&
+   git config -f config --default quux core.foo >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'canonicalizes --default with appropriate type' '
+   echo true >expect &&
+   git config -f config --default=true --bool core.foo >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'uses entry when available' '
+   echo bar