[PATCH/WIP 0/9] for-each-ref format improvements

The purpose of this series is to make "for-each-ref --format" powerful
enough to display what "branch -v" and "branch -vv" do so that we
could get rid of those display code and use for-each-ref code instead.

The benefits are clear: share more code, branch can also borrow
--sort and --format from for-each-ref, which should satisty users who
are not happy with what -v and -vv provides.

This version has not gotten there yet. I just want to post quick and
dirty changes and try to address some design issues.

Originally I wanted to introduce --pretty with git-log's pretty syntax
to for-each-ref, deprecating --format. But because --format has to be
there forever, we cannot remove its code, and the code change to make
pretty code ready for displaying refs may not be small. So I went with
enhancing --format syntax instead.

This series introduces:

 - %(current), which either shows "*" if the ref is pointed by HEAD
   or a space. Junio called it %(headness). I don't like that.
   I don't like %(current) either but we have to start somewhere.
   Name suggestion? %(marker)??

 - %(tracking[:upstream]) gives us the exact output that branch -v[v]
   does. %(upstream) does not include []. We can't change its
   semantics.
 
 - %(color:...) is pretty much the same as %C family in pretty code.
   I haven't added code for %(color:foo) == %C(foo) yet. There's a
   potential ambiguity here: %C(red) == %Cred or %C(red)??

 - %(...:aligned) to do left aligning. I'm not entirely sure about
   this. We might be able to share code with %>, %< and %>< from
   pretty.c. But we need improvements there too because in
   for-each-ref case, we could calculate column width but %< would
   require the user to specify the width.

   Do people expect fancy layout with for-each-ref (and branch)? If so
   we might need to have %(align) or something instead of the simple
   left alignment case in %(...:aligned)
 
 - We may need an equivalent of the space following % in pretty
   format. If the specifier produces something, then prepend a space,
   otherwise produce nothing. Do it like %C( tracking) vs
   %C(tracking)??

You can try this after applying the series, which should give you the
about close to 'branch -v'. %(tracking) coloring does not work though.

git for-each-ref --format='%(current) %(color:auto)%(refname:short:aligned)%(color:reset) %%(objectname:short) %(tracking) %(subject)' 'refs/heads/*'

Nguyễn Thái Ngọc Duy (9):
  quote.c: make sq_quote_print a slight wrapper of sq_quote_buf
  for-each-ref: convert to use *_quote_buf instead of _quote_print
  for-each-ref: avoid printing each element directly to stdout
  for-each-ref: add %(current) for current branch marker
  for-each-ref: add %(tracking[:upstream]) for tracking info
  for-each-ref: add %(color:...)
  for-each-ref: prepoplulate all atoms before show_ref()
  for-each-ref: merge show_ref into show_refs
  for-each-ref: support %(...:aligned) for left alignment

 branch.h               |  11 ++++
 builtin/branch.c       |  16 ++----
 builtin/for-each-ref.c | 146 ++++++++++++++++++++++++++++++++++++++++++-------
 quote.c                |  61 +++++++++------------
 quote.h                |   6 +-
 5 files changed, 170 insertions(+), 70 deletions(-)

-- 
1.8.2.83.gc99314b

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 quote.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/quote.c b/quote.c
index 911229f..c67f5d3 100644
--- a/quote.c
+++ b/quote.c

@@ -44,19 +44,10 @@ void sq_quote_buf(struct strbuf *dst, const char *src)
 
 void sq_quote_print(FILE *stream, const char *src)
 {
-	char c;
-
-	fputc('\'', stream);
-	while ((c = *src++)) {
-		if (need_bs_quote(c)) {
-			fputs("'\\", stream);
-			fputc(c, stream);
-			fputc('\'', stream);
-		} else {
-			fputc(c, stream);
-		}
-	}
-	fputc('\'', stream);
+	struct strbuf sb = STRBUF_INIT;
+	sq_quote_buf(&sb, src);
+	fputs(sb.buf, stream);
+	strbuf_release(&sb);
 }
 
 void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)

-- 
1.8.2.83.gc99314b

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 builtin/for-each-ref.c | 14 ++++++++++----
 quote.c                | 44 ++++++++++++++++++++++----------------------
 quote.h                |  6 +++---
 3 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 7f059c3..14151b4 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c

