Re: [PATCH 5/5] load_ref_decorations(): avoid parsing non-tag objects

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 23, 2021, 02:46 UTC
Message-ID: <YNKgkGkPiMgNubNE@nand.local>
In-Reply-To: <YNILCDz3LpHX7OX0@coredump.intra.peff.net>

On Tue, Jun 22, 2021 at 12:08:40PM -0400, Jeff King wrote:

Show 5 quoted lines

> If the packed-refs file ever learns to store all of the peeled layers,
> then we could switch to it. Or even if it stored a flag to indicate the
> peel was not multi-layer (because most of them aren't), then we could
> use it most of the time and fall back to a manual peel for the rare
> cases.

Yeah, I would be in favor of either of these. Of the two, the latter seems like the simplest thing, but the former provides you all of the information you could hope for.

I suppose that if you are already changing the format of packed-refs, then we might as well do the thing which provides the most information and allows us to optimize *all* cases, not just the vast majority of them.

Of course, that's all way outside of the scope of this patch, which shouldn't have to deal with either of those.

Show 27 quoted lines

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  log-tree.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/log-tree.c b/log-tree.c
> index 7b823786c2..8b700e9c14 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -134,6 +134,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,
>  			      int flags, void *cb_data)
>  {
>  	struct object *obj;
> +	enum object_type objtype;
>  	enum decoration_type type = DECORATION_NONE;
>  	struct decoration_filter *filter = (struct decoration_filter *)cb_data;
>
> @@ -155,9 +156,10 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,
>  		return 0;
>  	}
>
> -	obj = parse_object(the_repository, oid);
> -	if (!obj)
> +	objtype = oid_object_info(the_repository, oid, NULL);
> +	if (type < 0)
>  		return 0;
> +	obj = lookup_object_by_type(the_repository, oid, objtype);

The comments about s/type/obj&/ aside, this looks good to me (and the amended version below looks ready to be picked up, at least in my eyes).

One thing I did want to note which is elided by the limited context is that we *do* parse tags like you say, it's just hidden in the "while (obj->type == OBJ_TAG)" loop below.

So that's doing the right thing, but it wasn't clear from the limited context here that this patch was immediately correct.

Thanks, Taylor