Re: [PATCH 4/7] grep: read submodule entry with explicit repo
- From
Emily Shaffer <emilyshaffer@google.com>
- Date
- Aug 11, 2021, 21:44 UTC
- Message-ID
- <YRREyrAXBmxVoxRr@google.com>
- In-Reply-To
- <30ead880b38c5f572e609554b075ef81ff80ad87.1628618950.git.jonathantanmy@google.com>
On Tue, Aug 10, 2021 at 11:28:42AM -0700, Jonathan Tan wrote:
Show 6 quoted lines
> > 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 ;)
Show 24 quoted lines
>
> 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.
Show 12 quoted lines
> &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.
Show 6 quoted lines
> strbuf_release(&base);
> free(data);
> } else {
> --
> 2.33.0.rc1.237.g0d66db33f3-goog
> LGTM. Thanks.
Reviewed-by: Emily Shaffer <emilyshaffer@google.com>