{"thread":{"id":"28148","subject":"[PATCH 0/10] color and pager improvements","startedAt":"2011-08-18T04:58:24Z","lastAt":"2012-02-14T19:13:40Z","messageCount":37,"participants":["Jeff King","Steffen Daode Nurpmeso","Junio C Hamano","Ingo Brückl","Martin von Zweigbergk","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"173716","messageId":"20110818045821.GA17377@sigill.intra.peff.net","threadId":"28148","inReplyTo":null,"subject":"[PATCH 0/10] color and pager improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T04:58:24Z","receivedAt":"2011-08-18T04:58:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"While looking at the pager and color code today, I decided to tackle two\nlong-standing bugs, which entailed a lot of refactoring of the color\ncode. The result fixes some minor bugs, and is a little nicer for\ncalling code to use.\n\n [01/10]: t7006: modernize calls to unset\n [02/10]: test-lib: add helper functions for config\n [03/10]: t7006: use test_config helpers\n\nThese three are just cleanup I noticed before adding new tests to t7006;\nI hope that the helpers in 02/10 will be useful in a lot of other\nplaces, though.\n\n [04/10]: setup_pager: set GIT_PAGER_IN_USE\n\nThis fixes Ingo's problem from:\n\n http://article.gmane.org/gmane.comp.version-control.git/179430\n\nNamely that:\n\n git -p stash show\n\nfails to use colors properly.\n\n [05/10]: diff: refactor COLOR_DIFF from a flag into an int\n [06/10]: git_config_colorbool: refactor stdout_is_tty handling\n [07/10]: color: delay auto-color decision until point of use\n\nThese three fix the problem Steffen mentioned here:\n\n http://article.gmane.org/gmane.comp.version-control.git/177792\n\nNamely that pager.color doesn't work in many cases. This has been a\nproblem for years, but spread due to some pager-ordering changes late\nlast year (see the comments in 07/10). I actually wonder if anyone is\nreally using pager.color, as we haven't seen many complaints about it.\n\n [08/10]: config: refactor get_colorbool function\n [09/10]: diff: don't load color config in plumbing\n [10/10]: want_color: automatically fallback to color.ui\n\nThese three are refactoring that is made possible by 07/10. I think they\nmake the code cleaner, and hopefully the diffstat of 10/10 speaks for\nitself.\n\n-Peff\n"},{"id":"173717","messageId":"20110818050044.GA2889@sigill.intra.peff.net","threadId":"28148","inReplyTo":"20110818045821.GA17377@sigill.intra.peff.net","subject":"[PATCH 01/10] t7006: modernize calls to unset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T05:00:47Z","receivedAt":"2011-08-18T05:00:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These tests break &&-chaining to deal with broken \"unset\"\nimplementations. Instead, they should just use sane_unset.\n\nSigned-off-by: Jeff King \n---\n t/t7006-pager.sh | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex ed7575d..c0a3135 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -402,7 +402,7 @@ test_core_pager_subdir expect_success test_must_fail \\\n \t\t\t\t\t 'git -p apply expect &&\n \t>actual &&\n \tgit config --unset core.pager &&\n@@ -412,7 +412,7 @@ test_expect_success TTY 'command-specific pager' '\n '\n \n test_expect_success TTY 'command-specific pager overrides core.pager' '\n-\tunset PAGER GIT_PAGER;\n+\tsane_unset PAGER GIT_PAGER &&\n \techo \"foo:initial\" >expect &&\n \t>actual &&\n \tgit config core.pager \"exit 1\"\n-- \n1.7.6.10.g62f04\n"},{"id":"173718","messageId":"20110818050114.GB2889@sigill.intra.peff.net","threadId":"28148","inReplyTo":"20110818045821.GA17377@sigill.intra.peff.net","subject":"[PATCH 02/10] test-lib: add helper functions for config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T05:01:15Z","receivedAt":"2011-08-18T05:01:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are a few common tasks when working with configuration\nvariables in tests; this patch aims to make them a little\neasier to write and less error-prone.\n\nWhen setting a variable, you should typically make sure to\nclean it up after the test is finished, so as not to pollute\nother tests. Like:\n\n test_when_finished 'git config --unset foo.bar' &&\n git config foo.bar baz\n\nThis patch lets you just write:\n\n test_config foo.bar baz\n\nWhen clearing a variable that does not exist, git-config\nwill report a specific non-zero error code. Meaning that\ntests which call \"git config --unset\" often either rely on\nthe prior tests having actually set it, or must use\ntest_might_fail. With this patch, the previous:\n\n test_might_fail git config --unset foo.bar\n\nbecomes:\n\n test_unconfig foo.bar\n\nNot only is this easier to type, but it is more robust; it\nwill correctly detect errors from git-config besides \"key\nwas not set\".\n\nSigned-off-by: Jeff King \n---\n t/test-lib.sh | 18 ++++++++++++++++++\n 1 files changed, 18 insertions(+), 0 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex df25f17..926667a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -357,6 +357,24 @@ test_chmod () {\n \tgit update-index --add \"--chmod=$@\"\n }\n \n+# Unset a configuration variable, but don't fail if it doesn't exist.\n+test_unconfig () {\n+\tgit config --unset-all \"$@\"\n+\tconfig_status=$?\n+\tcase \"$config_status\" in\n+\t5) # ok, nothing to usnet\n+\t\tconfig_status=0\n+\t\t;;\n+\tesac\n+\treturn $config_status\n+}\n+\n+# Set git config, automatically unsetting it after the test is over.\n+test_config () {\n+\ttest_when_finished \"test_unconfig '$1'\" &&\n+\tgit config \"$@\"\n+}\n+\n # Use test_set_prereq to tell that a particular prerequisite is available.\n # The prerequisite can later be checked for in two ways:\n #\n-- \n1.7.6.10.g62f04\n"},{"id":"173719","messageId":"20110818050204.GC2889@sigill.intra.peff.net","threadId":"28148","inReplyTo":"20110818045821.GA17377@sigill.intra.peff.net","subject":"[PATCH 03/10] t7006: use test_config helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T05:02:06Z","receivedAt":"2011-08-18T05:02:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In some cases, this is just making the test script a little\nshorter and easier to read. However, there are several\nplaces where we didn't take proper precautions against\npolluting downstream tests with our config; this fixes them,\ntoo.\n\nSigned-off-by: Jeff King \n---\n t/t7006-pager.sh | 39 ++++++++++++++++++---------------------\n 1 files changed, 18 insertions(+), 21 deletions(-)\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex c0a3135..2ac729f 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -13,7 +13,7 @@ cleanup_fail() {\n \n test_expect_success 'setup' '\n \tsane_unset GIT_PAGER GIT_PAGER_IN_USE &&\n-\ttest_might_fail git config --unset core.pager &&\n+\ttest_unconfig core.pager &&\n \n \tPAGER=\"cat >paginated.out\" &&\n \texport PAGER &&\n@@ -94,21 +94,19 @@ test_expect_success TTY 'no pager with --no-pager' '\n \n test_expect_success TTY 'configuration can disable pager' '\n \trm -f paginated.out &&\n-\ttest_might_fail git config --unset pager.grep &&\n+\ttest_unconfig pager.grep &&\n \ttest_terminal git grep initial &&\n \ttest -e paginated.out &&\n \n \trm -f paginated.out &&\n-\tgit config pager.grep false &&\n-\ttest_when_finished \"git config --unset pager.grep\" &&\n+\ttest_config pager.grep false &&\n \ttest_terminal git grep initial &&\n \t! test -e paginated.out\n '\n \n test_expect_success TTY 'git config uses a pager if configured to' '\n \trm -f paginated.out &&\n-\tgit config pager.config true &&\n-\ttest_when_finished \"git config --unset pager.config\" &&\n+\ttest_config pager.config true &&\n \ttest_terminal git config --list &&\n \ttest -e paginated.out\n '\n@@ -116,8 +114,7 @@ test_expect_success TTY 'git config uses a pager if configured to' '\n test_expect_success TTY 'configuration can enable pager (from subdir)' '\n \trm -f paginated.out &&\n \tmkdir -p subdir &&\n-\tgit config pager.bundle true &&\n-\ttest_when_finished \"git config --unset pager.bundle\" &&\n+\ttest_config pager.bundle true &&\n \n \tgit bundle create test.bundle --all &&\n \trm -f paginated.out subdir/paginated.out &&\n@@ -150,7 +147,7 @@ test_expect_success 'tests can detect color' '\n \n test_expect_success 'no color when stdout is a regular file' '\n \trm -f colorless.log &&\n-\tgit config color.ui auto ||\n+\ttest_config color.ui auto ||\n \tcleanup_fail &&\n \n \tgit log >colorless.log &&\n@@ -159,7 +156,7 @@ test_expect_success 'no color when stdout is a regular file' '\n \n test_expect_success TTY 'color when writing to a pager' '\n \trm -f paginated.out &&\n-\tgit config color.ui auto ||\n+\ttest_config color.ui auto ||\n \tcleanup_fail &&\n \n \t(\n@@ -172,7 +169,7 @@ test_expect_success TTY 'color when writing to a pager' '\n \n test_expect_success 'color when writing to a file intended for a pager' '\n \trm -f colorful.log &&\n-\tgit config color.ui auto ||\n+\ttest_config color.ui auto ||\n \tcleanup_fail &&\n \n \t(\n@@ -221,7 +218,7 @@ test_default_pager() {\n \n \t$test_expectation SIMPLEPAGER,TTY \"$cmd - default pager is used by default\" \"\n \t\tsane_unset PAGER GIT_PAGER &&\n-\t\ttest_might_fail git config --unset core.pager &&\n+\t\ttest_unconfig core.pager &&\n \t\trm -f default_pager_used ||\n \t\tcleanup_fail &&\n \n@@ -244,7 +241,7 @@ test_PAGER_overrides() {\n \n \t$test_expectation TTY \"$cmd - PAGER overrides default pager\" \"\n \t\tsane_unset GIT_PAGER &&\n-\t\ttest_might_fail git config --unset core.pager &&\n+\t\ttest_unconfig core.pager &&\n \t\trm -f PAGER_used ||\n \t\tcleanup_fail &&\n \n@@ -277,7 +274,7 @@ test_core_pager() {\n \n \t\tPAGER=wc &&\n \t\texport PAGER &&\n-\t\tgit config core.pager 'wc >core.pager_used' &&\n+\t\ttest_config core.pager 'wc >core.pager_used' &&\n \t\t$full_command &&\n \t\t${if_local_config}test -e core.pager_used\n \t\"\n@@ -307,7 +304,7 @@ test_pager_subdir_helper() {\n \t\tPAGER=wc &&\n \t\tstampname=\\$(pwd)/core.pager_used &&\n \t\texport PAGER stampname &&\n-\t\tgit config core.pager 'wc >\\\"\\$stampname\\\"' &&\n+\t\ttest_config core.pager 'wc >\\\"\\$stampname\\\"' &&\n \t\tmkdir sub &&\n \t\t(\n \t\t\tcd sub &&\n@@ -324,7 +321,7 @@ test_GIT_PAGER_overrides() {\n \t\trm -f GIT_PAGER_used ||\n \t\tcleanup_fail &&\n \n-\t\tgit config core.pager wc &&\n+\t\ttest_config core.pager wc &&\n \t\tGIT_PAGER='wc >GIT_PAGER_used' &&\n \t\texport GIT_PAGER &&\n \t\t$full_command &&\n@@ -405,8 +402,8 @@ test_expect_success TTY 'command-specific pager' '\n \tsane_unset PAGER GIT_PAGER &&\n \techo \"foo:initial\" >expect &&\n \t>actual &&\n-\tgit config --unset core.pager &&\n-\tgit config pager.log \"sed s/^/foo:/ >actual\" &&\n+\ttest_unconfig core.pager &&\n+\ttest_config pager.log \"sed s/^/foo:/ >actual\" &&\n \ttest_terminal git log --format=%s -1 &&\n \ttest_cmp expect actual\n '\n@@ -415,8 +412,8 @@ test_expect_success TTY 'command-specific pager overrides core.pager' '\n \tsane_unset PAGER GIT_PAGER &&\n \techo \"foo:initial\" >expect &&\n \t>actual &&\n-\tgit config core.pager \"exit 1\"\n-\tgit config pager.log \"sed s/^/foo:/ >actual\" &&\n+\ttest_config core.pager \"exit 1\"\n+\ttest_config pager.log \"sed s/^/foo:/ >actual\" &&\n \ttest_terminal git log --format=%s -1 &&\n \ttest_cmp expect actual\n '\n@@ -425,7 +422,7 @@ test_expect_success TTY 'command-specific pager overridden by environment' '\n \tGIT_PAGER=\"sed s/^/foo:/ >actual\" && export GIT_PAGER &&\n \t>actual &&\n \techo \"foo:initial\" >expect &&\n-\tgit config pager.log \"exit 1\" &&\n+\ttest_config pager.log \"exit 1\" &&\n \ttest_terminal git log --format=%s -1 &&\n \ttest_cmp expect actual\n '\n-- \n1.7.6.10.g62f04\n"},{"id":"173720","messageId":"20110818050227.GD2889@sigill.intra.peff.net","threadId":"28148","inReplyTo":"20110818045821.GA17377@sigill.intra.peff.net","subject":"[PATCH 04/10] setup_pager: set GIT_PAGER_IN_USE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T05:02:29Z","receivedAt":"2011-08-18T05:02:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have always set a global \"spawned_pager\" variable when we\nstart the pager. This lets us make the auto-color decision\nlater in the program as as \"we are outputting to a terminal,\nor to a pager which can handle colors\".\n\nCommit 6e9af86 added support for the GIT_PAGER_IN_USE\nenvironment variable. An external program calling git (e.g.,\ngit-svn) could set this variable to indicate that it had\nalready started the pager, and that the decision about\nauto-coloring should take that into account.\n\nHowever, 6e9af86 failed to do the reverse, which is to tell\nexternal programs when git itself has started the pager.\nThus a git command implemented as an external script that\nhas the pager turned on (e.g., \"git -p stash show\") would\nnot realize it was going to a pager, and would suppress\ncolors.\n\nThis patch remedies that; we always set GIT_PAGER_IN_USE\nwhen we start the pager, and the value is respected by both\nthis program and any spawned children.\n\nSigned-off-by: Jeff King \n---\n pager.c | 8 +-------\n t/t7006-pager.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex dac358f..975955b 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -11,8 +11,6 @@\n * something different on Windows.\n */\n \n-static int spawned_pager;\n-\n #ifndef WIN32\n static void pager_preexec(void)\n {\n@@ -78,7 +76,7 @@ void setup_pager(void)\n \tif (!pager)\n \t\treturn;\n \n-\tspawned_pager = 1; /* means we are emitting to terminal */\n+\tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n \n \t/* spawn the pager */\n \tpager_argv[0] = pager;\n@@ -109,10 +107,6 @@ void setup_pager(void)\n int pager_in_use(void)\n {\n \tconst char *env;\n-\n-\tif (spawned_pager)\n-\t\treturn 1;\n-\n \tenv = getenv(\"GIT_PAGER_IN_USE\");\n \treturn env ? git_config_bool(\"GIT_PAGER_IN_USE\", env) : 0;\n }\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 2ac729f..4884e1b 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -181,6 +181,17 @@ test_expect_success 'color when writing to a file intended for a pager' '\n \tcolorful colorful.log\n '\n \n+test_expect_success TTY 'colors are sent to pager for external commands' '\n+\ttest_config alias.externallog \"!git log\" &&\n+\ttest_config color.ui auto &&\n+\t(\n+\t\tTERM=vt100 &&\n+\t\texport TERM &&\n+\t\ttest_terminal git -p externallog\n+\t) &&\n+\tcolorful paginated.out\n+'\n+\n # Use this helper to make it easy for the caller of your\n # terminal-using function to specify whether it should fail.\n # If you write\n-- \n1.7.6.10.g62f04\n"},{"id":"173721","messageId":"20110818050310.GE2889@sigill.intra.peff.net","threadId":"28148","inReplyTo":"20110818045821.GA17377@sigill.intra.peff.net","subject":"[PATCH 05/10] diff: refactor COLOR_DIFF from a flag into an int","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T05:03:12Z","receivedAt":"2011-08-18T05:03:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This lets us store more than just a bit flag for whether we\nwant color; we can also store whether we want automatic\ncolors. This can be useful for making the automatic-color\ndecision closer to the point of use.\n\nThis mostly just involves replacing DIFF_OPT_* calls with\nmanipulations of the flag. The biggest exception is that\ncalls to DIFF_OPT_TST must check for \"o->use_color > 0\",\nwhich lets an \"unknown\" value (i.e., the default) stay at\n\"no color\". In the previous code, a value of \"-1\" was not\npropagated at all.\n\nSigned-off-by: Jeff King \n---\n builtin/merge.c | 2 --\n combine-diff.c | 7 +++----\n diff.c | 41 +++++++++++++++++++----------------------\n diff.h | 5 +++--\n graph.c | 2 +-\n log-tree.c | 4 ++--\n wt-status.c | 2 +-\n 7 files changed, 29 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 325891e..7209edf 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -390,8 +390,6 @@ static void finish(const unsigned char *new_head, const char *msg)\n \t\topts.output_format |=\n \t\t\tDIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n \t\topts.detect_rename = DIFF_DETECT_RENAME;\n-\t\tif (diff_use_color_default > 0)\n-\t\t\tDIFF_OPT_SET(&opts, COLOR_DIFF);\n \t\tif (diff_setup_done(&opts) < 0)\n \t\t\tdie(_(\"diff_setup_done failed\"));\n \t\tdiff_tree_sha1(head, new_head, \"\", &opts);\ndiff --git a/combine-diff.c b/combine-diff.c\nindex be67cfc..c588c79 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -702,9 +702,8 @@ static void show_combined_header(struct combine_diff_path *elem,\n \tint abbrev = DIFF_OPT_TST(opt, FULL_INDEX) ? 40 : DEFAULT_ABBREV;\n \tconst char *a_prefix = opt->a_prefix ? opt->a_prefix : \"a/\";\n \tconst char *b_prefix = opt->b_prefix ? opt->b_prefix : \"b/\";\n-\tint use_color = DIFF_OPT_TST(opt, COLOR_DIFF);\n-\tconst char *c_meta = diff_get_color(use_color, DIFF_METAINFO);\n-\tconst char *c_reset = diff_get_color(use_color, DIFF_RESET);\n+\tconst char *c_meta = diff_get_color_opt(opt, DIFF_METAINFO);\n+\tconst char *c_reset = diff_get_color_opt(opt, DIFF_RESET);\n \tconst char *abb;\n \tint added = 0;\n \tint deleted = 0;\n@@ -964,7 +963,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\tshow_combined_header(elem, num_parent, dense, rev,\n \t\t\t\t mode_differs, 1);\n \t\tdump_sline(sline, cnt, num_parent,\n-\t\t\t DIFF_OPT_TST(opt, COLOR_DIFF), result_deleted);\n+\t\t\t opt->use_color, result_deleted);\n \t}\n \tfree(result);\n \ndiff --git a/diff.c b/diff.c\nindex 93ef9a2..2d86abe 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -583,11 +583,10 @@ static void emit_rewrite_diff(const char *name_a,\n \t\t\t struct diff_options *o)\n {\n \tint lc_a, lc_b;\n-\tint color_diff = DIFF_OPT_TST(o, COLOR_DIFF);\n \tconst char *name_a_tab, *name_b_tab;\n-\tconst char *metainfo = diff_get_color(color_diff, DIFF_METAINFO);\n-\tconst char *fraginfo = diff_get_color(color_diff, DIFF_FRAGINFO);\n-\tconst char *reset = diff_get_color(color_diff, DIFF_RESET);\n+\tconst char *metainfo = diff_get_color(o->use_color, DIFF_METAINFO);\n+\tconst char *fraginfo = diff_get_color(o->use_color, DIFF_FRAGINFO);\n+\tconst char *reset = diff_get_color(o->use_color, DIFF_RESET);\n \tstatic struct strbuf a_name = STRBUF_INIT, b_name = STRBUF_INIT;\n \tconst char *a_prefix, *b_prefix;\n \tchar *data_one, *data_two;\n@@ -623,7 +622,7 @@ static void emit_rewrite_diff(const char *name_a,\n \tsize_two = fill_textconv(textconv_two, two, &data_two);\n \n \tmemset(&ecbdata, 0, sizeof(ecbdata));\n-\tecbdata.color_diff = color_diff;\n+\tecbdata.color_diff = o->use_color > 0;\n \tecbdata.found_changesp = &o->found_changes;\n \tecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);\n \tecbdata.opt = o;\n@@ -1004,7 +1003,7 @@ static void free_diff_words_data(struct emit_callback *ecbdata)\n \n const char *diff_get_color(int diff_use_color, enum color_diff ix)\n {\n-\tif (diff_use_color)\n+\tif (diff_use_color > 0)\n \t\treturn diff_colors[ix];\n \treturn \"\";\n }\n@@ -1808,11 +1807,10 @@ static int is_conflict_marker(const char *line, int marker_size, unsigned long l\n static void checkdiff_consume(void *priv, char *line, unsigned long len)\n {\n \tstruct checkdiff_t *data = priv;\n-\tint color_diff = DIFF_OPT_TST(data->o, COLOR_DIFF);\n \tint marker_size = data->conflict_marker_size;\n-\tconst char *ws = diff_get_color(color_diff, DIFF_WHITESPACE);\n-\tconst char *reset = diff_get_color(color_diff, DIFF_RESET);\n-\tconst char *set = diff_get_color(color_diff, DIFF_FILE_NEW);\n+\tconst char *ws = diff_get_color(data->o->use_color, DIFF_WHITESPACE);\n+\tconst char *reset = diff_get_color(data->o->use_color, DIFF_RESET);\n+\tconst char *set = diff_get_color(data->o->use_color, DIFF_FILE_NEW);\n \tchar *err;\n \tchar *line_prefix = \"\";\n \tstruct strbuf *msgbuf;\n@@ -2157,7 +2155,7 @@ static void builtin_diff(const char *name_a,\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\tmemset(&ecbdata, 0, sizeof(ecbdata));\n \t\tecbdata.label_path = lbl;\n-\t\tecbdata.color_diff = DIFF_OPT_TST(o, COLOR_DIFF);\n+\t\tecbdata.color_diff = o->use_color > 0;\n \t\tecbdata.found_changesp = &o->found_changes;\n \t\tecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);\n \t\tif (ecbdata.ws_rule & WS_BLANK_AT_EOF)\n@@ -2205,7 +2203,7 @@ static void builtin_diff(const char *name_a,\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t}\n-\t\t\tif (DIFF_OPT_TST(o, COLOR_DIFF)) {\n+\t\t\tif (o->use_color > 0) {\n \t\t\t\tstruct diff_words_style *st = ecbdata.diff_words->style;\n \t\t\t\tst->old.color = diff_get_color_opt(o, DIFF_FILE_OLD);\n \t\t\t\tst->new.color = diff_get_color_opt(o, DIFF_FILE_NEW);\n@@ -2855,7 +2853,7 @@ static void run_diff_cmd(const char *pgm,\n \t\t */\n \t\tfill_metainfo(msg, name, other, one, two, o, p,\n \t\t\t &must_show_header,\n-\t\t\t DIFF_OPT_TST(o, COLOR_DIFF) && !pgm);\n+\t\t\t o->use_color > 0 && !pgm);\n \t\txfrm_msg = msg->len ? msg->buf : NULL;\n \t}\n \n@@ -3021,8 +3019,7 @@ void diff_setup(struct diff_options *options)\n \n \toptions->change = diff_change;\n \toptions->add_remove = diff_addremove;\n-\tif (diff_use_color_default > 0)\n-\t\tDIFF_OPT_SET(options, COLOR_DIFF);\n+\toptions->use_color = diff_use_color_default;\n \toptions->detect_rename = diff_detect_rename_default;\n \n \tif (diff_no_prefix) {\n@@ -3410,24 +3407,24 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \telse if (!strcmp(arg, \"--follow\"))\n \t\tDIFF_OPT_SET(options, FOLLOW_RENAMES);\n \telse if (!strcmp(arg, \"--color\"))\n-\t\tDIFF_OPT_SET(options, COLOR_DIFF);\n+\t\toptions->use_color = 1;\n \telse if (!prefixcmp(arg, \"--color=\")) {\n \t\tint value = git_config_colorbool(NULL, arg+8, -1);\n \t\tif (value == 0)\n-\t\t\tDIFF_OPT_CLR(options, COLOR_DIFF);\n+\t\t\toptions->use_color = 0;\n \t\telse if (value > 0)\n-\t\t\tDIFF_OPT_SET(options, COLOR_DIFF);\n+\t\t\toptions->use_color = 1;\n \t\telse\n \t\t\treturn error(\"option `color' expects \\\"always\\\", \\\"auto\\\", or \\\"never\\\"\");\n \t}\n \telse if (!strcmp(arg, \"--no-color\"))\n-\t\tDIFF_OPT_CLR(options, COLOR_DIFF);\n+\t\toptions->use_color = 0;\n \telse if (!strcmp(arg, \"--color-words\")) {\n-\t\tDIFF_OPT_SET(options, COLOR_DIFF);\n+\t\toptions->use_color = 1;\n \t\toptions->word_diff = DIFF_WORDS_COLOR;\n \t}\n \telse if (!prefixcmp(arg, \"--color-words=\")) {\n-\t\tDIFF_OPT_SET(options, COLOR_DIFF);\n+\t\toptions->use_color = 1;\n \t\toptions->word_diff = DIFF_WORDS_COLOR;\n \t\toptions->word_regex = arg + 14;\n \t}\n@@ -3440,7 +3437,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\tif (!strcmp(type, \"plain\"))\n \t\t\toptions->word_diff = DIFF_WORDS_PLAIN;\n \t\telse if (!strcmp(type, \"color\")) {\n-\t\t\tDIFF_OPT_SET(options, COLOR_DIFF);\n+\t\t\toptions->use_color = 1;\n \t\t\toptions->word_diff = DIFF_WORDS_COLOR;\n \t\t}\n \t\telse if (!strcmp(type, \"porcelain\"))\ndiff --git a/diff.h b/diff.h\nindex b920a20..8c66b59 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -58,7 +58,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n #define DIFF_OPT_SILENT_ON_REMOVE (1 << 5)\n #define DIFF_OPT_FIND_COPIES_HARDER (1 << 6)\n #define DIFF_OPT_FOLLOW_RENAMES (1 << 7)\n-#define DIFF_OPT_COLOR_DIFF (1 << 8)\n+/* (1 << 8) unused */\n /* (1 << 9) unused */\n #define DIFF_OPT_HAS_CHANGES (1 << 10)\n #define DIFF_OPT_QUICK (1 << 11)\n@@ -101,6 +101,7 @@ struct diff_options {\n \tconst char *single_follow;\n \tconst char *a_prefix, *b_prefix;\n \tunsigned flags;\n+\tint use_color;\n \tint context;\n \tint interhunkcontext;\n \tint break_opt;\n@@ -160,7 +161,7 @@ enum color_diff {\n };\n const char *diff_get_color(int diff_use_color, enum color_diff ix);\n #define diff_get_color_opt(o, ix) \\\n-\tdiff_get_color(DIFF_OPT_TST((o), COLOR_DIFF), ix)\n+\tdiff_get_color((o)->use_color, ix)\n \n \n extern const char mime_boundary_leader[];\ndiff --git a/graph.c b/graph.c\nindex 2f6893d..556834a 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -347,7 +347,7 @@ static struct commit_list *first_interesting_parent(struct git_graph *graph)\n \n static unsigned short graph_get_current_column_color(const struct git_graph *graph)\n {\n-\tif (!DIFF_OPT_TST(&graph->revs->diffopt, COLOR_DIFF))\n+\tif (graph->revs->diffopt.use_color <= 0)\n \t\treturn column_colors_max;\n \treturn graph->default_column_color;\n }\ndiff --git a/log-tree.c b/log-tree.c\nindex e945701..9ba8fb2 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -31,7 +31,7 @@ static char decoration_colors[][COLOR_MAXLEN] = {\n \n static const char *decorate_get_color(int decorate_use_color, enum decoration_type ix)\n {\n-\tif (decorate_use_color)\n+\tif (decorate_use_color > 0)\n \t\treturn decoration_colors[ix];\n \treturn \"\";\n }\n@@ -77,7 +77,7 @@ int parse_decorate_color_config(const char *var, const int ofs, const char *valu\n * for showing the commit sha1, use the same check for --decorate\n */\n #define decorate_get_color_opt(o, ix) \\\n-\tdecorate_get_color(DIFF_OPT_TST((o), COLOR_DIFF), ix)\n+\tdecorate_get_color((o)->use_color, ix)\n \n static void add_name_decoration(enum decoration_type type, const char *name, struct object *obj)\n {\ndiff --git a/wt-status.c b/wt-status.c\nindex 0237772..ee03431 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -681,7 +681,7 @@ static void wt_status_print_verbose(struct wt_status *s)\n \t * will have checked isatty on stdout).\n \t */\n \tif (s->fp != stdout)\n-\t\tDIFF_OPT_CLR(&rev.diffopt, COLOR_DIFF);\n+\t\trev.diffopt.use_color = 0;\n \trun_diff_index(&rev, 1);\n }\n \n-- \n1.7.6.10.g62f04\n"},{"id":"173722","messageId":"20110818050346.GF2889@sigill.intra.peff.net","threadId":"28148","inReplyTo":"20110818045821.GA17377@sigill.intra.peff.net","subject":"[PATCH 06/10] git_config_colorbool: refactor stdout_is_tty handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T05:03:48Z","receivedAt":"2011-08-18T05:03:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Usually this function figures out for itself whether stdout\nis a tty. However, it has an extra parameter just to allow\ngit-config to override the auto-detection for its\n--get-colorbool option.\n\nInstead of an extra parameter, let's just use a global\nvariable. This makes calling easier in the common case, and\nwill make refactoring the colorbool code much simpler.\n\nSigned-off-by: Jeff King \n---\n builtin/branch.c | 2 +-\n builtin/commit.c | 2 +-\n builtin/config.c | 23 +++++++----------------\n builtin/grep.c | 2 +-\n builtin/show-branch.c | 2 +-\n color.c | 11 ++++++-----\n color.h | 8 +++++++-\n diff.c | 4 ++--\n parse-options.c | 2 +-\n 9 files changed, 27 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 3142daa..b15fee5 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -71,7 +71,7 @@ static int parse_branch_color_slot(const char *var, int ofs)\n static int git_branch_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"color.branch\")) {\n-\t\tbranch_use_color = git_config_colorbool(var, value, -1);\n+\t\tbranch_use_color = git_config_colorbool(var, value);\n \t\treturn 0;\n \t}\n \tif (!prefixcmp(var, \"color.branch.\")) {\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e1af9b1..295803a 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1144,7 +1144,7 @@ static int git_status_config(const char *k, const char *v, void *cb)\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"status.color\") || !strcmp(k, \"color.status\")) {\n-\t\ts->use_color = git_config_colorbool(k, v, -1);\n+\t\ts->use_color = git_config_colorbool(k, v);\n \t\treturn 0;\n \t}\n \tif (!prefixcmp(k, \"status.color.\") || !prefixcmp(k, \"color.status.\")) {\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 211e118..5505ced 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -303,24 +303,17 @@ static void get_color(const char *def_color)\n \tfputs(parsed_color, stdout);\n }\n \n-static int stdout_is_tty;\n static int get_colorbool_found;\n static int get_diff_color_found;\n static int git_get_colorbool_config(const char *var, const char *value,\n \t\tvoid *cb)\n {\n-\tif (!strcmp(var, get_colorbool_slot)) {\n-\t\tget_colorbool_found =\n-\t\t\tgit_config_colorbool(var, value, stdout_is_tty);\n-\t}\n-\tif (!strcmp(var, \"diff.color\")) {\n-\t\tget_diff_color_found =\n-\t\t\tgit_config_colorbool(var, value, stdout_is_tty);\n-\t}\n-\tif (!strcmp(var, \"color.ui\")) {\n-\t\tgit_use_color_default = git_config_colorbool(var, value, stdout_is_tty);\n-\t\treturn 0;\n-\t}\n+\tif (!strcmp(var, get_colorbool_slot))\n+\t\tget_colorbool_found = git_config_colorbool(var, value);\n+\telse if (!strcmp(var, \"diff.color\"))\n+\t\tget_diff_color_found = git_config_colorbool(var, value);\n+\telse if (!strcmp(var, \"color.ui\"))\n+\t\tgit_use_color_default = git_config_colorbool(var, value);\n \treturn 0;\n }\n \n@@ -510,9 +503,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n \telse if (actions == ACTION_GET_COLORBOOL) {\n \t\tif (argc == 1)\n-\t\t\tstdout_is_tty = git_config_bool(\"command line\", argv[0]);\n-\t\telse if (argc == 0)\n-\t\t\tstdout_is_tty = isatty(1);\n+\t\t\tcolor_stdout_is_tty = git_config_bool(\"command line\", argv[0]);\n \t\treturn get_colorbool(argc != 0);\n \t}\n \ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex cccf8da..d80db22 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -325,7 +325,7 @@ static int grep_config(const char *var, const char *value, void *cb)\n \t}\n \n \tif (!strcmp(var, \"color.grep\"))\n-\t\topt->color = git_config_colorbool(var, value, -1);\n+\t\topt->color = git_config_colorbool(var, value);\n \telse if (!strcmp(var, \"color.grep.context\"))\n \t\tcolor = opt->color_context;\n \telse if (!strcmp(var, \"color.grep.filename\"))\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex facc63a..e6650b4 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -573,7 +573,7 @@ static int git_show_branch_config(const char *var, const char *value, void *cb)\n \t}\n \n \tif (!strcmp(var, \"color.showbranch\")) {\n-\t\tshowbranch_use_color = git_config_colorbool(var, value, -1);\n+\t\tshowbranch_use_color = git_config_colorbool(var, value);\n \t\treturn 0;\n \t}\n \ndiff --git a/color.c b/color.c\nindex 3db214c..67affa4 100644\n--- a/color.c\n+++ b/color.c\n@@ -2,6 +2,7 @@\n #include \"color.h\"\n \n int git_use_color_default = 0;\n+int color_stdout_is_tty = -1;\n \n /*\n * The list of available column colors.\n@@ -157,7 +158,7 @@ bad:\n \tdie(\"bad color value '%.*s' for variable '%s'\", value_len, value, var);\n }\n \n-int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)\n+int git_config_colorbool(const char *var, const char *value)\n {\n \tif (value) {\n \t\tif (!strcasecmp(value, \"never\"))\n@@ -177,9 +178,9 @@ int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)\n \n \t/* any normal truth value defaults to 'auto' */\n auto_color:\n-\tif (stdout_is_tty < 0)\n-\t\tstdout_is_tty = isatty(1);\n-\tif (stdout_is_tty || (pager_in_use() && pager_use_color)) {\n+\tif (color_stdout_is_tty < 0)\n+\t\tcolor_stdout_is_tty = isatty(1);\n+\tif (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {\n \t\tchar *term = getenv(\"TERM\");\n \t\tif (term && strcmp(term, \"dumb\"))\n \t\t\treturn 1;\n@@ -190,7 +191,7 @@ int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)\n int git_color_default_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"color.ui\")) {\n-\t\tgit_use_color_default = git_config_colorbool(var, value, -1);\n+\t\tgit_use_color_default = git_config_colorbool(var, value);\n \t\treturn 0;\n \t}\n \ndiff --git a/color.h b/color.h\nindex 68a926a..a190a25 100644\n--- a/color.h\n+++ b/color.h\n@@ -58,11 +58,17 @@ extern const char *column_colors_ansi[];\n extern const int column_colors_ansi_max;\n \n /*\n+ * Generally the color code will lazily figure this out itself, but\n+ * this provides a mechanism for callers to override autodetection.\n+ */\n+extern int color_stdout_is_tty;\n+\n+/*\n * Use this instead of git_default_config if you need the value of color.ui.\n */\n int git_color_default_config(const char *var, const char *value, void *cb);\n \n-int git_config_colorbool(const char *var, const char *value, int stdout_is_tty);\n+int git_config_colorbool(const char *var, const char *value);\n void color_parse(const char *value, const char *var, char *dst);\n void color_parse_mem(const char *value, int len, const char *var, char *dst);\n __attribute__((format (printf, 3, 4)))\ndiff --git a/diff.c b/diff.c\nindex 2d86abe..2dfc359 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -137,7 +137,7 @@ static int git_config_rename(const char *var, const char *value)\n int git_diff_ui_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"diff.color\") || !strcmp(var, \"color.diff\")) {\n-\t\tdiff_use_color_default = git_config_colorbool(var, value, -1);\n+\t\tdiff_use_color_default = git_config_colorbool(var, value);\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"diff.renames\")) {\n@@ -3409,7 +3409,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \telse if (!strcmp(arg, \"--color\"))\n \t\toptions->use_color = 1;\n \telse if (!prefixcmp(arg, \"--color=\")) {\n-\t\tint value = git_config_colorbool(NULL, arg+8, -1);\n+\t\tint value = git_config_colorbool(NULL, arg+8);\n \t\tif (value == 0)\n \t\t\toptions->use_color = 0;\n \t\telse if (value > 0)\ndiff --git a/parse-options.c b/parse-options.c\nindex 879ea82..be4383e 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -621,7 +621,7 @@ int parse_opt_color_flag_cb(const struct option *opt, const char *arg,\n \n \tif (!arg)\n \t\targ = unset ? \"never\" : (const char *)opt->defval;\n-\tvalue = git_config_colorbool(NULL, arg, -1);\n+\tvalue = git_config_colorbool(NULL, arg);\n \tif (value < 0)\n \t\treturn opterror(opt,\n \t\t\t\"expects \\\"always\\\", \\\"auto\\\", or \\\"never\\\"\", 0);\n-- \n1.7.6.10.g62f04\n"},{"id":"173723","messageId":"20110818050421.GG2889@sigill.intra.peff.net","threadId":"28148","inReplyTo":"20110818045821.GA17377@sigill.intra.peff.net","subject":"[PATCH 07/10] color: delay auto-color decision until point of use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T05:04:23Z","receivedAt":"2011-08-18T05:04:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we read a color value either from a config file or from\nthe command line, we use git_config_colorbool to convert it\nfrom the tristate always/never/auto into a single yes/no\nboolean value.\n\nThis has some timing implications with respect to starting\na pager.\n\nIf we start (or decide not to start) the pager before\nchecking the colorbool, everything is fine. Either isatty(1)\nwill give us the right information, or we will properly\ncheck for pager_in_use().\n\nHowever, if we decide to start a pager after we have checked\nthe colorbool, things are not so simple. If stdout is a tty,\nthen we will have already decided to use color. However, the\nuser may also have configured color.pager not to use color\nwith the pager. In this case, we need to actually turn off\ncolor. Unfortunately, the pager code has no idea which color\nvariables were turned on (and there are many of them\nthroughout the code, and they may even have been manipulated\nafter the colorbool selection by something like \"--color\" on\nthe command line).\n\nThis bug can be seen any time a pager is started after\nconfig and command line options are checked. This has\naffected \"git diff\" since 89d07f7 (diff: don't run pager if\nuser asked for a diff style exit code, 2007-08-12). It has\nalso affect the log family since 1fda91b (Fix 'git log'\nearly pager startup error case, 2010-08-24).\n\nThis patch splits the notion of parsing a colorbool and\nactually checking the configuration. The \"use_color\"\nvariables now have an additional possible value,\nGIT_COLOR_AUTO. Users of the variable should use the new\n\"want_color()\" wrapper, which will lazily determine and\ncache the auto-color decision.\n\nSigned-off-by: Jeff King \n---\n builtin/branch.c | 2 +-\n builtin/config.c | 2 ++\n builtin/show-branch.c | 4 ++--\n color.c | 20 ++++++++++++++++++--\n color.h | 11 +++++++++++\n diff.c | 17 +++++++----------\n graph.c | 2 +-\n grep.c | 2 +-\n log-tree.c | 2 +-\n t/t7006-pager.sh | 12 ++++++++++++\n wt-status.c | 4 +++-\n 11 files changed, 59 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex b15fee5..d6d3c7d 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -88,7 +88,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)\n \n static const char *branch_get_color(enum color_branch ix)\n {\n-\tif (branch_use_color > 0)\n+\tif (want_color(branch_use_color))\n \t\treturn branch_colors[ix];\n \treturn \"\";\n }\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 5505ced..3a09296 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -330,6 +330,8 @@ static int get_colorbool(int print)\n \t\t\tget_colorbool_found = git_use_color_default;\n \t}\n \n+\tget_colorbool_found = want_color(get_colorbool_found);\n+\n \tif (print) {\n \t\tprintf(\"%s\\n\", get_colorbool_found ? \"true\" : \"false\");\n \t\treturn 0;\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex e6650b4..4b726fa 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -26,14 +26,14 @@ static const char **default_arg;\n \n static const char *get_color_code(int idx)\n {\n-\tif (showbranch_use_color)\n+\tif (want_color(showbranch_use_color))\n \t\treturn column_colors_ansi[idx % column_colors_ansi_max];\n \treturn \"\";\n }\n \n static const char *get_color_reset_code(void)\n {\n-\tif (showbranch_use_color)\n+\tif (want_color(showbranch_use_color))\n \t\treturn GIT_COLOR_RESET;\n \treturn \"\";\n }\ndiff --git a/color.c b/color.c\nindex 67affa4..8586417 100644\n--- a/color.c\n+++ b/color.c\n@@ -166,7 +166,7 @@ int git_config_colorbool(const char *var, const char *value)\n \t\tif (!strcasecmp(value, \"always\"))\n \t\t\treturn 1;\n \t\tif (!strcasecmp(value, \"auto\"))\n-\t\t\tgoto auto_color;\n+\t\t\treturn GIT_COLOR_AUTO;\n \t}\n \n \tif (!var)\n@@ -177,7 +177,11 @@ int git_config_colorbool(const char *var, const char *value)\n \t\treturn 0;\n \n \t/* any normal truth value defaults to 'auto' */\n- auto_color:\n+\treturn GIT_COLOR_AUTO;\n+}\n+\n+static int check_auto_color(void)\n+{\n \tif (color_stdout_is_tty < 0)\n \t\tcolor_stdout_is_tty = isatty(1);\n \tif (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {\n@@ -188,6 +192,18 @@ int git_config_colorbool(const char *var, const char *value)\n \treturn 0;\n }\n \n+int want_color(int var)\n+{\n+\tstatic int want_auto = -1;\n+\n+\tif (var == GIT_COLOR_AUTO) {\n+\t\tif (want_auto < 0)\n+\t\t\twant_auto = check_auto_color();\n+\t\treturn want_auto;\n+\t}\n+\treturn var > 0;\n+}\n+\n int git_color_default_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"color.ui\")) {\ndiff --git a/color.h b/color.h\nindex a190a25..d715fd5 100644\n--- a/color.h\n+++ b/color.h\n@@ -49,6 +49,16 @@ struct strbuf;\n #define GIT_COLOR_NIL \"NIL\"\n \n /*\n+ * The first three are chosen to match common usage in the code, and what is\n+ * returned from git_config_colorbool. The \"auto\" value can be returned from\n+ * config_colorbool, and will be converted by want_color() into either 0 or 1.\n+ */\n+#define GIT_COLOR_UNKNOWN -1\n+#define GIT_COLOR_ALWAYS 0\n+#define GIT_COLOR_NEVER 1\n+#define GIT_COLOR_AUTO 2\n+\n+/*\n * This variable stores the value of color.ui\n */\n extern int git_use_color_default;\n@@ -69,6 +79,7 @@ extern int color_stdout_is_tty;\n int git_color_default_config(const char *var, const char *value, void *cb);\n \n int git_config_colorbool(const char *var, const char *value);\n+int want_color(int var);\n void color_parse(const char *value, const char *var, char *dst);\n void color_parse_mem(const char *value, int len, const char *var, char *dst);\n __attribute__((format (printf, 3, 4)))\ndiff --git a/diff.c b/diff.c\nindex 2dfc359..29cecf1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -622,7 +622,7 @@ static void emit_rewrite_diff(const char *name_a,\n \tsize_two = fill_textconv(textconv_two, two, &data_two);\n \n \tmemset(&ecbdata, 0, sizeof(ecbdata));\n-\tecbdata.color_diff = o->use_color > 0;\n+\tecbdata.color_diff = want_color(o->use_color);\n \tecbdata.found_changesp = &o->found_changes;\n \tecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);\n \tecbdata.opt = o;\n@@ -1003,7 +1003,7 @@ static void free_diff_words_data(struct emit_callback *ecbdata)\n \n const char *diff_get_color(int diff_use_color, enum color_diff ix)\n {\n-\tif (diff_use_color > 0)\n+\tif (want_color(diff_use_color))\n \t\treturn diff_colors[ix];\n \treturn \"\";\n }\n@@ -2155,7 +2155,7 @@ static void builtin_diff(const char *name_a,\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\tmemset(&ecbdata, 0, sizeof(ecbdata));\n \t\tecbdata.label_path = lbl;\n-\t\tecbdata.color_diff = o->use_color > 0;\n+\t\tecbdata.color_diff = want_color(o->use_color);\n \t\tecbdata.found_changesp = &o->found_changes;\n \t\tecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);\n \t\tif (ecbdata.ws_rule & WS_BLANK_AT_EOF)\n@@ -2203,7 +2203,7 @@ static void builtin_diff(const char *name_a,\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t}\n-\t\t\tif (o->use_color > 0) {\n+\t\t\tif (want_color(o->use_color)) {\n \t\t\t\tstruct diff_words_style *st = ecbdata.diff_words->style;\n \t\t\t\tst->old.color = diff_get_color_opt(o, DIFF_FILE_OLD);\n \t\t\t\tst->new.color = diff_get_color_opt(o, DIFF_FILE_NEW);\n@@ -2853,7 +2853,7 @@ static void run_diff_cmd(const char *pgm,\n \t\t */\n \t\tfill_metainfo(msg, name, other, one, two, o, p,\n \t\t\t &must_show_header,\n-\t\t\t o->use_color > 0 && !pgm);\n+\t\t\t want_color(o->use_color) && !pgm);\n \t\txfrm_msg = msg->len ? msg->buf : NULL;\n \t}\n \n@@ -3410,12 +3410,9 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\toptions->use_color = 1;\n \telse if (!prefixcmp(arg, \"--color=\")) {\n \t\tint value = git_config_colorbool(NULL, arg+8);\n-\t\tif (value == 0)\n-\t\t\toptions->use_color = 0;\n-\t\telse if (value > 0)\n-\t\t\toptions->use_color = 1;\n-\t\telse\n+\t\tif (value < 0)\n \t\t\treturn error(\"option `color' expects \\\"always\\\", \\\"auto\\\", or \\\"never\\\"\");\n+\t\toptions->use_color = value;\n \t}\n \telse if (!strcmp(arg, \"--no-color\"))\n \t\toptions->use_color = 0;\ndiff --git a/graph.c b/graph.c\nindex 556834a..7358416 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -347,7 +347,7 @@ static struct commit_list *first_interesting_parent(struct git_graph *graph)\n \n static unsigned short graph_get_current_column_color(const struct git_graph *graph)\n {\n-\tif (graph->revs->diffopt.use_color <= 0)\n+\tif (!want_color(graph->revs->diffopt.use_color))\n \t\treturn column_colors_max;\n \treturn graph->default_column_color;\n }\ndiff --git a/grep.c b/grep.c\nindex 04e9ba4..abf4288 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -430,7 +430,7 @@ static int word_char(char ch)\n static void output_color(struct grep_opt *opt, const void *data, size_t size,\n \t\t\t const char *color)\n {\n-\tif (opt->color && color && color[0]) {\n+\tif (want_color(opt->color) && color && color[0]) {\n \t\topt->output(opt, color, strlen(color));\n \t\topt->output(opt, data, size);\n \t\topt->output(opt, GIT_COLOR_RESET, strlen(GIT_COLOR_RESET));\ndiff --git a/log-tree.c b/log-tree.c\nindex 9ba8fb2..95d6d40 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -31,7 +31,7 @@ static char decoration_colors[][COLOR_MAXLEN] = {\n \n static const char *decorate_get_color(int decorate_use_color, enum decoration_type ix)\n {\n-\tif (decorate_use_color > 0)\n+\tif (want_color(decorate_use_color))\n \t\treturn decoration_colors[ix];\n \treturn \"\";\n }\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 4884e1b..4582336 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -167,6 +167,18 @@ test_expect_success TTY 'color when writing to a pager' '\n \tcolorful paginated.out\n '\n \n+test_expect_success TTY 'colors are suppressed by color.pager' '\n+\trm -f paginated.out &&\n+\ttest_config color.ui auto &&\n+\ttest_config color.pager false &&\n+\t(\n+\t\tTERM=vt100 &&\n+\t\texport TERM &&\n+\t\ttest_terminal git log\n+\t) &&\n+\t! colorful paginated.out\n+'\n+\n test_expect_success 'color when writing to a file intended for a pager' '\n \trm -f colorful.log &&\n \ttest_config color.ui auto ||\ndiff --git a/wt-status.c b/wt-status.c\nindex ee03431..8836a52 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -26,7 +26,9 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {\n \n static const char *color(int slot, struct wt_status *s)\n {\n-\tconst char *c = s->use_color > 0 ? s->color_palette[slot] : \"\";\n+\tconst char *c = \"\";\n+\tif (want_color(s->use_color))\n+\t\tc = s->color_palette[slot];\n \tif (slot == WT_STATUS_ONBRANCH && color_is_nil(c))\n \t\tc = s->color_palette[WT_STATUS_HEADER];\n \treturn c;\n-- \n1.7.6.10.g62f04\n"},{"id":"173724","messageId":"20110818050455.GH2889@sigill.intra.peff.net","threadId":"28148","inReplyTo":"20110818045821.GA17377@sigill.intra.peff.net","subject":"[PATCH 08/10] config: refactor get_colorbool function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T05:04:56Z","receivedAt":"2011-08-18T05:04:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"For \"git config --get-colorbool color.foo\", we use a custom\ncallback that looks not only for the key that the user gave\nus, but also for \"diff.color\" (for backwards compatibility)\nand \"color.ui\" (as a fallback).\n\nFor the former, we use a custom variable to store the\ndiff.color value. For the latter, though, we store it in the\nmain \"git_use_color_default\" variable, turning on color.ui\nfor any other parts of git that respect this value.\n\nIn practice, this doesn't cause any bugs, because git-config\nruns without caring about git_use_color_default, and then\nexits. But it crosses module boundaries in an unusual and\nconfusing way, and it makes refactoring color handling\nharder than it needs to be.\n\nSigned-off-by: Jeff King \n---\n builtin/config.c | 5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 3a09296..0b4ecac 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -305,6 +305,7 @@ static void get_color(const char *def_color)\n \n static int get_colorbool_found;\n static int get_diff_color_found;\n+static int get_color_ui_found;\n static int git_get_colorbool_config(const char *var, const char *value,\n \t\tvoid *cb)\n {\n@@ -313,7 +314,7 @@ static int git_get_colorbool_config(const char *var, const char *value,\n \telse if (!strcmp(var, \"diff.color\"))\n \t\tget_diff_color_found = git_config_colorbool(var, value);\n \telse if (!strcmp(var, \"color.ui\"))\n-\t\tgit_use_color_default = git_config_colorbool(var, value);\n+\t\tget_color_ui_found = git_config_colorbool(var, value);\n \treturn 0;\n }\n \n@@ -327,7 +328,7 @@ static int get_colorbool(int print)\n \t\tif (!strcmp(get_colorbool_slot, \"color.diff\"))\n \t\t\tget_colorbool_found = get_diff_color_found;\n \t\tif (get_colorbool_found < 0)\n-\t\t\tget_colorbool_found = git_use_color_default;\n+\t\t\tget_colorbool_found = get_color_ui_found;\n \t}\n \n \tget_colorbool_found = want_color(get_colorbool_found);\n-- \n1.7.6.10.g62f04\n"},{"id":"173725","messageId":"20110818050506.GI2889@sigill.intra.peff.net","threadId":"28148","inReplyTo":"20110818045821.GA17377@sigill.intra.peff.net","subject":"[PATCH 09/10] diff: don't load color config in plumbing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T05:05:08Z","receivedAt":"2011-08-18T05:05:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The diff config callback is split into two functions: one\nwhich loads \"ui\" config, and one which loads \"basic\" config.\nThe former chains to the latter, as the diff UI config is a\nsuperset of the plumbing config.\n\nThe color.diff variable is only loaded in the UI config.\nHowever, the basic config actually chains to\ngit_color_default_config, which loads color.ui. This doesn't\nactually cause any bugs, because the plumbing diff code does\nnot actually look at the value of color.ui.\n\nHowever, it is somewhat nonsensical, and it makes it\ndifficult to refactor the color code. It probably came about\nbecause there is no git_color_config to load only color\nconfig, but rather just git_color_default_config, which\nloads color config and chains to git_default_config.\n\nThis patch splits out the color-specific portion of\ngit_color_default_config so that the diff UI config can call\nit directly. This is perhaps better explained by the\nchaining of callbacks. Before we had:\n\n git_diff_ui_config\n -> git_diff_basic_config\n -> git_color_default_config\n -> git_default_config\n\nNow we have:\n\n git_diff_ui_config\n -> git_color_config\n -> git_diff_basic_config\n -> git_default_config\n\nSigned-off-by: Jeff King \n---\n color.c | 10 +++++++++-\n color.h | 4 +++-\n diff.c | 5 ++++-\n 3 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/color.c b/color.c\nindex 8586417..ec96fe1 100644\n--- a/color.c\n+++ b/color.c\n@@ -204,13 +204,21 @@ int want_color(int var)\n \treturn var > 0;\n }\n \n-int git_color_default_config(const char *var, const char *value, void *cb)\n+int git_color_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"color.ui\")) {\n \t\tgit_use_color_default = git_config_colorbool(var, value);\n \t\treturn 0;\n \t}\n \n+\treturn 0;\n+}\n+\n+int git_color_default_config(const char *var, const char *value, void *cb)\n+{\n+\tif (git_color_config(var, value, cb) < 0)\n+\t\treturn -1;\n+\n \treturn git_default_config(var, value, cb);\n }\n \ndiff --git a/color.h b/color.h\nindex d715fd5..5949bcd 100644\n--- a/color.h\n+++ b/color.h\n@@ -74,8 +74,10 @@ extern const int column_colors_ansi_max;\n extern int color_stdout_is_tty;\n \n /*\n- * Use this instead of git_default_config if you need the value of color.ui.\n+ * Use the first one if you need only color config; the second is a convenience\n+ * if you are just going to change to git_default_config, too.\n */\n+int git_color_config(const char *var, const char *value, void *cb);\n int git_color_default_config(const char *var, const char *value, void *cb);\n \n int git_config_colorbool(const char *var, const char *value);\ndiff --git a/diff.c b/diff.c\nindex 29cecf1..0a22320 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -164,6 +164,9 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"diff.ignoresubmodules\"))\n \t\thandle_ignore_submodules_arg(&default_diff_options, value);\n \n+\tif (git_color_config(var, value, cb) < 0)\n+\t\treturn -1;\n+\n \treturn git_diff_basic_config(var, value, cb);\n }\n \n@@ -212,7 +215,7 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \tif (!prefixcmp(var, \"submodule.\"))\n \t\treturn parse_submodule_config_option(var, value);\n \n-\treturn git_color_default_config(var, value, cb);\n+\treturn git_default_config(var, value, cb);\n }\n \n static char *quote_two(const char *one, const char *two)\n-- \n1.7.6.10.g62f04\n"},{"id":"173726","messageId":"20110818050533.GJ2889@sigill.intra.peff.net","threadId":"28148","inReplyTo":"20110818045821.GA17377@sigill.intra.peff.net","subject":"[PATCH 10/10] want_color: automatically fallback to color.ui","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T05:05:35Z","receivedAt":"2011-08-18T05:05:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"All of the \"do we want color\" flags default to -1 to\nindicate that we don't have any color configured. This value\nis handled in one of two ways:\n\n 1. In porcelain, we check early on whether the value is\n still -1 after reading the config, and set it to the\n value of color.ui (which defaults to 0).\n\n 2. In plumbing, it stays untouched as -1, and want_color\n defaults it to off.\n\nThis works fine, but means that every porcelain has to check\nand reassign its color flag. Now that want_color gives us a\nplace to put this check in a single spot, we can do that,\nsimplifying the calling code.\n\nSigned-off-by: Jeff King \n---\n builtin/branch.c | 3 ---\n builtin/commit.c | 11 +----------\n builtin/diff.c | 3 ---\n builtin/grep.c | 2 --\n builtin/log.c | 12 ------------\n builtin/merge.c | 4 ----\n builtin/show-branch.c | 3 ---\n color.c | 7 +++++--\n color.h | 5 -----\n 9 files changed, 6 insertions(+), 44 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex d6d3c7d..73d4170 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -673,9 +673,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_branch_config, NULL);\n \n-\tif (branch_use_color == -1)\n-\t\tbranch_use_color = git_use_color_default;\n-\n \ttrack = git_branch_track;\n \n \thead = resolve_ref(\"HEAD\", head_sha1, 0, NULL);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 295803a..9763146 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1237,10 +1237,6 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \n \tif (s.relative_paths)\n \t\ts.prefix = prefix;\n-\tif (s.use_color == -1)\n-\t\ts.use_color = git_use_color_default;\n-\tif (diff_use_color_default == -1)\n-\t\tdiff_use_color_default = git_use_color_default;\n \n \tswitch (status_format) {\n \tcase STATUS_FORMAT_SHORT:\n@@ -1394,15 +1390,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tgit_config(git_commit_config, &s);\n \tdetermine_whence(&s);\n \n-\tif (s.use_color == -1)\n-\t\ts.use_color = git_use_color_default;\n \targc = parse_and_validate_options(argc, argv, builtin_commit_usage,\n \t\t\t\t\t prefix, &s);\n-\tif (dry_run) {\n-\t\tif (diff_use_color_default == -1)\n-\t\t\tdiff_use_color_default = git_use_color_default;\n+\tif (dry_run)\n \t\treturn dry_run_commit(argc, argv, prefix, &s);\n-\t}\n \tindex_file = prepare_index(argc, argv, prefix, 0);\n \n \t/* Set up everything for writing the commit object. This includes\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 69cd5ee..1118689 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -277,9 +277,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tgitmodules_config();\n \tgit_config(git_diff_ui_config, NULL);\n \n-\tif (diff_use_color_default == -1)\n-\t\tdiff_use_color_default = git_use_color_default;\n-\n \tinit_revisions(&rev, prefix);\n \n \t/* If this is a no-index diff, just run it and exit there. */\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex d80db22..2cbf01f 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -896,8 +896,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tstrcpy(opt.color_sep, GIT_COLOR_CYAN);\n \topt.color = -1;\n \tgit_config(grep_config, &opt);\n-\tif (opt.color == -1)\n-\t\topt.color = git_use_color_default;\n \n \t/*\n \t * If there is no -- then the paths must exist in the working\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 5c2af59..d760ee0 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -359,9 +359,6 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_log_config, NULL);\n \n-\tif (diff_use_color_default == -1)\n-\t\tdiff_use_color_default = git_use_color_default;\n-\n \tinit_revisions(&rev, prefix);\n \trev.diff = 1;\n \trev.simplify_history = 0;\n@@ -446,9 +443,6 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_log_config, NULL);\n \n-\tif (diff_use_color_default == -1)\n-\t\tdiff_use_color_default = git_use_color_default;\n-\n \tinit_pathspec(&match_all, NULL);\n \tinit_revisions(&rev, prefix);\n \trev.diff = 1;\n@@ -524,9 +518,6 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_log_config, NULL);\n \n-\tif (diff_use_color_default == -1)\n-\t\tdiff_use_color_default = git_use_color_default;\n-\n \tinit_revisions(&rev, prefix);\n \tinit_reflog_walk(&rev.reflog_info);\n \trev.verbose_header = 1;\n@@ -549,9 +540,6 @@ int cmd_log(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_log_config, NULL);\n \n-\tif (diff_use_color_default == -1)\n-\t\tdiff_use_color_default = git_use_color_default;\n-\n \tinit_revisions(&rev, prefix);\n \trev.always_show_header = 1;\n \tmemset(&opt, 0, sizeof(opt));\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 7209edf..b75ae01 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1031,10 +1031,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_merge_config, NULL);\n \n-\t/* for color.ui */\n-\tif (diff_use_color_default == -1)\n-\t\tdiff_use_color_default = git_use_color_default;\n-\n \tif (branch_mergeoptions)\n \t\tparse_branch_merge_options(branch_mergeoptions);\n \targc = parse_options(argc, argv, prefix, builtin_merge_options,\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 4b726fa..4b480d7 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -685,9 +685,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \n \tgit_config(git_show_branch_config, NULL);\n \n-\tif (showbranch_use_color == -1)\n-\t\tshowbranch_use_color = git_use_color_default;\n-\n \t/* If nothing is specified, try the default first */\n \tif (ac == 1 && default_num) {\n \t\tac = default_num;\ndiff --git a/color.c b/color.c\nindex ec96fe1..e8e2681 100644\n--- a/color.c\n+++ b/color.c\n@@ -1,7 +1,7 @@\n #include \"cache.h\"\n #include \"color.h\"\n \n-int git_use_color_default = 0;\n+static int git_use_color_default = 0;\n int color_stdout_is_tty = -1;\n \n /*\n@@ -196,12 +196,15 @@ int want_color(int var)\n {\n \tstatic int want_auto = -1;\n \n+\tif (var < 0)\n+\t\tvar = git_use_color_default;\n+\n \tif (var == GIT_COLOR_AUTO) {\n \t\tif (want_auto < 0)\n \t\t\twant_auto = check_auto_color();\n \t\treturn want_auto;\n \t}\n-\treturn var > 0;\n+\treturn var;\n }\n \n int git_color_config(const char *var, const char *value, void *cb)\ndiff --git a/color.h b/color.h\nindex 5949bcd..3068a99 100644\n--- a/color.h\n+++ b/color.h\n@@ -58,11 +58,6 @@ struct strbuf;\n #define GIT_COLOR_NEVER 1\n #define GIT_COLOR_AUTO 2\n \n-/*\n- * This variable stores the value of color.ui\n- */\n-extern int git_use_color_default;\n-\n /* A default list of colors to use for commit graphs and show-branch output */\n extern const char *column_colors_ansi[];\n extern const int column_colors_ansi_max;\n-- \n1.7.6.10.g62f04\n"},{"id":"173798","messageId":"7v7h6a8ikn.fsf@alter.siamese.dyndns.org","threadId":"28148","inReplyTo":"20110818050044.GA2889@sigill.intra.peff.net","subject":"Re: [PATCH 01/10] t7006: modernize calls to unset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-18T21:05:41Z","receivedAt":"2011-08-18T21:05:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> These tests break &&-chaining to deal with broken \"unset\"\n> implementations. Instead, they should just use sane_unset.\n\nThanks. I checked with POSIX again, wondering if I should tone the\n\"broken\" down a bit, but it says:\n\n Unsetting a variable or function that was not previously set shall not\n be considered an error...\n\nso they deserve \"broken\" label.\n"},{"id":"173799","messageId":"7v1uwi8ikk.fsf@alter.siamese.dyndns.org","threadId":"28148","inReplyTo":"20110818050114.GB2889@sigill.intra.peff.net","subject":"Re: [PATCH 02/10] test-lib: add helper functions for config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-18T21:10:13Z","receivedAt":"2011-08-18T21:10:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n\n> ...\n> +# Unset a configuration variable, but don't fail if it doesn't exist.\n> +test_unconfig () {\n> +\tgit config --unset-all \"$@\"\n> +\tconfig_status=$?\n> +\tcase \"$config_status\" in\n> +\t5) # ok, nothing to usnet\n> +\t\tconfig_status=0\n> +\t\t;;\n\nWill queue with 's/nothing to usnet/nothing to unset/'.\n"},{"id":"173788","messageId":"20110818215820.GA7767@sigill.intra.peff.net","threadId":"28148","inReplyTo":"20110818045821.GA17377@sigill.intra.peff.net","subject":"Re: [PATCH 0/10] color and pager improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T21:58:20Z","receivedAt":"2011-08-18T21:58:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 17, 2011 at 09:58:23PM -0700, Jeff King wrote:\n\n> While looking at the pager and color code today, I decided to tackle two\n> long-standing bugs, which entailed a lot of refactoring of the color\n> code. The result fixes some minor bugs, and is a little nicer for\n> calling code to use.\n\nAnd here are two patches on top of the previous 10 to help Ingo's\nproblem a bit.\n\n [11/10]: support pager.* for aliases\n [12/10]: support pager.* for external commands\n\nWith these, you can do \"git config pager.stash false\" to turn off paging\non \"stash list\" and \"stash show\" (and turn it back on with \"git -p\nstash list\", of course).\n\nI'm slightly tempted to allow things like \"pager.stash.list\" and\n\"pager.branch.list\". It wouldn't be too hard to implement. But I'm not\nsure anybody actually cares. I think Ingo's original complaint was\nsimply that pager.stash didn't actually do anything, not that he wanted\nsome separate config for the various subcommands.\n\n-Peff\n"},{"id":"173789","messageId":"20110818215909.GA7799@sigill.intra.peff.net","threadId":"28148","inReplyTo":"20110818215820.GA7767@sigill.intra.peff.net","subject":"[PATCH 11/10] support pager.* for aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T21:59:10Z","receivedAt":"2011-08-18T21:59:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Until this patch, doing something like:\n\n git config alias.foo log\n git config pager.foo /some/specific/pager\n\nwould not respect pager.foo at all. With this patch, we\nwill use pager.foo for the \"foo\" alias. We will also\nfallback to pager.log if \"foo\" is a non-shell alias that\nuses the \"log\" command (but any pager.foo overrides\npager.log).\n\nSigned-off-by: Jeff King \n---\n git.c | 3 +++\n t/t7006-pager.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 34 insertions(+), 0 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 8828c18..375e9b2 100644\n--- a/git.c\n+++ b/git.c\n@@ -180,6 +180,9 @@ static int handle_alias(int *argcp, const char ***argv)\n \talias_command = (*argv)[0];\n \talias_string = alias_lookup(alias_command);\n \tif (alias_string) {\n+\t\tif (use_pager == -1)\n+\t\t\tuse_pager = check_pager_config(alias_command);\n+\n \t\tif (alias_string[0] == '!') {\n \t\t\tconst char **alias_argv;\n \t\t\tint argc = *argcp, i;\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 4582336..a8c6e85 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -450,4 +450,35 @@ test_expect_success TTY 'command-specific pager overridden by environment' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success TTY 'command-specific pager works for aliases' '\n+\tsane_unset PAGER GIT_PAGER &&\n+\techo \"foo:initial\" >expect &&\n+\t>actual &&\n+\ttest_config alias.aliaslog \"log --format=%s\" &&\n+\ttest_config pager.aliaslog \"sed s/^/foo:/ >actual\" &&\n+\ttest_terminal git aliaslog -1 &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success TTY 'non-shell alias falls back to command pager config' '\n+\tsane_unset PAGER GIT_PAGER &&\n+\techo \"foo:initial\" >expect &&\n+\t>actual &&\n+\ttest_config alias.aliaslog \"log --format=%s\" &&\n+\ttest_config pager.log \"sed s/^/foo:/ >actual\" &&\n+\ttest_terminal git aliaslog -1 &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success TTY 'alias-specific pager can override aliased command' '\n+\tsane_unset PAGER GIT_PAGER &&\n+\t>expect &&\n+\t>actual &&\n+\ttest_config alias.aliaslog \"log --format=%s\" &&\n+\ttest_config pager.log \"sed s/^/log:/ >actual\" &&\n+\ttest_config pager.aliaslog false &&\n+\ttest_terminal git aliaslog -1 &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.6.10.g62f04\n"},{"id":"173800","messageId":"7vvctu7402.fsf@alter.siamese.dyndns.org","threadId":"28148","inReplyTo":"20110818050421.GG2889@sigill.intra.peff.net","subject":"Re: [PATCH 07/10] color: delay auto-color decision until point of use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-18T21:59:37Z","receivedAt":"2011-08-18T21:59:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/color.h b/color.h\n> index a190a25..d715fd5 100644\n> --- a/color.h\n> +++ b/color.h\n> @@ -49,6 +49,16 @@ struct strbuf;\n> #define GIT_COLOR_NIL \"NIL\"\n> \n> /*\n> + * The first three are chosen to match common usage in the code, and what is\n> + * returned from git_config_colorbool. The \"auto\" value can be returned from\n> + * config_colorbool, and will be converted by want_color() into either 0 or 1.\n> + */\n> +#define GIT_COLOR_UNKNOWN -1\n> +#define GIT_COLOR_ALWAYS 0\n> +#define GIT_COLOR_NEVER 1\n> +#define GIT_COLOR_AUTO 2\n\nThe ALWAYS/NEVER somehow go against my intuition. Let me trace one\ncodepath starting from git_branch_config().\n\n branch_use_color is set from git_config_colorbool(\"color.branch\");\n -> given \"never\", git_config_colorbool() returns 0;\n branch_get_color() asks want_color(branch_use_color);\n -> want_color() returns if the given value is positive.\n\nBecause git_config_colorbool() does not use the above symbolic constants,\neverything goes well, but aren't these two swapped?\n"},{"id":"173790","messageId":"20110818215953.GA68667@sherwood.local","threadId":"28148","inReplyTo":"20110818045821.GA17377@sigill.intra.peff.net","subject":"Re: [PATCH 0/10] color and pager improvements","fromName":"Steffen Daode Nurpmeso","fromEmail":"sdaoden@googlemail.com","sentAt":"2011-08-18T21:59:53Z","receivedAt":"2011-08-18T21:59:53Z","isPatch":true,"sender":{"key":"sdaoden@googlemail.com","avatar":null},"body":"@ Jeff King wrote (2011-08-18 06:58+0200):\n> These three fix the problem Steffen mentioned here:\n\nUuuh, such a shame - you know that it was first noted by\nBenjamin Kudria (2008-07-23,\nhttp://marc.info/?l=git&m=121677902502581&w=2).\nAnd it was you who tried to resurrect the same issue last year.\n(The thing is: i did not search the archive first because it was\nclearly a bug. I did once you referred to your older patch.)\nBut great that you actually found the time to fix it!\n\n(I must admit though that i'm currently addicted to the coloured\noutput, because simply switching between my terms gives a clear\nindication of where i'm currently git(1)ing. :-> And that in\nturn is something which gives more and more fun the longer i use\nit! It is *really* fantastic once you get used to it. And do\ngc --aggressive and all your temporary fooling is cleaned up.)\n\nNow it's pretty unfortunate that i cannot offer fixes for\nanything.\n\nI have a dumb patch of 'rebase -i' which includes the TODO entry\nline ($rest) as a comment in the commit message, which is pretty\nuseful because i think about the rebase task and can store\ncomments in that very line. But it introduces commit\n--cleanup=strip and patches commit.c to add a --message-prefix\noption. This is no good yet.\n\nMichael J Gruber's today's shocking exercise on the german\nkeyboard layout - maybe i should really resurrect parts of that\nNBSP series?\n\nAnd referring to one sentence of yours from the past: no, refspec\nstuff *is* that hard: they are not a tree which is created via\n'refs_build_tree(); refs_merge_command_line();' upon program\nstart, with pointers to maybe instantiated .. whatever.\n/*\n * Note. This is used only by \"push\"; refspec matching rules for\n * push and fetch are subtly different, so do not try to reuse it\n * without thinking.\n */\nI gave up once i found that (in remote.c). (AFAIR it seems\nrefspecs are first build L->R, then pushed, then build again but\nin R->L direction. Which is why without a fetch= the remotes/\nref is not updated after a push. AFAIK - i gave up ...)\n\nBut i'm looking forward and really hope to be able to contribute\nsome useful and good stuff to great projects in the future.\nOpenBSD, for example. :-)\n\n--Steffen\nCiao, sdaoden(*)(gmail.com)\nASCII ribbon campaign ( ) More nuclear fission plants\n against HTML e-mail X can serve more coloured\n and proprietary attachments / \\ and sounding animations\n"},{"id":"173792","messageId":"20110818220132.GB7799@sigill.intra.peff.net","threadId":"28148","inReplyTo":"20110818215820.GA7767@sigill.intra.peff.net","subject":"[PATCH 12/10] support pager.* for external commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T22:01:32Z","receivedAt":"2011-08-18T22:01:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Without this patch, any commands that are not builtin would\nnot respect pager.* config. For example:\n\n git config pager.stash false\n git stash list\n\nwould still use a pager. With this patch, pager.stash now\nhas an effect. If it is not specified, we will still fall\nback to pager.log when we invoke \"log\" from \"stash list\".\n\nSigned-off-by: Jeff King \n---\nI think we didn't do this in the original pager.* patches because of\ninitialization order problems. It was dangerous to look at config too\nearly in the process, or something similar; I don't recall the exact\nproblems. But since work from Jonathan and Duy last summer, I think some\nof those issues have gone away. At least I couldn't find any problems.\nAnd I have been running with this patch since last November and haven't\nnoticed anything odd.\n\n git.c | 2 ++\n t/t7006-pager.sh | 36 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 38 insertions(+), 0 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 375e9b2..e8cff60 100644\n--- a/git.c\n+++ b/git.c\n@@ -462,6 +462,8 @@ static void execv_dashed_external(const char **argv)\n \tconst char *tmp;\n \tint status;\n \n+\tif (use_pager == -1)\n+\t\tuse_pager = check_pager_config(argv[0]);\n \tcommit_pager_choice();\n \n \tstrbuf_addf(&cmd, \"git-%s\", argv[0]);\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex a8c6e85..742238c 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -481,4 +481,40 @@ test_expect_success TTY 'alias-specific pager can override aliased command' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup external command' '\n+\tcat >git-external <<-\\EOF &&\n+\t#!/bin/sh\n+\tgit \"$@\"\n+\tEOF\n+\tchmod +x git-external\n+'\n+\n+test_expect_success TTY 'command-specific pager works for external commands' '\n+\tsane_unset PAGER GIT_PAGER &&\n+\techo \"foo:initial\" >expect &&\n+\t>actual &&\n+\ttest_config pager.external \"sed s/^/foo:/ >actual\" &&\n+\ttest_terminal git --exec-path=\"`pwd`\" external log --format=%s -1 &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success TTY 'sub-commands of externals use their own pager' '\n+\tsane_unset PAGER GIT_PAGER &&\n+\techo \"foo:initial\" >expect &&\n+\t>actual &&\n+\ttest_config pager.log \"sed s/^/foo:/ >actual\" &&\n+\ttest_terminal git --exec-path=. external log --format=%s -1 &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success TTY 'external command pagers override sub-commands' '\n+\tsane_unset PAGER GIT_PAGER &&\n+\t>expect &&\n+\t>actual &&\n+\ttest_config pager.external false &&\n+\ttest_config pager.log \"sed s/^/log:/ >actual\" &&\n+\ttest_terminal git --exec-path=. external log --format=%s -1 &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.6.10.g62f04\n"},{"id":"173801","messageId":"7vpqk27400.fsf@alter.siamese.dyndns.org","threadId":"28148","inReplyTo":"20110818045821.GA17377@sigill.intra.peff.net","subject":"Re: [PATCH 0/10] color and pager improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-18T22:02:34Z","receivedAt":"2011-08-18T22:02:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicely done.\n"},{"id":"173802","messageId":"20110818222817.GA8668@sigill.intra.peff.net","threadId":"28148","inReplyTo":"7vvctu7402.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 07/10] color: delay auto-color decision until point of use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T22:28:18Z","receivedAt":"2011-08-18T22:28:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 18, 2011 at 02:59:37PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > diff --git a/color.h b/color.h\n> > index a190a25..d715fd5 100644\n> > --- a/color.h\n> > +++ b/color.h\n> > @@ -49,6 +49,16 @@ struct strbuf;\n> > #define GIT_COLOR_NIL \"NIL\"\n> > \n> > /*\n> > + * The first three are chosen to match common usage in the code, and what is\n> > + * returned from git_config_colorbool. The \"auto\" value can be returned from\n> > + * config_colorbool, and will be converted by want_color() into either 0 or 1.\n> > + */\n> > +#define GIT_COLOR_UNKNOWN -1\n> > +#define GIT_COLOR_ALWAYS 0\n> > +#define GIT_COLOR_NEVER 1\n> > +#define GIT_COLOR_AUTO 2\n> \n> The ALWAYS/NEVER somehow go against my intuition. Let me trace one\n> codepath starting from git_branch_config().\n> \n> branch_use_color is set from git_config_colorbool(\"color.branch\");\n> -> given \"never\", git_config_colorbool() returns 0;\n> branch_get_color() asks want_color(branch_use_color);\n> -> want_color() returns if the given value is positive.\n> \n> Because git_config_colorbool() does not use the above symbolic constants,\n> everything goes well, but aren't these two swapped?\n\nOooops. Yes, they are completely swapped and I'm an idiot. But as you\nnoticed, we don't actually _use_ them anywhere. I started on replacing\nevery \"0\" with NEVER, every \"1\" with ALWAYS, and every \"-1\" with\nUNKNOWN. But it really bloated the patch, and didn't actually make the\ncode any more readable.\n\nThe only symbolic constant that is really necessary is the AUTO one. I\njust felt odd randomly defining \"2\" as GIT_COLOR_AUTO, but not defining\nthe other possible values of the enumeration. So definitely they should\nbe swapped. I'm also fine with just dropping all of them except AUTO.\n\n-Peff\n"},{"id":"173806","messageId":"4e4d94bb.00b9e5c4.bm000@wupperonline.de","threadId":"28148","inReplyTo":"20110818215820.GA7767@sigill.intra.peff.net","subject":"Re: [PATCH 0/10] color and pager improvements","fromName":"Ingo Brückl","fromEmail":"ib@wupperonline.de","sentAt":"2011-08-18T22:33:01Z","receivedAt":"2011-08-18T22:33:01Z","isPatch":true,"sender":{"key":"ib@wupperonline.de","avatar":"https://avatars.githubusercontent.com/u/123327?v=4"},"body":"Jeff King wrote on Thu, 18 Aug 2011 14:58:20 -0700:\n\n> I think Ingo's original complaint was simply that pager.stash didn't\n> actually do anything, not that he wanted some separate config for the\n> various subcommands.\n\nNo, you're wrong.\n\nMy goal was to be able to turn off paging for \"stash list\" only while all\nother stash commands should continue paging.\n\nIt is, of course, very usefull to be able to control paging for external\ncommands and aliases, but in my case I originally wanted to control a\nspecific subcommand.\n\nIngo\n"},{"id":"173808","messageId":"20110818224644.GC8481@sigill.intra.peff.net","threadId":"28148","inReplyTo":"4e4d94bb.00b9e5c4.bm000@wupperonline.de","subject":"Re: [PATCH 0/10] color and pager improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T22:46:44Z","receivedAt":"2011-08-18T22:46:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 19, 2011 at 12:33:01AM +0200, Ingo Brückl wrote:\n\n> Jeff King wrote on Thu, 18 Aug 2011 14:58:20 -0700:\n> \n> > I think Ingo's original complaint was simply that pager.stash didn't\n> > actually do anything, not that he wanted some separate config for the\n> > various subcommands.\n> \n> No, you're wrong.\n> \n> My goal was to be able to turn off paging for \"stash list\" only while all\n> other stash commands should continue paging.\n\nAh, OK. I think the only other stash command that pages is \"stash show\",\nbut I don't think it's unreasonable to want paging for that but not for\n\"list\".\n\nI'll take a look at implementing something for that. Though I'll be\ntraveling for the next few days, so it probably won't be for a bit.\n\n-Peff\n"},{"id":"173810","messageId":"7v8vqq72kp.fsf@alter.siamese.dyndns.org","threadId":"28148","inReplyTo":"20110818215909.GA7799@sigill.intra.peff.net","subject":"Re: [PATCH 11/10] support pager.* for aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-18T22:54:46Z","receivedAt":"2011-08-18T22:54:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Until this patch, doing something like:\n>\n> git config alias.foo log\n> git config pager.foo /some/specific/pager\n>\n> would not respect pager.foo at all.\n\nIs it a good thing? Looks too confusing and I am having a hard time to\ndecide if this is \"just because we could\" or \"because we need to be able\nto do this for such and such reasons\".\n"},{"id":"173811","messageId":"7v39gy72ie.fsf@alter.siamese.dyndns.org","threadId":"28148","inReplyTo":"20110818220132.GB7799@sigill.intra.peff.net","subject":"Re: [PATCH 12/10] support pager.* for external commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-18T22:56:09Z","receivedAt":"2011-08-18T22:56:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Without this patch, any commands that are not builtin would\n> not respect pager.* config. For example:\n>\n> git config pager.stash false\n> git stash list\n>\n> would still use a pager.\n\nUnlike the [11/10] patch, I can see why this is a good change.\nThanks.\n"},{"id":"173826","messageId":"20110819033733.GB2993@sigill.intra.peff.net","threadId":"28148","inReplyTo":"7v8vqq72kp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 11/10] support pager.* for aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-19T03:37:34Z","receivedAt":"2011-08-19T03:37:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 18, 2011 at 03:54:46PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Until this patch, doing something like:\n> >\n> > git config alias.foo log\n> > git config pager.foo /some/specific/pager\n> >\n> > would not respect pager.foo at all.\n> \n> Is it a good thing? Looks too confusing and I am having a hard time to\n> decide if this is \"just because we could\" or \"because we need to be able\n> to do this for such and such reasons\".\n\nI don't have a particular use for it myself. However, I don't see what's\nconfusing about it. Would would you expect the above commands to do with\nrespect to paging? I think the behavior after my patch does what users\nwill expect, whether they have configured pager.foo, pager.log, or\nnothing.\n\n-Peff\n"},{"id":"173828","messageId":"7vliuq5906.fsf@alter.siamese.dyndns.org","threadId":"28148","inReplyTo":"20110819033733.GB2993@sigill.intra.peff.net","subject":"Re: [PATCH 11/10] support pager.* for aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-19T04:18:49Z","receivedAt":"2011-08-19T04:18:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > Until this patch, doing something like:\n>> >\n>> > git config alias.foo log\n>> > git config pager.foo /some/specific/pager\n>> >\n>> > would not respect pager.foo at all.\n>> \n>> Is it a good thing? Looks too confusing and I am having a hard time to\n>> decide if this is \"just because we could\" or \"because we need to be able\n>> to do this for such and such reasons\".\n>\n> I don't have a particular use for it myself. However, I don't see what's\n> confusing about it. Would would you expect the above commands to do with\n> respect to paging?\n\nThe reason I found it confusing was that I expected the \"log\" command that\nis run as the expansion of the alias to be oblivious to the fact that the\nend user called it \"foo\", and ignore anything specific to \"foo\", including\n\"pager.foo\".\n"},{"id":"173830","messageId":"20110819044013.GA2163@sigill.intra.peff.net","threadId":"28148","inReplyTo":"7vliuq5906.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 11/10] support pager.* for aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-19T04:40:14Z","receivedAt":"2011-08-19T04:40:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 18, 2011 at 09:18:49PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> > Until this patch, doing something like:\n> >> >\n> >> > git config alias.foo log\n> >> > git config pager.foo /some/specific/pager\n> >> >\n> >> > would not respect pager.foo at all.\n> >> \n> >> Is it a good thing? Looks too confusing and I am having a hard time to\n> >> decide if this is \"just because we could\" or \"because we need to be able\n> >> to do this for such and such reasons\".\n> >\n> > I don't have a particular use for it myself. However, I don't see what's\n> > confusing about it. Would would you expect the above commands to do with\n> > respect to paging?\n> \n> The reason I found it confusing was that I expected the \"log\" command that\n> is run as the expansion of the alias to be oblivious to the fact that the\n> end user called it \"foo\", and ignore anything specific to \"foo\", including\n> \"pager.foo\".\n\nI think of it this way:\n\nIf the user thinks of the alias as just another form of \"log\", then we\ndo the right thing: we use log's pager config by default, and respect\npager.log. They never set pager.foo, because that is nonsensical in\ntheir mental model.\n\nIf the user thinks of the alias as its own command, then they would\nexpect pager.foo to work. And it does what they expect.\n\nBut like I said, I don't personally plan on using this. It was just the\nonly semantics that really made sense to me, and I noticed it because of\nworking on externals. And clearly it's not a lot of code.\n\n-Peff\n"},{"id":"173834","messageId":"7vei0i560a.fsf@alter.siamese.dyndns.org","threadId":"28148","inReplyTo":"20110819044013.GA2163@sigill.intra.peff.net","subject":"Re: [PATCH 11/10] support pager.* for aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-19T05:23:33Z","receivedAt":"2011-08-19T05:23:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Aug 18, 2011 at 09:18:49PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> >> > Until this patch, doing something like:\n>> >> >\n>> >> > git config alias.foo log\n>> >> > git config pager.foo /some/specific/pager\n>> >> >\n>> >> > would not respect pager.foo at all.\n>> >> \n>> >> Is it a good thing? Looks too confusing and I am having a hard time to\n>> >> decide if this is \"just because we could\" or \"because we need to be able\n>> >> to do this for such and such reasons\".\n>> >\n>> > I don't have a particular use for it myself. However, I don't see what's\n>> > confusing about it. Would would you expect the above commands to do with\n>> > respect to paging?\n>> \n>> The reason I found it confusing was that I expected the \"log\" command that\n>> is run as the expansion of the alias to be oblivious to the fact that the\n>> end user called it \"foo\", and ignore anything specific to \"foo\", including\n>> \"pager.foo\".\n>\n> I think of it this way:\n>\n> If the user thinks of the alias as just another form of \"log\", then we\n> do the right thing: we use log's pager config by default, and respect\n> pager.log. They never set pager.foo, because that is nonsensical in\n> their mental model.\n>\n> If the user thinks of the alias as its own command, then they would\n> expect pager.foo to work. And it does what they expect.\n>\n> But like I said, I don't personally plan on using this. It was just the\n> only semantics that really made sense to me,...\n\nI can see that argument, but once you start paying attention to \"*.foo\",\nyou have to keep supporting that forever, and also more importantly, you\nneed to worry about interactions between \"*.foo\" vs \"*.log\". Which one\nshould win? Should they combine if both are defined? My \"looks confusing\"\nincludes that can of worms.\n"},{"id":"173835","messageId":"7vaab6552a.fsf@alter.siamese.dyndns.org","threadId":"28148","inReplyTo":"7vei0i560a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 11/10] support pager.* for aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-19T05:43:57Z","receivedAt":"2011-08-19T05:43:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> If the user thinks of the alias as just another form of \"log\", then we\n>> do the right thing: we use log's pager config by default, and respect\n>> pager.log. They never set pager.foo, because that is nonsensical in\n>> their mental model.\n>>\n>> If the user thinks of the alias as its own command, then they would\n>> expect pager.foo to work. And it does what they expect.\n>>\n>> But like I said, I don't personally plan on using this. It was just the\n>> only semantics that really made sense to me,...\n>\n> I can see that argument, but once you start paying attention to \"*.foo\",\n> you have to keep supporting that forever, and also more importantly, you\n> need to worry about interactions between \"*.foo\" vs \"*.log\". Which one\n> should win? Should they combine if both are defined? My \"looks confusing\"\n> includes that can of worms.\n\nActually there is another thing that I think is much worse. If the user is\ntrained to think of the alias as its own command by seeing pager.foo to\nwork as you described, you cannot blame them if they would also expect\nthese to work, in the sense that only \"foo.*\" and \"bar.*\" respectively\nwould take effect, and they would override \"log.*\":\n\n\t[alias]\n \tfoo = log\n\t\tbar = !sh -c 'log \"$@\"' -\n\t[log]\n\t\tdate = default\n\t[foo]\n \tdate = iso\n\t[bar]\n\t\tdate = relative\n\nI do not think we want to go there.\n"},{"id":"173840","messageId":"4e4e03fb.6d8e455c.bm000@wupperonline.de","threadId":"28148","inReplyTo":"20110818224644.GC8481@sigill.intra.peff.net","subject":"Re: [PATCH 0/10] color and pager improvements","fromName":"Ingo Brückl","fromEmail":"ib@wupperonline.de","sentAt":"2011-08-19T06:34:13Z","receivedAt":"2011-08-19T06:34:13Z","isPatch":true,"sender":{"key":"ib@wupperonline.de","avatar":"https://avatars.githubusercontent.com/u/123327?v=4"},"body":"Jeff King wrote on Thu, 18 Aug 2011 15:46:44 -0700:\n\n> On Fri, Aug 19, 2011 at 12:33:01AM +0200, Ingo Brückl wrote:\n\n>> My goal was to be able to turn off paging for \"stash list\" only while all\n>> other stash commands should continue paging.\n\n> Ah, OK. I think the only other stash command that pages is \"stash show\",\n> but I don't think it's unreasonable to want paging for that but not for\n> \"list\".\n\nMaybe \"stash list\" simply should - like other commands - not paginate by\ndefault.\n\nIngo\n"},{"id":"173841","messageId":"20110819083001.GA2618@sigill.intra.peff.net","threadId":"28148","inReplyTo":"7vaab6552a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 11/10] support pager.* for aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-19T08:30:02Z","receivedAt":"2011-08-19T08:30:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 18, 2011 at 10:23:33PM -0700, Junio C Hamano wrote:\n\n> > I think of it this way:\n> >\n> > If the user thinks of the alias as just another form of \"log\", then we\n> > do the right thing: we use log's pager config by default, and respect\n> > pager.log. They never set pager.foo, because that is nonsensical in\n> > their mental model.\n> >\n> > If the user thinks of the alias as its own command, then they would\n> > expect pager.foo to work. And it does what they expect.\n> >\n> > But like I said, I don't personally plan on using this. It was just the\n> > only semantics that really made sense to me,...\n> \n> I can see that argument, but once you start paying attention to \"*.foo\",\n> you have to keep supporting that forever, and also more importantly, you\n> need to worry about interactions between \"*.foo\" vs \"*.log\". Which one\n> should win? Should they combine if both are defined? My \"looks confusing\"\n> includes that can of worms.\n\nIt seems obvious to me that the more-specific *.foo form would take\nprecedence over the *.log form, and anything else would be crazy. But\nthat is just my gut feeling. If you are confused or worried, then that\nis enough for me to say it is not worth pursuing. It's not a feature I\nreally care about; it was more about fixing something that looked\nobviously wrong while I was in the area.\n\n-Peff\n"},{"id":"174259","messageId":"20110825202512.GD6165@sigill.intra.peff.net","threadId":"28148","inReplyTo":"4e4e03fb.6d8e455c.bm000@wupperonline.de","subject":"Re: [PATCH 0/10] color and pager improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-25T20:25:12Z","receivedAt":"2011-08-25T20:25:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 19, 2011 at 08:34:13AM +0200, Ingo Brückl wrote:\n\n> Jeff King wrote on Thu, 18 Aug 2011 15:46:44 -0700:\n> \n> > On Fri, Aug 19, 2011 at 12:33:01AM +0200, Ingo Brückl wrote:\n> \n> >> My goal was to be able to turn off paging for \"stash list\" only while all\n> >> other stash commands should continue paging.\n> \n> > Ah, OK. I think the only other stash command that pages is \"stash show\",\n> > but I don't think it's unreasonable to want paging for that but not for\n> > \"list\".\n> \n> Maybe \"stash list\" simply should - like other commands - not paginate by\n> default.\n\nI have no real opinion on that. It only paginates as a side effect of\ncalling log.\n\nI do think \"git stash show\" paginating by default is probably helpful,\nthough.\n\nThe best way to get people's attention is probably to post a patch\nadding --no-pager to the git-log invocation of \"git stash list\". :)\n\n-Peff\n"},{"id":"174800","messageId":"alpine.DEB.2.00.1109032212310.12564@debian","threadId":"28148","inReplyTo":"20110818050533.GJ2889@sigill.intra.peff.net","subject":"Re: [PATCH 10/10] want_color: automatically fallback to color.ui","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-09-04T02:36:01Z","receivedAt":"2011-09-04T02:36:01Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"Hi Jeff,\n\nThis patch makes format-patch output color escape codes to file when\nrun with color.ui=always. Before the patch it did not do that. The\ndocumentation for color.ui says \"Set it to always if you want all\noutput not intended for machine consumption to use color\". Is\nformat-patch \"intended for machine consumption\" or not?\n\nI'm not sure why I had the parameter set to \"always\" instead of\n\"true/auto\" and maybe I should change it, but since this patch changes\nthe behavior, I thought I should let you know (it was not mentioned in\nthe commit message, so I'm not sure it was intentional).\n\n\nMartin\n"},{"id":"174808","messageId":"20110904125312.GA21724@sigill.intra.peff.net","threadId":"28148","inReplyTo":"alpine.DEB.2.00.1109032212310.12564@debian","subject":"Re: [PATCH 10/10] want_color: automatically fallback to color.ui","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-04T12:53:12Z","receivedAt":"2011-09-04T12:53:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 03, 2011 at 10:36:01PM -0400, Martin von Zweigbergk wrote:\n\n> This patch makes format-patch output color escape codes to file when\n> run with color.ui=always. Before the patch it did not do that. The\n> documentation for color.ui says \"Set it to always if you want all\n> output not intended for machine consumption to use color\". Is\n> format-patch \"intended for machine consumption\" or not?\n\nSorry, this is a regression. The old behavior was that commands had to\ncopy color.ui's value manually into diff_use_color_default. With my\npatch, the value is picked up automatically, and it is up to commands to\nload or not load the specified config.\n\nFor diff plumbing versus porcelain, we have separate config code paths\n(diff_basic versus diff_ui). I forgot that format-patch versus log has\nthe same situation, and needs the same split.\n\nIt's a long weekend here in the US, but I'll try to get a patch out on\nTuesday (and also check for any other similar regressions).\n\n> I'm not sure why I had the parameter set to \"always\" instead of\n> \"true/auto\" and maybe I should change it, but since this patch changes\n> the behavior, I thought I should let you know (it was not mentioned in\n> the commit message, so I'm not sure it was intentional).\n\nYeah, I don't think setting color.ui to \"always\" is all that useful. But\nthis behavior change was definitely not intentional. Thanks for\nnoticing.\n\n-Peff\n"},{"id":"174863","messageId":"20110905113158.GA1842@sherwood.local","threadId":"28148","inReplyTo":"20110904125312.GA21724@sigill.intra.peff.net","subject":"Re: [PATCH 10/10] want_color: automatically fallback to color.ui","fromName":"Steffen Daode Nurpmeso","fromEmail":"sdaoden@googlemail.com","sentAt":"2011-09-05T11:31:58Z","receivedAt":"2011-09-05T11:31:58Z","isPatch":true,"sender":{"key":"sdaoden@googlemail.com","avatar":null},"body":"@ Jeff King wrote (2011-09-04 14:53+0200):\n> Sorry, this is a regression.\n\nI should have found that.\nI apologize.\n\n--Steffen\nCiao, sdaoden(*)(gmail.com)\nASCII ribbon campaign ( ) More nuclear fission plants\n against HTML e-mail X can serve more coloured\n and proprietary attachments / \\ and sounding animations\n"},{"id":"184495","messageId":"CACBZZX596wnk2KE9QzUPMc=A6Mt8HbUs7F4rnAZbw1_RrcKHnw@mail.gmail.com","threadId":"28148","inReplyTo":"20110818220132.GB7799@sigill.intra.peff.net","subject":"Re: [PATCH 12/10] support pager.* for external commands","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2012-02-12T00:46:34Z","receivedAt":"2012-02-12T00:46:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Aug 19, 2011 at 00:01, Jeff King wrote:\n\n> +test_expect_success TTY 'command-specific pager works for external commands' '\n> +       sane_unset PAGER GIT_PAGER &&\n> +       echo \"foo:initial\" >expect &&\n> +       >actual &&\n> +       test_config pager.external \"sed s/^/foo:/ >actual\" &&\n> +       test_terminal git --exec-path=\"`pwd`\" external log --format=%s -1 &&\n> +       test_cmp expect actual\n\nFor reasons that I haven't looked into using sed like that breaks\nunder /usr/bin/ksh on Solaris. Just using:\n\n sed -e \\\"s/^/foo:/\\\"\n\nInstead fixes it, it's not broken with /usr/xpg4/bin/sh, so it's some\nksh peculiarity.\n\nThe error it gives is:\n\n sed s/^/foo:/ >actual: Not found\n\nIndicating that for some reason it's considering that whole \"sed\ns/^/foo:/ >actual\" string to be a single command.\n"},{"id":"184671","messageId":"20120214191340.GC12072@sigill.intra.peff.net","threadId":"28148","inReplyTo":"CACBZZX596wnk2KE9QzUPMc=A6Mt8HbUs7F4rnAZbw1_RrcKHnw@mail.gmail.com","subject":"Re: [PATCH 12/10] support pager.* for external commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-14T19:13:40Z","receivedAt":"2012-02-14T19:13:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 12, 2012 at 01:46:34AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> On Fri, Aug 19, 2011 at 00:01, Jeff King wrote:\n> \n> > +test_expect_success TTY 'command-specific pager works for external commands' '\n> > +       sane_unset PAGER GIT_PAGER &&\n> > +       echo \"foo:initial\" >expect &&\n> > +       >actual &&\n> > +       test_config pager.external \"sed s/^/foo:/ >actual\" &&\n> > +       test_terminal git --exec-path=\"`pwd`\" external log --format=%s -1 &&\n> > +       test_cmp expect actual\n> \n> For reasons that I haven't looked into using sed like that breaks\n> under /usr/bin/ksh on Solaris. Just using:\n> \n> sed -e \\\"s/^/foo:/\\\"\n> \n> Instead fixes it, it's not broken with /usr/xpg4/bin/sh, so it's some\n> ksh peculiarity.\n> \n> The error it gives is:\n> \n> sed s/^/foo:/ >actual: Not found\n> \n> Indicating that for some reason it's considering that whole \"sed\n> s/^/foo:/ >actual\" string to be a single command.\n\nHrm. Is the problem on the git-executing side, or is it on the setting\nup the config side?\n\nSadly (or perhaps not) I no longer have any Solaris machines to test on.\nCan you confirm that \"git config pager.external\" looks OK inside that\ntest? Can you confirm via GIT_TRACE=1 what is being sent to the shell?\n\nAlso, it looks like we actually run commands internally from git using\n\"sh -c\". So if it is the executing side that is wrong, I don't see how\n/usr/bin/ksh would be involved at all (it would either be /bin/sh, or\n/usr/xpg4/bin/sh if you have your PATH set).\n\n-Peff\n"}]}