[PATCH] parse-options-cb.c: use string_list_append_nodup in OPT_STRING_LIST()

[PATCH] parse-options-cb.c: use string_list_append_nodup in OPT_STRING_LIST()

[Nguyễn Thái Ngọc Duy]

If the given string list has strdup_strings set (*), the string will be
duplicated again. Pointless and leak memory. Ignore that flag.

(*) only interpret-trailers.c does it at the moment

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 parse-options-cb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/parse-options-cb.c b/parse-options-cb.c
index 239898d..8a1b6e6 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -144,7 +144,7 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
 	if (!arg)
 		return -1;
 
-	string_list_append(v, xstrdup(arg));
+	string_list_append_nodup(v, xstrdup(arg));
 	return 0;
 }
 
-- 
2.8.2.524.g6ff3d78

Re: [PATCH] parse-options-cb.c: use string_list_append_nodup in OPT_STRING_LIST()

[Jeff King]

On Fri, Jun 10, 2016 at 06:57:26PM +0700, Nguyễn Thái Ngọc Duy wrote:

> If the given string list has strdup_strings set (*), the string will be
> duplicated again. Pointless and leak memory. Ignore that flag.
> 
> (*) only interpret-trailers.c does it at the moment
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  parse-options-cb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index 239898d..8a1b6e6 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -144,7 +144,7 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
>  	if (!arg)
>  		return -1;
>  
> -	string_list_append(v, xstrdup(arg));
> +	string_list_append_nodup(v, xstrdup(arg));

Hmm. So I agree this is an improvement, in the sense that we are
double-allocating when v->strdup_strings is set.  But I think there's a
deeper issue here. Why are we always allocating in the first place?

If the memory we are getting in "arg" is not stable, then we _do_ need
to make a copy of it. But in that case, we want "strdup_strings" to be
set; without it any time we later run string_list_clear(), we leak the
allocated memory, because the struct has no idea that it is the owner of
the memory (and we do call string_list_clear() when we see "--no-foo").

If the memory _is_ stable, then we are fine to add a direct reference to
it, and can lose the extra xstrdup() here. Only the caller knows for
sure, so we should be respecting their value of strdup_strings (so lose
the xstrdup, but keep calling string_list_append()).

In practice, I suspect the memory _is_ stable, because we are generally
parsing command-line arguments. But it does not hurt to stay on the
conservative side, and always make a copy (in case we are parsing
something besides the global argv array) . Apparently I am the original
author of this code, in c8ba163 (parse-options: add OPT_STRING_LIST
helper, 2011-06-09), but there's no mention of this point there, in the
list archives, or in my brain.

So if we are doing the conservative thing, then I think the resulting
code should either look like:

  if (!v->strdup_strings)
	die("BUG: OPT_STRING_LIST should always use strdup_strings");
  string_list_append(v, arg);

or:

  /* silently enable for convenience */
  v->strdup_strings = 1;
  string_list_append(v, arg);

Of the two, I like the top one as it is less magical, but it would
require adjusting the initialization of the string-list for most of the
callers.

-Peff

Re: [PATCH] parse-options-cb.c: use string_list_append_nodup in OPT_STRING_LIST()

[Duy Nguyen]

