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

[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 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 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 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

[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] 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>
---
Made parse_sort_string take a "var" parameter, and if given will only warn
about invalid parameter, instead of error.

 Documentation/config.txt  |  5 ++++
 Documentation/git-tag.txt |  5 +++-
 builtin/tag.c             | 66 ++++++++++++++++++++++++++++++++++-------------
 t/t7004-tag.sh            | 36 ++++++++++++++++++++++++++
 4 files changed, 93 insertions(+), 19 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 9d7643f127e7..97c5317c28e5 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,49 @@ 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 *var, const char *value, int *sort)
+{
+	int type = 0, flags = 0;
+
+	if (skip_prefix(value, "-", &value))
+		flags |= REVERSE_SORT;
+
+	if (skip_prefix(value, "version:", &value) || skip_prefix(value, "v:", &value))
+		type = VERCMP_SORT;
+	else
+		type = STRCMP_SORT;
+
+	if (strcmp(value, "refname")) {
+		if (!var)
+			return error(_("unsupported sort specification '%s'"), value);
+		else {
+			warning(_("unsupported sort specification '%s' in variable '%s'"),
+				var, value);
+			return -1;
+		}
+	}
+
+	*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);
+		parse_sort_string(var, value, &tag_sort);
+		return 0;
+	}
+
+	status = git_gpg_config(var, value, cb);
 	if (status)
 		return status;
 	if (starts_with(var, "column."))
@@ -522,20 +564,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;
-	else
-		*sort = STRCMP_SORT;
-
-	if (strcmp(arg, "refname"))
-		die(_("unsupported sort specification %s"), arg);
-	*sort |= flags;
-	return 0;
+	return parse_sort_string(NULL, arg, sort);
 }
 
 int cmd_tag(int argc, const char **argv, const char *prefix)
@@ -548,7 +578,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;
@@ -574,7 +604,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
 		},
 
@@ -630,9 +660,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

[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

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

[Eric Sunshine]

On Fri, Jul 11, 2014 at 6:55 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> 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>
> ---
> Made parse_sort_string take a "var" parameter, and if given will only warn
> about invalid parameter, instead of error.

This seems unnecessarily ugly since it's hard-coding specialized
knowledge of the callers' error-reporting requirements into what
should be a generalized parsing function. If you instead make
parse_sort_string() responsible only for attempting to parse the
value, but leave error-reporting to the callers, then this ugliness
goes away. See below.

> diff --git a/builtin/tag.c b/builtin/tag.c
> index 9d7643f127e7..97c5317c28e5 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,49 @@ 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 *var, const char *value, int *sort)
> +{
> +       int type = 0, flags = 0;
> +
> +       if (skip_prefix(value, "-", &value))
> +               flags |= REVERSE_SORT;
> +
> +       if (skip_prefix(value, "version:", &value) || skip_prefix(value, "v:", &value))
> +               type = VERCMP_SORT;
> +       else
> +               type = STRCMP_SORT;
> +
> +       if (strcmp(value, "refname")) {
> +               if (!var)
> +                       return error(_("unsupported sort specification '%s'"), value);
> +               else {
> +                       warning(_("unsupported sort specification '%s' in variable '%s'"),
> +                               var, value);
> +                       return -1;

Just return -1 here, but don't print any diagnostics. Let the callers
do that. (See below.)

> +               }
> +       }
> +
> +       *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);
> +               parse_sort_string(var, value, &tag_sort);

    if (parse_sort_string(value, &tag_sort))
        warning(_("unsupported sort specification '%s' in variable '%s'"),
            var, value);

> +               return 0;
> +       }
> +
> +       status = git_gpg_config(var, value, cb);
>         if (status)
>                 return status;
>         if (starts_with(var, "column."))
> @@ -522,20 +564,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;
> -       else
> -               *sort = STRCMP_SORT;
> -
> -       if (strcmp(arg, "refname"))
> -               die(_("unsupported sort specification %s"), arg);
> -       *sort |= flags;
> -       return 0;
> +       return parse_sort_string(NULL, arg, sort);

    if (parse_sort_string(arg, sort))
        return error(_("unsupported sort specification '%s'"), arg);
    return 0;

