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