[PATCH v8 0/11] Port tag.c over to use ref-filter APIs

[PATCH v8 0/11] Port tag.c over to use ref-filter APIs

[Karthik Nayak]

This is part of my GSoC project to unify git tag -l, git branch -l,
git for-each-ref.  This patch series is continued from: Git (next)
https://github.com/git/git/commit/bf5418f49ff0cebc6e5ce04ad1417e1a47c81b61

This series consists of porting tag.c over to using the ref-filter APIs

Version 7 can be found here:
http://thread.gmane.org/gmane.comp.version-control.git/274990

Changes:
* Make padright a general align atom.
* Make print_value() and emit() output to a strbuf rather than stdout directly.

Interdiff:

diff --git a/Documentation/git-for-each-ref.txt
b/Documentation/git-for-each-ref.txt
index bcf319a..e89b9b0 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -127,11 +127,12 @@ color::
     Change output color.  Followed by `:<colorname>`, where names
     are described in `color.branch.*`.

-padright::
-    Pad succeeding atom or string to the right. Followed by
-    `:<value>`, where `value` states the total length of atom or
-    string including the padding. If the `value` is lesser than
-    the atom or string length, then no padding is performed.
+align::
+    Align succeeding atoms to the right, left or middle. Followed
+    by `:<type>,<paddinglength>`, where the `<type>` is either
+    left, right or middle and `<paddinglength>` is the total
+    length of the padding to be performed. If the atom length is
+    more than the padding length then no padding is performed.

 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 fc01117..529b29f 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -41,7 +41,7 @@ static int list_tags(struct ref_filter *filter,
struct ref_sorting *sorting, con
         filter->lines = 0;

     if (filter->lines)
-        format = "%(padright:16)%(refname:short)";
+        format = "%(align:left,16)%(refname:short)";
     else if (!format)
         format = "%(refname:short)";

diff --git a/ref-filter.c b/ref-filter.c
index 65d168e..afeab37 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -56,7 +56,7 @@ static struct {
     { "flag" },
     { "HEAD" },
     { "color" },
-    { "padright" },
+    { "align" },
 };

 /*
@@ -624,7 +624,7 @@ static void populate_value(struct ref_array_item *ref)
         const char *name = used_atom[i];
         struct atom_value *v = &ref->value[i];
         int deref = 0;
-        const char *refname;
+        const char *refname = NULL;
         const char *formatp;
         struct branch *branch = NULL;

@@ -664,8 +664,6 @@ static void populate_value(struct ref_array_item *ref)
             if (color_parse(name + 6, color) < 0)
                 die(_("unable to parse format"));
             v->s = xstrdup(color);
-            v->color = 1;
-            v->modifier_atom = 1;
             continue;
         } else if (!strcmp(name, "flag")) {
             char buf[256], *cp = buf;
@@ -693,17 +691,23 @@ static void populate_value(struct ref_array_item *ref)
             else
                 v->s = " ";
             continue;
-        } else if (starts_with(name, "padright:")) {
+        } else if (starts_with(name, "align:")) {
             const char *valp = NULL;
+            struct align *align = xmalloc(sizeof(struct align));

-            skip_prefix(name, "padright:", &valp);
-            if (!valp[0])
-                die(_("no value given with 'padright:'"));
-            if (strtoul_ui(valp, 10, (unsigned int *)&v->ul))
-                die(_("positive integer expected after ':' in padright:%u\n"),
-                    (unsigned int)v->ul);
-            v->modifier_atom = 1;
-            v->pad_to_right = 1;
+            skip_prefix(name, "align:", &valp);
+
+            if (skip_prefix(valp, "left,", &valp))
+                align->align_type = ALIGN_LEFT;
+            else if (skip_prefix(valp, "right,", &valp))
+                align->align_type = ALIGN_RIGHT;
+            else if (skip_prefix(valp, "middle,", &valp))
+                align->align_type = ALIGN_MIDDLE;
+            else
+                die(_("align: improper format"));
+            if (strtoul_ui(valp, 10, &align->align_value))
+                die(_("align: positive value expected"));
+            v->align = align;
             continue;
         } else
             continue;
@@ -1243,55 +1247,26 @@ 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 apply_formatting_state(struct ref_formatting_state *state,
-                   const char *buf, struct strbuf *value)
-{
-    if (state->color) {
-        strbuf_addstr(value, state->color);
-        state->color = NULL;
-    }
-    if (state->pad_to_right) {
-        if (!is_utf8(buf))
-            strbuf_addf(value, "%-*s", state->pad_to_right, buf);
-        else {
-            int utf8_compensation = strlen(buf) - utf8_strwidth(buf);
-            strbuf_addf(value, "%-*s", state->pad_to_right +
utf8_compensation, buf);
-        }
-        state->pad_to_right = 0;
-        return;
-    }
-
-    strbuf_addstr(value, buf);
-}
-
-static void print_value(struct atom_value *v, struct
ref_formatting_state *state)
+static void print_value(struct atom_value *v, struct
ref_formatting_state *state,
+            struct strbuf *output)
 {
-    struct strbuf value = STRBUF_INIT;
-    struct strbuf formatted = STRBUF_INIT;
-
-    apply_formatting_state(state, v->s, &value);
-
     switch (state->quote_style) {
     case QUOTE_NONE:
-        fputs(value.buf, stdout);
+        strbuf_addstr(output, v->s);
         break;
     case QUOTE_SHELL:
-        sq_quote_buf(&formatted, value.buf);
+        sq_quote_buf(output, v->s);
         break;
     case QUOTE_PERL:
-        perl_quote_buf(&formatted, value.buf);
+        perl_quote_buf(output, v->s);
         break;
     case QUOTE_PYTHON:
-        python_quote_buf(&formatted, value.buf);
+        python_quote_buf(output, v->s);
         break;
     case QUOTE_TCL:
-        tcl_quote_buf(&formatted, value.buf);
+        tcl_quote_buf(output, v->s);
         break;
     }
-    if (state->quote_style != QUOTE_NONE)
-        fputs(formatted.buf, stdout);
-    strbuf_release(&value);
-    strbuf_release(&formatted);
 }

 static int hex1(char ch)
@@ -1312,12 +1287,8 @@ static int hex2(const char *cp)
         return -1;
 }

-static void emit(const char *cp, const char *ep,
-         struct ref_formatting_state *state)
+static void emit(const char *cp, const char *ep, struct strbuf *output)
 {
-    struct strbuf value = STRBUF_INIT;
-    struct strbuf format = STRBUF_INIT;
-
     while (*cp && (!ep || cp < ep)) {
         if (*cp == '%') {
             if (cp[1] == '%')
@@ -1325,28 +1296,61 @@ static void emit(const char *cp, const char *ep,
             else {
                 int ch = hex2(cp + 1);
                 if (0 <= ch) {
-                    putchar(ch);
+                    strbuf_addch(output, ch);
                     cp += 3;
                     continue;
                 }
             }
         }
-        strbuf_addch(&value, *cp);
+        strbuf_addch(output, *cp);
         cp++;
     }
-    apply_formatting_state(state, value.buf, &format);
-    fputs(format.buf, stdout);
-    strbuf_release(&format);
-    strbuf_release(&value);
 }

-static void store_formatting_state(struct ref_formatting_state *state,
-                   struct atom_value *atomv)
+static void process_formatting_state(struct atom_value *atomv, struct
ref_formatting_state *state)
 {
-    if (atomv->color)
-        state->color = atomv->s;
-    if (atomv->pad_to_right)
-        state->pad_to_right = atomv->ul;
+    if (atomv->align) {
+        state->align = atomv->align;
+        atomv->align = NULL;
+    }
+}
+
+static void apply_formatting_state(struct ref_formatting_state
*state, struct strbuf *value,
+                   struct strbuf *format)
+{
+    if (state->align) {
+        int len = 0, buf_len = value->len;
+        struct align *align = state->align;
+
+        if (!value->buf)
+            return;
+        if (!is_utf8(value->buf)) {
+            len = value->len - utf8_strwidth(value->buf);
+            buf_len -= len;
+        }
+
+        if (align->align_value < buf_len) {
+            state->align = NULL;
+            strbuf_addbuf(format, value);
+            strbuf_release(value);
+            return;
+        }
+
+        if (align->align_type == ALIGN_LEFT)
+            strbuf_addf(format, "%-*s", len + align->align_value, value->buf);
+        else if (align->align_type == ALIGN_MIDDLE) {
+            int right = (align->align_value - buf_len)/2;
+            strbuf_addf(format, "%*s%-*s", align->align_value - right + len,
+                    value->buf, right, "");
+        } else if (align->align_type == ALIGN_RIGHT)
+            strbuf_addf(format, "%*s", align->align_value, value->buf);
+        strbuf_release(value);
+        state->align = NULL;
+        return;
+    }
+    strbuf_addbuf(format, value);
+    strbuf_release(value);
+
 }

 /*
@@ -1396,26 +1400,34 @@ void show_ref_array_item(struct ref_array_item
*info, const char *format,
              int quote_style, unsigned int lines)
 {
     const char *cp, *sp, *ep;
+    struct strbuf value = STRBUF_INIT;
+    struct strbuf final_buf = STRBUF_INIT;
     struct ref_formatting_state state;
+    int i;

     memset(&state, 0, sizeof(state));
     state.quote_style = quote_style;

     for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
-        struct atom_value *atomv;
+        struct atom_value *atomv = NULL;

         ep = strchr(sp, ')');
-        if (cp < sp)
-            emit(cp, sp, &state);
+        if (cp < sp) {
+            emit(cp, sp, &value);
+            apply_formatting_state(&state, &value, &final_buf);
+        }
         get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
-        if (atomv->modifier_atom)
-            store_formatting_state(&state, atomv);
-        else
-            print_value(atomv, &state);
+        if (atomv->align)
+            process_formatting_state(atomv, &state);
+        else {
+            print_value(atomv, &state, &value);
+            apply_formatting_state(&state, &value, &final_buf);
+        }
     }
     if (*cp) {
         sp = cp + strlen(cp);
-        emit(cp, sp, &state);
+        emit(cp, sp, &value);
+        apply_formatting_state(&state, &value, &final_buf);
     }
     if (need_color_reset_at_eol) {
         struct atom_value resetv;
@@ -1424,8 +1436,14 @@ void show_ref_array_item(struct ref_array_item
*info, const char *format,
         if (color_parse("reset", color) < 0)
             die("BUG: couldn't parse 'reset' as a color");
         resetv.s = color;
-        print_value(&resetv, &state);
+        print_value(&resetv, &state, &value);
+        apply_formatting_state(&state, &value, &final_buf);
     }
+
+    for (i = 0; i < final_buf.len; i++)
+        printf("%c", final_buf.buf[i]);
+    strbuf_release(&final_buf);
+
     if (lines > 0) {
         struct object_id oid;
         hashcpy(oid.hash, info->objectname);
diff --git a/ref-filter.h b/ref-filter.h
index b50a036..b3b9cd8 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -17,18 +17,25 @@
 #define FILTER_REFS_ALL 0x2
 #define FILTER_REFS_TAGS 0x4

-struct atom_value {
-    const char *s;
-    unsigned long ul; /* used for sorting when not FIELD_STR */
-    unsigned int modifier_atom : 1, /*  atoms which act as modifiers
for the next atom */
-        color : 1,
-        pad_to_right : 1;
-};
+#define ALIGN_LEFT 0x01
+#define ALIGN_RIGHT 0x02
+#define ALIGN_MIDDLE 0x04

 struct ref_formatting_state {
     int quote_style;
-    unsigned int pad_to_right;
-    const char *color;
+    struct align *align;
+    struct strbuf *sb;
+};
+
+struct align {
+    unsigned int align_type,
+        align_value;
+};
+
+struct atom_value {
+    const char *s;
+    struct align *align;
+    unsigned long ul; /* used for sorting when not FIELD_STR */
 };

 struct ref_sorting {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 842efde..7332bea 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -81,35 +81,83 @@ test_expect_success 'filtering with --contains' '
     test_cmp expect actual
 '

-test_expect_success 'padding to the right using `padright`' '
+test_expect_success 'left alignment' '
     cat >expect <<-\EOF &&
-    master    ||    master
-    side      ||    side
-    odd/spot  ||    odd/spot
-    double-tag||    double-tag
-    four      ||    four
-    one       ||    one
-    signed-tag||    signed-tag
-    three     ||    three
-    two       ||    two
+    refs/heads/master   |refs/heads/master
+    refs/heads/side     |refs/heads/side
+    refs/odd/spot       |refs/odd/spot
+    refs/tags/double-tag|refs/tags/double-tag
+    refs/tags/four      |refs/tags/four
+    refs/tags/one       |refs/tags/one
+    refs/tags/signed-tag|refs/tags/signed-tag
+    refs/tags/three     |refs/tags/three
+    refs/tags/two       |refs/tags/two
     EOF
-    git for-each-ref
--format="%(padright:10)%(refname:short)|%(padright:5)|%(refname:short)"
>actual &&
+    git for-each-ref --format="%(align:left,20)%(refname)|%(refname)"
>actual &&
     test_cmp expect actual
 '

-test_expect_success 'no padding when `padright` length is smaller
than atom length' '
+test_expect_success 'middle alignment' '
     cat >expect <<-\EOF &&
-    refs/heads/master|
-    refs/heads/side|
-    refs/odd/spot|
-    refs/tags/double-tag|
-    refs/tags/four|
-    refs/tags/one|
-    refs/tags/signed-tag|
-    refs/tags/three|
-    refs/tags/two|
+    |  refs/heads/master |refs/heads/master
+    |   refs/heads/side  |refs/heads/side
+    |    refs/odd/spot   |refs/odd/spot
+    |refs/tags/double-tag|refs/tags/double-tag
+    |   refs/tags/four   |refs/tags/four
+    |    refs/tags/one   |refs/tags/one
+    |refs/tags/signed-tag|refs/tags/signed-tag
+    |   refs/tags/three  |refs/tags/three
+    |    refs/tags/two   |refs/tags/two
     EOF
-    git for-each-ref --format="%(padright:5)%(refname)|" >actual &&
+    git for-each-ref
--format="|%(align:middle,20)%(refname)|%(refname)" >actual &&
+    test_cmp expect actual
+'
+
+test_expect_success 'right alignment' '
+    cat >expect <<-\EOF &&
+    |   refs/heads/master|refs/heads/master
+    |     refs/heads/side|refs/heads/side
+    |       refs/odd/spot|refs/odd/spot
+    |refs/tags/double-tag|refs/tags/double-tag
+    |      refs/tags/four|refs/tags/four
+    |       refs/tags/one|refs/tags/one
+    |refs/tags/signed-tag|refs/tags/signed-tag
+    |     refs/tags/three|refs/tags/three
+    |       refs/tags/two|refs/tags/two
+    EOF
+    git for-each-ref
--format="|%(align:right,20)%(refname)|%(refname)" >actual &&
+    test_cmp expect actual
+'
+
+test_expect_success 'alignment value lesser than atom value' '
+    cat >expect <<-\EOF &&
+    |refs/heads/master|
+    |refs/heads/side|
+    |  refs/odd/spot|
+    |refs/tags/double-tag|
+    | refs/tags/four|
+    |  refs/tags/one|
+    |refs/tags/signed-tag|
+    |refs/tags/three|
+    |  refs/tags/two|
+    EOF
+    git for-each-ref --format="|%(align:right,15)%(refname)|" >actual &&
+    test_cmp expect actual
+'
+
+test_expect_success 'non atom alignment' '
+    cat >expect <<-\EOF &&
+    |    |master  |  refs/heads/master|  refs/heads/master
+    |    |side  |  refs/heads/side|  refs/heads/side
+    |    |odd/spot  |  refs/odd/spot|  refs/odd/spot
+    |    |double-tag  |  refs/tags/double-tag|  refs/tags/double-tag
+    |    |four  |  refs/tags/four|  refs/tags/four
+    |    |one  |  refs/tags/one|  refs/tags/one
+    |    |signed-tag  |  refs/tags/signed-tag|  refs/tags/signed-tag
+    |    |three  |  refs/tags/three|  refs/tags/three
+    |    |two  |  refs/tags/two|  refs/tags/two
+    EOF
+    git for-each-ref
--format="|%(align:right,5)|%(refname:short)%(align:middle,5)|%(refname)%(align:left,3)|%(refname)"
>actual &&
     test_cmp expect actual
 '

-- 
Regards,
Karthik Nayak

[PATCH v8 01/11] ref-filter: print output to strbuf for formatting

[Karthik Nayak]

Introduce a strbuf `output` which will act as a substitute rather than
printing directly to stdout. This will be used for formatting
eventually.
---
 ref-filter.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 7561727..febdc45 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1190,30 +1190,25 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
 	qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);
 }
 
