patch, 2 partsobject.c: free replace map in raw_object_store_clear

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 <sbeller@google.com>
---
 object.c | 1 +
 1 file changed, 1 insertion(+)

This was missed in 5982da9d2ce (replace-object: allow prepare_replace_object to handle arbitrary repositories, 2018-04-11)

Technically the code works correctly as the replace_map is the same size in different repositories, however it is hard to read. So convert the code to the familiar pattern of dereferencing the pointer that we assign in the sizeof itself.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 replace_object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Stefan Beller <sbeller@google.com> writes:

;-)

We say

	ptr = xmalloc(sizeof(*ptr))

is better because 

	ptr = xmalloc(sizeof(typeof(*ptr)))

is easy to go stale unless we actually use typeof and instead say a concrete type like "struct oidmap".

This one was doing

	ptr = xmalloc(sizeof(*another_ptr))

and it was OK because ptr and another_ptr happened to be of the same type. I wonder if we are making it safer, or making it more obscure to seasoned C programmers, if we introduced a pair of helper macros, perhaps like these:

	#define ALLOCATE(ptr) (ptr) = xmalloc(sizeof(*(ptr)))
	#define CALLOCATE(ptr,cnt) (ptr) = xcalloc((cnt), sizeof(*(ptr)))

The change looks obviously good.  Will queue.

Thanks.

On Thu, May 10, 2018 at 07:23:13PM +0900, Junio C Hamano wrote:

I've often wondered that, too. It's the natural endgame of the ALLOC_ARRAY() road we've been going down.

-Peff

On 5/10/2018 7:56 AM, Jeff King wrote:

I'll chime in that I like this idea.

Because I'm trying to learn more about Coccinelle, I added the diff below and ran 'make coccicheck'. It resulted in a 1000-line patch on 'next'. I'll refrain from sending that patch series, but it's nice to know a "simple" transformation is possible.

Using `git grep` I see 230 instances of 'xmalloc' and 261 instances of 'xcalloc'. After the Coccinelle transformation, these are down to 194 and 190, respectively, because the rest allocate in the same line as the definition. It's worth thinking about the macro pattern for those cases.

Thanks for reporting the coccinelle experiment!

As we follow a strict declare before code, and we do not know if further declarations make use of this already, e.g. given

    struct foo *f = xmalloc(sizeof(*f));
    struct bar b = &f->baz;

we cannot split up the line declaring and assigning f, but the macro has to recreate the assignment upon declaration, for that we'd need to have something like

    ALLOCATE_TYPE(type, name);

which over complicates things IMHO.

Maybe it is worth identifying the pattern where 'f' is not used in further declarations, such that we can make patches as

-    struct foo *f = xmalloc(sizeof(*f));
+   struct foo *f;
    struct baz b = &unrelated;
+
+ ALLOCATE(f);
+

Thanks, Stefan

Stefan Beller <sbeller@google.com> writes:

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.

Hi Junio,

On Thu, May 10, 2018 at 3:16 AM, Junio C Hamano <gitster@pobox.com> wrote:

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.

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

Thanks, Stefan