From: Toon Claes <toon@iotcl.com>
Date: Thu, 11 Dec 2025 14:16:36 GMT
Subject: Re: [PATCH v2 1/2] builtin/gc: fix condition for whether to write commit graphs
Message-ID: <87ecp1gl2z.fsf@iotcl.com>
In-Reply-To: <20251211-odb-related-fixes-v2-1-bdf875ce51fc@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> The root cause of this memory leak is our use of `commit_list_append()`.
> This function expects as parameters the item to append and the _tail_ of
> the list to append. This tail will then be overwritten with the new tail
> of the list so that it can be used in subsequent calls. But we call it
> with `commit_list_append(parent->item, &stack)`, so we end up losing
> everything but the new item.
>
> This issue only surfaces when counting merge commits. Next to being a
> memory leak, it also shows that we're in fact miscounting as we only
> respect children of the last parent. All previous parents are discarded,
> so their children will be disregarded unless they are hit via another
> reference.
>
> While crafting a test case for the issue I was puzzled that I couldn't
> establish the proper border at which the auto-condition would be
> fulfilled. As it turns out, there's another bug: if an object is at the
> tip of any reference we don't mark it as seen. Consequently, if it is
> reachable via any other reference, we'd count that object twice.
>
> Fix both of these bugs so that we properly count objects without leaking
> any memory.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/gc.c           |  8 +++++---
>  t/t7900-maintenance.sh | 25 +++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 92c6e7b954..17ff68cbd9 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1130,8 +1130,10 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)
>  		return 0;
>  
>  	commit = lookup_commit(the_repository, maybe_peeled);
> -	if (!commit)
> +	if (!commit || commit->object.flags & SEEN)
>  		return 0;
> +	commit->object.flags |= SEEN;
> +
>  	if (repo_parse_commit(the_repository, commit) ||
>  	    commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)
>  		return 0;
> @@ -1141,7 +1143,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)
>  	if (data->num_not_in_graph >= data->limit)
>  		return 1;
>  
> -	commit_list_append(commit, &stack);
> +	commit_list_insert(commit, &stack);

commit_list_insert() prepends the commit to the beginning of the list,
while commit_list_append() appends it at the end. Because the list is
only used for counting, we don't care about the order. So this fix looks
good to me.

I also approve the other changes in this series.

-- 
Cheers,
Toon