-static void print_value(struct atom_value *v, int quote_style)
+static void print_value(struct atom_value *v, int quote_style, struct strbuf *output)
 {
-	struct strbuf sb = STRBUF_INIT;
 	switch (quote_style) {
 	case QUOTE_NONE:
-		fputs(v->s, stdout);
+		strbuf_addstr(output, v->s);
 		break;
 	case QUOTE_SHELL:
-		sq_quote_buf(&sb, v->s);
+		sq_quote_buf(output, v->s);
 		break;
 	case QUOTE_PERL:
-		perl_quote_buf(&sb, v->s);
+		perl_quote_buf(output, v->s);
 		break;
 	case QUOTE_PYTHON:
-		python_quote_buf(&sb, v->s);
+		python_quote_buf(output, v->s);
 		break;
 	case QUOTE_TCL:
-		tcl_quote_buf(&sb, v->s);
+		tcl_quote_buf(output, v->s);
 		break;
 	}
-	if (quote_style != QUOTE_NONE) {
-		fputs(sb.buf, stdout);
-		strbuf_release(&sb);
-	}
 }
 
 static int hex1(char ch)
@@ -1234,7 +1229,7 @@ static int hex2(const char *cp)
 		return -1;
 }
 
-static void emit(const char *cp, const char *ep)
+static void emit(const char *cp, const char *ep, struct strbuf *output)
 {
 	while (*cp && (!ep || cp < ep)) {
 		if (*cp == '%') {
@@ -1243,13 +1238,13 @@ static void emit(const char *cp, const char *ep)
 			else {
 				int ch = hex2(cp + 1);
 				if (0 <= ch) {
-					putchar(ch);
+					strbuf_addch(output, ch);
 					cp += 3;
 					continue;
 				}
 			}
 		}
-		putchar(*cp);
+		strbuf_addch(output, *cp);
 		cp++;
 	}
 }
