Re: [PATCH] config: support values longer than 1024 bytes

[Jeff King <peff@peff.net>]

On Wed, Apr 06, 2011 at 01:30:03AM +0200, Erik Faye-Lund wrote:

> The rather awkward return statement with strdup("") is because
> strbuf_detach returns NULL when there's nothing allocated. Even
> worse, it returns an uninitialized string if the string has been
> initialized with a non-zero 'hint'.

That seems like two bugs in the strbuf code.

I would expect strbuf_detach to _always_ return an allocated buffer,
even if it is xstrdup(""). Though the code in strbuf_detach is explicit
about returning NULL rather than slopbuf, so perhaps there is a case
where it is useful.

But for the other, one of the invariants of strbuf is that the string is
always NUL-terminated. So I would expect strbuf_init to properly
NUL-terminate after growing based on the hint.

>  static char *parse_value(void)
>  {
> -	static char value[1024];
> -	int quote = 0, comment = 0, len = 0, space = 0;
> +	struct strbuf value = STRBUF_INIT;
> +	int quote = 0, comment = 0, space = 0;

One thing you could about the strbuf_detach thing is to just use a
single static strbuf instead of the static array, and just reset its
length back to zero on each call. So:

  static struct strbuf value = STRBUF_INIT;
  ...
  strbuf_reset(&value);

> -			value[len] = 0;
> -			return value;
> +			return value.len ?
> +			    strbuf_detach(&value, NULL) :
> +			    strdup("");

Then this just becomes:

  return value.buf;

>  			default:
> +				strbuf_release(&value);
>  				return NULL;

And you can drop the release.

> @@ -226,7 +226,9 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
>  		if (!value)
>  			return -1;
>  	}
> -	return fn(name, value, data);
> +	ret = fn(name, value, data);
> +	free(value);
> +	return ret;

And drop this hunk, since callers no longer need to free.

I do wonder, though, if we could be reusing the unquote_c_style()
function in quote.c. They are obviously similar, but I haven't checked
if there is more going on in the config code.

-Peff