[v2] connected.c: reprepare packs for corner cases

Commit Message

Linus Arver via GitGitGadget March 13, 2020, 9:11 p.m. UTC

From: Derrick Stolee <[e-mail]>

While updating the microsoft/git fork on top of v2.26.0-rc0 and
consuming that build into Scalar, I noticed a corner case bug around
partial clone.

The "scalar clone" command can create a Git repository with the
proper config for using partial clone with the "blob:none" filter.
Instead of calling "git clone", it runs "git init" then sets a few
more config values before running "git fetch".

In our builds on v2.26.0-rc0, we noticed that our "git fetch"
command was failing with

  error: https://github.com/microsoft/scalar did not send all necessary objects

This does not happen if you copy the config file from a repository
created by "git clone --filter=blob:none <url>", but it does happen
when adding the config option "core.logAllRefUpdates = true".

By debugging, I was able to see that the loop inside
check_connnected() that checks if all refs are contained in
promisor packs actually did not have any packfiles in the packed_git
list.

I'm not sure what corner-case issues caused this config option to
prevent the reprepare_packed_git() from being called at the proper
spot during the fetch operation. This approach requires a situation
where we use the remote helper process, which makes it difficult to
test.

It is possible to place a reprepare_packed_git() call in the fetch code
closer to where we receive a pack, but that leaves an opening for a
later change to re-introduce this problem. Further, a concurrent repack
operation could replace the pack-file list we already loaded into
memory, causing this issue in an even harder to reproduce scenario.

It is really the responsibility of anyone looping through the list of
pack-files for a certain object to fall back to reprepare_packed_git()
on a fail-to-find. The loop in check_connected() does not have this
fallback, leading to this bug.

We _could_ try looping through the packs and only reprepare the packs
after a miss, but that change is more involved and has little value.
Since this case is isolated to the case when
opt->check_refs_are_promisor_objects_only is true, we are confident that
we are verifying the refs after downloading new data. This implies that
calling reprepare_packed_git() in advance is not a huge cost compared to
the rest of the operations already made.

Helped-by: Jeff King <[e-mail]>
Helped-by: Junio Hamano <[e-mail]>
Signed-off-by: Derrick Stolee <[e-mail]>
---
    connected.c: reprepare packs for corner cases
    
    I included how I found this (integrating v2.26.0-rc0 into Scalar), but I
    am able to reproduce it on my Linux machine using real fetches from
    github.com. I'm not sure why I was unable to reproduce the issue in test
    cases using the file:// URLs or the HTTP tests.
    
    Update in V2: I've updated the commit message to discuss the options
    presented on-list, but also provide why I'm keeping the code unchanged
    in light of that discussion.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-579%2Fderrickstolee%2Ffetch-reprepare-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-579/derrickstolee/fetch-reprepare-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/579

Range-diff vs v1:

 1:  cde7aa20ca8 ! 1:  696a51bd5a0 connected.c: reprepare packs for corner cases
     @@ -27,17 +27,31 @@
      
          I'm not sure what corner-case issues caused this config option to
          prevent the reprepare_packed_git() from being called at the proper
     -    spot during the fetch operation. Even worse, I have failed to create
     -    a test case to prevent a regression.
     +    spot during the fetch operation. This approach requires a situation
     +    where we use the remote helper process, which makes it difficult to
     +    test.
      
     -    Placing a reprepare_packed_git() call inside chck_connected() before
     -    looping through the packed_git list seems like the safest way to
     -    avoid this issue in the future. While reprepare_packed_git() does
     -    another scan of the pack directory, it is not terribly expensive as
     -    long as we do not run it in a loop. We check connectivity only a
     -    few times per command, so this will not have a meaningful performance
     -    impact. In exchange, we get extra safety around this check.
     +    It is possible to place a reprepare_packed_git() call in the fetch code
     +    closer to where we receive a pack, but that leaves an opening for a
     +    later change to re-introduce this problem. Further, a concurrent repack
     +    operation could replace the pack-file list we already loaded into
     +    memory, causing this issue in an even harder to reproduce scenario.
      
     +    It is really the responsibility of anyone looping through the list of
     +    pack-files for a certain object to fall back to reprepare_packed_git()
     +    on a fail-to-find. The loop in check_connected() does not have this
     +    fallback, leading to this bug.
     +
     +    We _could_ try looping through the packs and only reprepare the packs
     +    after a miss, but that change is more involved and has little value.
     +    Since this case is isolated to the case when
     +    opt->check_refs_are_promisor_objects_only is true, we are confident that
     +    we are verifying the refs after downloading new data. This implies that
     +    calling reprepare_packed_git() in advance is not a huge cost compared to
     +    the rest of the operations already made.
     +
     +    Helped-by: Jeff King <[e-mail]>
     +    Helped-by: Junio Hamano <[e-mail]>
          Signed-off-by: Derrick Stolee <[e-mail]>
      
       diff --git a/connected.c b/connected.c


 connected.c | 4 ++++
 1 file changed, 4 insertions(+)


base-commit: b4374e96c84ed9394fed363973eb540da308ed4f

Patch

diff --git a/connected.c b/connected.c
index 7e9bd1bc622..ac52b07b474 100644
--- a/connected.c
+++ b/connected.c
@@ -61,7 +61,11 @@  int check_connected(oid_iterate_fn fn, void *cb_data,
 		 * object is a promisor object. Instead, just make sure we
 		 * received, in a promisor packfile, the objects pointed to by
 		 * each wanted ref.
+		 *
+		 * Before checking for promisor packs, be sure we have the
+		 * latest pack-files loaded into memory.
 		 */
+		reprepare_packed_git(the_repository);
 		do {
 			struct packed_git *p;