>  }
>
>  int cmd_tag(int argc, const char **argv, const char *prefix)
> @@ -548,7 +578,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;
> @@ -574,7 +604,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
>                 },
>
> @@ -630,9 +660,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;

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> Made parse_sort_string take a "var" parameter, and if given will only warn
>> about invalid parameter, instead of error.
>
> This seems unnecessarily ugly since it's hard-coding specialized
> knowledge of the callers' error-reporting requirements into what
> should be a generalized parsing function. If you instead make
> parse_sort_string() responsible only for attempting to parse the
> value, but leave error-reporting to the callers, then this ugliness
> goes away. See below.

Yup, you are absolutely right.  Thanks for catching my silly.

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

[Jeff King]

On Sun, Jul 13, 2014 at 10:10:27AM -0700, Junio C Hamano wrote:

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> >> Made parse_sort_string take a "var" parameter, and if given will only warn
> >> about invalid parameter, instead of error.
> >
> > This seems unnecessarily ugly since it's hard-coding specialized
> > knowledge of the callers' error-reporting requirements into what
> > should be a generalized parsing function. If you instead make
> > parse_sort_string() responsible only for attempting to parse the
> > value, but leave error-reporting to the callers, then this ugliness
> > goes away. See below.
> 
> Yup, you are absolutely right.  Thanks for catching my silly.

I do not know if it is that silly. The reason we push the error
reporting down into reusable library functions, even though it is less
flexible or causes us to have "quiet" flags and such, is that the
library function knows more about the specific error.

In this case we are just saying "your sort specification is not valid",
so it is not adding much value, and returning "-1" provides enough
information for the caller to say that.  But would we eventually want to
diagnose errors more specifically? For example, in "foo:refname", we
could complain that "foo" is not a valid sort function. And in
"version:bar", we could complain that "bar" is not a valid sorting atom.

We can encode these error types into an enum, but it is often much
easier to report them at the time of discovery (e.g., because you have
the half-parsed string available that says "foo" or "bar"). This is a
general problem throughout a lot of our code.  In higher level languages
you might throw an exception with the error message and let the caller
decide how to report it. I wonder if it is too gross to do something
like:

  push_error_handler(collect_errors);

  /* all calls to error() in will push error text onto a stack */
  do_something();

  pop_error_handler();

  /* traverse the list, calling warning() on each, and clear the stack */
  print_errors_as_warnings();

One can imagine print_errors_as_errors, which would be useful when a
caller is not sure whether a full operation will succeed (e.g., you try
X, then Y, and only when both fail do you report an error). Or a caller
which does not call any print_error_* at all (i.e., replacing the
"quiet" flag that many library functions take).

I realize that I am reinventing the error-reporting wheel on a sleepy
Sunday afternoon without having thought about it much, so there is
probably some gotcha or case that makes this ugly, or perhaps it just
ends up verbose in practice. But one can dream.

-Peff

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

[Jeff King]

On Sun, Jul 13, 2014 at 01:33:56PM -0400, Jeff King wrote:

> I realize that I am reinventing the error-reporting wheel on a sleepy
> Sunday afternoon without having thought about it much, so there is
> probably some gotcha or case that makes this ugly, or perhaps it just
> ends up verbose in practice. But one can dream.

Just for fun...

---
diff --git a/builtin/tag.c b/builtin/tag.c
index 5e0744b..0f5be3b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -379,7 +379,10 @@ static int git_tag_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "tag.sort")) {
 		if (!value)
 			return config_error_nonbool(var);
-		parse_sort_string(value, &tag_sort);
+		set_error_routine(collect_errors);
+		if (parse_sort_string(value, &tag_sort) < 0)
+			print_errors(warning, "#{error} in config option 'tag.sort'");
+		pop_error_routine();
 		return 0;
 	}
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 9de3180..6bf91a6 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -346,6 +346,11 @@ extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_lis
 extern void set_error_routine(void (*routine)(const char *err, va_list params));
 extern void set_die_is_recursing_routine(int (*routine)(void));
 
+void pop_error_routine(void);
+void collect_errors(const char *err, va_list params);
+__attribute__((format (printf, 2, 3)))
+void print_errors(void (*func)(const char *, ...), const char *fmt, ...);
+
 extern int starts_with(const char *str, const char *prefix);
 extern int ends_with(const char *str, const char *suffix);
 
