[PATCH v2 00/10] Port tag.c to use ref-filter APIs

[PATCH v2 00/10] Port tag.c 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:
http://article.gmane.org/gmane.comp.version-control.git/273569

The previous RFC version is here:
http://thread.gmane.org/gmane.comp.version-control.git/272654

Changes in this version:
* Cleanup Documentation/tag
* Fixed grammatical errors
* Fixed a small merge conflict
* Other small changes

 Documentation/git-tag.txt |  39 ++++++++++---
 builtin/for-each-ref.c    |   3 +-
 builtin/tag.c             | 368
++++++++++++++++++++------------------------------------------------------------------------------------------------
 ref-filter.c              |  95 ++++++++++++++++++++++++++++--
 ref-filter.h              |   7 ++-
 t/t7004-tag.sh            |  51 +++++++++++++---
 6 files changed, 234 insertions(+), 329 deletions(-)

-- 
Regards,
Karthik Nayak

[PATCH v2 01/10] ref-filter: add %(refname:shortalign=X) option

[Karthik Nayak]

Add support for %(refname:shortalign=X) where X is a number.
This will print a shortened refname aligned to the left
followed by spaces for a total length of X characters.
If X is less than the shortened refname size, the entire
shortened refname is printed.

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 | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index dd0709d..3098497 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;
 
@@ -695,7 +696,23 @@ static void populate_value(struct ref_array_item *ref)
 			int num_ours, num_theirs;
 
 			formatp++;
-			if (!strcmp(formatp, "short"))
+			if (starts_with(formatp, "shortalign=")) {
+				const char *valp, *short_refname = NULL;
+				int val, len;
+
+				skip_prefix(formatp, "shortalign=", &valp);
+				val = atoi(valp);
+				refname = short_refname = shorten_unambiguous_ref(refname,
+										  warn_ambiguous_refs);
+				len = utf8_strwidth(refname);
+				if (val > len) {
+					struct strbuf buf = STRBUF_INIT;
+					strbuf_addstr(&buf, refname);
+					strbuf_addchars(&buf, ' ', val - len);
+					free((char *)short_refname);
+					refname = strbuf_detach(&buf, NULL);
+				}
+			} else if (!strcmp(formatp, "short"))
 				refname = shorten_unambiguous_ref(refname,
 						      warn_ambiguous_refs);
 			else if (!strcmp(formatp, "track") &&
-- 
2.4.5

[PATCH v2 02/10] ref-filter: add option to filter only tags

[Karthik Nayak]

Add an option in 'filter_refs()' to use 'for_each_tag_ref()'
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 +
 2 files changed, 3 insertions(+)

diff --git a/ref-filter.c b/ref-filter.c
index 3098497..e690177 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1152,6 +1152,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(ref_filter_handler, &ref_cbdata);
 	else if (type)
 		die("filter_refs: invalid type");
 
diff --git a/ref-filter.h b/ref-filter.h
index 6bf27d8..449ddb8 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
 
 struct atom_value {
 	const char *s;
-- 
2.4.5

[PATCH v2 03/10] ref-filter: support printing N lines from tag annotation

[Karthik Nayak]

In 'tag.c' we can print N lines from the annotation of the tag
using the '-n<num>' option. Copy code from 'tag.c' to 'ref-filter'
and modify '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           | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 ref-filter.h           |  3 ++-
 4 files changed, 54 insertions(+), 3 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 071d001..3b6a6cc 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 dupplicated 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 e690177..1b088b1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1273,7 +1273,48 @@ 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)
+/* Print 'lines' no of lines of a given 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;
 
@@ -1299,6 +1340,11 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 		resetv.s = color;
 		print_value(&resetv, quote_style);
 	}
+	if (lines > 0) {
+		struct object_id oid;
+		hashcpy(oid.hash, info->objectname);
+		show_tag_lines(&oid, lines);
+	}
 	putchar('\n');
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 449ddb8..5247475 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -55,6 +55,7 @@ struct ref_filter {
 	struct commit *merge_commit;
 
 	unsigned int with_commit_tag_algo : 1;
+	unsigned int lines;
 };
 
 struct ref_filter_cbdata {
@@ -87,7 +88,7 @@ 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);
+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.5

[PATCH v2 04/10] ref-filter: add support to sort by version

[Karthik Nayak]

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

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 | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 1b088b1..1516cd1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -11,8 +11,9 @@
 #include "ref-filter.h"
 #include "revision.h"
 #include "utf8.h"
+#include "version.h"
 
-typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
+typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME, FIELD_VER } cmp_type;
 
 static struct {
 	const char *name;
@@ -54,6 +55,8 @@ static struct {
 	{ "flag" },
 	{ "HEAD" },
 	{ "color" },
+	{ "version", FIELD_VER },
+	{ "v", FIELD_VER },
 };
 
 /*
@@ -630,7 +633,9 @@ static void populate_value(struct ref_array_item *ref)
 			name++;
 		}
 
-		if (starts_with(name, "refname"))
+		if (starts_with(name, "refname") ||
+		    starts_with(name, "version:") ||
+		    starts_with(name, "v:"))
 			refname = ref->refname;
 		else if (starts_with(name, "symref"))
 			refname = ref->symref ? ref->symref : "";
@@ -696,7 +701,13 @@ static void populate_value(struct ref_array_item *ref)
 			int num_ours, num_theirs;
 
 			formatp++;
-			if (starts_with(formatp, "shortalign=")) {
+			if (starts_with(name, "version") || starts_with(name, "v")) {
+				if (strcmp(formatp, "refname"))
+					die("unknown %.*s format %s",
+					    (int)(formatp - name), name, formatp);
+				v->s = refname;
+				continue;
+			} else if (starts_with(formatp, "shortalign=")) {
 				const char *valp, *short_refname = NULL;
 				int val, len;
 
@@ -1176,6 +1187,9 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 	case FIELD_STR:
 		cmp = strcmp(va->s, vb->s);
 		break;
+	case FIELD_VER:
+		cmp = versioncmp(va->s, vb->s);
+		break;
 	default:
 		if (va->ul < vb->ul)
 			cmp = -1;
-- 
2.4.5

[PATCH v2 05/10] ref-filter: add option to match literal pattern

[Karthik Nayak]

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

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           | 25 ++++++++++++++++++++++++-
 ref-filter.h           |  3 ++-
 3 files changed, 27 insertions(+), 2 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 1516cd1..0e19f44 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -956,6 +956,20 @@ 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/m") or a wildcard (e.g. the same ref
+ * matches "refs/heads/m*",too).
+ */
+static int match_pattern(const char **patterns, const char *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).
@@ -979,6 +993,15 @@ static int match_name_as_path(const char **pattern, const char *refname)
 	return 0;
 }
 
