Re: [PATCH v2 2/3] Refactor parts of in_delta_base_cache/cache_or_unpack_entry

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 25, 2013, 23:15 UTC
Message-ID: <7vtxnzul42.fsf@alter.siamese.dyndns.org>
In-Reply-To: <987ab8138000d0aaa7d1bb6242cced1344e4d339.1364234154.git.trast@student.ethz.ch>

Thomas Rast <trast@student.ethz.ch> writes:

Show 6 quoted lines

> The delta base cache lookup and test were shared.  Refactor them;
> we'll need both parts again.  Also, we'll use the clearing routine
> later.
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---

Looks like a very straight-forward rewrite.

The only little concern I may have is this cmp_* function tells us "I found it!" by returning true, which is counter-intuitive to the readers of the caller (not the callee).

I think it makes sense to compare delta-base-cache entries only for equality, so eq-delta-base-cache-entry might be a better name for it, perhaps?

Show 72 quoted lines

>  sha1_file.c | 45 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 71877a7..bd054d1 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1756,32 +1756,51 @@ static unsigned long pack_entry_hash(struct packed_git *p, off_t base_offset)
>  	return hash % MAX_DELTA_CACHE;
>  }
>  
> -static int in_delta_base_cache(struct packed_git *p, off_t base_offset)
> +static struct delta_base_cache_entry *
> +get_delta_base_cache_entry(struct packed_git *p, off_t base_offset)
>  {
>  	unsigned long hash = pack_entry_hash(p, base_offset);
> -	struct delta_base_cache_entry *ent = delta_base_cache + hash;
> +	return delta_base_cache + hash;
> +}
> +
> +static int cmp_delta_base_cache_entry(struct delta_base_cache_entry *ent,
> +				      struct packed_git *p, off_t base_offset)
> +{
>  	return (ent->data && ent->p == p && ent->base_offset == base_offset);
>  }
>  
> +static int in_delta_base_cache(struct packed_git *p, off_t base_offset)
> +{
> +	struct delta_base_cache_entry *ent;
> +	ent = get_delta_base_cache_entry(p, base_offset);
> +	return cmp_delta_base_cache_entry(ent, p, base_offset);
> +}
> +
> +static void clear_delta_base_cache_entry(struct delta_base_cache_entry *ent)
> +{
> +	ent->data = NULL;
> +	ent->lru.next->prev = ent->lru.prev;
> +	ent->lru.prev->next = ent->lru.next;
> +	delta_base_cached -= ent->size;
> +}
> +
>  static void *cache_or_unpack_entry(struct packed_git *p, off_t base_offset,
>  	unsigned long *base_size, enum object_type *type, int keep_cache)
>  {
> +	struct delta_base_cache_entry *ent;
>  	void *ret;
> -	unsigned long hash = pack_entry_hash(p, base_offset);
> -	struct delta_base_cache_entry *ent = delta_base_cache + hash;
>  
> -	ret = ent->data;
> -	if (!ret || ent->p != p || ent->base_offset != base_offset)
> +	ent = get_delta_base_cache_entry(p, base_offset);
> +
> +	if (!cmp_delta_base_cache_entry(ent, p, base_offset))
>  		return unpack_entry(p, base_offset, type, base_size);
>  
> -	if (!keep_cache) {
> -		ent->data = NULL;
> -		ent->lru.next->prev = ent->lru.prev;
> -		ent->lru.prev->next = ent->lru.next;
> -		delta_base_cached -= ent->size;
> -	} else {
> +	ret = ent->data;
> +
> +	if (!keep_cache)
> +		clear_delta_base_cache_entry(ent);
> +	else
>  		ret = xmemdupz(ent->data, ent->size);
> -	}
>  	*type = ent->type;
>  	*base_size = ent->size;
>  	return ret;