[PATCH v3 0/9] Port tag.c to use ref-filter APIs

[PATCH v3 0/9] 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: Git (next)
https://github.com/git/git/commit/bf5418f49ff0cebc6e5ce04ad1417e1a47c81b61

Version 2 can be found here:
http://article.gmane.org/gmane.comp.version-control.git/273569

Changes in this version:
* Grammatical errors were corrected.
* Added missing documentation and tests for "%(version:refname)"
* Made the alignment option an atom as per Duy's Suggestions
  (http://article.gmane.org/gmane.comp.version-control.git/273569)

DiffStat:
Documentation/git-for-each-ref.txt |   2 +
Documentation/git-tag.txt          |  38 ++++++++---
builtin/for-each-ref.c             |   3 +-
builtin/tag.c                      | 371 +++++++++++++++++++----------------------------------------------------------------------------------------
ref-filter.c                       | 148 ++++++++++++++++++++++++++++++++++++++-----
ref-filter.h                       |  15 +++--
t/t6302-for-each-ref-filter.sh     |  26 ++++++++
t/t7004-tag.sh                     |  51 ++++++++++++---


[PATCH v3 1/9] ref-filter: add option to align atoms to the left
[PATCH v3 2/9] ref-filter: add option to filter only tags
[PATCH v3 3/9] ref-filter: support printing N lines from tag
[PATCH v3 4/9] ref-filter: add support to sort by version
[PATCH v3 5/9] ref-filter: add option to match literal pattern
[PATCH v3 6/9] tag.c: use 'ref-filter' data structures
[PATCH v3 7/9] tag.c: use 'ref-filter' APIs
[PATCH v3 8/9] tag.c: implement '--format' option
[PATCH v3 9/9] tag.c: implement '--merged' and '--no-merged' options

[PATCH v3 1/9] ref-filter: add option to align atoms to the left

[Karthik Nayak]

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

Add a new atom "align" and support %(align:X) where X is a number.
This will align the preceeding atom value to the left followed by
spaces for a total length of X characters. If X is less than the item
size, the entire atom value is printed.

Helped-by: Duy Nguyen <pclouds@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c | 39 +++++++++++++++++++++++++++++++++++++--
 ref-filter.h |  1 +
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 7561727..b81a08d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -10,6 +10,8 @@
 #include "quote.h"
 #include "ref-filter.h"
 #include "revision.h"
+#include "utf8.h"
+#include "git-compat-util.h"
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
@@ -53,6 +55,7 @@ static struct {
 	{ "flag" },
 	{ "HEAD" },
 	{ "color" },
+	{ "align" },
 };
 
 /*
@@ -620,7 +623,7 @@ static void populate_value(struct ref_array_item *ref)
 		const char *name = used_atom[i];
 		struct atom_value *v = &ref->value[i];
 		int deref = 0;
-		const char *refname;
+		const char *refname = NULL;
 		const char *formatp;
 		struct branch *branch = NULL;
 
@@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)
 			else
 				v->s = " ";
 			continue;
+		} else if (starts_with(name, "align:")) {
+			const char *valp = NULL;
+
+			skip_prefix(name, "align:", &valp);
+			if (!valp[0])
+				die(_("No value given with 'align='"));
+			strtoul_ui(valp, 10, &ref->align_value);
+			if (ref->align_value < 1)
+				die(_("Value should be greater than zero"));
+			v->s = "";
+			continue;
 		} else
 			continue;
 
@@ -1254,17 +1268,38 @@ static void emit(const char *cp, const char *ep)
 	}
 }
 
+static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)
+{
+	if (v->s[0] && ref->align_value) {
+		unsigned int len = 0;
+
+		len = utf8_strwidth(v->s);
+		if (ref->align_value > len) {
+			struct strbuf buf = STRBUF_INIT;
+			strbuf_addstr(&buf, v->s);
+			if (!v->s[0])
+				free((char *)v->s);
+			strbuf_addchars(&buf, ' ', ref->align_value - len);
+			v->s = strbuf_detach(&buf, NULL);
+		}
+		ref->align_value = 0;
+	}
+}
+
 void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
 {
 	const char *cp, *sp, *ep;
 
 	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
 		struct atom_value *atomv;
+		int parsed_atom;
 
 		ep = strchr(sp, ')');
 		if (cp < sp)
 			emit(cp, sp);
-		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
+		parsed_atom = parse_ref_filter_atom(sp + 2, ep);
+		get_ref_atom_value(info, parsed_atom, &atomv);
+		assign_formating(info, parsed_atom, atomv);
 		print_value(atomv, quote_style);
 	}
 	if (*cp) {
diff --git a/ref-filter.h b/ref-filter.h
index 6bf27d8..12ffbc5 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -30,6 +30,7 @@ struct ref_sorting {
 struct ref_array_item {
 	unsigned char objectname[20];
 	int flag;
+	unsigned int align_value;
 	const char *symref;
 	struct commit *commit;
 	struct atom_value *value;
-- 
2.4.6

[PATCH v3 2/9] ref-filter: add option to filter only tags

[Karthik Nayak]

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

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 b81a08d..771c48d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1149,6 +1149,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 12ffbc5..c18781b 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.6

[PATCH v3 3/9] ref-filter: support printing N lines from tag annotation

[Karthik Nayak]

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

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

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/for-each-ref.c |  2 +-
 builtin/tag.c          |  4 ++++
 ref-filter.c           | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 ref-filter.h           |  8 ++++++--
 4 files changed, 58 insertions(+), 4 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..10b11ce 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -185,6 +185,10 @@ static enum contains_result contains(struct commit *candidate,
 	return contains_test(candidate, want);
 }
 
+/*
+ * Currently duplicated in ref-filter, will eventually be removed as
+ * we port tag.c to use ref-filter APIs.
+ */
 static void show_tag_lines(const struct object_id *oid, int lines)
 {
 	int i;
diff --git a/ref-filter.c b/ref-filter.c
index 771c48d..82731ac 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1288,7 +1288,48 @@ static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct
 	}
 }
 
-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;
 
@@ -1317,6 +1358,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 c18781b..7dfdea0 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -56,6 +56,7 @@ struct ref_filter {
 	struct commit *merge_commit;
 
 	unsigned int with_commit_tag_algo : 1;
+	unsigned int lines;
 };
 
 struct ref_filter_cbdata {
@@ -87,8 +88,11 @@ int parse_ref_filter_atom(const char *atom, const char *ep);
 int verify_ref_format(const char *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
-/*  Print the ref using the given format and quote_style */
-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);
+/*
+ * Print the ref using the given format and quote_style. If lines > 0,
+ * prints the "lines" no of lines of the objected pointed to.
+ */
+void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style, unsigned int lines);
 /*  Callback function for parsing the sort option */
 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);
 /*  Default sort option based on refname */
-- 
2.4.6

[PATCH v3 4/9] ref-filter: add support to sort by version

[Karthik Nayak]

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

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

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

Add documentation and tests for the same.

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

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index e49d578..cc91275 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -144,6 +144,8 @@ blank line.  Finally, the optional GPG signature is `contents:signature`.
 For sorting purposes, fields with numeric values sort in numeric
 order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).
 All other fields are used to sort in their byte-value order.
+There is also an option to sort by versions, this is done using the
+field names ('version:refname' or 'v:refname').
 
 In any case, a field name that refers to a field inapplicable to
 the object referred by the ref does not cause an error.  It
diff --git a/ref-filter.c b/ref-filter.c
index 82731ac..85c561e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -12,6 +12,7 @@
 #include "revision.h"
 #include "utf8.h"
 #include "git-compat-util.h"
+#include "version.h"
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
@@ -1169,18 +1170,22 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 
 	get_ref_atom_value(a, s->atom, &va);
 	get_ref_atom_value(b, s->atom, &vb);
-	switch (cmp_type) {
-	case FIELD_STR:
-		cmp = strcmp(va->s, vb->s);
-		break;
-	default:
-		if (va->ul < vb->ul)
-			cmp = -1;
-		else if (va->ul == vb->ul)
-			cmp = 0;
-		else
-			cmp = 1;
-		break;
+	if (s->version)
+		cmp = versioncmp(va->s, vb->s);
+	else {
+		switch (cmp_type) {
+		case FIELD_STR:
+			cmp = strcmp(va->s, vb->s);
+			break;
+		default:
+			if (va->ul < vb->ul)
+				cmp = -1;
+			else if (va->ul == vb->ul)
+				cmp = 0;
+			else
+				cmp = 1;
+			break;
+		}
 	}
 	return (s->reverse) ? -cmp : cmp;
 }
@@ -1395,6 +1400,9 @@ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
 		s->reverse = 1;
 		arg++;
 	}
