From: Jonathan Tan Date: Thu, 10 May 2018 17:16:15 GMT Subject: Re: [PATCH v4 00/13] object store: alloc Message-ID: <20180510101615.931eb0d219eac4c84cf8140d@google.com> In-Reply-To: <20180510004024.93974-1-sbeller@google.com> 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.