Skip to content

Commit f6415e4

Browse files
authored
Fix regression from batch object downloads (#824)
This is a fix for a performance regression introduced by #822. That change updated the packfile installation process to call a different packfile installation method after some upstream changes to the packfile data structures. However, while I thought I read the new packfile methods as including a modification of the MRU cache, it apparently does not fully install the packfile in the list. This manifests in something like `git checkout` where the missing blobs are queued for download, downloaded in blob packfiles, and then the checkout process continues. During the process of writing the data to the worktree, the "updating files" progress indicator slows to a crawl because it was "missing" the blobs and then downloaded them on-demand. This fixes the issue by being less fancy about packfiles and using `packfile_store_reprepare()` to just reset the full packfile list. This is more future-proof and isn't very expensive compared to the packfile download. I augmented a test to include tracing that shows a necessary blob is queued for packfile download and is not later downloaded via an immediate request. Without the code change, that test would fail. * [X] This change only applies to interactions with Azure DevOps and the GVFS Protocol.
2 parents 72f6d6d + 65222a5 commit f6415e4

File tree

2 files changed

+19
-31
lines changed

2 files changed

+19
-31
lines changed

gvfs-helper-client.c

Lines changed: 3 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -191,35 +191,6 @@ static void gh_client__update_loose_cache(const char *line)
191191
odb_loose_cache_add_new_oid(gh_client__chosen_odb, &oid);
192192
}
193193

194-
/*
195-
* Update the packed-git list to include the newly created packfile.
196-
*/
197-
static void gh_client__update_packed_git(const char *line)
198-
{
199-
struct strbuf path = STRBUF_INIT;
200-
const char *v1_filename;
201-
struct packed_git *p;
202-
int is_local;
203-
204-
if (!skip_prefix(line, "packfile ", &v1_filename))
205-
BUG("update_packed_git: invalid line '%s'", line);
206-
207-
/*
208-
* ODB[0] is the local .git/objects. All others are alternates.
209-
*/
210-
is_local = (gh_client__chosen_odb == the_repository->objects->sources);
211-
212-
strbuf_addf(&path, "%s/pack/%s",
213-
gh_client__chosen_odb->path, v1_filename);
214-
strbuf_strip_suffix(&path, ".pack");
215-
strbuf_addstr(&path, ".idx");
216-
217-
p = add_packed_git(the_repository, path.buf, path.len, is_local);
218-
if (p)
219-
packfile_store_add_pack(the_repository->objects->packfiles, p);
220-
strbuf_release(&path);
221-
}
222-
223194
/*
224195
* CAP_OBJECTS verbs return the same format response:
225196
*
@@ -279,7 +250,6 @@ static int gh_client__objects__receive_response(
279250
}
280251

281252
else if (starts_with(line, "packfile")) {
282-
gh_client__update_packed_git(line);
283253
ghc |= GHC__CREATED__PACKFILE;
284254
nr_packfile++;
285255
}
@@ -300,6 +270,9 @@ static int gh_client__objects__receive_response(
300270
}
301271
}
302272

273+
if (ghc & GHC__CREATED__PACKFILE)
274+
packfile_store_reprepare(the_repository->objects->packfiles);
275+
303276
*p_ghc = ghc;
304277
*p_nr_loose = nr_loose;
305278
*p_nr_packfile = nr_packfile;

t/t5799-gvfs-helper.sh

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1285,15 +1285,30 @@ test_expect_success 'integration: explicit commit/trees, implicit blobs: diff 2
12851285
>OUT.output 2>OUT.stderr
12861286
'
12871287

1288+
trace_has_queue_oid () {
1289+
oid=$1
1290+
grep "gh_client__queue_oid: $oid"
1291+
}
1292+
1293+
trace_has_immediate_oid () {
1294+
oid=$1
1295+
grep "gh_client__get_immediate: $oid"
1296+
}
1297+
12881298
test_expect_success 'integration: fully implicit: diff 2 commits' '
12891299
test_when_finished "per_test_cleanup" &&
12901300
start_gvfs_protocol_server &&
12911301
12921302
# Implicitly demand-load everything without any pre-seeding.
12931303
#
1304+
GIT_TRACE2_EVENT="$(pwd)/diff-trace.txt" \
12941305
git -C "$REPO_T1" -c core.useGVFSHelper=true \
12951306
diff $(cat m1.branch)..$(cat m3.branch) \
1296-
>OUT.output 2>OUT.stderr
1307+
>OUT.output 2>OUT.stderr &&
1308+
1309+
oid=$(git -C "$REPO_SRC" rev-parse main:file9.txt.t) &&
1310+
trace_has_queue_oid $oid <diff-trace.txt &&
1311+
! trace_has_immediate_oid $oid <diff-trace.txt
12971312
'
12981313

12991314
# T1 should be considered contaminated at this point.

0 commit comments

Comments
 (0)