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

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

Authored-by: Jeff King [off-list ref]
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

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

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 <redacted>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Updated to include changes due to Junio's feedback. This has not resolved
whether we should fail on a configuration error or simply warn. It appears that
we actually seem to error out more than warn, so I am unsure what the correct
action is here.

 Documentation/config.txt  |  5 +++++
 Documentation/git-tag.txt |  5 ++++-
 builtin/tag.c             | 56 +++++++++++++++++++++++++++++++++--------------
 t/t7004-tag.sh            | 25 +++++++++++++++++++++
 4 files changed, 74 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..f14aa346e0d4 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,41 @@ 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);
+		return parse_sort_string(value, &tag_sort);
+	}
+
+	status = git_gpg_config(var, value, cb);
 	if (status)
 		return status;
 	if (starts_with(var, "column."))

@@ -522,18 +556,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 +570,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 +596,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 +652,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..b1c9409eaa3f 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh

@@ -1423,6 +1423,31 @@ 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' '
+	test_must_fail git tag -l --sort=notvalid "foo*" >actual
+'
+
 run_with_limited_stack () {
 	(ulimit -s 64 && "$@")
 }

-- 
2.0.1.475.g9b8d714

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

Updated to include changes due to Junio's feedback. This has not resolved
whether we should fail on a configuration error or simply warn. It appears that
we actually seem to error out more than warn, so I am unsure what the correct
action is here.

Yeah, we're quite inconsistent there. In some cases we silently ignore
something unknown (e.g., a color.diff.* slot that we do not understand),
but in most cases if it is a config key we understand but a value we do
not, we complain and die.

It's probably user-unfriendly to be silent for those cases, though. The
user gets no feedback on why their config value is doing nothing.

I tend to think that warning is not much better than erroring out. It is
helpful if you are running a single-shot of an old version (which is
something that I do a lot when testing old versions), but would quickly
become irritating if you were actually using an old version of git
day-to-day.

I dunno. Maybe it is worth making life easier for people in the former
category.

+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;
+}

Regardless of how we handle the error, I think this version that
assembles the final bitfield at the end is easier to read than the
original.

-Peff

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 [off-list ref]

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 [off-list ref]

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

-Peff

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:

quoted

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

Authored-by: Jeff King [off-list ref]

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 [off-list ref]

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

On Fri, 2014-07-11 at 13:46 -0400, Jeff King wrote:

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

quoted

Updated to include changes due to Junio's feedback. This has not resolved
whether we should fail on a configuration error or simply warn. It appears that
we actually seem to error out more than warn, so I am unsure what the correct
action is here.

Yeah, we're quite inconsistent there. In some cases we silently ignore
something unknown (e.g., a color.diff.* slot that we do not understand),
but in most cases if it is a config key we understand but a value we do
not, we complain and die.

It's probably user-unfriendly to be silent for those cases, though. The
user gets no feedback on why their config value is doing nothing.

I tend to think that warning is not much better than erroring out. It is
helpful if you are running a single-shot of an old version (which is
something that I do a lot when testing old versions), but would quickly
become irritating if you were actually using an old version of git
day-to-day.

I dunno. Maybe it is worth making life easier for people in the former
category.

quoted

+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;
+}

Regardless of how we handle the error, I think this version that
assembles the final bitfield at the end is easier to read than the
original.

Yes, I figured setting it up all at the end makes more sense, and is
less prone to subtle bugs, since we don't directly modify sort using |=
or relying on particular values of sort initially.

I personally prefer error out on options, even though it can make it a
bit more difficult, though as far as I know unknown fields simply warn
or are ignored. (ie: old versions of git just ignore unknown fields in
configuration).

It's possible we should warn instead though, so that older gits work
with new sorts that they don't understand.

I am ok with warning but I don't know the best practice for how to warn
here instead of failing. Returning error causes a fatal "bad config"
message. Any thoughts?

Thanks,
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

On Fri, Jul 11, 2014 at 06:11:08PM +0000, Keller, Jacob E wrote:

I personally prefer error out on options, even though it can make it a
bit more difficult, though as far as I know unknown fields simply warn
or are ignored. (ie: old versions of git just ignore unknown fields in
configuration).

Right, we _have_ to ignore unknown config options, because we
specifically allow other programs built on git to store their config
with us (and anyway, our callback style of parsing means that no single
callback knows about all of the keys).

In the past we have staked out particular areas of the namespace,
though. E.g., the diff code said "I own all of color.diff.*, and if you
put in something I don't understand, I'll complain". That ended up being
annoying, and now we ignore slots we don't understand there.

So old gits will always silently ignore tag.sort if they don't know
about it, and we can't change that. The only thing we can change is:

It's possible we should warn instead though, so that older gits work
with new sorts that they don't understand.

Right. I think other config variables in similar situations will barf.
This is backwards-compatible as long as the new variables are a superset
(i.e., we only add new understood values, never remove or change the
meaning of existing values). It's just not forwards-compatible.

I am ok with warning but I don't know the best practice for how to warn
here instead of failing. Returning error causes a fatal "bad config"
message. Any thoughts?

The simplest thing is ignoring the return from parse_sort_string and
just calling "return 0". That will still say "error:", but continue on.
If you really want it to say "warning:", I think you'll have to pass a
flag into parse_sort_string. I'm not sure if it's worth the effort.

-Peff

On Fri, 2014-07-11 at 14:22 -0400, Jeff King wrote:

On Fri, Jul 11, 2014 at 06:11:08PM +0000, Keller, Jacob E wrote:

quoted

I personally prefer error out on options, even though it can make it a
bit more difficult, though as far as I know unknown fields simply warn
or are ignored. (ie: old versions of git just ignore unknown fields in
configuration).

Right, we _have_ to ignore unknown config options, because we
specifically allow other programs built on git to store their config
with us (and anyway, our callback style of parsing means that no single
callback knows about all of the keys).

In the past we have staked out particular areas of the namespace,
though. E.g., the diff code said "I own all of color.diff.*, and if you
put in something I don't understand, I'll complain". That ended up being
annoying, and now we ignore slots we don't understand there.

So old gits will always silently ignore tag.sort if they don't know
about it, and we can't change that. The only thing we can change is:

quoted

It's possible we should warn instead though, so that older gits work
with new sorts that they don't understand.

Right. I think other config variables in similar situations will barf.
This is backwards-compatible as long as the new variables are a superset
(i.e., we only add new understood values, never remove or change the
meaning of existing values). It's just not forwards-compatible.

So should I respin this so that config option doesn't error out?

quoted

I am ok with warning but I don't know the best practice for how to warn
here instead of failing. Returning error causes a fatal "bad config"
message. Any thoughts?

The simplest thing is ignoring the return from parse_sort_string and
just calling "return 0". That will still say "error:", but continue on.
If you really want it to say "warning:", I think you'll have to pass a
flag into parse_sort_string. I'm not sure if it's worth the effort.

-Peff

Ok this makes sense, I am fine leaving it as error. Should I respin to
make it not die though?

Thanks,
Jake