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

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 17 Jun 2021 09:04:57 +0200

On Sat, Jun 12 2021, ZheNing Hu via GitGitGadget wrote:

> From: ZheNing Hu <[email-masked]>
>
> Only tag and commit objects use `grab_sub_body_contents()` to grab
> object contents in the current codebase.  We want to teach the
> function to also handle blobs and trees to get their raw data,
> without parsing a blob (whose contents looks like a commit or a tag)
> incorrectly as a commit or a tag.
>
> Skip the block of code that is specific to handling commits and tags
> early when the given object is of a wrong type to help later
> addition to handle other types of objects in this function.
>
> Mentored-by: Christian Couder <[email-masked]>
> Mentored-by: Hariom Verma <[email-masked]>
> Helped-by: Junio C Hamano <[email-masked]>
> Signed-off-by: ZheNing Hu <[email-masked]>
> ---
>  ref-filter.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 4db0e40ff4c6..5cee6512fbaf 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1356,11 +1356,12 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size
>  }
>  
>  /* 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, struct expand_data *data)
>  {
>  	int i;
>  	const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;
>  	size_t sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;
> +	void *buf = data->content;
>  
>  	for (i = 0; i < used_atom_cnt; i++) {
>  		struct used_atom *atom = &used_atom[i];
> @@ -1371,10 +1372,13 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
>  			continue;
>  		if (deref)
>  			name++;
> -		if (strcmp(name, "body") &&
> -		    !starts_with(name, "subject") &&
> -		    !starts_with(name, "trailers") &&
> -		    !starts_with(name, "contents"))
> +
> +		if ((data->type != OBJ_TAG &&
> +		     data->type != OBJ_COMMIT) ||
> +		    (strcmp(name, "body") &&
> +		     !starts_with(name, "subject") &&
> +		     !starts_with(name, "trailers") &&
> +		     !starts_with(name, "contents")))

We have 4 "real" object types, commit, tree, blob, tag. Do you really
mean "not tag or commit" here, don't you mean "is tree or blob" instead?
I.e. do we really want to pass OBJ_NONE etc. here?

>  			continue;
>  		if (!subpos)
>  			find_subpos(buf,
> @@ -1438,17 +1442,19 @@ static void fill_missing_values(struct atom_value *val)
>   * pointed at by the ref itself; otherwise it is the object the
>   * ref (which is a tag) refers to.
>   */
> -static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf)
> +static void grab_values(struct atom_value *val, int deref, struct object *obj, struct expand_data *data)
>  {
> +	void *buf = data->content;
> +
>  	switch (obj->type) {
>  	case OBJ_TAG:
>  		grab_tag_values(val, deref, obj);
> -		grab_sub_body_contents(val, deref, buf);
> +		grab_sub_body_contents(val, deref, data);
>  		grab_person("tagger", val, deref, buf);
>  		break;
>  	case OBJ_COMMIT:
>  		grab_commit_values(val, deref, obj);
> -		grab_sub_body_contents(val, deref, buf);
> +		grab_sub_body_contents(val, deref, data);
>  		grab_person("author", val, deref, buf);
>  		grab_person("committer", val, deref, buf);
>  		break;
> @@ -1678,7 +1684,7 @@ static int get_object(struct ref_array_item *ref, int deref, struct object **obj
>  			return strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
>  					       oid_to_hex(&oi->oid), ref->refname);
>  		}
> -		grab_values(ref->value, deref, *obj, oi->content);
> +		grab_values(ref->value, deref, *obj, oi);
>  	}
>  
>  	grab_common_values(ref->value, deref, oi);