threads / rfc / 39720

RFC patch, 9 partsport tag.c to use ref-filter library

Subject: [RFC/PATCH 0/9] port tag.c to use ref-filter library

## tl;dr

23 messages between Jun 25, 2015 and Jul 12, 2015. Diffs are folded; open one to read it.

replies: 22people: 4as markdown or json

Karthik Nayak· Jun 25, 2015, 09:55 UTC · lore

This is part of the GSoC project to unify the commands `git tag -l`, `git branch -l` and `git for-each-ref`.

This is a follow up to http://article.gmane.org/gmane.comp.version-control.git/272641 which is still in the 6th iteration.

This ports over tag.c to use ref-filter APIs and adds --format, --sort, --merged and --no-merged options to the same.

 Documentation/git-tag.txt |  39 ++++++++++---
 builtin/for-each-ref.c    |   3 +-
 builtin/tag.c             | 362
++++++++++++++++++++------------------------------------------------------------------------------------------------
 ref-filter.c              | 100 ++++++++++++++++++++++++++++----
 ref-filter.h              |   7 ++-
 t/t7004-tag.sh            |  51 ++++++++++++++---
 6 files changed, 233 insertions(+), 329 deletions(-)
-- 
Regards,
Karthik Nayak
Karthik Nayak· Jun 25, 2015, 11:43 UTC · re: Karthik Nayak · lore

[RFC/PATCH 1/9] ref-filter: add %(refname:lalignX) option

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

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)
Show changes to ref-filter.c +16 −1
diff --git a/ref-filter.c b/ref-filter.c
index 00d06bf..299b048 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -695,7 +695,22 @@ static void populate_value(struct ref_array_item *ref)
 			int num_ours, num_theirs;
 
 			formatp++;
-			if (!strcmp(formatp, "short"))
+			if (starts_with(formatp, "lalign")) {
+				const char *valp;
+				int val;
+
+				skip_prefix(formatp, "lalign", &valp);
+				val = atoi(valp);
+				refname = shorten_unambiguous_ref(refname,
+								  warn_ambiguous_refs);
+				if (val > strlen(refname)) {
+					struct strbuf buf = STRBUF_INIT;
+					strbuf_addstr(&buf, refname);
+					strbuf_addchars(&buf, ' ', val - strlen(refname));
+					free((char *)refname);
+					refname = strbuf_detach(&buf, NULL);
+				}
+			} else if (!strcmp(formatp, "short"))
 				refname = shorten_unambiguous_ref(refname,
 						      warn_ambiguous_refs);
 			else if (!strcmp(formatp, "track") &&
-- 
2.4.4
Karthik Nayak· Jun 25, 2015, 11:43 UTC · re: Karthik Nayak · lore

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

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(+)
Show changes to 2 files +3 −0

ref-filter.c, ref-filter.h

diff --git a/ref-filter.c b/ref-filter.c
index 299b048..97432d1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1141,6 +1141,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 3c59431..dd28d17 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.4
Karthik Nayak· Jun 25, 2015, 11:43 UTC · re: Karthik Nayak · lore

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

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

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

builtin/for-each-ref.c, builtin/tag.c, ref-filter.c, ref-filter.h

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 5868c48..c318e33 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 767162e..9f300e0 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -180,6 +180,10 @@ static enum contains_result contains(struct commit *candidate,
 	return contains_test(candidate, want);
 }
 
+/*
+ * Currently dupplicated in ref-filter, will eventually be removed as
+ * we port tag.c to use ref-filter APIs.
+ */
 static void show_tag_lines(const struct object_id *oid, int lines)
 {
 	int i;
diff --git a/ref-filter.c b/ref-filter.c
index 97432d1..57a5cfb 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1262,7 +1262,48 @@ static void emit(const char *cp, const char *ep)
 	}
 }
 
-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
+/* Print 'lines' no of lines of a given oid */
+static void show_tag_lines(const struct object_id *oid, int lines)
+{
+	int i;
+	unsigned long size;
+	enum object_type type;
+	char *buf, *sp, *eol;
+	size_t len;
+
+	buf = read_sha1_file(oid->hash, &type, &size);
+	if (!buf)
+		die_errno("unable to read object %s", oid_to_hex(oid));
+	if (type != OBJ_COMMIT && type != OBJ_TAG)
+		goto free_return;
+	if (!size)
+		die("an empty %s object %s?",
+		    typename(type), oid_to_hex(oid));
+
+	/* skip header */
+	sp = strstr(buf, "\n\n");
+	if (!sp)
+		goto free_return;
+
+	/* only take up to "lines" lines, and strip the signature from a tag */
+	if (type == OBJ_TAG)
+		size = parse_signature(buf, size);
+	for (i = 0, sp += 2; i < lines && sp < buf + size; i++) {
+		if (i)
+			printf("\n    ");
+		eol = memchr(sp, '\n', size - (sp - buf));
+		len = eol ? eol - sp : size - (sp - buf);
+		fwrite(sp, len, 1, stdout);
+		if (!eol)
+			break;
+		sp = eol + 1;
+	}
+free_return:
+	free(buf);
+}
+
+void show_ref_array_item(struct ref_array_item *info, const char *format,
+			 int quote_style, unsigned int lines)
 {
 	const char *cp, *sp, *ep;
 
@@ -1288,6 +1329,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 dd28d17..6b6fb96 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -55,6 +55,7 @@ struct ref_filter {
 	struct commit *merge_commit;
 
 	unsigned int with_commit_tag_algo: 1;
+	unsigned int lines;
 };
 
 struct ref_filter_cbdata {
@@ -87,7 +88,7 @@ int verify_ref_format(const char *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
 /*  Print the ref using the given format and quote_style */
-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);
+void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style, unsigned int lines);
 /*  Callback function for parsing the sort option */
 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);
 /*  Default sort option based on refname */
