diff --git a/gvfs-helper.c b/gvfs-helper.c index f3c86ab830ae61..d6e2606721eb2c 100644 --- a/gvfs-helper.c +++ b/gvfs-helper.c @@ -1885,12 +1885,36 @@ static void my_finalize_packfile(struct gh__request_params *params, struct strbuf *final_path_idx, struct strbuf *final_filename) { + /* + * Install the .pack and .idx into the ODB pack directory. + * + * We might be racing with other instances of gvfs-helper if + * we, in parallel, both downloaded the exact same packfile + * (with the same checksum SHA) and try to install it at the + * same time. This might happen on Windows where the loser + * can get an EBUSY or EPERM trying to move/rename the + * tempfile into the pack dir, for example. + * + * So, we always install the .pack before the .idx for + * consistency. And only if *WE* created the .pack and .idx + * files, do we create the matching .keep (when requested). + * + * If we get an error and the target files already exist, we + * silently eat the error. Note that finalize_object_file() + * has already munged errno (and it has various creation + * strategies), so we don't bother looking at it. + */ if (finalize_object_file(temp_path_pack->buf, final_path_pack->buf) || finalize_object_file(temp_path_idx->buf, final_path_idx->buf)) { unlink(temp_path_pack->buf); unlink(temp_path_idx->buf); - unlink(final_path_pack->buf); - unlink(final_path_idx->buf); + + if (file_exists(final_path_pack->buf) && + file_exists(final_path_idx->buf)) { + trace2_printf("%s: assuming ok for %s", TR2_CAT, final_path_pack->buf); + goto assume_ok; + } + strbuf_addf(&status->error_message, "could not install packfile '%s'", final_path_pack->buf); @@ -1913,6 +1937,7 @@ static void my_finalize_packfile(struct gh__request_params *params, strbuf_release(&keep); } +assume_ok: if (params->result_list) { struct strbuf result_msg = STRBUF_INIT; diff --git a/t/t5799-gvfs-helper.sh b/t/t5799-gvfs-helper.sh index 29b5aa47b2f6aa..5c79654a63f0fa 100755 --- a/t/t5799-gvfs-helper.sh +++ b/t/t5799-gvfs-helper.sh @@ -372,6 +372,10 @@ verify_objects_in_shared_cache () { return 0 } +# gvfs-helper prints a "packfile " message for each received +# packfile to stdout. Verify that we received the expected number +# of packfiles. +# verify_received_packfile_count () { if test $# -eq 1 then @@ -414,6 +418,19 @@ verify_prefetch_keeps () { return 0 } +# Verify that the number of vfs- packfile present in the shared-cache +# matches our expectations. +# +verify_vfs_packfile_count () { + count=$(( $(ls -1 "$SHARED_CACHE_T1"/pack/vfs-*.pack | wc -l) )) + if test $count -ne $1 + then + echo "verify_vfs_packfile_count: expected $1; actual $count" + return 1 + fi + return 0 +} + per_test_cleanup () { stop_gvfs_protocol_server @@ -1176,6 +1193,103 @@ test_expect_success 'integration: fully implicit: diff 2 commits' ' >OUT.output 2>OUT.stderr ' +################################################################# +# Duplicate packfile tests. +# +# If we request a fixed set of blobs, we should get a unique packfile +# of the form "vfs-.{pack,idx}". It we request that same set +# again, the server should create and send the exact same packfile. +# True web servers might build the custom packfile in random order, +# but our test web server should give us consistent results. +# +# Verify that we can handle the duplicate pack and idx file properly. +################################################################# + +test_expect_success 'duplicate: vfs- packfile' ' + test_when_finished "per_test_cleanup" && + start_gvfs_protocol_server && + + git -C "$REPO_T1" gvfs-helper \ + --cache-server=disable \ + --remote=origin \ + --no-progress \ + post \ + <"$OIDS_BLOBS_FILE" >OUT.output 2>OUT.stderr && + verify_received_packfile_count 1 && + verify_vfs_packfile_count 1 && + + # Re-fetch the same packfile. We do not care if it replaces + # first one or if it silently fails to overwrite the existing + # one. We just confirm that afterwards we only have 1 packfile. + # + git -C "$REPO_T1" gvfs-helper \ + --cache-server=disable \ + --remote=origin \ + --no-progress \ + post \ + <"$OIDS_BLOBS_FILE" >OUT.output 2>OUT.stderr && + verify_received_packfile_count 1 && + verify_vfs_packfile_count 1 && + + stop_gvfs_protocol_server +' + +# Return the absolute pathname of the first received packfile. +# +first_received_packfile_pathname () { + fn=$(sed -n '/^packfile/p' OUT.output \ + 2>OUT.stderr && + verify_received_packfile_count 1 && + verify_vfs_packfile_count 1 && + + # Re-fetch the same packfile, but hold the existing packfile + # open for writing on an obscure (and randomly-chosen) file + # descriptor. + # + # This should cause the replacement-install to fail (at least + # on Windows) with an EBUSY or EPERM or something. + # + # Verify that that error is eaten. We do not care if the + # replacement is retried or if gvfs-helper simply discards the + # second instance. We just confirm that afterwards we only + # have 1 packfile on disk and that the command "lies" and reports + # that it created the existing packfile. (We want the lie because + # in normal usage, gh-client has already built the packed-git list + # in memory and is using gvfs-helper to fetch missing objects; + # gh-client does not care who does the fetch, but it needs to + # update its packed-git list and restart the object lookup.) + # + PACK=$(first_received_packfile_pathname) && + git -C "$REPO_T1" gvfs-helper \ + --cache-server=disable \ + --remote=origin \ + --no-progress \ + post \ + <"$OIDS_BLOBS_FILE" \ + >OUT.output \ + 2>OUT.stderr \ + 9>>"$PACK" && + verify_received_packfile_count 1 && + verify_vfs_packfile_count 1 && + + stop_gvfs_protocol_server +' + ################################################################# # Ensure that the SHA of the blob we received matches the SHA of # the blob we requested.