[PATCH v12 00/13] port tag.c to use ref-filter.c APIs

[PATCH v12 00/13] port tag.c to use ref-filter.c APIs

[Karthik Nayak]

This is part of my GSoC project to "unify the code used by branch -l, 
tag -l, and for-each-ref". This series ports over tag.c to use ref-filter
APIs.

Version 11 was posted here:
http://article.gmane.org/gmane.comp.version-control.git/275997

Changes in this version:
* Small style and formatting changes.
* Remove unnecessary variable from push_new_state().
* pop_state doesn't return a value now and attaches the buffer 
  into the previous state.
* use strcmp() rather than starts_with() while checking for 
  alignment position.
* Change attend to at_end

Karthik Nayak (13):
  ref-filter: move `struct atom_value` to ref-filter.c
  ref-filter: introduce ref_formatting_state
  ref-filter: introduce the ref_formatting_state stack machinery
  utf8: add function to align a string into given strbuf
  ref-filter: implement an `align` atom
  ref-filter: add option to filter out tags, branches and remotes
  ref-filter: support printing N lines from tag annotation
  ref-filter: add support to sort by version
  ref-filter: add option to match literal pattern
  tag.c: use 'ref-filter' data structures
  tag.c: use 'ref-filter' APIs
  tag.c: implement '--format' option
  tag.c: implement '--merged' and '--no-merged' options

 Documentation/git-for-each-ref.txt |  11 ++
 Documentation/git-tag.txt          |  34 +++-
 builtin/for-each-ref.c             |   3 +-
 builtin/tag.c                      | 367 +++++++------------------------------
 ref-filter.c                       | 312 +++++++++++++++++++++++++++----
 ref-filter.h                       |  32 +++-
 refs.c                             |   9 +
 refs.h                             |   1 +
 t/t6302-for-each-ref-filter.sh     |  84 +++++++++
 t/t7004-tag.sh                     |  51 +++++-
 utf8.c                             |  21 +++
 utf8.h                             |  15 ++
 12 files changed, 577 insertions(+), 363 deletions(-)


Interdiff:

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 3099631..760d719 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -128,8 +128,8 @@ color::
 	are described in `color.branch.*`.
 
 align::
-	left-, middle-, or right-align the content between %(align:..)
-	and %(end). Followed by `:<position>,<width>`, where the
+	Left-, middle-, or right-align the content between %(align:..)
+	and %(end). Followed by `:<width>>,<position>`, where the
 	`<position>` is either left, right or middle and `<width>` is
 	the total length of the content with alignment. If the
 	contents length is more than the width then no alignment is
diff --git a/ref-filter.c b/ref-filter.c
index a6086b2..665221b 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -66,9 +66,9 @@ struct align {
 };
 
 struct ref_formatting_state {
-	struct strbuf output;
 	struct ref_formatting_state *prev;
-	void (*attend)(struct ref_formatting_state *state);
+	struct strbuf output;
+	void (*at_end)(struct ref_formatting_state *state);
 	void *cb_data;
 	int quote_style;
 };
@@ -149,24 +149,25 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 	return at;
 }
 
-static struct ref_formatting_state *push_new_state(struct ref_formatting_state **state)
+static void push_new_state(struct ref_formatting_state **stack)
 {
-	struct ref_formatting_state *new = xcalloc(1, sizeof(struct ref_formatting_state));
-	struct ref_formatting_state *old = *state;
+	struct ref_formatting_state *s = xcalloc(1, sizeof(struct ref_formatting_state));
 
-	*state = new;
-	new->prev = old;
-	return new;
+	strbuf_init(&s->output, 0);
+	s->prev = *stack;
+	*stack = s;
 }
 
-static void pop_state(struct ref_formatting_state **state)
+static void pop_state(struct ref_formatting_state **stack)
 {
-	struct ref_formatting_state *current = *state;
+	struct ref_formatting_state *current = *stack;
 	struct ref_formatting_state *prev = current->prev;
 
+	if (prev)
+		strbuf_addbuf(&prev->output, &current->output);
 	strbuf_release(&current->output);
 	free(current);
-	*state = prev;
+	*stack = prev;
 }
 
 /*
@@ -641,29 +642,31 @@ static inline char *copy_advance(char *dst, const char *src)
 
 static void align_handler(struct ref_formatting_state *state)
 {
-	struct strbuf aligned = STRBUF_INIT;
-	struct ref_formatting_state *return_to = state->prev;
 	struct align *align = (struct align *)state->cb_data;
+	struct strbuf s = STRBUF_INIT;
 
-	strbuf_utf8_align(&aligned, align->position, align->width, state->output.buf);
-	strbuf_addbuf(&return_to->output, &aligned);
-	strbuf_release(&aligned);
+	strbuf_utf8_align(&s, align->position, align->width, state->output.buf);
+	strbuf_reset(&state->output);
+	strbuf_addbuf(&state->output, &s);
+	free(align);
 }
 
 static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)
 {
-	struct ref_formatting_state *new = push_new_state(state);
-	strbuf_init(&new->output, 0);
-	new->attend = align_handler;
+	struct ref_formatting_state *new;
+
+	push_new_state(state);
+	new = *state;
+	new->at_end = align_handler;
 	new->cb_data = atomv->align;
 }
 
 static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)
 {
 	struct ref_formatting_state *current = *state;
-	if (!current->attend)
+	if (!current->at_end)
 		die(_("format: `end` atom used without a supporting atom"));
-	current->attend(current);
+	current->at_end(current);
 	pop_state(state);
 }
 
@@ -771,11 +774,11 @@ static void populate_value(struct ref_array_item *ref)
 			if (strtoul_ui(valp, 10, &align->width))
 				die(_("positive width expected align:%s"), valp);
 
-			if (!ep || starts_with(ep + 1, "left"))
+			if (!ep || !strcmp(ep + 1, "left"))
 				align->position = ALIGN_LEFT;
-			else if (starts_with(ep + 1, "right"))
+			else if (!strcmp(ep + 1, "right"))
 				align->position = ALIGN_RIGHT;
-			else if (starts_with(ep + 1, "middle"))
+			else if (!strcmp(ep + 1, "middle"))
 				align->position = ALIGN_MIDDLE;
 			else
 				die(_("improper format entered align:%s"), ep + 1);
@@ -1305,7 +1308,7 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 
 	/*  Simple per-ref filtering */
 	if (type & FILTER_REFS_INCLUDE_BROKEN) {
-		type -= FILTER_REFS_INCLUDE_BROKEN;
+		type &= ~FILTER_REFS_INCLUDE_BROKEN;
 		broken = 1;
 	}
 
@@ -1484,7 +1487,7 @@ void show_ref_array_item(struct ref_array_item *info, const char *format,
 	struct strbuf *final_buf;
 	struct ref_formatting_state *state = NULL;
 
-	state = push_new_state(&state);
+	push_new_state(&state);
 	state->quote_style = quote_style;
 
 	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
diff --git a/ref-filter.h b/ref-filter.h
index 72fdf9e..8241066 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -5,7 +5,6 @@
 #include "refs.h"
 #include "commit.h"
 #include "parse-options.h"
-#include "utf8.h"
 
 /* Quoting styles */
 #define QUOTE_NONE 0
diff --git a/refs.c b/refs.c
index 0f18c34..a92bba1 100644
--- a/refs.c
+++ b/refs.c
@@ -2147,9 +2147,11 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data)
 
 int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data)
 {
+	unsigned int flag = 0;
+
 	if (broken)
-		broken = DO_FOR_EACH_INCLUDE_BROKEN;
-	return do_for_each_ref(&ref_cache, type, fn, 0, broken, cb_data);
+		flag = DO_FOR_EACH_INCLUDE_BROKEN;
+	return do_for_each_ref(&ref_cache, type, fn, 0, flag, cb_data);
 }
 
 int head_ref_namespaced(each_ref_fn fn, void *cb_data)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 5b73539..335396e 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1462,7 +1462,7 @@ test_expect_success 'invalid sort parameter on command line' '
 
 test_expect_success 'invalid sort parameter in configuratoin' '
 	git config tag.sort "v:notvalid" &&
-	test_must_fail git tag -l "foo*" >actual
+	test_must_fail git tag -l "foo*"
 '
 
 test_expect_success 'version sort with prerelease reordering' '
diff --git a/utf8.c b/utf8.c
index 0fb8e9d..00e10c8 100644
--- a/utf8.c
+++ b/utf8.c
@@ -660,7 +660,7 @@ void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int wid
 	if (position == ALIGN_LEFT)
 		strbuf_addf(buf, "%-*s", width + utf8_compensation, s);
 	else if (position == ALIGN_MIDDLE) {
-		int left = (width - display_len)/2;
+		int left = (width - display_len) / 2;
 		strbuf_addf(buf, "%*s%-*s", left, "", width - left + utf8_compensation, s);
 	} else if (position == ALIGN_RIGHT)
 		strbuf_addf(buf, "%*s", width + utf8_compensation, s);


-- 
2.5.0

[PATCH v12 01/13] ref-filter: move `struct atom_value` to ref-filter.c

[Karthik Nayak]

Since atom_value is only required for the internal working of
ref-filter it doesn't belong in the public header.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c | 5 +++++
 ref-filter.h | 5 +----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 46963a5..e53c77e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -55,6 +55,11 @@ static struct {
 	{ "color" },
 };
 
+struct atom_value {
+	const char *s;
+	unsigned long ul; /* used for sorting when not FIELD_STR */
+};
+
 /*
  * An atom is a valid field atom listed above, possibly prefixed with
  * a "*" to denote deref_tag().
diff --git a/ref-filter.h b/ref-filter.h
index 6bf27d8..45026d0 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -16,10 +16,7 @@
 #define FILTER_REFS_INCLUDE_BROKEN 0x1
 #define FILTER_REFS_ALL 0x2
 
-struct atom_value {
-	const char *s;
-	unsigned long ul; /* used for sorting when not FIELD_STR */
-};
+struct atom_value;
 
 struct ref_sorting {
 	struct ref_sorting *next;
-- 
2.5.0

[PATCH v12 02/13] ref-filter: introduce ref_formatting_state

[Karthik Nayak]

Introduce ref_formatting_state which will hold the formatted output
strbuf instead of directly printing to stdout. This will help us in
creating modifier atoms which modify the format specified before
printing to stdout.

Rename some functions to reflect the changes made:
print_value() -> append_atom()
emit()        -> append_literal()

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index e53c77e..dd62640 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -55,6 +55,11 @@ static struct {
 	{ "color" },
 };
 
+struct ref_formatting_state {
+	struct strbuf output;
+	int quote_style;
+};
+
 struct atom_value {
 	const char *s;
 	unsigned long ul; /* used for sorting when not FIELD_STR */
@@ -1195,30 +1200,25 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
 	qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);
 }
 
-static void print_value(struct atom_value *v, int quote_style)
+static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
 {
-	struct strbuf sb = STRBUF_INIT;
-	switch (quote_style) {
+	switch (state->quote_style) {
 	case QUOTE_NONE:
-		fputs(v->s, stdout);
+		strbuf_addstr(&state->output, v->s);
 		break;
 	case QUOTE_SHELL:
-		sq_quote_buf(&sb, v->s);
+		sq_quote_buf(&state->output, v->s);
 		break;
 	case QUOTE_PERL:
-		perl_quote_buf(&sb, v->s);
+		perl_quote_buf(&state->output, v->s);
 		break;
 	case QUOTE_PYTHON:
-		python_quote_buf(&sb, v->s);
+		python_quote_buf(&state->output, v->s);
 		break;
 	case QUOTE_TCL:
-		tcl_quote_buf(&sb, v->s);
+		tcl_quote_buf(&state->output, v->s);
 		break;
 	}
-	if (quote_style != QUOTE_NONE) {
-		fputs(sb.buf, stdout);
-		strbuf_release(&sb);
-	}
 }
 
 static int hex1(char ch)
@@ -1239,7 +1239,7 @@ static int hex2(const char *cp)
 		return -1;
 }
 
-static void emit(const char *cp, const char *ep)
+static void append_literal(const char *cp, const char *ep, struct ref_formatting_state *state)
 {
 	while (*cp && (!ep || cp < ep)) {
 		if (*cp == '%') {
@@ -1248,13 +1248,13 @@ static void emit(const char *cp, const char *ep)
 			else {
 				int ch = hex2(cp + 1);
 				if (0 <= ch) {
-					putchar(ch);
+					strbuf_addch(&state->output, ch);
 					cp += 3;
 					continue;
 				}
 			}
 		}
-		putchar(*cp);
+		strbuf_addch(&state->output, *cp);
 		cp++;
 	}
 }
@@ -1262,19 +1262,23 @@ static void emit(const char *cp, const char *ep)
 void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
 {
 	const char *cp, *sp, *ep;
+	struct ref_formatting_state state;
+
+	strbuf_init(&state.output, 0);
+	state.quote_style = quote_style;
 
 	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
 		struct atom_value *atomv;
 
 		ep = strchr(sp, ')');
 		if (cp < sp)
-			emit(cp, sp);
+			append_literal(cp, sp, &state);
 		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
-		print_value(atomv, quote_style);
+		append_atom(atomv, &state);
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);
-		emit(cp, sp);
+		append_literal(cp, sp, &state);
 	}
 	if (need_color_reset_at_eol) {
 		struct atom_value resetv;
@@ -1283,9 +1287,11 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 		if (color_parse("reset", color) < 0)
 			die("BUG: couldn't parse 'reset' as a color");
 		resetv.s = color;
-		print_value(&resetv, quote_style);
+		append_atom(&resetv, &state);
 	}
+	fwrite(state.output.buf, 1, state.output.len, stdout);
 	putchar('\n');
+	strbuf_release(&state.output);
 }
 
 /*  If no sorting option is given, use refname to sort as default */
-- 
2.5.0

[PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery

[Karthik Nayak]

Introduce a stack machinery for ref_formatting_state, this allows us
to push and pop states. Whenever we pop a state the strbuf from that
state is appended into the next state on the stack, this will allow us
to support nesting of modifier atoms.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c | 42 +++++++++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index dd62640..74532d3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -56,6 +56,7 @@ static struct {
 };
 
 struct ref_formatting_state {
+	struct ref_formatting_state *prev;
 	struct strbuf output;
 	int quote_style;
 };
@@ -134,6 +135,27 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 	return at;
 }
 
+static void push_new_state(struct ref_formatting_state **stack)
+{
+	struct ref_formatting_state *s = xcalloc(1, sizeof(struct ref_formatting_state));
+
+	strbuf_init(&s->output, 0);
+	s->prev = *stack;
+	*stack = s;
+}
+
+static void pop_state(struct ref_formatting_state **stack)
+{
+	struct ref_formatting_state *current = *stack;
+	struct ref_formatting_state *prev = current->prev;
+
+	if (prev)
+		strbuf_addbuf(&prev->output, &current->output);
+	strbuf_release(&current->output);
+	free(current);
+	*stack = prev;
+}
+
 /*
  * In a format string, find the next occurrence of %(atom).
  */
