[PATCH (resend) 0/3] Minor f-e-r enhacements

[PATCH (resend) 0/3] Minor f-e-r enhacements

[Ramkumar Ramachandra]

Hi,

I've been running git with these patches applied locally for a long
time. Although I've sent them to the list before, they've been
overlooked.

Thanks.

Ramkumar Ramachandra (3):
  for-each-ref: introduce %C(...) for color
  for-each-ref: introduce %(HEAD) asterisk marker
  for-each-ref: introduce %(upstream:track[short])

 Documentation/git-for-each-ref.txt | 14 ++++++-
 builtin/for-each-ref.c             | 75 ++++++++++++++++++++++++++++++++++----
 2 files changed, 79 insertions(+), 10 deletions(-)

-- 
1.8.5.rc0.3.gb488857

[PATCH 1/3] for-each-ref: introduce %C(...) for color

[Ramkumar Ramachandra]

Enhance 'git for-each-ref' with color formatting options.  You can now
use the following format in for-each-ref:

  %C(green)%(refname:short)%C(reset)

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-for-each-ref.txt |  4 +++-
 builtin/for-each-ref.c             | 23 +++++++++++++++++++----
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index f2e08d1..6fa4464 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -45,7 +45,9 @@ OPTIONS
 	It also interpolates `%%` to `%`, and `%xx` where `xx`
 	are hex digits interpolates to character with hex code
 	`xx`; for example `%00` interpolates to `\0` (NUL),
-	`%09` to `\t` (TAB) and `%0a` to `\n` (LF).
+	`%09` to `\t` (TAB) and `%0a` to `\n` (LF). Additionally,
+	colors can be specified using `%C(...)`, with names
+	described in color.branch.*.
 
 <pattern>...::
 	If one or more patterns are given, only refs are shown that
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 1d4083c..6da2903 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -9,6 +9,7 @@
 #include "quote.h"
 #include "parse-options.h"
 #include "remote.h"
+#include "color.h"
 
 /* Quoting styles */
 #define QUOTE_NONE 0