+static int filter_pattern_match(struct ref_filter *filter, const char *refname)
+{
+	if (!*filter->name_patterns)
+		return 1;
+	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
@@ -1047,7 +1070,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 5247475..633108c 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -54,7 +54,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.5

[PATCH v2 06/10] Documentation/tag: remove double occurance of "<pattern>"

[Karthik Nayak]

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 | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 034d10d..4b04c2b 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -14,7 +14,6 @@ SYNOPSIS
 'git tag' -d <tagname>...
 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
 	[--column[=<options>] | --no-column] [<pattern>...]
-	[<pattern>...]
 'git tag' -v <tagname>...
 
 DESCRIPTION
-- 
2.4.5

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

[Karthik Nayak]

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 3b6a6cc..bbda0c6 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,16 +579,16 @@ 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 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'),
@@ -604,14 +609,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()
@@ -620,6 +625,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);
 
@@ -636,7 +643,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;
@@ -649,18 +656,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.5

[PATCH v2 08/10] tag.c: use 'ref-filter' APIs

[Karthik Nayak]

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             | 343 
++++++----------------------------------------
  t/t7004-tag.sh            |   8 +-
  3 files changed, 50 insertions(+), 317 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 4b04c2b..02fb363 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] [<pattern>...]
+	[--column[=<options>] | --no-column] [--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 bbda0c6..36f8019 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -28,278 +28,34 @@ 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 dupplicated 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 = "%(refname:shortalign=16)";
+	else
+		format = "%(refname:short)";
+
+	verify_ref_format(format);
+	if (!sorting)
+		sorting = ref_default_sorting();
+	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 +122,23 @@ 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++;
+ 	}

-	*sort = (type | flags);
+	len = strlen(arg);
+	s->atom = parse_ref_filter_atom(arg, arg+len);

  	return 0;
  }
@@ -402,11 +146,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 +309,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;
@@ -586,6 +324,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"),
@@ -611,10 +350,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
@@ -622,7 +359,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));
@@ -648,6 +385,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)) {
@@ -656,10 +395,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 d1ff5c9..51a233f 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1450,13 +1450,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.5

[PATCH v2 08/10] tag.c: use 'ref-filter' APIs

[Karthik Nayak]

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             | 343 
++++++----------------------------------------
  t/t7004-tag.sh            |   8 +-
  3 files changed, 50 insertions(+), 317 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 4b04c2b..02fb363 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] [<pattern>...]
+	[--column[=<options>] | --no-column] [--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 bbda0c6..36f8019 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -28,278 +28,34 @@ 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 dupplicated 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 = "%(refname:shortalign=16)";
+	else
+		format = "%(refname:short)";
+
+	verify_ref_format(format);
+	if (!sorting)
+		sorting = ref_default_sorting();
+	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 +122,23 @@ 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++;
+ 	}

-	*sort = (type | flags);
+	len = strlen(arg);
+	s->atom = parse_ref_filter_atom(arg, arg+len);

  	return 0;
  }
@@ -402,11 +146,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 +309,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;
@@ -586,6 +324,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"),
@@ -611,10 +350,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
@@ -622,7 +359,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));
@@ -648,6 +385,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)) {
@@ -656,10 +395,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 d1ff5c9..51a233f 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1450,13 +1450,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.5

[PATCH v2 09/10] tag.c: implement '--format' option

[Karthik Nayak]

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

Add tests and documentation for the same.

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

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 02fb363..16e396c 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] [--sort=<key>] [<pattern>...]
+	[--column[=<options>] | --no-column] [--sort=<key>] [--format=<format>]
+	[<pattern>...]
  'git tag' -v <tagname>...

  DESCRIPTION
@@ -155,6 +156,19 @@ 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
+	`%(objectname) SPC %(objecttype) TAB %(refname)`.
+	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 36f8019..601b293 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 = "%(refname:shortalign=16)";
-	else
+	else if (!format)
  		format = "%(refname:short)";

  	verify_ref_format(format);
@@ -325,6 +324,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"),
@@ -356,6 +356,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()
  	};

@@ -395,8 +396,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 51a233f..e8cebb6 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1507,4 +1507,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 &&
+	foo1.10
+	foo1.3
+	foo1.6
+	foo1.6-rc1
+	foo1.6-rc2
+	EOF
+	git tag -l --format="%(refname)" "foo*" >actual &&
+	test_cmp expect actual
+'
+
  test_done
-- 
2.4.5

[PATCH v2 10/10] tag.c: implement '--merged' and '--no-merged' options

[Karthik Nayak]

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 | 10 +++++++++-
  builtin/tag.c             |  6 +++++-
  t/t7004-tag.sh            | 27 +++++++++++++++++++++++++++
  3 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 16e396c..74ed157 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] [--sort=<key>] [--format=<format>]
-	[<pattern>...]
+	[(--merged | --no-merged) [<commit>]] [<pattern>...]
  'git tag' -v <tagname>...

  DESCRIPTION
@@ -169,6 +169,14 @@ This option is only applicable when listing tags 
without annotation lines.
  	`%09` to `\t` (TAB) and `%0a` to `\n` (LF).
  	The fields are same as those in `git for-each-ref`.

+--merged [<commit>]::
+	Only list tags whose tips are reachable from the
+	specified commit (HEAD if not specified).
+
+--no-merged [<commit>]::
+	Only list tags whose tips are not reachable from the
+	specified commit (HEAD if not specified).
+

  CONFIGURATION
  -------------
diff --git a/builtin/tag.c b/builtin/tag.c
index 601b293..abd42a0 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[--merged [<commit>]] [--no-merged [<commit>]] [<pattern>...]"),
  	N_("git tag -v <tagname>..."),
  	NULL
  };
