[PATCH v4 0/6] Replacement for rr/for-each-ref-decoration

[PATCH v4 0/6] Replacement for rr/for-each-ref-decoration

[Ramkumar Ramachandra]

Hi,

The major change since v3 is in [6/6]: color no more auto-resets after
each token, but instead only to avoid leakage (thanks to Junio). Other
minor changes are in accordance with Eric Sunshine's review.

Thanks.

Ramkumar Ramachandra (6):
  t6300 (for-each-ref): clearly demarcate setup
  t6300 (for-each-ref): don't hardcode SHA-1 hexes
  for-each-ref: introduce %(HEAD) asterisk marker
  for-each-ref: introduce %(upstream:track[short])
  for-each-ref: introduce %(color:...) for color
  for-each-ref: avoid color leakage

 Documentation/git-for-each-ref.txt | 14 +++++-
 builtin/for-each-ref.c             | 93 +++++++++++++++++++++++++++++++++-----
 t/t6300-for-each-ref.sh            | 69 ++++++++++++++++++++++------
 3 files changed, 151 insertions(+), 25 deletions(-)

-- 
1.8.5.rc0.5.g70ebc73.dirty

[PATCH v4 1/6] t6300 (for-each-ref): clearly demarcate setup

[Ramkumar Ramachandra]

Condense the two-step setup into one step, and give it an appropriate
name.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t6300-for-each-ref.sh | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 752f5cb..64301e7 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -18,16 +18,13 @@ setdate_and_increment () {
     export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
 }
 
-test_expect_success 'Create sample commit with known timestamp' '
+test_expect_success setup '
 	setdate_and_increment &&
 	echo "Using $datestamp" > one &&
 	git add one &&
 	git commit -m "Initial" &&
 	setdate_and_increment &&
-	git tag -a -m "Tagging at $datestamp" testtag
-'
-
-test_expect_success 'Create upstream config' '
+	git tag -a -m "Tagging at $datestamp" testtag &&
 	git update-ref refs/remotes/origin/master master &&
 	git remote add origin nowhere &&
 	git config branch.master.remote origin &&
-- 
1.8.5.rc0.5.g70ebc73.dirty

[PATCH v4 2/6] t6300 (for-each-ref): don't hardcode SHA-1 hexes

[Ramkumar Ramachandra]

Use rev-parse in its place, making it easier for future patches to
modify the test script.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t6300-for-each-ref.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 64301e7..675c2a2 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -49,8 +49,8 @@ test_atom head refname refs/heads/master
 test_atom head upstream refs/remotes/origin/master
 test_atom head objecttype commit
 test_atom head objectsize 171
-test_atom head objectname 67a36f10722846e891fbada1ba48ed035de75581
-test_atom head tree 0e51c00fcb93dffc755546f27593d511e1bdb46f
+test_atom head objectname $(git rev-parse refs/heads/master)
+test_atom head tree $(git rev-parse refs/heads/master^{tree})
 test_atom head parent ''
 test_atom head numparent 0
 test_atom head object ''
@@ -82,11 +82,11 @@ test_atom tag refname refs/tags/testtag
 test_atom tag upstream ''
 test_atom tag objecttype tag
 test_atom tag objectsize 154
-test_atom tag objectname 98b46b1d36e5b07909de1b3886224e3e81e87322
+test_atom tag objectname $(git rev-parse refs/tags/testtag)
 test_atom tag tree ''
 test_atom tag parent ''
 test_atom tag numparent ''
-test_atom tag object '67a36f10722846e891fbada1ba48ed035de75581'
+test_atom tag object $(git rev-parse refs/tags/testtag^0)
 test_atom tag type 'commit'
 test_atom tag author ''
 test_atom tag authorname ''
@@ -302,7 +302,7 @@ test_expect_success 'Check short upstream format' '
 '
 
 cat >expected <<EOF
-67a36f1
+$(git rev-parse --short HEAD)
 EOF
 
 test_expect_success 'Check short objectname format' '
@@ -453,9 +453,9 @@ test_atom refs/tags/signed-long contents "subject line
 body contents
 $sig"
 