+	if (skip_prefix(arg, "version:", &arg) ||
+	    skip_prefix(arg, "v:", &arg))
+		s->version = 1;
 	len = strlen(arg);
 	s->atom = parse_ref_filter_atom(arg, arg+len);
 	return 0;
diff --git a/ref-filter.h b/ref-filter.h
index 7dfdea0..6f1646b 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -25,7 +25,7 @@ struct atom_value {
 struct ref_sorting {
 	struct ref_sorting *next;
 	int atom; /* index into used_atom array (internal) */
-	unsigned reverse : 1;
+	unsigned reverse : 1, version : 1;
 };
 
 struct ref_array_item {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 505a360..c31fd2f 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -81,4 +81,30 @@ test_expect_success 'filtering with --contains' '
 	test_cmp expect actual
 '
 
+test_expect_success 'setup for version sort' '
+	test_commit foo1.3 &&
+	test_commit foo1.6 &&
+	test_commit foo1.10
+'
+
+test_expect_success 'version sort' '
+	git tag -l --sort=version:refname | grep "foo" >actual &&
+	cat >expect <<-\EOF &&
+	foo1.3
+	foo1.6
+	foo1.10
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'reverse version sort' '
+	git tag -l --sort=-version:refname | grep "foo" >actual &&
+	cat >expect <<-\EOF &&
+	foo1.10
+	foo1.6
+	foo1.3
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.4.6

[PATCH v3 5/9] ref-filter: add option to match literal pattern

[Karthik Nayak]

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

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

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

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/for-each-ref.c |  1 +
 ref-filter.c           | 27 +++++++++++++++++++++++++--
 ref-filter.h           |  3 ++-
 3 files changed, 28 insertions(+), 3 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 85c561e..7ff3ded 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -943,9 +943,23 @@ 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).
+ * matches "refs/heads/m*", too).
  */
 static int match_name_as_path(const char **pattern, const char *refname)
 {
@@ -966,6 +980,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
@@ -1034,7 +1057,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 6f1646b..687a077 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -55,7 +55,8 @@ struct ref_filter {
 	} merge;
 	struct commit *merge_commit;
 
-	unsigned int with_commit_tag_algo : 1;
+	unsigned int with_commit_tag_algo : 1,
+		match_as_path : 1;
 	unsigned int lines;
 };
 
-- 
2.4.6

[PATCH v3 6/9] tag.c: use 'ref-filter' data structures

[Karthik Nayak]

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

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

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

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

diff --git a/builtin/tag.c b/builtin/tag.c
index 10b11ce..dadc686 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.6

[PATCH v3 7/9] tag.c: use 'ref-filter' APIs

[Karthik Nayak]

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

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

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

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

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

Modify documentation for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-tag.txt |  16 ++-
 builtin/tag.c             | 346 ++++++----------------------------------------
 t/t7004-tag.sh            |   8 +-
 3 files changed, 53 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 dadc686..52a179e 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 duplicated in ref-filter, will eventually be removed as
- * we port tag.c to use ref-filter APIs.
- */
-static void show_tag_lines(const struct object_id *oid, int lines)
-{
-	int i;
-	unsigned long size;
-	enum object_type type;
-	char *buf, *sp, *eol;
-	size_t len;
-
-	buf = read_sha1_file(oid->hash, &type, &size);
-	if (!buf)
-		die_errno("unable to read object %s", oid_to_hex(oid));
-	if (type != OBJ_COMMIT && type != OBJ_TAG)
-		goto free_return;
-	if (!size)
-		die("an empty %s object %s?",
-		    typename(type), oid_to_hex(oid));
-
-	/* skip header */
-	sp = strstr(buf, "\n\n");
-	if (!sp)
-		goto free_return;
-
-	/* only take up to "lines" lines, and strip the signature from a tag */
-	if (type == OBJ_TAG)
-		size = parse_signature(buf, size);
-	for (i = 0, sp += 2; i < lines && sp < buf + size; i++) {
-		if (i)
-			printf("\n    ");
-		eol = memchr(sp, '\n', size - (sp - buf));
-		len = eol ? eol - sp : size - (sp - buf);
-		fwrite(sp, len, 1, stdout);
-		if (!eol)
-			break;
-		sp = eol + 1;
-	}
-free_return:
-	free(buf);
-}
-
-static void ref_array_append(struct ref_array *array, const char *refname)
-{
-	size_t len = strlen(refname);
-	struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
-	memcpy(ref->refname, refname, len);
-	ref->refname[len] = '\0';
-	REALLOC_ARRAY(array->items, array->nr + 1);
-	array->items[array->nr++] = ref;
-}
-
-static int show_reference(const char *refname, const struct object_id *oid,
-			  int flag, void *cb_data)
-{
-	struct ref_filter_cbdata *data = cb_data;
-	struct ref_array *array = data->array;
-	struct ref_filter *filter = data->filter;
-
-	if (match_pattern(filter->name_patterns, refname)) {
-		if (filter->with_commit) {
-			struct commit *commit;
-
-			commit = lookup_commit_reference_gently(oid->hash, 1);
-			if (!commit)
-				return 0;
-			if (!contains(commit, filter->with_commit))
-				return 0;
-		}
-
-		if (filter->points_at.nr && !match_points_at(refname, oid->hash, &filter->points_at))
-			return 0;
-
-		if (!filter->lines) {
-			if (tag_sort)
-				ref_array_append(array, refname);
-			else
-				printf("%s\n", refname);
-			return 0;
-		}
-		printf("%-15s ", refname);
-		show_tag_lines(oid, filter->lines);
-		putchar('\n');
-	}
-
-	return 0;
-}
-
-static int sort_by_version(const void *a_, const void *b_)
-{
-	const struct ref_array_item *a = *((struct ref_array_item **)a_);
-	const struct ref_array_item *b = *((struct ref_array_item **)b_);
-	return versioncmp(a->refname, b->refname);
-}
-
-static int list_tags(struct ref_filter *filter, int sort)
+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
 {
 	struct ref_array array;
-	struct ref_filter_cbdata data;
+	char *format;
+	int i;
 
 	memset(&array, 0, sizeof(array));
-	data.array = &array;
-	data.filter = filter;
 
 	if (filter->lines == -1)
 		filter->lines = 0;
 
-	for_each_tag_ref(show_reference, &data);
-	if (sort) {
-		int i;
-		if ((sort & SORT_MASK) == VERCMP_SORT)
-			qsort(array.items, array.nr,
-			      sizeof(struct ref_array_item *), sort_by_version);
-		if (sort & REVERSE_SORT)
-			for (i = array.nr - 1; i >= 0; i--)
-				printf("%s\n", array.items[i]->refname);
-		else
-			for (i = 0; i < array.nr; i++)
-				printf("%s\n", array.items[i]->refname);
-		ref_array_clear(&array);
-	}
+	if (filter->lines)
+		format = "%(align:16)%(refname:short)";
+	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,26 @@ static const char tag_template_nocleanup[] =
 	"Lines starting with '%c' will be kept; you may remove them"
 	" yourself if you want to.\n");
 
-/*
- * Parse a sort string, and return 0 if parsed successfully. Will return
- * non-zero when the sort string does not parse into a known type. If var is
- * given, the error message becomes a warning and includes information about
- * the configuration value.
- */
-static int parse_sort_string(const char *var, const char *arg, int *sort)
+/* Parse arg given and add it the ref_sorting array */
+static int parse_sorting_string(const char *arg, struct ref_sorting **sorting_tail)
 {
-	int type = 0, flags = 0;
-
-	if (skip_prefix(arg, "-", &arg))
-		flags |= REVERSE_SORT;
+	struct ref_sorting *s;
+	int len;
 
-	if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
-		type = VERCMP_SORT;
-	else
-		type = STRCMP_SORT;
+	s = xcalloc(1, sizeof(*s));
+	s->next = *sorting_tail;
+	*sorting_tail = s;
 
-	if (strcmp(arg, "refname")) {
-		if (!var)
-			return error(_("unsupported sort specification '%s'"), arg);
-		else {
-			warning(_("unsupported sort specification '%s' in variable '%s'"),
-				var, arg);
-			return -1;
-		}
-	}
+	if (*arg == '-') {
+		s->reverse = 1;
+		arg++;
+ 	}
+	if (skip_prefix(arg, "version:", &arg) ||
+	    skip_prefix(arg, "v:", &arg))
+		s->version = 1;
 
-	*sort = (type | flags);
+	len = strlen(arg);
+	s->atom = parse_ref_filter_atom(arg, arg+len);
 
 	return 0;
 }
@@ -402,11 +149,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 +312,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 +327,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 +353,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
 		OPT_CONTAINS(&filter.with_commit, N_("print only tags that contain the commit")),
 		OPT_WITH(&filter.with_commit, N_("print only tags that contain the commit")),
