git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
	"Johannes Schindelin" <johannes.schindelin@gmx.de>,
	"Stefan Beller" <sbeller@google.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [RFC PATCH 3/4] color.ui config: don't die on unknown values
Date: Wed, 30 May 2018 21:06:40 +0000	[thread overview]
Message-ID: <20180530210641.19771-4-avarab@gmail.com> (raw)
In-Reply-To: <20180530210641.19771-1-avarab@gmail.com>

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".

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@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.
+	 */
+	if (git_parse_maybe_bool(value) < 0) {
+		static int warned = 0;
+		if (!warned++)
+			warning(_("unknown value '%s' for '%s', falling back to 'auto'"),
+				value, var);
+		return GIT_COLOR_AUTO;
+	}
+
 	/* Missing or explicit false to turn off colorization */
 	if (!git_config_bool(var, value))
 		return 0;
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 7541ba5edb..b16f2ac28b 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -309,6 +309,14 @@ test_expect_success 'no color when stdout is a regular file' '
 	! colorful colorless.log
 '
 
+test_expect_success 'unknown color.ui values default to "auto" (regular file)' '
+	rm -f colorless.log &&
+	test_config color.ui doesnotexist &&
+	git log >colorless.log 2>err &&
+	test_i18ngrep "falling back" err &&
+	! colorful colorless.log
+'
+
 test_expect_success TTY 'color when writing to a pager' '
 	rm -f paginated.out &&
 	test_config color.ui auto &&
@@ -316,6 +324,14 @@ test_expect_success TTY 'color when writing to a pager' '
 	colorful paginated.out
 '
 
+test_expect_success TTY 'unknown color.ui values default to "auto" (pager)' '
+	rm -f paginated.out &&
+	test_config color.ui doesnotexist &&
+	test_terminal git log 2>err &&
+	test_i18ngrep "falling back" err &&
+	colorful paginated.out
+'
+
 test_expect_success TTY 'colors are suppressed by color.pager' '
 	rm -f paginated.out &&
 	test_config color.ui auto &&
-- 
2.17.0.290.gded63e768a


  parent reply	other threads:[~2018-05-30 21:07 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-30 21:06 [PATCH 0/4] color.ui docs & add color.ui=isatty Ævar Arnfjörð Bjarmason
2018-05-30 21:06 ` [PATCH 1/4] config doc: move color.ui documentation to one place Ævar Arnfjörð Bjarmason
2018-05-31  5:25   ` Jeff King
2018-05-31  7:09     ` Ævar Arnfjörð Bjarmason
2018-06-01  5:31       ` Jeff King
2018-05-30 21:06 ` [PATCH 2/4] config doc: clarify "to a terminal" in color.ui Ævar Arnfjörð Bjarmason
2018-05-31  5:27   ` Jeff King
2018-05-30 21:06 ` Ævar Arnfjörð Bjarmason [this message]
2018-05-30 22:32   ` [RFC PATCH 3/4] color.ui config: don't die on unknown values Stefan Beller
2018-05-30 23:05   ` Junio C Hamano
2018-05-31  7:17     ` Ævar Arnfjörð Bjarmason
2018-06-01  5:53       ` Jeff King
2018-05-30 21:06 ` [RFC PATCH 4/4] color.ui config: add "isatty" setting Ævar Arnfjörð Bjarmason
2018-05-30 22:57   ` Junio C Hamano
2018-05-31  7:07     ` Ævar Arnfjörð Bjarmason
2018-05-31  5:38   ` Jeff King
2018-05-31  7:01     ` Ævar Arnfjörð Bjarmason
2018-06-01  5:30       ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180530210641.19771-4-avarab@gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=peff@peff.net \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).