-cat >expected <<\EOF
-408fe76d02a785a006c2e9c669b7be5589ede96d <committer@example.com> refs/tags/master
-90b5ebede4899eda64893bc2a4c8f1d6fb6dfc40 <committer@example.com> refs/tags/bogo
+cat >expected <<EOF
+$(git rev-parse refs/tags/master) <committer@example.com> refs/tags/master
+$(git rev-parse refs/tags/bogo) <committer@example.com> refs/tags/bogo
 EOF
 
 test_expect_success 'Verify sort with multiple keys' '
-- 
1.8.5.rc0.5.g70ebc73.dirty

[PATCH v4 3/6] 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 +++++++++++--
 t/t6300-for-each-ref.sh            |  2 ++
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index f2e08d1..8f87c9a 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::
+	'*' if HEAD matches current ref (the checked out branch), ' '
+	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, ':');
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 675c2a2..1d998f8 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -77,6 +77,7 @@ test_atom head contents:body ''
 test_atom head contents:signature ''
 test_atom head contents 'Initial
 '
+test_atom head HEAD '*'
 
 test_atom tag refname refs/tags/testtag
 test_atom tag upstream ''
@@ -110,6 +111,7 @@ test_atom tag contents:body ''
 test_atom tag contents:signature ''
 test_atom tag contents 'Tagging at 1151939927
 '
+test_atom tag HEAD ' '
 
 test_expect_success 'Check invalid atoms names are errors' '
 	test_must_fail git for-each-ref --format="%(INVALID)" refs/heads
-- 
1.8.5.rc0.5.g70ebc73.dirty

[PATCH v4 4/6] 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 +++++++++++++++++++++++++++++++++++---
 t/t6300-for-each-ref.sh            | 27 +++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 8f87c9a..92e82fd 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: ">" (ahead), "<" (behind), "<>" (ahead and behind),
+	or "=" (in sync).  Has no effect if the ref does not have
+	tracking information associated with it.
 
 HEAD::
 	'*' if HEAD matches current ref (the checked out branch), ' '
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);
 		}
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 1d998f8..d88d7ac 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -303,6 +303,33 @@ test_expect_success 'Check short upstream format' '
 	test_cmp expected actual
 '
 
+test_expect_success 'setup for upstream:track[short]' '
+	test_commit two
+'
+
+cat >expected <<EOF
+[ahead 1]
+EOF
+
+test_expect_success 'Check upstream:track format' '
+	git for-each-ref --format="%(upstream:track)" refs/heads >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<EOF
+>
+EOF
+
+test_expect_success 'Check upstream:trackshort format' '
+	git for-each-ref --format="%(upstream:trackshort)" refs/heads >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'Check that :track[short] cannot be used with other atoms' '
+	test_must_fail git for-each-ref --format="%(refname:track)" 2>/dev/null &&
+	test_must_fail git for-each-ref --format="%(refname:trackshort)" 2>/dev/null
+'
+
 cat >expected <<EOF
 $(git rev-parse --short HEAD)
 EOF
-- 
1.8.5.rc0.5.g70ebc73.dirty

[PATCH v4 5/6] 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)

where color names are described in color.branch.*.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-for-each-ref.txt |  4 ++++
 builtin/for-each-ref.c             | 11 +++++++++--
 t/t6300-for-each-ref.sh            | 17 +++++++++++++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 92e82fd..94f5c46 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -101,6 +101,10 @@ HEAD::
 	'*' if HEAD matches current ref (the checked out branch), ' '
 	otherwise.
 
+color::
+	Change output color.  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..2ff4e54 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,13 @@ 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:")) {
+			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)
 				cp = copy_advance(cp, ",symref");
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index d88d7ac..69e3155 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -343,6 +343,23 @@ 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)
+EOF
+
+test_expect_success 'Check %(color:...) ' '
+	git for-each-ref --format="%(objectname:short) %(color:green)%(refname:short)%(color:reset)" >actual &&
+	test_cmp expected actual
+'
+
 cat >expected <<\EOF
 heads/master
 tags/master
-- 
1.8.5.rc0.5.g70ebc73.dirty

[PATCH v4 6/6] for-each-ref: avoid color leakage

