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

[Stefan Beller]

The replace map for objects was missed to free in the object store in
the conversion of c1274495 ("replace-object: eliminate replace objects
prepared flag", 2018-04-11). We also missed to free the replaced objects
that are put into the replace map in that whole series.

Signed-off-by: Stefan Beller 
---
 object.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/object.c b/object.c
index 66cffaf6e51..97245fdea25 100644
--- a/object.c
+++ b/object.c
@@ -481,6 +481,9 @@ void raw_object_store_clear(struct raw_object_store *o)
FREE_AND_NULL(o->objectdir);
FREE_AND_NULL(o->alternate_db);
 
+   oidmap_free(o->replace_map, 1);
+   FREE_AND_NULL(o->replace_map);
+
free_alt_odbs(o);
o->alt_odb_tail = NULL;
 
-- 
2.17.0.582.gccdcbd54c44.dirty

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

[Stefan Beller]

Hi Junio,

On Thu, May 10, 2018 at 3:16 AM, Junio C Hamano  wrote:
> Stefan Beller  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)
>
>
> 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"?

This is the case. I wondered if I want to point to the exact commit
(which I have trouble pointing to as the whole topic has no memory
leak fixes, Closest would be d88f9fdf8b2 (replace-object: move
replace_map to object store, 2018-04-11))
or rather just point at the series. I did not think of the confusion
that might arise there.

> 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?

Ah, that is the best place to point at. Makes sense.
I'll reroll this,

Thanks,
Stefan

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

[Junio C Hamano]

Stefan Beller  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.

>
> Signed-off-by: Stefan Beller 
> ---
>  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;

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

[Stefan Beller]

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)

Signed-off-by: Stefan Beller 
---
 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;
-- 
2.17.0.255.g8bfb7c0704