{"thread":{"id":"37135","subject":"[PATCH v9 2/4] tag: fix --sort tests to use cat<<-\\EOF format","startedAt":"2014-07-15T23:32:56Z","lastAt":"2014-07-16T21:42:13Z","messageCount":11,"participants":["Jacob Keller","Jeff King","Keller, Jacob E","Junio C Hamano"],"isPatch":true,"patchVersion":9,"patchTotal":4},"messages":[{"id":"246116","messageId":"1405467179-16064-1-git-send-email-jacob.e.keller@intel.com","threadId":"37135","inReplyTo":null,"subject":"[PATCH v9 1/4] usage: make error functions a stack","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-15T23:32:56Z","receivedAt":"2014-07-15T23:32:56Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"Rename set_error_routine to be push_error_routine, and add a new\npop_error_routine. This allows temporary modifications of the error\nroutine over a small block of code.\n\nSigned-off-by: Jacob Keller <jacob.e.keller@intel.com>\n---\nRenamed set_error_routine to push_error_routine in order to match the\npop_error_routine.\n\n git-compat-util.h |  3 ++-\n run-command.c     |  2 +-\n usage.c           | 32 ++++++++++++++++++++++++++++----\n 3 files changed, 31 insertions(+), 6 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9de318071083..b650e3cb6cdc 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -343,7 +343,8 @@ static inline int const_error(void)\n #endif\n \n extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\n-extern void set_error_routine(void (*routine)(const char *err, va_list params));\n+extern void push_error_routine(void (*routine)(const char *err, va_list params));\n+extern void pop_error_routine(void);\n extern void set_die_is_recursing_routine(int (*routine)(void));\n \n extern int starts_with(const char *str, const char *prefix);\ndiff --git a/run-command.c b/run-command.c\nindex be07d4ad335b..a611b819c25d 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -360,7 +360,7 @@ fail_pipe:\n \t\t\tset_cloexec(child_err);\n \t\t}\n \t\tset_die_routine(die_child);\n-\t\tset_error_routine(error_child);\n+\t\tpush_error_routine(error_child);\n \n \t\tclose(notify_pipe[0]);\n \t\tset_cloexec(notify_pipe[1]);\ndiff --git a/usage.c b/usage.c\nindex ed146453cabe..3fe26771f7e6 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -57,18 +57,42 @@ 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 }\n \n-void set_error_routine(void (*routine)(const char *err, va_list params))\n+/* push error routine onto the error function stack */\n+void push_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+/* pop a single error routine off of the error function stack, thus reverting\n+ * to previous error. Should always be paired with a push_error_routine */\n+void pop_error_routine(void)\n+{\n+\tstruct error_func_list *efl = error_funcs;\n+\n+\tassert(error_funcs != &default_error_func);\n+\n+\tif (efl->next) {\n+\t\terror_funcs = efl->next;\n+\t\tfree(efl);\n+\t}\n }\n \n void set_die_is_recursing_routine(int (*routine)(void))\n@@ -144,7 +168,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-- \n2.0.1.475.g9b8d714\n"},{"id":"246115","messageId":"1405467179-16064-2-git-send-email-jacob.e.keller@intel.com","threadId":"37135","inReplyTo":"1405467179-16064-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v9 2/4] tag: fix --sort tests to use cat<<-\\EOF format","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-15T23:32:57Z","receivedAt":"2014-07-15T23:32:57Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"The --sort tests should use the better format for >expect to maintain\nindenting and ensure that no substitution is occurring. This makes\nparsing and understanding the tests a bit easier.\n\nSigned-off-by: Jacob Keller <jacob.e.keller@intel.com>\n---\n t/t7004-tag.sh | 40 ++++++++++++++++++++--------------------\n 1 file changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex e4ab0f5b6419..66010b0e7066 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1385,41 +1385,41 @@ test_expect_success 'lexical sort' '\n \tgit tag foo1.6 &&\n \tgit tag foo1.10 &&\n \tgit tag -l --sort=refname \"foo*\" >actual &&\n-\tcat >expect <<EOF &&\n-foo1.10\n-foo1.3\n-foo1.6\n-EOF\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.10\n+\tfoo1.3\n+\tfoo1.6\n+\tEOF\n \ttest_cmp expect actual\n '\n \n test_expect_success 'version sort' '\n \tgit tag -l --sort=version:refname \"foo*\" >actual &&\n-\tcat >expect <<EOF &&\n-foo1.3\n-foo1.6\n-foo1.10\n-EOF\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.3\n+\tfoo1.6\n+\tfoo1.10\n+\tEOF\n \ttest_cmp expect actual\n '\n \n test_expect_success 'reverse version sort' '\n \tgit tag -l --sort=-version:refname \"foo*\" >actual &&\n-\tcat >expect <<EOF &&\n-foo1.10\n-foo1.6\n-foo1.3\n-EOF\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.10\n+\tfoo1.6\n+\tfoo1.3\n+\tEOF\n \ttest_cmp expect actual\n '\n \n test_expect_success 'reverse lexical sort' '\n \tgit tag -l --sort=-refname \"foo*\" >actual &&\n-\tcat >expect <<EOF &&\n-foo1.6\n-foo1.3\n-foo1.10\n-EOF\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.6\n+\tfoo1.3\n+\tfoo1.10\n+\tEOF\n \ttest_cmp expect actual\n '\n \n-- \n2.0.1.475.g9b8d714\n"},{"id":"246118","messageId":"1405467179-16064-3-git-send-email-jacob.e.keller@intel.com","threadId":"37135","inReplyTo":"1405467179-16064-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v9 3/4] tag: update parsing to be more precise regarding errors","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-15T23:32:58Z","receivedAt":"2014-07-15T23:32:58Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"Update the parsing of sort string specifications so that it is able to\nproperly detect errors in the function type and allowed atoms.\n\nCc: Jeff King <peff@peff.net>\nSigned-off-by: Jacob Keller <jacob.e.keller@intel.com>\n---\n builtin/tag.c | 55 +++++++++++++++++++++++++++++++++++++++++--------------\n 1 file changed, 41 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex ef765563388c..7d82526e76be 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -522,24 +522,51 @@ 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+\tchar *value, *separator, *type, *atom;\n+\tint flags = 0, function = 0, err = 0;\n \n-\tif (*arg == '-') {\n+\t/* skip the '-' prefix for reverse sort order first */\n+\tif (skip_prefix(arg, \"-\", &arg))\n \t\tflags |= REVERSE_SORT;\n-\t\targ++;\n+\n+\t/* duplicate string so we can modify it in place */\n+\tvalue = xstrdup(arg);\n+\n+\t/* determine the sort function and the sorting atom */\n+\tseparator = strchr(value, ':');\n+\tif (separator) {\n+\t\t/* split the string at the separator with a NULL byte */\n+\t\t*separator = '\\0';\n+\t\ttype = value;\n+\t\tatom = separator + 1;\n+\t} else {\n+\t\t/* we have no separator, so assume the whole string is the * atom */\n+\t\ttype = NULL;\n+\t\tatom = value;\n \t}\n-\tif (starts_with(arg, \"version:\")) {\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+\n+\tif (type) {\n+\t\tif (!strcmp(type, \"version\") || !strcmp(type, \"v\"))\n+\t\t\tfunction = VERCMP_SORT;\n+\t\telse {\n+\t\t\terr = error(_(\"unsupported sort function '%s'\"), type);\n+\t\t\tgoto abort;\n+\t\t}\n+\n \t} else\n-\t\t*sort = STRCMP_SORT;\n-\tif (strcmp(arg, \"refname\"))\n-\t\tdie(_(\"unsupported sort specification %s\"), arg);\n-\t*sort |= flags;\n-\treturn 0;\n+\t\tfunction = STRCMP_SORT;\n+\n+\t/* for now, only the refname is a valid atom */\n+\tif (atom && strcmp(atom, \"refname\")) {\n+\t\terr = error(_(\"unsupported sort specification '%s'\"), atom);\n+\t\tgoto abort;\n+\t}\n+\n+\t*sort = (flags | function);\n+\n+abort:\n+\tfree(value);\n+\treturn err;\n }\n \n int cmd_tag(int argc, const char **argv, const char *prefix)\n-- \n2.0.1.475.g9b8d714\n"},{"id":"246117","messageId":"1405467179-16064-4-git-send-email-jacob.e.keller@intel.com","threadId":"37135","inReplyTo":"1405467179-16064-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v9 4/4] tag: support configuring --sort via .gitconfig","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-15T23:32:59Z","receivedAt":"2014-07-15T23:32:59Z","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 specification for git tags.\nCommand line option will override the configured value. Both methods use\nthe same syntax. Make use of (set/pop)_error_routine to temporarily\nmodify error reporting when parsing as a configuration option.\n\nCc: Jeff King <peff@peff.net>\nSigned-off-by: Jacob Keller <jacob.e.keller@intel.com>\n---\n Documentation/config.txt  |   5 ++\n Documentation/git-tag.txt |   5 +-\n builtin/tag.c             | 124 ++++++++++++++++++++++++++++------------------\n t/t7004-tag.sh            |  36 ++++++++++++++\n 4 files changed, 120 insertions(+), 50 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1d718bdb9662..c55c22ab7be9 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2354,6 +2354,11 @@ submodule.<name>.ignore::\n \t\"--ignore-submodules\" option. The 'git submodule' commands are not\n \taffected by this setting.\n \n+tag.sort::\n+\tThis variable controls the sort ordering of tags when displayed by\n+\tlinkgit:git-tag[1]. Without the \"--sort=<value>\" option provided, the\n+\tvalue of this variable will be used as the default.\n+\n tar.umask::\n \tThis variable can be used to restrict the permission bits of\n \ttar archive entries.  The default is 0002, which turns off the\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex b424a1bc48bb..320908369f06 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -99,7 +99,9 @@ OPTIONS\n \tSort in a specific order. Supported type is \"refname\"\n \t(lexicographic order), \"version:refname\" or \"v:refname\" (tag\n \tnames are treated as versions). Prepend \"-\" to reverse sort\n-\torder.\n+\torder. When this option is not given, the sort order defaults to the\n+\tvalue configured for the 'tag.sort' variable if it exists, or\n+\tlexicographic order otherwise. See linkgit:git-config[1].\n \n --column[=<options>]::\n --no-column::\n@@ -317,6 +319,7 @@ include::date-formats.txt[]\n SEE ALSO\n --------\n linkgit:git-check-ref-format[1].\n+linkgit:git-config[1].\n \n GIT\n ---\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 7d82526e76be..091536c61eab 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,76 @@ 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, int *sort)\n+{\n+\tchar *value, *separator, *type, *atom;\n+\tint flags = 0, function = 0, err = 0;\n+\n+\t/* skip the '-' prefix for reverse sort order first */\n+\tif (skip_prefix(arg, \"-\", &arg))\n+\t\tflags |= REVERSE_SORT;\n+\n+\t/* duplicate string so we can modify it in place */\n+\tvalue = xstrdup(arg);\n+\n+\t/* determine the sort function and the sorting atom */\n+\tseparator = strchr(value, ':');\n+\tif (separator) {\n+\t\t/* split the string at the separator with a NULL byte */\n+\t\t*separator = '\\0';\n+\t\ttype = value;\n+\t\tatom = separator + 1;\n+\t} else {\n+\t\t/* we have no separator, so assume the whole string is the * atom */\n+\t\ttype = NULL;\n+\t\tatom = value;\n+\t}\n+\n+\tif (type) {\n+\t\tif (!strcmp(type, \"version\") || !strcmp(type, \"v\"))\n+\t\t\tfunction = VERCMP_SORT;\n+\t\telse {\n+\t\t\terr = error(_(\"unsupported sort function '%s'\"), type);\n+\t\t\tgoto abort;\n+\t\t}\n+\n+\t} else\n+\t\tfunction = STRCMP_SORT;\n+\n+\t/* for now, only the refname is a valid atom */\n+\tif (atom && strcmp(atom, \"refname\")) {\n+\t\terr = error(_(\"unsupported sort specification '%s'\"), atom);\n+\t\tgoto abort;\n+\t}\n+\n+\t*sort = (flags | function);\n+\n+abort:\n+\tfree(value);\n+\treturn err;\n+}\n+\n+static void error_bad_sort_config(const char *err, va_list params)\n+{\n+\tvreportf(\"warning: tag.sort has \", err, params);\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+\n+\t\tpush_error_routine(error_bad_sort_config);\n+\t\tparse_sort_string(value, &tag_sort);\n+\t\tpop_error_routine();\n+\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,51 +591,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-\tchar *value, *separator, *type, *atom;\n-\tint flags = 0, function = 0, err = 0;\n \n-\t/* skip the '-' prefix for reverse sort order first */\n-\tif (skip_prefix(arg, \"-\", &arg))\n-\t\tflags |= REVERSE_SORT;\n-\n-\t/* duplicate string so we can modify it in place */\n-\tvalue = xstrdup(arg);\n-\n-\t/* determine the sort function and the sorting atom */\n-\tseparator = strchr(value, ':');\n-\tif (separator) {\n-\t\t/* split the string at the separator with a NULL byte */\n-\t\t*separator = '\\0';\n-\t\ttype = value;\n-\t\tatom = separator + 1;\n-\t} else {\n-\t\t/* we have no separator, so assume the whole string is the * atom */\n-\t\ttype = NULL;\n-\t\tatom = value;\n-\t}\n-\n-\tif (type) {\n-\t\tif (!strcmp(type, \"version\") || !strcmp(type, \"v\"))\n-\t\t\tfunction = VERCMP_SORT;\n-\t\telse {\n-\t\t\terr = error(_(\"unsupported sort function '%s'\"), type);\n-\t\t\tgoto abort;\n-\t\t}\n-\n-\t} else\n-\t\tfunction = STRCMP_SORT;\n-\n-\t/* for now, only the refname is a valid atom */\n-\tif (atom && strcmp(atom, \"refname\")) {\n-\t\terr = error(_(\"unsupported sort specification '%s'\"), atom);\n-\t\tgoto abort;\n-\t}\n-\n-\t*sort = (flags | function);\n-\n-abort:\n-\tfree(value);\n-\treturn err;\n+\treturn parse_sort_string(arg, sort);\n }\n \n int cmd_tag(int argc, const char **argv, const char *prefix)\n@@ -579,7 +605,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@@ -605,7 +631,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@@ -661,9 +687,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":"246142","messageId":"20140715234240.GB5572@peff.net","threadId":"37135","inReplyTo":"1405467179-16064-4-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v9 4/4] tag: support configuring --sort via .gitconfig","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-15T23:42:40Z","receivedAt":"2014-07-15T23:42:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 15, 2014 at 04:32:59PM -0700, Jacob Keller wrote:\n\n> +static void error_bad_sort_config(const char *err, va_list params)\n> +{\n> +\tvreportf(\"warning: tag.sort has \", err, params);\n> +}\n\nThis feels a little like an abuse of the \"prefix\" field of vreportf, but\nas you probably saw in my \"for fun\" patch, doing it right means\nformatting into a buffer and then reformatting that (which we're\nalready doing again in vreportf, but less flexibly). I dunno.\n\nAt any rate, this should be marked for translation, no?\n\n-Peff\n"},{"id":"246144","messageId":"1405467974.2577.21.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37135","inReplyTo":"20140715234240.GB5572@peff.net","subject":"Re: [PATCH v9 4/4] tag: support configuring --sort via .gitconfig","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-15T23:46:14Z","receivedAt":"2014-07-15T23:46: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 19:42 -0400, Jeff King wrote:\n> On Tue, Jul 15, 2014 at 04:32:59PM -0700, Jacob Keller wrote:\n> \n> > +static void error_bad_sort_config(const char *err, va_list params)\n> > +{\n> > +\tvreportf(\"warning: tag.sort has \", err, params);\n> > +}\n> \n> This feels a little like an abuse of the \"prefix\" field of vreportf, but\n> as you probably saw in my \"for fun\" patch, doing it right means\n> formatting into a buffer and then reformatting that (which we're\n> already doing again in vreportf, but less flexibly). I dunno.\n> \n> At any rate, this should be marked for translation, no?\n> \n> -Peff\n\nOh, yes it probably should. It's definitely a little bit abusive of the\nprefix field, but that seemed easier.\n\nThanks,\nJake\n"},{"id":"246174","messageId":"xmqqiomxrxq2.fsf@gitster.dls.corp.google.com","threadId":"37135","inReplyTo":"1405467974.2577.21.camel@jekeller-desk1.amr.corp.intel.com","subject":"Re: [PATCH v9 4/4] tag: support configuring --sort via .gitconfig","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-16T17:59:33Z","receivedAt":"2014-07-16T17:59:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Keller, Jacob E\" <jacob.e.keller@intel.com> writes:\n\n> On Tue, 2014-07-15 at 19:42 -0400, Jeff King wrote:\n>> On Tue, Jul 15, 2014 at 04:32:59PM -0700, Jacob Keller wrote:\n>> \n>> > +static void error_bad_sort_config(const char *err, va_list params)\n>> > +{\n>> > +\tvreportf(\"warning: tag.sort has \", err, params);\n>> > +}\n>> \n>> This feels a little like an abuse of the \"prefix\" field of vreportf, but\n>> as you probably saw in my \"for fun\" patch, doing it right means\n>> formatting into a buffer and then reformatting that (which we're\n>> already doing again in vreportf, but less flexibly). I dunno.\n>> \n>> At any rate, this should be marked for translation, no?\n>> \n>> -Peff\n>\n> Oh, yes it probably should. It's definitely a little bit abusive of the\n> prefix field, but that seemed easier.\n\nAnd i18n would automatically mean this will not work, no?  There is\nno guarantee that the translation of \"warning: \" will be followed\ndirectly by the translation of \"tag.sort has\" without any words from\nvariable part in all languages.\n\nAfter all, it seems to me that the one in\n\n    http://thread.gmane.org/gmane.comp.version-control.git/253346\n\nstruck the right balance among various abuses; let's use the error\nreporter from that version, instead of going down this rabbit hole.\n\nThanks.\n"},{"id":"246181","messageId":"1405540292.15392.4.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37135","inReplyTo":"xmqqiomxrxq2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v9 4/4] tag: support configuring --sort via .gitconfig","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-16T19:51:33Z","receivedAt":"2014-07-16T19:51:33Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, 2014-07-16 at 10:59 -0700, Junio C Hamano wrote:\n> \"Keller, Jacob E\" <jacob.e.keller@intel.com> writes:\n> \n> > On Tue, 2014-07-15 at 19:42 -0400, Jeff King wrote:\n> >> On Tue, Jul 15, 2014 at 04:32:59PM -0700, Jacob Keller wrote:\n> >> \n> >> > +static void error_bad_sort_config(const char *err, va_list params)\n> >> > +{\n> >> > +\tvreportf(\"warning: tag.sort has \", err, params);\n> >> > +}\n> >> \n> >> This feels a little like an abuse of the \"prefix\" field of vreportf, but\n> >> as you probably saw in my \"for fun\" patch, doing it right means\n> >> formatting into a buffer and then reformatting that (which we're\n> >> already doing again in vreportf, but less flexibly). I dunno.\n> >> \n> >> At any rate, this should be marked for translation, no?\n> >> \n> >> -Peff\n> >\n> > Oh, yes it probably should. It's definitely a little bit abusive of the\n> > prefix field, but that seemed easier.\n> \n> And i18n would automatically mean this will not work, no?  There is\n> no guarantee that the translation of \"warning: \" will be followed\n> directly by the translation of \"tag.sort has\" without any words from\n> variable part in all languages.\n> \n> After all, it seems to me that the one in\n> \n>     http://thread.gmane.org/gmane.comp.version-control.git/253346\n> \n> struck the right balance among various abuses; let's use the error\n> reporter from that version, instead of going down this rabbit hole.\n> \n> Thanks.\n> \n> \n> \n\nThis is fine with me :)\n\nThanks,\nJake\n"},{"id":"246183","messageId":"1405545219.15392.17.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37135","inReplyTo":"xmqqiomxrxq2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v9 4/4] tag: support configuring --sort via .gitconfig","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-16T21:13:39Z","receivedAt":"2014-07-16T21:13:39Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, 2014-07-16 at 10:59 -0700, Junio C Hamano wrote:\n> \"Keller, Jacob E\" <jacob.e.keller@intel.com> writes:\n> \n> > On Tue, 2014-07-15 at 19:42 -0400, Jeff King wrote:\n> >> On Tue, Jul 15, 2014 at 04:32:59PM -0700, Jacob Keller wrote:\n> >> \n> >> > +static void error_bad_sort_config(const char *err, va_list params)\n> >> > +{\n> >> > +\tvreportf(\"warning: tag.sort has \", err, params);\n> >> > +}\n> >> \n> >> This feels a little like an abuse of the \"prefix\" field of vreportf, but\n> >> as you probably saw in my \"for fun\" patch, doing it right means\n> >> formatting into a buffer and then reformatting that (which we're\n> >> already doing again in vreportf, but less flexibly). I dunno.\n> >> \n> >> At any rate, this should be marked for translation, no?\n> >> \n> >> -Peff\n> >\n> > Oh, yes it probably should. It's definitely a little bit abusive of the\n> > prefix field, but that seemed easier.\n> \n> And i18n would automatically mean this will not work, no?  There is\n> no guarantee that the translation of \"warning: \" will be followed\n> directly by the translation of \"tag.sort has\" without any words from\n> variable part in all languages.\n> \n> After all, it seems to me that the one in\n> \n>     http://thread.gmane.org/gmane.comp.version-control.git/253346\n> \n> struck the right balance among various abuses; let's use the error\n> reporter from that version, instead of going down this rabbit hole.\n> \n> Thanks.\n> \n> \n> \n\nSo is that patch series version acceptable then? Or should I spin one\nagain that is in that vein?\n\nThanks,\nJake\n"},{"id":"246185","messageId":"xmqq1ttlrnh5.fsf@gitster.dls.corp.google.com","threadId":"37135","inReplyTo":"1405545219.15392.17.camel@jekeller-desk1.amr.corp.intel.com","subject":"Re: [PATCH v9 4/4] tag: support configuring --sort via .gitconfig","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-16T21:40:54Z","receivedAt":"2014-07-16T21:40:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Keller, Jacob E\" <jacob.e.keller@intel.com> writes:\n\n>> After all, it seems to me that the one in\n>> \n>>     http://thread.gmane.org/gmane.comp.version-control.git/253346\n>> \n>> struck the right balance among various abuses; let's use the error\n>> reporter from that version, instead of going down this rabbit hole.\n>> \n>> Thanks.\n>\n> So is that patch series version acceptable then? Or should I spin one\n> again that is in that vein?\n\nI do not offhand know what other changes you had in v9 since\n$gmane/253346, so I'll leave it up to you.  If the only difference\nis the error reporting codepath, and you are happy with what I have\nin my tree\n\n    $ git log -p --reverse -3 a93d886b9\n\nthen we can proceed with that version.  Have there been any issues\nraised on that older version other than error reporting?\n"},{"id":"246187","messageId":"1405546933.15392.18.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37135","inReplyTo":"xmqq1ttlrnh5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v9 4/4] tag: support configuring --sort via .gitconfig","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-16T21:42:13Z","receivedAt":"2014-07-16T21:42:13Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, 2014-07-16 at 14:40 -0700, Junio C Hamano wrote:\n> \"Keller, Jacob E\" <jacob.e.keller@intel.com> writes:\n> \n> >> After all, it seems to me that the one in\n> >> \n> >>     http://thread.gmane.org/gmane.comp.version-control.git/253346\n> >> \n> >> struck the right balance among various abuses; let's use the error\n> >> reporter from that version, instead of going down this rabbit hole.\n> >> \n> >> Thanks.\n> >\n> > So is that patch series version acceptable then? Or should I spin one\n> > again that is in that vein?\n> \n> I do not offhand know what other changes you had in v9 since\n> $gmane/253346, so I'll leave it up to you.  If the only difference\n> is the error reporting codepath, and you are happy with what I have\n> in my tree\n> \n>     $ git log -p --reverse -3 a93d886b9\n> \n> then we can proceed with that version.  Have there been any issues\n> raised on that older version other than error reporting?\n> \n> \n> \n> \n\nI'll double check.\n\nThanks,\nJake\n"}]}