{"thread":{"id":"37102","subject":"[PATCH 1/2] tag: use skip_prefix instead of magic numbers","startedAt":"2014-07-10T23:52:13Z","lastAt":"2014-07-16T00:55:47Z","messageCount":10,"participants":["Jacob Keller","Junio C Hamano","Keller, Jacob E","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"245739","messageId":"1405036334-8093-1-git-send-email-jacob.e.keller@intel.com","threadId":"37102","inReplyTo":null,"subject":"[PATCH 1/2] tag: use skip_prefix instead of magic numbers","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-10T23:52:13Z","receivedAt":"2014-07-10T23:52:13Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"Make the parsing of the --sort parameter more readable by having\nskip_prefix keep our pointer up to date.\n\nAuthored-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":"245740","messageId":"1405036334-8093-2-git-send-email-jacob.e.keller@intel.com","threadId":"37102","inReplyTo":"1405036334-8093-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH 2/2 v4] tag: support configuring --sort via .gitconfig","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-10T23:52:14Z","receivedAt":"2014-07-10T23:52:14Z","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---\n- v4\n* base on top of suggested change by Jeff King to use skip_prefix instead\n\n Documentation/config.txt | 6 ++++++\n Documentation/git-tag.txt | 1 +\n builtin/tag.c | 46 ++++++++++++++++++++++++++++++++--------------\n t/t7004-tag.sh | 21 +++++++++++++++++++++\n 4 files changed, 60 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1d718bdb9662..ad8e75fed988 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2354,6 +2354,12 @@ 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 is used to control the sort ordering of tags. It is\n+\tinterpreted precisely the same way as \"--sort=\". If --sort is\n+\tgiven on the command line it will override the selection chosen in the\n+\tconfiguration. See linkgit:git-tag[1] for more details.\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..2d246725aeb5 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -317,6 +317,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..a53e27d4e7e4 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -18,6 +18,8 @@\n #include \"sha1-array.h\"\n #include \"column.h\"\n \n+static int tag_sort = 0;\n+\n static const char * const git_tag_usage[] = {\n \tN_(\"git tag [-a|-s|-u ] [-f] [-m |-F ] []\"),\n \tN_(\"git tag -d ...\"),\n@@ -346,9 +348,33 @@ 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+static int parse_sort_string(const char *arg)\n+{\n+\tint sort = 0;\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\tsort = VERCMP_SORT;\n+\n+\tif (strcmp(arg, \"refname\"))\n+\t\tdie(_(\"unsupported sort specification %s\"), arg);\n+\tsort |= flags;\n+\n+\treturn sort;\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\ttag_sort = parse_sort_string(value);\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,17 +548,9 @@ 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+\t*sort = parse_sort_string(arg);\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 }\n \n@@ -546,7 +564,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 +590,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 +646,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 e4ab0f5b6419..1e8300f6ed7c 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1423,6 +1423,27 @@ EOF\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 <actual &&\n+\tcat >expect < writes:\n\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 \n> Signed-off-by: Jacob Keller \n> ---\n> - v4\n> * base on top of suggested change by Jeff King to use skip_prefix instead\n>\n> Documentation/config.txt | 6 ++++++\n> Documentation/git-tag.txt | 1 +\n> builtin/tag.c | 46 ++++++++++++++++++++++++++++++++--------------\n> t/t7004-tag.sh | 21 +++++++++++++++++++++\n> 4 files changed, 60 insertions(+), 14 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 1d718bdb9662..ad8e75fed988 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2354,6 +2354,12 @@ 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 is used to control the sort ordering of tags. It is\n> +\tinterpreted precisely the same way as \"--sort=\". If --sort is\n> +\tgiven on the command line it will override the selection chosen in the\n> +\tconfiguration. See linkgit:git-tag[1] for more details.\n> +\n\nThis is not technically incorrect per-se, but the third sentence\ntalks about \"--sort\" on \"the command line\" while this applies only\nto \"the command line of the 'git tag' command\" and nobody else's\n\"--sort\" option.\n\nPerhaps rephrasing it like this may be easier to understand?\n\n\tWhen \"git tag\" command is used to list existing tags,\n without \"--sort=\" option on the command line,\n\tthe value of this variable is used as the default.\n\nThis way, it would be clear, without explicitly saying anything,\nthat the value will be spelled exactly the same way as you would\nspell the value for the --sort option on the command line.\n\n> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\n> index b424a1bc48bb..2d246725aeb5 100644\n> --- a/Documentation/git-tag.txt\n> +++ b/Documentation/git-tag.txt\n> @@ -317,6 +317,7 @@ include::date-formats.txt[]\n> SEE ALSO\n> --------\n> linkgit:git-check-ref-format[1].\n> +linkgit:git-config[1].\n\nIt is not particularly friendly to readers to refer to\n\"git-config[1]\" from any other page, if done without spelling out\nwhich variable the reader is expected to look up. Some addition\nto the description of the \"--sort\" option is needed if this SEE ALSO\nwere to be any useful, I guess?\n\n\t--sort=::\n ... (existing description) ...\n When this option is not given, the sort order\n defaults to the value configured for the `tag.sort`\n variable, if exists, or lexicographic otherwise.\n\nor something like that, perhaps?\n\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 7ccb6f3c581b..a53e27d4e7e4 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -18,6 +18,8 @@\n> #include \"sha1-array.h\"\n> #include \"column.h\"\n> \n> +static int tag_sort = 0;\n\nPlease do not initialize variables in bss segment to 0 by hand.\n\nIf this variable is meant to take one of these *CMP_SORT values\ndefined as macro later in this file, it is better to define this\nvariable somewhere after and close to the definitions of the macros.\nPerhaps immediately after the \"struct tag_filter\" is declared?\n\n> @@ -346,9 +348,33 @@ 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> +static int parse_sort_string(const char *arg)\n> +{\n> +\tint sort = 0;\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\tsort = VERCMP_SORT;\n> +\n> +\tif (strcmp(arg, \"refname\"))\n> +\t\tdie(_(\"unsupported sort specification %s\"), arg);\n\nHmm. I _thought_ we try to catch unsupported option value coming\nfrom the command line and die but at the same time we try *not* to\ndie but warn and whatever is sensible when the value comes from the\nconfiguration, so that .git/config or $HOME/.gitconfig can be shared\nby those who use different versions of Git.\n\nDo we already have many precedences where we see configuration value\nthat our version of Git do not yet understand?\n\nNot a very strong objection; just something that worries me.\n\n> +\tsort |= flags;\n> +\n> +\treturn sort;\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\ttag_sort = parse_sort_string(value);\n> +\t}\n> +\n\nWhy doesn't this return success after noticing that the variable is\nto be interpreted by this block and nobody else?\n\nWhen there is no reason to have things in a particular order, it is\ncustomary to add new things at the end, not in the front, unless the\nnew thing is so much more important than everything else---but then\nwe are no longer talking about the case where there is no reason to\nhave things in a particular order ;-).\n\nRemainder of the changes to builtin/tag.c looks good.\n\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index e4ab0f5b6419..1e8300f6ed7c 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -1423,6 +1423,27 @@ EOF\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 < +foo1.3\n> +foo1.6\n> +foo1.10\n> +EOF\n> +\ttest_cmp expect actual\n> +'\n\nPlease write the above like so:\n\n\t...\n\tcat >expect <<-\\EOF &&\n foo1.3\n ...\n EOF\n test_cmp expect actual\n\nThe dash immediately after the here-doc redirection lets us indent\nthe data with HT to allow the test boundaries easier to spot, and by\nquoting the token to end here-doc, we relieve the readers from\nhaving to wonder if there are variable substitutions going on that\nthey need to be careful about.\n\nOverall, I think this is done well. Thanks for working on it.\n"},{"id":"245789","messageId":"1405095622.22963.6.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37102","inReplyTo":"xmqqion4543l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T16:20:23Z","receivedAt":"2014-07-11T16:20:23Z","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 08:04 -0700, Junio C Hamano wrote:\n> Jacob Keller writes:\n> \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 \n> > Signed-off-by: Jacob Keller \n> > ---\n> > - v4\n> > * base on top of suggested change by Jeff King to use skip_prefix instead\n> >\n> > Documentation/config.txt | 6 ++++++\n> > Documentation/git-tag.txt | 1 +\n> > builtin/tag.c | 46 ++++++++++++++++++++++++++++++++--------------\n> > t/t7004-tag.sh | 21 +++++++++++++++++++++\n> > 4 files changed, 60 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/Documentation/config.txt b/Documentation/config.txt\n> > index 1d718bdb9662..ad8e75fed988 100644\n> > --- a/Documentation/config.txt\n> > +++ b/Documentation/config.txt\n> > @@ -2354,6 +2354,12 @@ 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 is used to control the sort ordering of tags. It is\n> > +\tinterpreted precisely the same way as \"--sort=\". If --sort is\n> > +\tgiven on the command line it will override the selection chosen in the\n> > +\tconfiguration. See linkgit:git-tag[1] for more details.\n> > +\n> \n> This is not technically incorrect per-se, but the third sentence\n> talks about \"--sort\" on \"the command line\" while this applies only\n> to \"the command line of the 'git tag' command\" and nobody else's\n> \"--sort\" option.\n> \n> Perhaps rephrasing it like this may be easier to understand?\n> \n> \tWhen \"git tag\" command is used to list existing tags,\n> without \"--sort=\" option on the command line,\n> \tthe value of this variable is used as the default.\n> \n> This way, it would be clear, without explicitly saying anything,\n> that the value will be spelled exactly the same way as you would\n> spell the value for the --sort option on the command line.\n> \n\nMakes sense.\n\n> > diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\n> > index b424a1bc48bb..2d246725aeb5 100644\n> > --- a/Documentation/git-tag.txt\n> > +++ b/Documentation/git-tag.txt\n> > @@ -317,6 +317,7 @@ include::date-formats.txt[]\n> > SEE ALSO\n> > --------\n> > linkgit:git-check-ref-format[1].\n> > +linkgit:git-config[1].\n> \n> It is not particularly friendly to readers to refer to\n> \"git-config[1]\" from any other page, if done without spelling out\n> which variable the reader is expected to look up. Some addition\n> to the description of the \"--sort\" option is needed if this SEE ALSO\n> were to be any useful, I guess?\n> \n> \t--sort=::\n> ... (existing description) ...\n> When this option is not given, the sort order\n> defaults to the value configured for the `tag.sort`\n> variable, if exists, or lexicographic otherwise.\n> \n> or something like that, perhaps?\n> \n\nAlright, this sounds good too.\n\n> > diff --git a/builtin/tag.c b/builtin/tag.c\n> > index 7ccb6f3c581b..a53e27d4e7e4 100644\n> > --- a/builtin/tag.c\n> > +++ b/builtin/tag.c\n> > @@ -18,6 +18,8 @@\n> > #include \"sha1-array.h\"\n> > #include \"column.h\"\n> > \n> > +static int tag_sort = 0;\n> \n> Please do not initialize variables in bss segment to 0 by hand.\n> \n> If this variable is meant to take one of these *CMP_SORT values\n> defined as macro later in this file, it is better to define this\n> variable somewhere after and close to the definitions of the macros.\n> Perhaps immediately after the \"struct tag_filter\" is declared?\n> \n\nI put it just above the struct tag_filter now, as this puts it right\nbelow the #defines regarding it's value.\n\n> > @@ -346,9 +348,33 @@ 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> > +static int parse_sort_string(const char *arg)\n> > +{\n> > +\tint sort = 0;\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\tsort = VERCMP_SORT;\n> > +\n> > +\tif (strcmp(arg, \"refname\"))\n> > +\t\tdie(_(\"unsupported sort specification %s\"), arg);\n> \n> Hmm. I _thought_ we try to catch unsupported option value coming\n> from the command line and die but at the same time we try *not* to\n> die but warn and whatever is sensible when the value comes from the\n> configuration, so that .git/config or $HOME/.gitconfig can be shared\n> by those who use different versions of Git.\n> \n> Do we already have many precedences where we see configuration value\n> that our version of Git do not yet understand?\n> \n> Not a very strong objection; just something that worries me.\n> \n\nI like this change, and I think I have a way to handle it. I will\nre-work this so that the die is handled by the normal block.\n\n> > +\tsort |= flags;\n> > +\n> > +\treturn sort;\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\ttag_sort = parse_sort_string(value);\n> > +\t}\n> > +\n> \n> Why doesn't this return success after noticing that the variable is\n> to be interpreted by this block and nobody else?\n> \n\nI was copying the git_gpg_config. But yes I agree, returning here would\nmake sense, and actually maybe fix git_gpg_config as well in a future\npatch.\n\n> When there is no reason to have things in a particular order, it is\n> customary to add new things at the end, not in the front, unless the\n> new thing is so much more important than everything else---but then\n> we are no longer talking about the case where there is no reason to\n> have things in a particular order ;-).\n> \n> Remainder of the changes to builtin/tag.c looks good.\n\nThanks.\n\n> \n> > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> > index e4ab0f5b6419..1e8300f6ed7c 100755\n> > --- a/t/t7004-tag.sh\n> > +++ b/t/t7004-tag.sh\n> > @@ -1423,6 +1423,27 @@ EOF\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 < > +foo1.3\n> > +foo1.6\n> > +foo1.10\n> > +EOF\n> > +\ttest_cmp expect actual\n> > +'\n> \n> Please write the above like so:\n> \n> \t...\n> \tcat >expect <<-\\EOF &&\n> foo1.3\n> ...\n> EOF\n> test_cmp expect actual\n> \n> The dash immediately after the here-doc redirection lets us indent\n> the data with HT to allow the test boundaries easier to spot, and by\n> quoting the token to end here-doc, we relieve the readers from\n> having to wonder if there are variable substitutions going on that\n> they need to be careful about.\n> \n> Overall, I think this is done well. Thanks for working on it.\n> --\n\nYep. I'll try to have a new version soon.\n\nRegards,\nJake\n\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n\n\n"},{"id":"245791","messageId":"xmqqtx6n507e.fsf@gitster.dls.corp.google.com","threadId":"37102","inReplyTo":"1405095622.22963.6.camel@jekeller-desk1.amr.corp.intel.com","subject":"Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-11T16:28:37Z","receivedAt":"2014-07-11T16:28:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Keller, Jacob E\" writes:\n\n> On Fri, 2014-07-11 at 08:04 -0700, Junio C Hamano wrote:\n> ...\n>> > +static int tag_sort = 0;\n>> \n>> Please do not initialize variables in bss segment to 0 by hand.\n>> \n>> If this variable is meant to take one of these *CMP_SORT values\n>> defined as macro later in this file, it is better to define this\n>> variable somewhere after and close to the definitions of the macros.\n>> Perhaps immediately after the \"struct tag_filter\" is declared?\n>\n> I put it just above the struct tag_filter now, as this puts it right\n> below the #defines regarding it's value.\n\nEither would be fine, but just to clarify.\n\nBecause these macro definitions are for the .sort field of that\nstructure, and the new tag_sort variable is the second user of that\nmacro, my suggestion to put it _after_ was to be in line with \"add\nnew things at the end, when there is no compelling reason not to\"\nbelow.\n\n>> When there is no reason to have things in a particular order, it is\n>> customary to add new things at the end, not in the front, unless the\n>> new thing is so much more important than everything else---but then\n>> we are no longer talking about the case where there is no reason to\n>> have things in a particular order ;-).\n\nThanks.\n"},{"id":"245794","messageId":"1405096840.22963.8.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37102","inReplyTo":"xmqqion4543l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T16:40:40Z","receivedAt":"2014-07-11T16:40:40Z","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 08:04 -0700, Junio C Hamano wrote:\n> Jacob Keller writes:\n> \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 \n> > Signed-off-by: Jacob Keller \n> > ---\n> > - v4\n> > * base on top of suggested change by Jeff King to use skip_prefix instead\n> >\n> > Documentation/config.txt | 6 ++++++\n> > Documentation/git-tag.txt | 1 +\n> > builtin/tag.c | 46 ++++++++++++++++++++++++++++++++--------------\n> > t/t7004-tag.sh | 21 +++++++++++++++++++++\n> > 4 files changed, 60 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/Documentation/config.txt b/Documentation/config.txt\n> > index 1d718bdb9662..ad8e75fed988 100644\n> > --- a/Documentation/config.txt\n> > +++ b/Documentation/config.txt\n> > @@ -2354,6 +2354,12 @@ 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 is used to control the sort ordering of tags. It is\n> > +\tinterpreted precisely the same way as \"--sort=\". If --sort is\n> > +\tgiven on the command line it will override the selection chosen in the\n> > +\tconfiguration. See linkgit:git-tag[1] for more details.\n> > +\n> \n> This is not technically incorrect per-se, but the third sentence\n> talks about \"--sort\" on \"the command line\" while this applies only\n> to \"the command line of the 'git tag' command\" and nobody else's\n> \"--sort\" option.\n> \n> Perhaps rephrasing it like this may be easier to understand?\n> \n> \tWhen \"git tag\" command is used to list existing tags,\n> without \"--sort=\" option on the command line,\n> \tthe value of this variable is used as the default.\n> \n> This way, it would be clear, without explicitly saying anything,\n> that the value will be spelled exactly the same way as you would\n> spell the value for the --sort option on the command line.\n> \n> > diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\n> > index b424a1bc48bb..2d246725aeb5 100644\n> > --- a/Documentation/git-tag.txt\n> > +++ b/Documentation/git-tag.txt\n> > @@ -317,6 +317,7 @@ include::date-formats.txt[]\n> > SEE ALSO\n> > --------\n> > linkgit:git-check-ref-format[1].\n> > +linkgit:git-config[1].\n> \n> It is not particularly friendly to readers to refer to\n> \"git-config[1]\" from any other page, if done without spelling out\n> which variable the reader is expected to look up. Some addition\n> to the description of the \"--sort\" option is needed if this SEE ALSO\n> were to be any useful, I guess?\n> \n> \t--sort=::\n> ... (existing description) ...\n> When this option is not given, the sort order\n> defaults to the value configured for the `tag.sort`\n> variable, if exists, or lexicographic otherwise.\n> \n> or something like that, perhaps?\n> \n> > diff --git a/builtin/tag.c b/builtin/tag.c\n> > index 7ccb6f3c581b..a53e27d4e7e4 100644\n> > --- a/builtin/tag.c\n> > +++ b/builtin/tag.c\n> > @@ -18,6 +18,8 @@\n> > #include \"sha1-array.h\"\n> > #include \"column.h\"\n> > \n> > +static int tag_sort = 0;\n> \n> Please do not initialize variables in bss segment to 0 by hand.\n> \n> If this variable is meant to take one of these *CMP_SORT values\n> defined as macro later in this file, it is better to define this\n> variable somewhere after and close to the definitions of the macros.\n> Perhaps immediately after the \"struct tag_filter\" is declared?\n> \n> > @@ -346,9 +348,33 @@ 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> > +static int parse_sort_string(const char *arg)\n> > +{\n> > +\tint sort = 0;\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\tsort = VERCMP_SORT;\n> > +\n> > +\tif (strcmp(arg, \"refname\"))\n> > +\t\tdie(_(\"unsupported sort specification %s\"), arg);\n> \n> Hmm. I _thought_ we try to catch unsupported option value coming\n> from the command line and die but at the same time we try *not* to\n> die but warn and whatever is sensible when the value comes from the\n> configuration, so that .git/config or $HOME/.gitconfig can be shared\n> by those who use different versions of Git.\n> \n> Do we already have many precedences where we see configuration value\n> that our version of Git do not yet understand?\n> \n> Not a very strong objection; just something that worries me.\n> \n> > +\tsort |= flags;\n> > +\n> > +\treturn sort;\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\ttag_sort = parse_sort_string(value);\n> > +\t}\n> > +\n> \n> Why doesn't this return success after noticing that the variable is\n> to be interpreted by this block and nobody else?\n> \n> When there is no reason to have things in a particular order, it is\n> customary to add new things at the end, not in the front, unless the\n> new thing is so much more important than everything else---but then\n> we are no longer talking about the case where there is no reason to\n> have things in a particular order ;-).\n> \n> Remainder of the changes to builtin/tag.c looks good.\n> \n> > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> > index e4ab0f5b6419..1e8300f6ed7c 100755\n> > --- a/t/t7004-tag.sh\n> > +++ b/t/t7004-tag.sh\n> > @@ -1423,6 +1423,27 @@ EOF\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 < > +foo1.3\n> > +foo1.6\n> > +foo1.10\n> > +EOF\n> > +\ttest_cmp expect actual\n> > +'\n> \n> Please write the above like so:\n> \n> \t...\n> \tcat >expect <<-\\EOF &&\n> foo1.3\n> ...\n> EOF\n> test_cmp expect actual\n> \n> The dash immediately after the here-doc redirection lets us indent\n> the data with HT to allow the test boundaries easier to spot, and by\n> quoting the token to end here-doc, we relieve the readers from\n> having to wonder if there are variable substitutions going on that\n> they need to be careful about.\n\nThis is not how the rest of the current tests work. I will submit a\npatch which fixes up the current --sort tests (but not every test, for\nnow) as well.\n\nThanks,\nJake\n\n> \n> Overall, I think this is done well. Thanks for working on it.\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n\n\n"},{"id":"245798","messageId":"1405099217.22963.11.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37102","inReplyTo":"xmqqion4543l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T17:20:18Z","receivedAt":"2014-07-11T17:20: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 08:04 -0700, Junio C Hamano wrote:\n> Jacob Keller writes:\n> \n\n> > +\n> > +\tif (strcmp(arg, \"refname\"))\n> > +\t\tdie(_(\"unsupported sort specification %s\"), arg);\n> \n> Hmm. I _thought_ we try to catch unsupported option value coming\n> from the command line and die but at the same time we try *not* to\n> die but warn and whatever is sensible when the value comes from the\n> configuration, so that .git/config or $HOME/.gitconfig can be shared\n> by those who use different versions of Git.\n> \n> Do we already have many precedences where we see configuration value\n> that our version of Git do not yet understand?\n> \n> Not a very strong objection; just something that worries me.\n\nBased on a cursory glance at how the config is parsed, we actually\ndie_on_error for most options. Does it makes sense to warn or not? I am\nnot really sure here.. At any rate I updated this to be a bit cleaner\nand use error( instead of die, so that it will work within how config is\nsupposed to.. Plus, this also makes it so that --sort shows the usage if\nit's invalid.\n\nPlease comment more on the new set I am sending so we can really\ndetermine what the correct course of action is here.\n\nRegards,\nJake\n"},{"id":"245816","messageId":"xmqqzjgf3g1j.fsf@gitster.dls.corp.google.com","threadId":"37102","inReplyTo":"1405096840.22963.8.camel@jekeller-desk1.amr.corp.intel.com","subject":"Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-11T18:29:28Z","receivedAt":"2014-07-11T18:29:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Keller, Jacob E\" writes:\n\n> This is not how the rest of the current tests work. I will submit a\n> patch which fixes up the current --sort tests (but not every test, for\n> now) as well.\n\nI do not want to pile more work that is unrelated to the task at\nhand on your plate, i.e. clean-up work, so I would assign a very low\npriority to \"fix up the current tests\". At the same time, existing\nmistakes are not excuses to introduce new similar ones, hence my\nsuggestions to the added code in the patch I saw.\n"},{"id":"245824","messageId":"1405111022.22963.44.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37102","inReplyTo":"xmqqzjgf3g1j.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T20:37:02Z","receivedAt":"2014-07-11T20:37:02Z","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 11:29 -0700, Junio C Hamano wrote:\n> \"Keller, Jacob E\" writes:\n> \n> > This is not how the rest of the current tests work. I will submit a\n> > patch which fixes up the current --sort tests (but not every test, for\n> > now) as well.\n> \n> I do not want to pile more work that is unrelated to the task at\n> hand on your plate, i.e. clean-up work, so I would assign a very low\n> priority to \"fix up the current tests\". At the same time, existing\n> mistakes are not excuses to introduce new similar ones, hence my\n> suggestions to the added code in the patch I saw.\n> \n\nIt was trivial to fix at least the --sort tests, so I submitted a patch\nthat goes before this one to fix those as well.\n\nThanks,\nJake\n"},{"id":"246147","messageId":"CACsJy8DOB1Uic=p+aP6T4LNbS5X9XUaBN-2qC1ZyC0XqeeGOuQ@mail.gmail.com","threadId":"37102","inReplyTo":"1405036334-8093-2-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-16T00:55:47Z","receivedAt":"2014-07-16T00:55:47Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jul 11, 2014 at 6:52 AM, 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\nKinda off topic, but I wish we could show these default overriding\nsettings when GIT_TRACE is set. E.g. if this knob is set, GIT_TRACE\nwould show 'git' 'tag' '--version=....' even though --version is not\nspecified from command line. Or some other way to know which knobs are\nactive when a command is run.\n-- \nDuy\n"}]}