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

[Junio C Hamano <gitster@pobox.com>]

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;