-- 
2.4.4
Karthik Nayak· Jun 25, 2015, 11:43 UTC · re: Karthik Nayak · lore

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

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

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)
Show changes to ref-filter.c +17 −3
diff --git a/ref-filter.c b/ref-filter.c
index 57a5cfb..e307fab 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -10,8 +10,9 @@
 #include "quote.h"
 #include "ref-filter.h"
 #include "revision.h"
+#include "version.h"
 
-typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
+typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME, FIELD_VER } cmp_type;
 
 static struct {
 	const char *name;
@@ -53,6 +54,8 @@ static struct {
 	{ "flag" },
 	{ "HEAD" },
 	{ "color" },
+	{ "version", FIELD_VER },
+	{ "v", FIELD_VER },
 };
 
 /*
@@ -629,7 +632,9 @@ static void populate_value(struct ref_array_item *ref)
 			name++;
 		}
 
-		if (starts_with(name, "refname"))
+		if (starts_with(name, "refname") ||
+		    starts_with(name, "version:") ||
+		    starts_with(name, "v:"))
 			refname = ref->refname;
 		else if (starts_with(name, "symref"))
 			refname = ref->symref ? ref->symref : "";
@@ -695,7 +700,13 @@ static void populate_value(struct ref_array_item *ref)
 			int num_ours, num_theirs;
 
 			formatp++;
-			if (starts_with(formatp, "lalign")) {
+			if (starts_with(name, "version") || starts_with(name, "v")) {
+				if (strcmp(formatp, "refname"))
+					die("unknown %.*s format %s",
+					    (int)(formatp - name), name, formatp);
+				v->s = refname;
+				continue;
+			} else if (starts_with(formatp, "lalign")) {
 				const char *valp;
 				int val;
 
@@ -1165,6 +1176,9 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 	case FIELD_STR:
 		cmp = strcmp(va->s, vb->s);
 		break;
+	case FIELD_VER:
+		cmp = versioncmp(va->s, vb->s);
+		break;
 	default:
 		if (va->ul < vb->ul)
 			cmp = -1;
-- 
2.4.4
Duy Nguyen· Jul 12, 2015, 09:09 UTC · re: Karthik Nayak · lore

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

On Thu, Jun 25, 2015 at 6:43 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.

If these v:refname and version:refname are from git-tag, you may want to see [1]. I would say "version:" or "v:" is in the same class as "-" (for reverse sort) and they should be parsed in parse_opt_ref_sorting() instead. They should not be treated as atom names. By stripping "version:" before pref_ref_filter_atom() is called in this function, you make "version:" work with all supported atoms.

[1] http://article.gmane.org/gmane.comp.version-control.git/242446
-- 
Duy
Karthik Nayak· Jul 12, 2015, 13:37 UTC · re: Duy Nguyen · lore

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

On Sun, Jul 12, 2015 at 2:39 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 15 quoted lines
> On Thu, Jun 25, 2015 at 6:43 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.
>
> If these v:refname and version:refname are from git-tag, you may want
> to see [1]. I would say "version:" or "v:" is in the same class as "-"
> (for reverse sort) and they should be parsed in
> parse_opt_ref_sorting() instead. They should not be treated as atom
> names. By stripping "version:" before pref_ref_filter_atom() is called
> in this function, you make "version:" work with all supported atoms.
>
> [1] http://article.gmane.org/gmane.comp.version-control.git/242446
> --
> Duy
Thanks for this, what you're saying makes sense, will follow.
-- 
Regards,
Karthik Nayak
Karthik Nayak· Jun 25, 2015, 11:43 UTC · re: Karthik Nayak · lore

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

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

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

builtin/for-each-ref.c, ref-filter.c, ref-filter.h

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index c318e33..01d5363 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 e307fab..1f97910 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -954,6 +954,20 @@ static int commit_contains(struct ref_filter *filter, struct commit *commit)
 
 /*
  * Return 1 if the refname matches one of the patterns, otherwise 0.
+ * A pattern can be a literal prefix (e.g. a refname "refs/heads/master"
+ * matches a pattern "refs/heads/m") or a wildcard (e.g. the same ref
+ * matches "refs/heads/m*",too).
+ */
+static int match_pattern(const char **patterns, const char *refname)
+{
+	for (; *patterns; patterns++)
+		if (!wildmatch(*patterns, refname, 0, NULL))
+			return 1;
+	return 0;
+}
+
+/*
+ * Return 1 if the refname matches one of the patterns, otherwise 0.
  * A pattern can be path prefix (e.g. a refname "refs/heads/master"
  * matches a pattern "refs/heads/") or a wildcard (e.g. the same ref
  * matches "refs/heads/m*",too).
@@ -977,6 +991,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 0;
+	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) see if it points to one of the sha1s
  * in a sha1_array.
@@ -1026,17 +1049,12 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	struct ref_array_item *ref;
 	struct commit *commit = NULL;
 
-	if (flag & REF_BAD_NAME) {
-		  warning("ignoring ref with broken name %s", refname);
-		  return 0;
-	}
-
 	if (flag & REF_ISBROKEN) {
 		warning("ignoring broken ref %s", refname);
 		return 0;
 	}
 
-	if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
+	if (!filter_pattern_match(filter, refname))
 		return 0;
 
 	if (!match_points_at(&filter->points_at, oid->hash, refname))
diff --git a/ref-filter.h b/ref-filter.h
index 6b6fb96..a4809c8 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -54,7 +54,8 @@ struct ref_filter {
 	} merge;
 	struct commit *merge_commit;
 
-	unsigned int with_commit_tag_algo: 1;
+	unsigned int with_commit_tag_algo: 1,
+		match_as_path: 1;
 	unsigned int lines;
 };
 
-- 
2.4.4
Karthik Nayak· Jun 26, 2015, 21:15 UTC · re: Karthik Nayak · lore

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

Just a few things I need to fix, personal note.
On 06/25/2015 05:13 PM, Karthik Nayak wrote:
Show 57 quoted lines
> Since 'ref-filter' only has an option to match path names.
> Add an option for regular pattern matching.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>   builtin/for-each-ref.c |  1 +
>   ref-filter.c           | 30 ++++++++++++++++++++++++------
>   ref-filter.h           |  3 ++-
>   3 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index c318e33..01d5363 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 e307fab..1f97910 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -954,6 +954,20 @@ static int commit_contains(struct ref_filter *filter, struct commit *commit)
>
>   /*
>    * Return 1 if the refname matches one of the patterns, otherwise 0.
> + * A pattern can be a literal prefix (e.g. a refname "refs/heads/master"
> + * matches a pattern "refs/heads/m") or a wildcard (e.g. the same ref
> + * matches "refs/heads/m*",too).
> + */
> +static int match_pattern(const char **patterns, const char *refname)
> +{
> +	for (; *patterns; patterns++)
> +		if (!wildmatch(*patterns, refname, 0, NULL))
> +			return 1;
> +	return 0;
> +}
> +
> +/*
> + * Return 1 if the refname matches one of the patterns, otherwise 0.
>    * A pattern can be path prefix (e.g. a refname "refs/heads/master"
>    * matches a pattern "refs/heads/") or a wildcard (e.g. the same ref
>    * matches "refs/heads/m*",too).
> @@ -977,6 +991,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 0;
Should return 1.
Show 17 quoted lines
> +	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) see if it points to one of the sha1s
>    * in a sha1_array.
> @@ -1026,17 +1049,12 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>   	struct ref_array_item *ref;
>   	struct commit *commit = NULL;
>
> -	if (flag & REF_BAD_NAME) {
> -		  warning("ignoring ref with broken name %s", refname);
> -		  return 0;
> -	}
> -
Undo this merge conflict.
Show 25 quoted lines
>   	if (flag & REF_ISBROKEN) {
>   		warning("ignoring broken ref %s", refname);
>   		return 0;
>   	}
>
> -	if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
> +	if (!filter_pattern_match(filter, refname))
>   		return 0;
>
>   	if (!match_points_at(&filter->points_at, oid->hash, refname))
> diff --git a/ref-filter.h b/ref-filter.h
> index 6b6fb96..a4809c8 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -54,7 +54,8 @@ struct ref_filter {
>   	} merge;
>   	struct commit *merge_commit;
>
> -	unsigned int with_commit_tag_algo: 1;
> +	unsigned int with_commit_tag_algo: 1,
> +		match_as_path: 1;
>   	unsigned int lines;
>   };
>
>
I'll add these changes to my local branch.
-- 
Regards,
Karthik
Junio C Hamano· Jun 29, 2015, 18:20 UTC · re: Karthik Nayak · lore

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

Karthik Nayak <karthik.188@gmail.com> writes:
> Since 'ref-filter' only has an option to match path names.
That is not a whole sentence ;-)
> Add an option for regular pattern matching.
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Show 5 quoted lines
> -	if (flag & REF_BAD_NAME) {
> -		  warning("ignoring ref with broken name %s", refname);
> -		  return 0;
> -	}
> -

Hmm, where did this check go in the new code? Or is it now OK not to warn or ignore, and if so why?

Show 10 quoted lines
>  	if (flag & REF_ISBROKEN) {
>  		warning("ignoring broken ref %s", refname);
>  		return 0;
>  	}
>  
> -	if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
> +	if (!filter_pattern_match(filter, refname))
>  		return 0;
>  
>  	if (!match_points_at(&filter->points_at, oid->hash, refname))
Show 11 quoted lines
> diff --git a/ref-filter.h b/ref-filter.h
> index 6b6fb96..a4809c8 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -54,7 +54,8 @@ struct ref_filter {
>  	} merge;
>  	struct commit *merge_commit;
>  
> -	unsigned int with_commit_tag_algo: 1;
> +	unsigned int with_commit_tag_algo: 1,
> +		match_as_path: 1;

Lose SP on both sides of the colon, or have SP on both sides (same for the last patch in the previous series).

Karthik Nayak· Jun 30, 2015, 13:36 UTC · re: Junio C Hamano · lore

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

On Mon, Jun 29, 2015 at 11:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Since 'ref-filter' only has an option to match path names.
>
> That is not a whole sentence ;-)
>
Argh! Noted.
Show 15 quoted lines
>> Add an option for regular pattern matching.
>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>
>> -     if (flag & REF_BAD_NAME) {
>> -               warning("ignoring ref with broken name %s", refname);
>> -               return 0;
>> -     }
>> -
>
> Hmm, where did this check go in the new code?  Or is it now OK not
> to warn or ignore, and if so why?
>

Merge conflict, I've replied with a fixing patch, shouldn't be there in the next version :)

Show 25 quoted lines
>>       if (flag & REF_ISBROKEN) {
>>               warning("ignoring broken ref %s", refname);
>>               return 0;
>>       }
>>
>> -     if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
>> +     if (!filter_pattern_match(filter, refname))
>>               return 0;
>>
>>       if (!match_points_at(&filter->points_at, oid->hash, refname))
>
>> diff --git a/ref-filter.h b/ref-filter.h
>> index 6b6fb96..a4809c8 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -54,7 +54,8 @@ struct ref_filter {
>>       } merge;
>>       struct commit *merge_commit;
>>
>> -     unsigned int with_commit_tag_algo: 1;
>> +     unsigned int with_commit_tag_algo: 1,
>> +             match_as_path: 1;
>
> Lose SP on both sides of the colon, or have SP on both sides
> (same for the last patch in the previous series).
Will Do!
Thanks for the quick review.
-- 
Regards,
Karthik Nayak
Karthik Nayak· Jun 25, 2015, 11:43 UTC · re: Karthik Nayak · lore

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

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(-)
Show changes to builtin/tag.c +57 −49
diff --git a/builtin/tag.c b/builtin/tag.c
index 9f300e0..65b6707 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;
 }
@@ -223,12 +216,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;
 
@@ -239,12 +244,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;
@@ -259,36 +264,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;
 }
@@ -569,16 +574,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'),
@@ -599,14 +604,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()
@@ -615,6 +620,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);
 
@@ -631,7 +638,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;
@@ -644,18 +651,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.4
Karthik Nayak· Jun 25, 2015, 11:43 UTC · re: Karthik Nayak · lore

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

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             | 337 ++++++----------------------------------------
 t/t7004-tag.sh            |   8 +-
 3 files changed, 49 insertions(+), 312 deletions(-)
Show changes to 3 files +49 −311

Documentation/git-tag.txt, builtin/tag.c, t/t7004-tag.sh

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 034d10d..1950d94 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>...]
 	[<pattern>...]
 'git tag' -v <tagname>...
 
@@ -95,14 +95,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 65b6707..d80120e 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -28,273 +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;
-}
-
-enum contains_result {
-	CONTAINS_UNKNOWN = -1,
-	CONTAINS_NO = 0,
-	CONTAINS_YES = 1
-};
-
-/*
- * Test whether the candidate or one of its parents is contained in the list.
- * Do not recurse to find out, though, but return -1 if inconclusive.
- */
-static enum contains_result contains_test(struct commit *candidate,
-			    const struct commit_list *want)
-{
-	/* was it previously marked as containing a want commit? */
-	if (candidate->object.flags & TMP_MARK)
-		return 1;
-	/* or marked as not possibly containing a want commit? */
-	if (candidate->object.flags & UNINTERESTING)
-		return 0;
-	/* or are we it? */
-	if (in_commit_list(want, candidate)) {
-		candidate->object.flags |= TMP_MARK;
-		return 1;
-	}
-
-	if (parse_commit(candidate) < 0)
-		return 0;
-
-	return -1;
-}
-
-/*
- * Mimicking the real stack, this stack lives on the heap, avoiding stack
- * overflows.
- *
- * At each recursion step, the stack items points to the commits whose
- * ancestors are to be inspected.
- */
-struct stack {
-	int nr, alloc;
-	struct stack_entry {
-		struct commit *commit;
-		struct commit_list *parents;
-	} *stack;
-};
-
-static void push_to_stack(struct commit *candidate, struct stack *stack)
-{
-	int index = stack->nr++;
-	ALLOC_GROW(stack->stack, stack->nr, stack->alloc);
-	stack->stack[index].commit = candidate;
-	stack->stack[index].parents = candidate->parents;
-}
-
-static enum contains_result contains(struct commit *candidate,
-		const struct commit_list *want)
-{
-	struct stack stack = { 0, 0, NULL };
-	int result = contains_test(candidate, want);
-
-	if (result != CONTAINS_UNKNOWN)
-		return result;
-
-	push_to_stack(candidate, &stack);
-	while (stack.nr) {
-		struct stack_entry *entry = &stack.stack[stack.nr - 1];
-		struct commit *commit = entry->commit;
-		struct commit_list *parents = entry->parents;
-
-		if (!parents) {
-			commit->object.flags |= UNINTERESTING;
-			stack.nr--;
-		}
-		/*
-		 * If we just popped the stack, parents->item has been marked,
-		 * therefore contains_test will return a meaningful 0 or 1.
-		 */
-		else switch (contains_test(parents->item, want)) {
-		case CONTAINS_YES:
-			commit->object.flags |= TMP_MARK;
-			stack.nr--;
-			break;
-		case CONTAINS_NO:
-			entry->parents = parents->next;
-			break;
-		case CONTAINS_UNKNOWN:
-			push_to_stack(parents->item, &stack);
-			break;
-		}
-	}
-	free(stack.stack);
-	return contains_test(candidate, want);
-}
-
-/*
- * Currently dupplicated in ref-filter, will eventually be removed as
- * we port tag.c to use ref-filter APIs.
- */
-static void show_tag_lines(const struct object_id *oid, int lines)
-{
-	int i;
-	unsigned long size;
-	enum object_type type;
-	char *buf, *sp, *eol;
-	size_t len;
-
-	buf = read_sha1_file(oid->hash, &type, &size);
-	if (!buf)
-		die_errno("unable to read object %s", oid_to_hex(oid));
-	if (type != OBJ_COMMIT && type != OBJ_TAG)
-		goto free_return;
-	if (!size)
-		die("an empty %s object %s?",
-		    typename(type), oid_to_hex(oid));
-
-	/* skip header */
-	sp = strstr(buf, "\n\n");
-	if (!sp)
-		goto free_return;
-
-	/* only take up to "lines" lines, and strip the signature from a tag */
-	if (type == OBJ_TAG)
-		size = parse_signature(buf, size);
-	for (i = 0, sp += 2; i < lines && sp < buf + size; i++) {
-		if (i)
-			printf("\n    ");
-		eol = memchr(sp, '\n', size - (sp - buf));
-		len = eol ? eol - sp : size - (sp - buf);
-		fwrite(sp, len, 1, stdout);
-		if (!eol)
-			break;
-		sp = eol + 1;
-	}
-free_return:
-	free(buf);
-}
-
-static void ref_array_append(struct ref_array *array, const char *refname)
-{
-	size_t len = strlen(refname);
-	struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
-	memcpy(ref->refname, refname, len);
-	ref->refname[len] = '\0';
-	REALLOC_ARRAY(array->items, array->nr + 1);
-	array->items[array->nr++] = ref;
-}
-
-static int show_reference(const char *refname, const struct object_id *oid,
-			  int flag, void *cb_data)
-{
-	struct ref_filter_cbdata *data = cb_data;
-	struct ref_array *array = data->array;
-	struct ref_filter *filter = data->filter;
-
-	if (match_pattern(filter->name_patterns, refname)) {
-		if (filter->with_commit) {
-			struct commit *commit;
-
-			commit = lookup_commit_reference_gently(oid->hash, 1);
-			if (!commit)
-				return 0;
-			if (!contains(commit, filter->with_commit))
-				return 0;
-		}
-
-		if (filter->points_at.nr && !match_points_at(refname, oid->hash, &filter->points_at))
-			return 0;
-
-		if (!filter->lines) {
-			if (tag_sort)
-				ref_array_append(array, refname);
-			else
-				printf("%s\n", refname);
-			return 0;
-		}
-		printf("%-15s ", refname);
-		show_tag_lines(oid, filter->lines);
-		putchar('\n');
-	}
-
-	return 0;
-}
-
-static int sort_by_version(const void *a_, const void *b_)
-{
-	const struct ref_array_item *a = *((struct ref_array_item **)a_);
-	const struct ref_array_item *b = *((struct ref_array_item **)b_);
-	return versioncmp(a->refname, b->refname);
-}
-
-static int list_tags(struct ref_filter *filter, int sort)
+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
 {
 	struct ref_array array;
-	struct ref_filter_cbdata data;
+	char *format;
+	int i;
 
 	memset(&array, 0, sizeof(array));
-	data.array = &array;
-	data.filter = filter;
 
 	if (filter->lines == -1)
 		filter->lines = 0;
 
-	for_each_tag_ref(show_reference, &data);
-	if (sort) {
-		int i;
-		if ((sort & SORT_MASK) == VERCMP_SORT)
-			qsort(array.items, array.nr,
-			      sizeof(struct ref_array_item *), sort_by_version);
-		if (sort & REVERSE_SORT)
-			for (i = array.nr - 1; i >= 0; i--)
-				printf("%s\n", array.items[i]->refname);
-		else
-			for (i = 0; i < array.nr; i++)
-				printf("%s\n", array.items[i]->refname);
-		ref_array_clear(&array);
-	}
+	if (filter->lines)
+		format = "%(refname:lalign16)";
+	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;
 }
 
@@ -361,35 +122,22 @@ static const char tag_template_nocleanup[] =
 	"Lines starting with '%c' will be kept; you may remove them"
 	" yourself if you want to.\n");
 
