{"thread":{"id":"38459","subject":"[PATCH] Add failing test for fetching from multiple packs over dumb httpd","startedAt":"2015-01-27T15:20:41Z","lastAt":"2015-01-27T20:46:44Z","messageCount":6,"participants":["Charles Bailey","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"255326","messageId":"1422372041-16474-1-git-send-email-charles@hashpling.org","threadId":"38459","inReplyTo":null,"subject":"[PATCH] Add failing test for fetching from multiple packs over dumb httpd","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-01-27T15:20:41Z","receivedAt":"2015-01-27T15:20:41Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"From: Charles Bailey <cbailey32@bloomberg.net>\n\nWhen objects are spread across multiple packs, if an initial fetch does\nrequire all pack files, a subsequent fetch for objects in packs not\nretrieved in the initial fetch will fail.\n---\n\nI'm not very familiar with the http client code so this analysis is based\npurely on observed behaviour.\n\nWhen fetching only some refs from a repository served over dumb httpd Git\nappears to download all of the index files for the available packs but then\nonly chooses the pack files that help it resolve the objects which we need.\n\nIf we then later try to fetch an object which is in a pack file for\nwhich Git has previously downloaded an index file, it seems to trip because it\nbelieves it already has the object locally due to the presence of the index\nfile but doesn't actually have it because it never retrieved the corresponding\npack file. It reports an error of the form \"Cannot obtain needed object ...\".\n\nManually deleting index files which have no corresponding local pack\nfile will allow a repeat of the failed fetch to succeed.\n\n t/t5550-http-fetch-dumb.sh | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex ac71418..cf2362a 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -165,6 +165,24 @@ test_expect_success 'fetch notices corrupt idx' '\n \t)\n '\n \n+test_expect_failure 'fetch packed branches' '\n+\tgit checkout --orphan branch1 &&\n+\techo base >file &&\n+\tgit add file &&\n+\tgit commit -m base &&\n+\tgit --bare init \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_packed_branches.git &&\n+\tgit push \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_packed_branches.git branch1 &&\n+\tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_packed_branches.git repack -d &&\n+\tgit checkout -b branch2 branch1 &&\n+\techo b2 >>file &&\n+\tgit commit -a -m b2 &&\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+'\n+\n test_expect_success 'did not use upload-pack service' '\n \tgrep '/git-upload-pack' <\"$HTTPD_ROOT_PATH\"/access.log >act\n \t: >exp\n-- \n2.0.2.611.g8c85416\n"},{"id":"255339","messageId":"20150127181220.GA17067@peff.net","threadId":"38459","inReplyTo":"1422372041-16474-1-git-send-email-charles@hashpling.org","subject":"Re: [PATCH] Add failing test for fetching from multiple packs over dumb httpd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-27T18:12:21Z","receivedAt":"2015-01-27T18:12:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 27, 2015 at 03:20:41PM +0000, Charles Bailey wrote:\n\n> From: Charles Bailey <cbailey32@bloomberg.net>\n> \n> When objects are spread across multiple packs, if an initial fetch does\n> require all pack files, a subsequent fetch for objects in packs not\n> retrieved in the initial fetch will fail.\n\ns/does/does not/, I think?\n\n> I'm not very familiar with the http client code so this analysis is based\n> purely on observed behaviour.\n\nDebugging the http code is a royal pain because all the work happens in\na separate helper. I use a git-remote-debug script like this:\n\n  #!/bin/sh\n  host=localhost:5001\n  proto=$(echo \"${2:-$1}\" | sed 's/:.*//')\n  prog=git-remote-$proto\n  echo >&2 \"gdb -ex 'target remote $host' $prog\"\n  gdbserver localhost:5001 \"$prog\" \"$@\"\n\nand then you can use:\n\n  git fetch debug::http://...\n\nin the test script, cut-and-paste the gdb command printed to stderr, and\nyou're dropped into the appropriate debugger without worrying about all\nof the stdio mess.\n\n> When fetching only some refs from a repository served over dumb httpd Git\n> appears to download all of the index files for the available packs but then\n> only chooses the pack files that help it resolve the objects which we need.\n\nRight. And it looks like we have special code in sha1_file.c to make\nsure we do not trust an index which does not have a matching packfile.\nSo that's good.\n\nThe http-walker code does its own check, in fetch_and_setup_pack_index,\nthat checks for an existing valid copy of the index. If we don't have\nit, we download the index and proceed. If we do, we skip straight to\ngrabbing the pack. But if we have it and it doesn't appear valid, we\nreturn an error. And there seems to be a bug with checking the validity.\n\nIt looks like the culprit is 7b64469 (Allow parse_pack_index on\ntemporary files, 2010-04-19). It added a new \"idx_path\" parameter to\nparse_pack_index, which we pass as NULL.  That causes its call to\ncheck_packed_git_idx to fail (because it has no idea what file we are\ntalking about!).\n\nThis seems to fix it:\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 30995e6..eda4d90 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1149,6 +1149,9 @@ struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)\n \tconst char *path = sha1_pack_name(sha1);\n \tstruct packed_git *p = alloc_packed_git(strlen(path) + 1);\n \n+\tif (!idx_path)\n+\t\tidx_path = sha1_pack_index_name(sha1);\n+\n \tstrcpy(p->pack_name, path);\n \thashcpy(p->sha1, sha1);\n \tif (check_packed_git_idx(idx_path, p)) {\n\n(Alternatively, we could pass in sha1_pack_index_name instead of NULL in\nthe first place, but I think it is reasonable for parse_pack_index to\ntake care of this).\n\nI think it may also make sense for fetch_and_setup_pack_index to delete\nand re-download a broken .idx file (rather than aborting), but I don't\nthink that's a big deal. It should only happen in the face of on-disk\ndata corruption, and the user can remove the broken .idx themselves.\n\n-Peff\n"},{"id":"255340","messageId":"20150127182939.GA18236@hashpling.org","threadId":"38459","inReplyTo":"20150127181220.GA17067@peff.net","subject":"Re: [PATCH] Add failing test for fetching from multiple packs over dumb httpd","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-01-27T18:29:39Z","receivedAt":"2015-01-27T18:29:39Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Tue, Jan 27, 2015 at 01:12:21PM -0500, Jeff King wrote:\n> On Tue, Jan 27, 2015 at 03:20:41PM +0000, Charles Bailey wrote:\n> \n> > From: Charles Bailey <cbailey32@bloomberg.net>\n> > \n> > When objects are spread across multiple packs, if an initial fetch does\n> > require all pack files, a subsequent fetch for objects in packs not\n> > retrieved in the initial fetch will fail.\n> \n> s/does/does not/, I think?\n\nYes, that's definitely what I meant to write.\n\n[...]\n> It looks like the culprit is 7b64469 (Allow parse_pack_index on\n> temporary files, 2010-04-19). It added a new \"idx_path\" parameter to\n> parse_pack_index, which we pass as NULL.  That causes its call to\n> check_packed_git_idx to fail (because it has no idea what file we are\n> talking about!).\n\nThat change looks like it went into 1.7.1.1. I cannot confirm this\nworking before then but we've definitely seen the bug in 1.7.12.3 and\nmore recent versions.\n\n> This seems to fix it:\n> \n> diff --git a/sha1_file.c b/sha1_file.c\n> index 30995e6..eda4d90 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1149,6 +1149,9 @@ struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)\n>  \tconst char *path = sha1_pack_name(sha1);\n>  \tstruct packed_git *p = alloc_packed_git(strlen(path) + 1);\n>  \n> +\tif (!idx_path)\n> +\t\tidx_path = sha1_pack_index_name(sha1);\n> +\n>  \tstrcpy(p->pack_name, path);\n>  \thashcpy(p->sha1, sha1);\n>  \tif (check_packed_git_idx(idx_path, p)) {\n\nIt certainly fixes my test script and I can give this patch a test in\nthe 'real' world.\n"},{"id":"255343","messageId":"20150127200226.GA19618@peff.net","threadId":"38459","inReplyTo":"20150127181220.GA17067@peff.net","subject":"[PATCH] dumb-http: do not pass NULL path to parse_pack_index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-27T20:02:27Z","receivedAt":"2015-01-27T20:02:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 27, 2015 at 01:12:20PM -0500, Jeff King wrote:\n\n> It looks like the culprit is 7b64469 (Allow parse_pack_index on\n> temporary files, 2010-04-19). It added a new \"idx_path\" parameter to\n> parse_pack_index, which we pass as NULL.  That causes its call to\n> check_packed_git_idx to fail (because it has no idea what file we are\n> talking about!).\n\nThis is not quite right. That refactoring commit correctly passes the\nresult of sha1_pack_index_name() for the newly-added parameter. It's the\n_next_ commit which then erroneously passes NULL. :)\n\nHere's my fix with a proper commit message. I tweaked the title on your\ntest, as I didn't find the original very descriptive. I also bumped the\ncall to sha1_pack_index_name() into the caller, as that makes more sense\nto me after reading the older commits (and there are literally no other\ncallers outside of this function).\n\n-- >8 --\nOnce upon a time, dumb http always fetched .idx files\ndirectly into their final location, and then checked their\nvalidity with parse_pack_index. This was refactored in\ncommit 750ef42 (http-fetch: Use temporary files for\npack-*.idx until verified, 2010-04-19), which uses the\nfollowing logic:\n\n  1. If we have the idx already in place, see if it's\n     valid (using parse_pack_index). If so, use it.\n\n  2. Otherwise, fetch the .idx to a tempfile, check\n     that, and if so move it into place.\n\n  3. Either way, fetch the pack itself if necessary.\n\nHowever, it got step 1 wrong. We pass a NULL path parameter\nto parse_pack_index, so an existing .idx file always looks\nbroken. Worse, we do not treat this broken .idx as an\nopportunity to re-fetch, but instead return an error,\nignoring the pack entirely. This can lead to a dumb-http\nfetch failing to retrieve the necessary objects.\n\nThis doesn't come up much in practice, because it must be a\npackfile that we found out about (and whose .idx we stored)\nduring an earlier dumb-http fetch, but whose packfile we\n_didn't_ fetch. I.e., we did a partial clone of a\nrepository, didn't need some packfiles, and now a followup\nfetch needs them.\n\nDiscovery and tests by Charles Bailey <charles@hashpling.org>.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm happy to flip the authorship on this. You have more lines in it than\nI do. :)\n\nAs I mentioned before, I think we could treat a broken .idx file\ndifferently (by deleting and refetching), but I doubt it matters in\npractice. The only reason this came up at all is that our test for\n\"broken\" was bogus, and it may be better to let a user diagnose and\ndeal with breakage.\n\n http.c                     |  2 +-\n t/t5550-http-fetch-dumb.sh | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/http.c b/http.c\nindex 040f362..f2373c0 100644\n--- a/http.c\n+++ b/http.c\n@@ -1240,7 +1240,7 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n \tint ret;\n \n \tif (has_pack_index(sha1)) {\n-\t\tnew_pack = parse_pack_index(sha1, NULL);\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;\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex ac71418..6da9422 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -165,6 +165,24 @@ test_expect_success 'fetch notices corrupt idx' '\n \t)\n '\n \n+test_expect_success 'fetch can handle previously-fetched .idx files' '\n+\tgit checkout --orphan branch1 &&\n+\techo base >file &&\n+\tgit add file &&\n+\tgit commit -m base &&\n+\tgit --bare init \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_packed_branches.git &&\n+\tgit push \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_packed_branches.git branch1 &&\n+\tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_packed_branches.git repack -d &&\n+\tgit checkout -b branch2 branch1 &&\n+\techo b2 >>file &&\n+\tgit commit -a -m b2 &&\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+'\n+\n test_expect_success 'did not use upload-pack service' '\n \tgrep '/git-upload-pack' <\"$HTTPD_ROOT_PATH\"/access.log >act\n \t: >exp\n-- \n2.3.0.rc1.287.g761fd19\n"},{"id":"255345","messageId":"20150127201921.GA24365@hashpling.org","threadId":"38459","inReplyTo":"20150127200226.GA19618@peff.net","subject":"Re: [PATCH] dumb-http: do not pass NULL path to parse_pack_index","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-01-27T20:19:21Z","receivedAt":"2015-01-27T20:19:21Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Tue, Jan 27, 2015 at 03:02:27PM -0500, Jeff King wrote:\n> Discovery and tests by Charles Bailey <charles@hashpling.org>.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I'm happy to flip the authorship on this. You have more lines in it than\n> I do. :)\n\nNo, I'm happy with you taking the blame/praise for this, it's definitely\nyour fix.\n"},{"id":"255348","messageId":"xmqq386wkl3f.fsf@gitster.dls.corp.google.com","threadId":"38459","inReplyTo":"20150127201921.GA24365@hashpling.org","subject":"Re: [PATCH] dumb-http: do not pass NULL path to parse_pack_index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-27T20:46:44Z","receivedAt":"2015-01-27T20:46:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> On Tue, Jan 27, 2015 at 03:02:27PM -0500, Jeff King wrote:\n>> Discovery and tests by Charles Bailey <charles@hashpling.org>.\n>> \n>> Signed-off-by: Jeff King <peff@peff.net>\n>> ---\n>> I'm happy to flip the authorship on this. You have more lines in it than\n>> I do. :)\n>\n> No, I'm happy with you taking the blame/praise for this, it's definitely\n> your fix.\n\nLooks good (rather, makes the original look obviously broken and\nmakes me wonder why our review process did not catch that bogus NULL\nin the first place).\n\nThanks, both.\n"}]}