[PATCH 1/3] tag: use skip_prefix instead of magic numbers

[PATCH 1/3] tag: use skip_prefix instead of magic numbers

[Jacob Keller]

From: Jeff King <peff@peff.net>

Make the parsing of the --sort parameter more readable by having
skip_prefix keep our pointer up to date.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 builtin/tag.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index ef765563388c..7ccb6f3c581b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -524,18 +524,12 @@ static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
 	int *sort = opt->value;
 	int flags = 0;
 
-	if (*arg == '-') {
+	if (skip_prefix(arg, "-", &arg))
 		flags |= REVERSE_SORT;
-		arg++;
-	}
-	if (starts_with(arg, "version:")) {
+
+	if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
 		*sort = VERCMP_SORT;
-		arg += 8;
-	} else if (starts_with(arg, "v:")) {
-		*sort = VERCMP_SORT;
-		arg += 2;
-	} else
-		*sort = STRCMP_SORT;
+
 	if (strcmp(arg, "refname"))
 		die(_("unsupported sort specification %s"), arg);
 	*sort |= flags;
-- 
2.0.1.475.g9b8d714

[PATCH 2/3] tag: fix --sort tests to use cat<<-\EOF format

[Jacob Keller]

The --sort tests should use the better format for >expect to maintain
indenting and ensure that no substitution is occurring. This makes
parsing and understanding the tests a bit easier.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 t/t7004-tag.sh | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e4ab0f5b6419..66010b0e7066 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1385,41 +1385,41 @@ test_expect_success 'lexical sort' '
 	git tag foo1.6 &&
 	git tag foo1.10 &&
 	git tag -l --sort=refname "foo*" >actual &&
-	cat >expect <<EOF &&
-foo1.10
-foo1.3
-foo1.6
-EOF
+	cat >expect <<-\EOF &&
+	foo1.10
+	foo1.3
+	foo1.6
+	EOF
 	test_cmp expect actual
 '
 
 test_expect_success 'version sort' '
 	git tag -l --sort=version:refname "foo*" >actual &&
-	cat >expect <<EOF &&
-foo1.3
-foo1.6
-foo1.10
-EOF
+	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 "foo*" >actual &&
-	cat >expect <<EOF &&
-foo1.10
-foo1.6
-foo1.3
-EOF
+	cat >expect <<-\EOF &&
+	foo1.10
+	foo1.6
+	foo1.3
+	EOF
 	test_cmp expect actual
 '
 
 test_expect_success 'reverse lexical sort' '
 	git tag -l --sort=-refname "foo*" >actual &&
-	cat >expect <<EOF &&
-foo1.6
-foo1.3
-foo1.10
-EOF
+	cat >expect <<-\EOF &&
+	foo1.6
+	foo1.3
+	foo1.10
+	EOF
 	test_cmp expect actual
 '
 
-- 
2.0.1.475.g9b8d714

[PATCH 3/3 v7] tag: support configuring --sort via .gitconfig

[Jacob Keller]

Add support for configuring default sort ordering for git tags. Command
line option will override this configured value, using the exact same
syntax.

Cc: Jeff King <peff@peff.net>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Updated warning texts based on Jeff's feedback. Also added translate specifier
to the warning string.

 Documentation/config.txt  |  5 ++++
 Documentation/git-tag.txt |  5 +++-
 builtin/tag.c             | 61 ++++++++++++++++++++++++++++++++++-------------
 t/t7004-tag.sh            | 36 ++++++++++++++++++++++++++++
 4 files changed, 90 insertions(+), 17 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1d718bdb9662..c55c22ab7be9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2354,6 +2354,11 @@ submodule.<name>.ignore::
 	"--ignore-submodules" option. The 'git submodule' commands are not
 	affected by this setting.
 
+tag.sort::
+	This variable controls the sort ordering of tags when displayed by
+	linkgit:git-tag[1]. Without the "--sort=<value>" option provided, the
+	value of this variable will be used as the default.
+
 tar.umask::
 	This variable can be used to restrict the permission bits of
 	tar archive entries.  The default is 0002, which turns off the
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index b424a1bc48bb..320908369f06 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -99,7 +99,9 @@ OPTIONS
 	Sort in a specific order. Supported type is "refname"
 	(lexicographic order), "version:refname" or "v:refname" (tag
 	names are treated as versions). Prepend "-" to reverse sort
-	order.
+	order. When this option is not given, the sort order defaults to the
+	value configured for the 'tag.sort' variable if it exists, or
+	lexicographic order otherwise. See linkgit:git-config[1].
 
 --column[=<options>]::
 --no-column::
