{"thread":{"id":"37118","subject":"[PATCH 2/3] tag: fix --sort tests to use cat<<-\\EOF format","startedAt":"2014-07-11T22:55:45Z","lastAt":"2014-07-15T23:38:36Z","messageCount":19,"participants":["Jacob Keller","Jeff King","Eric Sunshine","Junio C Hamano","Keller, Jacob E"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"245848","messageId":"1405119347-3308-1-git-send-email-jacob.e.keller@intel.com","threadId":"37118","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-11T22:55:45Z","receivedAt":"2014-07-11T22:55:45Z","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---\nFixed issue with patch in that we dropped the reset to STRCMP_SORT, discovered\nby Junio.\n\n builtin/tag.c | 14 +++++---------\n 1 file changed, 5 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex ef765563388c..9d7643f127e7 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -524,18 +524,14 @@ 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+\telse\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":"245847","messageId":"1405119347-3308-2-git-send-email-jacob.e.keller@intel.com","threadId":"37118","inReplyTo":"1405119347-3308-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-11T22:55:46Z","receivedAt":"2014-07-11T22:55:46Z","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":"245849","messageId":"1405119347-3308-3-git-send-email-jacob.e.keller@intel.com","threadId":"37118","inReplyTo":"1405119347-3308-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-11T22:55:47Z","receivedAt":"2014-07-11T22:55:47Z","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---\nMade parse_sort_string take a \"var\" parameter, and if given will only warn\nabout invalid parameter, instead of error.\n\n Documentation/config.txt | 5 ++++\n Documentation/git-tag.txt | 5 +++-\n builtin/tag.c | 66 ++++++++++++++++++++++++++++++++++-------------\n t/t7004-tag.sh | 36 ++++++++++++++++++++++++++\n 4 files changed, 93 insertions(+), 19 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 9d7643f127e7..97c5317c28e5 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,49 @@ 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 *var, const char *value, int *sort)\n+{\n+\tint type = 0, flags = 0;\n+\n+\tif (skip_prefix(value, \"-\", &value))\n+\t\tflags |= REVERSE_SORT;\n+\n+\tif (skip_prefix(value, \"version:\", &value) || skip_prefix(value, \"v:\", &value))\n+\t\ttype = VERCMP_SORT;\n+\telse\n+\t\ttype = STRCMP_SORT;\n+\n+\tif (strcmp(value, \"refname\")) {\n+\t\tif (!var)\n+\t\t\treturn error(_(\"unsupported sort specification '%s'\"), value);\n+\t\telse {\n+\t\t\twarning(_(\"unsupported sort specification '%s' in variable '%s'\"),\n+\t\t\t\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 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\tparse_sort_string(var, value, &tag_sort);\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,20 +564,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-\telse\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-\treturn 0;\n+\treturn parse_sort_string(NULL, arg, sort);\n }\n \n int cmd_tag(int argc, const char **argv, const char *prefix)\n@@ -548,7 +578,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@@ -574,7 +604,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@@ -630,9 +660,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":"245876","messageId":"20140712010243.GA17349@sigill.intra.peff.net","threadId":"37118","inReplyTo":"1405119347-3308-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-12T01:02:43Z","receivedAt":"2014-07-12T01:02:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 11, 2014 at 03:55:45PM -0700, Jacob Keller wrote:\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> Fixed issue with patch in that we dropped the reset to STRCMP_SORT, discovered\n> by Junio.\n\nI think what Junio queued in ce85604468 is already correct, so we are\nfine as long we build on that.\n\n-Peff\n"},{"id":"245902","messageId":"CAPig+cR9VCtNhk-FbqDM1LTCa8VeUTYXU4XEX36Rb5CxPFfLWQ@mail.gmail.com","threadId":"37118","inReplyTo":"1405119347-3308-3-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-07-13T03:29:40Z","receivedAt":"2014-07-13T03:29:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 11, 2014 at 6:55 PM, 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 \n> Signed-off-by: Jacob Keller \n> ---\n> Made parse_sort_string take a \"var\" parameter, and if given will only warn\n> about invalid parameter, instead of error.\n\nThis seems unnecessarily ugly since it's hard-coding specialized\nknowledge of the callers' error-reporting requirements into what\nshould be a generalized parsing function. If you instead make\nparse_sort_string() responsible only for attempting to parse the\nvalue, but leave error-reporting to the callers, then this ugliness\ngoes away. See below.\n\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 9d7643f127e7..97c5317c28e5 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> const char **patterns;\n> int lines;\n> @@ -346,9 +348,49 @@ static const char tag_template_nocleanup[] =\n> \"Lines starting with '%c' will be kept; you may remove them\"\n> \" 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 *var, const char *value, int *sort)\n> +{\n> + int type = 0, flags = 0;\n> +\n> + if (skip_prefix(value, \"-\", &value))\n> + flags |= REVERSE_SORT;\n> +\n> + if (skip_prefix(value, \"version:\", &value) || skip_prefix(value, \"v:\", &value))\n> + type = VERCMP_SORT;\n> + else\n> + type = STRCMP_SORT;\n> +\n> + if (strcmp(value, \"refname\")) {\n> + if (!var)\n> + return error(_(\"unsupported sort specification '%s'\"), value);\n> + else {\n> + warning(_(\"unsupported sort specification '%s' in variable '%s'\"),\n> + var, value);\n> + return -1;\n\nJust return -1 here, but don't print any diagnostics. Let the callers\ndo that. (See below.)\n\n> + }\n> + }\n> +\n> + *sort = (type | flags);\n> +\n> + return 0;\n> +}\n> +\n> static int git_tag_config(const char *var, const char *value, void *cb)\n> {\n> - int status = git_gpg_config(var, value, cb);\n> + int status;\n> +\n> + if (!strcmp(var, \"tag.sort\")) {\n> + if (!value)\n> + return config_error_nonbool(var);\n> + parse_sort_string(var, value, &tag_sort);\n\n if (parse_sort_string(value, &tag_sort))\n warning(_(\"unsupported sort specification '%s' in variable '%s'\"),\n var, value);\n\n> + return 0;\n> + }\n> +\n> + status = git_gpg_config(var, value, cb);\n> if (status)\n> return status;\n> if (starts_with(var, \"column.\"))\n> @@ -522,20 +564,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> int *sort = opt->value;\n> - int flags = 0;\n>\n> - if (skip_prefix(arg, \"-\", &arg))\n> - flags |= REVERSE_SORT;\n> -\n> - if (skip_prefix(arg, \"version:\", &arg) || skip_prefix(arg, \"v:\", &arg))\n> - *sort = VERCMP_SORT;\n> - else\n> - *sort = STRCMP_SORT;\n> -\n> - if (strcmp(arg, \"refname\"))\n> - die(_(\"unsupported sort specification %s\"), arg);\n> - *sort |= flags;\n> - return 0;\n> + return parse_sort_string(NULL, arg, sort);\n\n if (parse_sort_string(arg, sort))\n return error(_(\"unsupported sort specification '%s'\"), arg);\n return 0;\n\n> }\n>\n> int cmd_tag(int argc, const char **argv, const char *prefix)\n> @@ -548,7 +578,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> struct create_tag_options opt;\n> char *cleanup_arg = NULL;\n> int annotate = 0, force = 0, lines = -1;\n> - int cmdmode = 0, sort = 0;\n> + int cmdmode = 0;\n> const char *msgfile = NULL, *keyid = NULL;\n> struct msg_arg msg = { 0, STRBUF_INIT };\n> struct commit_list *with_commit = NULL;\n> @@ -574,7 +604,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> OPT__FORCE(&force, N_(\"replace the tag if exists\")),\n> OPT_COLUMN(0, \"column\", &colopts, N_(\"show tag list in columns\")),\n> {\n> - OPTION_CALLBACK, 0, \"sort\", &sort, N_(\"type\"), N_(\"sort tags\"),\n> + OPTION_CALLBACK, 0, \"sort\", &tag_sort, N_(\"type\"), N_(\"sort tags\"),\n> PARSE_OPT_NONEG, parse_opt_sort\n> },\n>\n> @@ -630,9 +660,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> copts.padding = 2;\n> run_column_filter(colopts, &copts);\n> }\n> - if (lines != -1 && sort)\n> + if (lines != -1 && tag_sort)\n> die(_(\"--sort and -n are incompatible\"));\n> - ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, sort);\n> + ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, tag_sort);\n> if (column_active(colopts))\n> stop_column_filter();\n> return ret;\n"},{"id":"245952","messageId":"xmqqfvi518xo.fsf@gitster.dls.corp.google.com","threadId":"37118","inReplyTo":"CAPig+cR9VCtNhk-FbqDM1LTCa8VeUTYXU4XEX36Rb5CxPFfLWQ@mail.gmail.com","subject":"Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-13T17:10:27Z","receivedAt":"2014-07-13T17:10:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> Made parse_sort_string take a \"var\" parameter, and if given will only warn\n>> about invalid parameter, instead of error.\n>\n> This seems unnecessarily ugly since it's hard-coding specialized\n> knowledge of the callers' error-reporting requirements into what\n> should be a generalized parsing function. If you instead make\n> parse_sort_string() responsible only for attempting to parse the\n> value, but leave error-reporting to the callers, then this ugliness\n> goes away. See below.\n\nYup, you are absolutely right. Thanks for catching my silly.\n"},{"id":"245954","messageId":"20140713173356.GA8406@sigill.intra.peff.net","threadId":"37118","inReplyTo":"xmqqfvi518xo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-13T17:33:56Z","receivedAt":"2014-07-13T17:33:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 13, 2014 at 10:10:27AM -0700, Junio C Hamano wrote:\n\n> Eric Sunshine writes:\n> \n> >> Made parse_sort_string take a \"var\" parameter, and if given will only warn\n> >> about invalid parameter, instead of error.\n> >\n> > This seems unnecessarily ugly since it's hard-coding specialized\n> > knowledge of the callers' error-reporting requirements into what\n> > should be a generalized parsing function. If you instead make\n> > parse_sort_string() responsible only for attempting to parse the\n> > value, but leave error-reporting to the callers, then this ugliness\n> > goes away. See below.\n> \n> Yup, you are absolutely right. Thanks for catching my silly.\n\nI do not know if it is that silly. The reason we push the error\nreporting down into reusable library functions, even though it is less\nflexible or causes us to have \"quiet\" flags and such, is that the\nlibrary function knows more about the specific error.\n\nIn this case we are just saying \"your sort specification is not valid\",\nso it is not adding much value, and returning \"-1\" provides enough\ninformation for the caller to say that. But would we eventually want to\ndiagnose errors more specifically? For example, in \"foo:refname\", we\ncould complain that \"foo\" is not a valid sort function. And in\n\"version:bar\", we could complain that \"bar\" is not a valid sorting atom.\n\nWe can encode these error types into an enum, but it is often much\neasier to report them at the time of discovery (e.g., because you have\nthe half-parsed string available that says \"foo\" or \"bar\"). This is a\ngeneral problem throughout a lot of our code. In higher level languages\nyou might throw an exception with the error message and let the caller\ndecide how to report it. I wonder if it is too gross to do something\nlike:\n\n push_error_handler(collect_errors);\n\n /* all calls to error() in will push error text onto a stack */\n do_something();\n\n pop_error_handler();\n\n /* traverse the list, calling warning() on each, and clear the stack */\n print_errors_as_warnings();\n\nOne can imagine print_errors_as_errors, which would be useful when a\ncaller is not sure whether a full operation will succeed (e.g., you try\nX, then Y, and only when both fail do you report an error). Or a caller\nwhich does not call any print_error_* at all (i.e., replacing the\n\"quiet\" flag that many library functions take).\n\nI realize that I am reinventing the error-reporting wheel on a sleepy\nSunday afternoon without having thought about it much, so there is\nprobably some gotcha or case that makes this ugly, or perhaps it just\nends up verbose in practice. But one can dream.\n\n-Peff\n"},{"id":"245955","messageId":"20140713183629.GA19293@sigill.intra.peff.net","threadId":"37118","inReplyTo":"20140713173356.GA8406@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-13T18:36:29Z","receivedAt":"2014-07-13T18:36:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 13, 2014 at 01:33:56PM -0400, Jeff King wrote:\n\n> I realize that I am reinventing the error-reporting wheel on a sleepy\n> Sunday afternoon without having thought about it much, so there is\n> probably some gotcha or case that makes this ugly, or perhaps it just\n> ends up verbose in practice. But one can dream.\n\nJust for fun...\n\n---\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 5e0744b..0f5be3b 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -379,7 +379,10 @@ static int git_tag_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"tag.sort\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tparse_sort_string(value, &tag_sort);\n+\t\tset_error_routine(collect_errors);\n+\t\tif (parse_sort_string(value, &tag_sort) < 0)\n+\t\t\tprint_errors(warning, \"#{error} in config option 'tag.sort'\");\n+\t\tpop_error_routine();\n \t\treturn 0;\n \t}\n \ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9de3180..6bf91a6 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -346,6 +346,11 @@ extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_lis\n extern void set_error_routine(void (*routine)(const char *err, va_list params));\n extern void set_die_is_recursing_routine(int (*routine)(void));\n \n+void pop_error_routine(void);\n+void collect_errors(const char *err, va_list params);\n+__attribute__((format (printf, 2, 3)))\n+void print_errors(void (*func)(const char *, ...), const char *fmt, ...);\n+\n extern int starts_with(const char *str, const char *prefix);\n extern int ends_with(const char *str, const char *suffix);\n \ndiff --git a/usage.c b/usage.c\nindex ed14645..055ccc7 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -57,10 +57,16 @@ static int die_is_recursing_builtin(void)\n * (ugh), so keep things static. */\n static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) = usage_builtin;\n static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_builtin;\n-static void (*error_routine)(const char *err, va_list params) = error_builtin;\n static void (*warn_routine)(const char *err, va_list params) = warn_builtin;\n static int (*die_is_recursing)(void) = die_is_recursing_builtin;\n \n+struct error_func_list {\n+\tvoid (*func)(const char *, va_list);\n+\tstruct error_func_list *next;\n+};\n+static struct error_func_list default_error_func = { error_builtin };\n+static struct error_func_list *error_funcs = &default_error_func;\n+\n void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params))\n {\n \tdie_routine = routine;\n@@ -68,7 +74,84 @@ void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list param\n \n void set_error_routine(void (*routine)(const char *err, va_list params))\n {\n-\terror_routine = routine;\n+\tstruct error_func_list *efl = xmalloc(sizeof(*efl));\n+\tefl->func = routine;\n+\tefl->next = error_funcs;\n+\terror_funcs = efl;\n+}\n+\n+void pop_error_routine(void)\n+{\n+\twhile (error_funcs != &default_error_func) {\n+\t\tstruct error_func_list *efl = error_funcs;\n+\t\terror_funcs = efl->next;\n+\t\tfree(efl);\n+\t}\n+}\n+\n+struct error_list {\n+\tstruct strbuf buf;\n+\tstruct error_list *next;\n+};\n+struct error_list *error_list;\n+struct error_list **error_list_tail = &error_list;\n+\n+void collect_errors(const char *fmt, va_list params)\n+{\n+\tstruct error_list *err = xmalloc(sizeof(*err));\n+\n+\tstrbuf_init(&err->buf, 0);\n+\tstrbuf_vaddf(&err->buf, fmt, params);\n+\terr->next = NULL;\n+\t*error_list_tail = err;\n+\terror_list_tail = &err->next;\n+}\n+\n+static void clear_errors(void)\n+{\n+\twhile (error_list) {\n+\t\tstruct error_list *next = error_list->next;\n+\t\tstrbuf_release(&error_list->buf);\n+\t\tfree(error_list);\n+\t\terror_list = next;\n+\t}\n+\terror_list_tail = &error_list;\n+}\n+\n+void print_errors(void (*func)(const char *, ...), const char *fmt, ...)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tva_list params;\n+\tint prefix_len, suffix_start;\n+\tconst char *p;\n+\tstruct error_list *el;\n+\n+\tva_start(params, fmt);\n+\tstrbuf_vaddf(&buf, fmt, params);\n+\tva_end(params);\n+\n+\t/*\n+\t * The intent here is that callers will put #{error} in their fmt\n+\t * string. Doing two layers of interpolation is gross, because we may\n+\t * accidentally find \"#{error}\" in one of the substitutions, not the\n+\t * original fmt. Ideally we would do it all in a single pass (and call\n+\t * #{error} \"%M\" or something), but that would require extending\n+\t * vsprintf, and there is no way to do that portably.\n+\t */\n+\tp = strstr(buf.buf, \"#{error}\");\n+\tif (!p)\n+\t\tdie(\"BUG: error printer does not want to print error!?\");\n+\tprefix_len = p - buf.buf;\n+\tsuffix_start = prefix_len + strlen(\"#{error}\");\n+\n+\tfor (el = error_list; el; el = el->next)\n+\t\tfunc(\"%.*s%.*s%.*s\",\n+\t\t prefix_len, buf.buf,\n+\t\t el->buf.len, el->buf.buf,\n+\t\t buf.len - suffix_start, buf.buf + suffix_start);\n+\n+\tstrbuf_release(&buf);\n+\tclear_errors();\n }\n \n void set_die_is_recursing_routine(int (*routine)(void))\n@@ -144,7 +227,7 @@ int error(const char *err, ...)\n \tva_list params;\n \n \tva_start(params, err);\n-\terror_routine(err, params);\n+\terror_funcs->func(err, params);\n \tva_end(params);\n \treturn -1;\n }\n"},{"id":"245993","messageId":"xmqqfvi3zwp7.fsf@gitster.dls.corp.google.com","threadId":"37118","inReplyTo":"20140713183629.GA19293@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-14T17:17:40Z","receivedAt":"2014-07-14T17:17:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Jul 13, 2014 at 01:33:56PM -0400, Jeff King wrote:\n>\n>> I realize that I am reinventing the error-reporting wheel on a sleepy\n>> Sunday afternoon without having thought about it much, so there is\n>> probably some gotcha or case that makes this ugly, or perhaps it just\n>> ends up verbose in practice. But one can dream.\n>\n> Just for fun...\n\nYes, that is fun.\n\nI actually think your \"In 'version:pefname' and 'wersion:refname',\nwe want be able to report 'pefname' and 'wersion' are misspelled,\nand returning -1 or enum would not cut it\" is a good argument. The\ncallee wants to have flexibility on _what_ to report, just as the\ncaller wants to have flexibility on _how_. In this particular code\npath, I think the former far outweighs the latter, and my suggestion\nI called \"silly\" might not be so silly but may have struck the right\nbalance. I dunno.\n\nIf you absolutely need to have both, you would need something like\nyour approach, of course, but I am not sure if it is worth it.\n\nI am not sure how well this meshes with i18n (I know the \"for fun\"\ndoes not even attempt to, but if we tried to, I suspect it may\nbecome even uglier). We would also need to override both error and\nwarning routines and have the reporter tag the errors in these two\ncategories, no?\n"},{"id":"246059","messageId":"1405435933.9147.1.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37118","inReplyTo":"xmqqfvi3zwp7.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-15T14:52:13Z","receivedAt":"2014-07-15T14:52:13Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, 2014-07-14 at 10:17 -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > On Sun, Jul 13, 2014 at 01:33:56PM -0400, Jeff King wrote:\n> >\n> >> I realize that I am reinventing the error-reporting wheel on a sleepy\n> >> Sunday afternoon without having thought about it much, so there is\n> >> probably some gotcha or case that makes this ugly, or perhaps it just\n> >> ends up verbose in practice. But one can dream.\n> >\n> > Just for fun...\n> \n> Yes, that is fun.\n> \n> I actually think your \"In 'version:pefname' and 'wersion:refname',\n> we want be able to report 'pefname' and 'wersion' are misspelled,\n> and returning -1 or enum would not cut it\" is a good argument. The\n> callee wants to have flexibility on _what_ to report, just as the\n> caller wants to have flexibility on _how_. In this particular code\n> path, I think the former far outweighs the latter, and my suggestion\n> I called \"silly\" might not be so silly but may have struck the right\n> balance. I dunno.\n> \n> If you absolutely need to have both, you would need something like\n> your approach, of course, but I am not sure if it is worth it.\n> \n> I am not sure how well this meshes with i18n (I know the \"for fun\"\n> does not even attempt to, but if we tried to, I suspect it may\n> become even uglier). We would also need to override both error and\n> warning routines and have the reporter tag the errors in these two\n> categories, no?\n> \n\nDo we want to go this way? Should I respin my patch (again)? I'm not\nsure we really need to get that complex.. I do like parsing errors a bit\ncleaner and indicating what part is bad.. Note that our current parsing\nmethod does not make it really possible to indicate which part is wrong.\n\nThanks,\nJake\n"},{"id":"246062","messageId":"xmqqfvi2wqvq.fsf@gitster.dls.corp.google.com","threadId":"37118","inReplyTo":"1405435933.9147.1.camel@jekeller-desk1.amr.corp.intel.com","subject":"Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-15T16:03:53Z","receivedAt":"2014-07-15T16:03:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Keller, Jacob E\" writes:\n\n> On Mon, 2014-07-14 at 10:17 -0700, Junio C Hamano wrote:\n>> Jeff King writes:\n>> \n>> > On Sun, Jul 13, 2014 at 01:33:56PM -0400, Jeff King wrote:\n>> >\n>> >> I realize that I am reinventing the error-reporting wheel on a sleepy\n>> >> Sunday afternoon without having thought about it much, so there is\n>> >> probably some gotcha or case that makes this ugly, or perhaps it just\n>> >> ends up verbose in practice. But one can dream.\n>> >\n>> > Just for fun...\n>> \n>> Yes, that is fun.\n>> \n>> I actually think your \"In 'version:pefname' and 'wersion:refname',\n>> we want be able to report 'pefname' and 'wersion' are misspelled,\n>> and returning -1 or enum would not cut it\" is a good argument. The\n>> callee wants to have flexibility on _what_ to report, just as the\n>> caller wants to have flexibility on _how_. In this particular code\n>> path, I think the former far outweighs the latter, and my suggestion\n>> I called \"silly\" might not be so silly but may have struck the right\n>> balance. I dunno.\n>> \n>> If you absolutely need to have both, you would need something like\n>> your approach, of course, but I am not sure if it is worth it.\n>> \n>> I am not sure how well this meshes with i18n (I know the \"for fun\"\n>> does not even attempt to, but if we tried to, I suspect it may\n>> become even uglier). We would also need to override both error and\n>> warning routines and have the reporter tag the errors in these two\n>> categories, no?\n>\n> Do we want to go this way?\n\nI do not speak for Peff, but I personally think this is just a \"fun\"\ndemonstration, nothing more, and my gut feeling is that it would\nmake things unnecessary complex without much real gain to pursue it\nfurther.\n"},{"id":"246069","messageId":"1405445234.2577.0.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37118","inReplyTo":"xmqqfvi2wqvq.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-15T17:27:14Z","receivedAt":"2014-07-15T17:27:14Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, 2014-07-15 at 09:03 -0700, Junio C Hamano wrote:\n> \"Keller, Jacob E\" writes:\n> \n> > On Mon, 2014-07-14 at 10:17 -0700, Junio C Hamano wrote:\n> >> Jeff King writes:\n> >> \n> >> > On Sun, Jul 13, 2014 at 01:33:56PM -0400, Jeff King wrote:\n> >> >\n> >> >> I realize that I am reinventing the error-reporting wheel on a sleepy\n> >> >> Sunday afternoon without having thought about it much, so there is\n> >> >> probably some gotcha or case that makes this ugly, or perhaps it just\n> >> >> ends up verbose in practice. But one can dream.\n> >> >\n> >> > Just for fun...\n> >> \n> >> Yes, that is fun.\n> >> \n> >> I actually think your \"In 'version:pefname' and 'wersion:refname',\n> >> we want be able to report 'pefname' and 'wersion' are misspelled,\n> >> and returning -1 or enum would not cut it\" is a good argument. The\n> >> callee wants to have flexibility on _what_ to report, just as the\n> >> caller wants to have flexibility on _how_. In this particular code\n> >> path, I think the former far outweighs the latter, and my suggestion\n> >> I called \"silly\" might not be so silly but may have struck the right\n> >> balance. I dunno.\n> >> \n> >> If you absolutely need to have both, you would need something like\n> >> your approach, of course, but I am not sure if it is worth it.\n> >> \n> >> I am not sure how well this meshes with i18n (I know the \"for fun\"\n> >> does not even attempt to, but if we tried to, I suspect it may\n> >> become even uglier). We would also need to override both error and\n> >> warning routines and have the reporter tag the errors in these two\n> >> categories, no?\n> >\n> > Do we want to go this way?\n> \n> I do not speak for Peff, but I personally think this is just a \"fun\"\n> demonstration, nothing more, and my gut feeling is that it would\n> make things unnecessary complex without much real gain to pursue it\n> further.\n\nI agree. But what about going back to the older setup where the caller\ncan output correct error message? I'm ok with using an enum style\nreturn, to be completely honest. I would prefer this, actually.\n\nThanks,\nJake\n\n"},{"id":"246072","messageId":"xmqq7g3ewkp8.fsf@gitster.dls.corp.google.com","threadId":"37118","inReplyTo":"1405445234.2577.0.camel@jekeller-desk1.amr.corp.intel.com","subject":"Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-15T18:17:23Z","receivedAt":"2014-07-15T18:17:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Keller, Jacob E\" writes:\n\n> On Tue, 2014-07-15 at 09:03 -0700, Junio C Hamano wrote:\n> ...\n>> >> Yes, that is fun.\n>> >> \n>> >> I actually think your \"In 'version:pefname' and 'wersion:refname',\n>> >> we want be able to report 'pefname' and 'wersion' are misspelled,\n>> >> and returning -1 or enum would not cut it\" is a good argument. The\n>> >> callee wants to have flexibility on _what_ to report, just as the\n>> >> caller wants to have flexibility on _how_. In this particular code\n>> >> path, I think the former far outweighs the latter, and my suggestion\n>> >> I called \"silly\" might not be so silly but may have struck the right\n>> >> balance. I dunno.\n> ...\n> I agree. But what about going back to the older setup where the caller\n> can output correct error message? I'm ok with using an enum style\n> return, to be completely honest. I would prefer this, actually.\n\nDepends on which older setup you mean, I think. The one that does\nnot let us easily give more context dependent diagnoses that lets us\ndistinguish between version:pefname and version:refname by returning\nonly -1 or an enum?\n"},{"id":"246075","messageId":"1405449076.2577.3.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37118","inReplyTo":"xmqq7g3ewkp8.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-15T18:31:16Z","receivedAt":"2014-07-15T18:31:16Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, 2014-07-15 at 11:17 -0700, Junio C Hamano wrote:\n> \"Keller, Jacob E\" writes:\n> \n> > On Tue, 2014-07-15 at 09:03 -0700, Junio C Hamano wrote:\n> > ...\n> >> >> Yes, that is fun.\n> >> >> \n> >> >> I actually think your \"In 'version:pefname' and 'wersion:refname',\n> >> >> we want be able to report 'pefname' and 'wersion' are misspelled,\n> >> >> and returning -1 or enum would not cut it\" is a good argument. The\n> >> >> callee wants to have flexibility on _what_ to report, just as the\n> >> >> caller wants to have flexibility on _how_. In this particular code\n> >> >> path, I think the former far outweighs the latter, and my suggestion\n> >> >> I called \"silly\" might not be so silly but may have struck the right\n> >> >> balance. I dunno.\n> > ...\n> > I agree. But what about going back to the older setup where the caller\n> > can output correct error message? I'm ok with using an enum style\n> > return, to be completely honest. I would prefer this, actually.\n> \n> Depends on which older setup you mean, I think. The one that does\n> not let us easily give more context dependent diagnoses that lets us\n> distinguish between version:pefname and version:refname by returning\n> only -1 or an enum?\n> \n\nI am going to re-submit this with an enum-style return. I am also\nchanging how we parse so that we can correctly report whether the sort\nfunction or sort atom is incorrect.\n\nThanks,\nJake\n"},{"id":"246079","messageId":"xmqqlhruv3ni.fsf@gitster.dls.corp.google.com","threadId":"37118","inReplyTo":"xmqq7g3ewkp8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-15T19:10:57Z","receivedAt":"2014-07-15T19:10:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Keller, Jacob E\" writes:\n>\n>> On Tue, 2014-07-15 at 09:03 -0700, Junio C Hamano wrote:\n>> ...\n>>> >> Yes, that is fun.\n>>> >> \n>>> >> I actually think your \"In 'version:pefname' and 'wersion:refname',\n>>> >> we want be able to report 'pefname' and 'wersion' are misspelled,\n>>> >> and returning -1 or enum would not cut it\" is a good argument. The\n>>> >> callee wants to have flexibility on _what_ to report, just as the\n>>> >> caller wants to have flexibility on _how_. In this particular code\n>>> >> path, I think the former far outweighs the latter, and my suggestion\n>>> >> I called \"silly\" might not be so silly but may have struck the right\n>>> >> balance. I dunno.\n>> ...\n>> I agree. But what about going back to the older setup where the caller\n>> can output correct error message? I'm ok with using an enum style\n>> return, to be completely honest. I would prefer this, actually.\n>\n> Depends on which older setup you mean, I think. The one that does\n> not let us easily give more context dependent diagnoses that lets us\n> distinguish between version:pefname and version:refname by returning\n> only -1 or an enum?\n\nIn case it was not clear, I do not think -1 or enum is a good idea.\n"},{"id":"246080","messageId":"xmqqha2iv3kr.fsf@gitster.dls.corp.google.com","threadId":"37118","inReplyTo":"1405449076.2577.3.camel@jekeller-desk1.amr.corp.intel.com","subject":"Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-15T19:12:36Z","receivedAt":"2014-07-15T19:12:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Keller, Jacob E\" writes:\n\n> I am going to re-submit this with an enum-style return. I am also\n> changing how we parse so that we can correctly report whether the sort\n> function or sort atom is incorrect.\n\nOh, our mails crossed, I guess. As long as it will leave the door\nopen for later enhancements for more context sensitive error\ndiagnosis, I do not particularly mind a solution around enum.\n"},{"id":"246094","messageId":"1405456151.2577.7.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37118","inReplyTo":"xmqqha2iv3kr.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-15T20:29:12Z","receivedAt":"2014-07-15T20:29:12Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, 2014-07-15 at 12:12 -0700, Junio C Hamano wrote:\n> \"Keller, Jacob E\" writes:\n> \n> > I am going to re-submit this with an enum-style return. I am also\n> > changing how we parse so that we can correctly report whether the sort\n> > function or sort atom is incorrect.\n> \n> Oh, our mails crossed, I guess. As long as it will leave the door\n> open for later enhancements for more context sensitive error\n> diagnosis, I do not particularly mind a solution around enum.\n\nHmm. I looked at coding it this way, and it actually makes it less\nsensitive than I would like. I'm not a fan of the extra \"value\"\nparameter, but I do like a more proper error display, and indeed one\nthat is more precise.\n\nI'll try to have a new series posted soon which takes these into\naccount.\n\nRegards,\nJake\n"},{"id":"246103","messageId":"1405459868.2577.9.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37118","inReplyTo":"xmqqha2iv3kr.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-15T21:31:09Z","receivedAt":"2014-07-15T21:31:09Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, 2014-07-15 at 12:12 -0700, Junio C Hamano wrote:\n> \"Keller, Jacob E\" writes:\n> \n> > I am going to re-submit this with an enum-style return. I am also\n> > changing how we parse so that we can correctly report whether the sort\n> > function or sort atom is incorrect.\n> \n> Oh, our mails crossed, I guess. As long as it will leave the door\n> open for later enhancements for more context sensitive error\n> diagnosis, I do not particularly mind a solution around enum.\n\nI just sent a v8 of the series. I think I mostly followed Peff's idea of\nusing a pop_error_routine function, but not as complex as his was. This\noverall results in more accurate errors, and doesn't clutter the\noriginal parse_sort_string with too much knowledge about what particular\nvalue is being parsed. Hopefully we can finally converge on a good set\nof patches.\n\nThanks,\nJake\n"},{"id":"246140","messageId":"20140715233836.GA5572@peff.net","threadId":"37118","inReplyTo":"xmqqfvi2wqvq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-15T23:38:36Z","receivedAt":"2014-07-15T23:38:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 15, 2014 at 09:03:53AM -0700, Junio C Hamano wrote:\n\n> > Do we want to go this way?\n> \n> I do not speak for Peff, but I personally think this is just a \"fun\"\n> demonstration, nothing more, and my gut feeling is that it would\n> make things unnecessary complex without much real gain to pursue it\n> further.\n\nYeah, it is a little too complicated for what it is buying us here. If\nwe had a real error stack with allocated error types or something, then\ncallers could do more useful programmatic things with it. But:\n\n 1. We usually only care about system errors in such a case, and get by\n with using errno.\n\n 2. I would not want to annotate all of the library-ish functions with\n case-specific return types. That is a lot of work for little gain.\n\nI think my favorite of the suggestions so far is basically the two-line:\n\n error: sort specification is bad...\n warning: ignoring invalid tag.sort\n\nThere are tons of places in git where we already do this kind of\n\"error chaining\" over multiple lines (and multiple calls to error()),\nand it doesn't require any new code or techniques.\n\nBut what is in v8 is not so bad from my cursory glance.\n\n-Peff\n\nPS I am traveling this week and will probably be a lot less responsive.\n Please don't let me hold up your conversations.\n"}]}