{"thread":{"id":"23475","subject":"git fetch over http:// left my repo broken","startedAt":"2010-04-15T09:51:25Z","lastAt":"2010-04-20T04:33:47Z","messageCount":46,"participants":["Christian Halstrick","Michael J Gruber","Ilari Liusvaara","Shawn O. Pearce","Johannes Sixt","Tay Ray Chuan","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"139571","messageId":"g2y8c627c4f1004150251l3dc2ad17n352b149ac739d309@mail.gmail.com","threadId":"23475","inReplyTo":null,"subject":"git fetch over http:// left my repo broken","fromName":"Christian Halstrick","fromEmail":"christian.halstrick@gmail.com","sentAt":"2010-04-15T09:51:25Z","receivedAt":"2010-04-15T09:51:25Z","isPatch":false,"sender":{"key":"christian.halstrick@gmail.com","avatar":"https://gravatar.com/avatar/3598bf518644c7dc32d4dcd8e0554b6a313e12103011862402c83ae2854203ce?d=mp&s=160"},"body":"Hi,\n\nsome days back I fetched from a github repo with http protocol and\nafterwards my local repo was broken. Since the fetch was done by a\ncronjob I don't know whether the fetch reported an error. Problem is\nthat one pack file was corrupted because the github servers put the\nrepo I wanted to clone into some maintenance mode while I was\nfetching. The pack file includes at the end the html source code -\nwhich makes these files clearly corrupted.\n\nGit should detect this error and let the fetch fail, right?\n\nAll this done on Linux (CentOS) with git version 1.6.6.1. Github repo\nwas http://github.com/sonatype/sonatype-tycho.git. This is not easy to\nreproduce - because you have to hit the maintenance times of github.\n\nHere is what I did:\n\n> git --version\ngit version 1.6.6.1\n> uname --all\nLinux wdfd00220954a.wdf.sap.corp 2.6.18-164.15.1.el5 #1 SMP Wed Mar 17\n11:30:06 EDT 2010 x86_64 x86_64 x86_64 GNU/Linux\n\n> git remote -v\norigin  http://github.com/sonatype/sonatype-tycho.git (fetch)\norigin  http://github.com/sonatype/sonatype-tycho.git (push)\n> git fetch origin\n...\n> git fsck --full\nerror: packfile\n./objects/pack/pack-b5dceb0bae390d6540f881be686839f7e691fa0b.pack does\nnot match index\nerror: packfile\n./objects/pack/pack-b5dceb0bae390d6540f881be686839f7e691fa0b.pack\ncannot be accessed\nerror: packfile\n./objects/pack/pack-b5dceb0bae390d6540f881be686839f7e691fa0b.pack does\nnot match index\nfatal: packfile\n./objects/pack/pack-b5dceb0bae390d6540f881be686839f7e691fa0b.pack\ncannot be accessed\n\n# get the size of the corrupted pack file\n> ls -l ./objects/pack/pack-b5dceb0bae390d6540f881be686839f7e691fa0b.pack\n-rw-r--r-- 1 git git 766920900 Apr 13 16:22\n./objects/pack/pack-b5dceb0bae390d6540f881be686839f7e691fa0b.pack\n\n# dump the start of the corrupted file: looks ok\n> xxd -l 64 ./objects/pack/pack-b5dceb0bae390d6540f881be686839f7e691fa0b.pack\n0000000: 5041 434b 0000 0002 0000 4bef 962b 789c  PACK......K..+x.\n0000010: 9591 cb72 dc20 1045 f77c 457f 8067 8a41  ...r. .E.|E..g.A\n0000020: 1a21 a552 a954 5cce 63e5 2adb 9b2c 7934  .!.R.T\\.c.*..,y4\n0000030: 2332 8856 00d9 99bf 37a3 781c 2f9d 0d05  #2.V....7.x./...\n\n# dump a section near the end of the corrupted file: html source code\nincluded here?\n>  xxd -s 766911990 -l 128 ./objects/pack/pack-b5dceb0bae390d6540f881be686839f7e691fa0b.pack\n2db625f6:d64b 2752 e70f 8f1a 6161 3c21 444f 4354  .K'R....aa grep -C5 -a \"maintenance\" ./objects/pack/pack-b5dceb0bae390d6540f881be686839f7e691fa0b.pack\n\n
\n \"Repo\n

Repository temporarily unavailable.

\n

The backend storage is temporarily offline. Usually this means the
\n storage server is undergoing maintenance. Your repository should
\n be available again very soon.

\n
\n\n \n>\n\n\nCiao\n Chris\n"},{"id":"139572","messageId":"4BC6E343.2030105@drmicha.warpmail.net","threadId":"23475","inReplyTo":"g2y8c627c4f1004150251l3dc2ad17n352b149ac739d309@mail.gmail.com","subject":"Re: git fetch over http:// left my repo broken","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-04-15T09:58:27Z","receivedAt":"2010-04-15T09:58:27Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Christian Halstrick venit, vidit, dixit 15.04.2010 11:51:\n> Hi,\n> \n> some days back I fetched from a github repo with http protocol and\n> afterwards my local repo was broken. Since the fetch was done by a\n> cronjob I don't know whether the fetch reported an error. Problem is\n> that one pack file was corrupted because the github servers put the\n> repo I wanted to clone into some maintenance mode while I was\n> fetching. The pack file includes at the end the html source code -\n> which makes these files clearly corrupted.\n> \n> Git should detect this error and let the fetch fail, right?\n\nRight. And Github should not pull your repo away from under your feet.\n\nBut still, Git should be able to deal with broken servers. The problem\nis: If the server does not report any problem but simply serves a broken\npack (with correct header), how should Git notice? It would require a\nfsck before accepting any new pack.\n\nIf you move away the broken pack, do you get any dangling refs?\n\nMichael\n"},{"id":"139575","messageId":"20100415113310.GA24305@LK-Perkele-V2.elisa-laajakaista.fi","threadId":"23475","inReplyTo":"g2y8c627c4f1004150251l3dc2ad17n352b149ac739d309@mail.gmail.com","subject":"Re: git fetch over http:// left my repo broken","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-04-15T11:33:11Z","receivedAt":"2010-04-15T11:33:11Z","isPatch":false,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Thu, Apr 15, 2010 at 11:51:25AM +0200, Christian Halstrick wrote:\n> Hi,\n> \n> some days back I fetched from a github repo with http protocol and\n> afterwards my local repo was broken. Since the fetch was done by a\n> cronjob I don't know whether the fetch reported an error. Problem is\n> that one pack file was corrupted because the github servers put the\n> repo I wanted to clone into some maintenance mode while I was\n> fetching. The pack file includes at the end the html source code -\n> which makes these files clearly corrupted.\n> \n> Git should detect this error and let the fetch fail, right?\n\nAt least with smart transports do verify pack hash (since \ngit-index-pack does check it).\n\nMaybe dumb HTTP transport grabs the index and pack and installs\nthe downloaded versions (FAIL) instead of generating index from\nthe pack (which would noitice the corruption)...\n\n-Ilari\n"},{"id":"139576","messageId":"20100415114319.GA28326@LK-Perkele-V2.elisa-laajakaista.fi","threadId":"23475","inReplyTo":"4BC6E343.2030105@drmicha.warpmail.net","subject":"Re: git fetch over http:// left my repo broken","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-04-15T11:43:19Z","receivedAt":"2010-04-15T11:43:19Z","isPatch":false,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Thu, Apr 15, 2010 at 11:58:27AM +0200, Michael J Gruber wrote:\n> Christian Halstrick venit, vidit, dixit 15.04.2010 11:51:\n> \n> But still, Git should be able to deal with broken servers. The problem\n> is: If the server does not report any problem but simply serves a broken\n> pack (with correct header), how should Git notice? It would require a\n> fsck before accepting any new pack.\n\nPack trailer hash. Apparently dumb HTTP fetch needs to bypass pack to index\nconversion somehow since index-pack aborts if trailer hash check fails (not\nto mention other failures corrupt pack may cause).\n\n-Ilari\n"},{"id":"139591","messageId":"20100415141504.GB17883@spearce.org","threadId":"23475","inReplyTo":"20100415114319.GA28326@LK-Perkele-V2.elisa-laajakaista.fi","subject":"Re: git fetch over http:// left my repo broken","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-15T14:15:04Z","receivedAt":"2010-04-15T14:15:04Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Ilari Liusvaara wrote:\n> On Thu, Apr 15, 2010 at 11:58:27AM +0200, Michael J Gruber wrote:\n> > Christian Halstrick venit, vidit, dixit 15.04.2010 11:51:\n> > \n> > But still, Git should be able to deal with broken servers. The problem\n> > is: If the server does not report any problem but simply serves a broken\n> > pack (with correct header), how should Git notice? It would require a\n> > fsck before accepting any new pack.\n> \n> Pack trailer hash. Apparently dumb HTTP fetch needs to bypass pack to index\n> conversion somehow since index-pack aborts if trailer hash check fails (not\n> to mention other failures corrupt pack may cause).\n\nOddly enough, http.c runs verify_pack() after the download,\nbut does so only after it swings the pack file into position.\nIf verify_pack() fails, it leaves the corrupt pack file in the\nobjects/pack directory. Talk about fail.\n\nI'll put together a patch shortly.\n\n-- \nShawn.\n"},{"id":"139605","messageId":"1271358560-8946-1-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100415141504.GB17883@spearce.org","subject":"[PATCH 0/6] detect dumb HTTP pack file corruption","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-15T19:09:14Z","receivedAt":"2010-04-15T19:09:14Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"This series tries to better detect and avoid corrupted pack and idx\nfiles downloaded over the dumb HTTP transport. It addresses the\nGitHub repository maintainence causing corruption issue reported\ntoday by Christian Halstrick.\n\nShawn O. Pearce (6):\n http.c: Remove bad free of static block\n t5550-http-fetch: Use subshell for repository operations\n http.c: Tiny refactoring of finish_http_pack_request\n http.c: Drop useless != NULL test in finish_http_pack_request\n http-fetch: Use index-pack rather than verify-pack to check packs\n http-fetch: Use temporary files for pack-*.idx until verified\n\n cache.h | 3 +-\n http.c | 118 +++++++++++++++++++++++++++++++++----------------\n http.h | 1 -\n pack-check.c | 15 +++++--\n pack.h | 1 +\n sha1_file.c | 17 +++++--\n t/t5550-http-fetch.sh | 37 ++++++++++++++-\n 7 files changed, 140 insertions(+), 52 deletions(-)\n"},{"id":"139606","messageId":"1271358560-8946-2-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100415141504.GB17883@spearce.org","subject":"[PATCH 1/6] http.c: Remove bad free of static block","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-15T19:09:15Z","receivedAt":"2010-04-15T19:09:15Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"The filename variable here is pointing to a block of memory that\nwas allocated by sha1_file.c and is also held in a static variable\nscoped within the sha1_pack_name() function. Doing a free() here is\nreturning that memory to the allocator while we might still try to\nreuse it on a subsequent sha1_pack_name() invocation. That's not\nacceptable, so don't free it.\n\nSigned-off-by: Shawn O. Pearce \n---\n http.c | 1 -\n 1 files changed, 0 insertions(+), 1 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 4814217..f26625e 100644\n--- a/http.c\n+++ b/http.c\n@@ -1082,7 +1082,6 @@ struct http_pack_request *new_http_pack_request(\n \treturn preq;\n \n abort:\n-\tfree(filename);\n \tfree(preq->url);\n \tfree(preq);\n \treturn NULL;\n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139607","messageId":"1271358560-8946-3-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100415141504.GB17883@spearce.org","subject":"[PATCH 2/6] t5550-http-fetch: Use subshell for repository operations","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-15T19:09:16Z","receivedAt":"2010-04-15T19:09:16Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Change into the server repository's directory using a subshell,\nso we can return back to the top of the trash directory before\ndoing anything more in the test script.\n\nSigned-off-by: Shawn O. Pearce \n---\n t/t5550-http-fetch.sh | 7 ++++---\n 1 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex 8cfce96..78c31c9 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -55,9 +55,10 @@ test_expect_success 'http remote detects correct HEAD' '\n \n test_expect_success 'fetch packed objects' '\n \tcp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n-\tcd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n-\tgit --bare repack &&\n-\tgit --bare prune-packed &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n+\t git --bare repack &&\n+\t git --bare prune-packed\n+\t) &&\n \tgit clone $HTTPD_URL/dumb/repo_pack.git\n '\n \n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139608","messageId":"1271358560-8946-4-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100415141504.GB17883@spearce.org","subject":"[PATCH 3/6] http.c: Tiny refactoring of finish_http_pack_request","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-15T19:09:17Z","receivedAt":"2010-04-15T19:09:17Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Always remove the struct packed_git from the active list, even\nif the rename of the temporary file fails.\n\nWhile we are here, simplify the code a bit by using a common\nlocal variable name (\"p\") to hold the relevant packed_git.\n\nSigned-off-by: Shawn O. Pearce \n---\n http.c | 16 ++++++++--------\n 1 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex f26625e..4558f11 100644\n--- a/http.c\n+++ b/http.c\n@@ -1002,8 +1002,9 @@ int finish_http_pack_request(struct http_pack_request *preq)\n {\n \tint ret;\n \tstruct packed_git **lst;\n+\tstruct packed_git *p = preq->target;\n \n-\tpreq->target->pack_size = ftell(preq->packfile);\n+\tp->pack_size = ftell(preq->packfile);\n \n \tif (preq->packfile != NULL) {\n \t\tfclose(preq->packfile);\n@@ -1011,18 +1012,17 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \t\tpreq->slot->local = NULL;\n \t}\n \n-\tret = move_temp_to_file(preq->tmpfile, preq->filename);\n-\tif (ret)\n-\t\treturn ret;\n-\n \tlst = preq->lst;\n-\twhile (*lst != preq->target)\n+\twhile (*lst != p)\n \t\tlst = &((*lst)->next);\n \t*lst = (*lst)->next;\n \n-\tif (verify_pack(preq->target))\n+\tret = move_temp_to_file(preq->tmpfile, preq->filename);\n+\tif (ret)\n+\t\treturn ret;\n+\tif (verify_pack(p))\n \t\treturn -1;\n-\tinstall_packed_git(preq->target);\n+\tinstall_packed_git(p);\n \n \treturn 0;\n }\n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139612","messageId":"1271358560-8946-5-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100415141504.GB17883@spearce.org","subject":"[PATCH 4/6] http.c: Drop useless != NULL test in finish_http_pack_request","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-15T19:09:18Z","receivedAt":"2010-04-15T19:09:18Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"The test preq->packfile != NULL is always true. If packfile was\nactually NULL when entering this function the ftell() above would\ncrash out with a SIGSEGV, resulting in never reaching this point.\n\nSimplify the code by just removing the conditional.\n\nSigned-off-by: Shawn O. Pearce \n---\n http.c | 9 +++------\n 1 files changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 4558f11..64e0c18 100644\n--- a/http.c\n+++ b/http.c\n@@ -1005,12 +1005,9 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \tstruct packed_git *p = preq->target;\n \n \tp->pack_size = ftell(preq->packfile);\n-\n-\tif (preq->packfile != NULL) {\n-\t\tfclose(preq->packfile);\n-\t\tpreq->packfile = NULL;\n-\t\tpreq->slot->local = NULL;\n-\t}\n+\tfclose(preq->packfile);\n+\tpreq->packfile = NULL;\n+\tpreq->slot->local = NULL;\n \n \tlst = preq->lst;\n \twhile (*lst != p)\n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139609","messageId":"1271358560-8946-6-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100415141504.GB17883@spearce.org","subject":"[PATCH 5/6] http-fetch: Use index-pack rather than verify-pack to check packs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-15T19:09:19Z","receivedAt":"2010-04-15T19:09:19Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"To ensure we don't leave a corrupt pack file positioned as though\nit were a valid pack file, run index-pack on the temporary pack\nbefore we rename it to its final name. If index-pack crashes out\nwhen it discovers file corruption (e.g. GitHub's error HTML at the\nend of the file), simply delete the temporary files to cleanup.\n\nBy waiting until the pack has been validated before we move it\nto its final name, we eliminate a race condition where another\nconcurrent reader might try to access the pack at the same time\nthat we are still trying to verify its not corrupt.\n\nSwitching from verify-pack to index-pack is a change in behavior,\nbut it should turn out better for users. The index-pack algorithm\ntries to minimize disk seeks, as well as the number of times any\ngiven object is inflated, by organizing its work along delta chains.\nThe verify-pack logic does not attempt to do this, thrashing the\ndelta base cache and the filesystem cache.\n\nBy recreating the index file locally, we also can automatically\nupgrade from a v1 pack table of contents to v2. This makes the\nCRC32 data available for use during later repacks, even if the\nserver didn't have them on hand.\n\nSigned-off-by: Shawn O. Pearce \n---\n cache.h | 1 +\n http.c | 42 ++++++++++++++++++++++++++++++++++--------\n http.h | 1 -\n sha1_file.c | 11 +++++++++--\n t/t5550-http-fetch.sh | 15 +++++++++++++++\n 5 files changed, 59 insertions(+), 11 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 5eb0573..4150603 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -916,6 +916,7 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\n \n extern void pack_report(void);\n extern int open_pack_index(struct packed_git *);\n+extern void close_pack_index(struct packed_git *);\n extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *);\n extern void close_pack_windows(struct packed_git *);\n extern void unuse_pack(struct pack_window **);\ndiff --git a/http.c b/http.c\nindex 64e0c18..aa3e380 100644\n--- a/http.c\n+++ b/http.c\n@@ -1,6 +1,7 @@\n #include \"http.h\"\n #include \"pack.h\"\n #include \"sideband.h\"\n+#include \"run-command.h\"\n \n int data_received;\n int active_requests;\n@@ -1000,11 +1001,13 @@ void release_http_pack_request(struct http_pack_request *preq)\n \n int finish_http_pack_request(struct http_pack_request *preq)\n {\n-\tint ret;\n \tstruct packed_git **lst;\n \tstruct packed_git *p = preq->target;\n+\tchar *tmp_idx;\n+\tstruct child_process ip;\n+\tconst char *ip_argv[8];\n \n-\tp->pack_size = ftell(preq->packfile);\n+\tclose_pack_index(p);\n \tfclose(preq->packfile);\n \tpreq->packfile = NULL;\n \tpreq->slot->local = NULL;\n@@ -1014,13 +1017,37 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \t\tlst = &((*lst)->next);\n \t*lst = (*lst)->next;\n \n-\tret = move_temp_to_file(preq->tmpfile, preq->filename);\n-\tif (ret)\n-\t\treturn ret;\n-\tif (verify_pack(p))\n+\ttmp_idx = xstrdup(preq->tmpfile);\n+\tstrcpy(tmp_idx + strlen(tmp_idx) - strlen(\".pack.temp\"),\n+\t \".idx.temp\");\n+\n+\tip_argv[0] = \"index-pack\";\n+\tip_argv[1] = \"-o\";\n+\tip_argv[2] = tmp_idx;\n+\tip_argv[3] = preq->tmpfile;\n+\tip_argv[4] = NULL;\n+\n+\tmemset(&ip, 0, sizeof(ip));\n+\tip.argv = ip_argv;\n+\tip.git_cmd = 1;\n+\tip.no_stdin = 1;\n+\tip.no_stdout = 1;\n+\n+\tif (run_command(&ip)) {\n+\t\tunlink(preq->tmpfile);\n+\t\tunlink(tmp_idx);\n+\t\tfree(tmp_idx);\n \t\treturn -1;\n-\tinstall_packed_git(p);\n+\t}\n \n+\tif (move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1))\n+\t || move_temp_to_file(tmp_idx, sha1_pack_index_name(p->sha1))) {\n+\t\tfree(tmp_idx);\n+\t\treturn -1;\n+\t}\n+\n+\tinstall_packed_git(p);\n+\tfree(tmp_idx);\n \treturn 0;\n }\n \n@@ -1043,7 +1070,6 @@ struct http_pack_request *new_http_pack_request(\n \tpreq->url = strbuf_detach(&buf, NULL);\n \n \tfilename = sha1_pack_name(target->sha1);\n-\tsnprintf(preq->filename, sizeof(preq->filename), \"%s\", filename);\n \tsnprintf(preq->tmpfile, sizeof(preq->tmpfile), \"%s.temp\", filename);\n \tpreq->packfile = fopen(preq->tmpfile, \"a\");\n \tif (!preq->packfile) {\ndiff --git a/http.h b/http.h\nindex 5c9441c..e4a8126 100644\n--- a/http.h\n+++ b/http.h\n@@ -152,7 +152,6 @@ struct http_pack_request\n \tstruct packed_git *target;\n \tstruct packed_git **lst;\n \tFILE *packfile;\n-\tchar filename[PATH_MAX];\n \tchar tmpfile[PATH_MAX];\n \tstruct curl_slist *range_header;\n \tstruct active_request_slot *slot;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex ff65328..820063e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -599,6 +599,14 @@ void unuse_pack(struct pack_window **w_cursor)\n \t}\n }\n \n+void close_pack_index(struct packed_git *p)\n+{\n+\tif (p->index_data) {\n+\t\tmunmap((void *)p->index_data, p->index_size);\n+\t\tp->index_data = NULL;\n+\t}\n+}\n+\n /*\n * This is used by git-repack in case a newly created pack happens to\n * contain the same set of objects as an existing one. In that case\n@@ -620,8 +628,7 @@ void free_pack_by_name(const char *pack_name)\n \t\t\tclose_pack_windows(p);\n \t\t\tif (p->pack_fd != -1)\n \t\t\t\tclose(p->pack_fd);\n-\t\t\tif (p->index_data)\n-\t\t\t\tmunmap((void *)p->index_data, p->index_size);\n+\t\t\tclose_pack_index(p);\n \t\t\tfree(p->bad_object_sha1);\n \t\t\t*pp = p->next;\n \t\t\tfree(p);\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex 78c31c9..bdac8d7 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -62,6 +62,21 @@ test_expect_success 'fetch packed objects' '\n \tgit clone $HTTPD_URL/dumb/repo_pack.git\n '\n \n+test_expect_success 'fetch notices corrupt pack' '\n+\tcp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n+\t p=`ls objects/pack/pack-*.pack` &&\n+\t chmod u+w $p &&\n+\t dd if=/dev/zero of=$p bs=256 count=1 seek=1\n+\t) &&\n+\tmkdir repo_bad1.git &&\n+\t(cd repo_bad1.git &&\n+\t git --bare init &&\n+\t test_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad1.git &&\n+\t test 0 = `ls objects/pack/pack-*.pack | wc -l`\n+\t)\n+'\n+\n test_expect_success 'did not use upload-pack service' '\n \tgrep '/git-upload-pack' <\"$HTTPD_ROOT_PATH\"/access.log >act\n \t: >exp\n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139611","messageId":"1271358560-8946-7-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100415141504.GB17883@spearce.org","subject":"[PATCH 6/6] http-fetch: Use temporary files for pack-*.idx until verified","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-15T19:09:20Z","receivedAt":"2010-04-15T19:09:20Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Verify that a downloaded pack-*.idx file is consistent and valid\nas an index file before we rename it into its final destination.\nThis prevents a corrupt index file from later being treated as a\nusable file, confusing readers.\n\nSigned-off-by: Shawn O. Pearce \n---\n cache.h | 2 +-\n http.c | 62 +++++++++++++++++++++++++++++++-----------------\n pack-check.c | 15 ++++++++---\n pack.h | 1 +\n sha1_file.c | 6 +++-\n t/t5550-http-fetch.sh | 15 ++++++++++++\n 6 files changed, 72 insertions(+), 29 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 4150603..0d101e4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -905,7 +905,7 @@ struct extra_have_objects {\n extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags, struct extra_have_objects *);\n extern int server_supports(const char *feature);\n \n-extern struct packed_git *parse_pack_index(unsigned char *sha1);\n+extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \n extern void prepare_packed_git(void);\n extern void reprepare_packed_git(void);\ndiff --git a/http.c b/http.c\nindex aa3e380..2d88034 100644\n--- a/http.c\n+++ b/http.c\n@@ -897,47 +897,65 @@ int http_fetch_ref(const char *base, struct ref *ref)\n }\n \n /* Helpers for fetching packs */\n-static int fetch_pack_index(unsigned char *sha1, const char *base_url)\n+static char *fetch_pack_index(unsigned char *sha1, const char *base_url)\n {\n-\tint ret = 0;\n-\tchar *hex = xstrdup(sha1_to_hex(sha1));\n-\tchar *filename;\n-\tchar *url = NULL;\n+\tchar *url, *tmp;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tif (has_pack_index(sha1)) {\n-\t\tret = 0;\n-\t\tgoto cleanup;\n-\t}\n-\n \tif (http_is_verbose)\n-\t\tfprintf(stderr, \"Getting index for pack %s\\n\", hex);\n+\t\tfprintf(stderr, \"Getting index for pack %s\\n\", sha1_to_hex(sha1));\n \n \tend_url_with_slash(&buf, base_url);\n-\tstrbuf_addf(&buf, \"objects/pack/pack-%s.idx\", hex);\n+\tstrbuf_addf(&buf, \"objects/pack/pack-%s.idx\", sha1_to_hex(sha1));\n \turl = strbuf_detach(&buf, NULL);\n \n-\tfilename = sha1_pack_index_name(sha1);\n-\tif (http_get_file(url, filename, 0) != HTTP_OK)\n-\t\tret = error(\"Unable to get pack index %s\\n\", url);\n+\tstrbuf_addf(&buf, \"%s.temp\", sha1_pack_index_name(sha1));\n+\ttmp = strbuf_detach(&buf, NULL);\n+\n+\tif (http_get_file(url, tmp, 0) != HTTP_OK) {\n+\t\terror(\"Unable to get pack index %s\\n\", url);\n+\t\tfree(tmp);\n+\t\ttmp = NULL;\n+\t}\n \n-cleanup:\n-\tfree(hex);\n \tfree(url);\n-\treturn ret;\n+\treturn tmp;\n }\n \n static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n \tunsigned char *sha1, const char *base_url)\n {\n \tstruct packed_git *new_pack;\n+\tchar *tmp_idx = NULL;\n \n-\tif (fetch_pack_index(sha1, base_url))\n-\t\treturn -1;\n+\tif (!has_pack_index(sha1)) {\n+\t\ttmp_idx = fetch_pack_index(sha1, base_url);\n+\t\tif (!tmp_idx)\n+\t\t\treturn -1;\n+\t}\n \n-\tnew_pack = parse_pack_index(sha1);\n-\tif (!new_pack)\n+\tnew_pack = parse_pack_index(sha1, tmp_idx);\n+\tif (!new_pack) {\n+\t\tif (tmp_idx) {\n+\t\t\tunlink(tmp_idx);\n+\t\t\tfree(tmp_idx);\n+\t\t}\n \t\treturn -1; /* parse_pack_index() already issued error message */\n+\t}\n+\n+\tif (tmp_idx) {\n+\t\tint ret;\n+\n+\t\tret = verify_pack_index(new_pack);\n+\t\tif (!ret) {\n+\t\t\tclose_pack_index(new_pack);\n+\t\t\tret = move_temp_to_file(tmp_idx, sha1_pack_index_name(sha1));\n+\t\t}\n+\t\tfree(tmp_idx);\n+\t\tif (ret)\n+\t\t\treturn -1;\n+\t}\n+\n \tnew_pack->next = *packs_head;\n \t*packs_head = new_pack;\n \treturn 0;\ndiff --git a/pack-check.c b/pack-check.c\nindex 166ca70..9baba12 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -133,14 +133,13 @@ static int verify_packfile(struct packed_git *p,\n \treturn err;\n }\n \n-int verify_pack(struct packed_git *p)\n+int verify_pack_index(struct packed_git *p)\n {\n \toff_t index_size;\n \tconst unsigned char *index_base;\n \tgit_SHA_CTX ctx;\n \tunsigned char sha1[20];\n \tint err = 0;\n-\tstruct pack_window *w_curs = NULL;\n \n \tif (open_pack_index(p))\n \t\treturn error(\"packfile %s index not opened\", p->pack_name);\n@@ -154,9 +153,17 @@ int verify_pack(struct packed_git *p)\n \tif (hashcmp(sha1, index_base + index_size - 20))\n \t\terr = error(\"Packfile index for %s SHA1 mismatch\",\n \t\t\t p->pack_name);\n+\treturn err;\n+}\n+\n+int verify_pack(struct packed_git *p)\n+{\n+\tint err = 0;\n+\tstruct pack_window *w_curs = NULL;\n \n-\t/* Verify pack file */\n-\terr |= verify_packfile(p, &w_curs);\n+\terr |= verify_pack_index(p);\n+\tif (!err)\n+\t\terr |= verify_packfile(p, &w_curs);\n \tunuse_pack(&w_curs);\n \n \treturn err;\ndiff --git a/pack.h b/pack.h\nindex d268c01..bb27576 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -57,6 +57,7 @@ struct pack_idx_entry {\n \n extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1);\n extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);\n+extern int verify_pack_index(struct packed_git *);\n extern int verify_pack(struct packed_git *);\n extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);\n extern char *index_pack_lockfile(int fd);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 820063e..232e14d 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -838,12 +838,14 @@ struct packed_git *add_packed_git(const char *path, int path_len, int local)\n \treturn p;\n }\n \n-struct packed_git *parse_pack_index(unsigned char *sha1)\n+struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)\n {\n-\tconst char *idx_path = sha1_pack_index_name(sha1);\n \tconst char *path = sha1_pack_name(sha1);\n \tstruct packed_git *p = alloc_packed_git(strlen(path) + 1);\n \n+\tif (!idx_path)\n+\t\tidx_path = sha1_pack_index_name(sha1);\n+\n \tstrcpy(p->pack_name, path);\n \thashcpy(p->sha1, sha1);\n \tif (check_packed_git_idx(idx_path, p)) {\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex bdac8d7..ee170d3 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -77,6 +77,21 @@ test_expect_success 'fetch notices corrupt pack' '\n \t)\n '\n \n+test_expect_success 'fetch notices corrupt idx' '\n+\tcp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t p=`ls objects/pack/pack-*.idx` &&\n+\t chmod u+w $p &&\n+\t dd if=/dev/zero of=$p bs=256 count=1 seek=1\n+\t) &&\n+\tmkdir repo_bad2.git &&\n+\t(cd repo_bad2.git &&\n+\t git --bare init &&\n+\t test_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad2.git &&\n+\t test 0 = `ls objects/pack | wc -l`\n+\t)\n+'\n+\n test_expect_success 'did not use upload-pack service' '\n \tgrep '/git-upload-pack' <\"$HTTPD_ROOT_PATH\"/access.log >act\n \t: >exp\n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139616","messageId":"201004152134.10555.j6t@kdbg.org","threadId":"23475","inReplyTo":"1271358560-8946-6-git-send-email-spearce@spearce.org","subject":"Re: [PATCH 5/6] http-fetch: Use index-pack rather than verify-pack to check packs","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-04-15T19:34:09Z","receivedAt":"2010-04-15T19:34:09Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 15. April 2010, Shawn O. Pearce wrote:\n> +test_expect_success 'fetch notices corrupt pack' '\n> +\tcp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git\n> \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git && +\t(cd\n> \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n> +\t p=`ls objects/pack/pack-*.pack` &&\n> +\t chmod u+w $p &&\n> +\t dd if=/dev/zero of=$p bs=256 count=1 seek=1\n\nSince the particular byte that overwrites the pack is irrelevant, please make \nthis:\n\n\tprintf %0256d 0 | dd of=$p bs=256 count=1 seek=1\n\nfor the benefit of us poor Windowsers who do not have /dev/zero.\n\nPerhaps you want to add conv=notrunc.\n\nDitto in patch 6/6.\n\nThanks,\n-- Hannes\n"},{"id":"139629","messageId":"1271366704-25262-1-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"201004152134.10555.j6t@kdbg.org","subject":"[PATCH v2 5/6] http-fetch: Use index-pack rather than verify-pack to check packs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-15T21:25:03Z","receivedAt":"2010-04-15T21:25:03Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"To ensure we don't leave a corrupt pack file positioned as though\nit were a valid pack file, run index-pack on the temporary pack\nbefore we rename it to its final name. If index-pack crashes out\nwhen it discovers file corruption (e.g. GitHub's error HTML at the\nend of the file), simply delete the temporary files to cleanup.\n\nBy waiting until the pack has been validated before we move it\nto its final name, we eliminate a race condition where another\nconcurrent reader might try to access the pack at the same time\nthat we are still trying to verify its not corrupt.\n\nSwitching from verify-pack to index-pack is a change in behavior,\nbut it should turn out better for users. The index-pack algorithm\ntries to minimize disk seeks, as well as the number of times any\ngiven object is inflated, by organizing its work along delta chains.\nThe verify-pack logic does not attempt to do this, thrashing the\ndelta base cache and the filesystem cache.\n\nBy recreating the index file locally, we also can automatically\nupgrade from a v1 pack table of contents to v2. This makes the\nCRC32 data available for use during later repacks, even if the\nserver didn't have them on hand.\n\nSigned-off-by: Shawn O. Pearce \n---\n cache.h | 1 +\n http.c | 42 ++++++++++++++++++++++++++++++++++--------\n http.h | 1 -\n sha1_file.c | 11 +++++++++--\n t/t5550-http-fetch.sh | 15 +++++++++++++++\n 5 files changed, 59 insertions(+), 11 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 5eb0573..4150603 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -916,6 +916,7 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\n \n extern void pack_report(void);\n extern int open_pack_index(struct packed_git *);\n+extern void close_pack_index(struct packed_git *);\n extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *);\n extern void close_pack_windows(struct packed_git *);\n extern void unuse_pack(struct pack_window **);\ndiff --git a/http.c b/http.c\nindex 64e0c18..aa3e380 100644\n--- a/http.c\n+++ b/http.c\n@@ -1,6 +1,7 @@\n #include \"http.h\"\n #include \"pack.h\"\n #include \"sideband.h\"\n+#include \"run-command.h\"\n \n int data_received;\n int active_requests;\n@@ -1000,11 +1001,13 @@ void release_http_pack_request(struct http_pack_request *preq)\n \n int finish_http_pack_request(struct http_pack_request *preq)\n {\n-\tint ret;\n \tstruct packed_git **lst;\n \tstruct packed_git *p = preq->target;\n+\tchar *tmp_idx;\n+\tstruct child_process ip;\n+\tconst char *ip_argv[8];\n \n-\tp->pack_size = ftell(preq->packfile);\n+\tclose_pack_index(p);\n \tfclose(preq->packfile);\n \tpreq->packfile = NULL;\n \tpreq->slot->local = NULL;\n@@ -1014,13 +1017,37 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \t\tlst = &((*lst)->next);\n \t*lst = (*lst)->next;\n \n-\tret = move_temp_to_file(preq->tmpfile, preq->filename);\n-\tif (ret)\n-\t\treturn ret;\n-\tif (verify_pack(p))\n+\ttmp_idx = xstrdup(preq->tmpfile);\n+\tstrcpy(tmp_idx + strlen(tmp_idx) - strlen(\".pack.temp\"),\n+\t \".idx.temp\");\n+\n+\tip_argv[0] = \"index-pack\";\n+\tip_argv[1] = \"-o\";\n+\tip_argv[2] = tmp_idx;\n+\tip_argv[3] = preq->tmpfile;\n+\tip_argv[4] = NULL;\n+\n+\tmemset(&ip, 0, sizeof(ip));\n+\tip.argv = ip_argv;\n+\tip.git_cmd = 1;\n+\tip.no_stdin = 1;\n+\tip.no_stdout = 1;\n+\n+\tif (run_command(&ip)) {\n+\t\tunlink(preq->tmpfile);\n+\t\tunlink(tmp_idx);\n+\t\tfree(tmp_idx);\n \t\treturn -1;\n-\tinstall_packed_git(p);\n+\t}\n \n+\tif (move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1))\n+\t || move_temp_to_file(tmp_idx, sha1_pack_index_name(p->sha1))) {\n+\t\tfree(tmp_idx);\n+\t\treturn -1;\n+\t}\n+\n+\tinstall_packed_git(p);\n+\tfree(tmp_idx);\n \treturn 0;\n }\n \n@@ -1043,7 +1070,6 @@ struct http_pack_request *new_http_pack_request(\n \tpreq->url = strbuf_detach(&buf, NULL);\n \n \tfilename = sha1_pack_name(target->sha1);\n-\tsnprintf(preq->filename, sizeof(preq->filename), \"%s\", filename);\n \tsnprintf(preq->tmpfile, sizeof(preq->tmpfile), \"%s.temp\", filename);\n \tpreq->packfile = fopen(preq->tmpfile, \"a\");\n \tif (!preq->packfile) {\ndiff --git a/http.h b/http.h\nindex 5c9441c..e4a8126 100644\n--- a/http.h\n+++ b/http.h\n@@ -152,7 +152,6 @@ struct http_pack_request\n \tstruct packed_git *target;\n \tstruct packed_git **lst;\n \tFILE *packfile;\n-\tchar filename[PATH_MAX];\n \tchar tmpfile[PATH_MAX];\n \tstruct curl_slist *range_header;\n \tstruct active_request_slot *slot;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex ff65328..820063e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -599,6 +599,14 @@ void unuse_pack(struct pack_window **w_cursor)\n \t}\n }\n \n+void close_pack_index(struct packed_git *p)\n+{\n+\tif (p->index_data) {\n+\t\tmunmap((void *)p->index_data, p->index_size);\n+\t\tp->index_data = NULL;\n+\t}\n+}\n+\n /*\n * This is used by git-repack in case a newly created pack happens to\n * contain the same set of objects as an existing one. In that case\n@@ -620,8 +628,7 @@ void free_pack_by_name(const char *pack_name)\n \t\t\tclose_pack_windows(p);\n \t\t\tif (p->pack_fd != -1)\n \t\t\t\tclose(p->pack_fd);\n-\t\t\tif (p->index_data)\n-\t\t\t\tmunmap((void *)p->index_data, p->index_size);\n+\t\t\tclose_pack_index(p);\n \t\t\tfree(p->bad_object_sha1);\n \t\t\t*pp = p->next;\n \t\t\tfree(p);\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex 78c31c9..1a4dfc9 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -62,6 +62,21 @@ test_expect_success 'fetch packed objects' '\n \tgit clone $HTTPD_URL/dumb/repo_pack.git\n '\n \n+test_expect_success 'fetch notices corrupt pack' '\n+\tcp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n+\t p=`ls objects/pack/pack-*.pack` &&\n+\t chmod u+w $p &&\n+\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n+\t) &&\n+\tmkdir repo_bad1.git &&\n+\t(cd repo_bad1.git &&\n+\t git --bare init &&\n+\t test_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad1.git &&\n+\t test 0 = `ls objects/pack/pack-*.pack | wc -l`\n+\t)\n+'\n+\n test_expect_success 'did not use upload-pack service' '\n \tgrep '/git-upload-pack' <\"$HTTPD_ROOT_PATH\"/access.log >act\n \t: >exp\n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139630","messageId":"1271366704-25262-2-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"201004152134.10555.j6t@kdbg.org","subject":"[PATCH v2 6/6] http-fetch: Use temporary files for pack-*.idx until verified","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-15T21:25:04Z","receivedAt":"2010-04-15T21:25:04Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Verify that a downloaded pack-*.idx file is consistent and valid\nas an index file before we rename it into its final destination.\nThis prevents a corrupt index file from later being treated as a\nusable file, confusing readers.\n\nSigned-off-by: Shawn O. Pearce \n---\n cache.h | 2 +-\n http.c | 62 +++++++++++++++++++++++++++++++-----------------\n pack-check.c | 15 ++++++++---\n pack.h | 1 +\n sha1_file.c | 6 +++-\n t/t5550-http-fetch.sh | 15 ++++++++++++\n 6 files changed, 72 insertions(+), 29 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 4150603..0d101e4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -905,7 +905,7 @@ struct extra_have_objects {\n extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags, struct extra_have_objects *);\n extern int server_supports(const char *feature);\n \n-extern struct packed_git *parse_pack_index(unsigned char *sha1);\n+extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \n extern void prepare_packed_git(void);\n extern void reprepare_packed_git(void);\ndiff --git a/http.c b/http.c\nindex aa3e380..2d88034 100644\n--- a/http.c\n+++ b/http.c\n@@ -897,47 +897,65 @@ int http_fetch_ref(const char *base, struct ref *ref)\n }\n \n /* Helpers for fetching packs */\n-static int fetch_pack_index(unsigned char *sha1, const char *base_url)\n+static char *fetch_pack_index(unsigned char *sha1, const char *base_url)\n {\n-\tint ret = 0;\n-\tchar *hex = xstrdup(sha1_to_hex(sha1));\n-\tchar *filename;\n-\tchar *url = NULL;\n+\tchar *url, *tmp;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tif (has_pack_index(sha1)) {\n-\t\tret = 0;\n-\t\tgoto cleanup;\n-\t}\n-\n \tif (http_is_verbose)\n-\t\tfprintf(stderr, \"Getting index for pack %s\\n\", hex);\n+\t\tfprintf(stderr, \"Getting index for pack %s\\n\", sha1_to_hex(sha1));\n \n \tend_url_with_slash(&buf, base_url);\n-\tstrbuf_addf(&buf, \"objects/pack/pack-%s.idx\", hex);\n+\tstrbuf_addf(&buf, \"objects/pack/pack-%s.idx\", sha1_to_hex(sha1));\n \turl = strbuf_detach(&buf, NULL);\n \n-\tfilename = sha1_pack_index_name(sha1);\n-\tif (http_get_file(url, filename, 0) != HTTP_OK)\n-\t\tret = error(\"Unable to get pack index %s\\n\", url);\n+\tstrbuf_addf(&buf, \"%s.temp\", sha1_pack_index_name(sha1));\n+\ttmp = strbuf_detach(&buf, NULL);\n+\n+\tif (http_get_file(url, tmp, 0) != HTTP_OK) {\n+\t\terror(\"Unable to get pack index %s\\n\", url);\n+\t\tfree(tmp);\n+\t\ttmp = NULL;\n+\t}\n \n-cleanup:\n-\tfree(hex);\n \tfree(url);\n-\treturn ret;\n+\treturn tmp;\n }\n \n static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n \tunsigned char *sha1, const char *base_url)\n {\n \tstruct packed_git *new_pack;\n+\tchar *tmp_idx = NULL;\n \n-\tif (fetch_pack_index(sha1, base_url))\n-\t\treturn -1;\n+\tif (!has_pack_index(sha1)) {\n+\t\ttmp_idx = fetch_pack_index(sha1, base_url);\n+\t\tif (!tmp_idx)\n+\t\t\treturn -1;\n+\t}\n \n-\tnew_pack = parse_pack_index(sha1);\n-\tif (!new_pack)\n+\tnew_pack = parse_pack_index(sha1, tmp_idx);\n+\tif (!new_pack) {\n+\t\tif (tmp_idx) {\n+\t\t\tunlink(tmp_idx);\n+\t\t\tfree(tmp_idx);\n+\t\t}\n \t\treturn -1; /* parse_pack_index() already issued error message */\n+\t}\n+\n+\tif (tmp_idx) {\n+\t\tint ret;\n+\n+\t\tret = verify_pack_index(new_pack);\n+\t\tif (!ret) {\n+\t\t\tclose_pack_index(new_pack);\n+\t\t\tret = move_temp_to_file(tmp_idx, sha1_pack_index_name(sha1));\n+\t\t}\n+\t\tfree(tmp_idx);\n+\t\tif (ret)\n+\t\t\treturn -1;\n+\t}\n+\n \tnew_pack->next = *packs_head;\n \t*packs_head = new_pack;\n \treturn 0;\ndiff --git a/pack-check.c b/pack-check.c\nindex 166ca70..9baba12 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -133,14 +133,13 @@ static int verify_packfile(struct packed_git *p,\n \treturn err;\n }\n \n-int verify_pack(struct packed_git *p)\n+int verify_pack_index(struct packed_git *p)\n {\n \toff_t index_size;\n \tconst unsigned char *index_base;\n \tgit_SHA_CTX ctx;\n \tunsigned char sha1[20];\n \tint err = 0;\n-\tstruct pack_window *w_curs = NULL;\n \n \tif (open_pack_index(p))\n \t\treturn error(\"packfile %s index not opened\", p->pack_name);\n@@ -154,9 +153,17 @@ int verify_pack(struct packed_git *p)\n \tif (hashcmp(sha1, index_base + index_size - 20))\n \t\terr = error(\"Packfile index for %s SHA1 mismatch\",\n \t\t\t p->pack_name);\n+\treturn err;\n+}\n+\n+int verify_pack(struct packed_git *p)\n+{\n+\tint err = 0;\n+\tstruct pack_window *w_curs = NULL;\n \n-\t/* Verify pack file */\n-\terr |= verify_packfile(p, &w_curs);\n+\terr |= verify_pack_index(p);\n+\tif (!err)\n+\t\terr |= verify_packfile(p, &w_curs);\n \tunuse_pack(&w_curs);\n \n \treturn err;\ndiff --git a/pack.h b/pack.h\nindex d268c01..bb27576 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -57,6 +57,7 @@ struct pack_idx_entry {\n \n extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1);\n extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);\n+extern int verify_pack_index(struct packed_git *);\n extern int verify_pack(struct packed_git *);\n extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);\n extern char *index_pack_lockfile(int fd);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 820063e..232e14d 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -838,12 +838,14 @@ struct packed_git *add_packed_git(const char *path, int path_len, int local)\n \treturn p;\n }\n \n-struct packed_git *parse_pack_index(unsigned char *sha1)\n+struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)\n {\n-\tconst char *idx_path = sha1_pack_index_name(sha1);\n \tconst char *path = sha1_pack_name(sha1);\n \tstruct packed_git *p = alloc_packed_git(strlen(path) + 1);\n \n+\tif (!idx_path)\n+\t\tidx_path = sha1_pack_index_name(sha1);\n+\n \tstrcpy(p->pack_name, path);\n \thashcpy(p->sha1, sha1);\n \tif (check_packed_git_idx(idx_path, p)) {\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex 1a4dfc9..fc675b5 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -77,6 +77,21 @@ test_expect_success 'fetch notices corrupt pack' '\n \t)\n '\n \n+test_expect_success 'fetch notices corrupt idx' '\n+\tcp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t p=`ls objects/pack/pack-*.idx` &&\n+\t chmod u+w $p &&\n+\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n+\t) &&\n+\tmkdir repo_bad2.git &&\n+\t(cd repo_bad2.git &&\n+\t git --bare init &&\n+\t test_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad2.git &&\n+\t test 0 = `ls objects/pack | wc -l`\n+\t)\n+'\n+\n test_expect_success 'did not use upload-pack service' '\n \tgrep '/git-upload-pack' <\"$HTTPD_ROOT_PATH\"/access.log >act\n \t: >exp\n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139648","messageId":"20100416100307.0000423f@unknown","threadId":"23475","inReplyTo":"1271366704-25262-2-git-send-email-spearce@spearce.org","subject":"Re: [PATCH v2 6/6] http-fetch: Use temporary files for pack-*.idx until verified","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2010-04-16T02:03:07Z","receivedAt":"2010-04-16T02:03:07Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi\n\nOn Fri, Apr 16, 2010 at 5:25 AM, Shawn O. Pearce wrote:\n> [snip]\n> diff --git a/pack-check.c b/pack-check.c\n> index 166ca70..9baba12 100644\n> --- a/pack-check.c\n> +++ b/pack-check.c\n> @@ -133,14 +133,13 @@ static int verify_packfile(struct packed_git *p,\n>        return err;\n>  }\n>\n> -int verify_pack(struct packed_git *p)\n> +int verify_pack_index(struct packed_git *p)\n>  {\n>        off_t index_size;\n>        const unsigned char *index_base;\n>        git_SHA_CTX ctx;\n>        unsigned char sha1[20];\n>        int err = 0;\n> -       struct pack_window *w_curs = NULL;\n>\n>        if (open_pack_index(p))\n>                return error(\"packfile %s index not opened\", p->pack_name);\n> @@ -154,9 +153,17 @@ int verify_pack(struct packed_git *p)\n>        if (hashcmp(sha1, index_base + index_size - 20))\n>                err = error(\"Packfile index for %s SHA1 mismatch\",\n>                            p->pack_name);\n> +       return err;\n> +}\n> +\n> +int verify_pack(struct packed_git *p)\n> +{\n> +       int err = 0;\n> +       struct pack_window *w_curs = NULL;\n>\n> -       /* Verify pack file */\n> -       err |= verify_packfile(p, &w_curs);\n> +       err |= verify_pack_index(p);\n> +       if (!err)\n> +               err |= verify_packfile(p, &w_curs);\n>        unuse_pack(&w_curs);\n>\n>        return err;\n> diff --git a/pack.h b/pack.h\n> index d268c01..bb27576 100644\n> --- a/pack.h\n> +++ b/pack.h\n> @@ -57,6 +57,7 @@ struct pack_idx_entry {\n>\n>  extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1);\n>  extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);\n> +extern int verify_pack_index(struct packed_git *);\n>  extern int verify_pack(struct packed_git *);\n>  extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);\n>  extern char *index_pack_lockfile(int fd);\n\nThese should probably go into a separate patch.\n\n> diff --git a/http.c b/http.c\n> index aa3e380..2d88034 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -897,47 +897,65 @@ int http_fetch_ref(const char *base, struct ref *ref)\n>  }\n>\n>  /* Helpers for fetching packs */\n> -static int fetch_pack_index(unsigned char *sha1, const char *base_url)\n> +static char *fetch_pack_index(unsigned char *sha1, const char *base_url)\n>  {\n> -       int ret = 0;\n> -       char *hex = xstrdup(sha1_to_hex(sha1));\n> [snip]\n>        if (http_is_verbose)\n> -               fprintf(stderr, \"Getting index for pack %s\\n\", hex);\n> +               fprintf(stderr, \"Getting index for pack %s\\n\", sha1_to_hex(sha1));\n>\n>        end_url_with_slash(&buf, base_url);\n> -       strbuf_addf(&buf, \"objects/pack/pack-%s.idx\", hex);\n> +       strbuf_addf(&buf, \"objects/pack/pack-%s.idx\", sha1_to_hex(sha1));\n>        url = strbuf_detach(&buf, NULL);\n\nI think the replacing of \"hex\" with \"sha1_to_hex(sha1)\" is unrelated.\n\n> -       if (has_pack_index(sha1)) {\n> -               ret = 0;\n> -               goto cleanup;\n> -       }\n> -\n\nIt probably should be mentioned in the commit message or elsewhere that\nas fetch_and_setup_pack_index() now checks for the pack index locally\nbefore fetching, we no longer need this check.\n\n>  static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n>        unsigned char *sha1, const char *base_url)\n>  {\n> [snip]\n> +               ret = verify_pack_index(new_pack);\n> +               if (!ret) {\n> +                       close_pack_index(new_pack);\n> +                       ret = move_temp_to_file(tmp_idx, sha1_pack_index_name(sha1));\n> +               }\n> +               free(tmp_idx);\n> +               if (ret)\n> +                       return -1;\n\nThe conflation of \"ret\" as the result of both verify_pack_index() and\nmove_temp_to_file() is pretty confusing.\n\nAlso, perhaps the below could be squashed in to reduce if()'s on tmp_idx.\n\n-- >8 --\ndiff --git a/http.c b/http.c\nindex 6b7b899..e5bb54a 100644\n--- a/http.c\n+++ b/http.c\n@@ -945,35 +945,37 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n {\n \tstruct packed_git *new_pack;\n \tchar *tmp_idx = NULL;\n+\tint ret;\n \n-\tif (!has_pack_index(sha1)) {\n-\t\ttmp_idx = fetch_pack_index(sha1, base_url);\n-\t\tif (!tmp_idx)\n-\t\t\treturn -1;\n+\tif (has_pack_index(sha1)) {\n+\t\tnew_pack = parse_pack_index(sha1, NULL);\n+\t\tif (!new_pack)\n+\t\t\treturn -1; /* parse_pack_index() already issued error message */\n+\t\tgoto add_pack;\n \t}\n \n+\ttmp_idx = fetch_pack_index(sha1, base_url);\n+\tif (!tmp_idx)\n+\t\treturn -1;\n+\n \tnew_pack = parse_pack_index(sha1, tmp_idx);\n \tif (!new_pack) {\n-\t\tif (tmp_idx) {\n-\t\t\tunlink(tmp_idx);\n-\t\t\tfree(tmp_idx);\n-\t\t}\n+\t\tunlink(tmp_idx);\n+\t\tfree(tmp_idx);\n+\n \t\treturn -1; /* parse_pack_index() already issued error message */\n \t}\n \n-\tif (tmp_idx) {\n-\t\tint ret;\n-\n-\t\tret = verify_pack_index(new_pack);\n-\t\tif (!ret) {\n-\t\t\tclose_pack_index(new_pack);\n-\t\t\tret = move_temp_to_file(tmp_idx, sha1_pack_index_name(sha1));\n-\t\t}\n-\t\tfree(tmp_idx);\n-\t\tif (ret)\n-\t\t\treturn -1;\n+\tret = verify_pack_index(new_pack);\n+\tif (!ret) {\n+\t\tclose_pack_index(new_pack);\n+\t\tret = move_temp_to_file(tmp_idx, sha1_pack_index_name(sha1));\n \t}\n+\tfree(tmp_idx);\n+\tif (ret)\n+\t\treturn -1;\n \n+add_pack:\n \tnew_pack->next = *packs_head;\n \t*packs_head = new_pack;\n \treturn 0;\n\n\n-- \nCheers,\nRay Chuan\n"},{"id":"139649","messageId":"g2sbe6fef0d1004151955g6fa785c0id852c91c78584b06@mail.gmail.com","threadId":"23475","inReplyTo":"1271366704-25262-1-git-send-email-spearce@spearce.org","subject":"Re: [PATCH v2 5/6] http-fetch: Use index-pack rather than verify-pack to check packs","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2010-04-16T02:55:08Z","receivedAt":"2010-04-16T02:55:08Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Fri, Apr 16, 2010 at 5:25 AM, Shawn O. Pearce wrote:\n> By recreating the index file locally, we also can automatically\n> upgrade from a v1 pack table of contents to v2.  This makes the\n> CRC32 data available for use during later repacks, even if the\n> server didn't have them on hand.\n\nThis is exceedingly interesting.\n\n> diff --git a/cache.h b/cache.h\n> index 5eb0573..4150603 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -916,6 +916,7 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\n>\n>  extern void pack_report(void);\n>  extern int open_pack_index(struct packed_git *);\n> +extern void close_pack_index(struct packed_git *);\n>  extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *);\n>  extern void close_pack_windows(struct packed_git *);\n>  extern void unuse_pack(struct pack_window **);\n> [snip]\n> diff --git a/sha1_file.c b/sha1_file.c\n> index ff65328..820063e 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -599,6 +599,14 @@ void unuse_pack(struct pack_window **w_cursor)\n>        }\n>  }\n>\n> +void close_pack_index(struct packed_git *p)\n> +{\n> +       if (p->index_data) {\n> +               munmap((void *)p->index_data, p->index_size);\n> +               p->index_data = NULL;\n> +       }\n> +}\n> +\n>  /*\n>  * This is used by git-repack in case a newly created pack happens to\n>  * contain the same set of objects as an existing one.  In that case\n> @@ -620,8 +628,7 @@ void free_pack_by_name(const char *pack_name)\n>                        close_pack_windows(p);\n>                        if (p->pack_fd != -1)\n>                                close(p->pack_fd);\n> -                       if (p->index_data)\n> -                               munmap((void *)p->index_data, p->index_size);\n> +                       close_pack_index(p);\n>                        free(p->bad_object_sha1);\n>                        *pp = p->next;\n>                        free(p);\n\nPerhaps these could go into a separate patch.\n\n> diff --git a/http.c b/http.c\n> index 64e0c18..aa3e380 100644\n> --- a/http.c\n> +++ b/http.c\n> [snip]\n> @@ -1014,13 +1017,37 @@ int finish_http_pack_request(struct http_pack_request *preq)\n>                lst = &((*lst)->next);\n>        *lst = (*lst)->next;\n>\n> -       ret = move_temp_to_file(preq->tmpfile, preq->filename);\n> [snip]\n> +       if (move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1))\n> [snip]\n> @@ -1043,7 +1070,6 @@ struct http_pack_request *new_http_pack_request(\n>        preq->url = strbuf_detach(&buf, NULL);\n>\n>        filename = sha1_pack_name(target->sha1);\n> -       snprintf(preq->filename, sizeof(preq->filename), \"%s\", filename);\n>        snprintf(preq->tmpfile, sizeof(preq->tmpfile), \"%s.temp\", filename);\n>        preq->packfile = fopen(preq->tmpfile, \"a\");\n>        if (!preq->packfile) {\n> [snip]\n> diff --git a/http.h b/http.h\n> index 5c9441c..e4a8126 100644\n> --- a/http.h\n> +++ b/http.h\n> @@ -152,7 +152,6 @@ struct http_pack_request\n>        struct packed_git *target;\n>        struct packed_git **lst;\n>        FILE *packfile;\n> -       char filename[PATH_MAX];\n>        char tmpfile[PATH_MAX];\n>        struct curl_slist *range_header;\n>        struct active_request_slot *slot;\n\nWhy this change? Just curious, nothing strong against it.\n\n> +       tmp_idx = xstrdup(preq->tmpfile);\n> +       strcpy(tmp_idx + strlen(tmp_idx) - strlen(\".pack.temp\"),\n> +              \".idx.temp\");\n\nCould we use a strbuf here?\n\n> [snip]\n> +       if (move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1))\n> +        || move_temp_to_file(tmp_idx, sha1_pack_index_name(p->sha1))) {\n\nHmm, when moving the pack index file, should we unlink() the old,\ndownloaded one first?\n\n-- \nCheers,\nRay Chuan\n"},{"id":"139767","messageId":"7v8w8m2c9r.fsf@alter.siamese.dyndns.org","threadId":"23475","inReplyTo":"1271358560-8946-1-git-send-email-spearce@spearce.org","subject":"Re: [PATCH 0/6] detect dumb HTTP pack file corruption","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-17T17:56:48Z","receivedAt":"2010-04-17T17:56:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hmph, I am getting failures from \"[index v2] 6 verify-pack detects CRC\nmismatch\" in t5302 when this is applied to 'maint' (or when the result is\nmerged to 'master').\n"},{"id":"139772","messageId":"20100417191107.GA15911@spearce.org","threadId":"23475","inReplyTo":"7v8w8m2c9r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/6] detect dumb HTTP pack file corruption","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-17T19:11:07Z","receivedAt":"2010-04-17T19:11:07Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> Hmph, I am getting failures from \"[index v2] 6 verify-pack detects CRC\n> mismatch\" in t5302 when this is applied to 'maint' (or when the result is\n> merged to 'master').\n\nOops. Well, I need to respin the series anyway to address Tay\nRay Chuan's comments. Clearly I failed to run the full test suite\nbefore sending this series. I promise to run the full suite before\nresending. :-)\n\n-- \nShawn.\n"},{"id":"139773","messageId":"20100417193010.GB15911@spearce.org","threadId":"23475","inReplyTo":"g2sbe6fef0d1004151955g6fa785c0id852c91c78584b06@mail.gmail.com","subject":"Re: [PATCH v2 5/6] http-fetch: Use index-pack rather than verify-pack to check packs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-17T19:30:10Z","receivedAt":"2010-04-17T19:30:10Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Tay Ray Chuan wrote:\n> > @@ -152,7 +152,6 @@ struct http_pack_request\n> >        struct packed_git *target;\n> >        struct packed_git **lst;\n> >        FILE *packfile;\n> > -       char filename[PATH_MAX];\n> \n> Why this change? Just curious, nothing strong against it.\n\nSplit into a new patch. I'll share my reasons in the commit message.\n:-)\n \n> > +       tmp_idx = xstrdup(preq->tmpfile);\n> > +       strcpy(tmp_idx + strlen(tmp_idx) - strlen(\".pack.temp\"),\n> > +              \".idx.temp\");\n> \n> Could we use a strbuf here?\n\nDoesn't seem worth it. I just started trying to rework this with a\nstrbuf and I just don't see any benefit here. We know the tmpfile\nends with \".pack.temp\" when we created this request structure. So\na strdup and overwrite of the tail just works.\n \n> > +       if (move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1))\n> > +        || move_temp_to_file(tmp_idx, sha1_pack_index_name(p->sha1))) {\n> \n> Hmm, when moving the pack index file, should we unlink() the old,\n> downloaded one first?\n\nYup, good point, thanks.\n\n-- \nShawn.\n"},{"id":"139776","messageId":"1271534864-31944-1-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100416100307.0000423f@unknown","subject":"[PATCH v3 01/11] http.c: Remove bad free of static block","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-17T20:07:34Z","receivedAt":"2010-04-17T20:07:34Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"The filename variable here is pointing to a block of memory that\nwas allocated by sha1_file.c and is also held in a static variable\nscoped within the sha1_pack_name() function. Doing a free() here is\nreturning that memory to the allocator while we might still try to\nreuse it on a subsequent sha1_pack_name() invocation. That's not\nacceptable, so don't free it.\n\nSigned-off-by: Shawn O. Pearce \n---\n http.c | 1 -\n 1 files changed, 0 insertions(+), 1 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 4814217..f26625e 100644\n--- a/http.c\n+++ b/http.c\n@@ -1082,7 +1082,6 @@ struct http_pack_request *new_http_pack_request(\n \treturn preq;\n \n abort:\n-\tfree(filename);\n \tfree(preq->url);\n \tfree(preq);\n \treturn NULL;\n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139783","messageId":"1271534864-31944-2-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100416100307.0000423f@unknown","subject":"[PATCH v3 02/11] t5550-http-fetch: Use subshell for repository operations","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-17T20:07:35Z","receivedAt":"2010-04-17T20:07:35Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Change into the server repository's directory using a subshell,\nso we can return back to the top of the trash directory before\ndoing anything more in the test script.\n\nSigned-off-by: Shawn O. Pearce \n---\n t/t5550-http-fetch.sh | 7 ++++---\n 1 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex 8cfce96..78c31c9 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -55,9 +55,10 @@ test_expect_success 'http remote detects correct HEAD' '\n \n test_expect_success 'fetch packed objects' '\n \tcp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n-\tcd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n-\tgit --bare repack &&\n-\tgit --bare prune-packed &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n+\t git --bare repack &&\n+\t git --bare prune-packed\n+\t) &&\n \tgit clone $HTTPD_URL/dumb/repo_pack.git\n '\n \n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139782","messageId":"1271534864-31944-3-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100416100307.0000423f@unknown","subject":"[PATCH v3 03/11] http.c: Tiny refactoring of finish_http_pack_request","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-17T20:07:36Z","receivedAt":"2010-04-17T20:07:36Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Always remove the struct packed_git from the active list, even\nif the rename of the temporary file fails.\n\nWhile we are here, simplify the code a bit by using a common\nlocal variable name (\"p\") to hold the relevant packed_git.\n\nSigned-off-by: Shawn O. Pearce \n---\n http.c | 16 ++++++++--------\n 1 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex f26625e..4558f11 100644\n--- a/http.c\n+++ b/http.c\n@@ -1002,8 +1002,9 @@ int finish_http_pack_request(struct http_pack_request *preq)\n {\n \tint ret;\n \tstruct packed_git **lst;\n+\tstruct packed_git *p = preq->target;\n \n-\tpreq->target->pack_size = ftell(preq->packfile);\n+\tp->pack_size = ftell(preq->packfile);\n \n \tif (preq->packfile != NULL) {\n \t\tfclose(preq->packfile);\n@@ -1011,18 +1012,17 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \t\tpreq->slot->local = NULL;\n \t}\n \n-\tret = move_temp_to_file(preq->tmpfile, preq->filename);\n-\tif (ret)\n-\t\treturn ret;\n-\n \tlst = preq->lst;\n-\twhile (*lst != preq->target)\n+\twhile (*lst != p)\n \t\tlst = &((*lst)->next);\n \t*lst = (*lst)->next;\n \n-\tif (verify_pack(preq->target))\n+\tret = move_temp_to_file(preq->tmpfile, preq->filename);\n+\tif (ret)\n+\t\treturn ret;\n+\tif (verify_pack(p))\n \t\treturn -1;\n-\tinstall_packed_git(preq->target);\n+\tinstall_packed_git(p);\n \n \treturn 0;\n }\n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139777","messageId":"1271534864-31944-4-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100416100307.0000423f@unknown","subject":"[PATCH v3 04/11] http.c: Drop useless != NULL test in finish_http_pack_request","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-17T20:07:37Z","receivedAt":"2010-04-17T20:07:37Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"The test preq->packfile != NULL is always true. If packfile was\nactually NULL when entering this function the ftell() above would\ncrash out with a SIGSEGV, resulting in never reaching this point.\n\nSimplify the code by just removing the conditional.\n\nSigned-off-by: Shawn O. Pearce \n---\n http.c | 9 +++------\n 1 files changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 4558f11..64e0c18 100644\n--- a/http.c\n+++ b/http.c\n@@ -1005,12 +1005,9 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \tstruct packed_git *p = preq->target;\n \n \tp->pack_size = ftell(preq->packfile);\n-\n-\tif (preq->packfile != NULL) {\n-\t\tfclose(preq->packfile);\n-\t\tpreq->packfile = NULL;\n-\t\tpreq->slot->local = NULL;\n-\t}\n+\tfclose(preq->packfile);\n+\tpreq->packfile = NULL;\n+\tpreq->slot->local = NULL;\n \n \tlst = preq->lst;\n \twhile (*lst != p)\n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139784","messageId":"1271534864-31944-5-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100416100307.0000423f@unknown","subject":"[PATCH v3 05/11] http.c: Don't store destination name in request structures","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-17T20:07:38Z","receivedAt":"2010-04-17T20:07:38Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"The destination name within the object store is easily computed\non demand, reusing a static buffer held by sha1_file.c. We don't\nneed to copy the entire path into the request structure for safe\nkeeping, when it can be easily reformatted after the download has\nbeen completed.\n\nThis reduces the size of the per-request structure, and removes\nyet another PATH_MAX based limit.\n\nSigned-off-by: Shawn O. Pearce \n---\n http-walker.c | 2 +-\n http.c | 14 ++++++--------\n http.h | 2 --\n 3 files changed, 7 insertions(+), 11 deletions(-)\n\ndiff --git a/http-walker.c b/http-walker.c\nindex ef99ae6..8ca76d0 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -510,7 +510,7 @@ static int fetch_object(struct walker *walker, struct alt_base *repo, unsigned c\n \t\tret = error(\"File %s has bad hash\", hex);\n \t} else if (req->rename < 0) {\n \t\tret = error(\"unable to write sha1 filename %s\",\n-\t\t\t req->filename);\n+\t\t\t sha1_file_name(req->sha1));\n \t}\n \n \trelease_http_object_request(req);\ndiff --git a/http.c b/http.c\nindex 64e0c18..c75eb95 100644\n--- a/http.c\n+++ b/http.c\n@@ -1014,7 +1014,7 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \t\tlst = &((*lst)->next);\n \t*lst = (*lst)->next;\n \n-\tret = move_temp_to_file(preq->tmpfile, preq->filename);\n+\tret = move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1));\n \tif (ret)\n \t\treturn ret;\n \tif (verify_pack(p))\n@@ -1043,7 +1043,6 @@ struct http_pack_request *new_http_pack_request(\n \tpreq->url = strbuf_detach(&buf, NULL);\n \n \tfilename = sha1_pack_name(target->sha1);\n-\tsnprintf(preq->filename, sizeof(preq->filename), \"%s\", filename);\n \tsnprintf(preq->tmpfile, sizeof(preq->tmpfile), \"%s.temp\", filename);\n \tpreq->packfile = fopen(preq->tmpfile, \"a\");\n \tif (!preq->packfile) {\n@@ -1133,7 +1132,6 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \tfreq->localfile = -1;\n \n \tfilename = sha1_file_name(sha1);\n-\tsnprintf(freq->filename, sizeof(freq->filename), \"%s\", filename);\n \tsnprintf(freq->tmpfile, sizeof(freq->tmpfile),\n \t\t \"%s.temp\", filename);\n \n@@ -1162,8 +1160,8 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \t}\n \n \tif (freq->localfile < 0) {\n-\t\terror(\"Couldn't create temporary file %s for %s: %s\",\n-\t\t freq->tmpfile, freq->filename, strerror(errno));\n+\t\terror(\"Couldn't create temporary file %s: %s\",\n+\t\t freq->tmpfile, strerror(errno));\n \t\tgoto abort;\n \t}\n \n@@ -1210,8 +1208,8 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \t\t\tprev_posn = 0;\n \t\t\tlseek(freq->localfile, 0, SEEK_SET);\n \t\t\tif (ftruncate(freq->localfile, 0) < 0) {\n-\t\t\t\terror(\"Couldn't truncate temporary file %s for %s: %s\",\n-\t\t\t\t\t freq->tmpfile, freq->filename, strerror(errno));\n+\t\t\t\terror(\"Couldn't truncate temporary file %s: %s\",\n+\t\t\t\t\t freq->tmpfile, strerror(errno));\n \t\t\t\tgoto abort;\n \t\t\t}\n \t\t}\n@@ -1287,7 +1285,7 @@ int finish_http_object_request(struct http_object_request *freq)\n \t\treturn -1;\n \t}\n \tfreq->rename =\n-\t\tmove_temp_to_file(freq->tmpfile, freq->filename);\n+\t\tmove_temp_to_file(freq->tmpfile, sha1_file_name(freq->sha1));\n \n \treturn freq->rename;\n }\ndiff --git a/http.h b/http.h\nindex 5c9441c..84bdbd0 100644\n--- a/http.h\n+++ b/http.h\n@@ -152,7 +152,6 @@ struct http_pack_request\n \tstruct packed_git *target;\n \tstruct packed_git **lst;\n \tFILE *packfile;\n-\tchar filename[PATH_MAX];\n \tchar tmpfile[PATH_MAX];\n \tstruct curl_slist *range_header;\n \tstruct active_request_slot *slot;\n@@ -167,7 +166,6 @@ extern void release_http_pack_request(struct http_pack_request *preq);\n struct http_object_request\n {\n \tchar *url;\n-\tchar filename[PATH_MAX];\n \tchar tmpfile[PATH_MAX];\n \tint localfile;\n \tCURLcode curl_result;\n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139785","messageId":"1271534864-31944-6-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100416100307.0000423f@unknown","subject":"[PATCH v3 06/11] http.c: Remove unnecessary strdup of sha1_to_hex result","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-17T20:07:39Z","receivedAt":"2010-04-17T20:07:39Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Most of the time the dumb HTTP transport is run without the verbose\nflag set, so we only need the result of sha1_to_hex(sha1) once, to\nconstruct the pack URL. Don't bother with an unnecessary malloc,\ncopy, free chain of this buffer.\n\nIf verbose is set, we'll format the SHA-1 twice now. But this\ntiny extra CPU time spent is nothing compared to the slowdown that\nis usually imposed by the verbose messages being sent to the tty,\nand its entirely trivial compared to the latency involved with the\nremote HTTP server sending something as big as a pack file.\n\nSigned-off-by: Shawn O. Pearce \n---\n http.c | 6 ++----\n 1 files changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex c75eb95..1a52740 100644\n--- a/http.c\n+++ b/http.c\n@@ -899,7 +899,6 @@ int http_fetch_ref(const char *base, struct ref *ref)\n static int fetch_pack_index(unsigned char *sha1, const char *base_url)\n {\n \tint ret = 0;\n-\tchar *hex = xstrdup(sha1_to_hex(sha1));\n \tchar *filename;\n \tchar *url = NULL;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -910,10 +909,10 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)\n \t}\n \n \tif (http_is_verbose)\n-\t\tfprintf(stderr, \"Getting index for pack %s\\n\", hex);\n+\t\tfprintf(stderr, \"Getting index for pack %s\\n\", sha1_to_hex(sha1));\n \n \tend_url_with_slash(&buf, base_url);\n-\tstrbuf_addf(&buf, \"objects/pack/pack-%s.idx\", hex);\n+\tstrbuf_addf(&buf, \"objects/pack/pack-%s.idx\", sha1_to_hex(sha1));\n \turl = strbuf_detach(&buf, NULL);\n \n \tfilename = sha1_pack_index_name(sha1);\n@@ -921,7 +920,6 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)\n \t\tret = error(\"Unable to get pack index %s\\n\", url);\n \n cleanup:\n-\tfree(hex);\n \tfree(url);\n \treturn ret;\n }\n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139779","messageId":"1271534864-31944-7-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100416100307.0000423f@unknown","subject":"[PATCH v3 07/11] Introduce close_pack_index to permit replacement","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-17T20:07:40Z","receivedAt":"2010-04-17T20:07:40Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"By closing the pack index, a caller can later overwrite the index\nwith an updated index file, possibly after converting from v1 to\nthe v2 format. Because p->index_data is NULL after close, on the\nnext access the index will be opened again and the other members\nwill be updated with new data.\n\nSigned-off-by: Shawn O. Pearce \n---\n cache.h | 1 +\n sha1_file.c | 11 +++++++++--\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 5eb0573..4150603 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -916,6 +916,7 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\n \n extern void pack_report(void);\n extern int open_pack_index(struct packed_git *);\n+extern void close_pack_index(struct packed_git *);\n extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *);\n extern void close_pack_windows(struct packed_git *);\n extern void unuse_pack(struct pack_window **);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex ff65328..820063e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -599,6 +599,14 @@ void unuse_pack(struct pack_window **w_cursor)\n \t}\n }\n \n+void close_pack_index(struct packed_git *p)\n+{\n+\tif (p->index_data) {\n+\t\tmunmap((void *)p->index_data, p->index_size);\n+\t\tp->index_data = NULL;\n+\t}\n+}\n+\n /*\n * This is used by git-repack in case a newly created pack happens to\n * contain the same set of objects as an existing one. In that case\n@@ -620,8 +628,7 @@ void free_pack_by_name(const char *pack_name)\n \t\t\tclose_pack_windows(p);\n \t\t\tif (p->pack_fd != -1)\n \t\t\t\tclose(p->pack_fd);\n-\t\t\tif (p->index_data)\n-\t\t\t\tmunmap((void *)p->index_data, p->index_size);\n+\t\t\tclose_pack_index(p);\n \t\t\tfree(p->bad_object_sha1);\n \t\t\t*pp = p->next;\n \t\t\tfree(p);\n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139778","messageId":"1271534864-31944-8-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100416100307.0000423f@unknown","subject":"[PATCH v3 08/11] Extract verify_pack_index for reuse from verify_pack","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-17T20:07:41Z","receivedAt":"2010-04-17T20:07:41Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"The dumb HTTP transport should verify an index is completely valid\nbefore trying to use it. That requires checking the header/footer\nbut also checking the complete content SHA-1. All of this logic is\nalready in the front half of verify_pack, so pull it out into a new\nfunction that can be reused.\n\nSigned-off-by: Shawn O. Pearce \n---\n pack-check.c | 15 ++++++++++++---\n pack.h | 1 +\n 2 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/pack-check.c b/pack-check.c\nindex 166ca70..395fb95 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -133,14 +133,13 @@ static int verify_packfile(struct packed_git *p,\n \treturn err;\n }\n \n-int verify_pack(struct packed_git *p)\n+int verify_pack_index(struct packed_git *p)\n {\n \toff_t index_size;\n \tconst unsigned char *index_base;\n \tgit_SHA_CTX ctx;\n \tunsigned char sha1[20];\n \tint err = 0;\n-\tstruct pack_window *w_curs = NULL;\n \n \tif (open_pack_index(p))\n \t\treturn error(\"packfile %s index not opened\", p->pack_name);\n@@ -154,8 +153,18 @@ int verify_pack(struct packed_git *p)\n \tif (hashcmp(sha1, index_base + index_size - 20))\n \t\terr = error(\"Packfile index for %s SHA1 mismatch\",\n \t\t\t p->pack_name);\n+\treturn err;\n+}\n+\n+int verify_pack(struct packed_git *p)\n+{\n+\tint err = 0;\n+\tstruct pack_window *w_curs = NULL;\n+\n+\terr |= verify_pack_index(p);\n+\tif (!p->index_data)\n+\t\treturn -1;\n \n-\t/* Verify pack file */\n \terr |= verify_packfile(p, &w_curs);\n \tunuse_pack(&w_curs);\n \ndiff --git a/pack.h b/pack.h\nindex d268c01..bb27576 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -57,6 +57,7 @@ struct pack_idx_entry {\n \n extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1);\n extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);\n+extern int verify_pack_index(struct packed_git *);\n extern int verify_pack(struct packed_git *);\n extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);\n extern char *index_pack_lockfile(int fd);\n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139781","messageId":"1271534864-31944-9-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100416100307.0000423f@unknown","subject":"[PATCH v3 09/11] Allow parse_pack_index on temporary files","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-17T20:07:42Z","receivedAt":"2010-04-17T20:07:42Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"The easiest way to verify a pack index is to open it through the\nstandard parse_pack_index function, permitting the header check\nto happen when the file is mapped. However, the dumb HTTP client\nneeds to verify a pack index before its moved into its proper file\nname within the objects/pack directory, to prevent a corrupt index\nfrom being made available. So permit the caller to specify the\nexact path of the index file.\n\nFor now we're still using the final destination name within the\nsole call site in http.c, but eventually we will start to parse\nthe temporary path instead.\n\nSigned-off-by: Shawn O. Pearce \n---\n cache.h | 2 +-\n http.c | 2 +-\n sha1_file.c | 3 +--\n 3 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 4150603..0d101e4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -905,7 +905,7 @@ struct extra_have_objects {\n extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags, struct extra_have_objects *);\n extern int server_supports(const char *feature);\n \n-extern struct packed_git *parse_pack_index(unsigned char *sha1);\n+extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \n extern void prepare_packed_git(void);\n extern void reprepare_packed_git(void);\ndiff --git a/http.c b/http.c\nindex 1a52740..9f3dfc1 100644\n--- a/http.c\n+++ b/http.c\n@@ -932,7 +932,7 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n \tif (fetch_pack_index(sha1, base_url))\n \t\treturn -1;\n \n-\tnew_pack = parse_pack_index(sha1);\n+\tnew_pack = parse_pack_index(sha1, sha1_pack_index_name(sha1));\n \tif (!new_pack)\n \t\treturn -1; /* parse_pack_index() already issued error message */\n \tnew_pack->next = *packs_head;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 820063e..74bba79 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -838,9 +838,8 @@ struct packed_git *add_packed_git(const char *path, int path_len, int local)\n \treturn p;\n }\n \n-struct packed_git *parse_pack_index(unsigned char *sha1)\n+struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)\n {\n-\tconst char *idx_path = sha1_pack_index_name(sha1);\n \tconst char *path = sha1_pack_name(sha1);\n \tstruct packed_git *p = alloc_packed_git(strlen(path) + 1);\n \n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139786","messageId":"1271534864-31944-10-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100416100307.0000423f@unknown","subject":"[PATCH v3 10/11] http-fetch: Use index-pack rather than verify-pack to check packs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-17T20:07:43Z","receivedAt":"2010-04-17T20:07:43Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"To ensure we don't leave a corrupt pack file positioned as though\nit were a valid pack file, run index-pack on the temporary pack\nbefore we rename it to its final name. If index-pack crashes out\nwhen it discovers file corruption (e.g. GitHub's error HTML at the\nend of the file), simply delete the temporary files to cleanup.\n\nBy waiting until the pack has been validated before we move it\nto its final name, we eliminate a race condition where another\nconcurrent reader might try to access the pack at the same time\nthat we are still trying to verify its not corrupt.\n\nSwitching from verify-pack to index-pack is a change in behavior,\nbut it should turn out better for users. The index-pack algorithm\ntries to minimize disk seeks, as well as the number of times any\ngiven object is inflated, by organizing its work along delta chains.\nThe verify-pack logic does not attempt to do this, thrashing the\ndelta base cache and the filesystem cache.\n\nBy recreating the index file locally, we also can automatically\nupgrade from a v1 pack table of contents to v2. This makes the\nCRC32 data available for use during later repacks, even if the\nserver didn't have them on hand.\n\nSigned-off-by: Shawn O. Pearce \n---\n http.c | 43 ++++++++++++++++++++++++++++++++++++-------\n t/t5550-http-fetch.sh | 15 +++++++++++++++\n 2 files changed, 51 insertions(+), 7 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 9f3dfc1..a7352b7 100644\n--- a/http.c\n+++ b/http.c\n@@ -1,6 +1,7 @@\n #include \"http.h\"\n #include \"pack.h\"\n #include \"sideband.h\"\n+#include \"run-command.h\"\n \n int data_received;\n int active_requests;\n@@ -998,11 +999,15 @@ void release_http_pack_request(struct http_pack_request *preq)\n \n int finish_http_pack_request(struct http_pack_request *preq)\n {\n-\tint ret;\n \tstruct packed_git **lst;\n \tstruct packed_git *p = preq->target;\n+\tchar *tmp_idx;\n+\tstruct child_process ip;\n+\tconst char *ip_argv[8];\n+\n+\tclose_pack_index(p);\n+\tunlink(sha1_pack_index_name(p->sha1));\n \n-\tp->pack_size = ftell(preq->packfile);\n \tfclose(preq->packfile);\n \tpreq->packfile = NULL;\n \tpreq->slot->local = NULL;\n@@ -1012,13 +1017,37 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \t\tlst = &((*lst)->next);\n \t*lst = (*lst)->next;\n \n-\tret = move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1));\n-\tif (ret)\n-\t\treturn ret;\n-\tif (verify_pack(p))\n+\ttmp_idx = xstrdup(preq->tmpfile);\n+\tstrcpy(tmp_idx + strlen(tmp_idx) - strlen(\".pack.temp\"),\n+\t \".idx.temp\");\n+\n+\tip_argv[0] = \"index-pack\";\n+\tip_argv[1] = \"-o\";\n+\tip_argv[2] = tmp_idx;\n+\tip_argv[3] = preq->tmpfile;\n+\tip_argv[4] = NULL;\n+\n+\tmemset(&ip, 0, sizeof(ip));\n+\tip.argv = ip_argv;\n+\tip.git_cmd = 1;\n+\tip.no_stdin = 1;\n+\tip.no_stdout = 1;\n+\n+\tif (run_command(&ip)) {\n+\t\tunlink(preq->tmpfile);\n+\t\tunlink(tmp_idx);\n+\t\tfree(tmp_idx);\n \t\treturn -1;\n-\tinstall_packed_git(p);\n+\t}\n \n+\tif (move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1))\n+\t || move_temp_to_file(tmp_idx, sha1_pack_index_name(p->sha1))) {\n+\t\tfree(tmp_idx);\n+\t\treturn -1;\n+\t}\n+\n+\tinstall_packed_git(p);\n+\tfree(tmp_idx);\n \treturn 0;\n }\n \ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex 78c31c9..1a4dfc9 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -62,6 +62,21 @@ test_expect_success 'fetch packed objects' '\n \tgit clone $HTTPD_URL/dumb/repo_pack.git\n '\n \n+test_expect_success 'fetch notices corrupt pack' '\n+\tcp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n+\t p=`ls objects/pack/pack-*.pack` &&\n+\t chmod u+w $p &&\n+\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n+\t) &&\n+\tmkdir repo_bad1.git &&\n+\t(cd repo_bad1.git &&\n+\t git --bare init &&\n+\t test_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad1.git &&\n+\t test 0 = `ls objects/pack/pack-*.pack | wc -l`\n+\t)\n+'\n+\n test_expect_success 'did not use upload-pack service' '\n \tgrep '/git-upload-pack' <\"$HTTPD_ROOT_PATH\"/access.log >act\n \t: >exp\n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139780","messageId":"1271534864-31944-11-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100416100307.0000423f@unknown","subject":"[PATCH v3 11/11] http-fetch: Use temporary files for pack-*.idx until verified","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-17T20:07:44Z","receivedAt":"2010-04-17T20:07:44Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Verify that a downloaded pack-*.idx file is consistent and valid\nas an index file before we rename it into its final destination.\nThis prevents a corrupt index file from later being treated as a\nusable file, confusing readers.\n\nSigned-off-by: Shawn O. Pearce \n---\n http.c | 56 ++++++++++++++++++++++++++++++++++--------------\n t/t5550-http-fetch.sh | 15 +++++++++++++\n 2 files changed, 54 insertions(+), 17 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex a7352b7..9d68d02 100644\n--- a/http.c\n+++ b/http.c\n@@ -897,18 +897,11 @@ int http_fetch_ref(const char *base, struct ref *ref)\n }\n \n /* Helpers for fetching packs */\n-static int fetch_pack_index(unsigned char *sha1, const char *base_url)\n+static char *fetch_pack_index(unsigned char *sha1, const char *base_url)\n {\n-\tint ret = 0;\n-\tchar *filename;\n-\tchar *url = NULL;\n+\tchar *url, *tmp;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tif (has_pack_index(sha1)) {\n-\t\tret = 0;\n-\t\tgoto cleanup;\n-\t}\n-\n \tif (http_is_verbose)\n \t\tfprintf(stderr, \"Getting index for pack %s\\n\", sha1_to_hex(sha1));\n \n@@ -916,26 +909,55 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)\n \tstrbuf_addf(&buf, \"objects/pack/pack-%s.idx\", sha1_to_hex(sha1));\n \turl = strbuf_detach(&buf, NULL);\n \n-\tfilename = sha1_pack_index_name(sha1);\n-\tif (http_get_file(url, filename, 0) != HTTP_OK)\n-\t\tret = error(\"Unable to get pack index %s\\n\", url);\n+\tstrbuf_addf(&buf, \"%s.temp\", sha1_pack_index_name(sha1));\n+\ttmp = strbuf_detach(&buf, NULL);\n+\n+\tif (http_get_file(url, tmp, 0) != HTTP_OK) {\n+\t\terror(\"Unable to get pack index %s\\n\", url);\n+\t\tfree(tmp);\n+\t\ttmp = NULL;\n+\t}\n \n-cleanup:\n \tfree(url);\n-\treturn ret;\n+\treturn tmp;\n }\n \n static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n \tunsigned char *sha1, const char *base_url)\n {\n \tstruct packed_git *new_pack;\n+\tchar *tmp_idx = NULL;\n+\tint ret;\n+\n+\tif (has_pack_index(sha1)) {\n+\t\tnew_pack = parse_pack_index(sha1, NULL);\n+\t\tif (!new_pack)\n+\t\t\treturn -1; /* parse_pack_index() already issued error message */\n+\t\tgoto add_pack;\n+\t}\n \n-\tif (fetch_pack_index(sha1, base_url))\n+\ttmp_idx = fetch_pack_index(sha1, base_url);\n+\tif (!tmp_idx)\n \t\treturn -1;\n \n-\tnew_pack = parse_pack_index(sha1, sha1_pack_index_name(sha1));\n-\tif (!new_pack)\n+\tnew_pack = parse_pack_index(sha1, tmp_idx);\n+\tif (!new_pack) {\n+\t\tunlink(tmp_idx);\n+\t\tfree(tmp_idx);\n+\n \t\treturn -1; /* parse_pack_index() already issued error message */\n+\t}\n+\n+\tret = verify_pack_index(new_pack);\n+\tif (!ret) {\n+\t\tclose_pack_index(new_pack);\n+\t\tret = move_temp_to_file(tmp_idx, sha1_pack_index_name(sha1));\n+\t}\n+\tfree(tmp_idx);\n+\tif (ret)\n+\t\treturn -1;\n+\n+add_pack:\n \tnew_pack->next = *packs_head;\n \t*packs_head = new_pack;\n \treturn 0;\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex 1a4dfc9..fc675b5 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -77,6 +77,21 @@ test_expect_success 'fetch notices corrupt pack' '\n \t)\n '\n \n+test_expect_success 'fetch notices corrupt idx' '\n+\tcp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t p=`ls objects/pack/pack-*.idx` &&\n+\t chmod u+w $p &&\n+\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n+\t) &&\n+\tmkdir repo_bad2.git &&\n+\t(cd repo_bad2.git &&\n+\t git --bare init &&\n+\t test_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad2.git &&\n+\t test 0 = `ls objects/pack | wc -l`\n+\t)\n+'\n+\n test_expect_success 'did not use upload-pack service' '\n \tgrep '/git-upload-pack' <\"$HTTPD_ROOT_PATH\"/access.log >act\n \t: >exp\n-- \n1.7.1.rc1.269.ga27c7\n"},{"id":"139807","messageId":"20100418110716.000050e5@unknown","threadId":"23475","inReplyTo":"1271534864-31944-10-git-send-email-spearce@spearce.org","subject":"Re: [PATCH v3 10/11] http-fetch: Use index-pack rather than verify-pack to check packs","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2010-04-18T03:07:16Z","receivedAt":"2010-04-18T03:07:16Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sat, 17 Apr 2010 13:07:43 -0700\n\"Shawn O. Pearce\" wrote:\n\n> diff --git a/http.c b/http.c\n> index 9f3dfc1..a7352b7 100644\n> --- a/http.c\n> +++ b/http.c\n[snip]\n> @@ -998,11 +999,15 @@ void release_http_pack_request(struct http_pack_request *preq)\n> \n> int finish_http_pack_request(struct http_pack_request *preq)\n> {\n[snip]\n> +\tunlink(sha1_pack_index_name(p->sha1));\n\nI think this should be done later, after we have run index-pack\nsuccessfully. A good place would be probably after the if() block here:\n\n> +\tif (run_command(&ip)) {\n> +\t\tunlink(preq->tmpfile);\n> +\t\tunlink(tmp_idx);\n> +\t\tfree(tmp_idx);\n> \t\treturn -1;\n> -\tinstall_packed_git(p);\n> +\t}\n\n-- \nCheers,\nRay Chuan\n"},{"id":"139808","messageId":"20100418111404.00000794@unknown","threadId":"23475","inReplyTo":"1271534864-31944-6-git-send-email-spearce@spearce.org","subject":"Re: [PATCH v3 06/11] http.c: Remove unnecessary strdup of sha1_to_hex result","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2010-04-18T03:14:04Z","receivedAt":"2010-04-18T03:14:04Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Sat, 17 Apr 2010 13:07:39 -0700\n\"Shawn O. Pearce\" wrote:\n\n> and its entirely trivial compared to the latency involved with the\n\nMinor nit: s/its/is/, ie. \"is entirely trivial\".\n\nJunio, perhaps you could squash that in?\n\n> Signed-off-by: Shawn O. Pearce \n\nAcked-by: Tay Ray Chuan \n\n-- \nCheers,\nRay Chuan\n"},{"id":"139809","messageId":"20100418113619.00007e39@unknown","threadId":"23475","inReplyTo":"1271534864-31944-5-git-send-email-spearce@spearce.org","subject":"Re: [PATCH v3 05/11] http.c: Don't store destination name in request structures","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2010-04-18T03:36:19Z","receivedAt":"2010-04-18T03:36:19Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sat, 17 Apr 2010 13:07:38 -0700\n\"Shawn O. Pearce\" wrote:\n\n> The destination name within the object store is easily computed\n> on demand, reusing a static buffer held by sha1_file.c. We don't\n> need to copy the entire path into the request structure for safe\n> keeping, when it can be easily reformatted after the download has\n> been completed.\n> \n> This reduces the size of the per-request structure, and removes\n> yet another PATH_MAX based limit.\n> \n> Signed-off-by: Shawn O. Pearce \n\nnow that there's a single user of char *filename, we might as well do\naway with it.\n\nPS. I think having the below as a separate patch is better than\nsquashing it in, as it might be detrimental to patch #05's readability\nin the latter case.\n\n-->8--\nFrom: Tay Ray Chuan \nSubject: [PATCH] http.c::new_http_pack_request: do away with the temp variable filename\n\nNow that the temporary variable char *filename is only used in one\nplace, do away with it and just call sha1_pack_name() directly.\n\nSigned-off-by: Tay Ray Chuan \n\ndiff --git a/http.c b/http.c\nindex c75eb95..110cff9 100644\n--- a/http.c\n+++ b/http.c\n@@ -1027,7 +1027,6 @@ int finish_http_pack_request(struct http_pack_request *preq)\n struct http_pack_request *new_http_pack_request(\n struct packed_git *target, const char *base_url)\n {\n- char *filename;\n long prev_posn = 0;\n char range[RANGE_HEADER_SIZE];\n struct strbuf buf = STRBUF_INIT;\n@@ -1042,8 +1041,8 @@ struct http_pack_request *new_http_pack_request(\n sha1_to_hex(target->sha1));\n preq->url = strbuf_detach(&buf, NULL);\n \n- filename = sha1_pack_name(target->sha1);\n- snprintf(preq->tmpfile, sizeof(preq->tmpfile), \"%s.temp\", filename);\n+ snprintf(preq->tmpfile, sizeof(preq->tmpfile), \"%s.temp\",\n+ sha1_pack_name(target->sha1));\n preq->packfile = fopen(preq->tmpfile, \"a\");\n if (!preq->packfile) {\n error(\"Unable to open local file %s for pack\",\n--\n\n-- \nCheers,\nRay Chuan\n"},{"id":"139810","messageId":"20100418115744.0000238b@unknown","threadId":"23475","inReplyTo":"1271534864-31944-11-git-send-email-spearce@spearce.org","subject":"Re: [PATCH v3 11/11] http-fetch: Use temporary files for pack-*.idx until verified","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2010-04-18T03:57:44Z","receivedAt":"2010-04-18T03:57:44Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sat, 17 Apr 2010 13:07:44 -0700\n\"Shawn O. Pearce\" wrote:\n\n> Verify that a downloaded pack-*.idx file is consistent and valid\n> as an index file before we rename it into its final destination.\n> This prevents a corrupt index file from later being treated as a\n> usable file, confusing readers.\n\nPerhaps this should be added in:\n\n Check that we do not have the pack index file before invoking\n fetch_and_setup_pack_index(); that way, we can do without the\n has_pack_index() check in fetch_and_setup_pack_index().\n\nThe above was referring to this hunk:\n\n> diff --git a/http.c b/http.c\n[snip]\n> -\tif (has_pack_index(sha1)) {\n> -\t\tret = 0;\n> -\t\tgoto cleanup;\n> -\t}\n> -\n\n-- \nCheers,\nRay Chuan\n"},{"id":"139898","messageId":"1271686990-16363-1-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100418115744.0000238b@unknown","subject":"[PATCH v4 00/11] Resend sp/maint-dumb-http-pack-reidx","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-19T14:23:04Z","receivedAt":"2010-04-19T14:23:04Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"This is a resend of the last half of the series, from patch 6/11\nto the end, to address some minor review comments.\n\nJunio, I think you need to reset my branch to 0da8b2e7c80a6d\n(\"http.c: Don't store destination name in structures\"), and\nthen apply this group.\n\nTotal series diff since v3 is this shocking one line change, most\nof the edits were to commit messages:\n\ndiff --git a/http.c b/http.c\nindex 83f6047..0813c9e 100644\n--- a/http.c\n+++ b/http.c\n@@ -1028,7 +1028,6 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \tconst char *ip_argv[8];\n \n \tclose_pack_index(p);\n-\tunlink(sha1_pack_index_name(p->sha1));\n \n \tfclose(preq->packfile);\n \tpreq->packfile = NULL;\n@@ -1062,6 +1061,8 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \t\treturn -1;\n \t}\n \n+\tunlink(sha1_pack_index_name(p->sha1));\n+\n \tif (move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1))\n \t || move_temp_to_file(tmp_idx, sha1_pack_index_name(p->sha1))) {\n \t\tfree(tmp_idx);\n"},{"id":"139904","messageId":"1271686990-16363-2-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100418115744.0000238b@unknown","subject":"[PATCH v4 06/11] http.c: Remove unnecessary strdup of sha1_to_hex result","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-19T14:23:05Z","receivedAt":"2010-04-19T14:23:05Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Most of the time the dumb HTTP transport is run without the verbose\nflag set, so we only need the result of sha1_to_hex(sha1) once, to\nconstruct the pack URL. Don't bother with an unnecessary malloc,\ncopy, free chain of this buffer.\n\nIf verbose is set, we'll format the SHA-1 twice now. But this\ntiny extra CPU time spent is nothing compared to the slowdown that\nis usually imposed by the verbose messages being sent to the tty,\nand is entirely trivial compared to the latency involved with the\nremote HTTP server sending something as big as a pack file.\n\nSigned-off-by: Shawn O. Pearce \nAcked-by: Tay Ray Chuan \nSigned-off-by: Junio C Hamano \n---\n\n Message fixed, Acked-by added.\n \n No code change from v3.\n\n http.c | 6 ++----\n 1 files changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 7ee1ba5..95e3b8b 100644\n--- a/http.c\n+++ b/http.c\n@@ -899,7 +899,6 @@ int http_fetch_ref(const char *base, struct ref *ref)\n static int fetch_pack_index(unsigned char *sha1, const char *base_url)\n {\n \tint ret = 0;\n-\tchar *hex = xstrdup(sha1_to_hex(sha1));\n \tchar *filename;\n \tchar *url = NULL;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -910,10 +909,10 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)\n \t}\n \n \tif (http_is_verbose)\n-\t\tfprintf(stderr, \"Getting index for pack %s\\n\", hex);\n+\t\tfprintf(stderr, \"Getting index for pack %s\\n\", sha1_to_hex(sha1));\n \n \tend_url_with_slash(&buf, base_url);\n-\tstrbuf_addf(&buf, \"objects/pack/pack-%s.idx\", hex);\n+\tstrbuf_addf(&buf, \"objects/pack/pack-%s.idx\", sha1_to_hex(sha1));\n \turl = strbuf_detach(&buf, NULL);\n \n \tfilename = sha1_pack_index_name(sha1);\n@@ -921,7 +920,6 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)\n \t\tret = error(\"Unable to get pack index %s\\n\", url);\n \n cleanup:\n-\tfree(hex);\n \tfree(url);\n \treturn ret;\n }\n-- \n1.7.1.rc1.279.g22727\n"},{"id":"139899","messageId":"1271686990-16363-3-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100418115744.0000238b@unknown","subject":"[PATCH v4 07/11] Introduce close_pack_index to permit replacement","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-19T14:23:06Z","receivedAt":"2010-04-19T14:23:06Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"By closing the pack index, a caller can later overwrite the index\nwith an updated index file, possibly after converting from v1 to\nthe v2 format. Because p->index_data is NULL after close, on the\nnext access the index will be opened again and the other members\nwill be updated with new data.\n\nSigned-off-by: Shawn O. Pearce \nSigned-off-by: Junio C Hamano \n---\n\n No change from v3.\n\n cache.h | 1 +\n sha1_file.c | 11 +++++++++--\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 6e54993..0eba039 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -911,6 +911,7 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\n \n extern void pack_report(void);\n extern int open_pack_index(struct packed_git *);\n+extern void close_pack_index(struct packed_git *);\n extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *);\n extern void close_pack_windows(struct packed_git *);\n extern void unuse_pack(struct pack_window **);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex c23cc5e..4e82654 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -606,6 +606,14 @@ void unuse_pack(struct pack_window **w_cursor)\n \t}\n }\n \n+void close_pack_index(struct packed_git *p)\n+{\n+\tif (p->index_data) {\n+\t\tmunmap((void *)p->index_data, p->index_size);\n+\t\tp->index_data = NULL;\n+\t}\n+}\n+\n /*\n * This is used by git-repack in case a newly created pack happens to\n * contain the same set of objects as an existing one. In that case\n@@ -627,8 +635,7 @@ void free_pack_by_name(const char *pack_name)\n \t\t\tclose_pack_windows(p);\n \t\t\tif (p->pack_fd != -1)\n \t\t\t\tclose(p->pack_fd);\n-\t\t\tif (p->index_data)\n-\t\t\t\tmunmap((void *)p->index_data, p->index_size);\n+\t\t\tclose_pack_index(p);\n \t\t\tfree(p->bad_object_sha1);\n \t\t\t*pp = p->next;\n \t\t\tfree(p);\n-- \n1.7.1.rc1.279.g22727\n"},{"id":"139901","messageId":"1271686990-16363-4-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100418115744.0000238b@unknown","subject":"[PATCH v4 08/11] Extract verify_pack_index for reuse from verify_pack","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-19T14:23:07Z","receivedAt":"2010-04-19T14:23:07Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"The dumb HTTP transport should verify an index is completely valid\nbefore trying to use it. That requires checking the header/footer\nbut also checking the complete content SHA-1. All of this logic is\nalready in the front half of verify_pack, so pull it out into a new\nfunction that can be reused.\n\nSigned-off-by: Shawn O. Pearce \nSigned-off-by: Junio C Hamano \n---\n\n No change from v3.\n\n pack-check.c | 15 ++++++++++++---\n pack.h | 1 +\n 2 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/pack-check.c b/pack-check.c\nindex 166ca70..395fb95 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -133,14 +133,13 @@ static int verify_packfile(struct packed_git *p,\n \treturn err;\n }\n \n-int verify_pack(struct packed_git *p)\n+int verify_pack_index(struct packed_git *p)\n {\n \toff_t index_size;\n \tconst unsigned char *index_base;\n \tgit_SHA_CTX ctx;\n \tunsigned char sha1[20];\n \tint err = 0;\n-\tstruct pack_window *w_curs = NULL;\n \n \tif (open_pack_index(p))\n \t\treturn error(\"packfile %s index not opened\", p->pack_name);\n@@ -154,8 +153,18 @@ int verify_pack(struct packed_git *p)\n \tif (hashcmp(sha1, index_base + index_size - 20))\n \t\terr = error(\"Packfile index for %s SHA1 mismatch\",\n \t\t\t p->pack_name);\n+\treturn err;\n+}\n+\n+int verify_pack(struct packed_git *p)\n+{\n+\tint err = 0;\n+\tstruct pack_window *w_curs = NULL;\n+\n+\terr |= verify_pack_index(p);\n+\tif (!p->index_data)\n+\t\treturn -1;\n \n-\t/* Verify pack file */\n \terr |= verify_packfile(p, &w_curs);\n \tunuse_pack(&w_curs);\n \ndiff --git a/pack.h b/pack.h\nindex b759a23..880f9c2 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -57,6 +57,7 @@ struct pack_idx_entry {\n \n extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1);\n extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);\n+extern int verify_pack_index(struct packed_git *);\n extern int verify_pack(struct packed_git *);\n extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);\n extern char *index_pack_lockfile(int fd);\n-- \n1.7.1.rc1.279.g22727\n"},{"id":"139900","messageId":"1271686990-16363-5-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100418115744.0000238b@unknown","subject":"[PATCH v4 09/11] Allow parse_pack_index on temporary files","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-19T14:23:08Z","receivedAt":"2010-04-19T14:23:08Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"The easiest way to verify a pack index is to open it through the\nstandard parse_pack_index function, permitting the header check\nto happen when the file is mapped. However, the dumb HTTP client\nneeds to verify a pack index before its moved into its proper file\nname within the objects/pack directory, to prevent a corrupt index\nfrom being made available. So permit the caller to specify the\nexact path of the index file.\n\nFor now we're still using the final destination name within the\nsole call site in http.c, but eventually we will start to parse\nthe temporary path instead.\n\nSigned-off-by: Shawn O. Pearce \nSigned-off-by: Junio C Hamano \n---\n\n No change from v3.\n\n cache.h | 2 +-\n http.c | 2 +-\n sha1_file.c | 3 +--\n 3 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 0eba039..7db23ef 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -900,7 +900,7 @@ struct extra_have_objects {\n extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags, struct extra_have_objects *);\n extern int server_supports(const char *feature);\n \n-extern struct packed_git *parse_pack_index(unsigned char *sha1);\n+extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \n extern void prepare_packed_git(void);\n extern void reprepare_packed_git(void);\ndiff --git a/http.c b/http.c\nindex 95e3b8b..9c62632 100644\n--- a/http.c\n+++ b/http.c\n@@ -932,7 +932,7 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n \tif (fetch_pack_index(sha1, base_url))\n \t\treturn -1;\n \n-\tnew_pack = parse_pack_index(sha1);\n+\tnew_pack = parse_pack_index(sha1, sha1_pack_index_name(sha1));\n \tif (!new_pack)\n \t\treturn -1; /* parse_pack_index() already issued error message */\n \tnew_pack->next = *packs_head;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4e82654..9f3f514 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -845,9 +845,8 @@ struct packed_git *add_packed_git(const char *path, int path_len, int local)\n \treturn p;\n }\n \n-struct packed_git *parse_pack_index(unsigned char *sha1)\n+struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)\n {\n-\tconst char *idx_path = sha1_pack_index_name(sha1);\n \tconst char *path = sha1_pack_name(sha1);\n \tstruct packed_git *p = alloc_packed_git(strlen(path) + 1);\n \n-- \n1.7.1.rc1.279.g22727\n"},{"id":"139903","messageId":"1271686990-16363-6-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100418115744.0000238b@unknown","subject":"[PATCH v4 10/11] http-fetch: Use index-pack rather than verify-pack to check packs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-19T14:23:09Z","receivedAt":"2010-04-19T14:23:09Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"To ensure we don't leave a corrupt pack file positioned as though\nit were a valid pack file, run index-pack on the temporary pack\nbefore we rename it to its final name. If index-pack crashes out\nwhen it discovers file corruption (e.g. GitHub's error HTML at the\nend of the file), simply delete the temporary files to cleanup.\n\nBy waiting until the pack has been validated before we move it\nto its final name, we eliminate a race condition where another\nconcurrent reader might try to access the pack at the same time\nthat we are still trying to verify its not corrupt.\n\nSwitching from verify-pack to index-pack is a change in behavior,\nbut it should turn out better for users. The index-pack algorithm\ntries to minimize disk seeks, as well as the number of times any\ngiven object is inflated, by organizing its work along delta chains.\nThe verify-pack logic does not attempt to do this, thrashing the\ndelta base cache and the filesystem cache.\n\nBy recreating the index file locally, we also can automatically\nupgrade from a v1 pack table of contents to v2. This makes the\nCRC32 data available for use during later repacks, even if the\nserver didn't have them on hand.\n\nSigned-off-by: Shawn O. Pearce \n---\n\n Moved unlink of index to after the index-pack is successful,\n per Tay Ray Chuan's request.\n\n Removed Junio SOB line since the logic changed.\n\n http.c | 44 +++++++++++++++++++++++++++++++++++++-------\n t/t5550-http-fetch.sh | 15 +++++++++++++++\n 2 files changed, 52 insertions(+), 7 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 9c62632..2ebd679 100644\n--- a/http.c\n+++ b/http.c\n@@ -1,6 +1,7 @@\n #include \"http.h\"\n #include \"pack.h\"\n #include \"sideband.h\"\n+#include \"run-command.h\"\n \n int data_received;\n int active_requests;\n@@ -998,11 +999,14 @@ void release_http_pack_request(struct http_pack_request *preq)\n \n int finish_http_pack_request(struct http_pack_request *preq)\n {\n-\tint ret;\n \tstruct packed_git **lst;\n \tstruct packed_git *p = preq->target;\n+\tchar *tmp_idx;\n+\tstruct child_process ip;\n+\tconst char *ip_argv[8];\n+\n+\tclose_pack_index(p);\n \n-\tp->pack_size = ftell(preq->packfile);\n \tfclose(preq->packfile);\n \tpreq->packfile = NULL;\n \tpreq->slot->local = NULL;\n@@ -1012,13 +1016,39 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \t\tlst = &((*lst)->next);\n \t*lst = (*lst)->next;\n \n-\tret = move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1));\n-\tif (ret)\n-\t\treturn ret;\n-\tif (verify_pack(p))\n+\ttmp_idx = xstrdup(preq->tmpfile);\n+\tstrcpy(tmp_idx + strlen(tmp_idx) - strlen(\".pack.temp\"),\n+\t \".idx.temp\");\n+\n+\tip_argv[0] = \"index-pack\";\n+\tip_argv[1] = \"-o\";\n+\tip_argv[2] = tmp_idx;\n+\tip_argv[3] = preq->tmpfile;\n+\tip_argv[4] = NULL;\n+\n+\tmemset(&ip, 0, sizeof(ip));\n+\tip.argv = ip_argv;\n+\tip.git_cmd = 1;\n+\tip.no_stdin = 1;\n+\tip.no_stdout = 1;\n+\n+\tif (run_command(&ip)) {\n+\t\tunlink(preq->tmpfile);\n+\t\tunlink(tmp_idx);\n+\t\tfree(tmp_idx);\n \t\treturn -1;\n-\tinstall_packed_git(p);\n+\t}\n+\n+\tunlink(sha1_pack_index_name(p->sha1));\n \n+\tif (move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1))\n+\t || move_temp_to_file(tmp_idx, sha1_pack_index_name(p->sha1))) {\n+\t\tfree(tmp_idx);\n+\t\treturn -1;\n+\t}\n+\n+\tinstall_packed_git(p);\n+\tfree(tmp_idx);\n \treturn 0;\n }\n \ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex 78c31c9..1a4dfc9 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -62,6 +62,21 @@ test_expect_success 'fetch packed objects' '\n \tgit clone $HTTPD_URL/dumb/repo_pack.git\n '\n \n+test_expect_success 'fetch notices corrupt pack' '\n+\tcp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n+\t p=`ls objects/pack/pack-*.pack` &&\n+\t chmod u+w $p &&\n+\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n+\t) &&\n+\tmkdir repo_bad1.git &&\n+\t(cd repo_bad1.git &&\n+\t git --bare init &&\n+\t test_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad1.git &&\n+\t test 0 = `ls objects/pack/pack-*.pack | wc -l`\n+\t)\n+'\n+\n test_expect_success 'did not use upload-pack service' '\n \tgrep '/git-upload-pack' <\"$HTTPD_ROOT_PATH\"/access.log >act\n \t: >exp\n-- \n1.7.1.rc1.279.g22727\n"},{"id":"139902","messageId":"1271686990-16363-7-git-send-email-spearce@spearce.org","threadId":"23475","inReplyTo":"20100418115744.0000238b@unknown","subject":"[PATCH v4 11/11] http-fetch: Use temporary files for pack-*.idx until verified","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-19T14:23:10Z","receivedAt":"2010-04-19T14:23:10Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Verify that a downloaded pack-*.idx file is consistent and valid\nas an index file before we rename it into its final destination.\nThis prevents a corrupt index file from later being treated as a\nusable file, confusing readers.\n\nCheck that we do not have the pack index file before invoking\nfetch_pack_index(); that way, we can do without the has_pack_index()\ncheck in fetch_pack_index().\n\nSigned-off-by: Shawn O. Pearce \nSigned-off-by: Junio C Hamano \n---\n\n Added paragraph describing the move of has_pack_index() to the\n commit message.\n \n No code change from v3.\n\n http.c | 56 ++++++++++++++++++++++++++++++++++--------------\n t/t5550-http-fetch.sh | 15 +++++++++++++\n 2 files changed, 54 insertions(+), 17 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 2ebd679..0813c9e 100644\n--- a/http.c\n+++ b/http.c\n@@ -897,18 +897,11 @@ int http_fetch_ref(const char *base, struct ref *ref)\n }\n \n /* Helpers for fetching packs */\n-static int fetch_pack_index(unsigned char *sha1, const char *base_url)\n+static char *fetch_pack_index(unsigned char *sha1, const char *base_url)\n {\n-\tint ret = 0;\n-\tchar *filename;\n-\tchar *url = NULL;\n+\tchar *url, *tmp;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tif (has_pack_index(sha1)) {\n-\t\tret = 0;\n-\t\tgoto cleanup;\n-\t}\n-\n \tif (http_is_verbose)\n \t\tfprintf(stderr, \"Getting index for pack %s\\n\", sha1_to_hex(sha1));\n \n@@ -916,26 +909,55 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)\n \tstrbuf_addf(&buf, \"objects/pack/pack-%s.idx\", sha1_to_hex(sha1));\n \turl = strbuf_detach(&buf, NULL);\n \n-\tfilename = sha1_pack_index_name(sha1);\n-\tif (http_get_file(url, filename, 0) != HTTP_OK)\n-\t\tret = error(\"Unable to get pack index %s\\n\", url);\n+\tstrbuf_addf(&buf, \"%s.temp\", sha1_pack_index_name(sha1));\n+\ttmp = strbuf_detach(&buf, NULL);\n+\n+\tif (http_get_file(url, tmp, 0) != HTTP_OK) {\n+\t\terror(\"Unable to get pack index %s\\n\", url);\n+\t\tfree(tmp);\n+\t\ttmp = NULL;\n+\t}\n \n-cleanup:\n \tfree(url);\n-\treturn ret;\n+\treturn tmp;\n }\n \n static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n \tunsigned char *sha1, const char *base_url)\n {\n \tstruct packed_git *new_pack;\n+\tchar *tmp_idx = NULL;\n+\tint ret;\n+\n+\tif (has_pack_index(sha1)) {\n+\t\tnew_pack = parse_pack_index(sha1, NULL);\n+\t\tif (!new_pack)\n+\t\t\treturn -1; /* parse_pack_index() already issued error message */\n+\t\tgoto add_pack;\n+\t}\n \n-\tif (fetch_pack_index(sha1, base_url))\n+\ttmp_idx = fetch_pack_index(sha1, base_url);\n+\tif (!tmp_idx)\n \t\treturn -1;\n \n-\tnew_pack = parse_pack_index(sha1, sha1_pack_index_name(sha1));\n-\tif (!new_pack)\n+\tnew_pack = parse_pack_index(sha1, tmp_idx);\n+\tif (!new_pack) {\n+\t\tunlink(tmp_idx);\n+\t\tfree(tmp_idx);\n+\n \t\treturn -1; /* parse_pack_index() already issued error message */\n+\t}\n+\n+\tret = verify_pack_index(new_pack);\n+\tif (!ret) {\n+\t\tclose_pack_index(new_pack);\n+\t\tret = move_temp_to_file(tmp_idx, sha1_pack_index_name(sha1));\n+\t}\n+\tfree(tmp_idx);\n+\tif (ret)\n+\t\treturn -1;\n+\n+add_pack:\n \tnew_pack->next = *packs_head;\n \t*packs_head = new_pack;\n \treturn 0;\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex 1a4dfc9..fc675b5 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -77,6 +77,21 @@ test_expect_success 'fetch notices corrupt pack' '\n \t)\n '\n \n+test_expect_success 'fetch notices corrupt idx' '\n+\tcp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t p=`ls objects/pack/pack-*.idx` &&\n+\t chmod u+w $p &&\n+\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n+\t) &&\n+\tmkdir repo_bad2.git &&\n+\t(cd repo_bad2.git &&\n+\t git --bare init &&\n+\t test_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad2.git &&\n+\t test 0 = `ls objects/pack | wc -l`\n+\t)\n+'\n+\n test_expect_success 'did not use upload-pack service' '\n \tgrep '/git-upload-pack' <\"$HTTPD_ROOT_PATH\"/access.log >act\n \t: >exp\n-- \n1.7.1.rc1.279.g22727\n"},{"id":"139905","messageId":"y2zbe6fef0d1004190735q1b2e69f0ja30c7badce49f63b@mail.gmail.com","threadId":"23475","inReplyTo":"1271686990-16363-6-git-send-email-spearce@spearce.org","subject":"Re: [PATCH v4 10/11] http-fetch: Use index-pack rather than verify-pack to check packs","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2010-04-19T14:35:39Z","receivedAt":"2010-04-19T14:35:39Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Mon, Apr 19, 2010 at 10:23 PM, Shawn O. Pearce wrote:\n> Signed-off-by: Shawn O. Pearce \n> ---\n>\n>  Moved unlink of index to after the index-pack is successful,\n>  per Tay Ray Chuan's request.\n\nLooks good.\n\nAcked-by: Tay Ray Chuan \n\n-- \nCheers,\nRay Chuan\n"},{"id":"139906","messageId":"20100419224643.00001ff1@unknown","threadId":"23475","inReplyTo":"1271686990-16363-1-git-send-email-spearce@spearce.org","subject":"Re: [PATCH v4 00/11] Resend sp/maint-dumb-http-pack-reidx","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2010-04-19T14:46:43Z","receivedAt":"2010-04-19T14:46:43Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Mon, 19 Apr 2010 07:23:04 -0700\n\"Shawn O. Pearce\" wrote:\n\n> This is a resend of the last half of the series, from patch 6/11\n> to the end, to address some minor review comments.\n> \n> Junio, I think you need to reset my branch to 0da8b2e7c80a6d\n> (\"http.c: Don't store destination name in structures\"), and\n> then apply this group.\n\nthe small patch below could also be applied to the rebased topic branch.\n\n-->8--\nFrom: Tay Ray Chuan \nSubject: [PATCH] http.c::new_http_pack_request: do away with the temp variable filename\n\nNow that the temporary variable char *filename is only used in one\nplace, do away with it and just call sha1_pack_name() directly.\n\nSigned-off-by: Tay Ray Chuan \n\ndiff --git a/http.c b/http.c\nindex c75eb95..110cff9 100644\n--- a/http.c\n+++ b/http.c\n@@ -1027,7 +1027,6 @@ int finish_http_pack_request(struct http_pack_request *preq)\n struct http_pack_request *new_http_pack_request(\n struct packed_git *target, const char *base_url)\n {\n- char *filename;\n long prev_posn = 0;\n char range[RANGE_HEADER_SIZE];\n struct strbuf buf = STRBUF_INIT;\n@@ -1042,8 +1041,8 @@ struct http_pack_request *new_http_pack_request(\n sha1_to_hex(target->sha1));\n preq->url = strbuf_detach(&buf, NULL);\n\n- filename = sha1_pack_name(target->sha1);\n- snprintf(preq->tmpfile, sizeof(preq->tmpfile), \"%s.temp\", filename);\n+ snprintf(preq->tmpfile, sizeof(preq->tmpfile), \"%s.temp\",\n+ sha1_pack_name(target->sha1));\n preq->packfile = fopen(preq->tmpfile, \"a\");\n if (!preq->packfile) {\n error(\"Unable to open local file %s for pack\",\n--\n\n-- \nCheers,\nRay Chuan\n"},{"id":"139907","messageId":"20100419144907.GC4295@spearce.org","threadId":"23475","inReplyTo":"20100419224643.00001ff1@unknown","subject":"Re: [PATCH v4 00/11] Resend sp/maint-dumb-http-pack-reidx","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-19T14:49:07Z","receivedAt":"2010-04-19T14:49:07Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Tay Ray Chuan wrote:\n> the small patch below could also be applied to the rebased topic branch.\n> \n> -->8--\n> From: Tay Ray Chuan \n> Subject: [PATCH] http.c::new_http_pack_request: do away with the temp variable filename\n> \n> Now that the temporary variable char *filename is only used in one\n> place, do away with it and just call sha1_pack_name() directly.\n> \n> Signed-off-by: Tay Ray Chuan \n\nFWIW, Acked-by: Shawn O. Pearce \n\n-- \nShawn.\n"},{"id":"139949","messageId":"u2sbe6fef0d1004192133l3af2f8d6v1eea4c8c02c6a7c5@mail.gmail.com","threadId":"23475","inReplyTo":"20100419144907.GC4295@spearce.org","subject":"Re: [PATCH v4 00/11] Resend sp/maint-dumb-http-pack-reidx","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2010-04-20T04:33:47Z","receivedAt":"2010-04-20T04:33:47Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi Junio,\n\nOn Mon, Apr 19, 2010 at 10:49 PM, Shawn O. Pearce wrote:\n> Tay Ray Chuan wrote:\n>> the small patch below could also be applied to the rebased topic branch.\n>>\n>> -->8--\n>> From: Tay Ray Chuan \n>> Subject: [PATCH] http.c::new_http_pack_request: do away with the temp variable filename\n>>\n>> Now that the temporary variable char *filename is only used in one\n>> place, do away with it and just call sha1_pack_name() directly.\n>>\n>> Signed-off-by: Tay Ray Chuan \n>\n> FWIW, Acked-by: Shawn O. Pearce \n\nI noticed that the inlined patch (in message\n<20100419224643.00001ff1@unknown>) being acked here wasn't applied to\nthe topic branch 'sp/maint-dumb-http-pack-reidx' in pu; just a\nheads-up in case you've missed something.\n\n-- \nCheers,\nRay Chuan\n"}]}