Re: [PATCH v2 6/9] [GSOC] ref-filter: introduce free_array_item_internal() function

From: ZheNing Hu <adlternative@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com>,
	Git List <git@vger.kernel.org>,
	Christian Couder <christian.couder@gmail.com>,
	Hariom Verma <hariom18599@gmail.com>,
	Bagas Sanjaya <bagasdotme@gmail.com>, Jeff King <peff@peff.net>
Subject: Re: [PATCH v2 6/9] [GSOC] ref-filter: introduce free_array_item_internal() function
Date: Thu, 17 Jun 2021 16:03:17 +0800	[thread overview]
Message-ID: <CAOLTT8T3Ov80w8LkUOw-vwqVB_WptiNZOCTPh7N5NGZwroAnnA@mail.gmail.com> (raw)
In-Reply-To: <xmqqv96ep7u6.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> 于2021年6月16日周三 下午3:49写道：
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > 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.
>

Well, this is indeed ignored by me.

So free_array_item() --> free_ref_array_item() and free_array_item()
--> free_ref_array_item().

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

Yeah, "internal" is an incorrect naming. The main purpose here is
to only free a ref_array_item's value.

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

Because batch_object_write() use a ref_array_item which is allocated on the
stack, and it's member symref is not used at all. ref_array_item's symref and
itself do not need free. The original free_array_item() will free all
dynamically
allocated content in ref_array_item. It cannot meet our requirements
at this time:
only free the ref_array_item's value.

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

Make sence.

--
ZheNing Hu