@@ -350,6 +350,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),
  		{
@@ -410,6 +412,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 e8cebb6..873aad3 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1523,4 +1523,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.5

Re: [PATCH v2 06/10] Documentation/tag: remove double occurance of "<pattern>"

[Christian Couder]

On Thu, Jul 9, 2015 at 12:27 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> 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 | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 034d10d..4b04c2b 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -14,7 +14,6 @@ SYNOPSIS
>  'git tag' -d <tagname>...
>  'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
>         [--column[=<options>] | --no-column] [<pattern>...]
> -       [<pattern>...]
>  'git tag' -v <tagname>...

As this patch could be applied directly to master and to maint maybe
you could send it at the top of this patch series or alone outside of
this patch series.

Re: [PATCH v2 08/10] tag.c: use 'ref-filter' APIs

[Matthieu Moy]

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

>  Documentation/git-tag.txt |  16 ++-
>  builtin/tag.c             | 343
> ++++++----------------------------------------
>  t/t7004-tag.sh            |   8 +-

This patch was sent with Thunderbird unlike the others in the series. It
has some line wrapping which make it unapplicable.

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

Re: [PATCH v2 08/10] tag.c: use 'ref-filter' APIs

[Karthik Nayak]

On Thu, Jul 9, 2015 at 6:18 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>  Documentation/git-tag.txt |  16 ++-
>>  builtin/tag.c             | 343
>> ++++++----------------------------------------
>>  t/t7004-tag.sh            |   8 +-
>
> This patch was sent with Thunderbird unlike the others in the series. It
> has some line wrapping which make it unapplicable.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

Yes, 'git send-email' started failing, hence 08/10, 09/10 and 10/10
were sent by thunderbird.
I'm trying to figure out why.
If anyone can help, this is what it's saying.
"[Net::SMTP::SSL] Connection closed at
/usr/lib/git-core/git-send-email line 1320.
fatal: 'send-email' appears to be a git command, but we were not
able to execute it. Maybe git-send-email is broken?"

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 06/10] Documentation/tag: remove double occurance of "<pattern>"

[Karthik Nayak]

On Thu, Jul 9, 2015 at 5:49 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Thu, Jul 9, 2015 at 12:27 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> 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 | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
>> index 034d10d..4b04c2b 100644
>> --- a/Documentation/git-tag.txt
>> +++ b/Documentation/git-tag.txt
>> @@ -14,7 +14,6 @@ SYNOPSIS
>>  'git tag' -d <tagname>...
>>  'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
>>         [--column[=<options>] | --no-column] [<pattern>...]
>> -       [<pattern>...]
>>  'git tag' -v <tagname>...
>
> As this patch could be applied directly to master and to maint maybe
> you could send it at the top of this patch series or alone outside of
> this patch series.

I'll do that :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 01/10] ref-filter: add %(refname:shortalign=X) option

[Matthieu Moy]

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

> Add support for %(refname:shortalign=X) where X is a number.
> This will print a shortened refname aligned to the left
> followed by spaces for a total length of X characters.
> If X is less than the shortened refname size, the entire
> shortened refname is printed.

Not really an issue, but you're wrapping your text at ~60 characters.
The common use is to wrap around 70 to 80. Using Emacs, auto-fill-mode
or M-q does this automatically. If you use another text editor, it can
probably do that for you too.

> 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 | 19 ++++++++++++++++++-

I think this would deserve a test and documentation. Even though your
motivation is for an internal implementation, some users may want to use
the feature in 'git for-each-ref --format=...'.

>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index dd0709d..3098497 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;
>  
> @@ -695,7 +696,23 @@ static void populate_value(struct ref_array_item *ref)
>  			int num_ours, num_theirs;
>  
>  			formatp++;
> -			if (!strcmp(formatp, "short"))
> +			if (starts_with(formatp, "shortalign=")) {
> +				const char *valp, *short_refname = NULL;
> +				int val, len;
> +
> +				skip_prefix(formatp, "shortalign=", &valp);
> +				val = atoi(valp);

You're silently accepting %(refname:shortalign=foo) and
%(refname:shortalign=). I think it would be better to reject such cases
explicitly.

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

Re: [PATCH v2 03/10] ref-filter: support printing N lines from tag annotation

[Matthieu Moy]

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

> In 'tag.c' we can print N lines from the annotation of the tag
> using the '-n<num>' option.

Not only annotation of the tag, but also from the commit message for
lightweight tags.

> --- 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 dupplicated in ref-filter, will eventually be removed as

dupplicated -> duplicated.

> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -55,6 +55,7 @@ struct ref_filter {
>  	struct commit *merge_commit;
>  
>  	unsigned int with_commit_tag_algo : 1;
> +	unsigned int lines;
>  };
>  
>  struct ref_filter_cbdata {
> @@ -87,7 +88,7 @@ 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);
> +void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style, unsigned int lines);

I would add "If lines > 0, prints the first 'lines' no of lines of the
object pointed to" or so to the docstring.

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

Re: [PATCH v2 04/10] ref-filter: add support to sort by version

[Matthieu Moy]

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

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

You should elaborate on why you need this. Given the context, I can
guess that you will need this to implement tag, but for example I first
wondered why you needed both version: and v:, but I guess it comes from
the fact that 'git tag --sort' can take version:refname or v:refname.

I think this deserves a test and documentation in for-each-ref.txt.

As-is, the code is a bit hard to understand. I first saw you were
allowing

  git for-each-ref --format '%(version:refname)'

(which you are, but only as a side effect), and then understood that
this was also allowing

  git for-each-ref --sort version:refname

A test would have shown me this immediately. Some hints in the commit
message would clearly have helped too.

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

Re: [PATCH v2 05/10] ref-filter: add option to match literal pattern

[Matthieu Moy]

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

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

Here also, a hint on why this is needed would be welcome.

> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -956,6 +956,20 @@ 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/m") or a wildcard (e.g. the same ref
> + * matches "refs/heads/m*",too).

Missing space after , (same in the hunk context below)

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

Re: [PATCH v2 08/10] tag.c: use 'ref-filter' APIs

[Matthieu Moy]

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

> +	s->atom = parse_ref_filter_atom(arg, arg+len);

Spaces around +.

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

Re: [PATCH v2 08/10] tag.c: use 'ref-filter' APIs

[Matthieu Moy]

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

> If anyone can help, this is what it's saying.
> "[Net::SMTP::SSL] Connection closed at

Perhaps your SMTP server thought you were sending too many emails to too
many people and closed the connection thinking you were a spammer.

If you're having this kind of issues, it may make sense to run
"format-patch" and "send-email" as two separate steps. This way, you can
re-run "send-email" on the pieces which failed manually (adjusting
--in-reply-to).

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

Re: [PATCH v2 08/10] tag.c: use 'ref-filter' APIs

[Karthik Nayak]

On Thu, Jul 9, 2015 at 7:13 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> If anyone can help, this is what it's saying.
>> "[Net::SMTP::SSL] Connection closed at
>
> Perhaps your SMTP server thought you were sending too many emails to too
> many people and closed the connection thinking you were a spammer.
>
> If you're having this kind of issues, it may make sense to run
> "format-patch" and "send-email" as two separate steps. This way, you can
> re-run "send-email" on the pieces which failed manually (adjusting
> --in-reply-to).
>

