patchfind_pack_entry(): do not keep packed_git pointer locally

> Commit f7c22cc (always start looking up objects in the last used pack
> first - 2007-05-30) introduces 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>
> ---

> diff --git a/sha1_file.c b/sha1_file.c
> index 88f2151..4ecc953 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -54,6 +54,8 @@ static struct cached_object empty_tree = {
>  	0
>  };
>  
> +static struct packed_git *find_pack_entry_last_found = (void *)1;
> +
>  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 (find_pack_entry_last_found == p)
> +				find_pack_entry_last_found = (void*)1;
>  			free(p);
>  			return;
>  		}
> @@ -2012,14 +2016,13 @@ int is_pack_valid(struct packed_git *p)
>  
>  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)
>  		return 0;
> -	p = (last_found == (void *)1) ? packed_git : last_found;
> +	p = (find_pack_entry_last_found == (void *)1) ? packed_git : find_pack_entry_last_found;
>  
>  	do {
>  		if (p->num_bad_objects) {
> @@ -2046,16 +2049,16 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
>  			e->offset = offset;
>  			e->p = p;
>  			hashcpy(e->sha1, sha1);
> -			last_found = p;
> +			find_pack_entry_last_found = p;
>  			return 1;
>  		}
>  
>  		next:
> -		if (p == last_found)
> +		if (p == find_pack_entry_last_found)
>  			p = packed_git;
>  		else
>  			p = p->next;
> -		if (p == last_found)
> +		if (p == find_pack_entry_last_found)
>  			p = p->next;
>  	} while (p);
>  	return 0;

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Commit f7c22cc (always start looking up objects in the last used pack
>> first - 2007-05-30) introduces 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>
>> ---
>
> Thanks. Two curiosities:
>
> - Why is there no hunk to actually clear the pointer in
>  free_pack_by_name() in this patch?

> @@ -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 (find_pack_entry_last_found == p)
> +                             find_pack_entry_last_found = (void*)1;
>                       free(p);
>                       return;
>               }

>> diff --git a/sha1_file.c b/sha1_file.c
>> index 88f2151..4ecc953 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -54,6 +54,8 @@ static struct cached_object empty_tree = {
>>       0
>>  };
>>
>> +static struct packed_git *find_pack_entry_last_found = (void *)1;
>> +
>>  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 (find_pack_entry_last_found == p)
>> +                             find_pack_entry_last_found = (void*)1;
>>                       free(p);
>>                       return;
>>               }
>> @@ -2012,14 +2016,13 @@ int is_pack_valid(struct packed_git *p)
>>
>>  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)
>>               return 0;
>> -     p = (last_found == (void *)1) ? packed_git : last_found;
>> +     p = (find_pack_entry_last_found == (void *)1) ? packed_git : find_pack_entry_last_found;
>>
>>       do {
>>               if (p->num_bad_objects) {
>> @@ -2046,16 +2049,16 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
>>                       e->offset = offset;
>>                       e->p = p;
>>                       hashcpy(e->sha1, sha1);
>> -                     last_found = p;
>> +                     find_pack_entry_last_found = p;
>>                       return 1;
>>               }
>>
>>               next:
>> -             if (p == last_found)
>> +             if (p == find_pack_entry_last_found)
>>                       p = packed_git;
>>               else
>>                       p = p->next;
>> -             if (p == last_found)
>> +             if (p == find_pack_entry_last_found)
>>                       p = p->next;
>>       } while (p);
>>       return 0;

> (Pulling Nico in for Q2 below. No snipping so he has a context)
> 
> 2012/1/31 Junio C Hamano <gitster@pobox.com>:
> > Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> >
> >> Commit f7c22cc (always start looking up objects in the last used pack
> >> first - 2007-05-30) introduces 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>
> >> ---

>  Changing INVALID_PACK to NULL breaks at least t1050.4. We may
>  restructure the p assignments at the end of find_pack_entry() to
>  allow setting INVALID_PACK to NULL.
>  
>  But I'm not really excited about that.

> -		if (p == last_found)
> +		if (p == find_pack_entry_last_found)
>  			p = packed_git;
>  		else
>  			p = p->next;
> -		if (p == last_found)
> +		/*
> +		 * p can be NULL from the else clause above, if initial
> +		 * f_p_e_last_found value (i.e. INVALID_PACK) is NULL, we may
> +		 * advance p again to an imaginary pack in invalid memory
> +		 */
> +		if (p == find_pack_entry_last_found)
>  			p = p->next;

> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> 
> > +		/*
> > +		 * p can be NULL from the else clause above, if initial
> > +		 * f_p_e_last_found value (i.e. INVALID_PACK) is NULL, we may
> > +		 * advance p again to an imaginary pack in invalid memory
> > +		 */

> The conversion in f7c22cc (always start looking up objects in the last 
> used pack first, 2007-05-30) wanted to turn the traversal that always 
> went from the tip of a linked list to instead first probe the 
> promising one, and then scan the list from the tip like it used to do, 
> except that it did not want to probe the one it thought promising 
> again.

> So perhaps restructuring the loop by making the logic to probe into a
> single pack into a helper function, e.g.
> 
> static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
> {
> 	if (last_found) {
> 		if (find_one(sha1, e, last_found))
> 			return 1;
> 	}
>         for (p = packed_git; p; p = p->next) {
>         	if (p == last_found || !find_one(sha1, e, p))
> 			continue;
> 		last_found = p;
> 		return 1;
> 	}
>         return 0;
> }
> 
> would make the resulting flow far easier to follow, no?

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

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

> 
> 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)

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

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

> Nicolas Pitre <nico@fluxnic.net> writes:
>
>>> +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.
>
> Perhaps "find_pack_entry_in_pack(sha1, e, p)"?
> That would go well with the caller "find_pack_entry(sha1, e)".

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Nicolas Pitre <nico@fluxnic.net> writes:
> >
> >>> +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.
> >
> > Perhaps "find_pack_entry_in_pack(sha1, e, p)"?
> > That would go well with the caller "find_pack_entry(sha1, e)".
> 
> I amended 1/2 (and adjusted 2/2 to match) to call it fill_pack_entry.
> Will push out the result tonight.