diff --git a/usage.c b/usage.c
index ed14645..055ccc7 100644
--- a/usage.c
+++ b/usage.c
@@ -57,10 +57,16 @@ static int die_is_recursing_builtin(void)
  * (ugh), so keep things static. */
 static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) = usage_builtin;
 static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_builtin;
-static void (*error_routine)(const char *err, va_list params) = error_builtin;
 static void (*warn_routine)(const char *err, va_list params) = warn_builtin;
 static int (*die_is_recursing)(void) = die_is_recursing_builtin;
 
+struct error_func_list {
+	void (*func)(const char *, va_list);
+	struct error_func_list *next;
+};
+static struct error_func_list default_error_func = { error_builtin };
+static struct error_func_list *error_funcs = &default_error_func;
+
 void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params))
 {
 	die_routine = routine;
@@ -68,7 +74,84 @@ void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list param
 
 void set_error_routine(void (*routine)(const char *err, va_list params))
 {
-	error_routine = routine;
+	struct error_func_list *efl = xmalloc(sizeof(*efl));
+	efl->func = routine;
+	efl->next = error_funcs;
+	error_funcs = efl;
+}
+
+void pop_error_routine(void)
+{
+	while (error_funcs != &default_error_func) {
+		struct error_func_list *efl = error_funcs;
+		error_funcs = efl->next;
+		free(efl);
+	}
+}
+
+struct error_list {
+	struct strbuf buf;
+	struct error_list *next;
+};
+struct error_list *error_list;
+struct error_list **error_list_tail = &error_list;
+
+void collect_errors(const char *fmt, va_list params)
+{
+	struct error_list *err = xmalloc(sizeof(*err));
+
+	strbuf_init(&err->buf, 0);
+	strbuf_vaddf(&err->buf, fmt, params);
+	err->next = NULL;
+	*error_list_tail = err;
+	error_list_tail = &err->next;
+}
+
+static void clear_errors(void)
+{
+	while (error_list) {
+		struct error_list *next = error_list->next;
+		strbuf_release(&error_list->buf);
+		free(error_list);
+		error_list = next;
+	}
+	error_list_tail = &error_list;
+}
+
+void print_errors(void (*func)(const char *, ...), const char *fmt, ...)
+{
+	struct strbuf buf = STRBUF_INIT;
+	va_list params;
+	int prefix_len, suffix_start;
+	const char *p;
+	struct error_list *el;
+
+	va_start(params, fmt);
+	strbuf_vaddf(&buf, fmt, params);
+	va_end(params);
+
+	/*
+	 * The intent here is that callers will put #{error} in their fmt
+	 * string. Doing two layers of interpolation is gross, because we may
+	 * accidentally find "#{error}" in one of the substitutions, not the
+	 * original fmt. Ideally we would do it all in a single pass (and call
+	 * #{error} "%M" or something), but that would require extending
+	 * vsprintf, and there is no way to do that portably.
+	 */
+	p = strstr(buf.buf, "#{error}");
+	if (!p)
+		die("BUG: error printer does not want to print error!?");
+	prefix_len = p - buf.buf;
+	suffix_start = prefix_len + strlen("#{error}");
+
+	for (el = error_list; el; el = el->next)
+		func("%.*s%.*s%.*s",
+		     prefix_len, buf.buf,
+		     el->buf.len, el->buf.buf,
+		     buf.len - suffix_start, buf.buf + suffix_start);
+
+	strbuf_release(&buf);
+	clear_errors();
 }
 
 void set_die_is_recursing_routine(int (*routine)(void))
@@ -144,7 +227,7 @@ int error(const char *err, ...)
 	va_list params;
 
 	va_start(params, err);
-	error_routine(err, params);
+	error_funcs->func(err, params);
 	va_end(params);
 	return -1;
 }

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Jul 13, 2014 at 01:33:56PM -0400, Jeff King wrote:
>
>> I realize that I am reinventing the error-reporting wheel on a sleepy
>> Sunday afternoon without having thought about it much, so there is
>> probably some gotcha or case that makes this ugly, or perhaps it just
>> ends up verbose in practice. But one can dream.
>
> Just for fun...

Yes, that is fun.