@@ -317,6 +319,7 @@ include::date-formats.txt[]
 SEE ALSO
 --------
 linkgit:git-check-ref-format[1].
+linkgit:git-config[1].
 
 GIT
 ---
diff --git a/builtin/tag.c b/builtin/tag.c
index 7ccb6f3c581b..6e0a8ed4d1f9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -32,6 +32,8 @@ static const char * const git_tag_usage[] = {
 #define SORT_MASK       0x7fff
 #define REVERSE_SORT    0x8000
 
+static int tag_sort;
+
 struct tag_filter {
 	const char **patterns;
 	int lines;
@@ -346,9 +348,46 @@ 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.
+ */
+static int parse_sort_string(const char *arg, int *sort)
+{
+	int type = 0, flags = 0;
+
+	if (skip_prefix(arg, "-", &arg))
+		flags |= REVERSE_SORT;
+
+	if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
+		type = VERCMP_SORT;
+	else
+		type = STRCMP_SORT;
+
+	if (strcmp(arg, "refname"))
+		return error(_("unsupported sort specification '%s'"), arg);
+
+	*sort = (type | flags);
+
+	return 0;
+}
+
 static int git_tag_config(const char *var, const char *value, void *cb)
 {
-	int status = git_gpg_config(var, value, cb);
+	int status;
+
+	if (!strcmp(var, "tag.sort")) {
+		if (!value)
+			return config_error_nonbool(var);
+		status = parse_sort_string(value, &tag_sort);
+		if (status) {
+			warning(_("ignoring configured sort specification from 'tag.sort'"));
+			tag_sort = STRCMP_SORT;
+		}
+		return 0;
+	}
+
+	status = git_gpg_config(var, value, cb);
 	if (status)
 		return status;
 	if (starts_with(var, "column."))
@@ -522,18 +561,8 @@ static int parse_opt_points_at(const struct option *opt __attribute__((unused)),
 static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
 {
 	int *sort = opt->value;
-	int flags = 0;
 
-	if (skip_prefix(arg, "-", &arg))
-		flags |= REVERSE_SORT;
-
-	if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
-		*sort = VERCMP_SORT;
-
-	if (strcmp(arg, "refname"))
-		die(_("unsupported sort specification %s"), arg);
-	*sort |= flags;
-	return 0;
+	return parse_sort_string(arg, sort);
 }
 
 int cmd_tag(int argc, const char **argv, const char *prefix)
@@ -546,7 +575,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct create_tag_options opt;
 	char *cleanup_arg = NULL;
 	int annotate = 0, force = 0, lines = -1;
-	int cmdmode = 0, sort = 0;
+	int cmdmode = 0;
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
 	struct commit_list *with_commit = NULL;
@@ -572,7 +601,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT__FORCE(&force, N_("replace the tag if exists")),
 		OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
 		{
-			OPTION_CALLBACK, 0, "sort", &sort, N_("type"), N_("sort tags"),
+			OPTION_CALLBACK, 0, "sort", &tag_sort, N_("type"), N_("sort tags"),
 			PARSE_OPT_NONEG, parse_opt_sort
 		},
 
@@ -628,9 +657,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			copts.padding = 2;
 			run_column_filter(colopts, &copts);
 		}
-		if (lines != -1 && sort)
+		if (lines != -1 && tag_sort)
 			die(_("--sort and -n are incompatible"));
-		ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, sort);
+		ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, tag_sort);
 		if (column_active(colopts))
 			stop_column_filter();
 		return ret;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 66010b0e7066..036665308841 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1423,6 +1423,42 @@ test_expect_success 'reverse lexical sort' '
 	test_cmp expect actual
 '
 
+test_expect_success 'configured lexical sort' '
+	git config tag.sort "v:refname" &&
+	git tag -l "foo*" >actual &&
+	cat >expect <<-\EOF &&
+	foo1.3
+	foo1.6
+	foo1.10
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'option override configured sort' '
+	git tag -l --sort=-refname "foo*" >actual &&
+	cat >expect <<-\EOF &&
+	foo1.6
+	foo1.3
+	foo1.10
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'invalid sort parameter on command line' '
+	test_must_fail git tag -l --sort=notvalid "foo*" >actual
+'
+
+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
+'
+
 run_with_limited_stack () {
 	(ulimit -s 64 && "$@")
 }
-- 
2.0.1.475.g9b8d714

Re: [PATCH 1/3] tag: use skip_prefix instead of magic numbers

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> From: Jeff King <peff@peff.net>
>
> Make the parsing of the --sort parameter more readable by having
> skip_prefix keep our pointer up to date.
>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  builtin/tag.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index ef765563388c..7ccb6f3c581b 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -524,18 +524,12 @@ static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
>  	int *sort = opt->value;
>  	int flags = 0;
>  
> -	if (*arg == '-') {
> +	if (skip_prefix(arg, "-", &arg))
>  		flags |= REVERSE_SORT;
> -		arg++;
> -	}
> -	if (starts_with(arg, "version:")) {
> +
> +	if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
>  		*sort = VERCMP_SORT;
> -		arg += 8;
> -	} else if (starts_with(arg, "v:")) {
> -		*sort = VERCMP_SORT;
> -		arg += 2;
> -	} else
> -		*sort = STRCMP_SORT;
> +

By losing "*sort = STRCMP_SORT", the version after this patch would
stop clearing what is pointed by opt->value, so

	git tag --sort=v:refname --sort=refname

would no longer implement the "last one wins" semantics, no?

Am I misreading the patch?  I somehow thought Peff's original was
clearing the variable...

>  	if (strcmp(arg, "refname"))
>  		die(_("unsupported sort specification %s"), arg);
>  	*sort |= flags;

Re: [PATCH 1/3] tag: use skip_prefix instead of magic numbers

[Keller, Jacob E]

On Fri, 2014-07-11 at 15:44 -0700, Junio C Hamano wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
> 
> > From: Jeff King <peff@peff.net>
> >
> > Make the parsing of the --sort parameter more readable by having
> > skip_prefix keep our pointer up to date.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > ---
> >  builtin/tag.c | 14 ++++----------
> >  1 file changed, 4 insertions(+), 10 deletions(-)
> >
> > diff --git a/builtin/tag.c b/builtin/tag.c
> > index ef765563388c..7ccb6f3c581b 100644
> > --- a/builtin/tag.c
> > +++ b/builtin/tag.c
> > @@ -524,18 +524,12 @@ static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
> >  	int *sort = opt->value;
> >  	int flags = 0;
> >  
> > -	if (*arg == '-') {
> > +	if (skip_prefix(arg, "-", &arg))
> >  		flags |= REVERSE_SORT;
> > -		arg++;
> > -	}
> > -	if (starts_with(arg, "version:")) {
> > +
> > +	if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
> >  		*sort = VERCMP_SORT;
> > -		arg += 8;
> > -	} else if (starts_with(arg, "v:")) {
> > -		*sort = VERCMP_SORT;
> > -		arg += 2;
> > -	} else
> > -		*sort = STRCMP_SORT;
> > +
> 
> By losing "*sort = STRCMP_SORT", the version after this patch would
> stop clearing what is pointed by opt->value, so
> 
> 	git tag --sort=v:refname --sort=refname
> 
> would no longer implement the "last one wins" semantics, no?
> 
> Am I misreading the patch?  I somehow thought Peff's original was
> clearing the variable...
> 
> >  	if (strcmp(arg, "refname"))
> >  		die(_("unsupported sort specification %s"), arg);
> >  	*sort |= flags;

Indeed. My patch fixes this up but I will re-work this so we don't
introduce an inbetween bug :)

