[PATCH v17 00/14] port tag.c to use ref-filter APIs

[PATCH v17 00/14] port tag.c to use ref-filter APIs

[Karthik Nayak]

This is part of the series of unifying the code used by
"git tag -l, git branch -l, git for-each-ref".

The previous version can be found here (version 16):
article.gmane.org/gmane.comp.version-control.git/277394

Changes in this version:
* The arguments of the %(align) atom are interchangeable.
* Small grammatical changes.
* Small changes in the tests to reflect changes in the align
atom code.

Karthik Nayak (14):
  ref-filter: move `struct atom_value` to ref-filter.c
  ref-filter: introduce ref_formatting_state and ref_formatting_stack
  utf8: add function to align a string into given strbuf
  ref-filter: introduce handler function for each atom
  ref-filter: introduce match_atom_name()
  ref-filter: implement an `align` atom
  ref-filter: add option to filter out tags, branches and remotes
  ref-filter: add support for %(contents:lines=X)
  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 |  17 +-
 Documentation/git-tag.txt          |  27 ++-
 builtin/for-each-ref.c             |   1 +
 builtin/tag.c                      | 369 ++++++---------------------------
 ref-filter.c                       | 412 ++++++++++++++++++++++++++++++++-----
 ref-filter.h                       |  25 ++-
 refs.c                             |   9 +
 refs.h                             |   1 +
 t/t6302-for-each-ref-filter.sh     | 174 ++++++++++++++++
 t/t7004-tag.sh                     |  47 ++++-
 utf8.c                             |  21 ++
 utf8.h                             |  15 ++
 12 files changed, 742 insertions(+), 376 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index c5154bb..16b4ac5 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -128,14 +128,15 @@ color::
 	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. If used with '--quote' everything in between
-	%(align:...) and %(end) is quoted, but if nested then only the
-	topmost level performs quoting.
+	Left-, middle-, or right-align the content between
+	%(align:...) and %(end). The "align:" is followed by `<width>`
+	and `<position>` in any order separated by a comma, where the
+	`<position>` is either left, right or middle, default being
+	left 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. If used with '--quote' everything
+	in between %(align:...) and %(end) is quoted, but if nested
+	then only the topmost level performs quoting.
 
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
diff --git a/builtin/tag.c b/builtin/tag.c
index f55dfda..081fe84 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -42,10 +42,11 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
 		filter->lines = 0;
 
 	if (!format) {
-		if (filter->lines)
-			format = to_free = xstrfmt("%%(align:15,left)%%(refname:short)%%(end) "
-						   "%%(contents:lines=%d)", filter->lines);
-		else
+		if (filter->lines) {
+			format = xstrfmt("%s %%(contents:lines=%d)",
+					 "%(align:15)%%(refname:short)%%(end)", filter->lines);
+			to_free = format;
+		} else
 			format = "%(refname:short)";
 	}
 
diff --git a/ref-filter.c b/ref-filter.c
index e3024d3..59716db 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -274,7 +274,7 @@ static int match_atom_name(const char *name, const char *atom_name, const char *
 	}
 	if (body[0] != ':')
 		return 0; /* "atom_namefoo" is not "atom_name" or "atom_name:..." */
-	*val = body + 1; /* "atomname:val" */
+	*val = body + 1; /* "atom_name:val" */
 	return 1;
 }
 
@@ -884,43 +884,40 @@ static void populate_value(struct ref_array_item *ref)
 			continue;
 		} else if (match_atom_name(name, "align", &valp)) {
 			struct align *align = &v->u.align;
-			struct strbuf **s;
+			struct strbuf **s, **to_free;
+			int width = -1;
 
 			if (!valp)
-				die(_("expected format: %%(align:<width>, <position>)"));
+				die(_("expected format: %%(align:<width>,<position>)"));
 
 			/*
 			 * TODO: Implement a function similar to strbuf_split_str()
-			 * which would strip the terminator at the end.
+			 * which would omit the separator from the end of each value.
 			 */
-			s = strbuf_split_str(valp, ',', 0);
-
-			/* If the position is given trim the ',' from the first strbuf */
-			if (s[1])
-				strbuf_setlen(s[0], s[0]->len - 1);
-			if (s[2])
-				die(_("align:<width>,<position> followed by garbage: %s"), s[2]->buf);
-
-			if (strtoul_ui(s[0]->buf, 10, &align->width))
-				die(_("positive width expected align:%s"), s[0]->buf);
-
-			/*
-			 * TODO: Implement a more general check, so that the values
-			 * do not always have to be in a specific order.
-			 */
-			if (!s[1])
-				align->position = ALIGN_LEFT;
-			else if (!strcmp(s[1]->buf, "left"))
-				align->position = ALIGN_LEFT;
-			else if (!strcmp(s[1]->buf, "right"))
-				align->position = ALIGN_RIGHT;
-			else if (!strcmp(s[1]->buf, "middle"))
-				align->position = ALIGN_MIDDLE;
-			else
-				die(_("improper format entered align:%s"), s[1]->buf);
-
-			strbuf_list_free(s);
+			s = to_free = strbuf_split_str(valp, ',', 0);
+
+			align->position = ALIGN_LEFT;
+
+			while (*s) {
+				if (s[1])
+					strbuf_setlen(s[0], s[0]->len - 1);
+				if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
+					;
+				else if (!strcmp(s[0]->buf, "left"))
+					align->position = ALIGN_LEFT;
+				else if (!strcmp(s[0]->buf, "right"))
+					align->position = ALIGN_RIGHT;
+				else if (!strcmp(s[0]->buf, "middle"))
+					align->position = ALIGN_MIDDLE;
+				else
+					die(_("improper format entered align:%s"), s[0]->buf);
+				s++;
+			}
 
+			if (width < 0)
+				die(_("positive width expected with the %%(align) atom"));
+			align->width = width;
+			strbuf_list_free(to_free);
 			v->handler = align_atom_handler;
 			continue;
 		} else if (!strcmp(name, "end")) {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 4bc1055..fe4796c 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -85,7 +85,7 @@ test_expect_success '%(color) must fail' '
 	test_must_fail git for-each-ref --format="%(color)%(refname)"
 '
 
-test_expect_success 'left alignment' '
+test_expect_success 'left alignment is default' '
 	cat >expect <<-\EOF &&
 	refname is refs/heads/master  |refs/heads/master
 	refname is refs/heads/side    |refs/heads/side
@@ -97,7 +97,7 @@ test_expect_success 'left alignment' '
 	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 &&
+	git for-each-ref --format="%(align:30)refname is %(refname)%(end)|%(refname)" >actual &&
 	test_cmp expect actual
 '
 
@@ -113,7 +113,7 @@ test_expect_success 'middle alignment' '
 	|  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 &&
+	git for-each-ref --format="|%(align:middle,30)refname is %(refname)%(end)|%(refname)" >actual &&
 	test_cmp expect actual
 '
 
@@ -137,17 +137,17 @@ test_expect_success 'right alignment' '
 
 test_expect_success 'alignment with format quote' "
 	cat >expect <<-\EOF &&
-	|'       master| A U Thor       '|
-	|'        side| A U Thor        '|
-	|'      odd/spot| A U Thor      '|
-	|'         double-tag|          '|
-	|'        four| A U Thor        '|
-	|'        one| A U Thor         '|
-	|'         signed-tag|          '|
-	|'       three| A U Thor        '|
-	|'        two| A U Thor         '|
+	|'      '\''master| A U Thor'\''      '|
+	|'       '\''side| A U Thor'\''       '|
+	|'     '\''odd/spot| A U Thor'\''     '|
+	|'        '\''double-tag| '\''        '|
+	|'       '\''four| A U Thor'\''       '|
+	|'       '\''one| A U Thor'\''        '|
+	|'        '\''signed-tag| '\''        '|
+	|'      '\''three| A U Thor'\''       '|
+	|'       '\''two| A U Thor'\''        '|
 	EOF
-	git for-each-ref --shell --format='|%(align:30,middle)%(refname:short)| %(authorname)%(end)|' >actual &&
+	git for-each-ref --shell --format=\"|%(align:30,middle)'%(refname:short)| %(authorname)'%(end)|\" >actual &&
 	test_cmp expect actual
 "
 
-- 
2.5.1

[PATCH v17 01/14] 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.1

[PATCH v17 02/14] ref-filter: introduce ref_formatting_state and ref_formatting_stack

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

Implement a stack machinery for ref_formatting_state, this allows us
to push and pop elements onto the stack. Whenever we pop an element
from the stack, the strbuf from that element is appended to the strbuf
of the next element on the stack, this will allow us to support
nesting of modifier atoms.

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 | 78 +++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 59 insertions(+), 19 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index e53c77e..432cea0 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -55,6 +55,18 @@ static struct {
 	{ "color" },
 };
 
+#define REF_FORMATTING_STATE_INIT  { 0, NULL }
+
+struct ref_formatting_stack {
+	struct ref_formatting_stack *prev;
+	struct strbuf output;
+};
+
+struct ref_formatting_state {
+	int quote_style;
+	struct ref_formatting_stack *stack;
+};
+
 struct atom_value {
 	const char *s;
 	unsigned long ul; /* used for sorting when not FIELD_STR */
@@ -129,6 +141,27 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 	return at;
 }
 
+static void push_stack_element(struct ref_formatting_stack **stack)
+{
+	struct ref_formatting_stack *s = xcalloc(1, sizeof(struct ref_formatting_stack));
+
+	strbuf_init(&s->output, 0);
+	s->prev = *stack;
+	*stack = s;
+}
+
+static void pop_stack_element(struct ref_formatting_stack **stack)
+{
+	struct ref_formatting_stack *current = *stack;
+	struct ref_formatting_stack *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).
  */
@@ -1195,30 +1228,27 @@ 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) {
+	struct strbuf *s = &state->stack->output;
+
+	switch (state->quote_style) {
 	case QUOTE_NONE:
-		fputs(v->s, stdout);
+		strbuf_addstr(s, v->s);
 		break;
 	case QUOTE_SHELL:
-		sq_quote_buf(&sb, v->s);
+		sq_quote_buf(s, v->s);
 		break;
 	case QUOTE_PERL:
-		perl_quote_buf(&sb, v->s);
+		perl_quote_buf(s, v->s);
 		break;
 	case QUOTE_PYTHON:
-		python_quote_buf(&sb, v->s);
+		python_quote_buf(s, v->s);
 		break;
 	case QUOTE_TCL:
-		tcl_quote_buf(&sb, v->s);
+		tcl_quote_buf(s, v->s);
 		break;
 	}
-	if (quote_style != QUOTE_NONE) {
-		fputs(sb.buf, stdout);
-		strbuf_release(&sb);
-	}
 }
 
 static int hex1(char ch)
@@ -1239,8 +1269,10 @@ 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)
 {
+	struct strbuf *s = &state->stack->output;
+
 	while (*cp && (!ep || cp < ep)) {
 		if (*cp == '%') {
 			if (cp[1] == '%')
@@ -1248,13 +1280,13 @@ static void emit(const char *cp, const char *ep)
 			else {
 				int ch = hex2(cp + 1);
 				if (0 <= ch) {
-					putchar(ch);
+					strbuf_addch(s, ch);
 					cp += 3;
 					continue;
 				}
 			}
 		}
-		putchar(*cp);
+		strbuf_addch(s, *cp);
 		cp++;
 	}
 }
@@ -1262,19 +1294,24 @@ 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 strbuf *final_buf;
+	struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
+
+	state.quote_style = quote_style;
+	push_stack_element(&state.stack);
 
 	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,8 +1320,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);
 	}
+	final_buf = &state.stack->output;
+	fwrite(final_buf->buf, 1, final_buf->len, stdout);
+	pop_stack_element(&state.stack);
 	putchar('\n');
 }
 
-- 
2.5.1

[PATCH v17 03/14] 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.1

[PATCH v17 04/14] ref-filter: introduce handler function for each atom

[Karthik Nayak]

Introduce a handler function for each atom, which is called when the
atom is processed in show_ref_array_item().

In this context make append_atom() as the default handler function and
extract quote_formatting() out of append_atom(). Bump this to the top.

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 | 54 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 432cea0..a993216 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -69,6 +69,7 @@ struct ref_formatting_state {
 
 struct atom_value {
 	const char *s;
+	void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
 	unsigned long ul; /* used for sorting when not FIELD_STR */
 };
 
@@ -141,6 +142,32 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 	return at;
 }
 