@@ -867,24 +867,30 @@ static void sort_refs(struct ref_sort *sort, struct refinfo **refs, int num_refs
 static void print_value(struct refinfo *ref, int atom, int quote_style)
 {
 	struct atom_value *v;
+	struct strbuf sb_buf = STRBUF_INIT;
+	struct strbuf *sb = &sb_buf;
 	get_value(ref, atom, &v);
 	switch (quote_style) {
 	case QUOTE_NONE:
 		fputs(v->s, stdout);
 		break;
 	case QUOTE_SHELL:
-		sq_quote_print(stdout, v->s);
+		sq_quote_buf(sb, v->s);
 		break;
 	case QUOTE_PERL:
-		perl_quote_print(stdout, v->s);
+		perl_quote_buf(sb, v->s);
 		break;
 	case QUOTE_PYTHON:
-		python_quote_print(stdout, v->s);
+		python_quote_buf(sb, v->s);
 		break;
 	case QUOTE_TCL:
-		tcl_quote_print(stdout, v->s);
+		tcl_quote_buf(sb, v->s);
 		break;
 	}
+	if (quote_style != QUOTE_NONE) {
+		fputs(sb->buf, stdout);
+		strbuf_release(sb);
+	}
 }
 
 static int hex1(char ch)

diff --git a/quote.c b/quote.c
index c67f5d3..e7a240d 100644
--- a/quote.c
+++ b/quote.c

@@ -454,72 +454,72 @@ int unquote_c_style(struct strbuf *sb, const char *quoted, const char **endp)
 
 /* quoting as a string literal for other languages */
 
-void perl_quote_print(FILE *stream, const char *src)
+void perl_quote_buf(struct strbuf *sb, const char *src)
 {
 	const char sq = '\'';
 	const char bq = '\\';
 	char c;
 
-	fputc(sq, stream);
+	strbuf_addch(sb, sq);
 	while ((c = *src++)) {
 		if (c == sq || c == bq)
-			fputc(bq, stream);
-		fputc(c, stream);
+			strbuf_addch(sb, bq);
+		strbuf_addch(sb, c);
 	}
-	fputc(sq, stream);
+	strbuf_addch(sb, sq);
 }
 
-void python_quote_print(FILE *stream, const char *src)
+void python_quote_buf(struct strbuf *sb, const char *src)
 {
 	const char sq = '\'';
 	const char bq = '\\';
 	const char nl = '\n';
 	char c;
 
-	fputc(sq, stream);
+	strbuf_addch(sb, sq);
 	while ((c = *src++)) {
 		if (c == nl) {
-			fputc(bq, stream);
-			fputc('n', stream);
+			strbuf_addch(sb, bq);
+			strbuf_addch(sb, 'n');
 			continue;
 		}
 		if (c == sq || c == bq)
-			fputc(bq, stream);
-		fputc(c, stream);
+			strbuf_addch(sb, bq);
+		strbuf_addch(sb, c);
 	}
-	fputc(sq, stream);
+	strbuf_addch(sb, sq);
 }
 
-void tcl_quote_print(FILE *stream, const char *src)
+void tcl_quote_buf(struct strbuf *sb, const char *src)
 {
 	char c;
 
-	fputc('"', stream);
+	strbuf_addch(sb, '"');
 	while ((c = *src++)) {
 		switch (c) {
 		case '[': case ']':
 		case '{': case '}':
 		case '$': case '\\': case '"':
-			fputc('\\', stream);
+			strbuf_addch(sb, '\\');
 		default:
-			fputc(c, stream);
+			strbuf_addch(sb, c);
 			break;
 		case '\f':
-			fputs("\\f", stream);
+			strbuf_addstr(sb, "\\f");
 			break;
 		case '\r':
-			fputs("\\r", stream);
+			strbuf_addstr(sb, "\\r");
 			break;
 		case '\n':
-			fputs("\\n", stream);
+			strbuf_addstr(sb, "\\n");
 			break;
 		case '\t':
-			fputs("\\t", stream);
+			strbuf_addstr(sb, "\\t");
 			break;
 		case '\v':
-			fputs("\\v", stream);
+			strbuf_addstr(sb, "\\v");
 			break;
 		}
 	}
-	fputc('"', stream);
+	strbuf_addch(sb, '"');
 }

diff --git a/quote.h b/quote.h
index 133155a..ed06df5 100644
--- a/quote.h
+++ b/quote.h

@@ -69,8 +69,8 @@ extern char *quote_path_relative(const char *in, int len,
 			  struct strbuf *out, const char *prefix);
 
 /* quoting as a string literal for other languages */
-extern void perl_quote_print(FILE *stream, const char *src);
-extern void python_quote_print(FILE *stream, const char *src);
-extern void tcl_quote_print(FILE *stream, const char *src);
+extern void perl_quote_buf(struct strbuf *sb, const char *src);
+extern void python_quote_buf(struct strbuf *sb, const char *src);
+extern void tcl_quote_buf(struct strbuf *sb, const char *src);
 
 #endif

-- 
1.8.2.83.gc99314b

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 builtin/for-each-ref.c | 48 +++++++++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 14151b4..08d4eb1 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c

@@ -864,15 +864,14 @@ 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 strbuf *sb, struct refinfo *ref,
+			int atom, int quote_style)
 {
 	struct atom_value *v;
-	struct strbuf sb_buf = STRBUF_INIT;
-	struct strbuf *sb = &sb_buf;
 	get_value(ref, atom, &v);
 	switch (quote_style) {
 	case QUOTE_NONE:
-		fputs(v->s, stdout);
+		strbuf_addstr(sb, v->s);
 		break;
 	case QUOTE_SHELL:
 		sq_quote_buf(sb, v->s);

@@ -887,10 +886,6 @@ static void print_value(struct refinfo *ref, int atom, int quote_style)
 		tcl_quote_buf(sb, v->s);
 		break;
 	}
-	if (quote_style != QUOTE_NONE) {
-		fputs(sb->buf, stdout);
-		strbuf_release(sb);
-	}
 }
 
 static int hex1(char ch)

@@ -911,7 +906,7 @@ static int hex2(const char *cp)
 		return -1;
 }
 
-static void emit(const char *cp, const char *ep)
+static void emit(struct strbuf *sb, const char *cp, const char *ep)
 {
 	while (*cp && (!ep || cp < ep)) {
 		if (*cp == '%') {

@@ -920,32 +915,47 @@ static void emit(const char *cp, const char *ep)
 			else {
 				int ch = hex2(cp + 1);
 				if (0 <= ch) {
-					putchar(ch);
+					strbuf_addch(sb, ch);
 					cp += 3;
 					continue;
 				}
 			}
 		}
-		putchar(*cp);
+		strbuf_addch(sb, *cp);
 		cp++;
 	}
 }
 
