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

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

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