I'm guessing the same. That's what I usually do, separate format-patch and
send-email.

>Spaces around +.

Thanks :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 03/10] ref-filter: support printing N lines from tag annotation

[Karthik Nayak]

On Thu, Jul 9, 2015 at 6:37 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> In 'tag.c' we can print N lines from the annotation of the tag
>> using the '-n<num>' option.
>
> Not only annotation of the tag, but also from the commit message for
> lightweight tags.

will do.

>
>> --- 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 dupplicated in ref-filter, will eventually be removed as
>
> dupplicated -> duplicated.
>

Thanks :)

>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -55,6 +55,7 @@ struct ref_filter {
>>       struct commit *merge_commit;
>>
>>       unsigned int with_commit_tag_algo : 1;
>> +     unsigned int lines;
>>  };
>>
>>  struct ref_filter_cbdata {
>> @@ -87,7 +88,7 @@ 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);
>> +void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style, unsigned int lines);
>
> I would add "If lines > 0, prints the first 'lines' no of lines of the
> object pointed to" or so to the docstring.
>

Will do, thanks.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 04/10] ref-filter: add support to sort by version

[Karthik Nayak]

On Thu, Jul 9, 2015 at 6:59 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Add support to sort by version using the "v:refname" and
>> "version:refname" option. This is achieved by using the
>> 'version_cmp()' function as the comparing function for qsort.
>
> You should elaborate on why you need this. Given the context, I can
> guess that you will need this to implement tag, but for example I first
> wondered why you needed both version: and v:, but I guess it comes from
> the fact that 'git tag --sort' can take version:refname or v:refname.
>
> I think this deserves a test and documentation in for-each-ref.txt.

I'll add it to "for-each-ref.txt" documentation.
About the tests, there are already tests for the same in git-tag.txt and
that's the only reason I did not repeat the tests in for-each-ref.

>
> As-is, the code is a bit hard to understand. I first saw you were
> allowing
>
>   git for-each-ref --format '%(version:refname)'
>
> (which you are, but only as a side effect), and then understood that
> this was also allowing
>
>   git for-each-ref --sort version:refname
>
> A test would have shown me this immediately. Some hints in the commit
> message would clearly have helped too.
>

Most of the sorting options have side effects in "--format", but yeah will add
details in the commit message.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 04/10] ref-filter: add support to sort by version

[Karthik Nayak]

On Fri, Jul 10, 2015 at 4:22 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Thu, Jul 9, 2015 at 6:59 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> Add support to sort by version using the "v:refname" and
>>> "version:refname" option. This is achieved by using the
>>> 'version_cmp()' function as the comparing function for qsort.
>>
>> You should elaborate on why you need this. Given the context, I can
>> guess that you will need this to implement tag, but for example I first
>> wondered why you needed both version: and v:, but I guess it comes from
>> the fact that 'git tag --sort' can take version:refname or v:refname.
>>
>> I think this deserves a test and documentation in for-each-ref.txt.
>
> I'll add it to "for-each-ref.txt" documentation.
> About the tests, there are already tests for the same in git-tag.txt and
> that's the only reason I did not repeat the tests in for-each-ref.
>

But since the porting is in a later commit, will add tests to for-each-ref.

>>
>> As-is, the code is a bit hard to understand. I first saw you were
>> allowing
>>
>>   git for-each-ref --format '%(version:refname)'
>>
>> (which you are, but only as a side effect), and then understood that
>> this was also allowing
>>
>>   git for-each-ref --sort version:refname
>>
>> A test would have shown me this immediately. Some hints in the commit
>> message would clearly have helped too.
>>
>
> Most of the sorting options have side effects in "--format", but yeah will add
> details in the commit message.
>
> --
> Regards,
> Karthik Nayak



-- 
Regards,
Karthik Nayak

Re: [PATCH v2 05/10] ref-filter: add option to match literal pattern

[Karthik Nayak]

On Thu, Jul 9, 2015 at 7:02 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Since 'ref-filter' only has an option to match path names
>> add an option for regular pattern matching.
>
> Here also, a hint on why this is needed would be welcome.
>

Will add.

>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -956,6 +956,20 @@ 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/m") or a wildcard (e.g. the same ref
>> + * matches "refs/heads/m*",too).
>
> Missing space after , (same in the hunk context below)
>

Noted. Thanks :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 04/10] ref-filter: add support to sort by version

[Matthieu Moy]

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

> On Fri, Jul 10, 2015 at 4:22 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Thu, Jul 9, 2015 at 6:59 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> Add support to sort by version using the "v:refname" and
>>>> "version:refname" option. This is achieved by using the
>>>> 'version_cmp()' function as the comparing function for qsort.
>>>
>>> You should elaborate on why you need this. Given the context, I can
>>> guess that you will need this to implement tag, but for example I first
>>> wondered why you needed both version: and v:, but I guess it comes from
>>> the fact that 'git tag --sort' can take version:refname or v:refname.
>>>
>>> I think this deserves a test and documentation in for-each-ref.txt.
>>
>> I'll add it to "for-each-ref.txt" documentation.
>> About the tests, there are already tests for the same in git-tag.txt and
>> that's the only reason I did not repeat the tests in for-each-ref.
>>
>
> But since the porting is in a later commit, will add tests to for-each-ref.

Yes: to me "it's tested through 'git tag'" is a good argument to do only
a superficial test, check that 'for-each-ref --sort v:refname' activate
the sorting, but no detailed corner-case testing. But not a good
argument to have no test at all on for-each-ref.

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

Re: [PATCH v2 01/10] ref-filter: add %(refname:shortalign=X) option

[Junio C Hamano]

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

> Add support for %(refname:shortalign=X) where X is a number.
> This will print a shortened refname aligned to the left
> followed by spaces for a total length of X characters.
> If X is less than the shortened refname size, the entire
> shortened refname is printed.
>
> 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 | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)

This may be enough to support the various existing formats that are
offered by "git branch" and/or "git tag", but I do not think if this
is the right approach in the longer term, or if we are painting
ourselves in a corner we cannot cleanly get out of later [*1*].
Will the "refname" stay to be the only thing that may want alignment
padding appended in the future?  Will it stay true that we want to
align only to the left?  Etc., etc.

Cc'ed Duy as %< in the pretty-format was his invention at around
a5752342 (pretty: support padding placeholders, %< %> and %><,
2013-04-19).

