{"thread":{"id":"52984","subject":"[PATCH] connected.c: reprepare packs for corner cases","startedAt":"2020-03-12T15:36:49Z","lastAt":"2020-03-13T21:12:02Z","messageCount":12,"participants":["Derrick Stolee via GitGitGadget","Jonathan Tan","Derrick Stolee","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"393132","messageId":"pull.579.git.1584027403779.gitgitgadget@gmail.com","threadId":"52984","inReplyTo":null,"subject":"[PATCH] connected.c: reprepare packs for corner cases","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-12T15:36:43Z","receivedAt":"2020-03-12T15:36:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nWhile updating the microsoft/git fork on top of v2.26.0-rc0 and\nconsuming that build into Scalar, I noticed a corner case bug around\npartial clone.\n\nThe \"scalar clone\" command can create a Git repository with the\nproper config for using partial clone with the \"blob:none\" filter.\nInstead of calling \"git clone\", it runs \"git init\" then sets a few\nmore config values before running \"git fetch\".\n\nIn our builds on v2.26.0-rc0, we noticed that our \"git fetch\"\ncommand was failing with\n\n  error: https://github.com/microsoft/scalar did not send all necessary objects\n\nThis does not happen if you copy the config file from a repository\ncreated by \"git clone --filter=blob:none <url>\", but it does happen\nwhen adding the config option \"core.logAllRefUpdates = true\".\n\nBy debugging, I was able to see that the loop inside\ncheck_connnected() that checks if all refs are contained in\npromisor packs actually did not have any packfiles in the packed_git\nlist.\n\nI'm not sure what corner-case issues caused this config option to\nprevent the reprepare_packed_git() from being called at the proper\nspot during the fetch operation. Even worse, I have failed to create\na test case to prevent a regression.\n\nPlacing a reprepare_packed_git() call inside chck_connected() before\nlooping through the packed_git list seems like the safest way to\navoid this issue in the future. While reprepare_packed_git() does\nanother scan of the pack directory, it is not terribly expensive as\nlong as we do not run it in a loop. We check connectivity only a\nfew times per command, so this will not have a meaningful performance\nimpact. In exchange, we get extra safety around this check.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n    connected.c: reprepare packs for corner cases\n    \n    I included how I found this (integrating v2.26.0-rc0 into Scalar), but I\n    am able to reproduce it on my Linux machine using real fetches from\n    github.com. I'm not sure why I was unable to reproduce the issue in test\n    cases using the file:// URLs or the HTTP tests.\n    \n    Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-579%2Fderrickstolee%2Ffetch-reprepare-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-579/derrickstolee/fetch-reprepare-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/579\n\n connected.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/connected.c b/connected.c\nindex 7e9bd1bc622..ac52b07b474 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -61,7 +61,11 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t * object is a promisor object. Instead, just make sure we\n \t\t * received, in a promisor packfile, the objects pointed to by\n \t\t * each wanted ref.\n+\t\t *\n+\t\t * Before checking for promisor packs, be sure we have the\n+\t\t * latest pack-files loaded into memory.\n \t\t */\n+\t\treprepare_packed_git(the_repository);\n \t\tdo {\n \t\t\tstruct packed_git *p;\n \n\nbase-commit: b4374e96c84ed9394fed363973eb540da308ed4f\n-- \ngitgitgadget\n"},{"id":"393136","messageId":"20200312163939.176298-1-jonathantanmy@google.com","threadId":"52984","inReplyTo":"pull.579.git.1584027403779.gitgitgadget@gmail.com","subject":"Re: [PATCH] connected.c: reprepare packs for corner cases","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-03-12T16:39:39Z","receivedAt":"2020-03-12T16:47:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Placing a reprepare_packed_git() call inside chck_connected() before\n> looping through the packed_git list seems like the safest way to\n> avoid this issue in the future. While reprepare_packed_git() does\n> another scan of the pack directory, it is not terribly expensive as\n> long as we do not run it in a loop. We check connectivity only a\n> few times per command, so this will not have a meaningful performance\n> impact. In exchange, we get extra safety around this check.\n\nThis also means that check_connected() now does the equivalent of\nreprepare_packed_git() in both its branches (the rev-list one, which\nspawns a new process and thus rereads the pack directory, and the fast\none). This will at least help callers to reason about its behavior more\nsimply, so it sounds like a good change to me.\n\n>     I included how I found this (integrating v2.26.0-rc0 into Scalar), but I\n>     am able to reproduce it on my Linux machine using real fetches from\n>     github.com. I'm not sure why I was unable to reproduce the issue in test\n>     cases using the file:// URLs or the HTTP tests.\n\nIf you remember how to reproduce it using real fetches from github.com,\ncould you give us reproduction steps?\n\nThanks for attempting to reproduce it in a test script.\n\n> diff --git a/connected.c b/connected.c\n> index 7e9bd1bc622..ac52b07b474 100644\n> --- a/connected.c\n> +++ b/connected.c\n> @@ -61,7 +61,11 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>  \t\t * object is a promisor object. Instead, just make sure we\n>  \t\t * received, in a promisor packfile, the objects pointed to by\n>  \t\t * each wanted ref.\n> +\t\t *\n> +\t\t * Before checking for promisor packs, be sure we have the\n> +\t\t * latest pack-files loaded into memory.\n>  \t\t */\n> +\t\treprepare_packed_git(the_repository);\n>  \t\tdo {\n>  \t\t\tstruct packed_git *p;\n\nThe code itself looks good to me.\n"},{"id":"393144","messageId":"fea393f7-de8a-73a3-6139-aa1f6a78089b@gmail.com","threadId":"52984","inReplyTo":"20200312163939.176298-1-jonathantanmy@google.com","subject":"Re: [PATCH] connected.c: reprepare packs for corner cases","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-03-12T17:34:34Z","receivedAt":"2020-03-12T17:34:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/12/2020 12:39 PM, Jonathan Tan wrote:\n>> Placing a reprepare_packed_git() call inside chck_connected() before\n>> looping through the packed_git list seems like the safest way to\n>> avoid this issue in the future. While reprepare_packed_git() does\n>> another scan of the pack directory, it is not terribly expensive as\n>> long as we do not run it in a loop. We check connectivity only a\n>> few times per command, so this will not have a meaningful performance\n>> impact. In exchange, we get extra safety around this check.\n> \n> This also means that check_connected() now does the equivalent of\n> reprepare_packed_git() in both its branches (the rev-list one, which\n> spawns a new process and thus rereads the pack directory, and the fast\n> one). This will at least help callers to reason about its behavior more\n> simply, so it sounds like a good change to me.\n> \n>>     I included how I found this (integrating v2.26.0-rc0 into Scalar), but I\n>>     am able to reproduce it on my Linux machine using real fetches from\n>>     github.com. I'm not sure why I was unable to reproduce the issue in test\n>>     cases using the file:// URLs or the HTTP tests.\n> \n> If you remember how to reproduce it using real fetches from github.com,\n> could you give us reproduction steps?\n\nSure. Run `git init`, then replace the `.git/config` file with this:\n\n[core]\n        repositoryformatversion = 1\n        filemode = false\n        bare = false\n        logallrefupdates = true\n        symlinks = false\n        ignorecase = true\n        repositoryFormat = 1\n[protocol]\n        version = 1\n[remote \"origin\"]\n        url = https://github.com/derrickstolee/TreeSearch\n        fetch = +refs/heads/*:refs/remotes/origin/*\n        promisor = true\n        partialclonefilter = blob:none\n[branch \"master\"]\n        remote = origin\n        merge = refs/heads/master\n\nthen run \"git fetch origin\". I purposefully put a very small repo that\nI have laying around, but this repros with git/git.\n\nThanks,\n-Stolee\n"},{"id":"393170","messageId":"xmqqzhcle23x.fsf@gitster.c.googlers.com","threadId":"52984","inReplyTo":"pull.579.git.1584027403779.gitgitgadget@gmail.com","subject":"Re: [PATCH] connected.c: reprepare packs for corner cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-12T20:42:58Z","receivedAt":"2020-03-12T20:43:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> While updating the microsoft/git fork on top of v2.26.0-rc0 and\n> consuming that build into Scalar, I noticed a corner case bug around\n> partial clone.\n>\n> The \"scalar clone\" command can create a Git repository with the\n> proper config for using partial clone with the \"blob:none\" filter.\n> Instead of calling \"git clone\", it runs \"git init\" then sets a few\n> more config values before running \"git fetch\".\n>\n> In our builds on v2.26.0-rc0, we noticed that our \"git fetch\"\n> command was failing with\n>\n>   error: https://github.com/microsoft/scalar did not send all necessary objects\n>\n> This does not happen if you copy the config file from a repository\n> created by \"git clone --filter=blob:none <url>\", but it does happen\n> when adding the config option \"core.logAllRefUpdates = true\".\n>\n> By debugging, I was able to see that the loop inside\n> check_connnected() that checks if all refs are contained in\n> promisor packs actually did not have any packfiles in the packed_git\n> list.\n> I'm not sure what corner-case issues caused this config option to\n> prevent the reprepare_packed_git() from being called at the proper\n> spot during the fetch operation. Even worse, I have failed to create\n> a test case to prevent a regression.\n>\n> Placing a reprepare_packed_git() call inside chck_connected() before\n> looping through the packed_git list seems like the safest way to\n> avoid this issue in the future.\n\nHmmm.  I am not sure if I am convinced that check_connected() is the\nbest place to do this.  Do we know the place that adds a new pack to\nthe repository, yet forgets to add it to the packed-git list, that\ncaused the failure you were observing?  Doing this change, without\ndescribing the answer to the question in the log message, makes it\nsmell rather like a random hack than a designed solution to me.\n\nIf lazy fetching of objects happen in multiple fetches before a\nsingle check_connected() sweeps them to check for connectivity, then\nperhaps the lazy fetching codepath needs to remember the fact that\nit added a new pack that is still not known to the packed-git list\n(or just add it immediately, without having to scan at all), and\ncheck_connected() would need to rescan only when there is at least\none such new pack?  That way, you do not have to penalize normal\ncallers of check_connected() that do not use lazy fetches at all,\nright?\n\nThanks.\n"},{"id":"393172","messageId":"20200312211638.GA872402@coredump.intra.peff.net","threadId":"52984","inReplyTo":"xmqqzhcle23x.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] connected.c: reprepare packs for corner cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-12T21:16:38Z","receivedAt":"2020-03-12T21:16:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 12, 2020 at 01:42:58PM -0700, Junio C Hamano wrote:\n\n> > Placing a reprepare_packed_git() call inside chck_connected() before\n> > looping through the packed_git list seems like the safest way to\n> > avoid this issue in the future.\n> \n> Hmmm.  I am not sure if I am convinced that check_connected() is the\n> best place to do this.  Do we know the place that adds a new pack to\n> the repository, yet forgets to add it to the packed-git list, that\n> caused the failure you were observing?  Doing this change, without\n> describing the answer to the question in the log message, makes it\n> smell rather like a random hack than a designed solution to me.\n\nThanks, I was just writing a very similar message. :)\n\n> If lazy fetching of objects happen in multiple fetches before a\n> single check_connected() sweeps them to check for connectivity, then\n> perhaps the lazy fetching codepath needs to remember the fact that\n> it added a new pack that is still not known to the packed-git list\n> (or just add it immediately, without having to scan at all), and\n> check_connected() would need to rescan only when there is at least\n> one such new pack?  That way, you do not have to penalize normal\n> callers of check_connected() that do not use lazy fetches at all,\n> right?\n\nI share your concern that it's not the best place for this. In practice,\nI do doubt that callers of check_connected() would notice, as it's a\npretty heavy-weight operation by itself. But I'd be concerned about\ngoing the other way: we know that calling it in check_connected() fixes\n_this_ problem, but we don't know if there's other code that would need\na similar fix.\n\nI was able to reproduce easily from Stolee's instructions, but I did\nnotice one interesting thing: the problem occurs only with the http\nprotocol, not with git:// or ssh. The obvious difference between them is\nthat http code is mostly running in the remote-helper program and in a\nspawned git-fetch-pack. But the check_connected() we hit is in the\nparent git-fetch process. So I _suspect_ that there's some low-level\ncode which calls reprepare() that happens in-process in most cases, but\nnot for the http case.\n\nIf I instrument Git like this:\n\ndiff --git a/common-main.c b/common-main.c\nindex 71e21dd20a..bf6ffe42b7 100644\n--- a/common-main.c\n+++ b/common-main.c\n@@ -49,6 +49,11 @@ int main(int argc, const char **argv)\n \ttrace2_cmd_start(argv);\n \ttrace2_collect_process_info(TRACE2_PROCESS_INFO_STARTUP);\n \n+\ttrace_printf(\"pid=%d, cmd=%s\",\n+\t\t     (int)getpid(),\n+\t\t     !ends_with(argv[0], \"git\") ? argv[0] :\n+\t\t     argv[1] ? argv[1] : argv[0]);\n+\n \tresult = cmd_main(argc, argv);\n \n \ttrace2_cmd_exit(result);\ndiff --git a/packfile.c b/packfile.c\nindex f4e752996d..832d020e6e 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1004,6 +1004,8 @@ void reprepare_packed_git(struct repository *r)\n {\n \tstruct object_directory *odb;\n \n+\ttrace_printf(\"repreparing in pid %d\", (int)getpid());\n+\n \tobj_read_lock();\n \tfor (odb = r->objects->odb; odb; odb = odb->next)\n \t\todb_clear_loose_cache(odb);\n\nthen running over the git protocol gives me:\n\n  17:13:52.642337 common-main.c:52        pid=882782, cmd=fetch\n  17:13:52.642871 git.c:439               trace: built-in: git fetch origin\n  17:13:52.725971 packfile.c:1007         repreparing in pid 882782\n  remote: Enumerating objects: 49, done.\n  17:13:52.793355 run-command.c:663       trace: run_command: git index-pack --stdin -v --fix-thin --promisor --pack_header=2,49\n  remote: Total 49 (delta 0), reused 0 (delta 0), pack-reused 49\n  17:13:52.796106 common-main.c:52        pid=882784, cmd=index-pack\n  17:13:52.796655 git.c:439               trace: built-in: git index-pack --stdin -v --fix-thin --promisor --pack_header=2,49\n  Receiving objects: 100% (49/49), 6.87 KiB | 6.87 MiB/s, done.\n  Resolving deltas: 100% (17/17), done.\n  17:13:52.811302 packfile.c:1007         repreparing in pid 882782\n  From git://github.com/derrickstolee/TreeSearch\n   * [new branch]      master     -> origin/master\n  17:13:52.812955 run-command.c:1616      run_processes_parallel: preparing to run up to 1 tasks\n  17:13:52.812986 run-command.c:1648      run_processes_parallel: done\n  17:13:52.813034 run-command.c:663       trace: run_command: git gc --auto\n  17:13:52.815435 common-main.c:52        pid=882788, cmd=gc\n  17:13:52.815941 git.c:439               trace: built-in: git gc --auto\n\nThere's an early reprepare, but the one after index-pack finishes\nresolving deltas is probably the interesting one. And it happens inside\nthe fetch process. But if we go back to http, I get:\n\n  17:15:28.234866 common-main.c:52        pid=882836, cmd=fetch\n  17:15:28.235403 git.c:439               trace: built-in: git fetch origin\n  17:15:28.236329 run-command.c:663       trace: run_command: GIT_DIR=.git git-remote-https origin https://github.com/derrickstolee/TreeSearch\n  17:15:28.244049 common-main.c:52        pid=882837, cmd=/home/peff/compile/git/git-remote-https\n  17:15:28.382586 packfile.c:1007         repreparing in pid 882836\n  17:15:28.382996 run-command.c:663       trace: run_command: git fetch-pack --stateless-rpc --stdin --lock-pack --include-tag --thin --from-promisor --filter=blob:none https://github.com/derrickstolee/TreeSearch/\n  17:15:28.386284 common-main.c:52        pid=882839, cmd=fetch-pack\n  17:15:28.386807 git.c:439               trace: built-in: git fetch-pack --stateless-rpc --stdin --lock-pack --include-tag --thin --from-promisor --filter=blob:none https://github.com/derrickstolee/TreeSearch/\n  remote: Enumerating objects: 49, done.\n  remote: Total 49 (delta 0), reused 0 (delta 0), pack-reused 49\n  17:15:28.444799 run-command.c:663       trace: run_command: git index-pack --stdin -v --fix-thin --promisor --pack_header=2,49\n  17:15:28.447441 common-main.c:52        pid=882841, cmd=index-pack\n  17:15:28.447944 git.c:439               trace: built-in: git index-pack --stdin -v --fix-thin --promisor --pack_header=2,49\n  Receiving objects: 100% (49/49), 6.87 KiB | 3.44 MiB/s, done.\n  Resolving deltas: 100% (17/17), done.\n  17:15:28.461871 packfile.c:1007         repreparing in pid 882839\n  error: https://github.com/derrickstolee/TreeSearch did not send all necessary objects\n  \n  17:15:28.463446 run-command.c:663       trace: run_command: git gc --auto\n  17:15:28.463949 common-main.c:52        pid=882845, cmd=gc\n  17:15:28.464056 git.c:439               trace: built-in: git gc --auto\n\nThere we see that same reprepare happen in 882839, which is the child\nfetch-pack. The parent fetch probably needs to reprepare itself after\nfetch-pack completes.\n\n-Peff\n"},{"id":"393174","messageId":"20200312212613.GB872402@coredump.intra.peff.net","threadId":"52984","inReplyTo":"20200312211638.GA872402@coredump.intra.peff.net","subject":"Re: [PATCH] connected.c: reprepare packs for corner cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-12T21:26:13Z","receivedAt":"2020-03-12T21:26:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 12, 2020 at 05:16:38PM -0400, Jeff King wrote:\n\n> There we see that same reprepare happen in 882839, which is the child\n> fetch-pack. The parent fetch probably needs to reprepare itself after\n> fetch-pack completes.\n\nActually, it's not fetch which is running fetch-pack, but rather the\nremote helper itself. So I think the simplest thing is for the\nremote-helper layer to do something like this:\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 20a7185ec4..25957e9a05 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -14,6 +14,7 @@\n #include \"refspec.h\"\n #include \"transport-internal.h\"\n #include \"protocol.h\"\n+#include \"packfile.h\"\n \n static int debug;\n \n@@ -672,6 +673,7 @@ static int fetch(struct transport *transport,\n {\n \tstruct helper_data *data = transport->data;\n \tint i, count;\n+\tint ret;\n \n \tget_helper(transport);\n \n@@ -710,13 +712,18 @@ static int fetch(struct transport *transport,\n \tif (data->transport_options.negotiation_tips)\n \t\twarning(\"Ignoring --negotiation-tip because the protocol does not support it.\");\n \n-\tif (data->fetch)\n-\t\treturn fetch_with_fetch(transport, nr_heads, to_fetch);\n+\tret = data->fetch ? fetch_with_fetch(transport, nr_heads, to_fetch) :\n+\t      data->import ? fetch_with_import(transport, nr_heads, to_fetch) :\n+\t      -1;\n \n-\tif (data->import)\n-\t\treturn fetch_with_import(transport, nr_heads, to_fetch);\n+\t/*\n+\t * We may have just received a pack through the helper sub-process;\n+\t * refresh the pack list.\n+\t */\n+\tif (!ret)\n+\t\treprepare_packed_git(the_repository);\n \n-\treturn -1;\n+\treturn ret;\n }\n \n static int push_update_ref_status(struct strbuf *buf,\n"},{"id":"393191","messageId":"7378a863-7e2a-455e-4635-e07938ef3381@gmail.com","threadId":"52984","inReplyTo":"20200312212613.GB872402@coredump.intra.peff.net","subject":"Re: [PATCH] connected.c: reprepare packs for corner cases","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-03-13T00:54:34Z","receivedAt":"2020-03-13T00:54:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/12/2020 5:26 PM, Jeff King wrote:\n> On Thu, Mar 12, 2020 at 05:16:38PM -0400, Jeff King wrote:\n> \n>> There we see that same reprepare happen in 882839, which is the child\n>> fetch-pack. The parent fetch probably needs to reprepare itself after\n>> fetch-pack completes.\n\nI agree with you and Junio that where I put the reprepare was non-\noptimal. The initial reason to put it there was that I found where\nthe error was happening, and thought that placing the reprepare there\nwas the best way to prevent this error from popping up in another case.\n\nThe result of a fetch failing and saying the remote did something wrong\nis quite alarming, and I wanted to avoid that from happening again in\nthe future from some other path.\n\nBut you're right, it's better to be as correct as possible.\n\n> Actually, it's not fetch which is running fetch-pack, but rather the\n> remote helper itself. So I think the simplest thing is for the\n> remote-helper layer to do something like this:\n\nI appreciate your root-causing this into the multi-process nature of\nfetch. I will update the commit message to include your details,\nespecially about how it does not reproduce over file or ssh protocol.\n \n> diff --git a/transport-helper.c b/transport-helper.c\n> index 20a7185ec4..25957e9a05 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -14,6 +14,7 @@\n>  #include \"refspec.h\"\n>  #include \"transport-internal.h\"\n>  #include \"protocol.h\"\n> +#include \"packfile.h\"\n>  \n>  static int debug;\n>  \n> @@ -672,6 +673,7 @@ static int fetch(struct transport *transport,\n>  {\n>  \tstruct helper_data *data = transport->data;\n>  \tint i, count;\n> +\tint ret;\n>  \n>  \tget_helper(transport);\n>  \n> @@ -710,13 +712,18 @@ static int fetch(struct transport *transport,\n>  \tif (data->transport_options.negotiation_tips)\n>  \t\twarning(\"Ignoring --negotiation-tip because the protocol does not support it.\");\n>  \n> -\tif (data->fetch)\n> -\t\treturn fetch_with_fetch(transport, nr_heads, to_fetch);\n> +\tret = data->fetch ? fetch_with_fetch(transport, nr_heads, to_fetch) :\n> +\t      data->import ? fetch_with_import(transport, nr_heads, to_fetch) :\n> +\t      -1;\n>  \n> -\tif (data->import)\n> -\t\treturn fetch_with_import(transport, nr_heads, to_fetch);\n> +\t/*\n> +\t * We may have just received a pack through the helper sub-process;\n> +\t * refresh the pack list.\n> +\t */\n> +\tif (!ret)\n> +\t\treprepare_packed_git(the_repository);\n>  \n> -\treturn -1;\n> +\treturn ret;\n>  }\n\nThis code looks correct, and should be the fix for the short-term.\n\nI wonder if we could do something more complicated in the long-term,\nwhich was recommended to me by Jeff Hostetler: add the pack to the\npacked_git list once we've indexed it. That way, we don't reprepare\nand scan the packs one-by-one, but instead we insert to the list\na single pack that we already know about.\n\nThanks,\n-Stolee\n"},{"id":"393192","messageId":"xmqq36addpiv.fsf@gitster.c.googlers.com","threadId":"52984","inReplyTo":"7378a863-7e2a-455e-4635-e07938ef3381@gmail.com","subject":"Re: [PATCH] connected.c: reprepare packs for corner cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-13T01:14:48Z","receivedAt":"2020-03-13T01:14:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> I wonder if we could do something more complicated in the long-term,\n> which was recommended to me by Jeff Hostetler: add the pack to the\n> packed_git list once we've indexed it. That way, we don't reprepare\n> and scan the packs one-by-one, but instead we insert to the list\n> a single pack that we already know about.\n\nYup, that was the first thing that came to my mind, i.e. the code\nthat installs a new packfile on disk in our repository must be ours,\nso we should be able to tell that code to add the new packfile to\nthe in-core list without having to scan.\n\nWe may still need to scan when that strategy \"fails\" to protect\nagainst racing with simultaneous repacking, though.\n"},{"id":"393193","messageId":"20200313023034.GA900007@coredump.intra.peff.net","threadId":"52984","inReplyTo":"7378a863-7e2a-455e-4635-e07938ef3381@gmail.com","subject":"Re: [PATCH] connected.c: reprepare packs for corner cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-13T02:30:34Z","receivedAt":"2020-03-13T02:30:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 12, 2020 at 08:54:34PM -0400, Derrick Stolee wrote:\n\n> On 3/12/2020 5:26 PM, Jeff King wrote:\n> > On Thu, Mar 12, 2020 at 05:16:38PM -0400, Jeff King wrote:\n> > \n> >> There we see that same reprepare happen in 882839, which is the child\n> >> fetch-pack. The parent fetch probably needs to reprepare itself after\n> >> fetch-pack completes.\n> \n> I agree with you and Junio that where I put the reprepare was non-\n> optimal. The initial reason to put it there was that I found where\n> the error was happening, and thought that placing the reprepare there\n> was the best way to prevent this error from popping up in another case.\n\nTo be fair to you, the place you put it is almost certainly fine in\npractice. The connectivity check is likely the first time we'll actually\nlook at the objects in the parent process, and once we've re-scanned,\nall the code after us is protected.\n\nBut I do still think it's cleaner to put it closer to the responsible\ncode.\n\nAlthough there was one thing that puzzled me when writing the previous\nemail. Why isn't this a problem for non-partial clones? And the answer\nis that in a normal repo, as soon as we try to look up a missing object,\nwe'd trigger the usual re-scan. But we _don't_ do object lookups in\ncheck_connected(). We do this funky loop over get_all_packs() ourselves,\nlooking only at promisor packs. And if we didn't find the object, then\nwe immediately complain in that loop with no fallback to re-scan.\n\nSo that would actually argue that your patch is putting it in the right\nplace. It's _not_ fetch's responsibility to reprepare_packed_git(). It's\nthe loop in check_connected() that is skipping the usual reprepare\nlogic, and shouldn't.\n\nAnd one fix (which you did) is to just preemptively reprepare right\nabove that loop. Some other solutions are:\n\n  - teach the loop to reprepare() when we don't find an object and see\n    if we picked up a new promisor pack\n\n  - reverse the loop logic: look up each object in the usual way (in all\n    packs), and see if the resulting pack is a promisor. I guess that\n    produces slightly different results though if an object is in the\n    new promisor pack _and_ available elsewhere; but isn't the point of\n    that check_refs_are_promisor_objects_only flag that we're doing a\n    clone?\n\n> I wonder if we could do something more complicated in the long-term,\n> which was recommended to me by Jeff Hostetler: add the pack to the\n> packed_git list once we've indexed it. That way, we don't reprepare\n> and scan the packs one-by-one, but instead we insert to the list\n> a single pack that we already know about.\n\nI don't think the parent git-fetch process even knows about the pack,\nthough. It just asked the remote-helper to somehow make the objects\narrive, and it doesn't know what form that took.\n\nreprepare_packed_git() should be pretty cheap, though. If we already\nhave a pack in the list, we won't re-open it. Finding out if we already\nhad one used to be O(n), making the whole re-scan quadratic. But that\nwas fixed recently in ec48540fe8 (packfile.c: speed up loading lots of\npackfiles, 2019-11-27), where we switched to keeping a hashmap of\nloaded pack names.\n\n-Peff\n"},{"id":"393194","messageId":"20200313023427.GB900007@coredump.intra.peff.net","threadId":"52984","inReplyTo":"20200313023034.GA900007@coredump.intra.peff.net","subject":"Re: [PATCH] connected.c: reprepare packs for corner cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-13T02:34:27Z","receivedAt":"2020-03-13T02:34:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 12, 2020 at 10:30:34PM -0400, Jeff King wrote:\n\n> So that would actually argue that your patch is putting it in the right\n> place. It's _not_ fetch's responsibility to reprepare_packed_git(). It's\n> the loop in check_connected() that is skipping the usual reprepare\n> logic, and shouldn't.\n> \n> And one fix (which you did) is to just preemptively reprepare right\n> above that loop. Some other solutions are:\n\nI know I've now suggested that your patch is both wrong and right. :)\n\nJust to be clear, at this point I think I'd be OK with either solution.\n\nIf it's going into check_connected(), the commit message should argue\nthat the loop there is at fault for not doing the usual fallback, and a\nsingle explicit reprepare() is cheap enough to cover the case we care\nabout (and that we don't have to worry about racing with somebody else\nrepacking because the point of that flag is that we're in a brand new\nrepo).\n\nRepreparing earlier in the transport-helper code _could_ still protect\nagainst other similar loops, which is an argument for putting it there.\nBut I'd be inclined to say those other loops should be corrected.\n\n-Peff\n"},{"id":"393205","messageId":"8eb7c0f3-0667-2eb7-7101-5790bcff784c@gmail.com","threadId":"52984","inReplyTo":"20200313023427.GB900007@coredump.intra.peff.net","subject":"Re: [PATCH] connected.c: reprepare packs for corner cases","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-03-13T12:43:49Z","receivedAt":"2020-03-13T12:43:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/12/2020 10:34 PM, Jeff King wrote:\n> On Thu, Mar 12, 2020 at 10:30:34PM -0400, Jeff King wrote:\n> \n>> So that would actually argue that your patch is putting it in the right\n>> place. It's _not_ fetch's responsibility to reprepare_packed_git(). It's\n>> the loop in check_connected() that is skipping the usual reprepare\n>> logic, and shouldn't.\n>>\n>> And one fix (which you did) is to just preemptively reprepare right\n>> above that loop. Some other solutions are:\n> \n> I know I've now suggested that your patch is both wrong and right. :)\n> \n> Just to be clear, at this point I think I'd be OK with either solution.\n> \n> If it's going into check_connected(), the commit message should argue\n> that the loop there is at fault for not doing the usual fallback, and a\n> single explicit reprepare() is cheap enough to cover the case we care\n> about (and that we don't have to worry about racing with somebody else\n> repacking because the point of that flag is that we're in a brand new\n> repo).\n\nThank you for bringing extra clarity here. We don't need to put the\nreprepare in the fetch logic because _most_ kinds of object lookups will\nreprepare when failing to find an expected object. This loop is special\nin that it is restricted to search the promisor packs. And, because it\nonly happens when a special mode (opt->check_refs_are_promisor_objects_only)\nit is reasonable to assume that we are specifically in the case that a\npack was added recently.\n\nWe could save some work in most cases by repreparing only after failing\nto find the object, but that also makes the code more complicated for\nlow value. At least, that is my opinion.\n \n> Repreparing earlier in the transport-helper code _could_ still protect\n> against other similar loops, which is an argument for putting it there.\n> But I'd be inclined to say those other loops should be corrected.\n\nRight. We should make it the responsibility of the code that is scanning\npack-files that they should be able to reprepare when failing to find\n\"expected\" objects. This also handles the concurrent repack case that\nJunio mentioned.\n\nTaking a quick glance at the callers of get_all_packs(), I see this is\nthe only such loop that both has an ability to \"fail\" based on an\nexpected object and doesn't have a fallback to reprepare.\n\nThanks,\n-Stolee\n"},{"id":"393236","messageId":"pull.579.v2.git.1584133915654.gitgitgadget@gmail.com","threadId":"52984","inReplyTo":"pull.579.git.1584027403779.gitgitgadget@gmail.com","subject":"[PATCH v2] connected.c: reprepare packs for corner cases","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-13T21:11:55Z","receivedAt":"2020-03-13T21:12:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nWhile updating the microsoft/git fork on top of v2.26.0-rc0 and\nconsuming that build into Scalar, I noticed a corner case bug around\npartial clone.\n\nThe \"scalar clone\" command can create a Git repository with the\nproper config for using partial clone with the \"blob:none\" filter.\nInstead of calling \"git clone\", it runs \"git init\" then sets a few\nmore config values before running \"git fetch\".\n\nIn our builds on v2.26.0-rc0, we noticed that our \"git fetch\"\ncommand was failing with\n\n  error: https://github.com/microsoft/scalar did not send all necessary objects\n\nThis does not happen if you copy the config file from a repository\ncreated by \"git clone --filter=blob:none <url>\", but it does happen\nwhen adding the config option \"core.logAllRefUpdates = true\".\n\nBy debugging, I was able to see that the loop inside\ncheck_connnected() that checks if all refs are contained in\npromisor packs actually did not have any packfiles in the packed_git\nlist.\n\nI'm not sure what corner-case issues caused this config option to\nprevent the reprepare_packed_git() from being called at the proper\nspot during the fetch operation. This approach requires a situation\nwhere we use the remote helper process, which makes it difficult to\ntest.\n\nIt is possible to place a reprepare_packed_git() call in the fetch code\ncloser to where we receive a pack, but that leaves an opening for a\nlater change to re-introduce this problem. Further, a concurrent repack\noperation could replace the pack-file list we already loaded into\nmemory, causing this issue in an even harder to reproduce scenario.\n\nIt is really the responsibility of anyone looping through the list of\npack-files for a certain object to fall back to reprepare_packed_git()\non a fail-to-find. The loop in check_connected() does not have this\nfallback, leading to this bug.\n\nWe _could_ try looping through the packs and only reprepare the packs\nafter a miss, but that change is more involved and has little value.\nSince this case is isolated to the case when\nopt->check_refs_are_promisor_objects_only is true, we are confident that\nwe are verifying the refs after downloading new data. This implies that\ncalling reprepare_packed_git() in advance is not a huge cost compared to\nthe rest of the operations already made.\n\nHelped-by: Jeff King <peff@peff.net>\nHelped-by: Junio Hamano <gitster@pobox.com>\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n    connected.c: reprepare packs for corner cases\n    \n    I included how I found this (integrating v2.26.0-rc0 into Scalar), but I\n    am able to reproduce it on my Linux machine using real fetches from\n    github.com. I'm not sure why I was unable to reproduce the issue in test\n    cases using the file:// URLs or the HTTP tests.\n    \n    Update in V2: I've updated the commit message to discuss the options\n    presented on-list, but also provide why I'm keeping the code unchanged\n    in light of that discussion.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-579%2Fderrickstolee%2Ffetch-reprepare-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-579/derrickstolee/fetch-reprepare-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/579\n\nRange-diff vs v1:\n\n 1:  cde7aa20ca8 ! 1:  696a51bd5a0 connected.c: reprepare packs for corner cases\n     @@ -27,17 +27,31 @@\n      \n          I'm not sure what corner-case issues caused this config option to\n          prevent the reprepare_packed_git() from being called at the proper\n     -    spot during the fetch operation. Even worse, I have failed to create\n     -    a test case to prevent a regression.\n     +    spot during the fetch operation. This approach requires a situation\n     +    where we use the remote helper process, which makes it difficult to\n     +    test.\n      \n     -    Placing a reprepare_packed_git() call inside chck_connected() before\n     -    looping through the packed_git list seems like the safest way to\n     -    avoid this issue in the future. While reprepare_packed_git() does\n     -    another scan of the pack directory, it is not terribly expensive as\n     -    long as we do not run it in a loop. We check connectivity only a\n     -    few times per command, so this will not have a meaningful performance\n     -    impact. In exchange, we get extra safety around this check.\n     +    It is possible to place a reprepare_packed_git() call in the fetch code\n     +    closer to where we receive a pack, but that leaves an opening for a\n     +    later change to re-introduce this problem. Further, a concurrent repack\n     +    operation could replace the pack-file list we already loaded into\n     +    memory, causing this issue in an even harder to reproduce scenario.\n      \n     +    It is really the responsibility of anyone looping through the list of\n     +    pack-files for a certain object to fall back to reprepare_packed_git()\n     +    on a fail-to-find. The loop in check_connected() does not have this\n     +    fallback, leading to this bug.\n     +\n     +    We _could_ try looping through the packs and only reprepare the packs\n     +    after a miss, but that change is more involved and has little value.\n     +    Since this case is isolated to the case when\n     +    opt->check_refs_are_promisor_objects_only is true, we are confident that\n     +    we are verifying the refs after downloading new data. This implies that\n     +    calling reprepare_packed_git() in advance is not a huge cost compared to\n     +    the rest of the operations already made.\n     +\n     +    Helped-by: Jeff King <peff@peff.net>\n     +    Helped-by: Junio Hamano <gitster@pobox.com>\n          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n      \n       diff --git a/connected.c b/connected.c\n\n\n connected.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/connected.c b/connected.c\nindex 7e9bd1bc622..ac52b07b474 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -61,7 +61,11 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t * object is a promisor object. Instead, just make sure we\n \t\t * received, in a promisor packfile, the objects pointed to by\n \t\t * each wanted ref.\n+\t\t *\n+\t\t * Before checking for promisor packs, be sure we have the\n+\t\t * latest pack-files loaded into memory.\n \t\t */\n+\t\treprepare_packed_git(the_repository);\n \t\tdo {\n \t\t\tstruct packed_git *p;\n \n\nbase-commit: b4374e96c84ed9394fed363973eb540da308ed4f\n-- \ngitgitgadget\n"}]}