Repository navigation
Fail fast on SHA256 repositories in FastFetch, gvfs clone, gvfs mount, and gvfs repair - #2116
Tyrie Vella (tyrielv) wants to merge 11 commits into
Conversation
Nathan Baird (ShiningMassXAcc)
left a comment
There was a problem hiding this comment.
The analysis in the PR description is excellent — in particular documenting that InProcessMount and RepairJobs have no unit-test seam and that manual real-repo verification stands in for that coverage. That is the right disclosure to make rather than quietly leaving the gap.
The most useful framing for this review, though, is that #2121 is the same pattern, by you, on the same four files, at the same insertion points, against the same base — and #2121 is better on three axes. Most of what follows is "bring this in line with your own newer PR."
Eight findings, ranked.
1. High — GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs:85 fails open on the most destructive path
if (ObjectFormat.TryIsSha256Repo(new GitProcess(this.Enlistment), out bool isSha256Repo, out string objectFormatReadError) &&
isSha256Repo)On a config-read failure TryIsSha256Repo returns false, the && collapses, and repair proceeds to wipe and rebuild .git/config — which the comment directly above describes as the hazard being prevented: "would silently erase extensions.objectformat and leave a SHA1-shaped config over live SHA256-hashed objects." objectFormatReadError is captured and never read.
So the one site where guessing wrong is most destructive is the only one of the four that ignores the read error, while FastFetchVerb, InProcessMount and CloneVerb all at least warn.
#2121's GitConfigRepairJob.TryFixIssues gets this right: it separates configReadable from isReftableByConfig, adds the physical .git/reftable/ directory fallback for exactly the "config too corrupt to read" case that repair runs in, and traces the read failure when it proceeds anyway. Please apply the same shape here.
2. Medium — no HasIssue check, so gvfs diagnose reports SHA256 enlistments as healthy
#2121 adds a check to GitConfigRepairJob.HasIssue returning IssueType.CantFix. This PR only guards TryFixIssues. Without the HasIssue half, gvfs diagnose and gvfs repair will declare a SHA256 enlistment healthy and say nothing.
3. Medium — GVFS/GVFS/CommandLine/CloneVerb.cs:860-865 comment is stale before it merges
"'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..."
#2115 ("Pin git init to sha1 during clone") merged earlier today, so this is already wrong. Plan narration in product code goes stale by construction. State the invariant instead — git init pins --object-format=sha1, and this check is defense-in-depth for repos that reached v1 by another route — and leave the sequencing in the PR description where it reads correctly forever.
4. Medium — two more comments that describe the work rather than the code
GitConfigRepairJob.cs:81: "a worse, silent form of the same corruption hazard this PR fixes elsewhere". "This PR" is meaningless to the next person who opens this file.- The new
WaitUntilMountedtest: "which is whate.ToString()(instead ofe.Message) used to produce" — history narration; the test should describe the behavior it asserts now.
5. Medium — GVFS/GVFS.Common/Git/ObjectFormat.cs, IsSha256Repo(GitProcess) is dead
Zero product callers — only ObjectFormatTests uses it. Its own doc comment even points readers away from it: "Use TryIsSha256Repo if the caller wants to distinguish and surface that failure", which every real caller does.
Either drop it, or make it earn its keep in HasIssue per finding 2 — which is exactly what #2121 does with RefStorage.IsReftableRepo.
6. Medium — GVFS/GVFS.Common/GVFSEnlistment.cs, WaitUntilMounted is a separate fix
The BrokenPipeException change from e.ToString() to e.Message is a different bug from "fail fast on SHA256". It changes user-facing text for every broken-pipe case, not just this one, and it overlaps #2105 ("Report the real reason..."). It is disclosed in the PR body so it is clearly deliberate, but it would be cleaner as its own PR, or at minimum cross-referenced on #2105 so the two changes do not fight over the same handler.
7. Medium — opposite read-failure policy from #2121, in the same methods
This PR warns and continues on a config-read failure at FastFetchVerb, InProcessMount and CloneVerb. #2121 makes the equivalent failure fatal at all three. Once both land, two adjacent checks inside MountWithLockAcquired and TryInitRepo will have opposite fail-open/fail-closed semantics, which is going to confuse whoever touches this next.
Worth picking one. #2121's fail-closed looks right to me, and the green functional suite there is good evidence that normal key-absent repos do not trip it.
8. Medium — this will conflict with #2121
Both PRs insert at the same lines of GVFS/FastFetch/FastFetchVerb.cs (~204), GVFS/GVFS.Mount/InProcessMount.cs (~280), GVFS/GVFS/CommandLine/CloneVerb.cs (~857) and GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs, both targeting vnext. Whichever merges second needs a rebase that also reconciles finding 7. Worth noting in both descriptions so it is not a surprise.
One thing I checked and want to explicitly clear: I suspected a FormatException hazard in this.tracer.RelatedWarning("Could not determine the repository's object format: " + objectFormatReadError) if git stderr contained a brace, since JsonTracer.RelatedWarning(string, params object[]) calls string.Format unconditionally. It is fine — ITracer also declares RelatedWarning(string message), and C# overload resolution prefers the non-params overload for a single string argument. No action needed. (#2121's comment on the FailMountAndExit variant is correct, though, because that one only has a params object[] overload.)
Also confirmed: no stray .github / AGENTS.md AI artifacts in the diff.
Pull request was converted to draft
a233c5b to
6641a8b
Compare
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 <tyrielv@gmail.com>
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 <tyrielv@gmail.com>
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 <tyrielv@gmail.com>
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 <tyrielv@gmail.com>
PR microsoft#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 microsoft#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 microsoft#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 microsoft#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 microsoft#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 <tyrielv@gmail.com>
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 <tyrielv@gmail.com>
Nathan's approval comment on microsoft#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 <tyrielv@gmail.com>
6641a8b to
f1a899c
Compare
Self-review (review-swarm iteration 3, run after rebasing onto vnext post microsoft#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 <tyrielv@gmail.com>
|
Note 🤖 Machine-drafted, reviewed and approved by Tyrie Vella (@tyrielv) before posting. Thanks for the deep comparison against #2121 - "bring this in line with your own newer PR" was exactly the right framing, and all eight are addressed. 1. 2. No 3. Stale 4. "This PR"/"used to produce" narration. Fixed both, and grepped the whole diff afterward to confirm neither phrase survives anywhere. 5. Dead 6. 7. Opposite read-failure policy from #2121. Fixed - FastFetch/clone/mount are now fail-closed, matching #2121's policy exactly, for the reason you gave (a missing key already succeeds via the normal path, so the error path only fires on a genuinely anomalous config). 8. Will conflict with #2121. #2121 merged, so I rebased onto One thing that came out of re-running my own review swarm after the rebase, in case it's useful: the Pushed at |
|
Correcting myself here. I opened my last review by praising the disclosure that "neither
this.Enlistment.UnmountGVFS();
File.WriteAllText(Path.Combine(this.Enlistment.RepoBackingRoot, ".git", "config"), "[cor");
this.Enlistment.TryMountGVFS().ShouldEqual(false, "...");
this.RepairWithoutConfirmShouldNotFix();
this.Enlistment.Repair(confirm: true);Writing Two caveats that keep this off the blocking list. #2121 set the same precedent — it added unit tests for So the ask is narrow: please adjust the description so it does not say no seam exists, since one does. Whether to use it is your call, and I would not hold the PR for it. |
|
Title nit: "Fail fast on SHA256 repositories in FastFetch and gvfs clone" now undersells the change. The PR covers four call sites — FastFetch, clone, mount, and repair — and the description itself argues mount is the important one, since that is where Since the title becomes the permanent subject line of the merge commit on |
Nathan Baird (ShiningMassXAcc)
left a comment
There was a problem hiding this comment.
Re-reviewed at 4722f37b. All eight points from my last pass are addressed, and the rebase onto vnext post-#2121 is clean — the merge base is exactly the vnext tip. I built and ran this rather than only reading it: GVFS.UnitTests builds with 0 warnings and 0 errors, and the full suite is 1037 passed / 0 failed / 11 skipped (the skips are the pre-existing PostIndexChangedHookTests that need a full-solution native build). All 13 new ObjectFormatTests and the new BrokenPipeMidPollProducesShortMessageAndLogsFullExceptionDetail pass. CI is green across all 24 functional shards and both architectures.
One trap worth recording for anyone else verifying this: running GVFS.UnitTests.exe --test "GVFS.UnitTests.Git.ObjectFormatTests" reports 7 of 13 failing with a NullReferenceException in GVFSEnlistment..ctor. That is not a defect in this PR — the namespace SetUpFixture that registers GVFSPlatform does not run under a single-class filter, and the already-merged, untouched RefStorageTests reproduces the identical 13/6/7 profile. Run the suite unfiltered.
On my finding 7 specifically, the reconciliation is good. Mount and clone are now fail-closed while repair warns and proceeds, which is the right split — repair is the one verb that must still run on a repo too broken to read. And InProcessMount passing git's stderr as a format argument to FailMountAndExit rather than concatenating it is the correct handling of the params object[] hazard I raised.
Two things to come back on, one of which is my own error:
- My finding 1 is only half-addressed. The config-read-error separation landed, but the physical fallback half — the part that made the #2121 shape work — did not, and the new comment now asserts that no such signal exists. I went and checked, and one does. Details inline on
GitConfigRepairJob.cs. - I was wrong to endorse the "no test seam" disclosure. I opened my last review praising it. A functional seam does exist and is close to a drop-in template. That one is on me rather than on you.
Plus a stale title and three nits, none blocking. One housekeeping note I should have raised last time: there is no linked issue on the PR. Referencing #2119 as the root cause this deliberately does not fix is right, but nothing tracks the fail-fast work itself.
Leaving this as a comment rather than re-approving, purely because of point 1 — repair is still the one path where guessing wrong loses data. Everything else here is optional polish.
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 microsoft#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 <tyrielv@gmail.com>
|
Note 🤖 Machine-drafted, reviewed and approved by Tyrie Vella (@tyrielv) before posting. Thanks for the second pass, Nathan - addressed all four items:
Also updated the PR title to reflect the full scope (mount + repair, not just FastFetch/clone). On the missing tracking issue: leaving that as-is for now - this PR's own description is the record of the fail-fast behavior, and #2119 already covers the shared root cause that would otherwise need its own issue. |
Nathan Baird (ShiningMassXAcc)
left a comment
There was a problem hiding this comment.
Review pass over this PR. 3 comment(s).
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 <tyrielv@gmail.com>
|
Note 🤖 Machine-drafted, reviewed and approved by Tyrie Vella (@tyrielv) before posting. Thanks for catching these, Nathan:
|
Nathan Baird (ShiningMassXAcc)
left a comment
There was a problem hiding this comment.
Re-review across the ten commits since my last pass. All seven of my earlier threads are substantively addressed in code — including the packed-refs physical fallback I asked for — but none are marked resolved, so the PR still reads as unanswered and my review is still sitting at CHANGES_REQUESTED. Resolving them would make the remaining state much easier to see.
Eleven observations below: three worth a decision, eight small. CI is green across every leg.
Context for anyone triaging: FastFetchVerb.cs is non-shipping — FastFetch is a pipeline artifact and is not attached to the public release — so changes there don't reach end users.
Low — PR description
The bullet stating that the git init pins are "version-gated" and added by this PR isn't accurate. GitProcess.Init pins --object-format=sha1 unconditionally; only --ref-format=files is version-gated, and both predate this PR. The code comment gets this right — it's just the description that drifts.
Low — PR description
The description still carries the superseded packed-refs claim — that it is "never touched by a GVFS enlistment's own packing maintenance" — which thread 7 corrected in the code itself. Since the description becomes the squash commit message, it's worth bringing back in line with what shipped.
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 <tyrielv@gmail.com>
|
Note 🤖 Machine-drafted, reviewed and approved by Tyrie Vella (@tyrielv) before posting. Thanks, Nathan - and apologies for leaving all those earlier threads unresolved, that's fixed now (all 16 resolved). Addressed everything from this round:
And the two PR-description items: fixed the inaccurate "version-gated" claim about the |
Problem and Context
Git 3.0 changes
git init's default hash algorithm from SHA1 to SHA256. The GVFS-protocol server (the only known implementer) has no plans to support SHA256, and VFS for Git's own code is hardcoded to SHA1's 20-byte / 40-hex-char object ids throughout (Sha1Id,SHA1Util.IsValidShaFormat,GitIndexGenerator, FastFetch's own index reader/writer). If a SHA256 repository ever reached this code, the result would be data corruption or an unpredictable crash rather than a clear, actionable 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" would mean rewriting every object with a new identity - effectively a fresh clone. The only sound mitigation is detecting a SHA256 repository early and failing fast, rather than attempting an automatic conversion.
gvfs mountwas initially left out of scope, on the assumption that oncegvfs clone'sgit initcall is pinned to--object-format=sha1(a separate change, since merged intovnext), the only enlistmentsgvfs mountwould ever see are onesgvfs cloneitself created. Manually testing that assumption against a hand-crafted SHA256 repository (core.repositoryformatversion=1,extensions.objectformat=sha256) showed the actual failure mode was worse than "an unsupported, self-inflicted state":TrySetRequiredGitConfigSettingsunconditionally force-writescore.repositoryformatversionback to0(to match plaingit init, with no awareness ofextensions.objectformat). Applied to a SHA256 repo, this leavescore.repositoryformatversion=0with 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 corrupted the enlistment's git config in place; evengvfs status/gvfs unmountstopped working until the config was hand-repaired.gvfs mountCLI then surfaced a rawBrokenPipeExceptionwith a full .NET stack trace on the console (the mount process's pipe breaks when it exits), not an actionable message.Given that,
gvfs mountis now covered by this PR too, as a second layer of defense alongside thegit initpin.Note on the shared root cause: the corruption mechanism above (
RequiredGitConfigunconditionally force-writingcore.repositoryformatversionback to0, clobbering whatever a repo-format-v1 extension requires) is not specific to SHA256 - the same force-write bricks areftable-backed repo identically, as documented in #2119. This PR deliberately does not touchRequiredGitConfigitself; it adds a pre-check ahead of each call site that would otherwise reach the corrupting write, scoped to theextensions.objectformat=sha256axis only. The ref-storage/reftable axis is addressed separately in #2121 (merged), which follows the same pattern at the same four call sites. This PR has been rebased ontovnextpost-#2121-merge, so every one of the four shared files (FastFetchVerb.cs,InProcessMount.cs,CloneVerb.cs,GitConfigRepairJob.cs) now carries both the ref-storage and object-format checks back-to-back, with their read-failure policies reconciled to match (fail-closed at FastFetch/clone/mount, warn-and-proceed at repair).Follow-up acknowledged, not implemented here: with both axes' checks now present at the same three strict-fail sites (FastFetch, clone, mount), the duplicated
TryIs*Repo → fail on read error → else fail on unsupported valueshape appears six times instead of the two-to-four it did before #2121 merged. A shared helper would collapse this, deliberately excludingGitConfigRepairJob, whose read-failure and physical-marker policies diverge too much to share. Deferred as a separate follow-up rather than expanding this PR's diff this late in its review cycle.Changes
GVFS.Common.Git.ObjectFormat, a small helper that readsextensions.objectformatfrom a repository's local git config and reports whether it identifies a SHA256 repository (following the existingGitProcess/CacheServerResolverconfig-reading pattern). Exposes both a simpleIsSha256Repoand aTryIsSha256Repo(git, out isSha256, out error)that distinguishes "key absent" (safe default, SHA1) from a genuine config-read failure.FastFetch's startup (FastFetchVerb.cs), 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 onesgvfs clonecreated, so it is the realistic path where a SHA256 repository could actually be encountered.gvfs clone'sTryInitRepo(CloneVerb.cs), right aftergit initsucceeds - defense-in-depth alongside the--object-format=sha1pin, covering a pre-existing repo that reached v1 by another route.gvfs mount'sInProcessMount.Mount(GVFS.Mount), right after the repo is confirmed valid and beforeTrySetRequiredGitConfigSettingsruns, preventing the config corruption described above.gvfs repair'sGitConfigRepairJob:TryFixIssuesfails clearly before the repair job wipes and rebuilds.git/config(a blind rebuild would otherwise eraseextensions.objectformatand leave a SHA1-shaped config over live SHA256-hashed objects), andHasIssuereports a SHA256 enlistment as an unfixable issue sogvfs repairno longer calls it healthy. Detection here checks bothextensions.objectformatand a physical fallback,GitConfigRepairJob.PackedRefsLookSha256():.git/packed-refsis written directly by VFS for Git on clone (not through git's own ref backend) and holds object ids at full hex width (64 chars for SHA256, 40 for SHA1) - and git always writes that full width regardless of who last rewrote the file (including its ownpack-refs/gc/maintenancetasks), so the width check is a sound format probe even when the config itself is too corrupt to read. This mirrors the physical fallback Fail fast on reftable repositories, and pin clone to files ref storage #2121 already added for ref storage (.git/reftable/). Each candidate line's leading token is also validated as actual hex of a known width before being trusted, rather than acting on the first line that merely contains a space - malformed input is the expected case on an enlistment this code only ever runs against once it is already broken..git/config) does not fail the mount outright; a read failure that persists through the retries is still fatal - mount would reach the same unreadable config again moments later viaTrySetRequiredGitConfigSettingsregardless. Repair is the deliberate exception: a read failure there is surfaced as a warning but does not block the repair when the packed-refs fallback also doesn't identify a SHA256 repo, since repair's whole purpose is to rebuild a corrupt config.GVFSEnlistment.WaitUntilMounted'sBrokenPipeExceptionandJsonExceptionhandlers to usee.Messageinstead ofe.ToString()/implicitToString()for the console-facing error, so a mount process that exits early or returns a malformed response no longer dumps a raw stack trace to the console. The full exception detail still goes to the trace log. This is a narrowly-scoped fix to these two handlers; Report the real reason a mount fails instead of a broken pipe #2105 is a much larger rework of mount failure reporting (including aGetStatus.MountErrorfield) that touches the same area and should be reconciled with this change when it lands.GVFSEnlistment.GitVersion's doc comment: it is read by the--ref-format=filesversion-gatedgit initpin (from Fail fast on reftable repositories, and pin clone to files ref storage #2121), so it is now load-bearing rather than purely informational/logging. (The--object-format=sha1pin is unconditional, not version-gated, and both pins predate this PR - they are not added by it.)PackedRefsLookSha256()(SHA256-width/SHA1-width/missing-file/header-and-peeled-line/non-hex-garbage cases, under the newGVFS.UnitTests.RepairJobsnamespace matching the productionGVFS.RepairJobslayout - the first unit tests for that namespace), and for theWaitUntilMountedmessage-cleanup fix (using a real named pipe to deterministically trigger each changed code path).RepairTests.DoesNotRebuildSha256RepoConfig, that corrupts an enlistment's config to look like a SHA256 repo, confirmsgvfs mountrefuses it, and confirmsgvfs repairrefuses to silently rebuild it as SHA1 rather than fixing it (asserting the config is byte-for-byte unchanged, not merely that it still mentions "sha256").All four call sites report the same clear error message explaining that VFS for Git only supports SHA1 repositories and that the repository must be re-cloned or re-initialized as SHA1 - none of them suggest or attempt any automatic conversion.
Both the mount-path fix and the
GitConfigRepairJobfix were manually verified end-to-end against a real GVFS enlistment (a hand-crafted SHA256 repo, mounted with a locally-builtGVFS.Mount.exe/gvfs.exe): the config is left untouched instead of corrupted, and the mount process's log shows the clear SHA1-only error text.RepairTests.DoesNotRebuildSha256RepoConfig(functional) now also covers this mount+repair scenario automatically, andPackedRefsLookSha256()(unit) covers the new physical-fallback parsing logic directly - the first unit tests forGVFS.RepairJobsin this codebase.