Re: [PATCH v4 00/13] object store: alloc

[Jonathan Tan <jonathantanmy@google.com>]

On Wed,  9 May 2018 17:40:11 -0700
Stefan Beller <sbeller@google.com> wrote:

>  		if (obj->type == OBJ_TREE)
> -			release_tree_node((struct tree*)obj);
> +			free_tree_buffer((struct tree*)obj);
>  		else if (obj->type == OBJ_COMMIT)
> -			release_commit_node((struct commit*)obj);
> +			release_commit_memory((struct commit*)obj);
>  		else if (obj->type == OBJ_TAG)
> -			release_tag_node((struct tag*)obj);
> +			free_tag_buffer((struct tag*)obj);

This might seem a bit bikesheddy, but I wouldn't call it
free_tag_buffer(), since what's being freed is not the buffer of the
object itself, but just a string. If you want such a function, I would
just call it release_tag_memory() to match release_commit_memory().

Other than that, all the patches look fine to me.

Some optional comments (this is almost certainly bikeshedding):

 - I would call them release_commit() and release_tag(), to match
   strbuf_release().
 - It might be better to just inline the handling of releasing commit
   and tag memory. This code already knows that, for a tree, it needs to
   free its buffer and only its buffer, so it is not much of a stretch
   to think that it similarly knows the details of commit and tag
   objects too.