> diff --git a/ref-filter.c b/ref-filter.c
> index dd0709d..3098497 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;
>  
> @@ -695,7 +696,23 @@ static void populate_value(struct ref_array_item *ref)
>  			int num_ours, num_theirs;
>  
>  			formatp++;
> -			if (!strcmp(formatp, "short"))
> +			if (starts_with(formatp, "shortalign=")) {

When adding a new thing to an existing list, we prefer to append it
at the end of the list, if there is no other reason not to do so
(e.g. "the existing list is sorted in this order, and the new
location was chosen to fit the new item to honor the existing
ordering rule" is a valid reason to put it at the beginning, if the
existing sorting rule dictates that the new thing must come at the
beginning).

> +				const char *valp, *short_refname = NULL;
> +				int val, len;
> +
> +				skip_prefix(formatp, "shortalign=", &valp);
> +				val = atoi(valp);

In newer code, we would want to avoid atoi() so that "foo:shortalign=1z"
that is a typo of "12" can be caught as an error.  Either strtol_i()
or strtoul_ui() may be better (we would need to adjust it further
when Michael decides to resurrect his numparse thing that has been
in the stalled bin for quite a while, though).

> +				refname = short_refname = shorten_unambiguous_ref(refname,
> +										  warn_ambiguous_refs);
> +				len = utf8_strwidth(refname);
> +				if (val > len) {
> +					struct strbuf buf = STRBUF_INIT;
> +					strbuf_addstr(&buf, refname);
> +					strbuf_addchars(&buf, ' ', val - len);
> +					free((char *)short_refname);
> +					refname = strbuf_detach(&buf, NULL);
> +				}

What should happen when the display column width of the string is
wider?  If a user wants to align the refs that are usually usually
short start the next thing at the 8th column, which should she use?

    "%(refname:shorta=7) %(next item)"
    "%(refname:shorta=8)%(next item)"

> +			} else if (!strcmp(formatp, "short"))
>  				refname = shorten_unambiguous_ref(refname,
>  						      warn_ambiguous_refs);
>  			else if (!strcmp(formatp, "track") &&

Re: [PATCH v2 05/10] ref-filter: add option to match literal pattern

[Junio C Hamano]

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

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

There is nothing "regular" about the pattern matching you are
adding.

Everywhere else we use patterns on refs we call wildmatch(), which
is an enhanced implementation of fnmatch(3), and you are doing the
same in this new codepath.

Just drop that word from here (and if you said something similar in
the documentation, drop "regular" ffrom there as well).  It would
invite confusion with regular expression matching, which we will not
do for refs.

Re: [PATCH v2 06/10] Documentation/tag: remove double occurance of "<pattern>"

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Thu, Jul 9, 2015 at 12:27 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> 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 | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
>> index 034d10d..4b04c2b 100644
>> --- a/Documentation/git-tag.txt
>> +++ b/Documentation/git-tag.txt
>> @@ -14,7 +14,6 @@ SYNOPSIS
>>  'git tag' -d <tagname>...
>>  'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
>>         [--column[=<options>] | --no-column] [<pattern>...]
>> -       [<pattern>...]
>>  'git tag' -v <tagname>...
>
> As this patch could be applied directly to master and to maint maybe
> you could send it at the top of this patch series or alone outside of
> this patch series.

Thanks. I'll pick this patch from the remainder of the series and
queue it on a separate topic.

Re: [PATCH v2 04/10] ref-filter: add support to sort by version

[Karthik Nayak]

On Fri, Jul 10, 2015 at 5:48 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Fri, Jul 10, 2015 at 4:22 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> On Thu, Jul 9, 2015 at 6:59 PM, Matthieu Moy
>>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>>
>>>>> Add support to sort by version using the "v:refname" and
>>>>> "version:refname" option. This is achieved by using the
>>>>> 'version_cmp()' function as the comparing function for qsort.
>>>>
>>>> You should elaborate on why you need this. Given the context, I can
>>>> guess that you will need this to implement tag, but for example I first
>>>> wondered why you needed both version: and v:, but I guess it comes from
>>>> the fact that 'git tag --sort' can take version:refname or v:refname.
>>>>
>>>> I think this deserves a test and documentation in for-each-ref.txt.
>>>
>>> I'll add it to "for-each-ref.txt" documentation.
>>> About the tests, there are already tests for the same in git-tag.txt and
>>> that's the only reason I did not repeat the tests in for-each-ref.
>>>
>>
>> But since the porting is in a later commit, will add tests to for-each-ref.
>
> Yes: to me "it's tested through 'git tag'" is a good argument to do only
> a superficial test, check that 'for-each-ref --sort v:refname' activate
> the sorting, but no detailed corner-case testing. But not a good
> argument to have no test at all on for-each-ref.
>

Agreed, I've written tests for the same.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 05/10] ref-filter: add option to match literal pattern

[Karthik Nayak]

On Fri, Jul 10, 2015 at 10:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Since 'ref-filter' only has an option to match path names
>> add an option for regular pattern matching.
>
> There is nothing "regular" about the pattern matching you are
> adding.
>
> Everywhere else we use patterns on refs we call wildmatch(), which
> is an enhanced implementation of fnmatch(3), and you are doing the
> same in this new codepath.
>
> Just drop that word from here (and if you said something similar in
> the documentation, drop "regular" ffrom there as well).  It would
> invite confusion with regular expression matching, which we will not
> do for refs.

Ok, will do. Thanks

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 01/10] ref-filter: add %(refname:shortalign=X) option

[Karthik Nayak]

On Thu, Jul 9, 2015 at 6:28 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Add support for %(refname:shortalign=X) where X is a number.
>> This will print a shortened refname aligned to the left
>> followed by spaces for a total length of X characters.
>> If X is less than the shortened refname size, the entire
>> shortened refname is printed.
>
> Not really an issue, but you're wrapping your text at ~60 characters.
> The common use is to wrap around 70 to 80. Using Emacs, auto-fill-mode
> or M-q does this automatically. If you use another text editor, it can
> probably do that for you too.
>

Thanks, I was just manually clipping it.

>> 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 | 19 ++++++++++++++++++-
>
> I think this would deserve a test and documentation. Even though your
> motivation is for an internal implementation, some users may want to use
> the feature in 'git for-each-ref --format=...'.
>

I didn't want to include documentation as this is mostly for internal use,
but will add with tests.