+static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
+{
+	switch (quote_style) {
+	case QUOTE_NONE:
+		strbuf_addstr(s, str);
+		break;
+	case QUOTE_SHELL:
+		sq_quote_buf(s, str);
+		break;
+	case QUOTE_PERL:
+		perl_quote_buf(s, str);
+		break;
+	case QUOTE_PYTHON:
+		python_quote_buf(s, str);
+		break;
+	case QUOTE_TCL:
+		tcl_quote_buf(s, str);
+		break;
+	}
+}
+
+static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
+{
+	quote_formatting(&state->stack->output, v->s, state->quote_style);
+}
+
 static void push_stack_element(struct ref_formatting_stack **stack)
 {
 	struct ref_formatting_stack *s = xcalloc(1, sizeof(struct ref_formatting_stack));
@@ -662,6 +689,8 @@ static void populate_value(struct ref_array_item *ref)
 		const char *formatp;
 		struct branch *branch = NULL;
 
+		v->handler = append_atom;
+
 		if (*name == '*') {
 			deref = 1;
 			name++;
@@ -1228,29 +1257,6 @@ 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 append_atom(struct atom_value *v, struct ref_formatting_state *state)
-{
-	struct strbuf *s = &state->stack->output;
-
-	switch (state->quote_style) {
-	case QUOTE_NONE:
-		strbuf_addstr(s, v->s);
-		break;
-	case QUOTE_SHELL:
-		sq_quote_buf(s, v->s);
-		break;
-	case QUOTE_PERL:
-		perl_quote_buf(s, v->s);
-		break;
-	case QUOTE_PYTHON:
-		python_quote_buf(s, v->s);
-		break;
-	case QUOTE_TCL:
-		tcl_quote_buf(s, v->s);
-		break;
-	}
-}
-
 static int hex1(char ch)
 {
 	if ('0' <= ch && ch <= '9')
@@ -1307,7 +1313,7 @@ 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);
-		append_atom(atomv, &state);
+		atomv->handler(atomv, &state);
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);
-- 
2.5.1

[PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Karthik Nayak]

Introduce match_atom_name() which helps in checking if a particular
atom is the atom we're looking for and if it has a value attached to
it or not.

Use it instead of starts_with() for checking the value of %(color:...)
atom. Write a test for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Thanks-to: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c                   | 23 +++++++++++++++++++++--
 t/t6302-for-each-ref-filter.sh |  4 ++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index a993216..70d36fe 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -189,6 +189,22 @@ static void pop_stack_element(struct ref_formatting_stack **stack)
 	*stack = prev;
 }
 
+static int match_atom_name(const char *name, const char *atom_name, const char **val)
+{
+	const char *body;
+
+	if (!skip_prefix(name, atom_name, &body))
+		return 0; /* doesn't even begin with "atom_name" */
+	if (!body[0] || !body[1]) {
+		*val = NULL; /* %(atom_name) and no customization */
+		return 1;
+	}
+	if (body[0] != ':')
+		return 0; /* "atom_namefoo" is not "atom_name" or "atom_name:..." */
+	*val = body + 1; /* "atom_name:val" */
+	return 1;
+}
+
 /*
  * In a format string, find the next occurrence of %(atom).
  */
@@ -687,6 +703,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;
 
 		v->handler = append_atom;
@@ -721,10 +738,12 @@ static void populate_value(struct ref_array_item *ref)
 			refname = branch_get_push(branch, NULL);
 			if (!refname)
 				continue;
-		} else if (starts_with(name, "color:")) {
+		} else if (match_atom_name(name, "color", &valp)) {
 			char color[COLOR_MAXLEN] = "";
 
-			if (color_parse(name + 6, color) < 0)
+			if (!valp)
+				die(_("expected format: %%(color:<color>)"));
+			if (color_parse(valp, color) < 0)
 				die(_("unable to parse format"));
 			v->s = xstrdup(color);
 			continue;
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 505a360..c4f0378 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -81,4 +81,8 @@ test_expect_success 'filtering with --contains' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(color) must fail' '
+	test_must_fail git for-each-ref --format="%(color)%(refname)"
+'
+
 test_done
-- 
2.5.1

[PATCH v17 06/14] ref-filter: implement an `align` atom

[Karthik Nayak]

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

The "align:" is followed by `<width>` and `<position>` in any order
separated by a comma, where the `<position>` is either left, right or
middle, default being left 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.  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 introduce an `at_end` function for each element of the stack which
is to be called when the `end` atom is encountered. Using this we
implement end_align_handler() for the `align` atom, this aligns the
final strbuf by calling `strbuf_utf8_align()` from utf8.c.

Ensure that quote formatting is performed on the whole of
%(align:...)...%(end) rather than individual atoms inside. We skip
quote formatting for individual atoms when the current stack element
is handling an %(align:...) atom and perform quote formatting at the
end when we encounter the %(end) atom of the second element of then
stack.

Add documentation and tests for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt |  11 ++++
 ref-filter.c                       | 109 ++++++++++++++++++++++++++++++++++++-
 t/t6302-for-each-ref-filter.sh     |  82 ++++++++++++++++++++++++++++
 3 files changed, 201 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index e49d578..3a271bf 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -127,6 +127,17 @@ 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). The "align:" is followed by `<width>`
+	and `<position>` in any order separated by a comma, where the
+	`<position>` is either left, right or middle, default being
+	left 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. If used with '--quote' everything
+	in between %(align:...) and %(end) is quoted, but if nested
+	then only the topmost level performs quoting.
+
 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 70d36fe..b8f719c 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,13 +54,22 @@ static struct {
 	{ "flag" },
 	{ "HEAD" },
 	{ "color" },
+	{ "align" },
+	{ "end" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
 
+struct align {
+	align_type position;
+	unsigned int width;
+};
+
 struct ref_formatting_stack {
 	struct ref_formatting_stack *prev;
 	struct strbuf output;
+	void (*at_end)(struct ref_formatting_stack *stack);
+	void *at_end_data;
 };
 
 struct ref_formatting_state {
@@ -69,6 +79,9 @@ struct ref_formatting_state {
 
 struct atom_value {
 	const char *s;
+	union {
+		struct align align;
+	} u;
 	void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
 	unsigned long ul; /* used for sorting when not FIELD_STR */
 };
@@ -165,7 +178,16 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
 
 static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
 {
-	quote_formatting(&state->stack->output, v->s, state->quote_style);
+	/*
+	 * Quote formatting is only done when the stack has a single
+	 * element. Otherwise quote formatting is done on the
+	 * element's entire output strbuf when the %(end) atom is
+	 * encountered.
+	 */
+	if (!state->stack->prev)
+		quote_formatting(&state->stack->output, v->s, state->quote_style);
+	else
+		strbuf_addstr(&state->stack->output, v->s);
 }
 
 static void push_stack_element(struct ref_formatting_stack **stack)
@@ -189,6 +211,48 @@ static void pop_stack_element(struct ref_formatting_stack **stack)
 	*stack = prev;
 }
 
+static void end_align_handler(struct ref_formatting_stack *stack)
+{
+	struct align *align = (struct align *)stack->at_end_data;
+	struct strbuf s = STRBUF_INIT;
+
+	strbuf_utf8_align(&s, align->position, align->width, stack->output.buf);
+	strbuf_swap(&stack->output, &s);
+	strbuf_release(&s);
+}
+
+static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+	struct ref_formatting_stack *new;
+
+	push_stack_element(&state->stack);
+	new = state->stack;
+	new->at_end = end_align_handler;
+	new->at_end_data = &atomv->u.align;
+}
+
+static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+	struct ref_formatting_stack *current = state->stack;
+	struct strbuf s = STRBUF_INIT;
+
+	if (!current->at_end)
+		die(_("format: %%(end) atom used without corresponding atom"));
+	current->at_end(current);
+
+	/*
+	 * Perform quote formatting when the stack element is that of
+	 * a supporting atom. If nested then perform quote formatting
+	 * only on the topmost supporting atom.
+	 */
+	if (!state->stack->prev->prev) {
+		quote_formatting(&s, current->output.buf, state->quote_style);
+		strbuf_swap(&current->output, &s);
+	}
+	strbuf_release(&s);
+	pop_stack_element(&state->stack);
+}
+
 static int match_atom_name(const char *name, const char *atom_name, const char **val)
 {
 	const char *body;
@@ -773,6 +837,47 @@ static void populate_value(struct ref_array_item *ref)
 			else
 				v->s = " ";
 			continue;
+		} else if (match_atom_name(name, "align", &valp)) {
+			struct align *align = &v->u.align;
+			struct strbuf **s, **to_free;
+			int width = -1;
+
+			if (!valp)
+				die(_("expected format: %%(align:<width>,<position>)"));
+
+			/*
+			 * TODO: Implement a function similar to strbuf_split_str()
+			 * which would omit the separator from the end of each value.
+			 */
+			s = to_free = strbuf_split_str(valp, ',', 0);
+
+			align->position = ALIGN_LEFT;
+
+			while (*s) {
+				if (s[1])
+					strbuf_setlen(s[0], s[0]->len - 1);
+				if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
+					;
+				else if (!strcmp(s[0]->buf, "left"))
+					align->position = ALIGN_LEFT;
+				else if (!strcmp(s[0]->buf, "right"))
+					align->position = ALIGN_RIGHT;
+				else if (!strcmp(s[0]->buf, "middle"))
+					align->position = ALIGN_MIDDLE;
+				else
+					die(_("improper format entered align:%s"), s[0]->buf);
+				s++;
+			}
+
+			if (width < 0)
+				die(_("positive width expected with the %%(align) atom"));
+			align->width = width;
+			strbuf_list_free(to_free);
+			v->handler = align_atom_handler;
+			continue;
+		} else if (!strcmp(name, "end")) {
+			v->handler = end_atom_handler;
+			continue;
 		} else
 			continue;
 
@@ -1347,6 +1452,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 		resetv.s = color;
 		append_atom(&resetv, &state);
 	}
+	if (state.stack->prev)
+		die(_("format: %%(end) atom missing"));
 	final_buf = &state.stack->output;
 	fwrite(final_buf->buf, 1, final_buf->len, stdout);
 	pop_stack_element(&state.stack);
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index c4f0378..f596035 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -85,4 +85,86 @@ test_expect_success '%(color) must fail' '
 	test_must_fail git for-each-ref --format="%(color)%(refname)"
 '
 
+test_expect_success 'left alignment is default' '
+	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)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:middle,30)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
+'
+
+# Individual atoms inside %(align:...) and %(end) must not be quoted.
+
+test_expect_success 'alignment with format quote' "
+	cat >expect <<-\EOF &&
+	|'      '\''master| A U Thor'\''      '|
+	|'       '\''side| A U Thor'\''       '|
+	|'     '\''odd/spot| A U Thor'\''     '|
+	|'        '\''double-tag| '\''        '|
+	|'       '\''four| A U Thor'\''       '|
+	|'       '\''one| A U Thor'\''        '|
+	|'        '\''signed-tag| '\''        '|
+	|'      '\''three| A U Thor'\''       '|
+	|'       '\''two| A U Thor'\''        '|
+	EOF
+	git for-each-ref --shell --format=\"|%(align:30,middle)'%(refname:short)| %(authorname)'%(end)|\" >actual &&
+	test_cmp expect actual
+"
+
+test_expect_success 'nested alignment with quote formatting' "
+	cat >expect <<-\EOF &&
+	|'         master               '|
+	|'           side               '|
+	|'       odd/spot               '|
+	|'     double-tag               '|
+	|'           four               '|
+	|'            one               '|
+	|'     signed-tag               '|
+	|'          three               '|
+	|'            two               '|
+	EOF
+	git for-each-ref --shell --format='|%(align:30,left)%(align:15,right)%(refname:short)%(end)%(end)|' >actual &&
+	test_cmp expect actual
+"
+
 test_done
-- 
2.5.1

[PATCH v17 07/14] 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_fullref_in()' 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 | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 ref-filter.h | 13 ++++++++++--
 refs.c       |  9 +++++++++
 refs.h       |  1 +
 4 files changed, 81 insertions(+), 7 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index b8f719c..7d2732a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1189,6 +1189,34 @@ 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 ||
+	    filter->kind == FILTER_REFS_REMOTES ||
+	    filter->kind == FILTER_REFS_TAGS)
+		return filter->kind;
+	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.
@@ -1199,6 +1227,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);
@@ -1210,6 +1239,11 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 		return 0;
 	}
 
+	/* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */
+	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;
 
@@ -1241,6 +1275,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;
 }
 
@@ -1317,17 +1352,37 @@ 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;
 
+	if (type & FILTER_REFS_INCLUDE_BROKEN)
+		broken = 1;
+	filter->kind = type & FILTER_REFS_KIND_MASK;
+
 	/*  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 (!filter->kind)
 		die("filter_refs: invalid type");
+	else {
+		/*
+		 * For common cases where we need only branches or remotes or tags,
+		 * we only iterate through those refs. If a mix of refs is needed,
+		 * we iterate over all refs and filter out required refs with the help
+		 * of filter_ref_kind().
+		 */
+		if (filter->kind == FILTER_REFS_BRANCHES)
+			ret = for_each_fullref_in("refs/heads/", ref_filter_handler, &ref_cbdata, broken);
+		else if (filter->kind == FILTER_REFS_REMOTES)
+			ret = for_each_fullref_in("refs/remotes/", ref_filter_handler, &ref_cbdata, broken);
+		else if (filter->kind == FILTER_REFS_TAGS)
+			ret = for_each_fullref_in("refs/tags/", ref_filter_handler, &ref_cbdata, broken);
+		else if (filter->kind & FILTER_REFS_ALL)
+			ret = for_each_fullref_in("", ref_filter_handler, &ref_cbdata, broken);
+		if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
+			head_ref(ref_filter_handler, &ref_cbdata);
+	}
+
 
 	/*  Filters that need revision walking */
 	if (filter->merge_commit)
