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)