I actually think your "In 'version:pefname' and 'wersion:refname',
we want be able to report 'pefname' and 'wersion' are misspelled,
and returning -1 or enum would not cut it" is a good argument.  The
callee wants to have flexibility on _what_ to report, just as the
caller wants to have flexibility on _how_.  In this particular code
path, I think the former far outweighs the latter, and my suggestion
I called "silly" might not be so silly but may have struck the right
balance.  I dunno.

If you absolutely need to have both, you would need something like
your approach, of course, but I am not sure if it is worth it.

I am not sure how well this meshes with i18n (I know the "for fun"
does not even attempt to, but if we tried to, I suspect it may
become even uglier).  We would also need to override both error and
warning routines and have the reporter tag the errors in these two
categories, no?

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

[Keller, Jacob E]

On Mon, 2014-07-14 at 10:17 -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Sun, Jul 13, 2014 at 01:33:56PM -0400, Jeff King wrote:
> >
> >> I realize that I am reinventing the error-reporting wheel on a sleepy
> >> Sunday afternoon without having thought about it much, so there is
> >> probably some gotcha or case that makes this ugly, or perhaps it just
> >> ends up verbose in practice. But one can dream.
> >
> > Just for fun...
> 
> Yes, that is fun.
> 
> I actually think your "In 'version:pefname' and 'wersion:refname',
> we want be able to report 'pefname' and 'wersion' are misspelled,
> and returning -1 or enum would not cut it" is a good argument.  The
> callee wants to have flexibility on _what_ to report, just as the
> caller wants to have flexibility on _how_.  In this particular code
> path, I think the former far outweighs the latter, and my suggestion
> I called "silly" might not be so silly but may have struck the right
> balance.  I dunno.
> 
> If you absolutely need to have both, you would need something like
> your approach, of course, but I am not sure if it is worth it.
> 
> I am not sure how well this meshes with i18n (I know the "for fun"
> does not even attempt to, but if we tried to, I suspect it may
> become even uglier).  We would also need to override both error and
> warning routines and have the reporter tag the errors in these two
> categories, no?
> 

Do we want to go this way? Should I respin my patch (again)? I'm not
sure we really need to get that complex.. I do like parsing errors a bit
cleaner and indicating what part is bad.. Note that our current parsing
method does not make it really possible to indicate which part is wrong.

Thanks,
Jake

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

[Junio C Hamano]

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

> On Mon, 2014-07-14 at 10:17 -0700, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>> 
>> > On Sun, Jul 13, 2014 at 01:33:56PM -0400, Jeff King wrote:
>> >
>> >> I realize that I am reinventing the error-reporting wheel on a sleepy
>> >> Sunday afternoon without having thought about it much, so there is
>> >> probably some gotcha or case that makes this ugly, or perhaps it just
>> >> ends up verbose in practice. But one can dream.
>> >
>> > Just for fun...
>> 
>> Yes, that is fun.
>> 
>> I actually think your "In 'version:pefname' and 'wersion:refname',
>> we want be able to report 'pefname' and 'wersion' are misspelled,
>> and returning -1 or enum would not cut it" is a good argument.  The
>> callee wants to have flexibility on _what_ to report, just as the
>> caller wants to have flexibility on _how_.  In this particular code
>> path, I think the former far outweighs the latter, and my suggestion
>> I called "silly" might not be so silly but may have struck the right
>> balance.  I dunno.
>> 
>> If you absolutely need to have both, you would need something like
>> your approach, of course, but I am not sure if it is worth it.
>> 
>> I am not sure how well this meshes with i18n (I know the "for fun"
>> does not even attempt to, but if we tried to, I suspect it may
>> become even uglier).  We would also need to override both error and
>> warning routines and have the reporter tag the errors in these two
>> categories, no?
>
> Do we want to go this way?

I do not speak for Peff, but I personally think this is just a "fun"
demonstration, nothing more, and my gut feeling is that it would
make things unnecessary complex without much real gain to pursue it
further.

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

[Keller, Jacob E]

