From 59f2d7e82fb78e23d0f3d0f7635d5f3a7abd6da2 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 24 Sep 2026 10:46:21 -0700 Subject: [PATCH 01/11] Fail fast on SHA256 repositories in FastFetch and gvfs clone Git 3.0 changes 'git init' to default to the SHA256 object format instead of SHA1. VFS for Git's own code assumes SHA1 (20-byte / 40-hex-char) object ids throughout (Sha1Id, SHA1Util, GitIndexGenerator, FastFetch's index reader/writer), and the GVFS-protocol server does not support SHA256. Without a check, a SHA256 repository reaching this code would produce data corruption or an unpredictable crash instead of a clear error. There is no supported way to convert an existing SHA256 repository to SHA1 in place - an object's id is a hash of its content plus the algorithm used to compute it, so "downgrading" means rewriting every object with a new identity, effectively a fresh clone. The only sound mitigation is detecting a SHA256 repository early and failing with an actionable error. Changes: - Add GVFS.Common.Git.ObjectFormat, a small helper that reads extensions.objectformat from a repository's local git config and reports whether it identifies a SHA256 repository. - Check it in FastFetch's startup, right after the enlistment is resolved and before any code that assumes SHA1-shaped object ids runs. FastFetch operates against arbitrary pre-existing repositories, not just ones 'gvfs clone' created, so it is the realistic path where a SHA256 repository could be encountered. - Check it in 'gvfs clone' right after 'git init', as defense-in-depth in case a future code path invokes 'git init' without pinning --object-format=sha1. 'gvfs mount' is intentionally out of scope: once 'gvfs clone' pins 'git init' to SHA1, the only enlistments 'gvfs mount' ever operates on are ones 'gvfs clone' itself created. A SHA256 .gvfs enlistment reaching 'gvfs mount' would require hand-editing .git config after the fact, an unsupported, self-inflicted state not worth guarding against. Assisted-by: Claude Sonnet 5 Signed-off-by: Tyrie Vella --- GVFS/FastFetch/FastFetchVerb.cs | 19 +++-- GVFS/GVFS.Common/Git/ObjectFormat.cs | 47 +++++++++++ GVFS/GVFS.UnitTests/Git/ObjectFormatTests.cs | 85 ++++++++++++++++++++ GVFS/GVFS/CommandLine/CloneVerb.cs | 9 +++ 4 files changed, 154 insertions(+), 6 deletions(-) create mode 100644 GVFS/GVFS.Common/Git/ObjectFormat.cs create mode 100644 GVFS/GVFS.UnitTests/Git/ObjectFormatTests.cs diff --git a/GVFS/FastFetch/FastFetchVerb.cs b/GVFS/FastFetch/FastFetchVerb.cs index 0b0c3147a7..c051ee3074 100644 --- a/GVFS/FastFetch/FastFetchVerb.cs +++ b/GVFS/FastFetch/FastFetchVerb.cs @@ -205,12 +205,13 @@ private int ExecuteWithExitCode() } // Check this before any code that reads or writes refs assuming the "files" - // ref storage format runs, since FastFetch can be pointed at an arbitrary - // pre-existing git repository rather than only ones 'gvfs clone' created. - // A genuine config-read failure (as opposed to the key simply being absent, - // which reports the repository as files-format) is treated as fatal here: a - // missing key does not reach this branch, so a read error means the repo's - // config is anomalous and continuing risks operating on an unsupported repo. + // ref storage format, or any code that assumes SHA1-shaped object ids, runs - + // since FastFetch can be pointed at an arbitrary pre-existing git repository + // rather than only ones 'gvfs clone' created. A genuine ref-storage config-read + // failure (as opposed to the key simply being absent, which reports the + // repository as files-format) is treated as fatal here: a missing key does not + // reach this branch, so a read error means the repo's config is anomalous and + // continuing risks operating on an unsupported repo. if (!RefStorage.TryIsReftableRepo(enlistment.CreateGitProcess(), out bool isReftableRepo, out string refStorageReadError)) { Console.WriteLine("Could not determine the repository's ref storage format: " + refStorageReadError); @@ -222,6 +223,12 @@ private int ExecuteWithExitCode() return ExitFailure; } + if (ObjectFormat.IsSha256Repo(enlistment.CreateGitProcess())) + { + Console.WriteLine(ObjectFormat.UnsupportedSha256ErrorMessage); + return ExitFailure; + } + string commitish = this.Commit ?? this.Branch; if (string.IsNullOrWhiteSpace(commitish)) { diff --git a/GVFS/GVFS.Common/Git/ObjectFormat.cs b/GVFS/GVFS.Common/Git/ObjectFormat.cs new file mode 100644 index 0000000000..2855193105 --- /dev/null +++ b/GVFS/GVFS.Common/Git/ObjectFormat.cs @@ -0,0 +1,47 @@ +using System; + +namespace GVFS.Common.Git +{ + /// + /// Detects whether a git repository was initialized with the SHA256 object format + /// (git's newer, opt-in hash algorithm) rather than the SHA1 format VFS for Git + /// assumes throughout its object id handling. + /// + /// There is no supported way to convert a SHA256 repository to SHA1 in place: an + /// object's id is a hash of its content plus the algorithm used to compute it, so + /// "downgrading" would mean rewriting every object with a new identity - effectively + /// a fresh clone. Callers should use this detection to fail fast with a clear error + /// rather than let SHA1-only code (e.g. Sha1Id, SHA1Util, index parsing) hit + /// corruption or an unpredictable crash on 20-byte/40-hex-char assumptions. + /// + public static class ObjectFormat + { + public const string ObjectFormatConfigName = "extensions.objectformat"; + public const string Sha256Value = "sha256"; + + public const string UnsupportedSha256ErrorMessage = + "VFS for Git only supports SHA1 repositories, but this repository was initialized " + + "with the SHA256 object format. There is no supported way to convert a SHA256 " + + "repository to SHA1 in place. Re-clone or re-initialize the repository as a SHA1 " + + "repository (for example, 'git init --object-format=sha1')."; + + /// + /// Returns true if the given value of extensions.objectformat identifies a SHA256 + /// repository. A missing/empty value means the repository defaults to SHA1. + /// + public static bool IsSha256(string objectFormatConfigValue) + { + return string.Equals(objectFormatConfigValue?.Trim(), Sha256Value, StringComparison.OrdinalIgnoreCase); + } + + /// + /// Reads extensions.objectformat from the local config of the repo backing the given + /// GitProcess, and returns true if it identifies a SHA256 repository. + /// + public static bool IsSha256Repo(GitProcess git) + { + GitProcess.ConfigResult result = git.GetFromLocalConfig(ObjectFormatConfigName); + return result.TryParseAsString(out string value, out string _) && IsSha256(value); + } + } +} diff --git a/GVFS/GVFS.UnitTests/Git/ObjectFormatTests.cs b/GVFS/GVFS.UnitTests/Git/ObjectFormatTests.cs new file mode 100644 index 0000000000..d39184dd9f --- /dev/null +++ b/GVFS/GVFS.UnitTests/Git/ObjectFormatTests.cs @@ -0,0 +1,85 @@ +using GVFS.Common.Git; +using GVFS.Tests.Should; +using GVFS.UnitTests.Mock.Git; +using NUnit.Framework; + +namespace GVFS.UnitTests.Git +{ + [TestFixture] + public class ObjectFormatTests + { + private const string ConfigCommand = "config --local " + ObjectFormat.ObjectFormatConfigName; + + [TestCase] + public void IsSha256IsTrueForSha256Value() + { + ObjectFormat.IsSha256("sha256").ShouldEqual(true); + } + + [TestCase] + public void IsSha256IsTrueForMixedCaseSha256Value() + { + ObjectFormat.IsSha256("SHA256").ShouldEqual(true); + } + + [TestCase] + public void IsSha256IsTrueForSha256ValueWithSurroundingWhitespace() + { + ObjectFormat.IsSha256(" sha256 \n").ShouldEqual(true); + } + + [TestCase] + public void IsSha256IsFalseForSha1Value() + { + ObjectFormat.IsSha256("sha1").ShouldEqual(false); + } + + [TestCase] + public void IsSha256IsFalseForNullValue() + { + ObjectFormat.IsSha256(null).ShouldEqual(false); + } + + [TestCase] + public void IsSha256IsFalseForEmptyValue() + { + ObjectFormat.IsSha256(string.Empty).ShouldEqual(false); + } + + [TestCase] + public void IsSha256RepoIsTrueWhenConfigIsSha256() + { + MockGitProcess git = new MockGitProcess(); + git.SetExpectedCommandResult( + ConfigCommand, + () => new GitProcess.Result("sha256\n", string.Empty, GitProcess.Result.SuccessCode)); + + ObjectFormat.IsSha256Repo(git).ShouldEqual(true); + } + + [TestCase] + public void IsSha256RepoIsFalseWhenConfigIsSha1() + { + MockGitProcess git = new MockGitProcess(); + git.SetExpectedCommandResult( + ConfigCommand, + () => new GitProcess.Result("sha1\n", string.Empty, GitProcess.Result.SuccessCode)); + + ObjectFormat.IsSha256Repo(git).ShouldEqual(false); + } + + [TestCase] + public void IsSha256RepoIsFalseWhenConfigIsMissing() + { + MockGitProcess git = new MockGitProcess(); + + // A missing config key causes 'git config' to exit non-zero with no stderr output, + // which is the observed behavior for a SHA1 repo that predates extensions.objectformat. + git.SetExpectedCommandResult( + ConfigCommand, + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.GenericFailureCode)); + + ObjectFormat.IsSha256Repo(git).ShouldEqual(false); + } + } +} diff --git a/GVFS/GVFS/CommandLine/CloneVerb.cs b/GVFS/GVFS/CommandLine/CloneVerb.cs index ce579d0459..8233df1bd0 100644 --- a/GVFS/GVFS/CommandLine/CloneVerb.cs +++ b/GVFS/GVFS/CommandLine/CloneVerb.cs @@ -888,6 +888,15 @@ private Result TryInitRepo(ITracer tracer, GitRefs refs, Enlistment enlistmentTo return new Result(RefStorage.UnsupportedReftableErrorMessage); } + // Defense-in-depth: 'git init' should always be pinned to SHA1 for GVFS + // enlistments, but detect a SHA256 repo here too in case that pin is ever + // bypassed by some other code path. + if (ObjectFormat.IsSha256Repo(enlistmentToInit.CreateGitProcess())) + { + tracer.RelatedError(ObjectFormat.UnsupportedSha256ErrorMessage); + return new Result(ObjectFormat.UnsupportedSha256ErrorMessage); + } + try { GVFSPlatform.Instance.FileSystem.EnsureDirectoryIsOwnedByCurrentUser(enlistmentToInit.DotGitRoot); From 0909d96a4fcbcb9448522f8175f23648cb385fc6 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 24 Sep 2026 10:58:42 -0700 Subject: [PATCH 02/11] Address review-swarm feedback on SHA256 detection Self-review (review-swarm) surfaced two cross-model-convergent findings on the SHA256 detection change: - GVFS.Common.Git.ObjectFormat.IsSha256Repo collapsed "config key missing" (a legitimate, safe default meaning SHA1) and "git config genuinely failed to read" into the same false result, so a real config-read failure was silently treated as "not SHA256" with no visibility. Split the helper into a TryIsSha256Repo(git, out isSha256, out error) that surfaces a read failure distinctly, plus a convenience IsSha256Repo wrapper. Both call sites now log/print a config-read error instead of swallowing it, while intentionally keeping the same fail-open behavior as GitProcess.TryGetFromConfig elsewhere in this codebase for optional config reads (documented in the XML doc comment). - CloneVerb.TryInitRepo's comment described its check as "defense-in-depth" behind a SHA1 pin on 'git init', but that pin does not exist anywhere in this codebase yet (it is a separate, planned change). Reworded the comment to say plainly that, until that pin lands, this check is the only thing preventing 'gvfs clone' from producing an unusable SHA256 enlistment once a Git 3.0+ client defaults 'init' to SHA256. Added three more unit tests for the new tri-state TryIsSha256Repo API (success/sha256, success/missing-key, and genuine read failure). Assisted-by: Claude Sonnet 5 Signed-off-by: Tyrie Vella --- GVFS/FastFetch/FastFetchVerb.cs | 6 ++- GVFS/GVFS.Common/Git/ObjectFormat.cs | 27 +++++++++++- GVFS/GVFS.UnitTests/Git/ObjectFormatTests.cs | 43 ++++++++++++++++++++ GVFS/GVFS/CommandLine/CloneVerb.cs | 17 ++++++-- 4 files changed, 87 insertions(+), 6 deletions(-) diff --git a/GVFS/FastFetch/FastFetchVerb.cs b/GVFS/FastFetch/FastFetchVerb.cs index c051ee3074..8acb21f13a 100644 --- a/GVFS/FastFetch/FastFetchVerb.cs +++ b/GVFS/FastFetch/FastFetchVerb.cs @@ -223,7 +223,11 @@ private int ExecuteWithExitCode() return ExitFailure; } - if (ObjectFormat.IsSha256Repo(enlistment.CreateGitProcess())) + if (!ObjectFormat.TryIsSha256Repo(enlistment.CreateGitProcess(), out bool isSha256Repo, out string objectFormatReadError)) + { + Console.WriteLine("Warning: could not determine the repository's object format: " + objectFormatReadError); + } + else if (isSha256Repo) { Console.WriteLine(ObjectFormat.UnsupportedSha256ErrorMessage); return ExitFailure; diff --git a/GVFS/GVFS.Common/Git/ObjectFormat.cs b/GVFS/GVFS.Common/Git/ObjectFormat.cs index 2855193105..5e46aebe49 100644 --- a/GVFS/GVFS.Common/Git/ObjectFormat.cs +++ b/GVFS/GVFS.Common/Git/ObjectFormat.cs @@ -37,11 +37,36 @@ public static bool IsSha256(string objectFormatConfigValue) /// /// Reads extensions.objectformat from the local config of the repo backing the given /// GitProcess, and returns true if it identifies a SHA256 repository. + /// + /// A config read failure (e.g. a broken git invocation, as opposed to the key simply + /// being absent) is treated the same as "not SHA256" here, matching the established + /// behavior of GitProcess.TryGetFromConfig elsewhere in this codebase for optional + /// config reads. Use if the caller wants to distinguish + /// and surface that failure instead of silently treating it as SHA1. /// public static bool IsSha256Repo(GitProcess git) + { + TryIsSha256Repo(git, out bool isSha256, out string _); + return isSha256; + } + + /// + /// Reads extensions.objectformat from the local config of the repo backing the given + /// GitProcess. Returns false and populates if the config + /// could not be read at all (a missing key is not an error - it simply means the + /// repository defaults to SHA1, and isSha256 is set to false). + /// + public static bool TryIsSha256Repo(GitProcess git, out bool isSha256, out string error) { GitProcess.ConfigResult result = git.GetFromLocalConfig(ObjectFormatConfigName); - return result.TryParseAsString(out string value, out string _) && IsSha256(value); + if (!result.TryParseAsString(out string value, out error)) + { + isSha256 = false; + return false; + } + + isSha256 = IsSha256(value); + return true; } } } diff --git a/GVFS/GVFS.UnitTests/Git/ObjectFormatTests.cs b/GVFS/GVFS.UnitTests/Git/ObjectFormatTests.cs index d39184dd9f..572e901f76 100644 --- a/GVFS/GVFS.UnitTests/Git/ObjectFormatTests.cs +++ b/GVFS/GVFS.UnitTests/Git/ObjectFormatTests.cs @@ -81,5 +81,48 @@ public void IsSha256RepoIsFalseWhenConfigIsMissing() ObjectFormat.IsSha256Repo(git).ShouldEqual(false); } + + [TestCase] + public void TryIsSha256RepoSucceedsWithNoErrorWhenConfigIsSha256() + { + MockGitProcess git = new MockGitProcess(); + git.SetExpectedCommandResult( + ConfigCommand, + () => new GitProcess.Result("sha256\n", string.Empty, GitProcess.Result.SuccessCode)); + + ObjectFormat.TryIsSha256Repo(git, out bool isSha256, out string error).ShouldEqual(true); + isSha256.ShouldEqual(true); + error.ShouldEqual(string.Empty); + } + + [TestCase] + public void TryIsSha256RepoSucceedsWithNoErrorWhenConfigIsMissing() + { + MockGitProcess git = new MockGitProcess(); + git.SetExpectedCommandResult( + ConfigCommand, + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.GenericFailureCode)); + + ObjectFormat.TryIsSha256Repo(git, out bool isSha256, out string error).ShouldEqual(true); + isSha256.ShouldEqual(false); + error.ShouldEqual(string.Empty); + } + + [TestCase] + public void TryIsSha256RepoFailsAndReportsErrorWhenConfigReadFails() + { + MockGitProcess git = new MockGitProcess(); + + // A genuine 'git config' failure (non-zero exit with real stderr content, as + // opposed to a missing key) should be surfaced distinctly rather than silently + // treated the same as "not SHA256". + git.SetExpectedCommandResult( + ConfigCommand, + () => new GitProcess.Result(string.Empty, "fatal: not a git repository", GitProcess.Result.GenericFailureCode)); + + ObjectFormat.TryIsSha256Repo(git, out bool isSha256, out string error).ShouldEqual(false); + isSha256.ShouldEqual(false); + error.ShouldNotBeNull(); + } } } diff --git a/GVFS/GVFS/CommandLine/CloneVerb.cs b/GVFS/GVFS/CommandLine/CloneVerb.cs index 8233df1bd0..50c1a4f98b 100644 --- a/GVFS/GVFS/CommandLine/CloneVerb.cs +++ b/GVFS/GVFS/CommandLine/CloneVerb.cs @@ -888,10 +888,19 @@ private Result TryInitRepo(ITracer tracer, GitRefs refs, Enlistment enlistmentTo return new Result(RefStorage.UnsupportedReftableErrorMessage); } - // Defense-in-depth: 'git init' should always be pinned to SHA1 for GVFS - // enlistments, but detect a SHA256 repo here too in case that pin is ever - // bypassed by some other code path. - if (ObjectFormat.IsSha256Repo(enlistmentToInit.CreateGitProcess())) + // 'git init' above is not currently pinned to SHA1 anywhere in this codebase - + // that pin is expected to land in a separate change. Until it does, this check + // is the only thing preventing 'gvfs clone' from silently producing an + // unusable SHA256 enlistment once a Git 3.0+ client defaults 'init' to SHA256; + // it becomes true defense-in-depth once the pin is in place. A config-read + // failure here (as opposed to the key simply being absent) is logged but does + // not block the clone, matching how other optional config reads in this + // codebase (e.g. GitProcess.TryGetFromConfig) treat read failures. + if (!ObjectFormat.TryIsSha256Repo(enlistmentToInit.CreateGitProcess(), out bool isSha256Repo, out string objectFormatReadError)) + { + tracer.RelatedWarning("Could not determine the new repository's object format: " + objectFormatReadError); + } + else if (isSha256Repo) { tracer.RelatedError(ObjectFormat.UnsupportedSha256ErrorMessage); return new Result(ObjectFormat.UnsupportedSha256ErrorMessage); From 0890bbfd05c1c05ec6d53c30768ca7afc2c9aa72 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 24 Sep 2026 11:46:27 -0700 Subject: [PATCH 03/11] Also fail fast on SHA256 repositories in gvfs mount Empirically tested the originally out-of-scope 'gvfs mount' path against a hand-crafted SHA256 repo (core.repositoryformatversion=1, extensions.objectformat=sha256) and found the current failure mode significantly worse than expected: - The background mount process (GVFS.Mount.exe) proceeds until TrySetRequiredGitConfigSettings, which unconditionally writes core.repositoryformatversion back to 0 (to match plain 'git init', with no knowledge of extensions.objectformat). Applied to a SHA256 repo, this leaves core.repositoryformatversion=0 with a v1-only extension still present - a config state real git itself refuses to parse at all ("repo version is 0, but v1-only extension found"). This corrupts the enlistment's git config in place; even 'gvfs status'/'gvfs unmount' stopped working until the config was hand-repaired. - The parent 'gvfs mount' CLI then surfaces a raw BrokenPipeException with a full .NET stack trace on the console (the mount process's pipe breaks when it exits), not any actionable message. Changes: - GVFS.Mount.InProcessMount.Mount now checks ObjectFormat.TryIsSha256Repo right after confirming the repo is valid (git.IsValidRepo()) and before TrySetRequiredGitConfigSettings runs, so a SHA256 repo is rejected with the same clear error used by FastFetch and 'gvfs clone', before any config mutation can corrupt it. - GVFSEnlistment.WaitUntilMounted's BrokenPipeException handler now uses e.Message instead of e.ToString() for the console-facing error, so a mount process that exits early (for this or any other reason) no longer dumps a raw stack trace to the console. The full exception detail still goes to the trace log, and the existing "Run 'gvfs log ...' for more info" hint continues to point at the mount process's own log, which carries the specific failure reason. Manually verified end-to-end against a real GVFS enlistment (a dotnet-built GVFS.Mount.exe run both standalone and via a matching dotnet-built gvfs.exe): the SHA256 repo's config is left untouched (repositoryformatversion stays 1, no corruption), the mount process exits cleanly with the SHA1-only error text in its log, and the console now shows a short "Could not connect to GVFS.Mount: Unable to send: GetStatus" line instead of a stack trace. No automated test is added for this end-to-end mount path: InProcessMount has no existing unit-test seam (it is not referenced by GVFS.UnitTests), and a functional test would need a real ProjFS mount, which this worktree's build cache does not have seeded (no native payload build). The manual, real-repo verification above stands in for that coverage. Assisted-by: Claude Sonnet 5 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/GVFSEnlistment.cs | 12 ++++++++++-- GVFS/GVFS.Mount/InProcessMount.cs | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/GVFS/GVFS.Common/GVFSEnlistment.cs b/GVFS/GVFS.Common/GVFSEnlistment.cs index 961d546242..30570f7e03 100644 --- a/GVFS/GVFS.Common/GVFSEnlistment.cs +++ b/GVFS/GVFS.Common/GVFSEnlistment.cs @@ -311,8 +311,16 @@ public static bool WaitUntilMounted( } catch (BrokenPipeException e) { - errorMessage = string.Format("Could not connect to GVFS.Mount: {0}", e); - tracer.RelatedError($"{nameof(WaitUntilMounted)}: {errorMessage}"); + // Keep the console-facing message short - the mount process may have + // exited legitimately (e.g. it already logged its own clear, specific + // failure reason and called Environment.Exit, which is enough to break + // an in-flight pipe write/read here). The full exception detail still + // goes to this trace, and 'gvfs log' points users at the mount + // process's own log, where the real root cause is recorded. + errorMessage = string.Format("Could not connect to GVFS.Mount: {0}", e.Message); + tracer.RelatedError( + new EventMetadata { { "Exception", e.ToString() } }, + $"{nameof(WaitUntilMounted)}: {errorMessage}"); return false; } catch (JsonException e) diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index a6b5d9e94b..a5d64de696 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -280,6 +280,8 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords) this.FailMountAndExit("The .git folder is missing or has invalid contents"); } + // Check this before TrySetRequiredGitConfigSettings, which unconditionally + // forces core.repositoryformatversion back to 0 (matching what 'git init' // Check this before TrySetRequiredGitConfigSettings, which unconditionally // forces core.repositoryformatversion back to 0 (matching what 'git init' // writes for a files-format repo) without knowing about extensions.refstorage. @@ -306,6 +308,18 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords) this.FailMountAndExit(RefStorage.UnsupportedReftableErrorMessage); } + // Same rationale as above, for SHA256: applying the required-config + // force-write to a SHA256 repo (extensions.objectformat) would brick it + // identically, so this check must also run before that write. + if (!ObjectFormat.TryIsSha256Repo(git, out bool isSha256Repo, out string objectFormatReadError)) + { + this.tracer.RelatedWarning("Could not determine the repository's object format: " + objectFormatReadError); + } + else if (isSha256Repo) + { + this.FailMountAndExit(ObjectFormat.UnsupportedSha256ErrorMessage); + } + if (!GVFSPlatform.Instance.FileSystem.IsFileSystemSupported(this.enlistment.WorkingDirectoryRoot, out string fsError)) { this.FailMountAndExit("FileSystem unsupported: " + fsError); From 7da04ed1d379095477c2028a9eaad7d8f628b43a Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 24 Sep 2026 12:02:38 -0700 Subject: [PATCH 04/11] Address review-swarm feedback on mount-side SHA256 detection Self-review (review-swarm iteration 2) on the mount commit surfaced one cross-model-convergent finding worth fixing, plus one test-coverage gap: - GitConfigRepairJob.TryFixIssues calls the same GVFSVerb.TrySetRequiredGitConfigSettings that InProcessMount now guards, but was not itself guarded. Worse, TryFixIssues first wipes .git/config to empty and rebuilds it from only the required/optional settings this codebase knows about - so on a SHA256 repo, 'gvfs repair' would have silently erased extensions.objectformat and rewritten a SHA1-shaped config over live SHA256-hashed objects, a more silent form of the same corruption hazard this PR fixes elsewhere. Added the same ObjectFormat.TryIsSha256Repo check to the top of TryFixIssues, before the config file is touched. There is no existing unit-test infrastructure for RepairJobs at all in this codebase, so no new test was added for this specific fix, consistent with the rest of this PR's manually-verified pieces. - GVFSEnlistment.WaitUntilMounted's BrokenPipeException message-cleanup fix (from the previous commit) had no test coverage, unlike the existing WaitUntilMountedProcessTrackingTests.cs, which only covers the connect-retry path. Added a test that opens a real named pipe server, accepts the client's connection, reads its first request, then disappears without responding - deterministically driving the exact BrokenPipeException branch this PR changed - and asserts the console-facing message stays short while the full exception detail is still captured via the tracer. Assisted-by: Claude Sonnet 5 Signed-off-by: Tyrie Vella --- .../WaitUntilMountedProcessTrackingTests.cs | 48 +++++++++++++++++++ GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs | 13 +++++ 2 files changed, 61 insertions(+) diff --git a/GVFS/GVFS.UnitTests/Common/WaitUntilMountedProcessTrackingTests.cs b/GVFS/GVFS.UnitTests/Common/WaitUntilMountedProcessTrackingTests.cs index cf04194ebe..e15a93c01f 100644 --- a/GVFS/GVFS.UnitTests/Common/WaitUntilMountedProcessTrackingTests.cs +++ b/GVFS/GVFS.UnitTests/Common/WaitUntilMountedProcessTrackingTests.cs @@ -1,9 +1,12 @@ using GVFS.Common; +using GVFS.Common.NamedPipes; using GVFS.Tests.Should; using GVFS.UnitTests.Mock.Common; using NUnit.Framework; using System; +using System.IO.Pipes; using System.Threading; +using System.Threading.Tasks; namespace GVFS.UnitTests.Common { @@ -84,5 +87,50 @@ public void DetectsLateProcessExitWhilePipeNeverAppears() // exit on the second poll, well within a few seconds. Assert.That(elapsed.TotalSeconds, Is.LessThan(10), "WaitUntilMounted should detect late process exit within a few connect retries"); } + + [TestCase] + public void BrokenPipeMidPollProducesShortMessageAndLogsFullExceptionDetail() + { + string pipeName = "GVFS_UnitTest_BrokenPipe_" + Guid.NewGuid().ToString("N"); + + using (NamedPipeServerStream serverStream = new NamedPipeServerStream( + pipeName, PipeDirection.InOut, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous)) + { + Task serverTask = Task.Run(() => + { + serverStream.WaitForConnection(); + + // Read the client's first GetStatus request using the same framing + // NamedPipeStreamWriter/Reader use, then disappear without responding - + // simulating the mount process exiting mid-poll (e.g. FailMountAndExit + // calling Environment.Exit while the client already sent a request). + new NamedPipeStreamReader(serverStream).ReadMessage(); + serverStream.Dispose(); + }); + + MockTracer tracer = new MockTracer(); + string errorMessage; + bool result = GVFSEnlistment.WaitUntilMounted( + tracer, + pipeName, + "C:\\fake\\root", + false, + out errorMessage); + + serverTask.Wait(TimeSpan.FromSeconds(10)); + + result.ShouldBeFalse(); + errorMessage.ShouldNotBeNull(); + errorMessage.ShouldContain("Could not connect to GVFS.Mount"); + + // The console-facing message must stay short - no embedded stack trace, + // which is what e.ToString() (instead of e.Message) used to produce. + errorMessage.ShouldNotContain(false, " at ", Environment.NewLine); + + // The full exception detail must still be captured for diagnostics. + tracer.RelatedErrorEvents.Count.ShouldEqual(1); + tracer.RelatedErrorEvents[0].ShouldContain("Exception"); + } + } } } diff --git a/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs b/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs index a7b33df884..fcad452730 100644 --- a/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs +++ b/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs @@ -121,6 +121,19 @@ public override FixResult TryFixIssues(List messages) this.Tracer.RelatedWarning("Could not determine the repository's ref storage format; proceeding with repair: " + refStorageReadError); } + // Unlike ref storage format, SHA256 has no physical on-disk marker independent + // of git config - extensions.objectformat is the only signal - so there is no + // directory-based fallback here the way ReftableBackendDirectoryExists() gives + // the check above. A SHA256 repo whose config is also corrupt cannot be + // distinguished from an ordinary corrupt SHA1 repo, and would be silently + // rebuilt as SHA1; this is an inherent limitation of this axis. + if (ObjectFormat.TryIsSha256Repo(new GitProcess(this.Enlistment), out bool isSha256Repo, out string objectFormatReadError) && + isSha256Repo) + { + messages.Add(ObjectFormat.UnsupportedSha256ErrorMessage); + return FixResult.Failure; + } + string configPath = Path.Combine(this.Enlistment.WorkingDirectoryBackingRoot, GVFSConstants.DotGit.Config); string configBackupPath; if (!this.TryRenameToBackupFile(configPath, out configBackupPath, messages)) From 7c7b5215e6c8b896a4c8a347536d22a77a1518d8 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Fri, 25 Sep 2026 17:00:02 -0700 Subject: [PATCH 05/11] Reconcile SHA256 read-failure policy with the ref-storage (reftable) fix PR #2121 adds an equivalent fail-fast check for reftable repositories (extensions.refstorage) at the same four call sites this PR already checks for SHA256 repositories (extensions.objectformat): FastFetch, 'gvfs clone', 'gvfs mount', and 'gvfs repair'. Reviewing both together surfaced a mismatch: this PR's ObjectFormat check warned and continued on a genuine config-read failure, while #2121's RefStorage check fails closed. Once both checks sit side by side at each call site, having one warn-and-continue and the other fail-closed for the same class of error (an unreadable config, as opposed to a missing key) is inconsistent and confusing. Align this PR's policy with #2121's, since fail-closed is the more defensible default: a missing extensions.objectformat key already reports the repository as SHA1 through the success path, so the error path only fires when the config is genuinely anomalous, and continuing in that state risks the very corruption this check exists to prevent. - FastFetch, clone (TryInitRepo), and mount: a config-read failure is now fatal, matching a missing/successfully-read config being the only non-fatal outcomes. - Mount passes the read error to FailMountAndExit as a format argument ("{0}") rather than concatenating it into the message string: FailMountAndExit routes through ITracer.RelatedError(string, params object[]), which runs string.Format, so a literal '{' in git's stderr (possible with a crafted or corrupt .git/config) would otherwise throw a FormatException instead of failing the mount cleanly. - Repair (GitConfigRepairJob) keeps its existing exception to this policy: a config-read failure there is surfaced as a warning but does not block the repair, because repair's whole purpose is to rebuild a corrupt config. Unlike ref storage format, SHA256 has no physical on-disk marker independent of git config, so - unlike #2121's reftable check, which falls back to checking for a .git/reftable/ directory when the config is unreadable - a SHA256 repo whose config is also corrupt cannot be distinguished from an ordinary corrupt SHA1 repo here. This is an inherent limitation of the object-format axis, not something to fix in this change. - Added a HasIssue check to GitConfigRepairJob so a SHA256 repository is reported as an unfixable issue (matching #2121's structure), instead of 'gvfs diagnose'/'gvfs repair' treating it as healthy. Added a unit test locking in that the non-Try IsSha256Repo overload still swallows a genuine read failure and reports "not SHA256" (only the Try overload distinguishes it), mirroring the equivalent RefStorage test. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/FastFetch/FastFetchVerb.cs | 8 +++- GVFS/GVFS.Mount/InProcessMount.cs | 15 +++++-- GVFS/GVFS.UnitTests/Git/ObjectFormatTests.cs | 16 ++++++++ GVFS/GVFS/CommandLine/CloneVerb.cs | 12 +++--- GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs | 42 ++++++++++++++++---- 5 files changed, 75 insertions(+), 18 deletions(-) diff --git a/GVFS/FastFetch/FastFetchVerb.cs b/GVFS/FastFetch/FastFetchVerb.cs index 8acb21f13a..32d41a65fc 100644 --- a/GVFS/FastFetch/FastFetchVerb.cs +++ b/GVFS/FastFetch/FastFetchVerb.cs @@ -223,9 +223,15 @@ private int ExecuteWithExitCode() return ExitFailure; } + // Same rationale as above, for SHA256: a genuine config-read failure (as + // opposed to the key simply being absent, which reports the repository as + // SHA1) is treated as fatal here: a missing key does not reach this branch, + // so a read error means the repo's config is anomalous and continuing risks + // operating on an unsupported repo. if (!ObjectFormat.TryIsSha256Repo(enlistment.CreateGitProcess(), out bool isSha256Repo, out string objectFormatReadError)) { - Console.WriteLine("Warning: could not determine the repository's object format: " + objectFormatReadError); + Console.WriteLine("Could not determine the repository's object format: " + objectFormatReadError); + return ExitFailure; } else if (isSha256Repo) { diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index a5d64de696..34c672fca3 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -280,8 +280,6 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords) this.FailMountAndExit("The .git folder is missing or has invalid contents"); } - // Check this before TrySetRequiredGitConfigSettings, which unconditionally - // forces core.repositoryformatversion back to 0 (matching what 'git init' // Check this before TrySetRequiredGitConfigSettings, which unconditionally // forces core.repositoryformatversion back to 0 (matching what 'git init' // writes for a files-format repo) without knowing about extensions.refstorage. @@ -310,10 +308,19 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords) // Same rationale as above, for SHA256: applying the required-config // force-write to a SHA256 repo (extensions.objectformat) would brick it - // identically, so this check must also run before that write. + // identically, so this check must also run before that write. A genuine + // config-read failure is fatal here rather than a warning: a missing key + // reports the repo as SHA1 and does not reach this branch, so a read error + // means the config is anomalous and proceeding into the force-write could + // brick an unsupported repo. if (!ObjectFormat.TryIsSha256Repo(git, out bool isSha256Repo, out string objectFormatReadError)) { - this.tracer.RelatedWarning("Could not determine the repository's object format: " + objectFormatReadError); + // Pass the git error as a format argument, not concatenated into the + // format string: FailMountAndExit routes through ITracer.RelatedError( + // string, params object[]), which runs string.Format, so a '{' in the + // git stderr (possible with a crafted/corrupt .git/config) would throw + // a FormatException instead of failing the mount cleanly. + this.FailMountAndExit("Could not determine the repository's object format: {0}", objectFormatReadError); } else if (isSha256Repo) { diff --git a/GVFS/GVFS.UnitTests/Git/ObjectFormatTests.cs b/GVFS/GVFS.UnitTests/Git/ObjectFormatTests.cs index 572e901f76..0832380183 100644 --- a/GVFS/GVFS.UnitTests/Git/ObjectFormatTests.cs +++ b/GVFS/GVFS.UnitTests/Git/ObjectFormatTests.cs @@ -82,6 +82,22 @@ public void IsSha256RepoIsFalseWhenConfigIsMissing() ObjectFormat.IsSha256Repo(git).ShouldEqual(false); } + [TestCase] + public void IsSha256RepoSwallowsGenuineReadFailureAndReturnsFalse() + { + MockGitProcess git = new MockGitProcess(); + + // The non-Try overload deliberately swallows a genuine config-read failure + // (non-zero exit with real stderr content) and reports "not SHA256", matching + // GitProcess.TryGetFromConfig's "failure == not set" convention. Callers that + // need to distinguish a read failure use TryIsSha256Repo instead. + git.SetExpectedCommandResult( + ConfigCommand, + () => new GitProcess.Result(string.Empty, "fatal: not a git repository", GitProcess.Result.GenericFailureCode)); + + ObjectFormat.IsSha256Repo(git).ShouldEqual(false); + } + [TestCase] public void TryIsSha256RepoSucceedsWithNoErrorWhenConfigIsSha256() { diff --git a/GVFS/GVFS/CommandLine/CloneVerb.cs b/GVFS/GVFS/CommandLine/CloneVerb.cs index 50c1a4f98b..1887ede424 100644 --- a/GVFS/GVFS/CommandLine/CloneVerb.cs +++ b/GVFS/GVFS/CommandLine/CloneVerb.cs @@ -892,13 +892,15 @@ private Result TryInitRepo(ITracer tracer, GitRefs refs, Enlistment enlistmentTo // that pin is expected to land in a separate change. Until it does, this check // is the only thing preventing 'gvfs clone' from silently producing an // unusable SHA256 enlistment once a Git 3.0+ client defaults 'init' to SHA256; - // it becomes true defense-in-depth once the pin is in place. A config-read - // failure here (as opposed to the key simply being absent) is logged but does - // not block the clone, matching how other optional config reads in this - // codebase (e.g. GitProcess.TryGetFromConfig) treat read failures. + // it becomes true defense-in-depth once the pin is in place. A genuine + // config-read failure is fatal here: this is our own just-initialized repo, so + // a read error (as opposed to the key being absent, which reports SHA1) means + // its config is unreadable and the clone cannot safely continue. if (!ObjectFormat.TryIsSha256Repo(enlistmentToInit.CreateGitProcess(), out bool isSha256Repo, out string objectFormatReadError)) { - tracer.RelatedWarning("Could not determine the new repository's object format: " + objectFormatReadError); + string readError = "Could not determine the new repository's object format: " + objectFormatReadError; + tracer.RelatedError(readError); + return new Result(readError); } else if (isSha256Repo) { diff --git a/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs b/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs index fcad452730..524d278ae9 100644 --- a/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs +++ b/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs @@ -38,6 +38,19 @@ public override IssueType HasIssue(List messages) return IssueType.CantFix; } + // A SHA256 repository is not something 'gvfs repair' can make usable (see + // TryFixIssues), so report it as an unfixable issue rather than letting + // 'gvfs diagnose'/'gvfs repair' declare the enlistment healthy. Unlike ref + // storage format, SHA256 has no physical on-disk marker independent of git + // config - extensions.objectformat is the only signal - so a config-read + // failure here is not itself flagged; it falls through to the normal config + // checks below, which is exactly what repair is meant to diagnose. + if (ObjectFormat.IsSha256Repo(git)) + { + messages.Add(ObjectFormat.UnsupportedSha256ErrorMessage); + return IssueType.CantFix; + } + GitProcess.ConfigResult originResult = git.GetOriginUrl(); string error; string originUrl; @@ -121,14 +134,27 @@ public override FixResult TryFixIssues(List messages) this.Tracer.RelatedWarning("Could not determine the repository's ref storage format; proceeding with repair: " + refStorageReadError); } - // Unlike ref storage format, SHA256 has no physical on-disk marker independent - // of git config - extensions.objectformat is the only signal - so there is no - // directory-based fallback here the way ReftableBackendDirectoryExists() gives - // the check above. A SHA256 repo whose config is also corrupt cannot be - // distinguished from an ordinary corrupt SHA1 repo, and would be silently - // rebuilt as SHA1; this is an inherent limitation of this axis. - if (ObjectFormat.TryIsSha256Repo(new GitProcess(this.Enlistment), out bool isSha256Repo, out string objectFormatReadError) && - isSha256Repo) + // Same rationale as above, for SHA256: TrySetRequiredGitConfigSettings also + // force-writes core.repositoryformatversion=0 and drops extensions.objectformat. + // Rebuilding a SHA256 repo's config that way would strip the objectformat + // extension while its objects/index remain SHA256-hashed, producing a + // differently-broken repo. There is no way for 'gvfs repair' to make a + // SHA256 repo usable by VFS for Git, so fail clearly instead of attempting + // the rebuild. + // + // Unlike ref storage format, SHA256 has no physical on-disk marker + // independent of git config - extensions.objectformat is the only signal - + // so a config-read failure here is NOT blocked, unlike at mount/clone/ + // FastFetch: repair's whole purpose is to rebuild a corrupt config, so a + // repo whose config is merely unreadable is exactly what repair must be + // allowed to fix. This is an inherent limitation: a SHA256 repo whose + // config is ALSO corrupt cannot be distinguished from an ordinary corrupt + // SHA1 repo here, and would be silently rebuilt as SHA1. + if (!ObjectFormat.TryIsSha256Repo(new GitProcess(this.Enlistment), out bool isSha256Repo, out string objectFormatReadError)) + { + this.Tracer.RelatedWarning("Could not determine the repository's object format; proceeding with repair: " + objectFormatReadError); + } + else if (isSha256Repo) { messages.Add(ObjectFormat.UnsupportedSha256ErrorMessage); return FixResult.Failure; From 8c5bf191387909377cec71da37fbdb2d676c6b3d Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Fri, 25 Sep 2026 17:05:07 -0700 Subject: [PATCH 06/11] Rebase onto vnext and address remaining review comments from Nathan Rebase onto current vnext to pick up the already-merged --object-format=sha1 pin in GitProcess.Init (from the separately-landed "Pin git init to sha1 during clone" change). This was a clean rebase with no conflicts, since that change only touches GitProcess.Init and its own test. Address the remaining actionable findings from review on this PR: - The comment in CloneVerb.TryInitRepo described 'git init' as "not currently pinned to SHA1 anywhere in this codebase," which was already stale relative to vnext (the pin landed there before this PR was even opened) and would only get staler. State the invariant instead of narrating the sequencing between the two changes: 'git init' pins --object-format=sha1, and this check is defense-in-depth for a pre-existing repo that reached v1 by another route. - Removed a "used to produce" history narration from the WaitUntilMounted unit test's comment in favor of describing the behavior the assertion checks now. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- .../WaitUntilMountedProcessTrackingTests.cs | 4 ++-- GVFS/GVFS/CommandLine/CloneVerb.cs | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/GVFS/GVFS.UnitTests/Common/WaitUntilMountedProcessTrackingTests.cs b/GVFS/GVFS.UnitTests/Common/WaitUntilMountedProcessTrackingTests.cs index e15a93c01f..562b5e12d4 100644 --- a/GVFS/GVFS.UnitTests/Common/WaitUntilMountedProcessTrackingTests.cs +++ b/GVFS/GVFS.UnitTests/Common/WaitUntilMountedProcessTrackingTests.cs @@ -123,8 +123,8 @@ public void BrokenPipeMidPollProducesShortMessageAndLogsFullExceptionDetail() errorMessage.ShouldNotBeNull(); errorMessage.ShouldContain("Could not connect to GVFS.Mount"); - // The console-facing message must stay short - no embedded stack trace, - // which is what e.ToString() (instead of e.Message) used to produce. + // The console-facing message must stay short - no embedded stack trace + // (a stack trace is what BrokenPipeException.ToString() would produce). errorMessage.ShouldNotContain(false, " at ", Environment.NewLine); // The full exception detail must still be captured for diagnostics. diff --git a/GVFS/GVFS/CommandLine/CloneVerb.cs b/GVFS/GVFS/CommandLine/CloneVerb.cs index 1887ede424..c672f62811 100644 --- a/GVFS/GVFS/CommandLine/CloneVerb.cs +++ b/GVFS/GVFS/CommandLine/CloneVerb.cs @@ -888,14 +888,14 @@ private Result TryInitRepo(ITracer tracer, GitRefs refs, Enlistment enlistmentTo return new Result(RefStorage.UnsupportedReftableErrorMessage); } - // 'git init' above is not currently pinned to SHA1 anywhere in this codebase - - // that pin is expected to land in a separate change. Until it does, this check - // is the only thing preventing 'gvfs clone' from silently producing an - // unusable SHA256 enlistment once a Git 3.0+ client defaults 'init' to SHA256; - // it becomes true defense-in-depth once the pin is in place. A genuine - // config-read failure is fatal here: this is our own just-initialized repo, so - // a read error (as opposed to the key being absent, which reports SHA1) means - // its config is unreadable and the clone cannot safely continue. + // 'git init' above pins --object-format=sha1, so a user's + // init.defaultObjectFormat=sha256 no longer produces a SHA256 repo on a + // supported git. This check remains as defense-in-depth: it covers any + // pre-existing repo that reached v1 by another route (e.g. an older GVFS + // build, or hand-edited config). A genuine config-read failure is fatal + // here: this is our own just-initialized repo, so a read error (as opposed + // to the key being absent, which reports SHA1) means its config is + // unreadable and the clone cannot safely continue. if (!ObjectFormat.TryIsSha256Repo(enlistmentToInit.CreateGitProcess(), out bool isSha256Repo, out string objectFormatReadError)) { string readError = "Could not determine the new repository's object format: " + objectFormatReadError; From f1a899cbf0b9b9b7e74cd1590f510d4ca4a7de41 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 6 Oct 2026 10:46:18 -0700 Subject: [PATCH 07/11] Fix stale GVFSEnlistment.GitVersion comment flagged on #2121 Nathan's approval comment on #2121 flagged, as a non-blocking nit, that GVFSEnlistment.GitVersion's doc comment still read "only used in logging during clone and mount to track version numbers" even though that PR made the property load-bearing: CloneVerb.CreateClone re-parses it to decide whether to pin 'git init' to --ref-format=files. Since this PR rebases onto that change, fix the comment here rather than leaving it stale for whoever reads this file next. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/GVFSEnlistment.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/GVFS/GVFS.Common/GVFSEnlistment.cs b/GVFS/GVFS.Common/GVFSEnlistment.cs index 30570f7e03..c65fff0ad7 100644 --- a/GVFS/GVFS.Common/GVFSEnlistment.cs +++ b/GVFS/GVFS.Common/GVFSEnlistment.cs @@ -124,7 +124,10 @@ public override string GitHeadLogPath public string GitStatusCacheFolder { get; private set; } public string GitStatusCachePath { get; private set; } - // These version properties are only used in logging during clone and mount to track version numbers + // GitVersion is load-bearing, not just logging: CloneVerb.CreateClone re-parses it + // to decide whether to pin 'git init' to --ref-format=files (GitProcess.Init). The + // other two (GVFSVersion, GVFSHooksVersion) remain logging-only, to track version + // numbers during clone and mount. public string GitVersion { get { return this.gitVersion; } From 4722f37bb6dd51e4ff4b8539a4e085f8938c45d4 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 6 Oct 2026 11:02:47 -0700 Subject: [PATCH 08/11] Address review-swarm feedback after rebase: fix overclaiming comments Self-review (review-swarm iteration 3, run after rebasing onto vnext post #2121-merge) surfaced two HIGH findings, both about comment accuracy rather than behavior: - GitConfigRepairJob's "SHA256 has no physical on-disk marker" comment overclaimed: a freshly-initialized SHA256 repo's loose objects do have a detectably longer path (64 hex chars vs 40) than SHA1's, so a reftable-style physical fallback is not literally impossible. Verified independently, though, that this signal is not practically usable: a GVFS enlistment runs with gc.auto=0 specifically so its own scheduled maintenance (PackfileMaintenanceStep) owns packing, and after a single repack no loose objects remain to probe - unlike RefStorage's .git/reftable/ directory, which persists regardless of maintenance. Reworded both HasIssue's and TryFixIssues's comments to state the real, verified reason a physical fallback isn't viable here, rather than a blanket "no marker exists" claim. - ObjectFormat.TryIsSha256Repo's doc comment implied "a genuine config-read failure is fatal" is a complete guarantee, but 'git config --local' produces the identical exit-1/empty-stderr signature whether a key is simply absent or the entire .git/config file is missing - verified by deleting .git/config from a real SHA256 test repo and confirming the read result is indistinguishable from "key absent". This is not unique to this check: it's the same ambiguity GitProcess.TryGetFromConfig has for every other optional config read in this codebase. Added a doc-comment note stating this limit explicitly, rather than leaving the stronger claim unqualified. No new test needed - the existing TryIsSha256RepoSucceedsWithNoErrorWhenConfigIsMissing test already asserts the exact git-level result shape a missing-config-file case would also produce. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/ObjectFormat.cs | 7 +++++++ GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs | 18 +++++++++++++----- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/GVFS/GVFS.Common/Git/ObjectFormat.cs b/GVFS/GVFS.Common/Git/ObjectFormat.cs index 5e46aebe49..b70d76683f 100644 --- a/GVFS/GVFS.Common/Git/ObjectFormat.cs +++ b/GVFS/GVFS.Common/Git/ObjectFormat.cs @@ -55,6 +55,13 @@ public static bool IsSha256Repo(GitProcess git) /// GitProcess. Returns false and populates if the config /// could not be read at all (a missing key is not an error - it simply means the /// repository defaults to SHA1, and isSha256 is set to false). + /// + /// Note the limits of this distinction: 'git config' reports a missing key and a + /// missing/unreadable .git/config file identically - exit code 1 with no stderr + /// content - so a repository whose entire config is gone is treated as "not SHA256" + /// here, not as a read failure. This matches the codebase-wide convention for + /// optional config reads (GitProcess.TryGetFromConfig has the same ambiguity) rather + /// than a gap specific to this check. /// public static bool TryIsSha256Repo(GitProcess git, out bool isSha256, out string error) { diff --git a/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs b/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs index 524d278ae9..2d10a39339 100644 --- a/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs +++ b/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs @@ -41,8 +41,10 @@ public override IssueType HasIssue(List messages) // A SHA256 repository is not something 'gvfs repair' can make usable (see // TryFixIssues), so report it as an unfixable issue rather than letting // 'gvfs diagnose'/'gvfs repair' declare the enlistment healthy. Unlike ref - // storage format, SHA256 has no physical on-disk marker independent of git - // config - extensions.objectformat is the only signal - so a config-read + // storage format, there is no cheap, reliable physical fallback here - a + // maintained GVFS enlistment (gc.auto=0, its own packing maintenance) has + // no loose objects left to probe for a longer SHA256 path, so + // extensions.objectformat remains the only practical signal - a config-read // failure here is not itself flagged; it falls through to the normal config // checks below, which is exactly what repair is meant to diagnose. if (ObjectFormat.IsSha256Repo(git)) @@ -142,12 +144,18 @@ public override FixResult TryFixIssues(List messages) // SHA256 repo usable by VFS for Git, so fail clearly instead of attempting // the rebuild. // - // Unlike ref storage format, SHA256 has no physical on-disk marker - // independent of git config - extensions.objectformat is the only signal - + // Unlike ref storage format, SHA256 objects/index are not cheap to probe for + // independently of git config: a freshly-initialized repo's loose objects do + // have a detectably longer path (64 hex chars vs. 40) and the index format + // differs, but a GVFS enlistment runs with gc.auto=0 and its own maintenance + // jobs that pack loose objects away (see PackfileMaintenanceStep) - so any + // real, maintained enlistment has no loose objects left to probe, making a + // reftable-style physical-directory fallback unreliable in the case that + // matters most. extensions.objectformat remains the only practical signal, // so a config-read failure here is NOT blocked, unlike at mount/clone/ // FastFetch: repair's whole purpose is to rebuild a corrupt config, so a // repo whose config is merely unreadable is exactly what repair must be - // allowed to fix. This is an inherent limitation: a SHA256 repo whose + // allowed to fix. This is an accepted limitation: a SHA256 repo whose // config is ALSO corrupt cannot be distinguished from an ordinary corrupt // SHA1 repo here, and would be silently rebuilt as SHA1. if (!ObjectFormat.TryIsSha256Repo(new GitProcess(this.Enlistment), out bool isSha256Repo, out string objectFormatReadError)) From 332ffc70a5cceda6fa387cb6fea7198e956d3ec7 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Mon, 12 Oct 2026 11:33:55 -0700 Subject: [PATCH 09/11] Address second review round from Nathan on PR 2116 Nathan verified and extended his own earlier feedback after re-reviewing the rebased PR, with four items: - GitConfigRepairJob's SHA256 detection was only half-addressed by the prior "comment-accuracy" fix: Nathan verified that .git/packed-refs (written directly by VFS for Git during clone, via GitRefs.ToPackedRefs and CloneVerb.TryInitRepo) holds object ids at full hex width - 64 chars for SHA256 vs 40 for SHA1 - and that this signal survives `git gc --aggressive --prune=now`, unlike loose objects. Implemented the physical fallback he proposed: added ObjectFormat.Sha256HexIdLength and GitConfigRepairJob.PackedRefsLookSha256(), wired into both HasIssue and TryFixIssues alongside the existing config check, mirroring the pattern RefStorage already uses for reftable's .git/reftable/ directory check. Added unit tests for the new parsing logic (SHA256-width/SHA1-width/missing-file/header-and-peeled-line cases) - the first unit tests ever written for GVFS.RepairJobs - and a functional test, RepairTests.DoesNotRebuildSha256RepoConfig, that exercises the full mount+repair refusal path against a real enlistment. - Reworded the SHA256 and (mirrored, same issue) RefStorage comments in CloneVerb.TryInitRepo from transition-narrating ("no longer produces") to invariant phrasing ("cannot produce"), per Nathan's suggestion. - Hoisted FastFetchVerb's and CloneVerb's two separate CreateGitProcess() calls (one per axis check) into a single shared GitProcess instance each, matching the pattern InProcessMount already used. - Disclosed the GVFSEnlistment.GitVersion comment fix (addressing Nathan's earlier #2121 nit) in the PR description's Changes list, since it was a third change bundled into this PR without being itemized. Also updated the PR title and description for the newly-implemented physical fallback and new test coverage. Assisted-by: Claude Sonnet 5 Signed-off-by: Tyrie Vella --- GVFS/FastFetch/FastFetchVerb.cs | 5 +- GVFS/GVFS.Common/Git/ObjectFormat.cs | 13 +++ .../EnlistmentPerTestCase/RepairTests.cs | 32 ++++++ .../CommandLine/GitConfigRepairJobTests.cs | 88 +++++++++++++++++ GVFS/GVFS/CommandLine/CloneVerb.cs | 28 +++--- GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs | 97 +++++++++++++------ 6 files changed, 220 insertions(+), 43 deletions(-) create mode 100644 GVFS/GVFS.UnitTests/CommandLine/GitConfigRepairJobTests.cs diff --git a/GVFS/FastFetch/FastFetchVerb.cs b/GVFS/FastFetch/FastFetchVerb.cs index 32d41a65fc..6762e258dd 100644 --- a/GVFS/FastFetch/FastFetchVerb.cs +++ b/GVFS/FastFetch/FastFetchVerb.cs @@ -212,7 +212,8 @@ private int ExecuteWithExitCode() // repository as files-format) is treated as fatal here: a missing key does not // reach this branch, so a read error means the repo's config is anomalous and // continuing risks operating on an unsupported repo. - if (!RefStorage.TryIsReftableRepo(enlistment.CreateGitProcess(), out bool isReftableRepo, out string refStorageReadError)) + GitProcess git = enlistment.CreateGitProcess(); + if (!RefStorage.TryIsReftableRepo(git, out bool isReftableRepo, out string refStorageReadError)) { Console.WriteLine("Could not determine the repository's ref storage format: " + refStorageReadError); return ExitFailure; @@ -228,7 +229,7 @@ private int ExecuteWithExitCode() // SHA1) is treated as fatal here: a missing key does not reach this branch, // so a read error means the repo's config is anomalous and continuing risks // operating on an unsupported repo. - if (!ObjectFormat.TryIsSha256Repo(enlistment.CreateGitProcess(), out bool isSha256Repo, out string objectFormatReadError)) + if (!ObjectFormat.TryIsSha256Repo(git, out bool isSha256Repo, out string objectFormatReadError)) { Console.WriteLine("Could not determine the repository's object format: " + objectFormatReadError); return ExitFailure; diff --git a/GVFS/GVFS.Common/Git/ObjectFormat.cs b/GVFS/GVFS.Common/Git/ObjectFormat.cs index b70d76683f..6a9e6f821e 100644 --- a/GVFS/GVFS.Common/Git/ObjectFormat.cs +++ b/GVFS/GVFS.Common/Git/ObjectFormat.cs @@ -19,6 +19,19 @@ public static class ObjectFormat public const string ObjectFormatConfigName = "extensions.objectformat"; public const string Sha256Value = "sha256"; + /// + /// Length, in hex characters, of a SHA256 object id (32 bytes) - twice + /// GVFSConstants.ShaStringLength, the equivalent SHA1 length. Object ids are + /// written at full hex width wherever git records them as text - notably + /// .git/packed-refs ("<objectid> <refname>") and loose ref files - so this + /// is a physical, on-disk signal of a repository's object format that survives + /// even when git config is unreadable. Unlike loose objects, packed-refs is not + /// touched by git's own packing maintenance, and VFS for Git writes it directly + /// on every clone (see GitRefs.ToPackedRefs, CloneVerb.TryInitRepo), so it is + /// reliably present to probe. + /// + public const int Sha256HexIdLength = 64; + public const string UnsupportedSha256ErrorMessage = "VFS for Git only supports SHA1 repositories, but this repository was initialized " + "with the SHA256 object format. There is no supported way to convert a SHA256 " + diff --git a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/RepairTests.cs b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/RepairTests.cs index f7d2516657..70d3746210 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/RepairTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/RepairTests.cs @@ -159,6 +159,38 @@ public void FixesCorruptGitConfig() this.Enlistment.MountGVFS(); } + [TestCase] + public void DoesNotRebuildSha256RepoConfig() + { + this.Enlistment.UnmountGVFS(); + + // Make this otherwise-ordinary enlistment's config look like a SHA256 repo - + // core.repositoryformatversion=1 plus extensions.objectformat=sha256, the shape + // 'git init --object-format=sha256' produces - while leaving the rest of the + // config (including the origin remote) intact and parseable. The enlistment's + // actual objects/refs are still genuinely SHA1-hashed; this exercises the + // config-based detection path specifically, not the packed-refs physical + // fallback (which only fires when the config itself is unreadable). + string configPath = Path.Combine(this.Enlistment.RepoBackingRoot, ".git", "config"); + string originalConfig = File.ReadAllText(configPath); + string sha256Config = originalConfig.Replace("repositoryformatversion = 0", "repositoryformatversion = 1") + + "[extensions]\n\tobjectformat = sha256\n"; + File.WriteAllText(configPath, sha256Config); + + this.Enlistment.TryMountGVFS().ShouldEqual(false, "GVFS shouldn't mount a SHA256 repo"); + + this.RepairWithoutConfirmShouldNotFix(); + + // Unlike FixesCorruptGitConfig above, repair must refuse here rather than fix: + // rebuilding the config from scratch would silently erase extensions.objectformat + // and leave a SHA1-shaped config over objects/refs that are declared SHA256. + this.Enlistment.Repair(confirm: true); + this.Enlistment.TryMountGVFS().ShouldEqual(false, "Repair must not make a SHA256 repo mountable"); + + string configAfterRepair = File.ReadAllText(configPath); + configAfterRepair.ShouldContain("sha256"); + } + private void CreateCorruptIndexAndRename(string indexPath, Action corruptionAction) { string tempIndexPath = indexPath + ".lock"; diff --git a/GVFS/GVFS.UnitTests/CommandLine/GitConfigRepairJobTests.cs b/GVFS/GVFS.UnitTests/CommandLine/GitConfigRepairJobTests.cs new file mode 100644 index 0000000000..5fc1d42f0d --- /dev/null +++ b/GVFS/GVFS.UnitTests/CommandLine/GitConfigRepairJobTests.cs @@ -0,0 +1,88 @@ +using GVFS.Common; +using GVFS.RepairJobs; +using GVFS.Tests.Should; +using GVFS.UnitTests.Mock.Common; +using NUnit.Framework; +using System; +using System.IO; + +namespace GVFS.UnitTests.CommandLine +{ + [TestFixture] + public class GitConfigRepairJobTests + { + private string testEnlistmentRoot; + private string dotGitRoot; + + [SetUp] + public void CreateTestEnlistmentRoot() + { + this.testEnlistmentRoot = Path.Combine(Path.GetTempPath(), "GVFSUnitTests_GitConfigRepairJob_" + Guid.NewGuid().ToString("N")); + + // Construct a real (non-mock) GVFSEnlistment pointed at a scratch directory purely + // to get DotGitRoot's platform-specific layout right - PackedRefsLookSha256 only + // does real file I/O under that path, so there is no need to mock GitProcess or + // register GVFSPlatform for these tests specifically (though the [SetUpFixture] in + // GVFS.UnitTests.Setup must still have run, as it does for the whole assembly). + GVFSEnlistment enlistment = new GVFSEnlistment(this.testEnlistmentRoot, "mock://repoUrl", "mock:\\git.exe", authentication: null); + this.dotGitRoot = enlistment.DotGitRoot; + Directory.CreateDirectory(this.dotGitRoot); + } + + [TearDown] + public void DeleteTestEnlistmentRoot() + { + if (Directory.Exists(this.testEnlistmentRoot)) + { + Directory.Delete(this.testEnlistmentRoot, recursive: true); + } + } + + [TestCase] + public void PackedRefsLookSha256IsTrueForSha256WidthObjectId() + { + this.WritePackedRefs("bb3fd16685b64a36d8d433fd0684d192277bb3b64f2b8441c5832d8d3ba8da91 refs/heads/master"); + + this.CreateJob().PackedRefsLookSha256().ShouldEqual(true); + } + + [TestCase] + public void PackedRefsLookSha256IsFalseForSha1WidthObjectId() + { + this.WritePackedRefs("a3541ccec5694b0334d91298e348dc7401c07ff7 refs/heads/master"); + + this.CreateJob().PackedRefsLookSha256().ShouldEqual(false); + } + + [TestCase] + public void PackedRefsLookSha256SkipsHeaderCommentAndPeeledLines() + { + // Matches the real shape 'git pack-refs'/GitRefs.ToPackedRefs produce: a leading + // '#' comment, and '^'-prefixed peeled object ids for annotated tags that must not + // be mistaken for the preceding ref's own object id. + this.WritePackedRefs( + "# pack-refs with: peeled fully-peeled", + "bb3fd16685b64a36d8d433fd0684d192277bb3b64f2b8441c5832d8d3ba8da91 refs/tags/v1", + "^0000000000000000000000000000000000000000000000000000000000000000"); + + this.CreateJob().PackedRefsLookSha256().ShouldEqual(true); + } + + [TestCase] + public void PackedRefsLookSha256IsFalseWhenFileIsMissing() + { + this.CreateJob().PackedRefsLookSha256().ShouldEqual(false); + } + + private GitConfigRepairJob CreateJob() + { + GVFSEnlistment enlistment = new GVFSEnlistment(this.testEnlistmentRoot, "mock://repoUrl", "mock:\\git.exe", authentication: null); + return new GitConfigRepairJob(new MockTracer(), new StringWriter(), enlistment); + } + + private void WritePackedRefs(params string[] lines) + { + File.WriteAllLines(Path.Combine(this.dotGitRoot, GVFSConstants.DotGit.PackedRefsName), lines); + } + } +} diff --git a/GVFS/GVFS/CommandLine/CloneVerb.cs b/GVFS/GVFS/CommandLine/CloneVerb.cs index c672f62811..00dca57d66 100644 --- a/GVFS/GVFS/CommandLine/CloneVerb.cs +++ b/GVFS/GVFS/CommandLine/CloneVerb.cs @@ -865,10 +865,10 @@ private Result TryInitRepo(ITracer tracer, GitRefs refs, Enlistment enlistmentTo } // 'git init' above pins --ref-format=files when the installed git supports it (git - // 2.45+), so a user's init.defaultRefFormat=reftable no longer produces a reftable - // repo on a supported git. This check remains as defense-in-depth: it covers the - // case where the git version could not be determined (the pin is then omitted) and - // any repo that reached v1 by another route. VFS for Git populates refs by writing + // 2.45+), so init.defaultRefFormat=reftable cannot produce a reftable repo on a + // supported git. This check remains as defense-in-depth: it covers the case where + // the git version could not be determined (the pin is then omitted) and any repo + // that reached v1 by another route. VFS for Git populates refs by writing // .git/packed-refs directly (below, and via GitRefs.ToPackedRefs) rather than // through git's ref backend, and a reftable repo ignores packed-refs - so such a // clone would silently produce an enlistment whose remote refs are invisible to @@ -876,7 +876,8 @@ private Result TryInitRepo(ITracer tracer, GitRefs refs, Enlistment enlistmentTo // fatal here: this is our own just-initialized repo, so a read error (as opposed to // the key being absent, which reports files-format) means its config is unreadable // and the clone cannot safely continue. - if (!RefStorage.TryIsReftableRepo(enlistmentToInit.CreateGitProcess(), out bool isReftableRepo, out string refStorageReadError)) + GitProcess gitForFormatChecks = enlistmentToInit.CreateGitProcess(); + if (!RefStorage.TryIsReftableRepo(gitForFormatChecks, out bool isReftableRepo, out string refStorageReadError)) { string readError = "Could not determine the new repository's ref storage format: " + refStorageReadError; tracer.RelatedError(readError); @@ -888,15 +889,14 @@ private Result TryInitRepo(ITracer tracer, GitRefs refs, Enlistment enlistmentTo return new Result(RefStorage.UnsupportedReftableErrorMessage); } - // 'git init' above pins --object-format=sha1, so a user's - // init.defaultObjectFormat=sha256 no longer produces a SHA256 repo on a - // supported git. This check remains as defense-in-depth: it covers any - // pre-existing repo that reached v1 by another route (e.g. an older GVFS - // build, or hand-edited config). A genuine config-read failure is fatal - // here: this is our own just-initialized repo, so a read error (as opposed - // to the key being absent, which reports SHA1) means its config is - // unreadable and the clone cannot safely continue. - if (!ObjectFormat.TryIsSha256Repo(enlistmentToInit.CreateGitProcess(), out bool isSha256Repo, out string objectFormatReadError)) + // 'git init' above pins --object-format=sha1, so init.defaultObjectFormat=sha256 + // cannot produce a SHA256 repo on a supported git. This check remains as + // defense-in-depth: it covers any pre-existing repo that reached v1 by another + // route (e.g. an older GVFS build, or hand-edited config). A genuine + // config-read failure is fatal here: this is our own just-initialized repo, so + // a read error (as opposed to the key being absent, which reports SHA1) means + // its config is unreadable and the clone cannot safely continue. + if (!ObjectFormat.TryIsSha256Repo(gitForFormatChecks, out bool isSha256Repo, out string objectFormatReadError)) { string readError = "Could not determine the new repository's object format: " + objectFormatReadError; tracer.RelatedError(readError); diff --git a/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs b/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs index 2d10a39339..1266891a2a 100644 --- a/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs +++ b/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs @@ -40,14 +40,14 @@ public override IssueType HasIssue(List messages) // A SHA256 repository is not something 'gvfs repair' can make usable (see // TryFixIssues), so report it as an unfixable issue rather than letting - // 'gvfs diagnose'/'gvfs repair' declare the enlistment healthy. Unlike ref - // storage format, there is no cheap, reliable physical fallback here - a - // maintained GVFS enlistment (gc.auto=0, its own packing maintenance) has - // no loose objects left to probe for a longer SHA256 path, so - // extensions.objectformat remains the only practical signal - a config-read - // failure here is not itself flagged; it falls through to the normal config - // checks below, which is exactly what repair is meant to diagnose. - if (ObjectFormat.IsSha256Repo(git)) + // 'gvfs diagnose'/'gvfs repair' declare the enlistment healthy. Detect it both + // from the config extension and from the physical .git/packed-refs object-id + // width: the latter still identifies a SHA256 repo when the config is too + // corrupt for 'git config' to read, which is exactly the state repair runs in. + // A config-read failure alone is otherwise swallowed as "not SHA256" so the + // normal config checks below still run - repair exists to diagnose exactly + // that kind of broken config. + if (ObjectFormat.IsSha256Repo(git) || this.PackedRefsLookSha256()) { messages.Add(ObjectFormat.UnsupportedSha256ErrorMessage); return IssueType.CantFix; @@ -144,30 +144,31 @@ public override FixResult TryFixIssues(List messages) // SHA256 repo usable by VFS for Git, so fail clearly instead of attempting // the rebuild. // - // Unlike ref storage format, SHA256 objects/index are not cheap to probe for - // independently of git config: a freshly-initialized repo's loose objects do - // have a detectably longer path (64 hex chars vs. 40) and the index format - // differs, but a GVFS enlistment runs with gc.auto=0 and its own maintenance - // jobs that pack loose objects away (see PackfileMaintenanceStep) - so any - // real, maintained enlistment has no loose objects left to probe, making a - // reftable-style physical-directory fallback unreliable in the case that - // matters most. extensions.objectformat remains the only practical signal, - // so a config-read failure here is NOT blocked, unlike at mount/clone/ - // FastFetch: repair's whole purpose is to rebuild a corrupt config, so a - // repo whose config is merely unreadable is exactly what repair must be - // allowed to fix. This is an accepted limitation: a SHA256 repo whose - // config is ALSO corrupt cannot be distinguished from an ordinary corrupt - // SHA1 repo here, and would be silently rebuilt as SHA1. - if (!ObjectFormat.TryIsSha256Repo(new GitProcess(this.Enlistment), out bool isSha256Repo, out string objectFormatReadError)) - { - this.Tracer.RelatedWarning("Could not determine the repository's object format; proceeding with repair: " + objectFormatReadError); - } - else if (isSha256Repo) + // Detect SHA256 from the config AND from the physical .git/packed-refs + // object-id width (PackedRefsLookSha256 - see ObjectFormat.Sha256HexIdLength). + // Loose objects are not a usable physical signal here: a GVFS enlistment runs + // with gc.auto=0 and its own maintenance jobs that pack loose objects away (see + // PackfileMaintenanceStep), so a maintained enlistment has none left to probe. + // packed-refs is not touched by that maintenance and VFS for Git writes it + // directly on every clone, so it is reliably present - this is what makes the + // fallback safe when the config is unreadable, mirroring the reftable directory + // check above. A config-read failure with no SHA256-width packed-refs is NOT + // blocked, unlike at mount/clone/FastFetch: repair's whole purpose is to rebuild + // a corrupt config, so a repo whose config is merely unreadable is exactly what + // repair must be allowed to fix. + bool packedRefsLookSha256 = this.PackedRefsLookSha256(); + bool objectFormatReadable = ObjectFormat.TryIsSha256Repo(new GitProcess(this.Enlistment), out bool isSha256ByConfig, out string objectFormatReadError); + if (packedRefsLookSha256 || (objectFormatReadable && isSha256ByConfig)) { messages.Add(ObjectFormat.UnsupportedSha256ErrorMessage); return FixResult.Failure; } + if (!objectFormatReadable) + { + this.Tracer.RelatedWarning("Could not determine the repository's object format; proceeding with repair: " + objectFormatReadError); + } + string configPath = Path.Combine(this.Enlistment.WorkingDirectoryBackingRoot, GVFSConstants.DotGit.Config); string configBackupPath; if (!this.TryRenameToBackupFile(configPath, out configBackupPath, messages)) @@ -226,5 +227,47 @@ private bool ReftableBackendDirectoryExists() return Directory.Exists(reftableDirectory); } + + internal bool PackedRefsLookSha256() + { + // Same DotGitRoot assumption as ReftableBackendDirectoryExists() above - holds for + // the primary enlistment repair targets, not a linked worktree. + // + // VFS for Git writes .git/packed-refs directly during clone (GitRefs.ToPackedRefs, + // consumed by CloneVerb.TryInitRepo) rather than through git's ref backend, so it + // is reliably present. Object ids are written at full hex width - 64 characters for + // SHA256, 40 for SHA1 - and unlike loose objects, packed-refs is never touched by + // git's own packing maintenance, so this signal survives on a long-lived, + // maintained enlistment where a loose-object probe would not. + string packedRefsPath = Path.Combine(this.Enlistment.DotGitRoot, GVFSConstants.DotGit.PackedRefsName); + + try + { + foreach (string line in File.ReadLines(packedRefsPath)) + { + if (line.Length == 0 || line[0] == '#' || line[0] == '^') + { + // '#' is the pack-refs header comment; '^' lines carry the peeled + // object id of an annotated tag, not a ref's own object id. + continue; + } + + int spaceIndex = line.IndexOf(' '); + if (spaceIndex <= 0) + { + continue; + } + + return spaceIndex == ObjectFormat.Sha256HexIdLength; + } + } + catch (IOException) + { + // packed-refs missing or unreadable - not itself a SHA256 signal, and the + // config-based check this is paired with already handles a read failure. + } + + return false; + } } } From b2612971a4b98a7da1a40b327a3cf48f0ca971ba Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 13 Oct 2026 13:02:40 -0700 Subject: [PATCH 10/11] Address third review round from Nathan on PR 2116 Three findings, all comment-accuracy/robustness, not behavior changes to the detection outcome itself: - GitConfigRepairJob.TryFixIssues's SHA256 guard is unreachable through 'gvfs repair' in practice: HasIssue runs the identical detection first and returns CantFix for the same condition, so RepairVerb.Execute never adds the job to its "fixable" set and never calls TryFixIssues for it. No clean unit-test seam exists to pin this directly (GitConfigRepairJob always constructs its own GitProcess rather than accepting an injectable one, and the functional test drives the HasIssue/CantFix path, not this one) - documented it explicitly as deliberate defense-in-depth instead, and softened RepairTests.cs's comment so it no longer implies it exercises this specific guard. - ObjectFormat.Sha256HexIdLength's doc comment, and the same claim repeated in GitConfigRepairJob.cs (TryFixIssues and PackedRefsLookSha256), asserted packed-refs is "never touched by git's own packing maintenance." That is wrong - 'git pack-refs', 'git gc', and 'git maintenance' all rewrite it. The actual, correct invariant (object ids are always serialized at full hex width in packed-refs regardless of who last wrote the file) still makes the width check sound; reworded all three sites to state that instead, with GitConfigRepairJob now pointing at ObjectFormat's doc as the single canonical explanation rather than repeating it. - GitConfigRepairJob.PackedRefsLookSha256's catch (IOException) does not cover UnauthorizedAccessException, which File.ReadLines can throw on a permission or locking problem; that would have escaped repair's HasIssue (RepairVerb.Execute has no try/catch around it) and taken down the verb instead of reporting the issue. Added the missing catch. Assisted-by: Claude Sonnet 5 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/ObjectFormat.cs | 16 ++++----- .../EnlistmentPerTestCase/RepairTests.cs | 10 ++++-- GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs | 35 ++++++++++++------- 3 files changed, 38 insertions(+), 23 deletions(-) diff --git a/GVFS/GVFS.Common/Git/ObjectFormat.cs b/GVFS/GVFS.Common/Git/ObjectFormat.cs index 6a9e6f821e..fb8e50c5f3 100644 --- a/GVFS/GVFS.Common/Git/ObjectFormat.cs +++ b/GVFS/GVFS.Common/Git/ObjectFormat.cs @@ -21,14 +21,14 @@ public static class ObjectFormat /// /// Length, in hex characters, of a SHA256 object id (32 bytes) - twice - /// GVFSConstants.ShaStringLength, the equivalent SHA1 length. Object ids are - /// written at full hex width wherever git records them as text - notably - /// .git/packed-refs ("<objectid> <refname>") and loose ref files - so this - /// is a physical, on-disk signal of a repository's object format that survives - /// even when git config is unreadable. Unlike loose objects, packed-refs is not - /// touched by git's own packing maintenance, and VFS for Git writes it directly - /// on every clone (see GitRefs.ToPackedRefs, CloneVerb.TryInitRepo), so it is - /// reliably present to probe. + /// GVFSConstants.ShaStringLength, the equivalent SHA1 length. Whatever writes + /// .git/packed-refs ("<objectid> <refname>") - VFS for Git's own clone + /// (GitRefs.ToPackedRefs, CloneVerb.TryInitRepo), or git itself via 'git pack-refs', + /// 'git gc', or a 'git maintenance' task - always serializes object ids at this full + /// hex width for the repository's actual object format. So a width check on + /// packed-refs is a reliable physical, on-disk signal of a repository's object + /// format that survives even when git config is unreadable, regardless of who most + /// recently wrote the file. /// public const int Sha256HexIdLength = 64; diff --git a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/RepairTests.cs b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/RepairTests.cs index 70d3746210..14f0fafeca 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/RepairTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/RepairTests.cs @@ -181,9 +181,13 @@ public void DoesNotRebuildSha256RepoConfig() this.RepairWithoutConfirmShouldNotFix(); - // Unlike FixesCorruptGitConfig above, repair must refuse here rather than fix: - // rebuilding the config from scratch would silently erase extensions.objectformat - // and leave a SHA1-shaped config over objects/refs that are declared SHA256. + // This drives HasIssue's CantFix path (same detection as above), so repair + // reports the job as unfixable and never calls TryFixIssues for it at all - + // it does not exercise TryFixIssues's own, separate SHA256 guard, which + // RepairVerb never reaches this job for in practice. What this test does pin is + // the end-to-end, user-visible outcome: repair must leave this repo exactly as + // broken (still SHA256, still unmountable) as it found it, not silently rebuild + // it as a SHA1-shaped config over SHA256-hashed objects/refs. this.Enlistment.Repair(confirm: true); this.Enlistment.TryMountGVFS().ShouldEqual(false, "Repair must not make a SHA256 repo mountable"); diff --git a/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs b/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs index 1266891a2a..b07ad27050 100644 --- a/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs +++ b/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs @@ -2,6 +2,7 @@ using GVFS.Common; using GVFS.Common.Git; using GVFS.Common.Tracing; +using System; using System.Collections.Generic; using System.IO; @@ -145,17 +146,22 @@ public override FixResult TryFixIssues(List messages) // the rebuild. // // Detect SHA256 from the config AND from the physical .git/packed-refs - // object-id width (PackedRefsLookSha256 - see ObjectFormat.Sha256HexIdLength). - // Loose objects are not a usable physical signal here: a GVFS enlistment runs - // with gc.auto=0 and its own maintenance jobs that pack loose objects away (see - // PackfileMaintenanceStep), so a maintained enlistment has none left to probe. - // packed-refs is not touched by that maintenance and VFS for Git writes it - // directly on every clone, so it is reliably present - this is what makes the - // fallback safe when the config is unreadable, mirroring the reftable directory - // check above. A config-read failure with no SHA256-width packed-refs is NOT + // object-id width (PackedRefsLookSha256 - see ObjectFormat.Sha256HexIdLength for + // why that width is a reliable physical signal). This mirrors the reftable + // directory check above, and is what makes the fallback safe when the config is + // unreadable. A config-read failure with no SHA256-width packed-refs is NOT // blocked, unlike at mount/clone/FastFetch: repair's whole purpose is to rebuild // a corrupt config, so a repo whose config is merely unreadable is exactly what // repair must be allowed to fix. + // + // In practice this whole block is unreachable through 'gvfs repair': HasIssue + // above runs this identical detection first and returns CantFix for the same + // condition, so RepairVerb.Execute never adds this job to its "fixable" set and + // never calls TryFixIssues for it (RepairTests.DoesNotRebuildSha256RepoConfig + // exercises exactly that HasIssue/CantFix path). The check stays here anyway, as + // defense-in-depth should TryFixIssues ever be called directly or RepairVerb's + // sequencing change, so a future caller does not silently rebuild a SHA256 + // repo's config as SHA1. bool packedRefsLookSha256 = this.PackedRefsLookSha256(); bool objectFormatReadable = ObjectFormat.TryIsSha256Repo(new GitProcess(this.Enlistment), out bool isSha256ByConfig, out string objectFormatReadError); if (packedRefsLookSha256 || (objectFormatReadable && isSha256ByConfig)) @@ -235,10 +241,9 @@ internal bool PackedRefsLookSha256() // // VFS for Git writes .git/packed-refs directly during clone (GitRefs.ToPackedRefs, // consumed by CloneVerb.TryInitRepo) rather than through git's ref backend, so it - // is reliably present. Object ids are written at full hex width - 64 characters for - // SHA256, 40 for SHA1 - and unlike loose objects, packed-refs is never touched by - // git's own packing maintenance, so this signal survives on a long-lived, - // maintained enlistment where a loose-object probe would not. + // is reliably present. See ObjectFormat.Sha256HexIdLength for why its object-id + // width is a reliable physical signal of the repository's object format even if + // the file is later rewritten by git itself. string packedRefsPath = Path.Combine(this.Enlistment.DotGitRoot, GVFSConstants.DotGit.PackedRefsName); try @@ -266,6 +271,12 @@ internal bool PackedRefsLookSha256() // packed-refs missing or unreadable - not itself a SHA256 signal, and the // config-based check this is paired with already handles a read failure. } + catch (UnauthorizedAccessException) + { + // Same rationale as the IOException case above - File.ReadLines surfaces a + // permission/locking problem as UnauthorizedAccessException rather than + // IOException, and this probe must not propagate either kind of failure. + } return false; } From 1be6e2b737b9f7d0a43c813a619755115f5fde46 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 15 Oct 2026 14:52:23 -0700 Subject: [PATCH 11/11] Address fourth review round from Nathan on PR 2116 Nine inline findings plus two PR-description items from the re-review: - Real bug (Medium): GitConfigRepairJob.PackedRefsLookSha256 returned a verdict based on the first line containing a space, without checking that the leading token was actually hex or the right width. A garbled line at ~40 chars could mask a genuine 64-char SHA256 signal further down the file (false negative -> data loss on repair), and a junk line with a space at index 64 could permanently misclassify a healthy SHA1 repo (false positive). Since this code only ever runs against an enlistment already flagged as broken, malformed input is the expected case. Now validates each candidate token as hex of a known object-id width (40 or 64) before trusting it, and keeps scanning past a line that fails that check instead of committing to it. Added unit tests for both failure modes. - GVFSEnlistment.WaitUntilMounted's catch (JsonException) still built its console-facing message from the raw exception, so a malformed mount response dumped a full stack trace - the BrokenPipeException fix nearby only covered one of the two paths. Gave it the same e.Message treatment and added a test exercising it via a real named pipe. - InProcessMount's mount-time config read for SHA256/reftable now retries a few times, a short delay apart, before failing - so a purely transient failure (e.g. antivirus briefly holding .git/config) does not fail mount outright. A failure that persists through the retries is still fatal, since mount would hit the same unreadable config again moments later anyway. - Consolidated a near-duplicate "format argument" comment in InProcessMount.cs into one copy plus a pointer. - Hoisted GitConfigRepairJob.TryFixIssues's two separate `new GitProcess(this.Enlistment)` constructions into one shared local, matching HasIssue and the earlier FastFetchVerb/CloneVerb fixes. - Strengthened RepairTests.DoesNotRebuildSha256RepoConfig's final assertion from ShouldContain("sha256") to an exact match against the config as written, so a half-rebuilt config (repositoryformatversion reset to 0, objectformat line surviving) would also fail the test. - Moved GitConfigRepairJobTests from GVFS.UnitTests.CommandLine to GVFS.UnitTests.RepairJobs, matching the production GVFS.RepairJobs layout - these are the first tests in that namespace, so this sets the convention other RepairJobs tests should follow. - Fixed GitConfigRepairJob.HasIssue's comment (and the matching PR description text) claiming this check also prevents 'gvfs diagnose' from reporting the enlistment healthy - diagnose does not run repair jobs at all, only RepairVerb does. - PR description: fixed the "SHA256/reftable version-gated git init pins" claim - only --ref-format=files is version-gated; the --object-format=sha1 pin is unconditional, and both predate this PR. - PR description: replaced the superseded "packed-refs is never touched by maintenance" claim (already corrected in code comments last round) with the actual invariant. - Allow-list vs deny-list detection (ObjectFormat.IsSha256) and the mount-time escape-hatch question (InProcessMount) were raised as deliberate design decisions rather than defects - left both as-is, matching the existing merged RefStorage precedent in both cases. Assisted-by: Claude Sonnet 5 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/GVFSEnlistment.cs | 9 ++- .../EnlistmentPerTestCase/RepairTests.cs | 6 +- GVFS/GVFS.Mount/InProcessMount.cs | 67 ++++++++++++++----- .../WaitUntilMountedProcessTrackingTests.cs | 44 ++++++++++++ .../GitConfigRepairJobTests.cs | 30 ++++++++- GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs | 54 ++++++++++++--- 6 files changed, 181 insertions(+), 29 deletions(-) rename GVFS/GVFS.UnitTests/{CommandLine => RepairJobs}/GitConfigRepairJobTests.cs (68%) diff --git a/GVFS/GVFS.Common/GVFSEnlistment.cs b/GVFS/GVFS.Common/GVFSEnlistment.cs index c65fff0ad7..7d040291b5 100644 --- a/GVFS/GVFS.Common/GVFSEnlistment.cs +++ b/GVFS/GVFS.Common/GVFSEnlistment.cs @@ -328,8 +328,13 @@ public static bool WaitUntilMounted( } catch (JsonException e) { - errorMessage = string.Format("Failed to parse response from GVFS.Mount.\n {0}", e); - tracer.RelatedError($"{nameof(WaitUntilMounted)}: {errorMessage}"); + // Same rationale as the BrokenPipeException case above: keep the + // console-facing message short, and send the full exception detail + // to the trace log instead of dumping a raw stack trace to the user. + errorMessage = string.Format("Failed to parse response from GVFS.Mount: {0}", e.Message); + tracer.RelatedError( + new EventMetadata { { "Exception", e.ToString() } }, + $"{nameof(WaitUntilMounted)}: {errorMessage}"); return false; } } diff --git a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/RepairTests.cs b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/RepairTests.cs index 14f0fafeca..0cfd978e74 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/RepairTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/RepairTests.cs @@ -191,8 +191,12 @@ public void DoesNotRebuildSha256RepoConfig() this.Enlistment.Repair(confirm: true); this.Enlistment.TryMountGVFS().ShouldEqual(false, "Repair must not make a SHA256 repo mountable"); + // Assert the config is byte-for-byte what was written, not merely that it still + // mentions "sha256" - a weaker check would pass even on a half-rebuilt config + // where repositoryformatversion was reset to 0 but extensions.objectformat + // survived, which is exactly the broken state this guard exists to prevent. string configAfterRepair = File.ReadAllText(configPath); - configAfterRepair.ShouldContain("sha256"); + configAfterRepair.ShouldEqual(sha256Config, "Repair must not modify a SHA256 repo's config at all"); } private void CreateCorruptIndexAndRename(string indexPath, Action corruptionAction) diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index 34c672fca3..cf9276b05f 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -291,8 +291,13 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords) // regardless. A genuine config-read failure is fatal here rather than a // warning: a missing key reports the repo as files-format and does not reach // this branch, so a read error means the config is anomalous and proceeding - // into the force-write could brick an unsupported repo. - if (!RefStorage.TryIsReftableRepo(git, out bool isReftableRepo, out string refStorageReadError)) + // into the force-write could brick an unsupported repo. Retry a few times + // first (see RetryShortConfigRead) to ride out a purely transient failure - + // e.g. AV momentarily holding .git/config - rather than failing mount on + // something that would have succeeded moments later; a failure that + // persists through the retries would fail mount later anyway once + // TrySetRequiredGitConfigSettings itself tries to read/write that config. + if (!RetryShortConfigRead(() => RefStorage.TryIsReftableRepo(git, out bool v, out string e) ? (true, v, e) : (false, v, e), out bool isReftableRepo, out string refStorageReadError)) { // Pass the git error as a format argument, not concatenated into the // format string: FailMountAndExit routes through ITracer.RelatedError( @@ -306,20 +311,19 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords) this.FailMountAndExit(RefStorage.UnsupportedReftableErrorMessage); } - // Same rationale as above, for SHA256: applying the required-config - // force-write to a SHA256 repo (extensions.objectformat) would brick it - // identically, so this check must also run before that write. A genuine - // config-read failure is fatal here rather than a warning: a missing key - // reports the repo as SHA1 and does not reach this branch, so a read error - // means the config is anomalous and proceeding into the force-write could - // brick an unsupported repo. - if (!ObjectFormat.TryIsSha256Repo(git, out bool isSha256Repo, out string objectFormatReadError)) + // Same rationale and retry policy as above, for SHA256: applying the + // required-config force-write to a SHA256 repo (extensions.objectformat) + // would brick it identically, so this check must also run before that + // write. A genuine config-read failure is fatal here rather than a + // warning: a missing key reports the repo as SHA1 and does not reach this + // branch, so a read error means the config is anomalous and proceeding + // into the force-write could brick an unsupported repo. + if (!RetryShortConfigRead(() => ObjectFormat.TryIsSha256Repo(git, out bool v, out string e) ? (true, v, e) : (false, v, e), out bool isSha256Repo, out string objectFormatReadError)) { - // Pass the git error as a format argument, not concatenated into the - // format string: FailMountAndExit routes through ITracer.RelatedError( - // string, params object[]), which runs string.Format, so a '{' in the - // git stderr (possible with a crafted/corrupt .git/config) would throw - // a FormatException instead of failing the mount cleanly. + // See the matching comment on the ref storage format check above: + // FailMountAndExit requires the git error as a format argument rather + // than concatenated into the format string, to avoid a FormatException + // on a stray '{' in crafted/corrupt git stderr. this.FailMountAndExit("Could not determine the repository's object format: {0}", objectFormatReadError); } else if (isSha256Repo) @@ -688,6 +692,39 @@ private void FailMountAndExit(string error, params object[] args) this.FailMountAndExit(ReturnCode.GenericError, error, args); } + /// + /// Retries a 'git config' read a few times, a short delay apart, before giving up - + /// so a purely transient failure (for example AV momentarily holding .git/config) + /// does not fail mount outright. A failure that persists through the retries is + /// still fatal to the caller: mount would reach the same unreadable config again + /// moments later via TrySetRequiredGitConfigSettings, so there is nothing to gain by + /// retrying longer than this. + /// + private static bool RetryShortConfigRead(Func<(bool success, bool value, string error)> tryRead, out bool value, out string error) + { + const int maxAttempts = 3; + const int delayMs = 100; + + (bool success, bool value, string error) result = (false, false, null); + for (int attempt = 1; attempt <= maxAttempts; attempt++) + { + result = tryRead(); + if (result.success) + { + break; + } + + if (attempt < maxAttempts) + { + Thread.Sleep(delayMs); + } + } + + value = result.value; + error = result.error; + return result.success; + } + private void FailMountAndExit(ReturnCode returnCode, string error, params object[] args) { this.currentState = MountState.MountFailed; diff --git a/GVFS/GVFS.UnitTests/Common/WaitUntilMountedProcessTrackingTests.cs b/GVFS/GVFS.UnitTests/Common/WaitUntilMountedProcessTrackingTests.cs index 562b5e12d4..64ae80bd8a 100644 --- a/GVFS/GVFS.UnitTests/Common/WaitUntilMountedProcessTrackingTests.cs +++ b/GVFS/GVFS.UnitTests/Common/WaitUntilMountedProcessTrackingTests.cs @@ -132,5 +132,49 @@ public void BrokenPipeMidPollProducesShortMessageAndLogsFullExceptionDetail() tracer.RelatedErrorEvents[0].ShouldContain("Exception"); } } + + [TestCase] + public void JsonExceptionMidPollProducesShortMessageAndLogsFullExceptionDetail() + { + string pipeName = "GVFS_UnitTest_JsonException_" + Guid.NewGuid().ToString("N"); + + using (NamedPipeServerStream serverStream = new NamedPipeServerStream( + pipeName, PipeDirection.InOut, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous)) + { + Task serverTask = Task.Run(() => + { + serverStream.WaitForConnection(); + + // Read the client's GetStatus request, then respond with a payload that + // is not valid JSON - simulating a truncated or malformed mount response - + // so Response.FromJson throws a JsonException. + new NamedPipeStreamReader(serverStream).ReadMessage(); + new NamedPipeStreamWriter(serverStream).WriteMessage("{ not valid json"); + }); + + MockTracer tracer = new MockTracer(); + string errorMessage; + bool result = GVFSEnlistment.WaitUntilMounted( + tracer, + pipeName, + "C:\\fake\\root", + false, + out errorMessage); + + serverTask.Wait(TimeSpan.FromSeconds(10)); + + result.ShouldBeFalse(); + errorMessage.ShouldNotBeNull(); + errorMessage.ShouldContain("Failed to parse response from GVFS.Mount"); + + // The console-facing message must stay short - no embedded stack trace + // (a stack trace is what JsonException.ToString() would produce). + errorMessage.ShouldNotContain(false, " at ", Environment.NewLine); + + // The full exception detail must still be captured for diagnostics. + tracer.RelatedErrorEvents.Count.ShouldEqual(1); + tracer.RelatedErrorEvents[0].ShouldContain("Exception"); + } + } } } diff --git a/GVFS/GVFS.UnitTests/CommandLine/GitConfigRepairJobTests.cs b/GVFS/GVFS.UnitTests/RepairJobs/GitConfigRepairJobTests.cs similarity index 68% rename from GVFS/GVFS.UnitTests/CommandLine/GitConfigRepairJobTests.cs rename to GVFS/GVFS.UnitTests/RepairJobs/GitConfigRepairJobTests.cs index 5fc1d42f0d..d22f207a91 100644 --- a/GVFS/GVFS.UnitTests/CommandLine/GitConfigRepairJobTests.cs +++ b/GVFS/GVFS.UnitTests/RepairJobs/GitConfigRepairJobTests.cs @@ -6,7 +6,7 @@ using System; using System.IO; -namespace GVFS.UnitTests.CommandLine +namespace GVFS.UnitTests.RepairJobs { [TestFixture] public class GitConfigRepairJobTests @@ -74,6 +74,34 @@ public void PackedRefsLookSha256IsFalseWhenFileIsMissing() this.CreateJob().PackedRefsLookSha256().ShouldEqual(false); } + [TestCase] + public void PackedRefsLookSha256SkipsNonHexGarbageAtSha1WidthAndFindsRealSignal() + { + // A 40-character garbled first token (not valid hex) must not be trusted as a + // definitive SHA1 answer - that would mask the genuine SHA256-width signal on + // the next line, which is the false-negative failure mode on exactly the + // corrupt enlistments this probe runs against. + this.WritePackedRefs( + "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz refs/heads/garbled", + "bb3fd16685b64a36d8d433fd0684d192277bb3b64f2b8441c5832d8d3ba8da91 refs/heads/master"); + + this.CreateJob().PackedRefsLookSha256().ShouldEqual(true); + } + + [TestCase] + public void PackedRefsLookSha256SkipsNonHexGarbageAtSha256WidthOnHealthySha1Repo() + { + // A 64-character garbled token (not valid hex) must not pin this to "SHA256" - + // that would permanently misclassify a healthy SHA1 repo, the false-positive + // failure mode. The genuine SHA1-width signal on the next line is the real + // answer. + this.WritePackedRefs( + "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz refs/heads/garbled", + "a3541ccec5694b0334d91298e348dc7401c07ff7 refs/heads/master"); + + this.CreateJob().PackedRefsLookSha256().ShouldEqual(false); + } + private GitConfigRepairJob CreateJob() { GVFSEnlistment enlistment = new GVFSEnlistment(this.testEnlistmentRoot, "mock://repoUrl", "mock:\\git.exe", authentication: null); diff --git a/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs b/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs index b07ad27050..d8c3646a5d 100644 --- a/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs +++ b/GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs @@ -41,13 +41,12 @@ public override IssueType HasIssue(List messages) // A SHA256 repository is not something 'gvfs repair' can make usable (see // TryFixIssues), so report it as an unfixable issue rather than letting - // 'gvfs diagnose'/'gvfs repair' declare the enlistment healthy. Detect it both - // from the config extension and from the physical .git/packed-refs object-id - // width: the latter still identifies a SHA256 repo when the config is too - // corrupt for 'git config' to read, which is exactly the state repair runs in. - // A config-read failure alone is otherwise swallowed as "not SHA256" so the - // normal config checks below still run - repair exists to diagnose exactly - // that kind of broken config. + // 'gvfs repair' declare the enlistment healthy. Detect it both from the config + // extension and from the physical .git/packed-refs object-id width: the latter + // still identifies a SHA256 repo when the config is too corrupt for 'git config' + // to read, which is exactly the state repair runs in. A config-read failure + // alone is otherwise swallowed as "not SHA256" so the normal config checks below + // still run - repair exists to diagnose exactly that kind of broken config. if (ObjectFormat.IsSha256Repo(git) || this.PackedRefsLookSha256()) { messages.Add(ObjectFormat.UnsupportedSha256ErrorMessage); @@ -106,6 +105,8 @@ public override IssueType HasIssue(List messages) public override FixResult TryFixIssues(List messages) { + GitProcess git = new GitProcess(this.Enlistment); + // Check before touching the config file at all: TryFixIssues rebuilds the // entire config from scratch (wiping it to empty, then writing only the // required/optional settings this codebase knows about via @@ -125,7 +126,7 @@ public override FixResult TryFixIssues(List messages) // files-format config, so a repo whose config is merely unreadable is exactly what // repair must be allowed to fix. bool reftableDirectoryExists = this.ReftableBackendDirectoryExists(); - bool configReadable = RefStorage.TryIsReftableRepo(new GitProcess(this.Enlistment), out bool isReftableByConfig, out string refStorageReadError); + bool configReadable = RefStorage.TryIsReftableRepo(git, out bool isReftableByConfig, out string refStorageReadError); if (reftableDirectoryExists || (configReadable && isReftableByConfig)) { messages.Add(RefStorage.UnsupportedReftableErrorMessage); @@ -163,7 +164,7 @@ public override FixResult TryFixIssues(List messages) // sequencing change, so a future caller does not silently rebuild a SHA256 // repo's config as SHA1. bool packedRefsLookSha256 = this.PackedRefsLookSha256(); - bool objectFormatReadable = ObjectFormat.TryIsSha256Repo(new GitProcess(this.Enlistment), out bool isSha256ByConfig, out string objectFormatReadError); + bool objectFormatReadable = ObjectFormat.TryIsSha256Repo(git, out bool isSha256ByConfig, out string objectFormatReadError); if (packedRefsLookSha256 || (objectFormatReadable && isSha256ByConfig)) { messages.Add(ObjectFormat.UnsupportedSha256ErrorMessage); @@ -244,6 +245,14 @@ internal bool PackedRefsLookSha256() // is reliably present. See ObjectFormat.Sha256HexIdLength for why its object-id // width is a reliable physical signal of the repository's object format even if // the file is later rewritten by git itself. + // + // This only ever runs against an enlistment 'gvfs repair' has already flagged as + // broken, so a malformed line is the expected case, not the exotic one: each + // candidate line's leading token must actually be hex and of a known object-id + // width before it is trusted, and a line that fails that check is skipped rather + // than treated as a definitive answer - a non-hex or wrong-width token picked up + // from the first space it happens to contain could otherwise mask a genuine + // SHA256-width line further down the file, or misclassify a healthy SHA1 repo. string packedRefsPath = Path.Combine(this.Enlistment.DotGitRoot, GVFSConstants.DotGit.PackedRefsName); try @@ -263,7 +272,19 @@ internal bool PackedRefsLookSha256() continue; } - return spaceIndex == ObjectFormat.Sha256HexIdLength; + if (spaceIndex == ObjectFormat.Sha256HexIdLength && IsHex(line, 0, spaceIndex)) + { + return true; + } + + if (spaceIndex == GVFSConstants.ShaStringLength && IsHex(line, 0, spaceIndex)) + { + return false; + } + + // Neither a valid SHA256-width nor SHA1-width hex object id - this line + // is not a reliable signal either way, so keep scanning rather than + // committing to it. } } catch (IOException) @@ -280,5 +301,18 @@ internal bool PackedRefsLookSha256() return false; } + + private static bool IsHex(string value, int start, int length) + { + for (int i = start; i < start + length; i++) + { + if (!Uri.IsHexDigit(value[i])) + { + return false; + } + } + + return true; + } } }