[Ramkumar Ramachandra]

To make sure that an invocation like the following doesn't leak color,

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

auto-reset at the end of the format string when the last color token
seen in the format string isn't a color-reset.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/for-each-ref.c  | 29 +++++++++++++++++++++++++----
 t/t6300-for-each-ref.sh |  2 +-
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 2ff4e54..04e35ba 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -23,6 +23,7 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 struct atom_value {
 	const char *s;
 	unsigned long ul; /* used for sorting when not FIELD_STR */
+	int color : 2; /* 1 indicates color, 2 indicates color-reset */
 };
 
 struct ref_sort {
@@ -668,6 +669,10 @@ static void populate_value(struct refinfo *ref)
 			char color[COLOR_MAXLEN] = "";
 
 			color_parse(name + 6, "--format", color);
+			if (!strcmp(name + 6, "reset"))
+				v->color = 2;
+			else
+				v->color = 1;
 			v->s = xstrdup(color);
 			continue;
 		} else if (!strcmp(name, "flag")) {
@@ -914,11 +919,9 @@ static void sort_refs(struct ref_sort *sort, struct refinfo **refs, int num_refs
 	qsort(refs, num_refs, sizeof(struct refinfo *), compare_refs);
 }
 
-static void print_value(struct refinfo *ref, int atom, int quote_style)
+static void print_value(struct atom_value *v, int quote_style)
 {
-	struct atom_value *v;
 	struct strbuf sb = STRBUF_INIT;
-	get_value(ref, atom, &v);
 	switch (quote_style) {
 	case QUOTE_NONE:
 		fputs(v->s, stdout);
@@ -983,17 +986,35 @@ 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;
+	struct atom_value *atomv, resetv;
+	int reset_color = 0;
+	char color[COLOR_MAXLEN] = "";
 
+	color_parse("reset", "--format", color);
+	resetv.s = color;
 	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);
+		get_value(info, parse_atom(sp + 2, ep), &atomv);
+		print_value(atomv, quote_style);
+
+		/*
+		 * reset_color is used to avoid color leakage; it
+		 * should be set when the last color atom is not a
+		 * color-reset.
+		 */
+		if (atomv->color == 1)
+			reset_color = 1;
+		else if (atomv->color == 2)
+			reset_color = 0;
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);
 		emit(cp, sp);
 	}
+	if (reset_color)
+		print_value(&resetv, quote_style);
 	putchar('\n');
 }
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 69e3155..46866ba 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -356,7 +356,7 @@ $(git rev-parse --short refs/tags/two) $(get_color green)two$(get_color reset)
 EOF
 
 test_expect_success 'Check %(color:...) ' '
-	git for-each-ref --format="%(objectname:short) %(color:green)%(refname:short)%(color:reset)" >actual &&
+	git for-each-ref --format="%(objectname:short) %(color:green)%(refname:short)" >actual &&
 	test_cmp expected actual
 '
 
-- 
1.8.5.rc0.5.g70ebc73.dirty

Re: [PATCH v4 6/6] for-each-ref: avoid color leakage

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> To make sure that an invocation like the following doesn't leak color,
>
>   $ git for-each-ref --format='%(subject)%(color:green)'
>
> auto-reset at the end of the format string when the last color token
> seen in the format string isn't a color-reset.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  builtin/for-each-ref.c  | 29 +++++++++++++++++++++++++----
>  t/t6300-for-each-ref.sh |  2 +-
>  2 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 2ff4e54..04e35ba 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -23,6 +23,7 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>  struct atom_value {
>  	const char *s;
>  	unsigned long ul; /* used for sorting when not FIELD_STR */
> +	int color : 2; /* 1 indicates color, 2 indicates color-reset */
>  };

Hmph.  It looks wasteful to have this information in atom_value.

Isn't a new single bit in "struct refinfo" all you need to keep
track of, to see the last %(color:something) you ever saw is for a
color that is not reset?

