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

From: Jeff King <peff@peff.net>
Date: Apr 8, 2024, 20:55 UTC
Message-ID: <20240408205511.GB1639295@coredump.intra.peff.net>
In-Reply-To: <ef8e5f43-5f27-4f0a-acf5-cf4f8281a8f8@gmail.com>

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

Show 24 quoted lines

> 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