-/*
- * Parse a sort string, and return 0 if parsed successfully. Will return
- * non-zero when the sort string does not parse into a known type. If var is
- * given, the error message becomes a warning and includes information about
- * the configuration value.
- */
-static int parse_sort_string(const char *var, const char *arg, int *sort)
+/* Parse arg given and add it the ref_sorting array */
+static int parse_sorting_string(const char *arg, struct ref_sorting **sorting_tail)
 {
-	int type = 0, flags = 0;
-
-	if (skip_prefix(arg, "-", &arg))
-		flags |= REVERSE_SORT;
+	struct ref_sorting *s;
+	int len;
 
-	if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
-		type = VERCMP_SORT;
-	else
-		type = STRCMP_SORT;
+	s = xcalloc(1, sizeof(*s));
+	s->next = *sorting_tail;
+	*sorting_tail = s;
 
-	if (strcmp(arg, "refname")) {
-		if (!var)
-			return error(_("unsupported sort specification '%s'"), arg);
-		else {
-			warning(_("unsupported sort specification '%s' in variable '%s'"),
-				var, arg);
-			return -1;
-		}
+	if (*arg == '-') {
+		s->reverse = 1;
+		arg++;
 	}
-
-	*sort = (type | flags);
+	len = strlen(arg);
+	s->atom = parse_ref_filter_atom(arg, arg+len);
 
 	return 0;
 }
@@ -397,11 +145,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;
 	}
 
