-
Notifications
You must be signed in to change notification settings - Fork 466
Fix corruption when unstaging changes made by git commands #1856
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.
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| namespace GVFS.Common.NamedPipes | ||
| { | ||
| public static partial class NamedPipeMessages | ||
| { | ||
| public static class PrepareForUnstage | ||
| { | ||
| public const string Request = "PreUnstage"; | ||
| public const string SuccessResult = "S"; | ||
| public const string FailureResult = "F"; | ||
|
|
||
| public class Response | ||
| { | ||
| public Response(string result) | ||
| { | ||
| this.Result = result; | ||
| } | ||
|
|
||
| public string Result { get; } | ||
|
|
||
| public Message CreateMessage() | ||
| { | ||
| return new Message(this.Result, null); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Diagnostics; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Threading.Tasks; | ||
| using GVFS.Common; | ||
| using GVFS.FunctionalTests.Properties; | ||
| using GVFS.FunctionalTests.Tests.EnlistmentPerTestCase; | ||
| using NUnit.Framework; | ||
|
|
||
| namespace GVFS.FunctionalTests.Tests.GitCommands | ||
| { | ||
| /// <summary> | ||
| /// This class is used to reproduce corruption scenarios in the GVFS virtual projection. | ||
| /// </summary> | ||
| [Category(Categories.GitCommands)] | ||
| [TestFixtureSource(typeof(GitRepoTests), nameof(GitRepoTests.ValidateWorkingTree))] | ||
| public class CorruptionReproTests : GitRepoTests | ||
| { | ||
| public CorruptionReproTests(Settings.ValidateWorkingTreeMode validateWorkingTree) | ||
| : base(enlistmentPerTest: true, validateWorkingTree: validateWorkingTree) | ||
| { | ||
| } | ||
|
|
||
| [TestCase] | ||
| public void ReproCherryPickRestoreCorruption() | ||
| { | ||
| // Reproduces a corruption scenario where git commands (like cherry-pick -n) | ||
| // stage changes directly, bypassing the filesystem. In VFS mode, these staged | ||
| // files have skip-worktree set and are not in the ModifiedPaths database. | ||
| // Without the fix, a subsequent "restore --staged" would fail to properly | ||
| // unstage them, leaving the index and projection in an inconsistent state. | ||
| // | ||
| // See https://github.com/microsoft/VFSForGit/issues/1855 | ||
|
|
||
| // Based on FunctionalTests/20170206_Conflict_Source | ||
| const string CherryPickCommit = "51d15f7584e81d59d44c1511ce17d7c493903390"; | ||
| const string StartingCommit = "db95d631e379d366d26d899523f8136a77441914"; | ||
|
|
||
| this.ControlGitRepo.Fetch(StartingCommit); | ||
| this.ControlGitRepo.Fetch(CherryPickCommit); | ||
|
|
||
| this.ValidateGitCommand($"checkout -b FunctionalTests/CherryPickRestoreCorruptionRepro {StartingCommit}"); | ||
|
|
||
| // Cherry-pick stages adds, deletes, and modifications without committing. | ||
| // In VFS mode, these changes are made directly by git in the index — they | ||
| // are not in ModifiedPaths, so all affected files still have skip-worktree set. | ||
| this.ValidateGitCommand($"cherry-pick -n {CherryPickCommit}"); | ||
|
|
||
| // Restore --staged for a single file first. This verifies that only the | ||
| // targeted file is added to ModifiedPaths, not all staged files (important | ||
| // for performance when there are many staged files, e.g. during merge | ||
| // conflict resolution). | ||
| // | ||
| // Before the fix: added files with skip-worktree would be skipped by | ||
| // restore --staged, remaining stuck as staged in the index. | ||
| this.ValidateGitCommand("restore --staged Test_ConflictTests/AddedFiles/AddedBySource.txt"); | ||
|
|
||
| // Restore --staged for everything remaining. Before the fix: | ||
| // - Modified files: restored in the index but invisible to git status | ||
| // because skip-worktree was set and the file wasn't in ModifiedPaths, | ||
| // so git never checked the working tree against the index. | ||
| // - Deleted files: same issue — deletions became invisible. | ||
| // - Added files: remained stuck as staged because restore --staged | ||
| // skipped them (skip-worktree set), and their ProjFS placeholders | ||
| // would later vanish when the projection reverted to HEAD. | ||
| this.ValidateGitCommand("restore --staged ."); | ||
|
|
||
| // Restore the working directory. Before the fix, this step would | ||
| // silently succeed but leave corrupted state: modified/deleted files | ||
| // had stale projected content that didn't match HEAD, and added files | ||
| // (as ProjFS placeholders) would vanish entirely since they're not in | ||
| // HEAD's tree. | ||
| this.ValidateGitCommand("restore -- ."); | ||
| this.FilesShouldMatchCheckoutOfSourceBranch(); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| using GVFS.Common.NamedPipes; | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
|
|
||
| namespace GVFS.Hooks | ||
| { | ||
| /// <summary> | ||
| /// Partial class for unstage-related pre-command handling. | ||
| /// Detects "restore --staged" and "checkout HEAD --" operations and sends | ||
| /// a PrepareForUnstage message to the GVFS mount process so it can add | ||
| /// staged files to ModifiedPaths before git clears skip-worktree. | ||
| /// </summary> | ||
| public partial class Program | ||
| { | ||
| /// <summary> | ||
| /// Detects whether the git command is an unstage operation that may need | ||
| /// special handling for VFS projections. | ||
| /// Matches: "restore --staged", "restore -S", "checkout HEAD --" | ||
| /// </summary> | ||
| private static bool IsUnstageOperation(string command, string[] args) | ||
| { | ||
| if (command == "restore") | ||
| { | ||
| return args.Any(arg => | ||
| arg.Equals("--staged", StringComparison.OrdinalIgnoreCase) || | ||
| // -S is --staged; char overload of IndexOf is case-sensitive, | ||
| // which is required because lowercase -s means --source | ||
| (arg.StartsWith("-") && !arg.StartsWith("--") && arg.IndexOf('S') >= 0)); | ||
| } | ||
|
|
||
| if (command == "checkout") | ||
| { | ||
| // "checkout HEAD -- <paths>" is an unstage+restore operation | ||
| bool hasHead = args.Any(arg => arg.Equals("HEAD", StringComparison.OrdinalIgnoreCase)); | ||
| bool hasDashDash = args.Any(arg => arg == "--"); | ||
| return hasHead && hasDashDash; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Extracts pathspec arguments from a restore --staged command. | ||
| /// Returns null-separated pathspecs, or empty string for all staged files. | ||
| /// </summary> | ||
| private static string GetRestorePathspec(string command, string[] args) | ||
| { | ||
| // args[0] = hook type, args[1] = git command, rest are arguments | ||
| // Skip flags (--staged, -S, --source=, -s, etc.) and extract paths | ||
| List<string> paths = new List<string>(); | ||
| bool pastDashDash = false; | ||
|
|
||
| for (int i = 2; i < args.Length; i++) | ||
| { | ||
| string arg = args[i]; | ||
|
|
||
| if (arg.StartsWith("--git-pid=")) | ||
| continue; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MEDIUM: Tree-ish arguments incorrectly forwarded as pathspecs (2/4 models agree) The parser adds every non-option token before At OS repo scale (2.5M files), accidental scope widening is dangerous. Recommendation: Parse |
||
| if (arg == "--") | ||
| { | ||
| pastDashDash = true; | ||
| continue; | ||
| } | ||
| if (!pastDashDash && arg.StartsWith("-")) | ||
| continue; | ||
|
|
||
| paths.Add(arg); | ||
| } | ||
|
|
||
| return paths.Count > 0 ? string.Join("\0", paths) : ""; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Sends a PrepareForUnstage message to the GVFS mount process, which will | ||
| /// add staged files matching the pathspec to ModifiedPaths so that git will | ||
| /// clear skip-worktree and process them. | ||
| /// </summary> | ||
| private static void SendPrepareForUnstageMessage(string command, string[] args) | ||
| { | ||
| string pathspec = GetRestorePathspec(command, args); | ||
| string message = string.IsNullOrEmpty(pathspec) | ||
| ? NamedPipeMessages.PrepareForUnstage.Request | ||
| : NamedPipeMessages.PrepareForUnstage.Request + "|" + pathspec; | ||
|
|
||
| bool succeeded = false; | ||
| string failureMessage = null; | ||
|
|
||
| try | ||
| { | ||
| using (NamedPipeClient pipeClient = new NamedPipeClient(enlistmentPipename)) | ||
| { | ||
| if (pipeClient.Connect()) | ||
| { | ||
| pipeClient.SendRequest(message); | ||
| string rawResponse = pipeClient.ReadRawResponse(); | ||
| if (rawResponse != null && rawResponse.StartsWith(NamedPipeMessages.PrepareForUnstage.SuccessResult)) | ||
| { | ||
| succeeded = true; | ||
| } | ||
| else | ||
| { | ||
| failureMessage = "GVFS mount process returned failure for PrepareForUnstage."; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| failureMessage = "Unable to connect to GVFS mount process."; | ||
| } | ||
| } | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| failureMessage = "Exception communicating with GVFS: " + e.Message; | ||
| } | ||
|
|
||
| if (!succeeded && failureMessage != null) | ||
| { | ||
| ExitWithError( | ||
| failureMessage, | ||
| "The unstage operation cannot safely proceed because GVFS was unable to", | ||
| "prepare the staged files. This could lead to index corruption.", | ||
| "", | ||
| "To resolve:", | ||
| " 1. Run 'gvfs unmount' and 'gvfs mount' to reset the GVFS state", | ||
| " 2. Retry the restore --staged command", | ||
| "If the problem persists, run 'gvfs repair' or re-clone the enlistment."); | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MEDIUM:
--pathspec-from-fileignored, falls back to all-staged scan (2/4 models agree)GetRestorePathspec()skips all--prefixed args without interpreting them.git restore --staged --pathspec-from-file=list.txtis treated as no pathspec, which downstream means "all staged files" (DiffCachedNameStatus(null)). On the OS repo, a narrowly-scoped unstage becomes a full-index walk.Recommendation: Explicitly support
--pathspec-from-file/--pathspec-file-nul, or fail closed with an error when those options are detected.