threads / patch / 46375

patch, 15 partsmaking user-format colors conditional on config/tty

Subject: [PATCH 0/15] making user-format colors conditional on config/tty

## tl;dr

30 messages between Jul 13, 2017 and Jul 14, 2017. Diffs are folded; open one to read it.

replies: 29people: 3as markdown or json

Jeff King· Jul 13, 2017, 14:55 UTC · lore
This is a cleanup of the patch I posted last October:
  https://public-inbox.org/git/20161010151517.6wszhuyp57yfncaj@sigill.intra.peff.net/

The general idea is that it's rather confusing that "%C(red)" in a pretty-print format does not currently respect color.ui, --no-color, or the usual isatty check on stdout. This series changes that. Note that this is a backwards-incompatible change, but the general sentiment in that earlier thread seemed to be that the existing behavior is arguably buggy. See patch 14 for more discussion.

The patch stalled back then because I wanted to make sure that ref-filter's color placeholders behaved the same. That required some refactoring which conflicted badly with kn/ref-filter-branch-list. Now that it has graduated, I was able to rebase on top.

This version also takes into account feedback from the original thread. And as I added tests, it surfaced a few corner cases around color config that I've dealt with here. The last two patches are the most interesting bits.

  [01/15]: check return value of verify_ref_format()
  [02/15]: docs/for-each-ref: update pointer to color syntax
  [03/15]: t: use test_decode_color rather than literal ANSI codes
  [04/15]: ref-filter: simplify automatic color reset
  [05/15]: ref-filter: abstract ref format into its own struct
  [06/15]: ref-filter: move need_color_reset_at_eol into ref_format
  [07/15]: ref-filter: provide a function for parsing sort options
  [08/15]: ref-filter: make parse_ref_filter_atom a private function
  [09/15]: ref-filter: factor out the parsing of sorting atoms
  [10/15]: ref-filter: pass ref_format struct to atom parsers
  [11/15]: color: check color.ui in git_default_config()
  [12/15]: for-each-ref: load config earlier
  [13/15]: rev-list: pass diffopt->use_colors through to pretty-print
  [14/15]: pretty: respect color settings for %C placeholders
  [15/15]: ref-filter: consult want_color() before emitting colors
 Documentation/git-for-each-ref.txt |   6 +-
 Documentation/pretty-formats.txt   |  18 ++++--
 builtin/branch.c                   |  21 +++---
 builtin/clean.c                    |   3 +-
 builtin/for-each-ref.c             |  27 ++++----
 builtin/grep.c                     |   2 +-
 builtin/rev-list.c                 |   1 +
 builtin/show-branch.c              |   2 +-
 builtin/tag.c                      |  61 ++++++------------
 builtin/verify-tag.c               |  14 ++--
 color.c                            |   8 ---
 config.c                           |   4 ++
 diff.c                             |   3 -
 pretty.c                           |  27 ++++++--
 ref-filter.c                       | 108 ++++++++++++++++++-------------
 ref-filter.h                       |  30 +++++++--
 t/t3203-branch-output.sh           |  31 +++++++++
 t/t4207-log-decoration-colors.sh   |  22 +++----
 t/t6006-rev-list-format.sh         | 129 ++++++++++++++++++++++++-------------
 t/t6300-for-each-ref.sh            |  39 +++++++----
 t/t7004-tag.sh                     |  25 +++++++
 t/test-lib-functions.sh            |   1 +
 22 files changed, 362 insertions(+), 220 deletions(-)
-Peff
Jeff King· Jul 13, 2017, 14:56 UTC · re: Jeff King · lore

[PATCH 01/15] check return value of verify_ref_format()

Users of the ref-filter code must call verify_ref_format() before formatting any refs, but most ignore its return value. This means we may print an error on a syntactically bogus pattern, but keep going anyway.

In most cases this results in a fatal error when we actually try to format a ref. But if you have no refs to show at all, then the behavior is confusing: git prints the error from verify_ref_format(), then exits with code 0 without showing any output. Let's instead abort immediately if we know we have a bogus format.

We'll output the usage information if we have it handy (just like the existing call in cmd_for_each_ref() does), and otherwise just die().

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c     | 4 +++-
 builtin/tag.c        | 7 ++++---
 builtin/verify-tag.c | 4 +++-
 3 files changed, 10 insertions(+), 5 deletions(-)
Show changes to 3 files +10 −5

builtin/branch.c, builtin/tag.c, builtin/verify-tag.c

diff --git a/builtin/branch.c b/builtin/branch.c
index 8a0595e11..e756a5667 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -409,7 +409,9 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 
 	if (!format)
 		format = to_free = build_format(filter, maxwidth, remote_prefix);
-	verify_ref_format(format);
+
+	if (verify_ref_format(format))
+		die(_("unable to parse format string"));
 
 	ref_array_sort(sorting, &array);
 
diff --git a/builtin/tag.c b/builtin/tag.c
index 01154ea8d..216629fb2 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -53,7 +53,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
 			format = "%(refname:lstrip=2)";
 	}
 
-	verify_ref_format(format);
+	if (verify_ref_format(format))
+		die(_("unable to parse format string"));
 	filter->with_commit_tag_algo = 1;
 	filter_refs(&array, filter, FILTER_REFS_TAGS);
 	ref_array_sort(sorting, &array);
@@ -501,8 +502,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (cmdmode == 'd')
 		return for_each_tag_name(argv, delete_tag, NULL);
 	if (cmdmode == 'v') {
-		if (format)
-			verify_ref_format(format);
+		if (format && verify_ref_format(format))
+			usage_with_options(git_tag_usage, options);
 		return for_each_tag_name(argv, verify_tag, format);
 	}
 
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index f9a5f7535..a10eca2b2 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -51,7 +51,9 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 		flags |= GPG_VERIFY_VERBOSE;
 
 	if (fmt_pretty) {
-		verify_ref_format(fmt_pretty);
+		if (verify_ref_format(fmt_pretty))
+			usage_with_options(verify_tag_usage,
+					   verify_tag_options);
 		flags |= GPG_VERIFY_OMIT_STATUS;
 	}
 
-- 
2.13.2.1157.gc6daca446
Jeff King· Jul 13, 2017, 14:56 UTC · re: Jeff King · lore

[PATCH 02/15] docs/for-each-ref: update pointer to color syntax

The documentation for the %(color) placeholder refers to the color.branch.* config for more details. But those details moved to their own section in b92c1a28f (Documentation/config.txt: describe 'color' value type in the "Values" section, 2015-03-03). Let's update our pointer. We can steal the text from 30cfe72d3 (pretty: fix document link for color specification, 2016-10-11), which fixed the same problem in a different place.

While we're at it, let's give an example, which makes the syntax much more clear than just the text.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-for-each-ref.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Show changes to Documentation/git-for-each-ref.txt +4 −2
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 03e187a10..cc42c1283 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -156,8 +156,10 @@ HEAD::
 	otherwise.
 
 color::
-	Change output color.  Followed by `:<colorname>`, where names
-	are described in `color.branch.*`.
+	Change output color. Followed by `:<colorname>`, where color
+	names are described under Values in the "CONFIGURATION FILE"
+	section of linkgit:git-config[1].  For example,
+	`%(color:bold red)`.
 
 align::
 	Left-, middle-, or right-align the content between
-- 
2.13.2.1157.gc6daca446
Junio C Hamano· Jul 13, 2017, 20:15 UTC · re: Jeff King · lore

Re: [PATCH 02/15] docs/for-each-ref: update pointer to color syntax

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> The documentation for the %(color) placeholder refers to the
> color.branch.* config for more details. But those details
> moved to their own section in b92c1a28f
> (Documentation/config.txt: describe 'color' value type in
> the "Values" section, 2015-03-03).  Let's update our
> pointer. We can steal the text from 30cfe72d3 (pretty: fix
> document link for color specification, 2016-10-11), which
> fixed the same problem in a different place.
Thanks.
> While we're at it, let's give an example, which makes the
> syntax much more clear than just the text.
Nice.  Thanks again.
Show 23 quoted lines
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/git-for-each-ref.txt | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 03e187a10..cc42c1283 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -156,8 +156,10 @@ HEAD::
>  	otherwise.
>  
>  color::
> -	Change output color.  Followed by `:<colorname>`, where names
> -	are described in `color.branch.*`.
> +	Change output color. Followed by `:<colorname>`, where color
> +	names are described under Values in the "CONFIGURATION FILE"
> +	section of linkgit:git-config[1].  For example,
> +	`%(color:bold red)`.
>  
>  align::
>  	Left-, middle-, or right-align the content between
Jeff King· Jul 13, 2017, 14:58 UTC · re: Jeff King · lore

[PATCH 03/15] t: use test_decode_color rather than literal ANSI codes

When we put literal ANSI terminal codes into our test scripts, it makes diffs on those scripts hard to read (the colors may be indistinguishable from diff coloring, or in the case of a reset, may not be visible at all).

Some scripts get around this by including human-readable names and converting to literal codes with a git-config hack. This makes the actual code diffs look OK, but test_cmp output suffers from the same problem.

Let's use test_decode_color instead, which turns the codes into obvious text tags.

Signed-off-by: Jeff King <peff@peff.net>
---
I really only need t6300 and t6006 converted to build on for the rest of
the series. But t4207 was easy to do. t4026 still uses raw codes, but
converting it would be a pretty big job, so I punted.
 t/t4207-log-decoration-colors.sh | 22 +++++++++------------
 t/t6006-rev-list-format.sh       | 42 +++++++++++++++++++++++++---------------
 t/t6300-for-each-ref.sh          | 18 ++++++++---------
 t/test-lib-functions.sh          |  1 +
 4 files changed, 44 insertions(+), 39 deletions(-)
Show changes to 4 files +44 −39

t/t4207-log-decoration-colors.sh, t/t6006-rev-list-format.sh, t/t6300-for-each-ref.sh, t/test-lib-functions.sh

diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index b972296f0..60f040cab 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -7,11 +7,6 @@ test_description='Test for "git log --decorate" colors'
 
 . ./test-lib.sh
 
-get_color ()
-{
-	git config --get-color no.such.slot "$1"
-}
-
 test_expect_success setup '
 	git config diff.color.commit yellow &&
 	git config color.decorate.branch green &&
@@ -20,14 +15,14 @@ test_expect_success setup '
 	git config color.decorate.stash magenta &&
 	git config color.decorate.HEAD cyan &&
 
-	c_reset=$(get_color reset) &&
+	c_reset="<RESET>" &&
 
-	c_commit=$(get_color yellow) &&
-	c_branch=$(get_color green) &&
-	c_remoteBranch=$(get_color red) &&
-	c_tag=$(get_color "reverse bold yellow") &&
-	c_stash=$(get_color magenta) &&
-	c_HEAD=$(get_color cyan) &&
+	c_commit="<YELLOW>" &&
+	c_branch="<GREEN>" &&
+	c_remoteBranch="<RED>" &&
+	c_tag="<BOLD;REVERSE;YELLOW>" &&
+	c_stash="<MAGENTA>" &&
+	c_HEAD="<CYAN>" &&
 
 	test_commit A &&
 	git clone . other &&
@@ -59,7 +54,8 @@ EOF
 # to this test since it does not contain any decoration, hence --first-parent
 test_expect_success 'Commit Decorations Colored Correctly' '
 	git log --first-parent --abbrev=10 --all --decorate --oneline --color=always |