@@ -559,13 +308,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;
@@ -581,6 +323,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"),
@@ -606,10 +349,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
@@ -617,7 +358,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));
@@ -643,6 +384,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)) {
@@ -651,10 +394,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.4
Karthik Nayak· Jun 25, 2015, 11:43 UTC · re: Karthik Nayak · lore

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

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

Add tests and documentation for the same.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-tag.txt | 15 ++++++++++++++-
 builtin/tag.c             | 14 ++++++++++----
 t/t7004-tag.sh            | 16 ++++++++++++++++
 3 files changed, 40 insertions(+), 5 deletions(-)
Show changes to 3 files +40 −5

Documentation/git-tag.txt, builtin/tag.c, t/t7004-tag.sh

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 1950d94..16e396c 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] [--sort=<key>] [<pattern>...]
+	[--column[=<options>] | --no-column] [--sort=<key>] [--format=<format>]
 	[<pattern>...]
 'git tag' -v <tagname>...
 
@@ -156,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 d80120e..91356c9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -30,12 +30,14 @@ 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;
 
+	if (!format)
+		check_format = 1;
+
 	memset(&array, 0, sizeof(array));
 
 	if (filter->lines == -1)
@@ -43,7 +45,7 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
 
 	if (filter->lines)
 		format = "%(refname:lalign16)";
