threads / patch / 28148

patch, 10 partscolor and pager improvements

Subject: [PATCH 0/10] color and pager improvements

## tl;dr

37 messages between Aug 18, 2011 and Feb 14, 2012. Diffs are folded; open one to read it.

replies: 36people: 6as markdown or json

Jeff King· Aug 18, 2011, 04:58 UTC · lore

While looking at the pager and color code today, I decided to tackle two long-standing bugs, which entailed a lot of refactoring of the color code. The result fixes some minor bugs, and is a little nicer for calling code to use.

  [01/10]: t7006: modernize calls to unset
  [02/10]: test-lib: add helper functions for config
  [03/10]: t7006: use test_config helpers

These three are just cleanup I noticed before adding new tests to t7006; I hope that the helpers in 02/10 will be useful in a lot of other places, though.

  [04/10]: setup_pager: set GIT_PAGER_IN_USE
This fixes Ingo's problem from:
  http://article.gmane.org/gmane.comp.version-control.git/179430
Namely that:
  git -p stash show
fails to use colors properly.
  [05/10]: diff: refactor COLOR_DIFF from a flag into an int
  [06/10]: git_config_colorbool: refactor stdout_is_tty handling
  [07/10]: color: delay auto-color decision until point of use
These three fix the problem Steffen mentioned here:
  http://article.gmane.org/gmane.comp.version-control.git/177792

Namely that pager.color doesn't work in many cases. This has been a problem for years, but spread due to some pager-ordering changes late last year (see the comments in 07/10). I actually wonder if anyone is really using pager.color, as we haven't seen many complaints about it.

  [08/10]: config: refactor get_colorbool function
  [09/10]: diff: don't load color config in plumbing
  [10/10]: want_color: automatically fallback to color.ui

These three are refactoring that is made possible by 07/10. I think they make the code cleaner, and hopefully the diffstat of 10/10 speaks for itself.

-Peff
Jeff King· Aug 18, 2011, 05:00 UTC · re: Jeff King · lore

[PATCH 01/10] t7006: modernize calls to unset

These tests break &&-chaining to deal with broken "unset" implementations. Instead, they should just use sane_unset.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7006-pager.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t7006-pager.sh +2 −2
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index ed7575d..c0a3135 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -402,7 +402,7 @@ test_core_pager_subdir    expect_success test_must_fail \
 					 'git -p apply </dev/null'
 
 test_expect_success TTY 'command-specific pager' '
-	unset PAGER GIT_PAGER;
+	sane_unset PAGER GIT_PAGER &&
 	echo "foo:initial" >expect &&
 	>actual &&
 	git config --unset core.pager &&
@@ -412,7 +412,7 @@ test_expect_success TTY 'command-specific pager' '
 '
 
 test_expect_success TTY 'command-specific pager overrides core.pager' '
-	unset PAGER GIT_PAGER;
+	sane_unset PAGER GIT_PAGER &&
 	echo "foo:initial" >expect &&
 	>actual &&
 	git config core.pager "exit 1"
-- 
1.7.6.10.g62f04
Junio C Hamano· Aug 18, 2011, 21:05 UTC · re: Jeff King · lore

Re: [PATCH 01/10] t7006: modernize calls to unset

Jeff King <peff@peff.net> writes:
> These tests break &&-chaining to deal with broken "unset"
> implementations. Instead, they should just use sane_unset.

Thanks. I checked with POSIX again, wondering if I should tone the "broken" down a bit, but it says:

  Unsetting a variable or function that was not previously set shall not
  be considered an error...
so they deserve "broken" label.
Jeff King· Aug 18, 2011, 05:01 UTC · re: Jeff King · lore

[PATCH 02/10] test-lib: add helper functions for config

There are a few common tasks when working with configuration variables in tests; this patch aims to make them a little easier to write and less error-prone.

When setting a variable, you should typically make sure to clean it up after the test is finished, so as not to pollute other tests. Like:

   test_when_finished 'git config --unset foo.bar' &&
   git config foo.bar baz
This patch lets you just write:
  test_config foo.bar baz

When clearing a variable that does not exist, git-config will report a specific non-zero error code. Meaning that tests which call "git config --unset" often either rely on the prior tests having actually set it, or must use test_might_fail. With this patch, the previous:

  test_might_fail git config --unset foo.bar
becomes:
  test_unconfig foo.bar

Not only is this easier to type, but it is more robust; it will correctly detect errors from git-config besides "key was not set".

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)
Show changes to t/test-lib.sh +18 −0
diff --git a/t/test-lib.sh b/t/test-lib.sh
index df25f17..926667a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -357,6 +357,24 @@ test_chmod () {
 	git update-index --add "--chmod=$@"
 }
 
+# Unset a configuration variable, but don't fail if it doesn't exist.
+test_unconfig () {
+	git config --unset-all "$@"
+	config_status=$?
+	case "$config_status" in
+	5) # ok, nothing to usnet
+		config_status=0
+		;;
+	esac
+	return $config_status
+}
+
+# Set git config, automatically unsetting it after the test is over.
+test_config () {
+	test_when_finished "test_unconfig '$1'" &&
+	git config "$@"
+}
+
 # Use test_set_prereq to tell that a particular prerequisite is available.
 # The prerequisite can later be checked for in two ways:
 #
-- 
1.7.6.10.g62f04
Junio C Hamano· Aug 18, 2011, 21:10 UTC · re: Jeff King · lore

Re: [PATCH 02/10] test-lib: add helper functions for config

Thanks.
Show 9 quoted lines
> ...
> +# Unset a configuration variable, but don't fail if it doesn't exist.
> +test_unconfig () {
> +	git config --unset-all "$@"
> +	config_status=$?
> +	case "$config_status" in
> +	5) # ok, nothing to usnet
> +		config_status=0
> +		;;
Will queue with 's/nothing to usnet/nothing to unset/'.
Jeff King· Aug 18, 2011, 05:02 UTC · re: Jeff King · lore

[PATCH 03/10] t7006: use test_config helpers

In some cases, this is just making the test script a little shorter and easier to read. However, there are several places where we didn't take proper precautions against polluting downstream tests with our config; this fixes them, too.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7006-pager.sh |   39 ++++++++++++++++++---------------------
 1 files changed, 18 insertions(+), 21 deletions(-)
