"ZheNing Hu via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: ZheNing Hu <adlternative@xxxxxxxxx>
>
> Introduce free_array_item_internal() for freeing ref_array_item value.
> It will be called internally by free_array_item(), and it will help
> `cat-file --batch` free ref_array_item's memory later.
As a file local static function, the horrible name free_array_item()
was tolerable. But before exposing a name like that to the outside
world, think twice if that is specific enough, and it is not. There
are 47 different kinds of "array"s we use in the system, but this
new helper function only works with ref_array_item and not on items
in any other kinds of arrays.
> -/* Free memory allocated for a ref_array_item */
> -static void free_array_item(struct ref_array_item *item)
> +void free_array_item_internal(struct ref_array_item *item)
> {
And "internal" is a horrible name to have as an external name. You
probably can come up with a more appropriate name when you imagine
yourself explaining to somebody who is relatively new to this part
of the codebase what the difference between free_array_item() and
this new helper is, where the difference comes from, why the symref
member (and no other member) is so special, etc.
I _think_ what is special is not the .symref but is the .value
field, IOW, you are trying to come up with an interface to free the
value part of ref_array_item without touching other things. But it
is not helpful at all to readers if you do not explain why you want
to do so. Why is the .value member so special? The ability to
clear only the .value member without touching other members is useful
because ...?
In any case, assuming that you'd establish why the .value member is
so special to deserve an externally callable function, when external
callers do not have to be able to free the item as a whole (i.e.
free_array_item() is still file-scope static), in the proposed log
message in an updated patch, I would imagine that
free_ref_array_item_value()
would be a more suitable name than the _internal thing. When it
happens, you might want to rename the static one to
free_ref_array_item() to match, even if it does not have external
callers.