-	else
+	else if (!format)
 		format = "%(refname:short)";
 
 	verify_ref_format(format);
@@ -324,6 +326,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct strbuf err = STRBUF_INIT;
 	struct ref_filter filter;
 	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
+	const char *format = NULL;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
@@ -355,6 +358,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()
 	};
 
@@ -394,8 +398,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.4
Karthik Nayak· Jun 25, 2015, 13:03 UTC · re: Karthik Nayak · lore

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

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

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

Documentation/git-tag.txt, builtin/tag.c, t/t7004-tag.sh

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 1950d94..16e396c 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] [--sort=<key>] [<pattern>...]
+	[--column[=<options>] | --no-column] [--sort=<key>] [--format=<format>]
 	[<pattern>...]
 'git tag' -v <tagname>...
 
@@ -156,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 d80120e..257526b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -30,10 +30,9 @@ static const char * const git_tag_usage[] = {
 
 static unsigned int colopts;
 
-static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
 	struct ref_array array;
-	char *format;
 	int i;
 
 	memset(&array, 0, sizeof(array));
@@ -43,7 +42,7 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
 
 	if (filter->lines)
 		format = "%(refname:lalign16)";
-	else
+	else if (!format)
 		format = "%(refname:short)";
 
 	verify_ref_format(format);
@@ -324,6 +323,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"),
@@ -355,6 +355,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()
 	};
 