@@ -155,10 +156,13 @@ static const char *find_next(const char *cp)
 	while (*cp) {
 		if (*cp == '%') {
 			/*
+			 * %C( is the start of a color;
 			 * %( is the start of an atom;
 			 * %% is a quoted per-cent.
 			 */
-			if (cp[1] == '(')
+			if (cp[1] == 'C' && cp[2] == '(')
+				return cp;
+			else if (cp[1] == '(')
 				return cp;
 			else if (cp[1] == '%')
 				cp++; /* skip over two % */
@@ -180,8 +184,11 @@ static int verify_format(const char *format)
 		const char *ep = strchr(sp, ')');
 		if (!ep)
 			return error("malformed format string %s", sp);
-		/* sp points at "%(" and ep points at the closing ")" */
-		parse_atom(sp + 2, ep);
+		/* Ignore color specifications: %C(
+		 * sp points at "%(" and ep points at the closing ")"
+		 */
+		if (prefixcmp(sp, "%C("))
+			parse_atom(sp + 2, ep);
 		cp = ep + 1;
 	}
 	return 0;
@@ -933,12 +940,20 @@ static void emit(const char *cp, const char *ep)
 static void show_ref(struct refinfo *info, const char *format, int quote_style)
 {
 	const char *cp, *sp, *ep;
+	char color[COLOR_MAXLEN] = "";
 
 	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
 		ep = strchr(sp, ')');
 		if (cp < sp)
 			emit(cp, sp);
-		print_value(info, parse_atom(sp + 2, ep), quote_style);
+
+		/* Do we have a color specification? */
+		if (!prefixcmp(sp, "%C("))
+			color_parse_mem(sp + 3, ep - sp - 3, "--format", color);
+		else {
+			printf("%s", color);
+			print_value(info, parse_atom(sp + 2, ep), quote_style);
+		}
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);
-- 
1.8.5.rc0.3.gb488857

[PATCH 2/3] for-each-ref: introduce %(HEAD) asterisk marker

[Ramkumar Ramachandra]

'git branch' shows which branch you are currently on with an '*', but
'git for-each-ref' misses this feature.  So, extend its format with
%(HEAD) for the same effect.

Now you can use the following format in for-each-ref:

  %C(red)%(HEAD)%C(reset) %(refname:short)

to display a red asterisk next to the current ref.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-for-each-ref.txt |  4 ++++
 builtin/for-each-ref.c             | 13 +++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 6fa4464..bb9c4c1 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -95,6 +95,10 @@ upstream::
 	from the displayed ref. Respects `:short` in the same way as
 	`refname` above.
 
+HEAD::
+	Used to indicate the currently checked out branch.  Is '*' if
+	HEAD points to the current ref, and ' ' otherwise.
+
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
 be used to specify the value in the header field.
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 6da2903..b841545 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -76,6 +76,7 @@ static struct {
 	{ "upstream" },
 	{ "symref" },
 	{ "flag" },
+	{ "HEAD" },
 };
 
 /*
@@ -682,8 +683,16 @@ static void populate_value(struct refinfo *ref)
 				v->s = xstrdup(buf + 1);
 			}
 			continue;
-		}
-		else
+		} else if (!strcmp(name, "HEAD")) {
+			const char *head;
+			unsigned char sha1[20];
+			head = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
+			if (!strcmp(ref->refname, head))
+				v->s = "*";
+			else
+				v->s = " ";
+			continue;
+		} else
 			continue;
 
 		formatp = strchr(name, ':');
-- 
1.8.5.rc0.3.gb488857

[PATCH 3/3] for-each-ref: introduce %(upstream:track[short])

[Ramkumar Ramachandra]

Introduce %(upstream:track) to display "[ahead M, behind N]" and
%(upstream:trackshort) to display "=", ">", "<", or "<>"
appropriately (inspired by contrib/completion/git-prompt.sh).

Now you can use the following format in for-each-ref:

  %C(green)%(refname:short)%C(reset)%(upstream:trackshort)

to display refs with terse tracking information.

Note that :track and :trackshort only work with "upstream", and error
out when used with anything else.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-for-each-ref.txt |  6 +++++-
 builtin/for-each-ref.c             | 39 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index bb9c4c1..3ef6aa8 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -93,7 +93,11 @@ objectname::
 upstream::
 	The name of a local ref which can be considered ``upstream''
 	from the displayed ref. Respects `:short` in the same way as
-	`refname` above.
+	`refname` above.  Additionally respects `:track` to show
+	"[ahead N, behind M]" and `:trackshort` to show the terse
+	version (like the prompt) ">", "<", "<>", or "=".  Has no
+	effect if the ref does not have tracking information
+	associated with it.
 
 HEAD::
 	Used to indicate the currently checked out branch.  Is '*' if
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index b841545..7d5c174 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -648,6 +648,7 @@ static void populate_value(struct refinfo *ref)
 		int deref = 0;
 		const char *refname;
 		const char *formatp;
+		struct branch *branch;
 
 		if (*name == '*') {
 			deref = 1;
@@ -659,7 +660,6 @@ static void populate_value(struct refinfo *ref)
 		else if (!prefixcmp(name, "symref"))
 			refname = ref->symref ? ref->symref : "";
 		else if (!prefixcmp(name, "upstream")) {
-			struct branch *branch;
 			/* only local branches may have an upstream */
 			if (prefixcmp(ref->refname, "refs/heads/"))
 				continue;
@@ -686,6 +686,7 @@ static void populate_value(struct refinfo *ref)
 		} else if (!strcmp(name, "HEAD")) {
 			const char *head;
 			unsigned char sha1[20];
+
 			head = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
 			if (!strcmp(ref->refname, head))
 				v->s = "*";
@@ -698,11 +699,45 @@ static void populate_value(struct refinfo *ref)
 		formatp = strchr(name, ':');
 		/* look for "short" refname format */
 		if (formatp) {
+			int num_ours, num_theirs;
+
 			formatp++;
 			if (!strcmp(formatp, "short"))
 				refname = shorten_unambiguous_ref(refname,
 						      warn_ambiguous_refs);
-			else
+			else if (!strcmp(formatp, "track") &&
+				!prefixcmp(name, "upstream")) {
+				char buf[40];
+
+				stat_tracking_info(branch, &num_ours, &num_theirs);
+				if (!num_ours && !num_theirs)
+					v->s = "";
+				else if (!num_ours) {
+					sprintf(buf, "[behind %d]", num_theirs);
+					v->s = xstrdup(buf);
+				} else if (!num_theirs) {
+					sprintf(buf, "[ahead %d]", num_ours);
+					v->s = xstrdup(buf);
+				} else {
+					sprintf(buf, "[ahead %d, behind %d]",
+						num_ours, num_theirs);
+					v->s = xstrdup(buf);
+				}
+				continue;
+			} else if (!strcmp(formatp, "trackshort") &&
+				!prefixcmp(name, "upstream")) {
+
+				stat_tracking_info(branch, &num_ours, &num_theirs);
+				if (!num_ours && !num_theirs)
+					v->s = "=";
+				else if (!num_ours)
+					v->s = "<";
+				else if (!num_theirs)
+					v->s = ">";
+				else
+					v->s = "<>";
+				continue;
+			} else
 				die("unknown %.*s format %s",
 				    (int)(formatp - name), name, formatp);
 		}
-- 
1.8.5.rc0.3.gb488857

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Enhance 'git for-each-ref' with color formatting options.  You can now
> use the following format in for-each-ref:
>
>   %C(green)%(refname:short)%C(reset)

So far, every magic for-each-ref takes were of form %(...); was
there a reason why this had to be %C(...), not %(color=blah), or
something more in-line with the existing other magic?

>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  Documentation/git-for-each-ref.txt |  4 +++-
>  builtin/for-each-ref.c             | 23 +++++++++++++++++++----
>  2 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index f2e08d1..6fa4464 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -45,7 +45,9 @@ OPTIONS
>  	It also interpolates `%%` to `%`, and `%xx` where `xx`
>  	are hex digits interpolates to character with hex code
>  	`xx`; for example `%00` interpolates to `\0` (NUL),
> -	`%09` to `\t` (TAB) and `%0a` to `\n` (LF).
> +	`%09` to `\t` (TAB) and `%0a` to `\n` (LF). Additionally,
> +	colors can be specified using `%C(...)`, with names
> +	described in color.branch.*.
>  
>  <pattern>...::
>  	If one or more patterns are given, only refs are shown that
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 1d4083c..6da2903 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -9,6 +9,7 @@
>  #include "quote.h"
>  #include "parse-options.h"
>  #include "remote.h"
> +#include "color.h"
>  
>  /* Quoting styles */
>  #define QUOTE_NONE 0
> @@ -155,10 +156,13 @@ static const char *find_next(const char *cp)
>  	while (*cp) {
>  		if (*cp == '%') {
>  			/*
> +			 * %C( is the start of a color;
>  			 * %( is the start of an atom;
>  			 * %% is a quoted per-cent.
>  			 */
> -			if (cp[1] == '(')
> +			if (cp[1] == 'C' && cp[2] == '(')
> +				return cp;
> +			else if (cp[1] == '(')
>  				return cp;
>  			else if (cp[1] == '%')
>  				cp++; /* skip over two % */
> @@ -180,8 +184,11 @@ static int verify_format(const char *format)
>  		const char *ep = strchr(sp, ')');
>  		if (!ep)
>  			return error("malformed format string %s", sp);
> -		/* sp points at "%(" and ep points at the closing ")" */
> -		parse_atom(sp + 2, ep);
> +		/* Ignore color specifications: %C(
> +		 * sp points at "%(" and ep points at the closing ")"
> +		 */
> +		if (prefixcmp(sp, "%C("))
> +			parse_atom(sp + 2, ep);
>  		cp = ep + 1;
>  	}
>  	return 0;
> @@ -933,12 +940,20 @@ static void emit(const char *cp, const char *ep)
>  static void show_ref(struct refinfo *info, const char *format, int quote_style)
>  {
>  	const char *cp, *sp, *ep;
> +	char color[COLOR_MAXLEN] = "";
>  
>  	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>  		ep = strchr(sp, ')');
>  		if (cp < sp)
>  			emit(cp, sp);
> -		print_value(info, parse_atom(sp + 2, ep), quote_style);
> +
> +		/* Do we have a color specification? */
> +		if (!prefixcmp(sp, "%C("))
> +			color_parse_mem(sp + 3, ep - sp - 3, "--format", color);
> +		else {
> +			printf("%s", color);
> +			print_value(info, parse_atom(sp + 2, ep), quote_style);
> +		}
>  	}
>  	if (*cp) {
>  		sp = cp + strlen(cp);

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> Enhance 'git for-each-ref' with color formatting options.  You can now
>> use the following format in for-each-ref:
>>
>>   %C(green)%(refname:short)%C(reset)
>
> So far, every magic for-each-ref takes were of form %(...); was
> there a reason why this had to be %C(...), not %(color=blah), or
> something more in-line with the existing other magic?

It is in-line with the color specification in pretty-formats. Would
you strongly prefer something else?

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>>
>>> Enhance 'git for-each-ref' with color formatting options.  You can now
>>> use the following format in for-each-ref:
>>>
>>>   %C(green)%(refname:short)%C(reset)
>>
>> So far, every magic for-each-ref takes were of form %(...); was
>> there a reason why this had to be %C(...), not %(color=blah), or
>> something more in-line with the existing other magic?
>
> It is in-line with the color specification in pretty-formats. Would
> you strongly prefer something else?

This patch is about for-each-ref and your series does not seem to
aim to unify it in any way with pretty-formats, so I would have
expected an enhancement in line with the former, not the latter.

Re: [PATCH 3/3] for-each-ref: introduce %(upstream:track[short])

[Junio C Hamano]

This patch needed on top of 3/3 for me to pass gcc cleanly.

-- >8 --
Subject: [PATCH] fixup! for-each-ref: introduce %(upstream:track[short])

The condition !prefixcmp(name, "upstream") must be true for the
variable "branch" to be reused, so the variable should be always set
when it gets used, but GCC does not seem to realize this fact.
---
 builtin/for-each-ref.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 7d5c174..871d86c 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -648,7 +648,7 @@ static void populate_value(struct refinfo *ref)
 		int deref = 0;
 		const char *refname;
 		const char *formatp;
-		struct branch *branch;
+		struct branch *branch = NULL;
 
 		if (*name == '*') {
 			deref = 1;
@@ -727,6 +727,7 @@ static void populate_value(struct refinfo *ref)
 			} else if (!strcmp(formatp, "trackshort") &&
 				!prefixcmp(name, "upstream")) {
 
+				assert(branch != NULL);
 				stat_tracking_info(branch, &num_ours, &num_theirs);
 				if (!num_ours && !num_theirs)
 					v->s = "=";
-- 
1.8.5-rc0-205-g5b7460b

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> This patch is about for-each-ref and your series does not seem to
> aim to unify it in any way with pretty-formats, so I would have
> expected an enhancement in line with the former, not the latter.

While I might never attempt a unification again, there's no harm in
getting the formats to resemble each other in part; it's likely that
users of f-e-r format will be familiar with pretty-formats.

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> This patch is about for-each-ref and your series does not seem to
>> aim to unify it in any way with pretty-formats, so I would have
>> expected an enhancement in line with the former, not the latter.
>
> While I might never attempt a unification again, there's no harm in
> getting the formats to resemble each other in part.

Yes, but...

> it's likely that
> users of f-e-r format will be familiar with pretty-formats.

... users of for-each-ref format will be _more_ familiar with
formats used by for-each-ref, and it would make a lot more sense to
keep the syntactic resemblance between existing features to show
magic things in for-each-ref and the new feature to show color
(which is merely one new "magic" to the vocabulary in the context of
for-each-ref), no?

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> ... users of for-each-ref format will be _more_ familiar with
> formats used by for-each-ref, and it would make a lot more sense to
> keep the syntactic resemblance between existing features to show
> magic things in for-each-ref and the new feature to show color
> (which is merely one new "magic" to the vocabulary in the context of
> for-each-ref), no?

Okay, so what do you suggest in place of %C(...)?

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> ... users of for-each-ref format will be _more_ familiar with
>> formats used by for-each-ref, and it would make a lot more sense to
>> keep the syntactic resemblance between existing features to show
>> magic things in for-each-ref and the new feature to show color
>> (which is merely one new "magic" to the vocabulary in the context of
>> for-each-ref), no?
>
> Okay, so what do you suggest in place of %C(...)?

If %(authordate) is "I want to see the author date here", and
%(authordate:short) is "I want to see the author date here in the
short form", you would expect "I want colored output in green" to be
spelled as %(color:green), or something, perhaps?

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> If %(authordate) is "I want to see the author date here", and
> %(authordate:short) is "I want to see the author date here in the
> short form", you would expect "I want colored output in green" to be
> spelled as %(color:green), or something, perhaps?

Last time, we almost managed a unification: tokens like %authordate in
f-e-r correspond to tokens like %ae in pretty-formats; %C(...) is
different in that it doesn't actually output anything, but changes the
color of tokens following it. While I'm not opposed to %(color:...), I
would prefer a color syntax that is different from other-token syntax,
like in pretty-formats.

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> ... %C(...) is
> different in that it doesn't actually output anything, but changes the
> color of tokens following it. While I'm not opposed to %(color:...), I
> would prefer a color syntax that is different from other-token syntax,
> like in pretty-formats.

You may prefer it, but I do not see why users prefer to memorize
that a magic that consumes no display output columns uses a syntax
different from all the other magic introducers that follows %(name
of the magic with string after colon to give more specifics to the
magic) syntax.

In all honesty, the %XY mnemonic syntax in pretty-format is a
syntactic disaster.  It is perfectly OK to have a set of often used
shorthand, but because we started without a consistent long-hand, we
ended up with %Cred and %C(yellow), leading us to a nonsense like
this (try it yourself and weep):

    $ git show -s --format='%CredAnd%CyellowAreNotTheSameColor'

It would have been much saner if we started from %(color:yellow),
%(subject), etc., i.e. have a single long-hand magic introducer
%(...), and added a set of often-used short-hands like %s.

I am not opposed to unify the internal implementations and the
external interfaces of pretty, for-each-ref and friends, but
modelling the external UI after the "mnemonic only with ad hoc
additions" mess the pretty-format uses is a huge mistake.

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Ramkumar Ramachandra]

Junio C Hamano <gitster@pobox.com> wrote:
>     $ git show -s --format='%CredAnd%CyellowAreNotTheSameColor'

Ouch, this is quite a disaster.

> It would have been much saner if we started from %(color:yellow),
> %(subject), etc., i.e. have a single long-hand magic introducer
> %(...), and added a set of often-used short-hands like %s.
>
> I am not opposed to unify the internal implementations and the
> external interfaces of pretty, for-each-ref and friends, but
> modelling the external UI after the "mnemonic only with ad hoc
> additions" mess the pretty-format uses is a huge mistake.

Okay, I'm convinced; I'll rework the series to do %(color:...) and
resubmit shortly.

Thanks.

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Phil Hord]

On Fri, Sep 27, 2013 at 8:10 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Enhance 'git for-each-ref' with color formatting options.  You can now
> use the following format in for-each-ref:
>
>   %C(green)%(refname:short)%C(reset)
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  Documentation/git-for-each-ref.txt |  4 +++-
>  builtin/for-each-ref.c             | 23 +++++++++++++++++++----
>  2 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index f2e08d1..078a116 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -45,7 +45,9 @@ OPTIONS
>         It also interpolates `%%` to `%`, and `%xx` where `xx`
>         are hex digits interpolates to character with hex code
>         `xx`; for example `%00` interpolates to `\0` (NUL),
> -       `%09` to `\t` (TAB) and `%0a` to `\n` (LF).
> +       `%09` to `\t` (TAB) and `%0a` to `\n` (LF). Additionally,
> +       colors can be specified using `%C(colorname)`. Use
> +       `%C(reset)` to reset the color.

Reduce the color explanation here and refer to the config page.
Something like pretty-formats does:

    '%C(...)': color specification, as described in color.branch.*
config option;

>
>  <pattern>...::
>         If one or more patterns are given, only refs are shown that
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 1d4083c..a1ca186 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -9,6 +9,7 @@
>  #include "quote.h"
>  #include "parse-options.h"
>  #include "remote.h"
> +#include "color.h"
>
>  /* Quoting styles */
>  #define QUOTE_NONE 0
> @@ -155,10 +156,13 @@ static const char *find_next(const char *cp)
>         while (*cp) {
>                 if (*cp == '%') {
>                         /*
> +                        * %C( is the start of a color;
>                          * %( is the start of an atom;
>                          * %% is a quoted per-cent.
>                          */
> -                       if (cp[1] == '(')
> +                       if (cp[1] == 'C' && cp[2] == '(')
> +                               return cp;
> +                       else if (cp[1] == '(')
>                                 return cp;
>                         else if (cp[1] == '%')
>                                 cp++; /* skip over two % */
> @@ -180,8 +184,11 @@ static int verify_format(const char *format)
>                 const char *ep = strchr(sp, ')');
>                 if (!ep)
>                         return error("malformed format string %s", sp);
> -               /* sp points at "%(" and ep points at the closing ")" */
> -               parse_atom(sp + 2, ep);
> +               /* Ignore color specifications: %C(
> +                * sp points at "%(" and ep points at the closing ")"
> +                */
> +               if (prefixcmp(sp, "%C("))
> +                       parse_atom(sp + 2, ep);
>                 cp = ep + 1;
>         }
>         return 0;
> @@ -933,12 +940,20 @@ static void emit(const char *cp, const char *ep)
>  static void show_ref(struct refinfo *info, const char *format, int quote_style)
>  {
>         const char *cp, *sp, *ep;
> +       char color[COLOR_MAXLEN];
>
>         for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>                 ep = strchr(sp, ')');
>                 if (cp < sp)
>                         emit(cp, sp);
> -               print_value(info, parse_atom(sp + 2, ep), quote_style);
> +
> +               /* Do we have a color specification? */
> +               if (!prefixcmp(sp, "%C("))
> +                       color_parse_mem(sp + 3, ep - sp - 3, "--format", color);
> +               else {
> +                       printf("%s", color);

'color' used uninitialized here?

> +                       print_value(info, parse_atom(sp + 2, ep), quote_style);
> +               }
>         }
>         if (*cp) {
>                 sp = cp + strlen(cp);
> --
> 1.8.4.478.g55109e3
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] for-each-ref: introduce %C(...) for color

[Ramkumar Ramachandra]

Enhance 'git for-each-ref' with color formatting options.  You can now
use the following format in for-each-ref:

  %C(green)%(refname:short)%C(reset)

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-for-each-ref.txt |  4 +++-
 builtin/for-each-ref.c             | 23 +++++++++++++++++++----
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index f2e08d1..078a116 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -45,7 +45,9 @@ OPTIONS
 	It also interpolates `%%` to `%`, and `%xx` where `xx`
 	are hex digits interpolates to character with hex code
 	`xx`; for example `%00` interpolates to `\0` (NUL),
-	`%09` to `\t` (TAB) and `%0a` to `\n` (LF).
+	`%09` to `\t` (TAB) and `%0a` to `\n` (LF). Additionally,
+	colors can be specified using `%C(colorname)`. Use
+	`%C(reset)` to reset the color.
 
 <pattern>...::
 	If one or more patterns are given, only refs are shown that
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 1d4083c..a1ca186 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -9,6 +9,7 @@
 #include "quote.h"
 #include "parse-options.h"
 #include "remote.h"
+#include "color.h"
 
 /* Quoting styles */
 #define QUOTE_NONE 0
@@ -155,10 +156,13 @@ static const char *find_next(const char *cp)
 	while (*cp) {
 		if (*cp == '%') {
 			/*
+			 * %C( is the start of a color;
 			 * %( is the start of an atom;
 			 * %% is a quoted per-cent.
 			 */
-			if (cp[1] == '(')
+			if (cp[1] == 'C' && cp[2] == '(')
+				return cp;
+			else if (cp[1] == '(')
 				return cp;
 			else if (cp[1] == '%')
 				cp++; /* skip over two % */
@@ -180,8 +184,11 @@ static int verify_format(const char *format)
 		const char *ep = strchr(sp, ')');
 		if (!ep)
 			return error("malformed format string %s", sp);
-		/* sp points at "%(" and ep points at the closing ")" */
-		parse_atom(sp + 2, ep);
+		/* Ignore color specifications: %C(
+		 * sp points at "%(" and ep points at the closing ")"
+		 */
+		if (prefixcmp(sp, "%C("))
+			parse_atom(sp + 2, ep);
 		cp = ep + 1;
 	}
 	return 0;
@@ -933,12 +940,20 @@ static void emit(const char *cp, const char *ep)
 static void show_ref(struct refinfo *info, const char *format, int quote_style)
 {
 	const char *cp, *sp, *ep;
+	char color[COLOR_MAXLEN];
 
 	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
 		ep = strchr(sp, ')');
 		if (cp < sp)
 			emit(cp, sp);
-		print_value(info, parse_atom(sp + 2, ep), quote_style);
+
+		/* Do we have a color specification? */
+		if (!prefixcmp(sp, "%C("))
+			color_parse_mem(sp + 3, ep - sp - 3, "--format", color);
+		else {
+			printf("%s", color);
+			print_value(info, parse_atom(sp + 2, ep), quote_style);
+		}
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);
-- 
1.8.4.478.g55109e3

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Ramkumar Ramachandra]

John Keeping wrote:
> Section 6.7.9 of the C11 standard says:
>
>     If an object that has automatic storage duration is not initialized
>     explicitly, its value is indeterminate.

Ah, thanks.  I'll initialize it to an empty string.

>> More importantly, aren't there numerous instances of this in the
>> codebase?
>
> Care to point at one?  I had a quick look and all places I inspected are
> either static or write to the array before reading it.

I'll run some Valgrind tests to see what it yields.

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Antoine Pelisse]

On Sat, May 25, 2013 at 1:50 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Antoine Pelisse wrote:
>> Is it not possible for "color" to be used uninitialized here ?
>
> My compiler didn't complain; what am I missing?  Doesn't the
> declaration char color[COLOR_MAXLEN]; initialize an empty string?

As John said, this is allocated on the stack.
What do you want it to be initialized to ?
Filled with zeros ? That's overkill because only the first char needs
to be zeroed.
The compiler can't know what to do and it lets you to take the best action.

> More importantly, aren't there numerous instances of this in the
> codebase?

I think Valgrind would be mad at us if there was many instances of
this. By the way Ramkumar, could you check if Valgrind complains with
your patch ?

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[John Keeping]

On Sat, May 25, 2013 at 05:20:29PM +0530, Ramkumar Ramachandra wrote:
> Antoine Pelisse wrote:
> > Is it not possible for "color" to be used uninitialized here ?
> 
> My compiler didn't complain; what am I missing?  Doesn't the
> declaration char color[COLOR_MAXLEN]; initialize an empty string?

Why would it?  The variable's begin allocated on the stack and the C
standard only zero-initializes variables with static storage duration;
Section 6.7.9 of the C11 standard says:

    If an object that has automatic storage duration is not initialized
    explicitly, its value is indeterminate.


I suspect the compiler doesn't complain because there is a path through
the function that initializes color before reading it (if we hit the
"if" branch in the loop before the "else" branch) and the compile
assumes that there is something in the function's contract that
guarantees that we follow this path.  But I don't think that's correct
so you do need to initialize color to the empty string.

> More importantly, aren't there numerous instances of this in the
> codebase?

Care to point at one?  I had a quick look and all places I inspected are
either static or write to the array before reading it.

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Ramkumar Ramachandra]

David Aguilar wrote:
> Can you please also update Documentation/?

Yeah, will do in the re-roll.  Duy is bringing in pretty-formats.
We'll probably need a separate document called pretty-ref-formats or
some such thing.

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Ramkumar Ramachandra]

