From 247b0eaa49364ddaea09e1babec1099f4c1b3c69 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Thu, 8 Jan 2026 15:57:56 -0500 Subject: [PATCH 1/3] gvfs-helper: skip collision check for loose objects When we are installing a loose object, finalize_object_file() first checks to see if the contents match what already exists in a loose object file of the target name. However, this doesn't check if the target is valid, it assumes the target is valid. However, in the case of a power outage or something like that, the file may be corrupt (for example: all NUL bytes). That is a common occurrence when we are needing to install a loose object _again_: we don't think we have it already so any copy that exists is probably bogus. Use the flagged version with FOF_SKIP_COLLISION_CHECK to avoid these types of errors, as seen in GitHub issue https://github.com/microsoft/git/issues/837. Signed-off-by: Derrick Stolee --- gvfs-helper.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/gvfs-helper.c b/gvfs-helper.c index d030c76e32cacb..9f96ceb97929e4 100644 --- a/gvfs-helper.c +++ b/gvfs-helper.c @@ -2459,7 +2459,16 @@ static void install_loose(struct gh__request_params *params, goto cleanup; } - if (finalize_object_file(the_repository, tmp_path.buf, loose_path.buf)) { + /* + * We skip collision check because the loose object in the target + * may be corrupt and we should override it with a better value + * instead of failing at this point. + * + * See https://github.com/microsoft/git/issues/837 + */ + if (finalize_object_file_flags(the_repository, + tmp_path.buf, loose_path.buf, + FOF_SKIP_COLLISION_CHECK)) { unlink(tmp_path.buf); strbuf_addf(&status->error_message, "could not install loose object '%s'", From 4b902640bff09b0bad60a858f271a828ee9c5acd Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Thu, 8 Jan 2026 16:28:22 -0500 Subject: [PATCH 2/3] gvfs-helper: emit advice on transient errors Users sometimes see transient network errors, but they are actually due to some other problem within the installation of a packfile. Observed resolutions include freeing up space on a full disk or deleting the shared object cache because something was broken due to a file corruption or power outage. This change only provides the advice to suggest those workarounds to help users help themselves. This is our first advice custom to the microsoft/git fork, so I have partitioned the key away from the others to avoid adjacent change conflicts (at least until upstream adds a new change at the end of the alphabetical list). We could consider providing a tool that does a more robust check of the shared object cache, but since 'git fsck' isn't safe to run as it may download missing objects, we do not have that ability at the moment. The good news is that it is safe to delete and rebuild the shared object cache as long as all local branches are pushed. The branches must be pushed because the local .git/objects/ directory is moved to the shared object cache in the 'cache-local-objects' maintenance task. Signed-off-by: Derrick Stolee --- advice.c | 3 +++ advice.h | 3 +++ gvfs-helper.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+) diff --git a/advice.c b/advice.c index 71ddedd4ad46bb..0d9f1fc1ee210b 100644 --- a/advice.c +++ b/advice.c @@ -93,6 +93,9 @@ static struct { [ADVICE_USE_CORE_FSMONITOR_CONFIG] = { "useCoreFSMonitorConfig" }, [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor" }, [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan" }, + + /* microsoft/git custom advice below: */ + [ADVICE_GVFS_HELPER_TRANSIENT_RETRY] = { "gvfs.transientRetry"}, }; static const char turn_off_instructions[] = diff --git a/advice.h b/advice.h index 849a5991379c11..00f7f7627e1c62 100644 --- a/advice.h +++ b/advice.h @@ -60,6 +60,9 @@ enum advice_type { ADVICE_USE_CORE_FSMONITOR_CONFIG, ADVICE_WAITING_FOR_EDITOR, ADVICE_WORKTREE_ADD_ORPHAN, + + /* microsoft/git custom advice below: */ + ADVICE_GVFS_HELPER_TRANSIENT_RETRY, }; int git_default_advice_config(const char *var, const char *value); diff --git a/gvfs-helper.c b/gvfs-helper.c index 9f96ceb97929e4..4163fe11c61f32 100644 --- a/gvfs-helper.c +++ b/gvfs-helper.c @@ -255,6 +255,7 @@ #include "packfile.h" #include "date.h" #include "versioncmp.h" +#include "advice.h" #define TR2_CAT "gvfs-helper" @@ -3016,6 +3017,32 @@ static int compute_transient_delay(int attempt) return v; } +static void gvfs_advice_on_retry(void) +{ + static int advice_given = 0; + + if (advice_given) + return; + advice_given = 1; + + if (gvfs_shared_cache_pathname.len) { + advise_if_enabled(ADVICE_GVFS_HELPER_TRANSIENT_RETRY, + "These retries may hint towards issues with your disk or\n" + "shared object cache. Check to see if your disk is full.\n" + "If your disk has space, then your shared object cache\n" + "may have corrupt files. Push all local branches then\n" + "delete '%s'\n" + "and run 'git fetch' to reload the cache.", + gvfs_shared_cache_pathname.buf); + } else { + advise_if_enabled(ADVICE_GVFS_HELPER_TRANSIENT_RETRY, + "These retries may hint towards issues with your disk.\n" + "Check to see if your disk is full. Note also that you\n" + "do not have a gvfs.sharedCache config, which is not\n" + "normal. You may need to delete and reclone this repo."); + } +} + /* * Robustly make an HTTP request. Retry if necessary to hide common * transient network errors and/or 429 blockages. @@ -3058,6 +3085,10 @@ static void do_req__with_robust_retry(const char *url_base, /*fallthru*/ case GH__RETRY_MODE__TRANSIENT: + /* + * Give advice for common reasons this could happen: + */ + gvfs_advice_on_retry(); params->k_transient_delay_sec = compute_transient_delay(params->k_attempt); continue; From e123e6964c22dd1b00d0172b06ae49ab586f32b8 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Thu, 8 Jan 2026 16:39:07 -0500 Subject: [PATCH 3/3] gvfs-helper: avoid collision check for packfiles Similar to a recent change to avoid the collision check for loose objects, do the same for prefetch packfiles. This should be more rare, but the same prefetch packfile could be downloaded from the same cache server so this isn't out of the range of possibility. Signed-off-by: Derrick Stolee --- gvfs-helper.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/gvfs-helper.c b/gvfs-helper.c index 4163fe11c61f32..77bc65996ded36 100644 --- a/gvfs-helper.c +++ b/gvfs-helper.c @@ -1940,12 +1940,21 @@ static void my_finalize_packfile(struct gh__request_params *params, * 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() + * silently eat the error. Note that finalize_object_file_flags() * has already munged errno (and it has various creation * strategies), so we don't bother looking at it. + * + * We use FOF_SKIP_COLLISION_CHECK in case the same packfile was + * attempted for install earlier but got corrupted or failed to + * flush due to a disk issue. This prevents a narrow failure case + * but is better than failing for silly reasons. */ - if (finalize_object_file(the_repository, temp_path_pack->buf, final_path_pack->buf) || - finalize_object_file(the_repository, temp_path_idx->buf, final_path_idx->buf)) { + if (finalize_object_file_flags(the_repository, + temp_path_pack->buf, final_path_pack->buf, + FOF_SKIP_COLLISION_CHECK) || + finalize_object_file_flags(the_repository, + temp_path_idx->buf, final_path_idx->buf, + FOF_SKIP_COLLISION_CHECK)) { unlink(temp_path_pack->buf); unlink(temp_path_idx->buf);