diff --git a/ref-filter.h b/ref-filter.h
index 45026d0..0913ba9 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -13,8 +13,15 @@
 #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
+#define FILTER_REFS_KIND_MASK      (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)
 
 struct atom_value;
 
@@ -27,6 +34,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 +59,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 4e15f60..a9469c2 100644
--- a/refs.c
+++ b/refs.c
@@ -2108,6 +2108,15 @@ int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)
 	return do_for_each_ref(&ref_cache, prefix, fn, strlen(prefix), 0, cb_data);
 }
 
+int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken)
+{
+	unsigned int flag = 0;
+
+	if (broken)
+		flag = DO_FOR_EACH_INCLUDE_BROKEN;
+	return do_for_each_ref(&ref_cache, prefix, fn, 0, flag, cb_data);
+}
+
 int for_each_ref_in_submodule(const char *submodule, const char *prefix,
 		each_ref_fn fn, void *cb_data)
 {
diff --git a/refs.h b/refs.h
index e9a5f32..6d30c98 100644
--- a/refs.h
+++ b/refs.h
@@ -173,6 +173,7 @@ typedef int each_ref_fn(const char *refname,
 extern int head_ref(each_ref_fn fn, void *cb_data);
 extern int for_each_ref(each_ref_fn fn, void *cb_data);
 extern int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);
+extern int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken);
 extern int for_each_tag_ref(each_ref_fn fn, void *cb_data);
 extern int for_each_branch_ref(each_ref_fn fn, void *cb_data);
 extern int for_each_remote_ref(each_ref_fn fn, void *cb_data);
-- 
2.5.1

[PATCH v17 08/14] ref-filter: add support for %(contents:lines=X)

[Karthik Nayak]

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 it to support appending of N lines from the annotation of tags
to the given strbuf.

Implement %(contents:lines=X) where X lines of the given object are
obtained.

Add documentation and test 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 ++-
 builtin/tag.c                      |  4 +++
 ref-filter.c                       | 45 ++++++++++++++++++++++++++++++++-
 ref-filter.h                       |  3 ++-
 t/t6302-for-each-ref-filter.sh     | 52 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 104 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 3a271bf..324ad2c 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -150,7 +150,8 @@ The complete message in a commit and tag object is `contents`.
 Its first line is `contents:subject`, where subject is the concatenation
 of all lines of the commit message up to the first blank line.  The next
 line is 'contents:body', where body is all of the lines after the first
-blank line.  Finally, the optional GPG signature is `contents:signature`.
+blank line.  The optional GPG signature is `contents:signature`.  The
+first `N` lines of the message is obtained using `contents:lines=N`.
 
 For sorting purposes, fields with numeric values sort in numeric
 order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).
diff --git a/builtin/tag.c b/builtin/tag.c
index 471d6b1..b0bc1c5 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 modified and used in ref-filter as append_lines(), 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 7d2732a..b098b16 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -56,6 +56,7 @@ static struct {
 	{ "color" },
 	{ "align" },
 	{ "end" },
+	{ "contents:lines" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -65,6 +66,11 @@ struct align {
 	unsigned int width;
 };
 
+struct contents {
+	unsigned int lines;
+	struct object_id oid;
+};
+
 struct ref_formatting_stack {
 	struct ref_formatting_stack *prev;
 	struct strbuf output;
@@ -81,6 +87,7 @@ struct atom_value {
 	const char *s;
 	union {
 		struct align align;
+		struct contents contents;
 	} u;
 	void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
 	unsigned long ul; /* used for sorting when not FIELD_STR */
@@ -643,6 +650,30 @@ static void find_subpos(const char *buf, unsigned long sz,
 	*nonsiglen = *sig - buf;
 }
 
+/*
+ * If 'lines' is greater than 0, append that many lines from the given
+ * 'buf' of length 'size' to the given strbuf.
+ */
+static void append_lines(struct strbuf *out, const char *buf, unsigned long size, int lines)
+{
+	int i;
+	const char *sp, *eol;
+	size_t len;
+
+	sp = buf;
+
+	for (i = 0; i < lines && sp < buf + size; i++) {
+		if (i)
+			strbuf_addstr(out, "\n    ");
+		eol = memchr(sp, '\n', size - (sp - buf));
+		len = eol ? eol - sp : size - (sp - buf);
+		strbuf_add(out, sp, len);
+		if (!eol)
+			break;
+		sp = eol + 1;
+	}
+}
+
 /* See grab_values */
 static void grab_sub_body_contents(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
 {
@@ -653,6 +684,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i];
 		struct atom_value *v = &val[i];
+		const char *valp = NULL;
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
@@ -662,7 +694,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 		    strcmp(name, "contents") &&
 		    strcmp(name, "contents:subject") &&
 		    strcmp(name, "contents:body") &&
-		    strcmp(name, "contents:signature"))
+		    strcmp(name, "contents:signature") &&
+		    !starts_with(name, "contents:lines="))
 			continue;
 		if (!subpos)
 			find_subpos(buf, sz,
@@ -682,6 +715,16 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 			v->s = xmemdupz(sigpos, siglen);
 		else if (!strcmp(name, "contents"))
 			v->s = xstrdup(subpos);
+		else if (skip_prefix(name, "contents:lines=", &valp)) {
+			struct strbuf s = STRBUF_INIT;
+			const char *contents_end = bodylen + bodypos - siglen;
+
+			if (strtoul_ui(valp, 10, &v->u.contents.lines))
+				die(_("positive value expected contents:lines=%s"), valp);
+			/*  Size is the length of the message after removing the signature */
+			append_lines(&s, subpos, contents_end - subpos, v->u.contents.lines);
+			v->s = strbuf_detach(&s, NULL);
+		}
 	}
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 0913ba9..ab76b22 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -59,7 +59,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 {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index f596035..bab1f28 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -167,4 +167,56 @@ test_expect_success 'nested alignment with quote formatting' "
 	test_cmp expect actual
 "
 
+test_expect_success 'check `%(contents:lines=1)`' '
+	cat >expect <<-\EOF &&
+	master |three
+	side |four
+	odd/spot |three
+	double-tag |Annonated doubly
+	four |four
+	one |one
+	signed-tag |A signed tag message
+	three |three
+	two |two
+	EOF
+	git for-each-ref --format="%(refname:short) |%(contents:lines=1)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check `%(contents:lines=0)`' '
+	cat >expect <<-\EOF &&
+	master |
+	side |
+	odd/spot |
+	double-tag |
+	four |
+	one |
+	signed-tag |
+	three |
+	two |
+	EOF
+	git for-each-ref --format="%(refname:short) |%(contents:lines=0)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check `%(contents:lines=99999)`' '
+	cat >expect <<-\EOF &&
+	master |three
+	side |four
+	odd/spot |three
+	double-tag |Annonated doubly
+	four |four
+	one |one
+	signed-tag |A signed tag message
+	three |three
+	two |two
+	EOF
+	git for-each-ref --format="%(refname:short) |%(contents:lines=99999)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '`%(contents:lines=-1)` should fail' '
+	test_must_fail git for-each-ref --format="%(refname:short) |%(contents:lines=-1)"
+'
+
 test_done
-- 
2.5.1

[PATCH v17 09/14] 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 324ad2c..16b4ac5 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -157,6 +157,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 b098b16..77035d1 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;
 
@@ -1442,19 +1444,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;
 }
 
@@ -1587,6 +1589,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 ab76b22..ef25b6e 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -28,7 +28,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 bab1f28..fe4796c 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -219,4 +219,40 @@ test_expect_success '`%(contents:lines=-1)` should fail' '
 	test_must_fail git for-each-ref --format="%(refname:short) |%(contents:lines=-1)"
 '
 
+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.1

[PATCH v17 10/14] 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 40f343b..4e9f6c2 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 77035d1..59716db 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1164,9 +1164,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)
 {
@@ -1187,6 +1211,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
@@ -1289,7 +1323,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 ef25b6e..a5cfa5e 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -59,7 +59,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.1

[PATCH v17 11/14] 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 b0bc1c5..fe66f7b 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.1

[PATCH v17 12/14] 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             | 345 ++++++----------------------------------------
 t/t7004-tag.sh            |   8 +-
 3 files changed, 53 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 fe66f7b..f257d45 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -28,278 +28,35 @@ 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 modified and used in ref-filter as append_lines(), 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, *to_free = NULL;
+	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 = xstrfmt("%s %%(contents:lines=%d)",
+				 "%(align:15)%%(refname:short)%%(end)", filter->lines);
+		to_free = format;
+	} 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, 0);
+	ref_array_clear(&array);
+	free(to_free);
+
 	return 0;
 }
 
@@ -366,35 +123,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 +150,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 +313,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 +329,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 +356,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 +365,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 +391,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 +401,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.1

[PATCH v17 13/14] tag.c: implement '--format' option

[Karthik Nayak]

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 |  8 +++++++-
 builtin/tag.c             | 24 ++++++++++++++----------
 t/t7004-tag.sh            | 12 ++++++++++++
 3 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 3ac4a96..0c7f4e6 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,11 @@ 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.  The format is the same as
+	that of linkgit:git-for-each-ref[1].  When unspecified,
+	defaults to `%(refname:short)`.
 
 CONFIGURATION
 -------------
diff --git a/builtin/tag.c b/builtin/tag.c
index f257d45..075d90b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -23,17 +23,17 @@ 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[--format=<format>] [<pattern>...]"),
 	N_("git tag -v <tagname>..."),
 	NULL
 };
 
 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, *to_free = NULL;
+	char *to_free = NULL;
 	int i;
 
 	memset(&array, 0, sizeof(array));
@@ -41,12 +41,14 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
 	if (filter->lines == -1)
 		filter->lines = 0;
 
-	if (filter->lines) {
-		format = xstrfmt("%s %%(contents:lines=%d)",
-				 "%(align:15)%%(refname:short)%%(end)", filter->lines);
-		to_free = format;
-	} else
-		format = "%(refname:short)";
+	if (!format) {
+		if (filter->lines) {
+			format = xstrfmt("%s %%(contents:lines=%d)",
+					 "%(align:15)%%(refname:short)%%(end)", filter->lines);
+			to_free = format;
+		} else
+			format = "%(refname:short)";
+	}
 
 	verify_ref_format(format);
 	filter_refs(&array, filter, FILTER_REFS_TAGS);
@@ -330,6 +332,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"),
@@ -362,6 +365,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()
 	};
 
@@ -402,7 +406,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			run_column_filter(colopts, &copts);
 		}
 		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..8987fb1 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1519,4 +1519,16 @@ EOF"
 	test_cmp expect actual
 '
 
+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.1

[PATCH v17 14/14] tag.c: implement '--merged' and '--no-merged' options

[Karthik Nayak]

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 0c7f4e6..3803bf7 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
@@ -165,6 +165,11 @@ This option is only applicable when listing tags without annotation lines.
 	that of linkgit:git-for-each-ref[1].  When unspecified,
 	defaults to `%(refname:short)`.
 
+--[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
 -------------
 By default, 'git tag' in sign-with-default mode (-s) will use your
diff --git a/builtin/tag.c b/builtin/tag.c
index 075d90b..081fe84 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[--format=<format>] [<pattern>...]"),
+		"\n\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"),
 	N_("git tag -v <tagname>..."),
 	NULL
 };
@@ -359,6 +359,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),
 		{
@@ -417,6 +419,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 8987fb1..3dd2f51 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1531,4 +1531,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.1

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Matthieu Moy]

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

> Introduce match_atom_name() which helps in checking if a particular
> atom is the atom we're looking for and if it has a value attached to
> it or not.
>
> Use it instead of starts_with() for checking the value of %(color:...)
> atom. Write a test for the same.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Thanks-to: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  ref-filter.c                   | 23 +++++++++++++++++++++--
>  t/t6302-for-each-ref-filter.sh |  4 ++++
>  2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index a993216..70d36fe 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -189,6 +189,22 @@ static void pop_stack_element(struct ref_formatting_stack **stack)
>  	*stack = prev;
>  }
>  
> +static int match_atom_name(const char *name, const char *atom_name, const char **val)
> +{
> +	const char *body;
> +
> +	if (!skip_prefix(name, atom_name, &body))
> +		return 0; /* doesn't even begin with "atom_name" */
> +	if (!body[0] || !body[1]) {
> +		*val = NULL; /* %(atom_name) and no customization */

The logic is still hard to follow. If I read correctly, this function
accepts "%(colorX)" the same ways as "%(color)" here. I first thought it
was a bug, but it doesn't seem to be since %(colorX) would have been
rejected earlier.

It would be a bug if colorX was actually a valid atom name though: you
would be returning 1 for match_atom_name(name, "color") when
name=="colorX". So, I would say this "we can accept one extra character
because some earlier code rejected it before" is too hard to follow for
reviwers and too fragile.

OTOH, you are now accepting %(atom:) as a synonym to %(atom), and it's
not clear whether this is a deliberate decition.

> +		return 1;
> +	}
> +	if (body[0] != ':')
> +		return 0; /* "atom_namefoo" is not "atom_name" or "atom_name:..." */
> +	*val = body + 1; /* "atom_name:val" */
> +	return 1;
> +}