-	sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" >out &&
+	sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" |
+	test_decode_color >out &&
 	test_cmp expected out
 '
 
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index a1dcdb81d..647218b4e 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -61,8 +61,9 @@ test_format () {
 # Feed to --format to provide predictable colored sequences.
 AUTO_COLOR='%C(auto,red)foo%C(auto,reset)'
 has_color () {
-	printf '\033[31mfoo\033[m\n' >expect &&
-	test_cmp expect "$1"
+	test_decode_color <"$1" >decoded &&
+	echo "<RED>foo<RESET>" >expect &&
+	test_cmp expect decoded
 }
 
 has_no_color () {
@@ -170,19 +171,27 @@ $added
 
 EOF
 
-test_format colors %Credfoo%Cgreenbar%Cbluebaz%Cresetxyzzy <<EOF
-commit $head2
-foobarbazxyzzy
-commit $head1
-foobarbazxyzzy
-EOF
+test_expect_success 'basic colors' '
+	cat >expect <<-EOF &&
+	commit $head2
+	<RED>foo<GREEN>bar<BLUE>baz<RESET>xyzzy
+	EOF
+	format="%Credfoo%Cgreenbar%Cbluebaz%Cresetxyzzy" &&
+	git rev-list --format="$format" -1 master >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	test_cmp expect actual
+'
 
-test_format advanced-colors '%C(red yellow bold)foo%C(reset)' <<EOF
-commit $head2
-foo
-commit $head1
-foo
-EOF
+test_expect_success 'advanced colors' '
+	cat >expect <<-EOF &&
+	commit $head2
+	<BOLD;RED;BYELLOW>foo<RESET>
+	EOF
+	format="%C(red yellow bold)foo%C(reset)" &&
+	git rev-list --format="$format" -1 master >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	test_cmp expect actual
+'
 
 test_expect_success '%C(auto,...) does not enable color by default' '
 	git log --format=$AUTO_COLOR -1 >actual &&
@@ -224,8 +233,9 @@ test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' '
 '
 
 test_expect_success '%C(auto) respects --color' '
-	git log --color --format="%C(auto)%H" -1 >actual &&
-	printf "\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect &&
+	git log --color --format="%C(auto)%H" -1 >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	echo "<YELLOW>$(git rev-parse HEAD)<RESET>" >expect &&
 	test_cmp expect actual
 '
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 834a9ed16..7872a2f54 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -412,20 +412,18 @@ test_expect_success 'Check for invalid refname format' '
 	test_must_fail git for-each-ref --format="%(refname:INVALID)"
 '
 
-get_color ()
-{
-	git config --get-color no.such.slot "$1"
-}
-
 cat >expected <<EOF
-$(git rev-parse --short refs/heads/master) $(get_color green)master$(get_color reset)
-$(git rev-parse --short refs/remotes/origin/master) $(get_color green)origin/master$(get_color reset)
-$(git rev-parse --short refs/tags/testtag) $(get_color green)testtag$(get_color reset)
-$(git rev-parse --short refs/tags/two) $(get_color green)two$(get_color reset)
+$(git rev-parse --short refs/heads/master) <GREEN>master<RESET>
+$(git rev-parse --short refs/remotes/origin/master) <GREEN>origin/master<RESET>
+$(git rev-parse --short refs/tags/testtag) <GREEN>testtag<RESET>
+$(git rev-parse --short refs/tags/two) <GREEN>two<RESET>
 EOF
 
 test_expect_success 'Check %(color:...) ' '
-	git for-each-ref --format="%(objectname:short) %(color:green)%(refname:short)" >actual &&
+	git for-each-ref \
+		--format="%(objectname:short) %(color:green)%(refname:short)" \
+		>actual.raw &&
+	test_decode_color <actual.raw >actual &&
 	test_cmp expected actual
 '
 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index db622c355..e09e93b38 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -42,6 +42,7 @@ test_decode_color () {
 		function name(n) {
 			if (n == 0) return "RESET";
 			if (n == 1) return "BOLD";
+			if (n == 7) return "REVERSE";
 			if (n == 30) return "BLACK";
 			if (n == 31) return "RED";
 			if (n == 32) return "GREEN";
-- 
2.13.2.1157.gc6daca446
Stefan Beller· Jul 13, 2017, 18:40 UTC · re: Jeff King · lore

Re: [PATCH 03/15] t: use test_decode_color rather than literal ANSI codes

On Thu, Jul 13, 2017 at 7:58 AM, Jeff King <peff@peff.net> wrote:
> I really only need t6300 and t6006 converted to build on for the rest of
> the series. But t4207 was easy to do. t4026 still uses raw codes, but
> converting it would be a pretty big job, so I punted.
>

I think it is good to have raw codes in at least one place to test that raw codes work, but then again it could be specific test calling out that this is tested.

Show 7 quoted lines
> @@ -59,7 +54,8 @@ EOF
>  # to this test since it does not contain any decoration, hence --first-parent
>  test_expect_success 'Commit Decorations Colored Correctly' '
>         git log --first-parent --abbrev=10 --all --decorate --oneline --color=always |
> -       sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" >out &&
> +       sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" |
> +       test_decode_color >out &&
Just some thoughts:

This extension of the pipe-chain is not making it worse as gits exit code is already hidden.

The sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" is sort of funny, because I would have expected it to break in the future with e.g. the sha1 to longer hash conversion. But as we specify --abbrev=10, this seems future proof. In an ideal world this would be encapsulated in a function (c.f. t/diff-lib.sh).

Show 10 quoted lines
> @@ -61,8 +61,9 @@ test_format () {
>  # Feed to --format to provide predictable colored sequences.
>  AUTO_COLOR='%C(auto,red)foo%C(auto,reset)'
>  has_color () {
> -       printf '\033[31mfoo\033[m\n' >expect &&
> -       test_cmp expect "$1"
> +       test_decode_color <"$1" >decoded &&
> +       echo "<RED>foo<RESET>" >expect &&
> +       test_cmp expect decoded
>  }
Thanks for removing hard coded colors :)
Jeff King· Jul 13, 2017, 18:45 UTC · re: Stefan Beller · lore

Re: [PATCH 03/15] t: use test_decode_color rather than literal ANSI codes

On Thu, Jul 13, 2017 at 11:40:32AM -0700, Stefan Beller wrote:
Show 9 quoted lines
> On Thu, Jul 13, 2017 at 7:58 AM, Jeff King <peff@peff.net> wrote:
> 
> > I really only need t6300 and t6006 converted to build on for the rest of
> > the series. But t4207 was easy to do. t4026 still uses raw codes, but
> > converting it would be a pretty big job, so I punted.
> 
> I think it is good to have raw codes in at least one place to test
> that raw codes work, but then again it could be specific test calling
> out that this is tested.

Yeah, that thought crossed my mind, too. t4026 would definitely be the place for it, as it is about exhaustively testing the various colors. The others are just about feeding color codes through config and user-formats.

Show 12 quoted lines
> > @@ -59,7 +54,8 @@ EOF
> >  # to this test since it does not contain any decoration, hence --first-parent
> >  test_expect_success 'Commit Decorations Colored Correctly' '
> >         git log --first-parent --abbrev=10 --all --decorate --oneline --color=always |
> > -       sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" >out &&
> > +       sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" |
> > +       test_decode_color >out &&
> 
> Just some thoughts:
> 
> This extension of the pipe-chain is not making it worse as gits exit code
> is already hidden.

Yes, I noticed the existing pipe-chain. We can fix it while we're here, though I think it's not a big deal in practice.

> The sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" is sort of funny, because
> I would have expected it to break in the future with e.g. the sha1 to longer
> hash conversion. But as we specify --abbrev=10, this seems future proof.
> In an ideal world this would be encapsulated in a function (c.f. t/diff-lib.sh).
I agree it's a bit gross. Possibly:
  git log --format='%C(auto)%d %s'

would be easier for the test to parse (I'm pretty sure that didn't exist back when this test was written).

-Peff
Junio C Hamano· Jul 13, 2017, 19:27 UTC · re: Jeff King · lore

Re: [PATCH 03/15] t: use test_decode_color rather than literal ANSI codes

Jeff King <peff@peff.net> writes:
Show 20 quoted lines
>> > @@ -59,7 +54,8 @@ EOF
>> >  # to this test since it does not contain any decoration, hence --first-parent
>> >  test_expect_success 'Commit Decorations Colored Correctly' '
>> >         git log --first-parent --abbrev=10 --all --decorate --oneline --color=always |
>> > -       sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" >out &&
>> > +       sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" |
>> > +       test_decode_color >out &&
>> 
>> Just some thoughts:
>> 
>> This extension of the pipe-chain is not making it worse as gits exit code
>> is already hidden.
>
> Yes, I noticed the existing pipe-chain. We can fix it while we're here,
> though I think it's not a big deal in practice.
>
>> The sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" is sort of funny, because
>> I would have expected it to break in the future with e.g. the sha1 to longer
>> hash conversion. But as we specify --abbrev=10, this seems future proof.
>> In an ideal world this would be encapsulated in a function (c.f. t/diff-lib.sh).

Actually, --abbrev=10 does not guarantee that the hex part is always 10 bytes long, so it is not future-proofing, but I expect it would work out fine in practice.

Show 6 quoted lines
> I agree it's a bit gross. Possibly:
>
>   git log --format='%C(auto)%d %s'
>
> would be easier for the test to parse (I'm pretty sure that didn't exist
> back when this test was written).
Yeah, that may make the test easier to read, too.
Thanks.
Jeff King· Jul 14, 2017, 11:50 UTC · re: Junio C Hamano · lore

Re: [PATCH 03/15] t: use test_decode_color rather than literal ANSI codes

On Thu, Jul 13, 2017 at 12:27:41PM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> > I agree it's a bit gross. Possibly:
> >
> >   git log --format='%C(auto)%d %s'
> >
> > would be easier for the test to parse (I'm pretty sure that didn't exist
> > back when this test was written).
> 
> Yeah, that may make the test easier to read, too.

I started on that, but the test is actually checking the coloring of the commit message, too. So switching to a custom format would lose that part of the test.

-Peff
Junio C Hamano· Jul 13, 2017, 20:18 UTC · re: Jeff King · lore

Re: [PATCH 03/15] t: use test_decode_color rather than literal ANSI codes

Jeff King <peff@peff.net> writes:
Show 12 quoted lines
> When we put literal ANSI terminal codes into our test
> scripts, it makes diffs on those scripts hard to read (the
> colors may be indistinguishable from diff coloring, or in
> the case of a reset, may not be visible at all).
>
> Some scripts get around this by including human-readable
> names and converting to literal codes with a git-config
> hack. This makes the actual code diffs look OK, but test_cmp
> output suffers from the same problem.
>
> Let's use test_decode_color instead, which turns the codes
> into obvious text tags.
Nice.  Thanks.
Jeff King· Jul 13, 2017, 14:58 UTC · re: Jeff King · lore

[PATCH 04/15] ref-filter: simplify automatic color reset

When the user-format doesn't add the closing color reset, we add one automatically. But we do so by parsing the "reset" string. We can just use the baked-in string literal, which is simpler.

Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
Show changes to ref-filter.c +1 −5
diff --git a/ref-filter.c b/ref-filter.c
index ae6ecbd1c..6da5be3ac 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2084,11 +2084,7 @@ void format_ref_array_item(struct ref_array_item *info, const char *format,
 	}
 	if (need_color_reset_at_eol) {
 		struct atom_value resetv;
-		char color[COLOR_MAXLEN] = "";
-
-		if (color_parse("reset", color) < 0)
-			die("BUG: couldn't parse 'reset' as a color");
-		resetv.s = color;
+		resetv.s = GIT_COLOR_RESET;
 		append_atom(&resetv, &state);
 	}
 	if (state.stack->prev)
-- 
2.13.2.1157.gc6daca446
Jeff King· Jul 13, 2017, 15:01 UTC · re: Jeff King · lore

[PATCH 05/15] ref-filter: abstract ref format into its own struct

The ref-filter module provides routines for formatting a ref for output. The fundamental interface for the format is a "const char *" containing the format, and any additional options need to be passed to each invocation of show_ref_array_item.

Instead, let's make a ref_format struct that holds the format, along with any associated format options. That will make some enhancements easier in the future:

  1. new formatting options can be added without disrupting
     existing callers
  2. some state can be carried in the struct rather than as
     global variables

For now this just has the text format itself along with the quote_style option, but we'll add more fields in future patches.

Signed-off-by: Jeff King <peff@peff.net>
---
A few of the spots are a bit ugly. E.g., when the format is optional you
might have to check whether format->format is NULL. It's possible that
it would be nicer if we abstracted this into a more opaque object with
accessors. But I didn't want to go overboard.
 builtin/branch.c       | 14 +++++++-------
 builtin/for-each-ref.c | 22 ++++++++++++----------
 builtin/tag.c          | 30 ++++++++++++++++--------------
 builtin/verify-tag.c   | 12 ++++++------
 ref-filter.c           | 22 ++++++++++++----------
 ref-filter.h           | 22 +++++++++++++++++-----
 6 files changed, 70 insertions(+), 52 deletions(-)
Show changes to 6 files +70 −52

builtin/branch.c, builtin/for-each-ref.c, builtin/tag.c, builtin/verify-tag.c, ref-filter.c, ref-filter.h

diff --git a/builtin/branch.c b/builtin/branch.c
index e756a5667..036fdc929 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -383,7 +383,7 @@ static char *build_format(struct ref_filter *filter, int maxwidth, const char *r
 	return strbuf_detach(&fmt, NULL);
 }
 
-static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
+static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting, struct ref_format *format)
 {
 	int i;
 	struct ref_array array;
@@ -407,8 +407,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 	if (filter->verbose)
 		maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
 
-	if (!format)
-		format = to_free = build_format(filter, maxwidth, remote_prefix);
+	if (!format->format)
+		format->format = to_free = build_format(filter, maxwidth, remote_prefix);
 
 	if (verify_ref_format(format))
 		die(_("unable to parse format string"));
@@ -416,7 +416,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 	ref_array_sort(sorting, &array);
 
 	for (i = 0; i < array.nr; i++) {
-		format_ref_array_item(array.items[i], format, 0, &out);
+		format_ref_array_item(array.items[i], format, &out);
 		if (column_active(colopts)) {
 			assert(!filter->verbose && "--column and --verbose are incompatible");
 			 /* format to a string_list to let print_columns() do its job */
@@ -551,7 +551,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	struct ref_filter filter;
 	int icase = 0;
 	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
-	const char *format = NULL;
+	struct ref_format format = REF_FORMAT_INIT;
 
 	struct option options[] = {
 		OPT_GROUP(N_("Generic options")),
@@ -595,7 +595,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			N_("print only branches of the object"), 0, parse_opt_object_name
 		},
 		OPT_BOOL('i', "ignore-case", &icase, N_("sorting and filtering are case insensitive")),
-		OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
+		OPT_STRING(  0 , "format", &format.format, N_("format"), N_("format to use for the output")),
 		OPT_END(),
 	};
 
@@ -669,7 +669,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!sorting)
 			sorting = ref_default_sorting();
 		sorting->ignore_case = icase;
-		print_ref_list(&filter, sorting, format);
+		print_ref_list(&filter, sorting, &format);
 		print_columns(&output, colopts, NULL);
 		string_list_clear(&output, 0);
 		return 0;
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 52be99cba..f47066b42 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -17,25 +17,25 @@ static char const * const for_each_ref_usage[] = {
 int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 {
 	int i;
-	const char *format = "%(objectname) %(objecttype)\t%(refname)";
 	struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
-	int maxcount = 0, quote_style = 0, icase = 0;
+	int maxcount = 0, icase = 0;
 	struct ref_array array;
 	struct ref_filter filter;
+	struct ref_format format = REF_FORMAT_INIT;
 
 	struct option opts[] = {
-		OPT_BIT('s', "shell", &quote_style,
+		OPT_BIT('s', "shell", &format.quote_style,
 			N_("quote placeholders suitably for shells"), QUOTE_SHELL),
-		OPT_BIT('p', "perl",  &quote_style,
+		OPT_BIT('p', "perl",  &format.quote_style,
 			N_("quote placeholders suitably for perl"), QUOTE_PERL),
-		OPT_BIT(0 , "python", &quote_style,
+		OPT_BIT(0 , "python", &format.quote_style,
 			N_("quote placeholders suitably for python"), QUOTE_PYTHON),
-		OPT_BIT(0 , "tcl",  &quote_style,
+		OPT_BIT(0 , "tcl",  &format.quote_style,
 			N_("quote placeholders suitably for Tcl"), QUOTE_TCL),
 
 		OPT_GROUP(""),
 		OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")),
-		OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
+		OPT_STRING(  0 , "format", &format.format, N_("format"), N_("format to use for the output")),
 		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
 			    N_("field name to sort on"), &parse_opt_ref_sorting),
 		OPT_CALLBACK(0, "points-at", &filter.points_at,
@@ -52,16 +52,18 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	memset(&array, 0, sizeof(array));
 	memset(&filter, 0, sizeof(filter));
 
+	format.format = "%(objectname) %(objecttype)\t%(refname)";
+
 	parse_options(argc, argv, prefix, opts, for_each_ref_usage, 0);
 	if (maxcount < 0) {
 		error("invalid --count argument: `%d'", maxcount);
 		usage_with_options(for_each_ref_usage, opts);
 	}
-	if (HAS_MULTI_BITS(quote_style)) {
+	if (HAS_MULTI_BITS(format.quote_style)) {
 		error("more than one quoting style?");
 		usage_with_options(for_each_ref_usage, opts);
 	}
-	if (verify_ref_format(format))
+	if (verify_ref_format(&format))
 		usage_with_options(for_each_ref_usage, opts);
 
 	if (!sorting)
@@ -80,7 +82,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	if (!maxcount || array.nr < maxcount)
 		maxcount = array.nr;
 	for (i = 0; i < maxcount; i++)
-		show_ref_array_item(array.items[i], format, quote_style);
+		show_ref_array_item(array.items[i], &format);
 	ref_array_clear(&array);
 	return 0;
 }
diff --git a/builtin/tag.c b/builtin/tag.c
index 216629fb2..207c9eb03 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -32,7 +32,8 @@ static const char * const git_tag_usage[] = {
 static unsigned int colopts;
 static int force_sign_annotate;
 
-static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
+		     struct ref_format *format)
 {
 	struct ref_array array;
 	char *to_free = NULL;
@@ -43,14 +44,14 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
 	if (filter->lines == -1)
 		filter->lines = 0;
 
-	if (!format) {
+	if (!format->format) {
 		if (filter->lines) {
 			to_free = xstrfmt("%s %%(contents:lines=%d)",
 					  "%(align:15)%(refname:lstrip=2)%(end)",
 					  filter->lines);
-			format = to_free;
+			format->format = to_free;
 		} else
-			format = "%(refname:lstrip=2)";
+			format->format = "%(refname:lstrip=2)";
 	}
 
 	if (verify_ref_format(format))
@@ -60,7 +61,7 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
 	ref_array_sort(sorting, &array);
 
 	for (i = 0; i < array.nr; i++)
-		show_ref_array_item(array.items[i], format, 0);
+		show_ref_array_item(array.items[i], format);
 	ref_array_clear(&array);
 	free(to_free);
 
@@ -106,17 +107,17 @@ static int verify_tag(const char *name, const char *ref,
 		      const struct object_id *oid, const void *cb_data)
 {
 	int flags;
-	const char *fmt_pretty = cb_data;
+	const struct ref_format *format = cb_data;
 	flags = GPG_VERIFY_VERBOSE;
 
-	if (fmt_pretty)
+	if (format->format)
 		flags = GPG_VERIFY_OMIT_STATUS;
 
 	if (gpg_verify_tag(oid->hash, name, flags))
 		return -1;
 
-	if (fmt_pretty)
-		pretty_print_ref(name, oid->hash, fmt_pretty);
+	if (format->format)
+		pretty_print_ref(name, oid->hash, format);
 
 	return 0;
 }
@@ -393,7 +394,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct strbuf err = STRBUF_INIT;
 	struct ref_filter filter;
 	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
-	const char *format = NULL;
+	struct ref_format format = REF_FORMAT_INIT;
 	int icase = 0;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
@@ -432,7 +433,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			N_("print only tags of the object"), PARSE_OPT_LASTARG_DEFAULT,
 			parse_opt_object_name, (intptr_t) "HEAD"
 		},
-		OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
+		OPT_STRING(  0 , "format", &format.format, N_("format"),
+			   N_("format to use for the output")),
 		OPT_BOOL('i', "ignore-case", &icase, N_("sorting and filtering are case insensitive")),
 		OPT_END()
 	};
@@ -484,7 +486,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			run_column_filter(colopts, &copts);
 		}
 		filter.name_patterns = argv;
-		ret = list_tags(&filter, sorting, format);
+		ret = list_tags(&filter, sorting, &format);
 		if (column_active(colopts))
 			stop_column_filter();
 		return ret;
@@ -502,9 +504,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (cmdmode == 'd')
 		return for_each_tag_name(argv, delete_tag, NULL);
 	if (cmdmode == 'v') {
-		if (format && verify_ref_format(format))
+		if (format.format && verify_ref_format(&format))
 			usage_with_options(git_tag_usage, options);
-		return for_each_tag_name(argv, verify_tag, format);
+		return for_each_tag_name(argv, verify_tag, &format);
 	}
 
 	if (msg.given || msgfile) {
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index a10eca2b2..87d73e856 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -32,11 +32,11 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 {
 	int i = 1, verbose = 0, had_error = 0;
 	unsigned flags = 0;
-	char *fmt_pretty = NULL;
+	struct ref_format format = REF_FORMAT_INIT;
 	const struct option verify_tag_options[] = {
 		OPT__VERBOSE(&verbose, N_("print tag contents")),
 		OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW),
-		OPT_STRING(  0 , "format", &fmt_pretty, N_("format"), N_("format to use for the output")),
+		OPT_STRING(0, "format", &format.format, N_("format"), N_("format to use for the output")),
 		OPT_END()
 	};
 
@@ -50,8 +50,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 	if (verbose)
 		flags |= GPG_VERIFY_VERBOSE;
 
-	if (fmt_pretty) {
-		if (verify_ref_format(fmt_pretty))
+	if (format.format) {
+		if (verify_ref_format(&format))
 			usage_with_options(verify_tag_usage,
 					   verify_tag_options);
 		flags |= GPG_VERIFY_OMIT_STATUS;
@@ -70,8 +70,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		if (fmt_pretty)
-			pretty_print_ref(name, sha1, fmt_pretty);
+		if (format.format)
+			pretty_print_ref(name, sha1, &format);
 	}
 	return had_error;
 }
diff --git a/ref-filter.c b/ref-filter.c
index 6da5be3ac..66d234bb1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -657,12 +657,12 @@ static const char *find_next(const char *cp)
  * Make sure the format string is well formed, and parse out
  * the used atoms.
  */
-int verify_ref_format(const char *format)
+int verify_ref_format(struct ref_format *format)
 {
 	const char *cp, *sp;
 
 	need_color_reset_at_eol = 0;
-	for (cp = format; *cp && (sp = find_next(cp)); ) {
+	for (cp = format->format; *cp && (sp = find_next(cp)); ) {
 		const char *color, *ep = strchr(sp, ')');
 		int at;
 
@@ -2060,16 +2060,17 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
 	}
 }
 
-void format_ref_array_item(struct ref_array_item *info, const char *format,
-			   int quote_style, struct strbuf *final_buf)
+void format_ref_array_item(struct ref_array_item *info,
+			   const struct ref_format *format,
+			   struct strbuf *final_buf)
 {
 	const char *cp, *sp, *ep;
 	struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
 
-	state.quote_style = quote_style;
+	state.quote_style = format->quote_style;
 	push_stack_element(&state.stack);
 
-	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
+	for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {
 		struct atom_value *atomv;
 
 		ep = strchr(sp, ')');
@@ -2093,23 +2094,24 @@ void format_ref_array_item(struct ref_array_item *info, const char *format,
 	pop_stack_element(&state.stack);
 }
 
-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
+void show_ref_array_item(struct ref_array_item *info,
+			 const struct ref_format *format)
 {
 	struct strbuf final_buf = STRBUF_INIT;
 
-	format_ref_array_item(info, format, quote_style, &final_buf);
+	format_ref_array_item(info, format, &final_buf);
 	fwrite(final_buf.buf, 1, final_buf.len, stdout);
 	strbuf_release(&final_buf);
 	putchar('\n');
 }
 
 void pretty_print_ref(const char *name, const unsigned char *sha1,
-		const char *format)
+		      const struct ref_format *format)
 {
 	struct ref_array_item *ref_item;
 	ref_item = new_ref_array_item(name, sha1, 0);
 	ref_item->kind = ref_kind_from_refname(name);
-	show_ref_array_item(ref_item, format, 0);
+	show_ref_array_item(ref_item, format);
 	free_array_item(ref_item);
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 6552024f0..2bb58879d 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -72,6 +72,17 @@ struct ref_filter {
 		verbose;
 };
 
+struct ref_format {
+	/*
+	 * Set these to define the format; make sure you call
+	 * verify_ref_format() afterwards to finalize.
+	 */
+	const char *format;
+	int quote_style;
+};
+
+#define REF_FORMAT_INIT { NULL, 0 }
+
 /*  Macros for checking --merged and --no-merged options */
 #define _OPT_MERGED_NO_MERGED(option, filter, h) \
 	{ OPTION_CALLBACK, 0, option, (filter), N_("commit"), (h), \
@@ -93,14 +104,15 @@ void ref_array_clear(struct ref_array *array);
 /*  Parse format string and sort specifiers */
 int parse_ref_filter_atom(const char *atom, const char *ep);
 /*  Used to verify if the given format is correct and to parse out the used atoms */
-int verify_ref_format(const char *format);
+int verify_ref_format(struct ref_format *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
 /*  Based on the given format and quote_style, fill the strbuf */
-void format_ref_array_item(struct ref_array_item *info, const char *format,
-			   int quote_style, struct strbuf *final_buf);
+void format_ref_array_item(struct ref_array_item *info,
+			   const struct ref_format *format,
+			   struct strbuf *final_buf);
 /*  Print the ref using the given format and quote_style */
-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);
+void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);
 /*  Callback function for parsing the sort option */
 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);
 /*  Default sort option based on refname */
@@ -117,6 +129,6 @@ void setup_ref_filter_porcelain_msg(void);
  * name must be a fully qualified refname.
  */
 void pretty_print_ref(const char *name, const unsigned char *sha1,
-		const char *format);
+		      const struct ref_format *format);
 
 #endif /*  REF_FILTER_H  */
-- 
2.13.2.1157.gc6daca446
Stefan Beller· Jul 13, 2017, 18:51 UTC · re: Jeff King · lore

Re: [PATCH 05/15] ref-filter: abstract ref format into its own struct

On Thu, Jul 13, 2017 at 8:01 AM, Jeff King <peff@peff.net> wrote:
Show 7 quoted lines
>  builtin/branch.c       | 14 +++++++-------
>  builtin/for-each-ref.c | 22 ++++++++++++----------
>  builtin/tag.c          | 30 ++++++++++++++++--------------
>  builtin/verify-tag.c   | 12 ++++++------
>  ref-filter.c           | 22 ++++++++++++----------
>  ref-filter.h           | 22 +++++++++++++++++-----
>  6 files changed, 70 insertions(+), 52 deletions(-)

The patch looks good to me. So some off-topic comments: I reviewed this patch from bottom up, i.e. I started looking at ref-filter.h, then ref-filter.c and then the rest. If only you had formatted the patches with an orderfile. ;)

Junio C Hamano· Jul 13, 2017, 20:34 UTC · re: Stefan Beller · lore

Re: [PATCH 05/15] ref-filter: abstract ref format into its own struct

Stefan Beller <sbeller@google.com> writes:
Show 14 quoted lines
> On Thu, Jul 13, 2017 at 8:01 AM, Jeff King <peff@peff.net> wrote:
>
>>  builtin/branch.c       | 14 +++++++-------
>>  builtin/for-each-ref.c | 22 ++++++++++++----------
>>  builtin/tag.c          | 30 ++++++++++++++++--------------
>>  builtin/verify-tag.c   | 12 ++++++------
>>  ref-filter.c           | 22 ++++++++++++----------
>>  ref-filter.h           | 22 +++++++++++++++++-----
>>  6 files changed, 70 insertions(+), 52 deletions(-)
>
> The patch looks good to me. So some off-topic comments:
> I reviewed this patch from bottom up, i.e. I started looking at
> ref-filter.h, then  ref-filter.c and then the rest. If only you had formatted
> the patches with an orderfile. ;)

As a reviewer, for this particular patchq, I actually appreciated that ref-filter.[ch] came at the end. That forced me to think.

When I see something that used to be 0 is lost from the parameter list of show_ref_array_item() at a callsite, I was forced to guess what it is by looking at what is moved into the new structure nearby, and keep doing that "figure out what is going on" game until the "author's answer" is revealed at the end of the patch.

By the time I reached ref-filter.[ch], I had a pretty good idea what was done by only looking at the callers and being able to see if my understanding matched the "author's answer" at the end of the patch turned out to be a very good way to double-check if the patch was sane. If I were given the author's answer upfront, especially an answer by somebody as competent as Peff, I'm sure I would have been swayed into believing that whatever is written in the patch must be correct without thinking the changes needed in the patch myself.

I do want to present from Doc to header to code when I am showing my patch to others, so this is probably a good example that illustrates that the preferred presentation order is not just personal preference, but is different on occasion even for the same person.

Junio C Hamano· Jul 13, 2017, 21:32 UTC · re: Junio C Hamano · lore

Re: [PATCH 05/15] ref-filter: abstract ref format into its own struct

Junio C Hamano <gitster@pobox.com> writes:
Show 24 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> On Thu, Jul 13, 2017 at 8:01 AM, Jeff King <peff@peff.net> wrote:
>>
>>>  builtin/branch.c       | 14 +++++++-------
>>>  builtin/for-each-ref.c | 22 ++++++++++++----------
>>>  builtin/tag.c          | 30 ++++++++++++++++--------------
>>>  builtin/verify-tag.c   | 12 ++++++------
>>>  ref-filter.c           | 22 ++++++++++++----------
>>>  ref-filter.h           | 22 +++++++++++++++++-----
>>>  6 files changed, 70 insertions(+), 52 deletions(-)
>>
>> The patch looks good to me. So some off-topic comments:
>> I reviewed this patch from bottom up, i.e. I started looking at
>> ref-filter.h, then  ref-filter.c and then the rest. If only you had formatted
>> the patches with an orderfile. ;)
>
> As a reviewer, for this particular patchq, I actually appreciated
> that ref-filter.[ch] came at the end.  That forced me to think.
> ...
> I do want to present from Doc to header to code when I am showing my
> patch to others, so this is probably a good example that illustrates
> that the preferred presentation order is not just personal
> preference, but is different on occasion even for the same person.

So when somebody wants to do a "from design and explanation to provider to consumer", we would probably want "doc, *.h, *.c at the top-level and then things inside builtin/ subdirectory" order. Of course, on the other hand, "I do not trust me not getting swayed by the fact that a developer more competent than me wrote the patch" reviewer would want to use the reverse order.

Can we actually express "top-level first and then builtin/*" order with the diff.orderfile mechanism? It's been a while since I last looked at the orderfile matching (which was when I originally wrote it) and I do not offhand know if we now allow wildmatch patterns and the directory level anchoring "/*.c" like we do in .gitignore files, without which it would be cumbersome to make ref-filter.c listed before builtin/branch.c in a generic way.

Stefan Beller· Jul 13, 2017, 22:01 UTC · re: Junio C Hamano · lore

Re: [PATCH 05/15] ref-filter: abstract ref format into its own struct

On Thu, Jul 13, 2017 at 2:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 33 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> On Thu, Jul 13, 2017 at 8:01 AM, Jeff King <peff@peff.net> wrote:
>>>
>>>>  builtin/branch.c       | 14 +++++++-------
>>>>  builtin/for-each-ref.c | 22 ++++++++++++----------
>>>>  builtin/tag.c          | 30 ++++++++++++++++--------------
>>>>  builtin/verify-tag.c   | 12 ++++++------
>>>>  ref-filter.c           | 22 ++++++++++++----------
>>>>  ref-filter.h           | 22 +++++++++++++++++-----
>>>>  6 files changed, 70 insertions(+), 52 deletions(-)
>>>
>>> The patch looks good to me. So some off-topic comments:
>>> I reviewed this patch from bottom up, i.e. I started looking at
>>> ref-filter.h, then  ref-filter.c and then the rest. If only you had formatted
>>> the patches with an orderfile. ;)
>>
>> As a reviewer, for this particular patchq, I actually appreciated
>> that ref-filter.[ch] came at the end.  That forced me to think.
>> ...
>> I do want to present from Doc to header to code when I am showing my
>> patch to others, so this is probably a good example that illustrates
>> that the preferred presentation order is not just personal
>> preference, but is different on occasion even for the same person.
>
> So when somebody wants to do a "from design and explanation to
> provider to consumer", we would probably want "doc, *.h, *.c at the
> top-level and then things inside builtin/ subdirectory" order.  Of
> course, on the other hand, "I do not trust me not getting swayed by
> the fact that a developer more competent than me wrote the patch"
> reviewer would want to use the reverse order.

I do not understand what you say here? Are you saying: "I can be tricked easier when the order is top-down, specifically when the more competent developer tries to?"

> Can we actually express "top-level first and then builtin/*" order
> with the diff.orderfile mechanism?

By reading the code, I think we snap to the first match. And matching is done via the wildmatch.{c,h}, that claims it has special-case '/' matching, and '**' ** work differently than '*',

> without which it would be cumbersome to make ref-filter.c listed
> before builtin/branch.c in a generic way.
Indeed.
Junio C Hamano· Jul 13, 2017, 22:45 UTC · re: Stefan Beller · lore

Re: [PATCH 05/15] ref-filter: abstract ref format into its own struct

Stefan Beller <sbeller@google.com> writes:
Show 10 quoted lines
>> So when somebody wants to do a "from design and explanation to
>> provider to consumer", we would probably want "doc, *.h, *.c at the
>> top-level and then things inside builtin/ subdirectory" order.  Of
>> course, on the other hand, "I do not trust me not getting swayed by
>> the fact that a developer more competent than me wrote the patch"
>> reviewer would want to use the reverse order.
>
> I do not understand what you say here? Are you saying:
> "I can be tricked easier when the order is top-down,
> specifically when the more competent developer tries to?"

I am not worried about the case in which patch author actively "tries to" deceive. It is more like "I am more likely to fail to spot mistakes the patch author failed to spot himself", when I start with reasoning, service provider implementations and then service consumer. When I am forced to think the problem myself before seeing the answer and then compare the result with patch author's answer, I am more likely to find such a mistake.

Show 6 quoted lines
>> Can we actually express "top-level first and then builtin/*" order
>> with the diff.orderfile mechanism?
>
> By reading the code, I think we snap to the first match. And matching
> is done via the wildmatch.{c,h}, that claims it has special-case '/' matching,
> and '**' **  work differently than '*',

I took a brief look at diffcore-order.c; I do not think "/*.c" would match only top-level .c files like gitignore files would.

Jeff King· Jul 13, 2017, 15:02 UTC · re: Jeff King · lore

[PATCH 06/15] ref-filter: move need_color_reset_at_eol into ref_format

Calling verify_ref_format() doesn't just confirm that the format is sane; it actually sets some global variables that will be used later when formatting the refs. These logically should belong to the ref_format, which would make it possible to use multiple formats within a single program invocation.

Let's move one such flag into the ref_format struct. There are still others that would need to be moved before it would be safe to use multiple formats, but this commit gives a blueprint for how that should look.

Signed-off-by: Jeff King <peff@peff.net>
---
This commit is strictly optional for this series, but I wanted to give a
sense of how the rest of the movement might look, since I was thinking
about it. The big thing to move would be the used_atoms array, but I
punted on that for now.
 ref-filter.c | 7 +++----
 ref-filter.h | 3 +++
 2 files changed, 6 insertions(+), 4 deletions(-)
Show changes to 2 files +6 −4

ref-filter.c, ref-filter.h

diff --git a/ref-filter.c b/ref-filter.c
index 66d234bb1..178396e1f 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -97,7 +97,6 @@ static struct used_atom {
 	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
-static int need_color_reset_at_eol;
 
 static void color_atom_parser(struct used_atom *atom, const char *color_value)
 {
@@ -661,7 +660,7 @@ int verify_ref_format(struct ref_format *format)
 {
 	const char *cp, *sp;
 
-	need_color_reset_at_eol = 0;
+	format->need_color_reset_at_eol = 0;
 	for (cp = format->format; *cp && (sp = find_next(cp)); ) {
 		const char *color, *ep = strchr(sp, ')');
 		int at;
@@ -673,7 +672,7 @@ int verify_ref_format(struct ref_format *format)
 		cp = ep + 1;
 
 		if (skip_prefix(used_atom[at].name, "color:", &color))
-			need_color_reset_at_eol = !!strcmp(color, "reset");
+			format->need_color_reset_at_eol = !!strcmp(color, "reset");
 	}
 	return 0;
 }
@@ -2083,7 +2082,7 @@ void format_ref_array_item(struct ref_array_item *info,
 		sp = cp + strlen(cp);
 		append_literal(cp, sp, &state);
 	}
-	if (need_color_reset_at_eol) {
+	if (format->need_color_reset_at_eol) {
 		struct atom_value resetv;
 		resetv.s = GIT_COLOR_RESET;
 		append_atom(&resetv, &state);
diff --git a/ref-filter.h b/ref-filter.h
index 2bb58879d..9e1e89c19 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -79,6 +79,9 @@ struct ref_format {
 	 */
 	const char *format;
 	int quote_style;
+
+	/* Internal state to ref-filter */
+	int need_color_reset_at_eol;
 };
 
 #define REF_FORMAT_INIT { NULL, 0 }
-- 
2.13.2.1157.gc6daca446
Junio C Hamano· Jul 13, 2017, 20:36 UTC · re: Jeff King · lore

Re: [PATCH 06/15] ref-filter: move need_color_reset_at_eol into ref_format

Jeff King <peff@peff.net> writes:
Show 18 quoted lines
> Calling verify_ref_format() doesn't just confirm that the
> format is sane; it actually sets some global variables that
> will be used later when formatting the refs. These logically
> should belong to the ref_format, which would make it
> possible to use multiple formats within a single program
> invocation.
>
> Let's move one such flag into the ref_format struct. There
> are still others that would need to be moved before it would
> be safe to use multiple formats, but this commit gives a
> blueprint for how that should look.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This commit is strictly optional for this series, but I wanted to give a
> sense of how the rest of the movement might look, since I was thinking
> about it. The big thing to move would be the used_atoms array, but I
> punted on that for now.

Heh, when I saw the hunk at 661,+7, used_atom[] was what immediately came to mind. It is OK to punt for the purpose of this patch, but moving these statics into the ref-format structure would be a good move in the longer term.

Thanks.
Show 58 quoted lines
>
>  ref-filter.c | 7 +++----
>  ref-filter.h | 3 +++
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 66d234bb1..178396e1f 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -97,7 +97,6 @@ static struct used_atom {
>  	} u;
>  } *used_atom;
>  static int used_atom_cnt, need_tagged, need_symref;
> -static int need_color_reset_at_eol;
>  
>  static void color_atom_parser(struct used_atom *atom, const char *color_value)
>  {
> @@ -661,7 +660,7 @@ int verify_ref_format(struct ref_format *format)
>  {
>  	const char *cp, *sp;
>  
> -	need_color_reset_at_eol = 0;
> +	format->need_color_reset_at_eol = 0;
>  	for (cp = format->format; *cp && (sp = find_next(cp)); ) {
>  		const char *color, *ep = strchr(sp, ')');
>  		int at;
> @@ -673,7 +672,7 @@ int verify_ref_format(struct ref_format *format)
>  		cp = ep + 1;
>  
>  		if (skip_prefix(used_atom[at].name, "color:", &color))
> -			need_color_reset_at_eol = !!strcmp(color, "reset");
> +			format->need_color_reset_at_eol = !!strcmp(color, "reset");
>  	}
>  	return 0;
>  }
> @@ -2083,7 +2082,7 @@ void format_ref_array_item(struct ref_array_item *info,
>  		sp = cp + strlen(cp);
>  		append_literal(cp, sp, &state);
>  	}
> -	if (need_color_reset_at_eol) {
> +	if (format->need_color_reset_at_eol) {
>  		struct atom_value resetv;
>  		resetv.s = GIT_COLOR_RESET;
>  		append_atom(&resetv, &state);
> diff --git a/ref-filter.h b/ref-filter.h
> index 2bb58879d..9e1e89c19 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -79,6 +79,9 @@ struct ref_format {
>  	 */
>  	const char *format;
>  	int quote_style;
> +
> +	/* Internal state to ref-filter */
> +	int need_color_reset_at_eol;
>  };
>  
>  #define REF_FORMAT_INIT { NULL, 0 }
Jeff King· Jul 13, 2017, 15:02 UTC · re: Jeff King · lore

[PATCH 07/15] ref-filter: provide a function for parsing sort options

The ref-filter module currently provides a callback suitable for parsing command-line --sort options. But since git-tag also supports the tag.sort config option, it needs a function whose implementation is quite similar, but with a slightly different interface. The end result is that builtin/tag.c has a copy-paste of parse_opt_ref_sorting().

Instead, let's provide a function to parse an arbitrary sort string, which we can then trivially wrap to make the parse_opt variant.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/tag.c | 26 +-------------------------
 ref-filter.c  | 13 ++++++++-----
 ref-filter.h  |  2 ++
 3 files changed, 11 insertions(+), 30 deletions(-)
Show changes to 3 files +11 −30

builtin/tag.c, ref-filter.c, ref-filter.h

diff --git a/builtin/tag.c b/builtin/tag.c
index 207c9eb03..66e35b823 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -136,30 +136,6 @@ static const char tag_template_nocleanup[] =
 	"Lines starting with '%c' will be kept; you may remove them"
 	" yourself if you want to.\n");
 
-/* Parse arg given and add it the ref_sorting array */
-static int parse_sorting_string(const char *arg, struct ref_sorting **sorting_tail)
-{
-	struct ref_sorting *s;
-	int len;
-
-	s = xcalloc(1, sizeof(*s));
-	s->next = *sorting_tail;
-	*sorting_tail = s;
-
-	if (*arg == '-') {
-		s->reverse = 1;
-		arg++;
-	}
-	if (skip_prefix(arg, "version:", &arg) ||
-	    skip_prefix(arg, "v:", &arg))
-		s->version = 1;
-
-	len = strlen(arg);
-	s->atom = parse_ref_filter_atom(arg, arg+len);
-
-	return 0;
-}
-
 static int git_tag_config(const char *var, const char *value, void *cb)
 {
 	int status;
@@ -168,7 +144,7 @@ static int git_tag_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "tag.sort")) {
 		if (!value)
 			return config_error_nonbool(var);
-		parse_sorting_string(value, sorting_tail);
+		parse_ref_sorting(sorting_tail, value);
 		return 0;
 	}
 
diff --git a/ref-filter.c b/ref-filter.c
index 178396e1f..432121219 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2126,15 +2126,11 @@ struct ref_sorting *ref_default_sorting(void)
 	return sorting;
 }
 
-int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
+void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)
 {
-	struct ref_sorting **sorting_tail = opt->value;
 	struct ref_sorting *s;
 	int len;
 
-	if (!arg) /* should --no-sort void the list ? */
-		return -1;
-
 	s = xcalloc(1, sizeof(*s));
 	s->next = *sorting_tail;
 	*sorting_tail = s;
@@ -2148,6 +2144,13 @@ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
 		s->version = 1;
 	len = strlen(arg);
 	s->atom = parse_ref_filter_atom(arg, arg+len);
+}
+
+int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
+{
+	if (!arg) /* should --no-sort void the list ? */
+		return -1;
+	parse_ref_sorting(opt->value, arg);
 	return 0;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 9e1e89c19..67fa6261b 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -116,6 +116,8 @@ void format_ref_array_item(struct ref_array_item *info,
 			   struct strbuf *final_buf);
 /*  Print the ref using the given format and quote_style */
 void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);
+/*  Parse a single sort specifier and add it to the list */
+void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);
 /*  Callback function for parsing the sort option */
 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);
 /*  Default sort option based on refname */
-- 
2.13.2.1157.gc6daca446
Jeff King· Jul 13, 2017, 15:02 UTC · re: Jeff King · lore

[PATCH 08/15] ref-filter: make parse_ref_filter_atom a private function

The parse_ref_filter_atom() function really shouldn't be exposed outside of ref-filter.c; its return value is an integer index into an array that is private in that file.

Since the previous commit removed the sole external caller (and replaced it with a public function at a more appropriately level), we can just make this static.

Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c | 2 +-
 ref-filter.h | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)
Show changes to 2 files +1 −3

ref-filter.c, ref-filter.h

diff --git a/ref-filter.c b/ref-filter.c
index 432121219..ee97d7218 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -364,7 +364,7 @@ struct atom_value {
 /*
  * Used to parse format string and sort specifiers
  */
-int parse_ref_filter_atom(const char *atom, const char *ep)
+static int parse_ref_filter_atom(const char *atom, const char *ep)
 {
 	const char *sp;
 	const char *arg;
diff --git a/ref-filter.h b/ref-filter.h
index 67fa6261b..1ffc3ca81 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -104,8 +104,6 @@ struct ref_format {
 int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int type);
 /*  Clear all memory allocated to ref_array */
 void ref_array_clear(struct ref_array *array);
-/*  Parse format string and sort specifiers */
-int parse_ref_filter_atom(const char *atom, const char *ep);
 /*  Used to verify if the given format is correct and to parse out the used atoms */
 int verify_ref_format(struct ref_format *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
-- 
2.13.2.1157.gc6daca446
Jeff King· Jul 13, 2017, 15:02 UTC · re: Jeff King · lore

[PATCH 09/15] ref-filter: factor out the parsing of sorting atoms

We parse sort strings as single formatting atoms, and just build on parse_ref_filter_atom(). Let's pull this idea into its own function, since it's about to get a little more complex. As a bonus, we can give the function a slightly more natural interface, since our single atoms are in their own strings.

Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
Show changes to ref-filter.c +8 −4
diff --git a/ref-filter.c b/ref-filter.c
index ee97d7218..080f5aceb 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2114,6 +2114,12 @@ void pretty_print_ref(const char *name, const unsigned char *sha1,
 	free_array_item(ref_item);
 }
 
+static int parse_sorting_atom(const char *atom)
+{
+	const char *end = atom + strlen(atom);
+	return parse_ref_filter_atom(atom, end);
+}
+
 /*  If no sorting option is given, use refname to sort as default */
 struct ref_sorting *ref_default_sorting(void)
 {
@@ -2122,14 +2128,13 @@ struct ref_sorting *ref_default_sorting(void)
 	struct ref_sorting *sorting = xcalloc(1, sizeof(*sorting));
 
 	sorting->next = NULL;
-	sorting->atom = parse_ref_filter_atom(cstr_name, cstr_name + strlen(cstr_name));
+	sorting->atom = parse_sorting_atom(cstr_name);
 	return sorting;
 }
 
 void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)
 {
 	struct ref_sorting *s;
-	int len;
 
 	s = xcalloc(1, sizeof(*s));
 	s->next = *sorting_tail;
@@ -2142,8 +2147,7 @@ void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)
 	if (skip_prefix(arg, "version:", &arg) ||
 	    skip_prefix(arg, "v:", &arg))
 		s->version = 1;
-	len = strlen(arg);
-	s->atom = parse_ref_filter_atom(arg, arg+len);
+	s->atom = parse_sorting_atom(arg);
 }
 
 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
-- 
2.13.2.1157.gc6daca446
Jeff King· Jul 13, 2017, 15:06 UTC · re: Jeff King · lore

[PATCH 10/15] ref-filter: pass ref_format struct to atom parsers

The callback for parsing each formatting atom gets to see only the atom struct (which it's filling in) and the text to be parsed. This doesn't leave any room for it to behave differently based on context known only to the ref_format.

We can solve this by passing in the surrounding ref_format to each parser. Note that this makes things slightly awkward for sort strings, which parse atoms without having a ref_format. We'll solve that by using a dummy ref_format with default parameters.

Signed-off-by: Jeff King <peff@peff.net>
---
If we eventually do move used_atoms into the format, we'd probably need
to either have a separate ref_format for the sorting atoms, or have them
share with the struct used for actual formatting. At this point I don't
think it matters at all. You can do nonsense like:
  git for-each-ref --sort='%(color:red)'

and after this series it will not respect your color.ui setting. But since in either case it's a ridiculous sort key (whether it shows the color or not, it's the same for all refs and so can't affect the sorting), it's not worth caring about.

If we do eventually refactor ref_format more such that the dummy struct doesn't cut it, I think we can deal with the issue then.

 ref-filter.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)
Show changes to ref-filter.c +25 −17
diff --git a/ref-filter.c b/ref-filter.c
index 080f5aceb..129a63677 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -98,7 +98,7 @@ static struct used_atom {
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
 
-static void color_atom_parser(struct used_atom *atom, const char *color_value)
+static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)
 {
 	if (!color_value)
 		die(_("expected format: %%(color:<color>)"));
@@ -126,7 +126,7 @@ static void refname_atom_parser_internal(struct refname_atom *atom,
 		die(_("unrecognized %%(%s) argument: %s"), name, arg);
 }
 
-static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
+static void remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
 	struct string_list params = STRING_LIST_INIT_DUP;
 	int i;
@@ -160,28 +160,28 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
 	string_list_clear(&params, 0);
 }
 
-static void body_atom_parser(struct used_atom *atom, const char *arg)
+static void body_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
 	if (arg)
 		die(_("%%(body) does not take arguments"));
 	atom->u.contents.option = C_BODY_DEP;
 }
 
-static void subject_atom_parser(struct used_atom *atom, const char *arg)
+static void subject_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
 	if (arg)
 		die(_("%%(subject) does not take arguments"));
 	atom->u.contents.option = C_SUB;
 }
 
-static void trailers_atom_parser(struct used_atom *atom, const char *arg)
+static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
 	if (arg)
 		die(_("%%(trailers) does not take arguments"));
 	atom->u.contents.option = C_TRAILERS;
 }
 
-static void contents_atom_parser(struct used_atom *atom, const char *arg)
+static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
 	if (!arg)
 		atom->u.contents.option = C_BARE;
@@ -201,7 +201,7 @@ static void contents_atom_parser(struct used_atom *atom, const char *arg)
 		die(_("unrecognized %%(contents) argument: %s"), arg);
 }
 
-static void objectname_atom_parser(struct used_atom *atom, const char *arg)
+static void objectname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
 	if (!arg)
 		atom->u.objectname.option = O_FULL;
@@ -218,7 +218,7 @@ static void objectname_atom_parser(struct used_atom *atom, const char *arg)
 		die(_("unrecognized %%(objectname) argument: %s"), arg);
 }
 
-static void refname_atom_parser(struct used_atom *atom, const char *arg)
+static void refname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
 	refname_atom_parser_internal(&atom->u.refname, arg, atom->name);
 }
@@ -234,7 +234,7 @@ static align_type parse_align_position(const char *s)
 	return -1;
 }
 
-static void align_atom_parser(struct used_atom *atom, const char *arg)
+static void align_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
 	struct align *align = &atom->u.align;
 	struct string_list params = STRING_LIST_INIT_DUP;
@@ -273,7 +273,7 @@ static void align_atom_parser(struct used_atom *atom, const char *arg)
 	string_list_clear(&params, 0);
 }
 