@@ -1262,23 +1284,24 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
 void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
 {
 	const char *cp, *sp, *ep;
-	struct ref_formatting_state state;
+	struct strbuf *final_buf;
+	struct ref_formatting_state *state = NULL;
 
-	strbuf_init(&state.output, 0);
-	state.quote_style = quote_style;
+	push_new_state(&state);
+	state->quote_style = quote_style;
 
 	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
 		struct atom_value *atomv;
 
 		ep = strchr(sp, ')');
 		if (cp < sp)
-			append_literal(cp, sp, &state);
+			append_literal(cp, sp, state);
 		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
-		append_atom(atomv, &state);
+		append_atom(atomv, state);
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);
-		append_literal(cp, sp, &state);
+		append_literal(cp, sp, state);
 	}
 	if (need_color_reset_at_eol) {
 		struct atom_value resetv;
@@ -1287,11 +1310,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 		if (color_parse("reset", color) < 0)
 			die("BUG: couldn't parse 'reset' as a color");
 		resetv.s = color;
-		append_atom(&resetv, &state);
+		append_atom(&resetv, state);
 	}
-	fwrite(state.output.buf, 1, state.output.len, stdout);
+	final_buf = &state->output;
+	fwrite(final_buf->buf, 1, final_buf->len, stdout);
+	pop_state(&state);
 	putchar('\n');
-	strbuf_release(&state.output);
 }
 
 /*  If no sorting option is given, use refname to sort as default */
-- 
2.5.0

[PATCH v12 04/13] utf8: add function to align a string into given strbuf

[Karthik Nayak]

Add strbuf_utf8_align() which will align a given string into a strbuf
as per given align_type and width. If the width is greater than the
string length then no alignment is performed.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 utf8.c | 21 +++++++++++++++++++++
 utf8.h | 15 +++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/utf8.c b/utf8.c
index 28e6d76..00e10c8 100644
--- a/utf8.c
+++ b/utf8.c
@@ -644,3 +644,24 @@ int skip_utf8_bom(char **text, size_t len)
 	*text += strlen(utf8_bom);
 	return 1;
 }
+
+void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int width,
+		       const char *s)
+{
+	int slen = strlen(s);
+	int display_len = utf8_strnwidth(s, slen, 0);
+	int utf8_compensation = slen - display_len;
+
+	if (display_len >= width) {
+		strbuf_addstr(buf, s);
+		return;
+	}
+
+	if (position == ALIGN_LEFT)
+		strbuf_addf(buf, "%-*s", width + utf8_compensation, s);
+	else if (position == ALIGN_MIDDLE) {
+		int left = (width - display_len) / 2;
+		strbuf_addf(buf, "%*s%-*s", left, "", width - left + utf8_compensation, s);
+	} else if (position == ALIGN_RIGHT)
+		strbuf_addf(buf, "%*s", width + utf8_compensation, s);
+}
diff --git a/utf8.h b/utf8.h
index 5a9e94b..7930b44 100644
--- a/utf8.h
+++ b/utf8.h
@@ -55,4 +55,19 @@ int mbs_chrlen(const char **text, size_t *remainder_p, const char *encoding);
  */
 int is_hfs_dotgit(const char *path);
 
+typedef enum {
+	ALIGN_LEFT,
+	ALIGN_MIDDLE,
+	ALIGN_RIGHT
+} align_type;
+
+/*
+ * Align the string given and store it into a strbuf as per the
+ * 'position' and 'width'. If the given string length is larger than
+ * 'width' than then the input string is not truncated and no
+ * alignment is done.
+ */
+void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int width,
+		       const char *s);
+
 #endif
-- 
2.5.0

[PATCH v12 05/13] ref-filter: implement an `align` atom

[Karthik Nayak]

Implement an `align` atom which left-, middle-, or right-aligns the
content between %(align:..) and %(end).

It is followed by `:<width>,<position>`, where the `<position>` is
either left, right or middle and `<width>` is the size of the area
into which the content will be placed. If the content between
%(align:) and %(end) is more than the width then no alignment is
performed. e.g. to align a refname atom to the middle with a total
width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".

We now have a `handler()` for each atom_value which will be called
when that atom_value is being parsed, and similarly an `at_end`
function for each state which is to be called when the `end` atom is
encountered. Using this implement the `align` atom which aligns the
given strbuf by calling `strbuf_utf8_align()` from utf8.c

This is done by assigning a 'handler' function to each atom_value and
a related 'at_end' function for each state. The align_handler()
defined uses strbuf_utf8_align() from utf8.c to perform the alignment.

Add documentation and tests for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt |  8 +++++
 ref-filter.c                       | 73 ++++++++++++++++++++++++++++++++++++++
 t/t6302-for-each-ref-filter.sh     | 48 +++++++++++++++++++++++++
 3 files changed, 129 insertions(+)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index e49d578..fe7889c 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -127,6 +127,14 @@ color::
 	Change output color.  Followed by `:<colorname>`, where names
 	are described in `color.branch.*`.
 
+align::
+	Left-, middle-, or right-align the content between %(align:..)
+	and %(end). Followed by `:<width>>,<position>`, where the
+	`<position>` is either left, right or middle and `<width>` is
+	the total length of the content with alignment. If the
+	contents length is more than the width then no alignment is
+	performed.
+
 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/ref-filter.c b/ref-filter.c
index 74532d3..ecbcc5a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -10,6 +10,7 @@
 #include "quote.h"
 #include "ref-filter.h"
 #include "revision.h"
+#include "utf8.h"
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
@@ -53,16 +54,27 @@ static struct {
 	{ "flag" },
 	{ "HEAD" },
 	{ "color" },
+	{ "align" },
+	{ "end" },
+};
+
+struct align {
+	align_type position;
+	unsigned int width;
 };
 
 struct ref_formatting_state {
 	struct ref_formatting_state *prev;
 	struct strbuf output;
+	void (*at_end)(struct ref_formatting_state *state);
+	void *cb_data;
 	int quote_style;
 };
 
 struct atom_value {
 	const char *s;
+	struct align *align;
+	void (*handler)(struct atom_value *atomv, struct ref_formatting_state **state);
 	unsigned long ul; /* used for sorting when not FIELD_STR */
 };
 
@@ -626,6 +638,36 @@ static inline char *copy_advance(char *dst, const char *src)
 	return dst;
 }
 
+static void align_handler(struct ref_formatting_state *state)
+{
+	struct align *align = (struct align *)state->cb_data;
+	struct strbuf s = STRBUF_INIT;
+
+	strbuf_utf8_align(&s, align->position, align->width, state->output.buf);
+	strbuf_reset(&state->output);
+	strbuf_addbuf(&state->output, &s);
+	free(align);
+}
+
+static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)
+{
+	struct ref_formatting_state *new;
+
+	push_new_state(state);
+	new = *state;
+	new->at_end = align_handler;
+	new->cb_data = atomv->align;
+}
+
+static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)
+{
+	struct ref_formatting_state *current = *state;
+	if (!current->at_end)
+		die(_("format: `end` atom used without a supporting atom"));
+	current->at_end(current);
+	pop_state(state);
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
@@ -654,6 +696,7 @@ static void populate_value(struct ref_array_item *ref)
 		int deref = 0;
 		const char *refname;
 		const char *formatp;
+		const char *valp;
 		struct branch *branch = NULL;
 
 		if (*name == '*') {
@@ -719,6 +762,34 @@ static void populate_value(struct ref_array_item *ref)
 			else
 				v->s = " ";
 			continue;
+		} else if (skip_prefix(name, "align:", &valp)) {
+			struct align *align = xmalloc(sizeof(struct align));
+			char *ep = strchr(valp, ',');
+
+			if (ep)
+				*ep = '\0';
+
+			if (strtoul_ui(valp, 10, &align->width))
+				die(_("positive width expected align:%s"), valp);
+
+			if (!ep || !strcmp(ep + 1, "left"))
+				align->position = ALIGN_LEFT;
+			else if (!strcmp(ep + 1, "right"))
+				align->position = ALIGN_RIGHT;
+			else if (!strcmp(ep + 1, "middle"))
+				align->position = ALIGN_MIDDLE;
+			else
+				die(_("improper format entered align:%s"), ep + 1);
+
+			if (ep)
+				*ep = ',';
+
+			v->align = align;
+			v->handler = align_atom_handler;
+			continue;
+		} else if (!strcmp(name, "end")) {
+			v->handler = end_atom_handler;
+			continue;
 		} else
 			continue;
 
@@ -1297,6 +1368,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 		if (cp < sp)
 			append_literal(cp, sp, state);
 		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
+		if (atomv->handler)
+			atomv->handler(atomv, &state);
 		append_atom(atomv, state);
 	}
 	if (*cp) {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 505a360..b252a50 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -81,4 +81,52 @@ test_expect_success 'filtering with --contains' '
 	test_cmp expect actual
 '
 
+test_expect_success 'left alignment' '
+	cat >expect <<-\EOF &&
+	refname is refs/heads/master  |refs/heads/master
+	refname is refs/heads/side    |refs/heads/side
+	refname is refs/odd/spot      |refs/odd/spot
+	refname is refs/tags/double-tag|refs/tags/double-tag
+	refname is refs/tags/four     |refs/tags/four
+	refname is refs/tags/one      |refs/tags/one
+	refname is refs/tags/signed-tag|refs/tags/signed-tag
+	refname is refs/tags/three    |refs/tags/three
+	refname is refs/tags/two      |refs/tags/two
+	EOF
+	git for-each-ref --format="%(align:30,left)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'middle alignment' '
+	cat >expect <<-\EOF &&
+	| refname is refs/heads/master |refs/heads/master
+	|  refname is refs/heads/side  |refs/heads/side
+	|   refname is refs/odd/spot   |refs/odd/spot
+	|refname is refs/tags/double-tag|refs/tags/double-tag
+	|  refname is refs/tags/four   |refs/tags/four
+	|   refname is refs/tags/one   |refs/tags/one
+	|refname is refs/tags/signed-tag|refs/tags/signed-tag
+	|  refname is refs/tags/three  |refs/tags/three
+	|   refname is refs/tags/two   |refs/tags/two
+	EOF
+	git for-each-ref --format="|%(align:30,middle)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'right alignment' '
+	cat >expect <<-\EOF &&
+	|  refname is refs/heads/master|refs/heads/master
+	|    refname is refs/heads/side|refs/heads/side
+	|      refname is refs/odd/spot|refs/odd/spot
+	|refname is refs/tags/double-tag|refs/tags/double-tag
+	|     refname is refs/tags/four|refs/tags/four
+	|      refname is refs/tags/one|refs/tags/one
+	|refname is refs/tags/signed-tag|refs/tags/signed-tag
+	|    refname is refs/tags/three|refs/tags/three
+	|      refname is refs/tags/two|refs/tags/two
+	EOF
+	git for-each-ref --format="|%(align:30,right)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.5.0

[PATCH v12 06/13] ref-filter: add option to filter out tags, branches and remotes

[Karthik Nayak]

From: Karthik Nayak <karthik.188@gmail.com>

Add a function called 'for_each_reftype_fullpath()' to refs.{c,h}
which iterates through each ref for the given path without trimming
the path and also accounting for broken refs, if mentioned.

Add 'filter_ref_kind()' in ref-filter.c to check the kind of ref being
handled and return the kind to 'ref_filter_handler()', where we
discard refs which we do not need and assign the kind to needed refs.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 ref-filter.h | 12 ++++++++++--
 refs.c       |  9 +++++++++
 refs.h       |  1 +
 4 files changed, 74 insertions(+), 7 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index ecbcc5a..58b6d40 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1101,6 +1101,36 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
 	return ref;
 }
 
+static int filter_ref_kind(struct ref_filter *filter, const char *refname)
+{
+	unsigned int i;
+
+	static struct {
+		const char *prefix;
+		unsigned int kind;
+	} ref_kind[] = {
+		{ "refs/heads/" , FILTER_REFS_BRANCHES },
+		{ "refs/remotes/" , FILTER_REFS_REMOTES },
+		{ "refs/tags/", FILTER_REFS_TAGS}
+	};
+
+	if (filter->kind == FILTER_REFS_BRANCHES)
+		return FILTER_REFS_BRANCHES;
+	else if (filter->kind == FILTER_REFS_REMOTES)
+		return FILTER_REFS_REMOTES;
+	else if (filter->kind == FILTER_REFS_TAGS)
+		return FILTER_REFS_TAGS;
+	else if (!strcmp(refname, "HEAD"))
+		return FILTER_REFS_DETACHED_HEAD;
+
+	for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
+		if (starts_with(refname, ref_kind[i].prefix))
+			return ref_kind[i].kind;
+	}
+
+	return FILTER_REFS_OTHERS;
+}
+
 /*
  * A call-back given to for_each_ref().  Filter refs and keep them for
  * later object processing.
@@ -1111,6 +1141,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	struct ref_filter *filter = ref_cbdata->filter;
 	struct ref_array_item *ref;
 	struct commit *commit = NULL;
+	unsigned int kind;
 
 	if (flag & REF_BAD_NAME) {
 		warning("ignoring ref with broken name %s", refname);
@@ -1122,6 +1153,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 		return 0;
 	}
 
+	kind = filter_ref_kind(filter, refname);
+	if (!(kind & filter->kind))
+		return 0;
+
 	if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
 		return 0;
 
@@ -1153,6 +1188,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 
 	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
 	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
+	ref->kind = kind;
 	return 0;
 }
 
@@ -1229,16 +1265,29 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 {
 	struct ref_filter_cbdata ref_cbdata;
 	int ret = 0;
+	unsigned int broken = 0;
 
 	ref_cbdata.array = array;
 	ref_cbdata.filter = filter;
 
 	/*  Simple per-ref filtering */
-	if (type & (FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN))
-		ret = for_each_rawref(ref_filter_handler, &ref_cbdata);
-	else if (type & FILTER_REFS_ALL)
-		ret = for_each_ref(ref_filter_handler, &ref_cbdata);
-	else if (type)
+	if (type & FILTER_REFS_INCLUDE_BROKEN) {
+		type &= ~FILTER_REFS_INCLUDE_BROKEN;
+		broken = 1;
+	}
+
+	filter->kind = type;
+	if (type == FILTER_REFS_BRANCHES)
+		ret = for_each_reftype_fullpath(ref_filter_handler, "refs/heads/", broken, &ref_cbdata);
+	else if (type == FILTER_REFS_REMOTES)
+		ret = for_each_reftype_fullpath(ref_filter_handler, "refs/remotes/", broken, &ref_cbdata);
+	else if (type == FILTER_REFS_TAGS)
+		ret = for_each_reftype_fullpath(ref_filter_handler, "refs/tags/", broken, &ref_cbdata);
+	else if (type & FILTER_REFS_ALL) {
+		ret = for_each_reftype_fullpath(ref_filter_handler, "", broken, &ref_cbdata);
+		if (type & FILTER_REFS_DETACHED_HEAD)
+			head_ref(ref_filter_handler, &ref_cbdata);
+	} else
 		die("filter_refs: invalid type");
 
 	/*  Filters that need revision walking */
