[PATCH v2 0/3] Minor f-e-r enhacements

[PATCH v2 0/3] Minor f-e-r enhacements

[Ramkumar Ramachandra]

Hi,

v2 uses the more sane %(color:...) as opposed to %C(...) for changing
the color of the output, as suggested by Junio. Everything else is the
same.

Thanks.

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

 Documentation/git-for-each-ref.txt | 14 +++++++-
 builtin/for-each-ref.c             | 66 ++++++++++++++++++++++++++++++++++----
 2 files changed, 72 insertions(+), 8 deletions(-)

-- 
1.8.5.rc0.3.g914176d.dirty

[PATCH v2 1/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:

  %(HEAD) %(refname:short)

to display an 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 f2e08d1..ab3da0e 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -93,6 +93,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 1d4083c..5f1842f 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -75,6 +75,7 @@ static struct {
 	{ "upstream" },
 	{ "symref" },
 	{ "flag" },
+	{ "HEAD" },
 };
 
 /*
@@ -675,8 +676,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.g914176d.dirty

[PATCH v2 2/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:

  %(refname:short)%(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             | 40 +++++++++++++++++++++++++++++++++++---
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index ab3da0e..c9b192e 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -91,7 +91,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 5f1842f..ed81407 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -641,6 +641,7 @@ static void populate_value(struct refinfo *ref)
 		int deref = 0;
 		const char *refname;
 		const char *formatp;
+		struct branch *branch;
 
 		if (*name == '*') {
 			deref = 1;
@@ -652,7 +653,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;
@@ -679,6 +679,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 = "*";
@@ -689,13 +690,46 @@ static void populate_value(struct refinfo *ref)
 			continue;
 
 		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.g914176d.dirty

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

[Junio C Hamano]

You need to squash 78465bb, which has been queued on the previous
round, in to this step.  There also is a similar issue introduced by
the updated 3/3:

builtin/for-each-ref.c: In function 'populate_value':
builtin/for-each-ref.c:701:13: error: 'refname' may be used uninitialized in this function [-Werror=uninitialized]


    700                         if (!strcmp(formatp, "short"))
    701                                 refname = shorten_unambiguous_ref(refname,
    702                                                       warn_ambiguous_refs);

Thanks.

-- >8 --
Date: Thu, 31 Oct 2013 14:17:36 -0700
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 ed81407..67168a1 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -641,7 +641,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;
@@ -719,6 +719,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-rc1-322-g9c05f5a

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> You need to squash 78465bb,...

oops; make it 2249926. 784 is a cherry-pick of it on top of this round.

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> builtin/for-each-ref.c: In function 'populate_value':
> builtin/for-each-ref.c:701:13: error: 'refname' may be used uninitialized in this function [-Werror=uninitialized]

In my defense, the gcc on my end (4.8.2) doesn't seem to complain. Are
you using extra cflags?

However, I do get the following compile warnings:

wt-status.c: In function ‘wt_status_print_unmerged_header’:
wt-status.c:187:2: warning: zero-length gnu_printf format string
[-Wformat-zero-length]
  status_printf_ln(s, c, "");
  ^
wt-status.c: In function ‘wt_status_print_cached_header’:
wt-status.c:203:2: warning: zero-length gnu_printf format string
[-Wformat-zero-length]
  status_printf_ln(s, c, "");
  ^
wt-status.c: In function ‘wt_status_print_dirty_header’:
wt-status.c:222:2: warning: zero-length gnu_printf format string
[-Wformat-zero-length]
  status_printf_ln(s, c, "");
  ^
wt-status.c: In function ‘wt_status_print_other_header’:
wt-status.c:234:2: warning: zero-length gnu_printf format string
[-Wformat-zero-length]
  status_printf_ln(s, c, "");
  ^
wt-status.c: In function ‘wt_status_print_trailer’:
wt-status.c:239:2: warning: zero-length gnu_printf format string
[-Wformat-zero-length]
  status_printf_ln(s, color(WT_STATUS_HEADER, s), "");
  ^
wt-status.c: In function ‘wt_status_print_other’:
wt-status.c:767:2: warning: zero-length gnu_printf format string
[-Wformat-zero-length]
  status_printf_ln(s, GIT_COLOR_NORMAL, "");
  ^
wt-status.c: In function ‘wt_status_print_tracking’:
wt-status.c:827:3: warning: zero-length gnu_printf format string
[-Wformat-zero-length]
   fprintf_ln(s->fp, "");
   ^
wt-status.c: In function ‘wt_status_print’:
wt-status.c:1243:3: warning: zero-length gnu_printf format string
[-Wformat-zero-length]
   status_printf(s, color(WT_STATUS_HEADER, s), "");
   ^
wt-status.c:1256:3: warning: zero-length gnu_printf format string
[-Wformat-zero-length]
   status_printf_ln(s, color(WT_STATUS_HEADER, s), "");
   ^
wt-status.c:1258:3: warning: zero-length gnu_printf format string
[-Wformat-zero-length]
   status_printf_ln(s, color(WT_STATUS_HEADER, s), "");
   ^
wt-status.c:1275:4: warning: zero-length gnu_printf format string
[-Wformat-zero-length]
    status_printf_ln(s, GIT_COLOR_NORMAL, "");
    ^
builtin/commit.c: In function ‘prepare_to_commit’:
builtin/commit.c:808:4: warning: zero-length gnu_printf format string
[-Wformat-zero-length]
    status_printf_ln(s, GIT_COLOR_NORMAL, "");
    ^

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

[Ramkumar Ramachandra]

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

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

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 c9b192e..2f3ac22 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -101,6 +101,10 @@ HEAD::
 	Used to indicate the currently checked out branch.  Is '*' if
 	HEAD points to the current ref, and ' ' otherwise.
 
+color::
+	Used to change color of output.  Followed by `:<colorname>`,
+	where names are described in `color.branch.*`.
+
 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 ed81407..c59fffe 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
@@ -76,6 +77,7 @@ static struct {
 	{ "symref" },
 	{ "flag" },
 	{ "HEAD" },
+	{ "color" },
 };
 
 /*
@@ -662,8 +664,9 @@ static void populate_value(struct refinfo *ref)
 			    !branch->merge[0]->dst)
 				continue;
 			refname = branch->merge[0]->dst;
-		}
-		else if (!strcmp(name, "flag")) {
+		} else if (!prefixcmp(name, "color")) {
+			;
+		} else if (!strcmp(name, "flag")) {
 			char buf[256], *cp = buf;
 			if (ref->flag & REF_ISSYMREF)
 				cp = copy_advance(cp, ",symref");
@@ -729,6 +732,12 @@ static void populate_value(struct refinfo *ref)
 				else
 					v->s = "<>";
 				continue;
+			} else if (!prefixcmp(name, "color")) {
+				char color[COLOR_MAXLEN] = "";
+
+				color_parse(formatp, "--format", color);
+				v->s = xstrdup(color);
+				continue;
 			} else
 				die("unknown %.*s format %s",
 				    (int)(formatp - name), name, formatp);
-- 
1.8.5.rc0.3.g914176d.dirty

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> +		} else if (!prefixcmp(name, "color")) {
> +			;

How is "%(color:short)" parsed with this code?

This part says, "Yeah, I see something starting with color", and
then later strchr(name, ':') will point formatp to "short".

Luckily, "%(colorgarbage:short)" does not even come this far because
parse_atom() would not have allowed the codeflow to, but comparing
with "color:" here may be a lot more defensive and safe, I think.

And find the color-value here, stuffing v->s inside this "else if",
continue, without letting the formatp part work on refname this
piece of code does not even set.  Just like how we handle "flag"
without falling thru to the formatp code.


> +		} else if (!strcmp(name, "flag")) {
>  			char buf[256], *cp = buf;
>  			if (ref->flag & REF_ISSYMREF)
>  				cp = copy_advance(cp, ",symref");
> @@ -729,6 +732,12 @@ static void populate_value(struct refinfo *ref)
>  				else
>  					v->s = "<>";
>  				continue;
> +			} else if (!prefixcmp(name, "color")) {
> +				char color[COLOR_MAXLEN] = "";
> +
> +				color_parse(formatp, "--format", color);
> +				v->s = xstrdup(color);
> +				continue;
>  			} else
>  				die("unknown %.*s format %s",
>  				    (int)(formatp - name), name, formatp);

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> +		} else if (!prefixcmp(name, "color")) {
>> +			;
>
> How is "%(color:short)" parsed with this code?
>
> This part says, "Yeah, I see something starting with color", and
> then later strchr(name, ':') will point formatp to "short".
>
> Luckily, "%(colorgarbage:short)" does not even come this far because
> parse_atom() would not have allowed the codeflow to, but comparing
> with "color:" here may be a lot more defensive and safe, I think.
>
> And find the color-value here, stuffing v->s inside this "else if",
> continue, without letting the formatp part work on refname this
> piece of code does not even set.  Just like how we handle "flag"
> without falling thru to the formatp code.

Perhaps like this (obviously not tested as these three patches did
not add any tests ;-)

I also think that there should be a mechanism to do "color:reset"
after each record is issued automatically, and also have the color
output honor --color=auto from the command line, i.e.

	git for-each-ref --color=auto --format='%(color:blue)%(subject)' | cat

should turn the coloring off.

So I think this patch may be a first step in the right direction,
but there are quite a lot more work that is needed before it gets
ready for production use.

Thanks.

 builtin/for-each-ref.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 9e07571..07a9385 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -664,8 +664,12 @@ static void populate_value(struct refinfo *ref)
 			    !branch->merge[0]->dst)
 				continue;
 			refname = branch->merge[0]->dst;
-		} else if (!prefixcmp(name, "color")) {
-			;
+		} else if (!prefixcmp(name, "color:")) {
+			char color[COLOR_MAXLEN] = "";
+
+			color_parse(name + 6, "--format", color);
+			v->s = xstrdup(color);
+			continue;
 		} else if (!strcmp(name, "flag")) {
 			char buf[256], *cp = buf;
 			if (ref->flag & REF_ISSYMREF)
@@ -733,12 +737,6 @@ static void populate_value(struct refinfo *ref)
 				else
 					v->s = "<>";
 				continue;
-			} else if (!prefixcmp(name, "color")) {
-				char color[COLOR_MAXLEN] = "";
-
-				color_parse(formatp, "--format", color);
-				v->s = xstrdup(color);
-				continue;
 			} else
 				die("unknown %.*s format %s",
 				    (int)(formatp - name), name, formatp);

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Perhaps like this (obviously not tested as these three patches did
> not add any tests ;-)

Sorry about that. I didn't notice t6300-for-each-ref.sh. Will fix in
the next round.

> I also think that there should be a mechanism to do "color:reset"
> after each record is issued automatically, and also have the color
> output honor --color=auto from the command line, i.e.
>
>         git for-each-ref --color=auto --format='%(color:blue)%(subject)' | cat
>
> should turn the coloring off.

We can add --color=auto later, but I'm wondering about auto-reset
color after each token. What happens if I do:

  $ git for-each-ref --format='%(subject)%(color:blue)'

No color, right? So, it should be auto-reset color after each token
_and_ at end of format-string.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> Perhaps like this (obviously not tested as these three patches did
>> not add any tests ;-)
>
> Sorry about that. I didn't notice t6300-for-each-ref.sh. Will fix in
> the next round.
>
>> I also think that there should be a mechanism to do "color:reset"
>> after each record is issued automatically, and also have the color
>> output honor --color=auto from the command line, i.e.
>>
>>         git for-each-ref --color=auto --format='%(color:blue)%(subject)' | cat
>>
>> should turn the coloring off.
>
> We can add --color=auto later, but I'm wondering about auto-reset
> color after each token. What happens if I do:
>
>   $ git for-each-ref --format='%(subject)%(color:blue)'
>
> No color, right? So, it should be auto-reset color after each token
> _and_ at end of format-string.

If you are saying, by after each token, that

	--format='%(color:blue)%(A)literal string%(B)'

should result in

	<color blue> <value for A> <color reset> "literal string" <value for B>

then I would disagree.  I was suggesting it to instead produce

	<color blue> <value for A> "literal string" <value for B> <color reset>

where the <color reset> always comes when some color is used and we
hit the end of the format string. A bonus point if we can make it so
that we emit the final reset only when the last "%(color:some)" is
not "%(color:reset)", but unconditional "reset if we ever used
color" is fine.

Thanks.

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> If you are saying, by after each token, that
>
>         --format='%(color:blue)%(A)literal string%(B)'
>
> should result in
>
>         <color blue> <value for A> <color reset> "literal string" <value for B>
>
> then I would disagree.

Hm, I didn't think it was a bad idea to reset after each token. The
whole point of having color is to make sure that two consecutive
tokens don't have the same color, no? Then again, my scheme would
result in extra unnecessary resets like

  %(color:blue)%(A)%(color:green)%(B)

being turned into:

  %(color:blue)%(A)%(color:reset)%(color:green)%(B)%(color:reset)

Here, the first %(color:reset) is completely unnecessary.

> I was suggesting it to instead produce
>
>         <color blue> <value for A> "literal string" <value for B> <color reset>
>
> where the <color reset> always comes when some color is used and we
> hit the end of the format string. A bonus point if we can make it so
> that we emit the final reset only when the last "%(color:some)" is
> not "%(color:reset)", but unconditional "reset if we ever used
> color" is fine.

Okay, a simple don't-leak-color. I'll submit another iteration soon.

Thanks.