@@ -1257,19 +1252,21 @@ 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 output = STRBUF_INIT;
+	int i;
 
 	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
 		struct atom_value *atomv;
 
 		ep = strchr(sp, ')');
 		if (cp < sp)
-			emit(cp, sp);
+			emit(cp, sp, &output);
 		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
-		print_value(atomv, quote_style);
+		print_value(atomv, quote_style, &output);
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);
-		emit(cp, sp);
+		emit(cp, sp, &output);
 	}
 	if (need_color_reset_at_eol) {
 		struct atom_value resetv;
@@ -1278,9 +1275,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 		if (color_parse("reset", color) < 0)
 			die("BUG: couldn't parse 'reset' as a color");
 		resetv.s = color;
-		print_value(&resetv, quote_style);
+		print_value(&resetv, quote_style, &output);
 	}
+	for (i = 0; i < output.len; i++)
+		printf("%c", output.buf[i]);
 	putchar('\n');
+	strbuf_release(&output);
 }
 
 /*  If no sorting option is given, use refname to sort as default */
-- 
2.4.6

[PATCH v8 02/11] ref-filter: introduce ref_formatting_state

[Karthik Nayak]

Introduce a ref_formatting_state which will eventually hold the values
of modifier atoms. Implement this within ref-filter.

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 | 49 +++++++++++++++++++++++++++++++++++++------------
 ref-filter.h |  4 ++++
 2 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index febdc45..c4c7064 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1190,9 +1190,10 @@ 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, struct strbuf *output)
