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

[Stefan Beller <sbeller@google.com>]

On Thu, May 10, 2018 at 10:16 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
> 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):

Who doesn't love some bikeshedding in late spring?

>
>  - I would call them release_commit() and release_tag(), to match
>    strbuf_release().

Why not commit_release and tag_release to also have the same order
of words as in 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.

We still call out to free_tree_buffer? Not sure I understand.