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

> 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