-static void if_atom_parser(struct used_atom *atom, const char *arg)
+static void if_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
 	if (!arg) {
 		atom->u.if_then_else.cmp_status = COMPARE_NONE;
@@ -287,7 +287,7 @@ static void if_atom_parser(struct used_atom *atom, const char *arg)
 	}
 }
 
-static void head_atom_parser(struct used_atom *atom, const char *arg)
+static void head_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
 	struct object_id unused;
 
@@ -297,7 +297,7 @@ static void head_atom_parser(struct used_atom *atom, const char *arg)
 static struct {
 	const char *name;
 	cmp_type cmp_type;
-	void (*parser)(struct used_atom *atom, const char *arg);
+	void (*parser)(const struct ref_format *format, struct used_atom *atom, const char *arg);
 } valid_atom[] = {
 	{ "refname" , FIELD_STR, refname_atom_parser },
 	{ "objecttype" },
@@ -364,7 +364,8 @@ struct atom_value {
 /*
  * Used to parse format string and sort specifiers
  */
-static int parse_ref_filter_atom(const char *atom, const char *ep)
+static int parse_ref_filter_atom(const struct ref_format *format,
+				 const char *atom, const char *ep)
 {
 	const char *sp;
 	const char *arg;
@@ -412,7 +413,7 @@ static int parse_ref_filter_atom(const char *atom, const char *ep)
 		arg = used_atom[at].name + (arg - atom) + 1;
 	memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));
 	if (valid_atom[i].parser)
-		valid_atom[i].parser(&used_atom[at], arg);
+		valid_atom[i].parser(format, &used_atom[at], arg);
 	if (*atom == '*')
 		need_tagged = 1;
 	if (!strcmp(valid_atom[i].name, "symref"))
@@ -668,7 +669,7 @@ int verify_ref_format(struct ref_format *format)
 		if (!ep)
 			return error(_("malformed format string %s"), sp);
 		/* sp points at "%(" and ep points at the closing ")" */
-		at = parse_ref_filter_atom(sp + 2, ep);
+		at = parse_ref_filter_atom(format, sp + 2, ep);
 		cp = ep + 1;
 
 		if (skip_prefix(used_atom[at].name, "color:", &color))
@@ -2075,7 +2076,9 @@ void format_ref_array_item(struct ref_array_item *info,
 		ep = strchr(sp, ')');
 		if (cp < sp)
 			append_literal(cp, sp, &state);
-		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
+		get_ref_atom_value(info,
+				   parse_ref_filter_atom(format, sp + 2, ep),
+				   &atomv);
 		atomv->handler(atomv, &state);
 	}
 	if (*cp) {
@@ -2116,8 +2119,13 @@ void pretty_print_ref(const char *name, const unsigned char *sha1,
 
 static int parse_sorting_atom(const char *atom)
 {
+	/*
+	 * This parses an atom using a dummy ref_format, since we don't
+	 * actually care about the formatting details.
+	 */
+	struct ref_format dummy = REF_FORMAT_INIT;
 	const char *end = atom + strlen(atom);
-	return parse_ref_filter_atom(atom, end);
+	return parse_ref_filter_atom(&dummy, atom, end);
 }
 
 /*  If no sorting option is given, use refname to sort as default */
-- 
2.13.2.1157.gc6daca446
Jeff King· Jul 13, 2017, 15:07 UTC · re: Jeff King · lore

[PATCH 11/15] color: check color.ui in git_default_config()

Back in prehistoric times, our decision on whether or not to show color by default relied on using a config callback that either did or didn't load color config like color.diff. When we introduced color.ui, we put it in the same boat: commands had to manually respect it by using git_color_config() or its git_color_default_config() convenience wrapper.

But in 4c7f1819b (make color.ui default to 'auto', 2013-06-10), that changed. Since then, we default color.ui to auto in all programs, meaning that even plumbing commands like "git diff-tree --pretty" might colorize the output. Nobody seems to have complained in the intervening years, presumably because the "is stdout a tty" check does a good job of catching the right cases.

But that leaves an interesting curiosity: color.ui defaults to auto even in plumbing, but you can't actually _disable_ the color via config. So if you really hate color and set "color.ui" to false, diff-tree will still show color (but porcelain like git-diff won't). Nobody noticed that either, probably because very few people disable color.

One could argue that the plumbing should _always_ disable color unless an explicit --color option is given on the command line. But in practice, this creates a lot of complications for scripts which do want plumbing to show user-visible output. They can't just pass "--color" blindly; they need to check the user's config and decide what to send.

Given that nobody has complained about the current behavior, let's assume it's a good path, and follow it to its conclusion: supporting color.ui everywhere.

Note that you can create havoc by setting color.ui=always in your config, but that's more or less already the case. We could disallow it entirely, but it is handy for one-offs like:

  git -c color.ui=always foo >not-a-tty
when "foo" does not take a --color option itself.
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c      | 2 +-
 builtin/clean.c       | 3 +--
 builtin/grep.c        | 2 +-
 builtin/show-branch.c | 2 +-
 color.c               | 8 --------
 config.c              | 4 ++++
 diff.c                | 3 ---
 7 files changed, 8 insertions(+), 16 deletions(-)
Show changes to 7 files +8 −16

builtin/branch.c, builtin/clean.c, builtin/grep.c, builtin/show-branch.c, color.c, config.c, diff.c

diff --git a/builtin/branch.c b/builtin/branch.c
index 036fdc929..d852ded49 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -92,7 +92,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)
 			return config_error_nonbool(var);
 		return color_parse(value, branch_colors[slot]);
 	}
