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

[Stefan Beller]

On Thu, May 10, 2018 at 1:56 PM, Jonathan Tan  wrote:
> On Thu, 10 May 2018 10:32:09 -0700
> Stefan Beller  wrote:
>
>> >  - 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 ?
>
> At this point in the discussion, either is fine.

ok, please express your opinion in form of a patch. ;)

I do not plan on resending this series unless someone comments
something that they themselves do not consider bikesheddy.

Stefan

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

[Jonathan Tan]

On Thu, 10 May 2018 10:32:09 -0700
Stefan Beller  wrote:

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

At this point in the discussion, either is fine.

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

I meant that since we call out to free_tree_buffer (as you said), this
shows that the code knows the internal details of a tree object (in that
it has a buffer, and that needs to be freed, and that is the only thing
that needs to be freed), so maybe the code should operate on the
internal details of commits and tags as well. But again, this is a minor
point.

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

[Stefan Beller]

On Thu, May 10, 2018 at 10:16 AM, Jonathan Tan  wrote:
> On Wed,  9 May 2018 17:40:11 -0700
> Stefan Beller  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.

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

[Jonathan Tan]

On Wed,  9 May 2018 17:40:11 -0700
Stefan Beller  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.