Improve missing disk layout metadata error

[Tyrie Vella (tyrielv)]

Problem and Context

When an enlistment is missing its persisted disk layout version, gvfs mount reports that a breaking disk-layout change can have occurred since clone. That guidance is misleading for current users because the more likely cause is incomplete .gvfs metadata.

Automation also cannot reliably distinguish this condition from other generic mount failures because the command exits with the generic error code.

Changes

• Add a distinct MissingDiskLayoutVersion return code for incomplete disk-layout metadata.
• Preserve existing generic handling for other disk-layout read and upgrade failures.
• Report clearer guidance when the disk layout version is absent from repo metadata, including the log pointer and a warning to push or back up work before recloning.
• Propagate structured disk-layout failure details from the upgrade path to gvfs mount.
• Add functional coverage for both a missing disk-layout key and a missing repo metadata file.
• Add unit coverage for missing and invalid disk-layout metadata values.

[Nathan Baird (ShiningMassXAcc)]

The user-facing goal is right — the old "check if a breaking change has been made to GVFS since cloning" text is genuinely misleading, and a distinct exit code is the correct fix for the automation problem. CI is green across the full matrix. But the central mechanism has a hole.

Seven findings, ranked.

---

1. High — GVFS/GVFS.Common/DiskLayoutUpgrades/DiskLayoutUpgrade.cs:139-148

TryCheckDiskLayoutVersion calls the 3-arg TryGetDiskLayoutVersion, throwing away the returnCode this PR just added, and then re-derives the decision with string equality:

if (error == RepoMetadata.MissingDiskLayoutVersionMessage)
{
    returnCode = ReturnCode.MissingDiskLayoutVersion;
}

That string comparison is load-bearing for the new exit code, and nothing tests it. Append the log path to the message, localize it, add a trailing period — and exit code 12 silently degrades to 3, which is exactly the automation problem this PR exists to fix.

It is also internally inconsistent: TryRunAllUpgrades -> TryFindUpgrade -> TryGetDiskLayoutVersion all plumb returnCode properly. Only the one method MountVerb depends on for the new code does not.

Fix should be small: call the 5-arg TryGetDiskLayoutVersion overload and use what it returns. It already sets GenericError at the top and MissingDiskLayoutVersion in exactly the right branch, so the whole string check can be deleted. returnCode is assigned at the top of the method, so definite assignment across the try/finally is fine.

2. Medium — UTF-8 BOM added to four files

GVFS.Common/RepoMetadata.cs, GVFS/CommandLine/MountVerb.cs, GVFS.FunctionalTests/Tests/EnlistmentPerFixture/MountTests.cs and GVFS.FunctionalTests/Tools/GVFSHelpers.cs each gained a UTF-8 BOM (EF BB BF) — verified by byte-comparing the blobs against the merge base. That is the only change to line 1 of each file, it is unrelated to the fix, and it permanently dirties git blame on the first line. Please strip it.

3. Medium — DiskLayoutUpgrade.cs:19, dead overload

The new 1-arg TryRunAllUpgrades(string enlistmentRoot) has zero callers — MountVerb was the only one and it moved to the 3-arg form. Dead code on day one; delete it.

(The 3-arg TryCheckDiskLayoutVersion overload is a different story — DehydrateVerb.cs:226 and RepairVerb.cs:95 still use it, so that one earns its keep.)

4. Medium — RepairVerb.cs:95 and DehydrateVerb.cs:226

Both still call the overload that discards the return code. gvfs repair is the command a user runs after hitting this failure, and gvfs dehydrate hits the same condition — so automation still cannot distinguish it from either. Is it intentional that only mount reports code 12?

5. Low — MountTests.cs:103

private const int GVFSMissingDiskLayoutVersion = 12; duplicates the enum value. The functional test project already references GVFS.Common (see the using GVFS.Common; in GVFSHelpers.cs), so (int)ReturnCode.MissingDiskLayoutVersion keeps them in lockstep. Worth doing for the neighbouring GVFSGenericError = 3 while you are there.

6. Low — GVFSHelpers.DeletePersistedValue

Near-verbatim copy of SavePersistedValue: identical FileStream/StreamReader loop, identical json.Substring(0, 2).ShouldEqual("A ") assertion, identical rewrite loop. The only difference is repoMetadata.Add(kvp.Key, kvp.Value) versus the key filter. Factor the read into a shared helper and have both write from the resulting dictionary.

