{"thread":{"id":"42574","subject":"[PATCH] parse-options-cb.c: use string_list_append_nodup in OPT_STRING_LIST()","startedAt":"2016-06-16T02:19:50Z","lastAt":"2016-08-03T07:37:30Z","messageCount":15,"participants":["Nguyễn Thái Ngọc Duy","Jeff King","Duy Nguyen","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"288891","messageId":"20160610115726.4805-1-pclouds@gmail.com","threadId":"42574","inReplyTo":null,"subject":"[PATCH] parse-options-cb.c: use string_list_append_nodup in OPT_STRING_LIST()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-10T11:57:26Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If the given string list has strdup_strings set (*), the string will be\nduplicated again. Pointless and leak memory. Ignore that flag.\n\n(*) only interpret-trailers.c does it at the moment\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n parse-options-cb.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex 239898d..8a1b6e6 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -144,7 +144,7 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)\n \tif (!arg)\n \t\treturn -1;\n \n-\tstring_list_append(v, xstrdup(arg));\n+\tstring_list_append_nodup(v, xstrdup(arg));\n \treturn 0;\n }\n \n-- \n2.8.2.524.g6ff3d78\n"},{"id":"289064","messageId":"20160612220316.GB5428@sigill.intra.peff.net","threadId":"42574","inReplyTo":"20160610115726.4805-1-pclouds@gmail.com","subject":"Re: [PATCH] parse-options-cb.c: use string_list_append_nodup in OPT_STRING_LIST()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-12T22:03:16Z","receivedAt":"2016-06-16T02:19:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 10, 2016 at 06:57:26PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> If the given string list has strdup_strings set (*), the string will be\n> duplicated again. Pointless and leak memory. Ignore that flag.\n> \n> (*) only interpret-trailers.c does it at the moment\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  parse-options-cb.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/parse-options-cb.c b/parse-options-cb.c\n> index 239898d..8a1b6e6 100644\n> --- a/parse-options-cb.c\n> +++ b/parse-options-cb.c\n> @@ -144,7 +144,7 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)\n>  \tif (!arg)\n>  \t\treturn -1;\n>  \n> -\tstring_list_append(v, xstrdup(arg));\n> +\tstring_list_append_nodup(v, xstrdup(arg));\n\nHmm. So I agree this is an improvement, in the sense that we are\ndouble-allocating when v->strdup_strings is set.  But I think there's a\ndeeper issue here. Why are we always allocating in the first place?\n\nIf the memory we are getting in \"arg\" is not stable, then we _do_ need\nto make a copy of it. But in that case, we want \"strdup_strings\" to be\nset; without it any time we later run string_list_clear(), we leak the\nallocated memory, because the struct has no idea that it is the owner of\nthe memory (and we do call string_list_clear() when we see \"--no-foo\").\n\nIf the memory _is_ stable, then we are fine to add a direct reference to\nit, and can lose the extra xstrdup() here. Only the caller knows for\nsure, so we should be respecting their value of strdup_strings (so lose\nthe xstrdup, but keep calling string_list_append()).\n\nIn practice, I suspect the memory _is_ stable, because we are generally\nparsing command-line arguments. But it does not hurt to stay on the\nconservative side, and always make a copy (in case we are parsing\nsomething besides the global argv array) . Apparently I am the original\nauthor of this code, in c8ba163 (parse-options: add OPT_STRING_LIST\nhelper, 2011-06-09), but there's no mention of this point there, in the\nlist archives, or in my brain.\n\nSo if we are doing the conservative thing, then I think the resulting\ncode should either look like:\n\n  if (!v->strdup_strings)\n\tdie(\"BUG: OPT_STRING_LIST should always use strdup_strings\");\n  string_list_append(v, arg);\n\nor:\n\n  /* silently enable for convenience */\n  v->strdup_strings = 1;\n  string_list_append(v, arg);\n\nOf the two, I like the top one as it is less magical, but it would\nrequire adjusting the initialization of the string-list for most of the\ncallers.\n\n-Peff\n"},{"id":"289066","messageId":"CACsJy8C+NtiXRo8NcU3rtgWrMSj8Zv3mYtdYfyvzwYRHifKVCQ@mail.gmail.com","threadId":"42574","inReplyTo":"20160612220316.GB5428@sigill.intra.peff.net","subject":"Re: [PATCH] parse-options-cb.c: use string_list_append_nodup in OPT_STRING_LIST()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-13T00:08:55Z","receivedAt":"2016-06-16T02:19:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jun 13, 2016 at 5:03 AM, Jeff King <peff@peff.net> wrote:\n> On Fri, Jun 10, 2016 at 06:57:26PM +0700, Nguyễn Thái Ngọc Duy wrote:\n>\n>> If the given string list has strdup_strings set (*), the string will be\n>> duplicated again. Pointless and leak memory. Ignore that flag.\n>>\n>> (*) only interpret-trailers.c does it at the moment\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n>> ---\n>>  parse-options-cb.c | 2 +-\n>>  1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/parse-options-cb.c b/parse-options-cb.c\n>> index 239898d..8a1b6e6 100644\n>> --- a/parse-options-cb.c\n>> +++ b/parse-options-cb.c\n>> @@ -144,7 +144,7 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)\n>>       if (!arg)\n>>               return -1;\n>>\n>> -     string_list_append(v, xstrdup(arg));\n>> +     string_list_append_nodup(v, xstrdup(arg));\n>\n> Hmm. So I agree this is an improvement, in the sense that we are\n> double-allocating when v->strdup_strings is set.  But I think there's a\n> deeper issue here. Why are we always allocating in the first place?\n>\n> If the memory we are getting in \"arg\" is not stable, then we _do_ need\n> to make a copy of it. But in that case, we want \"strdup_strings\" to be\n> set; without it any time we later run string_list_clear(), we leak the\n> allocated memory, because the struct has no idea that it is the owner of\n> the memory (and we do call string_list_clear() when we see \"--no-foo\").\n>\n> If the memory _is_ stable, then we are fine to add a direct reference to\n> it, and can lose the extra xstrdup() here. Only the caller knows for\n> sure, so we should be respecting their value of strdup_strings (so lose\n> the xstrdup, but keep calling string_list_append()).\n>\n> In practice, I suspect the memory _is_ stable, because we are generally\n> parsing command-line arguments. But it does not hurt to stay on the\n> conservative side, and always make a copy (in case we are parsing\n> something besides the global argv array) . Apparently I am the original\n> author of this code, in c8ba163 (parse-options: add OPT_STRING_LIST\n> helper, 2011-06-09), but there's no mention of this point there, in the\n> list archives, or in my brain.\n>\n> So if we are doing the conservative thing, then I think the resulting\n> code should either look like:\n>\n>   if (!v->strdup_strings)\n>         die(\"BUG: OPT_STRING_LIST should always use strdup_strings\");\n>   string_list_append(v, arg);\n\nI agree with the analysis. But this die() would hit all callers\n(except interpret-trailers) because they all initialize with _NODUP\nand setting strdup_strings may require auditing all access to the\nstring list in question, e.g. to change string_list_append(v,\nxstrdup(xxx)) to string_list_append(xxx). it may cause side effects if\nwe are not careful.\n\nSo far all callers are in builtin/, I think it will not take much time\nto verify that they all call parse_options() with global argv, then we\ncan just lose extra xstrdup() and stick to string_list_append().\nOPTION_STRING already assumes that argument strings are stable because\nthey are passed back as-is. Can we go with an easier route, adding a\ncomment on top of parse_options() stating that argv[] pointers may be\npassed back as-is and it's up to the caller to xstrdup() appropriately\nbefore argv[] memory is freed?\n\n>\n> or:\n>\n>   /* silently enable for convenience */\n>   v->strdup_strings = 1;\n>   string_list_append(v, arg);\n>\n> Of the two, I like the top one as it is less magical, but it would\n> require adjusting the initialization of the string-list for most of the\n> callers.\n>\n> -Peff\n\n\n\n-- \nDuy\n"},{"id":"289075","messageId":"20160613053203.GB3950@sigill.intra.peff.net","threadId":"42574","inReplyTo":"CACsJy8C+NtiXRo8NcU3rtgWrMSj8Zv3mYtdYfyvzwYRHifKVCQ@mail.gmail.com","subject":"[PATCH 0/3] fix parse-opt string_list leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-13T05:32:03Z","receivedAt":"2016-06-16T02:19:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 13, 2016 at 07:08:55AM +0700, Duy Nguyen wrote:\n\n> > So if we are doing the conservative thing, then I think the resulting\n> > code should either look like:\n> >\n> >   if (!v->strdup_strings)\n> >         die(\"BUG: OPT_STRING_LIST should always use strdup_strings\");\n> >   string_list_append(v, arg);\n> \n> I agree with the analysis. But this die() would hit all callers\n> (except interpret-trailers) because they all initialize with _NODUP\n> and setting strdup_strings may require auditing all access to the\n> string list in question, e.g. to change string_list_append(v,\n> xstrdup(xxx)) to string_list_append(xxx). it may cause side effects if\n> we are not careful.\n\nYep. It is not really fixing anything, so much as alerting us to broken\ncallers. We'd still have to fix the callers. :)\n\n> So far all callers are in builtin/, I think it will not take much time\n> to verify that they all call parse_options() with global argv, then we\n> can just lose extra xstrdup() and stick to string_list_append().\n> OPTION_STRING already assumes that argument strings are stable because\n> they are passed back as-is. Can we go with an easier route, adding a\n> comment on top of parse_options() stating that argv[] pointers may be\n> passed back as-is and it's up to the caller to xstrdup() appropriately\n> before argv[] memory is freed?\n\nYeah, the two options I laid out were the \"conservative\" side, where we\ndidn't make any assumptions about what is in passed into parse_options.\nBut I agree in practice that it's not likely to be a problem to just\npoint to the existing strings, and the fact that OPTION_STRING does so\nalready makes me even more confident.\n\nSo I'd suggest these patches:\n\n  [1/3]: parse_opt_string_list: stop allocating new strings\n  [2/3]: interpret-trailers: don't duplicate option strings\n  [3/3]: blame,shortlog: don't make local option variables static\n\nThe first one is what we've been discussing, and the others are just\nfollow-on cleanups.  I stopped short of a fourth patch to convert more\ncases of:\n\n  static struct string_list foo;\n\nto:\n\n  static struct string_list foo = STRING_LIST_INIT_NODUP;\n\nThe two are equivalent (mostly due to historical reasons). I tend to\nthink explicit is better than implicit for something like this (not\nbecause BSS auto-initialization isn't OK, but because there is an\nexplicit choice of dup/nodup that the writer made, and it is good to\ncommunicate that). But maybe people don't want the extra noise.\n\n-Peff\n"},{"id":"289076","messageId":"20160613053912.GA23880@sigill.intra.peff.net","threadId":"42574","inReplyTo":"20160613053203.GB3950@sigill.intra.peff.net","subject":"[PATCH 1/3] parse_opt_string_list: stop allocating new strings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-13T05:39:12Z","receivedAt":"2016-06-16T02:19:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The parse_opt_string_list callback is basically a thin\nwrapper to string_list_append() any string options we get.\nHowever, it calls:\n\n  string_list_append(v, xstrdup(arg));\n\nwhich duplicates the option value. This is wrong for two\nreasons:\n\n  1. If the string list has strdup_strings set, then we are\n     making an extra copy, which is simply leaked.\n\n  2. If the string list does not have strdup_strings set,\n     then we pass memory ownership to the string list, but\n     it does not realize this. If we later call\n     string_list_clear(), which can happen if \"--no-foo\" is\n     passed, then we will leak all of the existing entries.\n\nInstead, we should just pass the argument straight to\nstring_list_append, and it can decide whether to copy or not\nbased on its strdup_strings flag.\n\nIt's possible that some (buggy) caller could be relying on\nthis extra copy (e.g., because it parses some options from\nan allocated argv array and then frees the array), but it's\nnot likely. For one, we generally only use parse_options on\nthe argv given to us in main(). And two, such a caller is\nbroken anyway, because other option types like OPT_STRING()\ndo not make such a copy.  This patch brings us in line with\nthem.\n\nNoticed-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n parse-options-cb.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex 239898d..ba5acf3 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -144,7 +144,7 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)\n \tif (!arg)\n \t\treturn -1;\n \n-\tstring_list_append(v, xstrdup(arg));\n+\tstring_list_append(v, arg);\n \treturn 0;\n }\n \n-- \n2.9.0.rc2.149.gd580ccd\n"},{"id":"289077","messageId":"20160613053920.GB23880@sigill.intra.peff.net","threadId":"42574","inReplyTo":"20160613053203.GB3950@sigill.intra.peff.net","subject":"[PATCH 2/3] interpret-trailers: don't duplicate option strings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-13T05:39:20Z","receivedAt":"2016-06-16T02:19:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's no need to do so; the argv strings will last until\nthe end of the program.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/interpret-trailers.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex b99ae4b..175f147 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -20,7 +20,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n {\n \tint in_place = 0;\n \tint trim_empty = 0;\n-\tstruct string_list trailers = STRING_LIST_INIT_DUP;\n+\tstruct string_list trailers = STRING_LIST_INIT_NODUP;\n \n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"in-place\", &in_place, N_(\"edit files in place\")),\n-- \n2.9.0.rc2.149.gd580ccd\n"},{"id":"289078","messageId":"20160613053928.GC23880@sigill.intra.peff.net","threadId":"42574","inReplyTo":"20160613053203.GB3950@sigill.intra.peff.net","subject":"[PATCH 3/3] blame,shortlog: don't make local option variables static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-13T05:39:28Z","receivedAt":"2016-06-16T02:19:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's no need for these option variables to be static,\nexcept that they are referenced by the options array itself,\nwhich is static. But having all of this static is simply\nunnecessary and confusing (and inconsistent with most other\ncommands, which either use a static global option list or a\ntrue function-local one).\n\nNote that in some cases we may need to actually initialize\nthe variables (since we cannot rely on BSS to do so). This\nis a net improvement to readability, though, as we can use\nthe more verbose initializers for our string_lists.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/blame.c    | 12 ++++++------\n builtin/shortlog.c |  6 +++---\n 2 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 21f42b0..80d2431 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2522,12 +2522,12 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tenum object_type type;\n \tstruct commit *final_commit = NULL;\n \n-\tstatic struct string_list range_list;\n-\tstatic int output_option = 0, opt = 0;\n-\tstatic int show_stats = 0;\n-\tstatic const char *revs_file = NULL;\n-\tstatic const char *contents_from = NULL;\n-\tstatic const struct option options[] = {\n+\tstruct string_list range_list = STRING_LIST_INIT_NODUP;\n+\tint output_option = 0, opt = 0;\n+\tint show_stats = 0;\n+\tconst char *revs_file = NULL;\n+\tconst char *contents_from = NULL;\n+\tconst struct option options[] = {\n \t\tOPT_BOOL(0, \"incremental\", &incremental, N_(\"Show blame entries as we find them, incrementally\")),\n \t\tOPT_BOOL('b', NULL, &blank_boundary, N_(\"Show blank SHA-1 for boundary commits (Default: off)\")),\n \t\tOPT_BOOL(0, \"root\", &show_root, N_(\"Do not treat root commits as boundaries (Default: off)\")),\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex bfc082e..f83984e 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -233,11 +233,11 @@ void shortlog_init(struct shortlog *log)\n \n int cmd_shortlog(int argc, const char **argv, const char *prefix)\n {\n-\tstatic struct shortlog log;\n-\tstatic struct rev_info rev;\n+\tstruct shortlog log = { STRING_LIST_INIT_NODUP };\n+\tstruct rev_info rev;\n \tint nongit = !startup_info->have_repository;\n \n-\tstatic const struct option options[] = {\n+\tconst struct option options[] = {\n \t\tOPT_BOOL('n', \"numbered\", &log.sort_by_number,\n \t\t\t N_(\"sort output according to the number of commits per author\")),\n \t\tOPT_BOOL('s', \"summary\", &log.summary,\n-- \n2.9.0.rc2.149.gd580ccd\n"},{"id":"289094","messageId":"CACsJy8CAT54pTotUFKm-piWRppNFz9mjTsnz+5p1+7ykVg60HQ@mail.gmail.com","threadId":"42574","inReplyTo":"20160613053203.GB3950@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] fix parse-opt string_list leaks","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-13T09:36:14Z","receivedAt":"2016-06-16T02:19:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jun 13, 2016 at 12:32 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, Jun 13, 2016 at 07:08:55AM +0700, Duy Nguyen wrote:\n>\n>> > So if we are doing the conservative thing, then I think the resulting\n>> > code should either look like:\n>> >\n>> >   if (!v->strdup_strings)\n>> >         die(\"BUG: OPT_STRING_LIST should always use strdup_strings\");\n>> >   string_list_append(v, arg);\n>>\n>> I agree with the analysis. But this die() would hit all callers\n>> (except interpret-trailers) because they all initialize with _NODUP\n>> and setting strdup_strings may require auditing all access to the\n>> string list in question, e.g. to change string_list_append(v,\n>> xstrdup(xxx)) to string_list_append(xxx). it may cause side effects if\n>> we are not careful.\n>\n> Yep. It is not really fixing anything, so much as alerting us to broken\n> callers. We'd still have to fix the callers. :)\n>\n>> So far all callers are in builtin/, I think it will not take much time\n>> to verify that they all call parse_options() with global argv, then we\n>> can just lose extra xstrdup() and stick to string_list_append().\n>> OPTION_STRING already assumes that argument strings are stable because\n>> they are passed back as-is. Can we go with an easier route, adding a\n>> comment on top of parse_options() stating that argv[] pointers may be\n>> passed back as-is and it's up to the caller to xstrdup() appropriately\n>> before argv[] memory is freed?\n>\n> Yeah, the two options I laid out were the \"conservative\" side, where we\n> didn't make any assumptions about what is in passed into parse_options.\n> But I agree in practice that it's not likely to be a problem to just\n> point to the existing strings, and the fact that OPTION_STRING does so\n> already makes me even more confident.\n>\n> So I'd suggest these patches:\n>\n>   [1/3]: parse_opt_string_list: stop allocating new strings\n>   [2/3]: interpret-trailers: don't duplicate option strings\n>   [3/3]: blame,shortlog: don't make local option variables static\n\nAs usual, it's hard to find things to complain in your patches.\n\n> The first one is what we've been discussing, and the others are just\n> follow-on cleanups.  I stopped short of a fourth patch to convert more\n> cases of:\n>\n>   static struct string_list foo;\n>\n> to:\n>\n>   static struct string_list foo = STRING_LIST_INIT_NODUP;\n>\n> The two are equivalent (mostly due to historical reasons). I tend to\n> think explicit is better than implicit for something like this (not\n> because BSS auto-initialization isn't OK, but because there is an\n> explicit choice of dup/nodup that the writer made, and it is good to\n> communicate that). But maybe people don't want the extra noise.\n\nI'm on the explicit side. Unless you work a lot with string lists, I\ndon't think you can remember whether \"all zero\" initialization is dup\nor no dup.\n-- \nDuy\n"},{"id":"289096","messageId":"20160613100420.GA16229@sigill.intra.peff.net","threadId":"42574","inReplyTo":"CACsJy8CAT54pTotUFKm-piWRppNFz9mjTsnz+5p1+7ykVg60HQ@mail.gmail.com","subject":"[PATCH 4/3] use string_list initializer consistently","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-13T10:04:20Z","receivedAt":"2016-06-16T02:19:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 13, 2016 at 04:36:14PM +0700, Duy Nguyen wrote:\n\n> > So I'd suggest these patches:\n> >\n> >   [1/3]: parse_opt_string_list: stop allocating new strings\n> >   [2/3]: interpret-trailers: don't duplicate option strings\n> >   [3/3]: blame,shortlog: don't make local option variables static\n> \n> As usual, it's hard to find things to complain in your patches.\n\nThat's only because I work on the easy bits. I'm afraid of things like\nshallow-fetch refactoring. :)\n\n> > The first one is what we've been discussing, and the others are just\n> > follow-on cleanups.  I stopped short of a fourth patch to convert more\n> > cases of:\n> >\n> >   static struct string_list foo;\n> >\n> > to:\n> >\n> >   static struct string_list foo = STRING_LIST_INIT_NODUP;\n> >\n> > The two are equivalent (mostly due to historical reasons). I tend to\n> > think explicit is better than implicit for something like this (not\n> > because BSS auto-initialization isn't OK, but because there is an\n> > explicit choice of dup/nodup that the writer made, and it is good to\n> > communicate that). But maybe people don't want the extra noise.\n> \n> I'm on the explicit side. Unless you work a lot with string lists, I\n> don't think you can remember whether \"all zero\" initialization is dup\n> or no dup.\n\nHere's the patch for that.\n\n-- >8 --\nSubject: use string_list initializer consistently\n\nThere are two types of string_lists: those that own the\nstring memory, and those that don't. You can tell the\ndifference by the strdup_strings flag, and one should use\neither STRING_LIST_INIT_DUP, or STRING_LIST_INIT_NODUP as an\ninitializer.\n\nHistorically, the normal all-zeros initialization has\ncorresponded to the NODUP case. Many sites use no\ninitializer at all, and that works as a shorthand for that\ncase. But for a reader of the code, it can be hard to\nremember which is which. Let's be more explicit and actually\nhave each site declare which type it means to use.\n\nThis is a fairly mechanical conversion; I assumed each site\nwas correct as-is, and just switched them all to NODUP.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/apply.c               | 6 +++---\n builtin/blame.c               | 2 +-\n builtin/clone.c               | 4 ++--\n builtin/log.c                 | 6 +++---\n builtin/remote.c              | 2 +-\n notes.c                       | 2 +-\n submodule.c                   | 2 +-\n t/helper/test-parse-options.c | 2 +-\n 8 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex c770d7d..f27520f 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -270,7 +270,7 @@ struct image {\n  * the case where more than one patches touch the same file.\n  */\n \n-static struct string_list fn_table;\n+static struct string_list fn_table = STRING_LIST_INIT_NODUP;\n \n static uint32_t hash_line(const char *cp, size_t len)\n {\n@@ -1936,7 +1936,7 @@ static void prefix_patch(struct patch *p)\n  * include/exclude\n  */\n \n-static struct string_list limit_by_name;\n+static struct string_list limit_by_name = STRING_LIST_INIT_NODUP;\n static int has_include;\n static void add_name_limit(const char *name, int exclude)\n {\n@@ -3582,7 +3582,7 @@ static int check_to_create(const char *new_name, int ok_if_exists)\n  * it is perfectly fine, as the patch removes a/b to make room\n  * to create a directory a/b so that a/b/c can be created.\n  */\n-static struct string_list symlink_changes;\n+static struct string_list symlink_changes = STRING_LIST_INIT_NODUP;\n #define SYMLINK_GOES_AWAY 01\n #define SYMLINK_IN_RESULT 02\n \ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 80d2431..de70153 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -56,7 +56,7 @@ static int show_progress;\n static struct date_mode blame_date_mode = { DATE_ISO8601 };\n static size_t blame_date_width;\n \n-static struct string_list mailmap;\n+static struct string_list mailmap = STRING_LIST_INIT_NODUP;\n \n #ifndef DEBUG\n #define DEBUG 0\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 5f867e6..70d8213 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -49,8 +49,8 @@ static char *option_upload_pack = \"git-upload-pack\";\n static int option_verbosity;\n static int option_progress = -1;\n static enum transport_family family;\n-static struct string_list option_config;\n-static struct string_list option_reference;\n+static struct string_list option_config = STRING_LIST_INIT_NODUP;\n+static struct string_list option_reference = STRING_LIST_INIT_NODUP;\n static int option_dissociate;\n static int max_jobs = -1;\n \ndiff --git a/builtin/log.c b/builtin/log.c\nindex 099f4f7..8eef94f 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -674,9 +674,9 @@ static int auto_number = 1;\n \n static char *default_attach = NULL;\n \n-static struct string_list extra_hdr;\n-static struct string_list extra_to;\n-static struct string_list extra_cc;\n+static struct string_list extra_hdr = STRING_LIST_INIT_NODUP;\n+static struct string_list extra_to = STRING_LIST_INIT_NODUP;\n+static struct string_list extra_cc = STRING_LIST_INIT_NODUP;\n \n static void add_header(const char *value)\n {\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex d33766b..5ded301 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -247,7 +247,7 @@ struct branch_info {\n \tenum { NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE } rebase;\n };\n \n-static struct string_list branch_list;\n+static struct string_list branch_list = STRING_LIST_INIT_NODUP;\n \n static const char *abbrev_ref(const char *name, const char *prefix)\n {\ndiff --git a/notes.c b/notes.c\nindex e4e4854..df4660f 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -70,7 +70,7 @@ struct non_note {\n \n struct notes_tree default_notes_tree;\n \n-static struct string_list display_notes_refs;\n+static struct string_list display_notes_refs = STRING_LIST_INIT_NODUP;\n static struct notes_tree **display_notes_trees;\n \n static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\ndiff --git a/submodule.c b/submodule.c\nindex 4532b11..abc2ac2 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -17,7 +17,7 @@\n \n static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n static int parallel_jobs = 1;\n-static struct string_list changed_submodule_paths;\n+static struct string_list changed_submodule_paths = STRING_LIST_INIT_NODUP;\n static int initialized_fetch_ref_tips;\n static struct sha1_array ref_tips_before_fetch;\n static struct sha1_array ref_tips_after_fetch;\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex 8a1235d..2c63298 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -12,7 +12,7 @@ static int dry_run = 0, quiet = 0;\n static char *string = NULL;\n static char *file = NULL;\n static int ambiguous;\n-static struct string_list list;\n+static struct string_list list = STRING_LIST_INIT_NODUP;\n \n static struct {\n \tint called;\n-- \n2.9.0.rc2.149.gd580ccd\n"},{"id":"289099","messageId":"CACsJy8CoOJKLUQXPKx_KvHvwbUdOfyBKu_-v5F2U7ccrmCcewA@mail.gmail.com","threadId":"42574","inReplyTo":"20160613100420.GA16229@sigill.intra.peff.net","subject":"Re: [PATCH 4/3] use string_list initializer consistently","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-13T11:31:29Z","receivedAt":"2016-06-16T02:19:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jun 13, 2016 at 5:04 PM, Jeff King <peff@peff.net> wrote:\n> -- >8 --\n> Subject: use string_list initializer consistently\n>\n> There are two types of string_lists: those that own the\n> string memory, and those that don't. You can tell the\n> difference by the strdup_strings flag, and one should use\n> either STRING_LIST_INIT_DUP, or STRING_LIST_INIT_NODUP as an\n> initializer.\n>\n> Historically, the normal all-zeros initialization has\n> corresponded to the NODUP case. Many sites use no\n> initializer at all, and that works as a shorthand for that\n> case. But for a reader of the code, it can be hard to\n> remember which is which. Let's be more explicit and actually\n> have each site declare which type it means to use.\n>\n> This is a fairly mechanical conversion; I assumed each site\n> was correct as-is, and just switched them all to NODUP.\n\nLooking good. If we still want to reduce noise level down (by a tiny\nbit), we could remove _INIT because I think it's obvious from\n\nstruct string_list var = STRING_LIST_NODUP;\n\nthat's it's initialization (I wish we could write \"auto var =\nSTRING_LIST_NODUP;\")\n-- \nDuy\n"},{"id":"289157","messageId":"20160613173246.GA8245@sigill.intra.peff.net","threadId":"42574","inReplyTo":"CACsJy8CoOJKLUQXPKx_KvHvwbUdOfyBKu_-v5F2U7ccrmCcewA@mail.gmail.com","subject":"Re: [PATCH 4/3] use string_list initializer consistently","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-13T17:32:47Z","receivedAt":"2016-06-16T06:55:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 13, 2016 at 06:31:29PM +0700, Duy Nguyen wrote:\n\n> > This is a fairly mechanical conversion; I assumed each site\n> > was correct as-is, and just switched them all to NODUP.\n> \n> Looking good. If we still want to reduce noise level down (by a tiny\n> bit), we could remove _INIT because I think it's obvious from\n> \n> struct string_list var = STRING_LIST_NODUP;\n> \n> that's it's initialization (I wish we could write \"auto var =\n> STRING_LIST_NODUP;\")\n\nYeah, I've always felt it's a bit long. The \"INIT\" does match other\nsimilar macros, but I agree it could probably go. Definitely something\nfor a separate patch, though.\n\n-Peff\n"},{"id":"289182","messageId":"CAPig+cQudTwT6H35YgEvxdK9AAn0BwvUC8bi8-DxGYNh9rb_Lw@mail.gmail.com","threadId":"42574","inReplyTo":"20160613053928.GC23880@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] blame,shortlog: don't make local option variables static","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-06-14T04:32:15Z","receivedAt":"2016-06-16T06:55:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 13, 2016 at 1:39 AM, Jeff King <peff@peff.net> wrote:\n> There's no need for these option variables to be static,\n> except that they are referenced by the options array itself,\n> which is static. But having all of this static is simply\n> unnecessary and confusing (and inconsistent with most other\n> commands, which either use a static global option list or a\n> true function-local one).\n>\n> Note that in some cases we may need to actually initialize\n> the variables (since we cannot rely on BSS to do so). This\n> is a net improvement to readability, though, as we can use\n> the more verbose initializers for our string_lists.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> @@ -2522,12 +2522,12 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n> -       static struct string_list range_list;\n> -       static int output_option = 0, opt = 0;\n> -       static int show_stats = 0;\n> -       static const char *revs_file = NULL;\n> -       static const char *contents_from = NULL;\n> -       static const struct option options[] = {\n> +       struct string_list range_list = STRING_LIST_INIT_NODUP;\n\nRelated to this series, there's an additional \"fix\" which ought to be\nmade, probably as a separate patch. In particular, in cmd_blame():\n\n    if (lno && !range_list.nr)\n        string_list_append(&range_list, xstrdup(\"1\"));\n\nwhich supplies a default range (\"line 1 through end of file\") if -L\nwas not specified. I used xstrdup() on the literal \"1\" in 58dbfa2\n(blame: accept multiple -L ranges, 2013-08-06) to be consistent with\nparse_opt_string_list() which was unconditionally xstrdup'ing the\nargument (but no longer does as of patch 1/3 of this series).\n\n> +       int output_option = 0, opt = 0;\n> +       int show_stats = 0;\n> +       const char *revs_file = NULL;\n> +       const char *contents_from = NULL;\n> +       const struct option options[] = {\n>                 OPT_BOOL(0, \"incremental\", &incremental, N_(\"Show blame entries as we find them, incrementally\")),\n>                 OPT_BOOL('b', NULL, &blank_boundary, N_(\"Show blank SHA-1 for boundary commits (Default: off)\")),\n>                 OPT_BOOL(0, \"root\", &show_root, N_(\"Do not treat root commits as boundaries (Default: off)\")),\n"},{"id":"289184","messageId":"20160614050541.GA9909@sigill.intra.peff.net","threadId":"42574","inReplyTo":"CAPig+cQudTwT6H35YgEvxdK9AAn0BwvUC8bi8-DxGYNh9rb_Lw@mail.gmail.com","subject":"Re: [PATCH 3/3] blame,shortlog: don't make local option variables static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-14T05:05:41Z","receivedAt":"2016-06-16T06:55:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 14, 2016 at 12:32:15AM -0400, Eric Sunshine wrote:\n\n> > +       struct string_list range_list = STRING_LIST_INIT_NODUP;\n> \n> Related to this series, there's an additional \"fix\" which ought to be\n> made, probably as a separate patch. In particular, in cmd_blame():\n> \n>     if (lno && !range_list.nr)\n>         string_list_append(&range_list, xstrdup(\"1\"));\n> \n> which supplies a default range (\"line 1 through end of file\") if -L\n> was not specified. I used xstrdup() on the literal \"1\" in 58dbfa2\n> (blame: accept multiple -L ranges, 2013-08-06) to be consistent with\n> parse_opt_string_list() which was unconditionally xstrdup'ing the\n> argument (but no longer does as of patch 1/3 of this series).\n\nYeah, I'd agree that this is a minor bug both before and after the\nseries due to the leak. Want to roll a patch on top?\n\n-Peff\n"},{"id":"292827","messageId":"20160802105252.jyl5x7eanttw2swj@sigill.intra.peff.net","threadId":"42574","inReplyTo":"20160614050541.GA9909@sigill.intra.peff.net","subject":"[PATCH] blame: drop strdup of string literal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-02T10:52:53Z","receivedAt":"2016-08-02T10:59:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 14, 2016 at 01:05:41AM -0400, Jeff King wrote:\n\n> On Tue, Jun 14, 2016 at 12:32:15AM -0400, Eric Sunshine wrote:\n> \n> > > +       struct string_list range_list = STRING_LIST_INIT_NODUP;\n> > \n> > Related to this series, there's an additional \"fix\" which ought to be\n> > made, probably as a separate patch. In particular, in cmd_blame():\n> > \n> >     if (lno && !range_list.nr)\n> >         string_list_append(&range_list, xstrdup(\"1\"));\n> > \n> > which supplies a default range (\"line 1 through end of file\") if -L\n> > was not specified. I used xstrdup() on the literal \"1\" in 58dbfa2\n> > (blame: accept multiple -L ranges, 2013-08-06) to be consistent with\n> > parse_opt_string_list() which was unconditionally xstrdup'ing the\n> > argument (but no longer does as of patch 1/3 of this series).\n> \n> Yeah, I'd agree that this is a minor bug both before and after the\n> series due to the leak. Want to roll a patch on top?\n\nHere it is, just to tie up a loose end. I marked you as the author since\nthe hard part was noticing the issue and explaining the history, which\nyou already did above.\n\n-- >8 --\nFrom: Eric Sunshine <sunshine@sunshineco.com>\nSubject: [PATCH] blame: drop strdup of string literal\n\nThis strdup was added as part of 58dbfa2 (blame: accept\nmultiple -L ranges, 2013-08-06) to be consistent with\nparse_opt_string_list(), which appends to the same list.\n\nBut as of 7a7a517 (parse_opt_string_list: stop allocating\nnew strings, 2016-06-13), we should stop using strdup (to\nmatch parse_opt_string_list, and for all the reasons\ndescribed in that commit; namely that it does nothing useful\nand causes us to leak the memory).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/blame.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex ab66cde..29bd479 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2805,7 +2805,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tlno = prepare_lines(&sb);\n \n \tif (lno && !range_list.nr)\n-\t\tstring_list_append(&range_list, xstrdup(\"1\"));\n+\t\tstring_list_append(&range_list, \"1\");\n \n \tanchor = 1;\n \trange_set_init(&ranges, range_list.nr);\n-- \n2.9.2.670.g42e63de\n\n"},{"id":"292886","messageId":"CAPig+cT4x2ndMde8RMsseHcuSZpOoPgfXLRFUOyBK_iAz-xKyg@mail.gmail.com","threadId":"42574","inReplyTo":"20160802105252.jyl5x7eanttw2swj@sigill.intra.peff.net","subject":"Re: [PATCH] blame: drop strdup of string literal","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-08-03T07:36:35Z","receivedAt":"2016-08-03T07:37:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 2, 2016 at 6:52 AM, Jeff King <peff@peff.net> wrote:\n> On Tue, Jun 14, 2016 at 01:05:41AM -0400, Jeff King wrote:>\n>> On Tue, Jun 14, 2016 at 12:32:15AM -0400, Eric Sunshine wrote:\n>> > > +       struct string_list range_list = STRING_LIST_INIT_NODUP;\n>> >\n>> > Related to this series, there's an additional \"fix\" which ought to be\n>> > made, probably as a separate patch. In particular, in cmd_blame():\n>> >\n>> >     if (lno && !range_list.nr)\n>> >         string_list_append(&range_list, xstrdup(\"1\"));\n>> >\n>> > which supplies a default range (\"line 1 through end of file\") if -L\n>> > was not specified. I used xstrdup() on the literal \"1\" in 58dbfa2\n>> > (blame: accept multiple -L ranges, 2013-08-06) to be consistent with\n>> > parse_opt_string_list() which was unconditionally xstrdup'ing the\n>> > argument (but no longer does as of patch 1/3 of this series).\n>>\n>> Yeah, I'd agree that this is a minor bug both before and after the\n>> series due to the leak. Want to roll a patch on top?\n>\n> Here it is, just to tie up a loose end. I marked you as the author since\n> the hard part was noticing the issue and explaining the history, which\n> you already did above.\n\nThanks for picking up the slack, and my apologies for not being able\nto find time to submit the patch myself (my computer time is severely\nlimited these days).\n\n> -- >8 --\n> From: Eric Sunshine <sunshine@sunshineco.com>\n> Subject: [PATCH] blame: drop strdup of string literal\n>\n> This strdup was added as part of 58dbfa2 (blame: accept\n> multiple -L ranges, 2013-08-06) to be consistent with\n> parse_opt_string_list(), which appends to the same list.\n>\n> But as of 7a7a517 (parse_opt_string_list: stop allocating\n> new strings, 2016-06-13), we should stop using strdup (to\n> match parse_opt_string_list, and for all the reasons\n> described in that commit; namely that it does nothing useful\n> and causes us to leak the memory).\n\nA nice explanation, and the patch itself looks \"obviously correct\".\n\n> Signed-off-by: Jeff King <peff@peff.net>\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n\n> ---\n>  builtin/blame.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index ab66cde..29bd479 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2805,7 +2805,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n>         lno = prepare_lines(&sb);\n>\n>         if (lno && !range_list.nr)\n> -               string_list_append(&range_list, xstrdup(\"1\"));\n> +               string_list_append(&range_list, \"1\");\n>\n>         anchor = 1;\n>         range_set_init(&ranges, range_list.nr);\n> --\n> 2.9.2.670.g42e63de\n"}]}