-		{
-			OPTION_CALLBACK, 0, "sort", &tag_sort, N_("type"), N_("sort tags"),
-			PARSE_OPT_NONEG, parse_opt_sort
-		},
+		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		{
 			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
 			N_("print only tags of the object"), 0, parse_opt_object_name
@@ -622,7 +362,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	git_config(git_tag_config, NULL);
+	git_config(git_tag_config, sorting_tail);
 
 	memset(&opt, 0, sizeof(opt));
 	memset(&filter, 0, sizeof(filter));
@@ -648,6 +388,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			die(_("--column and -n are incompatible"));
 		colopts = 0;
 	}
+	if (!sorting)
+		sorting = ref_default_sorting();
 	if (cmdmode == 'l') {
 		int ret;
 		if (column_active(colopts)) {
@@ -656,10 +398,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			copts.padding = 2;
 			run_column_filter(colopts, &copts);
 		}
-		if (filter.lines != -1 && tag_sort)
-			die(_("--sort and -n are incompatible"));
 		filter.name_patterns = argv;
-		ret = list_tags(&filter, tag_sort);
+		ret = list_tags(&filter, sorting);
 		if (column_active(colopts))
 			stop_column_filter();
 		return ret;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 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.6

[PATCH v3 8/9] tag.c: implement '--format' option

[Karthik Nayak]

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

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

Add tests and documentation for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-tag.txt | 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 52a179e..cae113b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -30,10 +30,9 @@ static const char * const git_tag_usage[] = {
 
 static unsigned int colopts;
 
-static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
 	struct ref_array array;
-	char *format;
 	int i;
 
 	memset(&array, 0, sizeof(array));
@@ -43,7 +42,7 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
 
 	if (filter->lines)
 		format = "%(align:16)%(refname:short)";
-	else
+	else if (!format)
 		format = "%(refname:short)";
 
 	verify_ref_format(format);
@@ -328,6 +327,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct strbuf err = STRBUF_INIT;
 	struct ref_filter filter;
 	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
+	const char *format = NULL;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
@@ -359,6 +359,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
 			N_("print only tags of the object"), 0, parse_opt_object_name
 		},
+		OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
 		OPT_END()
 	};
 
@@ -398,8 +399,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			copts.padding = 2;
 			run_column_filter(colopts, &copts);
 		}
+		if (format && (filter.lines != -1))
+			die(_("--format and -n are incompatible"));
 		filter.name_patterns = argv;
-		ret = list_tags(&filter, sorting);
+		ret = list_tags(&filter, sorting, format);
 		if (column_active(colopts))
 			stop_column_filter();
 		return ret;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 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.6

[PATCH v3 9/9] tag.c: implement '--merged' and '--no-merged' options

[Karthik Nayak]

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

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

Add documentation and tests for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-tag.txt | 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 cae113b..0fa1d31 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
 };
@@ -353,6 +353,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
 		OPT_CONTAINS(&filter.with_commit, N_("print only tags that contain the commit")),
 		OPT_WITH(&filter.with_commit, N_("print only tags that contain the commit")),
+		OPT_MERGED(&filter, N_("print only tags that are merged")),
+		OPT_NO_MERGED(&filter, N_("print only tags that are not merged")),
 		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
 			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		{
@@ -413,6 +415,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		die(_("--contains option is only allowed with -l."));
 	if (filter.points_at.nr)
 		die(_("--points-at option is only allowed with -l."));
+	if (filter.merge_commit)
+		die(_("--merged and --no-merged option are only allowed with -l"));
 	if (cmdmode == 'd')
 		return for_each_tag_name(argv, delete_tag);
 	if (cmdmode == 'v')
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 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.6

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

[Christian Couder]

On Sun, Jul 19, 2015 at 12:00 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Using 'ref-filter' APIs implement the '--merged' and '--no-merged'
> options into 'tag.c'. The '--merged' option lets the user to only
> list tags merged into the named commit. The '--no-merged' option
> lets the user to only list tags not merged into the named commit.
> If no object is provided it assumes HEAD as the object.
>
> Add documentation and tests for the same.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  Documentation/git-tag.txt | 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>...]

Maybe [--[no-]merged [<commit>]] instead of [(--merged | --no-merged)
[<commit>]].

>  '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).

Here also you could write something like:

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

>
>  CONFIGURATION
>  -------------
> diff --git a/builtin/tag.c b/builtin/tag.c
> index cae113b..0fa1d31 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>...]"),

[--[no-]merged [<commit>]] here too.

Thanks,
Christian.

Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left

[Eric Sunshine]

On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Add a new atom "align" and support %(align:X) where X is a number.
> This will align the preceeding atom value to the left followed by
> spaces for a total length of X characters. If X is less than the item
> size, the entire atom value is printed.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 7561727..b81a08d 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -53,6 +55,7 @@ static struct {
>         { "flag" },
>         { "HEAD" },
>         { "color" },
> +       { "align" },

Not a new issue, but some compilers (Solaris?) complain about the
trailing comma.

>  };
>
>  /*
> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)
>                         else
>                                 v->s = " ";
>                         continue;
> +               } else if (starts_with(name, "align:")) {
> +                       const char *valp = NULL;
> +
> +                       skip_prefix(name, "align:", &valp);
> +                       if (!valp[0])
> +                               die(_("No value given with 'align='"));

The parser expects "align:", but the error message talks about
"align=". Also, current trend is to drop capitalization from the error
message.

> +                       strtoul_ui(valp, 10, &ref->align_value);
> +                       if (ref->align_value < 1)
> +                               die(_("Value should be greater than zero"));

Drop capitalization. Also, the user seeing this message won't
necessarily know to which value this refers. Better would be to
provide context ("'align:' value should be..."), and even better would
be to show the actual value at fault:

    die(_("value should be greater than zero: align:%u\n",
        ref_align_value);

or something.

> +                       v->s = "";
> +                       continue;
>                 } else
>                         continue;
>
> @@ -1254,17 +1268,38 @@ static void emit(const char *cp, const char *ep)
>         }
>  }
>
> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)
> +{
> +       if (v->s[0] && ref->align_value) {

Mental note: v->s[0] is not NUL ('\0').

Also, in this code base, this is typically written *v->s rather than v->s[0].

> +               unsigned int len = 0;
> +               len = utf8_strwidth(v->s);

You initialize 'len' to 0 but then immediately re-assign it.

> +               if (ref->align_value > len) {
> +                       struct strbuf buf = STRBUF_INIT;
> +                       strbuf_addstr(&buf, v->s);
> +                       if (!v->s[0])
> +                               free((char *)v->s);

We know from the "mental note" above that v->s[0] is not NUL ('\0'),
so this 'if' statement can never be true, thus is dead code.

> +                       strbuf_addchars(&buf, ' ', ref->align_value - len);
> +                       v->s = strbuf_detach(&buf, NULL);
> +               }
> +               ref->align_value = 0;
> +       }
> +}

Re: [PATCH v3 3/9] ref-filter: support printing N lines from tag annotation

[Eric Sunshine]

On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> 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.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 771c48d..82731ac 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1288,7 +1288,48 @@ static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct
>         }
>  }
>
> -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 */

This is difficult to read and understand. I presume you meant "no." as
shorthand for "number" but dropped the period. Even with the period,
it's still hard to read. Perhaps rewrite it as:

    If 'lines' is greater than 0, print that may lines of...

or something.

> diff --git a/ref-filter.h b/ref-filter.h
> index c18781b..7dfdea0 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -87,8 +88,11 @@ int parse_ref_filter_atom(const char *atom, const char *ep);
>  int verify_ref_format(const char *format);
>  /*  Sort the given ref_array as per the ref_sorting provided */
>  void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
> -/*  Print the ref using the given format and quote_style */
> -void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);
> +/*
> + * Print the ref using the given format and quote_style. If lines > 0,
> + * prints the "lines" no of lines of the objected pointed to.
> + */

Same problem. This literal "no" is quite confusing. Perhaps rewrite as above:

    If lines > 0, print that many lines of...

> +void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style, unsigned int lines);

ref-filter.h seems to be setting a precedent for overlong lines.
Wrapping this manually could improve readability.

>  /*  Callback function for parsing the sort option */
>  int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);
>  /*  Default sort option based on refname */
> --
> 2.4.6

Re: [PATCH v3 4/9] ref-filter: add support to sort by version

[Eric Sunshine]

On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> 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.

To agree with the actual code: s/version_cmp/versioncmp/

