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

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

[Jeff King]

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

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

[Jeff King]

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(-)

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
-^[[31mfoo^[[32mbar^[[34mbaz^[[mxyzzy
-commit $head1
-^[[31mfoo^[[32mbar^[[34mbaz^[[mxyzzy
-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
-^[[1;31;43mfoo^[[m
-commit $head1
-^[[1;31;43mfoo^[[m
-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

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

[Stefan Beller]

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.

> @@ -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).



> @@ -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 :)

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

[Jeff King]

On Thu, Jul 13, 2017 at 11:40:32AM -0700, Stefan Beller wrote:

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

> > @@ -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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

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

[Jeff King]

On Thu, Jul 13, 2017 at 12:27:41PM -0700, Junio C Hamano wrote:

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Stefan Beller]

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. ;)

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

[Junio C Hamano]

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.

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.

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

[Junio C Hamano]

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.

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.

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

[Stefan Beller]

On Thu, Jul 13, 2017 at 2:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

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

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

[Jeff King]

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(-)

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

>
>  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 }

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(+)

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Stefan Beller]

On Thu, Jul 13, 2017 at 7:55 AM, Jeff King <peff@peff.net> wrote:
> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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