>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index dd0709d..3098497 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;
>>
>> @@ -695,7 +696,23 @@ static void populate_value(struct ref_array_item *ref)
>>                       int num_ours, num_theirs;
>>
>>                       formatp++;
>> -                     if (!strcmp(formatp, "short"))
>> +                     if (starts_with(formatp, "shortalign=")) {
>> +                             const char *valp, *short_refname = NULL;
>> +                             int val, len;
>> +
>> +                             skip_prefix(formatp, "shortalign=", &valp);
>> +                             val = atoi(valp);
>
> You're silently accepting %(refname:shortalign=foo) and
> %(refname:shortalign=). I think it would be better to reject such cases
> explicitly.
>

Oh yeah! will do.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 05/10] ref-filter: add option to match literal pattern

[Matthieu Moy]

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

> On Fri, Jul 10, 2015 at 10:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> Since 'ref-filter' only has an option to match path names
>>> add an option for regular pattern matching.
>>
>> There is nothing "regular" about the pattern matching you are
>> adding.
>>
>> Everywhere else we use patterns on refs we call wildmatch(), which
>> is an enhanced implementation of fnmatch(3), and you are doing the
>> same in this new codepath.
>>
>> Just drop that word from here (and if you said something similar in
>> the documentation, drop "regular" ffrom there as well).  It would
>> invite confusion with regular expression matching, which we will not
>> do for refs.
>
> Ok, will do. Thanks

Just dropping "regular" leads to a strange sentence, since the path name
match is also a kind of pattern-matching. I'd write

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

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

Re: [PATCH v2 01/10] ref-filter: add %(refname:shortalign=X) option

[Matthieu Moy]

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

> On Thu, Jul 9, 2015 at 6:28 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>
>> I think this would deserve a test and documentation. Even though your
>> motivation is for an internal implementation, some users may want to use
>> the feature in 'git for-each-ref --format=...'.
>>
>
> I didn't want to include documentation as this is mostly for internal use,
> but will add with tests.

You need it for internal use, but it's still available to the users, and
may actually turn out to be useful to users.

Actually, you are rewritting tag and branch based on the internal C API,
but it should be possible to write similar commands based on the
command-line interface and by looking at the documentation. It's part of
the philosophy of Git ("toolkit design") to have plumbing commands that
allow writting high-level commands through scripting, or custom commands
through aliases.

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

Re: [PATCH v2 01/10] ref-filter: add %(refname:shortalign=X) option

[Karthik Nayak]

On Fri, Jul 10, 2015 at 9:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> This may be enough to support the various existing formats that are
> offered by "git branch" and/or "git tag", but I do not think if this
> is the right approach in the longer term, or if we are painting
> ourselves in a corner we cannot cleanly get out of later [*1*].
> Will the "refname" stay to be the only thing that may want alignment
> padding appended in the future?  Will it stay true that we want to
> align only to the left?  Etc., etc.
>
> Cc'ed Duy as %< in the pretty-format was his invention at around
> a5752342 (pretty: support padding placeholders, %< %> and %><,
> 2013-04-19).
>

I kinda had the same though, my only justification was that it was only being
internally used. I'll have another look if as to see if I can make it
universal somehow.
Let's see what Duy has to suggest.

>
> When adding a new thing to an existing list, we prefer to append it
> at the end of the list, if there is no other reason not to do so
> (e.g. "the existing list is sorted in this order, and the new
> location was chosen to fit the new item to honor the existing
> ordering rule" is a valid reason to put it at the beginning, if the
> existing sorting rule dictates that the new thing must come at the
> beginning).
>

my bad, will change it!

>
> In newer code, we would want to avoid atoi() so that "foo:shortalign=1z"
> that is a typo of "12" can be caught as an error.  Either strtol_i()
> or strtoul_ui() may be better (we would need to adjust it further
> when Michael decides to resurrect his numparse thing that has been
> in the stalled bin for quite a while, though).
>

Will have a look, thanks :)

>
> What should happen when the display column width of the string is
> wider?  If a user wants to align the refs that are usually usually
> short start the next thing at the 8th column, which should she use?
>
>     "%(refname:shorta=7) %(next item)"
>     "%(refname:shorta=8)%(next item)"
>

Both your examples would start the next item at the 8th column
(starting with 0),
the only difference being :
Case 1: when the refname is 8 columns wide
"%(refname:shorta=7) %(next item)": would give us eight columns of
refname + space + next item
"%(refname:shorta=8)%(next item)": would give us eight columns of
refname + next item
Case 2: when the refname < 8 columns wide
Both would give: upto 7 columns of refname + space + next item.

Thanks for the suggestions :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 05/10] ref-filter: add option to match literal pattern

[Karthik Nayak]

On Sat, Jul 11, 2015 at 2:56 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Fri, Jul 10, 2015 at 10:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> Since 'ref-filter' only has an option to match path names
>>>> add an option for regular pattern matching.
>>>
>>> There is nothing "regular" about the pattern matching you are
>>> adding.
>>>
>>> Everywhere else we use patterns on refs we call wildmatch(), which
>>> is an enhanced implementation of fnmatch(3), and you are doing the
>>> same in this new codepath.
>>>
>>> Just drop that word from here (and if you said something similar in
>>> the documentation, drop "regular" ffrom there as well).  It would
>>> invite confusion with regular expression matching, which we will not
>>> do for refs.
>>
>> Ok, will do. Thanks
>
> Just dropping "regular" leads to a strange sentence, since the path name
> match is also a kind of pattern-matching. I'd write
>
> Since 'ref-filter' only has an option to match path names, add an option
> for plain fnmatch pattern-matching.
>

Thanks for the heads up :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 01/10] ref-filter: add %(refname:shortalign=X) option

[Duy Nguyen]

On Sat, Jul 11, 2015 at 7:05 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Fri, Jul 10, 2015 at 9:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> This may be enough to support the various existing formats that are
>> offered by "git branch" and/or "git tag", but I do not think if this
>> is the right approach in the longer term, or if we are painting
>> ourselves in a corner we cannot cleanly get out of later [*1*].
>> Will the "refname" stay to be the only thing that may want alignment
>> padding appended in the future?  Will it stay true that we want to
>> align only to the left?  Etc., etc.
>>
>> Cc'ed Duy as %< in the pretty-format was his invention at around
>> a5752342 (pretty: support padding placeholders, %< %> and %><,
>> 2013-04-19).
>>
>
> I kinda had the same though, my only justification was that it was only being
> internally used. I'll have another look if as to see if I can make it
> universal somehow.
> Let's see what Duy has to suggest.

I guess if you can have multiple arguments after ':' in an atom, then
you have wiggle room for future. But it looks like you only accept one
argument after ':'.. (I only checked the version on 'pu'). Having an
"alignment atom" to augment the real one (like %< changes the behavior
of the next placeholder), could also work, but it adds dependency
between atoms, something I don't think ref-filter.c is ready for.