> This option is included to support sorting by versions in `git tag -l`
> which will eventaully be ported to use ref-filter APIs.
>
> Add documentation and tests for the same.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index e49d578..cc91275 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -144,6 +144,8 @@ blank line.  Finally, the optional GPG signature is `contents:signature`.
>  For sorting purposes, fields with numeric values sort in numeric
>  order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).
>  All other fields are used to sort in their byte-value order.
> +There is also an option to sort by versions, this is done using the
> +field names ('version:refname' or 'v:refname').

Assuming I'm a reader without prior knowledge, the first question
which pops into my mind is "what's the difference between
'version:refname' and 'v:refname'?" Is one just shorthand for the
other, or is there some subtle difference in behavior, or what? The
documentation should explain this better.

Also, why are there parentheses around 'version:refname' or
'v:refname'? And, you should use backticks rather than apostrophes, as
is done with the other field names.

>  In any case, a field name that refers to a field inapplicable to
>  the object referred by the ref does not cause an error.  It
> diff --git a/ref-filter.c b/ref-filter.c
> index 82731ac..85c561e 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1169,18 +1170,22 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
>
>         get_ref_atom_value(a, s->atom, &va);
>         get_ref_atom_value(b, s->atom, &vb);
> -       switch (cmp_type) {
> -       case FIELD_STR:
> -               cmp = strcmp(va->s, vb->s);
> -               break;
> -       default:
> -               if (va->ul < vb->ul)
> -                       cmp = -1;
> -               else if (va->ul == vb->ul)
> -                       cmp = 0;
> -               else
> -                       cmp = 1;
> -               break;
> +       if (s->version)
> +               cmp = versioncmp(va->s, vb->s);
> +       else {
> +               switch (cmp_type) {
> +               case FIELD_STR:
> +                       cmp = strcmp(va->s, vb->s);
> +                       break;
> +               default:
> +                       if (va->ul < vb->ul)
> +                               cmp = -1;
> +                       else if (va->ul == vb->ul)
> +                               cmp = 0;
> +                       else
> +                               cmp = 1;
> +                       break;
> +               }

The logic might be slightly easier to follow, and give a much less
noisy diff if you rewrite it like this instead:

    if (s->version)
        cmp = versioncmp(va->s, vb->s);
    else if (cmp_type == FIELD_STR)
        cmp = strcmp(va->s, vb->s);
    else {
        if (va->ul < vb->ul)
            cmp = -1;
        else if (va->ul == vb->ul)
            cmp = 0;
        else
            cmp = 1;
    }

Or, if you don't mind a noisy diff, you can outdent the other branches, as well:

    if (s->version)
       cmp = versioncmp(va->s, vb->s);
    else if (cmp_type == FIELD_STR)
       cmp = strcmp(va->s, vb->s);
    else if (va->ul < vb->ul)
       cmp = -1;
    else if (va->ul == vb->ul)
       cmp = 0;
    else
       cmp = 1;

(I rather prefer the latter, despite the noisy diff.)

>         }
>         return (s->reverse) ? -cmp : cmp;
>  }
> diff --git a/ref-filter.h b/ref-filter.h
> index 7dfdea0..6f1646b 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -25,7 +25,7 @@ struct atom_value {
>  struct ref_sorting {
>         struct ref_sorting *next;
>         int atom; /* index into used_atom array (internal) */
> -       unsigned reverse : 1;
> +       unsigned reverse : 1, version : 1;

This is difficult to read. Style elsewhere (if I'm not mistaken) is to
place the declaration on a line by itself.

>  };
>
>  struct ref_array_item {
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 505a360..c31fd2f 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -81,4 +81,30 @@ test_expect_success 'filtering with --contains' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'version sort' '
> +       git tag -l --sort=version:refname | grep "foo" >actual &&
> +       cat >expect <<-\EOF &&
> +       foo1.3
> +       foo1.6
> +       foo1.10
> +       EOF
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'reverse version sort' '
> +       git tag -l --sort=-version:refname | grep "foo" >actual &&

Maybe use 'v:refname' in one of these tests in order to exercise that
alias as well?

> +       cat >expect <<-\EOF &&
> +       foo1.10
> +       foo1.6
> +       foo1.3
> +       EOF
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.4.6

Re: [PATCH v3 5/9] ref-filter: add option to match literal pattern

[Eric Sunshine]

On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Since 'ref-filter' only has an option to match path names add an
> option for plain fnmatch pattern-matching.
>
> This is to support the pattern matching options which are used in `git
> tag -l` and `git branch -l` where we can match patterns like `git tag
> -l foo*` which would match all tags which has a "foo*" pattern.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 85c561e..7ff3ded 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -966,6 +980,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
> @@ -1034,7 +1057,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;

I find it much more difficult to grok the new logic due to
'*filter->name_patterns' having moved into the called function and its
negation inside the function returning 1 which is then negated (again)
upon return here. This sort of twisty logic places a higher cognitive
load on the reader. Retaining the original logic makes the code far
simpler to understand:

    if (*filter->name_patterns &&
        !filter_pattern_match(filter, refname))
        return 0;

although it's a bit less nicely encapsulated, so I dunno...

>         if (filter->points_at.nr && !match_points_at(&filter->points_at, oid->hash, refname))

Re: [PATCH v3 5/9] ref-filter: add option to match literal pattern

[Christian Couder]

On Mon, Jul 20, 2015 at 8:24 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Since 'ref-filter' only has an option to match path names add an
>> option for plain fnmatch pattern-matching.
>>
>> This is to support the pattern matching options which are used in `git
>> tag -l` and `git branch -l` where we can match patterns like `git tag
>> -l foo*` which would match all tags which has a "foo*" pattern.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 85c561e..7ff3ded 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -966,6 +980,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
>> @@ -1034,7 +1057,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;
>
> I find it much more difficult to grok the new logic due to
> '*filter->name_patterns' having moved into the called function and its
> negation inside the function returning 1 which is then negated (again)
> upon return here. This sort of twisty logic places a higher cognitive
> load on the reader. Retaining the original logic makes the code far
> simpler to understand:
>
>     if (*filter->name_patterns &&
>         !filter_pattern_match(filter, refname))
>         return 0;
>
> although it's a bit less nicely encapsulated, so I dunno...

I think a comment before filter_pattern_match() and perhaps also one
inside it might help. For example something like:

/* Return 1 if the refname matches one of the patterns, otherwise 0. */
static int filter_pattern_match(struct ref_filter *filter, const char *refname)
{
       if (!*filter->name_patterns)
               return 1; /* No pattern always matches */
       if (filter->match_as_path)
               return match_name_as_path(filter->name_patterns, refname);
       return match_pattern(filter->name_patterns, refname);
}

Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left

[Karthik Nayak]

On Mon, Jul 20, 2015 at 5:19 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Add a new atom "align" and support %(align:X) where X is a number.
>> This will align the preceeding atom value to the left followed by
>> spaces for a total length of X characters. If X is less than the item
>> size, the entire atom value is printed.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 7561727..b81a08d 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -53,6 +55,7 @@ static struct {
>>         { "flag" },
>>         { "HEAD" },
>>         { "color" },
>> +       { "align" },
>
> Not a new issue, but some compilers (Solaris?) complain about the
> trailing comma.
>

Ok will check.

>>  };
>>
>>  /*
>> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)
>>                         else
>>                                 v->s = " ";
>>                         continue;
>> +               } else if (starts_with(name, "align:")) {
>> +                       const char *valp = NULL;
>> +
>> +                       skip_prefix(name, "align:", &valp);
>> +                       if (!valp[0])
>> +                               die(_("No value given with 'align='"));
>
> The parser expects "align:", but the error message talks about
> "align=". Also, current trend is to drop capitalization from the error
> message.
>

Thanks will change.

>> +                       strtoul_ui(valp, 10, &ref->align_value);
>> +                       if (ref->align_value < 1)
>> +                               die(_("Value should be greater than zero"));
>
> Drop capitalization. Also, the user seeing this message won't
> necessarily know to which value this refers. Better would be to
> provide context ("'align:' value should be..."), and even better would
> be to show the actual value at fault:
>
>     die(_("value should be greater than zero: align:%u\n",
>         ref_align_value);
>
> or something.

Makes sense, thanks :)

>
>> +                       v->s = "";
>> +                       continue;
>>                 } else
>>                         continue;
>>
>> @@ -1254,17 +1268,38 @@ static void emit(const char *cp, const char *ep)
>>         }
>>  }
>>
>> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)
>> +{
>> +       if (v->s[0] && ref->align_value) {
>
> Mental note: v->s[0] is not NUL ('\0').
>
> Also, in this code base, this is typically written *v->s rather than v->s[0].
>

My bad, got confused with that :)

>> +               unsigned int len = 0;
>> +               len = utf8_strwidth(v->s);
>
> You initialize 'len' to 0 but then immediately re-assign it.

Will change.

>
>> +               if (ref->align_value > len) {
>> +                       struct strbuf buf = STRBUF_INIT;
>> +                       strbuf_addstr(&buf, v->s);
>> +                       if (!v->s[0])
>> +                               free((char *)v->s);
>
> We know from the "mental note" above that v->s[0] is not NUL ('\0'),
> so this 'if' statement can never be true, thus is dead code.

Yes, my bad. Will change.

>
>> +                       strbuf_addchars(&buf, ' ', ref->align_value - len);
>> +                       v->s = strbuf_detach(&buf, NULL);
>> +               }
>> +               ref->align_value = 0;
>> +       }
>> +}

Thanks.

-- 
Regards,
Karthik Nayak

Re: [PATCH v3 3/9] ref-filter: support printing N lines from tag annotation

[Karthik Nayak]

On Mon, Jul 20, 2015 at 5:32 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> 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.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 771c48d..82731ac 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1288,7 +1288,48 @@ static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct
>>         }
>>  }
>>
>> -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 */
>
> This is difficult to read and understand. I presume you meant "no." as
> shorthand for "number" but dropped the period. Even with the period,
> it's still hard to read. Perhaps rewrite it as:
>
>     If 'lines' is greater than 0, print that may lines of...
>
> or something.
>

Ah okay :) Will change it to a better explanation.

>> diff --git a/ref-filter.h b/ref-filter.h
>> index c18781b..7dfdea0 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -87,8 +88,11 @@ int parse_ref_filter_atom(const char *atom, const char *ep);
>>  int verify_ref_format(const char *format);
>>  /*  Sort the given ref_array as per the ref_sorting provided */
>>  void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
>> -/*  Print the ref using the given format and quote_style */
>> -void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);
>> +/*
>> + * Print the ref using the given format and quote_style. If lines > 0,
>> + * prints the "lines" no of lines of the objected pointed to.
>> + */
>
> Same problem. This literal "no" is quite confusing. Perhaps rewrite as above:
>
>     If lines > 0, print that many lines of...

Will change this too.

>
>> +void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style, unsigned int lines);
>
> ref-filter.h seems to be setting a precedent for overlong lines.
> Wrapping this manually could improve readability.
>

