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

[Junio C Hamano <gitster@pobox.com>]

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');
 }