@@ -394,8 +395,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.4
Karthik Nayak· Jun 25, 2015, 11:43 UTC · re: Karthik Nayak · lore

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

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             |  9 +++++----
 t/t7004-tag.sh            | 27 +++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 5 deletions(-)
Show changes to 3 files +41 −5

Documentation/git-tag.txt, builtin/tag.c, t/t7004-tag.sh

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 91356c9..9a1f7a5 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
 };
@@ -35,9 +35,6 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
 	struct ref_array array;
 	int i;
 
-	if (!format)
-		check_format = 1;
-
 	memset(&array, 0, sizeof(array));
 
 	if (filter->lines == -1)
@@ -352,6 +349,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),
 		{
@@ -412,6 +411,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.4
Christian Couder· Jun 27, 2015, 20:02 UTC · re: Karthik Nayak · lore

Re: [RFC/PATCH 1/9] ref-filter: add %(refname:lalignX) option

On Thu, Jun 25, 2015 at 1:43 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
Show 28 quoted lines
> Add support for %(refname:lalignX) where X is a number.
> This will print a shortened refname aligned to the left
> followed by spaces for a total length of X characters.
> If X is less than the shortened refname size, the entire
> shortened refname is printed.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  ref-filter.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 00d06bf..299b048 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -695,7 +695,22 @@ static void populate_value(struct ref_array_item *ref)
>                         int num_ours, num_theirs;
>
>                         formatp++;
> -                       if (!strcmp(formatp, "short"))
> +                       if (starts_with(formatp, "lalign")) {
> +                               const char *valp;
> +                               int val;
> +
> +                               skip_prefix(formatp, "lalign", &valp);
> +                               val = atoi(valp);

After thinking about such code, I wonder if it would be better to support %(refname:lalign=X) instead of %(refname:lalignX).

The reason why it might be interesting to require an = sign between "align" and the number X is that if we later want to introduce another option with a name that starts with "lalign", for example %(refname:lalignall=X) that would truncate the refname if it is bigger than X), we might be more backward compatible with old git versions that implement %(refname:lalign=X) but not %(refname:lalignall=X).

We will be more backward compatible because the above call to starts_with() would probably be something like:

                       if (starts_with(formatp, "lalign=")) {
which means that old git versions would ignore something like "lalignall=X".
Show 9 quoted lines
> +                               refname = shorten_unambiguous_ref(refname,
> +                                                                 warn_ambiguous_refs);
> +                               if (val > strlen(refname)) {
> +                                       struct strbuf buf = STRBUF_INIT;
> +                                       strbuf_addstr(&buf, refname);
> +                                       strbuf_addchars(&buf, ' ', val - strlen(refname));
> +                                       free((char *)refname);
> +                                       refname = strbuf_detach(&buf, NULL);
> +                               }

Thanks, Christian.

Christian Couder· Jun 28, 2015, 04:53 UTC · re: Christian Couder · lore

Re: [RFC/PATCH 1/9] ref-filter: add %(refname:lalignX) option

On Sat, Jun 27, 2015 at 10:02 PM, Christian Couder <christian.couder@gmail.com> wrote:

Show 25 quoted lines
> On Thu, Jun 25, 2015 at 1:43 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>
>> +                       if (starts_with(formatp, "lalign")) {
>> +                               const char *valp;
>> +                               int val;
>> +
>> +                               skip_prefix(formatp, "lalign", &valp);
>> +                               val = atoi(valp);
>
> After thinking about such code, I wonder if it would be better to
> support %(refname:lalign=X) instead of %(refname:lalignX).
>
> The reason why it might be interesting to require an = sign between
> "align" and the number X is that if we later want to introduce another
> option with a name that starts with "lalign", for example
> %(refname:lalignall=X) that would truncate the refname if it is bigger
> than X), we might be more backward compatible with old git versions
> that implement %(refname:lalign=X) but not %(refname:lalignall=X).
>
> We will be more backward compatible because the above call to
> starts_with() would probably be something like:
>
>                        if (starts_with(formatp, "lalign=")) {
>
> which means that old git versions would ignore something like "lalignall=X".

Another reason is that it would be simpler if we ever want to have arbitrary string parameters, like %(refname:substitute=%/%\%).

Karthik Nayak· Jun 28, 2015, 06:59 UTC · re: Christian Couder · lore

Re: [RFC/PATCH 1/9] ref-filter: add %(refname:lalignX) option

Hey Christian, On Sun, Jun 28, 2015 at 1:32 AM, Christian Couder <christian.couder@gmail.com> wrote:

Show 18 quoted lines
>
> After thinking about such code, I wonder if it would be better to
> support %(refname:lalign=X) instead of %(refname:lalignX).
>
> The reason why it might be interesting to require an = sign between
> "align" and the number X is that if we later want to introduce another
> option with a name that starts with "lalign", for example
> %(refname:lalignall=X) that would truncate the refname if it is bigger
> than X), we might be more backward compatible with old git versions
> that implement %(refname:lalign=X) but not %(refname:lalignall=X).
>
> We will be more backward compatible because the above call to
> starts_with() would probably be something like:
>
>                        if (starts_with(formatp, "lalign=")) {
>
> which means that old git versions would ignore something like "lalignall=X".
>

Good point! I agree with what you said, including an "=" sign would mean more compatibility in the future as we could have lalign* options. Will change this, thanks.

-- 
Regards,
Karthik Nayak
Duy Nguyen· Jun 27, 2015, 23:43 UTC · re: Karthik Nayak · lore

Re: [RFC/PATCH 1/9] ref-filter: add %(refname:lalignX) option

On Thu, Jun 25, 2015 at 6:43 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
Show 34 quoted lines
> Add support for %(refname:lalignX) where X is a number.
> This will print a shortened refname aligned to the left
> followed by spaces for a total length of X characters.
> If X is less than the shortened refname size, the entire
> shortened refname is printed.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  ref-filter.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 00d06bf..299b048 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -695,7 +695,22 @@ static void populate_value(struct ref_array_item *ref)
>                         int num_ours, num_theirs;
>
>                         formatp++;
> -                       if (!strcmp(formatp, "short"))
> +                       if (starts_with(formatp, "lalign")) {
> +                               const char *valp;
> +                               int val;
> +
> +                               skip_prefix(formatp, "lalign", &valp);
> +                               val = atoi(valp);
> +                               refname = shorten_unambiguous_ref(refname,
> +                                                                 warn_ambiguous_refs);
> +                               if (val > strlen(refname)) {
> +                                       struct strbuf buf = STRBUF_INIT;
> +                                       strbuf_addstr(&buf, refname);
> +                                       strbuf_addchars(&buf, ' ', val - strlen(refname));

We don't forbid non-ascii characters in refname so you probably want to use utf8_width() here instead of strlen()

Show 14 quoted lines
> +                                       free((char *)refname);
> +                                       refname = strbuf_detach(&buf, NULL);
> +                               }
> +                       } else if (!strcmp(formatp, "short"))
>                                 refname = shorten_unambiguous_ref(refname,
>                                                       warn_ambiguous_refs);
>                         else if (!strcmp(formatp, "track") &&
> --
> 2.4.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Duy
Junio C Hamano· Jun 28, 2015, 07:39 UTC · re: Karthik Nayak · lore

Re: [RFC/PATCH 1/9] ref-filter: add %(refname:lalignX) option

Karthik Nayak <karthik.188@gmail.com> writes:
Show 5 quoted lines
> Add support for %(refname:lalignX) where X is a number.
> This will print a shortened refname aligned to the left
> followed by spaces for a total length of X characters.
> If X is less than the shortened refname size, the entire
> shortened refname is printed.

Why would we even want this kind of thing in the first place? Is this to make it possible to re-implement some option that exists already in 'git branch' or 'git tag' as a thin wrapper on top of this underlying feature?

As a low-level plumbing, I'd rather not to see such an elaborate formatting option added to for-each-ref; after all, the design of the command to allow its output properly script-quoted is so that we can offload such non-essential (meaning: does not need anything only Git knows; computing the display width of a string and filling the output space is an example. As opposed to something like --merged that needs help from Git, which has the ultimate knowledge on the topology) processing to Porcelain that uses the command as plumbing.

Karthik Nayak· Jun 28, 2015, 09:13 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH 1/9] ref-filter: add %(refname:lalignX) option

On Sun, Jun 28, 2015 at 1:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 21 quoted lines
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Add support for %(refname:lalignX) where X is a number.
>> This will print a shortened refname aligned to the left
>> followed by spaces for a total length of X characters.
>> If X is less than the shortened refname size, the entire
>> shortened refname is printed.
>
> Why would we even want this kind of thing in the first place?  Is
> this to make it possible to re-implement some option that exists
> already in 'git branch' or 'git tag' as a thin wrapper on top of
> this underlying feature?
> As a low-level plumbing, I'd rather not to see such an elaborate
> formatting option added to for-each-ref; after all, the design of
> the command to allow its output properly script-quoted is so that we
> can offload such non-essential (meaning: does not need anything only
> Git knows; computing the display width of a string and filling the
> output space is an example.  As opposed to something like --merged
> that needs help from Git, which has the ultimate knowledge on the
> topology) processing to Porcelain that uses the command as plumbing.
>

Well this is to support the printing of tag in `git tag -l`, if you see the current implementation we print refs using "printf("%-15s ", refname);" whenever the "-n[<n>]" option is given, hence this patch to support printing of refs using a required spacing.

I couldn't find a way to do with the current options available in for-each-ref/ref-filter. Is there a better way to go about this?

-- 
Regards,
Karthik Nayak
Junio C Hamano· Jun 29, 2015, 04:44 UTC · re: Karthik Nayak · lore

Re: [RFC/PATCH 1/9] ref-filter: add %(refname:lalignX) option

Karthik Nayak <karthik.188@gmail.com> writes:
Show 9 quoted lines
> On Sun, Jun 28, 2015 at 1:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Why would we even want this kind of thing in the first place?  Is
>> this to make it possible to re-implement some option that exists
>> already in 'git branch' or 'git tag' as a thin wrapper on top of
>> this underlying feature?
>> ...
>
> Well this is to support the printing of tag in `git tag -l`,...

OK, then. Thanks.

← back to recent threads