Definitely!

Thanks for all the suggestions.

-- 
Regards,
Karthik Nayak

Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Add a new atom "align" and support %(align:X) where X is a number.
>> This will align the preceeding atom value to the left followed by
>> spaces for a total length of X characters. If X is less than the item
>> size, the entire atom value is printed.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 7561727..b81a08d 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -53,6 +55,7 @@ static struct {
>>         { "flag" },
>>         { "HEAD" },
>>         { "color" },
>> +       { "align" },
>
> Not a new issue, but some compilers (Solaris?) complain about the
> trailing comma.

Hmm, are you sure?  I thought we avoid trailing comma for enum
definitions, but not a list of values like this.

Re: [PATCH v3 4/9] ref-filter: add support to sort by version

[Karthik Nayak]

On Mon, Jul 20, 2015 at 7:09 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> To agree with the actual code: s/version_cmp/versioncmp/
>

Yeah! will change.

>
> Assuming I'm a reader without prior knowledge, the first question
> which pops into my mind is "what's the difference between
> 'version:refname' and 'v:refname'?" Is one just shorthand for the
> other, or is there some subtle difference in behavior, or what? The
> documentation should explain this better.
>

Will include more explanation.

> Also, why are there parentheses around 'version:refname' or
> 'v:refname'? And, you should use backticks rather than apostrophes, as
> is done with the other field names.
>

Wanted to show that they are the same command. Seems confusing now that
you mentioned, will change it.

>>  In any case, a field name that refers to a field inapplicable to
>>  the object referred by the ref does not cause an error.  It
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 82731ac..85c561e 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1169,18 +1170,22 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
>>
>>         get_ref_atom_value(a, s->atom, &va);
>>         get_ref_atom_value(b, s->atom, &vb);
>> -       switch (cmp_type) {
>> -       case FIELD_STR:
>> -               cmp = strcmp(va->s, vb->s);
>> -               break;
>> -       default:
>> -               if (va->ul < vb->ul)
>> -                       cmp = -1;
>> -               else if (va->ul == vb->ul)
>> -                       cmp = 0;
>> -               else
>> -                       cmp = 1;
>> -               break;
>> +       if (s->version)
>> +               cmp = versioncmp(va->s, vb->s);
>> +       else {
>> +               switch (cmp_type) {
>> +               case FIELD_STR:
>> +                       cmp = strcmp(va->s, vb->s);
>> +                       break;
>> +               default:
>> +                       if (va->ul < vb->ul)
>> +                               cmp = -1;
>> +                       else if (va->ul == vb->ul)
>> +                               cmp = 0;
>> +                       else
>> +                               cmp = 1;
>> +                       break;
>> +               }
>
> The logic might be slightly easier to follow, and give a much less
> noisy diff if you rewrite it like this instead:
>
>     if (s->version)
>         cmp = versioncmp(va->s, vb->s);
>     else if (cmp_type == FIELD_STR)
>         cmp = strcmp(va->s, vb->s);
>     else {
>         if (va->ul < vb->ul)
>             cmp = -1;
>         else if (va->ul == vb->ul)
>             cmp = 0;
>         else
>             cmp = 1;
>     }
>
> Or, if you don't mind a noisy diff, you can outdent the other branches, as well:
>
>     if (s->version)
>        cmp = versioncmp(va->s, vb->s);
>     else if (cmp_type == FIELD_STR)
>        cmp = strcmp(va->s, vb->s);
>     else if (va->ul < vb->ul)
>        cmp = -1;
>     else if (va->ul == vb->ul)
>        cmp = 0;
>     else
>        cmp = 1;
>
> (I rather prefer the latter, despite the noisy diff.)
>

Err! just didn't want to existing code. Your latter code usage seems
easier and simpler to follow. Will use that thanks :D

>>         }
>>         return (s->reverse) ? -cmp : cmp;
>>  }
>> diff --git a/ref-filter.h b/ref-filter.h
>> index 7dfdea0..6f1646b 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -25,7 +25,7 @@ struct atom_value {
>>  struct ref_sorting {
>>         struct ref_sorting *next;
>>         int atom; /* index into used_atom array (internal) */
>> -       unsigned reverse : 1;
>> +       unsigned reverse : 1, version : 1;
>
> This is difficult to read. Style elsewhere (if I'm not mistaken) is to
> place the declaration on a line by itself.
>

Yes just checked, thats the style. Thank you.

>>  };
>>
>>  struct ref_array_item {
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> index 505a360..c31fd2f 100755
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -81,4 +81,30 @@ test_expect_success 'filtering with --contains' '
>>         test_cmp expect actual
>>  '
>>
>> +test_expect_success 'version sort' '
>> +       git tag -l --sort=version:refname | grep "foo" >actual &&
>> +       cat >expect <<-\EOF &&
>> +       foo1.3
>> +       foo1.6
>> +       foo1.10
>> +       EOF
>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'reverse version sort' '
>> +       git tag -l --sort=-version:refname | grep "foo" >actual &&
>
> Maybe use 'v:refname' in one of these tests in order to exercise that
> alias as well?
>

The idea was to only include a minimal test as t7004 has tests for the same,
but I guess a minimal test for 'v:refname' is also required.

-- 
Regards,
Karthik Nayak

Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left

[Junio C Hamano]

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

> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)
>  			else
>  				v->s = " ";
>  			continue;
> +		} else if (starts_with(name, "align:")) {
> +			const char *valp = NULL;
> +
> +			skip_prefix(name, "align:", &valp);
> +			if (!valp[0])
> +				die(_("No value given with 'align='"));
> +			strtoul_ui(valp, 10, &ref->align_value);
> +			if (ref->align_value < 1)
> +				die(_("Value should be greater than zero"));
> +			v->s = "";
> +			continue;
>  		} else
>  			continue;
>  
> @@ -1254,17 +1268,38 @@ static void emit(const char *cp, const char *ep)
>  	}
>  }
>  
> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)
> +{
> +	if (v->s[0] && ref->align_value) {
> +		unsigned int len = 0;
> +
> +		len = utf8_strwidth(v->s);
> +		if (ref->align_value > len) {
> +			struct strbuf buf = STRBUF_INIT;
> +			strbuf_addstr(&buf, v->s);
> +			if (!v->s[0])
> +				free((char *)v->s);
> +			strbuf_addchars(&buf, ' ', ref->align_value - len);
> +			v->s = strbuf_detach(&buf, NULL);
> +		}
> +		ref->align_value = 0;
> +	}
> +}

What is your plan for this function?  Is it envisioned that this
will gain more variations of formatting options over time?
Otherwise it seems misnamed (it is not "assign formatting" but
merely "pad to the right").

I also doubt that the logic is sane.  More specifically, what does
that "if (v->s[0])" buy you?

