On Wed, May 30, 2018 at 2:06 PM, Ævar Arnfjörð Bjarmason
<ava...@gmail.com> wrote:
> Before this change git will die on any unknown color.ui values:
>
> $ git -c color.ui=doesnotexist show
> fatal: bad numeric config value 'doesnotexist' for 'color.ui': invalid
> unit
>
> This makes the failure mode of introducing any new values in the
> future really bad, as explained in the documentation being added
> here. Instead let's warn and fall back to "auto".
We had a similar regression in the protocol.version setting in 2.16 IIRC.
It makes sense to fix it this way.
> The reason for the !warned++ pattern is when stepping through this in
> the debugger I found that git_config_colorbool() is called more than
> once on e.g. a "show" if color.ui=foo is set in the config, but
> color.ui=bar in the command-line, and would then warn about
> both.
>
> Maybe we should warn about both in that case, but I don't know if
> there's other cases where not doing this would cause a warning flood,
> and in any case the user is unlikely to have such a bad value in
> multiple places, so this should be good enough.
agreed.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <ava...@gmail.com>
> ---
> Documentation/config.txt | 5 +++++
> color.c | 13 +++++++++++++
> t/t7006-pager.sh | 16 ++++++++++++++++
> 3 files changed, 34 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 4767363519..b882a88214 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1291,6 +1291,11 @@ color.ui::
> want such output to use color when written to the terminal (as
> determined by a call to `isatty(3)`) or to a pager (unless
> `color.pager` is set to false).
> ++
> +Setting this to some value unknown to git will warn and fall back to
> +`auto`. This is so that new values can be recognized in the future
> +without the git configuration file being incompatible between versions
> +to the point of most porcelain commands dying on the older version.
>
> column.ui::
> Specify whether supported commands should output in columns.
> diff --git a/color.c b/color.c
> index b1c24c69de..e52c6cdd29 100644
> --- a/color.c
> +++ b/color.c
> @@ -311,6 +311,19 @@ int git_config_colorbool(const char *var, const char
> *value)
> if (!var)
> return -1;
>
> + /*
> + * If future git versions introduce new color.ui settings we
> + * don't want to die right below when git_config_bool() fails
> + * to parse them as bool.
> + */
I am not sure we need to document this here. Ignoring unknown
(or warning about unknown) values is the standard for config keys
at least, for values we probably want a similar thing.
> + if (git_parse_maybe_bool(value) < 0) {
> + static int warned = 0;
As someone who moved a lot of global state into the repository object
this is a fine example where we do not want to have the counter not
in the repository? (although strictly speaking you'd want to have the
warning approximately once per repository that is configured wrongly
or do you want to have this warning once at all?)
I think we should take patches 1-3 at least.
Thanks,
Stefan
