{"thread":{"id":"37133","subject":"[PATCH v8 0/4] tag: configure default tag sort via .gitconfig","startedAt":"2014-07-15T21:29:10Z","lastAt":"2014-07-16T19:50:01Z","messageCount":11,"participants":["Jacob Keller","Junio C Hamano","Keller, Jacob E"],"isPatch":true,"patchVersion":8,"patchTotal":4},"messages":[{"id":"246096","messageId":"1405459754-4220-1-git-send-email-jacob.e.keller@intel.com","threadId":"37133","inReplyTo":null,"subject":"[PATCH v8 0/4] tag: configure default tag sort via .gitconfig","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-15T21:29:10Z","receivedAt":"2014-07-15T21:29:10Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"This patch series is hopefully the final version of a series of patches which\nupdates git-tag to allow the .gitconfig variable 'tag.sort' to be used as the\ndefault sort parameter. This version uses a new set/pop error routine setup\nwhich enables correctly modifying the error routine to handle the config vs the\ncommand line.\n\nIn addition, I split the patch such that all the changes to the original\nparse_opt_sort are shown, and the following patch which extracts this function\nis just a function extraction with no changes, making it easier to review the\nnew changes. One other minor bug fix is included as well.\n\nJacob Keller (4):\n usage: make error functions a stack\n tag: fix --sort tests to use cat<<-\\EOF format\n tag: update parsing to be more precise regarding errors\n tag: support configuring --sort via .gitconfig\n\n Documentation/config.txt | 5 +++\n Documentation/git-tag.txt | 5 ++-\n builtin/tag.c | 97 ++++++++++++++++++++++++++++++++++++-----------\n git-compat-util.h | 1 +\n t/t7004-tag.sh | 76 +++++++++++++++++++++++++++----------\n usage.c | 29 ++++++++++++--\n 6 files changed, 167 insertions(+), 46 deletions(-)\n\n-- \n2.0.1.475.g9b8d714\n"},{"id":"246099","messageId":"1405459754-4220-2-git-send-email-jacob.e.keller@intel.com","threadId":"37133","inReplyTo":"1405459754-4220-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v8 1/4] usage: make error functions a stack","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-15T21:29:11Z","receivedAt":"2014-07-15T21:29:11Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"Let error routine be a stack of error functions so that callers can\ntemporarily override the error_routine and then pop their modification\noff the stack. This enables customizing error for a small code segment.\n\nSigned-off-by: Jacob Keller \n---\nThis is a modification of Peff's original idea for handling multiple error\nroutines. I simplified it by not having the collect and other routines. I only\nmodify set_error_routine to be a \"push\" operation, with pop_error_routine being\nits opposite. I don't let pop_error_routine remove all the error routines,\ninstead only doing one with an assert check that we never call it too many times.\n\nThis enables temporarily modifying the error routine and then popping back to\nthe previous value.\n\n git-compat-util.h | 1 +\n usage.c | 29 ++++++++++++++++++++++++++---\n 2 files changed, 27 insertions(+), 3 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9de318071083..6d0416c90ad8 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -344,6 +344,7 @@ static inline int const_error(void)\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 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/usage.c b/usage.c\nindex ed146453cabe..fd9126a7ca0b 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -57,18 +57,41 @@ 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+/* push error routine onto the error function stack */\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+/* pop a single error routine off of the error function stack, thus reverting\n+ * to previous error. Should always be paired with a set_error_routine */\n+void pop_error_routine(void)\n+{\n+\tassert(error_funcs != &default_error_func);\n+\n+\tstruct error_func_list *efl = error_funcs;\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 +167,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":"246097","messageId":"1405459754-4220-3-git-send-email-jacob.e.keller@intel.com","threadId":"37133","inReplyTo":"1405459754-4220-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v8 2/4] tag: fix --sort tests to use cat<<-\\EOF format","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-15T21:29:12Z","receivedAt":"2014-07-15T21:29:12Z","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":"246100","messageId":"1405459754-4220-4-git-send-email-jacob.e.keller@intel.com","threadId":"37133","inReplyTo":"1405459754-4220-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v8 3/4] tag: update parsing to be more precise regarding errors","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-15T21:29:13Z","receivedAt":"2014-07-15T21:29:13Z","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 \nSigned-off-by: Jacob Keller \n---\nThis function should replace the one I think is already on one of the branches.\nThis version fully updates the parse_sort_opt to be like what will be the\nparse_sort_string function. I have ensured that the next patch is purely a move\nof this function, and does not contain modifications to make this easier to\nreview. Instead of using skip_prefix, I use strchr and check for the separator.\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":"246098","messageId":"1405459754-4220-5-git-send-email-jacob.e.keller@intel.com","threadId":"37133","inReplyTo":"1405459754-4220-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v8 4/4] tag: support configuring --sort via .gitconfig","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-15T21:29:14Z","receivedAt":"2014-07-15T21:29:14Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"Add support for configuring default sort 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 \nSigned-off-by: Jacob Keller \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..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 7d82526e76be..603cf688368c 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\tset_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":"246105","messageId":"xmqqbnsqtfdb.fsf@gitster.dls.corp.google.com","threadId":"37133","inReplyTo":"1405459754-4220-1-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v8 0/4] tag: configure default tag sort via .gitconfig","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-15T22:40:48Z","receivedAt":"2014-07-15T22:40:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> This patch series is hopefully the final version of a series of patches which\n> updates git-tag to allow the .gitconfig variable 'tag.sort' to be used as the\n> default sort parameter. This version uses a new set/pop error routine setup\n> which enables correctly modifying the error routine to handle the config vs the\n> command line.\n>\n> In addition, I split the patch such that all the changes to the original\n> parse_opt_sort are shown, and the following patch which extracts this function\n> is just a function extraction with no changes, making it easier to review the\n> new changes. One other minor bug fix is included as well.\n>\n> Jacob Keller (4):\n> usage: make error functions a stack\n> tag: fix --sort tests to use cat<<-\\EOF format\n> tag: update parsing to be more precise regarding errors\n> tag: support configuring --sort via .gitconfig\n\nThe first one looked somewhat tricky, but looked nice overall from a\ncursory reading.\n\nThanks.\n"},{"id":"246106","messageId":"xmqq7g3etf1o.fsf@gitster.dls.corp.google.com","threadId":"37133","inReplyTo":"1405459754-4220-2-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v8 1/4] usage: make error functions a stack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-15T22:47:47Z","receivedAt":"2014-07-15T22:47:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> extern void set_error_routine(void (*routine)(const char *err, va_list params));\n> +extern void pop_error_routine(void);\n\npop that undoes set smells somewhat weird. Perhaps we should rename\nset to push? That would allow us catch possible topics that add new\ncalls to set_error_routine() as well by forcing the system not to\nlink when they are merged without necessary fixes.\n\n> +/* push error routine onto the error function stack */\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> +/* pop a single error routine off of the error function stack, thus reverting\n> + * to previous error. Should always be paired with a set_error_routine */\n> +void pop_error_routine(void)\n> +{\n> +\tassert(error_funcs != &default_error_func);\n> +\n> +\tstruct error_func_list *efl = error_funcs;\n\ndecl-after-stmt. Can be fixed easily by flipping the above two\nlines.\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 +167,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":"246113","messageId":"1405466672.2577.14.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37133","inReplyTo":"xmqq7g3etf1o.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 1/4] usage: make error functions a stack","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-15T23:24:32Z","receivedAt":"2014-07-15T23:24:32Z","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 15:47 -0700, Junio C Hamano wrote:\n> Jacob Keller writes:\n> \n> > extern void set_error_routine(void (*routine)(const char *err, va_list params));\n> > +extern void pop_error_routine(void);\n> \n> pop that undoes set smells somewhat weird. Perhaps we should rename\n> set to push? That would allow us catch possible topics that add new\n> calls to set_error_routine() as well by forcing the system not to\n> link when they are merged without necessary fixes.\n> \n\nI thought about changing set too, but wasn't sure that made sense..?\nThat does make more sense now though. There *are* valid use cases where\na set_error_routine is used without a pop, (the one current use, I\nthink).\n\nI'll update this patch with that change.\n\n> > +/* push error routine onto the error function stack */\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> > +/* pop a single error routine off of the error function stack, thus reverting\n> > + * to previous error. Should always be paired with a set_error_routine */\n> > +void pop_error_routine(void)\n> > +{\n> > +\tassert(error_funcs != &default_error_func);\n> > +\n> > +\tstruct error_func_list *efl = error_funcs;\n> \n> decl-after-stmt. Can be fixed easily by flipping the above two\n> lines.\n\nOh, right yes. I'll fix that in a resend as well.\n\nThanks,\nJake\n\n\n"},{"id":"246114","messageId":"1405466783.2577.15.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37133","inReplyTo":"xmqq7g3etf1o.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 1/4] usage: make error functions a stack","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-15T23:26:23Z","receivedAt":"2014-07-15T23:26:23Z","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 15:47 -0700, Junio C Hamano wrote:\n> Jacob Keller writes:\n> \n> > extern void set_error_routine(void (*routine)(const char *err, va_list params));\n> > +extern void pop_error_routine(void);\n> \n> pop that undoes set smells somewhat weird. Perhaps we should rename\n> set to push? That would allow us catch possible topics that add new\n> calls to set_error_routine() as well by forcing the system not to\n> link when they are merged without necessary fixes.\n> \n\nAlso curious what your thoughts on making every set_*_routine to be a\nstack? For now I was only planning on error but it maybe makes sense to\nchange them all?\n\nThanks,\nJake\n\n"},{"id":"246146","messageId":"CAPc5daUhmeucDOakXpR_ViS0cfFDEyXUD_xoeP6qeCg33dW+1A@mail.gmail.com","threadId":"37133","inReplyTo":"1405466783.2577.15.camel@jekeller-desk1.amr.corp.intel.com","subject":"Re: [PATCH v8 1/4] usage: make error functions a stack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-16T00:49:30Z","receivedAt":"2014-07-16T00:49:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I actually am not a big fan of \"stack\" for a thing like this, to be honest.\nWouldn't it be sufficient for the callers who want specific behaviour from\nits callees to\n\n - save away the current error/warning routines;\n - set error/warning routines to its own custom versions;\n - call the callees;\n - set error/warning routines back to their original; and\n - return from it\n\nat least in the code paths under discussion?\n\n\nOn Tue, Jul 15, 2014 at 4:26 PM, Keller, Jacob E\n wrote:\n> On Tue, 2014-07-15 at 15:47 -0700, Junio C Hamano wrote:\n>> Jacob Keller writes:\n>>\n>> > extern void set_error_routine(void (*routine)(const char *err, va_list params));\n>> > +extern void pop_error_routine(void);\n>>\n>> pop that undoes set smells somewhat weird. Perhaps we should rename\n>> set to push? That would allow us catch possible topics that add new\n>> calls to set_error_routine() as well by forcing the system not to\n>> link when they are merged without necessary fixes.\n>>\n>\n> Also curious what your thoughts on making every set_*_routine to be a\n> stack? For now I was only planning on error but it maybe makes sense to\n> change them all?\n>\n> Thanks,\n> Jake\n>\n"},{"id":"246180","messageId":"1405540201.15392.3.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37133","inReplyTo":"CAPc5daUhmeucDOakXpR_ViS0cfFDEyXUD_xoeP6qeCg33dW+1A@mail.gmail.com","subject":"Re: [PATCH v8 1/4] usage: make error functions a stack","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-16T19:50:01Z","receivedAt":"2014-07-16T19:50:01Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"There was no way to get the current error routine now, and I figured\nthat a stack was a simple way of saving the old routine.\n\nEssentially these two paths would be the same as a \"save/restore\" except\nwe manage it via a stack. I don't really see how that would end up any\ndifferent. I mean I don't mind either approach.\n\nThanks,\nJake\n\nOn Tue, 2014-07-15 at 17:49 -0700, Junio C Hamano wrote:\n> I actually am not a big fan of \"stack\" for a thing like this, to be honest.\n> Wouldn't it be sufficient for the callers who want specific behaviour from\n> its callees to\n> \n> - save away the current error/warning routines;\n> - set error/warning routines to its own custom versions;\n> - call the callees;\n> - set error/warning routines back to their original; and\n> - return from it\n> \n> at least in the code paths under discussion?\n> \n> \n> On Tue, Jul 15, 2014 at 4:26 PM, Keller, Jacob E\n> wrote:\n> > On Tue, 2014-07-15 at 15:47 -0700, Junio C Hamano wrote:\n> >> Jacob Keller writes:\n> >>\n> >> > extern void set_error_routine(void (*routine)(const char *err, va_list params));\n> >> > +extern void pop_error_routine(void);\n> >>\n> >> pop that undoes set smells somewhat weird. Perhaps we should rename\n> >> set to push? That would allow us catch possible topics that add new\n> >> calls to set_error_routine() as well by forcing the system not to\n> >> link when they are merged without necessary fixes.\n> >>\n> >\n> > Also curious what your thoughts on making every set_*_routine to be a\n> > stack? For now I was only planning on error but it maybe makes sense to\n> > change them all?\n> >\n> > Thanks,\n> > Jake\n> >\n\n\n"}]}