Re: [PATCH 0/8] builtin/config: canonicalize "value_regex" with `--type=bool[-or-int]`

From: Jeff King <peff@peff.net>
Date: Nov 14, 2019, 06:29 UTC
Message-ID: <20191114062900.GD10643@sigill.intra.peff.net>
In-Reply-To: <cover.1573670565.git.martin.agren@gmail.com>

On Wed, Nov 13, 2019 at 07:54:59PM +0100, Martin Ågren wrote:

Show 8 quoted lines

> To find all config items "foo.*" that are configured as the Boolean
> value "true", you can try executing
> 
>   git config --type=bool --name-only --get-regexp '^foo\.' true
> 
> ... and hope that you didn't spell "true" as "on", "True" or "1" back
> when you populated your config. This shortcoming has been mentioned as a
> left-over bit [1] [2].

This seems like a good idea, but I wonder why we'd limit it to bools? It seems like any type would probably be better off matching a normalized version.

We already have two functions in builtin/config.c which handle types:

  - format_config() is for actually interpreting an existing value and
    writing it to stdout

  - normalize_value() is used when writing a new value into a config
    file, and normalizing what the user provided on the command-line

I don't think you'd want to use format_config() here. For example, if I say:

  git config --type=color --get-regexp ^foo red

I want to match the original "red" color, but _output_ the ANSI code. But normalize_value(), by contrast, leaves colors intact. So maybe it's a good match?

OTOH, I'd probably expect to match expanded pathnames or expiration dates there, too, and it doesn't expand those. Those ones are less clear to me. The whole premise of this series is making an assumption that "--type" is about how you want to match, and not just about what you want to output. In your example above it's clear that you don't care about the output type (because you're using --name-only), but for:

  git config --type=path --get-regexp ^foo /home/peff

it's unclear if you want to match a value of "~peff/foo", or if you simply want to output the expansion.

I wonder if we'd want to allow specifying the output type and the matching type separately? Maybe that just makes it more awkward to use for little benefit (I admit that it's hard to imagine somebody wanting to normalize bools on output but _not_ for matching).

Anyway. It would be nice if we could build on the existing logic in some way, rather than making a third place that has to handle every type we know about.

Show 10 quoted lines

> `--type=bool-or-int` gets the same treatment, except we need to to be
> able to handle the ints and regexes matching particular ints that we
> must expect. That said, even with `--type=bool` we can't move too
> aggressively towards *requiring* that the incoming "value_regex"
> canonializes as a Boolean value. The penultimate patch starts to warn on
> non-canonicalizing values; the final patch makes us bail out entirely.
> 
> The last patch is not meant for immediate inclusion, but I post it
> anyway. I can re-submit it at an appropriate time, or maybe it could
> slumber on pu until the time is ripe for completing the switch.

I think bailing on values that can't be converted is normal for other code paths. E.g., just trying to print:

  $ git -c foo.bar=abc config --type=bool foo.abr
  fatal: bad numeric config value 'abc' for 'foo.bar': invalid unit

-Peff