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

["Rubén Justo" <rjusto@gmail.com>]

On Mon, Apr 08, 2024 at 04:55:11PM -0400, Jeff King wrote:
> 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

Hi Peff.

Thanks for your ideas.

For the globals we have in environment.h, maybe we can keep them const
and avoid the other inconveniences, doing something like:

diff --git a/config.c b/config.c
index 146856567a..ead3565c27 100644
--- a/config.c
+++ b/config.c
@@ -1671,8 +1671,13 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
                return 0;
        }

-       if (!strcmp(var, "core.editor"))
-               return git_config_string(&editor_program, var, value);
+       if (!strcmp(var, "core.editor")) {
+               static struct strbuf editor_program_ = STRBUF_INIT;
+               if (git_config_strbuf(&editor_program_, var, value))
+                       return -1;
+               editor_program = editor_program_.buf;
+               return 0;
+       }

        if (!strcmp(var, "core.commentchar")) {
                if (!value)