{"thread":{"id":"6613","subject":"[PATCH 3/4] Refactor open_packed_git to return an error code.","startedAt":"2007-02-01T20:52:27Z","lastAt":"2007-02-02T06:21:09Z","messageCount":4,"participants":["Shawn O. Pearce","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"33280","messageId":"20070201205227.GB19009@spearce.org","threadId":"6613","inReplyTo":"a0b03fc086bb66e2aa2e386dcb4ff97fc837f07f.1170363130.git.spearce@spearce.org","subject":"[PATCH 2/4] Correct comment in prepare_packed_git_one.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-02-01T20:52:27Z","receivedAt":"2007-02-01T20:52:27Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"After staring at the comment and the associated for loop, I\nrealized the comment was completely bogus.  The section of\ncode its talking about is trying to avoid duplicate mapping\nof the same packfile.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n sha1_file.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex a42f94a..4aef244 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -779,7 +779,7 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\tif (!has_extension(de->d_name, \".idx\"))\n \t\t\tcontinue;\n \n-\t\t/* we have .idx.  Is it a file we can map? */\n+\t\t/* Don't reopen a pack we already have. */\n \t\tstrcpy(path + len, de->d_name);\n \t\tfor (p = packed_git; p; p = p->next) {\n \t\t\tif (!memcmp(path, p->pack_name, len + namelen - 4))\n-- \n1.5.0.rc3.1.ge4b0e\n"},{"id":"33279","messageId":"20070201205233.GC19009@spearce.org","threadId":"6613","inReplyTo":"a0b03fc086bb66e2aa2e386dcb4ff97fc837f07f.1170363130.git.spearce@spearce.org","subject":"[PATCH 3/4] Refactor open_packed_git to return an error code.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-02-01T20:52:33Z","receivedAt":"2007-02-01T20:52:33Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Because I want to reuse open_packed_git in a context where I don't\nwant the process to die if the packfile in question is bogus, I'm\nchanging its behavior to return error(\"...\") rather than die(\"...\")\nwhen it detects something is wrong with the packfile it was given.\n\nRight now we still must die out of use_pack should open_packed_git\nfail, as none of use_pack's callers are prepared to handle a failure\nfrom that function.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n sha1_file.c |   31 ++++++++++++++++---------------\n 1 files changed, 16 insertions(+), 15 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4aef244..277319b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -552,7 +552,7 @@ void unuse_pack(struct pack_window **w_cursor)\n \t}\n }\n \n-static void open_packed_git(struct packed_git *p)\n+static int open_packed_git(struct packed_git *p)\n {\n \tstruct stat st;\n \tstruct pack_header hdr;\n@@ -562,49 +562,50 @@ static void open_packed_git(struct packed_git *p)\n \n \tp->pack_fd = open(p->pack_name, O_RDONLY);\n \tif (p->pack_fd < 0 || fstat(p->pack_fd, &st))\n-\t\tdie(\"packfile %s cannot be opened\", p->pack_name);\n+\t\treturn -1;\n \n \t/* If we created the struct before we had the pack we lack size. */\n \tif (!p->pack_size) {\n \t\tif (!S_ISREG(st.st_mode))\n-\t\t\tdie(\"packfile %s not a regular file\", p->pack_name);\n+\t\t\treturn error(\"packfile %s not a regular file\", p->pack_name);\n \t\tp->pack_size = st.st_size;\n \t} else if (p->pack_size != st.st_size)\n-\t\tdie(\"packfile %s size changed\", p->pack_name);\n+\t\treturn error(\"packfile %s size changed\", p->pack_name);\n \n \t/* We leave these file descriptors open with sliding mmap;\n \t * there is no point keeping them open across exec(), though.\n \t */\n \tfd_flag = fcntl(p->pack_fd, F_GETFD, 0);\n \tif (fd_flag < 0)\n-\t\tdie(\"cannot determine file descriptor flags\");\n+\t\treturn error(\"cannot determine file descriptor flags\");\n \tfd_flag |= FD_CLOEXEC;\n \tif (fcntl(p->pack_fd, F_SETFD, fd_flag) == -1)\n-\t\tdie(\"cannot set FD_CLOEXEC\");\n+\t\treturn error(\"cannot set FD_CLOEXEC\");\n \n \t/* Verify we recognize this pack file format. */\n \tif (read_in_full(p->pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))\n-\t\tdie(\"file %s is far too short to be a packfile\", p->pack_name);\n+\t\treturn error(\"file %s is far too short to be a packfile\", p->pack_name);\n \tif (hdr.hdr_signature != htonl(PACK_SIGNATURE))\n-\t\tdie(\"file %s is not a GIT packfile\", p->pack_name);\n+\t\treturn error(\"file %s is not a GIT packfile\", p->pack_name);\n \tif (!pack_version_ok(hdr.hdr_version))\n-\t\tdie(\"packfile %s is version %u and not supported\"\n+\t\treturn error(\"packfile %s is version %u and not supported\"\n \t\t\t\" (try upgrading GIT to a newer version)\",\n \t\t\tp->pack_name, ntohl(hdr.hdr_version));\n \n \t/* Verify the pack matches its index. */\n \tif (num_packed_objects(p) != ntohl(hdr.hdr_entries))\n-\t\tdie(\"packfile %s claims to have %u objects\"\n+\t\treturn error(\"packfile %s claims to have %u objects\"\n \t\t\t\" while index size indicates %u objects\",\n \t\t\tp->pack_name, ntohl(hdr.hdr_entries),\n \t\t\tnum_packed_objects(p));\n \tif (lseek(p->pack_fd, p->pack_size - sizeof(sha1), SEEK_SET) == -1)\n-\t\tdie(\"end of packfile %s is unavailable\", p->pack_name);\n+\t\treturn error(\"end of packfile %s is unavailable\", p->pack_name);\n \tif (read_in_full(p->pack_fd, sha1, sizeof(sha1)) != sizeof(sha1))\n-\t\tdie(\"packfile %s signature is unavailable\", p->pack_name);\n+\t\treturn error(\"packfile %s signature is unavailable\", p->pack_name);\n \tidx_sha1 = ((unsigned char *)p->index_base) + p->index_size - 40;\n \tif (hashcmp(sha1, idx_sha1))\n-\t\tdie(\"packfile %s does not match index\", p->pack_name);\n+\t\treturn error(\"packfile %s does not match index\", p->pack_name);\n+\treturn 0;\n }\n \n static int in_window(struct pack_window *win, unsigned long offset)\n@@ -627,8 +628,8 @@ unsigned char* use_pack(struct packed_git *p,\n {\n \tstruct pack_window *win = *w_cursor;\n \n-\tif (p->pack_fd == -1)\n-\t\topen_packed_git(p);\n+\tif (p->pack_fd == -1 && open_packed_git(p))\n+\t\tdie(\"packfile %s cannot be accessed\", p->pack_name);\n \n \t/* Since packfiles end in a hash of their content and its\n \t * pointless to ask for an offset into the middle of that\n-- \n1.5.0.rc3.1.ge4b0e\n"},{"id":"33281","messageId":"20070201205238.GD19009@spearce.org","threadId":"6613","inReplyTo":"a0b03fc086bb66e2aa2e386dcb4ff97fc837f07f.1170363130.git.spearce@spearce.org","subject":"[PATCH 4/4] Don't find objects in packs which aren't available anymore.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-02-01T20:52:38Z","receivedAt":"2007-02-01T20:52:38Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Matthias Lederhofer identified a race condition where a Git reader\nprocess was able to locate an object in a packed_git index, but\nwas then preempted while a `git repack -a -d` ran and completed.\nBy the time the reader was able to seek in the packfile to get the\nobject data, the packfile no longer existed on disk.\n\nIn this particular case the reader process did not attempt to\nopen the packfile before it was deleted, so it did not already\nhave the pack_fd field popuplated.  With the packfile itself gone,\nthere was no way for the reader to open it and fetch the data.\n\nI'm fixing the race condition by teaching find_pack_entry to ignore\na packed_git whose packfile is not currently open and which cannot\nbe opened.  If none of the currently known packs can supply the\nobject, we will return 0 and the caller will decide the object is\nnot available.  If this is the first attempt at finding an object,\nthe caller will reprepare_packed_git and try again.  If it was\nthe second attempt, the caller will typically return NULL back,\nand an error message about a missing object will be reported.\n\nThis patch does not address the situation of a reader which is\nbeing starved out by a tight sequence of `git repack -a -d` runs.\nIn this particular case the reader will try twice, probably fail\nboth times, and declare the object in question cannot be found.\nAs it is highly unlikely that a real world `git repack -a -d` can\ncomplete faster than a reader can open a packfile, so I don't think\nthis is a huge concern.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n sha1_file.c |   12 ++++++++++++\n 1 files changed, 12 insertions(+), 0 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 277319b..37669d6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1405,6 +1405,18 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e, cons\n \t\t}\n \t\toffset = find_pack_entry_one(sha1, p);\n \t\tif (offset) {\n+\t\t\t/*\n+\t\t\t * We are about to tell the caller where they can\n+\t\t\t * locate the requested object.  We better make\n+\t\t\t * sure the packfile is still here and can be\n+\t\t\t * accessed before supplying that answer, as\n+\t\t\t * it may have been deleted since the index\n+\t\t\t * was loaded!\n+\t\t\t */\n+\t\t\tif (p->pack_fd == -1 && open_packed_git(p)) {\n+\t\t\t\terror(\"packfile %s cannot be accessed\", p->pack_name);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\te->offset = offset;\n \t\t\te->p = p;\n \t\t\thashcpy(e->sha1, sha1);\n-- \n1.5.0.rc3.1.ge4b0e\n"},{"id":"33342","messageId":"7vwt31jd0q.fsf@assigned-by-dhcp.cox.net","threadId":"6613","inReplyTo":"20070201205227.GB19009@spearce.org","subject":"Re: [PATCH 2/4] Correct comment in prepare_packed_git_one.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-02T06:21:09Z","receivedAt":"2007-02-02T06:21:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> After staring at the comment and the associated for loop, I\n> realized the comment was completely bogus.\n\nYeah, when the comment was written, the loop after the comment\ndid not exist.  The comment was talking about calling\nadd_packed_git() to see if the corresponding pack can be mapped,\nand then upon receiving NULL, not linking p to the packed_git\nlinked list.\n"}]}