Another thing, the atom value is also used for sorting. When used for
sorting, I think these padding spaces should not be generated or it
may confuse the sort algorithm. Left alignment may be ok, right or
center alignment (in future?), not  so much. Perhaps we should do the
padding in a separate phase, outside populate_value(). If you go this
route, having separate atoms for alignment works better: you don't
have to parse them in populate_value() which is for actual values, and
you can handle dependency easily (I think).

By the way, please consider adding _() back to translatable strings,
usually those die() or warn(), or "[ahead %s]"... In the last case,
because you don't really know how long the string is after
translation, avoid hard coding buffer size (to 40).
-- 
Duy

Re: [PATCH v2 01/10] ref-filter: add %(refname:shortalign=X) option

[Duy Nguyen]

On Sun, Jul 12, 2015 at 8:47 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> Another thing, the atom value is also used for sorting. When used for
> sorting, I think these padding spaces should not be generated or it
> may confuse the sort algorithm. Left alignment may be ok, right or
> center alignment (in future?), not  so much.\

Correction: I'm wrong. This is only true if the atom for sorting is in
the same atom list for display. But we create new atoms for sorting in
parse_opt_ref_sorting(), where we drop everything after ':', so
"shortalign=" shouldn't be there when populate_atom() for sorting is
called.
-- 
Duy

Re: [PATCH v2 08/10] tag.c: use 'ref-filter' APIs

[Duy Nguyen]

On Thu, Jul 9, 2015 at 5:58 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> -static int show_reference(const char *refname, const struct object_id *oid,
> -                         int flag, void *cb_data)
> -{
...
> -
> -       if (match_pattern(filter->name_patterns, refname)) {
....
> -               printf("%-15s ", refname);
> -               show_tag_lines(oid, filter->lines);
> -               putchar('\n');
> -       }
> -
> -       return 0;
> -}
...
> +       if (filter->lines)
> +               format = "%(refname:shortalign=16)";
> +       else
> +               format = "%(refname:short)";

I can see this is a faithful conversion, but this looks line an
opportunity to avoid this special limit 15/16. Even on git.git "git
tag -l -n1" already breaks alignment with *.msysgit.* tags (ok maybe
msysgit, not purely git.git) When you get to "branch -l", it
calculates the max-width automatically so you probably need
"%(refname:shortalign)" any way. "shortalign" (i.e. create the "align"
version for every modifier) does not look good because it could double
the number of modifiers and let's not thinking about truncation
options or right alignment..
-- 
Duy

Re: [PATCH v2 06/10] Documentation/tag: remove double occurance of "<pattern>"

[Karthik Nayak]

On Fri, Jul 10, 2015 at 10:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> On Thu, Jul 9, 2015 at 12:27 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> 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 | 1 -
>>>  1 file changed, 1 deletion(-)
>>>
>>> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
>>> index 034d10d..4b04c2b 100644
>>> --- a/Documentation/git-tag.txt
>>> +++ b/Documentation/git-tag.txt
>>> @@ -14,7 +14,6 @@ SYNOPSIS
>>>  'git tag' -d <tagname>...
>>>  'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
>>>         [--column[=<options>] | --no-column] [<pattern>...]
>>> -       [<pattern>...]
>>>  'git tag' -v <tagname>...
>>
>> As this patch could be applied directly to master and to maint maybe
>> you could send it at the top of this patch series or alone outside of
>> this patch series.
>
> Thanks. I'll pick this patch from the remainder of the series and
> queue it on a separate topic.

That'll be awesome :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 08/10] tag.c: use 'ref-filter' APIs

[Karthik Nayak]

On Sun, Jul 12, 2015 at 3:15 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Thu, Jul 9, 2015 at 5:58 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> -static int show_reference(const char *refname, const struct object_id *oid,
>> -                         int flag, void *cb_data)
>> -{
> ...
>> -
>> -       if (match_pattern(filter->name_patterns, refname)) {
> ....
>> -               printf("%-15s ", refname);
>> -               show_tag_lines(oid, filter->lines);
>> -               putchar('\n');
>> -       }
>> -
>> -       return 0;
>> -}
> ...
>> +       if (filter->lines)
>> +               format = "%(refname:shortalign=16)";
>> +       else
>> +               format = "%(refname:short)";
>
> I can see this is a faithful conversion, but this looks line an
> opportunity to avoid this special limit 15/16. Even on git.git "git
> tag -l -n1" already breaks alignment with *.msysgit.* tags (ok maybe
> msysgit, not purely git.git) When you get to "branch -l", it
> calculates the max-width automatically so you probably need
> "%(refname:shortalign)" any way. "shortalign" (i.e. create the "align"
> version for every modifier) does not look good because it could double
> the number of modifiers and let's not thinking about truncation
> options or right alignment..

What I was thinking of was getting rid of the whole "align" feature where
you provide a value to which it would align.

Something like:  --format="%(item:modifieralign)" which would use something
on the lines of what the max-width calculator in branch -l uses, to get the max
alignment size. But the problem is that ref-filter goes through the refs using
a function which has no connections with the atoms used. So a more practical
solution would be --format="%(item:modifieralign=X)" where we could provide a
means of calculating X via ref-filter. Something like this in tag.c:

int max_width = get_max_width("<item to get max_width of>");
use this max_width to then do a
--format="%(item:modifieralign=X)", where X = max_width

What do you think?

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 01/10] ref-filter: add %(refname:shortalign=X) option

[Karthik Nayak]

On Sun, Jul 12, 2015 at 7:17 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>
> I guess if you can have multiple arguments after ':' in an atom, then
> you have wiggle room for future. But it looks like you only accept one
> argument after ':'.. (I only checked the version on 'pu'). Having an
> "alignment atom" to augment the real one (like %< changes the behavior
> of the next placeholder), could also work, but it adds dependency
> between atoms, something I don't think ref-filter.c is ready for.
>

I was thinking of something on the lines of having a function which right
before printing checks if any "align" option is given to the end of a given
item and aligns it accordingly, this ensures that any item which needs to
have such an option can easily do so.

https://github.com/KarthikNayak/git/commit/0284320483d6442a6425fc665e740f9f975654a1

This is what I came up with, you could have a look and let me know if
you have any
suggestions.

> Another thing, the atom value is also used for sorting. When used for
> sorting, I think these padding spaces should not be generated or it
> may confuse the sort algorithm. Left alignment may be ok, right or
> center alignment (in future?), not  so much. Perhaps we should do the
> padding in a separate phase, outside populate_value(). If you go this
> route, having separate atoms for alignment works better: you don't
> have to parse them in populate_value() which is for actual values, and
> you can handle dependency easily (I think).

