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

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

[Nguyễn Thái Ngọc Duy]

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

[PATCH 1/9] quote.c: make sq_quote_print a slight wrapper of sq_quote_buf

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

Re: [PATCH 1/9] quote.c: make sq_quote_print a slight wrapper of sq_quote_buf

[Felipe Contreras]

On Sun, May 19, 2013 at 5:27 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  quote.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)

No functional changes I suppose.

-- 
Felipe Contreras

[PATCH 2/9] for-each-ref: convert to use *_quote_buf instead of _quote_print

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

Re: [PATCH 2/9] for-each-ref: convert to use *_quote_buf instead of _quote_print

[Felipe Contreras]

On Sun, May 19, 2013 at 5:27 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  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

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

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

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

[Ramkumar Ramachandra]

> [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.

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

[Duy Nguyen]

On Sun, May 19, 2013 at 6:17 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
>> [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

[PATCH 4/9] for-each-ref: add %(current) for current branch marker

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

Re: [PATCH 4/9] for-each-ref: add %(current) for current branch marker

[Ramkumar Ramachandra]

Nguyễn Thái Ngọc Duy wrote:
> 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" },
>  };

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

[PATCH 5/9] for-each-ref: add %(tracking[:upstream]) for tracking info

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

Re: [PATCH 5/9] for-each-ref: add %(tracking[:upstream]) for tracking info

[Ramkumar Ramachandra]

Nguyễn Thái Ngọc Duy wrote:
> 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 :|

Re: [PATCH 5/9] for-each-ref: add %(tracking[:upstream]) for tracking info

[Felipe Contreras]

On Sun, May 19, 2013 at 6:18 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Nguyễn Thái Ngọc Duy wrote:
>> 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

Re: [PATCH 5/9] for-each-ref: add %(tracking[:upstream]) for tracking info

[Duy Nguyen]

On Sun, May 19, 2013 at 6:38 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sun, May 19, 2013 at 6:18 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> Nguyễn Thái Ngọc Duy wrote:
>>> 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

Re: [PATCH 5/9] for-each-ref: add %(tracking[:upstream]) for tracking info

[Ramkumar Ramachandra]

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].

Re: [PATCH 5/9] for-each-ref: add %(tracking[:upstream]) for tracking info

[Felipe Contreras]

On Sun, May 19, 2013 at 6:48 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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].

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

[PATCH 6/9] for-each-ref: add %(color:...)

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

Re: [PATCH 6/9] for-each-ref: add %(color:...)

[Ramkumar Ramachandra]

Nguyễn Thái Ngọc Duy wrote:
> 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
> @@ -78,6 +81,7 @@ static struct {
>         { "current" },
>         { "tracking" },
>         { "tracking:upstream" },
> +       { "color" },
>  };

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.

>  /*
> @@ -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;
> +               }

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

[PATCH 7/9] for-each-ref: prepoplulate all atoms before show_ref()

[Nguyễn Thái Ngọc Duy]

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 <pclouds@gmail.com>
---
 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

[PATCH 8/9] for-each-ref: merge show_ref into show_refs

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

[PATCH 9/9] for-each-ref: support %(...:aligned) for left alignment

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

Re: [PATCH 9/9] for-each-ref: support %(...:aligned) for left alignment

[Ramkumar Ramachandra]

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:
> 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.

Re: [PATCH 9/9] for-each-ref: support %(...:aligned) for left alignment

[Duy Nguyen]

On Sun, May 19, 2013 at 6:32 PM, Ramkumar Ramachandra
<artagnon@gmail.com> 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:
>> 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

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

[Ramkumar Ramachandra]

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.

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

[Felipe Contreras]

On Sun, May 19, 2013 at 6:11 AM, Ramkumar Ramachandra
<artagnon@gmail.com> 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

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

[Duy Nguyen]

On Sun, May 19, 2013 at 6:11 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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.

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

>>  - %(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.

>>  - %(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.

>>  - %(...: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 %>...

>>    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? ;)

>>  - 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?

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

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

[Ramkumar Ramachandra]

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?

>> 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).

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

[Duy Nguyen]

On Sun, May 19, 2013 at 7:08 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
>> "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.

>> 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.

>
>>> 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.

>
>> 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

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

[Ramkumar Ramachandra]

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).

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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.

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> 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)??

"marker" will paint us into a corner we cannot get out of when we
want to mark refs with different criteria later.  At least "current"
tells you what kind of marker we are talking about and is 1000 times
better.