[PATCH 2/4] Correct comment in prepare_packed_git_one.

[PATCH 2/4] Correct comment in prepare_packed_git_one.

[Shawn O. Pearce]

After staring at the comment and the associated for loop, I
realized the comment was completely bogus.  The section of
code its talking about is trying to avoid duplicate mapping
of the same packfile.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 sha1_file.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index a42f94a..4aef244 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -779,7 +779,7 @@ static void prepare_packed_git_one(char *objdir, int local)
 		if (!has_extension(de->d_name, ".idx"))
 			continue;
 
-		/* we have .idx.  Is it a file we can map? */
+		/* Don't reopen a pack we already have. */
 		strcpy(path + len, de->d_name);
 		for (p = packed_git; p; p = p->next) {
 			if (!memcmp(path, p->pack_name, len + namelen - 4))
-- 
1.5.0.rc3.1.ge4b0e

[PATCH 3/4] Refactor open_packed_git to return an error code.

[Shawn O. Pearce]

Because I want to reuse open_packed_git in a context where I don't
want the process to die if the packfile in question is bogus, I'm
changing its behavior to return error("...") rather than die("...")
when it detects something is wrong with the packfile it was given.

Right now we still must die out of use_pack should open_packed_git
fail, as none of use_pack's callers are prepared to handle a failure
from that function.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 sha1_file.c |   31 ++++++++++++++++---------------
 1 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 4aef244..277319b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -552,7 +552,7 @@ void unuse_pack(struct pack_window **w_cursor)
 	}
 }
 
-static void open_packed_git(struct packed_git *p)
+static int open_packed_git(struct packed_git *p)
 {
 	struct stat st;
 	struct pack_header hdr;
@@ -562,49 +562,50 @@ static void open_packed_git(struct packed_git *p)
 
 	p->pack_fd = open(p->pack_name, O_RDONLY);
 	if (p->pack_fd < 0 || fstat(p->pack_fd, &st))
-		die("packfile %s cannot be opened", p->pack_name);
+		return -1;
 
 	/* If we created the struct before we had the pack we lack size. */
 	if (!p->pack_size) {
 		if (!S_ISREG(st.st_mode))
-			die("packfile %s not a regular file", p->pack_name);
+			return error("packfile %s not a regular file", p->pack_name);
 		p->pack_size = st.st_size;
 	} else if (p->pack_size != st.st_size)
-		die("packfile %s size changed", p->pack_name);
+		return error("packfile %s size changed", p->pack_name);
 
 	/* We leave these file descriptors open with sliding mmap;
 	 * there is no point keeping them open across exec(), though.
 	 */
 	fd_flag = fcntl(p->pack_fd, F_GETFD, 0);
 	if (fd_flag < 0)
-		die("cannot determine file descriptor flags");
+		return error("cannot determine file descriptor flags");
 	fd_flag |= FD_CLOEXEC;
 	if (fcntl(p->pack_fd, F_SETFD, fd_flag) == -1)
-		die("cannot set FD_CLOEXEC");
+		return error("cannot set FD_CLOEXEC");
 
 	/* Verify we recognize this pack file format. */
 	if (read_in_full(p->pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))
-		die("file %s is far too short to be a packfile", p->pack_name);
+		return error("file %s is far too short to be a packfile", p->pack_name);
 	if (hdr.hdr_signature != htonl(PACK_SIGNATURE))
-		die("file %s is not a GIT packfile", p->pack_name);
+		return error("file %s is not a GIT packfile", p->pack_name);
 	if (!pack_version_ok(hdr.hdr_version))
-		die("packfile %s is version %u and not supported"
+		return error("packfile %s is version %u and not supported"
 			" (try upgrading GIT to a newer version)",
 			p->pack_name, ntohl(hdr.hdr_version));
 
 	/* Verify the pack matches its index. */
 	if (num_packed_objects(p) != ntohl(hdr.hdr_entries))
-		die("packfile %s claims to have %u objects"
+		return error("packfile %s claims to have %u objects"
 			" while index size indicates %u objects",
 			p->pack_name, ntohl(hdr.hdr_entries),
 			num_packed_objects(p));
 	if (lseek(p->pack_fd, p->pack_size - sizeof(sha1), SEEK_SET) == -1)
-		die("end of packfile %s is unavailable", p->pack_name);
+		return error("end of packfile %s is unavailable", p->pack_name);
 	if (read_in_full(p->pack_fd, sha1, sizeof(sha1)) != sizeof(sha1))
-		die("packfile %s signature is unavailable", p->pack_name);
+		return error("packfile %s signature is unavailable", p->pack_name);
 	idx_sha1 = ((unsigned char *)p->index_base) + p->index_size - 40;
 	if (hashcmp(sha1, idx_sha1))
-		die("packfile %s does not match index", p->pack_name);
+		return error("packfile %s does not match index", p->pack_name);
+	return 0;
 }
 
 static int in_window(struct pack_window *win, unsigned long offset)
@@ -627,8 +628,8 @@ unsigned char* use_pack(struct packed_git *p,
 {
 	struct pack_window *win = *w_cursor;
 
-	if (p->pack_fd == -1)
-		open_packed_git(p);
+	if (p->pack_fd == -1 && open_packed_git(p))
+		die("packfile %s cannot be accessed", p->pack_name);
 
 	/* Since packfiles end in a hash of their content and its
 	 * pointless to ask for an offset into the middle of that
-- 
1.5.0.rc3.1.ge4b0e

[PATCH 4/4] Don't find objects in packs which aren't available anymore.

[Shawn O. Pearce]

Matthias Lederhofer identified a race condition where a Git reader
process was able to locate an object in a packed_git index, but
was then preempted while a `git repack -a -d` ran and completed.
By the time the reader was able to seek in the packfile to get the
object data, the packfile no longer existed on disk.

In this particular case the reader process did not attempt to
open the packfile before it was deleted, so it did not already
have the pack_fd field popuplated.  With the packfile itself gone,
there was no way for the reader to open it and fetch the data.

I'm fixing the race condition by teaching find_pack_entry to ignore
a packed_git whose packfile is not currently open and which cannot
be opened.  If none of the currently known packs can supply the
object, we will return 0 and the caller will decide the object is
not available.  If this is the first attempt at finding an object,
the caller will reprepare_packed_git and try again.  If it was
the second attempt, the caller will typically return NULL back,
and an error message about a missing object will be reported.

This patch does not address the situation of a reader which is
being starved out by a tight sequence of `git repack -a -d` runs.
In this particular case the reader will try twice, probably fail
both times, and declare the object in question cannot be found.
As it is highly unlikely that a real world `git repack -a -d` can
complete faster than a reader can open a packfile, so I don't think
this is a huge concern.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 sha1_file.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 277319b..37669d6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1405,6 +1405,18 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e, cons
 		}
 		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 (p->pack_fd == -1 && open_packed_git(p)) {
+				error("packfile %s cannot be accessed", p->pack_name);
+				continue;
+			}
 			e->offset = offset;
 			e->p = p;
 			hashcpy(e->sha1, sha1);
-- 
1.5.0.rc3.1.ge4b0e

Re: [PATCH 2/4] Correct comment in prepare_packed_git_one.

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> After staring at the comment and the associated for loop, I
> realized the comment was completely bogus.

Yeah, when the comment was written, the loop after the comment
did not exist.  The comment was talking about calling
add_packed_git() to see if the corresponding pack can be mapped,
and then upon receiving NULL, not linking p to the packed_git
linked list.