+static void print_value(struct atom_value *v, struct ref_formatting_state *state,
+			struct strbuf *output)
 {
-	switch (quote_style) {
+	switch (state->quote_style) {
 	case QUOTE_NONE:
 		strbuf_addstr(output, v->s);
 		break;
@@ -1249,24 +1250,47 @@ static void emit(const char *cp, const char *ep, struct strbuf *output)
 	}
 }
 
+static void process_formatting_state(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+	/* Based on the atomv values, the formatting state is set */
+}
+
+static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *value,
+				   struct strbuf *format)
+{
+	/* More formatting options to be evetually added */
+	strbuf_addbuf(format, value);
+	strbuf_release(value);
+}
+
 void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
 {
 	const char *cp, *sp, *ep;
-	struct strbuf output = STRBUF_INIT;
+	struct strbuf value = STRBUF_INIT;
+	struct strbuf final_buf = STRBUF_INIT;
+	struct ref_formatting_state state;
 	int i;
 
+	memset(&state, 0, sizeof(state));
+	state.quote_style = quote_style;
+
 	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
-		struct atom_value *atomv;
+		struct atom_value *atomv = NULL;
 
 		ep = strchr(sp, ')');
-		if (cp < sp)
-			emit(cp, sp, &output);
+		if (cp < sp) {
+			emit(cp, sp, &value);
+			apply_formatting_state(&state, &value, &final_buf);
+		}
 		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
-		print_value(atomv, quote_style, &output);
+		process_formatting_state(atomv, &state);
+		print_value(atomv, &state, &value);
+		apply_formatting_state(&state, &value, &final_buf);
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);
-		emit(cp, sp, &output);
+		emit(cp, sp, &value);
+		apply_formatting_state(&state, &value, &final_buf);
 	}
 	if (need_color_reset_at_eol) {
 		struct atom_value resetv;
@@ -1275,12 +1299,13 @@ 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, &output);
+		print_value(&resetv, &state, &value);
+		apply_formatting_state(&state, &value, &final_buf);
 	}
-	for (i = 0; i < output.len; i++)
-		printf("%c", output.buf[i]);
+	for (i = 0; i < final_buf.len; i++)
+		printf("%c", final_buf.buf[i]);
 	putchar('\n');
-	strbuf_release(&output);
+	strbuf_release(&final_buf);
 }
 
 /*  If no sorting option is given, use refname to sort as default */
diff --git a/ref-filter.h b/ref-filter.h
index 6bf27d8..b64677f 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -16,6 +16,10 @@
 #define FILTER_REFS_INCLUDE_BROKEN 0x1
 #define FILTER_REFS_ALL 0x2
 
+struct ref_formatting_state {
+	int quote_style;
+};
+
 struct atom_value {
 	const char *s;
 	unsigned long ul; /* used for sorting when not FIELD_STR */
-- 
2.4.6

[PATCH v8 03/11] ref-filter: implement an `align` atom

[Karthik Nayak]

Implement an `align` atom which will act as a modifier atom and align
succeeding atoms to the right, left or middle. It is followed by
`:<type>,<paddinglength>`, where the `<type>` is either left, right or
middle and `<paddinglength>` is the total length of the padding to be
performed. If the atom length is more than the padding length then no
padding is performed. e.g. to pad a succeeding atom to the middle with
a total padding size of 40 we can do a --format="%(align:middle,40).."

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 |  7 ++++
 ref-filter.c                       | 69 +++++++++++++++++++++++++++++---
 ref-filter.h                       | 12 ++++++
 t/t6302-for-each-ref-filter.sh     | 80 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 162 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index e49d578..bba6d83 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -127,6 +127,13 @@ color::
 	Change output color.  Followed by `:<colorname>`, where names
 	are described in `color.branch.*`.
 
+align::
+	Align succeeding atoms to the right, left or middle. Followed
+	by `:<type>,<paddinglength>`, where the `<type>` is either
+	left, right or middle and `<paddinglength>` is the total
+	length of the padding to be performed. If the atom length is
+	more than the padding length then no padding is performed.
+
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
 be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index c4c7064..46d8834 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,6 +54,7 @@ static struct {
 	{ "flag" },
 	{ "HEAD" },
 	{ "color" },
+	{ "align" },
 };
 
 /*
@@ -620,7 +622,7 @@ static void populate_value(struct ref_array_item *ref)
 		const char *name = used_atom[i];
 		struct atom_value *v = &ref->value[i];
 		int deref = 0;
-		const char *refname;
+		const char *refname = NULL;
 		const char *formatp;
 		struct branch *branch = NULL;
 
@@ -687,6 +689,24 @@ static void populate_value(struct ref_array_item *ref)
 			else
 				v->s = " ";
 			continue;
+		} else if (starts_with(name, "align:")) {
+			const char *valp = NULL;
+			struct align *align = xmalloc(sizeof(struct align));
+
+			skip_prefix(name, "align:", &valp);
+
+			if (skip_prefix(valp, "left,", &valp))
+				align->align_type = ALIGN_LEFT;
+			else if (skip_prefix(valp, "right,", &valp))
+				align->align_type = ALIGN_RIGHT;
+			else if (skip_prefix(valp, "middle,", &valp))
+				align->align_type = ALIGN_MIDDLE;
+			else
+				die(_("align: improper format"));
+			if (strtoul_ui(valp, 10, &align->align_value))
+				die(_("align: positive value expected"));
+			v->align = align;
+			continue;
 		} else
 			continue;
 
@@ -1252,15 +1272,48 @@ static void emit(const char *cp, const char *ep, struct strbuf *output)
 
 static void process_formatting_state(struct atom_value *atomv, struct ref_formatting_state *state)
 {
-	/* Based on the atomv values, the formatting state is set */
+	if (atomv->align) {
+		state->align = atomv->align;
+		atomv->align = NULL;
+	}
 }
 
 static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *value,
 				   struct strbuf *format)
 {
-	/* More formatting options to be evetually added */
+	if (state->align) {
+		int len = 0, buf_len = value->len;
+		struct align *align = state->align;
+
+		if (!value->buf)
+			return;
+		if (!is_utf8(value->buf)) {
+			len = value->len - utf8_strwidth(value->buf);
+			buf_len -= len;
+		}
+
+		if (align->align_value < buf_len) {
+			state->align = NULL;
+			strbuf_addbuf(format, value);
+			strbuf_release(value);
+			return;
+		}
+
+		if (align->align_type == ALIGN_LEFT)
+			strbuf_addf(format, "%-*s", len + align->align_value, value->buf);
+		else if (align->align_type == ALIGN_MIDDLE) {
+			int right = (align->align_value - buf_len)/2;
+			strbuf_addf(format, "%*s%-*s", align->align_value - right + len,
+				    value->buf, right, "");
+		} else if (align->align_type == ALIGN_RIGHT)
+			strbuf_addf(format, "%*s", align->align_value, value->buf);
+		strbuf_release(value);
+		state->align = NULL;
+		return;
+	}
 	strbuf_addbuf(format, value);
 	strbuf_release(value);
+
 }
 
 void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
@@ -1283,9 +1336,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 			apply_formatting_state(&state, &value, &final_buf);
 		}
 		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