On Tue, 2014-07-15 at 09:03 -0700, Junio C Hamano wrote:
> "Keller, Jacob E" <jacob.e.keller@intel.com> writes:
> 
> > On Mon, 2014-07-14 at 10:17 -0700, Junio C Hamano wrote:
> >> Jeff King <peff@peff.net> writes:
> >> 
> >> > On Sun, Jul 13, 2014 at 01:33:56PM -0400, Jeff King wrote:
> >> >
> >> >> I realize that I am reinventing the error-reporting wheel on a sleepy
> >> >> Sunday afternoon without having thought about it much, so there is
> >> >> probably some gotcha or case that makes this ugly, or perhaps it just
> >> >> ends up verbose in practice. But one can dream.
> >> >
> >> > Just for fun...
> >> 
> >> Yes, that is fun.
> >> 
> >> I actually think your "In 'version:pefname' and 'wersion:refname',
> >> we want be able to report 'pefname' and 'wersion' are misspelled,
> >> and returning -1 or enum would not cut it" is a good argument.  The
> >> callee wants to have flexibility on _what_ to report, just as the
> >> caller wants to have flexibility on _how_.  In this particular code
> >> path, I think the former far outweighs the latter, and my suggestion
> >> I called "silly" might not be so silly but may have struck the right
> >> balance.  I dunno.
> >> 
> >> If you absolutely need to have both, you would need something like
> >> your approach, of course, but I am not sure if it is worth it.
> >> 
> >> I am not sure how well this meshes with i18n (I know the "for fun"
> >> does not even attempt to, but if we tried to, I suspect it may
> >> become even uglier).  We would also need to override both error and
> >> warning routines and have the reporter tag the errors in these two
> >> categories, no?
> >
> > Do we want to go this way?
> 
> I do not speak for Peff, but I personally think this is just a "fun"
> demonstration, nothing more, and my gut feeling is that it would
> make things unnecessary complex without much real gain to pursue it
> further.

I agree. But what about going back to the older setup where the caller
can output correct error message? I'm ok with using an enum style
return, to be completely honest. I would prefer this, actually.

Thanks,
Jake

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

[Junio C Hamano]

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

> On Tue, 2014-07-15 at 09:03 -0700, Junio C Hamano wrote:
> ...
>> >> Yes, that is fun.
>> >> 
>> >> I actually think your "In 'version:pefname' and 'wersion:refname',
>> >> we want be able to report 'pefname' and 'wersion' are misspelled,
>> >> and returning -1 or enum would not cut it" is a good argument.  The
>> >> callee wants to have flexibility on _what_ to report, just as the
>> >> caller wants to have flexibility on _how_.  In this particular code
>> >> path, I think the former far outweighs the latter, and my suggestion
>> >> I called "silly" might not be so silly but may have struck the right
>> >> balance.  I dunno.
> ...
> I agree. But what about going back to the older setup where the caller
> can output correct error message? I'm ok with using an enum style
> return, to be completely honest. I would prefer this, actually.

Depends on which older setup you mean, I think.  The one that does
not let us easily give more context dependent diagnoses that lets us
distinguish between version:pefname and version:refname by returning
only -1 or an enum?

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

[Keller, Jacob E]

On Tue, 2014-07-15 at 11:17 -0700, Junio C Hamano wrote:
> "Keller, Jacob E" <jacob.e.keller@intel.com> writes:
> 
> > On Tue, 2014-07-15 at 09:03 -0700, Junio C Hamano wrote:
> > ...
> >> >> Yes, that is fun.
> >> >> 
> >> >> I actually think your "In 'version:pefname' and 'wersion:refname',
> >> >> we want be able to report 'pefname' and 'wersion' are misspelled,
> >> >> and returning -1 or enum would not cut it" is a good argument.  The
> >> >> callee wants to have flexibility on _what_ to report, just as the
> >> >> caller wants to have flexibility on _how_.  In this particular code
> >> >> path, I think the former far outweighs the latter, and my suggestion
> >> >> I called "silly" might not be so silly but may have struck the right
> >> >> balance.  I dunno.
> > ...
> > I agree. But what about going back to the older setup where the caller
> > can output correct error message? I'm ok with using an enum style
> > return, to be completely honest. I would prefer this, actually.
> 
> Depends on which older setup you mean, I think.  The one that does
> not let us easily give more context dependent diagnoses that lets us
> distinguish between version:pefname and version:refname by returning
> only -1 or an enum?
> 

I am going to re-submit this with an enum-style return. I am also
changing how we parse so that we can correctly report whether the sort
function or sort atom is incorrect.

Thanks,
Jake

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

[Junio C Hamano]

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

