From d1744cb87d11928956cff2cdf36406baafe39055 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Mon, 19 Oct 2026 14:49:18 -0700 Subject: [PATCH 1/3] Preserve GVFS locks when commands collide with aliases Git runs the pre-command hook with the name of a built-in command, or with git- when it will run a program or expand an alias. The hook used a static command inventory and an alias. config query to skip the lock for aliases. The inventory omitted built-ins such as stash and did not know custom programs. So a same-name alias could skip the lock for a command that Git ran as a built-in or a program. The read-only exemption list also applied to git-, so a program such as git-LOG on PATH ran without a lock. The config query also missed [alias ""] command. Git 2.56.0.vfs.0.4 does not run post-command for git- before it expands an alias. So an outer lock on a genuine alias blocks the first locking command of that alias until the user cancels. Decide from the raw hook argument: - Apply the read-only exemption list only to built-ins, and to Git's git-submodule and git-web--browse programs. - For git-, look for a program as Git for Windows does in path_lookup and lookup_prog. Use each raw PATH entry. Check "\git-.exe" with and without ".exe" through GetFileAttributesW, which fails for the same files as _waccess. - If Git will run a program, take the lock. - Otherwise, read the alias config keys with the parent git.exe (git config --name-only --get-regexp ^alias\.). Suppress recursive command hooks, decode the output as UTF-8, and retry once on a transient config access error. Match the keys as Git does: alias., alias.., and dotted names for any ASCII case, and alias..command only for the exact name. Skip the outer lock only when a key matches. - Stop the command with an error when GVFS cannot tell: the alias keys or the path of the parent git.exe cannot be read, or a PATH candidate does not fit Git's MAX_PATH buffer. In post-command for git-, release the lock only when the parent Git process owns it. Do not look up aliases or PATH again. If the mount is not reachable, return without an error, because only the mount holds locks. Use availability-only locking only for the built-in status, not for a git-status program. Remove the static command inventory. Tell the user to run gvfs status in the new lock-ownership errors. Document how aliases interact with command locks. Add unit tests for built-in and external names, programs found on PATH with Git's path rules, PATH candidates that do not fit Git's buffer, the case rules of each alias form, failed alias reads, the read-only list for git-, and lock-ownership responses. Add functional tests that run real Git with a lock holder for built-in and external collisions, both alias forms, an alias from a config file, a subsection alias with capital letters, nested aliases, command spellings, directories and invalid entries on PATH. The tests check that a script runs when it collides with an alias. Two functional tests call the hooks directly with a live Git PID. One deletes the external program between pre-command and post-command. The other locks the global config and checks that pre-command stops the command without a lock. Assisted-by: GPT-6.1 Sol Assisted-by: Claude Opus 5.5 Signed-off-by: Tyrie Vella --- .../GitReadAndGitLockTests.cs | 457 ++++++++++++++++++ GVFS/GVFS.Hooks/GitCommandLock.cs | 70 +++ GVFS/GVFS.Hooks/GitCommandResolver.cs | 255 ++++++++++ GVFS/GVFS.Hooks/KnownGitCommands.cs | 150 ------ GVFS/GVFS.Hooks/Program.CommandLocking.cs | 58 ++- GVFS/GVFS.Hooks/Program.cs | 71 ++- GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj | 7 +- .../Hooks/CommandLockingTests.cs | 94 +++- .../Hooks/GitCommandLockTests.cs | 99 ++++ .../Hooks/GitCommandResolverTests.cs | 380 +++++++++++++++ docs/faq.md | 18 + 11 files changed, 1474 insertions(+), 185 deletions(-) create mode 100644 GVFS/GVFS.Hooks/GitCommandLock.cs create mode 100644 GVFS/GVFS.Hooks/GitCommandResolver.cs delete mode 100644 GVFS/GVFS.Hooks/KnownGitCommands.cs create mode 100644 GVFS/GVFS.UnitTests/Hooks/GitCommandLockTests.cs create mode 100644 GVFS/GVFS.UnitTests/Hooks/GitCommandResolverTests.cs diff --git a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs index af26622b0b..e615cb7ab4 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs @@ -7,6 +7,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.IO; +using System.Text; using System.Threading; namespace GVFS.FunctionalTests.Tests.EnlistmentPerFixture @@ -200,5 +201,461 @@ public void OrphanedGVFSLockIsCleanedUp() // an error about "Waiting for GVFS.FunctionalTests.LockHolder" statusWait.Errors.ShouldEqual(string.Empty); } + + [TestCase("stash", "list")] + [TestCase("stash.exe", "list")] + [TestCase("for-each-repo", "--config=gvfs.tests.alias-repositories status --porcelain")] + [TestCase("checkout", "--detach HEAD")] + [Order(9)] + public void BuiltInWithSameNameAliasStillAcquiresLock(string command, string arguments) + { + string aliasName = command.EndsWith(".exe", StringComparison.Ordinal) ? Path.GetFileNameWithoutExtension(command) : command; + string repositoryConfig = command == "for-each-repo" + ? "-c \"gvfs.tests.alias-repositories=" + this.Enlistment.RepoRoot + "\" " + : string.Empty; + ProcessResult result = this.RunWithLockHolder(repositoryConfig + "-c alias." + aliasName + "=version " + command + " " + arguments); + result.ExitCode.ShouldEqual(0); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + + [TestCase("gvfs-lock-outer-alias", "status --porcelain")] + [TestCase("gvfs-lock-outer-alias", "!git status --porcelain")] + [TestCase("gvfs-lock-outer-alias", "gvfs-lock-inner-alias")] + [TestCase("MiXeD-lock-alias", "status --porcelain")] + [TestCase("git-prefixed-lock-alias", "status --porcelain")] + [Order(10)] + public void AliasInnerCommandStillAcquiresLock(string aliasName, string expansion) + { + string command = "-c alias.gvfs-lock-inner-alias=status " + + "-c \"alias." + aliasName + "=" + expansion + "\" " + aliasName; + ProcessResult result = this.RunWithLockHolder(command); + result.ExitCode.ShouldEqual(0); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + + [TestCase] + [Order(11)] + public void ReadOnlyAliasDoesNotAcquireAnOuterLock() + { + ProcessResult result = this.RunWithLockHolder( + "-c \"alias.gvfs-read-only-alias=!git rev-parse --show-toplevel\" gvfs-read-only-alias", + shouldWait: false); + result.ExitCode.ShouldEqual(0); + result.Output.ShouldContain(this.Enlistment.RepoRoot.Replace('\\', '/')); + result.Errors.Contains(ExpectedStatusWaitingText).ShouldBeFalse(); + } + + [TestCase] + [Order(12)] + public void UnknownExternalCommandStillAcquiresLock() + { + ProcessResult result = this.RunWithLockHolder("-c help.autocorrect=never gvfs-unknown-lock-command"); + result.ExitCode.ShouldEqual(1); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + + [TestCase("git-", false, false)] + [TestCase("GIT-", true, false)] + [TestCase("git-", false, true)] + [Order(13)] + public void ExternalWithSameNameAliasStillAcquiresLock(string prefix, bool uppercaseCommand, bool useExecPath) + { + const string CommandName = "gvfs-lock-external"; + string directory = Path.Combine(this.Enlistment.EnlistmentRoot, nameof(this.ExternalWithSameNameAliasStillAcquiresLock)); + Directory.CreateDirectory(directory); + string scriptPath = Path.Combine(directory, prefix + CommandName); + File.WriteAllText(scriptPath, "#!/bin/sh\necho external-script\nexec git status --porcelain\n"); + try + { + Dictionary environment = new Dictionary(); + if (useExecPath) + { + environment["GIT_EXEC_PATH"] = directory; + } + else + { + environment["PATH"] = directory + Path.PathSeparator + Environment.GetEnvironmentVariable("PATH"); + } + + string invocation = uppercaseCommand ? CommandName.ToUpperInvariant() : CommandName; + ProcessResult result = this.RunWithLockHolder("-c alias." + CommandName + "=version " + invocation, environment); + result.ExitCode.ShouldEqual(0); + result.Output.ShouldContain("external-script"); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + finally + { + File.Delete(scriptPath); + Directory.Delete(directory); + } + } + + [TestCase] + [Order(14)] + public void ExternalExecutableWithSameNameAliasStillAcquiresLock() + { + const string CommandName = "gvfs-lock-executable"; + string directory = Path.Combine(this.Enlistment.EnlistmentRoot, nameof(this.ExternalExecutableWithSameNameAliasStillAcquiresLock)); + Directory.CreateDirectory(directory); + string executablePath = Path.Combine(directory, "GIT-" + CommandName + ".EXE"); + File.Copy(Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.System), "cmd.exe"), executablePath); + try + { + Dictionary environment = new Dictionary(); + environment["PATH"] = directory + Path.PathSeparator + Environment.GetEnvironmentVariable("PATH"); + ProcessResult result = this.RunWithLockHolder("-c alias." + CommandName + "=version " + CommandName + " /d /c echo external-command", environment); + result.ExitCode.ShouldEqual(0); + result.Output.ShouldContain("external-command"); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + finally + { + File.Delete(executablePath); + Directory.Delete(directory); + } + } + + [TestCase] + [Order(15)] + public void DirectoryNamedAfterAnExternalDoesNotHideAnAlias() + { + const string CommandName = "gvfs-directory-alias"; + string directory = Path.Combine(this.Enlistment.EnlistmentRoot, nameof(this.DirectoryNamedAfterAnExternalDoesNotHideAnAlias)); + string commandDirectory = Path.Combine(directory, "git-" + CommandName); + Directory.CreateDirectory(commandDirectory); + try + { + Dictionary environment = new Dictionary(); + environment["PATH"] = directory + Path.PathSeparator + Environment.GetEnvironmentVariable("PATH"); + ProcessResult result = this.RunWithLockHolder( + "-c alias." + CommandName + "=version " + CommandName, + environment, + shouldWait: false); + result.ExitCode.ShouldEqual(0); + result.Output.ShouldContain("git version"); + } + finally + { + Directory.Delete(commandDirectory); + Directory.Delete(directory); + } + } + + [TestCase] + [Order(16)] + public void PostCommandReleasesOwnedLockAfterExternalDisappears() + { + const string CommandName = "gvfs-lock-race"; + string directory = Path.Combine(this.Enlistment.EnlistmentRoot, nameof(this.PostCommandReleasesOwnedLockAfterExternalDisappears)); + Directory.CreateDirectory(directory); + string executablePath = Path.Combine(directory, "git-" + CommandName); + File.WriteAllText(executablePath, "#!/bin/sh\nexit 99\n"); + string hooksExecutable = Path.Combine(Path.GetDirectoryName(Properties.Settings.Default.PathToGVFS), "GVFS.Hooks.exe"); + Dictionary environment = new Dictionary(); + environment["PATH"] = directory + Path.PathSeparator + Environment.GetEnvironmentVariable("PATH"); + environment["GIT_CONFIG_COUNT"] = "1"; + environment["GIT_CONFIG_KEY_0"] = "alias." + CommandName; + environment["GIT_CONFIG_VALUE_0"] = "checkout --detach HEAD"; + + // The test calls both hooks directly, so it does not depend on Git running post-command + // for git-. + // Keep the Git PID alive so orphan cleanup cannot hide a skipped release. + using (Process git = new Process()) + { + git.StartInfo = this.CreateGitStartInfo("hash-object --stdin", environment); + git.StartInfo.RedirectStandardInput = true; + bool started = false; + try + { + started = git.Start(); + started.ShouldBeTrue(); + string commandArguments = " git-" + CommandName + " --git-pid=" + git.Id; + ProcessStartInfo hook = this.CreateGitStartInfo("pre-command" + commandArguments, environment); + hook.FileName = hooksExecutable; + ProcessResult preCommand = ProcessHelper.Run(hook, timeoutMs: 30000); + preCommand.ExitCode.ShouldEqual(0, preCommand.Output + preCommand.Errors); + this.Enlistment.Status().ShouldContain("GVFS Lock: Held by git git-" + CommandName + " (PID:" + git.Id + ")"); + + File.Delete(executablePath); + hook.Arguments = "post-command" + commandArguments + " --exit_code=1"; + ProcessResult postCommand = ProcessHelper.Run(hook, timeoutMs: 30000); + postCommand.ExitCode.ShouldEqual(0, postCommand.Output + postCommand.Errors); + git.HasExited.ShouldBeFalse(); + this.Enlistment.Status().ShouldContain("GVFS Lock: Free"); + } + finally + { + if (started) + { + git.StandardInput.Close(); + if (!git.WaitForExit(10000)) + { + git.Kill(entireProcessTree: true); + git.WaitForExit(); + Assert.Fail("The parent Git process did not exit."); + } + } + + File.Delete(executablePath); + Directory.Delete(directory); + } + } + } + + [TestCase("\"C:\\invalid quoted path\"")] + [TestCase("C:\\invalid|path")] + [Order(17)] + public void InvalidPathEntryDoesNotPreventAGenuineAlias(string pathEntry) + { + Dictionary environment = new Dictionary(); + environment["PATH"] = pathEntry + Path.PathSeparator + Environment.GetEnvironmentVariable("PATH"); + ProcessResult result = this.RunWithLockHolder( + "-c alias.gvfs-invalid-path-alias=version gvfs-invalid-path-alias", + environment, + shouldWait: false); + result.ExitCode.ShouldEqual(0); + result.Output.ShouldContain("git version"); + } + + [TestCase] + [Order(18)] + public void SubsectionAliasInnerCommandStillAcquiresLock() + { + ProcessResult result = this.RunWithLockHolder( + "-c \"alias.gvfs-subsection-alias.command=status --porcelain\" gvfs-subsection-alias"); + result.ExitCode.ShouldEqual(0); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + + [TestCase] + [Order(19)] + public void ConfigFileAliasInnerCommandStillAcquiresLock() + { + const string AliasName = "gvfs-config-file-alias"; + GitHelpers.CheckGitCommandAgainstGVFSRepo(this.Enlistment.RepoRoot, "config --local alias." + AliasName + " \"status --porcelain\""); + try + { + ProcessResult result = this.RunWithLockHolder(AliasName); + result.ExitCode.ShouldEqual(0); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + finally + { + GitHelpers.CheckGitCommandAgainstGVFSRepo(this.Enlistment.RepoRoot, "config --local --unset alias." + AliasName); + } + } + + [TestCase] + [Order(20)] + public void SubsectionAliasWithCapitalLettersSkipsOnlyTheOuterLock() + { + ProcessResult result = this.RunWithLockHolder( + "-c \"alias.GVFS-Subsection-Alias.command=status --porcelain\" GVFS-Subsection-Alias"); + result.ExitCode.ShouldEqual(0); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + + [TestCase] + [Order(21)] + public void UnreadableAliasConfigStopsTheCommandBeforeTheLock() + { + string hooksExecutable = Path.Combine(Path.GetDirectoryName(Properties.Settings.Default.PathToGVFS), "GVFS.Hooks.exe"); + string configPath = Path.Combine(this.Enlistment.EnlistmentRoot, nameof(this.UnreadableAliasConfigStopsTheCommandBeforeTheLock) + ".gitconfig"); + File.WriteAllText(configPath, "[alias]\n\tgvfs-unreadable = status\n"); + + // The hook reads aliases with the parent Git's executable, so keep a Git process alive. + using (Process git = new Process()) + { + git.StartInfo = this.CreateGitStartInfo("hash-object --stdin"); + git.StartInfo.RedirectStandardInput = true; + bool started = false; + try + { + started = git.Start(); + started.ShouldBeTrue(); + Dictionary environment = new Dictionary(); + environment["GIT_CONFIG_GLOBAL"] = configPath; + ProcessStartInfo hook = this.CreateGitStartInfo("pre-command git-gvfs-unreadable --git-pid=" + git.Id, environment); + hook.FileName = hooksExecutable; + + ProcessResult preCommand; + using (new FileStream(configPath, FileMode.Open, FileAccess.Read, FileShare.None)) + { + preCommand = ProcessHelper.Run(hook, timeoutMs: 30000); + } + + preCommand.ExitCode.ShouldEqual(1, preCommand.Output + preCommand.Errors); + preCommand.Output.ShouldContain( + "Unable to read Git aliases (exit code 128).", + "GVFS cannot tell if 'git gvfs-unreadable' runs an alias or a program, so it stopped the command."); + this.Enlistment.Status().ShouldContain("GVFS Lock: Free"); + } + finally + { + if (started) + { + git.StandardInput.Close(); + if (!git.WaitForExit(10000)) + { + git.Kill(entireProcessTree: true); + git.WaitForExit(); + } + } + + File.Delete(configPath); + } + } + } + + private ProcessStartInfo CreateGitStartInfo(string command, Dictionary environment = null) + { + ProcessStartInfo startInfo = new ProcessStartInfo(Properties.Settings.Default.PathToGit, command); + startInfo.WorkingDirectory = this.Enlistment.RepoRoot; + startInfo.UseShellExecute = false; + startInfo.RedirectStandardOutput = true; + startInfo.RedirectStandardError = true; + startInfo.Environment["GIT_TERMINAL_PROMPT"] = "0"; + if (environment != null) + { + foreach (KeyValuePair entry in environment) + { + startInfo.Environment[entry.Key] = entry.Value; + } + } + + return startInfo; + } + + private ProcessResult RunWithLockHolder(string command, Dictionary environment = null, bool shouldWait = true) + { + using (Process holder = new Process()) + using (Process git = new Process()) + using (ManualResetEventSlim waiting = new ManualResetEventSlim(false)) + { + holder.StartInfo = new ProcessStartInfo(GitHelpers.LockHolderCommandPath) + { + WorkingDirectory = this.Enlistment.RepoRoot, + UseShellExecute = false, + RedirectStandardInput = true, + RedirectStandardOutput = true, + RedirectStandardError = true, + }; + holder.Start().ShouldBeTrue(); + bool gitStarted = false; + bool completed = false; + bool released = false; + StringBuilder output = new StringBuilder(); + StringBuilder errors = new StringBuilder(); + Thread errorReader = null; + try + { + this.Enlistment.WaitForLock("GVFS.FunctionalTests.LockHolder (PID:" + holder.Id + ")") + .ShouldBeTrue("The expected lock-holder process did not acquire the lock."); + git.StartInfo = this.CreateGitStartInfo(command, environment); + git.OutputDataReceived += (sender, args) => + { + if (args.Data != null) + { + output.AppendLine(args.Data); + } + }; + errorReader = new Thread(() => + { + int character; + while ((character = git.StandardError.Read()) != -1) + { + errors.Append((char)character); + if (!waiting.IsSet && errors.ToString().Contains(ExpectedStatusWaitingText)) + { + waiting.Set(); + } + } + }); + gitStarted = git.Start(); + gitStarted.ShouldBeTrue(); + git.BeginOutputReadLine(); + errorReader.Start(); + + if (shouldWait) + { + waiting.Wait(AcquireGVFSLockTimeout).ShouldBeTrue("Git did not wait for the lock holder."); + this.ReleaseLockHolder(holder); + released = true; + } + + git.WaitForExit(30000).ShouldBeTrue("Git did not finish within the test timeout."); + git.WaitForExit(); + errorReader.Join(10000).ShouldBeTrue("Git stderr did not close."); + if (!shouldWait) + { + holder.HasExited.ShouldBeFalse(); + this.Enlistment.Status().ShouldContain("GVFS.FunctionalTests.LockHolder (PID:" + holder.Id + ")"); + } + + completed = true; + return new ProcessResult(output.ToString(), errors.ToString(), git.ExitCode); + } + finally + { + // Clean up Git and its reader even if the holder release fails. After a test + // failure, report that failure, not a cleanup failure. + try + { + if (!released) + { + this.ReleaseLockHolder(holder, verify: completed); + } + } + finally + { + if (gitStarted && !git.WaitForExit(10000)) + { + git.Kill(entireProcessTree: true); + git.WaitForExit(); + } + + if (errorReader != null && (errorReader.ThreadState & System.Threading.ThreadState.Unstarted) == 0) + { + bool readerClosed = errorReader.Join(10000); + if (completed) + { + readerClosed.ShouldBeTrue("Git stderr did not close during cleanup."); + } + } + } + } + } + } + + private void ReleaseLockHolder(Process holder, bool verify = true) + { + if (!holder.HasExited) + { + try + { + holder.StandardInput.WriteLine("done"); + holder.StandardInput.Close(); + } + catch (IOException) when (!verify || holder.HasExited) + { + // The holder already exited, or cleanup after a failed test is best effort. + } + + if (!holder.WaitForExit(10000)) + { + holder.Kill(entireProcessTree: true); + holder.WaitForExit(); + if (verify) + { + Assert.Fail("The lock-holder process did not exit."); + } + } + } + + if (verify) + { + holder.ExitCode.ShouldEqual(0, holder.StandardError.ReadToEnd()); + } + } } } diff --git a/GVFS/GVFS.Hooks/GitCommandLock.cs b/GVFS/GVFS.Hooks/GitCommandLock.cs new file mode 100644 index 0000000000..39cc6f8a73 --- /dev/null +++ b/GVFS/GVFS.Hooks/GitCommandLock.cs @@ -0,0 +1,70 @@ +using GVFS.Common.NamedPipes; +using System; + +namespace GVFS.Hooks +{ + internal static class GitCommandLock + { + private const string CheckMount = "Run 'gvfs status' to check the mount."; + + public static bool TryCheckOwnership( + int pid, + bool isElevated, + string fullCommand, + Func sendRequest, + out bool ownsLock, + out string error) + { + ownsLock = false; + error = null; + NamedPipeMessages.LockRequest request = new NamedPipeMessages.LockRequest( + pid, isElevated, checkAvailabilityOnly: true, fullCommand, gitCommandSessionId: string.Empty); + NamedPipeMessages.Message message = sendRequest(request.CreateMessage(NamedPipeMessages.AcquireLock.AcquireRequest)); + if (message == null) + { + error = "GVFS returned no lock-ownership response. " + CheckMount; + return false; + } + + NamedPipeMessages.AcquireLock.Response response; + try + { + response = new NamedPipeMessages.AcquireLock.Response(message); + } + catch (Exception ex) when (ex is InvalidOperationException || ex is ArgumentException) + { + error = "GVFS returned an invalid lock-ownership response: " + ex.Message + " " + CheckMount; + return false; + } + + switch (response.Result) + { + case NamedPipeMessages.AcquireLock.AvailableResult: + case NamedPipeMessages.AcquireLock.DenyGVFSResult: + return true; + + case NamedPipeMessages.AcquireLock.DenyGitResult: + if (response.ResponseData == null || response.ResponseData.PID <= 0) + { + error = "GVFS returned a lock owner without a valid process ID. " + CheckMount; + return false; + } + + ownsLock = response.ResponseData.PID == pid; + return true; + + case NamedPipeMessages.AcquireLock.MountNotReadyResult: + error = "GVFS has not finished initializing. " + CheckMount; + return false; + + case NamedPipeMessages.AcquireLock.UnmountInProgressResult: + error = "GVFS is unmounting. " + CheckMount; + return false; + + default: + error = "GVFS returned an unexpected lock-ownership response: " + response.Result + ". " + CheckMount; + return false; + } + } + } +} diff --git a/GVFS/GVFS.Hooks/GitCommandResolver.cs b/GVFS/GVFS.Hooks/GitCommandResolver.cs new file mode 100644 index 0000000000..c4c6690bf1 --- /dev/null +++ b/GVFS/GVFS.Hooks/GitCommandResolver.cs @@ -0,0 +1,255 @@ +using GVFS.Common; +using System; +using System.ComponentModel; +using System.Diagnostics; +using System.IO; +using System.Runtime.InteropServices; +using System.Text; + +namespace GVFS.Hooks +{ + internal static class GitCommandResolver + { + public const uint InvalidFileAttributes = 0xFFFFFFFF; + public const string UnableToAccess = "unable to access '"; + + private const uint FileAttributeDirectory = 0x10; + + // Git builds each PATH candidate in a char[MAX_PATH] buffer. + private const int GitMaxPath = 260; + + /// + /// Determines whether Git will expand as an alias. Returns + /// false when GVFS cannot tell: PATH is empty, or the alias config keys cannot be read or + /// are incomplete. The caller must not guess, because an outer lock on a genuine alias + /// blocks the commands of that alias. + /// + public static bool TryIsAlias( + string hookCommand, + Func readAliasKeys, + string searchPath, + Func getFileAttributes, + out bool isAlias, + out string error) + { + isAlias = false; + error = null; + + // Git supplies canonical, unprefixed names for built-ins, and git- for external attempts. + if (!hookCommand.StartsWith("git-", StringComparison.Ordinal) || + hookCommand.IndexOfAny(new[] { '\\', '/' }) >= 0) + { + return true; + } + + string command = hookCommand.Substring("git-".Length); + if (string.IsNullOrEmpty(searchPath)) + { + // Git adds its exec-path to PATH before it runs the hook, so this is a defensive check. + error = $"Unable to resolve Git command '{command}': the inherited PATH is empty."; + return false; + } + + if (!TryFindProgramOnPath(hookCommand, searchPath, getFileAttributes, out bool programFound, out error)) + { + return false; + } + + if (programFound) + { + return true; + } + + ProcessResult result; + try + { + result = readAliasKeys(); + } + catch (Exception ex) when (ex is Win32Exception || ex is InvalidOperationException || + ex is ArgumentException || ex is IOException || ex is UnauthorizedAccessException) + { + error = $"Unable to read Git aliases: {FormatError(ex)}"; + return false; + } + + // git config exits with 1 when no key matches. It exits with 128 when it cannot read a + // config file, after warnings that name the file. + bool noAliases = result.ExitCode == 1 && string.IsNullOrEmpty(result.Output) && + (result.Errors ?? string.Empty).IndexOf(UnableToAccess, StringComparison.Ordinal) < 0; + if (result.ExitCode != 0 && !noAliases) + { + error = $"Unable to read Git aliases (exit code {result.ExitCode}). {result.Errors}".TrimEnd(); + return false; + } + + foreach (string key in (result.Output ?? string.Empty).Split('\n')) + { + if (IsAliasKeyFor(key, command)) + { + isAlias = true; + return true; + } + } + + return true; + } + + public static string GetProcessPath(int processId) + { + try + { + using (Process process = Process.GetProcessById(processId)) + { + string path = process.MainModule?.FileName; + if (!string.IsNullOrEmpty(path)) + { + return path; + } + } + } + catch (Exception ex) when (ex is ArgumentException || ex is Win32Exception || ex is InvalidOperationException) + { + throw new InvalidOperationException($"Unable to find the executable of the parent Git process (PID {processId}): {ex.Message}", ex); + } + + throw new InvalidOperationException($"Unable to find the executable of the parent Git process (PID {processId})."); + } + + public static ProcessStartInfo CreateAliasKeyQuery(string gitPath) + { + ProcessStartInfo processInfo = new ProcessStartInfo(gitPath); + processInfo.UseShellExecute = false; + + // The query reads no input. Do not give it the hook's standard input. + processInfo.RedirectStandardInput = true; + processInfo.RedirectStandardOutput = true; + processInfo.RedirectStandardError = true; + + // Git writes config names in UTF-8, whatever the console code page is. + processInfo.StandardOutputEncoding = Encoding.UTF8; + processInfo.StandardErrorEncoding = Encoding.UTF8; + processInfo.ArgumentList.Add("config"); + processInfo.ArgumentList.Add("--name-only"); + processInfo.ArgumentList.Add("--get-regexp"); + processInfo.ArgumentList.Add(@"^alias\."); + processInfo.Environment["COMMAND_HOOK_LOCK"] = "true"; + return processInfo; + } + + public static uint GetFileAttributes(string path) + { + return NativeMethods.GetFileAttributesW(path); + } + + /// + /// Mirrors Git's alias lookup. git config prints "alias.<name>" in lowercase, and Git + /// matches it for any ASCII case. It prints "alias.<subsection>.command" with the + /// subsection case unchanged, and Git matches only that exact subsection. It prints other + /// "alias.<subsection>.<key>" names as dotted alias names, which Git matches for any + /// ASCII case. Git treats an empty subsection ("alias..<name>") as plain [alias]. + /// + private static bool IsAliasKeyFor(string key, string command) + { + const string Section = "alias."; + const string CommandKey = ".command"; + if (!key.StartsWith(Section, StringComparison.Ordinal)) + { + return false; + } + + string name = key.Substring(Section.Length); + if (name.StartsWith(".", StringComparison.Ordinal)) + { + name = name.Substring(1); + } + else if (name.Length > CommandKey.Length && name.EndsWith(CommandKey, StringComparison.Ordinal)) + { + return string.Equals(name.Substring(0, name.Length - CommandKey.Length), command, StringComparison.Ordinal); + } + + return name.Length > 0 && EqualsIgnoringAsciiCase(name, command); + } + + private static bool EqualsIgnoringAsciiCase(string left, string right) + { + if (left.Length != right.Length) + { + return false; + } + + for (int i = 0; i < left.Length; ++i) + { + if (ToAsciiLower(left[i]) != ToAsciiLower(right[i])) + { + return false; + } + } + + return true; + } + + private static char ToAsciiLower(char c) + { + return c >= 'A' && c <= 'Z' ? (char)(c + ('a' - 'A')) : c; + } + + /// + /// Mirrors path_lookup and lookup_prog in Git for Windows (compat/mingw.c). Git checks + /// "<entry>\<command>.exe" for each nonempty PATH entry, then the same path without + /// ".exe". It uses _waccess, which fails for any file that it cannot access. Git builds + /// the path in a char[MAX_PATH] buffer and cuts a longer path. Returns false for a path + /// that Git cuts, because the conversion of a cut path is not reproduced here. + /// + private static bool TryFindProgramOnPath( + string hookCommand, + string searchPath, + Func getFileAttributes, + out bool found, + out string error) + { + found = false; + error = null; + bool isExe = hookCommand.EndsWith(".exe", StringComparison.OrdinalIgnoreCase); + foreach (string directory in searchPath.Split(';')) + { + if (directory.Length == 0) + { + continue; + } + + string path = directory + "\\" + hookCommand + ".exe"; + if (Encoding.UTF8.GetByteCount(path) >= GitMaxPath) + { + error = $"Unable to resolve Git command '{hookCommand.Substring("git-".Length)}': the PATH entry '{directory}' is too long for Git's path buffer."; + return false; + } + + if (!isExe && getFileAttributes(path) != InvalidFileAttributes) + { + found = true; + return true; + } + + uint attributes = getFileAttributes(path.Substring(0, path.Length - ".exe".Length)); + if (attributes != InvalidFileAttributes && (attributes & FileAttributeDirectory) == 0) + { + found = true; + return true; + } + } + + return true; + } + + private static string FormatError(Exception error) + { + return $"{error.GetType().Name} (0x{error.HResult:X8}): {error.Message}"; + } + + private static class NativeMethods + { + [DllImport("kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true)] + public static extern uint GetFileAttributesW(string fileName); + } + } +} diff --git a/GVFS/GVFS.Hooks/KnownGitCommands.cs b/GVFS/GVFS.Hooks/KnownGitCommands.cs deleted file mode 100644 index a392eace3c..0000000000 --- a/GVFS/GVFS.Hooks/KnownGitCommands.cs +++ /dev/null @@ -1,150 +0,0 @@ -using System.Collections.Generic; - -namespace GVFS.Hooks -{ - internal static class KnownGitCommands - { - private static HashSet knownCommands = new HashSet() - { - "add", - "am", - "annotate", - "apply", - "archive", - "bisect--helper", - "blame", - "branch", - "bundle", - "cat-file", - "check-attr", - "check-ignore", - "check-mailmap", - "check-ref-format", - "checkout", - "checkout-index", - "cherry", - "cherry-pick", - "clean", - "clone", - "column", - "commit", - "commit-tree", - "config", - "count-objects", - "credential", - "describe", - "diff", - "diff-files", - "diff-index", - "diff-tree", - "fast-export", - "fetch", - "fetch-pack", - "fmt-merge-msg", - "for-each-ref", - "format-patch", - "fsck", - "fsck-objects", - "gc", - "get-tar-commit-id", - "grep", - "hash-object", - "help", - "index-pack", - "init", - "init-db", - "interpret-trailers", - "log", - "ls-files", - "ls-remote", - "ls-tree", - "mailinfo", - "mailsplit", - "merge", - "merge-base", - "merge-file", - "merge-index", - "merge-ours", - "merge-recursive", - "merge-recursive-ours", - "merge-recursive-theirs", - "merge-subtree", - "merge-tree", - "mktag", - "mktree", - "mv", - "name-rev", - "notes", - "pack-objects", - "pack-redundant", - "pack-refs", - "patch-id", - "pickaxe", - "prune", - "prune-packed", - "pull", - "push", - "read-tree", - "rebase", - "rebase--helper", - "receive-pack", - "reflog", - "remote", - "remote-ext", - "remote-fd", - "repack", - "replace", - "rerere", - "reset", - "restore", - "rev-list", - "rev-parse", - "revert", - "rm", - "send-pack", - "shortlog", - "show", - "show-branch", - "show-ref", - "stage", - "status", - "stripspace", - "switch", - "symbolic-ref", - "tag", - "unpack-file", - "unpack-objects", - "update-index", - "update-ref", - "update-server-info", - "upload-archive", - "upload-archive--writer", - "var", - "verify-commit", - "verify-pack", - "verify-tag", - "version", - "whatchanged", - "worktree", - "write-tree", - - // Externals - "bisect", - "filter-branch", - "gui", - "merge-octopus", - "merge-one-file", - "merge-resolve", - "mergetool", - "parse-remote", - "quiltimport", - "rebase", - "submodule", - }; - - public static bool Contains(string gitCommand) - { - return knownCommands.Contains(gitCommand); - } - } -} diff --git a/GVFS/GVFS.Hooks/Program.CommandLocking.cs b/GVFS/GVFS.Hooks/Program.CommandLocking.cs index d535373325..b016a7d9ea 100644 --- a/GVFS/GVFS.Hooks/Program.CommandLocking.cs +++ b/GVFS/GVFS.Hooks/Program.CommandLocking.cs @@ -1,6 +1,5 @@ using GVFS.Common; -using GVFS.Common.Git; -using GVFS.Common.Tracing; +using System; using System.Linq; namespace GVFS.Hooks @@ -15,10 +14,41 @@ public partial class Program // 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) + internal static bool TryShouldLock( + string[] args, + Func readGitAliasKeys, + string searchPath, + Func getFileAttributes, + out bool shouldLock, + out string error) { + error = null; string gitCommand = GetGitCommand(args); + // Git reports a built-in by its name. A git- prefix means that Git found no built-in + // and will run a program or expand an alias, so the read-only list does not apply. + // git-submodule and git-web--browse are programs that Git ships in its exec-path, + // which Git puts first on PATH. They keep their exemptions. + if (args[1].StartsWith("git-", StringComparison.Ordinal) && + gitCommand != "submodule" && + gitCommand != "web--browse") + { + shouldLock = true; + if (!GitCommandResolver.TryIsAlias(args[1], readGitAliasKeys, searchPath, getFileAttributes, out bool isAlias, out error)) + { + return false; + } + + shouldLock = !isAlias; + return true; + } + + shouldLock = ShouldLockBuiltIn(gitCommand, args); + return true; + } + + private static bool ShouldLockBuiltIn(string gitCommand, string[] args) + { switch (gitCommand) { // Keep these alphabetically sorted @@ -94,12 +124,6 @@ internal static bool ShouldLock(string[] args) return false; } - if (!KnownGitCommands.Contains(gitCommand) && - IsAlias(gitCommand)) - { - return false; - } - return true; } @@ -113,21 +137,5 @@ private static string GetGitCommand(string[] args) return command; } - - private static bool IsAlias(string command) - { - string aliasKey = "alias." + command; - ProcessResult result = GitConfigReadRetry.InvokeResult( - aliasKey, - () => ProcessHelper.Run("git", "config --get " + aliasKey), - (ProcessResult processResult) => - processResult.ExitCode != 0 && - GitConfigReadRetry.IsTransientFileAccessError(processResult.Errors, lineMustContain: "unable to access"), - NullTracer.Instance, - GitConfigReadRetry.Delay, - ConfigReadMaxAttempts); - - return !string.IsNullOrEmpty(result.Output); - } } } diff --git a/GVFS/GVFS.Hooks/Program.cs b/GVFS/GVFS.Hooks/Program.cs index 3093ab15d9..401a46a96e 100644 --- a/GVFS/GVFS.Hooks/Program.cs +++ b/GVFS/GVFS.Hooks/Program.cs @@ -253,22 +253,74 @@ private static void RunLockRequest(string[] args, bool unattended, LockRequestDe { try { - if (ShouldLock(args)) + Lazy parentPid = new Lazy(() => GetParentPid(args)); + bool checkOwnership = GetHookType(args) == PostCommandHook && + args[1].StartsWith("git-", StringComparison.Ordinal); + bool shouldLock; + if (checkOwnership) + { + shouldLock = true; + } + else if (!TryShouldLock( + args, + () => ReadGitAliasKeys(parentPid.Value), + Environment.GetEnvironmentVariable("PATH"), + GitCommandResolver.GetFileAttributes, + out shouldLock, + out string resolveError)) + { + ExitWithError( + resolveError, + $"GVFS cannot tell if 'git {args[1].Substring("git-".Length)}' runs an alias or a program, so it stopped the command.", + "Run the command again. If this error continues, correct the problem that the first line describes."); + } + + if (shouldLock) { using (NamedPipeClient pipeClient = new NamedPipeClient(enlistmentPipename)) { if (!pipeClient.Connect()) { + if (checkOwnership) + { + // Locks exist only in the mount process, so there is nothing to release. + return; + } + ExitWithError("The repo does not appear to be mounted. Use 'gvfs status' to check."); } - int pid = GetParentPid(args); + int pid = parentPid.Value; if (pid == Program.InvalidProcessId || !GVFSHooksPlatform.IsProcessActive(pid)) { ExitWithError("GVFS.Hooks: Unable to find parent git.exe process " + "(PID: " + pid + ")."); } + if (checkOwnership) + { + // Executable and alias config can change after acquisition. + if (!GitCommandLock.TryCheckOwnership( + pid, + GVFSHooksPlatform.IsElevated(), + GenerateFullCommand(args), + request => + { + pipeClient.SendRequest(request); + return pipeClient.ReadResponse(); + }, + out bool ownsLock, + out string error)) + { + ExitWithError(error); + } + + if (!ownsLock) + { + return; + } + } + requestToRun(unattended, args, pid, pipeClient); } } @@ -385,7 +437,7 @@ private static bool CheckGVFSLockAvailabilityOnly(string[] args) // blocking other commands from running. The git argument // "--no-optional-locks" results in a 'negative' // value GIT_OPTIONAL_LOCKS environment variable. - return GetGitCommand(args).Equals("status", StringComparison.OrdinalIgnoreCase) && + return string.Equals(args[1], "status", StringComparison.Ordinal) && (args.Any(arg => arg.Equals("--no-lock-index", StringComparison.OrdinalIgnoreCase)) || IsGitEnvVarDisabled("GIT_OPTIONAL_LOCKS")); } @@ -432,6 +484,19 @@ private static string GetHookType(string[] args) return args[0].ToLowerInvariant(); } + private static ProcessResult ReadGitAliasKeys(int parentPid) + { + string gitPath = GitCommandResolver.GetProcessPath(parentPid); + return GitConfigReadRetry.InvokeResult( + "alias keys", + () => ProcessHelper.Run(GitCommandResolver.CreateAliasKeyQuery(gitPath)), + (ProcessResult result) => + GitConfigReadRetry.IsTransientFileAccessError(result.Errors, lineMustContain: GitCommandResolver.UnableToAccess), + NullTracer.Instance, + GitConfigReadRetry.Delay, + ConfigReadMaxAttempts); + } + /// /// 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 1d88885cf4..906a6955e5 100644 --- a/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj +++ b/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj @@ -27,8 +27,11 @@ - - Hooks\KnownGitCommands.cs + + Hooks\GitCommandLock.cs + + + Hooks\GitCommandResolver.cs Hooks\Program.CommandLocking.cs diff --git a/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs b/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs index 6b198795b3..433ee028e6 100644 --- a/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs +++ b/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs @@ -1,5 +1,7 @@ +using GVFS.Common; using GVFS.Tests.Should; using NUnit.Framework; +using System.Collections.Generic; using System.Linq; namespace GVFS.UnitTests.Hooks @@ -9,7 +11,6 @@ public class CommandLockingTests { [TestCase("annotate")] [TestCase("annotate --contents mock-file HEAD")] - [TestCase("git-annotate")] [TestCase("blame")] [TestCase("branch")] [TestCase("cat-file")] @@ -57,6 +58,7 @@ public class CommandLockingTests [TestCase("show-branch --reflog")] [TestCase("show-ref")] [TestCase("submodule")] + [TestCase("git-submodule")] [TestCase("symbolic-ref")] [TestCase("tag")] [TestCase("unpack-objects")] @@ -68,11 +70,11 @@ public class CommandLockingTests [TestCase("verify-tag --format=%(refname) mock-tag")] [TestCase("version")] [TestCase("web--browse")] + [TestCase("git-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) @@ -115,16 +117,90 @@ public void CommandsWithoutLockPreserveTheirPolicy(string command) [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); } + [TestCase("git-annotate")] + [TestCase("git-DIFF")] + [TestCase("git-LOG")] + [TestCase("git-worktree list")] + [TestCase("git-worktree add list")] + [TestCase("git-reset --soft")] + public void ExternalsNamedAfterBuiltInsAcquireLock(string command) + { + string hookCommand = command.Split(' ')[0]; + string[] args = CreatePreCommandArgs(command); + List candidates = new List(); + + GVFS.Hooks.Program.TryShouldLock( + args, + () => throw new AssertionException("A program must not query aliases."), + @"mock:\tools", + path => + { + candidates.Add(path); + return 0x80; + }, + out bool shouldLock, + out string error).ShouldBeTrue(); + + shouldLock.ShouldBeTrue(string.Join(" ", args)); + error.ShouldBeNull(); + candidates.ShouldMatchInOrder(@"mock:\tools\" + hookCommand + ".exe"); + } + + [TestCase("git-LOG")] + [TestCase("git-co")] + public void AliasesWithoutAProgramSkipTheOuterLock(string hookCommand) + { + int queries = 0; + GVFS.Hooks.Program.TryShouldLock( + CreatePreCommandArgs(hookCommand), + () => + { + ++queries; + return new ProcessResult("alias." + hookCommand.Substring("git-".Length).ToLowerInvariant() + "\n", string.Empty, 0); + }, + @"mock:\tools", + path => GVFS.Hooks.GitCommandResolver.InvalidFileAttributes, + out bool shouldLock, + out string error).ShouldBeTrue(); + + queries.ShouldEqual(1); + shouldLock.ShouldBeFalse(); + error.ShouldBeNull(); + } + + [TestCase] + public void AliasQueryFailuresAreReturnedToTheHook() + { + GVFS.Hooks.Program.TryShouldLock( + CreatePreCommandArgs("git-MiXeD"), + () => new ProcessResult(string.Empty, "fatal: bad config", 128), + @"mock:\tools", + path => GVFS.Hooks.GitCommandResolver.InvalidFileAttributes, + out bool shouldLock, + out string error).ShouldBeFalse(); + + shouldLock.ShouldBeTrue(); + error.ShouldContain("exit code 128", "fatal: bad config"); + } + + private static string[] CreatePreCommandArgs(string command) + { + return new[] { "pre-command" }.Concat(command.Split(' ')).Concat(new[] { "--git-pid=456" }).ToArray(); + } + private static void AssertLockPolicy(string command, bool shouldLock) { - foreach (string hook in new[] { "pre-command", "post-command" }) + // The hook sends post-command for git- to the lock-ownership check, not to this policy. + string[] hooks = command.StartsWith("git-", System.StringComparison.Ordinal) + ? new[] { "pre-command" } + : new[] { "pre-command", "post-command" }; + foreach (string hook in hooks) { string[] args = new[] { hook } .Concat(command.Split(' ')) @@ -133,7 +209,15 @@ private static void AssertLockPolicy(string command, bool shouldLock) : new[] { "--git-pid=456" }) .ToArray(); - GVFS.Hooks.Program.ShouldLock(args).ShouldEqual(shouldLock, string.Join(" ", args)); + GVFS.Hooks.Program.TryShouldLock( + args, + () => throw new AssertionException("Unexpected Git alias list for " + string.Join(" ", args)), + @"mock:\tools", + path => throw new AssertionException("Unexpected PATH search for " + path), + out bool actual, + out string error).ShouldBeTrue(error); + + actual.ShouldEqual(shouldLock, string.Join(" ", args)); } } } diff --git a/GVFS/GVFS.UnitTests/Hooks/GitCommandLockTests.cs b/GVFS/GVFS.UnitTests/Hooks/GitCommandLockTests.cs new file mode 100644 index 0000000000..c1cb79f065 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Hooks/GitCommandLockTests.cs @@ -0,0 +1,99 @@ +using GVFS.Common.NamedPipes; +using GVFS.Hooks; +using GVFS.Tests.Should; +using GVFS.UnitTests.Category; +using NUnit.Framework; + +namespace GVFS.UnitTests.Hooks +{ + [TestFixture] + public class GitCommandLockTests + { + [TestCase(123, true)] + [TestCase(456, false)] + public void OwnershipComparesTheTypedPidWithoutAcquiring(int ownerPid, bool expectedOwner) + { + int requests = 0; + GitCommandLock.TryCheckOwnership( + 123, + isElevated: false, + "git race", + request => + { + ++requests; + request.Header.ShouldEqual("AcquireLock"); + NamedPipeMessages.LockData data = new NamedPipeMessages.LockRequest(request.Body).RequestData; + data.PID.ShouldEqual(123); + data.CheckAvailabilityOnly.ShouldBeTrue(); + data.ParsedCommand.ShouldEqual("git race"); + return new NamedPipeMessages.AcquireLock.Response( + "LockDeniedGit", + new NamedPipeMessages.LockData(ownerPid, false, false, "git original", string.Empty)).CreateMessage(); + }, + out bool ownsLock, + out string error).ShouldBeTrue(); + + requests.ShouldEqual(1); + ownsLock.ShouldEqual(expectedOwner); + error.ShouldBeNull(); + } + + [TestCase("LockAvailable")] + [TestCase("LockDeniedGVFS")] + public void UnownedLocksNeedNoRelease(string result) + { + GitCommandLock.TryCheckOwnership( + 123, false, "git alias", + request => new NamedPipeMessages.Message(result, null), + out bool ownsLock, + out string error).ShouldBeTrue(); + + ownsLock.ShouldBeFalse(); + error.ShouldBeNull(); + } + + [TestCase("MountNotReady", "not finished initializing")] + [TestCase("UnmountInProgress", "unmounting")] + [TestCase("LockAcquired", "unexpected lock-ownership response")] + [TestCase("UnknownResponse", "unexpected lock-ownership response")] + [TestCase("LockDeniedGit", "without a valid process ID")] + public void InvalidOwnershipResponsesProduceADiagnostic(string result, string expectedError) + { + GitCommandLock.TryCheckOwnership( + 123, false, "git alias", + request => new NamedPipeMessages.Message(result, null), + out bool ownsLock, + out string error).ShouldBeFalse(); + + ownsLock.ShouldBeFalse(); + error.ShouldContain(expectedError); + } + + [TestCase] + public void MissingOwnershipResponseProducesADiagnostic() + { + GitCommandLock.TryCheckOwnership( + 123, false, "git alias", request => null, + out bool ownsLock, + out string error).ShouldBeFalse(); + + ownsLock.ShouldBeFalse(); + error.ShouldContain("no lock-ownership response"); + } + + [TestCase("malformed")] + [TestCase("123|false|false|-1|x|0|")] + [Category(CategoryConstants.ExceptionExpected)] + public void MalformedOwnershipResponseProducesADiagnostic(string body) + { + GitCommandLock.TryCheckOwnership( + 123, false, "git alias", + request => new NamedPipeMessages.Message("LockDeniedGit", body), + out bool ownsLock, + out string error).ShouldBeFalse(); + + ownsLock.ShouldBeFalse(); + error.ShouldContain("invalid lock-ownership response"); + } + } +} diff --git a/GVFS/GVFS.UnitTests/Hooks/GitCommandResolverTests.cs b/GVFS/GVFS.UnitTests/Hooks/GitCommandResolverTests.cs new file mode 100644 index 0000000000..0af8b5313a --- /dev/null +++ b/GVFS/GVFS.UnitTests/Hooks/GitCommandResolverTests.cs @@ -0,0 +1,380 @@ +using GVFS.Common; +using GVFS.Hooks; +using GVFS.Tests.Should; +using GVFS.UnitTests.Category; +using NUnit.Framework; +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.IO; +using Win32Exception = System.ComponentModel.Win32Exception; + +namespace GVFS.UnitTests.Hooks +{ + [TestFixture] + public class GitCommandResolverTests + { + private const string SearchPath = @"mock:\git\libexec;mock:\tools"; + private const uint NormalFile = 0x80; + private const uint Directory = 0x10; + private const string UnreadConfigErrors = + "warning: unable to access '.git/config': Permission denied\n" + + "warning: unable to access '.git/config': Permission denied\n" + + "fatal: unknown error occurred while reading the configuration files\n"; + + [TestCase("stash")] + [TestCase("for-each-repo")] + [TestCase("checkout")] + [TestCase("update-index")] + [TestCase("status")] + public void BuiltInsDoNotQueryAliasesOrSearchPath(string command) + { + GitCommandResolver.TryIsAlias( + command, + () => throw new AssertionException("A built-in must not query aliases."), + SearchPath, + path => throw new AssertionException("A built-in must not search PATH."), + out bool isAlias, + out string error).ShouldBeTrue(); + + isAlias.ShouldBeFalse(); + error.ShouldBeNull(); + } + + [TestCase("git-tools/command")] + [TestCase(@"git-tools\command")] + public void ExternalPathsDoNotQueryAliases(string command) + { + GitCommandResolver.TryIsAlias( + command, + () => throw new AssertionException("An external path must not query aliases."), + SearchPath, + path => throw new AssertionException("An external path must not search PATH."), + out bool isAlias, + out string error).ShouldBeTrue(); + + isAlias.ShouldBeFalse(); + error.ShouldBeNull(); + } + + [TestCase("git-co", "alias.st\nalias.co\nalias.co\n")] + [TestCase("git-CO", "alias.co\n")] + [TestCase("git-myAlias", "alias.myalias\n")] + [TestCase("git-sx", "alias.sx.command\n")] + [TestCase("git-MiXeD", "alias.MiXeD.command\n")] + [TestCase("git-a.b", "alias.a.b.command\n")] + [TestCase("git-mixed.foo", "alias.MiXeD.foo\n")] + [TestCase("git-CAF\u00e9.FOO", "alias.Caf\u00e9.foo\n")] + [TestCase("git-caf\u00e9", "alias.caf\u00e9.command\n")] + [TestCase("git-es", "alias..es\n")] + [TestCase("git-ES", "alias..es\n")] + [TestCase("git-command", "alias..command\n")] + [TestCase("git-git-prefixed", "alias.git-prefixed\n")] + public void AliasKeysThatGitExpandsSkipTheOuterLock(string hookCommand, string aliasKeys) + { + int queries = 0; + GitCommandResolver.TryIsAlias( + hookCommand, + () => + { + ++queries; + return new ProcessResult(aliasKeys, string.Empty, 0); + }, + SearchPath, + path => GitCommandResolver.InvalidFileAttributes, + out bool isAlias, + out string error).ShouldBeTrue(); + + queries.ShouldEqual(1); + isAlias.ShouldBeTrue(); + error.ShouldBeNull(); + } + + [TestCase("git-SX", "alias.sx.command\n", 0)] + [TestCase("git-zzfoO", "alias.zzfoo.command\n", 0)] + [TestCase("git-sx.command", "alias.sx.command\n", 0)] + [TestCase("git-A.B", "alias.a.b.command\n", 0)] + [TestCase("git-co", "alias.st\nalias.co-other\n", 0)] + [TestCase("git-co", "alias.\n", 0)] + [TestCase("git-", "alias.\nalias..\n", 0)] + [TestCase("git-.command", "alias..command\n", 0)] + [TestCase("git-CAF\u00c9.FOO", "alias.Caf\u00e9.foo\n", 0)] + [TestCase("git-co", "", 1)] + public void AliasKeysThatGitDoesNotExpandKeepTheLock(string hookCommand, string aliasKeys, int exitCode) + { + GitCommandResolver.TryIsAlias( + hookCommand, + () => new ProcessResult(aliasKeys, "13:00:00.000000 git.c:1 trace: built-in: git config\n", exitCode), + SearchPath, + path => GitCommandResolver.InvalidFileAttributes, + out bool isAlias, + out string error).ShouldBeTrue(); + + isAlias.ShouldBeFalse(); + error.ShouldBeNull(); + } + + [TestCase("git-co", @"mock:\git\libexec\git-co.exe")] + [TestCase("git-co", @"mock:\tools\git-co")] + [TestCase("git-Co", @"mock:\tools\GIT-CO.EXE")] + [TestCase("git-co.EXE", @"mock:\tools\git-co.EXE")] + [TestCase("git-git-prefixed", @"mock:\tools\git-git-prefixed")] + public void ProgramsTakePrecedenceWithoutAnAliasQuery(string hookCommand, string programPath) + { + GitCommandResolver.TryIsAlias( + hookCommand, + () => throw new AssertionException("A program must not query aliases."), + SearchPath, + path => string.Equals(path, programPath, StringComparison.OrdinalIgnoreCase) ? NormalFile : GitCommandResolver.InvalidFileAttributes, + out bool isAlias, + out string error).ShouldBeTrue(); + + isAlias.ShouldBeFalse(); + error.ShouldBeNull(); + } + + [TestCase(@"mock:\tools\git-co.exe", false)] + [TestCase(@"mock:\tools\git-co", true)] + public void DirectoriesMatchOnlyTheExeCandidate(string directoryPath, bool isAliasExpected) + { + GitCommandResolver.TryIsAlias( + "git-co", + () => new ProcessResult("alias.co\n", string.Empty, 0), + @"mock:\tools", + path => string.Equals(path, directoryPath, StringComparison.Ordinal) ? Directory : GitCommandResolver.InvalidFileAttributes, + out bool isAlias, + out string error).ShouldBeTrue(); + + isAlias.ShouldEqual(isAliasExpected); + error.ShouldBeNull(); + } + + [TestCase(128)] + [TestCase(1)] + public void UnreadConfigIsAFailure(int exitCode) + { + GitCommandResolver.TryIsAlias( + "git-co", + () => new ProcessResult(string.Empty, UnreadConfigErrors, exitCode), + SearchPath, + path => GitCommandResolver.InvalidFileAttributes, + out bool isAlias, + out string error).ShouldBeFalse(); + + isAlias.ShouldBeFalse(); + error.ShouldContain("Unable to read Git aliases (exit code " + exitCode + ")", "unable to access '.git/config'"); + } + + [TestCase(128, "")] + [TestCase(128, "alias.co\n")] + [TestCase(1, "alias.co\n")] + public void FailedAliasQueriesNeverGrantAnAliasExemption(int exitCode, string output) + { + GitCommandResolver.TryIsAlias( + "git-co", + () => new ProcessResult(output, "config read failed", exitCode), + SearchPath, + path => GitCommandResolver.InvalidFileAttributes, + out bool isAlias, + out string error).ShouldBeFalse(); + + isAlias.ShouldBeFalse(); + error.ShouldContain("Unable to read Git aliases", "exit code " + exitCode, "config read failed"); + } + + [TestCase] + [Category(CategoryConstants.ExceptionExpected)] + public void AliasQueryFailuresProducePreciseDiagnostics() + { + foreach (Exception failure in new Exception[] + { + new Win32Exception(5), + new InvalidOperationException("No executable path"), + new ArgumentException("No parent process"), + new IOException("Read failed"), + new UnauthorizedAccessException("Access denied"), + }) + { + GitCommandResolver.TryIsAlias( + "git-co", + () => throw failure, + SearchPath, + path => GitCommandResolver.InvalidFileAttributes, + out bool isAlias, + out string error).ShouldBeFalse(); + + isAlias.ShouldBeFalse(); + error.ShouldContain("Unable to read Git aliases", failure.GetType().Name, failure.HResult.ToString("X8"), failure.Message); + error.Contains(" at GVFS.").ShouldBeFalse(); + } + } + + [TestCase] + [Category(CategoryConstants.ExceptionExpected)] + public void MissingParentProcessIsNamedInTheError() + { + InvalidOperationException failure = Assert.Throws(() => GitCommandResolver.GetProcessPath(-1)); + failure.Message.ShouldContain("parent Git process (PID -1)"); + } + + [TestCase(null)] + [TestCase("")] + public void MissingSearchPathIsAFailure(string searchPath) + { + GitCommandResolver.TryIsAlias( + "git-co", + () => throw new AssertionException("An empty PATH must not query aliases."), + searchPath, + path => throw new AssertionException("An empty PATH must not be searched."), + out bool isAlias, + out string error).ShouldBeFalse(); + + isAlias.ShouldBeFalse(); + error.ShouldContain("inherited PATH is empty"); + } + + [TestCase] + public void SearchPathUsesRawEntriesAndSkipsEmptyEntries() + { + List candidates = new List(); + GitCommandResolver.TryIsAlias( + "git-co", + () => new ProcessResult("alias.co\n", string.Empty, 0), + @"; spaced ;relative tools;;mock:\tools\;", + path => + { + candidates.Add(path); + return GitCommandResolver.InvalidFileAttributes; + }, + out bool isAlias, + out string error).ShouldBeTrue(); + + candidates.ShouldMatchInOrder( + @" spaced \git-co.exe", + @" spaced \git-co", + @"relative tools\git-co.exe", + @"relative tools\git-co", + @"mock:\tools\\git-co.exe", + @"mock:\tools\\git-co"); + isAlias.ShouldBeTrue(); + error.ShouldBeNull(); + } + + [TestCase] + public void CandidatesThatFitGitsPathBufferAreChecked() + { + // Git builds "\.exe" in a 260-byte buffer. 259 bytes and the NUL fit. + string directory = @"mock:\" + new string('d', 242); + List candidates = new List(); + GitCommandResolver.TryIsAlias( + "git-co", + () => new ProcessResult("alias.co\n", string.Empty, 0), + directory, + path => + { + candidates.Add(path); + return GitCommandResolver.InvalidFileAttributes; + }, + out bool isAlias, + out string error).ShouldBeTrue(); + + candidates.ShouldMatchInOrder(directory + @"\git-co.exe", directory + @"\git-co"); + isAlias.ShouldBeTrue(); + } + + [TestCase(243, "")] + [TestCase(129, "\u00e9")] + public void CandidatesLongerThanGitsPathBufferAreFailures(int directoryLength, string filler) + { + string directory = @"mock:\" + (filler.Length == 0 ? new string('d', directoryLength) : string.Concat(System.Linq.Enumerable.Repeat(filler, directoryLength))); + GitCommandResolver.TryIsAlias( + "git-co", + () => throw new AssertionException("A failed PATH search must not query aliases."), + @"mock:\tools-without-co;" + directory + @";mock:\tools", + path => path.StartsWith(@"mock:\tools\", StringComparison.Ordinal) ? NormalFile : GitCommandResolver.InvalidFileAttributes, + out bool isAlias, + out string error).ShouldBeFalse(); + + isAlias.ShouldBeFalse(); + error.ShouldContain("is too long for Git's path buffer"); + } + + [TestCase] + public void ProgramsFoundBeforeALongEntryDoNotFail() + { + GitCommandResolver.TryIsAlias( + "git-co", + () => throw new AssertionException("A program must not query aliases."), + @"mock:\tools;mock:\" + new string('d', 300), + path => path == @"mock:\tools\git-co.exe" ? NormalFile : GitCommandResolver.InvalidFileAttributes, + out bool isAlias, + out string error).ShouldBeTrue(); + + isAlias.ShouldBeFalse(); + error.ShouldBeNull(); + } + + [TestCase] + public void ExecutableSuffixDoesNotAddASecondExtension() + { + List candidates = new List(); + GitCommandResolver.TryIsAlias( + "git-co.EXE", + () => new ProcessResult("alias.co.exe\n", string.Empty, 0), + @"mock:\tools", + path => + { + candidates.Add(path); + return GitCommandResolver.InvalidFileAttributes; + }, + out bool isAlias, + out string error).ShouldBeTrue(); + + candidates.ShouldMatchInOrder(@"mock:\tools\git-co.EXE"); + isAlias.ShouldBeTrue(); + error.ShouldBeNull(); + } + + [TestCase] + public void GetFileAttributesFailsForPathsThatGitCannotAccess() + { + string directory = Path.Combine(Path.GetTempPath(), "GitCommandResolverTests_" + Guid.NewGuid().ToString("N")); + System.IO.Directory.CreateDirectory(directory); + try + { + string file = Path.Combine(directory, "git-co"); + File.WriteAllText(file, string.Empty); + + (GitCommandResolver.GetFileAttributes(file) & Directory).ShouldEqual(0u); + (GitCommandResolver.GetFileAttributes(directory) & Directory).ShouldEqual(Directory); + GitCommandResolver.GetFileAttributes(Path.Combine(directory, "missing", "git-co")).ShouldEqual(GitCommandResolver.InvalidFileAttributes); + GitCommandResolver.GetFileAttributes("\"" + directory + "\"\\git-co").ShouldEqual(GitCommandResolver.InvalidFileAttributes); + } + finally + { + System.IO.Directory.Delete(directory, recursive: true); + } + } + + [TestCase] + public void CurrentProcessPathUsesTheSuppliedPid() + { + GitCommandResolver.GetProcessPath(Environment.ProcessId).ShouldEqual(Environment.ProcessPath); + } + + [TestCase] + public void AliasKeyQueryUsesTheParentGit() + { + const string ParentGit = @"mock:\custom git\bin\git.exe"; + ProcessStartInfo query = GitCommandResolver.CreateAliasKeyQuery(ParentGit); + + query.FileName.ShouldEqual(ParentGit); + query.UseShellExecute.ShouldBeFalse(); + query.RedirectStandardOutput.ShouldBeTrue(); + query.RedirectStandardError.ShouldBeTrue(); + query.ArgumentList.ShouldMatchInOrder("config", "--name-only", "--get-regexp", @"^alias\."); + query.StandardOutputEncoding.WebName.ShouldEqual("utf-8"); + query.Environment["COMMAND_HOOK_LOCK"].ShouldEqual("true"); + } + } +} diff --git a/docs/faq.md b/docs/faq.md index 5026fe6411..35b8cf01b0 100644 --- a/docs/faq.md +++ b/docs/faq.md @@ -25,6 +25,24 @@ you can help optimize Git command performance for developers in the repository by limiting the number of files Git needs to consider for many common operations. +### Do Git aliases change command locks? + +If Git runs a built-in command or an external `git-` program, an +alias with the same name does not remove the GVFS lock requirement. For +example, `alias.stash` does not change the lock requirement for +`git stash`. + +If Git expands an alias, VFS for Git does not lock the alias itself. +This applies to both alias forms: `alias.` and +`[alias ""] command`. Each command in the alias follows its normal +lock policy. For example, an alias that runs `checkout` still requires +the lock for `checkout`. + +If VFS for Git cannot read the Git configuration to find aliases, it +stops the command with an error. Run the command again. If the error +continues, run `git config --get-regexp ^alias\.` to check your Git +configuration. + ### Why the name change? This project was formerly known as GVFS (Git Virtual File System). It is From a1fa7d0008ac629d83cf7b2e55535975f442db8b Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 5 Nov 2026 11:38:48 -0800 Subject: [PATCH 2/3] Match dot-prefixed alias subsections as Git does The hook removed a leading dot from every alias key after "alias.". Git removes the dot only when the subsection is empty. Thus Git runs "alias..review.foo" as the alias ".review.foo", and it runs [alias ".review"] command as ".review". The hook did not find these aliases, so it took an outer lock, and the alias command deadlocked. Split each key at its last dot, as parse_config_key does. An empty subsection names a plain alias. A "command" variable names the exact subsection. Any other key names all of the text after "alias.". Add unit cases for both forms, and add functional tests that run both forms with real Git while another process holds the lock. Assisted-by: Claude Opus 5.5 Signed-off-by: Tyrie Vella --- .../GitReadAndGitLockTests.cs | 11 +++++++ GVFS/GVFS.Hooks/GitCommandResolver.cs | 30 +++++++++++-------- .../Hooks/GitCommandResolverTests.cs | 6 ++++ 3 files changed, 35 insertions(+), 12 deletions(-) diff --git a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs index e615cb7ab4..99fc8284e3 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs @@ -507,6 +507,17 @@ public void UnreadableAliasConfigStopsTheCommandBeforeTheLock() } } + [TestCase("alias..gvfs-dot-prefix.foo", ".gvfs-dot-prefix.foo")] + [TestCase("alias..gvfs-dot-subsection.command", ".gvfs-dot-subsection")] + [Order(22)] + public void DotPrefixedSubsectionAliasInnerCommandStillAcquiresLock(string aliasKey, string aliasName) + { + ProcessResult result = this.RunWithLockHolder( + "-c \"" + aliasKey + "=status --porcelain\" " + aliasName); + result.ExitCode.ShouldEqual(0); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + private ProcessStartInfo CreateGitStartInfo(string command, Dictionary environment = null) { ProcessStartInfo startInfo = new ProcessStartInfo(Properties.Settings.Default.PathToGit, command); diff --git a/GVFS/GVFS.Hooks/GitCommandResolver.cs b/GVFS/GVFS.Hooks/GitCommandResolver.cs index c4c6690bf1..09b0cd43f3 100644 --- a/GVFS/GVFS.Hooks/GitCommandResolver.cs +++ b/GVFS/GVFS.Hooks/GitCommandResolver.cs @@ -142,29 +142,35 @@ public static uint GetFileAttributes(string path) } /// - /// Mirrors Git's alias lookup. git config prints "alias.<name>" in lowercase, and Git - /// matches it for any ASCII case. It prints "alias.<subsection>.command" with the - /// subsection case unchanged, and Git matches only that exact subsection. It prints other - /// "alias.<subsection>.<key>" names as dotted alias names, which Git matches for any - /// ASCII case. Git treats an empty subsection ("alias..<name>") as plain [alias]. + /// Mirrors config_alias_cb in Git's alias.c. Like parse_config_key, it splits the text + /// after "alias." at the last dot into a subsection and a variable name. If the + /// subsection is empty, the variable name is the alias name, matched for any ASCII case. + /// If the variable name is "command", the alias name is the subsection, which must match + /// exactly. Otherwise the alias name is all of the text after "alias.", matched for any + /// ASCII case. Thus "alias..review.foo" names ".review.foo". /// private static bool IsAliasKeyFor(string key, string command) { const string Section = "alias."; - const string CommandKey = ".command"; if (!key.StartsWith(Section, StringComparison.Ordinal)) { return false; } string name = key.Substring(Section.Length); - if (name.StartsWith(".", StringComparison.Ordinal)) + int lastDot = name.LastIndexOf('.'); + if (lastDot >= 0) { - name = name.Substring(1); - } - else if (name.Length > CommandKey.Length && name.EndsWith(CommandKey, StringComparison.Ordinal)) - { - return string.Equals(name.Substring(0, name.Length - CommandKey.Length), command, StringComparison.Ordinal); + string subsection = name.Substring(0, lastDot); + string variable = name.Substring(lastDot + 1); + if (subsection.Length == 0) + { + name = variable; + } + else if (string.Equals(variable, "command", StringComparison.Ordinal)) + { + return string.Equals(subsection, command, StringComparison.Ordinal); + } } return name.Length > 0 && EqualsIgnoringAsciiCase(name, command); diff --git a/GVFS/GVFS.UnitTests/Hooks/GitCommandResolverTests.cs b/GVFS/GVFS.UnitTests/Hooks/GitCommandResolverTests.cs index 0af8b5313a..2121165ba6 100644 --- a/GVFS/GVFS.UnitTests/Hooks/GitCommandResolverTests.cs +++ b/GVFS/GVFS.UnitTests/Hooks/GitCommandResolverTests.cs @@ -69,6 +69,9 @@ public void ExternalPathsDoNotQueryAliases(string command) [TestCase("git-es", "alias..es\n")] [TestCase("git-ES", "alias..es\n")] [TestCase("git-command", "alias..command\n")] + [TestCase("git-.review.foo", "alias..review.foo\n")] + [TestCase("git-.REVIEW.FOO", "alias..review.foo\n")] + [TestCase("git-.review", "alias..review.command\n")] [TestCase("git-git-prefixed", "alias.git-prefixed\n")] public void AliasKeysThatGitExpandsSkipTheOuterLock(string hookCommand, string aliasKeys) { @@ -98,6 +101,9 @@ public void AliasKeysThatGitExpandsSkipTheOuterLock(string hookCommand, string a [TestCase("git-co", "alias.\n", 0)] [TestCase("git-", "alias.\nalias..\n", 0)] [TestCase("git-.command", "alias..command\n", 0)] + [TestCase("git-review.foo", "alias..review.foo\n", 0)] + [TestCase("git-.REVIEW", "alias..review.command\n", 0)] + [TestCase("git-review", "alias..review.command\n", 0)] [TestCase("git-CAF\u00c9.FOO", "alias.Caf\u00e9.foo\n", 0)] [TestCase("git-co", "", 1)] public void AliasKeysThatGitDoesNotExpandKeepTheLock(string hookCommand, string aliasKeys, int exitCode) From 2240a8c6d3a68aec9c2370fca3005ee3e552ba48 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 5 Nov 2026 11:38:49 -0800 Subject: [PATCH 3/3] Add tests for the hook's lock decisions in Program.cs The unit tests do not compile Program.cs, so two decisions there had no direct tests: which status commands only check the lock, and what post-command does when the mount is not available. Move the availability-only check into GitCommandLock, which the unit tests compile. Add unit tests that check status with --no-lock-index, status with GIT_OPTIONAL_LOCKS, git-status, STATUS, and other commands. Add a functional test that runs GVFS.Hooks.exe in a folder that has a .gvfs folder but no mount. The test checks that post-command for a git- program exits without output, and that post-command for status reports that the repo is not mounted. Assisted-by: Claude Opus 5.5 Signed-off-by: Tyrie Vella --- .../GitReadAndGitLockTests.cs | 30 +++++++++++++++++++ GVFS/GVFS.Hooks/GitCommandLock.cs | 20 +++++++++++++ GVFS/GVFS.Hooks/Program.cs | 26 +--------------- .../Hooks/GitCommandLockTests.cs | 16 ++++++++++ 4 files changed, 67 insertions(+), 25 deletions(-) diff --git a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs index 99fc8284e3..0427576b70 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs @@ -518,6 +518,36 @@ public void DotPrefixedSubsectionAliasInnerCommandStillAcquiresLock(string alias result.Errors.ShouldContain(ExpectedStatusWaitingText); } + [TestCase] + [Order(23)] + public void PostCommandForExternalIgnoresAnUnmountedEnlistment() + { + string hooksExecutable = Path.Combine(Path.GetDirectoryName(Properties.Settings.Default.PathToGVFS), "GVFS.Hooks.exe"); + string directory = Path.Combine(Path.GetTempPath(), nameof(this.PostCommandForExternalIgnoresAnUnmountedEnlistment) + "_" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(Path.Combine(directory, ".gvfs")); + try + { + ProcessStartInfo hook = this.CreateGitStartInfo("post-command git-gvfs-unmounted --exit_code=0"); + hook.FileName = hooksExecutable; + hook.WorkingDirectory = directory; + + // A false GIT_OPTIONAL_LOCKS makes post-command status skip the lock request. + hook.Environment.Remove("GIT_OPTIONAL_LOCKS"); + ProcessResult external = ProcessHelper.Run(hook, timeoutMs: 30000); + external.ExitCode.ShouldEqual(0, external.Output + external.Errors); + (external.Output + external.Errors).ShouldEqual(string.Empty); + + hook.Arguments = "post-command status --exit_code=0"; + ProcessResult builtIn = ProcessHelper.Run(hook, timeoutMs: 30000); + builtIn.ExitCode.ShouldEqual(1, builtIn.Output + builtIn.Errors); + builtIn.Output.ShouldContain("The repo does not appear to be mounted."); + } + finally + { + Directory.Delete(directory, recursive: true); + } + } + private ProcessStartInfo CreateGitStartInfo(string command, Dictionary environment = null) { ProcessStartInfo startInfo = new ProcessStartInfo(Properties.Settings.Default.PathToGit, command); diff --git a/GVFS/GVFS.Hooks/GitCommandLock.cs b/GVFS/GVFS.Hooks/GitCommandLock.cs index 39cc6f8a73..ecc32dced7 100644 --- a/GVFS/GVFS.Hooks/GitCommandLock.cs +++ b/GVFS/GVFS.Hooks/GitCommandLock.cs @@ -7,6 +7,18 @@ internal static class GitCommandLock { private const string CheckMount = "Run 'gvfs status' to check the mount."; + /// + /// Returns true if Git runs "status" without locks, so the hook only checks that the GVFS + /// lock is free. "--no-optional-locks" sets GIT_OPTIONAL_LOCKS to a false value. A raw + /// "git-status" is an external program, so it never qualifies. + /// + public static bool IsAvailabilityOnly(string[] args, Func getEnvironmentVariable) + { + return string.Equals(args[1], "status", StringComparison.Ordinal) && + (Array.Exists(args, arg => arg.Equals("--no-lock-index", StringComparison.OrdinalIgnoreCase)) || + IsFalseValue(getEnvironmentVariable("GIT_OPTIONAL_LOCKS"))); + } + public static bool TryCheckOwnership( int pid, bool isElevated, @@ -66,5 +78,13 @@ public static bool TryCheckOwnership( return false; } } + + private static bool IsFalseValue(string value) + { + return string.Equals(value, "false", StringComparison.OrdinalIgnoreCase) || + string.Equals(value, "no", StringComparison.OrdinalIgnoreCase) || + string.Equals(value, "off", StringComparison.OrdinalIgnoreCase) || + string.Equals(value, "0", StringComparison.OrdinalIgnoreCase); + } } } diff --git a/GVFS/GVFS.Hooks/Program.cs b/GVFS/GVFS.Hooks/Program.cs index 401a46a96e..600ee823a7 100644 --- a/GVFS/GVFS.Hooks/Program.cs +++ b/GVFS/GVFS.Hooks/Program.cs @@ -432,14 +432,7 @@ private static bool CheckGVFSLockAvailabilityOnly(string[] args) { try { - // Don't acquire the GVFS lock if the git command is not acquiring locks. - // This enables tools to run status commands without to the index and - // blocking other commands from running. The git argument - // "--no-optional-locks" results in a 'negative' - // value GIT_OPTIONAL_LOCKS environment variable. - return string.Equals(args[1], "status", StringComparison.Ordinal) && - (args.Any(arg => arg.Equals("--no-lock-index", StringComparison.OrdinalIgnoreCase)) || - IsGitEnvVarDisabled("GIT_OPTIONAL_LOCKS")); + return GitCommandLock.IsAvailabilityOnly(args, Environment.GetEnvironmentVariable); } catch (Exception e) { @@ -462,23 +455,6 @@ private static string BuildUpdatePlaceholderFailureMessage(List fileList return message; } - private static bool IsGitEnvVarDisabled(string envVar) - { - string envVarValue = Environment.GetEnvironmentVariable(envVar); - if (!string.IsNullOrEmpty(envVarValue)) - { - if (string.Equals(envVarValue, "false", StringComparison.OrdinalIgnoreCase) || - string.Equals(envVarValue, "no", StringComparison.OrdinalIgnoreCase) || - string.Equals(envVarValue, "off", StringComparison.OrdinalIgnoreCase) || - string.Equals(envVarValue, "0", StringComparison.OrdinalIgnoreCase)) - { - return true; - } - } - - return false; - } - private static string GetHookType(string[] args) { return args[0].ToLowerInvariant(); diff --git a/GVFS/GVFS.UnitTests/Hooks/GitCommandLockTests.cs b/GVFS/GVFS.UnitTests/Hooks/GitCommandLockTests.cs index c1cb79f065..88ed7a49a3 100644 --- a/GVFS/GVFS.UnitTests/Hooks/GitCommandLockTests.cs +++ b/GVFS/GVFS.UnitTests/Hooks/GitCommandLockTests.cs @@ -9,6 +9,22 @@ namespace GVFS.UnitTests.Hooks [TestFixture] public class GitCommandLockTests { + [TestCase(new[] { "post-command", "status", "--no-lock-index" }, null, true)] + [TestCase(new[] { "post-command", "status", "--NO-LOCK-INDEX" }, null, true)] + [TestCase(new[] { "pre-command", "status" }, "0", true)] + [TestCase(new[] { "pre-command", "status" }, "Off", true)] + [TestCase(new[] { "pre-command", "status" }, "1", false)] + [TestCase(new[] { "pre-command", "status" }, null, false)] + [TestCase(new[] { "post-command", "git-status", "--no-lock-index" }, "0", false)] + [TestCase(new[] { "post-command", "STATUS", "--no-lock-index" }, "0", false)] + [TestCase(new[] { "post-command", "log", "--no-lock-index" }, "0", false)] + public void OnlyCanonicalStatusWithoutLocksIsAvailabilityOnly(string[] args, string optionalLocks, bool expected) + { + GitCommandLock.IsAvailabilityOnly( + args, + name => name == "GIT_OPTIONAL_LOCKS" ? optionalLocks : null).ShouldEqual(expected); + } + [TestCase(123, true)] [TestCase(456, false)] public void OwnershipComparesTheTypedPidWithoutAcquiring(int ownerPid, bool expectedOwner)