Reversing the logic like this would be better IMHO:

static int match_atom_name(const char *name, const char *atom_name, const char **val)
{
        const char *body;
	if (!skip_prefix(name, atom_name, &body)) {
		/* doesn't even begin with "atom_name" */
		return 0;
	}
	if (!body[0]) {
		/* "atom_name" and no customization */
                *val = NULL;
                return 1;
	}
	if (body[0] != ':')
		/* "atom_namefoo" is not "atom_name" or "atom_name:..." */
		return 0;
	if (!body[1]) {
		/* "atom_name:" */
		*val = NULL;
		return 1;
	}
	/* "atom_name:... */
	*val = body + 1;
	return 1;
}

=> each case appears very clearly, and we check body[0] != ':' before
testing !body[1], so %(colorX) is rejected before noticing the '\0'
after the 'X'.

"atom_name:" appears explicitly. If we want to reject it, we know which
code to change.

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

Re: [PATCH v17 06/14] ref-filter: implement an `align` atom

[Matthieu Moy]

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

> +			/*
> +			 * TODO: Implement a function similar to strbuf_split_str()
> +			 * which would omit the separator from the end of each value.
> +			 */
> +			s = to_free = strbuf_split_str(valp, ',', 0);
> +
> +			align->position = ALIGN_LEFT;
> +
> +			while (*s) {
> +				if (s[1])

A comment like /* strip trailing ',' */ here would really help the
reader IMHO.

> +					strbuf_setlen(s[0], s[0]->len - 1);

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

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Junio C Hamano]

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

> Introduce match_atom_name() which helps in checking if a particular
> atom is the atom we're looking for and if it has a value attached to
> it or not.
>
> Use it instead of starts_with() for checking the value of %(color:...)
> atom. Write a test for the same.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Thanks-to: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  ref-filter.c                   | 23 +++++++++++++++++++++--
>  t/t6302-for-each-ref-filter.sh |  4 ++++
>  2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index a993216..70d36fe 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -189,6 +189,22 @@ static void pop_stack_element(struct ref_formatting_stack **stack)
>  	*stack = prev;
>  }
>  
> +static int match_atom_name(const char *name, const char *atom_name, const char **val)
> +{
> +	const char *body;
> +
> +	if (!skip_prefix(name, atom_name, &body))
> +		return 0; /* doesn't even begin with "atom_name" */
> +	if (!body[0] || !body[1]) {
> +		*val = NULL; /* %(atom_name) and no customization */

Why do we check body[1] here?  I do not understand why you are not
checking !body[0] alone nothing else in this if condition.

For (atom_name="align", name="aligna"), should the function say that
"%(aligna)" is an "%(align)" with no customization?

> +		return 1;
> +	}
> +	if (body[0] != ':')
> +		return 0; /* "atom_namefoo" is not "atom_name" or "atom_name:..." */
> +	*val = body + 1; /* "atom_name:val" */
> +	return 1;
> +}
> +
>  /*
>   * In a format string, find the next occurrence of %(atom).
>   */
> @@ -687,6 +703,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;
>  
>  		v->handler = append_atom;
> @@ -721,10 +738,12 @@ static void populate_value(struct ref_array_item *ref)
>  			refname = branch_get_push(branch, NULL);
>  			if (!refname)
>  				continue;
> -		} else if (starts_with(name, "color:")) {
> +		} else if (match_atom_name(name, "color", &valp)) {

Why use the helper only for this one?  Aren't existing calls to
starts_with() in the same if/else if/... cascade all potential bugs
that the new helper function is meant to help fixing?  For example,
the very fist one in the cascade:

	if (starts_with(name, "refname"))
        	refname = ref->refname;

is correct *ONLY* when name is "refname" or "refname:" followed by
something, and it should skip "refnamex" when such a new atom is
added to valid_atom[] list, i.e. a bug waiting to happen.  I think
the new helper is designed to prevent such a bug from happening.

>  			char color[COLOR_MAXLEN] = "";
>  
> -			if (color_parse(name + 6, color) < 0)
> +			if (!valp)
> +				die(_("expected format: %%(color:<color>)"));
> +			if (color_parse(valp, color) < 0)
>  				die(_("unable to parse format"));
>  			v->s = xstrdup(color);
>  			continue;
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 505a360..c4f0378 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -81,4 +81,8 @@ test_expect_success 'filtering with --contains' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success '%(color) must fail' '
> +	test_must_fail git for-each-ref --format="%(color)%(refname)"
> +'
> +
>  test_done

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

[Matthieu Moy]

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

> This is part of the series of unifying the code used by
> "git tag -l, git branch -l, git for-each-ref".
>
> The previous version can be found here (version 16):
> article.gmane.org/gmane.comp.version-control.git/277394
>
> Changes in this version:
> * The arguments of the %(align) atom are interchangeable.
> * Small grammatical changes.
> * Small changes in the tests to reflect changes in the align
> atom code.

Clearly, we're almost there. I did a few minor remarks. I suggest
(admitedly, Eric suggested of-list to suggest ;-) ) that you reply to
them by re-sending only individual patches that changed (replying to the
original patch) so that we can check the new patches individually. I
think we can do the finishing touches for each patch in a subthread of
this patch.

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

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Matthieu Moy]

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

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> -		} else if (starts_with(name, "color:")) {
>> +		} else if (match_atom_name(name, "color", &valp)) {
>
> Why use the helper only for this one?  Aren't existing calls to
> starts_with() in the same if/else if/... cascade all potential bugs
> that the new helper function is meant to help fixing?  For example,
> the very fist one in the cascade:
>
> 	if (starts_with(name, "refname"))
>         	refname = ref->refname;
>
> is correct *ONLY* when name is "refname" or "refname:" followed by
> something, and it should skip "refnamex" when such a new atom is
> added to valid_atom[] list, i.e. a bug waiting to happen.  I think
> the new helper is designed to prevent such a bug from happening.

I fully agree, but I also think that this should be a separate topic.

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

Re: [PATCH v17 06/14] ref-filter: implement an `align` atom

[Junio C Hamano]

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

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

Nicely done.

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Karthik Nayak]

On Thu, Sep 10, 2015 at 10:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Introduce match_atom_name() which helps in checking if a particular
>> atom is the atom we're looking for and if it has a value attached to
>> it or not.
>>
>> Use it instead of starts_with() for checking the value of %(color:...)
>> atom. Write a test for the same.
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Thanks-to: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  ref-filter.c                   | 23 +++++++++++++++++++++--
>>  t/t6302-for-each-ref-filter.sh |  4 ++++
>>  2 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index a993216..70d36fe 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -189,6 +189,22 @@ static void pop_stack_element(struct ref_formatting_stack **stack)
>>       *stack = prev;
>>  }
>>
>> +static int match_atom_name(const char *name, const char *atom_name, const char **val)
>> +{
>> +     const char *body;
>> +
>> +     if (!skip_prefix(name, atom_name, &body))
>> +             return 0; /* doesn't even begin with "atom_name" */
>> +     if (!body[0] || !body[1]) {
>> +             *val = NULL; /* %(atom_name) and no customization */
>
> Why do we check body[1] here?  I do not understand why you are not
> checking !body[0] alone nothing else in this if condition.
>
> For (atom_name="align", name="aligna"), should the function say that
> "%(aligna)" is an "%(align)" with no customization?
>

The check was for checking if there is anything after the colon,
Matthieu's modified version seems better though.

>
> Why use the helper only for this one?  Aren't existing calls to
> starts_with() in the same if/else if/... cascade all potential bugs
> that the new helper function is meant to help fixing?  For example,
> the very fist one in the cascade:
>
>         if (starts_with(name, "refname"))
>                 refname = ref->refname;
>
> is correct *ONLY* when name is "refname" or "refname:" followed by
> something, and it should skip "refnamex" when such a new atom is
> added to valid_atom[] list, i.e. a bug waiting to happen.  I think
> the new helper is designed to prevent such a bug from happening.
>

Yes definitely, but it would work with only refname, whereas
the color atom had no check before this patch, I didn't want to introduce
those patches in this series.

-- 
Regards,
Karthik Nayak

Re: [PATCH v17 08/14] ref-filter: add support for %(contents:lines=X)

[Junio C Hamano]

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

> diff --git a/ref-filter.c b/ref-filter.c
> index 7d2732a..b098b16 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -56,6 +56,7 @@ static struct {
>  	{ "color" },
>  	{ "align" },
>  	{ "end" },
> +	{ "contents:lines" },

Do we even need "contents:lines" and existing other "contents:blah"
in this list in the first place?  If they are needed, group them
together, not append at the end.

I wonder how this code sensibly can parse "%(contents:lines=6)".
After splitting the format string at %( and closing ), the code
calls parse_ref_filter_atom() and the rule that helper function uses
to figure out the atom-name proper (which is to be checked against
the valid_atom[] array) is to find the first colon, so

    %(contents:lines=6)

would cause "contents:lines=6" to be fed parse_ref_filter_atom(),
it cheks if "contents" is in the valid_atom[] array (it is), and
stores the whole thing in used_atom[].

So in that sense, match_atom_name() would do the right thing, but
that would make any reader of this code realize that she never saw
"contents:lines" entry in valid_atom[] array being used during this
process.

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Junio C Hamano]

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

> OTOH, you are now accepting %(atom:) as a synonym to %(atom), and it's
> not clear whether this is a deliberate decition.

I would say so.  When the caller wants to reject %(atom:), the
caller can tell it by checking val[0] == '\0' and reject that.

So it is better if you did not do this:

> 	if (!body[1]) {
> 		/* "atom_name:" */
> 		*val = NULL;
> 		return 1;
> 	}

which robs that information from the caller.  It should be
sufficient to just drop the check that allows "colorx" when
expecting "color" without making any other change, I would think.

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Junio C Hamano]

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

> The check was for checking if there is anything after the colon,

Why do you even care?  If %(color) expects more specific
customization by adding colon followed by specific data after it,
i.e. %(color:something), %(color:) should clearly be that "%(color)"
thing with no customization---if it is "not enough customization" or
"leaving everything default" depends on each atom, match_atom_name()
is not a good place to make that policy decision (i.e. Mattheiu's
rewrite to clear *valp to NULL when %(color:) is seen).  Instead,
point *val to body + 1 just as usual, and let the caller tell
between *val being NULL (not even any colon) and *val pointing at a
NUL byte (nothing after colon) if it cares.

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Karthik Nayak]

On Thu, Sep 10, 2015 at 10:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> The check was for checking if there is anything after the colon,
>
> Why do you even care?  If %(color) expects more specific
> customization by adding colon followed by specific data after it,
> i.e. %(color:something), %(color:) should clearly be that "%(color)"
> thing with no customization---if it is "not enough customization" or
> "leaving everything default" depends on each atom, match_atom_name()
> is not a good place to make that policy decision (i.e. Mattheiu's
> rewrite to clear *valp to NULL when %(color:) is seen).  Instead,
> point *val to body + 1 just as usual, and let the caller tell
> between *val being NULL (not even any colon) and *val pointing at a
> NUL byte (nothing after colon) if it cares.

It is one thing that the user can actually do the check themselves,
but doesn't it make more sense that when we're using colon we expect a
value after it, and something like %(color:) makes no sense when color
specifically needs a value after the colon.

Hence rather than assuming that the no value is given and fallback to
the default
%(color), wouldn't it be better to warn the user that there is no value?

But then again we can leave this decision to the user like you said.

-- 
Regards,
Karthik Nayak

Re: [PATCH v17 08/14] ref-filter: add support for %(contents:lines=X)

[Karthik Nayak]

On Thu, Sep 10, 2015 at 10:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 7d2732a..b098b16 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -56,6 +56,7 @@ static struct {
>>       { "color" },
>>       { "align" },
>>       { "end" },
>> +     { "contents:lines" },
>
> Do we even need "contents:lines" and existing other "contents:blah"
> in this list in the first place?  If they are needed, group them
> together, not append at the end.
>
> I wonder how this code sensibly can parse "%(contents:lines=6)".
> After splitting the format string at %( and closing ), the code
> calls parse_ref_filter_atom() and the rule that helper function uses
> to figure out the atom-name proper (which is to be checked against
> the valid_atom[] array) is to find the first colon, so
>
>     %(contents:lines=6)
>
> would cause "contents:lines=6" to be fed parse_ref_filter_atom(),
> it cheks if "contents" is in the valid_atom[] array (it is), and
> stores the whole thing in used_atom[].
>
> So in that sense, match_atom_name() would do the right thing, but
> that would make any reader of this code realize that she never saw
> "contents:lines" entry in valid_atom[] array being used during this
> process.

True.
We could remove all the contents:<subvalue> atoms from this list.

-- 
Regards,
Karthik Nayak

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Junio C Hamano]

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

> It is one thing that the user can actually do the check themselves,
> but doesn't it make more sense that when we're using colon we expect a
> value after it, and something like %(color:) makes no sense when color
> specifically needs a value after the colon.

