Re: [PATCH 4/7] grep: read submodule entry with explicit repo

[Emily Shaffer <emilyshaffer@google.com>]

On Tue, Aug 10, 2021 at 11:28:42AM -0700, Jonathan Tan wrote:
> 
> Replace an existing parse_object_or_die() call (which implicitly works
> on the_repository) with a function call that allows a repository to be
> passed in. There is no such direct equivalent to parse_object_or_die(),
> but we only need the type of the object, so replace with
> oid_object_info().

Always exciting to see less implicit use of the_repository ;)

> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  builtin/grep.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/grep.c b/builtin/grep.c
> index e454335e9d..9e61c7c993 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -457,27 +457,27 @@ static int grep_submodule(struct grep_opt *opt,
>  	subopt.repo = &subrepo;
>  
>  	if (oid) {
> -		struct object *object;
> +		enum object_type object_type;
>  		struct tree_desc tree;
>  		void *data;
>  		unsigned long size;
>  		struct strbuf base = STRBUF_INIT;
>  
>  		obj_read_lock();
> -		object = parse_object_or_die(oid, NULL);
> +		object_type = oid_object_info(&subrepo, oid, NULL);

One thing I wonder is whether we are missing out on some error
conditions we used to get with parse_object_or_die() by using
oid_object_info() instead. Do we need to be more defensive in
investigating 'oid' before calling that helper, now?

>  		obj_read_unlock();
>  		data = read_object_with_reference(&subrepo,
> -						  &object->oid, tree_type,
> +						  oid, tree_type,

And a handful of instances where we were using object->oid instead of
the oid we were already passed. Ok.

>  						  &size, NULL);
>  		if (!data)
> -			die(_("unable to read tree (%s)"), oid_to_hex(&object->oid));
> +			die(_("unable to read tree (%s)"), oid_to_hex(oid));
>  
>  		strbuf_addstr(&base, filename);
>  		strbuf_addch(&base, '/');
>  
>  		init_tree_desc(&tree, data, size);
>  		hit = grep_tree(&subopt, pathspec, &tree, &base, base.len,
> -				object->type == OBJ_COMMIT);
> +				object_type == OBJ_COMMIT);

And finally, using the type we got from oid_object_info instead. Ok.

>  		strbuf_release(&base);
>  		free(data);
>  	} else {
> -- 
> 2.33.0.rc1.237.g0d66db33f3-goog
> 

LGTM. Thanks.

Reviewed-by: Emily Shaffer <emilyshaffer@google.com>