Your caller is iterating over the elements in a format string,
e.g. 'A %(align:20)%(foo) B %(bar) C', and its caller is iterating
over a list of refs, e.g. 'maint', 'master' branches.  With that
format string, as long as %(foo) does not expand to something that
exceeds 20 display places or so, I'd expect literal 'B' for all refs
to align, but I do not think this code gives me that; what happens
if '%(foo)' happens to be an empty string for 'maint' but is a
string, say 'x', for 'master'?

Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left

[Karthik Nayak]

>
> What is your plan for this function?  Is it envisioned that this
> will gain more variations of formatting options over time?
> Otherwise it seems misnamed (it is not "assign formatting" but
> merely "pad to the right").
>

I wanna include an 'ifexists' atom in the future where an user can
specify a string format to be printed only if the preceding atom holds
a value.
e.g. '%(ifexists:refname is %s)%(refname)'
So I see that going in here.

> I also doubt that the logic is sane.  More specifically, what does
> that "if (v->s[0])" buy you?

Yeah, that's wrong, like Eric pointed out. Should have been
"if (v->s[0] == '\0')"

>
> Your caller is iterating over the elements in a format string,
> e.g. 'A %(align:20)%(foo) B %(bar) C', and its caller is iterating
> over a list of refs, e.g. 'maint', 'master' branches.  With that
> format string, as long as %(foo) does not expand to something that
> exceeds 20 display places or so, I'd expect literal 'B' for all refs
> to align, but I do not think this code gives me that; what happens
> if '%(foo)' happens to be an empty string for 'maint' but is a
> string, say 'x', for 'master'?

Oh yeah! That doesn't work, I have a fix in mind I'll reply to this
mail with a fix.

-- 
Regards,
Karthik Nayak

[PATCH v3 1/9] ref-filter: add option to align atoms to the left

[Karthik Nayak]

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

Add a new atom "align" and support %(align:X) where X is a number.
This will align the preceeding atom value to the left followed by
spaces for a total length of X characters. If X is less than the item
size, the entire atom value is printed.

Helped-by: Duy Nguyen <pclouds@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c | 41 +++++++++++++++++++++++++++++++++++++++--
 ref-filter.h |  1 +
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 7561727..93f59aa 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -10,6 +10,8 @@
 #include "quote.h"
 #include "ref-filter.h"
 #include "revision.h"
+#include "utf8.h"
+#include "git-compat-util.h"
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
@@ -53,6 +55,7 @@ static struct {
 	{ "flag" },
 	{ "HEAD" },
 	{ "color" },
+	{ "align" },
 };
 
 /*
@@ -620,7 +623,7 @@ static void populate_value(struct ref_array_item *ref)
 		const char *name = used_atom[i];
 		struct atom_value *v = &ref->value[i];
 		int deref = 0;
-		const char *refname;
+		const char *refname = NULL;
 		const char *formatp;
 		struct branch *branch = NULL;
 
@@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)
 			else
 				v->s = " ";
 			continue;
+		} else if (starts_with(name, "align:")) {
+			const char *valp = NULL;
+
+			skip_prefix(name, "align:", &valp);
+			if (!valp[0])
+				die(_("no value given with 'align:'"));
+			strtoul_ui(valp, 10, &ref->align_value);
+			if (ref->align_value < 1)
+				die(_("value should be greater than zero: align:%u"), ref->align_value);
+			v->s = "";
+			continue;
 		} else
 			continue;
 
@@ -1254,17 +1268,40 @@ static void emit(const char *cp, const char *ep)
 	}
 }
 
+static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)
+{
+	if (ref->align_value && !starts_with(used_atom[parsed_atom], "align")) {
+		unsigned int len = 0;
+
+		if (*v->s)
+			len = utf8_strwidth(v->s);
+		if (ref->align_value > len) {
+			struct strbuf buf = STRBUF_INIT;
+			if (*v->s)
+				strbuf_addstr(&buf, v->s);
+			if (*v->s && v->s[0] == '\0')
+				free((char *)v->s);
+			strbuf_addchars(&buf, ' ', ref->align_value - len);
+			v->s = strbuf_detach(&buf, NULL);
+		}
+		ref->align_value = 0;
+	}
+}
+
 void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
 {
 	const char *cp, *sp, *ep;
 
 	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
 		struct atom_value *atomv;
+		int parsed_atom;
 
 		ep = strchr(sp, ')');
 		if (cp < sp)
 			emit(cp, sp);
-		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
+		parsed_atom = parse_ref_filter_atom(sp + 2, ep);
+		get_ref_atom_value(info, parsed_atom, &atomv);
+		assign_formating(info, parsed_atom, atomv);
 		print_value(atomv, quote_style);
 	}
 	if (*cp) {
diff --git a/ref-filter.h b/ref-filter.h
index 6bf27d8..12ffbc5 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -30,6 +30,7 @@ struct ref_sorting {
 struct ref_array_item {
 	unsigned char objectname[20];
 	int flag;
+	unsigned int align_value;
 	const char *symref;
 	struct commit *commit;
 	struct atom_value *value;
-- 
2.4.6

Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left

[Junio C Hamano]

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

> Your caller is iterating over the elements in a format string,
> e.g. 'A %(align:20)%(foo) B %(bar) C', and its caller is iterating
> over a list of refs, e.g. 'maint', 'master' branches.  With that
> format string, as long as %(foo) does not expand to something that
> exceeds 20 display places or so, I'd expect literal 'B' for all refs
> to align, but I do not think this code gives me that; what happens
> if '%(foo)' happens to be an empty string for 'maint' but is a
> string, say 'x', for 'master'?

Having looked at the caller once again, I have to say that the
interface to this function is poorly designed.  'info' might have
been a convenient place to keep the "formatting state" during this
loop (e.g. "was the previous atom tell us to format this atom in a
special way and if so how?"), but that state does not belong to the
'info' thing we are getting from our caller.  It is something we'd
want to clear before we come into the for() loop, and mutate and
utilize while in the loop.  For example, if the caller ever wants
to show the same ref twice by calling this function with the same
ref twice, and if the format string ended with %(align:N), you do
not want that leftover state to right-pad the first atom in the
second invocation.

Imagine that in the future you might want to affect how things are
formatted based on how much we have already output for the ref so
far (e.g. limiting the total line length).  Where would you implement
such a feature and hook it in this loop?

I'd imagine that a sensible way to organize and structure the
codeflow to support this "align" and related enhancement we may want
to have in the future cleanly would be to teach "print_value" about
the "formatting state" and share it with this loop.  Roughly...

>  void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
>  {
>  	const char *cp, *sp, *ep;
>  

Insert something like this here:

	struct ref_formatting_state state;

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

>  	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>  		struct atom_value *atomv;
> +		int parsed_atom;
>  
>  		ep = strchr(sp, ')');
>  		if (cp < sp)
>  			emit(cp, sp);
> -		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
> +		parsed_atom = parse_ref_filter_atom(sp + 2, ep);
> +		get_ref_atom_value(info, parsed_atom, &atomv);
> +		assign_formating(info, parsed_atom, atomv);
>  		print_value(atomv, quote_style);

and replace all of the above with something like this (a separate
variable parsed_atom may not be necessary):

		get_ref_atom_value(&state, info,
				parse_ref_filter_atom(sp + 2, ep), &atomv);
		print_value(&state, atomv);

Things like %(align:20) are not really atoms in the sense that they
are not used as placeholders for attributes that refs being printed
have, but they are there solely in order to affect the "formating
state".  Introduce a new field "struct atom_value.pseudo_atom" to
tell print_value() that fact from get_ref_atom_value(), e.g.

	static void print_value(struct ref_formatting_state *state,
        			struct atom_value *v)
	{
		struct strbuf value = STRBUF_INIT;
		struct strbuf formatted = STRBUF_INIT;

        	if (v->pseudo_atom)
                	return;
		if (state->pad_to_right) {
                	strbuf_addf(&value, "%.*s", state->pad_to_right, v->s);
			state->pad_to_right = 0;
		}
		switch (state->quote_style) {                
		case QUOTE_SHELL:
                	sq_quote_buf(&formatted, value.buf);
                        break;
                        ...
		}
                fputs(formatted.buf, stdout);
                strbuf_release(&value);
                strbuf_release(&formatted);
	}

or something like that.  As this print_value() knows everything that
happens to a single output line during that loop and is allowed to
keep track of what it sees in 'state', this would give a natural and
codeflow to add 'limit the total line length' and things like that
if desired.

We may want to further clean up to update %(color) thing to clarify
that it is a pseudo atom.  I suspect %(align:20)%(color:blue) would
do a wrong thing with the current code, and it would be a reasonable
thing to allow both of these interchangeably:

  %(align:20)%(color:blue)%(refname:short)%(color:reset)
  %(color:blue)%(align:20)%(refname:short)%(color:reset)

and implementation of that would become more obvious once you have a
more explicit "formatting state" that is known to and shared among
get_value(), the for() loop that walks the format string, and
print_value().

Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left

[Eric Sunshine]

On Mon, Jul 20, 2015 at 12:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> Add a new atom "align" and support %(align:X) where X is a number.
>>> This will align the preceeding atom value to the left followed by
>>> spaces for a total length of X characters. If X is less than the item
>>> size, the entire atom value is printed.
>>>
>>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>>> ---
>>> diff --git a/ref-filter.c b/ref-filter.c
>>> index 7561727..b81a08d 100644
>>> --- a/ref-filter.c
>>> +++ b/ref-filter.c
>>> @@ -53,6 +55,7 @@ static struct {
>>>         { "flag" },
>>>         { "HEAD" },
>>>         { "color" },
>>> +       { "align" },
>>
>> Not a new issue, but some compilers (Solaris?) complain about the
>> trailing comma.
>
> Hmm, are you sure?  I thought we avoid trailing comma for enum
> definitions, but not a list of values like this.

It's been years since I encountered such a compiler, so it's possible
that my brain is conflating different cases of trailing commas...

Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left

[Eric Sunshine]

On Mon, Jul 20, 2015 at 1:22 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Add a new atom "align" and support %(align:X) where X is a number.
> This will align the preceeding atom value to the left followed by
> spaces for a total length of X characters. If X is less than the item
> size, the entire atom value is printed.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 7561727..93f59aa 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)
>                         else
>                                 v->s = " ";
>                         continue;
> +               } else if (starts_with(name, "align:")) {
> +                       const char *valp = NULL;
> +
> +                       skip_prefix(name, "align:", &valp);
> +                       if (!valp[0])
> +                               die(_("no value given with 'align:'"));
> +                       strtoul_ui(valp, 10, &ref->align_value);
> +                       if (ref->align_value < 1)
> +                               die(_("value should be greater than zero: align:%u"), ref->align_value);
> +                       v->s = "";

Mental note: v->s points at literal zero-length string ("").

> +                       continue;
>                 } else
>                         continue;
>
> @@ -1254,17 +1268,40 @@ static void emit(const char *cp, const char *ep)
>         }
>  }
>
> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)
> +{
> +       if (ref->align_value && !starts_with(used_atom[parsed_atom], "align")) {
> +               unsigned int len = 0;
> +
> +               if (*v->s)
> +                       len = utf8_strwidth(v->s);
> +               if (ref->align_value > len) {
> +                       struct strbuf buf = STRBUF_INIT;
> +                       if (*v->s)
> +                               strbuf_addstr(&buf, v->s);
> +                       if (*v->s && v->s[0] == '\0')
> +                               free((char *)v->s);

Is the "v->s[0] == '\0'" checking for the same literal zero-length
string assigned above? If so, attempting to free() that string doesn't
make sense, since it's not heap-allocated. Maybe you meant != '\0'?

Overall, this code is getting rather complex and difficult to follow
(especially with all the 'v->s' checks thrown in). Junio's proposed
'pseudo_atom' and 'ref_formatting_state' would go a long way toward
simplifying it.

> +                       strbuf_addchars(&buf, ' ', ref->align_value - len);
> +                       v->s = strbuf_detach(&buf, NULL);
> +               }
> +               ref->align_value = 0;
> +       }
> +}

Re: [PATCH v3 5/9] ref-filter: add option to match literal pattern

[Eric Sunshine]

On Mon, Jul 20, 2015 at 4:01 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Mon, Jul 20, 2015 at 8:24 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> +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);
>>> +}
>>> @@ -1034,7 +1057,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>>> -       if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
>>> +       if (!filter_pattern_match(filter, refname))
>>>                 return 0;
>>
>> I find it much more difficult to grok the new logic due to
>> '*filter->name_patterns' having moved into the called function and its
>> negation inside the function returning 1 which is then negated (again)
>> upon return here. This sort of twisty logic places a higher cognitive
>> load on the reader. Retaining the original logic makes the code far
>> simpler to understand:
>>
>>     if (*filter->name_patterns &&
>>         !filter_pattern_match(filter, refname))
>>         return 0;
>>
>> although it's a bit less nicely encapsulated, so I dunno...
>
> I think a comment before filter_pattern_match() and perhaps also one
> inside it might help. For example something like:
>
> /* Return 1 if the refname matches one of the patterns, otherwise 0. */
> static int filter_pattern_match(struct ref_filter *filter, const char *refname)
> {
>        if (!*filter->name_patterns)
>                return 1; /* No pattern always matches */
>        if (filter->match_as_path)
>                return match_name_as_path(filter->name_patterns, refname);
>        return match_pattern(filter->name_patterns, refname);
> }