If you imagine the format being built by scripts (we are talking
about plumbing feature --format here), I think you will realize that
it perfectly makes sense to allow them to say "%(atom:$modifiation)"
without having to worry about a special case where $modification
happened to end up being empty.  So no, I do not agree with your
statement at all.

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Karthik Nayak]

On Thu, Sep 10, 2015 at 11:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> It is one thing that the user can actually do the check themselves,
>> but doesn't it make more sense that when we're using colon we expect a
>> value after it, and something like %(color:) makes no sense when color
>> specifically needs a value after the colon.
>
> If you imagine the format being built by scripts (we are talking
> about plumbing feature --format here), I think you will realize that
> it perfectly makes sense to allow them to say "%(atom:$modifiation)"
> without having to worry about a special case where $modification
> happened to end up being empty.  So no, I do not agree with your
> statement at all.

Ah! that makes sense, thanks :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v17 13/14] tag.c: implement '--format' option

[Junio C Hamano]

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

> -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, *to_free = NULL;
> +	char *to_free = NULL;
>  	int i;

format is const char * while to_free is non-const char * here.

> @@ -41,12 +41,14 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
>  	if (filter->lines == -1)
>  		filter->lines = 0;
>  
> +	if (!format) {
> +		if (filter->lines) {
> +			format = xstrfmt("%s %%(contents:lines=%d)",
> +					 "%(align:15)%%(refname:short)%%(end)", filter->lines);

Hmmm, did this even pass tests and if so how?  What are these double
%% doing before refname and end?  Perhaps we do not have enough test
coverage?

> +			to_free = format;

This assignment discards constness.

Take the result of xstrfmt() to to_free (which is a non-const
pointer) and then assigning it to format (which is a const pointer)
would work it around.

Re: [PATCH v17 13/14] tag.c: implement '--format' option

[Karthik Nayak]

On Thu, Sep 10, 2015 at 11:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> -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, *to_free = NULL;
>> +     char *to_free = NULL;
>>       int i;
>
> format is const char * while to_free is non-const char * here.
>
>> @@ -41,12 +41,14 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
>>       if (filter->lines == -1)
>>               filter->lines = 0;
>>
>> +     if (!format) {
>> +             if (filter->lines) {
>> +                     format = xstrfmt("%s %%(contents:lines=%d)",
>> +                                      "%(align:15)%%(refname:short)%%(end)", filter->lines);
>
> Hmmm, did this even pass tests and if so how?  What are these double
> %% doing before refname and end?  Perhaps we do not have enough test
> coverage?
>

Seems like I didn't run the tests here, will change this.

>> +                     to_free = format;
>
> This assignment discards constness.
>
> Take the result of xstrfmt() to to_free (which is a non-const
> pointer) and then assigning it to format (which is a const pointer)
> would work it around.

Yeah!

-- 
Regards,
Karthik Nayak

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> -		} else if (starts_with(name, "color:")) {
>>> +		} else if (match_atom_name(name, "color", &valp)) {
>>
>> Why use the helper only for this one?  Aren't existing calls to
>> starts_with() in the same if/else if/... cascade all potential bugs
>> that the new helper function is meant to help fixing?  For example,
>> the very fist one in the cascade:
>>
>> 	if (starts_with(name, "refname"))
>>         	refname = ref->refname;
>>
>> is correct *ONLY* when name is "refname" or "refname:" followed by
>> something, and it should skip "refnamex" when such a new atom is
>> added to valid_atom[] list, i.e. a bug waiting to happen.  I think
>> the new helper is designed to prevent such a bug from happening.
>
> I fully agree, but I also think that this should be a separate topic.

Yeah, it can be a separate topic.  I am neutral (i.e. I certainly
would not insist that the existing one should be fixed with the
helper in the series, but I cannot quite say that I prefer the fix
to be made outside this topic, either).

Thanks.

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> OTOH, you are now accepting %(atom:) as a synonym to %(atom), and it's
>> not clear whether this is a deliberate decition.
>
> I would say so.  When the caller wants to reject %(atom:), the
> caller can tell it by checking val[0] == '\0' and reject that.
>
> So it is better if you did not do this:
>
>> 	if (!body[1]) {
>> 		/* "atom_name:" */
>> 		*val = NULL;
>> 		return 1;
>> 	}
>
> which robs that information from the caller.

OK. Just dropping this part lets the code fall back to

	/* "atom_name:... */
	*val = body + 1;
	return 1;

right below in my version. It also accepts it (return 1) but lets val
point to an empty string. Makes sense.

And indeed, without this, my code looks a lot like Karthik's one, just
dropping the "|| !body[1]" part in a condition.

In any case, I'd like to see "atom_name:" explicitly mentionned
somewhere in a comment, if only to make it clear that what is done with
it is deliberate (e.g. avoid having someone not following this
conversation later considering this %(atom:) thing to be a bug and try
to fix it).

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

[PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Karthik Nayak]

Introduce match_atom_name() which helps in checking if a particular
atom is the atom we're looking for and if it has a value attached to
it or not.

Use it instead of starts_with() for checking the value of %(color:...)
atom. Write a test for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Thanks-to: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c                   | 23 +++++++++++++++++++++--
 t/t6302-for-each-ref-filter.sh |  4 ++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index a993216..514de34 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -189,6 +189,22 @@ static void pop_stack_element(struct ref_formatting_stack **stack)
 	*stack = prev;
 }
 
+static int match_atom_name(const char *name, const char *atom_name, const char **val)
+{
+	const char *body;
+
+	if (!skip_prefix(name, atom_name, &body))
+		return 0; /* doesn't even begin with "atom_name" */
+	if (!body[0]) {
+		*val = NULL; /* %(atom_name) and no customization */
+		return 1;
+	}
+	if (body[0] != ':')
+		return 0; /* "atom_namefoo" is not "atom_name" or "atom_name:..." */
+	*val = body + 1; /* "atom_name:val" */
+	return 1;
+}
+
 /*
  * In a format string, find the next occurrence of %(atom).
  */
@@ -687,6 +703,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;
 
 		v->handler = append_atom;
@@ -721,10 +738,12 @@ static void populate_value(struct ref_array_item *ref)
 			refname = branch_get_push(branch, NULL);
 			if (!refname)
 				continue;
-		} else if (starts_with(name, "color:")) {
+		} else if (match_atom_name(name, "color", &valp)) {
 			char color[COLOR_MAXLEN] = "";
 
-			if (color_parse(name + 6, color) < 0)
+			if (!valp)
+				die(_("expected format: %%(color:<color>)"));
+			if (color_parse(valp, color) < 0)
 				die(_("unable to parse format"));
 			v->s = xstrdup(color);
 			continue;
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 505a360..c4f0378 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -81,4 +81,8 @@ test_expect_success 'filtering with --contains' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(color) must fail' '
+	test_must_fail git for-each-ref --format="%(color)%(refname)"
+'
+
 test_done
-- 
2.5.1

[PATCH v17 06/14] ref-filter: implement an `align` atom

[Karthik Nayak]

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

The "align:" is followed by `<width>` and `<position>` in any order
separated by a comma, where the `<position>` is either left, right or
middle, default being left 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.  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 introduce an `at_end` function for each element of the stack which
is to be called when the `end` atom is encountered. Using this we
implement end_align_handler() for the `align` atom, this aligns the
final strbuf by calling `strbuf_utf8_align()` from utf8.c.

Ensure that quote formatting is performed on the whole of
%(align:...)...%(end) rather than individual atoms inside. We skip
quote formatting for individual atoms when the current stack element
is handling an %(align:...) atom and perform quote formatting at the
end when we encounter the %(end) atom of the second element of then
stack.

Add documentation and tests for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt |  11 ++++
 ref-filter.c                       | 110 ++++++++++++++++++++++++++++++++++++-
 t/t6302-for-each-ref-filter.sh     |  82 +++++++++++++++++++++++++++
 3 files changed, 202 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index e49d578..3a271bf 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -127,6 +127,17 @@ 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). The "align:" is followed by `<width>`
+	and `<position>` in any order separated by a comma, where the
+	`<position>` is either left, right or middle, default being
+	left 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. If used with '--quote' everything
+	in between %(align:...) and %(end) is quoted, but if nested
+	then only the topmost level performs quoting.
+
 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 514de34..c65cd60 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,13 +54,22 @@ static struct {
 	{ "flag" },
 	{ "HEAD" },
 	{ "color" },
+	{ "align" },
+	{ "end" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
 
+struct align {
+	align_type position;
+	unsigned int width;
+};
+
 struct ref_formatting_stack {
 	struct ref_formatting_stack *prev;
 	struct strbuf output;
+	void (*at_end)(struct ref_formatting_stack *stack);
+	void *at_end_data;
 };
 
 struct ref_formatting_state {
@@ -69,6 +79,9 @@ struct ref_formatting_state {
 
 struct atom_value {
 	const char *s;
+	union {
+		struct align align;
+	} u;
 	void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
 	unsigned long ul; /* used for sorting when not FIELD_STR */
 };
@@ -165,7 +178,16 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
 
 static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
 {
-	quote_formatting(&state->stack->output, v->s, state->quote_style);
+	/*
+	 * Quote formatting is only done when the stack has a single
+	 * element. Otherwise quote formatting is done on the
+	 * element's entire output strbuf when the %(end) atom is
+	 * encountered.
+	 */
+	if (!state->stack->prev)
+		quote_formatting(&state->stack->output, v->s, state->quote_style);
+	else
+		strbuf_addstr(&state->stack->output, v->s);
 }
 
 static void push_stack_element(struct ref_formatting_stack **stack)
@@ -189,6 +211,48 @@ static void pop_stack_element(struct ref_formatting_stack **stack)
 	*stack = prev;
 }
 
+static void end_align_handler(struct ref_formatting_stack *stack)
+{
+	struct align *align = (struct align *)stack->at_end_data;
+	struct strbuf s = STRBUF_INIT;
+
+	strbuf_utf8_align(&s, align->position, align->width, stack->output.buf);
+	strbuf_swap(&stack->output, &s);
+	strbuf_release(&s);
+}
+
+static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+	struct ref_formatting_stack *new;
+
+	push_stack_element(&state->stack);
+	new = state->stack;
+	new->at_end = end_align_handler;
+	new->at_end_data = &atomv->u.align;
+}
+
+static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+	struct ref_formatting_stack *current = state->stack;
+	struct strbuf s = STRBUF_INIT;
+
+	if (!current->at_end)
+		die(_("format: %%(end) atom used without corresponding atom"));
+	current->at_end(current);
+
+	/*
+	 * Perform quote formatting when the stack element is that of
+	 * a supporting atom. If nested then perform quote formatting
+	 * only on the topmost supporting atom.
+	 */
+	if (!state->stack->prev->prev) {
+		quote_formatting(&s, current->output.buf, state->quote_style);
+		strbuf_swap(&current->output, &s);
+	}
+	strbuf_release(&s);
+	pop_stack_element(&state->stack);
+}
+
 static int match_atom_name(const char *name, const char *atom_name, const char **val)
 {
 	const char *body;
@@ -773,6 +837,48 @@ static void populate_value(struct ref_array_item *ref)
 			else
 				v->s = " ";
 			continue;
+		} else if (match_atom_name(name, "align", &valp)) {
+			struct align *align = &v->u.align;
+			struct strbuf **s, **to_free;
+			int width = -1;
+
+			if (!valp)
+				die(_("expected format: %%(align:<width>,<position>)"));
+
+			/*
+			 * TODO: Implement a function similar to strbuf_split_str()
+			 * which would omit the separator from the end of each value.
+			 */
+			s = to_free = strbuf_split_str(valp, ',', 0);
+
+			align->position = ALIGN_LEFT;
+
+			while (*s) {
+				/*  Strip trailing comma */
+				if (s[1])
+					strbuf_setlen(s[0], s[0]->len - 1);
+				if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
+					;
+				else if (!strcmp(s[0]->buf, "left"))
+					align->position = ALIGN_LEFT;
+				else if (!strcmp(s[0]->buf, "right"))
+					align->position = ALIGN_RIGHT;
+				else if (!strcmp(s[0]->buf, "middle"))
+					align->position = ALIGN_MIDDLE;
+				else
+					die(_("improper format entered align:%s"), s[0]->buf);
+				s++;
+			}
+
+			if (width < 0)
+				die(_("positive width expected with the %%(align) atom"));
+			align->width = width;
+			strbuf_list_free(to_free);
+			v->handler = align_atom_handler;
+			continue;
+		} else if (!strcmp(name, "end")) {
+			v->handler = end_atom_handler;
+			continue;
 		} else
 			continue;
 
@@ -1347,6 +1453,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 		resetv.s = color;
 		append_atom(&resetv, &state);
 	}
+	if (state.stack->prev)
+		die(_("format: %%(end) atom missing"));
 	final_buf = &state.stack->output;
 	fwrite(final_buf->buf, 1, final_buf->len, stdout);
 	pop_stack_element(&state.stack);
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index c4f0378..f596035 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -85,4 +85,86 @@ test_expect_success '%(color) must fail' '
 	test_must_fail git for-each-ref --format="%(color)%(refname)"
 '
 
+test_expect_success 'left alignment is default' '
+	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)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:middle,30)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
+'
+
+# Individual atoms inside %(align:...) and %(end) must not be quoted.
+
+test_expect_success 'alignment with format quote' "
+	cat >expect <<-\EOF &&
+	|'      '\''master| A U Thor'\''      '|
+	|'       '\''side| A U Thor'\''       '|
+	|'     '\''odd/spot| A U Thor'\''     '|
+	|'        '\''double-tag| '\''        '|
+	|'       '\''four| A U Thor'\''       '|
+	|'       '\''one| A U Thor'\''        '|
+	|'        '\''signed-tag| '\''        '|
+	|'      '\''three| A U Thor'\''       '|
+	|'       '\''two| A U Thor'\''        '|
+	EOF
+	git for-each-ref --shell --format=\"|%(align:30,middle)'%(refname:short)| %(authorname)'%(end)|\" >actual &&
+	test_cmp expect actual
+"
+
+test_expect_success 'nested alignment with quote formatting' "
+	cat >expect <<-\EOF &&
+	|'         master               '|
+	|'           side               '|
+	|'       odd/spot               '|
+	|'     double-tag               '|
+	|'           four               '|
+	|'            one               '|
+	|'     signed-tag               '|
+	|'          three               '|
+	|'            two               '|
+	EOF
+	git for-each-ref --shell --format='|%(align:30,left)%(align:15,right)%(refname:short)%(end)%(end)|' >actual &&
+	test_cmp expect actual
+"
+
 test_done
