Re: [PATCH v1 06/10] packfile: packed_object_info avoids packed_to_object_type

[Eric Wong <e@80x24.org>]

Patrick Steinhardt <ps@pks.im> wrote:
> On Mon, Jul 15, 2024 at 12:35:15AM +0000, Eric Wong wrote:
> > For calls the delta base cache, packed_to_object_type calls
> > can be omitted.  This prepares us to bypass content_limit for
> > non-blob types in the following commit.
> > 
> > Signed-off-by: Eric Wong <e@80x24.org>
> > ---
> >  packfile.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/packfile.c b/packfile.c
> > index b2660e14f9..c2ba6ab203 100644
> > --- a/packfile.c
> > +++ b/packfile.c
> > @@ -1522,7 +1522,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
> >  {
> >  	struct pack_window *w_curs = NULL;
> >  	off_t curpos = obj_offset;
> > -	enum object_type type;
> > +	enum object_type type, final_type = OBJ_BAD;
> >  	struct delta_base_cache_entry *ent;
> 
> I think it might help this patch to move `type` to the scopes where it's
> used to demonstrate that all code paths set `final_type` as expected.

The condition at the end of packed_object_info() requires the original
`type' to keep its top-level scope:

        if (oi->delta_base_oid) {
                if (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {

But yeah, the whole function is huge and remains a bit convoluted.
Inlining cache_or_unpack_entry in 4/10 helped some, I think.

> >  	/*
> > @@ -1534,7 +1534,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
> >  	ent = get_delta_base_cache_entry(p, obj_offset);
> >  	if (ent) {
> >  		oi->whence = OI_DBCACHED;
> > -		type = ent->type;
> > +		final_type = type = ent->type;
> >  		if (oi->sizep)
> >  			*oi->sizep = ent->size;
> >  		if (oi->contentp) {
> > @@ -1552,6 +1552,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
> >  	} else if (oi->contentp && !oi->content_limit) {
> >  		*oi->contentp = unpack_entry(r, p, obj_offset, &type,
> >  						oi->sizep);
> > +		final_type = type;
> >  		if (!*oi->contentp)
> >  			type = OBJ_BAD;
> >  	} else {
> > @@ -1581,6 +1582,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
> >  			if (oi->sizep && *oi->sizep <= oi->content_limit) {
> >  				*oi->contentp = unpack_entry(r, p, obj_offset,
> >  							&type, oi->sizep);
> > +				final_type = type;
> >  				if (!*oi->contentp)
> >  					type = OBJ_BAD;
> >  			} else {
> > @@ -1602,17 +1604,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,
> >  	}
> >  
> >  	if (oi->typep || oi->type_name) {
> > -		enum object_type ptot;
> > -		ptot = packed_to_object_type(r, p, obj_offset,
> > -					     type, &w_curs, curpos);
> > +		if (final_type < 0)
> > +			final_type = packed_to_object_type(r, p, obj_offset,
> > +						     type, &w_curs, curpos);
> 
> So this is the actual change we're interested in, right? Instead of
> unconditionally calling `packed_to_object_type()`, we skip that call in
> case we know that we have already figured out the correct object type.
> 
> Wouldn't it be easier to manage this with a single `type` variable,
> only, and then conditionally call `packed_to_object_type()` only in the
> cases where `type != OBJ_OFS_DELTA && type != OBJ_REF_DELTA`? Not sure
> whether that would be all that useful though given that the function
> already knows to exit without doing anything in case the type is already
> properly resolved. So maybe the next patch will enlighten me.

As I mentioned above, I think the `type' var remains necessary.