Antoine Pelisse wrote:
> Is it not possible for "color" to be used uninitialized here ?

My compiler didn't complain; what am I missing?  Doesn't the
declaration char color[COLOR_MAXLEN]; initialize an empty string?
More importantly, aren't there numerous instances of this in the
codebase?

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Eric Sunshine]

On Fri, May 24, 2013 at 10:19 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Since 'git branch' misses important options like --sort, --count, and
> --format that are present in 'git for-each-ref'.  Until we are in a
> position to fix 'git branch', let us enhance the 'git for-each-ref'
> format so it can atleast colorize output.

s/atleast/at least/

> You can use the following format in for-each-ref:
>
>   %C(green)%(refname:short)%C(reset)
>
> To display refs in green.  Future patches will attempt to extend the
> format even more to get useful output.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[David Aguilar]

On Fri, May 24, 2013 at 7:19 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Since 'git branch' misses important options like --sort, --count, and
> --format that are present in 'git for-each-ref'.  Until we are in a
> position to fix 'git branch', let us enhance the 'git for-each-ref'
> format so it can atleast colorize output.
>
> You can use the following format in for-each-ref:
>
>   %C(green)%(refname:short)%C(reset)
>
> To display refs in green.  Future patches will attempt to extend the
> format even more to get useful output.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  builtin/for-each-ref.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)