-- 
2.5.1

[PATCH v17 06/14] ref-filter: implement an `align` atom

[Karthik Nayak]

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

The "align:" is followed by `<width>` and `<position>` in any order
separated by a comma, where the `<position>` is either left, right or
middle, default being left 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.  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 introduce an `at_end` function for each element of the stack which
is to be called when the `end` atom is encountered. Using this we
implement end_align_handler() for the `align` atom, this aligns the
final strbuf by calling `strbuf_utf8_align()` from utf8.c.

Ensure that quote formatting is performed on the whole of
%(align:...)...%(end) rather than individual atoms inside. We skip
quote formatting for individual atoms when the current stack element
is handling an %(align:...) atom and perform quote formatting at the
end when we encounter the %(end) atom of the second element of then
stack.

Add documentation and tests for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt |  11 ++++
 ref-filter.c                       | 110 ++++++++++++++++++++++++++++++++++++-
 t/t6302-for-each-ref-filter.sh     |  82 +++++++++++++++++++++++++++
 3 files changed, 202 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index e49d578..3a271bf 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -127,6 +127,17 @@ 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). The "align:" is followed by `<width>`
+	and `<position>` in any order separated by a comma, where the
+	`<position>` is either left, right or middle, default being
+	left 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. If used with '--quote' everything
+	in between %(align:...) and %(end) is quoted, but if nested
+	then only the topmost level performs quoting.
+
 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 514de34..c65cd60 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,13 +54,22 @@ static struct {
 	{ "flag" },
 	{ "HEAD" },
 	{ "color" },
+	{ "align" },
+	{ "end" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
 
+struct align {
+	align_type position;
+	unsigned int width;
+};
+
 struct ref_formatting_stack {
 	struct ref_formatting_stack *prev;
 	struct strbuf output;
+	void (*at_end)(struct ref_formatting_stack *stack);
+	void *at_end_data;
 };
 
 struct ref_formatting_state {
@@ -69,6 +79,9 @@ struct ref_formatting_state {
 
 struct atom_value {
 	const char *s;
+	union {
+		struct align align;
+	} u;
 	void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
 	unsigned long ul; /* used for sorting when not FIELD_STR */
 };
@@ -165,7 +178,16 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
 
 static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
 {
-	quote_formatting(&state->stack->output, v->s, state->quote_style);
+	/*
+	 * Quote formatting is only done when the stack has a single
+	 * element. Otherwise quote formatting is done on the
+	 * element's entire output strbuf when the %(end) atom is
+	 * encountered.
+	 */
+	if (!state->stack->prev)
+		quote_formatting(&state->stack->output, v->s, state->quote_style);
+	else
+		strbuf_addstr(&state->stack->output, v->s);
 }
 
 static void push_stack_element(struct ref_formatting_stack **stack)
@@ -189,6 +211,48 @@ static void pop_stack_element(struct ref_formatting_stack **stack)
 	*stack = prev;
 }
 
+static void end_align_handler(struct ref_formatting_stack *stack)
+{
+	struct align *align = (struct align *)stack->at_end_data;
+	struct strbuf s = STRBUF_INIT;
+
+	strbuf_utf8_align(&s, align->position, align->width, stack->output.buf);
+	strbuf_swap(&stack->output, &s);
+	strbuf_release(&s);
+}
+
+static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+	struct ref_formatting_stack *new;
+
+	push_stack_element(&state->stack);
+	new = state->stack;
+	new->at_end = end_align_handler;
+	new->at_end_data = &atomv->u.align;
+}
+
+static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+	struct ref_formatting_stack *current = state->stack;
+	struct strbuf s = STRBUF_INIT;
+
+	if (!current->at_end)
+		die(_("format: %%(end) atom used without corresponding atom"));
+	current->at_end(current);
+
+	/*
+	 * Perform quote formatting when the stack element is that of
+	 * a supporting atom. If nested then perform quote formatting
+	 * only on the topmost supporting atom.
+	 */
+	if (!state->stack->prev->prev) {
+		quote_formatting(&s, current->output.buf, state->quote_style);
+		strbuf_swap(&current->output, &s);
+	}
+	strbuf_release(&s);
+	pop_stack_element(&state->stack);
+}
+
 static int match_atom_name(const char *name, const char *atom_name, const char **val)
 {
 	const char *body;
@@ -773,6 +837,48 @@ static void populate_value(struct ref_array_item *ref)
 			else
 				v->s = " ";
 			continue;
+		} else if (match_atom_name(name, "align", &valp)) {
+			struct align *align = &v->u.align;
+			struct strbuf **s, **to_free;
+			int width = -1;
+
+			if (!valp)
+				die(_("expected format: %%(align:<width>,<position>)"));
+
+			/*
+			 * TODO: Implement a function similar to strbuf_split_str()
+			 * which would omit the separator from the end of each value.
+			 */
+			s = to_free = strbuf_split_str(valp, ',', 0);
+
+			align->position = ALIGN_LEFT;
+
+			while (*s) {
+				/*  Strip trailing comma */
+				if (s[1])
+					strbuf_setlen(s[0], s[0]->len - 1);
+				if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
+					;
+				else if (!strcmp(s[0]->buf, "left"))
+					align->position = ALIGN_LEFT;
+				else if (!strcmp(s[0]->buf, "right"))
+					align->position = ALIGN_RIGHT;
+				else if (!strcmp(s[0]->buf, "middle"))
+					align->position = ALIGN_MIDDLE;
+				else
+					die(_("improper format entered align:%s"), s[0]->buf);
+				s++;
+			}
+
+			if (width < 0)
+				die(_("positive width expected with the %%(align) atom"));
+			align->width = width;
+			strbuf_list_free(to_free);
+			v->handler = align_atom_handler;
+			continue;
+		} else if (!strcmp(name, "end")) {
+			v->handler = end_atom_handler;
+			continue;
 		} else
 			continue;
 
@@ -1347,6 +1453,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 		resetv.s = color;
 		append_atom(&resetv, &state);
 	}
+	if (state.stack->prev)
+		die(_("format: %%(end) atom missing"));
 	final_buf = &state.stack->output;
 	fwrite(final_buf->buf, 1, final_buf->len, stdout);
 	pop_stack_element(&state.stack);
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index c4f0378..f596035 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -85,4 +85,86 @@ test_expect_success '%(color) must fail' '
 	test_must_fail git for-each-ref --format="%(color)%(refname)"
 '
 
+test_expect_success 'left alignment is default' '
+	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)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:middle,30)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
+'
+
+# Individual atoms inside %(align:...) and %(end) must not be quoted.
+
+test_expect_success 'alignment with format quote' "
+	cat >expect <<-\EOF &&
+	|'      '\''master| A U Thor'\''      '|
+	|'       '\''side| A U Thor'\''       '|
+	|'     '\''odd/spot| A U Thor'\''     '|
+	|'        '\''double-tag| '\''        '|
+	|'       '\''four| A U Thor'\''       '|
+	|'       '\''one| A U Thor'\''        '|
+	|'        '\''signed-tag| '\''        '|
+	|'      '\''three| A U Thor'\''       '|
+	|'       '\''two| A U Thor'\''        '|
+	EOF
+	git for-each-ref --shell --format=\"|%(align:30,middle)'%(refname:short)| %(authorname)'%(end)|\" >actual &&
+	test_cmp expect actual
+"
+
+test_expect_success 'nested alignment with quote formatting' "
+	cat >expect <<-\EOF &&
+	|'         master               '|
+	|'           side               '|
+	|'       odd/spot               '|
+	|'     double-tag               '|
+	|'           four               '|
+	|'            one               '|
+	|'     signed-tag               '|
+	|'          three               '|
+	|'            two               '|
+	EOF
+	git for-each-ref --shell --format='|%(align:30,left)%(align:15,right)%(refname:short)%(end)%(end)|' >actual &&
+	test_cmp expect actual
+"
+
 test_done
-- 
2.5.1

[PATCH v17 08/14] ref-filter: add support for %(contents:lines=X)

[Karthik Nayak]

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 it to support appending of N lines from the annotation of tags
to the given strbuf.

Implement %(contents:lines=X) where X lines of the given object are
obtained.

While we're at it, remove unused "contents:<suboption>" atoms from
the `valid_atom` array.

Add documentation and test 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 ++-
 builtin/tag.c                      |  4 +++
 ref-filter.c                       | 47 +++++++++++++++++++++++++++++++---
 ref-filter.h                       |  3 ++-
 t/t6302-for-each-ref-filter.sh     | 52 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 103 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 3a271bf..324ad2c 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -150,7 +150,8 @@ The complete message in a commit and tag object is `contents`.
 Its first line is `contents:subject`, where subject is the concatenation
 of all lines of the commit message up to the first blank line.  The next
 line is 'contents:body', where body is all of the lines after the first
-blank line.  Finally, the optional GPG signature is `contents:signature`.
+blank line.  The optional GPG signature is `contents:signature`.  The
+first `N` lines of the message is obtained using `contents:lines=N`.
 
 For sorting purposes, fields with numeric values sort in numeric
 order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).
diff --git a/builtin/tag.c b/builtin/tag.c
index 471d6b1..b0bc1c5 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 modified and used in ref-filter as append_lines(), 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 f046d82..32aab37 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -45,9 +45,6 @@ static struct {
 	{ "subject" },
 	{ "body" },
 	{ "contents" },
-	{ "contents:subject" },
-	{ "contents:body" },
-	{ "contents:signature" },
 	{ "upstream" },
 	{ "push" },
 	{ "symref" },
@@ -65,6 +62,11 @@ struct align {
 	unsigned int width;
 };
 
+struct contents {
+	unsigned int lines;
+	struct object_id oid;
+};
+
 struct ref_formatting_stack {
 	struct ref_formatting_stack *prev;
 	struct strbuf output;
@@ -81,6 +83,7 @@ struct atom_value {
 	const char *s;
 	union {
 		struct align align;
+		struct contents contents;
 	} u;
 	void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
 	unsigned long ul; /* used for sorting when not FIELD_STR */
@@ -643,6 +646,30 @@ static void find_subpos(const char *buf, unsigned long sz,
 	*nonsiglen = *sig - buf;
 }
 
+/*
+ * If 'lines' is greater than 0, append that many lines from the given
+ * 'buf' of length 'size' to the given strbuf.
+ */
+static void append_lines(struct strbuf *out, const char *buf, unsigned long size, int lines)
+{
+	int i;
+	const char *sp, *eol;
+	size_t len;
+
+	sp = buf;
+
+	for (i = 0; i < lines && sp < buf + size; i++) {
+		if (i)
+			strbuf_addstr(out, "\n    ");
+		eol = memchr(sp, '\n', size - (sp - buf));
+		len = eol ? eol - sp : size - (sp - buf);
+		strbuf_add(out, sp, len);
+		if (!eol)
+			break;
+		sp = eol + 1;
+	}
+}
+
 /* See grab_values */
 static void grab_sub_body_contents(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
 {
@@ -653,6 +680,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i];
 		struct atom_value *v = &val[i];
+		const char *valp = NULL;
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
@@ -662,7 +690,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 		    strcmp(name, "contents") &&
 		    strcmp(name, "contents:subject") &&
 		    strcmp(name, "contents:body") &&
-		    strcmp(name, "contents:signature"))
+		    strcmp(name, "contents:signature") &&
+		    !starts_with(name, "contents:lines="))
 			continue;
 		if (!subpos)
 			find_subpos(buf, sz,
@@ -682,6 +711,16 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 			v->s = xmemdupz(sigpos, siglen);
 		else if (!strcmp(name, "contents"))
 			v->s = xstrdup(subpos);
+		else if (skip_prefix(name, "contents:lines=", &valp)) {
+			struct strbuf s = STRBUF_INIT;
+			const char *contents_end = bodylen + bodypos - siglen;
+
+			if (strtoul_ui(valp, 10, &v->u.contents.lines))
+				die(_("positive value expected contents:lines=%s"), valp);
+			/*  Size is the length of the message after removing the signature */
+			append_lines(&s, subpos, contents_end - subpos, v->u.contents.lines);
+			v->s = strbuf_detach(&s, NULL);
+		}
 	}
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 0913ba9..ab76b22 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -59,7 +59,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 {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index f596035..bab1f28 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -167,4 +167,56 @@ test_expect_success 'nested alignment with quote formatting' "
 	test_cmp expect actual
 "
 
+test_expect_success 'check `%(contents:lines=1)`' '
+	cat >expect <<-\EOF &&
+	master |three
+	side |four
+	odd/spot |three
+	double-tag |Annonated doubly
+	four |four
+	one |one
+	signed-tag |A signed tag message
+	three |three
+	two |two
+	EOF
+	git for-each-ref --format="%(refname:short) |%(contents:lines=1)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check `%(contents:lines=0)`' '
+	cat >expect <<-\EOF &&
+	master |
+	side |
+	odd/spot |
+	double-tag |
+	four |
+	one |
+	signed-tag |
+	three |
+	two |
+	EOF
+	git for-each-ref --format="%(refname:short) |%(contents:lines=0)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check `%(contents:lines=99999)`' '
+	cat >expect <<-\EOF &&
+	master |three
+	side |four
+	odd/spot |three
+	double-tag |Annonated doubly
+	four |four
+	one |one
+	signed-tag |A signed tag message
+	three |three
+	two |two
+	EOF
+	git for-each-ref --format="%(refname:short) |%(contents:lines=99999)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '`%(contents:lines=-1)` should fail' '
+	test_must_fail git for-each-ref --format="%(refname:short) |%(contents:lines=-1)"
+'
+
 test_done
-- 
2.5.1

[PATCH v17 12/14] 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             | 345 ++++++----------------------------------------
 t/t7004-tag.sh            |   8 +-
 3 files changed, 53 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 fe66f7b..977a18c 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -28,278 +28,35 @@ 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 modified and used in ref-filter as append_lines(), 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, *to_free = NULL;
+	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) {
+		to_free = xstrfmt("%s %%(contents:lines=%d)",
+				 "%(align:15)%(refname:short)%(end)", filter->lines);
+		format = to_free;
+	} 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, 0);
+	ref_array_clear(&array);
+	free(to_free);
+
 	return 0;
 }
 
