{"thread":{"id":"37114","subject":"[PATCH 2/3] tag: fix --sort tests to use cat<<-\\EOF format","startedAt":"2014-07-11T17:24:05Z","lastAt":"2014-07-14T22:08:32Z","messageCount":15,"participants":["Jacob Keller","Jeff King","Keller, Jacob E","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"245800","messageId":"1405099447-27555-1-git-send-email-jacob.e.keller@intel.com","threadId":"37114","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-11T17:24:05Z","receivedAt":"2014-07-11T17:24:05Z","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":"245799","messageId":"1405099447-27555-2-git-send-email-jacob.e.keller@intel.com","threadId":"37114","inReplyTo":"1405099447-27555-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-11T17:24:06Z","receivedAt":"2014-07-11T17:24:06Z","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":"245801","messageId":"1405099447-27555-3-git-send-email-jacob.e.keller@intel.com","threadId":"37114","inReplyTo":"1405099447-27555-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH 3/3 v5] tag: support configuring --sort via .gitconfig","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T17:24:07Z","receivedAt":"2014-07-11T17:24:07Z","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 to include changes due to Junio's feedback. This has not resolved\nwhether we should fail on a configuration error or simply warn. It appears that\nwe actually seem to error out more than warn, so I am unsure what the correct\naction is here.\n\n Documentation/config.txt | 5 +++++\n Documentation/git-tag.txt | 5 ++++-\n builtin/tag.c | 56 +++++++++++++++++++++++++++++++++--------------\n t/t7004-tag.sh | 25 +++++++++++++++++++++\n 4 files changed, 74 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..f14aa346e0d4 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,41 @@ 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\treturn parse_sort_string(value, &tag_sort);\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 +556,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 +570,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 +596,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 +652,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..b1c9409eaa3f 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1423,6 +1423,31 @@ 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' '\n+\ttest_must_fail git tag -l --sort=notvalid \"foo*\" >actual\n+'\n+\n run_with_limited_stack () {\n \t(ulimit -s 64 && \"$@\")\n }\n-- \n2.0.1.475.g9b8d714\n"},{"id":"245805","messageId":"20140711174628.GC7856@sigill.intra.peff.net","threadId":"37114","inReplyTo":"1405099447-27555-3-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-11T17:46:28Z","receivedAt":"2014-07-11T17:46:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 11, 2014 at 10:24:07AM -0700, Jacob Keller wrote:\n\n> Updated to include changes due to Junio's feedback. This has not resolved\n> whether we should fail on a configuration error or simply warn. It appears that\n> we actually seem to error out more than warn, so I am unsure what the correct\n> action is here.\n\nYeah, we're quite inconsistent there. In some cases we silently ignore\nsomething unknown (e.g., a color.diff.* slot that we do not understand),\nbut in most cases if it is a config key we understand but a value we do\nnot, we complain and die.\n\nIt's probably user-unfriendly to be silent for those cases, though. The\nuser gets no feedback on why their config value is doing nothing.\n\nI tend to think that warning is not much better than erroring out. It is\nhelpful if you are running a single-shot of an old version (which is\nsomething that I do a lot when testing old versions), but would quickly\nbecome irritating if you were actually using an old version of git\nday-to-day.\n\nI dunno. Maybe it is worth making life easier for people in the former\ncategory.\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\nRegardless of how we handle the error, I think this version that\nassembles the final bitfield at the end is easier to read than the\noriginal.\n\n-Peff\n"},{"id":"245806","messageId":"20140711175056.GE7856@sigill.intra.peff.net","threadId":"37114","inReplyTo":"1405099447-27555-1-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH 1/3] tag: use skip_prefix instead of magic numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-11T17:50:56Z","receivedAt":"2014-07-11T17:50:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 11, 2014 at 10:24:05AM -0700, Jacob Keller wrote:\n\n> Make the parsing of the --sort parameter more readable by having\n> skip_prefix keep our pointer up to date.\n> \n> Authored-by: Jeff King \n\nI suspect Junio may just apply this on the version of the commit he has\nupstream, so you may not need this as part of your series.\n\nHowever, for reference, the right way to handle authorship is with a\n\n From: Jeff King \n\nat the top of your message body. Git-am will pick that up and turn it\ninto the author field of the commit.\n\n-Peff\n"},{"id":"245809","messageId":"1405101883.22963.29.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37114","inReplyTo":"20140711175056.GE7856@sigill.intra.peff.net","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-11T18:04:43Z","receivedAt":"2014-07-11T18:04:43Z","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:50 -0400, Jeff King wrote:\n> On Fri, Jul 11, 2014 at 10:24:05AM -0700, Jacob Keller wrote:\n> \n> > Make the parsing of the --sort parameter more readable by having\n> > skip_prefix keep our pointer up to date.\n> > \n> > Authored-by: Jeff King \n> \n> I suspect Junio may just apply this on the version of the commit he has\n> upstream, so you may not need this as part of your series.\n> \n> However, for reference, the right way to handle authorship is with a\n> \n> From: Jeff King \n> \n> at the top of your message body. Git-am will pick that up and turn it\n> into the author field of the commit.\n> \n\nAlright, thanks.\n\nRegards,\nJake\n\n> -Peff\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":"245810","messageId":"1405102267.22963.35.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37114","inReplyTo":"20140711174628.GC7856@sigill.intra.peff.net","subject":"Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T18:11:08Z","receivedAt":"2014-07-11T18:11:08Z","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:46 -0400, Jeff King wrote:\n> On Fri, Jul 11, 2014 at 10:24:07AM -0700, Jacob Keller wrote:\n> \n> > Updated to include changes due to Junio's feedback. This has not resolved\n> > whether we should fail on a configuration error or simply warn. It appears that\n> > we actually seem to error out more than warn, so I am unsure what the correct\n> > action is here.\n> \n> Yeah, we're quite inconsistent there. In some cases we silently ignore\n> something unknown (e.g., a color.diff.* slot that we do not understand),\n> but in most cases if it is a config key we understand but a value we do\n> not, we complain and die.\n> \n> It's probably user-unfriendly to be silent for those cases, though. The\n> user gets no feedback on why their config value is doing nothing.\n> \n> I tend to think that warning is not much better than erroring out. It is\n> helpful if you are running a single-shot of an old version (which is\n> something that I do a lot when testing old versions), but would quickly\n> become irritating if you were actually using an old version of git\n> day-to-day.\n> \n> I dunno. Maybe it is worth making life easier for people in the former\n> category.\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> Regardless of how we handle the error, I think this version that\n> assembles the final bitfield at the end is easier to read than the\n> original.\n> \n\nYes, I figured setting it up all at the end makes more sense, and is\nless prone to subtle bugs, since we don't directly modify sort using |=\nor relying on particular values of sort initially.\n\nI personally prefer error out on options, even though it can make it a\nbit more difficult, though as far as I know unknown fields simply warn\nor are ignored. (ie: old versions of git just ignore unknown fields in\nconfiguration).\n\nIt's possible we should warn instead though, so that older gits work\nwith new sorts that they don't understand.\n\nI am ok with warning but I don't know the best practice for how to warn\nhere instead of failing. Returning error causes a fatal \"bad config\"\nmessage. Any thoughts?\n\nThanks,\nJake\n\n> -Peff\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":"245812","messageId":"20140711182214.GF7856@sigill.intra.peff.net","threadId":"37114","inReplyTo":"1405102267.22963.35.camel@jekeller-desk1.amr.corp.intel.com","subject":"Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-11T18:22:14Z","receivedAt":"2014-07-11T18:22:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 11, 2014 at 06:11:08PM +0000, Keller, Jacob E wrote:\n\n> I personally prefer error out on options, even though it can make it a\n> bit more difficult, though as far as I know unknown fields simply warn\n> or are ignored. (ie: old versions of git just ignore unknown fields in\n> configuration).\n\nRight, we _have_ to ignore unknown config options, because we\nspecifically allow other programs built on git to store their config\nwith us (and anyway, our callback style of parsing means that no single\ncallback knows about all of the keys).\n\nIn the past we have staked out particular areas of the namespace,\nthough. E.g., the diff code said \"I own all of color.diff.*, and if you\nput in something I don't understand, I'll complain\". That ended up being\nannoying, and now we ignore slots we don't understand there.\n\nSo old gits will always silently ignore tag.sort if they don't know\nabout it, and we can't change that. The only thing we can change is:\n\n> It's possible we should warn instead though, so that older gits work\n> with new sorts that they don't understand.\n\nRight. I think other config variables in similar situations will barf.\nThis is backwards-compatible as long as the new variables are a superset\n(i.e., we only add new understood values, never remove or change the\nmeaning of existing values). It's just not forwards-compatible.\n\n> I am ok with warning but I don't know the best practice for how to warn\n> here instead of failing. Returning error causes a fatal \"bad config\"\n> message. Any thoughts?\n\nThe simplest thing is ignoring the return from parse_sort_string and\njust calling \"return 0\". That will still say \"error:\", but continue on.\nIf you really want it to say \"warning:\", I think you'll have to pass a\nflag into parse_sort_string. I'm not sure if it's worth the effort.\n\n-Peff\n"},{"id":"245817","messageId":"xmqqvbr33ff0.fsf@gitster.dls.corp.google.com","threadId":"37114","inReplyTo":"20140711175056.GE7856@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] tag: use skip_prefix instead of magic numbers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-11T18:42:59Z","receivedAt":"2014-07-11T18:42:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jul 11, 2014 at 10:24:05AM -0700, Jacob Keller wrote:\n>\n>> Make the parsing of the --sort parameter more readable by having\n>> skip_prefix keep our pointer up to date.\n>> \n>> Authored-by: Jeff King \n>\n> I suspect Junio may just apply this on the version of the commit he has\n> upstream, so you may not need this as part of your series.\n\nYou read me correctly ;-) That is what I was planning to do, to\nfork jk/tag-sort topic on top of the updated jk/skip-prefix topic.\n\n> However, for reference, the right way to handle authorship is with a\n>\n> From: Jeff King \n>\n> at the top of your message body. Git-am will pick that up and turn it\n> into the author field of the commit.\n\nThanks.\n"},{"id":"245823","messageId":"1405110973.22963.43.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37114","inReplyTo":"20140711182214.GF7856@sigill.intra.peff.net","subject":"Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T20:36:13Z","receivedAt":"2014-07-11T20:36:13Z","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 14:22 -0400, Jeff King wrote:\n> On Fri, Jul 11, 2014 at 06:11:08PM +0000, Keller, Jacob E wrote:\n> \n> > I personally prefer error out on options, even though it can make it a\n> > bit more difficult, though as far as I know unknown fields simply warn\n> > or are ignored. (ie: old versions of git just ignore unknown fields in\n> > configuration).\n> \n> Right, we _have_ to ignore unknown config options, because we\n> specifically allow other programs built on git to store their config\n> with us (and anyway, our callback style of parsing means that no single\n> callback knows about all of the keys).\n> \n> In the past we have staked out particular areas of the namespace,\n> though. E.g., the diff code said \"I own all of color.diff.*, and if you\n> put in something I don't understand, I'll complain\". That ended up being\n> annoying, and now we ignore slots we don't understand there.\n> \n> So old gits will always silently ignore tag.sort if they don't know\n> about it, and we can't change that. The only thing we can change is:\n> \n> > It's possible we should warn instead though, so that older gits work\n> > with new sorts that they don't understand.\n> \n> Right. I think other config variables in similar situations will barf.\n> This is backwards-compatible as long as the new variables are a superset\n> (i.e., we only add new understood values, never remove or change the\n> meaning of existing values). It's just not forwards-compatible.\n> \n\nSo should I respin this so that config option doesn't error out?\n\n> > I am ok with warning but I don't know the best practice for how to warn\n> > here instead of failing. Returning error causes a fatal \"bad config\"\n> > message. Any thoughts?\n> \n> The simplest thing is ignoring the return from parse_sort_string and\n> just calling \"return 0\". That will still say \"error:\", but continue on.\n> If you really want it to say \"warning:\", I think you'll have to pass a\n> flag into parse_sort_string. I'm not sure if it's worth the effort.\n> \n> -Peff\n\nOk this makes sense, I am fine leaving it as error. Should I respin to\nmake it not die though?\n\nThanks,\nJake\n"},{"id":"245834","messageId":"xmqqmwcf36jy.fsf@gitster.dls.corp.google.com","threadId":"37114","inReplyTo":"20140711174628.GC7856@sigill.intra.peff.net","subject":"Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-11T21:54:25Z","receivedAt":"2014-07-11T21:54:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jul 11, 2014 at 10:24:07AM -0700, Jacob Keller wrote:\n>\n>> Updated to include changes due to Junio's feedback. This has not resolved\n>> whether we should fail on a configuration error or simply warn. It appears that\n>> we actually seem to error out more than warn, so I am unsure what the correct\n>> action is here.\n>\n> Yeah, we're quite inconsistent there. In some cases we silently ignore\n> something unknown (e.g., a color.diff.* slot that we do not understand),\n> but in most cases if it is a config key we understand but a value we do\n> not, we complain and die.\n\nHm, that's bad---we've become less and less careful over time,\nperhaps?\n\nAs we want to be able to enhance semantics of existing configuration\nvariables without having to introduce new but similar ones, we would\nreally want to make sure that those who share the same .git/config\nor $HOME/.gitconfig across different versions of Git would not have\nto suffer too much (i.e. forcing them to \"config --unset\" when using\ntheir older Git is not nice).\n\n> It's probably user-unfriendly to be silent for those cases, though. The\n> user gets no feedback on why their config value is doing nothing.\n>\n> I tend to think that warning is not much better than erroring out. It is\n> helpful if you are running a single-shot of an old version (which is\n> something that I do a lot when testing old versions), but would quickly\n> become irritating if you were actually using an old version of git\n> day-to-day.\n>\n> I dunno. Maybe it is worth making life easier for people in the former\n> category.\n\n... \"former cat\" meaning \"less irritating for single-shot use\"? I\ndunno...\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> Regardless of how we handle the error, I think this version that\n> assembles the final bitfield at the end is easier to read than the\n> original.\n\nYes, this part really is nicely done, I agree.\n"},{"id":"245836","messageId":"1405116767.22963.53.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37114","inReplyTo":"xmqqmwcf36jy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T22:12:48Z","receivedAt":"2014-07-11T22:12:48Z","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 14:54 -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > On Fri, Jul 11, 2014 at 10:24:07AM -0700, Jacob Keller wrote:\n> >\n> >> Updated to include changes due to Junio's feedback. This has not resolved\n> >> whether we should fail on a configuration error or simply warn. It appears that\n> >> we actually seem to error out more than warn, so I am unsure what the correct\n> >> action is here.\n> >\n> > Yeah, we're quite inconsistent there. In some cases we silently ignore\n> > something unknown (e.g., a color.diff.* slot that we do not understand),\n> > but in most cases if it is a config key we understand but a value we do\n> > not, we complain and die.\n> \n> Hm, that's bad---we've become less and less careful over time,\n> perhaps?\n> \n> As we want to be able to enhance semantics of existing configuration\n> variables without having to introduce new but similar ones, we would\n> really want to make sure that those who share the same .git/config\n> or $HOME/.gitconfig across different versions of Git would not have\n> to suffer too much (i.e. forcing them to \"config --unset\" when using\n> their older Git is not nice).\n> \n> > It's probably user-unfriendly to be silent for those cases, though. The\n> > user gets no feedback on why their config value is doing nothing.\n> >\n> > I tend to think that warning is not much better than erroring out. It is\n> > helpful if you are running a single-shot of an old version (which is\n> > something that I do a lot when testing old versions), but would quickly\n> > become irritating if you were actually using an old version of git\n> > day-to-day.\n> >\n> > I dunno. Maybe it is worth making life easier for people in the former\n> > category.\n> \n> ... \"former cat\" meaning \"less irritating for single-shot use\"? I\n> dunno...\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> > Regardless of how we handle the error, I think this version that\n> > assembles the final bitfield at the end is easier to read than the\n> > original.\n> \n> Yes, this part really is nicely done, I agree.\n\nThe current revision of the patch prints an error and warning about not\nusing the configured tag value. It does still run. I added a test to\nensure this as well.\n\nThe easiest way to change overall behavior is to change the git-config's\n\"die_on_error\" to be false, so that we no longer die on configuration\nerrors.\n\nIt appears this would resolve the issue for many configuration options\n(not all, as I think a few are still hard coded to die) but it would be\na change that is well outside the scope of this patch.\n\nI think that for now behavior I added is good, as we *know* that\ntag.sort will add new parameters in the near future, so it makes no\nsense to have it die on a config that is only slightly newer than the\ngit version.\n\nGlad I could help. Junio if you could review the v7 I sent a bit ago for\nany possible mistakes that I forgot to clean up that would be great.\n\nThanks,\nJake\n"},{"id":"245877","messageId":"20140712011735.GB17349@sigill.intra.peff.net","threadId":"37114","inReplyTo":"xmqqmwcf36jy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-12T01:17:35Z","receivedAt":"2014-07-12T01:17:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 11, 2014 at 02:54:25PM -0700, Junio C Hamano wrote:\n\n> > Yeah, we're quite inconsistent there. In some cases we silently ignore\n> > something unknown (e.g., a color.diff.* slot that we do not understand),\n> > but in most cases if it is a config key we understand but a value we do\n> > not, we complain and die.\n> \n> Hm, that's bad---we've become less and less careful over time,\n> perhaps?\n\nI don't think so. I think we've always been not-very-lenient with\nparsing values. Two examples:\n\n 1. We sometimes extend a boolean variable to take additional values\n (like \"auto\" or \"always\"). Older gits will see those values and say\n \"not a boolean, and also not a number\" and barf.\n branch.autosetupmerge is an example of this (it learned \"always\" in\n 9ed36cf.\n\n 2. The parser for colors translates things like \"bold\" into ANSI\n codes. If it doesn't understand a keyword, it dies. If we add new\n color names or attributes, they will cause older git to die.\n\n There are many cases like this. Pretty much any enumerated value\n (like branch.autosetuprebase, for example) will face this whenever\n we add new possible values.\n\nSo I do not think we have ever had a rule, but if we did, it is probably\n\"silently ignore unknown keys, complain on unknown values\". Which makes\nwarning on tag.sort unlike most of the rest of the code.\n\nI do not mind it, though (I am one of the people it is most likely to\nhelp :) ). Just giving some context.\n\n-Peff\n"},{"id":"245951","messageId":"xmqqk37h19dc.fsf@gitster.dls.corp.google.com","threadId":"37114","inReplyTo":"20140712011735.GB17349@sigill.intra.peff.net","subject":"Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-13T17:01:03Z","receivedAt":"2014-07-13T17:01:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jul 11, 2014 at 02:54:25PM -0700, Junio C Hamano wrote:\n>\n>> > Yeah, we're quite inconsistent there. In some cases we silently ignore\n>> > something unknown (e.g., a color.diff.* slot that we do not understand),\n>> > but in most cases if it is a config key we understand but a value we do\n>> > not, we complain and die.\n>> \n>> Hm, that's bad---we've become less and less careful over time,\n>> perhaps?\n>\n> I don't think so. I think we've always been not-very-lenient with\n> parsing values. Two examples:\n> ...\n> So I do not think we have ever had a rule, but if we did, it is probably\n> \"silently ignore unknown keys, complain on unknown values\".\n\nYeah, somehow I was mixing up these two cases fuzzily in my mind\nwhile responding. Rejecting unknown keys is bad, but we cannot be\nperfectly forward compatible nor behave sensibly on unknown values\nwhile issuing errors against known-to-be-bad values, so your rule\nabove sounds like the most sensible thing to do.\n"},{"id":"246035","messageId":"02874ECE860811409154E81DA85FBB58571987B5@ORSMSX115.amr.corp.intel.com","threadId":"37114","inReplyTo":"xmqqk37h19dc.fsf@gitster.dls.corp.google.com","subject":"RE: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-14T22:08:32Z","receivedAt":"2014-07-14T22:08:32Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\n> -----Original Message-----\n> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Sunday, July 13, 2014 10:01 AM\n> To: Jeff King\n> Cc: Keller, Jacob E; git@vger.kernel.org\n> Subject: Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig\n> \n> Jeff King writes:\n> \n> > On Fri, Jul 11, 2014 at 02:54:25PM -0700, Junio C Hamano wrote:\n> >\n> >> > Yeah, we're quite inconsistent there. In some cases we silently\n> ignore\n> >> > something unknown (e.g., a color.diff.* slot that we do not\n> understand),\n> >> > but in most cases if it is a config key we understand but a value we\n> do\n> >> > not, we complain and die.\n> >>\n> >> Hm, that's bad---we've become less and less careful over time,\n> >> perhaps?\n> >\n> > I don't think so. I think we've always been not-very-lenient with\n> > parsing values. Two examples:\n> > ...\n> > So I do not think we have ever had a rule, but if we did, it is probably\n> > \"silently ignore unknown keys, complain on unknown values\".\n> \n> Yeah, somehow I was mixing up these two cases fuzzily in my mind\n> while responding. Rejecting unknown keys is bad, but we cannot be\n> perfectly forward compatible nor behave sensibly on unknown values\n> while issuing errors against known-to-be-bad values, so your rule\n> above sounds like the most sensible thing to do.\n\nThe only difference is whether we halt or ignore the unknown value we complained about. Personally I am ok with either, but prefer the \"complain and choose the default\" so that older gits don't completely stop. But in some cases, the change is not easily compatible so then stopping might be preferred as the old git will not behave as expected.\n\nThanks,\nJake\n"}]}