-		process_formatting_state(atomv, &state);
-		print_value(atomv, &state, &value);
-		apply_formatting_state(&state, &value, &final_buf);
+		if (atomv->align)
+			process_formatting_state(atomv, &state);
+		else {
+			print_value(atomv, &state, &value);
+			apply_formatting_state(&state, &value, &final_buf);
+		}
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);
@@ -1302,6 +1358,7 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 		print_value(&resetv, &state, &value);
 		apply_formatting_state(&state, &value, &final_buf);
 	}
+
 	for (i = 0; i < final_buf.len; i++)
 		printf("%c", final_buf.buf[i]);
 	putchar('\n');
diff --git a/ref-filter.h b/ref-filter.h
index b64677f..01f8cb3 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -16,12 +16,24 @@
 #define FILTER_REFS_INCLUDE_BROKEN 0x1
 #define FILTER_REFS_ALL 0x2
 
+#define ALIGN_LEFT 0x01
+#define ALIGN_RIGHT 0x02
+#define ALIGN_MIDDLE 0x04
+
 struct ref_formatting_state {
 	int quote_style;
+	struct align *align;
+	struct strbuf *sb;
+};
+
+struct align {
+	unsigned int align_type,
+		align_value;
 };
 
 struct atom_value {
 	const char *s;
+	struct align *align;
 	unsigned long ul; /* used for sorting when not FIELD_STR */
 };
 
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 505a360..87225dd 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -81,4 +81,84 @@ test_expect_success 'filtering with --contains' '
 	test_cmp expect actual
 '
 
+test_expect_success 'left alignment' '
+	cat >expect <<-\EOF &&
+	refs/heads/master   |refs/heads/master
+	refs/heads/side     |refs/heads/side
+	refs/odd/spot       |refs/odd/spot
+	refs/tags/double-tag|refs/tags/double-tag
+	refs/tags/four      |refs/tags/four
+	refs/tags/one       |refs/tags/one
+	refs/tags/signed-tag|refs/tags/signed-tag
+	refs/tags/three     |refs/tags/three
+	refs/tags/two       |refs/tags/two
+	EOF
+	git for-each-ref --format="%(align:left,20)%(refname)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'middle alignment' '
+	cat >expect <<-\EOF &&
+	|  refs/heads/master |refs/heads/master
+	|   refs/heads/side  |refs/heads/side
+	|    refs/odd/spot   |refs/odd/spot
+	|refs/tags/double-tag|refs/tags/double-tag
+	|   refs/tags/four   |refs/tags/four
+	|    refs/tags/one   |refs/tags/one
+	|refs/tags/signed-tag|refs/tags/signed-tag
+	|   refs/tags/three  |refs/tags/three
+	|    refs/tags/two   |refs/tags/two
+	EOF
+	git for-each-ref --format="|%(align:middle,20)%(refname)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'right alignment' '
+	cat >expect <<-\EOF &&
+	|   refs/heads/master|refs/heads/master
+	|     refs/heads/side|refs/heads/side
+	|       refs/odd/spot|refs/odd/spot
+	|refs/tags/double-tag|refs/tags/double-tag
+	|      refs/tags/four|refs/tags/four
+	|       refs/tags/one|refs/tags/one
+	|refs/tags/signed-tag|refs/tags/signed-tag
+	|     refs/tags/three|refs/tags/three
+	|       refs/tags/two|refs/tags/two
+	EOF
+	git for-each-ref --format="|%(align:right,20)%(refname)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment value lesser than atom value' '
+	cat >expect <<-\EOF &&
+	|refs/heads/master|
+	|refs/heads/side|
+	|  refs/odd/spot|
+	|refs/tags/double-tag|
+	| refs/tags/four|
+	|  refs/tags/one|
+	|refs/tags/signed-tag|
+	|refs/tags/three|
+	|  refs/tags/two|
+	EOF
+	git for-each-ref --format="|%(align:right,15)%(refname)|" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'non atom alignment' '
+	cat >expect <<-\EOF &&
+	|    |master  |  refs/heads/master|  refs/heads/master
+	|    |side  |  refs/heads/side|  refs/heads/side
+	|    |odd/spot  |  refs/odd/spot|  refs/odd/spot
+	|    |double-tag  |  refs/tags/double-tag|  refs/tags/double-tag
+	|    |four  |  refs/tags/four|  refs/tags/four
+	|    |one  |  refs/tags/one|  refs/tags/one
+	|    |signed-tag  |  refs/tags/signed-tag|  refs/tags/signed-tag
+	|    |three  |  refs/tags/three|  refs/tags/three
+	|    |two  |  refs/tags/two|  refs/tags/two
+	EOF
+	git for-each-ref --format="|%(align:right,5)|%(refname:short)%(align:middle,5)|%(refname)%(align:left,3)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.4.6

[PATCH v8 04/11] ref-filter: add option to filter only tags

[Karthik Nayak]

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

Add a functions called 'for_each_tag_ref_fullpath()' to refs.{c,h}
which iterates through each tag ref without trimming the path.

Add an option in 'filter_refs()' to use 'for_each_tag_ref_fullpath()'
and filter refs. This type checking is done by adding a
'FILTER_REFS_TAGS' in 'ref-filter.h'

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 | 2 ++
 ref-filter.h | 1 +
 refs.c       | 5 +++++
 refs.h       | 1 +
 4 files changed, 9 insertions(+)

diff --git a/ref-filter.c b/ref-filter.c
index 46d8834..01c9097 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1155,6 +1155,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 		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 & FILTER_REFS_TAGS)
+		ret = for_each_tag_ref_fullpath(ref_filter_handler, &ref_cbdata);
 	else if (type)
 		die("filter_refs: invalid type");
 
diff --git a/ref-filter.h b/ref-filter.h
index 01f8cb3..8d4e348 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -15,6 +15,7 @@
 
 #define FILTER_REFS_INCLUDE_BROKEN 0x1
 #define FILTER_REFS_ALL 0x2
+#define FILTER_REFS_TAGS 0x4
 
 #define ALIGN_LEFT 0x01
 #define ALIGN_RIGHT 0x02
diff --git a/refs.c b/refs.c
index 0b96ece..23ce483 100644
--- a/refs.c
+++ b/refs.c
@@ -2108,6 +2108,11 @@ int for_each_tag_ref(each_ref_fn fn, void *cb_data)
 	return for_each_ref_in("refs/tags/", fn, cb_data);
 }
 
+int for_each_tag_ref_fullpath(each_ref_fn fn, void *cb_data)
+{
+	return do_for_each_ref(&ref_cache, "refs/tags/", fn, 0, 0, cb_data);
+}
+
 int for_each_tag_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
 {
 	return for_each_ref_in_submodule(submodule, "refs/tags/", fn, cb_data);
diff --git a/refs.h b/refs.h
index e4e46c3..9eee2de 100644
--- a/refs.h
+++ b/refs.h
@@ -174,6 +174,7 @@ 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_tag_ref(each_ref_fn fn, void *cb_data);
+extern int for_each_tag_ref_fullpath(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);
 extern int for_each_replace_ref(each_ref_fn fn, void *cb_data);
-- 
2.4.6

[PATCH v8 05/11] ref-filter: support printing N lines from tag annotation

[Karthik Nayak]

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

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

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

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

[PATCH v8 06/11] 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 eventaully 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 bba6d83..e89b9b0 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -152,6 +152,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 9f3806a..1bc6d4b 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;
 
@@ -1175,19 +1177,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;
 }
 
