{"thread":{"id":"37116","subject":"[PATCH 2/3] tag: fix --sort tests to use cat<<-\\EOF format","startedAt":"2014-07-11T21:38:42Z","lastAt":"2014-07-11T22:47:12Z","messageCount":5,"participants":["Jacob Keller","Junio C Hamano","Keller, Jacob E"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"245832","messageId":"1405114724-23829-1-git-send-email-jacob.e.keller@intel.com","threadId":"37116","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-11T21:38:42Z","receivedAt":"2014-07-11T21:38:42Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jeff King \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 \nSigned-off-by: Jacob Keller \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":"245831","messageId":"1405114724-23829-2-git-send-email-jacob.e.keller@intel.com","threadId":"37116","inReplyTo":"1405114724-23829-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-11T21:38:43Z","receivedAt":"2014-07-11T21:38:43Z","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 \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 <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 <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 <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 <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":"245833","messageId":"1405114724-23829-3-git-send-email-jacob.e.keller@intel.com","threadId":"37116","inReplyTo":"1405114724-23829-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH 3/3 v7] tag: support configuring --sort via .gitconfig","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T21:38:44Z","receivedAt":"2014-07-11T21:38:44Z","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 \nSigned-off-by: Jacob Keller \n---\nUpdated warning texts based on Jeff's feedback. Also added translate specifier\nto the warning string.\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..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=\" 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[=]::\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..6e0a8ed4d1f9 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(_(\"ignoring configured sort specification from 'tag.sort'\"));\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":"245843","messageId":"xmqqwqbj1poi.fsf@gitster.dls.corp.google.com","threadId":"37116","inReplyTo":"1405114724-23829-1-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH 1/3] tag: use skip_prefix instead of magic numbers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-11T22:44:13Z","receivedAt":"2014-07-11T22:44:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> From: Jeff King \n>\n> Make the parsing of the --sort parameter more readable by having\n> skip_prefix keep our pointer up to date.\n>\n> Signed-off-by: Jeff King \n> Signed-off-by: Jacob Keller \n> ---\n> builtin/tag.c | 14 ++++----------\n> 1 file changed, 4 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 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\nBy losing \"*sort = STRCMP_SORT\", the version after this patch would\nstop clearing what is pointed by opt->value, so\n\n\tgit tag --sort=v:refname --sort=refname\n\nwould no longer implement the \"last one wins\" semantics, no?\n\nAm I misreading the patch? I somehow thought Peff's original was\nclearing the variable...\n\n> \tif (strcmp(arg, \"refname\"))\n> \t\tdie(_(\"unsupported sort specification %s\"), arg);\n> \t*sort |= flags;\n"},{"id":"245846","messageId":"1405118832.22963.60.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37116","inReplyTo":"xmqqwqbj1poi.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/3] tag: use skip_prefix instead of magic numbers","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T22:47:12Z","receivedAt":"2014-07-11T22:47:12Z","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:44 -0700, Junio C Hamano wrote:\n> Jacob Keller writes:\n> \n> > From: Jeff King \n> >\n> > Make the parsing of the --sort parameter more readable by having\n> > skip_prefix keep our pointer up to date.\n> >\n> > Signed-off-by: Jeff King \n> > Signed-off-by: Jacob Keller \n> > ---\n> > builtin/tag.c | 14 ++++----------\n> > 1 file changed, 4 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/builtin/tag.c b/builtin/tag.c\n> > index 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> \n> By losing \"*sort = STRCMP_SORT\", the version after this patch would\n> stop clearing what is pointed by opt->value, so\n> \n> \tgit tag --sort=v:refname --sort=refname\n> \n> would no longer implement the \"last one wins\" semantics, no?\n> \n> Am I misreading the patch? I somehow thought Peff's original was\n> clearing the variable...\n> \n> > \tif (strcmp(arg, \"refname\"))\n> > \t\tdie(_(\"unsupported sort specification %s\"), arg);\n> > \t*sort |= flags;\n\nIndeed. My patch fixes this up but I will re-work this so we don't\nintroduce an inbetween bug :)\n\nThanks,\nJake\n"}]}