diff --git a/ref-filter.h b/ref-filter.h
index 45026d0..99f081b 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -13,8 +13,14 @@
 #define QUOTE_PYTHON 4
 #define QUOTE_TCL 8
 
-#define FILTER_REFS_INCLUDE_BROKEN 0x1
-#define FILTER_REFS_ALL 0x2
+#define FILTER_REFS_INCLUDE_BROKEN 0x0001
+#define FILTER_REFS_TAGS           0x0002
+#define FILTER_REFS_BRANCHES       0x0004
+#define FILTER_REFS_REMOTES        0x0008
+#define FILTER_REFS_OTHERS         0x0010
+#define FILTER_REFS_ALL            (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
+				    FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
+#define FILTER_REFS_DETACHED_HEAD  0x0020
 
 struct atom_value;
 
@@ -27,6 +33,7 @@ struct ref_sorting {
 struct ref_array_item {
 	unsigned char objectname[20];
 	int flag;
+	unsigned int kind;
 	const char *symref;
 	struct commit *commit;
 	struct atom_value *value;
@@ -51,6 +58,7 @@ struct ref_filter {
 	struct commit *merge_commit;
 
 	unsigned int with_commit_tag_algo : 1;
+	unsigned int kind;
 };
 
 struct ref_filter_cbdata {
diff --git a/refs.c b/refs.c
index 2db2975..a92bba1 100644
--- a/refs.c
+++ b/refs.c
@@ -2145,6 +2145,15 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data)
 			       strlen(git_replace_ref_base), 0, cb_data);
 }
 
+int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data)
+{
+	unsigned int flag = 0;
+
+	if (broken)
+		flag = DO_FOR_EACH_INCLUDE_BROKEN;
+	return do_for_each_ref(&ref_cache, type, fn, 0, flag, cb_data);
+}
+
 int head_ref_namespaced(each_ref_fn fn, void *cb_data)
 {
 	struct strbuf buf = STRBUF_INIT;
diff --git a/refs.h b/refs.h
index 6a3fa6d..7f91a72 100644
--- a/refs.h
+++ b/refs.h
@@ -179,6 +179,7 @@ extern int for_each_remote_ref(each_ref_fn fn, void *cb_data);
 extern int for_each_replace_ref(each_ref_fn fn, void *cb_data);
 extern int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);
 extern int for_each_glob_ref_in(each_ref_fn fn, const char *pattern, const char *prefix, void *cb_data);
+extern int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data);
 
 extern int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);
 extern int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);
-- 
2.5.0

[PATCH v12 07/13] ref-filter: support printing N lines from tag annotation

[Karthik Nayak]

From: Karthik Nayak <karthik.188@gmail.com>

In 'tag.c' we can print N lines from the annotation of the tag using
the '-n<num>' option. Copy code from 'tag.c' to 'ref-filter' and
modify 'ref-filter' to support printing of N lines from the annotation
of tags.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/for-each-ref.c |  2 +-
 builtin/tag.c          |  4 ++++
 ref-filter.c           | 52 +++++++++++++++++++++++++++++++++++++++++++++++++-
 ref-filter.h           | 11 ++++++++---
 4 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 40f343b..e4a4f8a 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -74,7 +74,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	if (!maxcount || array.nr < maxcount)
 		maxcount = array.nr;
 	for (i = 0; i < maxcount; i++)
-		show_ref_array_item(array.items[i], format, quote_style);
+		show_ref_array_item(array.items[i], format, quote_style, 0);
 	ref_array_clear(&array);
 	return 0;
 }
diff --git a/builtin/tag.c b/builtin/tag.c
index 471d6b1..0fc7557 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -185,6 +185,10 @@ static enum contains_result contains(struct commit *candidate,
 	return contains_test(candidate, want);
 }
 
+/*
+ * Currently duplicated in ref-filter, will eventually be removed as
+ * we port tag.c to use ref-filter APIs.
+ */
 static void show_tag_lines(const struct object_id *oid, int lines)
 {
 	int i;
diff --git a/ref-filter.c b/ref-filter.c
index 58b6d40..a9fe54a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1401,7 +1401,51 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
 	}
 }
 
-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
+/*
+ * If 'lines' is greater than 0, print that many lines from the given
+ * object_id 'oid'.
+ */
+static void show_tag_lines(const struct object_id *oid, int lines)
+{
+	int i;
+	unsigned long size;
+	enum object_type type;
+	char *buf, *sp, *eol;
+	size_t len;
+
+	buf = read_sha1_file(oid->hash, &type, &size);
+	if (!buf)
+		die_errno("unable to read object %s", oid_to_hex(oid));
+	if (type != OBJ_COMMIT && type != OBJ_TAG)
+		goto free_return;
+	if (!size)
+		die("an empty %s object %s?",
+		    typename(type), oid_to_hex(oid));
+
+	/* skip header */
+	sp = strstr(buf, "\n\n");
+	if (!sp)
+		goto free_return;
+
+	/* only take up to "lines" lines, and strip the signature from a tag */
+	if (type == OBJ_TAG)
+		size = parse_signature(buf, size);
+	for (i = 0, sp += 2; i < lines && sp < buf + size; i++) {
+		if (i)
+			printf("\n    ");
+		eol = memchr(sp, '\n', size - (sp - buf));
+		len = eol ? eol - sp : size - (sp - buf);
+		fwrite(sp, len, 1, stdout);
+		if (!eol)
+			break;
+		sp = eol + 1;
+	}
+free_return:
+	free(buf);
+}
+
+void show_ref_array_item(struct ref_array_item *info, const char *format,
+			 int quote_style, unsigned int lines)
 {
 	const char *cp, *sp, *ep;
 	struct strbuf *final_buf;
@@ -1437,6 +1481,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 	final_buf = &state->output;
 	fwrite(final_buf->buf, 1, final_buf->len, stdout);
 	pop_state(&state);
+
+	if (lines > 0) {
+		struct object_id oid;
+		hashcpy(oid.hash, info->objectname);
+		show_tag_lines(&oid, lines);
+	}
 	putchar('\n');
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 99f081b..c599ea2 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -58,7 +58,8 @@ struct ref_filter {
 	struct commit *merge_commit;
 
 	unsigned int with_commit_tag_algo : 1;
-	unsigned int kind;
+	unsigned int kind,
+		lines;
 };
 
 struct ref_filter_cbdata {
@@ -90,8 +91,12 @@ int parse_ref_filter_atom(const char *atom, const char *ep);
 int verify_ref_format(const char *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
-/*  Print the ref using the given format and quote_style */
-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);
+/*
+ * Print the ref using the given format and quote_style. If 'lines' > 0,
+ * print that many lines of the the given ref.
+ */
+void show_ref_array_item(struct ref_array_item *info, const char *format,
+			 int quote_style, unsigned int lines);
 /*  Callback function for parsing the sort option */
 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);
 /*  Default sort option based on refname */
-- 
2.5.0

[PATCH v12 08/13] ref-filter: add support to sort by version

[Karthik Nayak]

From: Karthik Nayak <karthik.188@gmail.com>

Add support to sort by version using the "v:refname" and
"version:refname" option. This is achieved by using the 'versioncmp()'
function as the comparing function for qsort.

This option is included to support sorting by versions in `git tag -l`
which will eventually be ported to use ref-filter APIs.

Add documentation and tests for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt |  3 +++
 ref-filter.c                       | 15 ++++++++++-----
 ref-filter.h                       |  3 ++-
 t/t6302-for-each-ref-filter.sh     | 36 ++++++++++++++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index fe7889c..760d719 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -153,6 +153,9 @@ For sorting purposes, fields with numeric values sort in numeric
 order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).
 All other fields are used to sort in their byte-value order.
 
+There is also an option to sort by versions, this can be done by using
+the fieldname `version:refname` or its alias `v:refname`.
+
 In any case, a field name that refers to a field inapplicable to
 the object referred by the ref does not cause an error.  It
 returns an empty string instead.
diff --git a/ref-filter.c b/ref-filter.c
index a9fe54a..67c7846 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -11,6 +11,8 @@
 #include "ref-filter.h"
 #include "revision.h"
 #include "utf8.h"
+#include "git-compat-util.h"
+#include "version.h"
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
@@ -1305,19 +1307,19 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 
 	get_ref_atom_value(a, s->atom, &va);
 	get_ref_atom_value(b, s->atom, &vb);
-	switch (cmp_type) {
-	case FIELD_STR:
+	if (s->version)
+		cmp = versioncmp(va->s, vb->s);
+	else if (cmp_type == FIELD_STR)
 		cmp = strcmp(va->s, vb->s);
-		break;
-	default:
+	else {
 		if (va->ul < vb->ul)
 			cmp = -1;
 		else if (va->ul == vb->ul)
 			cmp = 0;
 		else
 			cmp = 1;
-		break;
 	}
+
 	return (s->reverse) ? -cmp : cmp;
 }
 
@@ -1519,6 +1521,9 @@ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
 		s->reverse = 1;
 		arg++;
 	}
+	if (skip_prefix(arg, "version:", &arg) ||
+	    skip_prefix(arg, "v:", &arg))
+		s->version = 1;
 	len = strlen(arg);
 	s->atom = parse_ref_filter_atom(arg, arg+len);
 	return 0;
diff --git a/ref-filter.h b/ref-filter.h
index c599ea2..5aa2f40 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -27,7 +27,8 @@ struct atom_value;
 struct ref_sorting {
 	struct ref_sorting *next;
 	int atom; /* index into used_atom array (internal) */
-	unsigned reverse : 1;
+	unsigned reverse : 1,
+		version : 1;
 };
 
 struct ref_array_item {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index b252a50..1c56879 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -129,4 +129,40 @@ test_expect_success 'right alignment' '
 	test_cmp expect actual
 '
 
+test_expect_success 'setup for version sort' '
+	test_commit foo1.3 &&
+	test_commit foo1.6 &&
+	test_commit foo1.10
+'
+
+test_expect_success 'version sort' '
+	git for-each-ref --sort=version:refname --format="%(refname:short)" refs/tags/ | grep "foo" >actual &&
+	cat >expect <<-\EOF &&
+	foo1.3
+	foo1.6
+	foo1.10
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'version sort (shortened)' '
+	git for-each-ref --sort=v:refname --format="%(refname:short)" refs/tags/ | grep "foo" >actual &&
+	cat >expect <<-\EOF &&
+	foo1.3
+	foo1.6
+	foo1.10
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'reverse version sort' '
+	git for-each-ref --sort=-version:refname --format="%(refname:short)" refs/tags/ | grep "foo" >actual &&
+	cat >expect <<-\EOF &&
+	foo1.10
+	foo1.6
+	foo1.3
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.5.0

[PATCH v12 09/13] ref-filter: add option to match literal pattern

[Karthik Nayak]

From: Karthik Nayak <karthik.188@gmail.com>

Since 'ref-filter' only has an option to match path names add an
option for plain fnmatch pattern-matching.

This is to support the pattern matching options which are used in `git
tag -l` and `git branch -l` where we can match patterns like `git tag
-l foo*` which would match all tags which has a "foo*" pattern.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/for-each-ref.c |  1 +
 ref-filter.c           | 40 +++++++++++++++++++++++++++++++++++++---
 ref-filter.h           |  3 ++-
 3 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index e4a4f8a..3ad6a64 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -68,6 +68,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 
 	filter.name_patterns = argv;
+	filter.match_as_path = 1;
 	filter_refs(&array, &filter, FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN);
 	ref_array_sort(sorting, &array);
 
diff --git a/ref-filter.c b/ref-filter.c
index 67c7846..665221b 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1033,9 +1033,33 @@ static int commit_contains(struct ref_filter *filter, struct commit *commit)
 
 /*
  * Return 1 if the refname matches one of the patterns, otherwise 0.
+ * A pattern can be a literal prefix (e.g. a refname "refs/heads/master"
+ * matches a pattern "refs/heads/mas") or a wildcard (e.g. the same ref
+ * matches "refs/heads/mas*", too).
+ */
+static int match_pattern(const char **patterns, const char *refname)
+{
+	/*
+	 * When no '--format' option is given we need to skip the prefix
+	 * for matching refs of tags and branches.
+	 */
+	(void)(skip_prefix(refname, "refs/tags/", &refname) ||
+	       skip_prefix(refname, "refs/heads/", &refname) ||
+	       skip_prefix(refname, "refs/remotes/", &refname) ||
+	       skip_prefix(refname, "refs/", &refname));
+
+	for (; *patterns; patterns++) {
+		if (!wildmatch(*patterns, refname, 0, NULL))
+			return 1;
+	}
+	return 0;
+}
+
+/*
+ * Return 1 if the refname matches one of the patterns, otherwise 0.
  * A pattern can be path prefix (e.g. a refname "refs/heads/master"
- * matches a pattern "refs/heads/") or a wildcard (e.g. the same ref
- * matches "refs/heads/m*",too).
+ * matches a pattern "refs/heads/" but not "refs/heads/m") or a
+ * wildcard (e.g. the same ref matches "refs/heads/m*", too).
  */
 static int match_name_as_path(const char **pattern, const char *refname)
 {
@@ -1056,6 +1080,16 @@ static int match_name_as_path(const char **pattern, const char *refname)
 	return 0;
 }
 
+/* Return 1 if the refname matches one of the patterns, otherwise 0. */
+static int filter_pattern_match(struct ref_filter *filter, const char *refname)
+{
+	if (!*filter->name_patterns)
+		return 1; /* No pattern always matches */
+	if (filter->match_as_path)
+		return match_name_as_path(filter->name_patterns, refname);
+	return match_pattern(filter->name_patterns, refname);
+}
+
 /*
  * Given a ref (sha1, refname), check if the ref belongs to the array
  * of sha1s. If the given ref is a tag, check if the given tag points
@@ -1159,7 +1193,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	if (!(kind & filter->kind))
 		return 0;
 
-	if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
+	if (!filter_pattern_match(filter, refname))
 		return 0;
 
 	if (filter->points_at.nr && !match_points_at(&filter->points_at, oid->hash, refname))
diff --git a/ref-filter.h b/ref-filter.h
index 5aa2f40..8241066 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -58,7 +58,8 @@ struct ref_filter {
 	} merge;
 	struct commit *merge_commit;
 
-	unsigned int with_commit_tag_algo : 1;
+	unsigned int with_commit_tag_algo : 1,
+		match_as_path : 1;
 	unsigned int kind,
 		lines;
 };
-- 
2.5.0

[PATCH v12 10/13] tag.c: use 'ref-filter' data structures

[Karthik Nayak]

From: Karthik Nayak <karthik.188@gmail.com>

Make 'tag.c' use 'ref-filter' data structures and make changes to
support the new data structures. This is a part of the process
of porting 'tag.c' to use 'ref-filter' APIs.

This is a temporary step before porting 'tag.c' to use 'ref-filter'
completely. As this is a temporary step, most of the code
introduced here will be removed when 'tag.c' is ported over to use
'ref-filter' APIs

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/tag.c | 106 +++++++++++++++++++++++++++++++---------------------------
 1 file changed, 57 insertions(+), 49 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 0fc7557..e96bae2 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -17,6 +17,7 @@
 #include "gpg-interface.h"
 #include "sha1-array.h"
 #include "column.h"
+#include "ref-filter.h"
 
 static const char * const git_tag_usage[] = {
 	N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] <tagname> [<head>]"),
@@ -34,15 +35,6 @@ static const char * const git_tag_usage[] = {
 
 static int tag_sort;
 
-struct tag_filter {
-	const char **patterns;
-	int lines;
-	int sort;
-	struct string_list tags;
-	struct commit_list *with_commit;
-};
-
-static struct sha1_array points_at;
 static unsigned int colopts;
 
 static int match_pattern(const char **patterns, const char *ref)
@@ -61,19 +53,20 @@ static int match_pattern(const char **patterns, const char *ref)
  * removed as we port tag.c to use the ref-filter APIs.
  */
 static const unsigned char *match_points_at(const char *refname,
-					    const unsigned char *sha1)
+					    const unsigned char *sha1,
+					    struct sha1_array *points_at)
 {
 	const unsigned char *tagged_sha1 = NULL;
 	struct object *obj;
 
-	if (sha1_array_lookup(&points_at, sha1) >= 0)
+	if (sha1_array_lookup(points_at, sha1) >= 0)
 		return sha1;
 	obj = parse_object(sha1);
 	if (!obj)
 		die(_("malformed object at '%s'"), refname);
 	if (obj->type == OBJ_TAG)
 		tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
-	if (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)
+	if (tagged_sha1 && sha1_array_lookup(points_at, tagged_sha1) >= 0)
 		return tagged_sha1;
 	return NULL;
 }
