summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDerrick Stolee <dstolee@microsoft.com>2020-03-13 21:11:55 (GMT)
committerJunio C Hamano <gitster@pobox.com>2020-03-15 22:39:00 (GMT)
commitb739d971e5f9997d90bc09b6c8cfdc677e3a0da6 (patch)
tree4798071a3eddc33caec82a3b6babbc1e8449322a
parent30e9940356dc67959877f4b2417da33ebdefbb79 (diff)
downloadgit-b739d971e5f9997d90bc09b6c8cfdc677e3a0da6.zip
git-b739d971e5f9997d90bc09b6c8cfdc677e3a0da6.tar.gz
git-b739d971e5f9997d90bc09b6c8cfdc677e3a0da6.tar.bz2
connected.c: reprepare packs for corner cases
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 <peff@peff.net> Helped-by: Junio Hamano <gitster@pobox.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
-rw-r--r--connected.c4
1 files changed, 4 insertions, 0 deletions
diff --git a/connected.c b/connected.c
index 7e9bd1b..ac52b07 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;