@@ -366,35 +123,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 +150,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 +313,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 +329,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 +356,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 +365,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 +391,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 +401,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.1

[PATCH v17 13/14] tag.c: implement '--format' option

[Karthik Nayak]

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 |  8 +++++++-
 builtin/tag.c             | 24 ++++++++++++++----------
 t/t7004-tag.sh            | 12 ++++++++++++
 3 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 3ac4a96..0c7f4e6 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,11 @@ 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.  The format is the same as
+	that of linkgit:git-for-each-ref[1].  When unspecified,
+	defaults to `%(refname:short)`.
 
 CONFIGURATION
 -------------
diff --git a/builtin/tag.c b/builtin/tag.c
index 977a18c..91c17b7 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -23,17 +23,17 @@ 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[--format=<format>] [<pattern>...]"),
 	N_("git tag -v <tagname>..."),
 	NULL
 };
 
 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, *to_free = NULL;
+	char *to_free = NULL;
 	int i;
 
 	memset(&array, 0, sizeof(array));
@@ -41,12 +41,14 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
 	if (filter->lines == -1)
 		filter->lines = 0;
 
-	if (filter->lines) {
-		to_free = xstrfmt("%s %%(contents:lines=%d)",
-				 "%(align:15)%(refname:short)%(end)", filter->lines);
-		format = to_free;
-	} else
-		format = "%(refname:short)";
+	if (!format) {
+		if (filter->lines) {
+			to_free = xstrfmt("%s %%(contents:lines=%d)",
+					  "%(align:15)%(refname:short)%(end)", filter->lines);
+			format = to_free;
+		} else
+			format = "%(refname:short)";
+	}
 
 	verify_ref_format(format);
 	filter_refs(&array, filter, FILTER_REFS_TAGS);
@@ -330,6 +332,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"),
@@ -362,6 +365,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()
 	};
 
@@ -402,7 +406,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			run_column_filter(colopts, &copts);
 		}
 		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..8987fb1 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1519,4 +1519,16 @@ EOF"
 	test_cmp expect actual
 '
 
+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.1

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

[Karthik Nayak]

On Thu, Sep 10, 2015 at 10:27 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> This is part of the series of unifying the code used by
>> "git tag -l, git branch -l, git for-each-ref".
>>
>> The previous version can be found here (version 16):
>> article.gmane.org/gmane.comp.version-control.git/277394
>>
>> Changes in this version:
>> * The arguments of the %(align) atom are interchangeable.
>> * Small grammatical changes.
>> * Small changes in the tests to reflect changes in the align
>> atom code.
>
> Clearly, we're almost there. I did a few minor remarks. I suggest
> (admitedly, Eric suggested of-list to suggest ;-) ) that you reply to
> them by re-sending only individual patches that changed (replying to the
> original patch) so that we can check the new patches individually. I
> think we can do the finishing touches for each patch in a subthread of
> this patch.
>

I replied with suggested changes by you and Junio.
Let me know if any other changes to be made :)

-- 
Regards,
Karthik Nayak

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

[Eric Sunshine]

On Fri, Sep 11, 2015 at 11:08 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Thu, Sep 10, 2015 at 10:27 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>> Changes in this version:
>>> * The arguments of the %(align) atom are interchangeable.
>>> * Small grammatical changes.
>>> * Small changes in the tests to reflect changes in the align
>>> atom code.
>>
>> Clearly, we're almost there. I did a few minor remarks. I suggest
>> (admitedly, Eric suggested of-list to suggest ;-) ) that you reply to
>> them by re-sending only individual patches that changed (replying to the
>> original patch) so that we can check the new patches individually. I
>> think we can do the finishing touches for each patch in a subthread of
>> this patch.
>
> I replied with suggested changes by you and Junio.
> Let me know if any other changes to be made :)

Hmm, but what actually changed in the re-sent patches? Without a link
to the discussion leading up to the re-send of changed-only patches,
and without an interdiff, the re-send is opaque and less accessible to
the reviewer; which is at odds with Matthieu's suggestion which was
intended to make review easier and more streamlined.

In addition to a link to the previous round and an interdiff, it would
be helpful to reviewers for you to annotate each patch (in the
commentary are below the "---" line after your sign-off) with a
description of the changes in that patch since the previous round in
order to focus the reviewer's attention (where it needs to be) on the
latest changes.

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> In addition to a link to the previous round and an interdiff, it would
> be helpful to reviewers for you to annotate each patch (in the
> commentary are below the "---" line after your sign-off) with a
> description of the changes in that patch since the previous round in
> order to focus the reviewer's attention (where it needs to be) on the
> latest changes.

I may have got confused by seeing the same v17 (if they were marked
as v18 or v17bis, it would have been easier to make sure I didn't
miss anything), but here is the difference between what I had last
night and what I queued.  The removal of !body[1] and flipping the
order of to_free/format are not seen because I already had a local
fix-up SQUASH??? commits queued in the yesterday's batch.


$ git diff --stat -p kn/for-each-tag@{4.hours} kn/for-each-tag
 ref-filter.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 226e94d..fd839ac 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -47,9 +47,6 @@ static struct {
 	{ "subject" },
 	{ "body" },
 	{ "contents" },
-	{ "contents:subject" },
-	{ "contents:body" },
-	{ "contents:signature" },
 	{ "upstream" },
 	{ "push" },
 	{ "symref" },
@@ -58,7 +55,6 @@ static struct {
 	{ "color" },
 	{ "align" },
 	{ "end" },
-	{ "contents:lines" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -899,6 +895,7 @@ static void populate_value(struct ref_array_item *ref)
 			align->position = ALIGN_LEFT;
 
 			while (*s) {
+				/*  Strip trailing comma */
 				if (s[1])
 					strbuf_setlen(s[0], s[0]->len - 1);
 				if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))

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

[Karthik Nayak]

On Fri, Sep 11, 2015 at 11:00 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> Hmm, but what actually changed in the re-sent patches? Without a link
> to the discussion leading up to the re-send of changed-only patches,
> and without an interdiff, the re-send is opaque and less accessible to
> the reviewer; which is at odds with Matthieu's suggestion which was
> intended to make review easier and more streamlined.
>

Should have put an interdiff, my bad!

> In addition to a link to the previous round and an interdiff, it would
> be helpful to reviewers for you to annotate each patch (in the
> commentary are below the "---" line after your sign-off) with a
> description of the changes in that patch since the previous round in
> order to focus the reviewer's attention (where it needs to be) on the
> latest changes.

WIll follow this next time :)

On Fri, Sep 11, 2015 at 11:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> In addition to a link to the previous round and an interdiff, it would
>> be helpful to reviewers for you to annotate each patch (in the
>> commentary are below the "---" line after your sign-off) with a
>> description of the changes in that patch since the previous round in
>> order to focus the reviewer's attention (where it needs to be) on the
>> latest changes.
>
> I may have got confused by seeing the same v17 (if they were marked
> as v18 or v17bis, it would have been easier to make sure I didn't
> miss anything), but here is the difference between what I had last
> night and what I queued.  The removal of !body[1] and flipping the
> order of to_free/format are not seen because I already had a local
> fix-up SQUASH??? commits queued in the yesterday's batch.
>
>
> $ git diff --stat -p kn/for-each-tag@{4.hours} kn/for-each-tag
>  ref-filter.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 226e94d..fd839ac 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -47,9 +47,6 @@ static struct {
>         { "subject" },
>         { "body" },
>         { "contents" },
> -       { "contents:subject" },
> -       { "contents:body" },
> -       { "contents:signature" },
>         { "upstream" },
>         { "push" },
>         { "symref" },
> @@ -58,7 +55,6 @@ static struct {
>         { "color" },
>         { "align" },
>         { "end" },
> -       { "contents:lines" },
>  };
>
>  #define REF_FORMATTING_STATE_INIT  { 0, NULL }
> @@ -899,6 +895,7 @@ static void populate_value(struct ref_array_item *ref)
>                         align->position = ALIGN_LEFT;
>
>                         while (*s) {
> +                               /*  Strip trailing comma */
>                                 if (s[1])
>                                         strbuf_setlen(s[0], s[0]->len - 1);
>                                 if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))

The complete interdiff:

diff --git a/builtin/tag.c b/builtin/tag.c
index 081fe84..f2f6e2d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -43,9 +43,9 @@ static int list_tags(struct ref_filter *filter,
struct ref_sorting *sorting, con

     if (!format) {
         if (filter->lines) {
-            format = xstrfmt("%s %%(contents:lines=%d)",
-                     "%(align:15)%%(refname:short)%%(end)", filter->lines);
-            to_free = format;
+            to_free = xstrfmt("%s %%(contents:lines=%d)",
+                      "%(align:15)%(refname:short)%(end)", filter->lines);
+            format = to_free;
         } else
             format = "%(refname:short)";
     }
diff --git a/ref-filter.c b/ref-filter.c
index 59716db..fd839ac 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -47,9 +47,6 @@ static struct {
     { "subject" },
     { "body" },
     { "contents" },
-    { "contents:subject" },
-    { "contents:body" },
-    { "contents:signature" },
     { "upstream" },
     { "push" },
     { "symref" },
@@ -58,7 +55,6 @@ static struct {
     { "color" },
     { "align" },
     { "end" },
-    { "contents:lines" },
 };

 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -268,7 +264,7 @@ static int match_atom_name(const char *name, const
char *atom_name, const char *

     if (!skip_prefix(name, atom_name, &body))
         return 0; /* doesn't even begin with "atom_name" */
-    if (!body[0] || !body[1]) {
+    if (!body[0]) {
         *val = NULL; /* %(atom_name) and no customization */
         return 1;
     }
@@ -899,6 +895,7 @@ static void populate_value(struct ref_array_item *ref)
             align->position = ALIGN_LEFT;

             while (*s) {
+                /*  Strip trailing comma */
                 if (s[1])
                     strbuf_setlen(s[0], s[0]->len - 1);
                 if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))


-- 
Regards,
Karthik Nayak

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

[Matthieu Moy]

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

> On Thu, Sep 10, 2015 at 10:27 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> This is part of the series of unifying the code used by
>>> "git tag -l, git branch -l, git for-each-ref".
>>>
>>> The previous version can be found here (version 16):
>>> article.gmane.org/gmane.comp.version-control.git/277394
>>>
>>> Changes in this version:
>>> * The arguments of the %(align) atom are interchangeable.
>>> * Small grammatical changes.
>>> * Small changes in the tests to reflect changes in the align
>>> atom code.
>>
>> Clearly, we're almost there. I did a few minor remarks. I suggest
>> (admitedly, Eric suggested of-list to suggest ;-) ) that you reply to
>> them by re-sending only individual patches that changed (replying to the
>> original patch) so that we can check the new patches individually. I
>> think we can do the finishing touches for each patch in a subthread of
>> this patch.
>>
>
> I replied with suggested changes by you and Junio.
> Let me know if any other changes to be made :)

I went through the patches you resent, it all looks good to me.

If I read correctly, Junio already applied it to pu.

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

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

[Karthik Nayak]

On Sat, Sep 12, 2015 at 2:44 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Thu, Sep 10, 2015 at 10:27 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> This is part of the series of unifying the code used by
>>>> "git tag -l, git branch -l, git for-each-ref".
>>>>
>>>> The previous version can be found here (version 16):
>>>> article.gmane.org/gmane.comp.version-control.git/277394
>>>>
>>>> Changes in this version:
>>>> * The arguments of the %(align) atom are interchangeable.
>>>> * Small grammatical changes.
>>>> * Small changes in the tests to reflect changes in the align
>>>> atom code.
>>>
>>> Clearly, we're almost there. I did a few minor remarks. I suggest
>>> (admitedly, Eric suggested of-list to suggest ;-) ) that you reply to
>>> them by re-sending only individual patches that changed (replying to the
>>> original patch) so that we can check the new patches individually. I
>>> think we can do the finishing touches for each patch in a subthread of
>>> this patch.
>>>
>>
>> I replied with suggested changes by you and Junio.
>> Let me know if any other changes to be made :)
>
> I went through the patches you resent, it all looks good to me.
>
> If I read correctly, Junio already applied it to pu.