@@ -228,12 +221,24 @@ free_return:
 	free(buf);
 }
 
+static void ref_array_append(struct ref_array *array, const char *refname)
+{
+	size_t len = strlen(refname);
+	struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
+	memcpy(ref->refname, refname, len);
+	ref->refname[len] = '\0';
+	REALLOC_ARRAY(array->items, array->nr + 1);
+	array->items[array->nr++] = ref;
+}
+
 static int show_reference(const char *refname, const struct object_id *oid,
 			  int flag, void *cb_data)
 {
-	struct tag_filter *filter = cb_data;
+	struct ref_filter_cbdata *data = cb_data;
+	struct ref_array *array = data->array;
+	struct ref_filter *filter = data->filter;
 
-	if (match_pattern(filter->patterns, refname)) {
+	if (match_pattern(filter->name_patterns, refname)) {
 		if (filter->with_commit) {
 			struct commit *commit;
 
@@ -244,12 +249,12 @@ static int show_reference(const char *refname, const struct object_id *oid,
 				return 0;
 		}
 
-		if (points_at.nr && !match_points_at(refname, oid->hash))
+		if (filter->points_at.nr && !match_points_at(refname, oid->hash, &filter->points_at))
 			return 0;
 
 		if (!filter->lines) {
-			if (filter->sort)
-				string_list_append(&filter->tags, refname);
+			if (tag_sort)
+				ref_array_append(array, refname);
 			else
 				printf("%s\n", refname);
 			return 0;
@@ -264,36 +269,36 @@ static int show_reference(const char *refname, const struct object_id *oid,
 
 static int sort_by_version(const void *a_, const void *b_)
 {
-	const struct string_list_item *a = a_;
-	const struct string_list_item *b = b_;
-	return versioncmp(a->string, b->string);
+	const struct ref_array_item *a = *((struct ref_array_item **)a_);
+	const struct ref_array_item *b = *((struct ref_array_item **)b_);
+	return versioncmp(a->refname, b->refname);
 }
 
-static int list_tags(const char **patterns, int lines,
-		     struct commit_list *with_commit, int sort)
+static int list_tags(struct ref_filter *filter, int sort)
 {
-	struct tag_filter filter;
+	struct ref_array array;
+	struct ref_filter_cbdata data;
+
+	memset(&array, 0, sizeof(array));
+	data.array = &array;
+	data.filter = filter;
 
-	filter.patterns = patterns;
-	filter.lines = lines;
-	filter.sort = sort;
-	filter.with_commit = with_commit;
-	memset(&filter.tags, 0, sizeof(filter.tags));
-	filter.tags.strdup_strings = 1;
+	if (filter->lines == -1)
+		filter->lines = 0;
 
-	for_each_tag_ref(show_reference, (void *)&filter);
+	for_each_tag_ref(show_reference, &data);
 	if (sort) {
 		int i;
 		if ((sort & SORT_MASK) == VERCMP_SORT)
-			qsort(filter.tags.items, filter.tags.nr,
-			      sizeof(struct string_list_item), sort_by_version);
+			qsort(array.items, array.nr,
+			      sizeof(struct ref_array_item *), sort_by_version);
 		if (sort & REVERSE_SORT)
-			for (i = filter.tags.nr - 1; i >= 0; i--)
-				printf("%s\n", filter.tags.items[i].string);
+			for (i = array.nr - 1; i >= 0; i--)
+				printf("%s\n", array.items[i]->refname);
 		else
-			for (i = 0; i < filter.tags.nr; i++)
-				printf("%s\n", filter.tags.items[i].string);
-		string_list_clear(&filter.tags, 0);
+			for (i = 0; i < array.nr; i++)
+				printf("%s\n", array.items[i]->refname);
+		ref_array_clear(&array);
 	}
 	return 0;
 }
@@ -574,17 +579,17 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	const char *object_ref, *tag;
 	struct create_tag_options opt;
 	char *cleanup_arg = NULL;
-	int annotate = 0, force = 0, lines = -1;
 	int create_reflog = 0;
+	int annotate = 0, force = 0;
 	int cmdmode = 0;
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
-	struct commit_list *with_commit = NULL;
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
+	struct ref_filter filter;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
-		{ OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
+		{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
 				N_("print <n> lines of each tag message"),
 				PARSE_OPT_OPTARG, NULL, 1 },
 		OPT_CMDMODE('d', "delete", &cmdmode, N_("delete tags"), 'd'),
@@ -606,14 +611,14 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 		OPT_GROUP(N_("Tag listing options")),
 		OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
-		OPT_CONTAINS(&with_commit, N_("print only tags that contain the commit")),
-		OPT_WITH(&with_commit, N_("print only tags that contain the commit")),
+		OPT_CONTAINS(&filter.with_commit, N_("print only tags that contain the commit")),
+		OPT_WITH(&filter.with_commit, N_("print only tags that contain the commit")),
 		{
 			OPTION_CALLBACK, 0, "sort", &tag_sort, N_("type"), N_("sort tags"),
 			PARSE_OPT_NONEG, parse_opt_sort
 		},
 		{
-			OPTION_CALLBACK, 0, "points-at", &points_at, N_("object"),
+			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
 			N_("print only tags of the object"), 0, parse_opt_object_name
 		},
 		OPT_END()
@@ -622,6 +627,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	git_config(git_tag_config, NULL);
 
 	memset(&opt, 0, sizeof(opt));
+	memset(&filter, 0, sizeof(filter));
+	filter.lines = -1;
 
 	argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
@@ -638,7 +645,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_tag_usage, options);
 
 	finalize_colopts(&colopts, -1);
-	if (cmdmode == 'l' && lines != -1) {
+	if (cmdmode == 'l' && filter.lines != -1) {
 		if (explicitly_enable_column(colopts))
 			die(_("--column and -n are incompatible"));
 		colopts = 0;
@@ -651,18 +658,19 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			copts.padding = 2;
 			run_column_filter(colopts, &copts);
 		}
-		if (lines != -1 && tag_sort)
+		if (filter.lines != -1 && tag_sort)
 			die(_("--sort and -n are incompatible"));
-		ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, tag_sort);
+		filter.name_patterns = argv;
+		ret = list_tags(&filter, tag_sort);
 		if (column_active(colopts))
 			stop_column_filter();
 		return ret;
 	}
-	if (lines != -1)
+	if (filter.lines != -1)
 		die(_("-n option is only allowed with -l."));
-	if (with_commit)
+	if (filter.with_commit)
 		die(_("--contains option is only allowed with -l."));
-	if (points_at.nr)
+	if (filter.points_at.nr)
 		die(_("--points-at option is only allowed with -l."));
 	if (cmdmode == 'd')
 		return for_each_tag_name(argv, delete_tag);
-- 
2.5.0

[PATCH v12 11/13] tag.c: use 'ref-filter' APIs

[Karthik Nayak]

From: Karthik Nayak <karthik.188@gmail.com>

Make 'tag.c' use 'ref-filter' APIs for iterating through refs, sorting
and printing of refs. This removes most of the code used in 'tag.c'
replacing it with calls to the 'ref-filter' library.

Make 'tag.c' use the 'filter_refs()' function provided by 'ref-filter'
to filter out tags based on the options set.

For printing tags we use 'show_ref_array_item()' function provided by
'ref-filter'.

We improve the sorting option provided by 'tag.c' by using the sorting
options provided by 'ref-filter'. This causes the test 'invalid sort
parameter on command line' in t7004 to fail, as 'ref-filter' throws an
error for all sorting fields which are incorrect. The test is changed
to reflect the same.

Modify documentation for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-tag.txt |  16 ++-
 builtin/tag.c             | 342 ++++++----------------------------------------
 t/t7004-tag.sh            |   8 +-
 3 files changed, 50 insertions(+), 316 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 84f6496..3ac4a96 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 	<tagname> [<commit> | <object>]
 'git tag' -d <tagname>...
 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
-	[--column[=<options>] | --no-column] [--create-reflog] [<pattern>...]
+	[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>] [<pattern>...]
 'git tag' -v <tagname>...
 
 DESCRIPTION
@@ -94,14 +94,16 @@ OPTIONS
 	using fnmatch(3)).  Multiple patterns may be given; if any of
 	them matches, the tag is shown.
 
---sort=<type>::
-	Sort in a specific order. Supported type is "refname"
-	(lexicographic order), "version:refname" or "v:refname" (tag
+--sort=<key>::
+	Sort based on the key given.  Prefix `-` to sort in
+	descending order of the value. You may use the --sort=<key> option
+	multiple times, in which case the last key becomes the primary
+	key. Also supports "version:refname" or "v:refname" (tag
 	names are treated as versions). The "version:refname" sort
 	order can also be affected by the
-	"versionsort.prereleaseSuffix" configuration variable. Prepend
-	"-" to reverse sort order. When this option is not given, the
-	sort order defaults to the value configured for the 'tag.sort'
+	"versionsort.prereleaseSuffix" configuration variable.
+	The keys supported are the same as those in `git for-each-ref`.
+	Sort order defaults to the value configured for the 'tag.sort'
 	variable if it exists, or lexicographic order otherwise. See
 	linkgit:git-config[1].
 
diff --git a/builtin/tag.c b/builtin/tag.c
index e96bae2..501fc52 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -28,278 +28,32 @@ static const char * const git_tag_usage[] = {
 	NULL
 };
 
-#define STRCMP_SORT     0	/* must be zero */
-#define VERCMP_SORT     1
-#define SORT_MASK       0x7fff
-#define REVERSE_SORT    0x8000
-
-static int tag_sort;
-
 static unsigned int colopts;
 
-static int match_pattern(const char **patterns, const char *ref)
-{
-	/* no pattern means match everything */
-	if (!*patterns)
-		return 1;
-	for (; *patterns; patterns++)
-		if (!wildmatch(*patterns, ref, 0, NULL))
-			return 1;
-	return 0;
-}
-
-/*
- * This is currently duplicated in ref-filter.c, and will eventually be
- * removed as we port tag.c to use the ref-filter APIs.
- */
-static const unsigned char *match_points_at(const char *refname,
-					    const unsigned char *sha1,
-					    struct sha1_array *points_at)
-{
-	const unsigned char *tagged_sha1 = NULL;
-	struct object *obj;
-
-	if (sha1_array_lookup(points_at, sha1) >= 0)
-		return sha1;
-	obj = parse_object(sha1);
-	if (!obj)
-		die(_("malformed object at '%s'"), refname);
-	if (obj->type == OBJ_TAG)
-		tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
-	if (tagged_sha1 && sha1_array_lookup(points_at, tagged_sha1) >= 0)
-		return tagged_sha1;
-	return NULL;
-}
-
-static int in_commit_list(const struct commit_list *want, struct commit *c)
-{
-	for (; want; want = want->next)
-		if (!hashcmp(want->item->object.sha1, c->object.sha1))
-			return 1;
-	return 0;
-}
-
-/*
- * The entire code segment for supporting the --contains option has been
- * copied over to ref-filter.{c,h}. This will be deleted evetually when
- * we port tag.c to use ref-filter APIs.
- */
-enum contains_result {
-	CONTAINS_UNKNOWN = -1,
-	CONTAINS_NO = 0,
-	CONTAINS_YES = 1
-};
-
-/*
- * Test whether the candidate or one of its parents is contained in the list.
- * Do not recurse to find out, though, but return -1 if inconclusive.
- */
-static enum contains_result contains_test(struct commit *candidate,
-			    const struct commit_list *want)
-{
-	/* was it previously marked as containing a want commit? */
-	if (candidate->object.flags & TMP_MARK)
-		return 1;
-	/* or marked as not possibly containing a want commit? */
-	if (candidate->object.flags & UNINTERESTING)
-		return 0;
-	/* or are we it? */
-	if (in_commit_list(want, candidate)) {
-		candidate->object.flags |= TMP_MARK;
-		return 1;
-	}
-
-	if (parse_commit(candidate) < 0)
-		return 0;
-
-	return -1;
-}
-
-/*
- * Mimicking the real stack, this stack lives on the heap, avoiding stack
- * overflows.
- *
- * At each recursion step, the stack items points to the commits whose
- * ancestors are to be inspected.
- */
-struct stack {
-	int nr, alloc;
-	struct stack_entry {
-		struct commit *commit;
-		struct commit_list *parents;
-	} *stack;
-};
-
-static void push_to_stack(struct commit *candidate, struct stack *stack)
-{
-	int index = stack->nr++;
-	ALLOC_GROW(stack->stack, stack->nr, stack->alloc);
-	stack->stack[index].commit = candidate;
-	stack->stack[index].parents = candidate->parents;
-}
-
-static enum contains_result contains(struct commit *candidate,
-		const struct commit_list *want)
-{
-	struct stack stack = { 0, 0, NULL };
-	int result = contains_test(candidate, want);
-
-	if (result != CONTAINS_UNKNOWN)
-		return result;
-
-	push_to_stack(candidate, &stack);
-	while (stack.nr) {
-		struct stack_entry *entry = &stack.stack[stack.nr - 1];
-		struct commit *commit = entry->commit;
-		struct commit_list *parents = entry->parents;
-
-		if (!parents) {
-			commit->object.flags |= UNINTERESTING;
-			stack.nr--;
-		}
-		/*
-		 * If we just popped the stack, parents->item has been marked,
-		 * therefore contains_test will return a meaningful 0 or 1.
-		 */
-		else switch (contains_test(parents->item, want)) {
-		case CONTAINS_YES:
-			commit->object.flags |= TMP_MARK;
-			stack.nr--;
-			break;
-		case CONTAINS_NO:
-			entry->parents = parents->next;
-			break;
-		case CONTAINS_UNKNOWN:
-			push_to_stack(parents->item, &stack);
-			break;
-		}
-	}
-	free(stack.stack);
-	return contains_test(candidate, want);
-}
-
-/*
- * Currently duplicated in ref-filter, will eventually be removed as
- * we port tag.c to use ref-filter APIs.
- */
-static void show_tag_lines(const struct object_id *oid, int lines)
-{
-	int i;
-	unsigned long size;
-	enum object_type type;
-	char *buf, *sp, *eol;
-	size_t len;
-
-	buf = read_sha1_file(oid->hash, &type, &size);
-	if (!buf)
-		die_errno("unable to read object %s", oid_to_hex(oid));
-	if (type != OBJ_COMMIT && type != OBJ_TAG)
-		goto free_return;
-	if (!size)
-		die("an empty %s object %s?",
-		    typename(type), oid_to_hex(oid));
-
-	/* skip header */
-	sp = strstr(buf, "\n\n");
-	if (!sp)
-		goto free_return;
-
-	/* only take up to "lines" lines, and strip the signature from a tag */
-	if (type == OBJ_TAG)
-		size = parse_signature(buf, size);
-	for (i = 0, sp += 2; i < lines && sp < buf + size; i++) {
-		if (i)
-			printf("\n    ");
-		eol = memchr(sp, '\n', size - (sp - buf));
-		len = eol ? eol - sp : size - (sp - buf);
-		fwrite(sp, len, 1, stdout);
-		if (!eol)
-			break;
-		sp = eol + 1;
-	}
-free_return:
-	free(buf);
-}
-
-static void ref_array_append(struct ref_array *array, const char *refname)
-{
-	size_t len = strlen(refname);
-	struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
-	memcpy(ref->refname, refname, len);
-	ref->refname[len] = '\0';
-	REALLOC_ARRAY(array->items, array->nr + 1);
-	array->items[array->nr++] = ref;
-}
-
-static int show_reference(const char *refname, const struct object_id *oid,
-			  int flag, void *cb_data)
-{
-	struct ref_filter_cbdata *data = cb_data;
-	struct ref_array *array = data->array;
-	struct ref_filter *filter = data->filter;
-
-	if (match_pattern(filter->name_patterns, refname)) {
-		if (filter->with_commit) {
-			struct commit *commit;
-
-			commit = lookup_commit_reference_gently(oid->hash, 1);
-			if (!commit)
-				return 0;
-			if (!contains(commit, filter->with_commit))
-				return 0;
-		}
-
-		if (filter->points_at.nr && !match_points_at(refname, oid->hash, &filter->points_at))
-			return 0;
-
-		if (!filter->lines) {
-			if (tag_sort)
-				ref_array_append(array, refname);
-			else
-				printf("%s\n", refname);
-			return 0;
-		}
-		printf("%-15s ", refname);
-		show_tag_lines(oid, filter->lines);
-		putchar('\n');
-	}
-
-	return 0;
-}
-
-static int sort_by_version(const void *a_, const void *b_)
-{
-	const struct ref_array_item *a = *((struct ref_array_item **)a_);
-	const struct ref_array_item *b = *((struct ref_array_item **)b_);
-	return versioncmp(a->refname, b->refname);
-}
-
-static int list_tags(struct ref_filter *filter, int sort)
+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
 {
 	struct ref_array array;
-	struct ref_filter_cbdata data;
+	char *format;
+	int i;
 
 	memset(&array, 0, sizeof(array));
-	data.array = &array;
-	data.filter = filter;
 
 	if (filter->lines == -1)
 		filter->lines = 0;
 
-	for_each_tag_ref(show_reference, &data);
-	if (sort) {
-		int i;
-		if ((sort & SORT_MASK) == VERCMP_SORT)
-			qsort(array.items, array.nr,
-			      sizeof(struct ref_array_item *), sort_by_version);
-		if (sort & REVERSE_SORT)
-			for (i = array.nr - 1; i >= 0; i--)
-				printf("%s\n", array.items[i]->refname);
-		else
-			for (i = 0; i < array.nr; i++)
-				printf("%s\n", array.items[i]->refname);
-		ref_array_clear(&array);
-	}
+	if (filter->lines)
+		format = "%(align:16,left)%(refname:short)%(end)";
+	else
+		format = "%(refname:short)";
+
+	verify_ref_format(format);
+	filter_refs(&array, filter, FILTER_REFS_TAGS);
+	ref_array_sort(sorting, &array);
+
+	for (i = 0; i < array.nr; i++)
+		show_ref_array_item(array.items[i], format, QUOTE_NONE, filter->lines);
+	ref_array_clear(&array);
+
 	return 0;
 }
 
@@ -366,35 +120,26 @@ static const char tag_template_nocleanup[] =
 	"Lines starting with '%c' will be kept; you may remove them"
 	" yourself if you want to.\n");
 
-/*
- * Parse a sort string, and return 0 if parsed successfully. Will return
- * non-zero when the sort string does not parse into a known type. If var is
- * given, the error message becomes a warning and includes information about
- * the configuration value.
- */
-static int parse_sort_string(const char *var, const char *arg, int *sort)
+/* Parse arg given and add it the ref_sorting array */
+static int parse_sorting_string(const char *arg, struct ref_sorting **sorting_tail)
 {
-	int type = 0, flags = 0;
-
-	if (skip_prefix(arg, "-", &arg))
-		flags |= REVERSE_SORT;
+	struct ref_sorting *s;
+	int len;
 
-	if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
-		type = VERCMP_SORT;
-	else
-		type = STRCMP_SORT;
+	s = xcalloc(1, sizeof(*s));
+	s->next = *sorting_tail;
+	*sorting_tail = s;
 
-	if (strcmp(arg, "refname")) {
-		if (!var)
-			return error(_("unsupported sort specification '%s'"), arg);
-		else {
-			warning(_("unsupported sort specification '%s' in variable '%s'"),
-				var, arg);
-			return -1;
-		}
+	if (*arg == '-') {
+		s->reverse = 1;
+		arg++;
 	}
+	if (skip_prefix(arg, "version:", &arg) ||
+	    skip_prefix(arg, "v:", &arg))
+		s->version = 1;
 
-	*sort = (type | flags);
+	len = strlen(arg);
+	s->atom = parse_ref_filter_atom(arg, arg+len);
 
 	return 0;
 }
@@ -402,11 +147,12 @@ static int parse_sort_string(const char *var, const char *arg, int *sort)
 static int git_tag_config(const char *var, const char *value, void *cb)
 {
 	int status;
+	struct ref_sorting **sorting_tail = (struct ref_sorting **)cb;
 
 	if (!strcmp(var, "tag.sort")) {
 		if (!value)
 			return config_error_nonbool(var);
-		parse_sort_string(var, value, &tag_sort);
+		parse_sorting_string(value, sorting_tail);
 		return 0;
 	}
 
@@ -564,13 +310,6 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
 	return check_refname_format(sb->buf, 0);
 }
 
-static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
-{
-	int *sort = opt->value;
-
-	return parse_sort_string(NULL, arg, sort);
-}
-
 int cmd_tag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -587,6 +326,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
 	struct ref_filter filter;
+	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
@@ -613,10 +353,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
 		OPT_CONTAINS(&filter.with_commit, N_("print only tags that contain the commit")),
 		OPT_WITH(&filter.with_commit, N_("print only tags that contain the commit")),
-		{
-			OPTION_CALLBACK, 0, "sort", &tag_sort, N_("type"), N_("sort tags"),
-			PARSE_OPT_NONEG, parse_opt_sort
-		},
+		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		{
 			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
 			N_("print only tags of the object"), 0, parse_opt_object_name
@@ -624,7 +362,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	git_config(git_tag_config, NULL);
+	git_config(git_tag_config, sorting_tail);
 
 	memset(&opt, 0, sizeof(opt));
 	memset(&filter, 0, sizeof(filter));
@@ -650,6 +388,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			die(_("--column and -n are incompatible"));
 		colopts = 0;
 	}
+	if (!sorting)
+		sorting = ref_default_sorting();
 	if (cmdmode == 'l') {
 		int ret;
 		if (column_active(colopts)) {
@@ -658,10 +398,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			copts.padding = 2;
 			run_column_filter(colopts, &copts);
 		}
-		if (filter.lines != -1 && tag_sort)
-			die(_("--sort and -n are incompatible"));
 		filter.name_patterns = argv;
-		ret = list_tags(&filter, tag_sort);
+		ret = list_tags(&filter, sorting);
 		if (column_active(colopts))
 			stop_column_filter();
 		return ret;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index d31788c..84153ef 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1462,13 +1462,7 @@ test_expect_success 'invalid sort parameter on command line' '
 
 test_expect_success 'invalid sort parameter in configuratoin' '
 	git config tag.sort "v:notvalid" &&
-	git tag -l "foo*" >actual &&
-	cat >expect <<-\EOF &&
-	foo1.10
-	foo1.3
-	foo1.6
-	EOF
-	test_cmp expect actual
+	test_must_fail git tag -l "foo*"
 '
 
 test_expect_success 'version sort with prerelease reordering' '
-- 
2.5.0

[PATCH v12 12/13] tag.c: implement '--format' option

[Karthik Nayak]

From: Karthik Nayak <karthik.188@gmail.com>

Implement the '--format' option provided by 'ref-filter'.
This lets the user list tags as per desired format similar
to the implementation in 'git for-each-ref'.

Add tests and documentation for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-tag.txt | 15 ++++++++++++++-
 builtin/tag.c             | 11 +++++++----
 t/t7004-tag.sh            | 16 ++++++++++++++++
 3 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 3ac4a96..75703c5 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -13,7 +13,8 @@ SYNOPSIS
 	<tagname> [<commit> | <object>]
 'git tag' -d <tagname>...
 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
-	[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>] [<pattern>...]
+	[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
+	[--format=<format>] [<pattern>...]
 'git tag' -v <tagname>...
 
 DESCRIPTION
@@ -158,6 +159,18 @@ This option is only applicable when listing tags without annotation lines.
 	The object that the new tag will refer to, usually a commit.
 	Defaults to HEAD.
 
+<format>::
+	A string that interpolates `%(fieldname)` from the object
+	pointed at by a ref being shown.  If `fieldname` is prefixed
+	with an asterisk (`*`) and the ref points at a tag object, the
+	value for the field in the object tag refers is used.  When
+	unspecified, defaults to `%(refname:short)`.  It also
+	interpolates `%%` to `%`, and `%xx` where `xx` 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).  The fields are same as those in `git
+	for-each-ref`.
+
 
 CONFIGURATION
 -------------
diff --git a/builtin/tag.c b/builtin/tag.c
index 501fc52..69997a4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -30,10 +30,9 @@ static const char * const git_tag_usage[] = {
 
 static unsigned int colopts;
 
-static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
 	struct ref_array array;
-	char *format;
 	int i;
 
 	memset(&array, 0, sizeof(array));
@@ -43,7 +42,7 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
 
 	if (filter->lines)
 		format = "%(align:16,left)%(refname:short)%(end)";
-	else
+	else if (!format)
 		format = "%(refname:short)";
 
 	verify_ref_format(format);
@@ -327,6 +326,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct strbuf err = STRBUF_INIT;
 	struct ref_filter filter;
 	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
+	const char *format = NULL;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
@@ -359,6 +359,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
 			N_("print only tags of the object"), 0, parse_opt_object_name
 		},
+		OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
 		OPT_END()
 	};
 
@@ -398,8 +399,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			copts.padding = 2;
 			run_column_filter(colopts, &copts);
 		}
+		if (format && (filter.lines != -1))
+			die(_("--format and -n are incompatible"));
 		filter.name_patterns = argv;
-		ret = list_tags(&filter, sorting);
+		ret = list_tags(&filter, sorting, format);
 		if (column_active(colopts))
 			stop_column_filter();
 		return ret;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 84153ef..26670e0 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1519,4 +1519,20 @@ EOF"
 	test_cmp expect actual
 '
 
+test_expect_success '--format cannot be used with -n' '
+	test_must_fail git tag -l -n4 --format="%(refname)"
+'
+
+test_expect_success '--format should list tags as per format given' '
+	cat >expect <<-\EOF &&
+	refname : refs/tags/foo1.10
+	refname : refs/tags/foo1.3
+	refname : refs/tags/foo1.6
+	refname : refs/tags/foo1.6-rc1
+	refname : refs/tags/foo1.6-rc2
+	EOF
+	git tag -l --format="refname : %(refname)" "foo*" >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.5.0

[PATCH v12 13/13] tag.c: implement '--merged' and '--no-merged' options

[Karthik Nayak]

From: Karthik Nayak <karthik.188@gmail.com>

Use 'ref-filter' APIs to implement the '--merged' and '--no-merged'
options into 'tag.c'. The '--merged' option lets the user to only list
tags merged into the named commit. The '--no-merged' option lets the
user to only list tags not merged into the named commit.  If no object
is provided it assumes HEAD as the object.

Add documentation and tests for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-tag.txt |  7 ++++++-
 builtin/tag.c             |  6 +++++-
 t/t7004-tag.sh            | 27 +++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 75703c5..c2785d9 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -14,7 +14,7 @@ SYNOPSIS
 'git tag' -d <tagname>...
 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
 	[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
-	[--format=<format>] [<pattern>...]
+	[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]
 'git tag' -v <tagname>...
 
 DESCRIPTION
@@ -171,6 +171,11 @@ This option is only applicable when listing tags without annotation lines.
 	`%0a` to `\n` (LF).  The fields are same as those in `git
 	for-each-ref`.
 
+--[no-]merged [<commit>]::
+	Only list tags whose tips are reachable, or not reachable
+	if '--no-merged' is used, from the specified commit ('HEAD'
+	if not specified).
+
 
 CONFIGURATION
 -------------
diff --git a/builtin/tag.c b/builtin/tag.c
index 69997a4..781d3e5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -23,7 +23,7 @@ static const char * const git_tag_usage[] = {
 	N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] <tagname> [<head>]"),
 	N_("git tag -d <tagname>..."),
 	N_("git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>]"
-		"\n\t\t[<pattern>...]"),
+		"\n\t\t[--[no-]merged [<commit>]] [<pattern>...]"),
 	N_("git tag -v <tagname>..."),
 	NULL
 };
@@ -353,6 +353,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
 		OPT_CONTAINS(&filter.with_commit, N_("print only tags that contain the commit")),
 		OPT_WITH(&filter.with_commit, N_("print only tags that contain the commit")),
+		OPT_MERGED(&filter, N_("print only tags that are merged")),
+		OPT_NO_MERGED(&filter, N_("print only tags that are not merged")),
 		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
 			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		{
@@ -413,6 +415,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		die(_("--contains option is only allowed with -l."));
 	if (filter.points_at.nr)
 		die(_("--points-at option is only allowed with -l."));
+	if (filter.merge_commit)
+		die(_("--merged and --no-merged option are only allowed with -l"));
 	if (cmdmode == 'd')
 		return for_each_tag_name(argv, delete_tag);
 	if (cmdmode == 'v')
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 26670e0..335396e 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1535,4 +1535,31 @@ test_expect_success '--format should list tags as per format given' '
 	test_cmp expect actual
 '
 
+test_expect_success 'setup --merged test tags' '
+	git tag mergetest-1 HEAD~2 &&
+	git tag mergetest-2 HEAD~1 &&
+	git tag mergetest-3 HEAD
+'
+
+test_expect_success '--merged cannot be used in non-list mode' '
+	test_must_fail git tag --merged=mergetest-2 foo
+'
+
+test_expect_success '--merged shows merged tags' '
+	cat >expect <<-\EOF &&
+	mergetest-1
+	mergetest-2
+	EOF
+	git tag -l --merged=mergetest-2 mergetest-* >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--no-merged show unmerged tags' '
+	cat >expect <<-\EOF &&
+	mergetest-3
+	EOF
+	git tag -l --no-merged=mergetest-2 mergetest-* >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.5.0

Re: [PATCH v12 00/13] port tag.c to use ref-filter.c APIs

[Eric Sunshine]

On Tue, Aug 18, 2015 at 2:37 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Version 11 was posted here:
> http://article.gmane.org/gmane.comp.version-control.git/275997
>
> Changes in this version:
> * Small style and formatting changes.
> * Remove unnecessary variable from push_new_state().
> * pop_state doesn't return a value now and attaches the buffer
>   into the previous state.
> * use strcmp() rather than starts_with() while checking for
>   alignment position.
> * Change attend to at_end
>
> Interdiff:
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 3099631..760d719 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -128,8 +128,8 @@ color::
>         are described in `color.branch.*`.
>
>  align::
> -       left-, middle-, or right-align the content between %(align:..)
> -       and %(end). Followed by `:<position>,<width>`, where the
> +       Left-, middle-, or right-align the content between %(align:..)
> +       and %(end). Followed by `:<width>>,<position>`, where the

I haven't had a chance to look closely at this version yet, but this
popped out while quickly scanning the interdiff since the previous
round had the same sort of problem:

    s/>>/>/

>         `<position>` is either left, right or middle and `<width>` is
>         the total length of the content with alignment. If the
>         contents length is more than the width then no alignment is

Re: [PATCH v12 00/13] port tag.c to use ref-filter.c APIs

[Karthik Nayak]

On Wed, Aug 19, 2015 at 12:48 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Aug 18, 2015 at 2:37 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Version 11 was posted here:
>> http://article.gmane.org/gmane.comp.version-control.git/275997
>>
>> Changes in this version:
>> * Small style and formatting changes.
>> * Remove unnecessary variable from push_new_state().
>> * pop_state doesn't return a value now and attaches the buffer
>>   into the previous state.
>> * use strcmp() rather than starts_with() while checking for
>>   alignment position.
>> * Change attend to at_end
>>
>> Interdiff:
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index 3099631..760d719 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -128,8 +128,8 @@ color::
>>         are described in `color.branch.*`.
>>
>>  align::
>> -       left-, middle-, or right-align the content between %(align:..)
>> -       and %(end). Followed by `:<position>,<width>`, where the
>> +       Left-, middle-, or right-align the content between %(align:..)
>> +       and %(end). Followed by `:<width>>,<position>`, where the
>
> I haven't had a chance to look closely at this version yet, but this
> popped out while quickly scanning the interdiff since the previous
> round had the same sort of problem:
>
>     s/>>/>/
>
>>         `<position>` is either left, right or middle and `<width>` is
>>         the total length of the content with alignment. If the
>>         contents length is more than the width then no alignment is

Weird, I thought I fixed that, I'll put in a squash.

-- 
Regards,
Karthik Nayak

Re: [PATCH v12 05/13] ref-filter: implement an `align` atom

[Karthik Nayak]

This needs to be squashed in:

diff --git a/Documentation/git-for-each-ref.txt
b/Documentation/git-for-each-ref.txt
index fe7889c..0a89f29 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -129,7 +129,7 @@ color::

 align::
        Left-, middle-, or right-align the content between %(align:..)
-       and %(end). Followed by `:<width>>,<position>`, where the
+       and %(end). Followed by `:<width>,<position>`, where the
        `<position>` is either left, right or middle and `<width>` is
        the total length of the content with alignment. If the
        contents length is more than the width then no alignment is

On Wed, Aug 19, 2015 at 12:07 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Implement an `align` atom which left-, middle-, or right-aligns the
> content between %(align:..) and %(end).
>
> It is followed by `:<width>,<position>`, where the `<position>` is
> either left, right or middle and `<width>` is the size of the area
> into which the content will be placed. If the content between
> %(align:) and %(end) is more than the width then no alignment is
> performed. e.g. to align a refname atom to the middle with a total
> width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".
>
> We now have a `handler()` for each atom_value which will be called
> when that atom_value is being parsed, and similarly an `at_end`
> function for each state which is to be called when the `end` atom is
> encountered. Using this implement the `align` atom which aligns the
> given strbuf by calling `strbuf_utf8_align()` from utf8.c
>
> This is done by assigning a 'handler' function to each atom_value and
> a related 'at_end' function for each state. The align_handler()
> defined uses strbuf_utf8_align() from utf8.c to perform the alignment.
>
> Add documentation and tests for the same.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  Documentation/git-for-each-ref.txt |  8 +++++
>  ref-filter.c                       | 73 ++++++++++++++++++++++++++++++++++++++
>  t/t6302-for-each-ref-filter.sh     | 48 +++++++++++++++++++++++++
>  3 files changed, 129 insertions(+)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index e49d578..fe7889c 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -127,6 +127,14 @@ color::
>         Change output color.  Followed by `:<colorname>`, where names
>         are described in `color.branch.*`.
>
> +align::
> +       Left-, middle-, or right-align the content between %(align:..)
> +       and %(end). Followed by `:<width>>,<position>`, where the
> +       `<position>` is either left, right or middle and `<width>` is
> +       the total length of the content with alignment. If the
> +       contents length is more than the width then no alignment is
> +       performed.
> +
>  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/ref-filter.c b/ref-filter.c
> index 74532d3..ecbcc5a 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -10,6 +10,7 @@
>  #include "quote.h"
>  #include "ref-filter.h"
>  #include "revision.h"
> +#include "utf8.h"
>
>  typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>
> @@ -53,16 +54,27 @@ static struct {
>         { "flag" },
>         { "HEAD" },
>         { "color" },
> +       { "align" },
> +       { "end" },
> +};
> +
> +struct align {
> +       align_type position;
> +       unsigned int width;
>  };
>
>  struct ref_formatting_state {
>         struct ref_formatting_state *prev;
>         struct strbuf output;
> +       void (*at_end)(struct ref_formatting_state *state);
> +       void *cb_data;
>         int quote_style;
>  };
>
>  struct atom_value {
>         const char *s;
> +       struct align *align;
> +       void (*handler)(struct atom_value *atomv, struct ref_formatting_state **state);
>         unsigned long ul; /* used for sorting when not FIELD_STR */
>  };
>
> @@ -626,6 +638,36 @@ static inline char *copy_advance(char *dst, const char *src)
>         return dst;
>  }
>
> +static void align_handler(struct ref_formatting_state *state)
> +{
> +       struct align *align = (struct align *)state->cb_data;
> +       struct strbuf s = STRBUF_INIT;
> +
> +       strbuf_utf8_align(&s, align->position, align->width, state->output.buf);
> +       strbuf_reset(&state->output);
> +       strbuf_addbuf(&state->output, &s);
> +       free(align);
> +}
> +
> +static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)
> +{
> +       struct ref_formatting_state *new;
> +
> +       push_new_state(state);
> +       new = *state;
> +       new->at_end = align_handler;
> +       new->cb_data = atomv->align;
> +}
> +
> +static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)
> +{
> +       struct ref_formatting_state *current = *state;
> +       if (!current->at_end)
> +               die(_("format: `end` atom used without a supporting atom"));
> +       current->at_end(current);
> +       pop_state(state);
> +}
> +
>  /*
>   * Parse the object referred by ref, and grab needed value.
>   */
> @@ -654,6 +696,7 @@ static void populate_value(struct ref_array_item *ref)
>                 int deref = 0;
>                 const char *refname;
>                 const char *formatp;
> +               const char *valp;
>                 struct branch *branch = NULL;
>
>                 if (*name == '*') {
> @@ -719,6 +762,34 @@ static void populate_value(struct ref_array_item *ref)
>                         else
>                                 v->s = " ";
>                         continue;
> +               } else if (skip_prefix(name, "align:", &valp)) {
> +                       struct align *align = xmalloc(sizeof(struct align));
> +                       char *ep = strchr(valp, ',');
> +
> +                       if (ep)
> +                               *ep = '\0';
> +
> +                       if (strtoul_ui(valp, 10, &align->width))
> +                               die(_("positive width expected align:%s"), valp);
> +
> +                       if (!ep || !strcmp(ep + 1, "left"))
> +                               align->position = ALIGN_LEFT;
> +                       else if (!strcmp(ep + 1, "right"))
> +                               align->position = ALIGN_RIGHT;
> +                       else if (!strcmp(ep + 1, "middle"))
> +                               align->position = ALIGN_MIDDLE;
> +                       else
> +                               die(_("improper format entered align:%s"), ep + 1);
> +
> +                       if (ep)
> +                               *ep = ',';
> +
> +                       v->align = align;
> +                       v->handler = align_atom_handler;
> +                       continue;
> +               } else if (!strcmp(name, "end")) {
> +                       v->handler = end_atom_handler;
> +                       continue;
>                 } else
>                         continue;
>
> @@ -1297,6 +1368,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
>                 if (cp < sp)
>                         append_literal(cp, sp, state);
>                 get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
> +               if (atomv->handler)
> +                       atomv->handler(atomv, &state);
>                 append_atom(atomv, state);
>         }
>         if (*cp) {
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 505a360..b252a50 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -81,4 +81,52 @@ test_expect_success 'filtering with --contains' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'left alignment' '
> +       cat >expect <<-\EOF &&
> +       refname is refs/heads/master  |refs/heads/master
> +       refname is refs/heads/side    |refs/heads/side
> +       refname is refs/odd/spot      |refs/odd/spot
> +       refname is refs/tags/double-tag|refs/tags/double-tag
> +       refname is refs/tags/four     |refs/tags/four
> +       refname is refs/tags/one      |refs/tags/one
> +       refname is refs/tags/signed-tag|refs/tags/signed-tag
> +       refname is refs/tags/three    |refs/tags/three
> +       refname is refs/tags/two      |refs/tags/two
> +       EOF
> +       git for-each-ref --format="%(align:30,left)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'middle alignment' '
> +       cat >expect <<-\EOF &&
> +       | refname is refs/heads/master |refs/heads/master
> +       |  refname is refs/heads/side  |refs/heads/side
> +       |   refname is refs/odd/spot   |refs/odd/spot
> +       |refname is refs/tags/double-tag|refs/tags/double-tag
> +       |  refname is refs/tags/four   |refs/tags/four
> +       |   refname is refs/tags/one   |refs/tags/one
> +       |refname is refs/tags/signed-tag|refs/tags/signed-tag
> +       |  refname is refs/tags/three  |refs/tags/three
> +       |   refname is refs/tags/two   |refs/tags/two
> +       EOF
> +       git for-each-ref --format="|%(align:30,middle)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'right alignment' '
> +       cat >expect <<-\EOF &&
> +       |  refname is refs/heads/master|refs/heads/master
> +       |    refname is refs/heads/side|refs/heads/side
> +       |      refname is refs/odd/spot|refs/odd/spot
> +       |refname is refs/tags/double-tag|refs/tags/double-tag
> +       |     refname is refs/tags/four|refs/tags/four
> +       |      refname is refs/tags/one|refs/tags/one
> +       |refname is refs/tags/signed-tag|refs/tags/signed-tag
> +       |    refname is refs/tags/three|refs/tags/three
> +       |      refname is refs/tags/two|refs/tags/two
> +       EOF
> +       git for-each-ref --format="|%(align:30,right)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.5.0
>



-- 
Regards,
Karthik Nayak

Re: [PATCH v12 12/13] tag.c: implement '--format' option

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -158,6 +159,18 @@ This option is only applicable when listing tags without annotation lines.
>  	The object that the new tag will refer to, usually a commit.
>  	Defaults to HEAD.
>  
> +<format>::
> +	A string that interpolates `%(fieldname)` from the object
> +	pointed at by a ref being shown.  If `fieldname` is prefixed
> +	with an asterisk (`*`) and the ref points at a tag object, the
> +	value for the field in the object tag refers is used.  When
> +	unspecified, defaults to `%(refname:short)`.  It also
> +	interpolates `%%` to `%`, and `%xx` where `xx` 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).  The fields are same as those in `git
> +	for-each-ref`.
> +

