Re: [PATCH v3 2/2] find_pack_entry(): do not keep packed_git pointer locally

[Nicolas Pitre <nico@fluxnic.net>]

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

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

> Commit f7c22cc (always start looking up objects in the last used pack
> first - 2007-05-30) introduce a static packed_git* pointer as an
> optimization.  The kept pointer however may become invalid if
> free_pack_by_name() happens to free that particular pack.
> 
> Current code base does not access packs after calling
> free_pack_by_name() so it should not be a problem. Anyway, move the
> pointer out so that free_pack_by_name() can reset it to avoid running
> into troubles in future.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

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


>  Since Junio's already done the hard work. It'd be silly of me not to
>  take advantage and credit for free :)

Maybe a little "Thanks to Junio for code layout suggestions" in the 
commit message would give him some credit back.

>  The new loop looks much better.

Indeed.

>  sha1_file.c |   27 +++++++++++++--------------
>  1 files changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/sha1_file.c b/sha1_file.c
> index ff5bf42..ebe77b3 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -54,6 +54,8 @@ static struct cached_object empty_tree = {
>  	0
>  };
>  
> +static struct packed_git *last_found_pack;
> +
>  static struct cached_object *find_cached_object(const unsigned char *sha1)
>  {
>  	int i;
> @@ -720,6 +722,8 @@ void free_pack_by_name(const char *pack_name)
>  			close_pack_index(p);
>  			free(p->bad_object_sha1);
>  			*pp = p->next;
> +			if (last_found_pack == p)
> +				last_found_pack = NULL;
>  			free(p);
>  			return;
>  		}
> @@ -2044,27 +2048,22 @@ static int find_pack_entry_1(const unsigned char *sha1,
>  
>  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;
>  
>  	prepare_packed_git();
>  	if (!packed_git)
>  		return 0;
> -	p = (last_found == (void *)1) ? packed_git : last_found;
>  
> -	do {
> -		if (find_pack_entry_1(sha1, p, e)) {
> -			last_found = p;
> -			return 1;
> -		}
> +	if (last_found_pack && find_pack_entry_1(sha1, last_found_pack, e))
> +		return 1;
>  
> -		if (p == last_found)
> -			p = packed_git;
> -		else
> -			p = p->next;
> -		if (p == last_found)
> -			p = p->next;
> -	} while (p);
> +	for (p = packed_git; p; p = p->next) {
> +		if (p == last_found_pack || !find_pack_entry_1(sha1, p, e))
> +			continue;
> +
> +		last_found_pack = p;
> +		return 1;
> +	}
>  	return 0;
>  }
>  
> -- 
> 1.7.8.36.g69ee2
>