On Mon, Jun 13, 2016 at 5:03 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Jun 10, 2016 at 06:57:26PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> If the given string list has strdup_strings set (*), the string will be
>> duplicated again. Pointless and leak memory. Ignore that flag.
>>
>> (*) only interpret-trailers.c does it at the moment
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  parse-options-cb.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/parse-options-cb.c b/parse-options-cb.c
>> index 239898d..8a1b6e6 100644
>> --- a/parse-options-cb.c
>> +++ b/parse-options-cb.c
>> @@ -144,7 +144,7 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
>>       if (!arg)
>>               return -1;
>>
>> -     string_list_append(v, xstrdup(arg));
>> +     string_list_append_nodup(v, xstrdup(arg));
>
> Hmm. So I agree this is an improvement, in the sense that we are
> double-allocating when v->strdup_strings is set.  But I think there's a
> deeper issue here. Why are we always allocating in the first place?
>
> If the memory we are getting in "arg" is not stable, then we _do_ need
> to make a copy of it. But in that case, we want "strdup_strings" to be
> set; without it any time we later run string_list_clear(), we leak the
> allocated memory, because the struct has no idea that it is the owner of
> the memory (and we do call string_list_clear() when we see "--no-foo").
>
> If the memory _is_ stable, then we are fine to add a direct reference to
> it, and can lose the extra xstrdup() here. Only the caller knows for
> sure, so we should be respecting their value of strdup_strings (so lose
> the xstrdup, but keep calling string_list_append()).
>
> In practice, I suspect the memory _is_ stable, because we are generally
> parsing command-line arguments. But it does not hurt to stay on the
> conservative side, and always make a copy (in case we are parsing
> something besides the global argv array) . Apparently I am the original
> author of this code, in c8ba163 (parse-options: add OPT_STRING_LIST
> helper, 2011-06-09), but there's no mention of this point there, in the
> list archives, or in my brain.
>
> So if we are doing the conservative thing, then I think the resulting
> code should either look like:
>
>   if (!v->strdup_strings)
>         die("BUG: OPT_STRING_LIST should always use strdup_strings");
>   string_list_append(v, arg);

I agree with the analysis. But this die() would hit all callers
(except interpret-trailers) because they all initialize with _NODUP
and setting strdup_strings may require auditing all access to the
string list in question, e.g. to change string_list_append(v,
xstrdup(xxx)) to string_list_append(xxx). it may cause side effects if
we are not careful.

So far all callers are in builtin/, I think it will not take much time
to verify that they all call parse_options() with global argv, then we
can just lose extra xstrdup() and stick to string_list_append().
OPTION_STRING already assumes that argument strings are stable because
they are passed back as-is. Can we go with an easier route, adding a
comment on top of parse_options() stating that argv[] pointers may be
passed back as-is and it's up to the caller to xstrdup() appropriately
before argv[] memory is freed?

>
> or:
>
>   /* silently enable for convenience */
>   v->strdup_strings = 1;
>   string_list_append(v, arg);
>
> Of the two, I like the top one as it is less magical, but it would
> require adjusting the initialization of the string-list for most of the
> callers.
>
> -Peff



-- 
Duy

[PATCH 0/3] fix parse-opt string_list leaks

[Jeff King]

On Mon, Jun 13, 2016 at 07:08:55AM +0700, Duy Nguyen wrote:

> > So if we are doing the conservative thing, then I think the resulting
> > code should either look like:
> >
> >   if (!v->strdup_strings)
> >         die("BUG: OPT_STRING_LIST should always use strdup_strings");
> >   string_list_append(v, arg);
> 
> I agree with the analysis. But this die() would hit all callers
> (except interpret-trailers) because they all initialize with _NODUP
> and setting strdup_strings may require auditing all access to the
> string list in question, e.g. to change string_list_append(v,
> xstrdup(xxx)) to string_list_append(xxx). it may cause side effects if
> we are not careful.

Yep. It is not really fixing anything, so much as alerting us to broken
callers. We'd still have to fix the callers. :)

> So far all callers are in builtin/, I think it will not take much time
> to verify that they all call parse_options() with global argv, then we
> can just lose extra xstrdup() and stick to string_list_append().
> OPTION_STRING already assumes that argument strings are stable because
> they are passed back as-is. Can we go with an easier route, adding a
> comment on top of parse_options() stating that argv[] pointers may be
> passed back as-is and it's up to the caller to xstrdup() appropriately
> before argv[] memory is freed?

Yeah, the two options I laid out were the "conservative" side, where we
didn't make any assumptions about what is in passed into parse_options.
But I agree in practice that it's not likely to be a problem to just
point to the existing strings, and the fact that OPTION_STRING does so
already makes me even more confident.

So I'd suggest these patches:

  [1/3]: parse_opt_string_list: stop allocating new strings
  [2/3]: interpret-trailers: don't duplicate option strings
  [3/3]: blame,shortlog: don't make local option variables static