Can you please also update Documentation/?
--
David

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Antoine Pelisse]

On Fri, May 24, 2013 at 4:19 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> @@ -928,12 +936,22 @@ static void emit(const char *cp, const char *ep)
>  static void show_ref(struct refinfo *info, const char *format, int quote_style)
>  {
>         const char *cp, *sp, *ep;
> +       char color[COLOR_MAXLEN];
>
>         for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>                 ep = strchr(sp, ')');
>                 if (cp < sp)
>                         emit(cp, sp);
> -               print_value(info, parse_atom(sp + 2, ep), quote_style);
> +
> +               /* Do we have a color specification? */
> +               if (!prefixcmp(sp, "%C("))
> +                       color_parse_mem(sp + 3,
> +                                       ep - sp - 3,
> +                                       "--format ", color);
> +               else {
> +                       printf("%s", color);

Is it not possible for "color" to be used uninitialized here ?

> +                       print_value(info, parse_atom(sp + 2, ep), quote_style);
> +               }
>         }
>         if (*cp) {
>                 sp = cp + strlen(cp);

[PATCH 1/3] for-each-ref: introduce %C(...) for color

[Ramkumar Ramachandra]

Since 'git branch' misses important options like --sort, --count, and
--format that are present in 'git for-each-ref'.  Until we are in a
position to fix 'git branch', let us enhance the 'git for-each-ref'
format so it can atleast colorize output.

You can use the following format in for-each-ref:

  %C(green)%(refname:short)%C(reset)

To display refs in green.  Future patches will attempt to extend the
format even more to get useful output.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/for-each-ref.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 7f059c3..1563b25 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -9,6 +9,7 @@
 #include "quote.h"
 #include "parse-options.h"
 #include "remote.h"
+#include "color.h"
 
 /* Quoting styles */
 #define QUOTE_NONE 0
@@ -155,10 +156,13 @@ static const char *find_next(const char *cp)
 	while (*cp) {
 		if (*cp == '%') {
 			/*
+			 * %C( is the start of a color;
 			 * %( is the start of an atom;
 			 * %% is a quoted per-cent.
 			 */
-			if (cp[1] == '(')
+			if (cp[1] == 'C' && cp[2] == '(')
+				return cp;
+			else if (cp[1] == '(')
 				return cp;
 			else if (cp[1] == '%')
 				cp++; /* skip over two % */
@@ -180,8 +184,12 @@ static int verify_format(const char *format)
 		const char *ep = strchr(sp, ')');
 		if (!ep)
 			return error("malformed format string %s", sp);
-		/* sp points at "%(" and ep points at the closing ")" */
-		parse_atom(sp + 2, ep);
+		/*
+		 * Ignore color specifications: %C(
+		 * sp points at "%(" and ep points at the closing ")"
+		 */
+		if (prefixcmp(sp, "%C("))
+			parse_atom(sp + 2, ep);
 		cp = ep + 1;
 	}
 	return 0;
@@ -928,12 +936,22 @@ static void emit(const char *cp, const char *ep)
 static void show_ref(struct refinfo *info, const char *format, int quote_style)
 {
 	const char *cp, *sp, *ep;
+	char color[COLOR_MAXLEN];
 
 	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
 		ep = strchr(sp, ')');
 		if (cp < sp)
 			emit(cp, sp);
-		print_value(info, parse_atom(sp + 2, ep), quote_style);
+
+		/* Do we have a color specification? */
+		if (!prefixcmp(sp, "%C("))
+			color_parse_mem(sp + 3,
+					ep - sp - 3,
+					"--format ", color);
+		else {
+			printf("%s", color);
+			print_value(info, parse_atom(sp + 2, ep), quote_style);
+		}
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);
-- 
1.8.3.rc3.2.g99b8f3f.dirty