[PATCH] find_pack_entry(): do not keep packed_git pointer locally

[PATCH] find_pack_entry(): do not keep packed_git pointer locally

[Nguyễn Thái Ngọc Duy]

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>
---
 sha1_file.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

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;
-- 
1.7.8.36.g69ee2

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

[Junio C Hamano]

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?

 - Could we make the magic (void *)1 value a #define'd constant? Perhaps
   we could even use NULL for that purpose?

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

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

[Nguyen Thai Ngoc Duy]

(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>
>> ---
>
> Thanks. Two curiosities:
>
> - Why is there no hunk to actually clear the pointer in
>  free_pack_by_name() in this patch?

I think it's there (the patch did work for me when I tried to
integrate repack to pack-objects).

-- 8<--
> @@ -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;
>               }
-- 8< --

>  - Could we make the magic (void *)1 value a #define'd constant? Perhaps
>   we could even use NULL for that purpose?

Q1. Sure.

Q2. No NULL is probably not suitable. I think Nico wanted to express
"we tried to find but found none (i.e. NULL)" too and 1 means "no we
have not tried".

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



-- 
Duy

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

[Nicolas Pitre]

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

On Tue, 31 Jan 2012, Nguyen Thai Ngoc Duy wrote:

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

Hmmm, good point.

> >> 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>
> >> ---
[...]
> >  - Could we make the magic (void *)1 value a #define'd constant? Perhaps
> >   we could even use NULL for that purpose?
> 
> Q1. Sure.

Indeed.  The idea might have been to use a non null value that cannot 
match any pointer...

> Q2. No NULL is probably not suitable. I think Nico wanted to express
> "we tried to find but found none (i.e. NULL)" too and 1 means "no we
> have not tried".

Well, I could imagine I might have thought about something like that.  
However, looking at the latest code in the master branch I can't see 
any way for last_found to ever be assigned a NULL value.  So if the
(void*)1 value might have been useful, it is certainly not anymore.


Nicolas

Re: [PATCH v2] find_pack_entry(): do not keep packed_git pointer locally

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

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

The patch to do so should be a trivial one on top of this anyway
(attached).

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

But I think the real issue is that the original loop is written in an
obscure way.  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?


 sha1_file.c |   16 +++++-----------
 1 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 77512a3..2286789 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -54,9 +54,7 @@ static struct cached_object empty_tree = {
 	0
 };
 
-/* INVALID_PACK cannot be NULL, see comments in find_pack_entry */
-#define INVALID_PACK (struct packed_git *)1
-static struct packed_git *find_pack_entry_last_found = INVALID_PACK;
+static struct packed_git *find_pack_entry_last_found;
 
 static struct cached_object *find_cached_object(const unsigned char *sha1)
 {
@@ -725,7 +723,7 @@ void free_pack_by_name(const char *pack_name)
 			free(p->bad_object_sha1);
 			*pp = p->next;
 			if (find_pack_entry_last_found == p)
-				find_pack_entry_last_found = INVALID_PACK;
+				find_pack_entry_last_found = NULL;
 			free(p);
 			return;
 		}
@@ -2024,7 +2022,7 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
 	prepare_packed_git();
 	if (!packed_git)
 		return 0;
-	p = find_pack_entry_last_found == INVALID_PACK ? packed_git : find_pack_entry_last_found;
+	p = !find_pack_entry_last_found ? packed_git : find_pack_entry_last_found;
 
 	do {
 		if (p->num_bad_objects) {
@@ -2060,12 +2058,8 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
 			p = packed_git;
 		else
 			p = p->next;
-		/*
-		 * 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)
+
+		if (p && p == find_pack_entry_last_found)
 			p = p->next;
 	} while (p);
 	return 0;

Re: [PATCH v2] find_pack_entry(): do not keep packed_git pointer locally

[Nicolas Pitre]

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

On Tue, 31 Jan 2012, Junio C Hamano wrote:

> 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
> > +		 */

Ah!  OK so that's why I initially came up with that (void*)1 value.

> But I think the real issue is that the original loop is written in an
> obscure way.

Can't disagree with that, especially when the original author (myself) 
doesn't see clearly through it anymore.

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

Exact.

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

Indeed.


Nicolas

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

[Nguyễn Thái Ngọc Duy]

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

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

[Nguyễn Thái Ngọc Duy]

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>
---
 Since Junio's already done the hard work. It'd be silly of me not to
 take advantage and credit for free :)

 The new loop looks much better.

 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

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

[Nicolas Pitre]

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

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

[Nicolas Pitre]

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

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

[Junio C Hamano]

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

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

[Junio C Hamano]

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.

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

[Nicolas Pitre]

On Wed, 1 Feb 2012, Junio C Hamano wrote:

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

Looks fine to me.


Nicolas

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

[Nguyễn Thái Ngọc Duy]

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.

Thanks to Junio for code layout suggestions.

Acked-by: Nicolas Pitre <nico@fluxnic.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Credit where credit is due. No code changes from pu.

 sha1_file.c |   27 +++++++++++++--------------
 1 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 61e51ed..6b1b512 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;
 		}
@@ -2046,27 +2050,22 @@ static int fill_pack_entry(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 (fill_pack_entry(sha1, e, p)) {
-			last_found = p;
-			return 1;
-		}
+	if (last_found_pack && fill_pack_entry(sha1, e, last_found_pack))
+		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 || !fill_pack_entry(sha1, e, p))
+			continue;
+
+		last_found_pack = p;
+		return 1;
+	}
 	return 0;
 }
 
-- 
1.7.8.36.g69ee2