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

[Junio C Hamano <gitster@pobox.com>]

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.

>  {
>  	struct pack_window *w_curs = NULL;
> -	unsigned long size;
>  	off_t curpos = obj_offset;
>  	enum object_type type;
> +	struct delta_base_cache_entry *ent;
>  
>  	/*
>  	 * We always get the representation type, but only convert it to
>  	 * a "real" type later if the caller is interested.
>  	 */
> -	if (oi->contentp && !oi->content_limit) {
> -		*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,
> -						      &type);
> +	oi->whence = OI_PACKED;
> +	ent = get_delta_base_cache_entry(p, obj_offset);
> +	if (ent) {
> +		oi->whence = OI_DBCACHED;

OK.  This is very straight-forward.  It is packed but if we grabbed
it from the delta-base-cache, that is the only case we know it is
dbcached.

> +		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?

> +	} else if (oi->contentp && !oi->content_limit) {
> +		*oi->contentp = unpack_entry(r, p, obj_offset, &type,
> +						oi->sizep);
>  		if (!*oi->contentp)
>  			type = OBJ_BAD;

Nice.  The code structure is still easy to follow, even though the
if/else cascade here are organized differently with more cases (used
to be "are we peeking the contents, or not?"---now it is "do this if
we can grab from the delta base cache, do one of these other things
if we have go to the packfile").