Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Tyrie Vella (tyrielv) merged 1 commit into from
Sep 28, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion GVFS/GVFS.FunctionalTests/Tests/GitCommands/CorruptionReproTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@
namespace GVFS.FunctionalTests.Tests.GitCommands
{
/// <summary>
/// This class is used to reproduce corruption scenarios in the GVFS virtual projection.
/// Reproduces reported scenarios that corrupted the GVFS virtual projection or
/// gave results that did not match a normal git repo. Each test is a black-box
/// regression test: it runs the command sequence that exposed the problem and
/// compares the results with the control repo.
/// </summary>
[Category(Categories.GitCommands)]
[TestFixtureSource(typeof(GitRepoTests), nameof(GitRepoTests.ValidateWorkingTree))]
Expand Down Expand Up @@ -76,6 +79,32 @@ public void ReproCherryPickRestoreCorruption()
this.FilesShouldMatchCheckoutOfSourceBranch();
}

/// <summary>
/// Regression test for a sequence of commands that a user ran and that
/// showed incorrect behavior: after "git blame" and "git reset --mixed",
/// GVFS did not report a changed file as modified. The control repo did.
///
/// This is a black-box test. It does not assert why the behavior was wrong.
/// It only runs the same commands and compares the results with the
/// control repo. ResetMixedTests.ResetMixedClearsSkipWorktreeOnHydratedPlaceholder
/// asserts the conditions that cause the problem.
///
/// The fix requires microsoft/git v2.55.0.vfs.0.3 or later.
///
/// On the FunctionalTests/20201014 branch, Readme.md is the only file that
/// differs between HEAD and HEAD~1.
/// </summary>
[TestCase]
public void ReproResetMixedSkipWorktree()

@ShiningMassXAcc Nathan Baird (ShiningMassXAcc) Sep 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tyrielv Tyrie Vella (tyrielv) Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@ShiningMassXAcc Nathan Baird (ShiningMassXAcc) Sep 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
this.ValidateGitCommand("blame Readme.md");

@ShiningMassXAcc Nathan Baird (ShiningMassXAcc) Sep 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tyrielv Tyrie Vella (tyrielv) Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ShiningMassXAcc Nathan Baird (ShiningMassXAcc) Sep 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.ValidateGitCommand("checkout -b tests/functional/ReproResetMixedSkipWorktree");

// Both repos must report Readme.md as modified.
this.ValidateGitCommand("reset HEAD~1");
this.ValidateGitCommand("ls-files -v Readme.md");
}

/// <summary>
/// Reproduction of a reported issue:
/// Restoring a file after its parent directory was deleted fails with
Expand Down
57 changes: 57 additions & 0 deletions GVFS/GVFS.FunctionalTests/Tests/GitCommands/ResetMixedTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
using GVFS.FunctionalTests.Properties;
using GVFS.FunctionalTests.Should;
using GVFS.FunctionalTests.Tools;
using GVFS.Tests.Should;
using NUnit.Framework;
using System.IO;

namespace GVFS.FunctionalTests.Tests.GitCommands
{
Expand Down Expand Up @@ -30,6 +33,52 @@ public void ResetMixedAfterPrefetch()
this.FilesShouldMatchCheckoutOfTargetBranch();
}

/// <summary>
/// A mixed reset must clear skip-worktree on a hydrated placeholder whose
/// index entry the reset changes. A hydrated placeholder is on disk but is
/// not in ModifiedPaths, so it still has skip-worktree. The reset does not
/// update the working tree, so the file on disk no longer matches the index.
/// If skip-worktree stays set, git does not compare the file to the index,
/// and reset and status do not report it as modified.
///
/// Placeholders that are not on disk do not have this problem, because git
/// writes them to disk and clears skip-worktree. For that reason, this test
/// asserts each precondition before the reset.
///
/// The git side of this behavior requires microsoft/git v2.55.0.vfs.0.3 or later.
/// </summary>
[TestCase]
public void ResetMixedClearsSkipWorktreeOnHydratedPlaceholder()
{
string filePath = Path.Combine("Test_ConflictTests", "ModifiedFiles", "ChangeInTarget.txt");
string gitPath = filePath.Replace(Path.DirectorySeparatorChar, TestConstants.GitPathSeparator);

// Create local branches for both commits in both repos.
this.ValidateGitCommand("checkout " + GitRepoTests.ConflictSourceBranch);
this.ValidateGitCommand("checkout " + GitRepoTests.ConflictTargetBranch);

// Precondition: the reset changes the index entry for the file.
GitProcess.InvokeProcess(
this.ControlGitRepo.RootPath,
$"diff --quiet {GitRepoTests.ConflictSourceBranch} {GitRepoTests.ConflictTargetBranch} -- {gitPath}")
.ExitCode.ShouldEqual(1, $"{gitPath} must differ between the reset source and target");

// Precondition: the file is a hydrated placeholder. A read hydrates it
// but does not add it to ModifiedPaths, so skip-worktree stays set.
this.Enlistment.GetVirtualPathTo(filePath).ShouldBeAFile(this.FileSystem).WithContents();
GVFSHelpers.ModifiedPathsShouldNotContain(this.Enlistment, this.FileSystem, gitPath);
this.SkipWorktreeFlagShouldBe(gitPath, expectedFlag: 'S');

// The reset output and status must report the file as modified, as in the control repo.
this.ValidateGitCommand("reset --mixed " + GitRepoTests.ConflictSourceBranch);

// After the reset, skip-worktree is cleared, and GVFS adds the file to
// ModifiedPaths so that later git commands also compare it to the index.
this.SkipWorktreeFlagShouldBe(gitPath, expectedFlag: 'H');
GVFSHelpers.ModifiedPathsShouldContain(this.Enlistment, this.FileSystem, gitPath);
this.FileContentsShouldMatch(filePath);
}

[TestCase]
public void ResetMixedAndCheckoutNewBranch()
{
Expand Down Expand Up @@ -108,5 +157,13 @@ protected override void CreateEnlistment()
this.ControlGitRepo.Fetch(GitRepoTests.ConflictTargetBranch);
this.ControlGitRepo.Fetch(GitRepoTests.ConflictSourceBranch);
}

private void SkipWorktreeFlagShouldBe(string gitPath, char expectedFlag)
{
// "ls-files -v" prefixes each entry with a tag: "S" means skip-worktree is set, "H" means it is not.
ProcessResult result = GitProcess.InvokeProcess(this.Enlistment.RepoRoot, "ls-files -v -- " + gitPath);
result.ExitCode.ShouldEqual(0, result.Errors);
result.Output.Trim().ShouldEqual($"{expectedFlag} {gitPath}");
}
}
}
5 changes: 5 additions & 0 deletions GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,11 @@ private static string FilterMessages(
return input;
}

/// <summary>
/// Runs a git command in the control repo and in the GVFS repo, and asserts
/// that the output and errors match. If the command is not "status", this
/// method then runs "status" in both repos and compares that output too.
/// </summary>
public static void ValidateGitCommand(
GVFSFunctionalTestEnlistment enlistment,
ControlGitRepo controlGitRepo,
Expand Down
Loading