threads / patch / 34024

patch, 15 partsTowards a more awesome git-branch

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

## tl;dr

30 messages between Jun 4, 2013 and Jun 6, 2013. Diffs are folded; open one to read it.

replies: 29people: 4as markdown or json

Ramkumar Ramachandra· Jun 4, 2013, 12:35 UTC · lore
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
Ramkumar Ramachandra· Jun 4, 2013, 12:35 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 3 files +34 −29

builtin/for-each-ref.c, quote.c, quote.h

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
Ramkumar Ramachandra· Jun 4, 2013, 12:35 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to builtin/for-each-ref.c +35 −20
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
Junio C Hamano· Jun 4, 2013, 23:13 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 60 quoted lines
> 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.

Duy Nguyen· Jun 4, 2013, 23:44 UTC · re: Junio C Hamano · lore

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

On Wed, Jun 5, 2013 at 6:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 70 quoted lines
> 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

Ramkumar Ramachandra· Jun 4, 2013, 12:35 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to builtin/tar-tree.c +5 −6
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
Ramkumar Ramachandra· Jun 4, 2013, 12:35 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 2 files +0 −19

quote.c, quote.h

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
Ramkumar Ramachandra· Jun 4, 2013, 12:35 UTC · re: Ramkumar Ramachandra · lore

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

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(+)
Show changes to 2 files +33 −0

commit.h, pretty.c

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
Ramkumar Ramachandra· Jun 4, 2013, 12:35 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to pretty.c +6 −5
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
Ramkumar Ramachandra· Jun 4, 2013, 12:35 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to pretty.c +12 −4
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
Ramkumar Ramachandra· Jun 4, 2013, 12:35 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 2 files +85 −4

Documentation/git-for-each-ref.txt, builtin/for-each-ref.c

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
Eric Sunshine· Jun 4, 2013, 21:12 UTC · re: Ramkumar Ramachandra · lore

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

On Tue, Jun 4, 2013 at 8:35 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
Show 29 quoted lines
> 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.

Show 10 quoted lines
> +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
Duy Nguyen· Jun 5, 2013, 13:21 UTC · re: Eric Sunshine · lore

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

On Wed, Jun 5, 2013 at 4:12 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 10 quoted lines
>> +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

Junio C Hamano· Jun 5, 2013, 17:09 UTC · re: Duy Nguyen · lore

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

Duy Nguyen <pclouds@gmail.com> writes:
Show 18 quoted lines
> 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.

Duy Nguyen· Jun 6, 2013, 00:07 UTC · re: Junio C Hamano · lore

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

On Thu, Jun 6, 2013 at 12:09 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
> 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.

Show 6 quoted lines
> 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?

Show 10 quoted lines
> 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

Ramkumar Ramachandra· Jun 4, 2013, 12:35 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 2 files +13 −6

builtin/for-each-ref.c, pretty.c

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
Ramkumar Ramachandra· Jun 4, 2013, 12:35 UTC · re: Ramkumar Ramachandra · lore

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

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(+)
Show changes to 2 files +45 −0

Documentation/git-for-each-ref.txt, builtin/for-each-ref.c

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
Duy Nguyen· Jun 4, 2013, 12:54 UTC · re: Ramkumar Ramachandra · lore

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

On Tue, Jun 4, 2013 at 7:35 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
Show 17 quoted lines
> +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

Ramkumar Ramachandra· Jun 5, 2013, 08:14 UTC · re: Duy Nguyen · lore

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

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 %<(*)?

Duy Nguyen· Jun 5, 2013, 10:15 UTC · re: Ramkumar Ramachandra · lore

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

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.
Show 6 quoted lines
>> 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

Ramkumar Ramachandra· Jun 4, 2013, 12:35 UTC · re: Ramkumar Ramachandra · lore

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

'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(-)
Show changes to 2 files +15 −2

Documentation/git-for-each-ref.txt, builtin/for-each-ref.c

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
Ramkumar Ramachandra· Jun 4, 2013, 12:35 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 2 files +45 −3

Documentation/git-for-each-ref.txt, builtin/for-each-ref.c

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
Eric Sunshine· Jun 4, 2013, 21:14 UTC · re: Ramkumar Ramachandra · lore

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

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?
Show 7 quoted lines
> 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>
Ramkumar Ramachandra· Jun 4, 2013, 12:35 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to builtin/for-each-ref.c +25 −24
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
Ramkumar Ramachandra· Jun 4, 2013, 12:35 UTC · re: Ramkumar Ramachandra · lore

[PATCH 14/15] pretty: introduce get_pretty_userformat

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(+)
Show changes to 2 files +26 −0

commit.h, pretty.c

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
Ramkumar Ramachandra· Jun 4, 2013, 12:35 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 2 files +12 −7

Documentation/git-for-each-ref.txt, builtin/for-each-ref.c

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 Nguyen· Jun 4, 2013, 12:57 UTC · re: Ramkumar Ramachandra · lore

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

On Tue, Jun 4, 2013 at 7:35 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
Show 12 quoted lines
> 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.

Show 78 quoted lines
> 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

Eric Sunshine· Jun 4, 2013, 21:15 UTC · re: Ramkumar Ramachandra · lore

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

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?
Show 11 quoted lines
> 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>
Duy Nguyen· Jun 4, 2013, 12:49 UTC · re: Ramkumar Ramachandra · lore

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

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.

Show 6 quoted lines
> 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

Ramkumar Ramachandra· Jun 5, 2013, 08:11 UTC · re: Duy Nguyen · lore

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

Duy Nguyen wrote:
Show 5 quoted lines
> 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.

← back to recent threads