This documentation should probably be shortened to stg like

	A string that interpolates `%(fieldname)` from the object
	pointed at by a ref being shown. The format is the same as the
	one of linkgit:git-for-each-ref[1]. When unspecified, defaults
	to `%(refname:short)`

Alternatively, you can extract the "FIELD NAMES" section of
git-for-each-ref.txt to a separate file and include it in the doc for
each command having this --format option (this is how it's done for "git
log --format" IIRC). But taking that much space to describe hexadecimal
escapes that very few people would use and not documenting the %(atoms)
is counter-productive IMHO.

I would favor the first option (keep it short, include a pointer) with
Junio's remark in mind: "git tag" and "git branch" are meant to be
simple commands, and the scary swiss-army-knife should remain "git
for-each-ref".

I am still (slightly) in favor of adding --format to tag and branch, as
long as it does not make the commands too scary.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v12 10/13] tag.c: use 'ref-filter' data structures

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> This is a temporary step before porting 'tag.c' to use 'ref-filter'
> completely. As this is a temporary step, most of the code
> introduced here will be removed when 'tag.c' is ported over to use
> 'ref-filter' APIs

If you resend: missing '.' at the end of sentence.

> -	if (lines != -1)
> +	if (filter.lines != -1)
>  		die(_("-n option is only allowed with -l."));
> -	if (with_commit)
> +	if (filter.with_commit)
>  		die(_("--contains option is only allowed with -l."));
> -	if (points_at.nr)
> +	if (filter.points_at.nr)
>  		die(_("--points-at option is only allowed with -l."));

