Re: [PATCH 1/2] object.c: free replace map in raw_object_store_clear

From: Junio C Hamano <gitster@pobox.com>
Date: May 10, 2018, 10:16 UTC
Message-ID: <xmqqy3grizo7.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180509234059.52156-1-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> The replace map for objects was missed to free in the object store in
> the conversion of 174774cd519 (Merge branch 'sb/object-store-replace',
> 2018-05-08)

I need a bit of clarification wrt the above. The above makes it sound like the merge needed a semantic conflict resolution (e.g. one side turned replace_map into a pointer while the other side added a place where the containing structure is freed and now the merge result needs to free the pointer in the new place that frees the containing structure, but the merge forgot to do so). Is that what is going on?

Or is this just a simple "the topic that ends at 174774cd519^2 had this leak that needs to be fixed by this patch; instead of rerolling this is an incremental, because the topic has already been merged to 'master' and it is too late now"?

Looking at this patch in the context of the side branch (instead of in the merged result) already makes sense to me, so I am guessing it is the latter (i.e. not a botched merge that missed semantic conflicts), in which case the proposed log message is a bit too alarming and points readers in a wrong direction. Shouldn't it point at, say, c1274495 ("replace-object: eliminate replace objects prepared flag", 2018-04-11) that turned the oidmap instance into a pointer in raw_object_store?

Thanks.

Show 18 quoted lines

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  object.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/object.c b/object.c
> index 9d5b10d5a20..ff28f90c5ef 100644
> --- a/object.c
> +++ b/object.c
> @@ -497,6 +497,7 @@ void raw_object_store_clear(struct raw_object_store *o)
>  {
>  	FREE_AND_NULL(o->objectdir);
>  	FREE_AND_NULL(o->alternate_db);
> +	FREE_AND_NULL(o->replace_map);
>  
>  	free_alt_odbs(o);
>  	o->alt_odb_tail = NULL;