-static void show_ref(struct refinfo *info, const char *format, int quote_style)
+static void show_ref(struct strbuf *sb, struct refinfo *info,
+		     const char *format, int quote_style)
 {
 	const char *cp, *sp, *ep;
 
 	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);
+			emit(sb, cp, sp);
+		print_value(sb, info, parse_atom(sp + 2, ep), quote_style);
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);
-		emit(cp, sp);
+		emit(sb, cp, sp);
+	}
+	strbuf_addch(sb, '\n');
+}
+
+static void show_refs(struct refinfo **refs, int maxcount,
+		      const char *format, int quote_style)
+{
+	struct strbuf sb = STRBUF_INIT;
+	int i;
+
+	for (i = 0; i < maxcount; i++) {
+		strbuf_reset(&sb);
+		show_ref(&sb, refs[i], format, quote_style);
+		fputs(sb.buf, stdout);
 	}
-	putchar('\n');
+	strbuf_release(&sb);
 }
 
 static struct ref_sort *default_sort(void)

@@ -988,7 +998,7 @@ static char const * const for_each_ref_usage[] = {
 
 int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 {
-	int i, num_refs;
+	int num_refs;
 	const char *format = "%(objectname) %(objecttype)\t%(refname)";
 	struct ref_sort *sort = NULL, **sort_tail = &sort;
 	int maxcount = 0, quote_style = 0;

@@ -1042,7 +1052,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 
 	if (!maxcount || num_refs < maxcount)
 		maxcount = num_refs;
-	for (i = 0; i < maxcount; i++)
-		show_ref(refs[i], format, quote_style);
+
+	show_refs(refs, maxcount, format, quote_style);
 	return 0;
 }

-- 
1.8.2.83.gc99314b

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 builtin/for-each-ref.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 08d4eb1..498d703 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c

@@ -75,6 +75,7 @@ static struct {
 	{ "upstream" },
 	{ "symref" },
 	{ "flag" },
+	{ "current" },
 };
 
 /*

@@ -90,6 +91,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 const char *head;
 
 /*
  * Used to parse format string and sort specifiers

@@ -676,6 +678,17 @@ static void populate_value(struct refinfo *ref)
 			}
 			continue;
 		}
+		else if (!strcmp(name, "current")) {
+			if (!head) {
+				unsigned char sha1[20];
+				head = resolve_refdup("HEAD", sha1, 0, NULL);
+			}
+			if (strcmp(head, "HEAD") && !strcmp(ref->refname, head))
+				v->s = "*";
+			else
+				v->s = " ";
+			continue;
+		}
 		else
 			continue;
 

-- 
1.8.2.83.gc99314b

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 builtin/branch.c       |  4 ++--
 builtin/for-each-ref.c | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 0836890..7d084da 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c

@@ -416,8 +416,8 @@ static int ref_cmp(const void *r1, const void *r2)
 	return strcmp(c1->name, c2->name);
 }
 
-static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
-		int show_upstream_ref)
+void fill_tracking_info(struct strbuf *stat, const char *branch_name,
+			int show_upstream_ref)
 {
 	int ours, theirs;
 	char *ref = NULL;

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 498d703..b10d48a 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c

@@ -76,6 +76,8 @@ static struct {
 	{ "symref" },
 	{ "flag" },
 	{ "current" },
+	{ "tracking" },
+	{ "tracking:upstream" },
 };
 
 /*

@@ -615,6 +617,8 @@ static inline char *copy_advance(char *dst, const char *src)
 	return dst;
 }
 
+extern void fill_tracking_info(struct strbuf *stat, const char *branch_name,
+			       int show_upstream_ref);
 /*
  * Parse the object referred by ref, and grab needed value.
  */

@@ -689,6 +693,20 @@ static void populate_value(struct refinfo *ref)
 				v->s = " ";
 			continue;
 		}
+		else if (!strcmp(name, "tracking") &&
+			 !prefixcmp(ref->refname, "refs/heads/")) {
+			struct strbuf sb = STRBUF_INIT;
+			fill_tracking_info(&sb, ref->refname + 11, 0);
+			v->s = sb.buf;
+			continue;
+		}
+		else if (!strcmp(name, "tracking:upstream") &&
+			 !prefixcmp(ref->refname, "refs/heads/")) {
+			struct strbuf sb = STRBUF_INIT;
+			fill_tracking_info(&sb, ref->refname + 11, 1);
+			v->s = sb.buf;
+			continue;
+		}
 		else
 			continue;
 

-- 
1.8.2.83.gc99314b

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 branch.h               | 11 +++++++++++
 builtin/branch.c       | 12 ++----------
 builtin/for-each-ref.c | 34 +++++++++++++++++++++++++++++++++-
 3 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/branch.h b/branch.h
index 64173ab..076babf 100644
--- a/branch.h
+++ b/branch.h

