Re: [PATCH v3 1/2] Factor find_pack_entry()'s core out

[Nicolas Pitre <nico@fluxnic.net>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2950 bytes --]

On Wed, 1 Feb 2012, Nguyễn Thái Ngọc Duy wrote:

> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  sha1_file.c |   59 +++++++++++++++++++++++++++++++++--------------------------
>  1 files changed, 33 insertions(+), 26 deletions(-)
> 
> diff --git a/sha1_file.c b/sha1_file.c
> index 88f2151..ff5bf42 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2010,11 +2010,42 @@ int is_pack_valid(struct packed_git *p)
>  	return !open_packed_git(p);
>  }
>  
> +static int find_pack_entry_1(const unsigned char *sha1,
> +			     struct packed_git *p, struct pack_entry *e)

This looks all goot but the name.  Pretty please, try to find something 
that is more descriptive than "1".  Suggestions: 
"find_pack_entry_lookup", "find_pack_entry_inner", etc.

With that fixed, you can add:

Acked-by: Nicolas Pitre <nico@fluxnic.net>

> +{
> +	off_t offset;
> +	if (p->num_bad_objects) {
> +		unsigned i;
> +		for (i = 0; i < p->num_bad_objects; i++)
> +			if (!hashcmp(sha1, p->bad_object_sha1 + 20 * i))
> +				return 0;
> +	}
> +
> +	offset = find_pack_entry_one(sha1, p);
> +	if (!offset)
> +		return 0;
> +
> +	/*
> +	 * We are about to tell the caller where they can locate the
> +	 * requested object.  We better make sure the packfile is
> +	 * still here and can be accessed before supplying that
> +	 * answer, as it may have been deleted since the index was
> +	 * loaded!
> +	 */
> +	if (!is_pack_valid(p)) {
> +		warning("packfile %s cannot be accessed", p->pack_name);
> +		return 0;
> +	}
> +	e->offset = offset;
> +	e->p = p;
> +	hashcpy(e->sha1, sha1);
> +	return 1;
> +}
> +
>  static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
>  {
>  	static struct packed_git *last_found = (void *)1;
>  	struct packed_git *p;
> -	off_t offset;
>  
>  	prepare_packed_git();
>  	if (!packed_git)
> @@ -2022,35 +2053,11 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
>  	p = (last_found == (void *)1) ? packed_git : last_found;
>  
>  	do {
> -		if (p->num_bad_objects) {
> -			unsigned i;
> -			for (i = 0; i < p->num_bad_objects; i++)
> -				if (!hashcmp(sha1, p->bad_object_sha1 + 20 * i))
> -					goto next;
> -		}
> -
> -		offset = find_pack_entry_one(sha1, p);
> -		if (offset) {
> -			/*
> -			 * We are about to tell the caller where they can
> -			 * locate the requested object.  We better make
> -			 * sure the packfile is still here and can be
> -			 * accessed before supplying that answer, as
> -			 * it may have been deleted since the index
> -			 * was loaded!
> -			 */
> -			if (!is_pack_valid(p)) {
> -				warning("packfile %s cannot be accessed", p->pack_name);
> -				goto next;
> -			}
> -			e->offset = offset;
> -			e->p = p;
> -			hashcpy(e->sha1, sha1);
> +		if (find_pack_entry_1(sha1, p, e)) {
>  			last_found = p;
>  			return 1;
>  		}
>  
> -		next:
>  		if (p == last_found)
>  			p = packed_git;
>  		else
> -- 
> 1.7.8.36.g69ee2
>