The first one is what we've been discussing, and the others are just
follow-on cleanups.  I stopped short of a fourth patch to convert more
cases of:

  static struct string_list foo;

to:

  static struct string_list foo = STRING_LIST_INIT_NODUP;

The two are equivalent (mostly due to historical reasons). I tend to
think explicit is better than implicit for something like this (not
because BSS auto-initialization isn't OK, but because there is an
explicit choice of dup/nodup that the writer made, and it is good to
communicate that). But maybe people don't want the extra noise.

-Peff

[PATCH 1/3] parse_opt_string_list: stop allocating new strings

[Jeff King]

The parse_opt_string_list callback is basically a thin
wrapper to string_list_append() any string options we get.
However, it calls:

  string_list_append(v, xstrdup(arg));

which duplicates the option value. This is wrong for two
reasons:

  1. If the string list has strdup_strings set, then we are
     making an extra copy, which is simply leaked.

  2. If the string list does not have strdup_strings set,
     then we pass memory ownership to the string list, but
     it does not realize this. If we later call
     string_list_clear(), which can happen if "--no-foo" is
     passed, then we will leak all of the existing entries.

Instead, we should just pass the argument straight to
string_list_append, and it can decide whether to copy or not
based on its strdup_strings flag.

It's possible that some (buggy) caller could be relying on
this extra copy (e.g., because it parses some options from
an allocated argv array and then frees the array), but it's
not likely. For one, we generally only use parse_options on
the argv given to us in main(). And two, such a caller is
broken anyway, because other option types like OPT_STRING()
do not make such a copy.  This patch brings us in line with
them.

Noticed-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 parse-options-cb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/parse-options-cb.c b/parse-options-cb.c
index 239898d..ba5acf3 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -144,7 +144,7 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
 	if (!arg)
 		return -1;
 
-	string_list_append(v, xstrdup(arg));
+	string_list_append(v, arg);
 	return 0;
 }
 
-- 
2.9.0.rc2.149.gd580ccd

[PATCH 2/3] interpret-trailers: don't duplicate option strings

[Jeff King]

There's no need to do so; the argv strings will last until
the end of the program.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/interpret-trailers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index b99ae4b..175f147 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -20,7 +20,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
 	int in_place = 0;
 	int trim_empty = 0;
