Add functional regression tests for reset --mixed skip-worktree on hydrated placeholders

[Tyrie Vella (tyrielv)]

Problem and Context

In a VFS for Git enlistment, a hydrated placeholder is a file that was read and written to disk by ProjFS, but not modified. It is not in ModifiedPaths, so it keeps the skip-worktree bit in the index.

git reset --mixed updates the index but does not touch the working tree. If git leaves skip-worktree set on a hydrated placeholder whose index entry the reset changes, git does not compare the file with the index. The reset output does not list the file, and git status reports a clean tree while the file on disk does not match the index.

Git fixed this in microsoft/git#935, first shipped in v2.55.0.vfs.0.3. Git's own tests (t1093-virtualfilesystem.sh) stub the VFS hooks, so they cannot cover real ProjFS hydration or ModifiedPaths. These tests cover that behavior in GVFS.

Changes

• CorruptionReproTests.ReproResetMixedSkipWorktree: a black-box regression test. It runs the command sequence that exposed the problem (blame Readme.md, then reset HEAD~1 on FunctionalTests/20201014) and compares every result with the control repo. It does not assert why the behavior was wrong.

• ResetMixedTests.ResetMixedClearsSkipWorktreeOnHydratedPlaceholder: a white-box test for the same behavior. It uses Test_ConflictTests/ModifiedFiles/ChangeInTarget.txt and a reset from FunctionalTests/20201014_Conflict_Target to FunctionalTests/20201014_Conflict_Source. Before the reset, it asserts each precondition:

  • the reset changes the file's index entry;
  • the file is hydrated by an explicit read;
  • the file is not in ModifiedPaths;
  • skip-worktree is set.

  After the reset, it asserts that skip-worktree is cleared, that GVFS adds the file to ModifiedPaths, and that the reset output, status, and file contents match the control repo.

• ResetMixedAfterPrefetch does not cover this case. On FunctionalTests/20201014_Conflict_Target, HEAD only deletes files from HEAD~1. The reset adds index entries for files that are not on disk, and git already handled that case correctly.

• GitHelpers.ValidateGitCommand: add a doc comment. It says that the method also compares git status output after every command other than status.

[Nathan Baird (ShiningMassXAcc)]

Adding a repro test for a known projection bug is exactly the right instinct, and the blame-to-hydrate trick plus the note about Readme.md being the only file differing between HEAD and HEAD~1 on FunctionalTests/20201014 is genuinely useful context. One blocking question and a few follow-ups.

Four findings, ranked.

---

1. High — the test is green, so what is it proving?

ValidateGitCommand fails when GVFS output diverges from the control repo. If the bug described in the doc comment were live, this test would be red. CI is fully green (35/35).

So which is it?

• microsoft/git#935 is already in the pinned git, and this is a genuine regression guard — great, and worth saying so in the PR body; or
• the test does not actually exercise the divergence, in which case it is a no-op that will never catch the regression it is named for.

The title says "depends on microsoft/git#935", which suggests the fix may not be in yet — and if so, a passing test is the wrong signal. Could you state in the PR which git version first makes this go from failing to passing? That is the one fact that tells a future reader whether this test has teeth.

2. Medium — the doc comment promises git status, but the test never runs it

The comment mentions status twice ("status shows it as modified", "status incorrectly reports clean"), yet the test only runs blame, checkout -b and reset HEAD~1.

And teardown will not cover for it: GitRepoTests.TearDownForTest -> TestValidationAndCleanup does CheckHeadCommitTree() and a deep working-tree structure comparison against the control repo. After a mixed reset the working trees are byte-identical in both repos — mixed reset does not touch the working tree — so the skip-worktree/status divergence cannot be caught there.

Everything therefore rests on reset's stdout differing. Adding this.ValidateGitCommand("status") is the direct assertion for the documented symptom and costs nothing.

3. Medium — "Expected / Actual (bug)" will ship as wrong documentation

Given CI is green, the block asserting "Actual (bug): reset output is empty, status reports clean" describes a state that is probably no longer true, so it lands in the repo already stale.

Suggest retargeting the comment at what the test guards — asserts that reset reports M Readme.md, which regresses when skip-worktree hides the working-tree/index mismatch — and moving the investigation narrative to the PR description. The branch/commit note about Readme.md is worth keeping verbatim.

4. Low — no closing state assertion

The sibling ReproCherryPickRestoreCorruption ends with this.FilesShouldMatchCheckoutOfSourceBranch();. This one ends on a bare command. Worth a closing assertion for symmetry, especially alongside finding 2.

(No branch cleanup needed — the fixture is enlistmentPerTest: true, so the tests/functional/ReproResetMixedSkipWorktree branch goes away with the enlistment.)

