Re: [PATCH v2 04/10] packfile: inline cache_or_unpack_entry

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

Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> writes:
> 
> > We need to check delta_base_cache anyways to fill in the
> > `whence' field in `struct object_info'.  Inlining (and getting
> > rid of) cache_or_unpack_entry() makes it easier to only do the
> > hashmap lookup once and avoid a redundant lookup later on.
> >
> > This code reorganization will also make an optimization to
> > use the cache entry directly easier to implement in the next
> > commit.
> 
> "cache entry" -> "cached entry"; we tend to use "cache entry"
> exclusively to mean an entry in the in-core index structure,
> and not the cached objects held in the object layer.

OK, will do for v3 (apologies for the delay, Real Life is awful :<).

> > +		type = ent->type;
> > +		if (oi->sizep)
> > +			*oi->sizep = ent->size;
> > +		if (oi->contentp) {
> > +			if (!oi->content_limit ||
> > +					ent->size <= oi->content_limit)
> > +				*oi->contentp = xmemdupz(ent->data, ent->size);
> > +			else
> > +				*oi->contentp = NULL; /* caller must stream */
> 
> This assignment of NULL is more explicit than the original; is it
> because the original assumed that *(oi->contentp) is initialized to
> NULL if oi->contentp asks us to give the contents?

The original code unconditionally assigned cache_or_unpack_entry() result
to *oi->contentp, and there's a bunch of non-cat-file callers which pass
a non-NULL *oi->contentp and expect packed_object_info() to NULL it.