@@ -1446,6 +1448,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 16cffab..c41432f 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -41,7 +41,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 87225dd..7332bea 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -161,4 +161,40 @@ test_expect_success 'non atom alignment' '
 	test_cmp expect actual
 '
 
+test_expect_success 'setup for version sort' '
+	test_commit foo1.3 &&
+	test_commit foo1.6 &&
+	test_commit foo1.10
+'
+
+test_expect_success 'version sort' '
+	git for-each-ref --sort=version:refname --format="%(refname:short)" refs/tags/ | grep "foo" >actual &&
+	cat >expect <<-\EOF &&
+	foo1.3
+	foo1.6
+	foo1.10
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'version sort (shortened)' '
+	git for-each-ref --sort=v:refname --format="%(refname:short)" refs/tags/ | grep "foo" >actual &&
+	cat >expect <<-\EOF &&
+	foo1.3
+	foo1.6
+	foo1.10
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'reverse version sort' '
+	git for-each-ref --sort=-version:refname --format="%(refname:short)" refs/tags/ | grep "foo" >actual &&
+	cat >expect <<-\EOF &&
+	foo1.10
+	foo1.6
+	foo1.3
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.4.6

[PATCH v8 07/11] 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           | 39 ++++++++++++++++++++++++++++++++++++---
 ref-filter.h           |  3 ++-
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index e4a4f8a..3ad6a64 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -68,6 +68,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 
 	filter.name_patterns = argv;
+	filter.match_as_path = 1;
 	filter_refs(&array, &filter, FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN);
 	ref_array_sort(sorting, &array);
 
diff --git a/ref-filter.c b/ref-filter.c
index 1bc6d4b..afeab37 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -950,9 +950,32 @@ 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));
+
+	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)
 {
@@ -973,6 +996,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
@@ -1041,7 +1074,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 		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 c41432f..b3b9cd8 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -71,7 +71,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 lines;
 };
 
-- 
2.4.6

[PATCH v8 08/11] tag.c: use 'ref-filter' data structures

[Karthik Nayak]

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

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

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

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

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

[PATCH v8 09/11] tag.c: use 'ref-filter' APIs

[Karthik Nayak]

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

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

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

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

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

Modify documentation for the same.

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

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

[PATCH v8 10/11] tag.c: implement '--format' option

[Karthik Nayak]

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

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

Add tests and documentation for the same.

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

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

[PATCH v8 11/11] tag.c: implement '--merged' and '--no-merged' options

[Karthik Nayak]

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

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

Add documentation and tests for the same.

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

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

Re: [PATCH v8 0/11] Port tag.c over to use ref-filter APIs

[Junio C Hamano]

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

> This is part of my GSoC project to unify git tag -l, git branch -l,
> git for-each-ref.  This patch series is continued from: Git (next)
> https://github.com/git/git/commit/bf5418f49ff0cebc6e5ce04ad1417e1a47c81b61
>
> This series consists of porting tag.c over to using the ref-filter APIs
>
> Version 7 can be found here:
> http://thread.gmane.org/gmane.comp.version-control.git/274990
>
> Changes:
> * Make padright a general align atom.
> * Make print_value() and emit() output to a strbuf rather than stdout directly.
>
> Interdiff:
>
> diff --git a/Documentation/git-for-each-ref.txt
> b/Documentation/git-for-each-ref.txt
> index bcf319a..e89b9b0 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -127,11 +127,12 @@ color::
>      Change output color.  Followed by `:<colorname>`, where names
>      are described in `color.branch.*`.
>
> -padright::
> -    Pad succeeding atom or string to the right. Followed by
> -    `:<value>`, where `value` states the total length of atom or
> -    string including the padding. If the `value` is lesser than
> -    the atom or string length, then no padding is performed.
> +align::
> +    Align succeeding atoms to the right, left or middle. Followed
> +    by `:<type>,<paddinglength>`, where the `<type>` is either
> +    left, right or middle and `<paddinglength>` is the total
> +    length of the padding to be performed. If the atom length is
> +    more than the padding length then no padding is performed.

It is very very dissapointing to allow the "next atom only"
implementation to squat on a good name "align:<type>,<width>",
especially when I thought that the list agreed

  %(align:<type>,<width>) any string with or without %(atom) %(end)

would be the way to go.

Re: [PATCH v8 01/11] ref-filter: print output to strbuf for formatting

[Junio C Hamano]

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

> Introduce a strbuf `output` which will act as a substitute rather than
> printing directly to stdout. This will be used for formatting
> eventually.
> ---

Missing sign-off; the patch looks like a good first step in a nice
direction.

>  ref-filter.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 7561727..febdc45 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1190,30 +1190,25 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
>  	qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);
>  }
>  
> -static void print_value(struct atom_value *v, int quote_style)
> +static void print_value(struct atom_value *v, int quote_style, struct strbuf *output)
>  {
> -	struct strbuf sb = STRBUF_INIT;
>  	switch (quote_style) {
>  	case QUOTE_NONE:
> -		fputs(v->s, stdout);
> +		strbuf_addstr(output, v->s);
>  		break;
>  	case QUOTE_SHELL:
> -		sq_quote_buf(&sb, v->s);
> +		sq_quote_buf(output, v->s);
>  		break;
>  	case QUOTE_PERL:
> -		perl_quote_buf(&sb, v->s);
> +		perl_quote_buf(output, v->s);
>  		break;
>  	case QUOTE_PYTHON:
> -		python_quote_buf(&sb, v->s);
> +		python_quote_buf(output, v->s);
>  		break;
>  	case QUOTE_TCL:
> -		tcl_quote_buf(&sb, v->s);
> +		tcl_quote_buf(output, v->s);
>  		break;
>  	}
> -	if (quote_style != QUOTE_NONE) {
> -		fputs(sb.buf, stdout);
> -		strbuf_release(&sb);
> -	}
>  }
>  
>  static int hex1(char ch)
> @@ -1234,7 +1229,7 @@ static int hex2(const char *cp)
>  		return -1;
>  }
>  
> -static void emit(const char *cp, const char *ep)
> +static void emit(const char *cp, const char *ep, struct strbuf *output)
>  {
>  	while (*cp && (!ep || cp < ep)) {
>  		if (*cp == '%') {
> @@ -1243,13 +1238,13 @@ static void emit(const char *cp, const char *ep)
>  			else {
>  				int ch = hex2(cp + 1);
>  				if (0 <= ch) {
> -					putchar(ch);
> +					strbuf_addch(output, ch);
>  					cp += 3;
>  					continue;
>  				}
>  			}
>  		}
> -		putchar(*cp);
> +		strbuf_addch(output, *cp);
>  		cp++;
>  	}
>  }
> @@ -1257,19 +1252,21 @@ 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 output = STRBUF_INIT;
> +	int i;
>  
>  	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>  		struct atom_value *atomv;
>  
>  		ep = strchr(sp, ')');
>  		if (cp < sp)
> -			emit(cp, sp);
> +			emit(cp, sp, &output);
>  		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
> -		print_value(atomv, quote_style);
> +		print_value(atomv, quote_style, &output);
>  	}
>  	if (*cp) {
>  		sp = cp + strlen(cp);
> -		emit(cp, sp);
> +		emit(cp, sp, &output);
>  	}
>  	if (need_color_reset_at_eol) {
>  		struct atom_value resetv;
> @@ -1278,9 +1275,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
>  		if (color_parse("reset", color) < 0)
>  			die("BUG: couldn't parse 'reset' as a color");
>  		resetv.s = color;
> -		print_value(&resetv, quote_style);
> +		print_value(&resetv, quote_style, &output);
>  	}
> +	for (i = 0; i < output.len; i++)
> +		printf("%c", output.buf[i]);
>  	putchar('\n');
> +	strbuf_release(&output);
>  }
>  
>  /*  If no sorting option is given, use refname to sort as default */