This was cleared out in your other reply.

>
> By the way, please consider adding _() back to translatable strings,
> usually those die() or warn(), or "[ahead %s]"... In the last case,
> because you don't really know how long the string is after
> translation, avoid hard coding buffer size (to 40).

Yes, sure, maybe a cleanup patch at the end to do all of cleanup any
such use cases :)

Thanks for all the suggestions.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 08/10] tag.c: use 'ref-filter' APIs

[Duy Nguyen]

On Mon, Jul 13, 2015 at 2:36 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> What I was thinking of was getting rid of the whole "align" feature where
> you provide a value to which it would align.
>
> Something like:  --format="%(item:modifieralign)" which would use something
> on the lines of what the max-width calculator in branch -l uses, to get the max
> alignment size. But the problem is that ref-filter goes through the refs using
> a function which has no connections with the atoms used. So a more practical
> solution would be --format="%(item:modifieralign=X)" where we could provide a
> means of calculating X via ref-filter. Something like this in tag.c:
>
> int max_width = get_max_width("<item to get max_width of>");
> use this max_width to then do a
> --format="%(item:modifieralign=X)", where X = max_width
>
> What do you think?

This is where separate "alignment atoms" (instead of alignment
modifiers) make sense. Suppose you introduce another function, let's
say print_all() for now, to wrap the "for (i < maxcount)" loop at the
end of for-each-ref, you would have total control over display and
formatting. populate_value() generates empty strings for these
alignment atoms (because they don't really have true values). Those
alignment atoms are recognized in print_all() and
show_ref_array_item(). In print_all(), if it sees max width needs to
be calculated (because the user does not specify the width), it can
call populate_value() for an atom for all rows. show_ref_array_item()
does the padding and even truncating if needed. This pattern is
similar to how print_columns() works, first we collect data of the
whole "table", then we place them line by line.

It sounds good to me. But it may not be the best option, I don't know.
And it may create unnecessary work. So you and your mentors decide.
-- 
Duy

Re: [PATCH v2 01/10] ref-filter: add %(refname:shortalign=X) option

[Duy Nguyen]

On Mon, Jul 13, 2015 at 2:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Sun, Jul 12, 2015 at 7:17 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>>
>> I guess if you can have multiple arguments after ':' in an atom, then
>> you have wiggle room for future. But it looks like you only accept one
>> argument after ':'.. (I only checked the version on 'pu'). Having an
>> "alignment atom" to augment the real one (like %< changes the behavior
>> of the next placeholder), could also work, but it adds dependency
>> between atoms, something I don't think ref-filter.c is ready for.
>>
>
> I was thinking of something on the lines of having a function which right
> before printing checks if any "align" option is given to the end of a given
> item and aligns it accordingly, this ensures that any item which needs to
> have such an option can easily do so.
>
> https://github.com/KarthikNayak/git/commit/0284320483d6442a6425fc665e740f9f975654a1
>
> This is what I came up with, you could have a look and let me know if
> you have any
> suggestions.

Yeah, pretty close to what I described in the other mail. Now if you
make "align" a separate atom, I think it would reduce clutter in
populate_value() (my personal opinion is this function looks too messy
already) and we can easily add more alignment options in future :)
-- 
Duy

Re: [PATCH v2 08/10] tag.c: use 'ref-filter' APIs

[Karthik Nayak]

On Mon, Jul 13, 2015 at 4:16 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Mon, Jul 13, 2015 at 2:36 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> What I was thinking of was getting rid of the whole "align" feature where
>> you provide a value to which it would align.
>>
>> Something like:  --format="%(item:modifieralign)" which would use something
>> on the lines of what the max-width calculator in branch -l uses, to get the max
>> alignment size. But the problem is that ref-filter goes through the refs using
>> a function which has no connections with the atoms used. So a more practical
>> solution would be --format="%(item:modifieralign=X)" where we could provide a
>> means of calculating X via ref-filter. Something like this in tag.c:
>>
>> int max_width = get_max_width("<item to get max_width of>");
>> use this max_width to then do a
>> --format="%(item:modifieralign=X)", where X = max_width
>>
>> What do you think?
>
> This is where separate "alignment atoms" (instead of alignment
> modifiers) make sense. Suppose you introduce another function, let's
> say print_all() for now, to wrap the "for (i < maxcount)" loop at the
> end of for-each-ref, you would have total control over display and
> formatting. populate_value() generates empty strings for these
> alignment atoms (because they don't really have true values). Those
> alignment atoms are recognized in print_all() and
> show_ref_array_item(). In print_all(), if it sees max width needs to
> be calculated (because the user does not specify the width), it can
> call populate_value() for an atom for all rows. show_ref_array_item()
> does the padding and even truncating if needed. This pattern is
> similar to how print_columns() works, first we collect data of the
> whole "table", then we place them line by line.
>
> It sounds good to me. But it may not be the best option, I don't know.
> And it may create unnecessary work. So you and your mentors decide.
> --
> Duy

Sounds good, but what you're saying goes on the lines of interdependence of
atoms, since we would have separate atoms. Not sure we want to do that right
now.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 01/10] ref-filter: add %(refname:shortalign=X) option

[Karthik Nayak]

On Mon, Jul 13, 2015 at 4:21 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Mon, Jul 13, 2015 at 2:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Sun, Jul 12, 2015 at 7:17 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>>>
>>> I guess if you can have multiple arguments after ':' in an atom, then
>>> you have wiggle room for future. But it looks like you only accept one
>>> argument after ':'.. (I only checked the version on 'pu'). Having an
>>> "alignment atom" to augment the real one (like %< changes the behavior
>>> of the next placeholder), could also work, but it adds dependency
>>> between atoms, something I don't think ref-filter.c is ready for.
>>>
>>
>> I was thinking of something on the lines of having a function which right
>> before printing checks if any "align" option is given to the end of a given
>> item and aligns it accordingly, this ensures that any item which needs to
>> have such an option can easily do so.
>>
>> https://github.com/KarthikNayak/git/commit/0284320483d6442a6425fc665e740f9f975654a1
>>
>> This is what I came up with, you could have a look and let me know if
>> you have any
>> suggestions.
>
> Yeah, pretty close to what I described in the other mail. Now if you
> make "align" a separate atom, I think it would reduce clutter in
> populate_value() (my personal opinion is this function looks too messy
> already) and we can easily add more alignment options in future :)
> --
> Duy

Yeah, that seems like the way to go, eventually :)

-- 
Regards,
Karthik Nayak