I'll push the next set of patches then.

-- 
Regards,
Karthik Nayak

Re: [PATCH v17 14/14] tag.c: implement '--merged' and '--no-merged' options

[John Keeping]

On Thu, Sep 10, 2015 at 09:52:49PM +0530, Karthik Nayak wrote:
> 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 0c7f4e6..3803bf7 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
> @@ -165,6 +165,11 @@ This option is only applicable when listing tags without annotation lines.
>  	that of linkgit:git-for-each-ref[1].  When unspecified,
>  	defaults to `%(refname:short)`.
>  
> +--[no-]merged [<commit>]::

We prefer to write --[no-]* as:

	--option::
	--no-option::

although this may be the first instance that we see this combination
with an argument.

I also found the "[<commit>]" syntax confusing and had to go and figure
out what PARSE_OPT_LASTARG_DEFAULT does; I wonder if it's worth
appending something like the following to the help for this option:

	The `commit` may be omitted if this is the final argument.

> +	Only list tags whose tips are reachable, or not reachable
> +	if '--no-merged' is used, from the specified commit ('HEAD'
> +	if not specified).
> +
>  CONFIGURATION
>  -------------
>  By default, 'git tag' in sign-with-default mode (-s) will use your
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 075d90b..081fe84 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[--format=<format>] [<pattern>...]"),
> +		"\n\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"),
>  	N_("git tag -v <tagname>..."),
>  	NULL
>  };
> @@ -359,6 +359,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),
>  		{
> @@ -417,6 +419,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 8987fb1..3dd2f51 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1531,4 +1531,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.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v17 14/14] tag.c: implement '--merged' and '--no-merged' options

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

>> +--[no-]merged [<commit>]::
>
> We prefer to write --[no-]* as:
>
> 	--option::
> 	--no-option::
>
> although this may be the first instance that we see this combination
> with an argument.
>
> I also found the "[<commit>]" syntax confusing and had to go and figure
> out what PARSE_OPT_LASTARG_DEFAULT does; I wonder if it's worth
> appending something like the following to the help for this option:
>
> 	The `commit` may be omitted if this is the final argument.

"may be omitted" must be followed by a description of what happens
when omitted (i.e. "defaults to ...").

Re: [PATCH v17 14/14] tag.c: implement '--merged' and '--no-merged' options

[Matthieu Moy]

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

> John Keeping <john@keeping.me.uk> writes:
>
>>> +--[no-]merged [<commit>]::
>>
>> We prefer to write --[no-]* as:
>>
>> 	--option::
>> 	--no-option::
>>
>> although this may be the first instance that we see this combination
>> with an argument.
>>
>> I also found the "[<commit>]" syntax confusing and had to go and figure
>> out what PARSE_OPT_LASTARG_DEFAULT does; I wonder if it's worth
>> appending something like the following to the help for this option:
>>
>> 	The `commit` may be omitted if this is the final argument.
>
> "may be omitted" must be followed by a description of what happens
> when omitted (i.e. "defaults to ...").

Then:

The `commit` may be omitted and defaults to HEAD if this is the final
argument.

Sounds good to me.

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

Re: [PATCH v17 14/14] tag.c: implement '--merged' and '--no-merged' options

[John Keeping]

On Fri, Sep 18, 2015 at 09:10:08AM +0200, Matthieu Moy wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > John Keeping <john@keeping.me.uk> writes:
> >
> >>> +--[no-]merged [<commit>]::
> >>
> >> We prefer to write --[no-]* as:
> >>
> >> 	--option::
> >> 	--no-option::
> >>
> >> although this may be the first instance that we see this combination
> >> with an argument.
> >>
> >> I also found the "[<commit>]" syntax confusing and had to go and figure
> >> out what PARSE_OPT_LASTARG_DEFAULT does; I wonder if it's worth
> >> appending something like the following to the help for this option:
> >>
> >> 	The `commit` may be omitted if this is the final argument.
> >
> > "may be omitted" must be followed by a description of what happens
> > when omitted (i.e. "defaults to ...").
> 
> Then:
> 
> The `commit` may be omitted and defaults to HEAD if this is the final
> argument.

I find that slightly confusing, although that might just be me.  It's
slightly longer, but I would write:

	The `commit` may be omitted if this is the final argument, in
	which case it defaults to `HEAD`.

I also had a look at git-branch(1), which has similar `--merged` and
`--no-merged` options and says:

	Only list branches whose tips are reachable from the specified
	commit (HEAD if not specified).  Implies `--list`.

The two options are listed separately in that case.

Re: [PATCH v17 14/14] tag.c: implement '--merged' and '--no-merged' options

[Matthieu Moy]

John Keeping <john@keeping.me.uk> writes:

> On Fri, Sep 18, 2015 at 09:10:08AM +0200, Matthieu Moy wrote:
>
>> The `commit` may be omitted and defaults to HEAD if this is the final
>> argument.
>
> I find that slightly confusing, although that might just be me.  It's
> slightly longer, but I would write:
>
> 	The `commit` may be omitted if this is the final argument, in
> 	which case it defaults to `HEAD`.

Probably clearer indeed.

> I also had a look at git-branch(1), which has similar `--merged` and
> `--no-merged` options and says:
>
> 	Only list branches whose tips are reachable from the specified
> 	commit (HEAD if not specified).  Implies `--list`.

I think this lacks the "may be omitted if this is the final argument",
which is actually the most confusing (see the other thread "git status
-u is mildly astonishing" for an example of similar confusion).

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

Re: [PATCH v17 14/14] tag.c: implement '--merged' and '--no-merged' options

[Karthik Nayak]

On Fri, Sep 18, 2015 at 2:12 PM, John Keeping <john@keeping.me.uk> wrote:
> On Fri, Sep 18, 2015 at 09:10:08AM +0200, Matthieu Moy wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > John Keeping <john@keeping.me.uk> writes:
>> >
>> >>> +--[no-]merged [<commit>]::
>> >>
>> >> We prefer to write --[no-]* as:
>> >>
>> >>    --option::
>> >>    --no-option::
>> >>
>> >> although this may be the first instance that we see this combination
>> >> with an argument.
>> >>
>> >> I also found the "[<commit>]" syntax confusing and had to go and figure
>> >> out what PARSE_OPT_LASTARG_DEFAULT does; I wonder if it's worth
>> >> appending something like the following to the help for this option:
>> >>
>> >>    The `commit` may be omitted if this is the final argument.
>> >
>> > "may be omitted" must be followed by a description of what happens
>> > when omitted (i.e. "defaults to ...").
>>
>> Then:
>>
>> The `commit` may be omitted and defaults to HEAD if this is the final
>> argument.
>
> I find that slightly confusing, although that might just be me.  It's
> slightly longer, but I would write:
>
>         The `commit` may be omitted if this is the final argument, in
>         which case it defaults to `HEAD`.
>

This seems good to be included.

> I also had a look at git-branch(1), which has similar `--merged` and
> `--no-merged` options and says:
>
>         Only list branches whose tips are reachable from the specified
>         commit (HEAD if not specified).  Implies `--list`.
>
> The two options are listed separately in that case.

Not sure this is much of a problem with regards to "--[no-]merged"
I mean isn't the square brackets self-explanatory?

-- 
Regards,
Karthik Nayak

Re: [PATCH v17 14/14] tag.c: implement '--merged' and '--no-merged' options

[Matthieu Moy]

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

> Not sure this is much of a problem with regards to "--[no-]merged"
> I mean isn't the square brackets self-explanatory?

Well, usually --no-foo means "cancel the effect of --foo", ie. "git
command --foo --no-foo" is equivalent to "git command".

Here, --no-merged=some-ref does not _cancel_ the effect but introduce a
new behavior that was not the default. So it may make sense to explain
this more clearly.

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

Re: [PATCH v17 14/14] tag.c: implement '--merged' and '--no-merged' options

[Karthik Nayak]

On Fri, Sep 18, 2015 at 8:49 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Not sure this is much of a problem with regards to "--[no-]merged"
>> I mean isn't the square brackets self-explanatory?
>
> Well, usually --no-foo means "cancel the effect of --foo", ie. "git
> command --foo --no-foo" is equivalent to "git command".
>
> Here, --no-merged=some-ref does not _cancel_ the effect but introduce a
> new behavior that was not the default. So it may make sense to explain
> this more clearly.

maybe we should s/no/not ? makes not sense to me

-- 
Regards,
Karthik Nayak

Re: [PATCH v17 14/14] tag.c: implement '--merged' and '--no-merged' options

[Matthieu Moy]

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

> On Fri, Sep 18, 2015 at 8:49 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> Not sure this is much of a problem with regards to "--[no-]merged"
>>> I mean isn't the square brackets self-explanatory?
>>
>> Well, usually --no-foo means "cancel the effect of --foo", ie. "git
>> command --foo --no-foo" is equivalent to "git command".
>>
>> Here, --no-merged=some-ref does not _cancel_ the effect but introduce a
>> new behavior that was not the default. So it may make sense to explain
>> this more clearly.
>
> maybe we should s/no/not ? makes not sense to me

It's too late to rename the option without a transition plan (people's
finger and scripts use --no-merged already).

But that would be a good idea to introduce --not-merged as a synonym to
--no-merged, and deprecate --no-merged (it can remain a deprecated alias
forever if needed, but we should promote --not-merged in the docs).

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

Re: [PATCH v17 14/14] tag.c: implement '--merged' and '--no-merged' options

[Karthik Nayak]

On Fri, Sep 18, 2015 at 9:00 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Fri, Sep 18, 2015 at 8:49 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> Not sure this is much of a problem with regards to "--[no-]merged"
>>>> I mean isn't the square brackets self-explanatory?
>>>
>>> Well, usually --no-foo means "cancel the effect of --foo", ie. "git
>>> command --foo --no-foo" is equivalent to "git command".
>>>
>>> Here, --no-merged=some-ref does not _cancel_ the effect but introduce a
>>> new behavior that was not the default. So it may make sense to explain
>>> this more clearly.
>>
>> maybe we should s/no/not ? makes not sense to me
>
> It's too late to rename the option without a transition plan (people's
> finger and scripts use --no-merged already).
>
> But that would be a good idea to introduce --not-merged as a synonym to
> --no-merged, and deprecate --no-merged (it can remain a deprecated alias
> forever if needed, but we should promote --not-merged in the docs).
>

Either ways, I'll add it to my personal list of TODO.
Also having a look around, even the "--contains" option is
similarly documented:

--contains [<commit>]::
    Only list tags which contain the specified commit (HEAD if not
    specified).

About the issue at hand, we should probably squash this in

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 3803bf7..19ef640 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>] [--[no-]merged [<commit>]] [<pattern>...]
+       [--format=<format>] [(--merged | --no-merged) [<commit>]] [<pattern>...]
 'git tag' -v <tagname>...

 DESCRIPTION
@@ -165,10 +165,15 @@ This option is only applicable when listing tags
without annotation lines.
        that of linkgit:git-for-each-ref[1].  When unspecified,
        defaults to `%(refname:short)`.

---[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).
+--merged [<commit>]::
+       Only list tags whose tips are reachable from the specified
+       commit (The `commit` may be omitted if this is the final
+       argument, in which case it defaults to `HEAD`).
+
+--no-merged [<commit>]::
+       Only list tags whose tips are not reachable from the specified
+       commit (The `commit` may be omitted if this is the final
+       argument, in which case it defaults to `HEAD`).

 CONFIGURATION
 -------------

-- 
Regards,
Karthik Nayak

Re: [PATCH v17 14/14] tag.c: implement '--merged' and '--no-merged' options

[Matthieu Moy]

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

> About the issue at hand, we should probably squash this in

Looks good to me.

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