Re: [PATCH v2 13/13] alloc: allow arbitrary repositories for alloc functions

[Duy Nguyen <pclouds@gmail.com>]

On Tue, May 8, 2018 at 12:59 AM, Stefan Beller <sbeller@google.com> wrote:
> @@ -501,9 +509,31 @@ void raw_object_store_clear(struct raw_object_store *o)
>  void parsed_object_pool_clear(struct parsed_object_pool *o)
>  {
>         /*
> -        * TOOD free objects in o->obj_hash.
> -        *
>          * As objects are allocated in slabs (see alloc.c), we do
>          * not need to free each object, but each slab instead.
> +        *
> +        * Before doing so, we need to free any additional memory
> +        * the objects may hold.
>          */
> +       unsigned i;
> +
> +       for (i = 0; i < o->obj_hash_size; i++) {
> +               struct object *obj = o->obj_hash[i];
> +
> +               if (!obj)
> +                       continue;
> +
> +               if (obj->type == OBJ_TREE) {
> +                       free(((struct tree*)obj)->buffer);

It would be nicer to keep this in separate functions, e.g.
release_tree_node() and release_commit_node() to go with
alloc_xxx_node().

> +               } else if (obj->type == OBJ_COMMIT) {
> +                       free_commit_list(((struct commit*)obj)->parents);
> +                       free(&((struct commit*)obj)->util);
> +               }
> +       }

I still don't see who frees obj_hash[] (or at least clears it if not
freed). If I'm going to use this to free memory in pack-objects then
I'd really prefer obj_hash[] freed because it's a big _big_ array.

Just to be clear, what I mean is

FREE_AND_NULL(o->obj_hash);
o->obj_hash_size = 0;

> +
> +       clear_alloc_state(o->blob_state);
> +       clear_alloc_state(o->tree_state);
> +       clear_alloc_state(o->commit_state);
> +       clear_alloc_state(o->tag_state);
> +       clear_alloc_state(o->object_state);
>  }
-- 
Duy