Re: [PATCH v4 6/6] for-each-ref: avoid color leakage

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
>> index 2ff4e54..04e35ba 100644
>> --- a/builtin/for-each-ref.c
>> +++ b/builtin/for-each-ref.c
>> @@ -23,6 +23,7 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>>  struct atom_value {
>>       const char *s;
>>       unsigned long ul; /* used for sorting when not FIELD_STR */
>> +     int color : 2; /* 1 indicates color, 2 indicates color-reset */
>>  };
>
> Hmph.  It looks wasteful to have this information in atom_value.

I wanted to avoid an ugly global. On the other end of the spectrum,
modifying the various functions to take an extra reset_color_leakage
parameter seems much too intrusive. Do you have any suggestions?

> Isn't a new single bit in "struct refinfo" all you need to keep
> track of, to see the last %(color:something) you ever saw is for a
> color that is not reset?

No; because I can only look at one atom and set v->color, at a time.

Re: [PATCH v4 6/6] for-each-ref: avoid color leakage

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>
>> Isn't a new single bit in "struct refinfo" all you need to keep
>> track of, to see the last %(color:something) you ever saw is for a
>> color that is not reset?
>
> No; because I can only look at one atom and set v->color, at a time.

That is probably because the patch is trying to collect a wrong kind
of information, I think. If the approach is to check if each atom is
a color atom, parse_atom() may be the right place, but that is not
necessary.

The only thing you need to know is if you need to emit a "reset"
that the user did not explicitly ask for at the end, and that is a
property of the format string, which is constant across refs you are
iterating over. You do not even need to know which one of the atom
is of the "color" kind.

My knee-jerk "adding it to struct refinfo" is not correct, either,
because what we want to know, i.e. "do we need an extra reset?" is
not a property for each ref.  It is similar to "what is the set of
atoms the format string is using?" and "do we need to peel tags in
order to show all information asked by the format string?"
(i.e. used_atom[] and need_tagged, respectively).

Unlike need_tagged which asks "is there any *refname that asks us to
peel tags?", however, "is the _last_ color:anything in the format
string not 'reset'?" depends on the order of appearance of atoms in
the format string, so this needs to be done in a loop that scans the
format string from left to right once at the very beginning, and we
have a perfect place to do so in verify_format().

So perhaps like this one on top?

 builtin/for-each-ref.c | 45 +++++++++++++++++++++------------------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 04e35ba..5ff51d1 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -23,7 +23,6 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 struct atom_value {
 	const char *s;
 	unsigned long ul; /* used for sorting when not FIELD_STR */
-	int color : 2; /* 1 indicates color, 2 indicates color-reset */
 };
 
 struct ref_sort {
@@ -94,6 +93,7 @@ static struct {
 static const char **used_atom;
 static cmp_type *used_atom_type;
 static int used_atom_cnt, sort_atom_limit, need_tagged, need_symref;
+static int need_color_reset_at_eol;
 
 /*
  * Used to parse format string and sort specifiers
@@ -180,13 +180,21 @@ static const char *find_next(const char *cp)
 static int verify_format(const char *format)
 {
 	const char *cp, *sp;
+	static const char color_reset[] = "color:reset";
+
+	need_color_reset_at_eol = 0;
 	for (cp = format; *cp && (sp = find_next(cp)); ) {
 		const char *ep = strchr(sp, ')');
+		int at;
+
 		if (!ep)
 			return error("malformed format string %s", sp);
 		/* sp points at "%(" and ep points at the closing ")" */
-		parse_atom(sp + 2, ep);
+		at = parse_atom(sp + 2, ep);
 		cp = ep + 1;
+
+		if (!memcmp(used_atom[at], "color:", 6))
+			need_color_reset_at_eol = !!strcmp(used_atom[at], color_reset);
 	}
 	return 0;
 }
@@ -644,7 +652,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;
@@ -669,10 +677,6 @@ static void populate_value(struct refinfo *ref)
 			char color[COLOR_MAXLEN] = "";
 
 			color_parse(name + 6, "--format", color);