Show changes to t/t7006-pager.sh +18 −21
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index c0a3135..2ac729f 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -13,7 +13,7 @@ cleanup_fail() {
 
 test_expect_success 'setup' '
 	sane_unset GIT_PAGER GIT_PAGER_IN_USE &&
-	test_might_fail git config --unset core.pager &&
+	test_unconfig core.pager &&
 
 	PAGER="cat >paginated.out" &&
 	export PAGER &&
@@ -94,21 +94,19 @@ test_expect_success TTY 'no pager with --no-pager' '
 
 test_expect_success TTY 'configuration can disable pager' '
 	rm -f paginated.out &&
-	test_might_fail git config --unset pager.grep &&
+	test_unconfig pager.grep &&
 	test_terminal git grep initial &&
 	test -e paginated.out &&
 
 	rm -f paginated.out &&
-	git config pager.grep false &&
-	test_when_finished "git config --unset pager.grep" &&
+	test_config pager.grep false &&
 	test_terminal git grep initial &&
 	! test -e paginated.out
 '
 
 test_expect_success TTY 'git config uses a pager if configured to' '
 	rm -f paginated.out &&
-	git config pager.config true &&
-	test_when_finished "git config --unset pager.config" &&
+	test_config pager.config true &&
 	test_terminal git config --list &&
 	test -e paginated.out
 '
@@ -116,8 +114,7 @@ test_expect_success TTY 'git config uses a pager if configured to' '
 test_expect_success TTY 'configuration can enable pager (from subdir)' '
 	rm -f paginated.out &&
 	mkdir -p subdir &&
-	git config pager.bundle true &&
-	test_when_finished "git config --unset pager.bundle" &&
+	test_config pager.bundle true &&
 
 	git bundle create test.bundle --all &&
 	rm -f paginated.out subdir/paginated.out &&
@@ -150,7 +147,7 @@ test_expect_success 'tests can detect color' '
 
 test_expect_success 'no color when stdout is a regular file' '
 	rm -f colorless.log &&
-	git config color.ui auto ||
+	test_config color.ui auto ||
 	cleanup_fail &&
 
 	git log >colorless.log &&
@@ -159,7 +156,7 @@ test_expect_success 'no color when stdout is a regular file' '
 
 test_expect_success TTY 'color when writing to a pager' '
 	rm -f paginated.out &&
-	git config color.ui auto ||
+	test_config color.ui auto ||
 	cleanup_fail &&
 
 	(
@@ -172,7 +169,7 @@ test_expect_success TTY 'color when writing to a pager' '
 
 test_expect_success 'color when writing to a file intended for a pager' '
 	rm -f colorful.log &&
-	git config color.ui auto ||
+	test_config color.ui auto ||
 	cleanup_fail &&
 
 	(
@@ -221,7 +218,7 @@ test_default_pager() {
 
 	$test_expectation SIMPLEPAGER,TTY "$cmd - default pager is used by default" "
 		sane_unset PAGER GIT_PAGER &&
-		test_might_fail git config --unset core.pager &&
+		test_unconfig core.pager &&
 		rm -f default_pager_used ||
 		cleanup_fail &&
 
@@ -244,7 +241,7 @@ test_PAGER_overrides() {
 
 	$test_expectation TTY "$cmd - PAGER overrides default pager" "
 		sane_unset GIT_PAGER &&
-		test_might_fail git config --unset core.pager &&
+		test_unconfig core.pager &&
 		rm -f PAGER_used ||
 		cleanup_fail &&
 
@@ -277,7 +274,7 @@ test_core_pager() {
 
 		PAGER=wc &&
 		export PAGER &&
-		git config core.pager 'wc >core.pager_used' &&
+		test_config core.pager 'wc >core.pager_used' &&
 		$full_command &&
 		${if_local_config}test -e core.pager_used
 	"
@@ -307,7 +304,7 @@ test_pager_subdir_helper() {
 		PAGER=wc &&
 		stampname=\$(pwd)/core.pager_used &&
 		export PAGER stampname &&
-		git config core.pager 'wc >\"\$stampname\"' &&
+		test_config core.pager 'wc >\"\$stampname\"' &&
 		mkdir sub &&
 		(
 			cd sub &&
@@ -324,7 +321,7 @@ test_GIT_PAGER_overrides() {
 		rm -f GIT_PAGER_used ||
 		cleanup_fail &&
 
-		git config core.pager wc &&
+		test_config core.pager wc &&
 		GIT_PAGER='wc >GIT_PAGER_used' &&
 		export GIT_PAGER &&
 		$full_command &&
@@ -405,8 +402,8 @@ test_expect_success TTY 'command-specific pager' '
 	sane_unset PAGER GIT_PAGER &&
 	echo "foo:initial" >expect &&
 	>actual &&
-	git config --unset core.pager &&
-	git config pager.log "sed s/^/foo:/ >actual" &&
+	test_unconfig core.pager &&
+	test_config pager.log "sed s/^/foo:/ >actual" &&
 	test_terminal git log --format=%s -1 &&
 	test_cmp expect actual
 '
@@ -415,8 +412,8 @@ test_expect_success TTY 'command-specific pager overrides core.pager' '
 	sane_unset PAGER GIT_PAGER &&
 	echo "foo:initial" >expect &&
 	>actual &&
-	git config core.pager "exit 1"
-	git config pager.log "sed s/^/foo:/ >actual" &&
+	test_config core.pager "exit 1"
+	test_config pager.log "sed s/^/foo:/ >actual" &&
 	test_terminal git log --format=%s -1 &&
 	test_cmp expect actual
 '
@@ -425,7 +422,7 @@ test_expect_success TTY 'command-specific pager overridden by environment' '
 	GIT_PAGER="sed s/^/foo:/ >actual" && export GIT_PAGER &&
 	>actual &&
 	echo "foo:initial" >expect &&
-	git config pager.log "exit 1" &&
+	test_config pager.log "exit 1" &&
 	test_terminal git log --format=%s -1 &&
 	test_cmp expect actual
 '
-- 
1.7.6.10.g62f04
Jeff King· Aug 18, 2011, 05:02 UTC · re: Jeff King · lore

[PATCH 04/10] setup_pager: set GIT_PAGER_IN_USE

We have always set a global "spawned_pager" variable when we start the pager. This lets us make the auto-color decision later in the program as as "we are outputting to a terminal, or to a pager which can handle colors".

Commit 6e9af86 added support for the GIT_PAGER_IN_USE environment variable. An external program calling git (e.g., git-svn) could set this variable to indicate that it had already started the pager, and that the decision about auto-coloring should take that into account.

However, 6e9af86 failed to do the reverse, which is to tell external programs when git itself has started the pager. Thus a git command implemented as an external script that has the pager turned on (e.g., "git -p stash show") would not realize it was going to a pager, and would suppress colors.

This patch remedies that; we always set GIT_PAGER_IN_USE when we start the pager, and the value is respected by both this program and any spawned children.

Signed-off-by: Jeff King <peff@peff.net>
---
 pager.c          |    8 +-------
 t/t7006-pager.sh |   11 +++++++++++
 2 files changed, 12 insertions(+), 7 deletions(-)
Show changes to 2 files +12 −7

pager.c, t/t7006-pager.sh

diff --git a/pager.c b/pager.c
index dac358f..975955b 100644
--- a/pager.c
+++ b/pager.c
@@ -11,8 +11,6 @@
  * something different on Windows.
  */
 
-static int spawned_pager;
-
 #ifndef WIN32
 static void pager_preexec(void)
 {
@@ -78,7 +76,7 @@ void setup_pager(void)
 	if (!pager)
 		return;
 
-	spawned_pager = 1; /* means we are emitting to terminal */
+	setenv("GIT_PAGER_IN_USE", "true", 1);
 
 	/* spawn the pager */
 	pager_argv[0] = pager;
@@ -109,10 +107,6 @@ void setup_pager(void)
 int pager_in_use(void)
 {
 	const char *env;
-
-	if (spawned_pager)
-		return 1;
-
 	env = getenv("GIT_PAGER_IN_USE");
 	return env ? git_config_bool("GIT_PAGER_IN_USE", env) : 0;
 }
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 2ac729f..4884e1b 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -181,6 +181,17 @@ test_expect_success 'color when writing to a file intended for a pager' '
 	colorful colorful.log
 '
 
+test_expect_success TTY 'colors are sent to pager for external commands' '
+	test_config alias.externallog "!git log" &&
+	test_config color.ui auto &&
+	(
+		TERM=vt100 &&
+		export TERM &&
+		test_terminal git -p externallog
+	) &&
+	colorful paginated.out
+'
+
 # Use this helper to make it easy for the caller of your
 # terminal-using function to specify whether it should fail.
 # If you write
-- 
1.7.6.10.g62f04
Jeff King· Aug 18, 2011, 05:03 UTC · re: Jeff King · lore

[PATCH 05/10] diff: refactor COLOR_DIFF from a flag into an int

This lets us store more than just a bit flag for whether we want color; we can also store whether we want automatic colors. This can be useful for making the automatic-color decision closer to the point of use.

This mostly just involves replacing DIFF_OPT_* calls with manipulations of the flag. The biggest exception is that calls to DIFF_OPT_TST must check for "o->use_color > 0", which lets an "unknown" value (i.e., the default) stay at "no color". In the previous code, a value of "-1" was not propagated at all.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/merge.c |    2 --
 combine-diff.c  |    7 +++----
 diff.c          |   41 +++++++++++++++++++----------------------
 diff.h          |    5 +++--
 graph.c         |    2 +-
 log-tree.c      |    4 ++--
 wt-status.c     |    2 +-
 7 files changed, 29 insertions(+), 34 deletions(-)
Show changes to 7 files +29 −34

builtin/merge.c, combine-diff.c, diff.c, diff.h, graph.c, log-tree.c, wt-status.c

diff --git a/builtin/merge.c b/builtin/merge.c
index 325891e..7209edf 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -390,8 +390,6 @@ static void finish(const unsigned char *new_head, const char *msg)
 		opts.output_format |=
 			DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
 		opts.detect_rename = DIFF_DETECT_RENAME;
-		if (diff_use_color_default > 0)
-			DIFF_OPT_SET(&opts, COLOR_DIFF);
 		if (diff_setup_done(&opts) < 0)
 			die(_("diff_setup_done failed"));
 		diff_tree_sha1(head, new_head, "", &opts);
diff --git a/combine-diff.c b/combine-diff.c
index be67cfc..c588c79 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -702,9 +702,8 @@ static void show_combined_header(struct combine_diff_path *elem,
 	int abbrev = DIFF_OPT_TST(opt, FULL_INDEX) ? 40 : DEFAULT_ABBREV;
 	const char *a_prefix = opt->a_prefix ? opt->a_prefix : "a/";
 	const char *b_prefix = opt->b_prefix ? opt->b_prefix : "b/";
-	int use_color = DIFF_OPT_TST(opt, COLOR_DIFF);
-	const char *c_meta = diff_get_color(use_color, DIFF_METAINFO);
-	const char *c_reset = diff_get_color(use_color, DIFF_RESET);
+	const char *c_meta = diff_get_color_opt(opt, DIFF_METAINFO);
+	const char *c_reset = diff_get_color_opt(opt, DIFF_RESET);
 	const char *abb;
 	int added = 0;
 	int deleted = 0;
@@ -964,7 +963,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 		show_combined_header(elem, num_parent, dense, rev,
 				     mode_differs, 1);
 		dump_sline(sline, cnt, num_parent,
-			   DIFF_OPT_TST(opt, COLOR_DIFF), result_deleted);
+			   opt->use_color, result_deleted);
 	}
 	free(result);
 
diff --git a/diff.c b/diff.c
index 93ef9a2..2d86abe 100644
--- a/diff.c
+++ b/diff.c
@@ -583,11 +583,10 @@ static void emit_rewrite_diff(const char *name_a,
 			      struct diff_options *o)
 {
 	int lc_a, lc_b;
-	int color_diff = DIFF_OPT_TST(o, COLOR_DIFF);
 	const char *name_a_tab, *name_b_tab;
-	const char *metainfo = diff_get_color(color_diff, DIFF_METAINFO);
-	const char *fraginfo = diff_get_color(color_diff, DIFF_FRAGINFO);
-	const char *reset = diff_get_color(color_diff, DIFF_RESET);
+	const char *metainfo = diff_get_color(o->use_color, DIFF_METAINFO);
+	const char *fraginfo = diff_get_color(o->use_color, DIFF_FRAGINFO);
+	const char *reset = diff_get_color(o->use_color, DIFF_RESET);
 	static struct strbuf a_name = STRBUF_INIT, b_name = STRBUF_INIT;
 	const char *a_prefix, *b_prefix;
 	char *data_one, *data_two;
@@ -623,7 +622,7 @@ static void emit_rewrite_diff(const char *name_a,
 	size_two = fill_textconv(textconv_two, two, &data_two);
 
 	memset(&ecbdata, 0, sizeof(ecbdata));
-	ecbdata.color_diff = color_diff;
+	ecbdata.color_diff = o->use_color > 0;
 	ecbdata.found_changesp = &o->found_changes;
 	ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);
 	ecbdata.opt = o;
@@ -1004,7 +1003,7 @@ static void free_diff_words_data(struct emit_callback *ecbdata)
 
 const char *diff_get_color(int diff_use_color, enum color_diff ix)
 {
-	if (diff_use_color)
+	if (diff_use_color > 0)
 		return diff_colors[ix];
 	return "";
 }
@@ -1808,11 +1807,10 @@ static int is_conflict_marker(const char *line, int marker_size, unsigned long l
 static void checkdiff_consume(void *priv, char *line, unsigned long len)
 {
 	struct checkdiff_t *data = priv;
-	int color_diff = DIFF_OPT_TST(data->o, COLOR_DIFF);
 	int marker_size = data->conflict_marker_size;
-	const char *ws = diff_get_color(color_diff, DIFF_WHITESPACE);
-	const char *reset = diff_get_color(color_diff, DIFF_RESET);
-	const char *set = diff_get_color(color_diff, DIFF_FILE_NEW);
+	const char *ws = diff_get_color(data->o->use_color, DIFF_WHITESPACE);
+	const char *reset = diff_get_color(data->o->use_color, DIFF_RESET);
+	const char *set = diff_get_color(data->o->use_color, DIFF_FILE_NEW);
 	char *err;
 	char *line_prefix = "";
 	struct strbuf *msgbuf;
@@ -2157,7 +2155,7 @@ static void builtin_diff(const char *name_a,
 		memset(&xecfg, 0, sizeof(xecfg));
 		memset(&ecbdata, 0, sizeof(ecbdata));
 		ecbdata.label_path = lbl;
-		ecbdata.color_diff = DIFF_OPT_TST(o, COLOR_DIFF);
+		ecbdata.color_diff = o->use_color > 0;
 		ecbdata.found_changesp = &o->found_changes;
 		ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);
 		if (ecbdata.ws_rule & WS_BLANK_AT_EOF)
@@ -2205,7 +2203,7 @@ static void builtin_diff(const char *name_a,
 					break;
 				}
 			}
-			if (DIFF_OPT_TST(o, COLOR_DIFF)) {
+			if (o->use_color > 0) {
 				struct diff_words_style *st = ecbdata.diff_words->style;
 				st->old.color = diff_get_color_opt(o, DIFF_FILE_OLD);
 				st->new.color = diff_get_color_opt(o, DIFF_FILE_NEW);
@@ -2855,7 +2853,7 @@ static void run_diff_cmd(const char *pgm,
 		 */
 		fill_metainfo(msg, name, other, one, two, o, p,
 			      &must_show_header,
-			      DIFF_OPT_TST(o, COLOR_DIFF) && !pgm);
+			      o->use_color > 0 && !pgm);
 		xfrm_msg = msg->len ? msg->buf : NULL;
 	}
 
@@ -3021,8 +3019,7 @@ void diff_setup(struct diff_options *options)
 
 	options->change = diff_change;
 	options->add_remove = diff_addremove;
-	if (diff_use_color_default > 0)
-		DIFF_OPT_SET(options, COLOR_DIFF);
+	options->use_color = diff_use_color_default;
 	options->detect_rename = diff_detect_rename_default;
 
 	if (diff_no_prefix) {
@@ -3410,24 +3407,24 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	else if (!strcmp(arg, "--follow"))
 		DIFF_OPT_SET(options, FOLLOW_RENAMES);
 	else if (!strcmp(arg, "--color"))
-		DIFF_OPT_SET(options, COLOR_DIFF);
+		options->use_color = 1;
 	else if (!prefixcmp(arg, "--color=")) {
 		int value = git_config_colorbool(NULL, arg+8, -1);
 		if (value == 0)
-			DIFF_OPT_CLR(options, COLOR_DIFF);
+			options->use_color = 0;
 		else if (value > 0)
-			DIFF_OPT_SET(options, COLOR_DIFF);
+			options->use_color = 1;
 		else
 			return error("option `color' expects \"always\", \"auto\", or \"never\"");
 	}
 	else if (!strcmp(arg, "--no-color"))
-		DIFF_OPT_CLR(options, COLOR_DIFF);
+		options->use_color = 0;
 	else if (!strcmp(arg, "--color-words")) {
-		DIFF_OPT_SET(options, COLOR_DIFF);
+		options->use_color = 1;
 		options->word_diff = DIFF_WORDS_COLOR;
 	}
 	else if (!prefixcmp(arg, "--color-words=")) {
-		DIFF_OPT_SET(options, COLOR_DIFF);
+		options->use_color = 1;
 		options->word_diff = DIFF_WORDS_COLOR;
 		options->word_regex = arg + 14;
 	}
@@ -3440,7 +3437,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		if (!strcmp(type, "plain"))
 			options->word_diff = DIFF_WORDS_PLAIN;
 		else if (!strcmp(type, "color")) {
-			DIFF_OPT_SET(options, COLOR_DIFF);
+			options->use_color = 1;
 			options->word_diff = DIFF_WORDS_COLOR;
 		}
 		else if (!strcmp(type, "porcelain"))
diff --git a/diff.h b/diff.h
index b920a20..8c66b59 100644
--- a/diff.h
+++ b/diff.h
@@ -58,7 +58,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 #define DIFF_OPT_SILENT_ON_REMOVE    (1 <<  5)
 #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)
 #define DIFF_OPT_FOLLOW_RENAMES      (1 <<  7)
-#define DIFF_OPT_COLOR_DIFF          (1 <<  8)
+/* (1 <<  8) unused */
 /* (1 <<  9) unused */
 #define DIFF_OPT_HAS_CHANGES         (1 << 10)
 #define DIFF_OPT_QUICK               (1 << 11)
@@ -101,6 +101,7 @@ struct diff_options {
 	const char *single_follow;
 	const char *a_prefix, *b_prefix;
 	unsigned flags;
+	int use_color;
 	int context;
 	int interhunkcontext;
 	int break_opt;
@@ -160,7 +161,7 @@ enum color_diff {
 };
 const char *diff_get_color(int diff_use_color, enum color_diff ix);
 #define diff_get_color_opt(o, ix) \
-	diff_get_color(DIFF_OPT_TST((o), COLOR_DIFF), ix)
+	diff_get_color((o)->use_color, ix)
 
 
 extern const char mime_boundary_leader[];
diff --git a/graph.c b/graph.c
index 2f6893d..556834a 100644
--- a/graph.c
+++ b/graph.c
@@ -347,7 +347,7 @@ static struct commit_list *first_interesting_parent(struct git_graph *graph)
 
 static unsigned short graph_get_current_column_color(const struct git_graph *graph)
 {
-	if (!DIFF_OPT_TST(&graph->revs->diffopt, COLOR_DIFF))
+	if (graph->revs->diffopt.use_color <= 0)
 		return column_colors_max;
 	return graph->default_column_color;
 }
diff --git a/log-tree.c b/log-tree.c
index e945701..9ba8fb2 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -31,7 +31,7 @@ static char decoration_colors[][COLOR_MAXLEN] = {
 
 static const char *decorate_get_color(int decorate_use_color, enum decoration_type ix)
 {
-	if (decorate_use_color)
+	if (decorate_use_color > 0)
 		return decoration_colors[ix];
 	return "";
 }
@@ -77,7 +77,7 @@ int parse_decorate_color_config(const char *var, const int ofs, const char *valu
  * for showing the commit sha1, use the same check for --decorate
  */
 #define decorate_get_color_opt(o, ix) \
-	decorate_get_color(DIFF_OPT_TST((o), COLOR_DIFF), ix)
+	decorate_get_color((o)->use_color, ix)
 
 static void add_name_decoration(enum decoration_type type, const char *name, struct object *obj)
 {
diff --git a/wt-status.c b/wt-status.c
index 0237772..ee03431 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -681,7 +681,7 @@ static void wt_status_print_verbose(struct wt_status *s)
 	 * will have checked isatty on stdout).
 	 */
 	if (s->fp != stdout)
-		DIFF_OPT_CLR(&rev.diffopt, COLOR_DIFF);
+		rev.diffopt.use_color = 0;
 	run_diff_index(&rev, 1);
 }
 
-- 
1.7.6.10.g62f04
Jeff King· Aug 18, 2011, 05:03 UTC · re: Jeff King · lore

[PATCH 06/10] git_config_colorbool: refactor stdout_is_tty handling

Usually this function figures out for itself whether stdout is a tty. However, it has an extra parameter just to allow git-config to override the auto-detection for its --get-colorbool option.

Instead of an extra parameter, let's just use a global variable. This makes calling easier in the common case, and will make refactoring the colorbool code much simpler.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c      |    2 +-
 builtin/commit.c      |    2 +-
 builtin/config.c      |   23 +++++++----------------
 builtin/grep.c        |    2 +-
 builtin/show-branch.c |    2 +-
 color.c               |   11 ++++++-----
 color.h               |    8 +++++++-
 diff.c                |    4 ++--
 parse-options.c       |    2 +-
 9 files changed, 27 insertions(+), 29 deletions(-)
Show changes to 9 files +27 −29

builtin/branch.c, builtin/commit.c, builtin/config.c, builtin/grep.c, builtin/show-branch.c, color.c, color.h, diff.c, parse-options.c

diff --git a/builtin/branch.c b/builtin/branch.c
index 3142daa..b15fee5 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -71,7 +71,7 @@ static int parse_branch_color_slot(const char *var, int ofs)
 static int git_branch_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "color.branch")) {
-		branch_use_color = git_config_colorbool(var, value, -1);
+		branch_use_color = git_config_colorbool(var, value);
 		return 0;
 	}
 	if (!prefixcmp(var, "color.branch.")) {
diff --git a/builtin/commit.c b/builtin/commit.c
index e1af9b1..295803a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1144,7 +1144,7 @@ static int git_status_config(const char *k, const char *v, void *cb)
 		return 0;
 	}
 	if (!strcmp(k, "status.color") || !strcmp(k, "color.status")) {
-		s->use_color = git_config_colorbool(k, v, -1);
+		s->use_color = git_config_colorbool(k, v);
 		return 0;
 	}
 	if (!prefixcmp(k, "status.color.") || !prefixcmp(k, "color.status.")) {
diff --git a/builtin/config.c b/builtin/config.c
index 211e118..5505ced 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -303,24 +303,17 @@ static void get_color(const char *def_color)
 	fputs(parsed_color, stdout);
 }
 
-static int stdout_is_tty;
 static int get_colorbool_found;
 static int get_diff_color_found;
 static int git_get_colorbool_config(const char *var, const char *value,
 		void *cb)
 {
-	if (!strcmp(var, get_colorbool_slot)) {
-		get_colorbool_found =
-			git_config_colorbool(var, value, stdout_is_tty);
-	}
-	if (!strcmp(var, "diff.color")) {
-		get_diff_color_found =
-			git_config_colorbool(var, value, stdout_is_tty);
-	}
-	if (!strcmp(var, "color.ui")) {
-		git_use_color_default = git_config_colorbool(var, value, stdout_is_tty);
-		return 0;
-	}
+	if (!strcmp(var, get_colorbool_slot))
+		get_colorbool_found = git_config_colorbool(var, value);
+	else if (!strcmp(var, "diff.color"))
+		get_diff_color_found = git_config_colorbool(var, value);
+	else if (!strcmp(var, "color.ui"))
+		git_use_color_default = git_config_colorbool(var, value);
 	return 0;
 }
 
@@ -510,9 +503,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 	else if (actions == ACTION_GET_COLORBOOL) {
 		if (argc == 1)
-			stdout_is_tty = git_config_bool("command line", argv[0]);
-		else if (argc == 0)
-			stdout_is_tty = isatty(1);
+			color_stdout_is_tty = git_config_bool("command line", argv[0]);
 		return get_colorbool(argc != 0);
 	}
 
diff --git a/builtin/grep.c b/builtin/grep.c
index cccf8da..d80db22 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -325,7 +325,7 @@ static int grep_config(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp(var, "color.grep"))
-		opt->color = git_config_colorbool(var, value, -1);
+		opt->color = git_config_colorbool(var, value);
 	else if (!strcmp(var, "color.grep.context"))
 		color = opt->color_context;
 	else if (!strcmp(var, "color.grep.filename"))
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index facc63a..e6650b4 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -573,7 +573,7 @@ static int git_show_branch_config(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp(var, "color.showbranch")) {
-		showbranch_use_color = git_config_colorbool(var, value, -1);
+		showbranch_use_color = git_config_colorbool(var, value);
 		return 0;
 	}
 
diff --git a/color.c b/color.c
index 3db214c..67affa4 100644
--- a/color.c
+++ b/color.c
@@ -2,6 +2,7 @@
 #include "color.h"
 
 int git_use_color_default = 0;
+int color_stdout_is_tty = -1;
 
 /*
  * The list of available column colors.
@@ -157,7 +158,7 @@ bad:
 	die("bad color value '%.*s' for variable '%s'", value_len, value, var);
 }
 
-int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
+int git_config_colorbool(const char *var, const char *value)
 {
 	if (value) {
 		if (!strcasecmp(value, "never"))
@@ -177,9 +178,9 @@ int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
 
 	/* any normal truth value defaults to 'auto' */
  auto_color:
-	if (stdout_is_tty < 0)
-		stdout_is_tty = isatty(1);
-	if (stdout_is_tty || (pager_in_use() && pager_use_color)) {
+	if (color_stdout_is_tty < 0)
+		color_stdout_is_tty = isatty(1);
+	if (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {
 		char *term = getenv("TERM");
 		if (term && strcmp(term, "dumb"))
 			return 1;
@@ -190,7 +191,7 @@ int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
 int git_color_default_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "color.ui")) {
-		git_use_color_default = git_config_colorbool(var, value, -1);
+		git_use_color_default = git_config_colorbool(var, value);
 		return 0;
 	}
 
diff --git a/color.h b/color.h
index 68a926a..a190a25 100644
--- a/color.h
+++ b/color.h
@@ -58,11 +58,17 @@ extern const char *column_colors_ansi[];
 extern const int column_colors_ansi_max;
 
 /*
+ * Generally the color code will lazily figure this out itself, but
+ * this provides a mechanism for callers to override autodetection.
+ */
+extern int color_stdout_is_tty;
+
+/*
  * Use this instead of git_default_config if you need the value of color.ui.
  */
 int git_color_default_config(const char *var, const char *value, void *cb);
 
-int git_config_colorbool(const char *var, const char *value, int stdout_is_tty);
+int git_config_colorbool(const char *var, const char *value);
 void color_parse(const char *value, const char *var, char *dst);
 void color_parse_mem(const char *value, int len, const char *var, char *dst);
 __attribute__((format (printf, 3, 4)))
diff --git a/diff.c b/diff.c
index 2d86abe..2dfc359 100644
--- a/diff.c
+++ b/diff.c
@@ -137,7 +137,7 @@ static int git_config_rename(const char *var, const char *value)
 int git_diff_ui_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
-		diff_use_color_default = git_config_colorbool(var, value, -1);
+		diff_use_color_default = git_config_colorbool(var, value);
 		return 0;
 	}
 	if (!strcmp(var, "diff.renames")) {
@@ -3409,7 +3409,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	else if (!strcmp(arg, "--color"))
 		options->use_color = 1;
 	else if (!prefixcmp(arg, "--color=")) {
-		int value = git_config_colorbool(NULL, arg+8, -1);
+		int value = git_config_colorbool(NULL, arg+8);
 		if (value == 0)
 			options->use_color = 0;
 		else if (value > 0)
diff --git a/parse-options.c b/parse-options.c
index 879ea82..be4383e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -621,7 +621,7 @@ int parse_opt_color_flag_cb(const struct option *opt, const char *arg,
 
 	if (!arg)
 		arg = unset ? "never" : (const char *)opt->defval;
-	value = git_config_colorbool(NULL, arg, -1);
+	value = git_config_colorbool(NULL, arg);
 	if (value < 0)
 		return opterror(opt,
 			"expects \"always\", \"auto\", or \"never\"", 0);
-- 
1.7.6.10.g62f04
Jeff King· Aug 18, 2011, 05:04 UTC · re: Jeff King · lore

[PATCH 07/10] color: delay auto-color decision until point of use

When we read a color value either from a config file or from the command line, we use git_config_colorbool to convert it from the tristate always/never/auto into a single yes/no boolean value.

This has some timing implications with respect to starting a pager.

If we start (or decide not to start) the pager before checking the colorbool, everything is fine. Either isatty(1) will give us the right information, or we will properly check for pager_in_use().

However, if we decide to start a pager after we have checked the colorbool, things are not so simple. If stdout is a tty, then we will have already decided to use color. However, the user may also have configured color.pager not to use color with the pager. In this case, we need to actually turn off color. Unfortunately, the pager code has no idea which color variables were turned on (and there are many of them throughout the code, and they may even have been manipulated after the colorbool selection by something like "--color" on the command line).

This bug can be seen any time a pager is started after config and command line options are checked. This has affected "git diff" since 89d07f7 (diff: don't run pager if user asked for a diff style exit code, 2007-08-12). It has also affect the log family since 1fda91b (Fix 'git log' early pager startup error case, 2010-08-24).

This patch splits the notion of parsing a colorbool and actually checking the configuration. The "use_color" variables now have an additional possible value, GIT_COLOR_AUTO. Users of the variable should use the new "want_color()" wrapper, which will lazily determine and cache the auto-color decision.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c      |    2 +-
 builtin/config.c      |    2 ++
 builtin/show-branch.c |    4 ++--
 color.c               |   20 ++++++++++++++++++--
 color.h               |   11 +++++++++++
 diff.c                |   17 +++++++----------
 graph.c               |    2 +-
 grep.c                |    2 +-
 log-tree.c            |    2 +-
 t/t7006-pager.sh      |   12 ++++++++++++
 wt-status.c           |    4 +++-
 11 files changed, 59 insertions(+), 19 deletions(-)
Show changes to 11 files +59 −19

builtin/branch.c, builtin/config.c, builtin/show-branch.c, color.c, color.h, diff.c, graph.c, grep.c, log-tree.c, t/t7006-pager.sh, wt-status.c

diff --git a/builtin/branch.c b/builtin/branch.c
index b15fee5..d6d3c7d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -88,7 +88,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)
 
 static const char *branch_get_color(enum color_branch ix)
 {
-	if (branch_use_color > 0)
+	if (want_color(branch_use_color))
 		return branch_colors[ix];
 	return "";
 }
diff --git a/builtin/config.c b/builtin/config.c
index 5505ced..3a09296 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -330,6 +330,8 @@ static int get_colorbool(int print)
 			get_colorbool_found = git_use_color_default;
 	}
 
+	get_colorbool_found = want_color(get_colorbool_found);
+
 	if (print) {
 		printf("%s\n", get_colorbool_found ? "true" : "false");
 		return 0;
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index e6650b4..4b726fa 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -26,14 +26,14 @@ static const char **default_arg;
 
 static const char *get_color_code(int idx)
 {
-	if (showbranch_use_color)
+	if (want_color(showbranch_use_color))
 		return column_colors_ansi[idx % column_colors_ansi_max];
 	return "";
 }
 
 static const char *get_color_reset_code(void)
 {
-	if (showbranch_use_color)
+	if (want_color(showbranch_use_color))
 		return GIT_COLOR_RESET;
 	return "";
 }
diff --git a/color.c b/color.c
index 67affa4..8586417 100644
--- a/color.c
+++ b/color.c
@@ -166,7 +166,7 @@ int git_config_colorbool(const char *var, const char *value)
 		if (!strcasecmp(value, "always"))
 			return 1;
 		if (!strcasecmp(value, "auto"))
-			goto auto_color;
+			return GIT_COLOR_AUTO;
 	}
 
 	if (!var)
@@ -177,7 +177,11 @@ int git_config_colorbool(const char *var, const char *value)
 		return 0;
 
 	/* any normal truth value defaults to 'auto' */
- auto_color:
+	return GIT_COLOR_AUTO;
+}
+
+static int check_auto_color(void)
+{
 	if (color_stdout_is_tty < 0)
 		color_stdout_is_tty = isatty(1);
 	if (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {
@@ -188,6 +192,18 @@ int git_config_colorbool(const char *var, const char *value)
 	return 0;
 }
 
+int want_color(int var)
+{
+	static int want_auto = -1;
+
+	if (var == GIT_COLOR_AUTO) {
+		if (want_auto < 0)
+			want_auto = check_auto_color();
+		return want_auto;
+	}
+	return var > 0;
+}
+
 int git_color_default_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "color.ui")) {
diff --git a/color.h b/color.h
index a190a25..d715fd5 100644
--- a/color.h
+++ b/color.h
@@ -49,6 +49,16 @@ struct strbuf;
 #define GIT_COLOR_NIL "NIL"
 
 /*
+ * The first three are chosen to match common usage in the code, and what is
+ * returned from git_config_colorbool. The "auto" value can be returned from
+ * config_colorbool, and will be converted by want_color() into either 0 or 1.
+ */
+#define GIT_COLOR_UNKNOWN -1
+#define GIT_COLOR_ALWAYS 0
+#define GIT_COLOR_NEVER  1
+#define GIT_COLOR_AUTO   2
+
+/*
  * This variable stores the value of color.ui
  */
 extern int git_use_color_default;
@@ -69,6 +79,7 @@ extern int color_stdout_is_tty;
 int git_color_default_config(const char *var, const char *value, void *cb);
 
 int git_config_colorbool(const char *var, const char *value);
+int want_color(int var);
 void color_parse(const char *value, const char *var, char *dst);
 void color_parse_mem(const char *value, int len, const char *var, char *dst);
 __attribute__((format (printf, 3, 4)))
diff --git a/diff.c b/diff.c
index 2dfc359..29cecf1 100644
--- a/diff.c
+++ b/diff.c
@@ -622,7 +622,7 @@ static void emit_rewrite_diff(const char *name_a,
 	size_two = fill_textconv(textconv_two, two, &data_two);
 
 	memset(&ecbdata, 0, sizeof(ecbdata));
-	ecbdata.color_diff = o->use_color > 0;
+	ecbdata.color_diff = want_color(o->use_color);
 	ecbdata.found_changesp = &o->found_changes;
 	ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);
 	ecbdata.opt = o;
@@ -1003,7 +1003,7 @@ static void free_diff_words_data(struct emit_callback *ecbdata)
 
 const char *diff_get_color(int diff_use_color, enum color_diff ix)
 {
-	if (diff_use_color > 0)
+	if (want_color(diff_use_color))
 		return diff_colors[ix];
 	return "";
 }
@@ -2155,7 +2155,7 @@ static void builtin_diff(const char *name_a,
 		memset(&xecfg, 0, sizeof(xecfg));
 		memset(&ecbdata, 0, sizeof(ecbdata));
 		ecbdata.label_path = lbl;
-		ecbdata.color_diff = o->use_color > 0;
+		ecbdata.color_diff = want_color(o->use_color);
 		ecbdata.found_changesp = &o->found_changes;
 		ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);
 		if (ecbdata.ws_rule & WS_BLANK_AT_EOF)
@@ -2203,7 +2203,7 @@ static void builtin_diff(const char *name_a,
 					break;
 				}
 			}
-			if (o->use_color > 0) {
+			if (want_color(o->use_color)) {
 				struct diff_words_style *st = ecbdata.diff_words->style;
 				st->old.color = diff_get_color_opt(o, DIFF_FILE_OLD);
 				st->new.color = diff_get_color_opt(o, DIFF_FILE_NEW);
@@ -2853,7 +2853,7 @@ static void run_diff_cmd(const char *pgm,
 		 */
 		fill_metainfo(msg, name, other, one, two, o, p,
 			      &must_show_header,
-			      o->use_color > 0 && !pgm);
+			      want_color(o->use_color) && !pgm);
 		xfrm_msg = msg->len ? msg->buf : NULL;
 	}
 
@@ -3410,12 +3410,9 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->use_color = 1;
 	else if (!prefixcmp(arg, "--color=")) {
 		int value = git_config_colorbool(NULL, arg+8);
-		if (value == 0)
-			options->use_color = 0;
-		else if (value > 0)
-			options->use_color = 1;
-		else
+		if (value < 0)
 			return error("option `color' expects \"always\", \"auto\", or \"never\"");
+		options->use_color = value;
 	}
 	else if (!strcmp(arg, "--no-color"))
 		options->use_color = 0;
diff --git a/graph.c b/graph.c
index 556834a..7358416 100644
--- a/graph.c
+++ b/graph.c
@@ -347,7 +347,7 @@ static struct commit_list *first_interesting_parent(struct git_graph *graph)
 
 static unsigned short graph_get_current_column_color(const struct git_graph *graph)
 {
-	if (graph->revs->diffopt.use_color <= 0)
+	if (!want_color(graph->revs->diffopt.use_color))
 		return column_colors_max;
 	return graph->default_column_color;
 }
diff --git a/grep.c b/grep.c
index 04e9ba4..abf4288 100644
--- a/grep.c
+++ b/grep.c
@@ -430,7 +430,7 @@ static int word_char(char ch)
 static void output_color(struct grep_opt *opt, const void *data, size_t size,
 			 const char *color)
 {
-	if (opt->color && color && color[0]) {
+	if (want_color(opt->color) && color && color[0]) {
 		opt->output(opt, color, strlen(color));
 		opt->output(opt, data, size);
 		opt->output(opt, GIT_COLOR_RESET, strlen(GIT_COLOR_RESET));
diff --git a/log-tree.c b/log-tree.c
index 9ba8fb2..95d6d40 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -31,7 +31,7 @@ static char decoration_colors[][COLOR_MAXLEN] = {
 
 static const char *decorate_get_color(int decorate_use_color, enum decoration_type ix)
 {
-	if (decorate_use_color > 0)
+	if (want_color(decorate_use_color))
 		return decoration_colors[ix];
 	return "";
 }
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 4884e1b..4582336 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -167,6 +167,18 @@ test_expect_success TTY 'color when writing to a pager' '
 	colorful paginated.out
 '
 
+test_expect_success TTY 'colors are suppressed by color.pager' '
+	rm -f paginated.out &&
+	test_config color.ui auto &&
+	test_config color.pager false &&
+	(
+		TERM=vt100 &&
+		export TERM &&
+		test_terminal git log
+	) &&
+	! colorful paginated.out
+'
+
 test_expect_success 'color when writing to a file intended for a pager' '
 	rm -f colorful.log &&
 	test_config color.ui auto ||
diff --git a/wt-status.c b/wt-status.c
index ee03431..8836a52 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -26,7 +26,9 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {
 
 static const char *color(int slot, struct wt_status *s)
 {
-	const char *c = s->use_color > 0 ? s->color_palette[slot] : "";
+	const char *c = "";
+	if (want_color(s->use_color))
+		c = s->color_palette[slot];
 	if (slot == WT_STATUS_ONBRANCH && color_is_nil(c))
 		c = s->color_palette[WT_STATUS_HEADER];
 	return c;
-- 
1.7.6.10.g62f04
Junio C Hamano· Aug 18, 2011, 21:59 UTC · re: Jeff King · lore

Re: [PATCH 07/10] color: delay auto-color decision until point of use

Jeff King <peff@peff.net> writes:
Show 16 quoted lines
> diff --git a/color.h b/color.h
> index a190a25..d715fd5 100644
> --- a/color.h
> +++ b/color.h
> @@ -49,6 +49,16 @@ struct strbuf;
>  #define GIT_COLOR_NIL "NIL"
>  
>  /*
> + * The first three are chosen to match common usage in the code, and what is
> + * returned from git_config_colorbool. The "auto" value can be returned from
> + * config_colorbool, and will be converted by want_color() into either 0 or 1.
> + */
> +#define GIT_COLOR_UNKNOWN -1
> +#define GIT_COLOR_ALWAYS 0
> +#define GIT_COLOR_NEVER  1
> +#define GIT_COLOR_AUTO   2

The ALWAYS/NEVER somehow go against my intuition. Let me trace one codepath starting from git_branch_config().

    branch_use_color is set from git_config_colorbool("color.branch");
    -> given "never", git_config_colorbool() returns 0;
    branch_get_color() asks want_color(branch_use_color);
    -> want_color() returns if the given value is positive.

Because git_config_colorbool() does not use the above symbolic constants, everything goes well, but aren't these two swapped?

Jeff King· Aug 18, 2011, 22:28 UTC · re: Junio C Hamano · lore

Re: [PATCH 07/10] color: delay auto-color decision until point of use

On Thu, Aug 18, 2011 at 02:59:37PM -0700, Junio C Hamano wrote:
Show 29 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > diff --git a/color.h b/color.h
> > index a190a25..d715fd5 100644
> > --- a/color.h
> > +++ b/color.h
> > @@ -49,6 +49,16 @@ struct strbuf;
> >  #define GIT_COLOR_NIL "NIL"
> >  
> >  /*
> > + * The first three are chosen to match common usage in the code, and what is
> > + * returned from git_config_colorbool. The "auto" value can be returned from
> > + * config_colorbool, and will be converted by want_color() into either 0 or 1.
> > + */
> > +#define GIT_COLOR_UNKNOWN -1
> > +#define GIT_COLOR_ALWAYS 0
> > +#define GIT_COLOR_NEVER  1
> > +#define GIT_COLOR_AUTO   2
> 
> The ALWAYS/NEVER somehow go against my intuition. Let me trace one
> codepath starting from git_branch_config().
> 
>     branch_use_color is set from git_config_colorbool("color.branch");
>     -> given "never", git_config_colorbool() returns 0;
>     branch_get_color() asks want_color(branch_use_color);
>     -> want_color() returns if the given value is positive.
> 
> Because git_config_colorbool() does not use the above symbolic constants,
> everything goes well, but aren't these two swapped?

Oooops. Yes, they are completely swapped and I'm an idiot. But as you noticed, we don't actually _use_ them anywhere. I started on replacing every "0" with NEVER, every "1" with ALWAYS, and every "-1" with UNKNOWN. But it really bloated the patch, and didn't actually make the code any more readable.

The only symbolic constant that is really necessary is the AUTO one. I just felt odd randomly defining "2" as GIT_COLOR_AUTO, but not defining the other possible values of the enumeration. So definitely they should be swapped. I'm also fine with just dropping all of them except AUTO.

-Peff
Jeff King· Aug 18, 2011, 05:04 UTC · re: Jeff King · lore

[PATCH 08/10] config: refactor get_colorbool function

For "git config --get-colorbool color.foo", we use a custom callback that looks not only for the key that the user gave us, but also for "diff.color" (for backwards compatibility) and "color.ui" (as a fallback).

For the former, we use a custom variable to store the diff.color value. For the latter, though, we store it in the main "git_use_color_default" variable, turning on color.ui for any other parts of git that respect this value.

In practice, this doesn't cause any bugs, because git-config runs without caring about git_use_color_default, and then exits. But it crosses module boundaries in an unusual and confusing way, and it makes refactoring color handling harder than it needs to be.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/config.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
Show changes to builtin/config.c +3 −2
diff --git a/builtin/config.c b/builtin/config.c
index 3a09296..0b4ecac 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -305,6 +305,7 @@ static void get_color(const char *def_color)
 
 static int get_colorbool_found;
 static int get_diff_color_found;
+static int get_color_ui_found;
 static int git_get_colorbool_config(const char *var, const char *value,
 		void *cb)
 {
@@ -313,7 +314,7 @@ static int git_get_colorbool_config(const char *var, const char *value,
 	else if (!strcmp(var, "diff.color"))
 		get_diff_color_found = git_config_colorbool(var, value);
 	else if (!strcmp(var, "color.ui"))
-		git_use_color_default = git_config_colorbool(var, value);
+		get_color_ui_found = git_config_colorbool(var, value);
 	return 0;
 }
 
@@ -327,7 +328,7 @@ static int get_colorbool(int print)
 		if (!strcmp(get_colorbool_slot, "color.diff"))
 			get_colorbool_found = get_diff_color_found;
 		if (get_colorbool_found < 0)
-			get_colorbool_found = git_use_color_default;
+			get_colorbool_found = get_color_ui_found;
 	}
 
 	get_colorbool_found = want_color(get_colorbool_found);
-- 
1.7.6.10.g62f04
Jeff King· Aug 18, 2011, 05:05 UTC · re: Jeff King · lore

[PATCH 09/10] diff: don't load color config in plumbing

The diff config callback is split into two functions: one which loads "ui" config, and one which loads "basic" config. The former chains to the latter, as the diff UI config is a superset of the plumbing config.

The color.diff variable is only loaded in the UI config. However, the basic config actually chains to git_color_default_config, which loads color.ui. This doesn't actually cause any bugs, because the plumbing diff code does not actually look at the value of color.ui.

However, it is somewhat nonsensical, and it makes it difficult to refactor the color code. It probably came about because there is no git_color_config to load only color config, but rather just git_color_default_config, which loads color config and chains to git_default_config.

This patch splits out the color-specific portion of git_color_default_config so that the diff UI config can call it directly. This is perhaps better explained by the chaining of callbacks. Before we had:

  git_diff_ui_config
    -> git_diff_basic_config
      -> git_color_default_config
        -> git_default_config
Now we have:
  git_diff_ui_config
    -> git_color_config
    -> git_diff_basic_config
      -> git_default_config
Signed-off-by: Jeff King <peff@peff.net>
---
 color.c |   10 +++++++++-
 color.h |    4 +++-
 diff.c  |    5 ++++-
 3 files changed, 16 insertions(+), 3 deletions(-)
Show changes to 3 files +16 −3

color.c, color.h, diff.c

diff --git a/color.c b/color.c
index 8586417..ec96fe1 100644
--- a/color.c
+++ b/color.c
@@ -204,13 +204,21 @@ int want_color(int var)
 	return var > 0;
 }
 
-int git_color_default_config(const char *var, const char *value, void *cb)
+int git_color_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "color.ui")) {
 		git_use_color_default = git_config_colorbool(var, value);
 		return 0;
 	}
 
+	return 0;
+}
+
+int git_color_default_config(const char *var, const char *value, void *cb)
+{
+	if (git_color_config(var, value, cb) < 0)
+		return -1;
+
 	return git_default_config(var, value, cb);
 }
 
diff --git a/color.h b/color.h
index d715fd5..5949bcd 100644
--- a/color.h
+++ b/color.h
@@ -74,8 +74,10 @@ extern const int column_colors_ansi_max;
 extern int color_stdout_is_tty;
 
 /*
- * Use this instead of git_default_config if you need the value of color.ui.
+ * Use the first one if you need only color config; the second is a convenience
+ * if you are just going to change to git_default_config, too.
  */
+int git_color_config(const char *var, const char *value, void *cb);
 int git_color_default_config(const char *var, const char *value, void *cb);
 
 int git_config_colorbool(const char *var, const char *value);
diff --git a/diff.c b/diff.c
index 29cecf1..0a22320 100644
--- a/diff.c
+++ b/diff.c
@@ -164,6 +164,9 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "diff.ignoresubmodules"))
 		handle_ignore_submodules_arg(&default_diff_options, value);
 
+	if (git_color_config(var, value, cb) < 0)
+		return -1;
+
 	return git_diff_basic_config(var, value, cb);
 }
 
@@ -212,7 +215,7 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 	if (!prefixcmp(var, "submodule."))
 		return parse_submodule_config_option(var, value);
 
-	return git_color_default_config(var, value, cb);
+	return git_default_config(var, value, cb);
 }
 
 static char *quote_two(const char *one, const char *two)
-- 
1.7.6.10.g62f04
Jeff King· Aug 18, 2011, 05:05 UTC · re: Jeff King · lore

[PATCH 10/10] want_color: automatically fallback to color.ui

All of the "do we want color" flags default to -1 to indicate that we don't have any color configured. This value is handled in one of two ways:

  1. In porcelain, we check early on whether the value is
     still -1 after reading the config, and set it to the
     value of color.ui (which defaults to 0).
  2. In plumbing, it stays untouched as -1, and want_color
     defaults it to off.

This works fine, but means that every porcelain has to check and reassign its color flag. Now that want_color gives us a place to put this check in a single spot, we can do that, simplifying the calling code.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c      |    3 ---
 builtin/commit.c      |   11 +----------
 builtin/diff.c        |    3 ---
 builtin/grep.c        |    2 --
 builtin/log.c         |   12 ------------
 builtin/merge.c       |    4 ----
 builtin/show-branch.c |    3 ---
 color.c               |    7 +++++--
 color.h               |    5 -----
 9 files changed, 6 insertions(+), 44 deletions(-)
Show changes to 9 files +6 −44

builtin/branch.c, builtin/commit.c, builtin/diff.c, builtin/grep.c, builtin/log.c, builtin/merge.c, builtin/show-branch.c, color.c, color.h

diff --git a/builtin/branch.c b/builtin/branch.c
index d6d3c7d..73d4170 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -673,9 +673,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 	git_config(git_branch_config, NULL);
 
-	if (branch_use_color == -1)
-		branch_use_color = git_use_color_default;
-
 	track = git_branch_track;
 
 	head = resolve_ref("HEAD", head_sha1, 0, NULL);
diff --git a/builtin/commit.c b/builtin/commit.c
index 295803a..9763146 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1237,10 +1237,6 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 
 	if (s.relative_paths)
 		s.prefix = prefix;
-	if (s.use_color == -1)
-		s.use_color = git_use_color_default;
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
 
 	switch (status_format) {
 	case STATUS_FORMAT_SHORT:
@@ -1394,15 +1390,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	git_config(git_commit_config, &s);
 	determine_whence(&s);
 
-	if (s.use_color == -1)
-		s.use_color = git_use_color_default;
 	argc = parse_and_validate_options(argc, argv, builtin_commit_usage,
 					  prefix, &s);
-	if (dry_run) {
-		if (diff_use_color_default == -1)
-			diff_use_color_default = git_use_color_default;
+	if (dry_run)
 		return dry_run_commit(argc, argv, prefix, &s);
-	}
 	index_file = prepare_index(argc, argv, prefix, 0);
 
 	/* Set up everything for writing the commit object.  This includes
diff --git a/builtin/diff.c b/builtin/diff.c
index 69cd5ee..1118689 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -277,9 +277,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	gitmodules_config();
 	git_config(git_diff_ui_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 
 	/* If this is a no-index diff, just run it and exit there. */
diff --git a/builtin/grep.c b/builtin/grep.c
index d80db22..2cbf01f 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -896,8 +896,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	strcpy(opt.color_sep, GIT_COLOR_CYAN);
 	opt.color = -1;
 	git_config(grep_config, &opt);
-	if (opt.color == -1)
-		opt.color = git_use_color_default;
 
 	/*
 	 * If there is no -- then the paths must exist in the working
diff --git a/builtin/log.c b/builtin/log.c
index 5c2af59..d760ee0 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -359,9 +359,6 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
 
 	git_config(git_log_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
 	rev.simplify_history = 0;
@@ -446,9 +443,6 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 
 	git_config(git_log_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_pathspec(&match_all, NULL);
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
@@ -524,9 +518,6 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
 
 	git_config(git_log_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 	init_reflog_walk(&rev.reflog_info);
 	rev.verbose_header = 1;
@@ -549,9 +540,6 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 
 	git_config(git_log_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 	rev.always_show_header = 1;
 	memset(&opt, 0, sizeof(opt));
diff --git a/builtin/merge.c b/builtin/merge.c
index 7209edf..b75ae01 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1031,10 +1031,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 	git_config(git_merge_config, NULL);
 
-	/* for color.ui */
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	if (branch_mergeoptions)
 		parse_branch_merge_options(branch_mergeoptions);
 	argc = parse_options(argc, argv, prefix, builtin_merge_options,
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 4b726fa..4b480d7 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -685,9 +685,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 
 	git_config(git_show_branch_config, NULL);
 
-	if (showbranch_use_color == -1)
-		showbranch_use_color = git_use_color_default;
-
 	/* If nothing is specified, try the default first */
 	if (ac == 1 && default_num) {
 		ac = default_num;
diff --git a/color.c b/color.c
index ec96fe1..e8e2681 100644
--- a/color.c
+++ b/color.c
@@ -1,7 +1,7 @@
 #include "cache.h"
 #include "color.h"
 
-int git_use_color_default = 0;
+static int git_use_color_default = 0;
 int color_stdout_is_tty = -1;
 
 /*
@@ -196,12 +196,15 @@ int want_color(int var)
 {
 	static int want_auto = -1;
 
+	if (var < 0)
+		var = git_use_color_default;
+
 	if (var == GIT_COLOR_AUTO) {
 		if (want_auto < 0)
 			want_auto = check_auto_color();
 		return want_auto;
 	}
-	return var > 0;
+	return var;
 }
 
 int git_color_config(const char *var, const char *value, void *cb)
diff --git a/color.h b/color.h
index 5949bcd..3068a99 100644
--- a/color.h
+++ b/color.h
@@ -58,11 +58,6 @@ struct strbuf;
 #define GIT_COLOR_NEVER  1
 #define GIT_COLOR_AUTO   2
 
-/*
- * This variable stores the value of color.ui
- */
-extern int git_use_color_default;
-
 /* A default list of colors to use for commit graphs and show-branch output */
 extern const char *column_colors_ansi[];
 extern const int column_colors_ansi_max;
-- 
1.7.6.10.g62f04
Martin von Zweigbergk· Sep 4, 2011, 02:36 UTC · re: Jeff King · lore

Re: [PATCH 10/10] want_color: automatically fallback to color.ui

Hi Jeff,

This patch makes format-patch output color escape codes to file when run with color.ui=always. Before the patch it did not do that. The documentation for color.ui says "Set it to always if you want all output not intended for machine consumption to use color". Is format-patch "intended for machine consumption" or not?

I'm not sure why I had the parameter set to "always" instead of "true/auto" and maybe I should change it, but since this patch changes the behavior, I thought I should let you know (it was not mentioned in the commit message, so I'm not sure it was intentional).

Martin
Jeff King· Sep 4, 2011, 12:53 UTC · re: Martin von Zweigbergk · lore

Re: [PATCH 10/10] want_color: automatically fallback to color.ui

On Sat, Sep 03, 2011 at 10:36:01PM -0400, Martin von Zweigbergk wrote:
Show 5 quoted lines
> This patch makes format-patch output color escape codes to file when
> run with color.ui=always. Before the patch it did not do that. The
> documentation for color.ui says "Set it to always if you want all
> output not intended for machine consumption to use color". Is
> format-patch "intended for machine consumption" or not?

Sorry, this is a regression. The old behavior was that commands had to copy color.ui's value manually into diff_use_color_default. With my patch, the value is picked up automatically, and it is up to commands to load or not load the specified config.

For diff plumbing versus porcelain, we have separate config code paths (diff_basic versus diff_ui). I forgot that format-patch versus log has the same situation, and needs the same split.

It's a long weekend here in the US, but I'll try to get a patch out on Tuesday (and also check for any other similar regressions).

> I'm not sure why I had the parameter set to "always" instead of
> "true/auto" and maybe I should change it, but since this patch changes
> the behavior, I thought I should let you know (it was not mentioned in
> the commit message, so I'm not sure it was intentional).

Yeah, I don't think setting color.ui to "always" is all that useful. But this behavior change was definitely not intentional. Thanks for noticing.

-Peff
Steffen Daode Nurpmeso· Sep 5, 2011, 11:31 UTC · re: Jeff King · lore

Re: [PATCH 10/10] want_color: automatically fallback to color.ui

@ Jeff King <peff@peff.net> wrote (2011-09-04 14:53+0200):
> Sorry, this is a regression.

I should have found that. I apologize.

--Steffen
Ciao, sdaoden(*)(gmail.com)
ASCII ribbon campaign           ( ) More nuclear fission plants
  against HTML e-mail            X    can serve more coloured
    and proprietary attachments / \     and sounding animations
Jeff King· Aug 18, 2011, 21:58 UTC · re: Jeff King · lore

Re: [PATCH 0/10] color and pager improvements

On Wed, Aug 17, 2011 at 09:58:23PM -0700, Jeff King wrote:
> While looking at the pager and color code today, I decided to tackle two
> long-standing bugs, which entailed a lot of refactoring of the color
> code. The result fixes some minor bugs, and is a little nicer for
> calling code to use.

And here are two patches on top of the previous 10 to help Ingo's problem a bit.

  [11/10]: support pager.* for aliases
  [12/10]: support pager.* for external commands

With these, you can do "git config pager.stash false" to turn off paging on "stash list" and "stash show" (and turn it back on with "git -p stash list", of course).

I'm slightly tempted to allow things like "pager.stash.list" and "pager.branch.list". It wouldn't be too hard to implement. But I'm not sure anybody actually cares. I think Ingo's original complaint was simply that pager.stash didn't actually do anything, not that he wanted some separate config for the various subcommands.

-Peff
Jeff King· Aug 18, 2011, 21:59 UTC · re: Jeff King · lore

[PATCH 11/10] support pager.* for aliases

Until this patch, doing something like:
  git config alias.foo log
  git config pager.foo /some/specific/pager

would not respect pager.foo at all. With this patch, we will use pager.foo for the "foo" alias. We will also fallback to pager.log if "foo" is a non-shell alias that uses the "log" command (but any pager.foo overrides pager.log).

Signed-off-by: Jeff King <peff@peff.net>
---
 git.c            |    3 +++
 t/t7006-pager.sh |   31 +++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 0 deletions(-)
Show changes to 2 files +34 −0

git.c, t/t7006-pager.sh

diff --git a/git.c b/git.c
index 8828c18..375e9b2 100644
--- a/git.c
+++ b/git.c
@@ -180,6 +180,9 @@ static int handle_alias(int *argcp, const char ***argv)
 	alias_command = (*argv)[0];
 	alias_string = alias_lookup(alias_command);
 	if (alias_string) {
+		if (use_pager == -1)
+			use_pager = check_pager_config(alias_command);
+
 		if (alias_string[0] == '!') {
 			const char **alias_argv;
 			int argc = *argcp, i;
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 4582336..a8c6e85 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -450,4 +450,35 @@ test_expect_success TTY 'command-specific pager overridden by environment' '
 	test_cmp expect actual
 '
 
+test_expect_success TTY 'command-specific pager works for aliases' '
+	sane_unset PAGER GIT_PAGER &&
+	echo "foo:initial" >expect &&
+	>actual &&
+	test_config alias.aliaslog "log --format=%s" &&
+	test_config pager.aliaslog "sed s/^/foo:/ >actual" &&
+	test_terminal git aliaslog -1 &&
+	test_cmp expect actual
+'
+
+test_expect_success TTY 'non-shell alias falls back to command pager config' '
+	sane_unset PAGER GIT_PAGER &&
+	echo "foo:initial" >expect &&
+	>actual &&
+	test_config alias.aliaslog "log --format=%s" &&
+	test_config pager.log "sed s/^/foo:/ >actual" &&
+	test_terminal git aliaslog -1 &&
+	test_cmp expect actual
+'
+
+test_expect_success TTY 'alias-specific pager can override aliased command' '
+	sane_unset PAGER GIT_PAGER &&
+	>expect &&
+	>actual &&
+	test_config alias.aliaslog "log --format=%s" &&
+	test_config pager.log "sed s/^/log:/ >actual" &&
+	test_config pager.aliaslog false &&
+	test_terminal git aliaslog -1 &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.6.10.g62f04
Junio C Hamano· Aug 18, 2011, 22:54 UTC · re: Jeff King · lore

Re: [PATCH 11/10] support pager.* for aliases

Jeff King <peff@peff.net> writes:
Show 6 quoted lines
> Until this patch, doing something like:
>
>   git config alias.foo log
>   git config pager.foo /some/specific/pager
>
> would not respect pager.foo at all.

Is it a good thing? Looks too confusing and I am having a hard time to decide if this is "just because we could" or "because we need to be able to do this for such and such reasons".

Jeff King· Aug 19, 2011, 03:37 UTC · re: Junio C Hamano · lore

Re: [PATCH 11/10] support pager.* for aliases

On Thu, Aug 18, 2011 at 03:54:46PM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > Until this patch, doing something like:
> >
> >   git config alias.foo log
> >   git config pager.foo /some/specific/pager
> >
> > would not respect pager.foo at all.
> 
> Is it a good thing? Looks too confusing and I am having a hard time to
> decide if this is "just because we could" or "because we need to be able
> to do this for such and such reasons".

I don't have a particular use for it myself. However, I don't see what's confusing about it. Would would you expect the above commands to do with respect to paging? I think the behavior after my patch does what users will expect, whether they have configured pager.foo, pager.log, or nothing.

-Peff
Junio C Hamano· Aug 19, 2011, 04:18 UTC · re: Jeff King · lore

Re: [PATCH 11/10] support pager.* for aliases

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
>> > Until this patch, doing something like:
>> >
>> >   git config alias.foo log
>> >   git config pager.foo /some/specific/pager
>> >
>> > would not respect pager.foo at all.
>> 
>> Is it a good thing? Looks too confusing and I am having a hard time to
>> decide if this is "just because we could" or "because we need to be able
>> to do this for such and such reasons".
>
> I don't have a particular use for it myself. However, I don't see what's
> confusing about it. Would would you expect the above commands to do with
> respect to paging?

The reason I found it confusing was that I expected the "log" command that is run as the expansion of the alias to be oblivious to the fact that the end user called it "foo", and ignore anything specific to "foo", including "pager.foo".

Jeff King· Aug 19, 2011, 04:40 UTC · re: Junio C Hamano · lore

Re: [PATCH 11/10] support pager.* for aliases

On Thu, Aug 18, 2011 at 09:18:49PM -0700, Junio C Hamano wrote:
Show 21 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> >> > Until this patch, doing something like:
> >> >
> >> >   git config alias.foo log
> >> >   git config pager.foo /some/specific/pager
> >> >
> >> > would not respect pager.foo at all.
> >> 
> >> Is it a good thing? Looks too confusing and I am having a hard time to
> >> decide if this is "just because we could" or "because we need to be able
> >> to do this for such and such reasons".
> >
> > I don't have a particular use for it myself. However, I don't see what's
> > confusing about it. Would would you expect the above commands to do with
> > respect to paging?
> 
> The reason I found it confusing was that I expected the "log" command that
> is run as the expansion of the alias to be oblivious to the fact that the
> end user called it "foo", and ignore anything specific to "foo", including
> "pager.foo".
I think of it this way:

If the user thinks of the alias as just another form of "log", then we do the right thing: we use log's pager config by default, and respect pager.log. They never set pager.foo, because that is nonsensical in their mental model.

If the user thinks of the alias as its own command, then they would expect pager.foo to work. And it does what they expect.

But like I said, I don't personally plan on using this. It was just the only semantics that really made sense to me, and I noticed it because of working on externals. And clearly it's not a lot of code.

-Peff
Junio C Hamano· Aug 19, 2011, 05:23 UTC · re: Jeff King · lore

Re: [PATCH 11/10] support pager.* for aliases

Jeff King <peff@peff.net> writes:
Show 36 quoted lines
> On Thu, Aug 18, 2011 at 09:18:49PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> >> > Until this patch, doing something like:
>> >> >
>> >> >   git config alias.foo log
>> >> >   git config pager.foo /some/specific/pager
>> >> >
>> >> > would not respect pager.foo at all.
>> >> 
>> >> Is it a good thing? Looks too confusing and I am having a hard time to
>> >> decide if this is "just because we could" or "because we need to be able
>> >> to do this for such and such reasons".
>> >
>> > I don't have a particular use for it myself. However, I don't see what's
>> > confusing about it. Would would you expect the above commands to do with
>> > respect to paging?
>> 
>> The reason I found it confusing was that I expected the "log" command that
>> is run as the expansion of the alias to be oblivious to the fact that the
>> end user called it "foo", and ignore anything specific to "foo", including
>> "pager.foo".
>
> I think of it this way:
>
> If the user thinks of the alias as just another form of "log", then we
> do the right thing: we use log's pager config by default, and respect
> pager.log. They never set pager.foo, because that is nonsensical in
> their mental model.
>
> If the user thinks of the alias as its own command, then they would
> expect pager.foo to work. And it does what they expect.
>
> But like I said, I don't personally plan on using this. It was just the
> only semantics that really made sense to me,...

I can see that argument, but once you start paying attention to "*.foo", you have to keep supporting that forever, and also more importantly, you need to worry about interactions between "*.foo" vs "*.log". Which one should win? Should they combine if both are defined? My "looks confusing" includes that can of worms.

Junio C Hamano· Aug 19, 2011, 05:43 UTC · re: Junio C Hamano · lore

Re: [PATCH 11/10] support pager.* for aliases

Junio C Hamano <gitster@pobox.com> writes:
Show 16 quoted lines
>> If the user thinks of the alias as just another form of "log", then we
>> do the right thing: we use log's pager config by default, and respect
>> pager.log. They never set pager.foo, because that is nonsensical in
>> their mental model.
>>
>> If the user thinks of the alias as its own command, then they would
>> expect pager.foo to work. And it does what they expect.
>>
>> But like I said, I don't personally plan on using this. It was just the
>> only semantics that really made sense to me,...
>
> I can see that argument, but once you start paying attention to "*.foo",
> you have to keep supporting that forever, and also more importantly, you
> need to worry about interactions between "*.foo" vs "*.log". Which one
> should win? Should they combine if both are defined? My "looks confusing"
> includes that can of worms.

Actually there is another thing that I think is much worse. If the user is trained to think of the alias as its own command by seeing pager.foo to work as you described, you cannot blame them if they would also expect these to work, in the sense that only "foo.*" and "bar.*" respectively would take effect, and they would override "log.*":

	[alias]
        	foo = log
		bar = !sh -c 'log "$@"' -
	[log]
		date = default
	[foo]
        	date = iso
	[bar]
		date = relative
I do not think we want to go there.
Jeff King· Aug 19, 2011, 08:30 UTC · re: Junio C Hamano · lore

Re: [PATCH 11/10] support pager.* for aliases

On Thu, Aug 18, 2011 at 10:23:33PM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> > I think of it this way:
> >
> > If the user thinks of the alias as just another form of "log", then we
> > do the right thing: we use log's pager config by default, and respect
> > pager.log. They never set pager.foo, because that is nonsensical in
> > their mental model.
> >
> > If the user thinks of the alias as its own command, then they would
> > expect pager.foo to work. And it does what they expect.
> >
> > But like I said, I don't personally plan on using this. It was just the
> > only semantics that really made sense to me,...
> 
> I can see that argument, but once you start paying attention to "*.foo",
> you have to keep supporting that forever, and also more importantly, you
> need to worry about interactions between "*.foo" vs "*.log". Which one
> should win? Should they combine if both are defined? My "looks confusing"
> includes that can of worms.

It seems obvious to me that the more-specific *.foo form would take precedence over the *.log form, and anything else would be crazy. But that is just my gut feeling. If you are confused or worried, then that is enough for me to say it is not worth pursuing. It's not a feature I really care about; it was more about fixing something that looked obviously wrong while I was in the area.

-Peff
Jeff King· Aug 18, 2011, 22:01 UTC · re: Jeff King · lore

[PATCH 12/10] support pager.* for external commands

Without this patch, any commands that are not builtin would not respect pager.* config. For example:

  git config pager.stash false
  git stash list

would still use a pager. With this patch, pager.stash now has an effect. If it is not specified, we will still fall back to pager.log when we invoke "log" from "stash list".

Signed-off-by: Jeff King <peff@peff.net>
---
I think we didn't do this in the original pager.* patches because of
initialization order problems. It was dangerous to look at config too
early in the process, or something similar; I don't recall the exact
problems. But since work from Jonathan and Duy last summer, I think some
of those issues have gone away. At least I couldn't find any problems.
And I have been running with this patch since last November and haven't
noticed anything odd.
 git.c            |    2 ++
 t/t7006-pager.sh |   36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 0 deletions(-)
Show changes to 2 files +38 −0

git.c, t/t7006-pager.sh

diff --git a/git.c b/git.c
index 375e9b2..e8cff60 100644
--- a/git.c
+++ b/git.c
@@ -462,6 +462,8 @@ static void execv_dashed_external(const char **argv)
 	const char *tmp;
 	int status;
 
+	if (use_pager == -1)
+		use_pager = check_pager_config(argv[0]);
 	commit_pager_choice();
 
 	strbuf_addf(&cmd, "git-%s", argv[0]);
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index a8c6e85..742238c 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -481,4 +481,40 @@ test_expect_success TTY 'alias-specific pager can override aliased command' '
 	test_cmp expect actual
 '
 
+test_expect_success 'setup external command' '
+	cat >git-external <<-\EOF &&
+	#!/bin/sh
+	git "$@"
+	EOF
+	chmod +x git-external
+'
+
+test_expect_success TTY 'command-specific pager works for external commands' '
+	sane_unset PAGER GIT_PAGER &&
+	echo "foo:initial" >expect &&
+	>actual &&
+	test_config pager.external "sed s/^/foo:/ >actual" &&
+	test_terminal git --exec-path="`pwd`" external log --format=%s -1 &&
+	test_cmp expect actual
+'
+
+test_expect_success TTY 'sub-commands of externals use their own pager' '
+	sane_unset PAGER GIT_PAGER &&
+	echo "foo:initial" >expect &&
+	>actual &&
+	test_config pager.log "sed s/^/foo:/ >actual" &&
+	test_terminal git --exec-path=. external log --format=%s -1 &&
+	test_cmp expect actual
+'
+
+test_expect_success TTY 'external command pagers override sub-commands' '
+	sane_unset PAGER GIT_PAGER &&
+	>expect &&
+	>actual &&
+	test_config pager.external false &&
+	test_config pager.log "sed s/^/log:/ >actual" &&
+	test_terminal git --exec-path=. external log --format=%s -1 &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.6.10.g62f04
Junio C Hamano· Aug 18, 2011, 22:56 UTC · re: Jeff King · lore

Re: [PATCH 12/10] support pager.* for external commands

Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> Without this patch, any commands that are not builtin would
> not respect pager.* config. For example:
>
>   git config pager.stash false
>   git stash list
>
> would still use a pager.

Unlike the [11/10] patch, I can see why this is a good change. Thanks.

Ævar Arnfjörð Bjarmason· Feb 12, 2012, 00:46 UTC · re: Jeff King · lore

Re: [PATCH 12/10] support pager.* for external commands

On Fri, Aug 19, 2011 at 00:01, Jeff King <peff@peff.net> wrote:
Show 7 quoted lines
> +test_expect_success TTY 'command-specific pager works for external commands' '
> +       sane_unset PAGER GIT_PAGER &&
> +       echo "foo:initial" >expect &&
> +       >actual &&
> +       test_config pager.external "sed s/^/foo:/ >actual" &&
> +       test_terminal git --exec-path="`pwd`" external log --format=%s -1 &&
> +       test_cmp expect actual

For reasons that I haven't looked into using sed like that breaks under /usr/bin/ksh on Solaris. Just using:

    sed -e \"s/^/foo:/\"

Instead fixes it, it's not broken with /usr/xpg4/bin/sh, so it's some ksh peculiarity.

The error it gives is:
    sed s/^/foo:/ >actual: Not found

Indicating that for some reason it's considering that whole "sed s/^/foo:/ >actual" string to be a single command.

Jeff King· Feb 14, 2012, 19:13 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 12/10] support pager.* for external commands

On Sun, Feb 12, 2012 at 01:46:34AM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 24 quoted lines
> On Fri, Aug 19, 2011 at 00:01, Jeff King <peff@peff.net> wrote:
> 
> > +test_expect_success TTY 'command-specific pager works for external commands' '
> > +       sane_unset PAGER GIT_PAGER &&
> > +       echo "foo:initial" >expect &&
> > +       >actual &&
> > +       test_config pager.external "sed s/^/foo:/ >actual" &&
> > +       test_terminal git --exec-path="`pwd`" external log --format=%s -1 &&
> > +       test_cmp expect actual
> 
> For reasons that I haven't looked into using sed like that breaks
> under /usr/bin/ksh on Solaris. Just using:
> 
>     sed -e \"s/^/foo:/\"
> 
> Instead fixes it, it's not broken with /usr/xpg4/bin/sh, so it's some
> ksh peculiarity.
> 
> The error it gives is:
> 
>     sed s/^/foo:/ >actual: Not found
> 
> Indicating that for some reason it's considering that whole "sed
> s/^/foo:/ >actual" string to be a single command.

Hrm. Is the problem on the git-executing side, or is it on the setting up the config side?

Sadly (or perhaps not) I no longer have any Solaris machines to test on. Can you confirm that "git config pager.external" looks OK inside that test? Can you confirm via GIT_TRACE=1 what is being sent to the shell?

Also, it looks like we actually run commands internally from git using "sh -c". So if it is the executing side that is wrong, I don't see how /usr/bin/ksh would be involved at all (it would either be /bin/sh, or /usr/xpg4/bin/sh if you have your PATH set).

-Peff
Ingo Brückl· Aug 18, 2011, 22:33 UTC · re: Jeff King · lore

Re: [PATCH 0/10] color and pager improvements

Jeff King wrote on Thu, 18 Aug 2011 14:58:20 -0700:
> I think Ingo's original complaint was simply that pager.stash didn't
> actually do anything, not that he wanted some separate config for the
> various subcommands.
No, you're wrong.

My goal was to be able to turn off paging for "stash list" only while all other stash commands should continue paging.

It is, of course, very usefull to be able to control paging for external commands and aliases, but in my case I originally wanted to control a specific subcommand.

Ingo
Jeff King· Aug 18, 2011, 22:46 UTC · re: Ingo Brückl · lore

Re: [PATCH 0/10] color and pager improvements

On Fri, Aug 19, 2011 at 12:33:01AM +0200, Ingo Brückl wrote:
Show 10 quoted lines
> Jeff King wrote on Thu, 18 Aug 2011 14:58:20 -0700:
> 
> > I think Ingo's original complaint was simply that pager.stash didn't
> > actually do anything, not that he wanted some separate config for the
> > various subcommands.
> 
> No, you're wrong.
> 
> My goal was to be able to turn off paging for "stash list" only while all
> other stash commands should continue paging.

Ah, OK. I think the only other stash command that pages is "stash show", but I don't think it's unreasonable to want paging for that but not for "list".

I'll take a look at implementing something for that. Though I'll be traveling for the next few days, so it probably won't be for a bit.

-Peff
Ingo Brückl· Aug 19, 2011, 06:34 UTC · re: Jeff King · lore

Re: [PATCH 0/10] color and pager improvements

Jeff King wrote on Thu, 18 Aug 2011 15:46:44 -0700:
> On Fri, Aug 19, 2011 at 12:33:01AM +0200, Ingo Brückl wrote:
>> My goal was to be able to turn off paging for "stash list" only while all
>> other stash commands should continue paging.
> Ah, OK. I think the only other stash command that pages is "stash show",
> but I don't think it's unreasonable to want paging for that but not for
> "list".

Maybe "stash list" simply should - like other commands - not paginate by default.

Ingo
Jeff King· Aug 25, 2011, 20:25 UTC · re: Ingo Brückl · lore

Re: [PATCH 0/10] color and pager improvements

On Fri, Aug 19, 2011 at 08:34:13AM +0200, Ingo Brückl wrote:
Show 13 quoted lines
> Jeff King wrote on Thu, 18 Aug 2011 15:46:44 -0700:
> 
> > On Fri, Aug 19, 2011 at 12:33:01AM +0200, Ingo Brückl wrote:
> 
> >> My goal was to be able to turn off paging for "stash list" only while all
> >> other stash commands should continue paging.
> 
> > Ah, OK. I think the only other stash command that pages is "stash show",
> > but I don't think it's unreasonable to want paging for that but not for
> > "list".
> 
> Maybe "stash list" simply should - like other commands - not paginate by
> default.

I have no real opinion on that. It only paginates as a side effect of calling log.

I do think "git stash show" paginating by default is probably helpful, though.

The best way to get people's attention is probably to post a patch adding --no-pager to the git-log invocation of "git stash list". :)

-Peff
Steffen Daode Nurpmeso· Aug 18, 2011, 21:59 UTC · re: Jeff King · lore

Re: [PATCH 0/10] color and pager improvements

@ Jeff King <peff@peff.net> wrote (2011-08-18 06:58+0200):
> These three fix the problem Steffen mentioned here:

Uuuh, such a shame - you know that it was first noted by Benjamin Kudria (2008-07-23, http://marc.info/?l=git&m=121677902502581&w=2). And it was you who tried to resurrect the same issue last year. (The thing is: i did not search the archive first because it was clearly a bug. I did once you referred to your older patch.) But great that you actually found the time to fix it!

(I must admit though that i'm currently addicted to the coloured output, because simply switching between my terms gives a clear indication of where i'm currently git(1)ing. :-> And that in turn is something which gives more and more fun the longer i use it! It is *really* fantastic once you get used to it. And do gc --aggressive and all your temporary fooling is cleaned up.)

Now it's pretty unfortunate that i cannot offer fixes for anything.

I have a dumb patch of 'rebase -i' which includes the TODO entry line ($rest) as a comment in the commit message, which is pretty useful because i think about the rebase task and can store comments in that very line. But it introduces commit --cleanup=strip and patches commit.c to add a --message-prefix option. This is no good yet.

Michael J Gruber's today's shocking exercise on the german keyboard layout - maybe i should really resurrect parts of that NBSP series?

And referring to one sentence of yours from the past: no, refspec
stuff *is* that hard: they are not a tree which is created via
'refs_build_tree(); refs_merge_command_line();' upon program
start, with pointers to maybe instantiated .. whatever.
/*
 * Note. This is used only by "push"; refspec matching rules for
 * push and fetch are subtly different, so do not try to reuse it
 * without thinking.
 */
I gave up once i found that (in remote.c).  (AFAIR it seems
refspecs are first build L->R, then pushed, then build again but
in R->L direction.  Which is why without a fetch= the remotes/
ref is not updated after a push.  AFAIK - i gave up ...)

But i'm looking forward and really hope to be able to contribute some useful and good stuff to great projects in the future. OpenBSD, for example. :-)

--Steffen
Ciao, sdaoden(*)(gmail.com)
ASCII ribbon campaign           ( ) More nuclear fission plants
  against HTML e-mail            X    can serve more coloured
    and proprietary attachments / \     and sounding animations

← back to recent threads