[Tyrie Vella (tyrielv)]

Note

This comment was drafted by an AI assistant and reviewed by Tyrie Vella (@tyrielv) before posting.

Thanks for the review. I pushed db9c4cec.

1. Regression guard or no-op? It is a regression guard. The git fix (reset --mixed: clear skip-worktree for all changed entries in VFS mode git#935) first shipped in v2.55.0.vfs.0.3. This PR's June CI runs were red on v2.54.x. The current run is green because CI now installs v2.55.0.vfs.0.10. Git's t1093 tests stub the VFS hooks, so git cannot test this with real ProjFS hydration. That end-to-end coverage is the purpose of this PR. The PR title, description, and doc comment now say this.

2. status never runs. It does run. GitHelpers.ValidateGitCommand runs status in both repos and compares the output after every command except status itself. So reset HEAD~1 is followed by a status comparison. An explicit ValidateGitCommand("status") would only repeat it. The doc comment now says this, so the next reader does not have to find it.

3. Stale "Actual (bug)" text. Agreed. I rewrote the doc comment to describe what the test guards against, and I kept the Readme.md / FunctionalTests/20201014 note.

4. Closing assertion. FilesShouldMatchCheckoutOfSourceBranch() checks the cherry-pick fixture's Test_ConflictTests files, so it does not apply here. Instead I added ValidateGitCommand("ls-files -v Readme.md"), which checks the mechanism directly: skip-worktree must be cleared, as in the control repo.

[Nathan Baird (ShiningMassXAcc)]

Thanks — db9c4cec addresses all four points from my last pass. I re-checked each one against the code rather than taking the replies at face value, and they hold up:

1. Regression guard or no-op? — Resolved. Confirmed independently: microsoft/git#935 merged 2026-07-23, and origin/master CI pins v2.55.0.vfs.0.10 (.github/workflows/build.yaml), which is ≥ the v2.55.0.vfs.0.3 first-shipping version. The title, the PR body, and the doc comment now all state the version dependency. This is a real forward-looking guard.
2. status never runs — Resolved; my premise was wrong. GitHelpers.ValidateGitCommand re-invokes itself with status after every non-status command (GitHelpers.cs:154-157), so reset HEAD~1 is followed by a status comparison. An explicit ValidateGitCommand("status") would be pure duplication. Withdrawn.
3. Stale "Expected / Actual (bug)" text — Resolved. The block is gone, the comment now describes what the test guards, and the Readme.md / FunctionalTests/20201014 note is kept.
4. No closing assertion — Resolved, and your reasoning checks out: FilesShouldMatchCheckoutOfSourceBranch is entirely Test_ConflictTests files (GitRepoTests.cs:624-636), so it genuinely doesn't apply to this fixture branch. ls-files -v Readme.md is a better assertion anyway — it tests the mechanism rather than a side effect.

Two other things I checked while I was in here, both fine: GIT_QUIET=true (GitHelpers.cs:144) does not suppress Unstaged changes after reset: — I ran it against v2.55.0.vfs.0.8 — so the reset-output comparison really is doing work; and Readme.md is projected in sparse mode (SparseTests.cs:686), so the SparseMode fixture variant is safe.

Four new comments below, none blocking. The first two are about whether this test keeps its teeth over time; the last two are polish. Note that #2 is arguably the deeper version of my original finding 1 — the version dependency is now documented, but the precondition it relies on still isn't asserted.

[Nathan Baird (ShiningMassXAcc)]

Consider moving this to ResetMixedTests.cs — and could you say how it differs from ResetMixedAfterPrefetch?

On placement: ResetMixedTests is configured identically to this fixture (enlistmentPerTest: true, same TestFixtureSource(GitRepoTests.ValidateWorkingTree), same Categories.GitCommands) and already holds seven reset --mixed tests. CorruptionReproTests is documented as "reproduce corruption scenarios in the GVFS virtual projection", and both of its existing tests link a VFSForGit projection-corruption issue (#1855, #1901). This one is a reset --mixed behavior guard for a git-side fix, so it reads like it belongs next to its siblings.

The part I care more about: ResetMixedTests.ResetMixedAfterPrefetch (lines 25-31) already does

this.ValidateGitCommand("checkout " + GitRepoTests.ConflictTargetBranch);
this.Enlistment.Prefetch("--files * --hydrate");
this.ValidateGitCommand("reset --mixed HEAD~1");

which is hydrate-then-mixed-reset with the same output-and-status comparison against the control repo. From the outside that looks like the same scenario this PR is adding. Either it doesn't actually reach the buggy path — in which case knowing why is valuable, because it may be a hole worth closing in that test too — or there's real overlap here. I couldn't check it myself: the fixture branches live in the ForTests repo, not this one, so I can't diff FunctionalTests/20201014_Conflict_Target HEAD against HEAD~1. You can.

[Tyrie Vella (tyrielv)]

Note

This comment was drafted by an AI assistant and reviewed by Tyrie Vella (@tyrielv) before posting.

I kept this test in CorruptionReproTests because that class is for black-box regression tests. Each test runs a command sequence that exposed a real problem, without assumptions about the cause. The class doc comment now says this. I also added ResetMixedTests.ResetMixedClearsSkipWorktreeOnHydratedPlaceholder, a white-box test next to its siblings.

About ResetMixedAfterPrefetch: on FunctionalTests/20201014_Conflict_Target, HEAD only deletes 9 files from HEAD~1 (all D under GVFlt_MoveFileTest/LongFileName). The reset adds index entries for files that are not on disk, which is the !file_exists() path that git already handled. So it never reaches the hydrated-placeholder case. The new white-box test uses a real content change (ChangeInTarget.txt, Target → Source).

[Nathan Baird (ShiningMassXAcc)]

Verified — and splitting this into a black-box repro plus a white-box test is a better answer than the one I asked for.

The class doc rewrite addresses my premise directly: I was reading the old "reproduce corruption scenarios in the GVFS virtual projection" wording, which is what made the placement look wrong. Defining the fixture as black-box regression tests makes the division of labour explicit, and the new test sits next to its siblings where I wanted it.

The ResetMixedAfterPrefetch explanation is exactly the distinction I was fishing for — HEAD only deleting files relative to HEAD~1 means the reset adds index entries for files that aren't on disk, which is the !file_exists() path git already handled, so it never reaches the hydrated case. That's a real gap in the old test, now covered by using an actual content change. I couldn't check any of this myself: the fixture branches live in the ForTests repo rather than this one. Thanks for running it down.

[Nathan Baird (ShiningMassXAcc)]

This is the deeper version of my finding 1 last round, and the one thing I'd still like changed. You've now documented the version dependency, but the precondition the test relies on is never asserted — so the test can go quietly toothless without ever going red.

Per the root cause in microsoft/git#935, the pre-fix code only left skip-worktree set when file_exists(two->path) was true. For a file that is not materialized, the old code already cleared skip-worktree. So if blame ever stops materializing Readme.md — a git-side change to how blame builds its fake working-tree commit, a ProjFS change, a projection change — this test still passes while guarding nothing at all. The entire scenario hangs on an unasserted side effect of git blame's internals, which is a thin thread for a test whose whole value is catching a future regression.

Two one-liners pin both halves of the state the doc comment claims, and both have precedent in this suite:

// Guarantee hydration rather than relying on blame's internals, and pin the
// "hydrated but not in ModifiedPaths" precondition the reset depends on.
this.Enlistment.GetVirtualPathTo("Readme.md").ShouldBeAFile(this.FileSystem).WithContents();
GVFSHelpers.ModifiedPathsShouldNotContain(this.Enlistment, this.FileSystem, "Readme.md");

CheckoutTests.cs:338 uses the read-to-hydrate idiom; CheckoutTests.cs:354 uses the ModifiedPaths assertion. The explicit read also makes hydration deterministic instead of incidental, which is worth having regardless of the assertion.

[Tyrie Vella (tyrielv)]

Note

This comment was drafted by an AI assistant and reviewed by Tyrie Vella (@tyrielv) before posting.

Agreed, but I made this change in the new white-box test and left the repro unchanged. ResetMixedClearsSkipWorktreeOnHydratedPlaceholder hydrates the file with an explicit read. It then asserts that the index entry changes, that the file is not in ModifiedPaths, and that ls-files -v shows S before the reset. After the reset, it asserts H and that the file is in ModifiedPaths. If hydration or the projection changes, those precondition checks fail instead of the test passing with no effect. The repro stays a black-box record of the original command sequence.

[Nathan Baird (ShiningMassXAcc)]

Verified, and this is the outcome I was after.

ResetMixedClearsSkipWorktreeOnHydratedPlaceholder pins every link in the chain: the control-repo diff --quiet exit code proves the reset genuinely changes the index entry, and S before / H after plus the ModifiedPaths pair pin the hydrated-placeholder state on both sides of the reset. That's stronger than the two one-liners I suggested.

The key improvement is that hydration is now an explicit read rather than an incidental side effect of blame's internals, so the coverage can't quietly evaporate — which was the entire risk I was raising. Agreed that leaving the repro unasserted is the right call now that its doc comment says plainly that it's a black-box record and names the test that asserts the mechanism.

[Nathan Baird (ShiningMassXAcc)]

Terminology nit: in GVFS a full file is specifically the result of a write. ProjFS notifies OnFilePreConvertToFull → OnFileConvertedToFull (FileSystemVirtualizer.cs:293-302, FileSystemCallbacks.cs:611), and that handler calls AddModifiedPathAndRemoveFromPlaceholderList (FileSystemCallbacks.cs:946-950). So in this codebase "full file" implies "in ModifiedPaths" — which makes "a full file on disk, but NOT in ModifiedPaths" contradict itself.

Reading a placeholder gives you a hydrated placeholder, which is the term microsoft/git#935 uses and the state this test actually wants. Suggest swapping the wording.

[Tyrie Vella (tyrielv)]

Note

This comment was drafted by an AI assistant and reviewed by Tyrie Vella (@tyrielv) before posting.

Good catch. I replaced the wording with "hydrated placeholder". The repro no longer describes the mechanism, and the white-box test uses the correct term.

[Nathan Baird (ShiningMassXAcc)]

Verified — "full file" is gone, and the new test uses "hydrated placeholder" throughout. Dropping the mechanism description from the repro altogether is the cleaner call, since that's the test that deliberately doesn't claim to know the cause.

[Nathan Baird (ShiningMassXAcc)]

Follow-on from my finding 3 last round — this is minor, and mostly a consequence of answering my own review in the code. Two paragraphs here are PR-description material rather than code documentation, and both are already in the PR body almost verbatim:

• "Git's own tests stub the VFS hooks, so they cannot cover real ProjFS hydration. This test covers the behavior end to end." — the PR body has this (with the more useful t1093-virtualfilesystem.sh filename, which this version drops). It's the argument for the PR existing, not something the next reader of this test needs.
• "Each ValidateGitCommand call also runs git status in both repos and compares the output…" — also in the PR body. More to the point, this documents GitHelpers.ValidateGitCommand (GitHelpers.cs:154-157), not this test. It'll rot if the harness changes, and it invites a copy of the same paragraph in every future test that uses the helper. If it's worth writing down permanently — and given I got it wrong last round, it might be — the helper's own doc comment is the place.

Worth keeping, because they're genuinely local to this test: the v2.55.0.vfs.0.3 version dependency, and the note that Readme.md is the only file differing between HEAD and HEAD~1 on FunctionalTests/20201014.

[Tyrie Vella (tyrielv)]

Note

This comment was drafted by an AI assistant and reviewed by Tyrie Vella (@tyrielv) before posting.

Agreed. I removed both paragraphs from the test. The ValidateGitCommand status behavior is now in the doc comment on GitHelpers.ValidateGitCommand. The version requirement and the Readme.md / FunctionalTests/20201014 note stay in the test.

[Nathan Baird (ShiningMassXAcc)]

Verified — both paragraphs are gone, and moving the status behavior onto GitHelpers.ValidateGitCommand's own doc comment is exactly right. It's now documented once, next to the code it actually describes, instead of being restated in every test that uses the helper. Keeping the version requirement and the Readme.md / FunctionalTests/20201014 note in the test is the right split — those are genuinely local to it.

[Nathan Baird (ShiningMassXAcc)]

Approving. All four points from my last pass verify as resolved in b35b9b6f6, and I checked the code rather than taking the replies at face value.

1. Fixture placement / ResetMixedAfterPrefetch overlap — Resolved, and better than what I asked for: the fixture's purpose is now stated as black-box regression testing (my premise came from the old class doc), and the mechanism gets its own white-box test next to its siblings. The overlap question is answered concretely.
2. Unasserted hydration precondition — Resolved. The new test asserts that the reset really changes the index entry, and pins S → H plus the ModifiedPaths transition around it. Hydration is now an explicit read, so the coverage no longer depends on git blame's internals — that was the toothless-green risk.
3. "full file" terminology — Resolved; "hydrated placeholder" throughout.
4. PR-description residue — Resolved, including moving the ValidateGitCommand status behavior onto the helper's own doc comment.

Two things I checked on the new code, both clean: Test_ConflictTests is in GitRepoTests.SparseModeFolders, so the new ChangeInTarget.txt path is projected in the SparseMode fixture variant; and the AI-cruft scan over the added lines comes back empty. The ModifiedPathsShouldContain assertion after the reset is empirically backed by the functional shards passing on both x64 and arm64.

Net: this went from one test with a documented-but-unasserted precondition to a repro plus a mechanism test, with the harness behavior documented where it belongs. Nice work.