-	return git_color_default_config(var, value, cb);
+	return git_default_config(var, value, cb);
 }
 
 static const char *branch_get_color(enum color_branch ix)
diff --git a/builtin/clean.c b/builtin/clean.c
index 057fc97fe..c1bafda5b 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -125,8 +125,7 @@ static int git_clean_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	/* inspect the color.ui config variable and others */
-	return git_color_default_config(var, value, cb);
+	return git_default_config(var, value, cb);
 }
 
 static const char *clean_get_color(enum color_clean ix)
diff --git a/builtin/grep.c b/builtin/grep.c
index 0d6e66973..a7157f563 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -284,7 +284,7 @@ static int wait_all(void)
 static int grep_cmd_config(const char *var, const char *value, void *cb)
 {
 	int st = grep_config(var, value, cb);
-	if (git_color_default_config(var, value, cb) < 0)
+	if (git_default_config(var, value, cb) < 0)
 		st = -1;
 
 	if (!strcmp(var, "grep.threads")) {
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 7073a3eb9..28f245c8c 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -554,7 +554,7 @@ static int git_show_branch_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	return git_color_default_config(var, value, cb);
+	return git_default_config(var, value, cb);
 }
 
 static int omit_in_dense(struct commit *commit, struct commit **rev, int n)
diff --git a/color.c b/color.c
index 31b6207a0..7aa8b076f 100644
--- a/color.c
+++ b/color.c
@@ -361,14 +361,6 @@ int git_color_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
-int git_color_default_config(const char *var, const char *value, void *cb)
-{
-	if (git_color_config(var, value, cb) < 0)
-		return -1;
-
-	return git_default_config(var, value, cb);
-}
-
 void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb)
 {
 	if (*color)
diff --git a/config.c b/config.c
index a9356c138..bc290e756 100644
--- a/config.c
+++ b/config.c
@@ -16,6 +16,7 @@
 #include "string-list.h"
 #include "utf8.h"
 #include "dir.h"
+#include "color.h"
 
 struct config_source {
 	struct config_source *prev;
@@ -1350,6 +1351,9 @@ int git_default_config(const char *var, const char *value, void *dummy)
 	if (starts_with(var, "advice."))
 		return git_default_advice_config(var, value);
 
+	if (git_color_config(var, value, dummy) < 0)
+		return -1;
+
 	if (!strcmp(var, "pager.color") || !strcmp(var, "color.pager")) {
 		pager_use_color = git_config_bool(var,value);
 		return 0;
diff --git a/diff.c b/diff.c
index 85e714f6c..9c3825803 100644
--- a/diff.c
+++ b/diff.c
@@ -299,9 +299,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	if (git_color_config(var, value, cb) < 0)
-		return -1;
-
 	return git_diff_basic_config(var, value, cb);
 }
 
-- 
2.13.2.1157.gc6daca446
Jeff King· Jul 13, 2017, 15:07 UTC · re: Jeff King · lore

[PATCH 12/15] for-each-ref: load config earlier

In most commands we load config before parsing command line options, since it lets the latter override the former with a simple variable assignment. In the case of for-each-ref, though, we do it in the reverse order. This is OK with the current code, since there's no interaction between the config and command-line options.

However, as the ref-filter code starts to care about config during verify_ref_format(), we'll want to make sure the config is loaded. Let's bump the config to the usual spot near the top of the function.

We can drop the comment there; it's impossible to keep a "why we load the config" comment like this up to date with every config option we might be interested in. And indeed, it's already stale; we'd care about core.abbrev, for instance, when %(objectname:short) is used.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/for-each-ref.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
Show changes to builtin/for-each-ref.c +2 −3
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index f47066b42..5d7c921a7 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -54,6 +54,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 
 	format.format = "%(objectname) %(objecttype)\t%(refname)";
 
+	git_config(git_default_config, NULL);
+
 	parse_options(argc, argv, prefix, opts, for_each_ref_usage, 0);
 	if (maxcount < 0) {
 		error("invalid --count argument: `%d'", maxcount);
@@ -71,9 +73,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	sorting->ignore_case = icase;
 	filter.ignore_case = icase;
 
-	/* for warn_ambiguous_refs */
-	git_config(git_default_config, NULL);
-
 	filter.name_patterns = argv;
 	filter.match_as_path = 1;
 	filter_refs(&array, &filter, FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN);
-- 
2.13.2.1157.gc6daca446
Jeff King· Jul 13, 2017, 15:07 UTC · re: Jeff King · lore

[PATCH 13/15] rev-list: pass diffopt->use_colors through to pretty-print

When rev-list pretty-prints a commit, it creates a new pretty_print_context and copies items from the rev_info struct. We don't currently copy the "use_color" field, though. Nobody seems to have noticed because the only part of pretty.c that cares is the %C(auto,...) placeholder, and presumably not many people use that with the rev-list plumbing (as opposed to with git-log).

It will become more noticeable in a future patch, though, when we start treating all user-format colors as auto-colors (in which case it would become impossible to format colors with rev-list, even with --color=always).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/rev-list.c         |  1 +
 t/t6006-rev-list-format.sh | 11 +++++++++++
 2 files changed, 12 insertions(+)
Show changes to 2 files +12 −0

builtin/rev-list.c, t/t6006-rev-list-format.sh

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 95d84d5cd..fee10d856 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -122,6 +122,7 @@ static void show_commit(struct commit *commit, void *data)
 		ctx.date_mode_explicit = revs->date_mode_explicit;
 		ctx.fmt = revs->commit_format;
 		ctx.output_encoding = get_log_output_encoding();
+		ctx.color = revs->diffopt.use_color;
 		pretty_print_commit(&ctx, commit, &buf);
 		if (buf.len) {
 			if (revs->commit_format != CMIT_FMT_ONELINE)
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 647218b4e..7b97a90ba 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -245,6 +245,17 @@ test_expect_success '%C(auto) respects --no-color' '
 	test_cmp expect actual
 '
 
+test_expect_success 'rev-list %C(auto,...) respects --color' '
+	git rev-list --color --format="%C(auto,green)foo%C(auto,reset)" \
+		-1 HEAD >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	cat >expect <<-EOF &&
+	commit $(git rev-parse HEAD)
+	<GREEN>foo<RESET>
+	EOF
+	test_cmp expect actual
+'
+
 iconv -f utf-8 -t $test_encoding > commit-msg <<EOF
 Test printing of complex bodies
 
-- 
2.13.2.1157.gc6daca446
Jeff King· Jul 13, 2017, 15:08 UTC · re: Jeff King · lore

[PATCH 14/15] pretty: respect color settings for %C placeholders

The color placeholders have traditionally been unconditional, showing colors even when git is not otherwise configured to do so. This was not so bad for their original use, which was on the command-line (and the user could decide at that moment whether to add colors or not). But these days we have configured formats via pretty.*, and those should operate correctly in multiple contexts.

In 3082517 (log --format: teach %C(auto,black) to respect color config, 2012-12-17), we gave an extended placeholder that could be used to accomplish this. But it's rather clunky to use, because you have to specify it individually for each color (and their matching resets) in the format. We shied away from just switching the default to auto, because it is technically breaking backwards compatibility.

However, there's not really a use case for unconditional colors. The most plausible reason you would want them is to redirect "git log" output to a file. But there, the right answer is --color=always, as it does the right thing both with custom user-format colors and git-generated colors.

So let's switch to the more useful default. In the off-chance that somebody really does find a use for unconditional colors without wanting to enable the rest of git's colors, we provide a new %C(always,...) to enable the old behavior. And we can remind them of --color=always in the documentation.

Signed-off-by: Jeff King <peff@peff.net>
---
The test diff is a lot easier to read with "-w", as it moves all of the
%C(auto,...) tests into a loop that checks all three variants.
 Documentation/pretty-formats.txt | 18 +++++----
 pretty.c                         | 27 +++++++++++--
 t/t6006-rev-list-format.sh       | 84 ++++++++++++++++++++++++----------------
 3 files changed, 84 insertions(+), 45 deletions(-)
Show changes to 3 files +84 −45

Documentation/pretty-formats.txt, pretty.c, t/t6006-rev-list-format.sh

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 4d6dac577..973d19606 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -173,13 +173,17 @@ endif::git-rev-list[]
 - '%Cblue': switch color to blue
 - '%Creset': reset color
 - '%C(...)': color specification, as described under Values in the
-  "CONFIGURATION FILE" section of linkgit:git-config[1];
-  adding `auto,` at the beginning (e.g. `%C(auto,red)`) will emit
-  color only when colors are enabled for log output (by `color.diff`,
-  `color.ui`, or `--color`, and respecting the `auto` settings of the
-  former if we are going to a terminal). `auto` alone (i.e.
-  `%C(auto)`) will turn on auto coloring on the next placeholders
-  until the color is switched again.
+  "CONFIGURATION FILE" section of linkgit:git-config[1].
+  By default, colors are shown only when enabled for log output (by
+  `color.diff`, `color.ui`, or `--color`, and respecting the `auto`
+  settings of the former if we are going to a terminal). `%C(auto,...)`
+  is accepted as a historical synonym for the default (e.g.,
+  `%C(auto,red)`). Specifying `%C(always,...) will show the colors
+  even when color is not otherwise enabled (though consider
+  just using `--color=always` to enable color for the whole output,
+  including this format and anything else git might color).  `auto`
+  alone (i.e. `%C(auto)`) will turn on auto coloring on the next
+  placeholders until the color is switched again.
 - '%m': left (`<`), right (`>`) or boundary (`-`) mark
 - '%n': newline
 - '%%': a raw '%'
diff --git a/pretty.c b/pretty.c
index e4b561c58..fd781e54f 100644
--- a/pretty.c
+++ b/pretty.c
@@ -947,6 +947,7 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */
 			  struct format_commit_context *c)
 {
 	const char *rest = placeholder;
+	const char *basic_color = NULL;
 
 	if (placeholder[1] == '(') {
 		const char *begin = placeholder + 2;
@@ -955,23 +956,41 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */
 
 		if (!end)
 			return 0;
+
 		if (skip_prefix(begin, "auto,", &begin)) {
 			if (!want_color(c->pretty_ctx->color))
 				return end - placeholder + 1;
+		} else if(skip_prefix(begin, "always,", &begin)) {
+			/* nothing to do; we do not respect want_color at all */
+		} else {
+			/* the default is the same as "auto" */
+			if (!want_color(c->pretty_ctx->color))
+				return end - placeholder + 1;
 		}
+
 		if (color_parse_mem(begin, end - begin, color) < 0)
 			die(_("unable to parse --pretty format"));
 		strbuf_addstr(sb, color);
 		return end - placeholder + 1;
 	}
+
+	/*
+	 * We handle things like "%C(red)" above; for historical reasons, there
+	 * are a few colors that can be specified without parentheses (and
+	 * they cannot support things like "auto" or "always" at all).
+	 */
 	if (skip_prefix(placeholder + 1, "red", &rest))
-		strbuf_addstr(sb, GIT_COLOR_RED);
+		basic_color = GIT_COLOR_RED;
 	else if (skip_prefix(placeholder + 1, "green", &rest))
-		strbuf_addstr(sb, GIT_COLOR_GREEN);
+		basic_color = GIT_COLOR_GREEN;
 	else if (skip_prefix(placeholder + 1, "blue", &rest))
-		strbuf_addstr(sb, GIT_COLOR_BLUE);
+		basic_color = GIT_COLOR_BLUE;
 	else if (skip_prefix(placeholder + 1, "reset", &rest))
-		strbuf_addstr(sb, GIT_COLOR_RESET);
+		basic_color = GIT_COLOR_RESET;
+
+	if (basic_color && want_color(c->pretty_ctx->color))
+		strbuf_addstr(sb, basic_color);
+
 	return rest - placeholder;
 }
 
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 7b97a90ba..b326d550f 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -59,7 +59,10 @@ test_format () {
 }
 
 # Feed to --format to provide predictable colored sequences.
+BASIC_COLOR='%Credfoo%Creset'
+COLOR='%C(red)foo%C(reset)'
 AUTO_COLOR='%C(auto,red)foo%C(auto,reset)'
+ALWAYS_COLOR='%C(always,red)foo%C(always,reset)'
 has_color () {
 	test_decode_color <"$1" >decoded &&
 	echo "<RED>foo<RESET>" >expect &&
@@ -177,7 +180,7 @@ test_expect_success 'basic colors' '
 	<RED>foo<GREEN>bar<BLUE>baz<RESET>xyzzy
 	EOF
 	format="%Credfoo%Cgreenbar%Cbluebaz%Cresetxyzzy" &&
-	git rev-list --format="$format" -1 master >actual.raw &&
+	git rev-list --color --format="$format" -1 master >actual.raw &&
 	test_decode_color <actual.raw >actual &&
 	test_cmp expect actual
 '
@@ -188,48 +191,61 @@ test_expect_success 'advanced colors' '
 	<BOLD;RED;BYELLOW>foo<RESET>
 	EOF
 	format="%C(red yellow bold)foo%C(reset)" &&
-	git rev-list --format="$format" -1 master >actual.raw &&
+	git rev-list --color --format="$format" -1 master >actual.raw &&
 	test_decode_color <actual.raw >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success '%C(auto,...) does not enable color by default' '
-	git log --format=$AUTO_COLOR -1 >actual &&
-	has_no_color actual
-'
-
-test_expect_success '%C(auto,...) enables colors for color.diff' '
-	git -c color.diff=always log --format=$AUTO_COLOR -1 >actual &&
-	has_color actual
-'
-
-test_expect_success '%C(auto,...) enables colors for color.ui' '
-	git -c color.ui=always log --format=$AUTO_COLOR -1 >actual &&
-	has_color actual
-'
+for spec in \
+	"%Cred:$BASIC_COLOR" \
+	"%C(...):$COLOR" \
+	"%C(auto,...):$AUTO_COLOR"
+do
+	desc=${spec%%:*}
+	color=${spec#*:}
+	test_expect_success "$desc does not enable color by default" '
+		git log --format=$color -1 >actual &&
+		has_no_color actual
+	'
 
-test_expect_success '%C(auto,...) respects --color' '
-	git log --format=$AUTO_COLOR -1 --color >actual &&
-	has_color actual
-'
+	test_expect_success "$desc enables colors for color.diff" '
+		git -c color.diff=always log --format=$color -1 >actual &&
+		has_color actual
+	'
 
-test_expect_success '%C(auto,...) respects --no-color' '
-	git -c color.ui=always log --format=$AUTO_COLOR -1 --no-color >actual &&
-	has_no_color actual
-'
+	test_expect_success "$desc enables colors for color.ui" '
+		git -c color.ui=always log --format=$color -1 >actual &&
+		has_color actual
+	'
 
-test_expect_success TTY '%C(auto,...) respects --color=auto (stdout is tty)' '
-	test_terminal env TERM=vt100 \
-		git log --format=$AUTO_COLOR -1 --color=auto >actual &&
-	has_color actual
-'
+	test_expect_success "$desc respects --color" '
+		git log --format=$color -1 --color >actual &&
+		has_color actual
+	'
 
-test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' '
-	(
-		TERM=vt100 && export TERM &&
-		git log --format=$AUTO_COLOR -1 --color=auto >actual &&
+	test_expect_success "$desc respects --no-color" '
+		git -c color.ui=always log --format=$color -1 --no-color >actual &&
 		has_no_color actual
-	)
+	'
+
+	test_expect_success TTY "$desc respects --color=auto (stdout is tty)" '
+		test_terminal env TERM=vt100 \
+			git log --format=$color -1 --color=auto >actual &&
+		has_color actual
+	'
+
+	test_expect_success "$desc respects --color=auto (stdout not tty)" '
+		(
+			TERM=vt100 && export TERM &&
+			git log --format=$color -1 --color=auto >actual &&
+			has_no_color actual
+		)
+	'
+done
+
+test_expect_success '%C(always,...) enables color even without tty' '
+	git log --format=$ALWAYS_COLOR -1 >actual &&
+	has_color actual
 '
 
 test_expect_success '%C(auto) respects --color' '
-- 
2.13.2.1157.gc6daca446
Jeff King· Jul 13, 2017, 15:09 UTC · re: Jeff King · lore

[PATCH 15/15] ref-filter: consult want_color() before emitting colors

When color placeholders like %(color:red) are used in a ref-filter format, we unconditionally output the colors, even if the user has asked us for no colors. This usually isn't a problem when the user is constructing a --format on the command line, but it means we may do the wrong thing when the format is fed from a script or alias. For example:

   $ git config alias.b 'branch --format=%(color:green)%(refname)'
   $ git b --no-color
should probably omit the green color. Likewise, running:
   $ git b >branches

should probably also omit the color, just as we would for all baked-in coloring (and as we recently started to do for user-specified colors in --pretty formats).

This commit makes both of those cases work by teaching the ref-filter code to consult want_color() before outputting any color. The color flag in ref_format defaults to "-1", which means we'll consult color.ui, which in turn defaults to the usual isatty() check on stdout. However, callers like git-branch which support their own color config (and command-line options) can override that.

The new tests independently cover all three of the callers of ref-filter (for-each-ref, tag, and branch). Even though these seem redundant, it confirms that we've correctly plumbed through all of the necessary config to make colors work by default.

Signed-off-by: Jeff King <peff@peff.net>
---
The "correctly plumbed" bits happened in the earlier patches. But it was
these tests that showed me they were broken.
 builtin/branch.c         |  1 +
 ref-filter.c             |  8 ++++++++
 ref-filter.h             |  3 ++-
 t/t3203-branch-output.sh | 31 +++++++++++++++++++++++++++++++
 t/t6300-for-each-ref.sh  | 37 ++++++++++++++++++++++++++-----------
 t/t7004-tag.sh           | 25 +++++++++++++++++++++++++
 6 files changed, 93 insertions(+), 12 deletions(-)
Show changes to 6 files +93 −12

builtin/branch.c, ref-filter.c, ref-filter.h, t/t3203-branch-output.sh, t/t6300-for-each-ref.sh, t/t7004-tag.sh

diff --git a/builtin/branch.c b/builtin/branch.c
index d852ded49..16d391b40 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -409,6 +409,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 
 	if (!format->format)
 		format->format = to_free = build_format(filter, maxwidth, remote_prefix);
+	format->use_color = branch_use_color;
 
 	if (verify_ref_format(format))
 		die(_("unable to parse format string"));
diff --git a/ref-filter.c b/ref-filter.c
index 129a63677..bc591f4f3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -104,6 +104,12 @@ static void color_atom_parser(const struct ref_format *format, struct used_atom
 		die(_("expected format: %%(color:<color>)"));
 	if (color_parse(color_value, atom->u.color) < 0)
 		die(_("unrecognized color: %%(color:%s)"), color_value);
+	/*
+	 * We check this after we've parsed the color, which lets us complain
+	 * about syntactically bogus color names even if they won't be used.
+	 */
+	if (!want_color(format->use_color))
+		color_parse("", atom->u.color);
 }
 
 static void refname_atom_parser_internal(struct refname_atom *atom,
@@ -675,6 +681,8 @@ int verify_ref_format(struct ref_format *format)
 		if (skip_prefix(used_atom[at].name, "color:", &color))
 			format->need_color_reset_at_eol = !!strcmp(color, "reset");
 	}
+	if (format->need_color_reset_at_eol && !want_color(format->use_color))
+		format->need_color_reset_at_eol = 0;
 	return 0;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 1ffc3ca81..0d98342b3 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -79,12 +79,13 @@ struct ref_format {
 	 */
 	const char *format;
 	int quote_style;
+	int use_color;
 
 	/* Internal state to ref-filter */
 	int need_color_reset_at_eol;
 };
 
-#define REF_FORMAT_INIT { NULL, 0 }
+#define REF_FORMAT_INIT { NULL, 0, -1 }
 
 /*  Macros for checking --merged and --no-merged options */
 #define _OPT_MERGED_NO_MERGED(option, filter, h) \
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index a428ae670..d2aec0f38 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -2,6 +2,7 @@
 
 test_description='git branch display tests'
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 test_expect_success 'make commits' '
 	echo content >file &&
@@ -239,4 +240,34 @@ test_expect_success 'git branch --format option' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success "set up color tests" '
+	echo "<RED>master<RESET>" >expect.color &&
+	echo "master" >expect.bare &&
+	color_args="--format=%(color:red)%(refname:short) --list master"
+'
+
+test_expect_success '%(color) omitted without tty' '
+	TERM=vt100 git branch $color_args >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	test_cmp expect.bare actual
+'
+
+test_expect_success TTY '%(color) present with tty' '
+	test_terminal env TERM=vt100 git branch $color_args >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	test_cmp expect.color actual
+'
+
+test_expect_success 'color.branch=always overrides auto-color' '
+	git -c color.branch=always branch $color_args >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	test_cmp expect.color actual
+'
+
+test_expect_success '--color overrides auto-color' '
+	git branch --color $color_args >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	test_cmp expect.color actual
+'
+
 test_done
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 7872a2f54..2274a4b73 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -7,6 +7,7 @@ test_description='for-each-ref test'
 
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 # Mon Jul 3 23:18:43 2006 +0000
 datestamp=1151968723
@@ -412,19 +413,33 @@ test_expect_success 'Check for invalid refname format' '
 	test_must_fail git for-each-ref --format="%(refname:INVALID)"
 '
 
-cat >expected <<EOF
-$(git rev-parse --short refs/heads/master) <GREEN>master<RESET>
-$(git rev-parse --short refs/remotes/origin/master) <GREEN>origin/master<RESET>
-$(git rev-parse --short refs/tags/testtag) <GREEN>testtag<RESET>
-$(git rev-parse --short refs/tags/two) <GREEN>two<RESET>
-EOF
+test_expect_success 'set up color tests' '
+	cat >expected.color <<-EOF &&
+	$(git rev-parse --short refs/heads/master) <GREEN>master<RESET>
+	$(git rev-parse --short refs/remotes/origin/master) <GREEN>origin/master<RESET>
+	$(git rev-parse --short refs/tags/testtag) <GREEN>testtag<RESET>
+	$(git rev-parse --short refs/tags/two) <GREEN>two<RESET>
+	EOF
+	sed "s/<[^>]*>//g" <expected.color >expected.bare &&
+	color_format="%(objectname:short) %(color:green)%(refname:short)"
+'
 
-test_expect_success 'Check %(color:...) ' '
-	git for-each-ref \
-		--format="%(objectname:short) %(color:green)%(refname:short)" \
-		>actual.raw &&
+test_expect_success TTY '%(color) shows color with a tty' '
+	test_terminal env TERM=vt100 \
+		git for-each-ref --format="$color_format" >actual.raw &&
 	test_decode_color <actual.raw >actual &&
-	test_cmp expected actual
+	test_cmp expected.color actual
+'
+
+test_expect_success '%(color) does not show color without tty' '
+	TERM=vt100 git for-each-ref --format="$color_format" >actual &&
+	test_cmp expected.bare actual
+'
+
+test_expect_success 'color.ui=always can override tty check' '
+	git -c color.ui=always for-each-ref --format="$color_format" >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	test_cmp expected.color actual
 '
 
 cat >expected <<\EOF
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 0ef7b9439..dd5ba450e 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -9,6 +9,7 @@ Tests for operations with tags.'
 
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 # creating and listing lightweight tags:
 
@@ -1900,6 +1901,30 @@ test_expect_success '--format should list tags as per format given' '
 	test_cmp expect actual
 '
 
+test_expect_success "set up color tests" '
+	echo "<RED>v1.0<RESET>" >expect.color &&
+	echo "v1.0" >expect.bare &&
+	color_args="--format=%(color:red)%(refname:short) --list v1.0"
+'
+
+test_expect_success '%(color) omitted without tty' '
+	TERM=vt100 git tag $color_args >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	test_cmp expect.bare actual
+'
+
+test_expect_success TTY '%(color) present with tty' '
+	test_terminal env TERM=vt100 git tag $color_args >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	test_cmp expect.color actual
+'
+
+test_expect_success 'color.ui=always overrides auto-color' '
+	git -c color.ui=always tag $color_args >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	test_cmp expect.color actual
+'
+
 test_expect_success 'setup --merged test tags' '
 	git tag mergetest-1 HEAD~2 &&
 	git tag mergetest-2 HEAD~1 &&
-- 
2.13.2.1157.gc6daca446
Stefan Beller· Jul 13, 2017, 19:14 UTC · re: Jeff King · lore

Re: [PATCH 0/15] making user-format colors conditional on config/tty

On Thu, Jul 13, 2017 at 7:55 AM, Jeff King <peff@peff.net> wrote:
Show 21 quoted lines
> This is a cleanup of the patch I posted last October:
>
>   https://public-inbox.org/git/20161010151517.6wszhuyp57yfncaj@sigill.intra.peff.net/
>
> The general idea is that it's rather confusing that "%C(red)" in a
> pretty-print format does not currently respect color.ui, --no-color, or
> the usual isatty check on stdout. This series changes that. Note that
> this is a backwards-incompatible change, but the general sentiment in
> that earlier thread seemed to be that the existing behavior is arguably
> buggy. See patch 14 for more discussion.
>
> The patch stalled back then because I wanted to make sure that
> ref-filter's color placeholders behaved the same. That required some
> refactoring which conflicted badly with kn/ref-filter-branch-list. Now
> that it has graduated, I was able to rebase on top.
>
> This version also takes into account feedback from the original thread.
> And as I added tests, it surfaced a few corner cases around color config
> that I've dealt with here.  The last two patches are the most
> interesting bits.
>
I have reviewed these slightly and think this series is a good change.

Thanks, Stefan

Junio C Hamano· Jul 13, 2017, 20:46 UTC · re: Jeff King · lore

Re: [PATCH 0/15] making user-format colors conditional on config/tty

Jeff King <peff@peff.net> writes:
Show 20 quoted lines
> This version also takes into account feedback from the original thread.
> And as I added tests, it surfaced a few corner cases around color config
> that I've dealt with here.  The last two patches are the most
> interesting bits.
>
>   [01/15]: check return value of verify_ref_format()
>   [02/15]: docs/for-each-ref: update pointer to color syntax
>   [03/15]: t: use test_decode_color rather than literal ANSI codes
>   [04/15]: ref-filter: simplify automatic color reset
>   [05/15]: ref-filter: abstract ref format into its own struct
>   [06/15]: ref-filter: move need_color_reset_at_eol into ref_format
>   [07/15]: ref-filter: provide a function for parsing sort options
>   [08/15]: ref-filter: make parse_ref_filter_atom a private function
>   [09/15]: ref-filter: factor out the parsing of sorting atoms
>   [10/15]: ref-filter: pass ref_format struct to atom parsers
>   [11/15]: color: check color.ui in git_default_config()
>   [12/15]: for-each-ref: load config earlier
>   [13/15]: rev-list: pass diffopt->use_colors through to pretty-print
>   [14/15]: pretty: respect color settings for %C placeholders
>   [15/15]: ref-filter: consult want_color() before emitting colors

Overall I think the endgame is what we want to have in the future (rather, what I wish we had from the beginning). I'd have to think about 11, 14 and 15 a bit more before saying anything that would be remotely useful.

Thanks.
Show 26 quoted lines
>
>  Documentation/git-for-each-ref.txt |   6 +-
>  Documentation/pretty-formats.txt   |  18 ++++--
>  builtin/branch.c                   |  21 +++---
>  builtin/clean.c                    |   3 +-
>  builtin/for-each-ref.c             |  27 ++++----
>  builtin/grep.c                     |   2 +-
>  builtin/rev-list.c                 |   1 +
>  builtin/show-branch.c              |   2 +-
>  builtin/tag.c                      |  61 ++++++------------
>  builtin/verify-tag.c               |  14 ++--
>  color.c                            |   8 ---
>  config.c                           |   4 ++
>  diff.c                             |   3 -
>  pretty.c                           |  27 ++++++--
>  ref-filter.c                       | 108 ++++++++++++++++++-------------
>  ref-filter.h                       |  30 +++++++--
>  t/t3203-branch-output.sh           |  31 +++++++++
>  t/t4207-log-decoration-colors.sh   |  22 +++----
>  t/t6006-rev-list-format.sh         | 129 ++++++++++++++++++++++++-------------
>  t/t6300-for-each-ref.sh            |  39 +++++++----
>  t/t7004-tag.sh                     |  25 +++++++
>  t/test-lib-functions.sh            |   1 +
>  22 files changed, 362 insertions(+), 220 deletions(-)
>
> -Peff

← back to recent threads