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

Subsystems: the rest

STALE3737d

5 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:52:54

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 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

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:52:54

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

From: Nicolas Pitre <nico@fluxnic.net>
Date: 2016-06-15 22:52:54

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

quoted hunk

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 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>

quoted hunk

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

From: Nicolas Pitre <nico@fluxnic.net>
Date: 2016-06-15 22:52:54

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

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.

quoted hunk

 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

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

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:52:55

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help