Thanks,
Jake

[PATCH 1/3] tag: use skip_prefix instead of magic numbers

[Jacob Keller]

From: Jeff King <peff@peff.net>

Make the parsing of the --sort parameter more readable by having
skip_prefix keep our pointer up to date.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Fixed issue with patch in that we dropped the reset to STRCMP_SORT, discovered
by Junio.

 builtin/tag.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index ef765563388c..9d7643f127e7 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -524,18 +524,14 @@ static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
 	int *sort = opt->value;
 	int flags = 0;
 
-	if (*arg == '-') {
+	if (skip_prefix(arg, "-", &arg))
 		flags |= REVERSE_SORT;
-		arg++;
-	}
-	if (starts_with(arg, "version:")) {
+
+	if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
 		*sort = VERCMP_SORT;
-		arg += 8;
-	} else if (starts_with(arg, "v:")) {
-		*sort = VERCMP_SORT;
-		arg += 2;
-	} else
+	else
 		*sort = STRCMP_SORT;
+
 	if (strcmp(arg, "refname"))
 		die(_("unsupported sort specification %s"), arg);
 	*sort |= flags;
-- 
2.0.1.475.g9b8d714

Re: [PATCH 1/3] tag: use skip_prefix instead of magic numbers

[Jeff King]

On Fri, Jul 11, 2014 at 03:55:45PM -0700, Jacob Keller wrote:

> From: Jeff King <peff@peff.net>
> 
> Make the parsing of the --sort parameter more readable by having
> skip_prefix keep our pointer up to date.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> Fixed issue with patch in that we dropped the reset to STRCMP_SORT, discovered
> by Junio.