-			if (!strcmp(name + 6, "reset"))
-				v->color = 2;
-			else
-				v->color = 1;
 			v->s = xstrdup(color);
 			continue;
 		} else if (!strcmp(name, "flag")) {
@@ -730,7 +734,7 @@ static void populate_value(struct refinfo *ref)
 				continue;
 			} else if (!strcmp(formatp, "trackshort") &&
 				!prefixcmp(name, "upstream")) {
-
+				assert(branch);
 				stat_tracking_info(branch, &num_ours, &num_theirs);
 				if (!num_ours && !num_theirs)
 					v->s = "=";
@@ -986,35 +990,28 @@ 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;
-	struct atom_value *atomv, resetv;
-	int reset_color = 0;
-	char color[COLOR_MAXLEN] = "";
 
-	color_parse("reset", "--format", color);
-	resetv.s = color;
 	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
+		struct atom_value *atomv;
+
 		ep = strchr(sp, ')');
 		if (cp < sp)
 			emit(cp, sp);
 		get_value(info, parse_atom(sp + 2, ep), &atomv);
 		print_value(atomv, quote_style);
-
-		/*
-		 * reset_color is used to avoid color leakage; it
-		 * should be set when the last color atom is not a
-		 * color-reset.
-		 */
-		if (atomv->color == 1)
-			reset_color = 1;
-		else if (atomv->color == 2)
-			reset_color = 0;
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);
 		emit(cp, sp);
 	}
-	if (reset_color)
+	if (need_color_reset_at_eol) {
+		struct atom_value resetv;
+		char color[COLOR_MAXLEN] = "";
+
+		color_parse("reset", "--format", color);
+		resetv.s = color;
 		print_value(&resetv, quote_style);
+	}
 	putchar('\n');
 }
 

Re: [PATCH v4 6/6] for-each-ref: avoid color leakage

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>>> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
>>> index 2ff4e54..04e35ba 100644
>>> --- a/builtin/for-each-ref.c
>>> +++ b/builtin/for-each-ref.c
>>> @@ -23,6 +23,7 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>>>  struct atom_value {
>>>       const char *s;
>>>       unsigned long ul; /* used for sorting when not FIELD_STR */
>>> +     int color : 2; /* 1 indicates color, 2 indicates color-reset */
>>>  };
>>
>> Hmph.  It looks wasteful to have this information in atom_value.
>
> I wanted to avoid an ugly global. On the other end of the spectrum,
> modifying the various functions to take an extra reset_color_leakage
> parameter seems much too intrusive. Do you have any suggestions?

We already represent information that is for the format string as
existing globals. It means that, if we ever want to make the program
accept and use more than one format string, we can't.  We need one
set of them for each such format string before we can use more than
one.

If you want to solve that problem, complaining by using a subjective
word "ugly" does not help us much.  The right approach to the
solution would be to first think what each global really means and
decide which ones are per-format properties.  Then turn them into a
proper abstraction by defining a structure to hold the currently
considered format string and these various "per format string"
properties.

Once you do that, you can optionally make the code pass that single
structure around, and that will remove the global, but I think that
step can wait until we actually find a need for it.

Re: [PATCH v4 6/6] for-each-ref: avoid color leakage

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> My knee-jerk "adding it to struct refinfo" is not correct, either,
> because what we want to know, i.e. "do we need an extra reset?" is
> not a property for each ref.  It is similar to "what is the set of
> atoms the format string is using?" and "do we need to peel tags in
> order to show all information asked by the format string?"
> (i.e. used_atom[] and need_tagged, respectively).

It's mostly cruft carried over from my reset-color-after-each-token
implementation. My severe distaste for global variables prevented me
from looking for the simpler solution.

> Unlike need_tagged which asks "is there any *refname that asks us to
> peel tags?", however, "is the _last_ color:anything in the format
> string not 'reset'?" depends on the order of appearance of atoms in
> the format string, so this needs to be done in a loop that scans the
> format string from left to right once at the very beginning, and we
> have a perfect place to do so in verify_format().

I should be shot for my laziness and lack of ingenuity. Yeah,
verify_format() is a much better place to put the logic than
populate_value().

> So perhaps like this one on top?
>
>  builtin/for-each-ref.c | 45 +++++++++++++++++++++------------------------
>  1 file changed, 21 insertions(+), 24 deletions(-)
>
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 04e35ba..5ff51d1 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c

Thanks for taking the time to do this. And apologies for the laziness.