{"thread":{"id":"37115","subject":"[PATCH 1/3] tag: use skip_prefix instead of magic numbers","startedAt":"2014-07-11T20:51:33Z","lastAt":"2014-07-11T22:30:18Z","messageCount":8,"participants":["Jacob Keller","Keller, Jacob E","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"245825","messageId":"1405111895-17451-1-git-send-email-jacob.e.keller@intel.com","threadId":"37115","inReplyTo":null,"subject":"[PATCH 1/3] tag: use skip_prefix instead of magic numbers","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T20:51:33Z","receivedAt":"2014-07-11T20:51:33Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nMake the parsing of the --sort parameter more readable by having\nskip_prefix keep our pointer up to date.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Jacob Keller <jacob.e.keller@intel.com>\n---\nFixed authorship. I don't expect this version to be taken, but it helps me in\nreview, and I figured it is good to send the whole series.\n\n builtin/tag.c | 14 ++++----------\n 1 file changed, 4 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex ef765563388c..7ccb6f3c581b 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -524,18 +524,12 @@ static int parse_opt_sort(const struct option *opt, const char *arg, int unset)\n \tint *sort = opt->value;\n \tint flags = 0;\n \n-\tif (*arg == '-') {\n+\tif (skip_prefix(arg, \"-\", &arg))\n \t\tflags |= REVERSE_SORT;\n-\t\targ++;\n-\t}\n-\tif (starts_with(arg, \"version:\")) {\n+\n+\tif (skip_prefix(arg, \"version:\", &arg) || skip_prefix(arg, \"v:\", &arg))\n \t\t*sort = VERCMP_SORT;\n-\t\targ += 8;\n-\t} else if (starts_with(arg, \"v:\")) {\n-\t\t*sort = VERCMP_SORT;\n-\t\targ += 2;\n-\t} else\n-\t\t*sort = STRCMP_SORT;\n+\n \tif (strcmp(arg, \"refname\"))\n \t\tdie(_(\"unsupported sort specification %s\"), arg);\n \t*sort |= flags;\n-- \n2.0.1.475.g9b8d714\n"},{"id":"245827","messageId":"1405111895-17451-2-git-send-email-jacob.e.keller@intel.com","threadId":"37115","inReplyTo":"1405111895-17451-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH 2/3] tag: fix --sort tests to use cat<<-\\EOF format","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T20:51:34Z","receivedAt":"2014-07-11T20:51:34Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"The --sort tests should use the better format for >expect to maintain\nindenting and ensure that no substitution is occurring. This makes\nparsing and understanding the tests a bit easier.\n\nSigned-off-by: Jacob Keller <jacob.e.keller@intel.com>\n---\n t/t7004-tag.sh | 40 ++++++++++++++++++++--------------------\n 1 file changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex e4ab0f5b6419..66010b0e7066 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1385,41 +1385,41 @@ test_expect_success 'lexical sort' '\n \tgit tag foo1.6 &&\n \tgit tag foo1.10 &&\n \tgit tag -l --sort=refname \"foo*\" >actual &&\n-\tcat >expect <<EOF &&\n-foo1.10\n-foo1.3\n-foo1.6\n-EOF\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.10\n+\tfoo1.3\n+\tfoo1.6\n+\tEOF\n \ttest_cmp expect actual\n '\n \n test_expect_success 'version sort' '\n \tgit tag -l --sort=version:refname \"foo*\" >actual &&\n-\tcat >expect <<EOF &&\n-foo1.3\n-foo1.6\n-foo1.10\n-EOF\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.3\n+\tfoo1.6\n+\tfoo1.10\n+\tEOF\n \ttest_cmp expect actual\n '\n \n test_expect_success 'reverse version sort' '\n \tgit tag -l --sort=-version:refname \"foo*\" >actual &&\n-\tcat >expect <<EOF &&\n-foo1.10\n-foo1.6\n-foo1.3\n-EOF\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.10\n+\tfoo1.6\n+\tfoo1.3\n+\tEOF\n \ttest_cmp expect actual\n '\n \n test_expect_success 'reverse lexical sort' '\n \tgit tag -l --sort=-refname \"foo*\" >actual &&\n-\tcat >expect <<EOF &&\n-foo1.6\n-foo1.3\n-foo1.10\n-EOF\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.6\n+\tfoo1.3\n+\tfoo1.10\n+\tEOF\n \ttest_cmp expect actual\n '\n \n-- \n2.0.1.475.g9b8d714\n"},{"id":"245826","messageId":"1405111895-17451-3-git-send-email-jacob.e.keller@intel.com","threadId":"37115","inReplyTo":"1405111895-17451-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH 3/3] tag: support configuring --sort via .gitconfig","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T20:51:35Z","receivedAt":"2014-07-11T20:51:35Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"Add support for configuring default sort ordering for git tags. Command\nline option will override this configured value, using the exact same\nsyntax.\n\nCc: Jeff King <peff@peff.net>\nSigned-off-by: Jacob Keller <jacob.e.keller@intel.com>\n---\nUpdated based on Junio's suggestions, as well as making sure that we don't bail\nif we can't understand config's option. We still print error message followed\nby a warning about using default order. In addition, added a few more tests to\nensure that these work as expected.\n\n Documentation/config.txt  |  5 ++++\n Documentation/git-tag.txt |  5 +++-\n builtin/tag.c             | 61 ++++++++++++++++++++++++++++++++++-------------\n t/t7004-tag.sh            | 36 ++++++++++++++++++++++++++++\n 4 files changed, 90 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1d718bdb9662..c55c22ab7be9 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2354,6 +2354,11 @@ submodule.<name>.ignore::\n \t\"--ignore-submodules\" option. The 'git submodule' commands are not\n \taffected by this setting.\n \n+tag.sort::\n+\tThis variable controls the sort ordering of tags when displayed by\n+\tlinkgit:git-tag[1]. Without the \"--sort=<value>\" option provided, the\n+\tvalue of this variable will be used as the default.\n+\n tar.umask::\n \tThis variable can be used to restrict the permission bits of\n \ttar archive entries.  The default is 0002, which turns off the\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex b424a1bc48bb..320908369f06 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -99,7 +99,9 @@ OPTIONS\n \tSort in a specific order. Supported type is \"refname\"\n \t(lexicographic order), \"version:refname\" or \"v:refname\" (tag\n \tnames are treated as versions). Prepend \"-\" to reverse sort\n-\torder.\n+\torder. When this option is not given, the sort order defaults to the\n+\tvalue configured for the 'tag.sort' variable if it exists, or\n+\tlexicographic order otherwise. See linkgit:git-config[1].\n \n --column[=<options>]::\n --no-column::\n@@ -317,6 +319,7 @@ include::date-formats.txt[]\n SEE ALSO\n --------\n linkgit:git-check-ref-format[1].\n+linkgit:git-config[1].\n \n GIT\n ---\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 7ccb6f3c581b..cb37b26159a6 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -32,6 +32,8 @@ static const char * const git_tag_usage[] = {\n #define SORT_MASK       0x7fff\n #define REVERSE_SORT    0x8000\n \n+static int tag_sort;\n+\n struct tag_filter {\n \tconst char **patterns;\n \tint lines;\n@@ -346,9 +348,46 @@ static const char tag_template_nocleanup[] =\n \t\"Lines starting with '%c' will be kept; you may remove them\"\n \t\" yourself if you want to.\\n\");\n \n+/*\n+ * Parse a sort string, and return 0 if parsed successfully. Will return\n+ * non-zero when the sort string does not parse into a known type.\n+ */\n+static int parse_sort_string(const char *arg, int *sort)\n+{\n+\tint type = 0, flags = 0;\n+\n+\tif (skip_prefix(arg, \"-\", &arg))\n+\t\tflags |= REVERSE_SORT;\n+\n+\tif (skip_prefix(arg, \"version:\", &arg) || skip_prefix(arg, \"v:\", &arg))\n+\t\ttype = VERCMP_SORT;\n+\telse\n+\t\ttype = STRCMP_SORT;\n+\n+\tif (strcmp(arg, \"refname\"))\n+\t\treturn error(_(\"unsupported sort specification %s\"), arg);\n+\n+\t*sort = (type | flags);\n+\n+\treturn 0;\n+}\n+\n static int git_tag_config(const char *var, const char *value, void *cb)\n {\n-\tint status = git_gpg_config(var, value, cb);\n+\tint status;\n+\n+\tif (!strcmp(var, \"tag.sort\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\tstatus = parse_sort_string(value, &tag_sort);\n+\t\tif (status) {\n+\t\t\twarning(\"using default lexicographic sort order\");\n+\t\t\ttag_sort = STRCMP_SORT;\n+\t\t}\n+\t\treturn 0;\n+\t}\n+\n+\tstatus = git_gpg_config(var, value, cb);\n \tif (status)\n \t\treturn status;\n \tif (starts_with(var, \"column.\"))\n@@ -522,18 +561,8 @@ static int parse_opt_points_at(const struct option *opt __attribute__((unused)),\n static int parse_opt_sort(const struct option *opt, const char *arg, int unset)\n {\n \tint *sort = opt->value;\n-\tint flags = 0;\n \n-\tif (skip_prefix(arg, \"-\", &arg))\n-\t\tflags |= REVERSE_SORT;\n-\n-\tif (skip_prefix(arg, \"version:\", &arg) || skip_prefix(arg, \"v:\", &arg))\n-\t\t*sort = VERCMP_SORT;\n-\n-\tif (strcmp(arg, \"refname\"))\n-\t\tdie(_(\"unsupported sort specification %s\"), arg);\n-\t*sort |= flags;\n-\treturn 0;\n+\treturn parse_sort_string(arg, sort);\n }\n \n int cmd_tag(int argc, const char **argv, const char *prefix)\n@@ -546,7 +575,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tstruct create_tag_options opt;\n \tchar *cleanup_arg = NULL;\n \tint annotate = 0, force = 0, lines = -1;\n-\tint cmdmode = 0, sort = 0;\n+\tint cmdmode = 0;\n \tconst char *msgfile = NULL, *keyid = NULL;\n \tstruct msg_arg msg = { 0, STRBUF_INIT };\n \tstruct commit_list *with_commit = NULL;\n@@ -572,7 +601,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT__FORCE(&force, N_(\"replace the tag if exists\")),\n \t\tOPT_COLUMN(0, \"column\", &colopts, N_(\"show tag list in columns\")),\n \t\t{\n-\t\t\tOPTION_CALLBACK, 0, \"sort\", &sort, N_(\"type\"), N_(\"sort tags\"),\n+\t\t\tOPTION_CALLBACK, 0, \"sort\", &tag_sort, N_(\"type\"), N_(\"sort tags\"),\n \t\t\tPARSE_OPT_NONEG, parse_opt_sort\n \t\t},\n \n@@ -628,9 +657,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tcopts.padding = 2;\n \t\t\trun_column_filter(colopts, &copts);\n \t\t}\n-\t\tif (lines != -1 && sort)\n+\t\tif (lines != -1 && tag_sort)\n \t\t\tdie(_(\"--sort and -n are incompatible\"));\n-\t\tret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, sort);\n+\t\tret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, tag_sort);\n \t\tif (column_active(colopts))\n \t\t\tstop_column_filter();\n \t\treturn ret;\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 66010b0e7066..036665308841 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1423,6 +1423,42 @@ test_expect_success 'reverse lexical sort' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'configured lexical sort' '\n+\tgit config tag.sort \"v:refname\" &&\n+\tgit tag -l \"foo*\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.3\n+\tfoo1.6\n+\tfoo1.10\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'option override configured sort' '\n+\tgit tag -l --sort=-refname \"foo*\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.6\n+\tfoo1.3\n+\tfoo1.10\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'invalid sort parameter on command line' '\n+\ttest_must_fail git tag -l --sort=notvalid \"foo*\" >actual\n+'\n+\n+test_expect_success 'invalid sort parameter in configuratoin' '\n+\tgit config tag.sort \"v:notvalid\" &&\n+\tgit tag -l \"foo*\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.10\n+\tfoo1.3\n+\tfoo1.6\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n run_with_limited_stack () {\n \t(ulimit -s 64 && \"$@\")\n }\n-- \n2.0.1.475.g9b8d714\n"},{"id":"245828","messageId":"1405112090.22963.45.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37115","inReplyTo":"1405111895-17451-3-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T20:54:50Z","receivedAt":"2014-07-11T20:54:50Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, 2014-07-11 at 13:51 -0700, Jacob Keller wrote:\n> Add support for configuring default sort ordering for git tags. Command\n> line option will override this configured value, using the exact same\n> syntax.\n> \n> Cc: Jeff King <peff@peff.net>\n> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>\n> ---\n> Updated based on Junio's suggestions, as well as making sure that we don't bail\n> if we can't understand config's option. We still print error message followed\n> by a warning about using default order. In addition, added a few more tests to\n> ensure that these work as expected.\n> \n>  Documentation/config.txt  |  5 ++++\n>  Documentation/git-tag.txt |  5 +++-\n>  builtin/tag.c             | 61 ++++++++++++++++++++++++++++++++++-------------\n>  t/t7004-tag.sh            | 36 ++++++++++++++++++++++++++++\n>  4 files changed, 90 insertions(+), 17 deletions(-)\n> \n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 1d718bdb9662..c55c22ab7be9 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2354,6 +2354,11 @@ submodule.<name>.ignore::\n>  \t\"--ignore-submodules\" option. The 'git submodule' commands are not\n>  \taffected by this setting.\n>  \n> +tag.sort::\n> +\tThis variable controls the sort ordering of tags when displayed by\n> +\tlinkgit:git-tag[1]. Without the \"--sort=<value>\" option provided, the\n> +\tvalue of this variable will be used as the default.\n> +\n>  tar.umask::\n>  \tThis variable can be used to restrict the permission bits of\n>  \ttar archive entries.  The default is 0002, which turns off the\n> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\n> index b424a1bc48bb..320908369f06 100644\n> --- a/Documentation/git-tag.txt\n> +++ b/Documentation/git-tag.txt\n> @@ -99,7 +99,9 @@ OPTIONS\n>  \tSort in a specific order. Supported type is \"refname\"\n>  \t(lexicographic order), \"version:refname\" or \"v:refname\" (tag\n>  \tnames are treated as versions). Prepend \"-\" to reverse sort\n> -\torder.\n> +\torder. When this option is not given, the sort order defaults to the\n> +\tvalue configured for the 'tag.sort' variable if it exists, or\n> +\tlexicographic order otherwise. See linkgit:git-config[1].\n>  \n>  --column[=<options>]::\n>  --no-column::\n> @@ -317,6 +319,7 @@ include::date-formats.txt[]\n>  SEE ALSO\n>  --------\n>  linkgit:git-check-ref-format[1].\n> +linkgit:git-config[1].\n>  \n>  GIT\n>  ---\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 7ccb6f3c581b..cb37b26159a6 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -32,6 +32,8 @@ static const char * const git_tag_usage[] = {\n>  #define SORT_MASK       0x7fff\n>  #define REVERSE_SORT    0x8000\n>  \n> +static int tag_sort;\n> +\n>  struct tag_filter {\n>  \tconst char **patterns;\n>  \tint lines;\n> @@ -346,9 +348,46 @@ static const char tag_template_nocleanup[] =\n>  \t\"Lines starting with '%c' will be kept; you may remove them\"\n>  \t\" yourself if you want to.\\n\");\n>  \n> +/*\n> + * Parse a sort string, and return 0 if parsed successfully. Will return\n> + * non-zero when the sort string does not parse into a known type.\n> + */\n> +static int parse_sort_string(const char *arg, int *sort)\n> +{\n> +\tint type = 0, flags = 0;\n> +\n> +\tif (skip_prefix(arg, \"-\", &arg))\n> +\t\tflags |= REVERSE_SORT;\n> +\n> +\tif (skip_prefix(arg, \"version:\", &arg) || skip_prefix(arg, \"v:\", &arg))\n> +\t\ttype = VERCMP_SORT;\n> +\telse\n> +\t\ttype = STRCMP_SORT;\n> +\n> +\tif (strcmp(arg, \"refname\"))\n> +\t\treturn error(_(\"unsupported sort specification %s\"), arg);\n> +\n> +\t*sort = (type | flags);\n> +\n> +\treturn 0;\n> +}\n> +\n>  static int git_tag_config(const char *var, const char *value, void *cb)\n>  {\n> -\tint status = git_gpg_config(var, value, cb);\n> +\tint status;\n> +\n> +\tif (!strcmp(var, \"tag.sort\")) {\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(var);\n> +\t\tstatus = parse_sort_string(value, &tag_sort);\n> +\t\tif (status) {\n> +\t\t\twarning(\"using default lexicographic sort order\");\n\nOops, I should also use warning(_(\"\")) here as well I believe...\n\nSorry for thrash.\n\nThanks,\nJake\n\n> +\t\t\ttag_sort = STRCMP_SORT;\n> +\t\t}\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tstatus = git_gpg_config(var, value, cb);\n>  \tif (status)\n>  \t\treturn status;\n>  \tif (starts_with(var, \"column.\"))\n> @@ -522,18 +561,8 @@ static int parse_opt_points_at(const struct option *opt __attribute__((unused)),\n>  static int parse_opt_sort(const struct option *opt, const char *arg, int unset)\n>  {\n>  \tint *sort = opt->value;\n> -\tint flags = 0;\n>  \n> -\tif (skip_prefix(arg, \"-\", &arg))\n> -\t\tflags |= REVERSE_SORT;\n> -\n> -\tif (skip_prefix(arg, \"version:\", &arg) || skip_prefix(arg, \"v:\", &arg))\n> -\t\t*sort = VERCMP_SORT;\n> -\n> -\tif (strcmp(arg, \"refname\"))\n> -\t\tdie(_(\"unsupported sort specification %s\"), arg);\n> -\t*sort |= flags;\n> -\treturn 0;\n> +\treturn parse_sort_string(arg, sort);\n>  }\n>  \n>  int cmd_tag(int argc, const char **argv, const char *prefix)\n> @@ -546,7 +575,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \tstruct create_tag_options opt;\n>  \tchar *cleanup_arg = NULL;\n>  \tint annotate = 0, force = 0, lines = -1;\n> -\tint cmdmode = 0, sort = 0;\n> +\tint cmdmode = 0;\n>  \tconst char *msgfile = NULL, *keyid = NULL;\n>  \tstruct msg_arg msg = { 0, STRBUF_INIT };\n>  \tstruct commit_list *with_commit = NULL;\n> @@ -572,7 +601,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \t\tOPT__FORCE(&force, N_(\"replace the tag if exists\")),\n>  \t\tOPT_COLUMN(0, \"column\", &colopts, N_(\"show tag list in columns\")),\n>  \t\t{\n> -\t\t\tOPTION_CALLBACK, 0, \"sort\", &sort, N_(\"type\"), N_(\"sort tags\"),\n> +\t\t\tOPTION_CALLBACK, 0, \"sort\", &tag_sort, N_(\"type\"), N_(\"sort tags\"),\n>  \t\t\tPARSE_OPT_NONEG, parse_opt_sort\n>  \t\t},\n>  \n> @@ -628,9 +657,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \t\t\tcopts.padding = 2;\n>  \t\t\trun_column_filter(colopts, &copts);\n>  \t\t}\n> -\t\tif (lines != -1 && sort)\n> +\t\tif (lines != -1 && tag_sort)\n>  \t\t\tdie(_(\"--sort and -n are incompatible\"));\n> -\t\tret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, sort);\n> +\t\tret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, tag_sort);\n>  \t\tif (column_active(colopts))\n>  \t\t\tstop_column_filter();\n>  \t\treturn ret;\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index 66010b0e7066..036665308841 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -1423,6 +1423,42 @@ test_expect_success 'reverse lexical sort' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'configured lexical sort' '\n> +\tgit config tag.sort \"v:refname\" &&\n> +\tgit tag -l \"foo*\" >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\tfoo1.3\n> +\tfoo1.6\n> +\tfoo1.10\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'option override configured sort' '\n> +\tgit tag -l --sort=-refname \"foo*\" >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\tfoo1.6\n> +\tfoo1.3\n> +\tfoo1.10\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'invalid sort parameter on command line' '\n> +\ttest_must_fail git tag -l --sort=notvalid \"foo*\" >actual\n> +'\n> +\n> +test_expect_success 'invalid sort parameter in configuratoin' '\n> +\tgit config tag.sort \"v:notvalid\" &&\n> +\tgit tag -l \"foo*\" >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\tfoo1.10\n> +\tfoo1.3\n> +\tfoo1.6\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n> +\n>  run_with_limited_stack () {\n>  \t(ulimit -s 64 && \"$@\")\n>  }\n\n\n"},{"id":"245829","messageId":"20140711210633.GA12546@sigill.intra.peff.net","threadId":"37115","inReplyTo":"1405111895-17451-3-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-11T21:06:33Z","receivedAt":"2014-07-11T21:06:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 11, 2014 at 01:51:35PM -0700, Jacob Keller wrote:\n\n> +\tif (!strcmp(var, \"tag.sort\")) {\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(var);\n> +\t\tstatus = parse_sort_string(value, &tag_sort);\n> +\t\tif (status) {\n> +\t\t\twarning(\"using default lexicographic sort order\");\n> +\t\t\ttag_sort = STRCMP_SORT;\n> +\t\t}\n\nI think this is a good compromise of the issues we discussed earlier. As\nyou noted, it should probably be marked for translation. I'm also not\nsure the message content is clear in all situations. If I have a broken\ntag.sort, I get:\n\n  $ git config tag.sort bogus\n  $ git tag >/dev/null\n  error: unsupported sort specification bogus\n  warning: using default lexicographic sort order\n\nNot too bad, though reminding me that the value \"bogus\" came from\ntag.sort would be nice. But what if I override it:\n\n  $ git tag --sort=v:refname >/dev/null\n  error: unsupported sort specification bogus\n  warning: using default lexicographic sort order\n\nThat's actively wrong, because we are using v:refname from the\ncommand-line. Perhaps we could phrase it like:\n\n  warning: ignoring invalid config option tag.sort\n\nor similar, which helps both cases.\n\nAs an aside, I also think the error line could more clearly mark the\nliteral contents of the variable. E.g., one of:\n\n  error: unsupported sort specification: bogus\n\nor\n\n  error: unsupported sort specification 'bogus'\n\n-Peff\n"},{"id":"245830","messageId":"1405112907.22963.48.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37115","inReplyTo":"20140711210633.GA12546@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T21:08:28Z","receivedAt":"2014-07-11T21:08:28Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, 2014-07-11 at 17:06 -0400, Jeff King wrote:\n> On Fri, Jul 11, 2014 at 01:51:35PM -0700, Jacob Keller wrote:\n> \n> > +\tif (!strcmp(var, \"tag.sort\")) {\n> > +\t\tif (!value)\n> > +\t\t\treturn config_error_nonbool(var);\n> > +\t\tstatus = parse_sort_string(value, &tag_sort);\n> > +\t\tif (status) {\n> > +\t\t\twarning(\"using default lexicographic sort order\");\n> > +\t\t\ttag_sort = STRCMP_SORT;\n> > +\t\t}\n> \n> I think this is a good compromise of the issues we discussed earlier. As\n> you noted, it should probably be marked for translation. I'm also not\n> sure the message content is clear in all situations. If I have a broken\n> tag.sort, I get:\n> \n>   $ git config tag.sort bogus\n>   $ git tag >/dev/null\n>   error: unsupported sort specification bogus\n>   warning: using default lexicographic sort order\n> \n> Not too bad, though reminding me that the value \"bogus\" came from\n> tag.sort would be nice. But what if I override it:\n> \n>   $ git tag --sort=v:refname >/dev/null\n>   error: unsupported sort specification bogus\n>   warning: using default lexicographic sort order\n> \n> That's actively wrong, because we are using v:refname from the\n> command-line. Perhaps we could phrase it like:\n> \n>   warning: ignoring invalid config option tag.sort\n> \n\nok that makes more sense. I can't really avoid printing the warning at\nall since config parsing happens before the option parsing.\n\nI like this wording. I will respin again :D\n\nThanks,\nJake\n\n> or similar, which helps both cases.\n> \n> As an aside, I also think the error line could more clearly mark the\n> literal contents of the variable. E.g., one of:\n> \n>   error: unsupported sort specification: bogus\n> \n> or\n> \n>   error: unsupported sort specification 'bogus'\n> \n> -Peff\n\n\n"},{"id":"245837","messageId":"xmqqha2n35hq.fsf@gitster.dls.corp.google.com","threadId":"37115","inReplyTo":"20140711210633.GA12546@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-11T22:17:21Z","receivedAt":"2014-07-11T22:17:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Jul 11, 2014 at 01:51:35PM -0700, Jacob Keller wrote:\n>\n>> +\tif (!strcmp(var, \"tag.sort\")) {\n>> +\t\tif (!value)\n>> +\t\t\treturn config_error_nonbool(var);\n>> +\t\tstatus = parse_sort_string(value, &tag_sort);\n>> +\t\tif (status) {\n>> +\t\t\twarning(\"using default lexicographic sort order\");\n>> +\t\t\ttag_sort = STRCMP_SORT;\n>> +\t\t}\n>\n> I think this is a good compromise of the issues we discussed earlier. As\n> you noted, it should probably be marked for translation. I'm also not\n> sure the message content is clear in all situations. If I have a broken\n> tag.sort, I get:\n>\n>   $ git config tag.sort bogus\n>   $ git tag >/dev/null\n>   error: unsupported sort specification bogus\n>   warning: using default lexicographic sort order\n>\n> Not too bad, though reminding me that the value \"bogus\" came from\n> tag.sort would be nice. But what if I override it:\n>\n>   $ git tag --sort=v:refname >/dev/null\n>   error: unsupported sort specification bogus\n>   warning: using default lexicographic sort order\n>\n> That's actively wrong, because we are using v:refname from the\n> command-line.  Perhaps we could phrase it like:\n>\n>   warning: ignoring invalid config option tag.sort\n>\n> or similar, which helps both cases.\n\nHmph.  Looks like a mild-enough middle ground, I guess.  As\nparse_sort_string() is private for \"git tag\" implementation, I\nactually would not mind if we taught a bit more context to it and\nphrase its messages differently.  Perhaps something like this, where\nthe config parser will tell what variable it came from with \"var\"\nand the command line parser will pass NULL there.\n\nstatic int parse_sort_string(const char *var, const char *value, int *sort)\n{\n\t...\n\tif (strcmp(value, \"refname\")) {\n\t\tif (!var)\n                \treturn error(_(\"unsupported sort specification '%s'\"), value);\n\t\telse {\n                \twarning(_(\"unsupported sort specification '%s' in variable '%s'\"),\n                        \tvar, value);\n\t\t\treturn -1;\n\t\t}\n\t}\n\n\t*sort = (type | flags);\n\n\treturn 0;\n}\n\n> As an aside, I also think the error line could more clearly mark the\n> literal contents of the variable. E.g., one of:\n>\n>   error: unsupported sort specification: bogus\n>\n> or\n>\n>   error: unsupported sort specification 'bogus'\n\nYup.\n"},{"id":"245842","messageId":"1405117818.22963.54.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37115","inReplyTo":"xmqqha2n35hq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T22:30:18Z","receivedAt":"2014-07-11T22:30:18Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, 2014-07-11 at 15:17 -0700, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n> > On Fri, Jul 11, 2014 at 01:51:35PM -0700, Jacob Keller wrote:\n> >\n> >> +\tif (!strcmp(var, \"tag.sort\")) {\n> >> +\t\tif (!value)\n> >> +\t\t\treturn config_error_nonbool(var);\n> >> +\t\tstatus = parse_sort_string(value, &tag_sort);\n> >> +\t\tif (status) {\n> >> +\t\t\twarning(\"using default lexicographic sort order\");\n> >> +\t\t\ttag_sort = STRCMP_SORT;\n> >> +\t\t}\n> >\n> > I think this is a good compromise of the issues we discussed earlier. As\n> > you noted, it should probably be marked for translation. I'm also not\n> > sure the message content is clear in all situations. If I have a broken\n> > tag.sort, I get:\n> >\n> >   $ git config tag.sort bogus\n> >   $ git tag >/dev/null\n> >   error: unsupported sort specification bogus\n> >   warning: using default lexicographic sort order\n> >\n> > Not too bad, though reminding me that the value \"bogus\" came from\n> > tag.sort would be nice. But what if I override it:\n> >\n> >   $ git tag --sort=v:refname >/dev/null\n> >   error: unsupported sort specification bogus\n> >   warning: using default lexicographic sort order\n> >\n> > That's actively wrong, because we are using v:refname from the\n> > command-line.  Perhaps we could phrase it like:\n> >\n> >   warning: ignoring invalid config option tag.sort\n> >\n> > or similar, which helps both cases.\n> \n> Hmph.  Looks like a mild-enough middle ground, I guess.  As\n> parse_sort_string() is private for \"git tag\" implementation, I\n> actually would not mind if we taught a bit more context to it and\n> phrase its messages differently.  Perhaps something like this, where\n> the config parser will tell what variable it came from with \"var\"\n> and the command line parser will pass NULL there.\n> \n> static int parse_sort_string(const char *var, const char *value, int *sort)\n> {\n> \t...\n> \tif (strcmp(value, \"refname\")) {\n> \t\tif (!var)\n>                 \treturn error(_(\"unsupported sort specification '%s'\"), value);\n> \t\telse {\n>                 \twarning(_(\"unsupported sort specification '%s' in variable '%s'\"),\n>                         \tvar, value);\n> \t\t\treturn -1;\n> \t\t}\n> \t}\n> \n> \t*sort = (type | flags);\n> \n> \treturn 0;\n> }\n> \n\nOk..  I suppose that could be done.\n\nThanks,\nJake\n\n> > As an aside, I also think the error line could more clearly mark the\n> > literal contents of the variable. E.g., one of:\n> >\n> >   error: unsupported sort specification: bogus\n> >\n> > or\n> >\n> >   error: unsupported sort specification 'bogus'\n> \n> Yup.\n\n\n"}]}