Re: [PATCH v8 02/11] ref-filter: introduce ref_formatting_state

[Junio C Hamano]

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

> Introduce a ref_formatting_state which will eventually hold the values
> of modifier atoms. Implement this within ref-filter.
>
> 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 | 49 +++++++++++++++++++++++++++++++++++++------------
>  ref-filter.h |  4 ++++
>  2 files changed, 41 insertions(+), 12 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index febdc45..c4c7064 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1190,9 +1190,10 @@ 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, struct strbuf *output)
> +static void print_value(struct atom_value *v, struct ref_formatting_state *state,
> +			struct strbuf *output)
>  {

I expect that the state would eventually become a stack of states
(i.e. the topmost one passed around, each having a pointer to the
previous level) in order to implement that "divert" mechanism for
(possibly nested) if ... end construct.

With that in mind, I suspect that state->output should be "where the
current level would output to", i.e. no need to pass state and
output around separately.

> +static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *value,
> +				   struct strbuf *format)
> +{

The name "format" feels quite misleading; the readers would expect
that you would use it in "strbuf_addf(format, value)", but that is
not what is going on here.

> @@ -1275,12 +1299,13 @@ 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, &output);
> +		print_value(&resetv, &state, &value);
> +		apply_formatting_state(&state, &value, &final_buf);
>  	}
> -	for (i = 0; i < output.len; i++)
> -		printf("%c", output.buf[i]);
> +	for (i = 0; i < final_buf.len; i++)
> +		printf("%c", final_buf.buf[i]);
>  	putchar('\n');
> -	strbuf_release(&output);
> +	strbuf_release(&final_buf);
>  }
>  
>  /*  If no sorting option is given, use refname to sort as default */
> diff --git a/ref-filter.h b/ref-filter.h
> index 6bf27d8..b64677f 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -16,6 +16,10 @@
>  #define FILTER_REFS_INCLUDE_BROKEN 0x1
>  #define FILTER_REFS_ALL 0x2
>  
> +struct ref_formatting_state {
> +	int quote_style;
> +};
> +
>  struct atom_value {
>  	const char *s;
>  	unsigned long ul; /* used for sorting when not FIELD_STR */

Re: [PATCH v8 0/11] Port tag.c over to use ref-filter APIs

[Karthik Nayak]

On Tue, Aug 4, 2015 at 1:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> This is part of my GSoC project to unify git tag -l, git branch -l,
>> git for-each-ref.  This patch series is continued from: Git (next)
>> https://github.com/git/git/commit/bf5418f49ff0cebc6e5ce04ad1417e1a47c81b61
>>
>> This series consists of porting tag.c over to using the ref-filter APIs
>>
>> Version 7 can be found here:
>> http://thread.gmane.org/gmane.comp.version-control.git/274990
>>
>> Changes:
>> * Make padright a general align atom.
>> * Make print_value() and emit() output to a strbuf rather than stdout directly.
>>
>> Interdiff:
>>
>> diff --git a/Documentation/git-for-each-ref.txt
>> b/Documentation/git-for-each-ref.txt
>> index bcf319a..e89b9b0 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -127,11 +127,12 @@ color::
>>      Change output color.  Followed by `:<colorname>`, where names
>>      are described in `color.branch.*`.
>>
>> -padright::
>> -    Pad succeeding atom or string to the right. Followed by
>> -    `:<value>`, where `value` states the total length of atom or
>> -    string including the padding. If the `value` is lesser than
>> -    the atom or string length, then no padding is performed.
>> +align::
>> +    Align succeeding atoms to the right, left or middle. Followed
>> +    by `:<type>,<paddinglength>`, where the `<type>` is either
>> +    left, right or middle and `<paddinglength>` is the total
>> +    length of the padding to be performed. If the atom length is
>> +    more than the padding length then no padding is performed.
>
> It is very very dissapointing to allow the "next atom only"
> implementation to squat on a good name "align:<type>,<width>",
> especially when I thought that the list agreed
>
>   %(align:<type>,<width>) any string with or without %(atom) %(end)
>
> would be the way to go.

>From what I read, I thought we wanted the next atom or string to be
aligned, if we need to align everything within the %(end) atom. I could
do that :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v8 01/11] ref-filter: print output to strbuf for formatting

[Karthik Nayak]

On Tue, Aug 4, 2015 at 2:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Introduce a strbuf `output` which will act as a substitute rather than
>> printing directly to stdout. This will be used for formatting
>> eventually.
>> ---
>
> Missing sign-off; the patch looks like a good first step in a nice
> direction.
>

Will add, Thanks :)

