[PATCH 00/15] Towards a more awesome git-branch

[PATCH 00/15] Towards a more awesome git-branch

[Ramkumar Ramachandra]

Hi,

Duy and I have been working on this topic for some time now.  Here's a
review candidate.  Duy did most of the chunky work, and I mostly
did review/documentation.  The key patches are:

[5/15]  is a brilliant patch that made this entire thing possible.
[10/15] is another brilliant patch to auto-calculate widths.

Duy is currently writing code to factor out -v[v] and adding --sort,
--count to git-branch, but I'm having a lot of difficulty working with
a series of this size.  I'm leaning towards getting this merged before
tackling branch (who wants to review a 40-part series?).  Especially
after [14/15] and [15/15], git for-each-ref should be usable in its
own right.  I currently have:

[pretty]
hot = %C(red)%(HEAD)%C(reset) %C(green)%(refname:short)%C(reset)%(upstream:trackshort)
[alias]
hot = for-each-ref --pretty=hot --count 10 --sort='-committerdate' refs/heads

Which is really useful and manageable until we port these options to
branch and get nice configuration variables.

Nguyễn Thái Ngọc Duy (8):
  for-each-ref, quote: convert *_quote_print -> *_quote_buf
  for-each-ref: don't print out elements directly
  pretty: extend pretty_print_context with callback
  pretty: allow passing NULL commit to format_commit_message()
  for-each-ref: get --pretty using format_commit_message
  for-each-ref: teach verify_format() about pretty's syntax
  for-each-ref: introduce format specifier %>(*) and %<(*)
  for-each-ref: improve responsiveness of %(upstream:track)

Ramkumar Ramachandra (7):
  tar-tree: remove dependency on sq_quote_print()
  quote: remove sq_quote_print()
  pretty: limit recursion in format_commit_one()
  for-each-ref: introduce %(HEAD) marker
  for-each-ref: introduce %(upstream:track[short])
  pretty: introduce get_pretty_userformat
  for-each-ref: use get_pretty_userformat in --pretty

 Documentation/git-for-each-ref.txt |  42 +++++-
 builtin/for-each-ref.c             | 274 ++++++++++++++++++++++++++++++-------
 builtin/tar-tree.c                 |  11 +-
 commit.h                           |   9 ++
 pretty.c                           |  77 ++++++++++-
 quote.c                            |  61 +++------
 quote.h                            |   8 +-
 7 files changed, 372 insertions(+), 110 deletions(-)

-- 
1.8.3.GIT

[PATCH 01/15] for-each-ref, quote: convert *_quote_print -> *_quote_buf

[Ramkumar Ramachandra]

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

for-each-ref.c:print_value() currently prints values to stdout
immediately using {sq|perl|python|tcl}_quote_print, giving us no
opportunity to do any further processing.  In preparation for getting
print_value() to accept an additional strbuf argument to write to,
convert the *_quote_print functions and callers to *_quote_buf.

[rr: commit message, minor modifications]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/for-each-ref.c | 13 +++++++++----
 quote.c                | 44 ++++++++++++++++++++++----------------------
 quote.h                |  6 +++---
 3 files changed, 34 insertions(+), 29 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 7f059c3..1d4083c 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -867,24 +867,29 @@ 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 = STRBUF_INIT;
 	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 911229f..8c294df 100644
--- a/quote.c
+++ b/quote.c
@@ -463,72 +463,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.3.GIT

[PATCH 02/15] for-each-ref: don't print out elements directly

[Ramkumar Ramachandra]

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

Currently, the entire callchain starting from show_ref() parses and
prints immediately.  This inflexibility limits our ability to extend the
parser.  So, convert the entire callchain to accept a strbuf argument to
write to.  Also introduce a show_refs() helper that calls show_ref() in
a loop to avoid cluttering up cmd_for_each_ref() with the task of
initializing/freeing the strbuf.

[rr: commit message]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/for-each-ref.c | 55 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 1d4083c..e2d6c5a 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -864,31 +864,31 @@ 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 = STRBUF_INIT;
 	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);
+		sq_quote_buf(sb, v->s);
 		break;
 	case QUOTE_PERL:
-		perl_quote_buf(&sb, v->s);
+		perl_quote_buf(sb, v->s);
 		break;
 	case QUOTE_PYTHON:
-		python_quote_buf(&sb, v->s);
+		python_quote_buf(sb, v->s);
 		break;
 	case QUOTE_TCL:
-		tcl_quote_buf(&sb, v->s);
+		tcl_quote_buf(sb, v->s);
 		break;
 	}
 	if (quote_style != QUOTE_NONE) {
-		fputs(sb.buf, stdout);
-		strbuf_release(&sb);
+		fputs(sb->buf, stdout);
+		strbuf_release(sb);
 	}
 }
 
@@ -910,7 +910,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 == '%') {
@@ -919,32 +919,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);
 	}
-	putchar('\n');
+	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);
+	}
+	strbuf_release(&sb);
 }
 
 static struct ref_sort *default_sort(void)
@@ -987,7 +1002,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;
@@ -1041,7 +1056,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.3.GIT

[PATCH 03/15] tar-tree: remove dependency on sq_quote_print()

[Ramkumar Ramachandra]

Currently, there is exactly one caller of sq_quote_print(), namely
cmd_tar_tree().  In the interest of removing sq_quote_print() and
simplification, replace it with an equivalent call to sq_quote_argv().
No functional changes intended.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/tar-tree.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/tar-tree.c b/builtin/tar-tree.c
index 3f1e701..ba3ffe6 100644
--- a/builtin/tar-tree.c
+++ b/builtin/tar-tree.c
@@ -26,8 +26,8 @@ int cmd_tar_tree(int argc, const char **argv, const char *prefix)
 	 * $0 tree-ish basedir ==>
 	 * 	git archive --format-tar --prefix=basedir tree-ish
 	 */
-	int i;
 	const char **nargv = xcalloc(sizeof(*nargv), argc + 3);
+	struct strbuf sb = STRBUF_INIT;
 	char *basedir_arg;
 	int nargc = 0;
 
@@ -65,11 +65,10 @@ int cmd_tar_tree(int argc, const char **argv, const char *prefix)
 	fprintf(stderr,
 		"*** \"git tar-tree\" is now deprecated.\n"
 		"*** Running \"git archive\" instead.\n***");
-	for (i = 0; i < nargc; i++) {
-		fputc(' ', stderr);
-		sq_quote_print(stderr, nargv[i]);
-	}
-	fputc('\n', stderr);
+	sq_quote_argv(&sb, nargv, 0);
+	strbuf_addch(&sb, '\n');
+	fputs(sb.buf, stderr);
+	strbuf_release(&sb);
 	return cmd_archive(nargc, nargv, prefix);
 }
 
-- 
1.8.3.GIT

[PATCH 04/15] quote: remove sq_quote_print()

[Ramkumar Ramachandra]

Remove sq_quote_print() since it has no callers.  A nicer alternative
sq_quote_buf() exists: its callers aren't forced to print immediately.

For historical context, sq_quote_print() was first introduced in
575ba9d6 (GIT_TRACE: show which built-in/external commands are executed,
2006-06-25) for the purpose of printing argv for $GIT_TRACE.  Today, we
achieve this using trace_argv_printf() -> sq_quote_argv() ->
sq_quote_buf(), which ultimately fills in a strbuf.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 quote.c | 17 -----------------
 quote.h |  2 --
 2 files changed, 19 deletions(-)

diff --git a/quote.c b/quote.c
index 8c294df..778b39a 100644
--- a/quote.c
+++ b/quote.c
@@ -42,23 +42,6 @@ void sq_quote_buf(struct strbuf *dst, const char *src)
 	free(to_free);
 }
 
