Re: [PATCH] pathspec: rename free_pathspec() to clear_pathspec()

From: Jeff King <peff@peff.net>
Date: Jun 2, 2016, 21:29 UTC
Message-ID: <20160602212938.GA13071@sigill.intra.peff.net>
In-Reply-To: <xmqq60trthl4.fsf@gitster.mtv.corp.google.com>

On Thu, Jun 02, 2016 at 02:18:47PM -0700, Junio C Hamano wrote:

> The function takes a pointer to a pathspec structure, and releases
> the resources held by it, but does not free() the structure itself.
> Such a function should be called "clear", not "free".

Hmm, makes sense, though...

Show 9 quoted lines

>  * This is just something I noticed.  Among the hits in
> 
>     $ git grep free_ \*.h
> 
>    I think free_notes() is also a candidate for such renaming, but
>    because we are not actively working on that subsystem, we may
>    want to leave that dog sleeping to avoid unnecessary code churn.
>    The same for diff_free_filespec_data(), for which a better name
>    would have been diff_filespec_clear().

I think diff_filespec_clear() would not be quite right. It is freeing only the allocated _data_, but leaving the other portions intact. Generally our _clear() functions reset the object back to an initial state, from which it can be reused. I don't see that as a big problem because there is an other object for the verb "free" here: "data". We are just freeing its data, but the rest of the object remains intact and we may fill in the data again later.

But I think pathspec is in similar boat; it has not been cleared back to its initial state. But it is in a much _worse_ state than the filespec, which you can continue to use. It is in a totally broken state where "nr" does not correspond to the actual number of items, the has_wildcard flag is bogus, etc.

So I think it would be OK to move it to "clear", but we should probably also zero the whole thing, too.

-Peff