@@ -52,4 +52,15 @@ extern void install_branch_config(int flag, const char *local, const char *origi
  */
 extern int read_branch_desc(struct strbuf *, const char *branch_name);
 
+enum color_branch {
+	BRANCH_COLOR_RESET = 0,
+	BRANCH_COLOR_PLAIN = 1,
+	BRANCH_COLOR_REMOTE = 2,
+	BRANCH_COLOR_LOCAL = 3,
+	BRANCH_COLOR_CURRENT = 4,
+	BRANCH_COLOR_UPSTREAM = 5
+};
+extern int git_branch_config(const char *var, const char *value, void *cb);
+extern const char *branch_get_color(enum color_branch ix);
+
 #endif

diff --git a/builtin/branch.c b/builtin/branch.c
index 7d084da..1aa282c 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c

@@ -43,14 +43,6 @@ static char branch_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_GREEN,	/* CURRENT */
 	GIT_COLOR_BLUE,		/* UPSTREAM */
 };
-enum color_branch {
-	BRANCH_COLOR_RESET = 0,
-	BRANCH_COLOR_PLAIN = 1,
-	BRANCH_COLOR_REMOTE = 2,
-	BRANCH_COLOR_LOCAL = 3,
-	BRANCH_COLOR_CURRENT = 4,
-	BRANCH_COLOR_UPSTREAM = 5
-};
 
 static enum merge_filter {
 	NO_FILTER = 0,

@@ -79,7 +71,7 @@ static int parse_branch_color_slot(const char *var, int ofs)
 	return -1;
 }
 
-static int git_branch_config(const char *var, const char *value, void *cb)
+int git_branch_config(const char *var, const char *value, void *cb)
 {
 	if (!prefixcmp(var, "column."))
 		return git_column_config(var, value, "branch", &colopts);

@@ -99,7 +91,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)
 	return git_color_default_config(var, value, cb);
 }
 