> "Keller, Jacob E" <jacob.e.keller@intel.com> writes:
>
>> On Tue, 2014-07-15 at 09:03 -0700, Junio C Hamano wrote:
>> ...
>>> >> Yes, that is fun.
>>> >> 
>>> >> I actually think your "In 'version:pefname' and 'wersion:refname',
>>> >> we want be able to report 'pefname' and 'wersion' are misspelled,
>>> >> and returning -1 or enum would not cut it" is a good argument.  The
>>> >> callee wants to have flexibility on _what_ to report, just as the
>>> >> caller wants to have flexibility on _how_.  In this particular code
>>> >> path, I think the former far outweighs the latter, and my suggestion
>>> >> I called "silly" might not be so silly but may have struck the right
>>> >> balance.  I dunno.
>> ...
>> I agree. But what about going back to the older setup where the caller
>> can output correct error message? I'm ok with using an enum style
>> return, to be completely honest. I would prefer this, actually.
>
> Depends on which older setup you mean, I think.  The one that does
> not let us easily give more context dependent diagnoses that lets us
> distinguish between version:pefname and version:refname by returning
> only -1 or an enum?

In case it was not clear, I do not think -1 or enum is a good idea.

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

[Junio C Hamano]

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

> I am going to re-submit this with an enum-style return. I am also
> changing how we parse so that we can correctly report whether the sort
> function or sort atom is incorrect.

Oh, our mails crossed, I guess.  As long as it will leave the door
open for later enhancements for more context sensitive error
diagnosis, I do not particularly mind a solution around enum.

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

[Keller, Jacob E]

On Tue, 2014-07-15 at 12:12 -0700, Junio C Hamano wrote:
> "Keller, Jacob E" <jacob.e.keller@intel.com> writes:
> 
> > I am going to re-submit this with an enum-style return. I am also
> > changing how we parse so that we can correctly report whether the sort
> > function or sort atom is incorrect.
> 
> Oh, our mails crossed, I guess.  As long as it will leave the door
> open for later enhancements for more context sensitive error
> diagnosis, I do not particularly mind a solution around enum.

Hmm. I looked at coding it this way, and it actually makes it less
sensitive than I would like. I'm not a fan of the extra "value"
parameter, but I do like a more proper error display, and indeed one
that is more precise.

I'll try to have a new series posted soon which takes these into
account.

Regards,
Jake

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

[Keller, Jacob E]

On Tue, 2014-07-15 at 12:12 -0700, Junio C Hamano wrote:
> "Keller, Jacob E" <jacob.e.keller@intel.com> writes:
> 
> > I am going to re-submit this with an enum-style return. I am also
> > changing how we parse so that we can correctly report whether the sort
> > function or sort atom is incorrect.
> 
> Oh, our mails crossed, I guess.  As long as it will leave the door
> open for later enhancements for more context sensitive error
> diagnosis, I do not particularly mind a solution around enum.

I just sent a v8 of the series. I think I mostly followed Peff's idea of
using a pop_error_routine function, but not as complex as his was. This
overall results in more accurate errors, and doesn't clutter the
original parse_sort_string with too much knowledge about what particular
value is being parsed. Hopefully we can finally converge on a good set
of patches.

Thanks,
Jake

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

[Jeff King]

On Tue, Jul 15, 2014 at 09:03:53AM -0700, Junio C Hamano wrote:

> > Do we want to go this way?
> 
> I do not speak for Peff, but I personally think this is just a "fun"
> demonstration, nothing more, and my gut feeling is that it would
> make things unnecessary complex without much real gain to pursue it
> further.

Yeah, it is a little too complicated for what it is buying us here. If
we had a real error stack with allocated error types or something, then
callers could do more useful programmatic things with it. But:

  1. We usually only care about system errors in such a case, and get by
     with using errno.

  2. I would not want to annotate all of the library-ish functions with
     case-specific return types. That is a lot of work for little gain.

I think my favorite of the suggestions so far is basically the two-line:

  error: sort specification is bad...
  warning: ignoring invalid tag.sort

There are tons of places in git where we already do this kind of
"error chaining" over multiple lines (and multiple calls to error()),
and it doesn't require any new code or techniques.

But what is in v8 is not so bad from my cursory glance.

-Peff

PS I am traveling this week and will probably be a lot less responsive.
   Please don't let me hold up your conversations.