Yes, the comments do improve the situation and may be a reasonable compromise...

Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left

[Karthik Nayak]

On Mon, Jul 20, 2015 at 11:31 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Jul 20, 2015 at 1:22 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Add a new atom "align" and support %(align:X) where X is a number.
>> This will align the preceeding atom value to the left followed by
>> spaces for a total length of X characters. If X is less than the item
>> size, the entire atom value is printed.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 7561727..93f59aa 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)
>>                         else
>>                                 v->s = " ";
>>                         continue;
>> +               } else if (starts_with(name, "align:")) {
>> +                       const char *valp = NULL;
>> +
>> +                       skip_prefix(name, "align:", &valp);
>> +                       if (!valp[0])
>> +                               die(_("no value given with 'align:'"));
>> +                       strtoul_ui(valp, 10, &ref->align_value);
>> +                       if (ref->align_value < 1)
>> +                               die(_("value should be greater than zero: align:%u"), ref->align_value);
>> +                       v->s = "";
>
> Mental note: v->s points at literal zero-length string ("").
>
>> +                       continue;
>>                 } else
>>                         continue;
>>
>> @@ -1254,17 +1268,40 @@ static void emit(const char *cp, const char *ep)
>>         }
>>  }
>>
>> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)
>> +{
>> +       if (ref->align_value && !starts_with(used_atom[parsed_atom], "align")) {
>> +               unsigned int len = 0;
>> +
>> +               if (*v->s)
>> +                       len = utf8_strwidth(v->s);
>> +               if (ref->align_value > len) {
>> +                       struct strbuf buf = STRBUF_INIT;
>> +                       if (*v->s)
>> +                               strbuf_addstr(&buf, v->s);
>> +                       if (*v->s && v->s[0] == '\0')
>> +                               free((char *)v->s);
>
> Is the "v->s[0] == '\0'" checking for the same literal zero-length
> string assigned above? If so, attempting to free() that string doesn't
> make sense, since it's not heap-allocated. Maybe you meant != '\0'?
>
> Overall, this code is getting rather complex and difficult to follow
> (especially with all the 'v->s' checks thrown in). Junio's proposed
> 'pseudo_atom' and 'ref_formatting_state' would go a long way toward
> simplifying it.
>

You're right, thats what I meant.
Having a look at Junio's proposed idea.

-- 
Regards,
Karthik Nayak

Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left

[Karthik Nayak]

On Mon, Jul 20, 2015 at 10:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Your caller is iterating over the elements in a format string,
>> e.g. 'A %(align:20)%(foo) B %(bar) C', and its caller is iterating
>> over a list of refs, e.g. 'maint', 'master' branches.  With that
>> format string, as long as %(foo) does not expand to something that
>> exceeds 20 display places or so, I'd expect literal 'B' for all refs
>> to align, but I do not think this code gives me that; what happens
>> if '%(foo)' happens to be an empty string for 'maint' but is a
>> string, say 'x', for 'master'?
>
> Having looked at the caller once again, I have to say that the
> interface to this function is poorly designed.  'info' might have
> been a convenient place to keep the "formatting state" during this
> loop (e.g. "was the previous atom tell us to format this atom in a
> special way and if so how?"), but that state does not belong to the
> 'info' thing we are getting from our caller.  It is something we'd
> want to clear before we come into the for() loop, and mutate and
> utilize while in the loop.  For example, if the caller ever wants
> to show the same ref twice by calling this function with the same
> ref twice, and if the format string ended with %(align:N), you do
> not want that leftover state to right-pad the first atom in the
> second invocation.

You do have a point, my current implementation won't work with the
scenario you just mentioned.

>
> Imagine that in the future you might want to affect how things are
> formatted based on how much we have already output for the ref so
> far (e.g. limiting the total line length).  Where would you implement
> such a feature and hook it in this loop?
>
> I'd imagine that a sensible way to organize and structure the
> codeflow to support this "align" and related enhancement we may want
> to have in the future cleanly would be to teach "print_value" about
> the "formatting state" and share it with this loop.  Roughly...
>
>>  void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
>>  {
>>       const char *cp, *sp, *ep;
>>
>
> Insert something like this here:
>
>         struct ref_formatting_state state;
>
>         memset(&state, 0, sizeof(state));
>         state.quote_style = quote_style;
>
>>       for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>>               struct atom_value *atomv;
>> +             int parsed_atom;
>>
>>               ep = strchr(sp, ')');
>>               if (cp < sp)
>>                       emit(cp, sp);
>> -             get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
>> +             parsed_atom = parse_ref_filter_atom(sp + 2, ep);
>> +             get_ref_atom_value(info, parsed_atom, &atomv);
>> +             assign_formating(info, parsed_atom, atomv);
>>               print_value(atomv, quote_style);
>
> and replace all of the above with something like this (a separate
> variable parsed_atom may not be necessary):
>
>                 get_ref_atom_value(&state, info,
>                                 parse_ref_filter_atom(sp + 2, ep), &atomv);
>                 print_value(&state, atomv);
>
> Things like %(align:20) are not really atoms in the sense that they
> are not used as placeholders for attributes that refs being printed
> have, but they are there solely in order to affect the "formating
> state".  Introduce a new field "struct atom_value.pseudo_atom" to
> tell print_value() that fact from get_ref_atom_value(), e.g.
>
>         static void print_value(struct ref_formatting_state *state,
>                                 struct atom_value *v)
>         {
>                 struct strbuf value = STRBUF_INIT;
>                 struct strbuf formatted = STRBUF_INIT;
>
>                 if (v->pseudo_atom)
>                         return;
>                 if (state->pad_to_right) {
>                         strbuf_addf(&value, "%.*s", state->pad_to_right, v->s);
>                         state->pad_to_right = 0;
>                 }
>                 switch (state->quote_style) {
>                 case QUOTE_SHELL:
>                         sq_quote_buf(&formatted, value.buf);
>                         break;
>                         ...
>                 }
>                 fputs(formatted.buf, stdout);
>                 strbuf_release(&value);
>                 strbuf_release(&formatted);
>         }
>
> or something like that.  As this print_value() knows everything that
> happens to a single output line during that loop and is allowed to
> keep track of what it sees in 'state', this would give a natural and
> codeflow to add 'limit the total line length' and things like that
> if desired.

This implementation looks good. Will include this, thanks a bunch.

>
> We may want to further clean up to update %(color) thing to clarify
> that it is a pseudo atom.  I suspect %(align:20)%(color:blue) would
> do a wrong thing with the current code, and it would be a reasonable
> thing to allow both of these interchangeably:
>
>   %(align:20)%(color:blue)%(refname:short)%(color:reset)
>   %(color:blue)%(align:20)%(refname:short)%(color:reset)
>
> and implementation of that would become more obvious once you have a
> more explicit "formatting state" that is known to and shared among
> get_value(), the for() loop that walks the format string, and
> print_value().

Yup! ill put in a fix for %(color:<color>) with this patch.

Thanks a lot.



-- 
Regards,
Karthik Nayak

Re: [PATCH v3 5/9] ref-filter: add option to match literal pattern

[Karthik Nayak]

On Mon, Jul 20, 2015 at 11:42 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Jul 20, 2015 at 4:01 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> On Mon, Jul 20, 2015 at 8:24 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>> +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);
>>>> +}
>>>> @@ -1034,7 +1057,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>>>> -       if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
>>>> +       if (!filter_pattern_match(filter, refname))
>>>>                 return 0;
>>>
>>> I find it much more difficult to grok the new logic due to
>>> '*filter->name_patterns' having moved into the called function and its
>>> negation inside the function returning 1 which is then negated (again)
>>> upon return here. This sort of twisty logic places a higher cognitive
>>> load on the reader. Retaining the original logic makes the code far
>>> simpler to understand:
>>>
>>>     if (*filter->name_patterns &&
>>>         !filter_pattern_match(filter, refname))
>>>         return 0;
>>>
>>> although it's a bit less nicely encapsulated, so I dunno...
>>
>> I think a comment before filter_pattern_match() and perhaps also one
>> inside it might help. For example something like:
>>
>> /* Return 1 if the refname matches one of the patterns, otherwise 0. */
>> static int filter_pattern_match(struct ref_filter *filter, const char *refname)
>> {
>>        if (!*filter->name_patterns)
>>                return 1; /* No pattern always matches */
>>        if (filter->match_as_path)
>>                return match_name_as_path(filter->name_patterns, refname);
>>        return match_pattern(filter->name_patterns, refname);
>> }
>
> Yes, the comments do improve the situation and may be a reasonable compromise...

Yes, these comments would help, thanks :D

-- 
Regards,
Karthik Nayak

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

[Karthik Nayak]

On Mon, Jul 20, 2015 at 12:50 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Sun, Jul 19, 2015 at 12:00 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Using 'ref-filter' APIs implement the '--merged' and '--no-merged'
>> options into 'tag.c'. The '--merged' option lets the user to only
>> list tags merged into the named commit. The '--no-merged' option
>> lets the user to only list tags not merged into the named commit.
>> If no object is provided it assumes HEAD as the object.
>>
>> Add documentation and tests for the same.
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  Documentation/git-tag.txt | 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>...]
>
> Maybe [--[no-]merged [<commit>]] instead of [(--merged | --no-merged)
> [<commit>]].
>

Looks better. will use.

>>  '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).
>
> Here also you could write something like:
>
> +--[no-]merged [<commit>]::
> +       Only list tags whose tips are reachable, or not reachable
> +       if --no-merged is used, from the specified commit
> +       (HEAD if not specified).
>
>>
>>  CONFIGURATION
>>  -------------
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index cae113b..0fa1d31 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>...]"),
>
> [--[no-]merged [<commit>]] here too.
>
> Thanks,
> Christian.

Here too, thanks :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left

[Matthieu Moy]

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

> +			strtoul_ui(valp, 10, &ref->align_value);
> +			if (ref->align_value < 1)
> +				die(_("Value should be greater than zero"));

You're not checking the return value of strtoul_ui, which returns -1
before assigning align_value if the value can't be parsed. As a result,
you're testing an undefined value in the 'if' statement in this case.

You should test the return value and issue a distinct error message in
this case like

if (strtoul_ui(valp, 10, &ref->align_value))
	die(_("positive integer expected after ':' in align:%u\n",
	    ref_align_value));

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

Re: [PATCH v3 5/9] ref-filter: add option to match literal pattern

[Matthieu Moy]

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

> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -943,9 +943,23 @@ 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

While you're there, why not say explicitly

   * A pattern can be path prefix (e.g. a refname "refs/heads/master"
   * matches a pattern "refs/heads/" but not "refs/heads/m")
                                     ^^^^^^^^^^^^^^^^^^^^^^
   ...

(I find it frustrating when the docstrings for two function look
identical and I have to find out the 1-character difference to
understand ...)

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

Re: [PATCH v3 8/9] tag.c: implement '--format' option

[Matthieu Moy]

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

> --- 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)`.

I think this last sentence is taken from for-each-ref where it is true,
but for 'git tag', the default is just %(refname:short), as written
here:

> -	else
> +	else if (!format)
>  		format = "%(refname:short)";

right?

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

This tests the pattern argument, but the the test still passes if I
remove the --format option, so it does not test what it claims.

Also, why does "git tag"'s %(refname) behave like "git for-each-ref"'s
%(refname:short)? I find it very confusing as I think --format's
argument should be interpreted the same way for all ref-listing
commands. Actually I didn't find a way to have "git tag" display the
full refname other than with --format "refs/tags/%(refname)".

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

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

[Matthieu Moy]

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

> +--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).

You may want to spell it `HEAD` (with backticks).

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

Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left

[Karthik Nayak]

On Thu, Jul 23, 2015 at 12:14 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +                     strtoul_ui(valp, 10, &ref->align_value);
>> +                     if (ref->align_value < 1)
>> +                             die(_("Value should be greater than zero"));
>
> You're not checking the return value of strtoul_ui, which returns -1
> before assigning align_value if the value can't be parsed. As a result,
> you're testing an undefined value in the 'if' statement in this case.
>
> You should test the return value and issue a distinct error message in
> this case like
>
> if (strtoul_ui(valp, 10, &ref->align_value))
>         die(_("positive integer expected after ':' in align:%u\n",
>             ref_align_value));
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

Makes sense, thanks :)

-- 
Regards,
Karthik Nayak

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

[Karthik Nayak]

On Thu, Jul 23, 2015 at 1:10 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +--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).
>
> You may want to spell it `HEAD` (with backticks).
>

Will do!

-- 
Regards,
Karthik Nayak