-	struct string_list trailers = STRING_LIST_INIT_DUP;
+	struct string_list trailers = STRING_LIST_INIT_NODUP;
 
 	struct option options[] = {
 		OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
-- 
2.9.0.rc2.149.gd580ccd

[PATCH 3/3] blame,shortlog: don't make local option variables static

[Jeff King]

There's no need for these option variables to be static,
except that they are referenced by the options array itself,
which is static. But having all of this static is simply
unnecessary and confusing (and inconsistent with most other
commands, which either use a static global option list or a
true function-local one).

Note that in some cases we may need to actually initialize
the variables (since we cannot rely on BSS to do so). This
is a net improvement to readability, though, as we can use
the more verbose initializers for our string_lists.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/blame.c    | 12 ++++++------
 builtin/shortlog.c |  6 +++---
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 21f42b0..80d2431 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2522,12 +2522,12 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	enum object_type type;
 	struct commit *final_commit = NULL;
 
-	static struct string_list range_list;
-	static int output_option = 0, opt = 0;
-	static int show_stats = 0;
-	static const char *revs_file = NULL;
-	static const char *contents_from = NULL;
-	static const struct option options[] = {
+	struct string_list range_list = STRING_LIST_INIT_NODUP;
+	int output_option = 0, opt = 0;
+	int show_stats = 0;
+	const char *revs_file = NULL;
+	const char *contents_from = NULL;
+	const struct option options[] = {
 		OPT_BOOL(0, "incremental", &incremental, N_("Show blame entries as we find them, incrementally")),
 		OPT_BOOL('b', NULL, &blank_boundary, N_("Show blank SHA-1 for boundary commits (Default: off)")),
 		OPT_BOOL(0, "root", &show_root, N_("Do not treat root commits as boundaries (Default: off)")),
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index bfc082e..f83984e 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -233,11 +233,11 @@ void shortlog_init(struct shortlog *log)
 
 int cmd_shortlog(int argc, const char **argv, const char *prefix)
 {
-	static struct shortlog log;
-	static struct rev_info rev;
+	struct shortlog log = { STRING_LIST_INIT_NODUP };
+	struct rev_info rev;
 	int nongit = !startup_info->have_repository;
 
-	static const struct option options[] = {
+	const struct option options[] = {
 		OPT_BOOL('n', "numbered", &log.sort_by_number,
 			 N_("sort output according to the number of commits per author")),
 		OPT_BOOL('s', "summary", &log.summary,
-- 
2.9.0.rc2.149.gd580ccd

Re: [PATCH 3/3] blame,shortlog: don't make local option variables static

[Eric Sunshine]

On Mon, Jun 13, 2016 at 1:39 AM, Jeff King <peff@peff.net> wrote:
> There's no need for these option variables to be static,
> except that they are referenced by the options array itself,
> which is static. But having all of this static is simply
> unnecessary and confusing (and inconsistent with most other
> commands, which either use a static global option list or a
> true function-local one).
>
> Note that in some cases we may need to actually initialize
> the variables (since we cannot rely on BSS to do so). This
> is a net improvement to readability, though, as we can use
> the more verbose initializers for our string_lists.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/builtin/blame.c b/builtin/blame.c
> @@ -2522,12 +2522,12 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
> -       static struct string_list range_list;
> -       static int output_option = 0, opt = 0;
> -       static int show_stats = 0;
> -       static const char *revs_file = NULL;
> -       static const char *contents_from = NULL;
> -       static const struct option options[] = {
> +       struct string_list range_list = STRING_LIST_INIT_NODUP;

Related to this series, there's an additional "fix" which ought to be
made, probably as a separate patch. In particular, in cmd_blame():

    if (lno && !range_list.nr)
        string_list_append(&range_list, xstrdup("1"));

which supplies a default range ("line 1 through end of file") if -L
was not specified. I used xstrdup() on the literal "1" in 58dbfa2
(blame: accept multiple -L ranges, 2013-08-06) to be consistent with
parse_opt_string_list() which was unconditionally xstrdup'ing the
argument (but no longer does as of patch 1/3 of this series).

> +       int output_option = 0, opt = 0;
> +       int show_stats = 0;
> +       const char *revs_file = NULL;
> +       const char *contents_from = NULL;
> +       const struct option options[] = {
>                 OPT_BOOL(0, "incremental", &incremental, N_("Show blame entries as we find them, incrementally")),
>                 OPT_BOOL('b', NULL, &blank_boundary, N_("Show blank SHA-1 for boundary commits (Default: off)")),
>                 OPT_BOOL(0, "root", &show_root, N_("Do not treat root commits as boundaries (Default: off)")),

Re: [PATCH 3/3] blame,shortlog: don't make local option variables static

[Jeff King]

On Tue, Jun 14, 2016 at 12:32:15AM -0400, Eric Sunshine wrote:

> > +       struct string_list range_list = STRING_LIST_INIT_NODUP;
> 
> Related to this series, there's an additional "fix" which ought to be
> made, probably as a separate patch. In particular, in cmd_blame():
> 
>     if (lno && !range_list.nr)
>         string_list_append(&range_list, xstrdup("1"));
> 
> which supplies a default range ("line 1 through end of file") if -L
> was not specified. I used xstrdup() on the literal "1" in 58dbfa2
> (blame: accept multiple -L ranges, 2013-08-06) to be consistent with
> parse_opt_string_list() which was unconditionally xstrdup'ing the
> argument (but no longer does as of patch 1/3 of this series).

Yeah, I'd agree that this is a minor bug both before and after the
series due to the leak. Want to roll a patch on top?

-Peff

[PATCH] blame: drop strdup of string literal

[Jeff King]

On Tue, Jun 14, 2016 at 01:05:41AM -0400, Jeff King wrote:

> On Tue, Jun 14, 2016 at 12:32:15AM -0400, Eric Sunshine wrote:
> 
> > > +       struct string_list range_list = STRING_LIST_INIT_NODUP;
> > 
> > Related to this series, there's an additional "fix" which ought to be
> > made, probably as a separate patch. In particular, in cmd_blame():
> > 
> >     if (lno && !range_list.nr)
> >         string_list_append(&range_list, xstrdup("1"));
> > 
> > which supplies a default range ("line 1 through end of file") if -L
> > was not specified. I used xstrdup() on the literal "1" in 58dbfa2
> > (blame: accept multiple -L ranges, 2013-08-06) to be consistent with
> > parse_opt_string_list() which was unconditionally xstrdup'ing the
> > argument (but no longer does as of patch 1/3 of this series).
> 
> Yeah, I'd agree that this is a minor bug both before and after the
> series due to the leak. Want to roll a patch on top?

Here it is, just to tie up a loose end. I marked you as the author since
the hard part was noticing the issue and explaining the history, which
you already did above.

-- >8 --
From: Eric Sunshine <sunshine@sunshineco.com>
Subject: [PATCH] blame: drop strdup of string literal

This strdup was added as part of 58dbfa2 (blame: accept
multiple -L ranges, 2013-08-06) to be consistent with
parse_opt_string_list(), which appends to the same list.

But as of 7a7a517 (parse_opt_string_list: stop allocating
new strings, 2016-06-13), we should stop using strdup (to
match parse_opt_string_list, and for all the reasons
described in that commit; namely that it does nothing useful
and causes us to leak the memory).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/blame.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index ab66cde..29bd479 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2805,7 +2805,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	lno = prepare_lines(&sb);
 
 	if (lno && !range_list.nr)
-		string_list_append(&range_list, xstrdup("1"));
+		string_list_append(&range_list, "1");
 
 	anchor = 1;
 	range_set_init(&ranges, range_list.nr);
-- 
2.9.2.670.g42e63de

Re: [PATCH] blame: drop strdup of string literal

[Eric Sunshine]

On Tue, Aug 2, 2016 at 6:52 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Jun 14, 2016 at 01:05:41AM -0400, Jeff King wrote:>
>> On Tue, Jun 14, 2016 at 12:32:15AM -0400, Eric Sunshine wrote:
>> > > +       struct string_list range_list = STRING_LIST_INIT_NODUP;
>> >
>> > Related to this series, there's an additional "fix" which ought to be
>> > made, probably as a separate patch. In particular, in cmd_blame():
>> >
>> >     if (lno && !range_list.nr)
>> >         string_list_append(&range_list, xstrdup("1"));
>> >
>> > which supplies a default range ("line 1 through end of file") if -L
>> > was not specified. I used xstrdup() on the literal "1" in 58dbfa2
>> > (blame: accept multiple -L ranges, 2013-08-06) to be consistent with
>> > parse_opt_string_list() which was unconditionally xstrdup'ing the
>> > argument (but no longer does as of patch 1/3 of this series).
>>
>> Yeah, I'd agree that this is a minor bug both before and after the
>> series due to the leak. Want to roll a patch on top?
>
> Here it is, just to tie up a loose end. I marked you as the author since
> the hard part was noticing the issue and explaining the history, which
> you already did above.

Thanks for picking up the slack, and my apologies for not being able
to find time to submit the patch myself (my computer time is severely
limited these days).

> -- >8 --
> From: Eric Sunshine <sunshine@sunshineco.com>
> Subject: [PATCH] blame: drop strdup of string literal
>
> This strdup was added as part of 58dbfa2 (blame: accept
> multiple -L ranges, 2013-08-06) to be consistent with
> parse_opt_string_list(), which appends to the same list.
>
> But as of 7a7a517 (parse_opt_string_list: stop allocating
> new strings, 2016-06-13), we should stop using strdup (to
> match parse_opt_string_list, and for all the reasons
> described in that commit; namely that it does nothing useful
> and causes us to leak the memory).

A nice explanation, and the patch itself looks "obviously correct".

> Signed-off-by: Jeff King <peff@peff.net>

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>

> ---
>  builtin/blame.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index ab66cde..29bd479 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2805,7 +2805,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>         lno = prepare_lines(&sb);
>
>         if (lno && !range_list.nr)
> -               string_list_append(&range_list, xstrdup("1"));
> +               string_list_append(&range_list, "1");
>
>         anchor = 1;
>         range_set_init(&ranges, range_list.nr);
> --
> 2.9.2.670.g42e63de

Re: [PATCH 0/3] fix parse-opt string_list leaks

[Duy Nguyen]

On Mon, Jun 13, 2016 at 12:32 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Jun 13, 2016 at 07:08:55AM +0700, Duy Nguyen wrote:
>
>> > So if we are doing the conservative thing, then I think the resulting
>> > code should either look like:
>> >
>> >   if (!v->strdup_strings)
>> >         die("BUG: OPT_STRING_LIST should always use strdup_strings");
>> >   string_list_append(v, arg);
>>
>> I agree with the analysis. But this die() would hit all callers
>> (except interpret-trailers) because they all initialize with _NODUP
>> and setting strdup_strings may require auditing all access to the
>> string list in question, e.g. to change string_list_append(v,
>> xstrdup(xxx)) to string_list_append(xxx). it may cause side effects if
>> we are not careful.
>
> Yep. It is not really fixing anything, so much as alerting us to broken
> callers. We'd still have to fix the callers. :)
>
>> So far all callers are in builtin/, I think it will not take much time
>> to verify that they all call parse_options() with global argv, then we
>> can just lose extra xstrdup() and stick to string_list_append().
>> OPTION_STRING already assumes that argument strings are stable because
>> they are passed back as-is. Can we go with an easier route, adding a
>> comment on top of parse_options() stating that argv[] pointers may be
>> passed back as-is and it's up to the caller to xstrdup() appropriately
>> before argv[] memory is freed?
>
> Yeah, the two options I laid out were the "conservative" side, where we
> didn't make any assumptions about what is in passed into parse_options.
> But I agree in practice that it's not likely to be a problem to just
> point to the existing strings, and the fact that OPTION_STRING does so
> already makes me even more confident.
>
> So I'd suggest these patches:
>
>   [1/3]: parse_opt_string_list: stop allocating new strings
>   [2/3]: interpret-trailers: don't duplicate option strings
>   [3/3]: blame,shortlog: don't make local option variables static

As usual, it's hard to find things to complain in your patches.

> The first one is what we've been discussing, and the others are just
> follow-on cleanups.  I stopped short of a fourth patch to convert more
> cases of:
>
>   static struct string_list foo;
>
> to:
>
>   static struct string_list foo = STRING_LIST_INIT_NODUP;
>
> The two are equivalent (mostly due to historical reasons). I tend to
> think explicit is better than implicit for something like this (not
> because BSS auto-initialization isn't OK, but because there is an
> explicit choice of dup/nodup that the writer made, and it is good to
> communicate that). But maybe people don't want the extra noise.

I'm on the explicit side. Unless you work a lot with string lists, I
don't think you can remember whether "all zero" initialization is dup
or no dup.
-- 
Duy

[PATCH 4/3] use string_list initializer consistently

[Jeff King]

On Mon, Jun 13, 2016 at 04:36:14PM +0700, Duy Nguyen wrote:

> > So I'd suggest these patches:
> >
> >   [1/3]: parse_opt_string_list: stop allocating new strings
> >   [2/3]: interpret-trailers: don't duplicate option strings
> >   [3/3]: blame,shortlog: don't make local option variables static
> 
> As usual, it's hard to find things to complain in your patches.

That's only because I work on the easy bits. I'm afraid of things like
shallow-fetch refactoring. :)

> > The first one is what we've been discussing, and the others are just
> > follow-on cleanups.  I stopped short of a fourth patch to convert more
> > cases of:
> >
> >   static struct string_list foo;
> >
> > to:
> >
> >   static struct string_list foo = STRING_LIST_INIT_NODUP;
> >
> > The two are equivalent (mostly due to historical reasons). I tend to
> > think explicit is better than implicit for something like this (not
> > because BSS auto-initialization isn't OK, but because there is an
> > explicit choice of dup/nodup that the writer made, and it is good to
> > communicate that). But maybe people don't want the extra noise.
> 
> I'm on the explicit side. Unless you work a lot with string lists, I
> don't think you can remember whether "all zero" initialization is dup
> or no dup.

Here's the patch for that.

-- >8 --
Subject: use string_list initializer consistently

There are two types of string_lists: those that own the
string memory, and those that don't. You can tell the
difference by the strdup_strings flag, and one should use
either STRING_LIST_INIT_DUP, or STRING_LIST_INIT_NODUP as an
initializer.

Historically, the normal all-zeros initialization has
corresponded to the NODUP case. Many sites use no
initializer at all, and that works as a shorthand for that
case. But for a reader of the code, it can be hard to
remember which is which. Let's be more explicit and actually
have each site declare which type it means to use.

This is a fairly mechanical conversion; I assumed each site
was correct as-is, and just switched them all to NODUP.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/apply.c               | 6 +++---
 builtin/blame.c               | 2 +-
 builtin/clone.c               | 4 ++--
 builtin/log.c                 | 6 +++---
 builtin/remote.c              | 2 +-
 notes.c                       | 2 +-
 submodule.c                   | 2 +-
 t/helper/test-parse-options.c | 2 +-
 8 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index c770d7d..f27520f 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -270,7 +270,7 @@ struct image {
  * the case where more than one patches touch the same file.
  */
 
-static struct string_list fn_table;
+static struct string_list fn_table = STRING_LIST_INIT_NODUP;
 
 static uint32_t hash_line(const char *cp, size_t len)
 {
@@ -1936,7 +1936,7 @@ static void prefix_patch(struct patch *p)
  * include/exclude
  */
 
-static struct string_list limit_by_name;
+static struct string_list limit_by_name = STRING_LIST_INIT_NODUP;
 static int has_include;
 static void add_name_limit(const char *name, int exclude)
 {
@@ -3582,7 +3582,7 @@ static int check_to_create(const char *new_name, int ok_if_exists)
  * it is perfectly fine, as the patch removes a/b to make room
  * to create a directory a/b so that a/b/c can be created.
  */
-static struct string_list symlink_changes;
+static struct string_list symlink_changes = STRING_LIST_INIT_NODUP;
 #define SYMLINK_GOES_AWAY 01
 #define SYMLINK_IN_RESULT 02
 
diff --git a/builtin/blame.c b/builtin/blame.c
index 80d2431..de70153 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -56,7 +56,7 @@ static int show_progress;
 static struct date_mode blame_date_mode = { DATE_ISO8601 };
 static size_t blame_date_width;
 
-static struct string_list mailmap;
+static struct string_list mailmap = STRING_LIST_INIT_NODUP;
 
 #ifndef DEBUG
 #define DEBUG 0
diff --git a/builtin/clone.c b/builtin/clone.c
index 5f867e6..70d8213 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -49,8 +49,8 @@ static char *option_upload_pack = "git-upload-pack";
 static int option_verbosity;
 static int option_progress = -1;
 static enum transport_family family;
-static struct string_list option_config;
-static struct string_list option_reference;
+static struct string_list option_config = STRING_LIST_INIT_NODUP;
+static struct string_list option_reference = STRING_LIST_INIT_NODUP;
 static int option_dissociate;
 static int max_jobs = -1;
 
diff --git a/builtin/log.c b/builtin/log.c
index 099f4f7..8eef94f 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -674,9 +674,9 @@ static int auto_number = 1;
 
 static char *default_attach = NULL;
 
-static struct string_list extra_hdr;
-static struct string_list extra_to;
-static struct string_list extra_cc;
+static struct string_list extra_hdr = STRING_LIST_INIT_NODUP;
+static struct string_list extra_to = STRING_LIST_INIT_NODUP;
+static struct string_list extra_cc = STRING_LIST_INIT_NODUP;
 
 static void add_header(const char *value)
 {
diff --git a/builtin/remote.c b/builtin/remote.c
index d33766b..5ded301 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -247,7 +247,7 @@ struct branch_info {
 	enum { NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE } rebase;
 };
 
-static struct string_list branch_list;
+static struct string_list branch_list = STRING_LIST_INIT_NODUP;
 
 static const char *abbrev_ref(const char *name, const char *prefix)
 {
diff --git a/notes.c b/notes.c
index e4e4854..df4660f 100644
--- a/notes.c
+++ b/notes.c
@@ -70,7 +70,7 @@ struct non_note {
 
 struct notes_tree default_notes_tree;
 
-static struct string_list display_notes_refs;
+static struct string_list display_notes_refs = STRING_LIST_INIT_NODUP;
 static struct notes_tree **display_notes_trees;
 
 static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,
diff --git a/submodule.c b/submodule.c
index 4532b11..abc2ac2 100644
--- a/submodule.c
+++ b/submodule.c
@@ -17,7 +17,7 @@
 
 static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
 static int parallel_jobs = 1;
-static struct string_list changed_submodule_paths;
+static struct string_list changed_submodule_paths = STRING_LIST_INIT_NODUP;
 static int initialized_fetch_ref_tips;
 static struct sha1_array ref_tips_before_fetch;
 static struct sha1_array ref_tips_after_fetch;
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index 8a1235d..2c63298 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -12,7 +12,7 @@ static int dry_run = 0, quiet = 0;
 static char *string = NULL;
 static char *file = NULL;
 static int ambiguous;
-static struct string_list list;
+static struct string_list list = STRING_LIST_INIT_NODUP;
 
 static struct {
 	int called;
-- 
2.9.0.rc2.149.gd580ccd

Re: [PATCH 4/3] use string_list initializer consistently

[Duy Nguyen]

On Mon, Jun 13, 2016 at 5:04 PM, Jeff King <peff@peff.net> wrote:
> -- >8 --
> Subject: use string_list initializer consistently
>
> There are two types of string_lists: those that own the
> string memory, and those that don't. You can tell the
> difference by the strdup_strings flag, and one should use
> either STRING_LIST_INIT_DUP, or STRING_LIST_INIT_NODUP as an
> initializer.
>
> Historically, the normal all-zeros initialization has
> corresponded to the NODUP case. Many sites use no
> initializer at all, and that works as a shorthand for that
> case. But for a reader of the code, it can be hard to
> remember which is which. Let's be more explicit and actually
> have each site declare which type it means to use.
>
> This is a fairly mechanical conversion; I assumed each site
> was correct as-is, and just switched them all to NODUP.

Looking good. If we still want to reduce noise level down (by a tiny
bit), we could remove _INIT because I think it's obvious from

struct string_list var = STRING_LIST_NODUP;

that's it's initialization (I wish we could write "auto var =
STRING_LIST_NODUP;")
-- 
Duy

Re: [PATCH 4/3] use string_list initializer consistently

[Jeff King]

On Mon, Jun 13, 2016 at 06:31:29PM +0700, Duy Nguyen wrote:

> > This is a fairly mechanical conversion; I assumed each site
> > was correct as-is, and just switched them all to NODUP.
> 
> Looking good. If we still want to reduce noise level down (by a tiny
> bit), we could remove _INIT because I think it's obvious from
> 
> struct string_list var = STRING_LIST_NODUP;
> 
> that's it's initialization (I wish we could write "auto var =
> STRING_LIST_NODUP;")

Yeah, I've always felt it's a bit long. The "INIT" does match other
similar macros, but I agree it could probably go. Definitely something
for a separate patch, though.

-Peff