I think what Junio queued in ce85604468 is already correct, so we are
fine as long we build on that.

-Peff

[PATCH 1/3] tag: use skip_prefix instead of magic numbers

[Jacob Keller]

From: Jeff King <peff@peff.net>

Make the parsing of the --sort parameter more readable by having
skip_prefix keep our pointer up to date.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Fixed authorship. I don't expect this version to be taken, but it helps me in
review, and I figured it is good to send the whole series.

 builtin/tag.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index ef765563388c..7ccb6f3c581b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -524,18 +524,12 @@ static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
 	int *sort = opt->value;
 	int flags = 0;
 
-	if (*arg == '-') {
+	if (skip_prefix(arg, "-", &arg))
 		flags |= REVERSE_SORT;
-		arg++;
-	}
-	if (starts_with(arg, "version:")) {
+
+	if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
 		*sort = VERCMP_SORT;
-		arg += 8;
-	} else if (starts_with(arg, "v:")) {
-		*sort = VERCMP_SORT;
-		arg += 2;
-	} else
-		*sort = STRCMP_SORT;
+
 	if (strcmp(arg, "refname"))
 		die(_("unsupported sort specification %s"), arg);
 	*sort |= flags;
-- 
2.0.1.475.g9b8d714

[PATCH 1/3] tag: use skip_prefix instead of magic numbers

[Jacob Keller]

Make the parsing of the --sort parameter more readable by having
skip_prefix keep our pointer up to date.

Authored-by: Jeff King <peff@peff.net>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 builtin/tag.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index ef765563388c..7ccb6f3c581b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -524,18 +524,12 @@ static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
 	int *sort = opt->value;
 	int flags = 0;
 
-	if (*arg == '-') {
+	if (skip_prefix(arg, "-", &arg))
 		flags |= REVERSE_SORT;
-		arg++;
-	}
-	if (starts_with(arg, "version:")) {
+
+	if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
 		*sort = VERCMP_SORT;
-		arg += 8;
-	} else if (starts_with(arg, "v:")) {
-		*sort = VERCMP_SORT;
-		arg += 2;
-	} else
-		*sort = STRCMP_SORT;
+
 	if (strcmp(arg, "refname"))
 		die(_("unsupported sort specification %s"), arg);
 	*sort |= flags;
-- 
2.0.1.475.g9b8d714

Re: [PATCH 1/3] tag: use skip_prefix instead of magic numbers

[Jeff King]

On Fri, Jul 11, 2014 at 10:24:05AM -0700, Jacob Keller wrote:

> Make the parsing of the --sort parameter more readable by having
> skip_prefix keep our pointer up to date.
> 
> Authored-by: Jeff King <peff@peff.net>

I suspect Junio may just apply this on the version of the commit he has
upstream, so you may not need this as part of your series.

However, for reference, the right way to handle authorship is with a

  From: Jeff King <peff@peff.net>

at the top of your message body. Git-am will pick that up and turn it
into the author field of the commit.

-Peff

Re: [PATCH 1/3] tag: use skip_prefix instead of magic numbers

[Keller, Jacob E]

On Fri, 2014-07-11 at 13:50 -0400, Jeff King wrote:
> On Fri, Jul 11, 2014 at 10:24:05AM -0700, Jacob Keller wrote:
> 
> > Make the parsing of the --sort parameter more readable by having
> > skip_prefix keep our pointer up to date.
> > 
> > Authored-by: Jeff King <peff@peff.net>
> 
> I suspect Junio may just apply this on the version of the commit he has
> upstream, so you may not need this as part of your series.
> 
> However, for reference, the right way to handle authorship is with a
> 
>   From: Jeff King <peff@peff.net>
> 
> at the top of your message body. Git-am will pick that up and turn it
> into the author field of the commit.
> 

Alright, thanks.

Regards,
Jake

> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] tag: use skip_prefix instead of magic numbers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jul 11, 2014 at 10:24:05AM -0700, Jacob Keller wrote:
>
>> Make the parsing of the --sort parameter more readable by having
>> skip_prefix keep our pointer up to date.
>> 
>> Authored-by: Jeff King <peff@peff.net>
>
> I suspect Junio may just apply this on the version of the commit he has
> upstream, so you may not need this as part of your series.

You read me correctly ;-)  That is what I was planning to do, to
fork jk/tag-sort topic on top of the updated jk/skip-prefix topic.

> However, for reference, the right way to handle authorship is with a
>
>   From: Jeff King <peff@peff.net>
>
> at the top of your message body. Git-am will pick that up and turn it
> into the author field of the commit.

Thanks.