7. Low — assert against the const, not the prose

Both functional tests assert on a substring of the message text. The message is now a public const, so RepoMetadata.MissingDiskLayoutVersionMessage would stop it drifting — and would also directly guard finding 1 if you would rather not restructure that method.

---

No AI-cruft or stray .github artifacts in this diff.

[Nathan Baird (ShiningMassXAcc)]

Cross-cutting note on base branches (posting here because this PR is one of the master-targeted ones; it applies equally to #2117 and #2018).

The eight PRs tagged in pr-review-chat today split across two bases:

• vnext: Keep authentication adaptive after indeterminate /gvfs/config probes #2104, Pin git init to sha1 during clone #2115, Fail fast on SHA256 repositories in FastFetch, gvfs clone, gvfs mount, and gvfs repair #2116, Fail fast on reftable repositories, and pin clone to files ref storage #2121, Stream large -z git output instead of buffering it against the cap #2045
• master: Add functional regression tests for reset --mixed skip-worktree on hydrated placeholders #2018, Retry native C++ project builds on transient MSBuild PCH failure #2117, Improve missing disk layout metadata error #2118

The branches have genuinely diverged — 22 commits in vnext that are not in master, and 7 in master that are not in vnext — so both are live and this is not automatically wrong.

But master is the repo's GitHub default branch, which means a PR opened without an explicit base lands there by accident. Worth confirming #2018, #2117 and #2118 are deliberately targeting master. The disk-layout error change in particular ships to a different audience than the SHA256 (#2116) and reftable (#2121) work it conceptually sits beside, and #2116's own description cross-references #2119 as the shared root cause — so if these are meant to travel together, three of them are currently on the wrong branch.

[Tyrie Vella (tyrielv)]

Note

🤖 Machine-drafted reply — reviewed & approved by Tyrie before posting

Thanks, confirmed these targets are intentional.

#2018 and #2117 are test infrastructure changes, so they do not affect shipped runtime behavior and are fine for master.

#2118 is a small improvement to an existing error condition: clearer guidance and a distinct exit code when disk layout metadata is missing. I want that on the shippable line, so master is intentional.

#2116, #2121, and the related Git 3.0 preparation work are different. Those preemptively fix issues that are rare today but will become common when Git 3.0 ships. vnext should ship before then, so vnext is the intended target for that work.

[Tyrie Vella (tyrielv)]

Note

🤖 Machine-drafted reply — reviewed & approved by Tyrie before posting

Thanks — addressed the review feedback in 881644ee.

• TryCheckDiskLayoutVersion now uses the structured TryGetDiskLayoutVersion return code instead of string-matching the message.
• Removed the accidental UTF-8 BOMs.
• Deleted the unused TryRunAllUpgrades(string) overload.
• Plumbed MissingDiskLayoutVersion through repair and dehydrate.
• Updated the functional tests to use ReturnCode and RepoMetadata.MissingDiskLayoutVersionMessage.
• Refactored the metadata read/write helper duplication in GVFSHelpers.

CI is green across the matrix.

[Nathan Baird (ShiningMassXAcc)]

Review pass over this PR. 6 comment(s).

These findings concern code the diff does not touch directly, so GitHub cannot anchor them inline:

GVFS/GVFS/CommandLine/MountVerb.cs (around line 142) - LOW

This reads as though both ReportErrorAndExit calls could run; it is only correct because the first one throws VerbAbortedException. An if/else would make the exclusivity explicit and survive someone later changing how ReportErrorAndExit terminates.

(PR description) - LOW

Small description nit: the bullet about propagating "structured disk-layout failure details … instead of re-reading metadata after failure" does not match the baseline — MountVerb.PreCreateEnlistment on master never re-read metadata after a failure. The change is real; the "instead of" framing describes something that was not there. Worth rewording so the description matches the diff.

[Nathan Baird (ShiningMassXAcc)]

Two things about the new message:

1. It drops the log pointer. Every other TryCheckDiskLayoutVersion failure ends with Run 'gvfs log <enlistmentRoot>' for more details. (see DiskLayoutUpgrade.cs:134). We do write a log on this path, so omitting the pointer means the user has the one piece of diagnostic information we produced and no idea it exists.

2. "Recloning this enlistment is the recommended fix" is unqualified. That is advice to destroy an enlistment that may be very large and may hold unpushed commits or uncommitted work, offered at the exact moment we have told the user we cannot read its metadata. GVFS.Hooks/Program.Unstage.cs:102 is a good model for how we caveat destructive guidance elsewhere.

Suggest adding the log pointer, and a short "make sure your work is pushed or backed up first" ahead of the reclone recommendation.

[Nathan Baird (ShiningMassXAcc)]

Using (int)ReturnCode.MissingDiskLayoutVersion and RepoMetadata.MissingDiskLayoutVersionMessage makes the assertion move in lockstep with the product: if someone renumbers the enum or rewrites the message, the shipped contract changes and the test stays green.

That contract is the whole point of this PR — the exit code and message are what scripts and users key off. CONTRIBUTING.md asks functional tests to treat the product as a black box for exactly this reason.

Suggest pinning the literal 12 and a stable phrase from the message instead, with a comment noting these are a shipped contract.

(I realize this cuts against what the previous review asked for — flagging it so you two can settle on one answer rather than me quietly reversing it.)

[Nathan Baird (ShiningMassXAcc)]

After this change the 3-argument TryCheckDiskLayoutVersion overload and the new private 3-argument TryFindUpgrade have no callers left in the tree. Same dead-overload cleanup the prior review asked for on the other pair — suggest deleting both so there is only one shape of each to maintain.

[Nathan Baird (ShiningMassXAcc)]

The new functional tests cover gvfs mount only, but this PR also changes the return-code plumbing in repair (here) and dehydrate (DehydrateVerb.cs:228), plus the returnCode != MissingDiskLayoutVersion fall-through at DiskLayoutUpgrade.cs:131-135. Those three branches are what decides whether a user gets the new actionable message or the old generic one, and none of them are exercised.

A functional test per verb would be expensive. A unit test over RepoMetadata.TryGetOnDiskLayoutVersion using MockFileSystem and a mock:\ root would cover the core cheaply — GVFS.UnitTests/Virtual/TestsWithCommonRepo.cs has the pattern.

[Nathan Baird (ShiningMassXAcc)]

Re-review. Commit 0ccec54 addresses all four of my earlier threads — the log pointer and backup caveat are in the message, the dead overloads are gone, the functional test pins the literal 12, and RepoMetadataTests.cs is new coverage. I'm satisfied on all of those.

Three remaining observations below, none of them blocking. CI is green across every leg.

Also worth calling out as a positive that the description doesn't mention: the GVFSHelpers change from repoMetadata.Add(k, v) to an indexer assignment when replaying the append-only log quietly fixes a latent ArgumentException on duplicated keys.

Medium — GVFS/GVFS.Mount/InProcessMount.cs ~L1538 (not in this diff)

This call site still uses the 3-argument TryGetOnDiskLayoutVersion, so the mount process prints the new "reclone" text but exits with GenericError (3) rather than the new 12, and without the gvfs log pointer.

That leaves the same user-visible message reachable under two different exit codes, which undercuts the automation story the new code is justified by. Worth routing this one through the new overload too.

[Nathan Baird (ShiningMassXAcc)]

Medium — The new out string error from TryRunAllUpgrades is discarded for every return code except MissingDiskLayoutVersion — including the catch path, where it carries the exception detail and is the only place that detail exists.

Either surface it on the other failure codes, or drop the out-parameter and keep the specific message construction local to the one branch that uses it. As written it reads like an oversight rather than a decision.

[Nathan Baird (ShiningMassXAcc)]

Medium — Both tests assert only on MissingDiskLayoutVersionMessage. Nothing pins the gvfs log pointer that was added in response to the earlier review, so removing that append would leave CI fully green.

This is the same regression shape I flagged last pass — the valuable half of the message is the half that isn't asserted. A ShouldContain on the log-pointer fragment would close it.

[Tyrie Vella (tyrielv)]

Note

🤖 Machine-drafted reply — reviewed & approved by Tyrie before posting

Thanks — addressed the two active follow-up comments in 439c98ad.

• MountVerb now includes diskLayoutError in the generic upgrade-failure message, so the non-MissingDiskLayoutVersion path no longer discards the detail returned by TryRunAllUpgrades.
• The missing-layout functional tests now assert that the command output includes the gvfs log pointer, in addition to the shipped exit code and stable incomplete-metadata phrase.

Focused local validation passed for the two missing-layout mount functional tests. GitHub checks are running on the new head.