It may make sense to factor these checks into a function like

  void check_filter_consistancy(struct ref_filter *filter)

in ref-filter.c, since for-each-ref, branch and tag will eventually have
the same set of constraints on the options.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v12 01/13] ref-filter: move `struct atom_value` to ref-filter.c

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> +static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)
> +{
> +	struct ref_formatting_state *current = *state;
> +	if (!current->at_end)
> +		die(_("format: `end` atom used without a supporting atom"));

You error out on %(end) without %(align), but not on %(align) without
%(end).

You should probably check that the stack is empty at the end and error
out otherwise.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> +static void pop_state(struct ref_formatting_state **stack)
> +{
> +	struct ref_formatting_state *current = *stack;
> +	struct ref_formatting_state *prev = current->prev;
> +
> +	if (prev)
> +		strbuf_addbuf(&prev->output, &current->output);

I find this "if (prev)" suspicious: if there's a previous element in the
stack, push to it, but otherwise, you're throwing away the content of
the stack top silently.

Given the rest of the patch, this is correct, since you're using
state->output before pop_state(), but I find it weird to have the same
function to actually pop a state, and to destroy the last element.

Just thinking out loudly, I don't have specific alternative to propose
here.

> @@ -1262,23 +1284,24 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
>  void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
>  {
>  	const char *cp, *sp, *ep;
> -	struct ref_formatting_state state;
> +	struct strbuf *final_buf;
> +	struct ref_formatting_state *state = NULL;
>  
> -	strbuf_init(&state.output, 0);
> -	state.quote_style = quote_style;
> +	push_new_state(&state);
> +	state->quote_style = quote_style;

I do not think that the quote_style should belong to the stack. At the
moment, only the bottom of the stack has it set, and as a result you're
getting weird results like:

$ ./git for-each-ref --shell --format '|%(align:80,left)<%(author)>%(end)|' | head -n 3 
|<Junio C Hamano <gitster@pobox.com> 1435173702 -0700>                           ''|
|<Junio C Hamano <gitster@pobox.com> 1435173701 -0700>                           ''|
|<Junio C Hamano <gitster@pobox.com> 1433277352 -0700>                           ''|

See, the '' are inserted where the %(end) was, but not around atoms as
one would expect.

One stupid fix would be to propagate the quote_style accross the stack,
like this:

--- a/ref-filter.c
+++ b/ref-filter.c
@@ -155,6 +155,8 @@ static void push_new_state(struct ref_formatting_state **stack)
 
        strbuf_init(&s->output, 0);
        s->prev = *stack;
+       if (*stack)
+               s->quote_style = (*stack)->quote_style;
        *stack = s;
 }
 

After applying this, I do get the '' around the author (= correct
behavior I think), but then one wonders even more why this is part of
the stack.

You replaced the quote_style argument with ref_formatting_state, and I
think you should have kept this argument and added ref_formatting_state.
The other option is to add an extra indirection like

struct ref_formatting_state {
	int quote_style;
	struct ref_formatting_stack *stack;
}

(ref_formatting_stack would be what you currently call
ref_formatting_state). But that's probably overkill.

Also, after applying my toy patch above, I get useless '' around
%(align) and %(end). I can get rid of them with

--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1499,7 +1501,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format,
                get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
                if (atomv->handler)
                        atomv->handler(atomv, &state);
-               append_atom(atomv, state);
+               else
+                       append_atom(atomv, state);
        }
        if (*cp) {
                sp = cp + strlen(cp);

Unless I missed something, this second patch is sensible anyway and
should be squashed into [PATCH v12 05/13]: you don't need to call
append_atom() when you have a handler, right?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v12 01/13] ref-filter: move `struct atom_value` to ref-filter.c

[Karthik Nayak]

On Wed, Aug 19, 2015 at 8:26 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)
>> +{
>> +     struct ref_formatting_state *current = *state;
>> +     if (!current->at_end)
>> +             die(_("format: `end` atom used without a supporting atom"));
>
> You error out on %(end) without %(align), but not on %(align) without
> %(end).
>
> You should probably check that the stack is empty at the end and error
> out otherwise.
>

You're right,

    if (state->prev)
        die(_("format: `end` atom missing"));

should do before printing.

-- 
Regards,
Karthik Nayak

Re: [PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery

[Karthik Nayak]

On Wed, Aug 19, 2015 at 8:26 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +static void pop_state(struct ref_formatting_state **stack)
>> +{
>> +     struct ref_formatting_state *current = *stack;
>> +     struct ref_formatting_state *prev = current->prev;
>> +
>> +     if (prev)
>> +             strbuf_addbuf(&prev->output, &current->output);
>
> I find this "if (prev)" suspicious: if there's a previous element in the
> stack, push to it, but otherwise, you're throwing away the content of
> the stack top silently.
>
> Given the rest of the patch, this is correct, since you're using
> state->output before pop_state(), but I find it weird to have the same
> function to actually pop a state, and to destroy the last element.
>
> Just thinking out loudly, I don't have specific alternative to propose
> here.
>

Hmm, but destroying the last element is also pop'ing it off the stack in a way.
I can't think of a something else.

>> @@ -1262,23 +1284,24 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
>>  void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
>>  {
>>       const char *cp, *sp, *ep;
>> -     struct ref_formatting_state state;
>> +     struct strbuf *final_buf;
>> +     struct ref_formatting_state *state = NULL;
>>
>> -     strbuf_init(&state.output, 0);
>> -     state.quote_style = quote_style;
>> +     push_new_state(&state);
>> +     state->quote_style = quote_style;
>
> I do not think that the quote_style should belong to the stack. At the
> moment, only the bottom of the stack has it set, and as a result you're
> getting weird results like:
>
> $ ./git for-each-ref --shell --format '|%(align:80,left)<%(author)>%(end)|' | head -n 3
> |<Junio C Hamano <gitster@pobox.com> 1435173702 -0700>                           ''|
> |<Junio C Hamano <gitster@pobox.com> 1435173701 -0700>                           ''|
> |<Junio C Hamano <gitster@pobox.com> 1433277352 -0700>                           ''|
>
> See, the '' are inserted where the %(end) was, but not around atoms as
> one would expect.
>
> One stupid fix would be to propagate the quote_style accross the stack,
> like this:
>
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -155,6 +155,8 @@ static void push_new_state(struct ref_formatting_state **stack)
>
>         strbuf_init(&s->output, 0);
>         s->prev = *stack;
> +       if (*stack)
> +               s->quote_style = (*stack)->quote_style;
>         *stack = s;
>  }
>
>

This seems about right, why do you think it's a stupid fix?

> After applying this, I do get the '' around the author (= correct
> behavior I think), but then one wonders even more why this is part of
> the stack.
>
> You replaced the quote_style argument with ref_formatting_state, and I
> think you should have kept this argument and added ref_formatting_state.
> The other option is to add an extra indirection like
>
> struct ref_formatting_state {
>         int quote_style;
>         struct ref_formatting_stack *stack;
> }
>
> (ref_formatting_stack would be what you currently call
> ref_formatting_state). But that's probably overkill.
>

Yes, seems like an overkill.

> Also, after applying my toy patch above, I get useless '' around
> %(align) and %(end). I can get rid of them with
>
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1499,7 +1501,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format,
>                 get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
>                 if (atomv->handler)
>                         atomv->handler(atomv, &state);
> -               append_atom(atomv, state);
> +               else
> +                       append_atom(atomv, state);
>         }
>         if (*cp) {
>                 sp = cp + strlen(cp);
>
> Unless I missed something, this second patch is sensible anyway and
> should be squashed into [PATCH v12 05/13]: you don't need to call
> append_atom() when you have a handler, right?

Yes, this I'll squash into 05/13.

-- 
Regards,
Karthik Nayak

Re: [PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -155,6 +155,8 @@ static void push_new_state(struct ref_formatting_state **stack)
>>
>>         strbuf_init(&s->output, 0);
>>         s->prev = *stack;
>> +       if (*stack)
>> +               s->quote_style = (*stack)->quote_style;
>>         *stack = s;
>>  }
>>
>>
>
> This seems about right, why do you think it's a stupid fix?

If you have a stack of N elemments, why replicate a field N times if all
the N instances always have the same value?

There's nothing to be pushed or poped with quote_style, so having it in
the stack is confusing to the reader (one has to infer the property "all
instances have the same value" by reading the code instead of having
just one variable), and error-prone for the author: you already got
it wrong once.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v12 10/13] tag.c: use 'ref-filter' data structures

[Karthik Nayak]

On Wed, Aug 19, 2015 at 8:26 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> This is a temporary step before porting 'tag.c' to use 'ref-filter'
>> completely. As this is a temporary step, most of the code
>> introduced here will be removed when 'tag.c' is ported over to use
>> 'ref-filter' APIs
>
> If you resend: missing '.' at the end of sentence.
>

Ok will add.

>> -     if (lines != -1)
>> +     if (filter.lines != -1)
>>               die(_("-n option is only allowed with -l."));
>> -     if (with_commit)
>> +     if (filter.with_commit)
>>               die(_("--contains option is only allowed with -l."));
>> -     if (points_at.nr)
>> +     if (filter.points_at.nr)
>>               die(_("--points-at option is only allowed with -l."));
>
> It may make sense to factor these checks into a function like
>
>   void check_filter_consistancy(struct ref_filter *filter)
>
> in ref-filter.c, since for-each-ref, branch and tag will eventually have
> the same set of constraints on the options.
>

Ah! this is needed only in branch and tag where we need to see if the options
are used with tag.c and both have a different sort of check for this.
Might need more of a cleanup in branch.c and tag.c before we could do something
like this so that we could have a similar check.

for reference branch.c uses:

    if (!!delete + !!rename + !!new_upstream +
        list + unset_upstream > 1)
        usage_with_options(builtin_branch_usage, options);

whereas tag.c uses what you stated above.

-- 
Regards,
Karthik Nayak

Re: [PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery

[Karthik Nayak]

On Wed, Aug 19, 2015 at 9:14 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>> --- a/ref-filter.c
>>> +++ b/ref-filter.c
>>> @@ -155,6 +155,8 @@ static void push_new_state(struct ref_formatting_state **stack)
>>>
>>>         strbuf_init(&s->output, 0);
>>>         s->prev = *stack;
>>> +       if (*stack)
>>> +               s->quote_style = (*stack)->quote_style;
>>>         *stack = s;
>>>  }
>>>
>>>
>>
>> This seems about right, why do you think it's a stupid fix?
>
> If you have a stack of N elemments, why replicate a field N times if all
> the N instances always have the same value?
>
> There's nothing to be pushed or poped with quote_style, so having it in
> the stack is confusing to the reader (one has to infer the property "all
> instances have the same value" by reading the code instead of having
> just one variable), and error-prone for the author: you already got
> it wrong once.

Thats also there, I'll guess it makes more sense to remove it from
ref_formatting_state.

-- 
Regards,
Karthik Nayak

Re: [PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery

[Karthik Nayak]

On Wed, Aug 19, 2015 at 9:24 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Wed, Aug 19, 2015 at 9:14 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>>> --- a/ref-filter.c
>>>> +++ b/ref-filter.c
>>>> @@ -155,6 +155,8 @@ static void push_new_state(struct ref_formatting_state **stack)
>>>>
>>>>         strbuf_init(&s->output, 0);
>>>>         s->prev = *stack;
>>>> +       if (*stack)
>>>> +               s->quote_style = (*stack)->quote_style;
>>>>         *stack = s;
>>>>  }
>>>>
>>>>
>>>
>>> This seems about right, why do you think it's a stupid fix?
>>
>> If you have a stack of N elemments, why replicate a field N times if all
>> the N instances always have the same value?
>>
>> There's nothing to be pushed or poped with quote_style, so having it in
>> the stack is confusing to the reader (one has to infer the property "all
>> instances have the same value" by reading the code instead of having
>> just one variable), and error-prone for the author: you already got
>> it wrong once.
>
> Thats also there, I'll guess it makes more sense to remove it from
> ref_formatting_state.
>

Speaking of quote_value, The quote doesn't work well with color's
for e.g.
git for-each-ref --shell --format="%(color:green)%(refname)"
'''refs/heads/allow-unknown-type'''
Seems like an simple fix, probably after GSoC I'll do this :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> You replaced the quote_style argument with ref_formatting_state, and I
> think you should have kept this argument and added ref_formatting_state.
> The other option is to add an extra indirection like
>
> struct ref_formatting_state {
> 	int quote_style;
> 	struct ref_formatting_stack *stack;
> }
>
> (ref_formatting_stack would be what you currently call
> ref_formatting_state). But that's probably overkill.

I think this is the right way to go.  As you explained in your later
messages, this is conceptually a global setting that applies to
anybody working in the callchain and not something individual
recursion levels would want to muck with.

Thanks.

Re: [PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> Speaking of quote_value, The quote doesn't work well with color's
> for e.g.
> git for-each-ref --shell --format="%(color:green)%(refname)"
> '''refs/heads/allow-unknown-type'''
> Seems like an simple fix, probably after GSoC I'll do this :)

Anyway, the %(color) is really meant to be displayed on-screen, and the
quoting is really meant to feed the value to another program, so I can
hardly imagine a use-case where you would want both.

But the current behavior seems fine to me: the color escape sequence is
quoted, which is good. For example, you can

x=$(git for-each-ref --shell --format="nocolor%(color:green)%(refname)" | head -n 1)
sh -c "echo $x"

it will actually display "nocolor" without color, then a green refname.
I'm not sure the quoting is really necessary, but it doesn't harm and it
makes sense since the escape sequence contains a '[' which is a shell
metacharacter.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery

[Karthik Nayak]

On Thu, Aug 20, 2015 at 12:59 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Speaking of quote_value, The quote doesn't work well with color's
>> for e.g.
>> git for-each-ref --shell --format="%(color:green)%(refname)"
>> '''refs/heads/allow-unknown-type'''
>> Seems like an simple fix, probably after GSoC I'll do this :)
>
> Anyway, the %(color) is really meant to be displayed on-screen, and the
> quoting is really meant to feed the value to another program, so I can
> hardly imagine a use-case where you would want both.
>
> But the current behavior seems fine to me: the color escape sequence is
> quoted, which is good. For example, you can
>
> x=$(git for-each-ref --shell --format="nocolor%(color:green)%(refname)" | head -n 1)
> sh -c "echo $x"
>
> it will actually display "nocolor" without color, then a green refname.
> I'm not sure the quoting is really necessary, but it doesn't harm and it
> makes sense since the escape sequence contains a '[' which is a shell
> metacharacter.
>

Thanks for explaining that! I agree with whatever you've said.

-- 
Regards,
Karthik Nayak

Re: [PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery

[Karthik Nayak]

On Thu, Aug 20, 2015 at 12:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> You replaced the quote_style argument with ref_formatting_state, and I
>> think you should have kept this argument and added ref_formatting_state.
>> The other option is to add an extra indirection like
>>
>> struct ref_formatting_state {
>>       int quote_style;
>>       struct ref_formatting_stack *stack;
>> }
>>
>> (ref_formatting_stack would be what you currently call
>> ref_formatting_state). But that's probably overkill.
>
> I think this is the right way to go.  As you explained in your later
> messages, this is conceptually a global setting that applies to
> anybody working in the callchain and not something individual
> recursion levels would want to muck with.
>
> Thanks.
>

I'll work on this :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v12 12/13] tag.c: implement '--format' option

[Karthik Nayak]

On Wed, Aug 19, 2015 at 8:23 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> --- a/Documentation/git-tag.txt
>> +++ b/Documentation/git-tag.txt
>> @@ -158,6 +159,18 @@ This option is only applicable when listing tags without annotation lines.
>>       The object that the new tag will refer to, usually a commit.
>>       Defaults to HEAD.
>>
>> +<format>::
>> +     A string that interpolates `%(fieldname)` from the object
>> +     pointed at by a ref being shown.  If `fieldname` is prefixed
>> +     with an asterisk (`*`) and the ref points at a tag object, the
>> +     value for the field in the object tag refers is used.  When
>> +     unspecified, defaults to `%(refname:short)`.  It also
>> +     interpolates `%%` to `%`, and `%xx` where `xx` 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).  The fields are same as those in `git
>> +     for-each-ref`.
>> +
>
> This documentation should probably be shortened to stg like
>
>         A string that interpolates `%(fieldname)` from the object
>         pointed at by a ref being shown. The format is the same as the
>         one of linkgit:git-for-each-ref[1]. When unspecified, defaults
>         to `%(refname:short)`
>

I guess this makes sense with what you're saying below, the --format option
for tag.c is more of an extra add-on, and such a discrioption may not be needed.
Will change this.

> Alternatively, you can extract the "FIELD NAMES" section of
> git-for-each-ref.txt to a separate file and include it in the doc for
> each command having this --format option (this is how it's done for "git
> log --format" IIRC). But taking that much space to describe hexadecimal
> escapes that very few people would use and not documenting the %(atoms)
> is counter-productive IMHO.
>

I guess, It'll be better to write a document on the whole of
ref-filter. I'll probably do that
at the end of this.

> I would favor the first option (keep it short, include a pointer) with
> Junio's remark in mind: "git tag" and "git branch" are meant to be
> simple commands, and the scary swiss-army-knife should remain "git
> for-each-ref".
>
> I am still (slightly) in favor of adding --format to tag and branch, as
> long as it does not make the commands too scary.
>

Will change this, thanks for the suggestions.


-- 
Regards,
Karthik Nayak

Re: [PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Speaking of quote_value, The quote doesn't work well with color's
>> for e.g.
>> git for-each-ref --shell --format="%(color:green)%(refname)"
>> '''refs/heads/allow-unknown-type'''
>> Seems like an simple fix, probably after GSoC I'll do this :)
>
> Anyway, the %(color) is really meant to be displayed on-screen, and the
> quoting is really meant to feed the value to another program, so I can
> hardly imagine a use-case where you would want both.
>
> But the current behavior seems fine to me: the color escape sequence is
> quoted, which is good. For example, you can
>
> x=$(git for-each-ref --shell --format="nocolor%(color:green)%(refname)" | head -n 1)
> sh -c "echo $x"
>
> it will actually display "nocolor" without color, then a green refname.
> I'm not sure the quoting is really necessary, but it doesn't harm and it
> makes sense since the escape sequence contains a '[' which is a shell
> metacharacter.

The point of --shell/--tcl/... is so that you can have --format
safely write executable scripts in the specified language.  Your
format string might look like this:

        --format="short=%(refname:short) long=%(refname)"

and one entry in the output in "--shell" mode would expand to

        short='master' long='refs/heads/master'

that can be eval'ed as a script safely without having to worry about
expanded atom values having characters that have special meanings in
the target language.  Your "nocolor" example works the same way:

        --format="var=%(color:green)%(refname)" --shell

would scan 'var=', emit it as literal, see %(color:green) atom, show
it quoted, see %(refname), show it quoted, notice that color is not
terminated and pretend as if it saw %(color:reset) and show it
quoted, which would result in something like:

        var='ESC[32m''master''ESC[m'

Note that the example _knows_ that the quoting rule of the target
language, namely, two 'quoted' 'strings' next to each other are
simply concatenated.  When using a hypothetical target language
whose quoting rule is different, e.g. "type two single-quotes inside
a pair of single-quote to represent a literal single-quote", then
you would write something like this to produce a script in that
language:

        --format="
            var1=%(color:green);
            var2=%(refname);
            var=var1+var2;
        "

as your format string (and it will not be used with --shell).  And
the atom-quoting code that knows the language specific rules would
quote %(atom) properly.  Perhaps the language uses `' for its string
quoting, in which case one entry of the output might look like

	var1=`ESC[32m';
        var2=`refs/heads/master';
        var=var1+var2;

which would be in the valid syntax of that hypothetical language.

Maybe you have an atom %(headstar) that expands to an asterisk for
the currently checked out branch, in order to mimick 'git branch -l'.

Using that, you might use --shell --format to invent a shorter output
format that does not show the asterisk but indicates the current
branch only with color, like so:

        --format='
            if test -z %(headstar)
            then
                echo %(refname:short)
            else
                echo %(color:green)%(refname:short)%(color:reset)
            fi
        '

and you would want %(headstar)'s expansion to be '*' or ''.

If we introduce %(if:empty)%(then)%(else)%(end), the above may
become something like this, removing the need for --shell
altogether:

        %(if:empty)%(headstar)%(then
        )%(refname:short)%(else
        )%(color:green)%(refname:short)%(color:reset)%(end)

With the current implementation, it is likely that this needs to be
a single long line; we may want to extend parsing of atoms to allow
a LF+whitespace before the close parenthesis to make the string more
readable like the above example, but that is an unrelated tangent.

But you should still be able to use "--shell" this way, assigning
the whole thing to a variable:
    
        --format='
                line=%(if:empty)%(headstar)%(then
                )%(refname:short)%(else
                )%(color:green)%(refname:short)%(color:reset)%(end)
                echo "$line"
        '

So I think 'quote' should apply only to the top-level atoms in the
nested %(magic)...%(end) world.  Expand %(if:empty)...%(end) and
then apply the quoting rule specific to the target language to make
the result safe to use as the RHS of the target language.  None of
the atoms that appear internally (e.g. %(headstar) that is being
tested for emptyness) must NOT be quoted.

If you have %(align:40)%(atom) and string%(end), the same logic
applies.  %(atom) is not a top level item (it is inside %(align))
so you would expand "%(atom) and string" without quoting, measure
its display width, align to 40-cols and then if --shell or any
quoting is in effect, applyl that, so that the user can do:

	--format='
            right=%(align:40)%(refname)%(end)
            left=%(align:20,right)%(refname:short)%(end)
            echo "$left $right"
	'

one entry of the output from which would expand to

	right='refs/heads/master                       '
        left='              master'
        echo "$left $right"

because the rule is to quote the whole %(align)...%(end) construct
only once.

Re: [PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> You replaced the quote_style argument with ref_formatting_state, and I
>> think you should have kept this argument and added ref_formatting_state.
>> The other option is to add an extra indirection like
>>
>> struct ref_formatting_state {
>> 	int quote_style;
>> 	struct ref_formatting_stack *stack;
>> }
>>
>> (ref_formatting_stack would be what you currently call
>> ref_formatting_state). But that's probably overkill.
>
> I think this is the right way to go.  As you explained in your later
> messages, this is conceptually a global setting that applies to
> anybody working in the callchain and not something individual
> recursion levels would want to muck with.

The fact that this is conceptually a global setting does not change,
but I think the deeper levels should not care or even _know_ that
language-specific quoting rules exist (see other post).

Sorry for the confusion.

Re: [PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> So I think 'quote' should apply only to the top-level atoms in the
> nested %(magic)...%(end) world.

This is true in most cases, but I think there would also be use-cases
where you would want the opposite, like:

--format '
    %(if:whatever)
    echo %(refname)
    %(end)
'

I'm not sure what's best, but if both can make sense, perhaps we should
just keep the simplest to implement, i.e. the current behavior.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> So I think 'quote' should apply only to the top-level atoms in the
>> nested %(magic)...%(end) world.
>
> This is true in most cases, but I think there would also be use-cases
> where you would want the opposite, like:
>
> --format '
>     %(if:whatever)
>     echo %(refname)
>     %(end)
> '
>
> I'm not sure what's best, but if both can make sense, perhaps we should
> just keep the simplest to implement, i.e. the current behavior.

I am reasonably sure what's best, as --{shell,tcl,...} with --format
is my invention ;-)

The whole point of these "language specific quotes" to have
"--format" output to be an executable script is so that the user can
express control in that scripting language.  We must be able process
the examples in the message you are responding to, i.e. allowing
%(atom) and %(magic)...%(end) correctly assigned to a variable of
the target language.  If that implementation happens to also grok
your "%(if:whatever)...%(then)echo %(refname)%(end)" example in a
way you expect, that would be great, but if not, then I do not think
it is worth worrying about it.  On the other hand, a solution that
does not solve the primary use case is worthless, even if it is
simple to implement.

I do not think we deeply mind if we forbid use if %(if)...%(end)
when quoting is in use, if the current implementation too broken
beyond salvaging.  I however think that %(align:40)%(atom)%(end)
would want to be usable even with quoting, and I suspect that an
implementation that groks %(align) correctly would automatically
grok %(if), too.

So...

Re: [PATCH v12 05/13] ref-filter: implement an `align` atom

[Eric Sunshine]

On Tue, Aug 18, 2015 at 2:37 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Implement an `align` atom which left-, middle-, or right-aligns the
> content between %(align:..) and %(end).
>
> It is followed by `:<width>,<position>`, where the `<position>` is
> either left, right or middle and `<width>` is the size of the area
> into which the content will be placed. If the content between
> %(align:) and %(end) is more than the width then no alignment is
> performed. e.g. to align a refname atom to the middle with a total
> width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".
>
> We now have a `handler()` for each atom_value which will be called
> when that atom_value is being parsed, and similarly an `at_end`
> function for each state which is to be called when the `end` atom is
> encountered. Using this implement the `align` atom which aligns the
> given strbuf by calling `strbuf_utf8_align()` from utf8.c
>
> This is done by assigning a 'handler' function to each atom_value and
> a related 'at_end' function for each state. The align_handler()
> defined uses strbuf_utf8_align() from utf8.c to perform the alignment.
>
> Add documentation and tests for the same.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 74532d3..ecbcc5a 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -626,6 +638,36 @@ static inline char *copy_advance(char *dst, const char *src)
>         return dst;
>  }
>
> +static void align_handler(struct ref_formatting_state *state)
> +{
> +       struct align *align = (struct align *)state->cb_data;
> +       struct strbuf s = STRBUF_INIT;
> +
> +       strbuf_utf8_align(&s, align->position, align->width, state->output.buf);
> +       strbuf_reset(&state->output);
> +       strbuf_addbuf(&state->output, &s);
> +       free(align);
> +}

Leaking 'strbuf s' here.

Also, this operation can be expressed more concisely as:

    strbuf_utf8_align(&s, ...);
    strbuf_swap(&state->output, &s);
    strbuf_release(&s);

Seeing this is also making me question my earlier suggestion of making
pop_state() responsible for appending the current state's output to
the previous state's output. The reason is that if align_handler() is
responsible for appending to the previous state's output, then all the
above string handling collapses to the one line:

    strbuf_utf8_align(&state->prev->output, ..., state->output.buf);

which is even simpler, and doesn't involve a temporary strbuf or
swapping of strbuf contents.

On the other hand, it won't always be the case that all handlers can
get by with such simple code, and they might end up creating temporary
strbuf(s) and such anyhow, so I don't feel too strongly about it, and
it can always be changed at a later date (by someone) if that approach
ends up being better.

Re: [PATCH v12 05/13] ref-filter: implement an `align` atom

[Karthik Nayak]

On Fri, Aug 21, 2015 at 1:53 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Aug 18, 2015 at 2:37 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Implement an `align` atom which left-, middle-, or right-aligns the
>> content between %(align:..) and %(end).
>>
>> It is followed by `:<width>,<position>`, where the `<position>` is
>> either left, right or middle and `<width>` is the size of the area
>> into which the content will be placed. If the content between
>> %(align:) and %(end) is more than the width then no alignment is
>> performed. e.g. to align a refname atom to the middle with a total
>> width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".
>>
>> We now have a `handler()` for each atom_value which will be called
>> when that atom_value is being parsed, and similarly an `at_end`
>> function for each state which is to be called when the `end` atom is
>> encountered. Using this implement the `align` atom which aligns the
>> given strbuf by calling `strbuf_utf8_align()` from utf8.c
>>
>> This is done by assigning a 'handler' function to each atom_value and
>> a related 'at_end' function for each state. The align_handler()
>> defined uses strbuf_utf8_align() from utf8.c to perform the alignment.
>>
>> Add documentation and tests for the same.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 74532d3..ecbcc5a 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -626,6 +638,36 @@ static inline char *copy_advance(char *dst, const char *src)
>>         return dst;
>>  }
>>
>> +static void align_handler(struct ref_formatting_state *state)
>> +{
>> +       struct align *align = (struct align *)state->cb_data;
>> +       struct strbuf s = STRBUF_INIT;
>> +
>> +       strbuf_utf8_align(&s, align->position, align->width, state->output.buf);
>> +       strbuf_reset(&state->output);
>> +       strbuf_addbuf(&state->output, &s);
>> +       free(align);
>> +}
>
> Leaking 'strbuf s' here.
>
> Also, this operation can be expressed more concisely as:
>
>     strbuf_utf8_align(&s, ...);
>     strbuf_swap(&state->output, &s);
>     strbuf_release(&s);
>

This seems neater.

> Seeing this is also making me question my earlier suggestion of making
> pop_state() responsible for appending the current state's output to
> the previous state's output. The reason is that if align_handler() is
> responsible for appending to the previous state's output, then all the
> above string handling collapses to the one line:
>
>     strbuf_utf8_align(&state->prev->output, ..., state->output.buf);
>
> which is even simpler, and doesn't involve a temporary strbuf or
> swapping of strbuf contents.
>
> On the other hand, it won't always be the case that all handlers can
> get by with such simple code, and they might end up creating temporary
> strbuf(s) and such anyhow, so I don't feel too strongly about it, and
> it can always be changed at a later date (by someone) if that approach
> ends up being better.

I did try to tell the same on your previous suggestion, maybe I wasn't
clear enough.
But I see pros and cons for both, so with only one modifier atom at the moment,
It's hard to fixate on one method. Maybe we could change it 'if needed' when we
introduce new atoms eventually.

-- 
Regards,
Karthik Nayak