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

Junio C Hamano [off-list ref] 于2021年6月3日周四 上午10:10写道：

"ZheNing Hu via GitGitGadget" [off-list ref] writes:

quoted

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

    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