On Thu, Aug 08, 2024 at 10:12:26AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@xxxxxx> writes: > > > diff --git a/config.c b/config.c > > index 6421894614..cb78b652ee 100644 > > --- a/config.c > > +++ b/config.c > > @@ -1596,7 +1596,9 @@ static int git_default_core_config(const char *var, const char *value, > > else if (value[0]) { > > if (strchr(value, '\n')) > > return error(_("%s cannot contain newline"), var); > > - comment_line_str = xstrdup(value); > > + free(comment_line_str_allocated); > > + comment_line_str = comment_line_str_allocated = > > + xstrdup(value); > > If you are to follow the _to_free pattern, you do not have to > allocate here, no? We borrow the value in the configset and point > at it via comment_line_str, and clear comment_line_str_to_free > because there is nothing to free now. I.e. > > comment_line_str = value; > FREE_AND_NULL(comment_line_str_allocated); Only if it is guaranteed that the configuration will never be re-read, which would end up discarding memory owned by the old string. Which should be the case already, but to the best of my knowledge we do not document the expected lifetime of config strings anywhere. > I still think the approach taken by the previous iteration was > simpler and much less error prone, though. I personally prefer this iteration. I feel that it is way more discoverable to have an explicit indicator that something needs to be freed, which the `_allocated` suffix brings us. With the old version, the caller needs to become aware that the constant string may sometimes need to be freed, and that sometimes is figured out by comparing to a magic variable, which feels worse to me. Ultimately, both solutions are okay-ish, but I don't consider either of them to be great. As mentioned elsewhere, I think the best solution would be to adapt the `struct strbuf` interface to have an initializer like `STRBUF_INIT_CONST("foobar")` that allows us to initialize it with a string constant. There wouldn't be any need to have two variables anymore, and the `strbuf` API would handle the lifecycle of its contents for us. In any case, I'd say this is a #leftoverbit and is better done in a subsequent patch series. I don't really think it makes sense to reroll this version to swap out the patch for the first version again, but am happy to adapt if you prefer that. Thanks! Patrick