Re: [PATCH 1/2] [GSOC] ref-filter: add obj-type check in grab contents

From: ZheNing Hu <adlternative@gmail.com>
Date: Jun 3, 2021, 04:52 UTC
Message-ID: <CAOLTT8TzZHsZfLtgS8G=fvE5hQzP-1PBnS8yyj41y2PtZtBh0g@mail.gmail.com>
In-Reply-To: <xmqq1r9j7l60.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> 于2021年6月3日周四 上午10:10写道：

Show 25 quoted lines

>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> >  /* See grab_values */
> > -static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
> > +static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf,
> > +                                struct object *obj)
>
> Neither this step or the next change needs anything but type member
> of the 'obj' (and 'buf' is coming from oi.content of the result of
> asking about that same 'obj').
>
> I wonder if we should do one of the following:
>
>  (1) stop passing "void *buf" and instead "struct expand_data
>      *data", and use "data->content" to access "buf", which would
>      allow you to access "data->type" to perform the added check.
>
>  (2) instead of adding "struct obj *obj" to the parameters, just add
>      "enum object_type type", as that is the only thing you need.
>
> Obviously (2) is with lessor impact, but if it can be done safely
> without breaking the code [*], (1) would probably be a much more
> preferrable direction to go in the longer term.
>

I agree with (1). In future versions of grab_sub_body_contents(), we will use the content of "data" more frequently instead of using the crude "obj". The type provided by "obj" can also be provided by "data". So yes, I would be very willing to let grab_sub_body_contents() only use "data". (delete "obj")

E.g.

static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data)

Using (2), we will need more parameters to pass other object info.

Show 6 quoted lines

>     Side note [*].  A caller is allowed to choose to feed "buf" that
>     is different from "oi.content" (perhaps buf may sometimes want
>     to be a utf-8 recoded version of oi.content for certain types of
>     objects) with the current system, but if we pass expand_data
>     throughout the callchain, such a caller is broken, for example.
>

Just see the situation in front of us: grab_sub_body_contents() have only one caller: grab_values(). If someone need a function like grab_sub_body_contents() to grab another buf, they can use rewrite a more universal function interface:

static void grab_sub_body_contents(struct atom_value *val, int deref,
struct expand_data *data)
{
   grab_sub_body_contents_internal(val, deref, data->content,
data->size, data->type);
}

static void grab_sub_body_contents_internal(struct atom_value *val,
int deref, void *buf,
                                           unsigned long buf_size,
enum object_type type)
{
...
}

But for the time being, the above one is sufficient.

Thanks. -- ZheNing Hu