-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);
-}
-
 void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)
 {
 	int i;
diff --git a/quote.h b/quote.h
index ed06df5..251f3cc 100644
--- a/quote.h
+++ b/quote.h
@@ -27,8 +27,6 @@ struct strbuf;
  * excluding the final null regardless of the buffer size.
  */
 
-extern void sq_quote_print(FILE *stream, const char *src);
-
 extern void sq_quote_buf(struct strbuf *, const char *src);
 extern void sq_quote_argv(struct strbuf *, const char **argv, size_t maxlen);
 
-- 
1.8.3.GIT

[PATCH 05/15] pretty: extend pretty_print_context with callback

[Ramkumar Ramachandra]

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

The struct pretty_print_context contains the context in which the
placeholders in format_commit_one() should be parsed.  Although
format_commit_one() primarily acts as a parser, there is no way for a
caller to plug in custom callbacks.  Now, callers can:

1. Parse a custom placeholder that is not supported by
   format_commit_one(), and act on it independently of the pretty
   machinery.

2. Parse a custom placeholder to substitute the custom placeholder with
   a placeholder that format_commit_one() understands.  This is
   especially useful for supporting %>(*), where * is substituted with a
   length computed by the caller.

To support these two usecases, the interface for the function looks
like:

typedef size_t (*format_message_fn)(struct strbuf *sb,
				const char *placeholder,
				void *format_context,
				void *user_data,
				struct strbuf *placeholder_subst)

It is exactly like format_commit_one(), except that there are two
additional fields: user_data (to pass custom data to the callback), and
placeholder_subst (to set the substitution).  The callback should return
the length of the original string parsed, and optionally set
placeholder_subst.

[rr: commit message, minor modifications]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 commit.h |  8 ++++++++
 pretty.c | 25 +++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/commit.h b/commit.h
index 67bd509..04bd935 100644
--- a/commit.h
+++ b/commit.h
@@ -78,6 +78,12 @@ enum cmit_fmt {
 	CMIT_FMT_UNSPECIFIED
 };
 
+typedef size_t (*format_message_fn)(struct strbuf *sb,
+				const char *placeholder,
+				void *format_context,
+				void *user_data,
+				struct strbuf *placeholder_subst);
+
 struct pretty_print_context {
 	enum cmit_fmt fmt;
 	int abbrev;
@@ -92,6 +98,8 @@ struct pretty_print_context {
 	const char *output_encoding;
 	struct string_list *mailmap;
 	int color;
+	format_message_fn format;
+	void *user_data;
 };
 
 struct userformat_want {
diff --git a/pretty.c b/pretty.c
index 9e43154..095e5ba 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1069,6 +1069,31 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	struct commit_list *p;
 	int h1, h2;
 
+	if (c->pretty_ctx->format) {
+		struct strbuf subst = STRBUF_INIT;
+		int ret = c->pretty_ctx->format(sb, placeholder, context,
+						c->pretty_ctx->user_data,
+						&subst);
+		if (ret && subst.len) {
+			/*
+			 * Something was parsed by format(), and a
+			 * placeholder-substitution was set.
+			 * Recursion is required to override the
+			 * return value of format_commit_one() with
+			 * ret: the length of the original string
+			 * before substitution.
+			 */
+			ret = format_commit_one(sb, subst.buf, context) ? ret : 0;
+			strbuf_release(&subst);
+			return ret;
+		} else if (ret)
+			/*
+			 * Something was parsed by format(), but
+			 * no placeholder-substitution was set.
+			 */
+			return ret;
+	}
+
 	/* these are independent of the commit */
 	switch (placeholder[0]) {
 	case 'C':
-- 
1.8.3.GIT

[PATCH 06/15] pretty: limit recursion in format_commit_one()

[Ramkumar Ramachandra]

To make sure that a pretty_ctx->format substitution doesn't result in an
infinite recursion, change the prototype of format_commit_one() to
accept one last argument: no_recurse.  So, a single substitution by
format() must yield a result that can be parsed by format_commit_one()
without the help of format().

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 pretty.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/pretty.c b/pretty.c
index 095e5ba..0063f2d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1061,7 +1061,8 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
 
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
-				void *context)
+				void *context,
+				int no_recurse)
 {
 	struct format_commit_context *c = context;
 	const struct commit *commit = c->commit;
@@ -1069,7 +1070,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	struct commit_list *p;
 	int h1, h2;
 
-	if (c->pretty_ctx->format) {
+	if (!no_recurse && c->pretty_ctx->format) {
 		struct strbuf subst = STRBUF_INIT;
 		int ret = c->pretty_ctx->format(sb, placeholder, context,
 						c->pretty_ctx->user_data,
@@ -1083,7 +1084,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 			 * ret: the length of the original string
 			 * before substitution.
 			 */
-			ret = format_commit_one(sb, subst.buf, context) ? ret : 0;
+			ret = format_commit_one(sb, subst.buf, context, 1) ? ret : 0;
 			strbuf_release(&subst);
 			return ret;
 		} else if (ret)
@@ -1332,7 +1333,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
 	}
 	while (1) {
 		int modifier = *placeholder == 'C';
-		int consumed = format_commit_one(&local_sb, placeholder, c);
+		int consumed = format_commit_one(&local_sb, placeholder, c, 0);
 		total_consumed += consumed;
 
 		if (!modifier)
@@ -1452,7 +1453,7 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
 	if (((struct format_commit_context *)context)->flush_type != no_flush)
 		consumed = format_and_pad_commit(sb, placeholder, context);
 	else
-		consumed = format_commit_one(sb, placeholder, context);
+		consumed = format_commit_one(sb, placeholder, context, 0);
 	if (magic == NO_MAGIC)
 		return consumed;
 
-- 
1.8.3.GIT

[PATCH 07/15] pretty: allow passing NULL commit to format_commit_message()

[Ramkumar Ramachandra]

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

The new formatter, for-each-ref, may use non-commit placeholders only.
While it could audit the format line and warn/exclude commit
placeholders, that's a lot more work than simply ignore them.
Unrecognized placeholders are displayed as-is, pretty obvious that they
are not handled.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 pretty.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/pretty.c b/pretty.c
index 0063f2d..816aa32 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1156,6 +1156,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	}
 
 	/* these depend on the commit */
+	if (!commit)
+		return 0;
+
 	if (!commit->object.parsed)
 		parse_object(commit->object.sha1);
 
@@ -1276,6 +1279,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	}
 
 
+	if (!c->message)
+		return 0;
+
 	/* For the rest we have to parse the commit header. */
 	if (!c->commit_header_parsed)
 		parse_commit_header(c);
@@ -1510,9 +1516,10 @@ void format_commit_message(const struct commit *commit,
 	context.commit = commit;
 	context.pretty_ctx = pretty_ctx;
 	context.wrap_start = sb->len;
-	context.message = logmsg_reencode(commit,
-					  &context.commit_encoding,
-					  output_enc);
+	if (commit)
+		context.message = logmsg_reencode(commit,
+						  &context.commit_encoding,
+						  output_enc);
 
 	strbuf_expand(sb, format, format_commit_item, &context);
 	rewrap_message_tail(sb, &context, 0, 0, 0);
@@ -1535,7 +1542,8 @@ void format_commit_message(const struct commit *commit,
 	}
 
 	free(context.commit_encoding);
-	logmsg_free(context.message, commit);
+	if (commit)
+		logmsg_free(context.message, commit);
 	free(context.signature_check.gpg_output);
 	free(context.signature_check.signer);
 }
-- 
1.8.3.GIT

[PATCH 08/15] for-each-ref: get --pretty using format_commit_message

[Ramkumar Ramachandra]

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

--format is very limited in its capabilities.  Introduce --pretty, which
extends the existing --format with pretty-formats.  In --pretty:

 - Existing --format %(atom) is available. They also accept some pretty
   magic.  For example, you can use "% (atom)" to only display a leading
   space if the atom produces something.

 - %ab to display a hex character 0xab is not available as it may
   conflict with other pretty's placeholders.  Use %xab instead.

 - Many pretty placeholders are designed to work on commits.  While some
   of them should work on tags too, they don't (yet).

 - Unsupported atoms cause for-each-ref to exit early and report.
   Unsupported pretty placeholders are displayed as-is.

 - Pretty placeholders can not be used as a sorting criteria.

--format is considered deprecated. If the user hits a bug specific in
--format code, they are advised to migrate to --pretty.

[rr: documentation]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-for-each-ref.txt | 22 ++++++++++++-
 builtin/for-each-ref.c             | 67 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 85 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index f2e08d1..6135812 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -9,7 +9,8 @@ SYNOPSIS
 --------
 [verse]
 'git for-each-ref' [--count=<count>] [--shell|--perl|--python|--tcl]
-		   [(--sort=<key>)...] [--format=<format>] [<pattern>...]
+		   [(--sort=<key>)...] [--format=<format>|--pretty=<pretty>]
+		   [<pattern>...]
 
 DESCRIPTION
 -----------
@@ -47,6 +48,25 @@ OPTIONS
 	`xx`; for example `%00` interpolates to `\0` (NUL),
 	`%09` to `\t` (TAB) and `%0a` to `\n` (LF).
 
+<pretty>::
+	A format string with supporting placeholders described in the
+	"PRETTY FORMATS" section in linkgit:git-log[1].  Additionally
+	supports placeholders from `<format>`
+	(i.e. `%[*](fieldname)`).
++
+Caveats:
+
+1. Many of the placeholders in "PRETTY FORMATS" are designed to work
+   specifically on commit objects: when non-commit objects are
+   supplied, those placeholders won't work.
+
+2. Does not interpolate `%ab` (where `ab` are hex digits) with the
+   corresponding hex code.  To print a byte from a hex code, use
+   `%xab` (from pretty-formats) instead.
+
+3. Only the placeholders inherited from `<format>` will respect
+   quoting settings.
+
 <pattern>...::
 	If one or more patterns are given, only refs are shown that
 	match against at least one pattern, either using fnmatch(3) or
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index e2d6c5a..576a882 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -962,6 +962,58 @@ static void show_refs(struct refinfo **refs, int maxcount,
 	strbuf_release(&sb);
 }
 
+struct format_one_atom_context {
+	struct refinfo *info;
+	int quote_style;
+};
+
+static size_t format_one_atom(struct strbuf *sb, const char *placeholder,
+			      void *format_context, void *user_data,
+			      struct strbuf *subst)
+{
+	struct format_one_atom_context *ctx = user_data;
+	const char *ep;
+
+	if (*placeholder == '%') {
+		strbuf_addch(sb, '%');
+		return 1;
+	}
+
+	if (*placeholder != '(')
+		return 0;
+
+	ep = strchr(placeholder + 1, ')');
+	if (!ep)
+		return 0;
+	print_value(sb, ctx->info, parse_atom(placeholder + 1, ep),
+		    ctx->quote_style);
+	return ep + 1 - placeholder;
+}
+
+static void show_pretty_refs(struct refinfo **refs, int maxcount,
+			     const char *format, int quote_style)
+{
+	struct pretty_print_context ctx = {0};
+	struct format_one_atom_context fctx;
+	struct strbuf sb = STRBUF_INIT;
+	int i;
+
+	ctx.format = format_one_atom;
+	ctx.user_data = &fctx;
+	fctx.quote_style = quote_style;
+	for (i = 0; i < maxcount; i++) {
+		struct commit *commit = NULL;
+		fctx.info = refs[i];
+		if (sha1_object_info(refs[i]->objectname, NULL) == OBJ_COMMIT)
+			commit = lookup_commit(refs[i]->objectname);
+		strbuf_reset(&sb);
+		format_commit_message(commit, format, &sb, &ctx);
+		strbuf_addch(&sb, '\n');
+		fputs(sb.buf, stdout);
+	}
+	strbuf_release(&sb);
+}
+
 static struct ref_sort *default_sort(void)
 {
 	static const char cstr_name[] = "refname";
@@ -1003,7 +1055,9 @@ static char const * const for_each_ref_usage[] = {
 int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 {
 	int num_refs;
-	const char *format = "%(objectname) %(objecttype)\t%(refname)";
+	const char *default_format = "%(objectname) %(objecttype)\t%(refname)";
+	const char *format = default_format;
+	const char *pretty = NULL;
 	struct ref_sort *sort = NULL, **sort_tail = &sort;
 	int maxcount = 0, quote_style = 0;
 	struct refinfo **refs;
@@ -1022,6 +1076,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_GROUP(""),
 		OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")),
 		OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
+		OPT_STRING(  0 , "pretty", &pretty, N_("format"), N_("alternative format to use for the output")),
 		OPT_CALLBACK(0 , "sort", sort_tail, N_("key"),
 			    N_("field name to sort on"), &opt_parse_sort),
 		OPT_END(),
@@ -1036,7 +1091,10 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		error("more than one quoting style?");
 		usage_with_options(for_each_ref_usage, opts);
 	}
-	if (verify_format(format))
+	if (format != default_format && pretty)
+		die("--format and --pretty cannot be used together");
+	if ((pretty && verify_format(pretty)) ||
+	    (!pretty && verify_format(format)))
 		usage_with_options(for_each_ref_usage, opts);
 
 	if (!sort)
@@ -1057,6 +1115,9 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	if (!maxcount || num_refs < maxcount)
 		maxcount = num_refs;
 
-	show_refs(refs, maxcount, format, quote_style);
+	if (pretty)
+		show_pretty_refs(refs, maxcount, pretty, quote_style);
+	else
+		show_refs(refs, maxcount, format, quote_style);
 	return 0;
 }
-- 
1.8.3.GIT

[PATCH 09/15] for-each-ref: teach verify_format() about pretty's syntax

[Ramkumar Ramachandra]

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

Pretty format accepts either ' ', '+' or '-' after '%' and before the
placeholder name to modify certain behaviors. Teach verify_format()
about this so that it finds atom "upstream" in, for example,
'% (upstream)'. This is important because verify_format populates
used_atom, which get_value() and populate_value() later rely on.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/for-each-ref.c | 15 +++++++++------
 pretty.c               |  4 ++++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 576a882..f8a3880 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -150,7 +150,7 @@ static int parse_atom(const char *atom, const char *ep)
 /*
  * In a format string, find the next occurrence of %(atom).
  */
-static const char *find_next(const char *cp)
+static const char *find_next(const char *cp, int pretty)
 {
 	while (*cp) {
 		if (*cp == '%') {
@@ -160,6 +160,9 @@ static const char *find_next(const char *cp)
 			 */
 			if (cp[1] == '(')
 				return cp;
+			else if (pretty && cp[1] && cp[2] == '(' &&
+				 strchr(" +-", cp[1])) /* see format_commit_item() */
+				return cp + 1;
 			else if (cp[1] == '%')
 				cp++; /* skip over two % */
 			/* otherwise this is a singleton, literal % */
@@ -173,10 +176,10 @@ static const char *find_next(const char *cp)
  * Make sure the format string is well formed, and parse out
  * the used atoms.
  */
-static int verify_format(const char *format)
+static int verify_format(const char *format, int pretty)
 {
 	const char *cp, *sp;
-	for (cp = format; *cp && (sp = find_next(cp)); ) {
+	for (cp = format; *cp && (sp = find_next(cp, pretty)); ) {
 		const char *ep = strchr(sp, ')');
 		if (!ep)
 			return error("malformed format string %s", sp);
@@ -935,7 +938,7 @@ static void show_ref(struct strbuf *sb, struct refinfo *info,
 {
 	const char *cp, *sp, *ep;
 
-	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
+	for (cp = format; *cp && (sp = find_next(cp, 0)); cp = ep + 1) {
 		ep = strchr(sp, ')');
 		if (cp < sp)
 			emit(sb, cp, sp);
@@ -1093,8 +1096,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	}
 	if (format != default_format && pretty)
 		die("--format and --pretty cannot be used together");
-	if ((pretty && verify_format(pretty)) ||
-	    (!pretty && verify_format(format)))
+	if ((pretty && verify_format(pretty, 1)) ||
+	    (!pretty && verify_format(format, 0)))
 		usage_with_options(for_each_ref_usage, opts);
 
 	if (!sort)
diff --git a/pretty.c b/pretty.c
index 816aa32..28c0a72 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1439,6 +1439,10 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
 		ADD_SP_BEFORE_NON_EMPTY
 	} magic = NO_MAGIC;
 
+	/*
+	 * Note: any modification in what placeholder[0] contains
+	 * should be redone in builtin/for-each-ref.c:find_next().
+	 */
 	switch (placeholder[0]) {
 	case '-':
 		magic = DEL_LF_BEFORE_EMPTY;
-- 
1.8.3.GIT

[PATCH 10/15] for-each-ref: introduce format specifier %>(*) and %<(*)

[Ramkumar Ramachandra]

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

Pretty placeholders %>(N) and %<(N) require a user provided width N,
which makes sense because the commit chain could be really long and the
user only needs to look at a few at the top, going to the end just to
calculate the best width wastes CPU cycles.

for-each-ref is different; the display set is small, and we display them
all at once. We even support sorting, which goes through all display
items anyway.  This patch introduces new %>(*) and %<(*), which are
supposed to be followed immediately by %(fieldname) (i.e. original
for-each-ref specifiers, not ones coming from pretty.c). They calculate
the best width for the %(fieldname), ignoring ansi escape sequences if
any.

[rr: documentation]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-for-each-ref.txt |  7 +++++++
 builtin/for-each-ref.c             | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 6135812..7d6db7f 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -47,6 +47,10 @@ OPTIONS
 	are hex digits interpolates to character with hex code
 	`xx`; for example `%00` interpolates to `\0` (NUL),
 	`%09` to `\t` (TAB) and `%0a` to `\n` (LF).
++
+Placeholders `%<(*)` and `%>(*)` work like `%<(<N>)` and `%>(<N>)`
+respectively, except that the width of the next placeholder is
+calculated.
 
 <pretty>::
 	A format string with supporting placeholders described in the
@@ -67,6 +71,9 @@ Caveats:
 3. Only the placeholders inherited from `<format>` will respect
    quoting settings.
 
+3. Only the placeholders inherited from `<format>` will work with the
+   alignment placeholders `%<(*)` and '%>(*)`.
+
 <pattern>...::
 	If one or more patterns are given, only refs are shown that
 	match against at least one pattern, either using fnmatch(3) or
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index f8a3880..053a622 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -9,6 +9,7 @@
 #include "quote.h"
 #include "parse-options.h"
 #include "remote.h"
+#include "utf8.h"
 
 /* Quoting styles */
 #define QUOTE_NONE 0
@@ -966,10 +967,30 @@ static void show_refs(struct refinfo **refs, int maxcount,
 }
 
 struct format_one_atom_context {
+	struct refinfo **refs;
+	int maxcount;
+
 	struct refinfo *info;
 	int quote_style;
 };
 
+static unsigned int get_atom_width(struct format_one_atom_context *ctx,
+				   const char *start, const char *end)
+{
+	struct strbuf sb = STRBUF_INIT;
+	int i, atom = parse_atom(start, end);
+	unsigned int len = 0, sb_len;
+	for (i = 0; i < ctx->maxcount; i++) {
+		print_value(&sb, ctx->refs[i], atom, ctx->quote_style);
+		sb_len = utf8_strnwidth(sb.buf, sb.len, 1);
+		if (sb_len > len)
+			len = sb_len;
+		strbuf_reset(&sb);
+	}
+	strbuf_release(&sb);
+	return len;
+}
+
 static size_t format_one_atom(struct strbuf *sb, const char *placeholder,
 			      void *format_context, void *user_data,
 			      struct strbuf *subst)
@@ -982,6 +1003,21 @@ static size_t format_one_atom(struct strbuf *sb, const char *placeholder,
 		return 1;
 	}
 
+	/*
+	 * Substitute %>(*)%(atom) and friends with real width.
+	 */
+	if (*placeholder == '>' || *placeholder == '<') {
+		const char *star = placeholder + 1;
+		if (!prefixcmp(star, "(*)%(") &&
+		    ((ep = strchr(star + strlen("(*)%("), ')')) != NULL)) {
+			star++;
+			strbuf_addf(subst, "%c(%u)",
+				    *placeholder,
+				    get_atom_width(ctx, star + strlen("*)%("), ep));
+			return 1 + strlen("(*)");
+		}
+	}
+
 	if (*placeholder != '(')
 		return 0;
 
@@ -1003,6 +1039,8 @@ static void show_pretty_refs(struct refinfo **refs, int maxcount,
 
 	ctx.format = format_one_atom;
 	ctx.user_data = &fctx;
+	fctx.refs = refs;
+	fctx.maxcount = maxcount;
 	fctx.quote_style = quote_style;
 	for (i = 0; i < maxcount; i++) {
 		struct commit *commit = NULL;
-- 
1.8.3.GIT

[PATCH 11/15] for-each-ref: introduce %(HEAD) marker

[Ramkumar Ramachandra]

'git branch' shows which branch you are currently on with an '*', but
'git for-each-ref' misses this feature.  So, extend the format with
%(HEAD) to do exactly the same thing.

Now you can use the following format in for-each-ref:

  %C(red)%(HEAD)%C(reset) %C(green)%(refname:short)%C(reset)

to display a red asterisk next to the current ref.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-for-each-ref.txt |  4 ++++
 builtin/for-each-ref.c             | 13 +++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 7d6db7f..d529296 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -120,6 +120,10 @@ upstream::
 	from the displayed ref. Respects `:short` in the same way as
 	`refname` above.
 
+HEAD::
+	Useful to indicate the currently checked out branch.  Is '*'
+	if HEAD points to the current ref, and ' ' otherwise.
+
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
 be used to specify the value in the header field.
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 053a622..b0b8236 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -76,6 +76,7 @@ static struct {
 	{ "upstream" },
 	{ "symref" },
 	{ "flag" },
+	{ "HEAD" },
 };
 
 /*
@@ -679,8 +680,16 @@ static void populate_value(struct refinfo *ref)
 				v->s = xstrdup(buf + 1);
 			}
 			continue;
-		}
-		else
+		} else if (!strcmp(name, "HEAD")) {
+			const char *head;
+			unsigned char sha1[20];
+			head = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
+			if (!strcmp(ref->refname, head))
+				v->s = "*";
+			else
+				v->s = " ";
+			continue;
+		} else
 			continue;
 
 		formatp = strchr(name, ':');
-- 
1.8.3.GIT

[PATCH 12/15] for-each-ref: introduce %(upstream:track[short])

[Ramkumar Ramachandra]

Introduce %(upstream:track) to display "[ahead M, behind N]" and
%(upstream:trackshort) to display "=", ">", "<", or "<>"
appropriately (inspired by the contrib/completion/git-prompt.sh).

Now you can use the following format in for-each-ref:

  %C(green)%(refname:short)%C(reset)%(upstream:trackshort)

to display refs with terse tracking information.

Note that :track and :trackshort only works with upstream, and errors
out when used with anything else.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-for-each-ref.txt |  6 +++++-
 builtin/for-each-ref.c             | 42 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index d529296..05ff7ba 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -118,7 +118,11 @@ objectname::
 upstream::
 	The name of a local ref which can be considered ``upstream''
 	from the displayed ref. Respects `:short` in the same way as
-	`refname` above.
+	`refname` above.  Additionally respects `:track` to show
+	"[ahead N, behind M]" and `:trackshort` to show the terse
+	version (like the prompt) ">", "<", "<>", or "=".  Has no
+	effect if the ref does not have tracking information
+	associated with it.
 
 HEAD::
 	Useful to indicate the currently checked out branch.  Is '*'
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index b0b8236..adc965e 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -628,6 +628,7 @@ static void populate_value(struct refinfo *ref)
 	int eaten, i;
 	unsigned long size;
 	const unsigned char *tagged;
+	int upstream_present = 0;
 
 	ref->value = xcalloc(sizeof(struct atom_value), used_atom_cnt);
 
@@ -645,6 +646,7 @@ static void populate_value(struct refinfo *ref)
 		int deref = 0;
 		const char *refname;
 		const char *formatp;
+		struct branch *branch;
 
 		if (*name == '*') {
 			deref = 1;
@@ -656,7 +658,6 @@ static void populate_value(struct refinfo *ref)
 		else if (!prefixcmp(name, "symref"))
 			refname = ref->symref ? ref->symref : "";
 		else if (!prefixcmp(name, "upstream")) {
-			struct branch *branch;
 			/* only local branches may have an upstream */
 			if (prefixcmp(ref->refname, "refs/heads/"))
 				continue;
@@ -666,6 +667,7 @@ static void populate_value(struct refinfo *ref)
 			    !branch->merge[0]->dst)
 				continue;
 			refname = branch->merge[0]->dst;
+			upstream_present = 1;
 		}
 		else if (!strcmp(name, "flag")) {
 			char buf[256], *cp = buf;
@@ -683,6 +685,7 @@ static void populate_value(struct refinfo *ref)
 		} else if (!strcmp(name, "HEAD")) {
 			const char *head;
 			unsigned char sha1[20];
+
 			head = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
 			if (!strcmp(ref->refname, head))
 				v->s = "*";
@@ -695,11 +698,46 @@ static void populate_value(struct refinfo *ref)
 		formatp = strchr(name, ':');
 		/* look for "short" refname format */
 		if (formatp) {
+			int num_ours, num_theirs;
+
 			formatp++;
 			if (!strcmp(formatp, "short"))
 				refname = shorten_unambiguous_ref(refname,
 						      warn_ambiguous_refs);
-			else
+			else if (!strcmp(formatp, "track") &&
+				!prefixcmp(name, "upstream")) {
+				char buf[40];
+
+				if (!upstream_present)
+					continue;
+				if (!stat_tracking_info(branch, &num_ours, &num_theirs))
+					v->s = "";
+				else if (!num_ours) {
+					sprintf(buf, "[behind %d]", num_theirs);
+					v->s = xstrdup(buf);
+				} else if (!num_theirs) {
+					sprintf(buf, "[ahead %d]", num_ours);
+					v->s = xstrdup(buf);
+				} else {
+					sprintf(buf, "[ahead %d, behind %d]",
+						num_ours, num_theirs);
+					v->s = xstrdup(buf);
+				}
+				continue;
+			} else if (!strcmp(formatp, "trackshort") &&
+				!prefixcmp(name, "upstream")) {
+				if (!upstream_present)
+					continue;
+				if (!stat_tracking_info(branch, &num_ours, &num_theirs))
+					v->s = "=";
+				else if (!num_ours)
+					v->s = "<";
+				else if (!num_theirs)
+					v->s = ">";
+				else
+					v->s = "<>";
+				continue;
+			} else
 				die("unknown %.*s format %s",
 				    (int)(formatp - name), name, formatp);
 		}
-- 
1.8.3.GIT

[PATCH 13/15] for-each-ref: improve responsiveness of %(upstream:track)

[Ramkumar Ramachandra]

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

Before anything is printed, for-each-ref sorts all refs first.  As
part of the sorting, populate_value() is called to fill the values in
for all atoms/placeholders per entry. By the time sort_refs() is done,
pretty much all data is already retrieved.

This works fine when data can be cheaply retrieved before
%(upstream:track) comes into the picture. It may take a noticeable
amount of time to process %(upstream:track) for each entry. All
entries add up and make --format='%(refname)%(upstream:track)' seem
hung for a few seconds, then display everything at once.

Improve the responsiveness by only processing the one atom (*) at a
time so that processing one atom for all entries (e.g. sorting) won't
cause much delay (unless you choose a "heavy" atom to process).

(*) This is not entirely correct. If you sort by an atom that needs
object database access, then it will fill all atoms that need odb.
Which is not a bad thing. We don't want to access odb once at sorting
phase and again at display phase.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/for-each-ref.c | 49 +++++++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index adc965e..edaa6b2 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -565,20 +565,6 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 }
 
 /*
- * We want to have empty print-string for field requests
- * that do not apply (e.g. "authordate" for a tag object)
- */
-static void fill_missing_values(struct atom_value *val)
-{
-	int i;
-	for (i = 0; i < used_atom_cnt; i++) {
-		struct atom_value *v = &val[i];
-		if (v->s == NULL)
-			v->s = "";
-	}
-}
-
-/*
  * val is a list of atom_value to hold returned values.  Extract
  * the values for atoms in used_atom array out of (obj, buf, sz).
  * when deref is false, (obj, buf, sz) is the object that is
@@ -621,7 +607,7 @@ static inline char *copy_advance(char *dst, const char *src)
 /*
  * Parse the object referred by ref, and grab needed value.
  */
-static void populate_value(struct refinfo *ref)
+static void populate_value(struct refinfo *ref, int only_this_atom)
 {
 	void *buf;
 	struct object *obj;
@@ -630,13 +616,15 @@ static void populate_value(struct refinfo *ref)
 	const unsigned char *tagged;
 	int upstream_present = 0;
 
-	ref->value = xcalloc(sizeof(struct atom_value), used_atom_cnt);
+	if (!ref->value) {
+		ref->value = xcalloc(sizeof(struct atom_value), used_atom_cnt);
 
-	if (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {
-		unsigned char unused1[20];
-		ref->symref = resolve_refdup(ref->refname, unused1, 1, NULL);
-		if (!ref->symref)
-			ref->symref = "";
+		if (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {
+			unsigned char unused1[20];
+			ref->symref = resolve_refdup(ref->refname, unused1, 1, NULL);
+			if (!ref->symref)
+				ref->symref = "";
+		}
 	}
 
 	/* Fill in specials first */
@@ -648,6 +636,9 @@ static void populate_value(struct refinfo *ref)
 		const char *formatp;
 		struct branch *branch;
 
+		if (only_this_atom != -1 && only_this_atom != i)
+			continue;
+
 		if (*name == '*') {
 			deref = 1;
 			name++;
@@ -754,6 +745,10 @@ static void populate_value(struct refinfo *ref)
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct atom_value *v = &ref->value[i];
+
+		if (only_this_atom != -1 && only_this_atom != i)
+			continue;
+
 		if (v->s == NULL)
 			goto need_obj;
 	}
@@ -809,9 +804,15 @@ static void populate_value(struct refinfo *ref)
  */
 static void get_value(struct refinfo *ref, int atom, struct atom_value **v)
 {
-	if (!ref->value) {
-		populate_value(ref);
-		fill_missing_values(ref->value);
+	if (!ref->value || !ref->value[atom].s) {
+		populate_value(ref, atom);
+		/*
+		 * We want to have empty print-string for field
+		 * requests that do not apply (e.g. "authordate" for a
+		 * tag object)
+		 */
+		if (!ref->value[atom].s)
+			ref->value[atom].s = "";
 	}
 	*v = &ref->value[atom];
 }
-- 
1.8.3.GIT

[PATCH 14/15] pretty: introduce get_pretty_userformat

[Ramkumar Ramachandra]

This helper function is intended to be used by callers implementing
--pretty themselves; it parses pretty.* configuration variables
recursively and hands the user-defined format back to the caller.  No
builtins are supported, as CMT_FMT_* are really only useful when
displaying commits.  Callers might like to define their own builtins in
the future.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 commit.h |  1 +
 pretty.c | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/commit.h b/commit.h
index 04bd935..48424c9 100644
--- a/commit.h
+++ b/commit.h
@@ -113,6 +113,7 @@ extern char *logmsg_reencode(const struct commit *commit,
 			     const char *output_encoding);
 extern void logmsg_free(char *msg, const struct commit *commit);
 extern void get_commit_format(const char *arg, struct rev_info *);
+extern const char *get_pretty_userformat(const char *arg);
 extern const char *format_subject(struct strbuf *sb, const char *msg,
 				  const char *line_separator);
 extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);
diff --git a/pretty.c b/pretty.c
index 28c0a72..70e4e44 100644
--- a/pretty.c
+++ b/pretty.c
@@ -174,6 +174,31 @@ void get_commit_format(const char *arg, struct rev_info *rev)
 }
 
 /*
+ * Function to parse --pretty string, lookup pretty.* configuration
+ * variables and return the format string, assuming no builtin
+ * formats.  Not limited to commits, unlike get_commit_format().
+ */
+const char *get_pretty_userformat(const char *arg)
+{
+	struct cmt_fmt_map *commit_format;
+
+	if (!arg || !*arg)
+		return NULL;
+
+	if (!prefixcmp(arg, "format:") || !prefixcmp(arg, "tformat:"))
+		return xstrdup(strchr(arg, ':' + 1));
+
+	if (strchr(arg, '%'))
+		return xstrdup(arg);
+
+	commit_format = find_commit_format(arg);
+	if (!commit_format || commit_format->format != CMIT_FMT_USERFORMAT)
+		die("invalid --pretty format: %s", arg);
+
+	return xstrdup(commit_format->user_format);
+}
+
+/*
  * Generic support for pretty-printing the header
  */
 static int get_one_line(const char *msg)
-- 
1.8.3.GIT

[PATCH 15/15] for-each-ref: use get_pretty_userformat in --pretty

[Ramkumar Ramachandra]

Use get_pretty_userformat() to interpret the --pretty string.  This
means that leading you can now reference a format specified in a pretty.*
configuration variable as an argument to 'git for-each-ref --pretty='.
There are two caveats:

1. A leading "format:" or "tformat:" is automatically stripped and
   ignored.  Separator semantics are not configurable (yet).

2. No built-in formats are available.  The ones specified in
   pretty-formats (oneline, short etc) don't make sense when displaying
   refs anyway.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-for-each-ref.txt |  3 +++
 builtin/for-each-ref.c             | 16 +++++++++-------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 05ff7ba..7f3cba5 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -60,6 +60,9 @@ calculated.
 +
 Caveats:
 
+0. No built-in formats from PRETTY FORMATS (like oneline, short) are
+   available.
+
 1. Many of the placeholders in "PRETTY FORMATS" are designed to work
    specifically on commit objects: when non-commit objects are
    supplied, those placeholders won't work.
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index edaa6b2..c00ab05 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -1146,7 +1146,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	int num_refs;
 	const char *default_format = "%(objectname) %(objecttype)\t%(refname)";
 	const char *format = default_format;
-	const char *pretty = NULL;
+	const char *pretty_raw = NULL, *pretty_userformat = NULL;
 	struct ref_sort *sort = NULL, **sort_tail = &sort;
 	int maxcount = 0, quote_style = 0;
 	struct refinfo **refs;
@@ -1165,13 +1165,15 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_GROUP(""),
 		OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")),
 		OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
-		OPT_STRING(  0 , "pretty", &pretty, N_("format"), N_("alternative format to use for the output")),
+		OPT_STRING(  0 , "pretty", &pretty_raw, N_("format"), N_("alternative format to use for the output")),
 		OPT_CALLBACK(0 , "sort", sort_tail, N_("key"),
 			    N_("field name to sort on"), &opt_parse_sort),
 		OPT_END(),
 	};
 
 	parse_options(argc, argv, prefix, opts, for_each_ref_usage, 0);
+	if (pretty_raw)
+		pretty_userformat = get_pretty_userformat(pretty_raw);
 	if (maxcount < 0) {
 		error("invalid --count argument: `%d'", maxcount);
 		usage_with_options(for_each_ref_usage, opts);
@@ -1180,10 +1182,10 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		error("more than one quoting style?");
 		usage_with_options(for_each_ref_usage, opts);
 	}
-	if (format != default_format && pretty)
+	if (format != default_format && pretty_userformat)
 		die("--format and --pretty cannot be used together");
-	if ((pretty && verify_format(pretty, 1)) ||
-	    (!pretty && verify_format(format, 0)))
+	if ((pretty_userformat && verify_format(pretty_userformat, 1)) ||
+	    (!pretty_userformat && verify_format(format, 0)))
 		usage_with_options(for_each_ref_usage, opts);
 
 	if (!sort)
@@ -1204,8 +1206,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	if (!maxcount || num_refs < maxcount)
 		maxcount = num_refs;
 
-	if (pretty)
-		show_pretty_refs(refs, maxcount, pretty, quote_style);
+	if (pretty_userformat)
+		show_pretty_refs(refs, maxcount, pretty_userformat, quote_style);
 	else
 		show_refs(refs, maxcount, format, quote_style);
 	return 0;
-- 
1.8.3.GIT

Re: [PATCH 00/15] Towards a more awesome git-branch

[Duy Nguyen]

On Tue, Jun 4, 2013 at 7:35 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Hi,
>
> Duy and I have been working on this topic for some time now.  Here's a
> review candidate.

I'm still hung up one the detached HEAD thing. It's a bit quirky to
put in for-each-ref, but for-each-ref can't truly replace branch
--list until it can display detached HEAD. But I think we can finish
this part and get it in first. Should be useful for some people
already.

> Duy is currently writing code to factor out -v[v] and adding --sort,
> --count to git-branch, but I'm having a lot of difficulty working with
> a series of this size.  I'm leaning towards getting this merged before
> tackling branch (who wants to review a 40-part series?).  Especially
> after [14/15] and [15/15], git for-each-ref should be usable in its
> own right.

Agreed (a bit of concern about 14 and 15/15, though). Signed-off-by:
me for the whole series.
--
Duy

Re: [PATCH 10/15] for-each-ref: introduce format specifier %>(*) and %<(*)

[Duy Nguyen]

On Tue, Jun 4, 2013 at 7:35 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> +static unsigned int get_atom_width(struct format_one_atom_context *ctx,
> +                                  const char *start, const char *end)
> +{
> +       struct strbuf sb = STRBUF_INIT;
> +       int i, atom = parse_atom(start, end);
> +       unsigned int len = 0, sb_len;
> +       for (i = 0; i < ctx->maxcount; i++) {
> +               print_value(&sb, ctx->refs[i], atom, ctx->quote_style);
> +               sb_len = utf8_strnwidth(sb.buf, sb.len, 1);
> +               if (sb_len > len)
> +                       len = sb_len;
> +               strbuf_reset(&sb);
> +       }
> +       strbuf_release(&sb);
> +       return len;
> +}
> +

I mentioned it before and I do it again. This is not optimal. We
should cache the result of get_atom_width() after the first
calculation. I haven't done it yet because I'm still not sure if it's
worth supporting %<(*)%non-atom at this stage. Caching for atoms only
is much easier because atom is indexed. But I guess it's ok in this
shape unless you run this over hundreds of refs.
--
Duy

Re: [PATCH 15/15] for-each-ref: use get_pretty_userformat in --pretty

[Duy Nguyen]

On Tue, Jun 4, 2013 at 7:35 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Use get_pretty_userformat() to interpret the --pretty string.  This
> means that leading you can now reference a format specified in a pretty.*
> configuration variable as an argument to 'git for-each-ref --pretty='.
> There are two caveats:
>
> 1. A leading "format:" or "tformat:" is automatically stripped and
>    ignored.  Separator semantics are not configurable (yet).
>
> 2. No built-in formats are available.  The ones specified in
>    pretty-formats (oneline, short etc) don't make sense when displaying
>    refs anyway.
>

I spoke too early about the concern with this patch. At first it
looked like you replace default formats with config keys. But nope,
this is brilliant.

> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  Documentation/git-for-each-ref.txt |  3 +++
>  builtin/for-each-ref.c             | 16 +++++++++-------
>  2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 05ff7ba..7f3cba5 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -60,6 +60,9 @@ calculated.
>  +
>  Caveats:
>
> +0. No built-in formats from PRETTY FORMATS (like oneline, short) are
> +   available.
> +
>  1. Many of the placeholders in "PRETTY FORMATS" are designed to work
>     specifically on commit objects: when non-commit objects are
>     supplied, those placeholders won't work.
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index edaa6b2..c00ab05 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -1146,7 +1146,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>         int num_refs;
>         const char *default_format = "%(objectname) %(objecttype)\t%(refname)";
>         const char *format = default_format;
> -       const char *pretty = NULL;
> +       const char *pretty_raw = NULL, *pretty_userformat = NULL;
>         struct ref_sort *sort = NULL, **sort_tail = &sort;
>         int maxcount = 0, quote_style = 0;
>         struct refinfo **refs;
> @@ -1165,13 +1165,15 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>                 OPT_GROUP(""),
>                 OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")),
>                 OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
> -               OPT_STRING(  0 , "pretty", &pretty, N_("format"), N_("alternative format to use for the output")),
> +               OPT_STRING(  0 , "pretty", &pretty_raw, N_("format"), N_("alternative format to use for the output")),
>                 OPT_CALLBACK(0 , "sort", sort_tail, N_("key"),
>                             N_("field name to sort on"), &opt_parse_sort),
>                 OPT_END(),
>         };
>
>         parse_options(argc, argv, prefix, opts, for_each_ref_usage, 0);
> +       if (pretty_raw)
> +               pretty_userformat = get_pretty_userformat(pretty_raw);
>         if (maxcount < 0) {
>                 error("invalid --count argument: `%d'", maxcount);
>                 usage_with_options(for_each_ref_usage, opts);
> @@ -1180,10 +1182,10 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>                 error("more than one quoting style?");
>                 usage_with_options(for_each_ref_usage, opts);
>         }
> -       if (format != default_format && pretty)
> +       if (format != default_format && pretty_userformat)
>                 die("--format and --pretty cannot be used together");
> -       if ((pretty && verify_format(pretty, 1)) ||
> -           (!pretty && verify_format(format, 0)))
> +       if ((pretty_userformat && verify_format(pretty_userformat, 1)) ||
> +           (!pretty_userformat && verify_format(format, 0)))
>                 usage_with_options(for_each_ref_usage, opts);
>
>         if (!sort)
> @@ -1204,8 +1206,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>         if (!maxcount || num_refs < maxcount)
>                 maxcount = num_refs;
>
> -       if (pretty)
> -               show_pretty_refs(refs, maxcount, pretty, quote_style);
> +       if (pretty_userformat)
> +               show_pretty_refs(refs, maxcount, pretty_userformat, quote_style);
>         else
>                 show_refs(refs, maxcount, format, quote_style);
>         return 0;
> --
> 1.8.3.GIT
>



--
Duy

Re: [PATCH 08/15] for-each-ref: get --pretty using format_commit_message

[Eric Sunshine]

On Tue, Jun 4, 2013 at 8:35 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> [rr: documentation]
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  Documentation/git-for-each-ref.txt | 22 ++++++++++++-
>  builtin/for-each-ref.c             | 67 ++++++++++++++++++++++++++++++++++++--
>  2 files changed, 85 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index f2e08d1..6135812 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -47,6 +48,25 @@ OPTIONS
>         `xx`; for example `%00` interpolates to `\0` (NUL),
>         `%09` to `\t` (TAB) and `%0a` to `\n` (LF).
>
> +<pretty>::
> +       A format string with supporting placeholders described in the
> +       "PRETTY FORMATS" section in linkgit:git-log[1].  Additionally
> +       supports placeholders from `<format>`
> +       (i.e. `%[*](fieldname)`).
> ++
> +Caveats:
> +
> +1. Many of the placeholders in "PRETTY FORMATS" are designed to work
> +   specifically on commit objects: when non-commit objects are
> +   supplied, those placeholders won't work.

Should "won't work" be expanded upon? It's not clear if this means
that git will outright crash, or if it will abort with an appropriate
error message, or if the directive will be displayed as-is or removed
from the output.

> +2. Does not interpolate `%ab` (where `ab` are hex digits) with the
> +   corresponding hex code.  To print a byte from a hex code, use
> +   `%xab` (from pretty-formats) instead.
> +
> +3. Only the placeholders inherited from `<format>` will respect
> +   quoting settings.
> +
>  <pattern>...::
>         If one or more patterns are given, only refs are shown that
>         match against at least one pattern, either using fnmatch(3) or

Re: [PATCH 12/15] for-each-ref: introduce %(upstream:track[short])

[Eric Sunshine]

On Tue, Jun 4, 2013 at 8:35 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Introduce %(upstream:track) to display "[ahead M, behind N]" and
> %(upstream:trackshort) to display "=", ">", "<", or "<>"
> appropriately (inspired by the contrib/completion/git-prompt.sh).

Bikeshedding: s/trackshort/trackbrief/ perhaps?

> Now you can use the following format in for-each-ref:
>
>   %C(green)%(refname:short)%C(reset)%(upstream:trackshort)
>
> to display refs with terse tracking information.
>
> Note that :track and :trackshort only works with upstream, and errors

s/works/work/
s/errors/error/

> out when used with anything else.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

Re: [PATCH 15/15] for-each-ref: use get_pretty_userformat in --pretty

[Eric Sunshine]

On Tue, Jun 4, 2013 at 8:35 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Use get_pretty_userformat() to interpret the --pretty string.  This
> means that leading you can now reference a format specified in a pretty.*

s/leading// perhaps?

> configuration variable as an argument to 'git for-each-ref --pretty='.
> There are two caveats:
>
> 1. A leading "format:" or "tformat:" is automatically stripped and
>    ignored.  Separator semantics are not configurable (yet).
>
> 2. No built-in formats are available.  The ones specified in
>    pretty-formats (oneline, short etc) don't make sense when displaying
>    refs anyway.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

Re: [PATCH 02/15] for-each-ref: don't print out elements directly

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> Currently, the entire callchain starting from show_ref() parses and
> prints immediately.  This inflexibility limits our ability to extend the
> parser.  So, convert the entire callchain to accept a strbuf argument to
> write to.  Also introduce a show_refs() helper that calls show_ref() in
> a loop to avoid cluttering up cmd_for_each_ref() with the task of
> initializing/freeing the strbuf.
>
> [rr: commit message]
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  builtin/for-each-ref.c | 55 ++++++++++++++++++++++++++++++++------------------
>  1 file changed, 35 insertions(+), 20 deletions(-)
>
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 1d4083c..e2d6c5a 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -864,31 +864,31 @@ 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 = STRBUF_INIT;
>  	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);
> +		sq_quote_buf(sb, v->s);
>  		break;
>  	case QUOTE_PERL:
> -		perl_quote_buf(&sb, v->s);
> +		perl_quote_buf(sb, v->s);
>  		break;
>  	case QUOTE_PYTHON:
> -		python_quote_buf(&sb, v->s);
> +		python_quote_buf(sb, v->s);
>  		break;
>  	case QUOTE_TCL:
> -		tcl_quote_buf(&sb, v->s);
> +		tcl_quote_buf(sb, v->s);
>  		break;
>  	}
>  	if (quote_style != QUOTE_NONE) {
> -		fputs(sb.buf, stdout);
> -		strbuf_release(&sb);
> +		fputs(sb->buf, stdout);
> +		strbuf_release(sb);
>  	}
>  }

The change in the first step made some sense; if asked not to quote,
it just emits the atom with fputs() in the switch (quote_style), so
there is nothing more to do after the switch.

But this change cannot be correct.  It prints literally to sb, and
leaves the function without emitting anything nor freeing the
resource in sb.

Re: [PATCH 02/15] for-each-ref: don't print out elements directly

[Duy Nguyen]

On Wed, Jun 5, 2013 at 6:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>
>> Currently, the entire callchain starting from show_ref() parses and
>> prints immediately.  This inflexibility limits our ability to extend the
>> parser.  So, convert the entire callchain to accept a strbuf argument to
>> write to.  Also introduce a show_refs() helper that calls show_ref() in
>> a loop to avoid cluttering up cmd_for_each_ref() with the task of
>> initializing/freeing the strbuf.
>>
>> [rr: commit message]
>>
>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>> ---
>>  builtin/for-each-ref.c | 55 ++++++++++++++++++++++++++++++++------------------
>>  1 file changed, 35 insertions(+), 20 deletions(-)
>>
>> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
>> index 1d4083c..e2d6c5a 100644
>> --- a/builtin/for-each-ref.c
>> +++ b/builtin/for-each-ref.c
>> @@ -864,31 +864,31 @@ 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 = STRBUF_INIT;
>>       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);
>> +             sq_quote_buf(sb, v->s);
>>               break;
>>       case QUOTE_PERL:
>> -             perl_quote_buf(&sb, v->s);
>> +             perl_quote_buf(sb, v->s);
>>               break;
>>       case QUOTE_PYTHON:
>> -             python_quote_buf(&sb, v->s);
>> +             python_quote_buf(sb, v->s);
>>               break;
>>       case QUOTE_TCL:
>> -             tcl_quote_buf(&sb, v->s);
>> +             tcl_quote_buf(sb, v->s);
>>               break;
>>       }
>>       if (quote_style != QUOTE_NONE) {
>> -             fputs(sb.buf, stdout);
>> -             strbuf_release(&sb);
>> +             fputs(sb->buf, stdout);
>> +             strbuf_release(sb);
>>       }
>>  }
>
> The change in the first step made some sense; if asked not to quote,
> it just emits the atom with fputs() in the switch (quote_style), so
> there is nothing more to do after the switch.
>
> But this change cannot be correct.  It prints literally to sb, and
> leaves the function without emitting anything nor freeing the
> resource in sb.

Everything will/should be emitted in show_refs(), so the "if
(quote_style !=..)" block should be removed. The end result still
looks correct due to the order of fputs (in print_value and
show_refs). It only breaks down when %<(*) is used together because
then "sb" content matters. Will fix.
--
Duy

Re: [PATCH 00/15] Towards a more awesome git-branch

[Ramkumar Ramachandra]

Duy Nguyen wrote:
> I'm still hung up one the detached HEAD thing. It's a bit quirky to
> put in for-each-ref, but for-each-ref can't truly replace branch
> --list until it can display detached HEAD. But I think we can finish
> this part and get it in first. Should be useful for some people
> already.

Right.  The branch is called hot-branch; let's polish it for
inclusion, and rebase for-each-ref-pretty onto it when it's done (no
need to stall work there).

> Signed-off-by:
> me for the whole series.

Thanks.  Will add in the re-roll.

Re: [PATCH 10/15] for-each-ref: introduce format specifier %>(*) and %<(*)

[Ramkumar Ramachandra]

Duy Nguyen wrote:
> I mentioned it before and I do it again. This is not optimal.

Yeah, I'll attempt to fix this, but it's not urgent.

> But I guess it's ok in this
> shape unless you run this over hundreds of refs.

Oh, you can run over a hundred refs just fine, for scripting purposes;
but why would you want to run over a hundred refs with a pretty that
includes %>(*) or %<(*)?

Re: [PATCH 10/15] for-each-ref: introduce format specifier %>(*) and %<(*)

[Duy Nguyen]

On Wed, Jun 5, 2013 at 3:14 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Duy Nguyen wrote:
>> I mentioned it before and I do it again. This is not optimal.
>
> Yeah, I'll attempt to fix this, but it's not urgent.

Agreed it's not urgent.

>> But I guess it's ok in this
>> shape unless you run this over hundreds of refs.
>
> Oh, you can run over a hundred refs just fine, for scripting purposes;
> but why would you want to run over a hundred refs with a pretty that
> includes %>(*) or %<(*)?

Good point. "git for-each-ref|wc -l" on my git repo says I have 673
refs. A naive use for-each-ref --pretty without any ref patterns could
happen. But I guess people will quickly learn to limit the ref
selection soon after doing that :-)
--
Duy

Re: [PATCH 08/15] for-each-ref: get --pretty using format_commit_message

[Duy Nguyen]

On Wed, Jun 5, 2013 at 4:12 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> +Caveats:
>> +
>> +1. Many of the placeholders in "PRETTY FORMATS" are designed to work
>> +   specifically on commit objects: when non-commit objects are
>> +   supplied, those placeholders won't work.
>
> Should "won't work" be expanded upon? It's not clear if this means
> that git will outright crash, or if it will abort with an appropriate
> error message, or if the directive will be displayed as-is or removed
> from the output.

It will be displayed as-is but that's a bit inconsistent: %(unknown)
prints error and aborts while %unknown simply produces %unknown. The
latter is how "git log --format" does it. But I think we could make
for-each-ref --pretty to do the former for %unknown. It'll be
consistent with %(unknown) and we do not need to elaborate much (it's
pretty obvious when it does not work).
--
Duy

Re: [PATCH 08/15] for-each-ref: get --pretty using format_commit_message

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Jun 5, 2013 at 4:12 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> +Caveats:
>>> +
>>> +1. Many of the placeholders in "PRETTY FORMATS" are designed to work
>>> +   specifically on commit objects: when non-commit objects are
>>> +   supplied, those placeholders won't work.
>>
>> Should "won't work" be expanded upon? It's not clear if this means
>> that git will outright crash, or if it will abort with an appropriate
>> error message, or if the directive will be displayed as-is or removed
>> from the output.
>
> It will be displayed as-is but that's a bit inconsistent: %(unknown)
> prints error and aborts while %unknown simply produces %unknown. The
> latter is how "git log --format" does it. But I think we could make
> for-each-ref --pretty to do the former for %unknown. It'll be
> consistent with %(unknown) and we do not need to elaborate much (it's
> pretty obvious when it does not work).

The Caveat Eric is asking about talks about "what happens to a
%(field) that only makes sense for a commit when showing a ref
pointing at a non-commit?", but you are answering "what happend to a
%(invalidfield) that is not defined", aren't you?

IIRC, the reason we show literal from "log --format" is to make it
easier for the person who misspelt %placeholder to spot it in the
output, and also make it easier for the person who use %placeholder
meant for newer versions of Git with an older one.  It would be a
bit unnice to die for the latter, especially if the format string is
in a script or something.

To "log --format", all input objects are expected to be commits, so
it does not have the "what does %(authordate) give when given a blob"
issue.

But for "for-each-ref --format", it is perfectly normal that you may
feed a non-commit; it makes the mechanism unusable if you errored
out %(authordate) when showing a ref that points at a tag, doesn't
it?  Substituting an inapplicable placeholder with an empty string
would be an easies way out, unless it learns a flexible/elaborate
conditional formatting mechanism, I would think.

Re: [PATCH 08/15] for-each-ref: get --pretty using format_commit_message

[Duy Nguyen]

On Thu, Jun 6, 2013 at 12:09 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Wed, Jun 5, 2013 at 4:12 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> +Caveats:
>>>> +
>>>> +1. Many of the placeholders in "PRETTY FORMATS" are designed to work
>>>> +   specifically on commit objects: when non-commit objects are
>>>> +   supplied, those placeholders won't work.
>>>
>>> Should "won't work" be expanded upon? It's not clear if this means
>>> that git will outright crash, or if it will abort with an appropriate
>>> error message, or if the directive will be displayed as-is or removed
>>> from the output.
>>
>> It will be displayed as-is but that's a bit inconsistent: %(unknown)
>> prints error and aborts while %unknown simply produces %unknown. The
>> latter is how "git log --format" does it. But I think we could make
>> for-each-ref --pretty to do the former for %unknown. It'll be
>> consistent with %(unknown) and we do not need to elaborate much (it's
>> pretty obvious when it does not work).
>
> The Caveat Eric is asking about talks about "what happens to a
> %(field) that only makes sense for a commit when showing a ref
> pointing at a non-commit?", but you are answering "what happend to a
> %(invalidfield) that is not defined", aren't you?

Because %(field) is treated like %(invalidfield) in this case. I'm not
saying this is the best thing to do though.

> IIRC, the reason we show literal from "log --format" is to make it
> easier for the person who misspelt %placeholder to spot it in the
> output, and also make it easier for the person who use %placeholder
> meant for newer versions of Git with an older one.  It would be a
> bit unnice to die for the latter, especially if the format string is
> in a script or something.

That makes more sense for for-each-ref than log because for-each-ref
is a plumbing and should support scripting. But old for-each-ref will
happily reject %(newplacholder) right away. Should we take this
opportunity to change this behavior in for-each-ref too?

> To "log --format", all input objects are expected to be commits, so
> it does not have the "what does %(authordate) give when given a blob"
> issue.
>
> But for "for-each-ref --format", it is perfectly normal that you may
> feed a non-commit; it makes the mechanism unusable if you errored
> out %(authordate) when showing a ref that points at a tag, doesn't
> it?  Substituting an inapplicable placeholder with an empty string
> would be an easies way out, unless it learns a flexible/elaborate
> conditional formatting mechanism, I would think.

There is a blurred line here. %H (or %h) should produce an object name
even for tags or blobs, but right now it produces "%H" instead. The
same applies for %ad and friends, but these are clearly for commits
and should probably behave like %(authordate), i.e. producing empty
string.
--
Duy

[PATCH 03/15] tar-tree: remove dependency on sq_quote_print()

[Ramkumar Ramachandra]

Currently, there is exactly one caller of sq_quote_print(), namely
cmd_tar_tree().  In the interest of removing sq_quote_print() and
simplification, replace it with an equivalent call to sq_quote_argv().
No functional changes intended.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/tar-tree.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/tar-tree.c b/builtin/tar-tree.c
index 3f1e701..ba3ffe6 100644
--- a/builtin/tar-tree.c
+++ b/builtin/tar-tree.c
@@ -26,8 +26,8 @@ int cmd_tar_tree(int argc, const char **argv, const char *prefix)
 	 * $0 tree-ish basedir ==>
 	 * 	git archive --format-tar --prefix=basedir tree-ish
 	 */
-	int i;
 	const char **nargv = xcalloc(sizeof(*nargv), argc + 3);
+	struct strbuf sb = STRBUF_INIT;
 	char *basedir_arg;
 	int nargc = 0;
 
@@ -65,11 +65,10 @@ int cmd_tar_tree(int argc, const char **argv, const char *prefix)
 	fprintf(stderr,
 		"*** \"git tar-tree\" is now deprecated.\n"
 		"*** Running \"git archive\" instead.\n***");
-	for (i = 0; i < nargc; i++) {
-		fputc(' ', stderr);
-		sq_quote_print(stderr, nargv[i]);
-	}
-	fputc('\n', stderr);
+	sq_quote_argv(&sb, nargv, 0);
+	strbuf_addch(&sb, '\n');
+	fputs(sb.buf, stderr);
+	strbuf_release(&sb);
 	return cmd_archive(nargc, nargv, prefix);
 }
 
-- 
1.8.3.2.736.g869de25