-static const char *branch_get_color(enum color_branch ix)
+const char *branch_get_color(enum color_branch ix)
 {
 	if (want_color(branch_use_color))
 		return branch_colors[ix];

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index b10d48a..db5c211 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c

@@ -9,6 +9,8 @@
 #include "quote.h"
 #include "parse-options.h"
 #include "remote.h"
+#include "color.h"
+#include "branch.h"
 
 /* Quoting styles */
 #define QUOTE_NONE 0

@@ -36,6 +38,7 @@ struct refinfo {
 	int flag;
 	const char *symref;
 	struct atom_value *value;
+	int auto_color;
 };
 
 static struct {

@@ -78,6 +81,7 @@ static struct {
 	{ "current" },
 	{ "tracking" },
 	{ "tracking:upstream" },
+	{ "color" },
 };
 
 /*

@@ -707,6 +711,21 @@ static void populate_value(struct refinfo *ref)
 			v->s = sb.buf;
 			continue;
 		}
+		else if (!prefixcmp(name, "color:")) {
+			const char *color = name + 6;
+			ref->auto_color = 0;
+			if (!prefixcmp(color, "red"))
+				v->s = GIT_COLOR_RED;
+			else if (!prefixcmp(color, "green"))
+				v->s = GIT_COLOR_GREEN;
+			else if (!prefixcmp(color, "blue"))
+				v->s = GIT_COLOR_BLUE;
+			else if (!prefixcmp(color, "reset"))
+				v->s = GIT_COLOR_RESET;
+			else if (!prefixcmp(color, "auto"))
+				ref->auto_color = 1;
+			continue;
+		}
 		else
 			continue;
 

@@ -730,6 +749,19 @@ static void populate_value(struct refinfo *ref)
 			sprintf(s, "%s^{}", refname);
 			v->s = s;
 		}
+
+		if (ref->auto_color) {
+			if (!head) {
+				unsigned char sha1[20];
+				head = resolve_refdup("HEAD", sha1, 0, NULL);
+			}
+			if (strcmp(head, "HEAD") && !strcmp(ref->refname, head)) {
+				struct strbuf sb = STRBUF_INIT;
+				strbuf_addstr(&sb, branch_get_color(BRANCH_COLOR_CURRENT));
+				strbuf_addstr(&sb, v->s);
+				v->s = sb.buf;
+			}
+		}
 	}
 
 	for (i = 0; i < used_atom_cnt; i++) {

@@ -1071,7 +1103,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	sort_atom_limit = used_atom_cnt;
 
 	/* for warn_ambiguous_refs */
-	git_config(git_default_config, NULL);
+	git_config(git_branch_config, NULL);
 
 	memset(&cbdata, 0, sizeof(cbdata));
 	cbdata.grab_pattern = argv;

-- 
1.8.2.83.gc99314b

By the time show_ref() is called, atom values for all refs are
ready. This can be taken advantage of later.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 builtin/for-each-ref.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index db5c211..a9d189c 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c

@@ -1010,15 +1010,24 @@ static void show_ref(struct strbuf *sb, struct refinfo *info,
 static void show_refs(struct refinfo **refs, int maxcount,
 		      const char *format, int quote_style)
 {
-	struct strbuf sb = STRBUF_INIT;
+	struct strbuf *sb;
 	int i;
 
+	sb = xmalloc(sizeof(*sb) * maxcount);
 	for (i = 0; i < maxcount; i++) {
-		strbuf_reset(&sb);
-		show_ref(&sb, refs[i], format, quote_style);
-		fputs(sb.buf, stdout);
+		strbuf_init(sb + i, 256);
+		if (!refs[i]->value) {
+			populate_value(refs[i]);
+			fill_missing_values(refs[i]->value);
+		}
+	}
+
+	for (i = 0; i < maxcount; i++) {
+		show_ref(sb + i, refs[i], format, quote_style);
+		fputs(sb[i].buf, stdout);
+		strbuf_release(sb + i);
 	}
-	strbuf_release(&sb);
+	free(sb);
 }
 
 static struct ref_sort *default_sort(void)

-- 
1.8.2.83.gc99314b

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 builtin/for-each-ref.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 1390da8..3240ca0 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c

@@ -11,6 +11,7 @@
 #include "remote.h"
 #include "color.h"
 #include "branch.h"
+#include "utf8.h"
 
 /* Quoting styles */
 #define QUOTE_NONE 0

@@ -733,7 +734,7 @@ static void populate_value(struct refinfo *ref)
 		/* look for "short" refname format */
 		if (formatp) {
 			formatp++;
-			if (!strcmp(formatp, "short"))
+			if (!prefixcmp(formatp, "short"))
 				refname = shorten_unambiguous_ref(refname,
 						      warn_ambiguous_refs);
 			else

@@ -994,7 +995,7 @@ static void show_refs(struct refinfo **refs, int maxcount,
 {
 	struct strbuf *sb;
 	const char *cp, *sp, *ep;
-	int i, atom;
+	int i, atom, aligned, max_length, len;
 
 	sb = xmalloc(sizeof(*sb) * maxcount);
 	for (i = 0; i < maxcount; i++) {

@@ -1012,8 +1013,26 @@ static void show_refs(struct refinfo **refs, int maxcount,
 				emit(sb + i, cp, sp);
 		}
 		atom = parse_atom(sp + 2, ep);
-		for (i = 0; i < maxcount; i++)
+		aligned = !suffixcmp(used_atom[atom], ":aligned");
+		for (i = 0, max_length = 0; aligned && i < maxcount; i++) {
+			struct atom_value *v;
+			get_value(refs[i], atom, &v);
+			len = utf8_strnwidth(v->s, -1, 1);
+			if (len > max_length)
+				max_length = len;
+		}
+		for (i = 0; i < maxcount; i++) {
+			int old_len = sb[i].len;
 			print_value(sb + i, refs[i], atom, quote_style);
+			if (aligned) {
+				len = max_length -
+					utf8_strnwidth(sb[i].buf + old_len, -1, 1);
+				while (len) {
+					strbuf_addch(sb + i, ' ');
+					len--;
+				}
+			}
+		}
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);

-- 
1.8.2.83.gc99314b

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 builtin/for-each-ref.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index a9d189c..1390da8 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c

@@ -989,29 +989,12 @@ static void emit(struct strbuf *sb, const char *cp, const char *ep)
 	}
 }
 
-static void show_ref(struct strbuf *sb, struct refinfo *info,
-		     const char *format, int quote_style)
-{
-	const char *cp, *sp, *ep;
-
-	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
-		ep = strchr(sp, ')');
-		if (cp < sp)
-			emit(sb, cp, sp);
-		print_value(sb, info, parse_atom(sp + 2, ep), quote_style);
-	}
-	if (*cp) {
-		sp = cp + strlen(cp);
-		emit(sb, cp, sp);
-	}
-	strbuf_addch(sb, '\n');
-}
-
 static void show_refs(struct refinfo **refs, int maxcount,
 		      const char *format, int quote_style)
 {
 	struct strbuf *sb;
-	int i;
+	const char *cp, *sp, *ep;
+	int i, atom;
 
 	sb = xmalloc(sizeof(*sb) * maxcount);
 	for (i = 0; i < maxcount; i++) {

@@ -1022,8 +1005,24 @@ static void show_refs(struct refinfo **refs, int maxcount,
 		}
 	}
 
+	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
+		ep = strchr(sp, ')');
+		if (cp < sp) {
+			for (i = 0; i < maxcount; i++)
+				emit(sb + i, cp, sp);
+		}
+		atom = parse_atom(sp + 2, ep);
+		for (i = 0; i < maxcount; i++)
+			print_value(sb + i, refs[i], atom, quote_style);
+	}
+	if (*cp) {
+		sp = cp + strlen(cp);
+		for (i = 0; i < maxcount; i++)
+			emit(sb + i, cp, sp);
+	}
+
 	for (i = 0; i < maxcount; i++) {
-		show_ref(sb + i, refs[i], format, quote_style);
+		strbuf_addch(sb + i, '\n');
 		fputs(sb[i].buf, stdout);
 		strbuf_release(sb + i);
 	}

-- 
1.8.2.83.gc99314b

Nguyễn Thái Ngọc Duy wrote:

The purpose of this series is to make "for-each-ref --format" powerful
enough to display what "branch -v" and "branch -vv" do so that we
could get rid of those display code and use for-each-ref code instead.

Damn, you beat me to it.  I just introduced color, and was working on
alignment.  See $gmane/224692.

Yes, I think this is the direction we should be taking.  Poorly
thought-out stuff like -v and -vv should be deprecated.

This series introduces:

 - %(current), which either shows "*" if the ref is pointed by HEAD
   or a space. Junio called it %(headness). I don't like that.
   I don't like %(current) either but we have to start somewhere.
   Name suggestion? %(marker)??

How about %(HEAD)?

 - %(tracking[:upstream]) gives us the exact output that branch -v[v]
   does. %(upstream) does not include []. We can't change its
   semantics.

There's already an atom called "upstream", and "upstream:short" works.
 Why not introduce "upstream:diff" for "[ahead x, behind y]" and
"upstream:shortdiff" for "<>" (like in the prompt)?

 - %(color:...) is pretty much the same as %C family in pretty code.
   I haven't added code for %(color:foo) == %C(foo) yet. There's a
   potential ambiguity here: %C(red) == %Cred or %C(red)??

I'd vote for dropping %C<name> altogether and just go with %C(<name>).
 Why do we need %(color:<name>) at all?

 - %(...:aligned) to do left aligning. I'm not entirely sure about
   this. We might be able to share code with %>, %< and %>< from
   pretty.c. But we need improvements there too because in
   for-each-ref case, we could calculate column width but %< would
   require the user to specify the width.

Yeah, I think we should go with the %> and %< you introduced in
pretty.c.  Yes, I want to be able to specify width.

   Do people expect fancy layout with for-each-ref (and branch)? If so
   we might need to have %(align) or something instead of the simple
   left alignment case in %(...:aligned)

Why should we deviate from the pretty case?  What is different here?

 - We may need an equivalent of the space following % in pretty
   format. If the specifier produces something, then prepend a space,
   otherwise produce nothing. Do it like %C( tracking) vs
   %C(tracking)??

Yeah, sounds good.

You can try this after applying the series, which should give you the
about close to 'branch -v'. %(tracking) coloring does not work though.

Why doesn't %(tracking) coloring work?

Nguyễn Thái Ngọc Duy (9):

I'll have a look at this.

Also, I think it'll help to have a --pretty="format:<string>"
equivalent to --format="<string>" so that we can introduce pretty
names like oneline, short, medium, full.  We can eventually deprecate
--format for consistency.

Nguyễn Thái Ngọc Duy wrote:

So you've implemented it as another atom.  Good.

[PATCH 3/9] for-each-ref: avoid printing each element directly to stdout

Why did you do this?  Atoms are designed to be independent of each
other.  I'll keep reading: might find out in a later patch.

Nguyễn Thái Ngọc Duy wrote:

You just threw the upstream atom (and "upstream:short") out the window :|

Nguyễn Thái Ngọc Duy wrote:

No, I intentionally did not make color an atom in $gmane/224692.  It
doesn't print any information about the branch, like the other atoms
do: have it augment the final printing.

So I can't have %(color:yellow)? :(
Why are you parsing it here when you can use color_parse_name()?

I don't think [7/9] and [8/9] belong in this series.  Let's see how
you've used it in :aligned.

Nguyễn Thái Ngọc Duy wrote:

Why?!  Why are you denying me the pleasure of using %<, %<|, %>, %>|,
%>>, %>>|, %<>, and %<>| that you invented in pretty?  The code is
already there: you just have to hook it up.

On Sun, May 19, 2013 at 6:11 AM, Ramkumar Ramachandra
[off-list ref] wrote:

Yes, I think this is the direction we should be taking.  Poorly
thought-out stuff like -v and -vv should be deprecated.

Of course not. They are useful and user-friendly.

The only question is what should be the format by default.

Also, I think it'll help to have a --pretty="format:<string>"
equivalent to --format="<string>" so that we can introduce pretty
names like oneline, short, medium, full.  We can eventually deprecate
--format for consistency.

I don't see the point of --pretty. I think there should be shortcuts,
something like --hash for the SHA1s only, and --names for the refs
only, or something like that.

-- 
Felipe Contreras

On Sun, May 19, 2013 at 6:18 AM, Ramkumar Ramachandra
[off-list ref] wrote:

Nguyễn Thái Ngọc Duy wrote:

quoted

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 498d703..b10d48a 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c

@@ -76,6 +76,8 @@ static struct {
        { "symref" },
        { "flag" },
        { "current" },
+       { "tracking" },
+       { "tracking:upstream" },
 };

You just threw the upstream atom (and "upstream:short") out the window :|

Huh? Those don't print the tracking information, do they?
"tracking:upstream" prints the upstream, but other things as well I
suppose.

-- 
Felipe Contreras

On Sun, May 19, 2013 at 5:27 AM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 quote.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

No functional changes I suppose.

-- 
Felipe Contreras

On Sun, May 19, 2013 at 5:27 AM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 builtin/for-each-ref.c | 14 ++++++++++----
 quote.c                | 44 ++++++++++++++++++++++----------------------
 quote.h                |  6 +++---
 3 files changed, 35 insertions(+), 29 deletions(-)

No functional changes I suppose.

-- 
Felipe Contreras

On Sun, May 19, 2013 at 6:32 PM, Ramkumar Ramachandra
[off-list ref] wrote:

I don't think [7/9] and [8/9] belong in this series.  Let's see how
you've used it in :aligned.

Nguyễn Thái Ngọc Duy wrote:

quoted

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 1390da8..3240ca0 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c

@@ -1012,8 +1013,26 @@ static void show_refs(struct refinfo **refs, int maxcount,
                                emit(sb + i, cp, sp);
                }
                atom = parse_atom(sp + 2, ep);
-               for (i = 0; i < maxcount; i++)
+               aligned = !suffixcmp(used_atom[atom], ":aligned");
+               for (i = 0, max_length = 0; aligned && i < maxcount; i++) {
+                       struct atom_value *v;
+                       get_value(refs[i], atom, &v);
+                       len = utf8_strnwidth(v->s, -1, 1);
+                       if (len > max_length)
+                               max_length = len;

Why?!  Why are you denying me the pleasure of using %<, %<|, %>, %>|,
%>>, %>>|, %<>, and %<>| that you invented in pretty?  The code is
already there: you just have to hook it up.

Because for-each-ref only understands %(...) not %<|, i.e. % followed
by a (. We need more changes in for-each-ref code to make it accept
%<|, I think. Also %<|(N) needs "N". In case of "branch -v" that
should be calculated automatically, so we need a syntax to say "align
to the left, use the smallest width that fits all". I guess "%<|(*)"?
--
Duy

On Sun, May 19, 2013 at 6:38 PM, Felipe Contreras
[off-list ref] wrote:

On Sun, May 19, 2013 at 6:18 AM, Ramkumar Ramachandra
[off-list ref] wrote:

quoted

Nguyễn Thái Ngọc Duy wrote:

quoted

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 498d703..b10d48a 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c

@@ -76,6 +76,8 @@ static struct {
        { "symref" },
        { "flag" },
        { "current" },
+       { "tracking" },
+       { "tracking:upstream" },
 };

You just threw the upstream atom (and "upstream:short") out the window :|

Huh? Those don't print the tracking information, do they?
"tracking:upstream" prints the upstream, but other things as well I
suppose.

Exactly. I already explained why %(upstream) can't be used in 00/09.
"tracking" may not be perfect. Somebody might want
"tracking:upstream:short". It does not look quite nice.
--
Duy

Duy Nguyen wrote:

Exactly. I already explained why %(upstream) can't be used in 00/09.
"tracking" may not be perfect. Somebody might want
"tracking:upstream:short". It does not look quite nice.

Which is why I suggested keeping upstream, upstream:short, and
introducing upstream:diff and upstream:shortdiff (or :tracking if you
prefer that) in [0/9].

On Sun, May 19, 2013 at 6:11 PM, Ramkumar Ramachandra
[off-list ref] wrote:

Nguyễn Thái Ngọc Duy wrote:

quoted

The purpose of this series is to make "for-each-ref --format" powerful
enough to display what "branch -v" and "branch -vv" do so that we
could get rid of those display code and use for-each-ref code instead.

Damn, you beat me to it.  I just introduced color, and was working on
alignment.  See $gmane/224692.

Hmm.. I missed that mail (or I wouldn't have worked on this already).
Do you want to take over?

quoted

 - %(tracking[:upstream]) gives us the exact output that branch -v[v]
   does. %(upstream) does not include []. We can't change its
   semantics.

There's already an atom called "upstream", and "upstream:short" works.
 Why not introduce "upstream:diff" for "[ahead x, behind y]" and
"upstream:shortdiff" for "<>" (like in the prompt)?

"branch -vv" shows [upstream: ahead x, behind y]. We need a syntax to
cover that too.

quoted

 - %(color:...) is pretty much the same as %C family in pretty code.
   I haven't added code for %(color:foo) == %C(foo) yet. There's a
   potential ambiguity here: %C(red) == %Cred or %C(red)??

I'd vote for dropping %C<name> altogether and just go with %C(<name>).
 Why do we need %(color:<name>) at all?

pretty and for-each-ref format seem to be on the opposite: one is
terse, one verbose. Unless you are going to introduce a lot of new
specifiers (and in the worst case, bring all pretty specifiers over,
unify underlying code), I think we should stick with %(xx) convention.

quoted

 - %(...:aligned) to do left aligning. I'm not entirely sure about
   this. We might be able to share code with %>, %< and %>< from
   pretty.c. But we need improvements there too because in
   for-each-ref case, we could calculate column width but %< would
   require the user to specify the width.

Yeah, I think we should go with the %> and %< you introduced in
pretty.c.  Yes, I want to be able to specify width.

I still think we should follow %(...), e.g. %(left:N), %(right:N) as
equivalent of %< and %>...

quoted

   Do people expect fancy layout with for-each-ref (and branch)? If so
   we might need to have %(align) or something instead of the simple
   left alignment case in %(...:aligned)

Why should we deviate from the pretty case?  What is different here?

Laziness plays a big factor :) So again, you want to take over? ;)

quoted

 - We may need an equivalent of the space following % in pretty
   format. If the specifier produces something, then prepend a space,
   otherwise produce nothing. Do it like %C( tracking) vs
   %C(tracking)??

Yeah, sounds good.

quoted

You can try this after applying the series, which should give you the
about close to 'branch -v'. %(tracking) coloring does not work though.

Why doesn't %(tracking) coloring work?

it uses builtin/branch.c:branch_use_color. Eventually
fill_tracking_info() should be moved to for-each-ref.c and pass
branch_use_color in as an argument. But for now, I just leave it
broken.
--
Duy

On Sun, May 19, 2013 at 6:17 PM, Ramkumar Ramachandra
[off-list ref] wrote:

quoted

[PATCH 3/9] for-each-ref: avoid printing each element directly to stdout

Why did you do this?  Atoms are designed to be independent of each
other.  I'll keep reading: might find out in a later patch.

Sorry for the lack of explanation in the message. I wanted to discuss
about syntax more than code. As you see later down the series, we'll
need to process an atom for all refs, then output all refs in the end.
We can't do that wihout buffering.
--
Duy

On Sun, May 19, 2013 at 6:48 AM, Ramkumar Ramachandra
[off-list ref] wrote:

Duy Nguyen wrote:

quoted

Exactly. I already explained why %(upstream) can't be used in 00/09.
"tracking" may not be perfect. Somebody might want
"tracking:upstream:short". It does not look quite nice.

Which is why I suggested keeping upstream, upstream:short, and
introducing upstream:diff and upstream:shortdiff (or :tracking if you
prefer that) in [0/9].

Yeah, but there won't be any upstream in %(tracking). Besides, if we
manage to get downstream, we could do %(tracking:downstream). I think
%(tracking) and %(short:tracking) make sense.

-- 
Felipe Contreras

Duy Nguyen wrote:

Hmm.. I missed that mail (or I wouldn't have worked on this already).
Do you want to take over?

Oh, we can collaborate on one beautiful series :)

"branch -vv" shows [upstream: ahead x, behind y]. We need a syntax to
cover that too.

Can't we construct that using [%(upstream:short): %(upstream:diff)]?
It's nothing fundamental.

pretty and for-each-ref format seem to be on the opposite: one is
terse, one verbose. Unless you are going to introduce a lot of new
specifiers (and in the worst case, bring all pretty specifiers over,
unify underlying code), I think we should stick with %(xx) convention.

We can stick to using the existing %(...) atoms: there's no need to go
as far as %an versus %aN.  The atoms cannot be consistent with
pretty-formats anyway, because pretty-formats has completely different
atoms.  For the _new_ stuff like color and alignment, we can be
consistent with pretty-formats, no?

quoted

Why should we deviate from the pretty case?  What is different here?

Laziness plays a big factor :) So again, you want to take over? ;)

It's just a matter of modifying the parsing/printing layer, instead of
introducing new atoms in the current parser.  Doesn't $gmane/224692
demonstrate that the former can, in fact, be easier?

it uses builtin/branch.c:branch_use_color. Eventually
fill_tracking_info() should be moved to for-each-ref.c and pass
branch_use_color in as an argument. But for now, I just leave it
broken.

Auto-color can come later: it's not that urgent.  What's more
important is that we give users the flexibility to set their own
colors now.

Can you give me a pull URL to your series, so we can start
collaborating?  Mine's at gh:artagnon/git (branch: hot-branch).

On Sun, May 19, 2013 at 7:08 PM, Ramkumar Ramachandra
[off-list ref] wrote:

quoted

"branch -vv" shows [upstream: ahead x, behind y]. We need a syntax to
cover that too.

Can't we construct that using [%(upstream:short): %(upstream:diff)]?
It's nothing fundamental.

If there is no upstream, [] should not be shown. We don't have
conditional specifiers so [] should be produced by one of the
specifiers. But yes it's nothing fundamental.

quoted

pretty and for-each-ref format seem to be on the opposite: one is
terse, one verbose. Unless you are going to introduce a lot of new
specifiers (and in the worst case, bring all pretty specifiers over,
unify underlying code), I think we should stick with %(xx) convention.

We can stick to using the existing %(...) atoms: there's no need to go
as far as %an versus %aN.  The atoms cannot be consistent with
pretty-formats anyway, because pretty-formats has completely different
atoms.  For the _new_ stuff like color and alignment, we can be
consistent with pretty-formats, no?

I don't think you can easily borrow parsing code from pretty-formats
(but I may be wrong). Anyway new stuff with new syntax would look
alien in for-each-ref format lines. Either we bring --pretty to
for-each-ref, leaving all for-each-ref atoms behind in --format, or we
should follow %(..) convention if we add new stuff to --format.

quoted

quoted

Why should we deviate from the pretty case?  What is different here?

Laziness plays a big factor :) So again, you want to take over? ;)

