{"thread":{"id":"46375","subject":"[PATCH 0/15] making user-format colors conditional on config/tty","startedAt":"2017-07-13T14:56:06Z","lastAt":"2017-07-14T11:50:56Z","messageCount":30,"participants":["Jeff King","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":15},"messages":[{"id":"324351","messageId":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","threadId":"46375","inReplyTo":null,"subject":"[PATCH 0/15] making user-format colors conditional on config/tty","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T14:55:53Z","receivedAt":"2017-07-13T14:56:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a cleanup of the patch I posted last October:\n\n https://public-inbox.org/git/20161010151517.6wszhuyp57yfncaj@sigill.intra.peff.net/\n\nThe general idea is that it's rather confusing that \"%C(red)\" in a\npretty-print format does not currently respect color.ui, --no-color, or\nthe usual isatty check on stdout. This series changes that. Note that\nthis is a backwards-incompatible change, but the general sentiment in\nthat earlier thread seemed to be that the existing behavior is arguably\nbuggy. See patch 14 for more discussion.\n\nThe patch stalled back then because I wanted to make sure that\nref-filter's color placeholders behaved the same. That required some\nrefactoring which conflicted badly with kn/ref-filter-branch-list. Now\nthat it has graduated, I was able to rebase on top.\n\nThis version also takes into account feedback from the original thread.\nAnd as I added tests, it surfaced a few corner cases around color config\nthat I've dealt with here. The last two patches are the most\ninteresting bits.\n\n [01/15]: check return value of verify_ref_format()\n [02/15]: docs/for-each-ref: update pointer to color syntax\n [03/15]: t: use test_decode_color rather than literal ANSI codes\n [04/15]: ref-filter: simplify automatic color reset\n [05/15]: ref-filter: abstract ref format into its own struct\n [06/15]: ref-filter: move need_color_reset_at_eol into ref_format\n [07/15]: ref-filter: provide a function for parsing sort options\n [08/15]: ref-filter: make parse_ref_filter_atom a private function\n [09/15]: ref-filter: factor out the parsing of sorting atoms\n [10/15]: ref-filter: pass ref_format struct to atom parsers\n [11/15]: color: check color.ui in git_default_config()\n [12/15]: for-each-ref: load config earlier\n [13/15]: rev-list: pass diffopt->use_colors through to pretty-print\n [14/15]: pretty: respect color settings for %C placeholders\n [15/15]: ref-filter: consult want_color() before emitting colors\n\n Documentation/git-for-each-ref.txt | 6 +-\n Documentation/pretty-formats.txt | 18 ++++--\n builtin/branch.c | 21 +++---\n builtin/clean.c | 3 +-\n builtin/for-each-ref.c | 27 ++++----\n builtin/grep.c | 2 +-\n builtin/rev-list.c | 1 +\n builtin/show-branch.c | 2 +-\n builtin/tag.c | 61 ++++++------------\n builtin/verify-tag.c | 14 ++--\n color.c | 8 ---\n config.c | 4 ++\n diff.c | 3 -\n pretty.c | 27 ++++++--\n ref-filter.c | 108 ++++++++++++++++++-------------\n ref-filter.h | 30 +++++++--\n t/t3203-branch-output.sh | 31 +++++++++\n t/t4207-log-decoration-colors.sh | 22 +++----\n t/t6006-rev-list-format.sh | 129 ++++++++++++++++++++++++-------------\n t/t6300-for-each-ref.sh | 39 +++++++----\n t/t7004-tag.sh | 25 +++++++\n t/test-lib-functions.sh | 1 +\n 22 files changed, 362 insertions(+), 220 deletions(-)\n\n-Peff\n"},{"id":"324352","messageId":"20170713145610.gpomty7guf54lmsb@sigill.intra.peff.net","threadId":"46375","inReplyTo":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","subject":"[PATCH 01/15] check return value of verify_ref_format()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T14:56:10Z","receivedAt":"2017-07-13T14:56:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Users of the ref-filter code must call verify_ref_format()\nbefore formatting any refs, but most ignore its return\nvalue. This means we may print an error on a syntactically\nbogus pattern, but keep going anyway.\n\nIn most cases this results in a fatal error when we actually\ntry to format a ref. But if you have no refs to show at all,\nthen the behavior is confusing: git prints the error from\nverify_ref_format(), then exits with code 0 without showing\nany output. Let's instead abort immediately if we know we\nhave a bogus format.\n\nWe'll output the usage information if we have it handy (just\nlike the existing call in cmd_for_each_ref() does), and\notherwise just die().\n\nSigned-off-by: Jeff King \n---\n builtin/branch.c | 4 +++-\n builtin/tag.c | 7 ++++---\n builtin/verify-tag.c | 4 +++-\n 3 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 8a0595e11..e756a5667 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -409,7 +409,9 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \n \tif (!format)\n \t\tformat = to_free = build_format(filter, maxwidth, remote_prefix);\n-\tverify_ref_format(format);\n+\n+\tif (verify_ref_format(format))\n+\t\tdie(_(\"unable to parse format string\"));\n \n \tref_array_sort(sorting, &array);\n \ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 01154ea8d..216629fb2 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -53,7 +53,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con\n \t\t\tformat = \"%(refname:lstrip=2)\";\n \t}\n \n-\tverify_ref_format(format);\n+\tif (verify_ref_format(format))\n+\t\tdie(_(\"unable to parse format string\"));\n \tfilter->with_commit_tag_algo = 1;\n \tfilter_refs(&array, filter, FILTER_REFS_TAGS);\n \tref_array_sort(sorting, &array);\n@@ -501,8 +502,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (cmdmode == 'd')\n \t\treturn for_each_tag_name(argv, delete_tag, NULL);\n \tif (cmdmode == 'v') {\n-\t\tif (format)\n-\t\t\tverify_ref_format(format);\n+\t\tif (format && verify_ref_format(format))\n+\t\t\tusage_with_options(git_tag_usage, options);\n \t\treturn for_each_tag_name(argv, verify_tag, format);\n \t}\n \ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex f9a5f7535..a10eca2b2 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -51,7 +51,9 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n \t\tflags |= GPG_VERIFY_VERBOSE;\n \n \tif (fmt_pretty) {\n-\t\tverify_ref_format(fmt_pretty);\n+\t\tif (verify_ref_format(fmt_pretty))\n+\t\t\tusage_with_options(verify_tag_usage,\n+\t\t\t\t\t verify_tag_options);\n \t\tflags |= GPG_VERIFY_OMIT_STATUS;\n \t}\n \n-- \n2.13.2.1157.gc6daca446\n\n"},{"id":"324353","messageId":"20170713145620.pwsyvsudtnyfcvxg@sigill.intra.peff.net","threadId":"46375","inReplyTo":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","subject":"[PATCH 02/15] docs/for-each-ref: update pointer to color syntax","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T14:56:21Z","receivedAt":"2017-07-13T14:56:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The documentation for the %(color) placeholder refers to the\ncolor.branch.* config for more details. But those details\nmoved to their own section in b92c1a28f\n(Documentation/config.txt: describe 'color' value type in\nthe \"Values\" section, 2015-03-03). Let's update our\npointer. We can steal the text from 30cfe72d3 (pretty: fix\ndocument link for color specification, 2016-10-11), which\nfixed the same problem in a different place.\n\nWhile we're at it, let's give an example, which makes the\nsyntax much more clear than just the text.\n\nSigned-off-by: Jeff King \n---\n Documentation/git-for-each-ref.txt | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 03e187a10..cc42c1283 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -156,8 +156,10 @@ HEAD::\n \totherwise.\n \n color::\n-\tChange output color. Followed by `:`, where names\n-\tare described in `color.branch.*`.\n+\tChange output color. Followed by `:`, where color\n+\tnames are described under Values in the \"CONFIGURATION FILE\"\n+\tsection of linkgit:git-config[1]. For example,\n+\t`%(color:bold red)`.\n \n align::\n \tLeft-, middle-, or right-align the content between\n-- \n2.13.2.1157.gc6daca446\n\n"},{"id":"324354","messageId":"20170713145841.4win5tbx4efwjsa2@sigill.intra.peff.net","threadId":"46375","inReplyTo":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","subject":"[PATCH 03/15] t: use test_decode_color rather than literal ANSI codes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T14:58:41Z","receivedAt":"2017-07-13T14:58:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we put literal ANSI terminal codes into our test\nscripts, it makes diffs on those scripts hard to read (the\ncolors may be indistinguishable from diff coloring, or in\nthe case of a reset, may not be visible at all).\n\nSome scripts get around this by including human-readable\nnames and converting to literal codes with a git-config\nhack. This makes the actual code diffs look OK, but test_cmp\noutput suffers from the same problem.\n\nLet's use test_decode_color instead, which turns the codes\ninto obvious text tags.\n\nSigned-off-by: Jeff King \n---\nI really only need t6300 and t6006 converted to build on for the rest of\nthe series. But t4207 was easy to do. t4026 still uses raw codes, but\nconverting it would be a pretty big job, so I punted.\n\n t/t4207-log-decoration-colors.sh | 22 +++++++++------------\n t/t6006-rev-list-format.sh | 42 +++++++++++++++++++++++++---------------\n t/t6300-for-each-ref.sh | 18 ++++++++---------\n t/test-lib-functions.sh | 1 +\n 4 files changed, 44 insertions(+), 39 deletions(-)\n\ndiff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh\nindex b972296f0..60f040cab 100755\n--- a/t/t4207-log-decoration-colors.sh\n+++ b/t/t4207-log-decoration-colors.sh\n@@ -7,11 +7,6 @@ test_description='Test for \"git log --decorate\" colors'\n \n . ./test-lib.sh\n \n-get_color ()\n-{\n-\tgit config --get-color no.such.slot \"$1\"\n-}\n-\n test_expect_success setup '\n \tgit config diff.color.commit yellow &&\n \tgit config color.decorate.branch green &&\n@@ -20,14 +15,14 @@ test_expect_success setup '\n \tgit config color.decorate.stash magenta &&\n \tgit config color.decorate.HEAD cyan &&\n \n-\tc_reset=$(get_color reset) &&\n+\tc_reset=\"\" &&\n \n-\tc_commit=$(get_color yellow) &&\n-\tc_branch=$(get_color green) &&\n-\tc_remoteBranch=$(get_color red) &&\n-\tc_tag=$(get_color \"reverse bold yellow\") &&\n-\tc_stash=$(get_color magenta) &&\n-\tc_HEAD=$(get_color cyan) &&\n+\tc_commit=\"\" &&\n+\tc_branch=\"\" &&\n+\tc_remoteBranch=\"\" &&\n+\tc_tag=\"\" &&\n+\tc_stash=\"\" &&\n+\tc_HEAD=\"\" &&\n \n \ttest_commit A &&\n \tgit clone . other &&\n@@ -59,7 +54,8 @@ EOF\n # to this test since it does not contain any decoration, hence --first-parent\n test_expect_success 'Commit Decorations Colored Correctly' '\n \tgit log --first-parent --abbrev=10 --all --decorate --oneline --color=always |\n-\tsed \"s/[0-9a-f]\\{10,10\\}/COMMIT_ID/\" >out &&\n+\tsed \"s/[0-9a-f]\\{10,10\\}/COMMIT_ID/\" |\n+\ttest_decode_color >out &&\n \ttest_cmp expected out\n '\n \ndiff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\nindex a1dcdb81d..647218b4e 100755\n--- a/t/t6006-rev-list-format.sh\n+++ b/t/t6006-rev-list-format.sh\n@@ -61,8 +61,9 @@ test_format () {\n # Feed to --format to provide predictable colored sequences.\n AUTO_COLOR='%C(auto,red)foo%C(auto,reset)'\n has_color () {\n-\tprintf '\\033[31mfoo\\033[m\\n' >expect &&\n-\ttest_cmp expect \"$1\"\n+\ttest_decode_color <\"$1\" >decoded &&\n+\techo \"foo\" >expect &&\n+\ttest_cmp expect decoded\n }\n \n has_no_color () {\n@@ -170,19 +171,27 @@ $added\n \n EOF\n \n-test_format colors %Credfoo%Cgreenbar%Cbluebaz%Cresetxyzzy <expect <<-EOF &&\n+\tcommit $head2\n+\tfoobarbazxyzzy\n+\tEOF\n+\tformat=\"%Credfoo%Cgreenbar%Cbluebaz%Cresetxyzzy\" &&\n+\tgit rev-list --format=\"$format\" -1 master >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expect actual\n+'\n \n-test_format advanced-colors '%C(red yellow bold)foo%C(reset)' <expect <<-EOF &&\n+\tcommit $head2\n+\tfoo\n+\tEOF\n+\tformat=\"%C(red yellow bold)foo%C(reset)\" &&\n+\tgit rev-list --format=\"$format\" -1 master >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expect actual\n+'\n \n test_expect_success '%C(auto,...) does not enable color by default' '\n \tgit log --format=$AUTO_COLOR -1 >actual &&\n@@ -224,8 +233,9 @@ test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' '\n '\n \n test_expect_success '%C(auto) respects --color' '\n-\tgit log --color --format=\"%C(auto)%H\" -1 >actual &&\n-\tprintf \"\\\\033[33m%s\\\\033[m\\\\n\" $(git rev-parse HEAD) >expect &&\n+\tgit log --color --format=\"%C(auto)%H\" -1 >actual.raw &&\n+\ttest_decode_color actual &&\n+\techo \"$(git rev-parse HEAD)\" >expect &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 834a9ed16..7872a2f54 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -412,20 +412,18 @@ test_expect_success 'Check for invalid refname format' '\n \ttest_must_fail git for-each-ref --format=\"%(refname:INVALID)\"\n '\n \n-get_color ()\n-{\n-\tgit config --get-color no.such.slot \"$1\"\n-}\n-\n cat >expected <master\n+$(git rev-parse --short refs/remotes/origin/master) origin/master\n+$(git rev-parse --short refs/tags/testtag) testtag\n+$(git rev-parse --short refs/tags/two) two\n EOF\n \n test_expect_success 'Check %(color:...) ' '\n-\tgit for-each-ref --format=\"%(objectname:short) %(color:green)%(refname:short)\" >actual &&\n+\tgit for-each-ref \\\n+\t\t--format=\"%(objectname:short) %(color:green)%(refname:short)\" \\\n+\t\t>actual.raw &&\n+\ttest_decode_color actual &&\n \ttest_cmp expected actual\n '\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex db622c355..e09e93b38 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -42,6 +42,7 @@ test_decode_color () {\n \t\tfunction name(n) {\n \t\t\tif (n == 0) return \"RESET\";\n \t\t\tif (n == 1) return \"BOLD\";\n+\t\t\tif (n == 7) return \"REVERSE\";\n \t\t\tif (n == 30) return \"BLACK\";\n \t\t\tif (n == 31) return \"RED\";\n \t\t\tif (n == 32) return \"GREEN\";\n-- \n2.13.2.1157.gc6daca446\n\n"},{"id":"324355","messageId":"20170713145856.ckciq3z5fparijaz@sigill.intra.peff.net","threadId":"46375","inReplyTo":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","subject":"[PATCH 04/15] ref-filter: simplify automatic color reset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T14:58:56Z","receivedAt":"2017-07-13T14:59:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the user-format doesn't add the closing color reset, we\nadd one automatically. But we do so by parsing the \"reset\"\nstring. We can just use the baked-in string literal, which\nis simpler.\n\nSigned-off-by: Jeff King \n---\n ref-filter.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ae6ecbd1c..6da5be3ac 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2084,11 +2084,7 @@ void format_ref_array_item(struct ref_array_item *info, const char *format,\n \t}\n \tif (need_color_reset_at_eol) {\n \t\tstruct atom_value resetv;\n-\t\tchar color[COLOR_MAXLEN] = \"\";\n-\n-\t\tif (color_parse(\"reset\", color) < 0)\n-\t\t\tdie(\"BUG: couldn't parse 'reset' as a color\");\n-\t\tresetv.s = color;\n+\t\tresetv.s = GIT_COLOR_RESET;\n \t\tappend_atom(&resetv, &state);\n \t}\n \tif (state.stack->prev)\n-- \n2.13.2.1157.gc6daca446\n\n"},{"id":"324356","messageId":"20170713150118.eof3xgu4zujmo6u6@sigill.intra.peff.net","threadId":"46375","inReplyTo":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","subject":"[PATCH 05/15] ref-filter: abstract ref format into its own struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T15:01:18Z","receivedAt":"2017-07-13T15:01:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The ref-filter module provides routines for formatting a ref\nfor output. The fundamental interface for the format is a\n\"const char *\" containing the format, and any additional\noptions need to be passed to each invocation of\nshow_ref_array_item.\n\nInstead, let's make a ref_format struct that holds the\nformat, along with any associated format options. That will\nmake some enhancements easier in the future:\n\n 1. new formatting options can be added without disrupting\n existing callers\n\n 2. some state can be carried in the struct rather than as\n global variables\n\nFor now this just has the text format itself along with the\nquote_style option, but we'll add more fields in future patches.\n\nSigned-off-by: Jeff King \n---\nA few of the spots are a bit ugly. E.g., when the format is optional you\nmight have to check whether format->format is NULL. It's possible that\nit would be nicer if we abstracted this into a more opaque object with\naccessors. But I didn't want to go overboard.\n\n builtin/branch.c | 14 +++++++-------\n builtin/for-each-ref.c | 22 ++++++++++++----------\n builtin/tag.c | 30 ++++++++++++++++--------------\n builtin/verify-tag.c | 12 ++++++------\n ref-filter.c | 22 ++++++++++++----------\n ref-filter.h | 22 +++++++++++++++++-----\n 6 files changed, 70 insertions(+), 52 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex e756a5667..036fdc929 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -383,7 +383,7 @@ static char *build_format(struct ref_filter *filter, int maxwidth, const char *r\n \treturn strbuf_detach(&fmt, NULL);\n }\n \n-static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)\n+static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting, struct ref_format *format)\n {\n \tint i;\n \tstruct ref_array array;\n@@ -407,8 +407,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \tif (filter->verbose)\n \t\tmaxwidth = calc_maxwidth(&array, strlen(remote_prefix));\n \n-\tif (!format)\n-\t\tformat = to_free = build_format(filter, maxwidth, remote_prefix);\n+\tif (!format->format)\n+\t\tformat->format = to_free = build_format(filter, maxwidth, remote_prefix);\n \n \tif (verify_ref_format(format))\n \t\tdie(_(\"unable to parse format string\"));\n@@ -416,7 +416,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \tref_array_sort(sorting, &array);\n \n \tfor (i = 0; i < array.nr; i++) {\n-\t\tformat_ref_array_item(array.items[i], format, 0, &out);\n+\t\tformat_ref_array_item(array.items[i], format, &out);\n \t\tif (column_active(colopts)) {\n \t\t\tassert(!filter->verbose && \"--column and --verbose are incompatible\");\n \t\t\t /* format to a string_list to let print_columns() do its job */\n@@ -551,7 +551,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tstruct ref_filter filter;\n \tint icase = 0;\n \tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n-\tconst char *format = NULL;\n+\tstruct ref_format format = REF_FORMAT_INIT;\n \n \tstruct option options[] = {\n \t\tOPT_GROUP(N_(\"Generic options\")),\n@@ -595,7 +595,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"print only branches of the object\"), 0, parse_opt_object_name\n \t\t},\n \t\tOPT_BOOL('i', \"ignore-case\", &icase, N_(\"sorting and filtering are case insensitive\")),\n-\t\tOPT_STRING( 0 , \"format\", &format, N_(\"format\"), N_(\"format to use for the output\")),\n+\t\tOPT_STRING( 0 , \"format\", &format.format, N_(\"format\"), N_(\"format to use for the output\")),\n \t\tOPT_END(),\n \t};\n \n@@ -669,7 +669,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (!sorting)\n \t\t\tsorting = ref_default_sorting();\n \t\tsorting->ignore_case = icase;\n-\t\tprint_ref_list(&filter, sorting, format);\n+\t\tprint_ref_list(&filter, sorting, &format);\n \t\tprint_columns(&output, colopts, NULL);\n \t\tstring_list_clear(&output, 0);\n \t\treturn 0;\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 52be99cba..f47066b42 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -17,25 +17,25 @@ static char const * const for_each_ref_usage[] = {\n int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n-\tconst char *format = \"%(objectname) %(objecttype)\\t%(refname)\";\n \tstruct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n-\tint maxcount = 0, quote_style = 0, icase = 0;\n+\tint maxcount = 0, icase = 0;\n \tstruct ref_array array;\n \tstruct ref_filter filter;\n+\tstruct ref_format format = REF_FORMAT_INIT;\n \n \tstruct option opts[] = {\n-\t\tOPT_BIT('s', \"shell\", "e_style,\n+\t\tOPT_BIT('s', \"shell\", &format.quote_style,\n \t\t\tN_(\"quote placeholders suitably for shells\"), QUOTE_SHELL),\n-\t\tOPT_BIT('p', \"perl\", "e_style,\n+\t\tOPT_BIT('p', \"perl\", &format.quote_style,\n \t\t\tN_(\"quote placeholders suitably for perl\"), QUOTE_PERL),\n-\t\tOPT_BIT(0 , \"python\", "e_style,\n+\t\tOPT_BIT(0 , \"python\", &format.quote_style,\n \t\t\tN_(\"quote placeholders suitably for python\"), QUOTE_PYTHON),\n-\t\tOPT_BIT(0 , \"tcl\", "e_style,\n+\t\tOPT_BIT(0 , \"tcl\", &format.quote_style,\n \t\t\tN_(\"quote placeholders suitably for Tcl\"), QUOTE_TCL),\n \n \t\tOPT_GROUP(\"\"),\n \t\tOPT_INTEGER( 0 , \"count\", &maxcount, N_(\"show only matched refs\")),\n-\t\tOPT_STRING( 0 , \"format\", &format, N_(\"format\"), N_(\"format to use for the output\")),\n+\t\tOPT_STRING( 0 , \"format\", &format.format, N_(\"format\"), N_(\"format to use for the output\")),\n \t\tOPT_CALLBACK(0 , \"sort\", sorting_tail, N_(\"key\"),\n \t\t\t N_(\"field name to sort on\"), &parse_opt_ref_sorting),\n \t\tOPT_CALLBACK(0, \"points-at\", &filter.points_at,\n@@ -52,16 +52,18 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tmemset(&array, 0, sizeof(array));\n \tmemset(&filter, 0, sizeof(filter));\n \n+\tformat.format = \"%(objectname) %(objecttype)\\t%(refname)\";\n+\n \tparse_options(argc, argv, prefix, opts, for_each_ref_usage, 0);\n \tif (maxcount < 0) {\n \t\terror(\"invalid --count argument: `%d'\", maxcount);\n \t\tusage_with_options(for_each_ref_usage, opts);\n \t}\n-\tif (HAS_MULTI_BITS(quote_style)) {\n+\tif (HAS_MULTI_BITS(format.quote_style)) {\n \t\terror(\"more than one quoting style?\");\n \t\tusage_with_options(for_each_ref_usage, opts);\n \t}\n-\tif (verify_ref_format(format))\n+\tif (verify_ref_format(&format))\n \t\tusage_with_options(for_each_ref_usage, opts);\n \n \tif (!sorting)\n@@ -80,7 +82,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tif (!maxcount || array.nr < maxcount)\n \t\tmaxcount = array.nr;\n \tfor (i = 0; i < maxcount; i++)\n-\t\tshow_ref_array_item(array.items[i], format, quote_style);\n+\t\tshow_ref_array_item(array.items[i], &format);\n \tref_array_clear(&array);\n \treturn 0;\n }\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 216629fb2..207c9eb03 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -32,7 +32,8 @@ static const char * const git_tag_usage[] = {\n static unsigned int colopts;\n static int force_sign_annotate;\n \n-static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)\n+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n+\t\t struct ref_format *format)\n {\n \tstruct ref_array array;\n \tchar *to_free = NULL;\n@@ -43,14 +44,14 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con\n \tif (filter->lines == -1)\n \t\tfilter->lines = 0;\n \n-\tif (!format) {\n+\tif (!format->format) {\n \t\tif (filter->lines) {\n \t\t\tto_free = xstrfmt(\"%s %%(contents:lines=%d)\",\n \t\t\t\t\t \"%(align:15)%(refname:lstrip=2)%(end)\",\n \t\t\t\t\t filter->lines);\n-\t\t\tformat = to_free;\n+\t\t\tformat->format = to_free;\n \t\t} else\n-\t\t\tformat = \"%(refname:lstrip=2)\";\n+\t\t\tformat->format = \"%(refname:lstrip=2)\";\n \t}\n \n \tif (verify_ref_format(format))\n@@ -60,7 +61,7 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con\n \tref_array_sort(sorting, &array);\n \n \tfor (i = 0; i < array.nr; i++)\n-\t\tshow_ref_array_item(array.items[i], format, 0);\n+\t\tshow_ref_array_item(array.items[i], format);\n \tref_array_clear(&array);\n \tfree(to_free);\n \n@@ -106,17 +107,17 @@ static int verify_tag(const char *name, const char *ref,\n \t\t const struct object_id *oid, const void *cb_data)\n {\n \tint flags;\n-\tconst char *fmt_pretty = cb_data;\n+\tconst struct ref_format *format = cb_data;\n \tflags = GPG_VERIFY_VERBOSE;\n \n-\tif (fmt_pretty)\n+\tif (format->format)\n \t\tflags = GPG_VERIFY_OMIT_STATUS;\n \n \tif (gpg_verify_tag(oid->hash, name, flags))\n \t\treturn -1;\n \n-\tif (fmt_pretty)\n-\t\tpretty_print_ref(name, oid->hash, fmt_pretty);\n+\tif (format->format)\n+\t\tpretty_print_ref(name, oid->hash, format);\n \n \treturn 0;\n }\n@@ -393,7 +394,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_filter filter;\n \tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n-\tconst char *format = NULL;\n+\tstruct ref_format format = REF_FORMAT_INIT;\n \tint icase = 0;\n \tstruct option options[] = {\n \t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list tag names\"), 'l'),\n@@ -432,7 +433,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"print only tags of the object\"), PARSE_OPT_LASTARG_DEFAULT,\n \t\t\tparse_opt_object_name, (intptr_t) \"HEAD\"\n \t\t},\n-\t\tOPT_STRING( 0 , \"format\", &format, N_(\"format\"), N_(\"format to use for the output\")),\n+\t\tOPT_STRING( 0 , \"format\", &format.format, N_(\"format\"),\n+\t\t\t N_(\"format to use for the output\")),\n \t\tOPT_BOOL('i', \"ignore-case\", &icase, N_(\"sorting and filtering are case insensitive\")),\n \t\tOPT_END()\n \t};\n@@ -484,7 +486,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\trun_column_filter(colopts, &copts);\n \t\t}\n \t\tfilter.name_patterns = argv;\n-\t\tret = list_tags(&filter, sorting, format);\n+\t\tret = list_tags(&filter, sorting, &format);\n \t\tif (column_active(colopts))\n \t\t\tstop_column_filter();\n \t\treturn ret;\n@@ -502,9 +504,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (cmdmode == 'd')\n \t\treturn for_each_tag_name(argv, delete_tag, NULL);\n \tif (cmdmode == 'v') {\n-\t\tif (format && verify_ref_format(format))\n+\t\tif (format.format && verify_ref_format(&format))\n \t\t\tusage_with_options(git_tag_usage, options);\n-\t\treturn for_each_tag_name(argv, verify_tag, format);\n+\t\treturn for_each_tag_name(argv, verify_tag, &format);\n \t}\n \n \tif (msg.given || msgfile) {\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex a10eca2b2..87d73e856 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -32,11 +32,11 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n {\n \tint i = 1, verbose = 0, had_error = 0;\n \tunsigned flags = 0;\n-\tchar *fmt_pretty = NULL;\n+\tstruct ref_format format = REF_FORMAT_INIT;\n \tconst struct option verify_tag_options[] = {\n \t\tOPT__VERBOSE(&verbose, N_(\"print tag contents\")),\n \t\tOPT_BIT(0, \"raw\", &flags, N_(\"print raw gpg status output\"), GPG_VERIFY_RAW),\n-\t\tOPT_STRING( 0 , \"format\", &fmt_pretty, N_(\"format\"), N_(\"format to use for the output\")),\n+\t\tOPT_STRING(0, \"format\", &format.format, N_(\"format\"), N_(\"format to use for the output\")),\n \t\tOPT_END()\n \t};\n \n@@ -50,8 +50,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n \tif (verbose)\n \t\tflags |= GPG_VERIFY_VERBOSE;\n \n-\tif (fmt_pretty) {\n-\t\tif (verify_ref_format(fmt_pretty))\n+\tif (format.format) {\n+\t\tif (verify_ref_format(&format))\n \t\t\tusage_with_options(verify_tag_usage,\n \t\t\t\t\t verify_tag_options);\n \t\tflags |= GPG_VERIFY_OMIT_STATUS;\n@@ -70,8 +70,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (fmt_pretty)\n-\t\t\tpretty_print_ref(name, sha1, fmt_pretty);\n+\t\tif (format.format)\n+\t\t\tpretty_print_ref(name, sha1, &format);\n \t}\n \treturn had_error;\n }\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 6da5be3ac..66d234bb1 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -657,12 +657,12 @@ static const char *find_next(const char *cp)\n * Make sure the format string is well formed, and parse out\n * the used atoms.\n */\n-int verify_ref_format(const char *format)\n+int verify_ref_format(struct ref_format *format)\n {\n \tconst char *cp, *sp;\n \n \tneed_color_reset_at_eol = 0;\n-\tfor (cp = format; *cp && (sp = find_next(cp)); ) {\n+\tfor (cp = format->format; *cp && (sp = find_next(cp)); ) {\n \t\tconst char *color, *ep = strchr(sp, ')');\n \t\tint at;\n \n@@ -2060,16 +2060,17 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting\n \t}\n }\n \n-void format_ref_array_item(struct ref_array_item *info, const char *format,\n-\t\t\t int quote_style, struct strbuf *final_buf)\n+void format_ref_array_item(struct ref_array_item *info,\n+\t\t\t const struct ref_format *format,\n+\t\t\t struct strbuf *final_buf)\n {\n \tconst char *cp, *sp, *ep;\n \tstruct ref_formatting_state state = REF_FORMATTING_STATE_INIT;\n \n-\tstate.quote_style = quote_style;\n+\tstate.quote_style = format->quote_style;\n \tpush_stack_element(&state.stack);\n \n-\tfor (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n+\tfor (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n \t\tstruct atom_value *atomv;\n \n \t\tep = strchr(sp, ')');\n@@ -2093,23 +2094,24 @@ void format_ref_array_item(struct ref_array_item *info, const char *format,\n \tpop_stack_element(&state.stack);\n }\n \n-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n+void show_ref_array_item(struct ref_array_item *info,\n+\t\t\t const struct ref_format *format)\n {\n \tstruct strbuf final_buf = STRBUF_INIT;\n \n-\tformat_ref_array_item(info, format, quote_style, &final_buf);\n+\tformat_ref_array_item(info, format, &final_buf);\n \tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n \tstrbuf_release(&final_buf);\n \tputchar('\\n');\n }\n \n void pretty_print_ref(const char *name, const unsigned char *sha1,\n-\t\tconst char *format)\n+\t\t const struct ref_format *format)\n {\n \tstruct ref_array_item *ref_item;\n \tref_item = new_ref_array_item(name, sha1, 0);\n \tref_item->kind = ref_kind_from_refname(name);\n-\tshow_ref_array_item(ref_item, format, 0);\n+\tshow_ref_array_item(ref_item, format);\n \tfree_array_item(ref_item);\n }\n \ndiff --git a/ref-filter.h b/ref-filter.h\nindex 6552024f0..2bb58879d 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -72,6 +72,17 @@ struct ref_filter {\n \t\tverbose;\n };\n \n+struct ref_format {\n+\t/*\n+\t * Set these to define the format; make sure you call\n+\t * verify_ref_format() afterwards to finalize.\n+\t */\n+\tconst char *format;\n+\tint quote_style;\n+};\n+\n+#define REF_FORMAT_INIT { NULL, 0 }\n+\n /* Macros for checking --merged and --no-merged options */\n #define _OPT_MERGED_NO_MERGED(option, filter, h) \\\n \t{ OPTION_CALLBACK, 0, option, (filter), N_(\"commit\"), (h), \\\n@@ -93,14 +104,15 @@ void ref_array_clear(struct ref_array *array);\n /* Parse format string and sort specifiers */\n int parse_ref_filter_atom(const char *atom, const char *ep);\n /* Used to verify if the given format is correct and to parse out the used atoms */\n-int verify_ref_format(const char *format);\n+int verify_ref_format(struct ref_format *format);\n /* Sort the given ref_array as per the ref_sorting provided */\n void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);\n /* Based on the given format and quote_style, fill the strbuf */\n-void format_ref_array_item(struct ref_array_item *info, const char *format,\n-\t\t\t int quote_style, struct strbuf *final_buf);\n+void format_ref_array_item(struct ref_array_item *info,\n+\t\t\t const struct ref_format *format,\n+\t\t\t struct strbuf *final_buf);\n /* Print the ref using the given format and quote_style */\n-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);\n+void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n /* Callback function for parsing the sort option */\n int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);\n /* Default sort option based on refname */\n@@ -117,6 +129,6 @@ void setup_ref_filter_porcelain_msg(void);\n * name must be a fully qualified refname.\n */\n void pretty_print_ref(const char *name, const unsigned char *sha1,\n-\t\tconst char *format);\n+\t\t const struct ref_format *format);\n \n #endif /* REF_FILTER_H */\n-- \n2.13.2.1157.gc6daca446\n\n"},{"id":"324357","messageId":"20170713150230.sy7zuksbgjcrllxh@sigill.intra.peff.net","threadId":"46375","inReplyTo":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","subject":"[PATCH 06/15] ref-filter: move need_color_reset_at_eol into ref_format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T15:02:30Z","receivedAt":"2017-07-13T15:02:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Calling verify_ref_format() doesn't just confirm that the\nformat is sane; it actually sets some global variables that\nwill be used later when formatting the refs. These logically\nshould belong to the ref_format, which would make it\npossible to use multiple formats within a single program\ninvocation.\n\nLet's move one such flag into the ref_format struct. There\nare still others that would need to be moved before it would\nbe safe to use multiple formats, but this commit gives a\nblueprint for how that should look.\n\nSigned-off-by: Jeff King \n---\nThis commit is strictly optional for this series, but I wanted to give a\nsense of how the rest of the movement might look, since I was thinking\nabout it. The big thing to move would be the used_atoms array, but I\npunted on that for now.\n\n ref-filter.c | 7 +++----\n ref-filter.h | 3 +++\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 66d234bb1..178396e1f 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -97,7 +97,6 @@ static struct used_atom {\n \t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n-static int need_color_reset_at_eol;\n \n static void color_atom_parser(struct used_atom *atom, const char *color_value)\n {\n@@ -661,7 +660,7 @@ int verify_ref_format(struct ref_format *format)\n {\n \tconst char *cp, *sp;\n \n-\tneed_color_reset_at_eol = 0;\n+\tformat->need_color_reset_at_eol = 0;\n \tfor (cp = format->format; *cp && (sp = find_next(cp)); ) {\n \t\tconst char *color, *ep = strchr(sp, ')');\n \t\tint at;\n@@ -673,7 +672,7 @@ int verify_ref_format(struct ref_format *format)\n \t\tcp = ep + 1;\n \n \t\tif (skip_prefix(used_atom[at].name, \"color:\", &color))\n-\t\t\tneed_color_reset_at_eol = !!strcmp(color, \"reset\");\n+\t\t\tformat->need_color_reset_at_eol = !!strcmp(color, \"reset\");\n \t}\n \treturn 0;\n }\n@@ -2083,7 +2082,7 @@ void format_ref_array_item(struct ref_array_item *info,\n \t\tsp = cp + strlen(cp);\n \t\tappend_literal(cp, sp, &state);\n \t}\n-\tif (need_color_reset_at_eol) {\n+\tif (format->need_color_reset_at_eol) {\n \t\tstruct atom_value resetv;\n \t\tresetv.s = GIT_COLOR_RESET;\n \t\tappend_atom(&resetv, &state);\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 2bb58879d..9e1e89c19 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -79,6 +79,9 @@ struct ref_format {\n \t */\n \tconst char *format;\n \tint quote_style;\n+\n+\t/* Internal state to ref-filter */\n+\tint need_color_reset_at_eol;\n };\n \n #define REF_FORMAT_INIT { NULL, 0 }\n-- \n2.13.2.1157.gc6daca446\n\n"},{"id":"324358","messageId":"20170713150244.ygpefrbaz53rl6e6@sigill.intra.peff.net","threadId":"46375","inReplyTo":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","subject":"[PATCH 07/15] ref-filter: provide a function for parsing sort options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T15:02:44Z","receivedAt":"2017-07-13T15:02:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The ref-filter module currently provides a callback suitable\nfor parsing command-line --sort options. But since git-tag\nalso supports the tag.sort config option, it needs a\nfunction whose implementation is quite similar, but with a\nslightly different interface. The end result is that\nbuiltin/tag.c has a copy-paste of parse_opt_ref_sorting().\n\nInstead, let's provide a function to parse an arbitrary\nsort string, which we can then trivially wrap to make the\nparse_opt variant.\n\nSigned-off-by: Jeff King \n---\n builtin/tag.c | 26 +-------------------------\n ref-filter.c | 13 ++++++++-----\n ref-filter.h | 2 ++\n 3 files changed, 11 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 207c9eb03..66e35b823 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -136,30 +136,6 @@ static const char tag_template_nocleanup[] =\n \t\"Lines starting with '%c' will be kept; you may remove them\"\n \t\" yourself if you want to.\\n\");\n \n-/* Parse arg given and add it the ref_sorting array */\n-static int parse_sorting_string(const char *arg, struct ref_sorting **sorting_tail)\n-{\n-\tstruct ref_sorting *s;\n-\tint len;\n-\n-\ts = xcalloc(1, sizeof(*s));\n-\ts->next = *sorting_tail;\n-\t*sorting_tail = s;\n-\n-\tif (*arg == '-') {\n-\t\ts->reverse = 1;\n-\t\targ++;\n-\t}\n-\tif (skip_prefix(arg, \"version:\", &arg) ||\n-\t skip_prefix(arg, \"v:\", &arg))\n-\t\ts->version = 1;\n-\n-\tlen = strlen(arg);\n-\ts->atom = parse_ref_filter_atom(arg, arg+len);\n-\n-\treturn 0;\n-}\n-\n static int git_tag_config(const char *var, const char *value, void *cb)\n {\n \tint status;\n@@ -168,7 +144,7 @@ static int git_tag_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"tag.sort\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tparse_sorting_string(value, sorting_tail);\n+\t\tparse_ref_sorting(sorting_tail, value);\n \t\treturn 0;\n \t}\n \ndiff --git a/ref-filter.c b/ref-filter.c\nindex 178396e1f..432121219 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2126,15 +2126,11 @@ struct ref_sorting *ref_default_sorting(void)\n \treturn sorting;\n }\n \n-int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)\n+void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)\n {\n-\tstruct ref_sorting **sorting_tail = opt->value;\n \tstruct ref_sorting *s;\n \tint len;\n \n-\tif (!arg) /* should --no-sort void the list ? */\n-\t\treturn -1;\n-\n \ts = xcalloc(1, sizeof(*s));\n \ts->next = *sorting_tail;\n \t*sorting_tail = s;\n@@ -2148,6 +2144,13 @@ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)\n \t\ts->version = 1;\n \tlen = strlen(arg);\n \ts->atom = parse_ref_filter_atom(arg, arg+len);\n+}\n+\n+int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)\n+{\n+\tif (!arg) /* should --no-sort void the list ? */\n+\t\treturn -1;\n+\tparse_ref_sorting(opt->value, arg);\n \treturn 0;\n }\n \ndiff --git a/ref-filter.h b/ref-filter.h\nindex 9e1e89c19..67fa6261b 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -116,6 +116,8 @@ void format_ref_array_item(struct ref_array_item *info,\n \t\t\t struct strbuf *final_buf);\n /* Print the ref using the given format and quote_style */\n void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n+/* Parse a single sort specifier and add it to the list */\n+void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);\n /* Callback function for parsing the sort option */\n int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);\n /* Default sort option based on refname */\n-- \n2.13.2.1157.gc6daca446\n\n"},{"id":"324359","messageId":"20170713150252.4xx2nqi2w45s7cu3@sigill.intra.peff.net","threadId":"46375","inReplyTo":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","subject":"[PATCH 08/15] ref-filter: make parse_ref_filter_atom a private function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T15:02:52Z","receivedAt":"2017-07-13T15:02:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The parse_ref_filter_atom() function really shouldn't be\nexposed outside of ref-filter.c; its return value is an\ninteger index into an array that is private in that file.\n\nSince the previous commit removed the sole external caller\n(and replaced it with a public function at a more\nappropriately level), we can just make this static.\n\nSigned-off-by: Jeff King \n---\n ref-filter.c | 2 +-\n ref-filter.h | 2 --\n 2 files changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 432121219..ee97d7218 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -364,7 +364,7 @@ struct atom_value {\n /*\n * Used to parse format string and sort specifiers\n */\n-int parse_ref_filter_atom(const char *atom, const char *ep)\n+static int parse_ref_filter_atom(const char *atom, const char *ep)\n {\n \tconst char *sp;\n \tconst char *arg;\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 67fa6261b..1ffc3ca81 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -104,8 +104,6 @@ struct ref_format {\n int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int type);\n /* Clear all memory allocated to ref_array */\n void ref_array_clear(struct ref_array *array);\n-/* Parse format string and sort specifiers */\n-int parse_ref_filter_atom(const char *atom, const char *ep);\n /* Used to verify if the given format is correct and to parse out the used atoms */\n int verify_ref_format(struct ref_format *format);\n /* Sort the given ref_array as per the ref_sorting provided */\n-- \n2.13.2.1157.gc6daca446\n\n"},{"id":"324360","messageId":"20170713150258.i62ohcembl2wfrqm@sigill.intra.peff.net","threadId":"46375","inReplyTo":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","subject":"[PATCH 09/15] ref-filter: factor out the parsing of sorting atoms","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T15:02:58Z","receivedAt":"2017-07-13T15:03:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We parse sort strings as single formatting atoms, and just\nbuild on parse_ref_filter_atom(). Let's pull this idea into\nits own function, since it's about to get a little more\ncomplex. As a bonus, we can give the function a slightly\nmore natural interface, since our single atoms are in their\nown strings.\n\nSigned-off-by: Jeff King \n---\n ref-filter.c | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ee97d7218..080f5aceb 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2114,6 +2114,12 @@ void pretty_print_ref(const char *name, const unsigned char *sha1,\n \tfree_array_item(ref_item);\n }\n \n+static int parse_sorting_atom(const char *atom)\n+{\n+\tconst char *end = atom + strlen(atom);\n+\treturn parse_ref_filter_atom(atom, end);\n+}\n+\n /* If no sorting option is given, use refname to sort as default */\n struct ref_sorting *ref_default_sorting(void)\n {\n@@ -2122,14 +2128,13 @@ struct ref_sorting *ref_default_sorting(void)\n \tstruct ref_sorting *sorting = xcalloc(1, sizeof(*sorting));\n \n \tsorting->next = NULL;\n-\tsorting->atom = parse_ref_filter_atom(cstr_name, cstr_name + strlen(cstr_name));\n+\tsorting->atom = parse_sorting_atom(cstr_name);\n \treturn sorting;\n }\n \n void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)\n {\n \tstruct ref_sorting *s;\n-\tint len;\n \n \ts = xcalloc(1, sizeof(*s));\n \ts->next = *sorting_tail;\n@@ -2142,8 +2147,7 @@ void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)\n \tif (skip_prefix(arg, \"version:\", &arg) ||\n \t skip_prefix(arg, \"v:\", &arg))\n \t\ts->version = 1;\n-\tlen = strlen(arg);\n-\ts->atom = parse_ref_filter_atom(arg, arg+len);\n+\ts->atom = parse_sorting_atom(arg);\n }\n \n int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)\n-- \n2.13.2.1157.gc6daca446\n\n"},{"id":"324362","messageId":"20170713150640.qnefucyb4buggo6q@sigill.intra.peff.net","threadId":"46375","inReplyTo":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","subject":"[PATCH 10/15] ref-filter: pass ref_format struct to atom parsers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T15:06:40Z","receivedAt":"2017-07-13T15:07:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The callback for parsing each formatting atom gets to see\nonly the atom struct (which it's filling in) and the text to\nbe parsed. This doesn't leave any room for it to behave\ndifferently based on context known only to the ref_format.\n\nWe can solve this by passing in the surrounding ref_format\nto each parser. Note that this makes things slightly awkward\nfor sort strings, which parse atoms without having a\nref_format. We'll solve that by using a dummy ref_format\nwith default parameters.\n\nSigned-off-by: Jeff King \n---\nIf we eventually do move used_atoms into the format, we'd probably need\nto either have a separate ref_format for the sorting atoms, or have them\nshare with the struct used for actual formatting. At this point I don't\nthink it matters at all. You can do nonsense like:\n\n git for-each-ref --sort='%(color:red)'\n\nand after this series it will not respect your color.ui setting. But\nsince in either case it's a ridiculous sort key (whether it shows the\ncolor or not, it's the same for all refs and so can't affect the\nsorting), it's not worth caring about.\n\nIf we do eventually refactor ref_format more such that the dummy struct\ndoesn't cut it, I think we can deal with the issue then.\n\n ref-filter.c | 42 +++++++++++++++++++++++++-----------------\n 1 file changed, 25 insertions(+), 17 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 080f5aceb..129a63677 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -98,7 +98,7 @@ static struct used_atom {\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n \n-static void color_atom_parser(struct used_atom *atom, const char *color_value)\n+static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)\n {\n \tif (!color_value)\n \t\tdie(_(\"expected format: %%(color:)\"));\n@@ -126,7 +126,7 @@ static void refname_atom_parser_internal(struct refname_atom *atom,\n \t\tdie(_(\"unrecognized %%(%s) argument: %s\"), name, arg);\n }\n \n-static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)\n+static void remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n {\n \tstruct string_list params = STRING_LIST_INIT_DUP;\n \tint i;\n@@ -160,28 +160,28 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)\n \tstring_list_clear(¶ms, 0);\n }\n \n-static void body_atom_parser(struct used_atom *atom, const char *arg)\n+static void body_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n {\n \tif (arg)\n \t\tdie(_(\"%%(body) does not take arguments\"));\n \tatom->u.contents.option = C_BODY_DEP;\n }\n \n-static void subject_atom_parser(struct used_atom *atom, const char *arg)\n+static void subject_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n {\n \tif (arg)\n \t\tdie(_(\"%%(subject) does not take arguments\"));\n \tatom->u.contents.option = C_SUB;\n }\n \n-static void trailers_atom_parser(struct used_atom *atom, const char *arg)\n+static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n {\n \tif (arg)\n \t\tdie(_(\"%%(trailers) does not take arguments\"));\n \tatom->u.contents.option = C_TRAILERS;\n }\n \n-static void contents_atom_parser(struct used_atom *atom, const char *arg)\n+static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n {\n \tif (!arg)\n \t\tatom->u.contents.option = C_BARE;\n@@ -201,7 +201,7 @@ static void contents_atom_parser(struct used_atom *atom, const char *arg)\n \t\tdie(_(\"unrecognized %%(contents) argument: %s\"), arg);\n }\n \n-static void objectname_atom_parser(struct used_atom *atom, const char *arg)\n+static void objectname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n {\n \tif (!arg)\n \t\tatom->u.objectname.option = O_FULL;\n@@ -218,7 +218,7 @@ static void objectname_atom_parser(struct used_atom *atom, const char *arg)\n \t\tdie(_(\"unrecognized %%(objectname) argument: %s\"), arg);\n }\n \n-static void refname_atom_parser(struct used_atom *atom, const char *arg)\n+static void refname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n {\n \trefname_atom_parser_internal(&atom->u.refname, arg, atom->name);\n }\n@@ -234,7 +234,7 @@ static align_type parse_align_position(const char *s)\n \treturn -1;\n }\n \n-static void align_atom_parser(struct used_atom *atom, const char *arg)\n+static void align_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n {\n \tstruct align *align = &atom->u.align;\n \tstruct string_list params = STRING_LIST_INIT_DUP;\n@@ -273,7 +273,7 @@ static void align_atom_parser(struct used_atom *atom, const char *arg)\n \tstring_list_clear(¶ms, 0);\n }\n \n-static void if_atom_parser(struct used_atom *atom, const char *arg)\n+static void if_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n {\n \tif (!arg) {\n \t\tatom->u.if_then_else.cmp_status = COMPARE_NONE;\n@@ -287,7 +287,7 @@ static void if_atom_parser(struct used_atom *atom, const char *arg)\n \t}\n }\n \n-static void head_atom_parser(struct used_atom *atom, const char *arg)\n+static void head_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n {\n \tstruct object_id unused;\n \n@@ -297,7 +297,7 @@ static void head_atom_parser(struct used_atom *atom, const char *arg)\n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n-\tvoid (*parser)(struct used_atom *atom, const char *arg);\n+\tvoid (*parser)(const struct ref_format *format, struct used_atom *atom, const char *arg);\n } valid_atom[] = {\n \t{ \"refname\" , FIELD_STR, refname_atom_parser },\n \t{ \"objecttype\" },\n@@ -364,7 +364,8 @@ struct atom_value {\n /*\n * Used to parse format string and sort specifiers\n */\n-static int parse_ref_filter_atom(const char *atom, const char *ep)\n+static int parse_ref_filter_atom(const struct ref_format *format,\n+\t\t\t\t const char *atom, const char *ep)\n {\n \tconst char *sp;\n \tconst char *arg;\n@@ -412,7 +413,7 @@ static int parse_ref_filter_atom(const char *atom, const char *ep)\n \t\targ = used_atom[at].name + (arg - atom) + 1;\n \tmemset(&used_atom[at].u, 0, sizeof(used_atom[at].u));\n \tif (valid_atom[i].parser)\n-\t\tvalid_atom[i].parser(&used_atom[at], arg);\n+\t\tvalid_atom[i].parser(format, &used_atom[at], arg);\n \tif (*atom == '*')\n \t\tneed_tagged = 1;\n \tif (!strcmp(valid_atom[i].name, \"symref\"))\n@@ -668,7 +669,7 @@ int verify_ref_format(struct ref_format *format)\n \t\tif (!ep)\n \t\t\treturn error(_(\"malformed format string %s\"), sp);\n \t\t/* sp points at \"%(\" and ep points at the closing \")\" */\n-\t\tat = parse_ref_filter_atom(sp + 2, ep);\n+\t\tat = parse_ref_filter_atom(format, sp + 2, ep);\n \t\tcp = ep + 1;\n \n \t\tif (skip_prefix(used_atom[at].name, \"color:\", &color))\n@@ -2075,7 +2076,9 @@ void format_ref_array_item(struct ref_array_item *info,\n \t\tep = strchr(sp, ')');\n \t\tif (cp < sp)\n \t\t\tappend_literal(cp, sp, &state);\n-\t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n+\t\tget_ref_atom_value(info,\n+\t\t\t\t parse_ref_filter_atom(format, sp + 2, ep),\n+\t\t\t\t &atomv);\n \t\tatomv->handler(atomv, &state);\n \t}\n \tif (*cp) {\n@@ -2116,8 +2119,13 @@ void pretty_print_ref(const char *name, const unsigned char *sha1,\n \n static int parse_sorting_atom(const char *atom)\n {\n+\t/*\n+\t * This parses an atom using a dummy ref_format, since we don't\n+\t * actually care about the formatting details.\n+\t */\n+\tstruct ref_format dummy = REF_FORMAT_INIT;\n \tconst char *end = atom + strlen(atom);\n-\treturn parse_ref_filter_atom(atom, end);\n+\treturn parse_ref_filter_atom(&dummy, atom, end);\n }\n \n /* If no sorting option is given, use refname to sort as default */\n-- \n2.13.2.1157.gc6daca446\n\n"},{"id":"324363","messageId":"20170713150703.phnuyezu5lwjtsdi@sigill.intra.peff.net","threadId":"46375","inReplyTo":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","subject":"[PATCH 11/15] color: check color.ui in git_default_config()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T15:07:03Z","receivedAt":"2017-07-13T15:07:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Back in prehistoric times, our decision on whether or not to\nshow color by default relied on using a config callback that\neither did or didn't load color config like color.diff.\nWhen we introduced color.ui, we put it in the same boat:\ncommands had to manually respect it by using git_color_config()\nor its git_color_default_config() convenience wrapper.\n\nBut in 4c7f1819b (make color.ui default to 'auto',\n2013-06-10), that changed. Since then, we default color.ui\nto auto in all programs, meaning that even plumbing commands\nlike \"git diff-tree --pretty\" might colorize the output.\nNobody seems to have complained in the intervening years,\npresumably because the \"is stdout a tty\" check does a good\njob of catching the right cases.\n\nBut that leaves an interesting curiosity: color.ui defaults\nto auto even in plumbing, but you can't actually _disable_\nthe color via config. So if you really hate color and set\n\"color.ui\" to false, diff-tree will still show color (but\nporcelain like git-diff won't). Nobody noticed that either,\nprobably because very few people disable color.\n\nOne could argue that the plumbing should _always_ disable\ncolor unless an explicit --color option is given on the\ncommand line. But in practice, this creates a lot of\ncomplications for scripts which do want plumbing to show\nuser-visible output. They can't just pass \"--color\" blindly;\nthey need to check the user's config and decide what to\nsend.\n\nGiven that nobody has complained about the current behavior,\nlet's assume it's a good path, and follow it to its\nconclusion: supporting color.ui everywhere.\n\nNote that you can create havoc by setting color.ui=always in\nyour config, but that's more or less already the case. We\ncould disallow it entirely, but it is handy for one-offs\nlike:\n\n git -c color.ui=always foo >not-a-tty\n\nwhen \"foo\" does not take a --color option itself.\n\nSigned-off-by: Jeff King \n---\n builtin/branch.c | 2 +-\n builtin/clean.c | 3 +--\n builtin/grep.c | 2 +-\n builtin/show-branch.c | 2 +-\n color.c | 8 --------\n config.c | 4 ++++\n diff.c | 3 ---\n 7 files changed, 8 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 036fdc929..d852ded49 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -92,7 +92,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)\n \t\t\treturn config_error_nonbool(var);\n \t\treturn color_parse(value, branch_colors[slot]);\n \t}\n-\treturn git_color_default_config(var, value, cb);\n+\treturn git_default_config(var, value, cb);\n }\n \n static const char *branch_get_color(enum color_branch ix)\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 057fc97fe..c1bafda5b 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -125,8 +125,7 @@ static int git_clean_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\t/* inspect the color.ui config variable and others */\n-\treturn git_color_default_config(var, value, cb);\n+\treturn git_default_config(var, value, cb);\n }\n \n static const char *clean_get_color(enum color_clean ix)\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 0d6e66973..a7157f563 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -284,7 +284,7 @@ static int wait_all(void)\n static int grep_cmd_config(const char *var, const char *value, void *cb)\n {\n \tint st = grep_config(var, value, cb);\n-\tif (git_color_default_config(var, value, cb) < 0)\n+\tif (git_default_config(var, value, cb) < 0)\n \t\tst = -1;\n \n \tif (!strcmp(var, \"grep.threads\")) {\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 7073a3eb9..28f245c8c 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -554,7 +554,7 @@ static int git_show_branch_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\treturn git_color_default_config(var, value, cb);\n+\treturn git_default_config(var, value, cb);\n }\n \n static int omit_in_dense(struct commit *commit, struct commit **rev, int n)\ndiff --git a/color.c b/color.c\nindex 31b6207a0..7aa8b076f 100644\n--- a/color.c\n+++ b/color.c\n@@ -361,14 +361,6 @@ int git_color_config(const char *var, const char *value, void *cb)\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-\n void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb)\n {\n \tif (*color)\ndiff --git a/config.c b/config.c\nindex a9356c138..bc290e756 100644\n--- a/config.c\n+++ b/config.c\n@@ -16,6 +16,7 @@\n #include \"string-list.h\"\n #include \"utf8.h\"\n #include \"dir.h\"\n+#include \"color.h\"\n \n struct config_source {\n \tstruct config_source *prev;\n@@ -1350,6 +1351,9 @@ int git_default_config(const char *var, const char *value, void *dummy)\n \tif (starts_with(var, \"advice.\"))\n \t\treturn git_default_advice_config(var, value);\n \n+\tif (git_color_config(var, value, dummy) < 0)\n+\t\treturn -1;\n+\n \tif (!strcmp(var, \"pager.color\") || !strcmp(var, \"color.pager\")) {\n \t\tpager_use_color = git_config_bool(var,value);\n \t\treturn 0;\ndiff --git a/diff.c b/diff.c\nindex 85e714f6c..9c3825803 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -299,9 +299,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\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-- \n2.13.2.1157.gc6daca446\n\n"},{"id":"324364","messageId":"20170713150714.p6272nblhg53sdrd@sigill.intra.peff.net","threadId":"46375","inReplyTo":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","subject":"[PATCH 12/15] for-each-ref: load config earlier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T15:07:14Z","receivedAt":"2017-07-13T15:07:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In most commands we load config before parsing command line\noptions, since it lets the latter override the former with a\nsimple variable assignment. In the case of for-each-ref,\nthough, we do it in the reverse order. This is OK with\nthe current code, since there's no interaction between the\nconfig and command-line options.\n\nHowever, as the ref-filter code starts to care about config\nduring verify_ref_format(), we'll want to make sure the\nconfig is loaded. Let's bump the config to the usual spot\nnear the top of the function.\n\nWe can drop the comment there; it's impossible to keep a\n\"why we load the config\" comment like this up to date with\nevery config option we might be interested in. And indeed,\nit's already stale; we'd care about core.abbrev, for\ninstance, when %(objectname:short) is used.\n\nSigned-off-by: Jeff King \n---\n builtin/for-each-ref.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex f47066b42..5d7c921a7 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -54,6 +54,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \n \tformat.format = \"%(objectname) %(objecttype)\\t%(refname)\";\n \n+\tgit_config(git_default_config, NULL);\n+\n \tparse_options(argc, argv, prefix, opts, for_each_ref_usage, 0);\n \tif (maxcount < 0) {\n \t\terror(\"invalid --count argument: `%d'\", maxcount);\n@@ -71,9 +73,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tsorting->ignore_case = icase;\n \tfilter.ignore_case = icase;\n \n-\t/* for warn_ambiguous_refs */\n-\tgit_config(git_default_config, NULL);\n-\n \tfilter.name_patterns = argv;\n \tfilter.match_as_path = 1;\n \tfilter_refs(&array, &filter, FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN);\n-- \n2.13.2.1157.gc6daca446\n\n"},{"id":"324365","messageId":"20170713150729.fccsplqohebnqb4l@sigill.intra.peff.net","threadId":"46375","inReplyTo":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","subject":"[PATCH 13/15] rev-list: pass diffopt->use_colors through to pretty-print","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T15:07:30Z","receivedAt":"2017-07-13T15:07:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When rev-list pretty-prints a commit, it creates a new\npretty_print_context and copies items from the rev_info\nstruct. We don't currently copy the \"use_color\" field,\nthough. Nobody seems to have noticed because the only part\nof pretty.c that cares is the %C(auto,...) placeholder, and\npresumably not many people use that with the rev-list\nplumbing (as opposed to with git-log).\n\nIt will become more noticeable in a future patch, though,\nwhen we start treating all user-format colors as auto-colors\n(in which case it would become impossible to format colors\nwith rev-list, even with --color=always).\n\nSigned-off-by: Jeff King \n---\n builtin/rev-list.c | 1 +\n t/t6006-rev-list-format.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 95d84d5cd..fee10d856 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -122,6 +122,7 @@ static void show_commit(struct commit *commit, void *data)\n \t\tctx.date_mode_explicit = revs->date_mode_explicit;\n \t\tctx.fmt = revs->commit_format;\n \t\tctx.output_encoding = get_log_output_encoding();\n+\t\tctx.color = revs->diffopt.use_color;\n \t\tpretty_print_commit(&ctx, commit, &buf);\n \t\tif (buf.len) {\n \t\t\tif (revs->commit_format != CMIT_FMT_ONELINE)\ndiff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\nindex 647218b4e..7b97a90ba 100755\n--- a/t/t6006-rev-list-format.sh\n+++ b/t/t6006-rev-list-format.sh\n@@ -245,6 +245,17 @@ test_expect_success '%C(auto) respects --no-color' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rev-list %C(auto,...) respects --color' '\n+\tgit rev-list --color --format=\"%C(auto,green)foo%C(auto,reset)\" \\\n+\t\t-1 HEAD >actual.raw &&\n+\ttest_decode_color actual &&\n+\tcat >expect <<-EOF &&\n+\tcommit $(git rev-parse HEAD)\n+\tfoo\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n iconv -f utf-8 -t $test_encoding > commit-msg <\n---\nThe test diff is a lot easier to read with \"-w\", as it moves all of the\n%C(auto,...) tests into a loop that checks all three variants.\n\n Documentation/pretty-formats.txt | 18 +++++----\n pretty.c | 27 +++++++++++--\n t/t6006-rev-list-format.sh | 84 ++++++++++++++++++++++++----------------\n 3 files changed, 84 insertions(+), 45 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 4d6dac577..973d19606 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -173,13 +173,17 @@ endif::git-rev-list[]\n - '%Cblue': switch color to blue\n - '%Creset': reset color\n - '%C(...)': color specification, as described under Values in the\n- \"CONFIGURATION FILE\" section of linkgit:git-config[1];\n- adding `auto,` at the beginning (e.g. `%C(auto,red)`) will emit\n- color only when colors are enabled for log output (by `color.diff`,\n- `color.ui`, or `--color`, and respecting the `auto` settings of the\n- former if we are going to a terminal). `auto` alone (i.e.\n- `%C(auto)`) will turn on auto coloring on the next placeholders\n- until the color is switched again.\n+ \"CONFIGURATION FILE\" section of linkgit:git-config[1].\n+ By default, colors are shown only when enabled for log output (by\n+ `color.diff`, `color.ui`, or `--color`, and respecting the `auto`\n+ settings of the former if we are going to a terminal). `%C(auto,...)`\n+ is accepted as a historical synonym for the default (e.g.,\n+ `%C(auto,red)`). Specifying `%C(always,...) will show the colors\n+ even when color is not otherwise enabled (though consider\n+ just using `--color=always` to enable color for the whole output,\n+ including this format and anything else git might color). `auto`\n+ alone (i.e. `%C(auto)`) will turn on auto coloring on the next\n+ placeholders until the color is switched again.\n - '%m': left (`<`), right (`>`) or boundary (`-`) mark\n - '%n': newline\n - '%%': a raw '%'\ndiff --git a/pretty.c b/pretty.c\nindex e4b561c58..fd781e54f 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -947,6 +947,7 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n \t\t\t struct format_commit_context *c)\n {\n \tconst char *rest = placeholder;\n+\tconst char *basic_color = NULL;\n \n \tif (placeholder[1] == '(') {\n \t\tconst char *begin = placeholder + 2;\n@@ -955,23 +956,41 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n \n \t\tif (!end)\n \t\t\treturn 0;\n+\n \t\tif (skip_prefix(begin, \"auto,\", &begin)) {\n \t\t\tif (!want_color(c->pretty_ctx->color))\n \t\t\t\treturn end - placeholder + 1;\n+\t\t} else if(skip_prefix(begin, \"always,\", &begin)) {\n+\t\t\t/* nothing to do; we do not respect want_color at all */\n+\t\t} else {\n+\t\t\t/* the default is the same as \"auto\" */\n+\t\t\tif (!want_color(c->pretty_ctx->color))\n+\t\t\t\treturn end - placeholder + 1;\n \t\t}\n+\n \t\tif (color_parse_mem(begin, end - begin, color) < 0)\n \t\t\tdie(_(\"unable to parse --pretty format\"));\n \t\tstrbuf_addstr(sb, color);\n \t\treturn end - placeholder + 1;\n \t}\n+\n+\t/*\n+\t * We handle things like \"%C(red)\" above; for historical reasons, there\n+\t * are a few colors that can be specified without parentheses (and\n+\t * they cannot support things like \"auto\" or \"always\" at all).\n+\t */\n \tif (skip_prefix(placeholder + 1, \"red\", &rest))\n-\t\tstrbuf_addstr(sb, GIT_COLOR_RED);\n+\t\tbasic_color = GIT_COLOR_RED;\n \telse if (skip_prefix(placeholder + 1, \"green\", &rest))\n-\t\tstrbuf_addstr(sb, GIT_COLOR_GREEN);\n+\t\tbasic_color = GIT_COLOR_GREEN;\n \telse if (skip_prefix(placeholder + 1, \"blue\", &rest))\n-\t\tstrbuf_addstr(sb, GIT_COLOR_BLUE);\n+\t\tbasic_color = GIT_COLOR_BLUE;\n \telse if (skip_prefix(placeholder + 1, \"reset\", &rest))\n-\t\tstrbuf_addstr(sb, GIT_COLOR_RESET);\n+\t\tbasic_color = GIT_COLOR_RESET;\n+\n+\tif (basic_color && want_color(c->pretty_ctx->color))\n+\t\tstrbuf_addstr(sb, basic_color);\n+\n \treturn rest - placeholder;\n }\n \ndiff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\nindex 7b97a90ba..b326d550f 100755\n--- a/t/t6006-rev-list-format.sh\n+++ b/t/t6006-rev-list-format.sh\n@@ -59,7 +59,10 @@ test_format () {\n }\n \n # Feed to --format to provide predictable colored sequences.\n+BASIC_COLOR='%Credfoo%Creset'\n+COLOR='%C(red)foo%C(reset)'\n AUTO_COLOR='%C(auto,red)foo%C(auto,reset)'\n+ALWAYS_COLOR='%C(always,red)foo%C(always,reset)'\n has_color () {\n \ttest_decode_color <\"$1\" >decoded &&\n \techo \"foo\" >expect &&\n@@ -177,7 +180,7 @@ test_expect_success 'basic colors' '\n \tfoobarbazxyzzy\n \tEOF\n \tformat=\"%Credfoo%Cgreenbar%Cbluebaz%Cresetxyzzy\" &&\n-\tgit rev-list --format=\"$format\" -1 master >actual.raw &&\n+\tgit rev-list --color --format=\"$format\" -1 master >actual.raw &&\n \ttest_decode_color actual &&\n \ttest_cmp expect actual\n '\n@@ -188,48 +191,61 @@ test_expect_success 'advanced colors' '\n \tfoo\n \tEOF\n \tformat=\"%C(red yellow bold)foo%C(reset)\" &&\n-\tgit rev-list --format=\"$format\" -1 master >actual.raw &&\n+\tgit rev-list --color --format=\"$format\" -1 master >actual.raw &&\n \ttest_decode_color actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success '%C(auto,...) does not enable color by default' '\n-\tgit log --format=$AUTO_COLOR -1 >actual &&\n-\thas_no_color actual\n-'\n-\n-test_expect_success '%C(auto,...) enables colors for color.diff' '\n-\tgit -c color.diff=always log --format=$AUTO_COLOR -1 >actual &&\n-\thas_color actual\n-'\n-\n-test_expect_success '%C(auto,...) enables colors for color.ui' '\n-\tgit -c color.ui=always log --format=$AUTO_COLOR -1 >actual &&\n-\thas_color actual\n-'\n+for spec in \\\n+\t\"%Cred:$BASIC_COLOR\" \\\n+\t\"%C(...):$COLOR\" \\\n+\t\"%C(auto,...):$AUTO_COLOR\"\n+do\n+\tdesc=${spec%%:*}\n+\tcolor=${spec#*:}\n+\ttest_expect_success \"$desc does not enable color by default\" '\n+\t\tgit log --format=$color -1 >actual &&\n+\t\thas_no_color actual\n+\t'\n \n-test_expect_success '%C(auto,...) respects --color' '\n-\tgit log --format=$AUTO_COLOR -1 --color >actual &&\n-\thas_color actual\n-'\n+\ttest_expect_success \"$desc enables colors for color.diff\" '\n+\t\tgit -c color.diff=always log --format=$color -1 >actual &&\n+\t\thas_color actual\n+\t'\n \n-test_expect_success '%C(auto,...) respects --no-color' '\n-\tgit -c color.ui=always log --format=$AUTO_COLOR -1 --no-color >actual &&\n-\thas_no_color actual\n-'\n+\ttest_expect_success \"$desc enables colors for color.ui\" '\n+\t\tgit -c color.ui=always log --format=$color -1 >actual &&\n+\t\thas_color actual\n+\t'\n \n-test_expect_success TTY '%C(auto,...) respects --color=auto (stdout is tty)' '\n-\ttest_terminal env TERM=vt100 \\\n-\t\tgit log --format=$AUTO_COLOR -1 --color=auto >actual &&\n-\thas_color actual\n-'\n+\ttest_expect_success \"$desc respects --color\" '\n+\t\tgit log --format=$color -1 --color >actual &&\n+\t\thas_color actual\n+\t'\n \n-test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' '\n-\t(\n-\t\tTERM=vt100 && export TERM &&\n-\t\tgit log --format=$AUTO_COLOR -1 --color=auto >actual &&\n+\ttest_expect_success \"$desc respects --no-color\" '\n+\t\tgit -c color.ui=always log --format=$color -1 --no-color >actual &&\n \t\thas_no_color actual\n-\t)\n+\t'\n+\n+\ttest_expect_success TTY \"$desc respects --color=auto (stdout is tty)\" '\n+\t\ttest_terminal env TERM=vt100 \\\n+\t\t\tgit log --format=$color -1 --color=auto >actual &&\n+\t\thas_color actual\n+\t'\n+\n+\ttest_expect_success \"$desc respects --color=auto (stdout not tty)\" '\n+\t\t(\n+\t\t\tTERM=vt100 && export TERM &&\n+\t\t\tgit log --format=$color -1 --color=auto >actual &&\n+\t\t\thas_no_color actual\n+\t\t)\n+\t'\n+done\n+\n+test_expect_success '%C(always,...) enables color even without tty' '\n+\tgit log --format=$ALWAYS_COLOR -1 >actual &&\n+\thas_color actual\n '\n \n test_expect_success '%C(auto) respects --color' '\n-- \n2.13.2.1157.gc6daca446\n\n"},{"id":"324367","messageId":"20170713150932.y6i7un63b5fzmaei@sigill.intra.peff.net","threadId":"46375","inReplyTo":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","subject":"[PATCH 15/15] ref-filter: consult want_color() before emitting colors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T15:09:32Z","receivedAt":"2017-07-13T15:09:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When color placeholders like %(color:red) are used in a\nref-filter format, we unconditionally output the colors,\neven if the user has asked us for no colors. This usually\nisn't a problem when the user is constructing a --format on\nthe command line, but it means we may do the wrong thing\nwhen the format is fed from a script or alias. For example:\n\n $ git config alias.b 'branch --format=%(color:green)%(refname)'\n $ git b --no-color\n\nshould probably omit the green color. Likewise, running:\n\n $ git b >branches\n\nshould probably also omit the color, just as we would for\nall baked-in coloring (and as we recently started to do for\nuser-specified colors in --pretty formats).\n\nThis commit makes both of those cases work by teaching\nthe ref-filter code to consult want_color() before\noutputting any color. The color flag in ref_format defaults\nto \"-1\", which means we'll consult color.ui, which in turn\ndefaults to the usual isatty() check on stdout. However,\ncallers like git-branch which support their own color config\n(and command-line options) can override that.\n\nThe new tests independently cover all three of the callers\nof ref-filter (for-each-ref, tag, and branch). Even though\nthese seem redundant, it confirms that we've correctly\nplumbed through all of the necessary config to make colors\nwork by default.\n\nSigned-off-by: Jeff King \n---\nThe \"correctly plumbed\" bits happened in the earlier patches. But it was\nthese tests that showed me they were broken.\n\n builtin/branch.c | 1 +\n ref-filter.c | 8 ++++++++\n ref-filter.h | 3 ++-\n t/t3203-branch-output.sh | 31 +++++++++++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 37 ++++++++++++++++++++++++++-----------\n t/t7004-tag.sh | 25 +++++++++++++++++++++++++\n 6 files changed, 93 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex d852ded49..16d391b40 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -409,6 +409,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \n \tif (!format->format)\n \t\tformat->format = to_free = build_format(filter, maxwidth, remote_prefix);\n+\tformat->use_color = branch_use_color;\n \n \tif (verify_ref_format(format))\n \t\tdie(_(\"unable to parse format string\"));\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 129a63677..bc591f4f3 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -104,6 +104,12 @@ static void color_atom_parser(const struct ref_format *format, struct used_atom\n \t\tdie(_(\"expected format: %%(color:)\"));\n \tif (color_parse(color_value, atom->u.color) < 0)\n \t\tdie(_(\"unrecognized color: %%(color:%s)\"), color_value);\n+\t/*\n+\t * We check this after we've parsed the color, which lets us complain\n+\t * about syntactically bogus color names even if they won't be used.\n+\t */\n+\tif (!want_color(format->use_color))\n+\t\tcolor_parse(\"\", atom->u.color);\n }\n \n static void refname_atom_parser_internal(struct refname_atom *atom,\n@@ -675,6 +681,8 @@ int verify_ref_format(struct ref_format *format)\n \t\tif (skip_prefix(used_atom[at].name, \"color:\", &color))\n \t\t\tformat->need_color_reset_at_eol = !!strcmp(color, \"reset\");\n \t}\n+\tif (format->need_color_reset_at_eol && !want_color(format->use_color))\n+\t\tformat->need_color_reset_at_eol = 0;\n \treturn 0;\n }\n \ndiff --git a/ref-filter.h b/ref-filter.h\nindex 1ffc3ca81..0d98342b3 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -79,12 +79,13 @@ struct ref_format {\n \t */\n \tconst char *format;\n \tint quote_style;\n+\tint use_color;\n \n \t/* Internal state to ref-filter */\n \tint need_color_reset_at_eol;\n };\n \n-#define REF_FORMAT_INIT { NULL, 0 }\n+#define REF_FORMAT_INIT { NULL, 0, -1 }\n \n /* Macros for checking --merged and --no-merged options */\n #define _OPT_MERGED_NO_MERGED(option, filter, h) \\\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex a428ae670..d2aec0f38 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -2,6 +2,7 @@\n \n test_description='git branch display tests'\n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n test_expect_success 'make commits' '\n \techo content >file &&\n@@ -239,4 +240,34 @@ test_expect_success 'git branch --format option' '\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success \"set up color tests\" '\n+\techo \"master\" >expect.color &&\n+\techo \"master\" >expect.bare &&\n+\tcolor_args=\"--format=%(color:red)%(refname:short) --list master\"\n+'\n+\n+test_expect_success '%(color) omitted without tty' '\n+\tTERM=vt100 git branch $color_args >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expect.bare actual\n+'\n+\n+test_expect_success TTY '%(color) present with tty' '\n+\ttest_terminal env TERM=vt100 git branch $color_args >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expect.color actual\n+'\n+\n+test_expect_success 'color.branch=always overrides auto-color' '\n+\tgit -c color.branch=always branch $color_args >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expect.color actual\n+'\n+\n+test_expect_success '--color overrides auto-color' '\n+\tgit branch --color $color_args >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expect.color actual\n+'\n+\n test_done\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 7872a2f54..2274a4b73 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -7,6 +7,7 @@ test_description='for-each-ref test'\n \n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n+. \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n # Mon Jul 3 23:18:43 2006 +0000\n datestamp=1151968723\n@@ -412,19 +413,33 @@ test_expect_success 'Check for invalid refname format' '\n \ttest_must_fail git for-each-ref --format=\"%(refname:INVALID)\"\n '\n \n-cat >expected <master\n-$(git rev-parse --short refs/remotes/origin/master) origin/master\n-$(git rev-parse --short refs/tags/testtag) testtag\n-$(git rev-parse --short refs/tags/two) two\n-EOF\n+test_expect_success 'set up color tests' '\n+\tcat >expected.color <<-EOF &&\n+\t$(git rev-parse --short refs/heads/master) master\n+\t$(git rev-parse --short refs/remotes/origin/master) origin/master\n+\t$(git rev-parse --short refs/tags/testtag) testtag\n+\t$(git rev-parse --short refs/tags/two) two\n+\tEOF\n+\tsed \"s/<[^>]*>//g\" expected.bare &&\n+\tcolor_format=\"%(objectname:short) %(color:green)%(refname:short)\"\n+'\n \n-test_expect_success 'Check %(color:...) ' '\n-\tgit for-each-ref \\\n-\t\t--format=\"%(objectname:short) %(color:green)%(refname:short)\" \\\n-\t\t>actual.raw &&\n+test_expect_success TTY '%(color) shows color with a tty' '\n+\ttest_terminal env TERM=vt100 \\\n+\t\tgit for-each-ref --format=\"$color_format\" >actual.raw &&\n \ttest_decode_color actual &&\n-\ttest_cmp expected actual\n+\ttest_cmp expected.color actual\n+'\n+\n+test_expect_success '%(color) does not show color without tty' '\n+\tTERM=vt100 git for-each-ref --format=\"$color_format\" >actual &&\n+\ttest_cmp expected.bare actual\n+'\n+\n+test_expect_success 'color.ui=always can override tty check' '\n+\tgit -c color.ui=always for-each-ref --format=\"$color_format\" >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expected.color actual\n '\n \n cat >expected <<\\EOF\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 0ef7b9439..dd5ba450e 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -9,6 +9,7 @@ Tests for operations with tags.'\n \n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n+. \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n # creating and listing lightweight tags:\n \n@@ -1900,6 +1901,30 @@ test_expect_success '--format should list tags as per format given' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success \"set up color tests\" '\n+\techo \"v1.0\" >expect.color &&\n+\techo \"v1.0\" >expect.bare &&\n+\tcolor_args=\"--format=%(color:red)%(refname:short) --list v1.0\"\n+'\n+\n+test_expect_success '%(color) omitted without tty' '\n+\tTERM=vt100 git tag $color_args >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expect.bare actual\n+'\n+\n+test_expect_success TTY '%(color) present with tty' '\n+\ttest_terminal env TERM=vt100 git tag $color_args >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expect.color actual\n+'\n+\n+test_expect_success 'color.ui=always overrides auto-color' '\n+\tgit -c color.ui=always tag $color_args >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expect.color actual\n+'\n+\n test_expect_success 'setup --merged test tags' '\n \tgit tag mergetest-1 HEAD~2 &&\n \tgit tag mergetest-2 HEAD~1 &&\n-- \n2.13.2.1157.gc6daca446\n"},{"id":"324407","messageId":"CAGZ79kaE7wjFR-=DiPU5xwknS1nxDHR5+S6+UGPFTF0uNBA3dQ@mail.gmail.com","threadId":"46375","inReplyTo":"20170713145841.4win5tbx4efwjsa2@sigill.intra.peff.net","subject":"Re: [PATCH 03/15] t: use test_decode_color rather than literal ANSI codes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-13T18:40:32Z","receivedAt":"2017-07-13T18:40:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 13, 2017 at 7:58 AM, Jeff King wrote:\n\n> I really only need t6300 and t6006 converted to build on for the rest of\n> the series. But t4207 was easy to do. t4026 still uses raw codes, but\n> converting it would be a pretty big job, so I punted.\n>\n\nI think it is good to have raw codes in at least one place to test\nthat raw codes work, but then again it could be specific test calling\nout that this is tested.\n\n> @@ -59,7 +54,8 @@ EOF\n> # to this test since it does not contain any decoration, hence --first-parent\n> test_expect_success 'Commit Decorations Colored Correctly' '\n> git log --first-parent --abbrev=10 --all --decorate --oneline --color=always |\n> - sed \"s/[0-9a-f]\\{10,10\\}/COMMIT_ID/\" >out &&\n> + sed \"s/[0-9a-f]\\{10,10\\}/COMMIT_ID/\" |\n> + test_decode_color >out &&\n\nJust some thoughts:\n\nThis extension of the pipe-chain is not making it worse as gits exit code\nis already hidden.\n\nThe sed \"s/[0-9a-f]\\{10,10\\}/COMMIT_ID/\" is sort of funny, because\nI would have expected it to break in the future with e.g. the sha1 to longer\nhash conversion. But as we specify --abbrev=10, this seems future proof.\nIn an ideal world this would be encapsulated in a function (c.f. t/diff-lib.sh).\n\n\n\n> @@ -61,8 +61,9 @@ test_format () {\n> # Feed to --format to provide predictable colored sequences.\n> AUTO_COLOR='%C(auto,red)foo%C(auto,reset)'\n> has_color () {\n> - printf '\\033[31mfoo\\033[m\\n' >expect &&\n> - test_cmp expect \"$1\"\n> + test_decode_color <\"$1\" >decoded &&\n> + echo \"foo\" >expect &&\n> + test_cmp expect decoded\n> }\n\nThanks for removing hard coded colors :)\n"},{"id":"324409","messageId":"20170713184541.ferc3eif7j4mazpg@sigill.intra.peff.net","threadId":"46375","inReplyTo":"CAGZ79kaE7wjFR-=DiPU5xwknS1nxDHR5+S6+UGPFTF0uNBA3dQ@mail.gmail.com","subject":"Re: [PATCH 03/15] t: use test_decode_color rather than literal ANSI codes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T18:45:41Z","receivedAt":"2017-07-13T18:45:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 13, 2017 at 11:40:32AM -0700, Stefan Beller wrote:\n\n> On Thu, Jul 13, 2017 at 7:58 AM, Jeff King wrote:\n> \n> > I really only need t6300 and t6006 converted to build on for the rest of\n> > the series. But t4207 was easy to do. t4026 still uses raw codes, but\n> > converting it would be a pretty big job, so I punted.\n> \n> I think it is good to have raw codes in at least one place to test\n> that raw codes work, but then again it could be specific test calling\n> out that this is tested.\n\nYeah, that thought crossed my mind, too. t4026 would definitely be the\nplace for it, as it is about exhaustively testing the various colors.\nThe others are just about feeding color codes through config and\nuser-formats.\n\n> > @@ -59,7 +54,8 @@ EOF\n> > # to this test since it does not contain any decoration, hence --first-parent\n> > test_expect_success 'Commit Decorations Colored Correctly' '\n> > git log --first-parent --abbrev=10 --all --decorate --oneline --color=always |\n> > - sed \"s/[0-9a-f]\\{10,10\\}/COMMIT_ID/\" >out &&\n> > + sed \"s/[0-9a-f]\\{10,10\\}/COMMIT_ID/\" |\n> > + test_decode_color >out &&\n> \n> Just some thoughts:\n> \n> This extension of the pipe-chain is not making it worse as gits exit code\n> is already hidden.\n\nYes, I noticed the existing pipe-chain. We can fix it while we're here,\nthough I think it's not a big deal in practice.\n\n> The sed \"s/[0-9a-f]\\{10,10\\}/COMMIT_ID/\" is sort of funny, because\n> I would have expected it to break in the future with e.g. the sha1 to longer\n> hash conversion. But as we specify --abbrev=10, this seems future proof.\n> In an ideal world this would be encapsulated in a function (c.f. t/diff-lib.sh).\n\nI agree it's a bit gross. Possibly:\n\n git log --format='%C(auto)%d %s'\n\nwould be easier for the test to parse (I'm pretty sure that didn't exist\nback when this test was written).\n\n-Peff\n"},{"id":"324410","messageId":"CAGZ79kag1B37FBrmDzbRFNVODHp=n1h=xSq_pi1b7Fs4wLoRBg@mail.gmail.com","threadId":"46375","inReplyTo":"20170713150118.eof3xgu4zujmo6u6@sigill.intra.peff.net","subject":"Re: [PATCH 05/15] ref-filter: abstract ref format into its own struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-13T18:51:44Z","receivedAt":"2017-07-13T18:51:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 13, 2017 at 8:01 AM, Jeff King wrote:\n\n> builtin/branch.c | 14 +++++++-------\n> builtin/for-each-ref.c | 22 ++++++++++++----------\n> builtin/tag.c | 30 ++++++++++++++++--------------\n> builtin/verify-tag.c | 12 ++++++------\n> ref-filter.c | 22 ++++++++++++----------\n> ref-filter.h | 22 +++++++++++++++++-----\n> 6 files changed, 70 insertions(+), 52 deletions(-)\n\nThe patch looks good to me. So some off-topic comments:\nI reviewed this patch from bottom up, i.e. I started looking at\nref-filter.h, then ref-filter.c and then the rest. If only you had formatted\nthe patches with an orderfile. ;)\n"},{"id":"324416","messageId":"CAGZ79kY_Vt5hK0FbzJgSZOxhNMKJ0f8N+uSmTTuF8nXu0vA-Xg@mail.gmail.com","threadId":"46375","inReplyTo":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","subject":"Re: [PATCH 0/15] making user-format colors conditional on config/tty","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-13T19:14:26Z","receivedAt":"2017-07-13T19:14:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 13, 2017 at 7:55 AM, Jeff King wrote:\n> This is a cleanup of the patch I posted last October:\n>\n> https://public-inbox.org/git/20161010151517.6wszhuyp57yfncaj@sigill.intra.peff.net/\n>\n> The general idea is that it's rather confusing that \"%C(red)\" in a\n> pretty-print format does not currently respect color.ui, --no-color, or\n> the usual isatty check on stdout. This series changes that. Note that\n> this is a backwards-incompatible change, but the general sentiment in\n> that earlier thread seemed to be that the existing behavior is arguably\n> buggy. See patch 14 for more discussion.\n>\n> The patch stalled back then because I wanted to make sure that\n> ref-filter's color placeholders behaved the same. That required some\n> refactoring which conflicted badly with kn/ref-filter-branch-list. Now\n> that it has graduated, I was able to rebase on top.\n>\n> This version also takes into account feedback from the original thread.\n> And as I added tests, it surfaced a few corner cases around color config\n> that I've dealt with here. The last two patches are the most\n> interesting bits.\n>\n\nI have reviewed these slightly and think this series is a good change.\n\nThanks,\nStefan\n"},{"id":"324420","messageId":"xmqqeftkw30y.fsf@gitster.mtv.corp.google.com","threadId":"46375","inReplyTo":"20170713184541.ferc3eif7j4mazpg@sigill.intra.peff.net","subject":"Re: [PATCH 03/15] t: use test_decode_color rather than literal ANSI codes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-13T19:27:41Z","receivedAt":"2017-07-13T19:27:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > @@ -59,7 +54,8 @@ EOF\n>> > # to this test since it does not contain any decoration, hence --first-parent\n>> > test_expect_success 'Commit Decorations Colored Correctly' '\n>> > git log --first-parent --abbrev=10 --all --decorate --oneline --color=always |\n>> > - sed \"s/[0-9a-f]\\{10,10\\}/COMMIT_ID/\" >out &&\n>> > + sed \"s/[0-9a-f]\\{10,10\\}/COMMIT_ID/\" |\n>> > + test_decode_color >out &&\n>> \n>> Just some thoughts:\n>> \n>> This extension of the pipe-chain is not making it worse as gits exit code\n>> is already hidden.\n>\n> Yes, I noticed the existing pipe-chain. We can fix it while we're here,\n> though I think it's not a big deal in practice.\n>\n>> The sed \"s/[0-9a-f]\\{10,10\\}/COMMIT_ID/\" is sort of funny, because\n>> I would have expected it to break in the future with e.g. the sha1 to longer\n>> hash conversion. But as we specify --abbrev=10, this seems future proof.\n>> In an ideal world this would be encapsulated in a function (c.f. t/diff-lib.sh).\n\nActually, --abbrev=10 does not guarantee that the hex part is always\n10 bytes long, so it is not future-proofing, but I expect it would\nwork out fine in practice.\n\n> I agree it's a bit gross. Possibly:\n>\n> git log --format='%C(auto)%d %s'\n>\n> would be easier for the test to parse (I'm pretty sure that didn't exist\n> back when this test was written).\n\nYeah, that may make the test easier to read, too.\n\nThanks.\n"},{"id":"324436","messageId":"xmqqwp7cum8k.fsf@gitster.mtv.corp.google.com","threadId":"46375","inReplyTo":"20170713145620.pwsyvsudtnyfcvxg@sigill.intra.peff.net","subject":"Re: [PATCH 02/15] docs/for-each-ref: update pointer to color syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-13T20:15:39Z","receivedAt":"2017-07-13T20:15:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The documentation for the %(color) placeholder refers to the\n> color.branch.* config for more details. But those details\n> moved to their own section in b92c1a28f\n> (Documentation/config.txt: describe 'color' value type in\n> the \"Values\" section, 2015-03-03). Let's update our\n> pointer. We can steal the text from 30cfe72d3 (pretty: fix\n> document link for color specification, 2016-10-11), which\n> fixed the same problem in a different place.\n\nThanks.\n\n> While we're at it, let's give an example, which makes the\n> syntax much more clear than just the text.\n\nNice. Thanks again.\n\n>\n> Signed-off-by: Jeff King \n> ---\n> Documentation/git-for-each-ref.txt | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n> index 03e187a10..cc42c1283 100644\n> --- a/Documentation/git-for-each-ref.txt\n> +++ b/Documentation/git-for-each-ref.txt\n> @@ -156,8 +156,10 @@ HEAD::\n> \totherwise.\n> \n> color::\n> -\tChange output color. Followed by `:`, where names\n> -\tare described in `color.branch.*`.\n> +\tChange output color. Followed by `:`, where color\n> +\tnames are described under Values in the \"CONFIGURATION FILE\"\n> +\tsection of linkgit:git-config[1]. For example,\n> +\t`%(color:bold red)`.\n> \n> align::\n> \tLeft-, middle-, or right-align the content between\n"},{"id":"324437","messageId":"xmqqshi0um3d.fsf@gitster.mtv.corp.google.com","threadId":"46375","inReplyTo":"20170713145841.4win5tbx4efwjsa2@sigill.intra.peff.net","subject":"Re: [PATCH 03/15] t: use test_decode_color rather than literal ANSI codes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-13T20:18:46Z","receivedAt":"2017-07-13T20:18:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When we put literal ANSI terminal codes into our test\n> scripts, it makes diffs on those scripts hard to read (the\n> colors may be indistinguishable from diff coloring, or in\n> the case of a reset, may not be visible at all).\n>\n> Some scripts get around this by including human-readable\n> names and converting to literal codes with a git-config\n> hack. This makes the actual code diffs look OK, but test_cmp\n> output suffers from the same problem.\n>\n> Let's use test_decode_color instead, which turns the codes\n> into obvious text tags.\n\nNice. Thanks.\n\n"},{"id":"324438","messageId":"xmqqlgnsulco.fsf@gitster.mtv.corp.google.com","threadId":"46375","inReplyTo":"CAGZ79kag1B37FBrmDzbRFNVODHp=n1h=xSq_pi1b7Fs4wLoRBg@mail.gmail.com","subject":"Re: [PATCH 05/15] ref-filter: abstract ref format into its own struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-13T20:34:47Z","receivedAt":"2017-07-13T20:34:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Thu, Jul 13, 2017 at 8:01 AM, Jeff King wrote:\n>\n>> builtin/branch.c | 14 +++++++-------\n>> builtin/for-each-ref.c | 22 ++++++++++++----------\n>> builtin/tag.c | 30 ++++++++++++++++--------------\n>> builtin/verify-tag.c | 12 ++++++------\n>> ref-filter.c | 22 ++++++++++++----------\n>> ref-filter.h | 22 +++++++++++++++++-----\n>> 6 files changed, 70 insertions(+), 52 deletions(-)\n>\n> The patch looks good to me. So some off-topic comments:\n> I reviewed this patch from bottom up, i.e. I started looking at\n> ref-filter.h, then ref-filter.c and then the rest. If only you had formatted\n> the patches with an orderfile. ;)\n\nAs a reviewer, for this particular patchq, I actually appreciated\nthat ref-filter.[ch] came at the end. That forced me to think.\n\nWhen I see something that used to be 0 is lost from the parameter\nlist of show_ref_array_item() at a callsite, I was forced to guess\nwhat it is by looking at what is moved into the new structure\nnearby, and keep doing that \"figure out what is going on\" game until\nthe \"author's answer\" is revealed at the end of the patch. \n\nBy the time I reached ref-filter.[ch], I had a pretty good idea what\nwas done by only looking at the callers and being able to see if my\nunderstanding matched the \"author's answer\" at the end of the patch\nturned out to be a very good way to double-check if the patch was\nsane. If I were given the author's answer upfront, especially an\nanswer by somebody as competent as Peff, I'm sure I would have been\nswayed into believing that whatever is written in the patch must be\ncorrect without thinking the changes needed in the patch myself.\n\nI do want to present from Doc to header to code when I am showing my\npatch to others, so this is probably a good example that illustrates\nthat the preferred presentation order is not just personal\npreference, but is different on occasion even for the same person.\n"},{"id":"324440","messageId":"xmqqh8ygul90.fsf@gitster.mtv.corp.google.com","threadId":"46375","inReplyTo":"20170713150230.sy7zuksbgjcrllxh@sigill.intra.peff.net","subject":"Re: [PATCH 06/15] ref-filter: move need_color_reset_at_eol into ref_format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-13T20:36:59Z","receivedAt":"2017-07-13T20:37:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Calling verify_ref_format() doesn't just confirm that the\n> format is sane; it actually sets some global variables that\n> will be used later when formatting the refs. These logically\n> should belong to the ref_format, which would make it\n> possible to use multiple formats within a single program\n> invocation.\n>\n> Let's move one such flag into the ref_format struct. There\n> are still others that would need to be moved before it would\n> be safe to use multiple formats, but this commit gives a\n> blueprint for how that should look.\n>\n> Signed-off-by: Jeff King \n> ---\n> This commit is strictly optional for this series, but I wanted to give a\n> sense of how the rest of the movement might look, since I was thinking\n> about it. The big thing to move would be the used_atoms array, but I\n> punted on that for now.\n\nHeh, when I saw the hunk at 661,+7, used_atom[] was what immediately\ncame to mind. It is OK to punt for the purpose of this patch, but\nmoving these statics into the ref-format structure would be a good\nmove in the longer term.\n\nThanks.\n\n>\n> ref-filter.c | 7 +++----\n> ref-filter.h | 3 +++\n> 2 files changed, 6 insertions(+), 4 deletions(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 66d234bb1..178396e1f 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -97,7 +97,6 @@ static struct used_atom {\n> \t} u;\n> } *used_atom;\n> static int used_atom_cnt, need_tagged, need_symref;\n> -static int need_color_reset_at_eol;\n> \n> static void color_atom_parser(struct used_atom *atom, const char *color_value)\n> {\n> @@ -661,7 +660,7 @@ int verify_ref_format(struct ref_format *format)\n> {\n> \tconst char *cp, *sp;\n> \n> -\tneed_color_reset_at_eol = 0;\n> +\tformat->need_color_reset_at_eol = 0;\n> \tfor (cp = format->format; *cp && (sp = find_next(cp)); ) {\n> \t\tconst char *color, *ep = strchr(sp, ')');\n> \t\tint at;\n> @@ -673,7 +672,7 @@ int verify_ref_format(struct ref_format *format)\n> \t\tcp = ep + 1;\n> \n> \t\tif (skip_prefix(used_atom[at].name, \"color:\", &color))\n> -\t\t\tneed_color_reset_at_eol = !!strcmp(color, \"reset\");\n> +\t\t\tformat->need_color_reset_at_eol = !!strcmp(color, \"reset\");\n> \t}\n> \treturn 0;\n> }\n> @@ -2083,7 +2082,7 @@ void format_ref_array_item(struct ref_array_item *info,\n> \t\tsp = cp + strlen(cp);\n> \t\tappend_literal(cp, sp, &state);\n> \t}\n> -\tif (need_color_reset_at_eol) {\n> +\tif (format->need_color_reset_at_eol) {\n> \t\tstruct atom_value resetv;\n> \t\tresetv.s = GIT_COLOR_RESET;\n> \t\tappend_atom(&resetv, &state);\n> diff --git a/ref-filter.h b/ref-filter.h\n> index 2bb58879d..9e1e89c19 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -79,6 +79,9 @@ struct ref_format {\n> \t */\n> \tconst char *format;\n> \tint quote_style;\n> +\n> +\t/* Internal state to ref-filter */\n> +\tint need_color_reset_at_eol;\n> };\n> \n> #define REF_FORMAT_INIT { NULL, 0 }\n"},{"id":"324441","messageId":"xmqqd194uktg.fsf@gitster.mtv.corp.google.com","threadId":"46375","inReplyTo":"20170713145553.3epnsw23zajwg3ee@sigill.intra.peff.net","subject":"Re: [PATCH 0/15] making user-format colors conditional on config/tty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-13T20:46:19Z","receivedAt":"2017-07-13T20:46:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This version also takes into account feedback from the original thread.\n> And as I added tests, it surfaced a few corner cases around color config\n> that I've dealt with here. The last two patches are the most\n> interesting bits.\n>\n> [01/15]: check return value of verify_ref_format()\n> [02/15]: docs/for-each-ref: update pointer to color syntax\n> [03/15]: t: use test_decode_color rather than literal ANSI codes\n> [04/15]: ref-filter: simplify automatic color reset\n> [05/15]: ref-filter: abstract ref format into its own struct\n> [06/15]: ref-filter: move need_color_reset_at_eol into ref_format\n> [07/15]: ref-filter: provide a function for parsing sort options\n> [08/15]: ref-filter: make parse_ref_filter_atom a private function\n> [09/15]: ref-filter: factor out the parsing of sorting atoms\n> [10/15]: ref-filter: pass ref_format struct to atom parsers\n> [11/15]: color: check color.ui in git_default_config()\n> [12/15]: for-each-ref: load config earlier\n> [13/15]: rev-list: pass diffopt->use_colors through to pretty-print\n> [14/15]: pretty: respect color settings for %C placeholders\n> [15/15]: ref-filter: consult want_color() before emitting colors\n\nOverall I think the endgame is what we want to have in the future\n(rather, what I wish we had from the beginning). I'd have to think\nabout 11, 14 and 15 a bit more before saying anything that would be\nremotely useful.\n\nThanks.\n\n>\n> Documentation/git-for-each-ref.txt | 6 +-\n> Documentation/pretty-formats.txt | 18 ++++--\n> builtin/branch.c | 21 +++---\n> builtin/clean.c | 3 +-\n> builtin/for-each-ref.c | 27 ++++----\n> builtin/grep.c | 2 +-\n> builtin/rev-list.c | 1 +\n> builtin/show-branch.c | 2 +-\n> builtin/tag.c | 61 ++++++------------\n> builtin/verify-tag.c | 14 ++--\n> color.c | 8 ---\n> config.c | 4 ++\n> diff.c | 3 -\n> pretty.c | 27 ++++++--\n> ref-filter.c | 108 ++++++++++++++++++-------------\n> ref-filter.h | 30 +++++++--\n> t/t3203-branch-output.sh | 31 +++++++++\n> t/t4207-log-decoration-colors.sh | 22 +++----\n> t/t6006-rev-list-format.sh | 129 ++++++++++++++++++++++++-------------\n> t/t6300-for-each-ref.sh | 39 +++++++----\n> t/t7004-tag.sh | 25 +++++++\n> t/test-lib-functions.sh | 1 +\n> 22 files changed, 362 insertions(+), 220 deletions(-)\n>\n> -Peff\n"},{"id":"324453","messageId":"xmqqmv88t444.fsf@gitster.mtv.corp.google.com","threadId":"46375","inReplyTo":"xmqqlgnsulco.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 05/15] ref-filter: abstract ref format into its own struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-13T21:32:27Z","receivedAt":"2017-07-13T21:32:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Stefan Beller writes:\n>\n>> On Thu, Jul 13, 2017 at 8:01 AM, Jeff King wrote:\n>>\n>>> builtin/branch.c | 14 +++++++-------\n>>> builtin/for-each-ref.c | 22 ++++++++++++----------\n>>> builtin/tag.c | 30 ++++++++++++++++--------------\n>>> builtin/verify-tag.c | 12 ++++++------\n>>> ref-filter.c | 22 ++++++++++++----------\n>>> ref-filter.h | 22 +++++++++++++++++-----\n>>> 6 files changed, 70 insertions(+), 52 deletions(-)\n>>\n>> The patch looks good to me. So some off-topic comments:\n>> I reviewed this patch from bottom up, i.e. I started looking at\n>> ref-filter.h, then ref-filter.c and then the rest. If only you had formatted\n>> the patches with an orderfile. ;)\n>\n> As a reviewer, for this particular patchq, I actually appreciated\n> that ref-filter.[ch] came at the end. That forced me to think.\n> ...\n> I do want to present from Doc to header to code when I am showing my\n> patch to others, so this is probably a good example that illustrates\n> that the preferred presentation order is not just personal\n> preference, but is different on occasion even for the same person.\n\nSo when somebody wants to do a \"from design and explanation to\nprovider to consumer\", we would probably want \"doc, *.h, *.c at the\ntop-level and then things inside builtin/ subdirectory\" order. Of\ncourse, on the other hand, \"I do not trust me not getting swayed by\nthe fact that a developer more competent than me wrote the patch\"\nreviewer would want to use the reverse order.\n\nCan we actually express \"top-level first and then builtin/*\" order\nwith the diff.orderfile mechanism? It's been a while since I last\nlooked at the orderfile matching (which was when I originally wrote\nit) and I do not offhand know if we now allow wildmatch patterns and\nthe directory level anchoring \"/*.c\" like we do in .gitignore files,\nwithout which it would be cumbersome to make ref-filter.c listed\nbefore builtin/branch.c in a generic way.\n"},{"id":"324457","messageId":"CAGZ79kaOfcvVGUewVZMyZ7zVUcVESyzsdOBnAvG6FebuZ5HFQw@mail.gmail.com","threadId":"46375","inReplyTo":"xmqqmv88t444.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 05/15] ref-filter: abstract ref format into its own struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-13T22:01:19Z","receivedAt":"2017-07-13T22:01:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 13, 2017 at 2:32 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Stefan Beller writes:\n>>\n>>> On Thu, Jul 13, 2017 at 8:01 AM, Jeff King wrote:\n>>>\n>>>> builtin/branch.c | 14 +++++++-------\n>>>> builtin/for-each-ref.c | 22 ++++++++++++----------\n>>>> builtin/tag.c | 30 ++++++++++++++++--------------\n>>>> builtin/verify-tag.c | 12 ++++++------\n>>>> ref-filter.c | 22 ++++++++++++----------\n>>>> ref-filter.h | 22 +++++++++++++++++-----\n>>>> 6 files changed, 70 insertions(+), 52 deletions(-)\n>>>\n>>> The patch looks good to me. So some off-topic comments:\n>>> I reviewed this patch from bottom up, i.e. I started looking at\n>>> ref-filter.h, then ref-filter.c and then the rest. If only you had formatted\n>>> the patches with an orderfile. ;)\n>>\n>> As a reviewer, for this particular patchq, I actually appreciated\n>> that ref-filter.[ch] came at the end. That forced me to think.\n>> ...\n>> I do want to present from Doc to header to code when I am showing my\n>> patch to others, so this is probably a good example that illustrates\n>> that the preferred presentation order is not just personal\n>> preference, but is different on occasion even for the same person.\n>\n> So when somebody wants to do a \"from design and explanation to\n> provider to consumer\", we would probably want \"doc, *.h, *.c at the\n> top-level and then things inside builtin/ subdirectory\" order. Of\n> course, on the other hand, \"I do not trust me not getting swayed by\n> the fact that a developer more competent than me wrote the patch\"\n> reviewer would want to use the reverse order.\n\nI do not understand what you say here? Are you saying:\n\"I can be tricked easier when the order is top-down,\nspecifically when the more competent developer tries to?\"\n\n> Can we actually express \"top-level first and then builtin/*\" order\n> with the diff.orderfile mechanism?\n\nBy reading the code, I think we snap to the first match. And matching\nis done via the wildmatch.{c,h}, that claims it has special-case '/' matching,\nand '**' ** work differently than '*',\n\n> without which it would be cumbersome to make ref-filter.c listed\n> before builtin/branch.c in a generic way.\n\nIndeed.\n"},{"id":"324461","messageId":"xmqqd194t0qi.fsf@gitster.mtv.corp.google.com","threadId":"46375","inReplyTo":"CAGZ79kaOfcvVGUewVZMyZ7zVUcVESyzsdOBnAvG6FebuZ5HFQw@mail.gmail.com","subject":"Re: [PATCH 05/15] ref-filter: abstract ref format into its own struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-13T22:45:25Z","receivedAt":"2017-07-13T22:45:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> So when somebody wants to do a \"from design and explanation to\n>> provider to consumer\", we would probably want \"doc, *.h, *.c at the\n>> top-level and then things inside builtin/ subdirectory\" order. Of\n>> course, on the other hand, \"I do not trust me not getting swayed by\n>> the fact that a developer more competent than me wrote the patch\"\n>> reviewer would want to use the reverse order.\n>\n> I do not understand what you say here? Are you saying:\n> \"I can be tricked easier when the order is top-down,\n> specifically when the more competent developer tries to?\"\n\nI am not worried about the case in which patch author actively\n\"tries to\" deceive. It is more like \"I am more likely to fail to\nspot mistakes the patch author failed to spot himself\", when I start\nwith reasoning, service provider implementations and then service\nconsumer. When I am forced to think the problem myself before\nseeing the answer and then compare the result with patch author's\nanswer, I am more likely to find such a mistake.\n\n>> Can we actually express \"top-level first and then builtin/*\" order\n>> with the diff.orderfile mechanism?\n>\n> By reading the code, I think we snap to the first match. And matching\n> is done via the wildmatch.{c,h}, that claims it has special-case '/' matching,\n> and '**' ** work differently than '*',\n\nI took a brief look at diffcore-order.c; I do not think \"/*.c\" would\nmatch only top-level .c files like gitignore files would.\n"},{"id":"324498","messageId":"20170714115043.5sqytmol4hwehfwv@sigill.intra.peff.net","threadId":"46375","inReplyTo":"xmqqeftkw30y.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 03/15] t: use test_decode_color rather than literal ANSI codes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-14T11:50:43Z","receivedAt":"2017-07-14T11:50:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 13, 2017 at 12:27:41PM -0700, Junio C Hamano wrote:\n\n> > I agree it's a bit gross. Possibly:\n> >\n> > git log --format='%C(auto)%d %s'\n> >\n> > would be easier for the test to parse (I'm pretty sure that didn't exist\n> > back when this test was written).\n> \n> Yeah, that may make the test easier to read, too.\n\nI started on that, but the test is actually checking the coloring of the\ncommit message, too. So switching to a custom format would lose that\npart of the test.\n\n-Peff\n"}]}