diff --git a/GVFS/GVFS.FunctionalTests.LockHolder/Program.cs b/GVFS/GVFS.FunctionalTests.LockHolder/Program.cs index ed0693cba3..b8362179c7 100644 --- a/GVFS/GVFS.FunctionalTests.LockHolder/Program.cs +++ b/GVFS/GVFS.FunctionalTests.LockHolder/Program.cs @@ -1,4 +1,6 @@ -using System.CommandLine; +using System; +using System.CommandLine; +using System.Threading; namespace GVFS.FunctionalTests.LockHolder { @@ -6,6 +8,29 @@ public class Program { public static void Main(string[] args) { + if (args.Length > 0 && args[0] == "--wait-for-event") + { + string eventName = Environment.GetEnvironmentVariable("GVFS_TEST_HOOK_WAIT_EVENT_NAME"); + if (string.IsNullOrEmpty(eventName)) + { + Console.Error.WriteLine("The test did not supply the hook event name."); + Environment.Exit(1); + } + + using (EventWaitHandle ready = EventWaitHandle.OpenExisting(eventName + ".Ready")) + using (EventWaitHandle release = EventWaitHandle.OpenExisting(eventName + ".Release")) + { + ready.Set(); + if (!release.WaitOne(TimeSpan.FromSeconds(30))) + { + Console.Error.WriteLine("Timed out before the test released the command hook."); + Environment.Exit(1); + } + } + + return; + } + RootCommand rootCommand = AcquireGVFSLockVerb.BuildRootCommand(); rootCommand.Parse(args).Invoke(); } diff --git a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs index cb7af7571d..af26622b0b 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs @@ -4,6 +4,7 @@ using GVFS.Tests.Should; using NUnit.Framework; using System; +using System.Collections.Generic; using System.Diagnostics; using System.IO; using System.Threading; @@ -48,14 +49,82 @@ public void GitBranch() "remotes/origin/" + Properties.Settings.Default.Commitish); } - [TestCase, Order(4)] - public void GitCommandWaitsWhileAnotherIsRunning() + [TestCase("status")] + [TestCase("for-each-repo")] + [Order(4)] + public void GitCommandWaitsWhileAnotherIsRunning(string command) { - int pid; - GitHelpers.AcquireGVFSLock(this.Enlistment, out pid, resetTimeout: 3000); + string emptyConfigPath = null; + Dictionary environmentVariables = null; + try + { + if (command == "for-each-repo") + { + emptyConfigPath = Path.Combine(this.Enlistment.EnlistmentRoot, "empty-git-config-" + Guid.NewGuid().ToString("N")); + File.WriteAllText(emptyConfigPath, string.Empty); + environmentVariables = new Dictionary + { + { "GIT_CONFIG_GLOBAL", emptyConfigPath }, + { "GIT_CONFIG_NOSYSTEM", "1" }, + { "GIT_CONFIG_COUNT", "0" }, + { "GIT_CONFIG_PARAMETERS", string.Empty }, + }; + + ProcessResult alias = GitProcess.InvokeProcess( + this.Enlistment.RepoRoot, + "config --get alias.for-each-repo", + environmentVariables); + alias.ExitCode.ShouldEqual(1); + alias.Output.ShouldBeEmpty(); + + command = "for-each-repo --config=gvfs.test-lock-policy-repos-" + Guid.NewGuid().ToString("N"); + } - ProcessResult statusWait = GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, "status", removeWaitingMessages: false); - statusWait.Errors.ShouldContain(ExpectedStatusWaitingText); + int pid; + GitHelpers.AcquireGVFSLock(this.Enlistment, out pid, resetTimeout: 3000); + + ProcessResult result = GitHelpers.InvokeGitAgainstGVFSRepo( + this.Enlistment.RepoRoot, + command, + environmentVariables, + removeWaitingMessages: false); + result.ExitCode.ShouldEqual(0); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + finally + { + if (emptyConfigPath != null) + { + File.Delete(emptyConfigPath); + } + } + } + + [TestCase("worktree list")] + [TestCase("worktree list --porcelain")] + [TestCase("worktree list --porcelain -z")] + [TestCase("annotate HEAD -- Readme.md")] + [TestCase("check-ref-format --branch topic")] + [TestCase("cherry HEAD HEAD")] + [TestCase("count-objects -vH")] + [TestCase("ls-remote --get-url origin")] + [TestCase("range-diff HEAD~1..HEAD HEAD~1..HEAD")] + [TestCase("shortlog -sn HEAD")] + [TestCase("show-branch --list")] + public void ReadOnlyCommandDoesNotWaitForLock(string command) + { + GitHelpers.RunWithGVFSLock( + this.Enlistment, + () => + { + ProcessResult result = GitProcess.InvokeProcess( + this.Enlistment.RepoRoot, + command, + timeoutMs: AcquireGVFSLockTimeout); + + result.ExitCode.ShouldEqual(0); + result.Errors.ShouldNotContain(false, ExpectedStatusWaitingText); + }); } [TestCase, Order(5)] diff --git a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CreatePlaceholderTests.cs b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CreatePlaceholderTests.cs index ccf231e246..01b9ce3517 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CreatePlaceholderTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CreatePlaceholderTests.cs @@ -1,7 +1,10 @@ using GVFS.FunctionalTests.Properties; using GVFS.FunctionalTests.Should; using GVFS.FunctionalTests.Tools; +using GVFS.Tests.Should; using NUnit.Framework; +using System; +using System.Diagnostics; using System.IO; using System.Runtime.InteropServices; using System.Threading; @@ -19,6 +22,18 @@ public CreatePlaceholderTests(Settings.ValidateWorkingTreeMode validateWorkingTr { } + [TestCase] + public void AllowsPlaceholderCreationForDefaultAnnotate() + { + ProcessResult result = GitHelpers.InvokeGitAgainstGVFSRepo( + this.Enlistment.RepoRoot, + $"annotate -- \"{GitHelpers.ConvertPathToGitFormat(FileToRead)}\""); + + result.ExitCode.ShouldEqual(0); + result.Errors.ShouldBeEmpty(); + string.IsNullOrWhiteSpace(result.Output).ShouldBeFalse("Annotate did not return file annotations."); + } + [TestCase("check-attr --stdin --all")] [TestCase("check-ignore --stdin")] [TestCase("check-mailmap --stdin")] @@ -44,6 +59,67 @@ public void BlocksPlaceholderCreationWhileGitCommandIsRunning(string commandToRu this.CheckPlaceholderCreation(commandToRun, shouldAllow: false); } + [TestCase("worktree list")] + [TestCase("worktree list --porcelain")] + [TestCase("worktree list --porcelain -z")] + public void AllowsPlaceholderCreationWhileWorktreeListIsRunning(string command) + { + string hookListPath = this.Enlistment.GetVirtualPathTo(".git", "hooks", "post-command.hooks"); + string originalHookList = File.ReadAllText(hookListPath); + string eventName = "GVFS.WorktreeList." + Guid.NewGuid().ToString("N"); + string expectedContent = File.ReadAllText(Path.Combine(this.ControlGitRepo.RootPath, FileToRead)); + + using (EventWaitHandle ready = new EventWaitHandle(false, EventResetMode.ManualReset, eventName + ".Ready")) + using (EventWaitHandle release = new EventWaitHandle(false, EventResetMode.ManualReset, eventName + ".Release")) + using (Process git = new Process()) + { + git.StartInfo = new ProcessStartInfo(Settings.Default.PathToGit, command) + { + WorkingDirectory = this.Enlistment.RepoRoot, + UseShellExecute = false, + RedirectStandardOutput = true, + RedirectStandardError = true, + }; + git.StartInfo.Environment["GVFS_TEST_HOOK_WAIT_EVENT_NAME"] = eventName; + + bool started = false; + try + { + // Pause the post-command chain before GVFS can release an incorrectly acquired lock. + string waitHook = $"\"{GitHelpers.LockHolderCommandPath}\" --wait-for-event"; + File.WriteAllText(hookListPath, waitHook + Environment.NewLine + originalHookList); + + started = git.Start(); + started.ShouldBeTrue(); + bool reachedHook = ready.WaitOne(TimeSpan.FromSeconds(30)); + if (!reachedHook && git.HasExited) + { + Assert.Fail($"Git exited before the wait hook. Exit code: {git.ExitCode}. Errors: {git.StandardError.ReadToEnd()}"); + } + + reachedHook.ShouldBeTrue("Git did not reach the post-command hook."); + + File.ReadAllText(this.Enlistment.GetVirtualPathTo(FileToRead)).ShouldEqual(expectedContent); + + release.Set(); + git.WaitForExit(10000).ShouldBeTrue("Git did not exit after the test released the hook."); + git.ExitCode.ShouldEqual(0); + git.StandardError.ReadToEnd().ShouldBeEmpty(); + } + finally + { + release.Set(); + if (started && !git.WaitForExit(10000)) + { + git.Kill(entireProcessTree: true); + git.WaitForExit(); + } + + File.WriteAllText(hookListPath, originalHookList); + } + } + } + private void CheckPlaceholderCreation(string command, bool shouldAllow) { string eofCharacter = "\x04"; diff --git a/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs b/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs index 48418452d2..3d75ec817e 100644 --- a/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs +++ b/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs @@ -23,7 +23,7 @@ public static class GitHelpers private const string WindowsPathSeparator = "\\"; private const string GitPathSeparator = "/"; - private static string LockHolderCommandPath + public static string LockHolderCommandPath { get { @@ -189,6 +189,46 @@ public static ManualResetEventSlim AcquireGVFSLock( out processId); } + public static void RunWithGVFSLock(GVFSFunctionalTestEnlistment enlistment, Action action) + { + ArgumentNullException.ThrowIfNull(enlistment); + ArgumentNullException.ThrowIfNull(action); + + ProcessStartInfo processInfo = new ProcessStartInfo(LockHolderCommandPath) + { + WorkingDirectory = enlistment.RepoRoot, + UseShellExecute = false, + RedirectStandardInput = true, + RedirectStandardOutput = true, + RedirectStandardError = true, + }; + + using (Process holdingProcess = Process.Start(processInfo)) + { + try + { + enlistment.WaitForLock(LockHolderCommandName) + .ShouldBeTrue("The test lock holder did not acquire the GVFS lock."); + action(); + } + finally + { + holdingProcess.StandardInput.WriteLine("done"); + holdingProcess.StandardInput.Close(); + + bool exited = holdingProcess.WaitForExit(10000); + if (!exited) + { + holdingProcess.Kill(entireProcessTree: true); + holdingProcess.WaitForExit(); + } + + exited.ShouldBeTrue("The lock holder did not exit after the test released it."); + holdingProcess.ExitCode.ShouldEqual(0); + } + } + } + /// /// Run the specified Git command. This method will return once the GVFSLock has been acquired. /// diff --git a/GVFS/GVFS.Hooks/Program.CommandLocking.cs b/GVFS/GVFS.Hooks/Program.CommandLocking.cs new file mode 100644 index 0000000000..f759ae75e4 --- /dev/null +++ b/GVFS/GVFS.Hooks/Program.CommandLocking.cs @@ -0,0 +1,119 @@ +using GVFS.Common; +using System.Linq; + +namespace GVFS.Hooks +{ + public partial class Program + { + // The lock serializes changes to the enlistment's index and tracked working tree with GVFS projection updates. + // Before adding an exemption, check all supported options, subcommands, and child-command paths. + // Ref and object writes alone do not require this lock. Index refreshes do. + // Keep arbitrary Git dispatchers locked because nested command hooks can be suppressed. + // Excluded readers can observe concurrent working-tree changes. They do not receive snapshot isolation. + internal static bool ShouldLock(string[] args) + { + string gitCommand = GetGitCommand(args); + + switch (gitCommand) + { + // Keep these alphabetically sorted + case "annotate": + case "blame": + case "branch": + case "cat-file": + case "check-attr": + case "check-ignore": + case "check-mailmap": + case "check-ref-format": + case "cherry": + case "commit-graph": + case "config": + case "count-objects": + case "credential": + case "diff": + case "diff-files": + case "diff-index": + case "diff-tree": + case "difftool": + case "fetch": + case "for-each-ref": + case "help": + case "hash-object": + case "index-pack": + case "log": + case "ls-files": + case "ls-remote": + case "ls-tree": + case "merge-base": + case "multi-pack-index": + case "name-rev": + case "pack-objects": + case "push": + case "range-diff": + case "remote": + case "rev-list": + case "rev-parse": + case "shortlog": + case "show": + case "show-branch": + case "show-ref": + case "symbolic-ref": + case "tag": + case "unpack-objects": + case "update-ref": + case "verify-commit": + case "verify-pack": + case "verify-tag": + case "version": + case "web--browse": + return false; + + case "worktree": + return WorktreeCommandParser.GetSubcommand(args) != "list"; + + /* + * There are several git commands that are "unsupported" in virtualized (VFS4G) + * enlistments that are blocked by git. Usually, these are blocked before they acquire + * a GVFSLock, but the submodule command is different, and is blocked after acquiring the + * GVFS lock. This can cause issues if another action is attempting to create placeholders. + * As we know the submodule command is a no-op, allow it to proceed without acquiring the + * GVFSLock. Issue #1164 documents the need for git to block all unsupported commands + * before calling the pre-command hook. + */ + case "submodule": + return false; + } + + if (gitCommand == "reset" && args.Contains("--soft")) + { + return false; + } + + if (!KnownGitCommands.Contains(gitCommand) && + IsAlias(gitCommand)) + { + return false; + } + + return true; + } + + private static string GetGitCommand(string[] args) + { + string command = args[1].ToLowerInvariant(); + if (command.StartsWith("git-")) + { + command = command.Substring(4); + } + + return command; + } + + private static bool IsAlias(string command) + { + ProcessResult result = ProcessHelper.Run("git", "config --get alias." + command); + + return !string.IsNullOrEmpty(result.Output); + } + } +} diff --git a/GVFS/GVFS.Hooks/Program.cs b/GVFS/GVFS.Hooks/Program.cs index 940b2cf37d..49d72af064 100644 --- a/GVFS/GVFS.Hooks/Program.cs +++ b/GVFS/GVFS.Hooks/Program.cs @@ -425,103 +425,11 @@ private static bool IsGitEnvVarDisabled(string envVar) return false; } - private static bool ShouldLock(string[] args) - { - string gitCommand = GetGitCommand(args); - - switch (gitCommand) - { - // Keep these alphabetically sorted - case "blame": - case "branch": - case "cat-file": - case "check-attr": - case "check-ignore": - case "check-mailmap": - case "commit-graph": - case "config": - case "credential": - case "diff": - case "diff-files": - case "diff-index": - case "diff-tree": - case "difftool": - case "fetch": - case "for-each-ref": - case "help": - case "hash-object": - case "index-pack": - case "log": - case "ls-files": - case "ls-tree": - case "merge-base": - case "multi-pack-index": - case "name-rev": - case "pack-objects": - case "push": - case "remote": - case "rev-list": - case "rev-parse": - case "show": - case "show-ref": - case "symbolic-ref": - case "tag": - case "unpack-objects": - case "update-ref": - case "version": - case "web--browse": - return false; - - /* - * There are several git commands that are "unsupported" in virtualized (VFS4G) - * enlistments that are blocked by git. Usually, these are blocked before they acquire - * a GVFSLock, but the submodule command is different, and is blocked after acquiring the - * GVFS lock. This can cause issues if another action is attempting to create placeholders. - * As we know the submodule command is a no-op, allow it to proceed without acquiring the - * GVFSLock. I have filed issue #1164 to track having git block all unsupported commands - * before calling the pre-command hook. - */ - case "submodule": - return false; - } - - if (gitCommand == "reset" && args.Contains("--soft")) - { - return false; - } - - if (!KnownGitCommands.Contains(gitCommand) && - IsAlias(gitCommand)) - { - return false; - } - - return true; - } - private static string GetHookType(string[] args) { return args[0].ToLowerInvariant(); } - private static string GetGitCommand(string[] args) - { - string command = args[1].ToLowerInvariant(); - if (command.StartsWith("git-")) - { - command = command.Substring(4); - } - - return command; - } - - private static bool IsAlias(string command) - { - ProcessResult result = ProcessHelper.Run("git", "config --get alias." + command); - - return !string.IsNullOrEmpty(result.Output); - } - /// /// Extracts the git exit code from hook args. Git appends --exit_code=N /// to post-command hook arguments. Returns null if the argument is diff --git a/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj b/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj index 0517e82f15..1d88885cf4 100644 --- a/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj +++ b/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj @@ -27,6 +27,12 @@ + + Hooks\KnownGitCommands.cs + + + Hooks\Program.CommandLocking.cs + Hooks\UnstageCommandParser.cs @@ -57,4 +63,3 @@ - diff --git a/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs b/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs new file mode 100644 index 0000000000..6b198795b3 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs @@ -0,0 +1,140 @@ +using GVFS.Tests.Should; +using NUnit.Framework; +using System.Linq; + +namespace GVFS.UnitTests.Hooks +{ + [TestFixture] + public class CommandLockingTests + { + [TestCase("annotate")] + [TestCase("annotate --contents mock-file HEAD")] + [TestCase("git-annotate")] + [TestCase("blame")] + [TestCase("branch")] + [TestCase("cat-file")] + [TestCase("check-attr")] + [TestCase("check-ignore")] + [TestCase("check-mailmap")] + [TestCase("check-ref-format --branch topic")] + [TestCase("check-ref-format --normalize refs/heads/topic")] + [TestCase("check-ref-format --refspec-pattern refs/heads/*")] + [TestCase("cherry -v HEAD~2 HEAD HEAD~1")] + [TestCase("commit-graph")] + [TestCase("config")] + [TestCase("count-objects")] + [TestCase("count-objects -vH")] + [TestCase("credential")] + [TestCase("diff")] + [TestCase("diff-files")] + [TestCase("diff-index")] + [TestCase("diff-tree")] + [TestCase("difftool")] + [TestCase("fetch")] + [TestCase("for-each-ref")] + [TestCase("help")] + [TestCase("hash-object")] + [TestCase("index-pack")] + [TestCase("log")] + [TestCase("ls-files")] + [TestCase("ls-remote --symref origin")] + [TestCase("ls-remote --get-url origin")] + [TestCase("ls-tree")] + [TestCase("merge-base")] + [TestCase("multi-pack-index")] + [TestCase("name-rev")] + [TestCase("pack-objects")] + [TestCase("push")] + [TestCase("range-diff HEAD~2..HEAD~1 HEAD~1..HEAD")] + [TestCase("remote")] + [TestCase("reset --soft")] + [TestCase("rev-list")] + [TestCase("rev-parse")] + [TestCase("shortlog -sn HEAD")] + [TestCase("show")] + [TestCase("show-branch --list")] + [TestCase("show-branch --merge-base")] + [TestCase("show-branch --reflog")] + [TestCase("show-ref")] + [TestCase("submodule")] + [TestCase("symbolic-ref")] + [TestCase("tag")] + [TestCase("unpack-objects")] + [TestCase("update-ref")] + [TestCase("verify-commit --raw -v HEAD")] + [TestCase("verify-pack -v mock-pack.idx")] + [TestCase("verify-pack -s mock-pack.idx")] + [TestCase("verify-tag --raw -v mock-tag")] + [TestCase("verify-tag --format=%(refname) mock-tag")] + [TestCase("version")] + [TestCase("web--browse")] + [TestCase("worktree list")] + [TestCase("worktree list --porcelain")] + [TestCase("worktree list --porcelain -z")] + [TestCase("worktree list --verbose --expire=now")] + [TestCase("git-worktree list")] + [TestCase("WORKTREE LIST")] + [TestCase("worktree --git-pid=123 list")] + public void CommandsWithoutLockPreserveTheirPolicy(string command) + { + AssertLockPolicy(command, shouldLock: false); + } + + [TestCase("add")] + [TestCase("apply")] + [TestCase("checkout")] + [TestCase("checkout-index")] + [TestCase("cherry-pick")] + [TestCase("clean")] + [TestCase("commit")] + [TestCase("describe")] + [TestCase("describe --dirty")] + [TestCase("describe --broken")] + [TestCase("grep --open-files-in-pager=mock-pager pattern")] + [TestCase("merge")] + [TestCase("mv")] + [TestCase("pull")] + [TestCase("read-tree")] + [TestCase("rebase")] + [TestCase("reset")] + [TestCase("reset --mixed")] + [TestCase("reset --hard")] + [TestCase("restore")] + [TestCase("revert")] + [TestCase("rm")] + [TestCase("status")] + [TestCase("status --no-lock-index")] + [TestCase("update-index")] + [TestCase("worktree")] + [TestCase("worktree --git-pid=123")] + [TestCase("worktree add -b branch list")] + [TestCase("worktree remove list")] + [TestCase("worktree move list new-path")] + [TestCase("worktree lock list")] + [TestCase("worktree unlock list")] + [TestCase("worktree prune")] + [TestCase("worktree repair")] + [TestCase("worktree unknown list")] + [TestCase("git-worktree add list")] + [TestCase("write-tree")] + public void CommandsWithLockPreserveTheirPolicy(string command) + { + AssertLockPolicy(command, shouldLock: true); + } + + private static void AssertLockPolicy(string command, bool shouldLock) + { + foreach (string hook in new[] { "pre-command", "post-command" }) + { + string[] args = new[] { hook } + .Concat(command.Split(' ')) + .Concat(hook == "post-command" + ? new[] { "--git-pid=456", "--exit_code=0" } + : new[] { "--git-pid=456" }) + .ToArray(); + + GVFS.Hooks.Program.ShouldLock(args).ShouldEqual(shouldLock, string.Join(" ", args)); + } + } + } +}