>>  ref-filter.c | 36 ++++++++++++++++++------------------
>>  1 file changed, 18 insertions(+), 18 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 7561727..febdc45 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1190,30 +1190,25 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
>>       qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);
>>  }
>>
>> -static void print_value(struct atom_value *v, int quote_style)
>> +static void print_value(struct atom_value *v, int quote_style, struct strbuf *output)
>>  {
>> -     struct strbuf sb = STRBUF_INIT;
>>       switch (quote_style) {
>>       case QUOTE_NONE:
>> -             fputs(v->s, stdout);
>> +             strbuf_addstr(output, v->s);
>>               break;
>>       case QUOTE_SHELL:
>> -             sq_quote_buf(&sb, v->s);
>> +             sq_quote_buf(output, v->s);
>>               break;
>>       case QUOTE_PERL:
>> -             perl_quote_buf(&sb, v->s);
>> +             perl_quote_buf(output, v->s);
>>               break;
>>       case QUOTE_PYTHON:
>> -             python_quote_buf(&sb, v->s);
>> +             python_quote_buf(output, v->s);
>>               break;
>>       case QUOTE_TCL:
>> -             tcl_quote_buf(&sb, v->s);
>> +             tcl_quote_buf(output, v->s);
>>               break;
>>       }
>> -     if (quote_style != QUOTE_NONE) {
>> -             fputs(sb.buf, stdout);
>> -             strbuf_release(&sb);
>> -     }
>>  }
>>
>>  static int hex1(char ch)
>> @@ -1234,7 +1229,7 @@ static int hex2(const char *cp)
>>               return -1;
>>  }
>>
>> -static void emit(const char *cp, const char *ep)
>> +static void emit(const char *cp, const char *ep, struct strbuf *output)
>>  {
>>       while (*cp && (!ep || cp < ep)) {
>>               if (*cp == '%') {
>> @@ -1243,13 +1238,13 @@ static void emit(const char *cp, const char *ep)
>>                       else {
>>                               int ch = hex2(cp + 1);
>>                               if (0 <= ch) {
>> -                                     putchar(ch);
>> +                                     strbuf_addch(output, ch);
>>                                       cp += 3;
>>                                       continue;
>>                               }
>>                       }
>>               }
>> -             putchar(*cp);
>> +             strbuf_addch(output, *cp);
>>               cp++;
>>       }
>>  }
>> @@ -1257,19 +1252,21 @@ 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 output = STRBUF_INIT;
>> +     int i;
>>
>>       for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>>               struct atom_value *atomv;
>>
>>               ep = strchr(sp, ')');
>>               if (cp < sp)
>> -                     emit(cp, sp);
>> +                     emit(cp, sp, &output);
>>               get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
>> -             print_value(atomv, quote_style);
>> +             print_value(atomv, quote_style, &output);
>>       }
>>       if (*cp) {
>>               sp = cp + strlen(cp);
>> -             emit(cp, sp);
>> +             emit(cp, sp, &output);
>>       }
>>       if (need_color_reset_at_eol) {
>>               struct atom_value resetv;
>> @@ -1278,9 +1275,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
>>               if (color_parse("reset", color) < 0)
>>                       die("BUG: couldn't parse 'reset' as a color");
>>               resetv.s = color;
>> -             print_value(&resetv, quote_style);
>> +             print_value(&resetv, quote_style, &output);
>>       }
>> +     for (i = 0; i < output.len; i++)
>> +             printf("%c", output.buf[i]);
>>       putchar('\n');
>> +     strbuf_release(&output);
>>  }
>>
>>  /*  If no sorting option is given, use refname to sort as default */



-- 
Regards,
Karthik Nayak

Re: [PATCH v8 02/11] ref-filter: introduce ref_formatting_state

[Karthik Nayak]

On Tue, Aug 4, 2015 at 2:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Introduce a ref_formatting_state which will eventually hold the values
>> of modifier atoms. Implement this within ref-filter.
>>
>> 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 | 49 +++++++++++++++++++++++++++++++++++++------------
>>  ref-filter.h |  4 ++++
>>  2 files changed, 41 insertions(+), 12 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index febdc45..c4c7064 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1190,9 +1190,10 @@ 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, struct strbuf *output)
>> +static void print_value(struct atom_value *v, struct ref_formatting_state *state,
>> +                     struct strbuf *output)
>>  {
>
> I expect that the state would eventually become a stack of states
> (i.e. the topmost one passed around, each having a pointer to the
> previous level) in order to implement that "divert" mechanism for
> (possibly nested) if ... end construct.
>

Makes sense :)

> With that in mind, I suspect that state->output should be "where the
> current level would output to", i.e. no need to pass state and
> output around separately.
>

Will do!

>> +static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *value,
>> +                                struct strbuf *format)
>> +{
>
> The name "format" feels quite misleading; the readers would expect
> that you would use it in "strbuf_addf(format, value)", but that is
> not what is going on here.
>

will change it to final i guess.

>> @@ -1275,12 +1299,13 @@ 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, &output);
>> +             print_value(&resetv, &state, &value);
>> +             apply_formatting_state(&state, &value, &final_buf);
>>       }
>> -     for (i = 0; i < output.len; i++)
>> -             printf("%c", output.buf[i]);
>> +     for (i = 0; i < final_buf.len; i++)
>> +             printf("%c", final_buf.buf[i]);
>>       putchar('\n');
>> -     strbuf_release(&output);
>> +     strbuf_release(&final_buf);
>>  }
>>
>>  /*  If no sorting option is given, use refname to sort as default */
>> diff --git a/ref-filter.h b/ref-filter.h
>> index 6bf27d8..b64677f 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -16,6 +16,10 @@
>>  #define FILTER_REFS_INCLUDE_BROKEN 0x1
>>  #define FILTER_REFS_ALL 0x2
>>
>> +struct ref_formatting_state {
>> +     int quote_style;
>> +};
>> +
>>  struct atom_value {
>>       const char *s;
>>       unsigned long ul; /* used for sorting when not FIELD_STR */



-- 
Regards,
Karthik Nayak

Re: [PATCH v8 0/11] Port tag.c over to use ref-filter APIs

[Junio C Hamano]

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

> On Tue, Aug 4, 2015 at 1:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> It is very very dissapointing to allow the "next atom only"
>> implementation to squat on a good name "align:<type>,<width>",
>> especially when I thought that the list agreed
>>
>>   %(align:<type>,<width>) any string with or without %(atom) %(end)
>>
>> would be the way to go.
>
> From what I read, I thought we wanted the next atom or string to be
> aligned, if we need to align everything within the %(end) atom.

Is that a serious comment?

Did I read too much into your $gmane/275119, expecting that you
understood everything you are saying "That's a good way to go" to?

> I could do that :)

Sure ;-)

Re: [PATCH v8 0/11] Port tag.c over to use ref-filter APIs

[Karthik Nayak]

On Tue, Aug 4, 2015 at 3:38 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Tue, Aug 4, 2015 at 1:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>>> It is very very dissapointing to allow the "next atom only"
>>> implementation to squat on a good name "align:<type>,<width>",
>>> especially when I thought that the list agreed
>>>
>>>   %(align:<type>,<width>) any string with or without %(atom) %(end)
>>>
>>> would be the way to go.
>>
>> From what I read, I thought we wanted the next atom or string to be
>> aligned, if we need to align everything within the %(end) atom.
>
> Is that a serious comment?
>
> Did I read too much into your $gmane/275119, expecting that you
> understood everything you are saying "That's a good way to go" to?

Sorry, I kinda was thinking only WRT to the %(if) and %(end) part of it.
Even though you clearly mentioned about %(align) also.

>
>> I could do that :)
>
> Sure ;-)

I have it ready, will wait to see if there are more comments and send with
next iteration of the series.

-- 
Regards,
Karthik Nayak