It's just a matter of modifying the parsing/printing layer, instead of
introducing new atoms in the current parser.  Doesn't $gmane/224692
demonstrate that the former can, in fact, be easier?

Yes it looks so.

quoted

it uses builtin/branch.c:branch_use_color. Eventually
fill_tracking_info() should be moved to for-each-ref.c and pass
branch_use_color in as an argument. But for now, I just leave it
broken.

Auto-color can come later: it's not that urgent.  What's more
important is that we give users the flexibility to set their own
colors now.

Can you give me a pull URL to your series, so we can start
collaborating?  Mine's at gh:artagnon/git (branch: hot-branch).

https://github.com/pclouds/git/tree/for-each-ref-pretty

I merged 07/09 and 08/09 together as they should be one.
--
Duy

Duy Nguyen wrote:

I don't think you can easily borrow parsing code from pretty-formats
(but I may be wrong). Anyway new stuff with new syntax would look
alien in for-each-ref format lines. Either we bring --pretty to
for-each-ref, leaving all for-each-ref atoms behind in --format, or we
should follow %(..) convention if we add new stuff to --format.

Why so extremist?  pretty-formats has %(...), %C(...) as well as %...,
so why shouldn't we?  Our format is undocumented, and I doubt anyone
even uses it; we're not breaking anyone's expectations.  I'm just
saying that our format can be a little reminiscent of pretty-formats,
nothing more.  There's no need to borrow parsing code: we can do it
ourselves, I think.  There is no need to go to the other extreme and
throw out the existing --format and start out with a --pretty from
scratch either: the current code isn't so bad that we can't build on
top of it.  Sure, we can eventually deprecate --format and move to
--pretty for consistency (but that's a long-term goal).

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

Originally I wanted to introduce --pretty with git-log's pretty syntax
to for-each-ref, deprecating --format.

If you are going to unify the two mechanisms, I think the "--format"
option of "for-each-ref" needs to become a superset of what the
"--pretty" option of "log/rev-list" can express and shared between
the two.  The former has to handle anything that can be at the tip
of a ref, including commits, so it needs to be able to do anything
the latter can.