On Mon, Aug 12, 2024 at 01:32:53PM -0700, Junio C Hamano wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

On Thu, Aug 08, 2024 at 10:12:26AM -0700, Junio C Hamano wrote:

quoted

Patrick Steinhardt [off-list ref] writes:

quoted

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.

Then let's mark it as #leftoverbits to document it.  Many other code
paths depend on it.

Okay.

quoted

quoted

I still think the approach taken by the previous iteration was
simpler and much less error prone, though.

I personally prefer this iteration.

If so, then let's fully take advantage of the fact that you have a
to-free variable dedicated for the comment_line_str variable.

Can do.

I still think it is a maintenance burden to keep them always in sync
(which is another thing the developers have to remember---when they
are updating _this_ particular variable, an extra rule applies and
they need to take care of this _allocated thing associated with it),
and the first approach, by not forcing all the other assignment code
paths to worry about it, simplifies the mental model for developers
greatly (i.e. we know we do not own the initial value, but
everything else we allocate thus we free everything but the initial
value), in exchange for a slightly wasteful allocation.

The approach in the second patch is worse in two counds compared to
the original.  It does wasteful allocation (which we do not have
to---the fix was shown above).  It also burdens the developers to
know that they have to manually manage the _allocated half of the
two-variable pair.

Well, the developer has to manage the allocation in both versions of
this patch series. In the first iteration it just wasn't as bad because
I didn't bother to adjust all sites where we set up the comment string.
So I was just about to convert it back to the first iteration, but then
again saw that we now have to carry this ugly construct everywhere:

    if (comment_line_str != comment_line_str_default)
        free((char *) comment_line_str);
    comme_line_str = xstrdup(value);

vs

    free(comment_line_str_to_free);
    comment_line_str = comment_line_str_to_free = xstrdup(value);

I certainly think that the maintenance headache of the first version is
higher than having to maintain both variables. The worst that can happen
for the second version is that we leak memory because we don't update
the `_to_free` string. The worst that can happen in the above code
snippet is that one isn't aware of the condition of when the string
needs to be freed and then unconditionally frees it, leading to a
segfault.

But as said, ultimately I think neither of these versions is great.

Patrick