Re: [PATCH 0/12] git_config_string() considered harmful

[Jeff King <peff@peff.net>]

On Sun, Apr 07, 2024 at 07:58:02PM +0200, Rubén Justo wrote:

> After Junio's series and yours, I'm on the fence now, but my envision was
> to introduce:
> 
> --- >8 ---
> diff --git a/config.c b/config.c
> index eebce8c7e0..7322bdfb94 100644
> --- a/config.c
> +++ b/config.c
> @@ -1345,6 +1345,15 @@ int git_config_string(const char **dest, const char *var, const char *value)
>  	return 0;
>  }
>  
> +int git_config_strbuf(struct strbuf *dest, const char *var, const char *value)
> +{
> +	if (!value)
> +		return config_error_nonbool(var);
> +	strbuf_reset(dest);
> +	strbuf_addstr(dest, value);
> +	return 0;
> +}
> +
>  int git_config_pathname(const char **dest, const char *var, const char *value)
>  {
>  	if (!value)

Hmm. I think that is nice in some ways, because it is a much bigger
signal about memory ownership than just dropping "const" from the
pointer.

But it is less nice in other ways. Every _user_ of the value now needs
to care that it is a strbuf, and use foo.buf consistently (which you
obviously noticed). Likewise, any downstream writers of the variable
need to treat it like a strbuf, too. So the parse-options OPT_FILENAME()
macro, for example, needs to be replaced with a strbuf-aware variant
(though arguably that is an improvement, as using the wrong one would
fail catastrophically, whereas using a non-const pointer with
OPT_FILENAME() creates a subtle bug).

I'm also not sure what the solution is for setting default values, like:

  const char *my_config = "default";

Of course that is a problem with my solution, too. Perhaps in the long
run we need to accept that they should always default to NULL, and
readers of the variable need to fill in the default when they look at it
(possibly with an accessor function).

Or I guess the alternative is to stop using bare pointers, and carry a
bit which tells us if something is heap-allocated. Which starts to look
an awful lot like a strbuf. ;)

I think in the past we've talked about being able to initialize a strbuf
like:

  struct strbuf foo = STRBUF_INIT_VAL("bar");

That would use "bar" instead of the usual slopbuf, but we can still tell
it's not a heap buffer by the fact that foo.alloc is 0.

-Peff