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

Commit Message

ZheNing Hu June 15, 2021, 1:29 p.m. UTC

From: ZheNing Hu <[e-mail]>

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.

Mentored-by: Christian Couder <[e-mail]>
Mentored-by: Hariom Verma <[e-mail]>
Signed-off-by: ZheNing Hu <[e-mail]>
---
 ref-filter.c | 11 ++++++++---
 ref-filter.h |  2 ++
 2 files changed, 10 insertions(+), 3 deletions(-)

Comments

Junio C Hamano June 16, 2021, 7:49 a.m. UTC | #1

"ZheNing Hu via GitGitGadget" <[e-mail]> writes:

> From: ZheNing Hu <[e-mail]>
>
> 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.

ZheNing Hu June 17, 2021, 8:03 a.m. UTC | #2

Junio C Hamano <[e-mail]> 于2021年6月16日周三 下午3:49写道：
>
> "ZheNing Hu via GitGitGadget" <[e-mail]> writes:
>
> > From: ZheNing Hu <[e-mail]>
> >
> > 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

Patch

diff --git a/ref-filter.c b/ref-filter.c
index 420c0bf9384f..aa2ce634106b 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2282,16 +2282,21 @@  static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	return 0;
 }
 
-/*  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)
 {
-	free((char *)item->symref);
 	if (item->value) {
 		int i;
 		for (i = 0; i < used_atom_cnt; i++)
 			free((char *)item->value[i].s);
 		free(item->value);
 	}
+}
+
+/*  Free memory allocated for a ref_array_item */
+static void free_array_item(struct ref_array_item *item)
+{
+	free((char *)item->symref);
+	free_array_item_internal(item);
 	free(item);
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 9dc07476a584..d4531fef5f91 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -111,6 +111,8 @@  struct ref_format {
 int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int type);
 /*  Clear all memory allocated to ref_array */
 void ref_array_clear(struct ref_array *array);
+/* Free array item's value */
+void free_array_item_internal(struct ref_array_item *item);
 /*  Used to verify if the given format is correct and to parse out the used atoms */
 int verify_ref_format(struct ref_format *format);
 /*  Sort the given ref_array as per the ref_sorting provided */