{"thread":{"id":"62370","subject":"Bug report: v2.47.0 cannot fetch version 1 pack indexes","startedAt":"2024-10-19T23:30:49Z","lastAt":"2024-10-28T00:14:43Z","messageCount":31,"participants":["fox","Eric Sunshine","Jeff King","Taylor Blau"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"505524","messageId":"BA07EFA0-0793-420D-BED9-ACD7CEBE0112@townlong-yak.com","threadId":"62370","inReplyTo":null,"subject":"Bug report: v2.47.0 cannot fetch version 1 pack indexes","fromName":"fox","fromEmail":"fox.gbr@townlong-yak.com","sentAt":"2024-10-19T23:22:34Z","receivedAt":"2024-10-19T23:30:49Z","isPatch":false,"sender":{"key":"fox.gbr@townlong-yak.com","avatar":null},"body":"What did you do before the bug happened? (Steps to reproduce your issue)\n\n1. Run git clone https://www.townlong-yak.com/test.git\n\n\nWhat did you expect to happen? (Expected behavior)\n\nA local \"test\" folder to be created with a clone of the remote repository.\n\n\nWhat happened instead? (Actual behavior)\n\nThe command produces the following output:\nCloning into 'test'...\nerror: files '/Users/me/test/.git/objects/pack/tmp_idx_WT81vv' and '/Users/me/test/.git/objects/pack/pack-427331d91391b00844273eeb3879cb479ce2c995.idx' differ in contents\nfatal: unable to rename temporary '*.idx' file to '/Users/me/test/.git/objects/pack/pack-427331d91391b00844273eeb3879cb479ce2c995.idx'\nerror: Unable to find 6261a9d9f7704c02a5421ff733919ab18793aa7d under https://www.townlong-yak.com/test.git\nCannot obtain needed object 6261a9d9f7704c02a5421ff733919ab18793aa7d\nerror: fetch failed.\n\n\nWhat's different between what you expected and what actually happened?\n\nThe clone fails and no test folder exists after the command completes.\n\n\nAnything else you want to add:\n\nThis scenario works correctly with git v2.46.2 and earlier versions, and began failing with v2.47.0.\n\nRunning git-bisect identifies b1b8dfde6929ec9463eca0a858c4adb9786d7c93 as the first bad commit,\nsuggesting that the .idx file downloaded from the remote is now expected to be byte-for-byte\nidentical with a locally-generated version; due to format differences, they are not.\n\nThe remote idx is in the original (version 1) format, and git verify-pack seems satisfied with it.\nDid v2.47.0 intend to block fetching such indices?\n\n[System Info]\ngit version:\ngit version 2.47.0.163.g1226f6d8fa\ncpu: x86_64\nbuilt from commit: 1226f6d8faf60d03cbb3b021c68d48364bf67ac0\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nfeature: fsmonitor--daemon\nlibcurl: 8.7.1\nzlib: 1.2.12\nuname: Darwin 24.0.0 Darwin Kernel Version 24.0.0: Tue Sep 24 23:36:30 PDT 2024; root:xnu-11215.1.12~1/RELEASE_X86_64 x86_64\ncompiler info: clang: 16.0.0 (clang-1600.0.26.3)\nlibc info: no libc information available\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\n\n"},{"id":"505526","messageId":"CAPig+cRPAs85koyaQxN5eaV60-qyGka7HrDMuoMoWjT0CpS6SQ@mail.gmail.com","threadId":"62370","inReplyTo":"BA07EFA0-0793-420D-BED9-ACD7CEBE0112@townlong-yak.com","subject":"Re: Bug report: v2.47.0 cannot fetch version 1 pack indexes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-20T00:37:08Z","receivedAt":"2024-10-20T00:37:21Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Oct 19, 2024 at 7:31 PM fox wrote:\n> What did you do before the bug happened? (Steps to reproduce your issue)\n> 1. Run git clone https://www.townlong-yak.com/test.git\n> Cloning into 'test'...\n> error: files '/Users/me/test/.git/objects/pack/tmp_idx_WT81vv' and '/Users/me/test/.git/objects/pack/pack-427331d91391b00844273eeb3879cb479ce2c995.idx' differ in contents\n> fatal: unable to rename temporary '*.idx' file to '/Users/me/test/.git/objects/pack/pack-427331d91391b00844273eeb3879cb479ce2c995.idx'\n> error: Unable to find 6261a9d9f7704c02a5421ff733919ab18793aa7d under https://www.townlong-yak.com/test.git\n> Cannot obtain needed object 6261a9d9f7704c02a5421ff733919ab18793aa7d\n> error: fetch failed.\n\nI can reproduce this problem.\n\n> Running git-bisect identifies b1b8dfde6929ec9463eca0a858c4adb9786d7c93 as the first bad commit,\n> suggesting that the .idx file downloaded from the remote is now expected to be byte-for-byte\n> identical with a locally-generated version; due to format differences, they are not.\n\nCc:'ing the authors of that commit.\n"},{"id":"505528","messageId":"20241020012455.GA599236@coredump.intra.peff.net","threadId":"62370","inReplyTo":"BA07EFA0-0793-420D-BED9-ACD7CEBE0112@townlong-yak.com","subject":"Re: Bug report: v2.47.0 cannot fetch version 1 pack indexes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-20T01:24:55Z","receivedAt":"2024-10-20T01:24:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 20, 2024 at 01:22:34AM +0200, fox wrote:\n\n> Running git-bisect identifies b1b8dfde6929ec9463eca0a858c4adb9786d7c93\n> as the first bad commit, suggesting that the .idx file downloaded from\n> the remote is now expected to be byte-for-byte identical with a\n> locally-generated version; due to format differences, they are not.\n> \n> The remote idx is in the original (version 1) format, and git\n> verify-pack seems satisfied with it. Did v2.47.0 intend to block\n> fetching such indices?\n\nInteresting case. No, we did not intend to block fetching those indices.\n\nWe knew there was a potential problem with dumb http here, but the idea\nwas that if we had a local file \"foo.idx\" (or \"foo.pack\"), that we'd\nnever try to download the other side in the first place, preferring\nours.\n\nAnd AFAICT that does hold. But here we have the opposite problem! We\ndownload the pack idx from the remote first, so that we know what it\nclaims to have (which tells us whether we want to grab the matching\npack). And then we grab the matching pack, but instead of using the .idx\nwe downloaded, we index it again ourselves.\n\nWhich is probably a good idea to make sure that we prefer our\nlocally-checked copy versus what the other side has. But obviously there\nis no guarantee that we'll come up with the same bytes as the other side\nif there are version or configuration differences.\n\nOne option is to just discard the remote version before re-indexing,\nlike this:\n\ndiff --git a/http.c b/http.c\nindex d59e59f66b..6879d7ba1b 100644\n--- a/http.c\n+++ b/http.c\n@@ -2500,6 +2500,7 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n \tint tmpfile_fd;\n \tint ret = 0;\n+\tsize_t len;\n \n \tfclose(preq->packfile);\n \tpreq->packfile = NULL;\n@@ -2517,6 +2518,18 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \telse\n \t\tip.no_stdout = 1;\n \n+\t/*\n+\t * We're about to reindex the packfile, so throw away the .idx\n+\t * we downloaded from the remote in order to prefer ours.\n+\t */\n+\tif (strip_suffix(preq->tmpfile.buf, \".pack.temp\", &len)) {\n+\t\tstruct strbuf idx = STRBUF_INIT;\n+\t\tstrbuf_add(&idx, preq->tmpfile.buf, len);\n+\t\tstrbuf_addstr(&idx, \".idx\");\n+\t\tunlink(idx.buf);\n+\t\tstrbuf_release(&idx);\n+\t}\n+\n \tif (run_command(&ip)) {\n \t\tret = -1;\n \t\tgoto cleanup;\n\n\nThere are a few things I don't like there, though:\n\n - obviously reversing the tempfile name back to the idx is hacky. We\n could probably store the original idx that led us to this pack and\n use that.\n\n - I don't _think_ there is any case where that .idx file is precious.\n We wouldn't be indexing the .pack file if we didn't just download\n it, and we wouldn't have downloaded it if we already had a\n .idx/.pack pair. OTOH the name we got from the other side isn't\n necessarily the same one we'll use locally; we're feeding the pack\n via \"index-pack --stdin\", so it will do its own hash to come up with\n the name. The other side could have used a different scheme, or even\n be trying to intentionally trick us.\n\nSo I feel like the root of the problem is that we moved the other side's\n\"pack-1234abcd.idx\" into place at all! We do not plan to use it, but\nonly need to access it via our custom packed_git structs to see whether\nwe want to download the matching pack. And in fact I'd suspect there are\nother possible bugs/trickery lurking, if the other side gives us a\nbroken .idx that does not match its pack (our odb would now have the\nbroken file with no matching pack).\n\nSo IMHO a cleaner fix is that we should keep the stuff we download from\nthe remote marked as temporary files, and then throw them away when we\ncomplete the fetch (rather than just expecting index-pack to overwrite\nthem).\n\n-Peff\n"},{"id":"505529","messageId":"20241020024022.GA615104@coredump.intra.peff.net","threadId":"62370","inReplyTo":"20241020012455.GA599236@coredump.intra.peff.net","subject":"Re: Bug report: v2.47.0 cannot fetch version 1 pack indexes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-20T02:40:22Z","receivedAt":"2024-10-20T02:40:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 19, 2024 at 09:24:55PM -0400, Jeff King wrote:\n\n> So I feel like the root of the problem is that we moved the other side's\n> \"pack-1234abcd.idx\" into place at all! We do not plan to use it, but\n> only need to access it via our custom packed_git structs to see whether\n> we want to download the matching pack. And in fact I'd suspect there are\n> other possible bugs/trickery lurking, if the other side gives us a\n> broken .idx that does not match its pack (our odb would now have the\n> broken file with no matching pack).\n> \n> So IMHO a cleaner fix is that we should keep the stuff we download from\n> the remote marked as temporary files, and then throw them away when we\n> complete the fetch (rather than just expecting index-pack to overwrite\n> them).\n\nAnd here's what that might look like. It stores the temporary index\noutside of the pack/ directory, and then we don't \"finalize\" it into\nplace. We have to teach parse_pack to retain the original name, but\nthat's OK. It's used only by the http walker anyway.\n\ndiff --git a/http.c b/http.c\nindex d59e59f66b..6df032e40f 100644\n--- a/http.c\n+++ b/http.c\n@@ -19,6 +19,7 @@\n #include \"string-list.h\"\n #include \"object-file.h\"\n #include \"object-store-ll.h\"\n+#include \"tempfile.h\"\n \n static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);\n static int trace_curl_data = 1;\n@@ -2388,8 +2389,24 @@ static char *fetch_pack_index(unsigned char *hash, const char *base_url)\n \tstrbuf_addf(&buf, \"objects/pack/pack-%s.idx\", hash_to_hex(hash));\n \turl = strbuf_detach(&buf, NULL);\n \n-\tstrbuf_addf(&buf, \"%s.temp\", sha1_pack_index_name(hash));\n-\ttmp = strbuf_detach(&buf, NULL);\n+\t/*\n+\t * Don't put this into packs/, since it's just temporary and we don't\n+\t * want to confuse it with our local .idx files. We'll generate our\n+\t * own index if we choose to download the matching packfile.\n+\t *\n+\t * It's tempting to use xmks_tempfile() here, but it's important that\n+\t * the file not exist, otherwise http_get_file() complains. So we\n+\t * create a filename that should be unique, and then just register it\n+\t * as a tempfile so that it will get cleaned up on exit.\n+\t *\n+\t * Arguably it would be better to hold on to the tempfile handle so\n+\t * that we can remove it as soon as we download the pack and generate\n+\t * the real index, but that might need more surgery.\n+\t */\n+\ttmp = xstrfmt(\"%s/tmp_pack_%s.idx\",\n+\t\t repo_get_object_directory(the_repository),\n+\t\t hash_to_hex(hash));\n+\tregister_tempfile(tmp);\n \n \tif (http_get_file(url, tmp, NULL) != HTTP_OK) {\n \t\terror(\"Unable to get pack index %s\", url);\n@@ -2427,10 +2444,8 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n \t}\n \n \tret = verify_pack_index(new_pack);\n-\tif (!ret) {\n+\tif (!ret)\n \t\tclose_pack_index(new_pack);\n-\t\tret = finalize_object_file(tmp_idx, sha1_pack_index_name(sha1));\n-\t}\n \tfree(tmp_idx);\n \tif (ret)\n \t\treturn -1;\ndiff --git a/packfile.c b/packfile.c\nindex df4ba67719..16d3bcf7f7 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -237,13 +237,22 @@ static struct packed_git *alloc_packed_git(int extra)\n \treturn p;\n }\n \n+static char *pack_path_from_idx(const char *idx_path)\n+{\n+\tsize_t len;\n+\tif (!strip_suffix(idx_path, \".idx\", &len))\n+\t\tBUG(\"idx path does not end in .idx: %s\", idx_path);\n+\treturn xstrfmt(\"%.*s.pack\", (int)len, idx_path);\n+}\n+\n struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)\n {\n-\tconst char *path = sha1_pack_name(sha1);\n+\tchar *path = pack_path_from_idx(idx_path);\n \tsize_t alloc = st_add(strlen(path), 1);\n \tstruct packed_git *p = alloc_packed_git(alloc);\n \n \tmemcpy(p->pack_name, path, alloc); /* includes NUL */\n+\tfree(path);\n \thashcpy(p->hash, sha1, the_repository->hash_algo);\n \tif (check_packed_git_idx(idx_path, p)) {\n \t\tfree(p);\n"},{"id":"505742","messageId":"Zxa0YwXfWt3qWWld@nand.local","threadId":"62370","inReplyTo":"CAPig+cRPAs85koyaQxN5eaV60-qyGka7HrDMuoMoWjT0CpS6SQ@mail.gmail.com","subject":"Re: Bug report: v2.47.0 cannot fetch version 1 pack indexes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-21T20:06:59Z","receivedAt":"2024-10-21T20:07:02Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Oct 19, 2024 at 08:37:08PM -0400, Eric Sunshine wrote:\n> On Sat, Oct 19, 2024 at 7:31 PM fox wrote:\n> > What did you do before the bug happened? (Steps to reproduce your issue)\n> > 1. Run git clone https://www.townlong-yak.com/test.git\n> > Cloning into 'test'...\n> > error: files '/Users/me/test/.git/objects/pack/tmp_idx_WT81vv' and '/Users/me/test/.git/objects/pack/pack-427331d91391b00844273eeb3879cb479ce2c995.idx' differ in contents\n> > fatal: unable to rename temporary '*.idx' file to '/Users/me/test/.git/objects/pack/pack-427331d91391b00844273eeb3879cb479ce2c995.idx'\n> > error: Unable to find 6261a9d9f7704c02a5421ff733919ab18793aa7d under https://www.townlong-yak.com/test.git\n> > Cannot obtain needed object 6261a9d9f7704c02a5421ff733919ab18793aa7d\n> > error: fetch failed.\n>\n> I can reproduce this problem.\n\nThanks for reproducing. This is definitely unintentional breakage, which\nI see that Peff has already diagnosed and proposed a fix for. Let's read\non ...\n\nThanks,\nTaylor\n"},{"id":"505745","messageId":"Zxa4XU+j4+SSmk9c@nand.local","threadId":"62370","inReplyTo":"20241020012455.GA599236@coredump.intra.peff.net","subject":"Re: Bug report: v2.47.0 cannot fetch version 1 pack indexes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-21T20:23:57Z","receivedAt":"2024-10-21T20:24:00Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Oct 19, 2024 at 09:24:55PM -0400, Jeff King wrote:\n> There are a few things I don't like there, though:\n>\n> - obviously reversing the tempfile name back to the idx is hacky. We\n> could probably store the original idx that led us to this pack and\n> use that.\n\nTBH, I don't think that this is all that hacky, but...\n\n> - I don't _think_ there is any case where that .idx file is precious.\n> We wouldn't be indexing the .pack file if we didn't just download\n> it, and we wouldn't have downloaded it if we already had a\n> .idx/.pack pair. OTOH the name we got from the other side isn't\n> necessarily the same one we'll use locally; we're feeding the pack\n> via \"index-pack --stdin\", so it will do its own hash to come up with\n> the name. The other side could have used a different scheme, or even\n> be trying to intentionally trick us.\n>\n> So I feel like the root of the problem is that we moved the other side's\n> \"pack-1234abcd.idx\" into place at all! We do not plan to use it, but\n> only need to access it via our custom packed_git structs to see whether\n> we want to download the matching pack. And in fact I'd suspect there are\n> other possible bugs/trickery lurking, if the other side gives us a\n> broken .idx that does not match its pack (our odb would now have the\n> broken file with no matching pack).\n>\n> So IMHO a cleaner fix is that we should keep the stuff we download from\n> the remote marked as temporary files, and then throw them away when we\n> complete the fetch (rather than just expecting index-pack to overwrite\n> them).\n\n...this seems like a much better idea to me. We shouldn't be keeping the\nprovided *.idx file given that we plan on overwriting it later on in the\nprocess.\n\nThis feels like more fallout similar to the one described by c177d3dc50\n(pack-objects: use finalize_object_file() to rename pack/idx/etc,\n2024-09-26), in particularly the paragraph beginning with \"This has some\ntest and real-world fallout ...\" and the one immediately below it.\n\nSo I think that your \"cleaner fix\" is the way to go.\n\nThanks,\nTaylor\n"},{"id":"505746","messageId":"Zxa6ixwP2aOJdfmL@nand.local","threadId":"62370","inReplyTo":"20241020024022.GA615104@coredump.intra.peff.net","subject":"Re: Bug report: v2.47.0 cannot fetch version 1 pack indexes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-21T20:33:15Z","receivedAt":"2024-10-21T20:33:19Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Oct 19, 2024 at 10:40:22PM -0400, Jeff King wrote:\n> diff --git a/http.c b/http.c\n> index d59e59f66b..6df032e40f 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -19,6 +19,7 @@\n> #include \"string-list.h\"\n> #include \"object-file.h\"\n> #include \"object-store-ll.h\"\n> +#include \"tempfile.h\"\n>\n> static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);\n> static int trace_curl_data = 1;\n> @@ -2388,8 +2389,24 @@ static char *fetch_pack_index(unsigned char *hash, const char *base_url)\n> \tstrbuf_addf(&buf, \"objects/pack/pack-%s.idx\", hash_to_hex(hash));\n> \turl = strbuf_detach(&buf, NULL);\n>\n> -\tstrbuf_addf(&buf, \"%s.temp\", sha1_pack_index_name(hash));\n> -\ttmp = strbuf_detach(&buf, NULL);\n> +\t/*\n> +\t * Don't put this into packs/, since it's just temporary and we don't\n> +\t * want to confuse it with our local .idx files. We'll generate our\n> +\t * own index if we choose to download the matching packfile.\n> +\t *\n> +\t * It's tempting to use xmks_tempfile() here, but it's important that\n> +\t * the file not exist, otherwise http_get_file() complains. So we\n> +\t * create a filename that should be unique, and then just register it\n> +\t * as a tempfile so that it will get cleaned up on exit.\n> +\t *\n> +\t * Arguably it would be better to hold on to the tempfile handle so\n> +\t * that we can remove it as soon as we download the pack and generate\n> +\t * the real index, but that might need more surgery.\n> +\t */\n> +\ttmp = xstrfmt(\"%s/tmp_pack_%s.idx\",\n> +\t\t repo_get_object_directory(the_repository),\n> +\t\t hash_to_hex(hash));\n> +\tregister_tempfile(tmp);\n\nMakes perfect sense, and the comment above here is much appreciated.\n\nI thought about trying to use some intermediate state of the strbuf here\nto avoid an extra xstrfmt() call, but couldn't come up with anything I\ndidn't think was awkward.\n\n> \tif (http_get_file(url, tmp, NULL) != HTTP_OK) {\n> \t\terror(\"Unable to get pack index %s\", url);\n> @@ -2427,10 +2444,8 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n> \t}\n>\n> \tret = verify_pack_index(new_pack);\n> -\tif (!ret) {\n> +\tif (!ret)\n> \t\tclose_pack_index(new_pack);\n> -\t\tret = finalize_object_file(tmp_idx, sha1_pack_index_name(sha1));\n\nAnd here's where we stop calling finalize_object_file(). Good.\n\n> -\t}\n> \tfree(tmp_idx);\n> \tif (ret)\n> \t\treturn -1;\n> diff --git a/packfile.c b/packfile.c\n> index df4ba67719..16d3bcf7f7 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -237,13 +237,22 @@ static struct packed_git *alloc_packed_git(int extra)\n> \treturn p;\n> }\n>\n> +static char *pack_path_from_idx(const char *idx_path)\n> +{\n> +\tsize_t len;\n> +\tif (!strip_suffix(idx_path, \".idx\", &len))\n> +\t\tBUG(\"idx path does not end in .idx: %s\", idx_path);\n> +\treturn xstrfmt(\"%.*s.pack\", (int)len, idx_path);\n> +}\n> +\n> struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)\n> {\n> -\tconst char *path = sha1_pack_name(sha1);\n> +\tchar *path = pack_path_from_idx(idx_path);\n\nHuh. I would have thought we have such a helper function already. I\nguess we probably do, but that it's also defined statically because it's\nso easy to write.\n\nIn any case, this looks like the right thing to do here. It would be\nnice to have a corresponding test here, since unlike the other\nfinalize_object_file() changes, this one can be provoked\ndeterministically.\n\nWould you mind submitting this as a bona-fide patch, which I can then\npick up and start merging down?\n\nIn any event, I appreciate you figuring out what was going on here and\noutlining a couple of paths forward.\n\nThanks,\nTaylor\n"},{"id":"505800","messageId":"20241022050040.GA1247135@coredump.intra.peff.net","threadId":"62370","inReplyTo":"Zxa4XU+j4+SSmk9c@nand.local","subject":"Re: Bug report: v2.47.0 cannot fetch version 1 pack indexes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-22T05:00:40Z","receivedAt":"2024-10-22T05:00:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 21, 2024 at 04:23:57PM -0400, Taylor Blau wrote:\n\n> On Sat, Oct 19, 2024 at 09:24:55PM -0400, Jeff King wrote:\n> > There are a few things I don't like there, though:\n> >\n> > - obviously reversing the tempfile name back to the idx is hacky. We\n> > could probably store the original idx that led us to this pack and\n> > use that.\n> \n> TBH, I don't think that this is all that hacky, but...\n\nMostly I was worried that there was some case where the filename would\nnot be the usual pack-$hash.idx that we expected. But having now looked\nat the code quite a bit to produce the follow-on patch, I think these\nnames are always generated locally from sha1_pack_name(), etc.\n\nAnother variant of this approach is to actually teach index-pack a flag\nto say \"it's OK to overwrite an existing .idx file, even if the bytes\ndon't match\". That's the simplest fix _conceptually_, in that it takes\nus back to the pre-2.47 behavior. But plumbing through that option would\nrequire a lot of ugly and confusing boilerplate code.\n\n> > So IMHO a cleaner fix is that we should keep the stuff we download from\n> > the remote marked as temporary files, and then throw them away when we\n> > complete the fetch (rather than just expecting index-pack to overwrite\n> > them).\n> \n> ...this seems like a much better idea to me. We shouldn't be keeping the\n> provided *.idx file given that we plan on overwriting it later on in the\n> process.\n\nI dug on this a little more, and...I don't love the design of the\ndumb-http protocol.\n\nA big question here is: what should the life cycle of those downloaded\n.idx files be? And how much should we try to avoid re-downloading them\n(keeping in mind that the .idx for linux.git is almost 300MB)?\n\nIn a normal dumb-http clone, something like this happens with the\ncurrent code:\n\n 1. We see we need some object X.\n\n 2. We download _all_ of the .idx files offered by the remote, since we\n have no idea in which one we'll find X. (There's an obvious\n low-hanging optimization to stop when we find X, but in the worst\n case we need all of them anyway).\n\n 3. For each one, we download the matching pack if needed. In most\n cases you need all of them, but there might be some pack P for\n which we only have the .idx. We leave that bare .idx sitting in the\n objects directory.\n\nSo now we have all of the other side's .idx files locally, but not\nnecessarily all of their .packs. And here are some interesting follow-on\ncases:\n\n a. You do a follow-up fetch, and now you need object Y. The saved idx\n of P is useful, because you can avoid downloading it again. Yay.\n You likewise should avoid re-downloading any for which you did get\n the matching pack, because you'll have pack-$hash.idx locally (just\n your generated copy, not the one you got from the other side).\n\n b. The other side repacks, and then you fetch. Now all their pack\n names are changed. So your saved idx of P is useless. But also you\n end up downloading all of the new .idx files anyway (even though\n you probably already have 99% of the objects).\n\n c. You repack, then fetch. Now the other side has the same packs, but\n your local copies all have new names. You re-download every idx\n from the other side, not realizing that you already saw them last\n time.\n\nWith my patch, case (a) doesn't save P (because now we treat it like a\ntempfile), so we re-download it unnecessarily. That's a little worse.\nI think the other two cases are largely the same.\n\nBut there's a flip side here. What if we kept the .idx files we got from\nthe other side in objects/pack/remote-idx/, basically as a cache? Now\ncase (c) avoids a lot of pointless downloading, though it comes at the\ncost of storing those extra .idx files (which, before we repack, are\nbasically duplicates of the .idx files we generated ourselves; if we\nreally wanted to get clever we could probably hard-link or something).\n\nWe didn't improve (b). And in fact, we've created a new issue: we're\nstill holding on to the old cached .idx files. We'd have to have some\nscheme to get rid of them. I suppose that happens when we fetch the\nother side's objects/info/packs file and see that they're no longer\nmentioned (though if you want to be pedantic, this really should be a\nper-remote cache, since in theory two remotes could hold the same pack).\n\nSo...what does it all mean for this fix?\n\nI think the patch I posted earlier, to keep the .idx files as separate\ntempfiles, is only slightly worse than the status quo (it doesn't keep\nP.idx that we didn't need). And given my general feelings towards the\ndumb-http protocol, maybe that is the right place to stop.\n\nIt just feels like we _could_ be improving things with a better managed\n.idx cache system. And that would fix the regression at the same time.\nBut I'm not sure it's worth sinking time and complexity into this\nterribly inefficient protocol.\n\n(Part of why I laid all this out is that until wrote all of the above, I\nwasn't completely convinced that case (a) was the only one that\nsuffered, and that it was so mild).\n\n-Peff\n"},{"id":"505803","messageId":"20241022051402.GB1247135@coredump.intra.peff.net","threadId":"62370","inReplyTo":"Zxa6ixwP2aOJdfmL@nand.local","subject":"Re: Bug report: v2.47.0 cannot fetch version 1 pack indexes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-22T05:14:02Z","receivedAt":"2024-10-22T05:14:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 21, 2024 at 04:33:15PM -0400, Taylor Blau wrote:\n\n> > @@ -2388,8 +2389,24 @@ static char *fetch_pack_index(unsigned char *hash, const char *base_url)\n> > \tstrbuf_addf(&buf, \"objects/pack/pack-%s.idx\", hash_to_hex(hash));\n> > \turl = strbuf_detach(&buf, NULL);\n> >\n> > -\tstrbuf_addf(&buf, \"%s.temp\", sha1_pack_index_name(hash));\n> > -\ttmp = strbuf_detach(&buf, NULL);\n> > +\t/*\n> > +\t * Don't put this into packs/, since it's just temporary and we don't\n> > +\t * want to confuse it with our local .idx files. We'll generate our\n> > +\t * own index if we choose to download the matching packfile.\n> > +\t *\n> > +\t * It's tempting to use xmks_tempfile() here, but it's important that\n> > +\t * the file not exist, otherwise http_get_file() complains. So we\n> > +\t * create a filename that should be unique, and then just register it\n> > +\t * as a tempfile so that it will get cleaned up on exit.\n> > +\t *\n> > +\t * Arguably it would be better to hold on to the tempfile handle so\n> > +\t * that we can remove it as soon as we download the pack and generate\n> > +\t * the real index, but that might need more surgery.\n> > +\t */\n> > +\ttmp = xstrfmt(\"%s/tmp_pack_%s.idx\",\n> > +\t\t repo_get_object_directory(the_repository),\n> > +\t\t hash_to_hex(hash));\n> > +\tregister_tempfile(tmp);\n> \n> Makes perfect sense, and the comment above here is much appreciated.\n> \n> I thought about trying to use some intermediate state of the strbuf here\n> to avoid an extra xstrfmt() call, but couldn't come up with anything I\n> didn't think was awkward.\n\nI don't think there's any useful intermediate state. The earlier %s is\nthe base url, but here it's our local directory.\n\nWe could continue to re-use the scratch strbuf as the existing code did\n(and which xstrfmt() is doing under the hood). It wasn't really\nintentional for me to change that, but I went through a lot of attempts\nto get here (using mks_tempfile(), and so on).\n\n> > +static char *pack_path_from_idx(const char *idx_path)\n> > +{\n> > +\tsize_t len;\n> > +\tif (!strip_suffix(idx_path, \".idx\", &len))\n> > +\t\tBUG(\"idx path does not end in .idx: %s\", idx_path);\n> > +\treturn xstrfmt(\"%.*s.pack\", (int)len, idx_path);\n> > +}\n> > +\n> > struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)\n> > {\n> > -\tconst char *path = sha1_pack_name(sha1);\n> > +\tchar *path = pack_path_from_idx(idx_path);\n> \n> Huh. I would have thought we have such a helper function already. I\n> guess we probably do, but that it's also defined statically because it's\n> so easy to write.\n\nI thought so, too, but couldn't find one. We have pack_bitmap_filename()\n(and so on for .rev and .midx files) that goes from .pack to those\nextensions. But here we want to go from .idx to .pack. I think most\nstuff goes from \".pack\" because that's what we store in the packed_git\nstruct.\n\nThere's also sha1_pack_index_name(), but that goes from a csum-file hash\nto a filename.\n\nI grepped around and strip_suffix() seems to be par for the course in\nsimilar situations within pack/repack code, so I think it's OK here.\n\n> In any case, this looks like the right thing to do here. It would be\n> nice to have a corresponding test here, since unlike the other\n> finalize_object_file() changes, this one can be provoked\n> deterministically.\n> \n> Would you mind submitting this as a bona-fide patch, which I can then\n> pick up and start merging down?\n\nYeah, the test is easy:\n\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 58189c9f7d..50a7b98813 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -507,4 +507,14 @@ test_expect_success 'fetching via http alternates works' '\n \tgit -c http.followredirects=true clone \"$HTTPD_URL/dumb/alt-child.git\"\n '\n \n+test_expect_success 'dumb http can fetch index v1' '\n+\tserver=$HTTPD_DOCUMENT_ROOT_PATH/idx-v1.git &&\n+\tgit init --bare \"$server\" &&\n+\tgit -C \"$server\" --work-tree=. commit --allow-empty -m foo &&\n+\tgit -C \"$server\" -c pack.indexVersion=1 gc &&\n+\n+\tgit clone \"$HTTPD_URL/dumb/idx-v1.git\" &&\n+\tgit -C idx-v1 fsck\n+'\n+\n test_done\n\nI raised some other more philosophical issues in the other part of the\nthread, but assuming the answer is \"no, let's do the simplest thing\",\nthen I think this approach is OK.\n\nI'd also like to see if I can clean things up around parse_pack_index(),\nwhose semantics I'm changing here (and which violates all manner of\nassumptions that we usually have about packed_git structs). It's used\nonly by the dumb-http code, and I think we want to refactor it a bit so\nthat nobody else is tempted to use it.\n\nI'll try to send something out tonight or tomorrow.\n\n-Peff\n"},{"id":"505836","messageId":"ZxfCTO+COaoablPe@nand.local","threadId":"62370","inReplyTo":"20241022051402.GB1247135@coredump.intra.peff.net","subject":"Re: Bug report: v2.47.0 cannot fetch version 1 pack indexes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-22T15:18:36Z","receivedAt":"2024-10-22T15:18:39Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 22, 2024 at 01:14:02AM -0400, Jeff King wrote:\n> > > +static char *pack_path_from_idx(const char *idx_path)\n> > > +{\n> > > +\tsize_t len;\n> > > +\tif (!strip_suffix(idx_path, \".idx\", &len))\n> > > +\t\tBUG(\"idx path does not end in .idx: %s\", idx_path);\n> > > +\treturn xstrfmt(\"%.*s.pack\", (int)len, idx_path);\n> > > +}\n> > > +\n> > > struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)\n> > > {\n> > > -\tconst char *path = sha1_pack_name(sha1);\n> > > +\tchar *path = pack_path_from_idx(idx_path);\n> >\n> > Huh. I would have thought we have such a helper function already. I\n> > guess we probably do, but that it's also defined statically because it's\n> > so easy to write.\n>\n> I thought so, too, but couldn't find one. We have pack_bitmap_filename()\n> (and so on for .rev and .midx files) that goes from .pack to those\n> extensions. But here we want to go from .idx to .pack. I think most\n> stuff goes from \".pack\" because that's what we store in the packed_git\n> struct.\n>\n> There's also sha1_pack_index_name(), but that goes from a csum-file hash\n> to a filename.\n>\n> I grepped around and strip_suffix() seems to be par for the course in\n> similar situations within pack/repack code, so I think it's OK here.\n\nIt would kind of be nice to have a convenient function like:\n\n const char *pack_ext_name(const struct packed_git *p,\n const char *ext);\n\nin our codebase, but that is well outside the scope of this patch ;-).\n\n> > In any case, this looks like the right thing to do here. It would be\n> > nice to have a corresponding test here, since unlike the other\n> > finalize_object_file() changes, this one can be provoked\n> > deterministically.\n> >\n> > Would you mind submitting this as a bona-fide patch, which I can then\n> > pick up and start merging down?\n>\n> Yeah, the test is easy:\n>\n> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\n> index 58189c9f7d..50a7b98813 100755\n> --- a/t/t5550-http-fetch-dumb.sh\n> +++ b/t/t5550-http-fetch-dumb.sh\n> @@ -507,4 +507,14 @@ test_expect_success 'fetching via http alternates works' '\n> \tgit -c http.followredirects=true clone \"$HTTPD_URL/dumb/alt-child.git\"\n> '\n>\n> +test_expect_success 'dumb http can fetch index v1' '\n> +\tserver=$HTTPD_DOCUMENT_ROOT_PATH/idx-v1.git &&\n> +\tgit init --bare \"$server\" &&\n> +\tgit -C \"$server\" --work-tree=. commit --allow-empty -m foo &&\n> +\tgit -C \"$server\" -c pack.indexVersion=1 gc &&\n> +\n> +\tgit clone \"$HTTPD_URL/dumb/idx-v1.git\" &&\n> +\tgit -C idx-v1 fsck\n> +'\n> +\n> test_done\n\nBeautifully written. I wondered why you didn't use `test_commit -C\n$server`, but it's because that repository is bare, and so needs\n`--work-tree=.`, so what you wrote here makes sense.\n\n> I raised some other more philosophical issues in the other part of the\n> thread, but assuming the answer is \"no, let's do the simplest thing\",\n> then I think this approach is OK.\n>\n> I'd also like to see if I can clean things up around parse_pack_index(),\n> whose semantics I'm changing here (and which violates all manner of\n> assumptions that we usually have about packed_git structs). It's used\n> only by the dumb-http code, and I think we want to refactor it a bit so\n> that nobody else is tempted to use it.\n>\n> I'll try to send something out tonight or tomorrow.\n\nSounds all good. Thanks!\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"505837","messageId":"ZxfJw1Ju8WBDktj1@nand.local","threadId":"62370","inReplyTo":"20241022050040.GA1247135@coredump.intra.peff.net","subject":"Re: Bug report: v2.47.0 cannot fetch version 1 pack indexes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-22T15:50:27Z","receivedAt":"2024-10-22T15:50:30Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 22, 2024 at 01:00:40AM -0400, Jeff King wrote:\n> So...what does it all mean for this fix?\n>\n> I think the patch I posted earlier, to keep the .idx files as separate\n> tempfiles, is only slightly worse than the status quo (it doesn't keep\n> P.idx that we didn't need). And given my general feelings towards the\n> dumb-http protocol, maybe that is the right place to stop.\n>\n> It just feels like we _could_ be improving things with a better managed\n> .idx cache system. And that would fix the regression at the same time.\n> But I'm not sure it's worth sinking time and complexity into this\n> terribly inefficient protocol.\n>\n> (Part of why I laid all this out is that until wrote all of the above, I\n> wasn't completely convinced that case (a) was the only one that\n> suffered, and that it was so mild).\n\nAll very well analyzed. I share your feelings about the dumb-HTTP\nprotocol. It would be nice to fix, but I have a feeling that our\ncollective time is probably better spent pursuing a reasonable band-aid\nand then focusing our attention elsewhere ;-).\n\nThanks,\nTaylor\n"},{"id":"506082","messageId":"20241025064148.GA2110169@coredump.intra.peff.net","threadId":"62370","inReplyTo":"20241022051402.GB1247135@coredump.intra.peff.net","subject":"[PATCH 0/11] dumb-http pack index v1 regression + cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-25T06:41:48Z","receivedAt":"2024-10-25T06:41:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 22, 2024 at 01:14:02AM -0400, Jeff King wrote:\n\n> I'd also like to see if I can clean things up around parse_pack_index(),\n> whose semantics I'm changing here (and which violates all manner of\n> assumptions that we usually have about packed_git structs). It's used\n> only by the dumb-http code, and I think we want to refactor it a bit so\n> that nobody else is tempted to use it.\n> \n> I'll try to send something out tonight or tomorrow.\n\nThis took me a little longer than I expected, mostly due to triggering\nan (existing) bug related to midx. So here's the series I came up with.\n\nThe first patch fixes the midx bug, which otherwise causes spurious test\nfailures in patch 3.\n\nThe second patch beefs up the existing tests for the area we're\ntouching.\n\nPatch 3 is the actual regression fix. I apologize in advance for the\nlength of the commit message. ;) I tried to summarize all of the weird\ndumb-http implications I discovered.\n\nIf we wanted to we could stop there for maint, and treat the rest as a\nseparate series. But since it's cleanup with no intended behavior\nchange, it's pretty low-risk.\n\nPatches 4-10 are mostly about cleaning up crufty interfaces that I ran\nacross while working on the earlier patches.\n\nAnd then the final one is an oddity where I think the code is doing the\nwrong thing, but it's not possible to actually trigger the bug in\npractice.\n\n [01/11]: midx: avoid duplicate packed_git entries\n [02/11]: t5550: count fetches in \"previously-fetched .idx\" test\n [03/11]: dumb-http: store downloaded pack idx as tempfile\n [04/11]: packfile: drop has_pack_index()\n [05/11]: packfile: drop sha1_pack_name()\n [06/11]: packfile: drop sha1_pack_index_name()\n [07/11]: packfile: warn people away from parse_packed_git()\n [08/11]: http-walker: use object_id instead of bare hash\n [09/11]: packfile: convert find_sha1_pack() to use object_id\n [10/11]: packfile: use object_id in find_pack_entry_one()\n [11/11]: packfile: use oidread() instead of hashcpy() to fill object_id\n\n builtin/fast-import.c | 6 ++---\n builtin/pack-objects.c | 4 +--\n builtin/pack-redundant.c | 5 +++-\n connected.c | 4 +--\n http-push.c | 4 +--\n http-walker.c | 25 ++++++++---------\n http.c | 43 ++++++++++++++++++++---------\n midx.c | 22 ++++++++++++---\n pack-bitmap.c | 4 +--\n packfile.c | 51 ++++++++++++++---------------------\n packfile.h | 40 +++++++++++++--------------\n t/helper/test-find-pack.c | 2 +-\n t/t5200-update-server-info.sh | 8 ++++++\n t/t5550-http-fetch-dumb.sh | 28 +++++++++++++++++--\n walker.c | 4 +--\n walker.h | 4 +--\n 16 files changed, 153 insertions(+), 101 deletions(-)\n\n"},{"id":"506083","messageId":"20241025064340.GA2110355@coredump.intra.peff.net","threadId":"62370","inReplyTo":"20241025064148.GA2110169@coredump.intra.peff.net","subject":"[PATCH 01/11] midx: avoid duplicate packed_git entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-25T06:43:40Z","receivedAt":"2024-10-25T06:43:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we scan a pack directory to load the idx entries we find into the\npacked_git list, we skip any of them that are contained in a midx. We\nthen load them later lazily if we actually need to access the\ncorresponding pack, referencing them both from the midx struct and the\npacked_git list.\n\nThe lazy-load in the midx code checks to see if the midx already\nmentions the pack, but doesn't otherwise check the packed_git list. This\nmakes sense, since we should have added any pack to both lists.\n\nBut there's a loophole! If we call close_object_store(), that frees the\nmidx entirely, but _not_ the packed_git structs, which we must keep\naround for Reasons[1]. If we then try to look up more objects, we'll\nauto-load the midx again, which won't realize that we've already loaded\nthose packs, and will create duplicate entries in the packed_git list.\n\nThis is possibly inefficient, because it means we may open and map the\npack redundantly. But it can also lead to weird user-visible behavior.\nThe case I found is in \"git repack\", which closes and reopens the midx\nafter repacking and then calls update_server_info(). We end up writing\nthe duplicate entries into objects/info/packs.\n\nWe could obviously de-dup them while writing that file, but it seems\nlike a violation of more core assumptions that we end up with these\nduplicate structs at all.\n\nWe can avoid the duplicates reasonably efficiently by checking their\nnames in the pack_map hash. This annoyingly does require a little more\nthan a straight hash lookup due to the naming conventions, but it should\nonly happen when we are about to actually open a pack. I don't think one\nextra malloc will be noticeable there.\n\n[1] I'm not entirely sure of all the details, except that we generally\n assume the packed_git structs never go away. We noted this\n restriction in the comment added by 6f1e9394e2 (object: fix leaking\n packfiles when closing object store, 2024-08-08), but it's somewhat\n vague. At any rate, if you try freeing the structs in\n close_object_store(), you can observe segfaults all over the test\n suite. So it might be fixable, but it's not trivial.\n\nSigned-off-by: Jeff King \n---\n+cc Stolee here as the original midx author. I can't think of a good\nreason we'd want to avoid this dup-detection here.\n\n midx.c | 20 +++++++++++++++++---\n t/t5200-update-server-info.sh | 8 ++++++++\n 2 files changed, 25 insertions(+), 3 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 67e0d64004..479812cb9b 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -445,6 +445,7 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n \t\t uint32_t pack_int_id)\n {\n \tstruct strbuf pack_name = STRBUF_INIT;\n+\tstruct strbuf key = STRBUF_INIT;\n \tstruct packed_git *p;\n \n \tpack_int_id = midx_for_pack(&m, pack_int_id);\n@@ -455,16 +456,29 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n \tstrbuf_addf(&pack_name, \"%s/pack/%s\", m->object_dir,\n \t\t m->pack_names[pack_int_id]);\n \n-\tp = add_packed_git(pack_name.buf, pack_name.len, m->local);\n+\t/* pack_map holds the \".pack\" name, but we have the .idx */\n+\tstrbuf_addbuf(&key, &pack_name);\n+\tstrbuf_strip_suffix(&key, \".idx\");\n+\tstrbuf_addstr(&key, \".pack\");\n+\tp = hashmap_get_entry_from_hash(&r->objects->pack_map,\n+\t\t\t\t\tstrhash(key.buf), key.buf,\n+\t\t\t\t\tstruct packed_git, packmap_ent);\n+\tif (!p) {\n+\t\tp = add_packed_git(pack_name.buf, pack_name.len, m->local);\n+\t\tif (p) {\n+\t\t\tinstall_packed_git(r, p);\n+\t\t\tlist_add_tail(&p->mru, &r->objects->packed_git_mru);\n+\t\t}\n+\t}\n+\n \tstrbuf_release(&pack_name);\n+\tstrbuf_release(&key);\n \n \tif (!p)\n \t\treturn 1;\n \n \tp->multi_pack_index = 1;\n \tm->packs[pack_int_id] = p;\n-\tinstall_packed_git(r, p);\n-\tlist_add_tail(&p->mru, &r->objects->packed_git_mru);\n \n \treturn 0;\n }\ndiff --git a/t/t5200-update-server-info.sh b/t/t5200-update-server-info.sh\nindex ed9dfd624c..cc51c73986 100755\n--- a/t/t5200-update-server-info.sh\n+++ b/t/t5200-update-server-info.sh\n@@ -39,4 +39,12 @@ test_expect_success 'info/refs updates when changes are made' '\n \t! test_cmp a b\n '\n \n+test_expect_success 'midx does not create duplicate pack entries' '\n+\tgit repack -d --write-midx &&\n+\tgit repack -d &&\n+\tgrep ^P .git/objects/info/packs >packs &&\n+\tuniq -d dups &&\n+\ttest_must_be_empty dups\n+'\n+\n test_done\n-- \n2.47.0.363.g6e72b256be\n\n"},{"id":"506084","messageId":"20241025064417.GB2110355@coredump.intra.peff.net","threadId":"62370","inReplyTo":"20241025064148.GA2110169@coredump.intra.peff.net","subject":"[PATCH 02/11] t5550: count fetches in \"previously-fetched .idx\" test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-25T06:44:17Z","receivedAt":"2024-10-25T06:44:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have a test in t5550 that looks at index fetching over dumb http. It\ncreates two branches, each of which is completely stored in its own\npack, then fetches the branches independently. What should (and does)\nhappen is that the first fetch grabs both .idx files and one .pack file,\nand then the fetch of the second branch re-uses the previously\ndownloaded .idx files (fetching none) and grabs the now-required .pack\nfile.\n\nSince the next few patches will be touching this area of the code, let's\nbeef up the test a little by checking that we're downloading the\nexpected items at each step.\n\nSigned-off-by: Jeff King \n---\n t/t5550-http-fetch-dumb.sh | 18 ++++++++++++++++--\n 1 file changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 58189c9f7d..3968b82260 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -307,6 +307,14 @@ test_expect_success 'fetch notices corrupt idx' '\n \t)\n '\n \n+# usage: count_fetches \n+count_fetches () {\n+\t# ignore grep exit code; it may return non-zero if we are expecting no\n+\t# matches\n+\tgrep \"GET .*objects/pack/pack-[a-z0-9]*.$2\" \"$3\" >trace.count\n+\ttest_line_count = \"$1\" trace.count\n+}\n+\n test_expect_success 'fetch can handle previously-fetched .idx files' '\n \tgit checkout --orphan branch1 &&\n \techo base >file &&\n@@ -321,8 +329,14 @@ test_expect_success 'fetch can handle previously-fetched .idx files' '\n \tgit push \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_packed_branches.git branch2 &&\n \tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_packed_branches.git repack -d &&\n \tgit --bare init clone_packed_branches.git &&\n-\tgit --git-dir=clone_packed_branches.git fetch \"$HTTPD_URL\"/dumb/repo_packed_branches.git branch1:branch1 &&\n-\tgit --git-dir=clone_packed_branches.git fetch \"$HTTPD_URL\"/dumb/repo_packed_branches.git branch2:branch2\n+\tGIT_TRACE_CURL=$PWD/one.trace git --git-dir=clone_packed_branches.git \\\n+\t\tfetch \"$HTTPD_URL\"/dumb/repo_packed_branches.git branch1:branch1 &&\n+\tcount_fetches 2 idx one.trace &&\n+\tcount_fetches 1 pack one.trace &&\n+\tGIT_TRACE_CURL=$PWD/two.trace git --git-dir=clone_packed_branches.git \\\n+\t\tfetch \"$HTTPD_URL\"/dumb/repo_packed_branches.git branch2:branch2 &&\n+\tcount_fetches 0 idx two.trace &&\n+\tcount_fetches 1 pack two.trace\n '\n \n test_expect_success 'did not use upload-pack service' '\n-- \n2.47.0.363.g6e72b256be\n\n"},{"id":"506085","messageId":"20241025065806.GC2110355@coredump.intra.peff.net","threadId":"62370","inReplyTo":"20241025064148.GA2110169@coredump.intra.peff.net","subject":"[PATCH 03/11] dumb-http: store downloaded pack idx as tempfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-25T06:58:06Z","receivedAt":"2024-10-25T06:58:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This patch fixes a regression in b1b8dfde69 (finalize_object_file():\nimplement collision check, 2024-09-26) where fetching a v1 pack idx file\nover the dumb-http protocol would cause the fetch to fail.\n\nThe core of the issue is that dumb-http stores the idx we fetch from the\nremote at the same path that will eventually hold the idx we generate\nfrom \"index-pack --stdin\". The sequence is something like this:\n\n 0. We realize we need some object X, which we don't have locally, and\n nor does the other side have it as a loose object.\n\n 1. We download the list of remote packs from objects/info/packs.\n\n 2. For each entry in that file, we download each pack index and store\n it locally in .git/objects/pack/pack-$hash.idx (the $hash is not\n something we can verify yet and is given to us by the remote).\n\n 3. We check each pack index we got to see if it has object X. When we\n find a match, we download the matching .pack file from the remote\n to a tempfile. We feed that to \"index-pack --stdin\", which\n reindexes the pack, rather than trusting that it has what the other\n side claims it does. In most cases, this will end up generating the\n exact same (byte-for-byte) pack index which we'll store at the same\n pack-$hash.idx path, because the index generation and $hash id are\n computed based on what's in the packfile. But:\n\n a. The other side might have used other options to generate the\n index. For instance we use index v2 by default, but long ago\n it was v1 (and you can still ask for v1 explicitly).\n\n b. The other side might even use a different mechanism to\n determine $hash. E.g., long ago it was based on the sorted\n list of objects in the packfile, but we switched to using the\n pack checksum in 1190a1acf8 (pack-objects: name pack files\n after trailer hash, 2013-12-05).\n\nThe regression we saw in the real world was (3a). A recent client\nfetching from a server with a v1 index downloaded that index, then\ncomplained about trying to overwrite it with its own v2 index. This\ncollision is otherwise harmless; we know we want to replace the remote\nversion with our local one, but the collision check doesn't realize\nthat.\n\nThere are a few options to fix it:\n\n - we could teach index-pack a command-line option to ignore only pack\n idx collisions, and use it when the dumb-http code invokes\n index-pack. This would be an awkward thing to expose users to and\n would involve a lot of boilerplate to get the option down to the\n collision code.\n\n - we could delete the remote .idx file right before running\n index-pack. It should be redundant at that point (since we've just\n downloaded the matching pack). But it feels risky to delete\n something from our own .git/objects based on what the other side has\n said. I'm not entirely positive that a malicious server couldn't lie\n about which pack-$hash.idx it has and get us to delete something\n precious.\n\n - we can stop co-mingling the downloaded idx files in our local\n objects directory. This is a slightly bigger change but I think\n fixes the root of the problem more directly.\n\nThis patch implements the third option. The big design questions are:\nwhere do we store the downloaded files, and how do we manage their\nlifetimes?\n\nThere are some additional quirks to the dumb-http system we should\nconsider. Remember that in step 2 we downloaded every pack index, but in\nstep 3 we may only download some of the matching packs. What happens to\nthose other idx files now? They sit in the .git/objects/pack directory,\npossibly waiting to be used at a later date. That may save bandwidth for\na subsequent fetch, but it also creates a lot of weird corner cases:\n\n - our local object directory now has semi-untrusted .idx files sitting\n around, without their matching .pack\n\n - in case 3b, we noted that we might not generate the same hash as the\n other side. In that case even if we download the matching pack,\n our index-pack invocation will store it in a different\n pack-$hash.idx file. And the unmatched .idx will sit there forever.\n\n - if the server repacks, it may delete the old packs. Now we have\n these orphaned .idx files sitting around locally that will never be\n used (nor deleted).\n\n - if we repack locally we may delete our local version of the server's\n pack index and not realize we have it. So we'll download it again,\n even though we have all of the objects it mentions.\n\nI think the right solution here is probably some more complex cache\nmanagement system: download the remote .idx files to their own storage\ndirectory, mark them as \"seen\" when we get their matching pack (to avoid\nre-downloading even if we repack), and then delete them when the\nserver's objects/info/refs no longer mentions them.\n\nBut since the dumb http protocol is so ancient and so inferior to the\nsmart http protocol, I don't think it's worth spending a lot of time\ncreating such a system. For this patch I'm just downloading the idx\nfiles to .git/objects/tmp_pack_*, and marking them as tempfiles to be\ndeleted when we exit (and due to the name, any we miss due to a crash,\netc, should eventually be removed by \"git gc\" runs based on timestamps).\n\nThat is slightly worse for one case: if we download an idx but not the\nmatching pack, we won't retain that idx for subsequent runs. But the\nflip side is that we're making other cases better (we never hold on to\nuseless idx files forever). I suspect that worse case does not even come\nup often, since it implies that the packs are generated to match\ndistinct parts of history (i.e., in practice even in a repo with many\npacks you're going to end up grabbing all of those packs to do a clone).\nIf somebody really cares about that, I think the right path forward is a\nmanaged cache directory as above, and this patch is providing the first\nstep in that direction anyway (by moving things out of the objects/pack/\ndirectory).\n\nThere are two test changes. One demonstrates the broken v1 index case\n(it double-checks the resulting clone with fsck to be careful, but prior\nto this patch it actually fails at the clone step). The other tweaks the\nexpectation for a test that covers the \"slightly worse\" case to\naccommodate the extra index download.\n\nThe code changes are fairly simple. We stop using finalize_object_file()\nto copy the remote's index file into place, and leave it as a tempfile.\nWe give the tempfile a real \".idx\" name, since the packfile code expects\nthat, and thus we make sure it is out of the usual packs/ directory (so\nwe'd never mistake it for a real local .idx).\n\nWe also have to change parse_pack_index(), which creates a temporary\npacked_git to access our index (we need this because all of the pack idx\ncode assumes we have that struct). It reads the index data from the\ntempfile, but prior to this patch would speculatively write the\nfinalized name into the packed_git struct using the pack-$hash we expect\nto use.\n\nI was mildly surprised that this worked at all, since we call\nverify_pack_index() on the packed_git which mentions the final name\nbefore moving the file into place! But it works because\nparse_pack_index() leaves the mmap-ed data in the struct, so the\nlazy-open in verify_pack_index() never triggers, and we read from the\ntempfile, ignoring the filename in the struct completely. Hacky, but it\nworks.\n\nAfter this patch, parse_pack_index() now uses the index filename we pass\nin to derive a matching .pack name. This is OK to change because there\nare only two callers, both in the dumb http code (and the other passes\nin an existing pack-$hash.idx name, so the derived name is going to be\npack-$hash.pack, which is what we were using anyway).\n\nI'll follow up with some more cleanups in that area, but this patch is\nsufficient to fix the regression.\n\nReported-by: fox \nSigned-off-by: Jeff King \n---\n http.c | 25 ++++++++++++++++++++-----\n packfile.c | 11 ++++++++++-\n t/t5550-http-fetch-dumb.sh | 12 +++++++++++-\n 3 files changed, 41 insertions(+), 7 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex d59e59f66b..9642cad2e3 100644\n--- a/http.c\n+++ b/http.c\n@@ -19,6 +19,7 @@\n #include \"string-list.h\"\n #include \"object-file.h\"\n #include \"object-store-ll.h\"\n+#include \"tempfile.h\"\n \n static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);\n static int trace_curl_data = 1;\n@@ -2388,8 +2389,24 @@ static char *fetch_pack_index(unsigned char *hash, const char *base_url)\n \tstrbuf_addf(&buf, \"objects/pack/pack-%s.idx\", hash_to_hex(hash));\n \turl = strbuf_detach(&buf, NULL);\n \n-\tstrbuf_addf(&buf, \"%s.temp\", sha1_pack_index_name(hash));\n-\ttmp = strbuf_detach(&buf, NULL);\n+\t/*\n+\t * Don't put this into packs/, since it's just temporary and we don't\n+\t * want to confuse it with our local .idx files. We'll generate our\n+\t * own index if we choose to download the matching packfile.\n+\t *\n+\t * It's tempting to use xmks_tempfile() here, but it's important that\n+\t * the file not exist, otherwise http_get_file() complains. So we\n+\t * create a filename that should be unique, and then just register it\n+\t * as a tempfile so that it will get cleaned up on exit.\n+\t *\n+\t * In theory we could hold on to the tempfile and delete these as soon\n+\t * as we download the matching pack, but it would take a bit of\n+\t * refactoring. Leaving them until the process ends is probably OK.\n+\t */\n+\ttmp = xstrfmt(\"%s/tmp_pack_%s.idx\",\n+\t\t repo_get_object_directory(the_repository),\n+\t\t hash_to_hex(hash));\n+\tregister_tempfile(tmp);\n \n \tif (http_get_file(url, tmp, NULL) != HTTP_OK) {\n \t\terror(\"Unable to get pack index %s\", url);\n@@ -2427,10 +2444,8 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n \t}\n \n \tret = verify_pack_index(new_pack);\n-\tif (!ret) {\n+\tif (!ret)\n \t\tclose_pack_index(new_pack);\n-\t\tret = finalize_object_file(tmp_idx, sha1_pack_index_name(sha1));\n-\t}\n \tfree(tmp_idx);\n \tif (ret)\n \t\treturn -1;\ndiff --git a/packfile.c b/packfile.c\nindex df4ba67719..16d3bcf7f7 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -237,13 +237,22 @@ static struct packed_git *alloc_packed_git(int extra)\n \treturn p;\n }\n \n+static char *pack_path_from_idx(const char *idx_path)\n+{\n+\tsize_t len;\n+\tif (!strip_suffix(idx_path, \".idx\", &len))\n+\t\tBUG(\"idx path does not end in .idx: %s\", idx_path);\n+\treturn xstrfmt(\"%.*s.pack\", (int)len, idx_path);\n+}\n+\n struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)\n {\n-\tconst char *path = sha1_pack_name(sha1);\n+\tchar *path = pack_path_from_idx(idx_path);\n \tsize_t alloc = st_add(strlen(path), 1);\n \tstruct packed_git *p = alloc_packed_git(alloc);\n \n \tmemcpy(p->pack_name, path, alloc); /* includes NUL */\n+\tfree(path);\n \thashcpy(p->hash, sha1, the_repository->hash_algo);\n \tif (check_packed_git_idx(idx_path, p)) {\n \t\tfree(p);\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 3968b82260..706540ab74 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -335,7 +335,7 @@ test_expect_success 'fetch can handle previously-fetched .idx files' '\n \tcount_fetches 1 pack one.trace &&\n \tGIT_TRACE_CURL=$PWD/two.trace git --git-dir=clone_packed_branches.git \\\n \t\tfetch \"$HTTPD_URL\"/dumb/repo_packed_branches.git branch2:branch2 &&\n-\tcount_fetches 0 idx two.trace &&\n+\tcount_fetches 1 idx two.trace &&\n \tcount_fetches 1 pack two.trace\n '\n \n@@ -521,4 +521,14 @@ test_expect_success 'fetching via http alternates works' '\n \tgit -c http.followredirects=true clone \"$HTTPD_URL/dumb/alt-child.git\"\n '\n \n+test_expect_success 'dumb http can fetch index v1' '\n+\tserver=$HTTPD_DOCUMENT_ROOT_PATH/idx-v1.git &&\n+\tgit init --bare \"$server\" &&\n+\tgit -C \"$server\" --work-tree=. commit --allow-empty -m foo &&\n+\tgit -C \"$server\" -c pack.indexVersion=1 gc &&\n+\n+\tgit clone \"$HTTPD_URL/dumb/idx-v1.git\" &&\n+\tgit -C idx-v1 fsck\n+'\n+\n test_done\n-- \n2.47.0.363.g6e72b256be\n\n"},{"id":"506086","messageId":"20241025070009.GD2110355@coredump.intra.peff.net","threadId":"62370","inReplyTo":"20241025064148.GA2110169@coredump.intra.peff.net","subject":"[PATCH 04/11] packfile: drop has_pack_index()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-25T07:00:09Z","receivedAt":"2024-10-25T07:00:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The has_pack_index() function has several oddities that may make it\nsurprising if you are trying to find out if we have a pack with some\n$hash:\n\n - it is not looking for a valid pack that we found while searching\n object directories. It just looks for any pack-$hash.idx file in the\n pack directory.\n\n - it only looks in the local directory, not any alternates\n\n - it takes a bare \"unsigned char\" hash, which we try to avoid these\n days\n\nThe only caller it has is in the dumb http code; it wants to know if we\nalready have the pack idx in question. This can happen if we downloaded\nthe pack (and generated its index) during a previous fetch.\n\nBefore the previous patch (\"dumb-http: store downloaded pack idx as\ntempfile\"), it could also happen if we downloaded the .idx from the\nremote but didn't get the matching .pack. But since that patch, we don't\nhold on to those .idx files. So there's no need to look for the .idx\nfile in the filesystem; we can just scan through the packed_git list to\nsee if we have it.\n\nThat lets us simplify the dumb http code a bit, as we know that if we\nhave the .idx we have the matching .pack already. And it lets us get rid\nof this odd function that is unlikely to be needed again.\n\nSigned-off-by: Jeff King \n---\n http.c | 15 ++++++++-------\n packfile.c | 8 --------\n packfile.h | 2 --\n 3 files changed, 8 insertions(+), 17 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 9642cad2e3..03802b9049 100644\n--- a/http.c\n+++ b/http.c\n@@ -2420,15 +2420,17 @@ static char *fetch_pack_index(unsigned char *hash, const char *base_url)\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+\tstruct packed_git *new_pack, *p;\n \tchar *tmp_idx = NULL;\n \tint ret;\n \n-\tif (has_pack_index(sha1)) {\n-\t\tnew_pack = parse_pack_index(sha1, sha1_pack_index_name(sha1));\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+\t * If we already have the pack locally, no need to fetch its index or\n+\t * even add it to list; we already have all of its objects.\n+\t */\n+\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n+\t\tif (hasheq(p->hash, sha1, the_repository->hash_algo))\n+\t\t\treturn 0;\n \t}\n \n \ttmp_idx = fetch_pack_index(sha1, base_url);\n@@ -2450,7 +2452,6 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,\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/packfile.c b/packfile.c\nindex 16d3bcf7f7..0ead2290d4 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2160,14 +2160,6 @@ int has_object_kept_pack(const struct object_id *oid, unsigned flags)\n \treturn find_kept_pack_entry(the_repository, oid, flags, &e);\n }\n \n-int has_pack_index(const unsigned char *sha1)\n-{\n-\tstruct stat st;\n-\tif (stat(sha1_pack_index_name(sha1), &st))\n-\t\treturn 0;\n-\treturn 1;\n-}\n-\n int for_each_object_in_pack(struct packed_git *p,\n \t\t\t each_packed_object_fn cb, void *data,\n \t\t\t enum for_each_object_flags flags)\ndiff --git a/packfile.h b/packfile.h\nindex 0f78658229..b4df3546a3 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -193,8 +193,6 @@ int find_kept_pack_entry(struct repository *r, const struct object_id *oid, unsi\n int has_object_pack(const struct object_id *oid);\n int has_object_kept_pack(const struct object_id *oid, unsigned flags);\n \n-int has_pack_index(const unsigned char *sha1);\n-\n /*\n * Return 1 if an object in a promisor packfile is or refers to the given\n * object, 0 otherwise.\n-- \n2.47.0.363.g6e72b256be\n\n"},{"id":"506087","messageId":"20241025070055.GE2110355@coredump.intra.peff.net","threadId":"62370","inReplyTo":"20241025064148.GA2110169@coredump.intra.peff.net","subject":"[PATCH 05/11] packfile: drop sha1_pack_name()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-25T07:00:55Z","receivedAt":"2024-10-25T07:00:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The sha1_pack_name() function has a few ugly bits:\n\n - it writes into a static strbuf (and not even a ring buffer of them),\n which can lead to subtle invalidation problems\n\n - it uses the term \"sha1\", but it's really using the_hash_algo, which\n could be sha256\n\nThere's only one caller of it left. And in fact that caller is better\noff using the underlying odb_pack_name() function itself, since it's\njust copying the result into its own strbuf anyway.\n\nConverting that caller lets us get rid of this now-obselete function.\n\nSigned-off-by: Jeff King \n---\n http.c | 3 ++-\n packfile.c | 6 ------\n packfile.h | 7 -------\n 3 files changed, 2 insertions(+), 14 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 03802b9049..510332ab04 100644\n--- a/http.c\n+++ b/http.c\n@@ -2579,7 +2579,8 @@ struct http_pack_request *new_direct_http_pack_request(\n \n \tpreq->url = url;\n \n-\tstrbuf_addf(&preq->tmpfile, \"%s.temp\", sha1_pack_name(packed_git_hash));\n+\todb_pack_name(&preq->tmpfile, packed_git_hash, \"pack\");\n+\tstrbuf_addstr(&preq->tmpfile, \".temp\");\n \tpreq->packfile = fopen(preq->tmpfile.buf, \"a\");\n \tif (!preq->packfile) {\n \t\terror(\"Unable to open local file %s for pack\",\ndiff --git a/packfile.c b/packfile.c\nindex 0ead2290d4..48d650161f 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -35,12 +35,6 @@ char *odb_pack_name(struct strbuf *buf,\n \treturn buf->buf;\n }\n \n-char *sha1_pack_name(const unsigned char *sha1)\n-{\n-\tstatic struct strbuf buf = STRBUF_INIT;\n-\treturn odb_pack_name(&buf, sha1, \"pack\");\n-}\n-\n char *sha1_pack_index_name(const unsigned char *sha1)\n {\n \tstatic struct strbuf buf = STRBUF_INIT;\ndiff --git a/packfile.h b/packfile.h\nindex b4df3546a3..2bbcc58571 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -31,13 +31,6 @@ struct pack_entry {\n */\n char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);\n \n-/*\n- * Return the name of the (local) packfile with the specified sha1 in\n- * its name. The return value is a pointer to memory that is\n- * overwritten each time this function is called.\n- */\n-char *sha1_pack_name(const unsigned char *sha1);\n-\n /*\n * Return the name of the (local) pack index file with the specified\n * sha1 in its name. The return value is a pointer to memory that is\n-- \n2.47.0.363.g6e72b256be\n\n"},{"id":"506088","messageId":"20241025070125.GF2110355@coredump.intra.peff.net","threadId":"62370","inReplyTo":"20241025064148.GA2110169@coredump.intra.peff.net","subject":"[PATCH 06/11] packfile: drop sha1_pack_index_name()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-25T07:01:25Z","receivedAt":"2024-10-25T07:01:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Like sha1_pack_name() that we dropped in the previous commit, this\nfunction uses an error-prone static strbuf and the somewhat misleading\nname \"sha1\".\n\nThe only caller left is in pack-redundant.c. While this command is\nmarked for potential removal in our BreakingChanges document, we still\nhave it for now. But it's simple enough to convert it to use its own\nstrbuf with the underlying odb_pack_name() function, letting us drop the\notherwise obsolete function.\n\nNote that odb_pack_name() does its own strbuf_reset(), so it's safe to\nuse directly within a loop like this.\n\nSigned-off-by: Jeff King \n---\n builtin/pack-redundant.c | 5 ++++-\n packfile.c | 6 ------\n packfile.h | 7 -------\n 3 files changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex 5809613002..d2c1c4e5ec 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -13,6 +13,7 @@\n \n #include \"packfile.h\"\n #include \"object-store-ll.h\"\n+#include \"strbuf.h\"\n \n #define BLKSIZE 512\n \n@@ -591,6 +592,7 @@ static void load_all(void)\n int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, struct repository *repo UNUSED) {\n \tint i; int i_still_use_this = 0; struct pack_list *min = NULL, *red, *pl;\n \tstruct llist *ignore;\n+\tstruct strbuf idx_name = STRBUF_INIT;\n \tchar buf[GIT_MAX_HEXSZ + 2]; /* hex hash + \\n + \\0 */\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n@@ -688,7 +690,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s\n \tpl = red = pack_list_difference(local_packs, min);\n \twhile (pl) {\n \t\tprintf(\"%s\\n%s\\n\",\n-\t\t sha1_pack_index_name(pl->pack->hash),\n+\t\t odb_pack_name(&idx_name, pl->pack->hash, \"idx\"),\n \t\t pl->pack->pack_name);\n \t\tpl = pl->next;\n \t}\n@@ -699,5 +701,6 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s\n \tpack_list_free(red);\n \tpack_list_free(min);\n \tllist_free(ignore);\n+\tstrbuf_release(&idx_name);\n \treturn 0;\n }\ndiff --git a/packfile.c b/packfile.c\nindex 48d650161f..0d4fd2737a 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -35,12 +35,6 @@ char *odb_pack_name(struct strbuf *buf,\n \treturn buf->buf;\n }\n \n-char *sha1_pack_index_name(const unsigned char *sha1)\n-{\n-\tstatic struct strbuf buf = STRBUF_INIT;\n-\treturn odb_pack_name(&buf, sha1, \"idx\");\n-}\n-\n static unsigned int pack_used_ctr;\n static unsigned int pack_mmap_calls;\n static unsigned int peak_pack_open_windows;\ndiff --git a/packfile.h b/packfile.h\nindex 2bbcc58571..6812abaae0 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -31,13 +31,6 @@ struct pack_entry {\n */\n char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);\n \n-/*\n- * Return the name of the (local) pack index file with the specified\n- * sha1 in its name. The return value is a pointer to memory that is\n- * overwritten each time this function is called.\n- */\n-char *sha1_pack_index_name(const unsigned char *sha1);\n-\n /*\n * Return the basename of the packfile, omitting any containing directory\n * (e.g., \"pack-1234abcd[...].pack\").\n-- \n2.47.0.363.g6e72b256be\n\n"},{"id":"506089","messageId":"20241025070222.GG2110355@coredump.intra.peff.net","threadId":"62370","inReplyTo":"20241025064148.GA2110169@coredump.intra.peff.net","subject":"[PATCH 07/11] packfile: warn people away from parse_packed_git()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-25T07:02:22Z","receivedAt":"2024-10-25T07:02:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"With a name like parse_packed_git(), you might think it's the right way\nto access a local pack index and its associated objects. But not so!\nIt's a one-off used by the dumb-http code to access pack idx files we've\ndownloaded from the remote, but whose packs we might not have.\n\nThere's only one caller left for this function, and ideally we'd drop it\ncompletely and just inline it there. But that would require exposing\nother internals from packfile.[ch], like alloc_packed_git() and\ncheck_packed_git_idx().\n\nSo let's leave it be for now, and just warn people that it's probably\nnot what they're looking for. Perhaps in the long run if we eventually\ndrop dumb-http support, we can remove the function entirely then.\n\nSigned-off-by: Jeff King \n---\n packfile.h | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/packfile.h b/packfile.h\nindex 6812abaae0..ad393be9f1 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -37,6 +37,15 @@ char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *e\n */\n const char *pack_basename(struct packed_git *p);\n \n+/*\n+ * Parse the pack idx file found at idx_path and create a packed_git struct\n+ * which can be used with find_pack_entry_one().\n+ *\n+ * You probably don't want to use this function! It skips most of the normal\n+ * sanity checks (including whether we even have the matching .pack file),\n+ * and does not add the resulting packed_git struct to the internal list of\n+ * packs. You probably want add_packed_git() instead.\n+ */\n struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \n typedef void each_file_in_pack_dir_fn(const char *full_path, size_t full_path_len,\n@@ -193,7 +202,7 @@ int is_promisor_object(const struct object_id *oid);\n *\n * This function should not be used directly. It is exposed here only so that we\n * have a convenient entry-point for fuzz testing. For real uses, you should\n- * probably use open_pack_index() or parse_pack_index() instead.\n+ * probably use open_pack_index() instead.\n */\n int load_idx(const char *path, const unsigned int hashsz, void *idx_map,\n \t size_t idx_size, struct packed_git *p);\n-- \n2.47.0.363.g6e72b256be\n\n"},{"id":"506090","messageId":"20241025070342.GH2110355@coredump.intra.peff.net","threadId":"62370","inReplyTo":"20241025064148.GA2110169@coredump.intra.peff.net","subject":"[PATCH 08/11] http-walker: use object_id instead of bare hash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-25T07:03:42Z","receivedAt":"2024-10-25T07:03:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We long ago switched most code to using object_id structs instead of\nbare \"unsigned char *\" hashes. This gives us more type safety from the\ncompiler, and generally makes it easier to understand what we expect in\neach parameter.\n\nBut the dumb-http code has lagged behind. And indeed, the whole \"walker\"\nsubsystem interface has the same problem, though http-walker is the only\nuser left.\n\nSo let's update the walker interface to pass object_id structs (which we\nalready have anyway at all call sites!), and likewise use those within\nthe http-walker methods that it calls.\n\nThis cleans up the dumb-http code a bit, but will also let us fix a few\nmore commonly used helper functions.\n\nSigned-off-by: Jeff King \n---\n http-walker.c | 25 +++++++++++++------------\n walker.c | 4 ++--\n walker.h | 4 ++--\n 3 files changed, 17 insertions(+), 16 deletions(-)\n\ndiff --git a/http-walker.c b/http-walker.c\nindex fb2d86d5e7..36dd1f33c0 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -147,14 +147,14 @@ static int fill_active_slot(void *data UNUSED)\n \treturn 0;\n }\n \n-static void prefetch(struct walker *walker, unsigned char *sha1)\n+static void prefetch(struct walker *walker, const struct object_id *oid)\n {\n \tstruct object_request *newreq;\n \tstruct walker_data *data = walker->data;\n \n \tnewreq = xmalloc(sizeof(*newreq));\n \tnewreq->walker = walker;\n-\toidread(&newreq->oid, sha1, the_repository->hash_algo);\n+\toidcpy(&newreq->oid, oid);\n \tnewreq->repo = data->alt;\n \tnewreq->state = WAITING;\n \tnewreq->req = NULL;\n@@ -422,7 +422,8 @@ static int fetch_indices(struct walker *walker, struct alt_base *repo)\n \treturn ret;\n }\n \n-static int http_fetch_pack(struct walker *walker, struct alt_base *repo, unsigned char *sha1)\n+static int http_fetch_pack(struct walker *walker, struct alt_base *repo,\n+\t\t\t const struct object_id *oid)\n {\n \tstruct packed_git *target;\n \tint ret;\n@@ -431,7 +432,7 @@ static int http_fetch_pack(struct walker *walker, struct alt_base *repo, unsigne\n \n \tif (fetch_indices(walker, repo))\n \t\treturn -1;\n-\ttarget = find_sha1_pack(sha1, repo->packs);\n+\ttarget = find_sha1_pack(oid->hash, repo->packs);\n \tif (!target)\n \t\treturn -1;\n \tclose_pack_index(target);\n@@ -440,7 +441,7 @@ static int http_fetch_pack(struct walker *walker, struct alt_base *repo, unsigne\n \t\tfprintf(stderr, \"Getting pack %s\\n\",\n \t\t\thash_to_hex(target->hash));\n \t\tfprintf(stderr, \" which contains %s\\n\",\n-\t\t\thash_to_hex(sha1));\n+\t\t\toid_to_hex(oid));\n \t}\n \n \tpreq = new_http_pack_request(target->hash, repo->base);\n@@ -477,17 +478,17 @@ static void abort_object_request(struct object_request *obj_req)\n \trelease_object_request(obj_req);\n }\n \n-static int fetch_object(struct walker *walker, unsigned char *hash)\n+static int fetch_object(struct walker *walker, const struct object_id *oid)\n {\n-\tchar *hex = hash_to_hex(hash);\n+\tchar *hex = oid_to_hex(oid);\n \tint ret = 0;\n \tstruct object_request *obj_req = NULL;\n \tstruct http_object_request *req;\n \tstruct list_head *pos, *head = &object_queue_head;\n \n \tlist_for_each(pos, head) {\n \t\tobj_req = list_entry(pos, struct object_request, node);\n-\t\tif (hasheq(obj_req->oid.hash, hash, the_repository->hash_algo))\n+\t\tif (oideq(&obj_req->oid, oid))\n \t\t\tbreak;\n \t}\n \tif (!obj_req)\n@@ -548,20 +549,20 @@ static int fetch_object(struct walker *walker, unsigned char *hash)\n \treturn ret;\n }\n \n-static int fetch(struct walker *walker, unsigned char *hash)\n+static int fetch(struct walker *walker, const struct object_id *oid)\n {\n \tstruct walker_data *data = walker->data;\n \tstruct alt_base *altbase = data->alt;\n \n-\tif (!fetch_object(walker, hash))\n+\tif (!fetch_object(walker, oid))\n \t\treturn 0;\n \twhile (altbase) {\n-\t\tif (!http_fetch_pack(walker, altbase, hash))\n+\t\tif (!http_fetch_pack(walker, altbase, oid))\n \t\t\treturn 0;\n \t\tfetch_alternates(walker, data->alt->base);\n \t\taltbase = altbase->next;\n \t}\n-\treturn error(\"Unable to find %s under %s\", hash_to_hex(hash),\n+\treturn error(\"Unable to find %s under %s\", oid_to_hex(oid),\n \t\t data->alt->base);\n }\n \ndiff --git a/walker.c b/walker.c\nindex 807a7a3881..5ea7e5b392 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -157,7 +157,7 @@ static int process(struct walker *walker, struct object *obj)\n \telse {\n \t\tif (obj->flags & COMPLETE)\n \t\t\treturn 0;\n-\t\twalker->prefetch(walker, obj->oid.hash);\n+\t\twalker->prefetch(walker, &obj->oid);\n \t}\n \n \tobject_list_insert(obj, process_queue_end);\n@@ -186,7 +186,7 @@ static int loop(struct walker *walker)\n \t\t * the queue because we needed to fetch it first.\n \t\t */\n \t\tif (! (obj->flags & TO_SCAN)) {\n-\t\t\tif (walker->fetch(walker, obj->oid.hash)) {\n+\t\t\tif (walker->fetch(walker, &obj->oid)) {\n \t\t\t\tstop_progress(&progress);\n \t\t\t\treport_missing(obj);\n \t\t\t\treturn -1;\ndiff --git a/walker.h b/walker.h\nindex d40b016bab..25aaa3631c 100644\n--- a/walker.h\n+++ b/walker.h\n@@ -6,8 +6,8 @@\n struct walker {\n \tvoid *data;\n \tint (*fetch_ref)(struct walker *, struct ref *ref);\n-\tvoid (*prefetch)(struct walker *, unsigned char *sha1);\n-\tint (*fetch)(struct walker *, unsigned char *sha1);\n+\tvoid (*prefetch)(struct walker *, const struct object_id *oid);\n+\tint (*fetch)(struct walker *, const struct object_id *oid);\n \tvoid (*cleanup)(struct walker *);\n \tint get_verbosely;\n \tint get_progress;\n-- \n2.47.0.363.g6e72b256be\n\n"},{"id":"506091","messageId":"20241025070503.GI2110355@coredump.intra.peff.net","threadId":"62370","inReplyTo":"20241025064148.GA2110169@coredump.intra.peff.net","subject":"[PATCH 09/11] packfile: convert find_sha1_pack() to use object_id","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-25T07:05:03Z","receivedAt":"2024-10-25T07:05:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The find_sha1_pack() function has a few problems:\n\n - it's badly named, since it works with any object hash\n\n - it takes the hash as a bare pointer rather than an object_id struct\n\nWe can fix both of these easily, as all callers actually have a real\nobject_id anyway.\n\nI also found the existence of this function somewhat confusing, as it is\nabout looking in an arbitrary set of linked packed_git structs. It's\ngood for things like dumb-http which are looking in downloaded remote\npacks, and not our local packs. But despite the name, it is not a good\nway to find the pack which contains a local object (it skips the use of\nthe midx, the pack mru list, and so on).\n\nSo let's also add an explanatory comment above the declaration that may\npoint people in the right direction.\n\nI suspect the calls in fast-import.c, which use the packed_git list from\nthe repository struct, could actually just be using find_pack_entry().\nBut since we'd need to keep it anyway for dumb-http, I didn't dig\nfurther there. If we eventually drop dumb-http support, then it might be\nworth examining them to see if we can get rid of the function entirely.\n\nSigned-off-by: Jeff King \n---\n builtin/fast-import.c | 6 ++----\n http-push.c | 4 ++--\n http-walker.c | 2 +-\n packfile.c | 6 +++---\n packfile.h | 9 +++++++--\n 5 files changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 1e7ab67f6e..76d5c20f14 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -966,8 +966,7 @@ static int store_object(\n \tif (e->idx.offset) {\n \t\tduplicate_count_by_type[type]++;\n \t\treturn 1;\n-\t} else if (find_sha1_pack(oid.hash,\n-\t\t\t\t get_all_packs(the_repository))) {\n+\t} else if (find_oid_pack(&oid, get_all_packs(the_repository))) {\n \t\te->type = type;\n \t\te->pack_id = MAX_PACK_ID;\n \t\te->idx.offset = 1; /* just not zero! */\n@@ -1167,8 +1166,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)\n \t\tduplicate_count_by_type[OBJ_BLOB]++;\n \t\ttruncate_pack(&checkpoint);\n \n-\t} else if (find_sha1_pack(oid.hash,\n-\t\t\t\t get_all_packs(the_repository))) {\n+\t} else if (find_oid_pack(&oid, get_all_packs(the_repository))) {\n \t\te->type = OBJ_BLOB;\n \t\te->pack_id = MAX_PACK_ID;\n \t\te->idx.offset = 1; /* just not zero! */\ndiff --git a/http-push.c b/http-push.c\nindex aad89f2eab..4d24e6b8d4 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -309,7 +309,7 @@ static void start_fetch_packed(struct transfer_request *request)\n \tstruct transfer_request *check_request = request_queue_head;\n \tstruct http_pack_request *preq;\n \n-\ttarget = find_sha1_pack(request->obj->oid.hash, repo->packs);\n+\ttarget = find_oid_pack(&request->obj->oid, repo->packs);\n \tif (!target) {\n \t\tfprintf(stderr, \"Unable to fetch %s, will not be able to update server info refs\\n\", oid_to_hex(&request->obj->oid));\n \t\trepo->can_update_info_refs = 0;\n@@ -681,7 +681,7 @@ static int add_send_request(struct object *obj, struct remote_lock *lock)\n \t\tget_remote_object_list(obj->oid.hash[0]);\n \tif (obj->flags & (REMOTE | PUSHING))\n \t\treturn 0;\n-\ttarget = find_sha1_pack(obj->oid.hash, repo->packs);\n+\ttarget = find_oid_pack(&obj->oid, repo->packs);\n \tif (target) {\n \t\tobj->flags |= REMOTE;\n \t\treturn 0;\ndiff --git a/http-walker.c b/http-walker.c\nindex 36dd1f33c0..43cde0ebe5 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -432,7 +432,7 @@ static int http_fetch_pack(struct walker *walker, struct alt_base *repo,\n \n \tif (fetch_indices(walker, repo))\n \t\treturn -1;\n-\ttarget = find_sha1_pack(oid->hash, repo->packs);\n+\ttarget = find_oid_pack(oid, repo->packs);\n \tif (!target)\n \t\treturn -1;\n \tclose_pack_index(target);\ndiff --git a/packfile.c b/packfile.c\nindex 0d4fd2737a..c51eab15a5 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2010,13 +2010,13 @@ int is_pack_valid(struct packed_git *p)\n \treturn !open_packed_git(p);\n }\n \n-struct packed_git *find_sha1_pack(const unsigned char *sha1,\n-\t\t\t\t struct packed_git *packs)\n+struct packed_git *find_oid_pack(const struct object_id *oid,\n+\t\t\t\t struct packed_git *packs)\n {\n \tstruct packed_git *p;\n \n \tfor (p = packs; p; p = p->next) {\n-\t\tif (find_pack_entry_one(sha1, p))\n+\t\tif (find_pack_entry_one(oid->hash, p))\n \t\t\treturn p;\n \t}\n \treturn NULL;\ndiff --git a/packfile.h b/packfile.h\nindex ad393be9f1..3baffa940c 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -79,8 +79,13 @@ struct packed_git *get_all_packs(struct repository *r);\n */\n unsigned long repo_approximate_object_count(struct repository *r);\n \n-struct packed_git *find_sha1_pack(const unsigned char *sha1,\n-\t\t\t\t struct packed_git *packs);\n+/*\n+ * Find the pack within the \"packs\" list whose index contains the object \"oid\".\n+ * For general object lookups, you probably don't want this; use\n+ * find_pack_entry() instead.\n+ */\n+struct packed_git *find_oid_pack(const struct object_id *oid,\n+\t\t\t\t struct packed_git *packs);\n \n void pack_report(void);\n \n-- \n2.47.0.363.g6e72b256be\n\n"},{"id":"506092","messageId":"20241025070606.GJ2110355@coredump.intra.peff.net","threadId":"62370","inReplyTo":"20241025064148.GA2110169@coredump.intra.peff.net","subject":"[PATCH 10/11] packfile: use object_id in find_pack_entry_one()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-25T07:06:06Z","receivedAt":"2024-10-25T07:06:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The main function we use to search a pack index for an object is\nfind_pack_entry_one(). That function still takes a bare pointer to the\nhash, despite the fact that its underlying bsearch_pack() function needs\nan object_id struct. And so we end up making an extra copy of the hash\ninto the struct just to do a lookup.\n\nAs it turns out, all callers but one already have such an object_id. So\nwe can just take a pointer to that struct and use it directly. This\navoids the extra copy and provides a more type-safe interface.\n\nThe one exception is get_delta_base() in packfile.c, when we are chasing\na REF_DELTA from inside the pack (and thus we have a pointer directly to\nthe mmap'd pack memory, not a struct). We can just bump the hashcpy()\nfrom inside find_pack_entry_one() to this one caller that needs it.\n\nSigned-off-by: Jeff King \n---\n builtin/pack-objects.c | 4 ++--\n connected.c | 4 ++--\n midx.c | 2 +-\n pack-bitmap.c | 4 ++--\n packfile.c | 16 ++++++++--------\n packfile.h | 4 ++--\n t/helper/test-find-pack.c | 2 +-\n 7 files changed, 18 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 0fc0680b40..0800714267 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1556,7 +1556,7 @@ static int want_object_in_pack_one(struct packed_git *p,\n \tif (p == *found_pack)\n \t\toffset = *found_offset;\n \telse\n-\t\toffset = find_pack_entry_one(oid->hash, p);\n+\t\toffset = find_pack_entry_one(oid, p);\n \n \tif (offset) {\n \t\tif (!*found_pack) {\n@@ -3984,7 +3984,7 @@ static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)\n \twhile (p) {\n \t\tif ((!p->pack_local || p->pack_keep ||\n \t\t\t\tp->pack_keep_in_core) &&\n-\t\t\tfind_pack_entry_one(oid->hash, p)) {\n+\t\t\tfind_pack_entry_one(oid, p)) {\n \t\t\tlast_found = p;\n \t\t\treturn 1;\n \t\t}\ndiff --git a/connected.c b/connected.c\nindex 87cc4b57a1..a9e2e13995 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -78,7 +78,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\t\t\tif (!p->pack_promisor)\n \t\t\t\t\tcontinue;\n-\t\t\t\tif (find_pack_entry_one(oid->hash, p))\n+\t\t\t\tif (find_pack_entry_one(oid, p))\n \t\t\t\t\tgoto promisor_pack_found;\n \t\t\t}\n \t\t\t/*\n@@ -144,7 +144,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t * are sure the ref is good and not sending it to\n \t\t * rev-list for verification.\n \t\t */\n-\t\tif (new_pack && find_pack_entry_one(oid->hash, new_pack))\n+\t\tif (new_pack && find_pack_entry_one(oid, new_pack))\n \t\t\tcontinue;\n \n \t\tif (fprintf(rev_list_in, \"%s\\n\", oid_to_hex(oid)) < 0)\ndiff --git a/midx.c b/midx.c\nindex 479812cb9b..e82d4f2e65 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -987,7 +987,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t\t}\n \n \t\tm_offset = e.offset;\n-\t\tp_offset = find_pack_entry_one(oid.hash, e.p);\n+\t\tp_offset = find_pack_entry_one(&oid, e.p);\n \n \t\tif (m_offset != p_offset)\n \t\t\tmidx_report(_(\"incorrect object offset for oid[%d] = %s: %\"PRIx64\" != %\"PRIx64),\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 32b222a7af..4fa9dfc771 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -935,7 +935,7 @@ static inline int bitmap_position_packfile(struct bitmap_index *bitmap_git,\n \t\t\t\t\t const struct object_id *oid)\n {\n \tuint32_t pos;\n-\toff_t offset = find_pack_entry_one(oid->hash, bitmap_git->pack);\n+\toff_t offset = find_pack_entry_one(oid, bitmap_git->pack);\n \tif (!offset)\n \t\treturn -1;\n \n@@ -1609,7 +1609,7 @@ static int in_bitmapped_pack(struct bitmap_index *bitmap_git,\n \t\t\tif (bsearch_midx(&object->oid, bitmap_git->midx, NULL))\n \t\t\t\treturn 1;\n \t\t} else {\n-\t\t\tif (find_pack_entry_one(object->oid.hash, bitmap_git->pack) > 0)\n+\t\t\tif (find_pack_entry_one(&object->oid, bitmap_git->pack) > 0)\n \t\t\t\treturn 1;\n \t\t}\n \t}\ndiff --git a/packfile.c b/packfile.c\nindex c51eab15a5..005ca670b4 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1239,7 +1239,9 @@ off_t get_delta_base(struct packed_git *p,\n \t\t*curpos += used;\n \t} else if (type == OBJ_REF_DELTA) {\n \t\t/* The base entry _must_ be in the same pack */\n-\t\tbase_offset = find_pack_entry_one(base_info, p);\n+\t\tstruct object_id oid;\n+\t\thashcpy(oid.hash, base_info, the_repository->hash_algo);\n+\t\tbase_offset = find_pack_entry_one(&oid, p);\n \t\t*curpos += the_hash_algo->rawsz;\n \t} else\n \t\tdie(\"I am totally screwed\");\n@@ -1971,20 +1973,18 @@ off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)\n \t}\n }\n \n-off_t find_pack_entry_one(const unsigned char *sha1,\n-\t\t\t\t struct packed_git *p)\n+off_t find_pack_entry_one(const struct object_id *oid,\n+\t\t\t struct packed_git *p)\n {\n \tconst unsigned char *index = p->index_data;\n-\tstruct object_id oid;\n \tuint32_t result;\n \n \tif (!index) {\n \t\tif (open_pack_index(p))\n \t\t\treturn 0;\n \t}\n \n-\thashcpy(oid.hash, sha1, the_repository->hash_algo);\n-\tif (bsearch_pack(&oid, p, &result))\n+\tif (bsearch_pack(oid, p, &result))\n \t\treturn nth_packed_object_offset(p, result);\n \treturn 0;\n }\n@@ -2016,7 +2016,7 @@ struct packed_git *find_oid_pack(const struct object_id *oid,\n \tstruct packed_git *p;\n \n \tfor (p = packs; p; p = p->next) {\n-\t\tif (find_pack_entry_one(oid->hash, p))\n+\t\tif (find_pack_entry_one(oid, p))\n \t\t\treturn p;\n \t}\n \treturn NULL;\n@@ -2033,7 +2033,7 @@ static int fill_pack_entry(const struct object_id *oid,\n \t oidset_contains(&p->bad_objects, oid))\n \t\treturn 0;\n \n-\toffset = find_pack_entry_one(oid->hash, p);\n+\toffset = find_pack_entry_one(oid, p);\n \tif (!offset)\n \t\treturn 0;\n \ndiff --git a/packfile.h b/packfile.h\nindex 3baffa940c..08f88a7ff5 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -154,10 +154,10 @@ int nth_packed_object_id(struct object_id *, struct packed_git *, uint32_t n);\n off_t nth_packed_object_offset(const struct packed_git *, uint32_t n);\n \n /*\n- * If the object named sha1 is present in the specified packfile,\n+ * If the object named by oid is present in the specified packfile,\n * return its offset within the packfile; otherwise, return 0.\n */\n-off_t find_pack_entry_one(const unsigned char *sha1, struct packed_git *);\n+off_t find_pack_entry_one(const struct object_id *oid, struct packed_git *);\n \n int is_pack_valid(struct packed_git *);\n void *unpack_entry(struct repository *r, struct packed_git *, off_t, enum object_type *, unsigned long *);\ndiff --git a/t/helper/test-find-pack.c b/t/helper/test-find-pack.c\nindex 14b2b0c12c..85a69a4e55 100644\n--- a/t/helper/test-find-pack.c\n+++ b/t/helper/test-find-pack.c\n@@ -40,7 +40,7 @@ int cmd__find_pack(int argc, const char **argv)\n \t\tdie(\"cannot parse %s as an object name\", argv[0]);\n \n \tfor (p = get_all_packs(the_repository); p; p = p->next)\n-\t\tif (find_pack_entry_one(oid.hash, p)) {\n+\t\tif (find_pack_entry_one(&oid, p)) {\n \t\t\tprintf(\"%s\\n\", p->pack_name);\n \t\t\tactual_count++;\n \t\t}\n-- \n2.47.0.363.g6e72b256be\n\n"},{"id":"506093","messageId":"20241025070810.GK2110355@coredump.intra.peff.net","threadId":"62370","inReplyTo":"20241025064148.GA2110169@coredump.intra.peff.net","subject":"[PATCH 11/11] packfile: use oidread() instead of hashcpy() to fill object_id","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-25T07:08:10Z","receivedAt":"2024-10-25T07:08:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When chasing a REF_DELTA, we need to pull the raw hash bytes out of the\nmmap'd packfile into an object_id struct. We do that with a raw\nhashcpy() of the appropriate length (that happens directly now, though\nbefore the previous commit it happened inside find_pack_entry_one(),\nalso using a hashcpy).\n\nBut I think this creates a potentially dangerous situation due to\nd4d364b2c7 (hash: convert `oidcmp()` and `oideq()` to compare whole\nhash, 2024-06-14). When using sha1, we'll have uninitialized bytes in\nthe latter part of the object_id.hash buffer, which could fool oideq(),\netc.\n\nWe should use oidread() instead, which correctly zero-pads the extra\nbytes, as of c98d762ed9 (global: ensure that object IDs are always\npadded, 2024-06-14).\n\nAs far as I can see, this has not been a problem in practice because the\nobject_id we feed to find_pack_entry_one() is never used with oideq(),\netc. It is being compared to the bytes mmap'd from a pack idx file,\nwhich of course do not have the extra padding bytes themselves. So\nthere's no bug here, but this just puzzled me while looking at the code.\nWe should do the more obviously safe thing, both for future-proofing and\nto avoid confusing readers.\n\nSigned-off-by: Jeff King \n---\n+cc Patrick for any wisdom here. I'd guess that the conversions from\nc98d762ed9 were found by using ASan, valgrind, or similar with the new\noideq() implementation. And so this case, because it actually is safe,\nwas not flagged.\n\n packfile.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 005ca670b4..9560f0a33c 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1240,7 +1240,7 @@ off_t get_delta_base(struct packed_git *p,\n \t} else if (type == OBJ_REF_DELTA) {\n \t\t/* The base entry _must_ be in the same pack */\n \t\tstruct object_id oid;\n-\t\thashcpy(oid.hash, base_info, the_repository->hash_algo);\n+\t\toidread(&oid, base_info, the_repository->hash_algo);\n \t\tbase_offset = find_pack_entry_one(&oid, p);\n \t\t*curpos += the_hash_algo->rawsz;\n \t} else\n-- \n2.47.0.363.g6e72b256be\n"},{"id":"506123","messageId":"ZxwJHY/BQJvpGT1s@nand.local","threadId":"62370","inReplyTo":"20241025064340.GA2110355@coredump.intra.peff.net","subject":"Re: [PATCH 01/11] midx: avoid duplicate packed_git entries","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-25T21:09:49Z","receivedAt":"2024-10-25T21:09:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 25, 2024 at 02:43:40AM -0400, Jeff King wrote:\n> +cc Stolee here as the original midx author. I can't think of a good\n> reason we'd want to avoid this dup-detection here.\n>\n> midx.c | 20 +++++++++++++++++---\n> t/t5200-update-server-info.sh | 8 ++++++++\n> 2 files changed, 25 insertions(+), 3 deletions(-)\n\n:-). I swear I have written this exact patch before. I think this was\nway back in the early days of MIDX bitmaps, and working around this bug\nwas the reason that we don't generate the MIDX/bitmap at the end of a\nrepack in the same process.\n\nI don't recall all of the details at the time (or even if the above\nrationale was what I was thinking back then or not), but I think the\nchange below here is obviously correct.\n\nThanks,\nTaylor\n"},{"id":"506125","messageId":"ZxwLOxT17OhLX/Rd@nand.local","threadId":"62370","inReplyTo":"20241025065806.GC2110355@coredump.intra.peff.net","subject":"Re: [PATCH 03/11] dumb-http: store downloaded pack idx as tempfile","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-25T21:18:51Z","receivedAt":"2024-10-25T21:18:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 25, 2024 at 02:58:06AM -0400, Jeff King wrote:\n> This patch fixes a regression in b1b8dfde69 (finalize_object_file():\n> implement collision check, 2024-09-26) where fetching a v1 pack idx file\n> over the dumb-http protocol would cause the fetch to fail.\n\nMakes sense, and matches our discussion from elsewhere on the list\nsometime last week.\n\n> There are a few options to fix it:\n>\n> - we could teach index-pack a command-line option to ignore only pack\n> idx collisions, and use it when the dumb-http code invokes\n> index-pack. This would be an awkward thing to expose users to and\n> would involve a lot of boilerplate to get the option down to the\n> collision code.\n>\n> - we could delete the remote .idx file right before running\n> index-pack. It should be redundant at that point (since we've just\n> downloaded the matching pack). But it feels risky to delete\n> something from our own .git/objects based on what the other side has\n> said. I'm not entirely positive that a malicious server couldn't lie\n> about which pack-$hash.idx it has and get us to delete something\n> precious.\n>\n> - we can stop co-mingling the downloaded idx files in our local\n> objects directory. This is a slightly bigger change but I think\n> fixes the root of the problem more directly.\n>\n> This patch implements the third option. The big design questions are:\n> where do we store the downloaded files, and how do we manage their\n> lifetimes?\n\nYep, the third option is definitely the most sensible here.\n\n> I think the right solution here is probably some more complex cache\n> management system: download the remote .idx files to their own storage\n> directory, mark them as \"seen\" when we get their matching pack (to avoid\n> re-downloading even if we repack), and then delete them when the\n> server's objects/info/refs no longer mentions them.\n>\n> But since the dumb http protocol is so ancient and so inferior to the\n> smart http protocol, I don't think it's worth spending a lot of time\n> creating such a system. For this patch I'm just downloading the idx\n> files to .git/objects/tmp_pack_*, and marking them as tempfiles to be\n> deleted when we exit (and due to the name, any we miss due to a crash,\n> etc, should eventually be removed by \"git gc\" runs based on timestamps).\n>\n> That is slightly worse for one case: [...]\n\nOK. I think the more robust version of the solution you identified here\nmakes sense and is probably the right thing to do if we wanted to spend\nmore time and effort on fixing this part of the dumb HTTP protocol.\n\nBut I'm willing to believe that there are so few users of the protocol\nat this point that it's almost certainly not worth the effort. So I'm\nsupportive of the stopping point that you chose here.\n\n> diff --git a/http.c b/http.c\n> index d59e59f66b..9642cad2e3 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -19,6 +19,7 @@\n> #include \"string-list.h\"\n> #include \"object-file.h\"\n> #include \"object-store-ll.h\"\n> +#include \"tempfile.h\"\n>\n> static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);\n> static int trace_curl_data = 1;\n> @@ -2388,8 +2389,24 @@ static char *fetch_pack_index(unsigned char *hash, const char *base_url)\n> \tstrbuf_addf(&buf, \"objects/pack/pack-%s.idx\", hash_to_hex(hash));\n> \turl = strbuf_detach(&buf, NULL);\n>\n> -\tstrbuf_addf(&buf, \"%s.temp\", sha1_pack_index_name(hash));\n> -\ttmp = strbuf_detach(&buf, NULL);\n> +\t/*\n> +\t * Don't put this into packs/, since it's just temporary and we don't\n> +\t * want to confuse it with our local .idx files. We'll generate our\n> +\t * own index if we choose to download the matching packfile.\n\nI actually think putting it in $GIT_DIR/objects/pack is fine and perhaps\npreferable, since that's where we put existing in-progress temporary\nfiles. We'll ignore anything that doesn't end with \"*.idx\", so I think\nit would be fine.\n\nI don't feel strongly about it. It just feels a little weird to stick\nthese temporary files in one place, and stick other (similar) temporary\nflies in another.\n\n> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\n> index 3968b82260..706540ab74 100755\n> --- a/t/t5550-http-fetch-dumb.sh\n> +++ b/t/t5550-http-fetch-dumb.sh\n> @@ -335,7 +335,7 @@ test_expect_success 'fetch can handle previously-fetched .idx files' '\n> \tcount_fetches 1 pack one.trace &&\n> \tGIT_TRACE_CURL=$PWD/two.trace git --git-dir=clone_packed_branches.git \\\n> \t\tfetch \"$HTTPD_URL\"/dumb/repo_packed_branches.git branch2:branch2 &&\n> -\tcount_fetches 0 idx two.trace &&\n> +\tcount_fetches 1 idx two.trace &&\n> \tcount_fetches 1 pack two.trace\n> '\n>\n> @@ -521,4 +521,14 @@ test_expect_success 'fetching via http alternates works' '\n> \tgit -c http.followredirects=true clone \"$HTTPD_URL/dumb/alt-child.git\"\n> '\n>\n> +test_expect_success 'dumb http can fetch index v1' '\n> +\tserver=$HTTPD_DOCUMENT_ROOT_PATH/idx-v1.git &&\n> +\tgit init --bare \"$server\" &&\n> +\tgit -C \"$server\" --work-tree=. commit --allow-empty -m foo &&\n> +\tgit -C \"$server\" -c pack.indexVersion=1 gc &&\n> +\n> +\tgit clone \"$HTTPD_URL/dumb/idx-v1.git\" &&\n> +\tgit -C idx-v1 fsck\n> +'\n\nVery nicely done.\n\nThanks,\nTaylor\n"},{"id":"506126","messageId":"ZxwNP1hKyi4eGHNO@nand.local","threadId":"62370","inReplyTo":"20241025070009.GD2110355@coredump.intra.peff.net","subject":"Re: [PATCH 04/11] packfile: drop has_pack_index()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-25T21:27:27Z","receivedAt":"2024-10-25T21:27:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 25, 2024 at 03:00:09AM -0400, Jeff King wrote:\n> ---\n> http.c | 15 ++++++++-------\n> packfile.c | 8 --------\n> packfile.h | 2 --\n> 3 files changed, 8 insertions(+), 17 deletions(-)\n\nAll makes sense. Good, let's keep reading...\n\nThanks,\nTaylor\n"},{"id":"506127","messageId":"ZxwNkGlnwX6esIEE@nand.local","threadId":"62370","inReplyTo":"20241025070222.GG2110355@coredump.intra.peff.net","subject":"Re: [PATCH 07/11] packfile: warn people away from parse_packed_git()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-25T21:28:48Z","receivedAt":"2024-10-25T21:28:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 25, 2024 at 03:02:22AM -0400, Jeff King wrote:\n> With a name like parse_packed_git(), you might think it's the right way\n> to access a local pack index and its associated objects. But not so!\n> It's a one-off used by the dumb-http code to access pack idx files we've\n> downloaded from the remote, but whose packs we might not have.\n>\n> There's only one caller left for this function, and ideally we'd drop it\n> completely and just inline it there. But that would require exposing\n> other internals from packfile.[ch], like alloc_packed_git() and\n> check_packed_git_idx().\n>\n> So let's leave it be for now, and just warn people that it's probably\n> not what they're looking for. Perhaps in the long run if we eventually\n> drop dumb-http support, we can remove the function entirely then.\n\nThat seems like a good step and the right place to stop rather than\nexposing more internals from packfile.c. Nicely done.\n\nThanks,\nTaylor\n"},{"id":"506128","messageId":"ZxwOs23zYByRmMap@nand.local","threadId":"62370","inReplyTo":"20241025070606.GJ2110355@coredump.intra.peff.net","subject":"Re: [PATCH 10/11] packfile: use object_id in find_pack_entry_one()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-25T21:33:39Z","receivedAt":"2024-10-25T21:33:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 25, 2024 at 03:06:06AM -0400, Jeff King wrote:\n> The one exception is get_delta_base() in packfile.c, when we are chasing\n> a REF_DELTA from inside the pack (and thus we have a pointer directly to\n> the mmap'd pack memory, not a struct). We can just bump the hashcpy()\n> from inside find_pack_entry_one() to this one caller that needs it.\n\nMakes sense.\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 0fc0680b40..0800714267 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1556,7 +1556,7 @@ static int want_object_in_pack_one(struct packed_git *p,\n> \tif (p == *found_pack)\n> \t\toffset = *found_offset;\n> \telse\n> -\t\toffset = find_pack_entry_one(oid->hash, p);\n> +\t\toffset = find_pack_entry_one(oid, p);\n\nThis and all of the similar changes that follow it are trivially correct\nand pleasing to read.\n\n> diff --git a/packfile.c b/packfile.c\n> index c51eab15a5..005ca670b4 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -1239,7 +1239,9 @@ off_t get_delta_base(struct packed_git *p,\n> \t\t*curpos += used;\n> \t} else if (type == OBJ_REF_DELTA) {\n> \t\t/* The base entry _must_ be in the same pack */\n> -\t\tbase_offset = find_pack_entry_one(base_info, p);\n> +\t\tstruct object_id oid;\n> +\t\thashcpy(oid.hash, base_info, the_repository->hash_algo);\n> +\t\tbase_offset = find_pack_entry_one(&oid, p);\n\nHere's the one that needed to turn its bare hash into an object_id that\nit can pass a pointer to. And...\n\n> @@ -1971,20 +1973,18 @@ off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)\n> \t}\n> }\n>\n> -off_t find_pack_entry_one(const unsigned char *sha1,\n> -\t\t\t\t struct packed_git *p)\n> +off_t find_pack_entry_one(const struct object_id *oid,\n> +\t\t\t struct packed_git *p)\n> {\n> \tconst unsigned char *index = p->index_data;\n> -\tstruct object_id oid;\n> \tuint32_t result;\n>\n> \tif (!index) {\n> \t\tif (open_pack_index(p))\n> \t\t\treturn 0;\n> \t}\n>\n> -\thashcpy(oid.hash, sha1, the_repository->hash_algo);\n\n...here's the original location of that hashcpy() that moved to its only\nuseful caller in get_delta_base(). Makes sense.\n\nThe remaining conversions are trivial.\n\nThanks,\nTaylor\n"},{"id":"506129","messageId":"ZxwPCWuhrZoSJg1v@nand.local","threadId":"62370","inReplyTo":"20241025064148.GA2110169@coredump.intra.peff.net","subject":"Re: [PATCH 0/11] dumb-http pack index v1 regression + cleanups","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-25T21:35:05Z","receivedAt":"2024-10-25T21:35:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 25, 2024 at 02:41:48AM -0400, Jeff King wrote:\n> 16 files changed, 153 insertions(+), 101 deletions(-)\n\nI was glad that you worked on this, because I figured I might end up\n~tricking you into~ getting you to take a closer look at some of the\nother potential fixups in the dumb http code.\n\nThe series was a pleasant read, and it looks quite good to me. Let's\nwait for some other reviewers to chime in before we start merging this\ndown, but otherwise I think this is good to go.\n\nThanks,\nTaylor\n"},{"id":"506138","messageId":"20241026060238.GA2227062@coredump.intra.peff.net","threadId":"62370","inReplyTo":"ZxwLOxT17OhLX/Rd@nand.local","subject":"Re: [PATCH 03/11] dumb-http: store downloaded pack idx as tempfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-26T06:02:38Z","receivedAt":"2024-10-26T06:02:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 25, 2024 at 05:18:51PM -0400, Taylor Blau wrote:\n\n> > -\tstrbuf_addf(&buf, \"%s.temp\", sha1_pack_index_name(hash));\n> > -\ttmp = strbuf_detach(&buf, NULL);\n> > +\t/*\n> > +\t * Don't put this into packs/, since it's just temporary and we don't\n> > +\t * want to confuse it with our local .idx files. We'll generate our\n> > +\t * own index if we choose to download the matching packfile.\n> \n> I actually think putting it in $GIT_DIR/objects/pack is fine and perhaps\n> preferable, since that's where we put existing in-progress temporary\n> files. We'll ignore anything that doesn't end with \"*.idx\", so I think\n> it would be fine.\n\nBut the new tempfile _does_ end with .idx. And it (kinda) has to,\nbecause that's an assumption made by packed_git: it stores\npath/to/foo.pack, and then assumes it can access /path/to/foo.idx as\nneeded.\n\nI say \"kinda\" because as I mentioned elsewhere, the prior code cheats a\nbit and assumes that we can access the idx file through the original\nmmap we made, even if the \".idx\" the packed_git mentions does not yet\nexist. I _think_ that's pretty foolproof and that we'd never unmap or\nclose an idx (though we certainly do for packs themselves). But it\ndoesn't feel like a great thing to rely on.\n\n> I don't feel strongly about it. It just feels a little weird to stick\n> these temporary files in one place, and stick other (similar) temporary\n> flies in another.\n\nOne difference is that these are pure tempfiles. We'll never \"finalize\"\nthem by renaming them into place, so there's no chance of having to do a\ncross-directory link/rename. They just get created and then deleted[1].\n\nPerhaps something like \"objects/temp-remote-index/*.idx\" would be a\nmore descriptive name (and certainly what I'd use if making a more full\ncaching system), but there are complications:\n\n 1. We don't have good cleanup routines for directories. Would that\n just become a semi-permanent empty directory in a dumb-http repo?\n\n 2. git-prune cleans up tmp_* in objects/ and objects/pack, but would\n have to learn to look there, too.\n\nSo just sticking them in objects/ seemed like the path of least\nresistance to me.\n\n-Peff\n\n[1] They really could be opened anywhere, like /tmp. Which I was tempted\n to do, but since they're non-trivial in size it seemed like keeping\n them inside the repo directory was the best bet.\n\n Curiously, if we lean into the \"we will never close a mapped idx\",\n then we could create them, open and map them, and then delete them\n immediately, which is a somewhat common tempfile idiom. At least on\n Unix systems. I'm not sure how Windows would like that. :)\n"},{"id":"506181","messageId":"Zx7XcLlMAF68t1ZZ@nand.local","threadId":"62370","inReplyTo":"20241026060238.GA2227062@coredump.intra.peff.net","subject":"Re: [PATCH 03/11] dumb-http: store downloaded pack idx as tempfile","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-28T00:14:40Z","receivedAt":"2024-10-28T00:14:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Oct 26, 2024 at 02:02:38AM -0400, Jeff King wrote:\n> So just sticking them in objects/ seemed like the path of least\n> resistance to me.\n\nOK, reading it all back, I tend to agree that sticking them in objects/\nmakes sense. Thanks!\n\nThanks,\nTaylor\n"}]}