From 206ab39d8589c1fd8d4ff95ecc4001babe0d347a Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 6 Oct 2026 11:35:10 -0700 Subject: [PATCH 1/3] Retry transient Git config reads Use one bounded retry policy for git.exe and libgit2 config reads. Retry only transient file access failures and preserve existing missing-key behavior. Retry the libgit2 repository open, which parses .git\config. Retry the git config alias lookup in the hooks. Let latency-sensitive hook callers use a smaller attempt budget. Match a transient failure only on the reason after the last "': " in an error line. A path or key that contains "Permission denied" no longer matches. Trace when a transient failure persists after all attempts. Do not trace config names that contain URL credentials. Add Enlistment.CreateGitProcess(ITracer). Use it for the mount cache server, retry, status cache, and required-config readers, so retries reach the mount trace. Assisted-by: GPT-5.6 Sol Assisted-by: Claude Sonnet 5.5 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Enlistment.cs | 6 + GVFS/GVFS.Common/Git/GitConfigReadRetry.cs | 172 +++++++++++++ GVFS/GVFS.Common/Git/GitProcess.cs | 87 +++++-- GVFS/GVFS.Common/Git/LibGit2Repo.cs | 113 ++++++++- GVFS/GVFS.Common/GitStatusCacheConfig.cs | 2 +- GVFS/GVFS.Common/Http/CacheServerResolver.cs | 17 +- GVFS/GVFS.Common/RetryConfig.cs | 2 +- GVFS/GVFS.Hooks/GVFS.Hooks.csproj | 3 + GVFS/GVFS.Hooks/Program.cs | 18 +- GVFS/GVFS.Mount/InProcessMount.cs | 2 +- GVFS/GVFS.Mount/InProcessMountVerb.cs | 2 +- .../Common/ConfigReaderTracerTests.cs | 82 ++++++ .../Common/LibGit2RepoConfigLookupTests.cs | 238 +++++++++++++++++- .../Git/GitConfigReadRetryTests.cs | 138 ++++++++++ GVFS/GVFS.UnitTests/Git/GitProcessTests.cs | 178 +++++++++++++ .../Mock/Common/MockGVFSEnlistment.cs | 6 + .../GVFS.UnitTests/Mock/Git/MockGitProcess.cs | 7 +- 17 files changed, 1034 insertions(+), 39 deletions(-) create mode 100644 GVFS/GVFS.Common/Git/GitConfigReadRetry.cs create mode 100644 GVFS/GVFS.UnitTests/Common/ConfigReaderTracerTests.cs create mode 100644 GVFS/GVFS.UnitTests/Git/GitConfigReadRetryTests.cs diff --git a/GVFS/GVFS.Common/Enlistment.cs b/GVFS/GVFS.Common/Enlistment.cs index a9208b0edf..dcc0b58452 100644 --- a/GVFS/GVFS.Common/Enlistment.cs +++ b/GVFS/GVFS.Common/Enlistment.cs @@ -1,5 +1,6 @@ using GVFS.Common.FileSystem; using GVFS.Common.Git; +using GVFS.Common.Tracing; using System; using System.IO; @@ -117,5 +118,10 @@ public virtual GitProcess CreateGitProcess() { return new GitProcess(this); } + + public virtual GitProcess CreateGitProcess(ITracer tracer) + { + return new GitProcess(this, tracer); + } } } diff --git a/GVFS/GVFS.Common/Git/GitConfigReadRetry.cs b/GVFS/GVFS.Common/Git/GitConfigReadRetry.cs new file mode 100644 index 0000000000..690df4ce20 --- /dev/null +++ b/GVFS/GVFS.Common/Git/GitConfigReadRetry.cs @@ -0,0 +1,172 @@ +using GVFS.Common.Tracing; +using System; +using System.Threading; + +namespace GVFS.Common.Git +{ + internal static class GitConfigReadRetry + { + internal const int MaxAttempts = 5; + internal const int InitialDelayMilliseconds = 20; + + private static readonly string[] TransientReasons = + { + "Permission denied", + "Device or resource busy", + "being used by another process", + "sharing violation", + }; + + internal static T InvokeResult( + string configName, + Func readConfig, + Func isTransientFailure, + ITracer tracer, + Action delay, + int maxAttempts = MaxAttempts) + { + for (int attempt = 1; ; ++attempt) + { + T result = readConfig(); + if (!isTransientFailure(result)) + { + return result; + } + + if (attempt >= maxAttempts) + { + TraceExhausted(tracer, configName, attempt); + return result; + } + + TimeSpan retryDelay = GetRetryDelay(attempt); + TraceRetry(tracer, configName, attempt, maxAttempts, retryDelay); + delay(retryDelay); + } + } + + internal static T InvokeCatching( + string configName, + Func readConfig, + Func isTransientFailure, + ITracer tracer, + Action delay, + int maxAttempts = MaxAttempts) + { + for (int attempt = 1; ; ++attempt) + { + try + { + return readConfig(); + } + catch (Exception e) when (isTransientFailure(e)) + { + if (attempt >= maxAttempts) + { + TraceExhausted(tracer, configName, attempt); + throw; + } + + TimeSpan retryDelay = GetRetryDelay(attempt); + TraceRetry(tracer, configName, attempt, maxAttempts, retryDelay); + delay(retryDelay); + } + } + } + + internal static bool IsTransientLibGit2Error(Exception exception) + { + return + exception is LibGit2Exception && + IsTransientFileAccessError(exception.Message); + } + + /// + /// Both git.exe and libgit2 report a file failure as ... '<path>': <reason>. + /// Only the reason after the final ': is tested, so a path or key that contains + /// a transient phrase does not match. + /// + internal static bool IsTransientFileAccessError(string error, string lineMustContain = null) + { + if (string.IsNullOrEmpty(error)) + { + return false; + } + + foreach (string line in error.Split('\n')) + { + if (lineMustContain != null && !ContainsOrdinalIgnoreCase(line, lineMustContain)) + { + continue; + } + + int reasonSeparator = line.LastIndexOf("': ", StringComparison.Ordinal); + if (reasonSeparator < 0) + { + continue; + } + + string reason = line.Substring(reasonSeparator + 3); + foreach (string transientReason in TransientReasons) + { + if (ContainsOrdinalIgnoreCase(reason, transientReason)) + { + return true; + } + } + } + + return false; + } + + internal static bool ContainsOrdinalIgnoreCase(string value, string expected) + { + return + value != null && + value.IndexOf(expected, StringComparison.OrdinalIgnoreCase) >= 0; + } + + internal static void Delay(TimeSpan delay) + { + Thread.Sleep(delay); + } + + private static TimeSpan GetRetryDelay(int failedAttempt) + { + return TimeSpan.FromMilliseconds(InitialDelayMilliseconds * (1 << (failedAttempt - 1))); + } + + // A repository URL with embedded credentials can become part of a deprecated cache + // server config key name. + private static string GetTraceableConfigName(string configName) + { + return configName != null && configName.IndexOf('@') >= 0 + ? "" + : configName; + } + + private static void TraceRetry(ITracer tracer, string configName, int failedAttempt, int maxAttempts, TimeSpan retryDelay) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("ConfigName", GetTraceableConfigName(configName)); + metadata.Add("AttemptNumber", failedAttempt); + metadata.Add("MaxAttempts", maxAttempts); + metadata.Add("RetryDelayMilliseconds", retryDelay.TotalMilliseconds); + tracer.RelatedWarning( + metadata, + "Transient failure while reading git config. Retrying.", + Keywords.Telemetry); + } + + private static void TraceExhausted(ITracer tracer, string configName, int attempts) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("ConfigName", GetTraceableConfigName(configName)); + metadata.Add("Attempts", attempts); + tracer.RelatedWarning( + metadata, + "Transient failure while reading git config persisted after all attempts.", + Keywords.Telemetry); + } + } +} diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index df54900b7c..ce96e30f72 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -56,6 +56,8 @@ public class GitProcess : ICredentialStore private string gitBinPath; private string workingDirectoryRoot; private string dotGitRoot; + private readonly ITracer tracer; + private readonly Action configReadDelay; private Process executingProcess; private bool stopping; @@ -85,11 +87,26 @@ static GitProcess() } public GitProcess(Enlistment enlistment) - : this(enlistment.GitBinPath, enlistment.WorkingDirectoryBackingRoot) + : this(enlistment.GitBinPath, enlistment.WorkingDirectoryBackingRoot, NullTracer.Instance, GitConfigReadRetry.Delay) + { + } + + public GitProcess(Enlistment enlistment, ITracer tracer) + : this(enlistment.GitBinPath, enlistment.WorkingDirectoryBackingRoot, tracer, GitConfigReadRetry.Delay) + { + } + + internal GitProcess(Enlistment enlistment, ITracer tracer, Action configReadDelay) + : this(enlistment.GitBinPath, enlistment.WorkingDirectoryBackingRoot, tracer, configReadDelay) { } public GitProcess(string gitBinPath, string workingDirectoryRoot) + : this(gitBinPath, workingDirectoryRoot, NullTracer.Instance, GitConfigReadRetry.Delay) + { + } + + internal GitProcess(string gitBinPath, string workingDirectoryRoot, ITracer tracer, Action configReadDelay) { if (string.IsNullOrWhiteSpace(gitBinPath)) { @@ -98,6 +115,8 @@ public GitProcess(string gitBinPath, string workingDirectoryRoot) this.gitBinPath = gitBinPath; this.workingDirectoryRoot = workingDirectoryRoot; + this.tracer = tracer ?? NullTracer.Instance; + this.configReadDelay = configReadDelay ?? GitConfigReadRetry.Delay; if (this.workingDirectoryRoot != null) { @@ -127,22 +146,28 @@ public static Result Init(Enlistment enlistment) public static ConfigResult GetFromGlobalConfig(string gitBinPath, string settingName) { - return new ConfigResult( - new GitProcess(gitBinPath, workingDirectoryRoot: null).InvokeGitOutsideEnlistment("config --global " + settingName), + GitProcess git = new GitProcess(gitBinPath, workingDirectoryRoot: null); + return new ConfigResult(git.InvokeConfigRead( + settingName, + () => git.InvokeGitOutsideEnlistment("config --global " + settingName)), settingName); } public static ConfigResult GetFromSystemConfig(string gitBinPath, string settingName) { - return new ConfigResult( - new GitProcess(gitBinPath, workingDirectoryRoot: null).InvokeGitOutsideEnlistment("config --system " + settingName), + GitProcess git = new GitProcess(gitBinPath, workingDirectoryRoot: null); + return new ConfigResult(git.InvokeConfigRead( + settingName, + () => git.InvokeGitOutsideEnlistment("config --system " + settingName)), settingName); } public static ConfigResult GetFromFileConfig(string gitBinPath, string configFile, string settingName) { - return new ConfigResult( - new GitProcess(gitBinPath, workingDirectoryRoot: null).InvokeGitOutsideEnlistment("config --file " + configFile + " " + settingName), + GitProcess git = new GitProcess(gitBinPath, workingDirectoryRoot: null); + return new ConfigResult(git.InvokeConfigRead( + settingName, + () => git.InvokeGitOutsideEnlistment("config --file " + configFile + " " + settingName)), settingName); } @@ -469,7 +494,9 @@ public Result SetInFileConfig(string configFile, string settingName, string valu public bool TryGetConfigUrlMatch(string section, string repositoryUrl, out Dictionary configSettings) { // See GetFromConfig for why pre-command hook is disabled. - Result result = this.InvokeGitAgainstDotGitFolder($"config --get-urlmatch {section} {repositoryUrl}", usePreCommandHook: false); + Result result = this.InvokeConfigRead( + section, + () => this.InvokeGitAgainstDotGitFolder($"config --get-urlmatch {section} {repositoryUrl}", usePreCommandHook: false)); if (result.ExitCodeIsFailure) { configSettings = null; @@ -485,7 +512,11 @@ public bool TryGetAllConfig(bool localOnly, out Dictionary this.InvokeGitAgainstDotGitFolder("config --list " + localParameter, usePreCommandHook: false)), + "--list"); if (result.TryParseAsString(out string output, out string _, string.Empty)) { @@ -515,16 +546,22 @@ public virtual ConfigResult GetFromConfig(string settingName, bool forceOutsideE // mutation happens), and skipping the hook makes us robust to a broken // hook config in the enlistment - which is exactly what we'd be trying // to repair via TryUpdateHooks at mount time. - return + Result result = fileSystem.DirectoryExists(this.workingDirectoryRoot) && !forceOutsideEnlistment - ? new ConfigResult(this.InvokeGitAgainstDotGitFolder(command, usePreCommandHook: false), settingName) - : new ConfigResult(this.InvokeGitOutsideEnlistment(command), settingName); + ? this.InvokeConfigRead(settingName, () => this.InvokeGitAgainstDotGitFolder(command, usePreCommandHook: false)) + : this.InvokeConfigRead(settingName, () => this.InvokeGitOutsideEnlistment(command)); + + return new ConfigResult(result, settingName); } public ConfigResult GetFromLocalConfig(string settingName) { // See GetFromConfig above for why pre-command hook is disabled here. - return new ConfigResult(this.InvokeGitAgainstDotGitFolder("config --local " + settingName, usePreCommandHook: false), settingName); + return new ConfigResult( + this.InvokeConfigRead( + settingName, + () => this.InvokeGitAgainstDotGitFolder("config --local " + settingName, usePreCommandHook: false)), + settingName); } /// @@ -556,7 +593,12 @@ public ConfigResult GetOriginUrl() { /* Disable precommand hook because this config call is used during mounting process * which needs to be able to fix a bad precommand hook configuration. */ - return new ConfigResult(this.InvokeGitAgainstDotGitFolder("config --local remote.origin.url", usePreCommandHook: false), "remote.origin.url"); + const string ConfigName = "remote.origin.url"; + return new ConfigResult( + this.InvokeConfigRead( + ConfigName, + () => this.InvokeGitAgainstDotGitFolder("config --local " + ConfigName, usePreCommandHook: false)), + ConfigName); } public Result DiffTree(string sourceTreeish, string targetTreeish, Action onResult) @@ -1138,6 +1180,23 @@ private static bool CurrentProcessHasVisibleConsoleWindowSafe() } } + private Result InvokeConfigRead(string configName, Func readConfig) + { + return GitConfigReadRetry.InvokeResult( + configName, + readConfig, + IsTransientConfigReadFailure, + this.tracer, + this.configReadDelay); + } + + private static bool IsTransientConfigReadFailure(Result result) + { + return + result.ExitCodeIsFailure && + GitConfigReadRetry.IsTransientFileAccessError(result.Errors, lineMustContain: "unable to access"); + } + private static string ParseValue(string contents, string prefix) { int startIndex = contents.IndexOf(prefix) + prefix.Length; diff --git a/GVFS/GVFS.Common/Git/LibGit2Repo.cs b/GVFS/GVFS.Common/Git/LibGit2Repo.cs index 6667dd2c82..d7de0dd5ef 100644 --- a/GVFS/GVFS.Common/Git/LibGit2Repo.cs +++ b/GVFS/GVFS.Common/Git/LibGit2Repo.cs @@ -29,19 +29,42 @@ public class LibGit2Repo : IDisposable { private bool disposedValue = false; private IntPtr odbHandle = IntPtr.Zero; + private readonly Action configReadDelay; + private readonly int configReadMaxAttempts; public delegate void MultiVarConfigCallback(string value); public LibGit2Repo(ITracer tracer, string repoPath) + : this(tracer, repoPath, GitConfigReadRetry.Delay, GitConfigReadRetry.MaxAttempts) + { + } + + internal LibGit2Repo(ITracer tracer, string repoPath, Action configReadDelay, int configReadMaxAttempts) { this.Tracer = tracer; + this.configReadDelay = configReadDelay ?? GitConfigReadRetry.Delay; + this.configReadMaxAttempts = configReadMaxAttempts; InitNative(); - IntPtr repoHandle; - if (TryOpenRepo(repoPath, out repoHandle) != Native.ResultCode.Success) + // Opening the repo parses .git\config, so it is subject to the same transient failures as a config read. + IntPtr repoHandle = IntPtr.Zero; + string reason = null; + Native.ResultCode openResult = GitConfigReadRetry.InvokeResult( + "repository open", + () => + { + Native.ResultCode result = TryOpenRepo(repoPath, out repoHandle); + reason = result == Native.ResultCode.Success ? null : GetLastNativeError(); + return result; + }, + (Native.ResultCode result) => result != Native.ResultCode.Success && GitConfigReadRetry.IsTransientFileAccessError(reason), + tracer, + this.configReadDelay, + this.configReadMaxAttempts); + + if (openResult != Native.ResultCode.Success) { - string reason = GetLastNativeError(); string message = "Couldn't open repo at " + repoPath + ": " + reason; tracer.RelatedWarning(message); @@ -62,8 +85,15 @@ protected LibGit2Repo() } protected LibGit2Repo(ITracer tracer) + : this(tracer, GitConfigReadRetry.Delay) + { + } + + protected LibGit2Repo(ITracer tracer, Action configReadDelay) { this.Tracer = tracer; + this.configReadDelay = configReadDelay ?? GitConfigReadRetry.Delay; + this.configReadMaxAttempts = GitConfigReadRetry.MaxAttempts; } ~LibGit2Repo() @@ -274,6 +304,17 @@ public virtual string[] GetMissingSubTrees(string treeSha) /// Name of the config entry /// The config value, or null if not found. public virtual string GetConfigString(string name) + { + return GitConfigReadRetry.InvokeCatching( + name, + () => this.GetConfigStringOnce(name), + GitConfigReadRetry.IsTransientLibGit2Error, + this.Tracer, + this.configReadDelay, + this.configReadMaxAttempts); + } + + protected virtual string GetConfigStringOnce(string name) { IntPtr configHandle; if (Native.Config.GetConfig(out configHandle, this.RepoHandle) != Native.ResultCode.Success) @@ -323,6 +364,17 @@ public virtual string GetConfigString(string name) } public virtual bool? GetConfigBool(string name) + { + return GitConfigReadRetry.InvokeCatching( + name, + () => this.GetConfigBoolOnce(name), + GitConfigReadRetry.IsTransientLibGit2Error, + this.Tracer, + this.configReadDelay, + this.configReadMaxAttempts); + } + + protected virtual bool? GetConfigBoolOnce(string name) { IntPtr configHandle; if (Native.Config.GetConfig(out configHandle, this.RepoHandle) != Native.ResultCode.Success) @@ -375,13 +427,14 @@ public bool GetConfigBoolOrDefault(string key, bool defaultValue) /// LibGit2RepoInvoker.InitializeSharedRepo forces the object store to load, which is /// wasted work when all that's needed is a single config value. Falls back to /// if the repo can't be opened or the read fails for any - /// reason. + /// reason. bounds the transient-failure retries + /// for latency-sensitive callers. /// - public static bool GetConfigBoolOrDefault(ITracer tracer, string repoPath, string key, bool defaultValue) + public static bool GetConfigBoolOrDefault(ITracer tracer, string repoPath, string key, bool defaultValue, int configReadMaxAttempts = GitConfigReadRetry.MaxAttempts) { try { - using (LibGit2Repo repo = new LibGit2Repo(tracer, repoPath)) + using (LibGit2Repo repo = new LibGit2Repo(tracer, repoPath, GitConfigReadRetry.Delay, configReadMaxAttempts)) { return repo.GetConfigBoolOrDefault(key, defaultValue); } @@ -400,14 +453,46 @@ public static bool GetConfigBoolOrDefault(ITracer tracer, string repoPath, strin } public void ForEachMultiVarConfig(string key, MultiVarConfigCallback callback) + { + List values = GitConfigReadRetry.InvokeCatching( + key, + () => this.GetMultiVarConfigValues(key), + GitConfigReadRetry.IsTransientLibGit2Error, + this.Tracer, + this.configReadDelay, + this.configReadMaxAttempts); + + foreach (string value in values) + { + callback(value); + } + } + + public static void ForEachMultiVarConfigInGlobalAndSystemConfig(string key, MultiVarConfigCallback callback) + { + List values = GitConfigReadRetry.InvokeCatching( + key, + () => GetGlobalAndSystemMultiVarConfigValues(key), + GitConfigReadRetry.IsTransientLibGit2Error, + NullTracer.Instance, + GitConfigReadRetry.Delay); + + foreach (string value in values) + { + callback(value); + } + } + + protected virtual List GetMultiVarConfigValues(string key) { if (Native.Config.GetConfig(out IntPtr configHandle, this.RepoHandle) != Native.ResultCode.Success) { throw new LibGit2Exception($"Failed to get config handle: {Native.GetLastError()}"); } + try { - ForEachMultiVarConfig(configHandle, key, callback); + return GetMultiVarConfigValues(configHandle, key); } finally { @@ -415,15 +500,16 @@ public void ForEachMultiVarConfig(string key, MultiVarConfigCallback callback) } } - public static void ForEachMultiVarConfigInGlobalAndSystemConfig(string key, MultiVarConfigCallback callback) + private static List GetGlobalAndSystemMultiVarConfigValues(string key) { if (Native.Config.GetGlobalAndSystemConfig(out IntPtr configHandle) != Native.ResultCode.Success) { throw new LibGit2Exception($"Failed to get global and system config handle: {Native.GetLastError()}"); } + try { - ForEachMultiVarConfig(configHandle, key, callback); + return GetMultiVarConfigValues(configHandle, key); } finally { @@ -431,14 +517,15 @@ public static void ForEachMultiVarConfigInGlobalAndSystemConfig(string key, Mult } } - private static void ForEachMultiVarConfig(IntPtr configHandle, string key, MultiVarConfigCallback callback) + private static List GetMultiVarConfigValues(IntPtr configHandle, string key) { + List values = new List(); Native.Config.GitConfigMultivarCallback nativeCallback = (entryPtr, payload) => { try { - var entry = Marshal.PtrToStructure(entryPtr); - callback(entry.GetValue()); + Native.Config.GitConfigEntry entry = Marshal.PtrToStructure(entryPtr); + values.Add(entry.GetValue()); } catch (Exception) { @@ -455,6 +542,8 @@ private static void ForEachMultiVarConfig(IntPtr configHandle, string key, Multi { throw new LibGit2Exception($"Failed to get multivar config for '{key}': {Native.GetLastError()}"); } + + return values; } /// diff --git a/GVFS/GVFS.Common/GitStatusCacheConfig.cs b/GVFS/GVFS.Common/GitStatusCacheConfig.cs index 82598fb4ac..415f1e40a1 100644 --- a/GVFS/GVFS.Common/GitStatusCacheConfig.cs +++ b/GVFS/GVFS.Common/GitStatusCacheConfig.cs @@ -25,7 +25,7 @@ public GitStatusCacheConfig(TimeSpan backOffTime) public static bool TryLoadFromGitConfig(ITracer tracer, Enlistment enlistment, out GitStatusCacheConfig gitStatusCacheConfig, out string error) { - return TryLoadFromGitConfig(tracer, new GitProcess(enlistment), out gitStatusCacheConfig, out error); + return TryLoadFromGitConfig(tracer, enlistment.CreateGitProcess(tracer), out gitStatusCacheConfig, out error); } public static bool TryLoadFromGitConfig(ITracer tracer, GitProcess git, out GitStatusCacheConfig gitStatusCacheConfig, out string error) diff --git a/GVFS/GVFS.Common/Http/CacheServerResolver.cs b/GVFS/GVFS.Common/Http/CacheServerResolver.cs index 7a0c6e1a73..34a9c17215 100644 --- a/GVFS/GVFS.Common/Http/CacheServerResolver.cs +++ b/GVFS/GVFS.Common/Http/CacheServerResolver.cs @@ -18,10 +18,10 @@ public CacheServerResolver( this.enlistment = enlistment; } - public static CacheServerInfo GetCacheServerFromConfig(Enlistment enlistment) + public static CacheServerInfo GetCacheServerFromConfig(Enlistment enlistment, ITracer tracer = null) { - GitProcess git = enlistment.CreateGitProcess(); - string url = GetUrlFromConfig(enlistment); + GitProcess git = CreateGitProcess(enlistment, tracer); + string url = GetUrlFromConfig(enlistment, tracer); string prefetchCacheServerUrl = GetEndpointUrlFromConfig(git, GVFSConstants.GitConfig.PrefetchCacheServer); string getCacheServerUrl = GetEndpointUrlFromConfig(git, GVFSConstants.GitConfig.GetCacheServer); string postCacheServerUrl = GetEndpointUrlFromConfig(git, GVFSConstants.GitConfig.PostCacheServer); @@ -36,9 +36,9 @@ public static CacheServerInfo GetCacheServerFromConfig(Enlistment enlistment) sizesCacheServerUrl); } - public static string GetUrlFromConfig(Enlistment enlistment) + public static string GetUrlFromConfig(Enlistment enlistment, ITracer tracer = null) { - GitProcess git = enlistment.CreateGitProcess(); + GitProcess git = CreateGitProcess(enlistment, tracer); // TODO 1057500: Remove support for encoded-repo-url cache config setting return @@ -170,6 +170,13 @@ private static string GetValueFromConfig(GitProcess git, string configName, bool return value; } + private static GitProcess CreateGitProcess(Enlistment enlistment, ITracer tracer) + { + return tracer == null + ? enlistment.CreateGitProcess() + : enlistment.CreateGitProcess(tracer); + } + private static string GetEndpointUrlFromConfig(GitProcess git, string configName) { string url = GetValueFromConfig(git, configName, localOnly: true); diff --git a/GVFS/GVFS.Common/RetryConfig.cs b/GVFS/GVFS.Common/RetryConfig.cs index 4462a18bcc..1fa7b070f4 100644 --- a/GVFS/GVFS.Common/RetryConfig.cs +++ b/GVFS/GVFS.Common/RetryConfig.cs @@ -38,7 +38,7 @@ public int MaxAttempts public static bool TryLoadFromGitConfig(ITracer tracer, Enlistment enlistment, out RetryConfig retryConfig, out string error) { - return TryLoadFromGitConfig(tracer, new GitProcess(enlistment), out retryConfig, out error); + return TryLoadFromGitConfig(tracer, enlistment.CreateGitProcess(tracer), out retryConfig, out error); } public static bool TryLoadFromGitConfig(ITracer tracer, GitProcess git, out RetryConfig retryConfig, out string error) diff --git a/GVFS/GVFS.Hooks/GVFS.Hooks.csproj b/GVFS/GVFS.Hooks/GVFS.Hooks.csproj index 3b996578e7..1e0c60aaa5 100644 --- a/GVFS/GVFS.Hooks/GVFS.Hooks.csproj +++ b/GVFS/GVFS.Hooks/GVFS.Hooks.csproj @@ -23,6 +23,9 @@ Common\Git\LibGit2Exception.cs + + Common\Git\GitConfigReadRetry.cs + Common\Git\LibGit2Repo.cs diff --git a/GVFS/GVFS.Hooks/Program.cs b/GVFS/GVFS.Hooks/Program.cs index 940b2cf37d..7a595a03c5 100644 --- a/GVFS/GVFS.Hooks/Program.cs +++ b/GVFS/GVFS.Hooks/Program.cs @@ -22,6 +22,9 @@ public partial class Program private const int PostCommandSpinnerDelayMs = 500; + // Hooks run on the git command path, so config reads retry at most once. + private const int ConfigReadMaxAttempts = 2; + private static string enlistmentRoot; private static string enlistmentPipename; private static string normalizedCurrentDirectory; @@ -171,11 +174,13 @@ private static bool HasShortFlag(string arg, string flag) private static bool ConfigurationAllowsHydrationStatus() { + // See ConfigReadMaxAttempts for why the retry budget is small here. return LibGit2Repo.GetConfigBoolOrDefault( NullTracer.Instance, normalizedCurrentDirectory, GVFSConstants.GitConfig.ShowHydrationStatus, - GVFSConstants.GitConfig.ShowHydrationStatusDefault); + GVFSConstants.GitConfig.ShowHydrationStatusDefault, + ConfigReadMaxAttempts); } /// @@ -517,7 +522,16 @@ private static string GetGitCommand(string[] args) private static bool IsAlias(string command) { - ProcessResult result = ProcessHelper.Run("git", "config --get alias." + 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.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index 0a5c930b5f..4e396efaa0 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -1900,7 +1900,7 @@ private bool TrySetRequiredGitConfigSettings() { Dictionary requiredSettings = RequiredGitConfig.GetRequiredSettings(this.enlistment); - GitProcess git = new GitProcess(this.enlistment); + GitProcess git = this.enlistment.CreateGitProcess(this.tracer); Dictionary existingConfigSettings; if (!git.TryGetAllConfig(localOnly: true, configSettings: out existingConfigSettings)) diff --git a/GVFS/GVFS.Mount/InProcessMountVerb.cs b/GVFS/GVFS.Mount/InProcessMountVerb.cs index 16328f60d6..6bb0cad92e 100644 --- a/GVFS/GVFS.Mount/InProcessMountVerb.cs +++ b/GVFS/GVFS.Mount/InProcessMountVerb.cs @@ -112,7 +112,7 @@ public void Execute() JsonTracer tracer = this.CreateTracer(enlistment, verbosity, keywords); - CacheServerInfo cacheServer = CacheServerResolver.GetCacheServerFromConfig(enlistment); + CacheServerInfo cacheServer = CacheServerResolver.GetCacheServerFromConfig(enlistment, tracer); tracer.WriteStartEvent( enlistment.WorkingDirectoryRoot, diff --git a/GVFS/GVFS.UnitTests/Common/ConfigReaderTracerTests.cs b/GVFS/GVFS.UnitTests/Common/ConfigReaderTracerTests.cs new file mode 100644 index 0000000000..a361644279 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Common/ConfigReaderTracerTests.cs @@ -0,0 +1,82 @@ +using GVFS.Common; +using GVFS.Common.Git; +using GVFS.Common.Http; +using GVFS.Common.Tracing; +using GVFS.Tests.Should; +using GVFS.UnitTests.Mock.Common; +using GVFS.UnitTests.Mock.Git; +using NUnit.Framework; +using System.Collections.Generic; + +namespace GVFS.UnitTests.Common +{ + // The mount process passes its tracer to every config reader so that retries are traced. + [TestFixture] + public class ConfigReaderTracerTests + { + [TestCase] + public void CacheServerResolverCreatesItsGitProcessWithTheTracer() + { + MockTracer tracer = new MockTracer(); + TracerCapturingEnlistment enlistment = this.CreateEnlistment(); + + CacheServerResolver.GetCacheServerFromConfig(enlistment, tracer); + + (enlistment.CapturedTracers.Count > 0).ShouldBeTrue(); + foreach (ITracer capturedTracer in enlistment.CapturedTracers) + { + capturedTracer.ShouldBeSameAs(tracer); + } + } + + [TestCase] + public void RetryConfigCreatesItsGitProcessWithTheTracer() + { + MockTracer tracer = new MockTracer(); + TracerCapturingEnlistment enlistment = this.CreateEnlistment(); + + RetryConfig.TryLoadFromGitConfig(tracer, enlistment, out RetryConfig _, out string _); + + enlistment.CapturedTracers.Count.ShouldEqual(1); + enlistment.CapturedTracers[0].ShouldBeSameAs(tracer); + } + + [TestCase] + public void GitStatusCacheConfigCreatesItsGitProcessWithTheTracer() + { + MockTracer tracer = new MockTracer(); + TracerCapturingEnlistment enlistment = this.CreateEnlistment(); + + GitStatusCacheConfig.TryLoadFromGitConfig(tracer, enlistment, out GitStatusCacheConfig _, out string _); + + enlistment.CapturedTracers.Count.ShouldEqual(1); + enlistment.CapturedTracers[0].ShouldBeSameAs(tracer); + } + + private TracerCapturingEnlistment CreateEnlistment() + { + MockGitProcess git = new MockGitProcess(); + git.SetExpectedCommandResult( + "config", + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.GenericFailureCode), + matchPrefix: true); + return new TracerCapturingEnlistment(git); + } + + private class TracerCapturingEnlistment : MockGVFSEnlistment + { + public TracerCapturingEnlistment(MockGitProcess gitProcess) + : base(gitProcess) + { + } + + public List CapturedTracers { get; } = new List(); + + public override GitProcess CreateGitProcess(ITracer tracer) + { + this.CapturedTracers.Add(tracer); + return base.CreateGitProcess(tracer); + } + } + } +} diff --git a/GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs b/GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs index 843d423b5f..c6932cadac 100644 --- a/GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs +++ b/GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs @@ -3,6 +3,7 @@ using GVFS.UnitTests.Mock.Common; using NUnit.Framework; using System; +using System.Collections.Generic; using System.IO; namespace GVFS.UnitTests.Common @@ -10,6 +11,9 @@ namespace GVFS.UnitTests.Common [TestFixture] public class LibGit2RepoConfigLookupTests { + private const string LockedConfigError = + "Failed to get config handle: could not open 'C:\\repo\\.git\\config': The process cannot access the file because it is being used by another process."; + [TestCase] public void GetConfigBoolOrDefaultOnRepoReturnsConfiguredValue() { @@ -68,6 +72,145 @@ public void GetConfigBoolOrDefaultOnRepoReturnsDefaultOnInvalidDataExceptionAndL } } + [TestCase] + public void GetConfigBoolRetriesTransientLibGit2Failure() + { + MockTracer tracer = new MockTracer(); + int delayCount = 0; + Queue results = new Queue(); + results.Enqueue(new LibGit2Exception(LockedConfigError)); + results.Enqueue(true); + + using (MockConfigRepo repo = new MockConfigRepo(tracer, results, delay => ++delayCount)) + { + repo.GetConfigBool("gvfs.test").ShouldEqual(true); + } + + delayCount.ShouldEqual(1); + tracer.RelatedWarningEvents.Count.ShouldEqual(1); + tracer.RelatedWarningEvents[0].ShouldContain("Transient failure while reading git config"); + } + + [TestCase] + public void GetConfigStringRetriesTransientLibGit2Failure() + { + int delayCount = 0; + Queue results = new Queue(); + results.Enqueue(new LibGit2Exception(LockedConfigError)); + results.Enqueue("configured-value"); + + using (MockConfigRepo repo = new MockConfigRepo(new MockTracer(), results, delay => ++delayCount)) + { + repo.GetConfigString("gvfs.test").ShouldEqual("configured-value"); + } + + delayCount.ShouldEqual(1); + } + + [TestCase] + public void GetConfigBoolRethrowsAfterRetryBudget() + { + MockTracer tracer = new MockTracer(); + int delayCount = 0; + Queue results = new Queue(); + for (int i = 0; i < GitConfigReadRetry.MaxAttempts; ++i) + { + results.Enqueue(new LibGit2Exception(LockedConfigError)); + } + + using (MockConfigRepo repo = new MockConfigRepo(tracer, results, delay => ++delayCount)) + { + Assert.Throws(() => repo.GetConfigBool("gvfs.test")); + } + + delayCount.ShouldEqual(GitConfigReadRetry.MaxAttempts - 1); + results.Count.ShouldEqual(0); + tracer.RelatedWarningEvents.Count.ShouldEqual(GitConfigReadRetry.MaxAttempts); + tracer.RelatedWarningEvents[GitConfigReadRetry.MaxAttempts - 1].ShouldContain("persisted after all attempts"); + } + + [TestCase] + public void GetConfigBoolDoesNotRetryNonTransientLibGit2Failure() + { + int delayCount = 0; + Queue results = new Queue(); + results.Enqueue(new LibGit2Exception("Failed to get config handle: could not open 'C:\\repo\\.git\\config': The system cannot find the file specified.")); + results.Enqueue(true); + + using (MockConfigRepo repo = new MockConfigRepo(new MockTracer(), results, delay => ++delayCount)) + { + Assert.Throws(() => repo.GetConfigBool("gvfs.test")); + } + + delayCount.ShouldEqual(0); + results.Count.ShouldEqual(1); + } + + [TestCase] + public void ForEachMultiVarConfigInvokesCallbackOncePerValueAcrossRetries() + { + int delayCount = 0; + Queue results = new Queue(); + results.Enqueue(new LibGit2Exception(LockedConfigError)); + results.Enqueue(new List { "first", "second" }); + List callbackValues = new List(); + + using (MockConfigRepo repo = new MockConfigRepo(new MockTracer(), results, delay => ++delayCount)) + { + repo.ForEachMultiVarConfig("safe.directory", callbackValues.Add); + } + + delayCount.ShouldEqual(1); + callbackValues.ShouldMatchInOrder("first", "second"); + } + + [TestCase] + public void RepositoryOpenRetriesTransientFailure() + { + MockTracer tracer = new MockTracer(); + int delayCount = 0; + Queue openErrors = new Queue(); + openErrors.Enqueue("could not open 'C:\\repo\\.git\\config': The process cannot access the file because it is being used by another process."); + openErrors.Enqueue("could not open 'C:\\repo\\.git\\config': Permission denied"); + + using (MockOpenRepo.Create(tracer, openErrors, delay => ++delayCount, GitConfigReadRetry.MaxAttempts)) + { + } + + delayCount.ShouldEqual(2); + openErrors.Count.ShouldEqual(0); + } + + [TestCase] + public void RepositoryOpenStopsAfterConfiguredAttempts() + { + int delayCount = 0; + Queue openErrors = new Queue(); + for (int i = 0; i < 5; ++i) + { + openErrors.Enqueue("could not open 'C:\\repo\\.git\\config': Permission denied"); + } + + Assert.Throws( + () => MockOpenRepo.Create(new MockTracer(), openErrors, delay => ++delayCount, maxAttempts: 2)); + + delayCount.ShouldEqual(1); + openErrors.Count.ShouldEqual(3); + } + + [TestCase] + public void RepositoryOpenDoesNotRetryNonTransientFailure() + { + int delayCount = 0; + Queue openErrors = new Queue(); + openErrors.Enqueue("could not find repository at 'C:\\repo'"); + + Assert.Throws( + () => MockOpenRepo.Create(new MockTracer(), openErrors, delay => ++delayCount, GitConfigReadRetry.MaxAttempts)); + + delayCount.ShouldEqual(0); + } + [TestCase] public void GetConfigBoolOrDefaultOnPathReturnsDefaultForMissingRepoAndLogsExactlyOnce() { @@ -100,6 +243,7 @@ private class MockConfigRepo : LibGit2Repo { private readonly bool? value; private readonly Exception exceptionToThrow; + private readonly Queue results; public MockConfigRepo(MockTracer tracer, bool? value) : base(tracer) @@ -113,8 +257,25 @@ public MockConfigRepo(MockTracer tracer, Exception exceptionToThrow) this.exceptionToThrow = exceptionToThrow; } - public override bool? GetConfigBool(string name) + public MockConfigRepo(MockTracer tracer, Queue results, Action configReadDelay) + : base(tracer, configReadDelay) { + this.results = results; + } + + protected override bool? GetConfigBoolOnce(string name) + { + if (this.results != null) + { + object result = this.results.Dequeue(); + if (result is Exception exception) + { + throw exception; + } + + return (bool?)result; + } + if (this.exceptionToThrow != null) { throw this.exceptionToThrow; @@ -123,6 +284,28 @@ public MockConfigRepo(MockTracer tracer, Exception exceptionToThrow) return this.value; } + protected override string GetConfigStringOnce(string name) + { + object result = this.results.Dequeue(); + if (result is Exception exception) + { + throw exception; + } + + return (string)result; + } + + protected override List GetMultiVarConfigValues(string key) + { + object result = this.results.Dequeue(); + if (result is Exception exception) + { + throw exception; + } + + return (List)result; + } + // This mock never calls the base LibGit2Repo(tracer, repoPath) constructor, so it // never initializes native libgit2 state or a real RepoHandle. Override Dispose(bool) // to skip the base implementation's native Free/Shutdown calls, which would otherwise @@ -132,5 +315,58 @@ protected override void Dispose(bool disposing) { } } + + // Exercises the real LibGit2Repo(tracer, repoPath, ...) constructor with the native open + // replaced. State is passed through thread-static fields because the base constructor + // calls the virtual members before derived constructors run. + private class MockOpenRepo : LibGit2Repo + { + [ThreadStatic] + private static Queue pendingOpenErrors; + + private MockOpenRepo(MockTracer tracer, Action configReadDelay, int maxAttempts) + : base(tracer, "C:\\repo", configReadDelay, maxAttempts) + { + } + + private string LastOpenError { get; set; } + + public static MockOpenRepo Create(MockTracer tracer, Queue openErrors, Action configReadDelay, int maxAttempts) + { + pendingOpenErrors = openErrors; + try + { + return new MockOpenRepo(tracer, configReadDelay, maxAttempts); + } + finally + { + pendingOpenErrors = null; + } + } + + protected override void InitNative() + { + } + + protected override void ShutdownNative() + { + } + + protected override Native.ResultCode TryOpenRepo(string path, out IntPtr repoHandle) + { + repoHandle = IntPtr.Zero; + this.LastOpenError = pendingOpenErrors.Count > 0 ? pendingOpenErrors.Dequeue() : null; + return this.LastOpenError == null ? Native.ResultCode.Success : Native.ResultCode.Failure; + } + + protected override string GetLastNativeError() + { + return this.LastOpenError; + } + + protected override void Dispose(bool disposing) + { + } + } } } diff --git a/GVFS/GVFS.UnitTests/Git/GitConfigReadRetryTests.cs b/GVFS/GVFS.UnitTests/Git/GitConfigReadRetryTests.cs new file mode 100644 index 0000000000..682b70bef1 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Git/GitConfigReadRetryTests.cs @@ -0,0 +1,138 @@ +using GVFS.Common.Git; +using GVFS.Tests.Should; +using GVFS.UnitTests.Mock.Common; +using NUnit.Framework; +using System; +using System.Collections.Generic; + +namespace GVFS.UnitTests.Git +{ + [TestFixture] + public class GitConfigReadRetryTests + { + [TestCase("warning: unable to access '.git/config': Permission denied", true)] + [TestCase("warning: unable to access '.git/config': Permission denied\r\nfatal: unknown error occurred while reading the configuration files", true)] + [TestCase("could not open 'C:\\repo\\.git\\config': The process cannot access the file because it is being used by another process.", true)] + [TestCase("error: unable to access 'C:/repo/.git/config': Device or resource busy", true)] + [TestCase("fatal: unable to access 'C:/Permission denied/.git/config': No such file or directory", false)] + [TestCase("error: could not open 'C:/Device or resource busy/config': The system cannot find the file specified.", false)] + [TestCase("Permission denied", false)] + [TestCase("fatal: bad config line 4 in file .git/config", false)] + [TestCase("", false)] + [TestCase(null, false)] + public void IsTransientFileAccessErrorMatchesOnlyTheFailureReason(string error, bool expected) + { + GitConfigReadRetry.IsTransientFileAccessError(error).ShouldEqual(expected); + } + + [TestCase] + public void IsTransientFileAccessErrorHonorsRequiredLinePhrase() + { + const string Error = "warning: could not open 'config': Permission denied"; + + GitConfigReadRetry.IsTransientFileAccessError(Error).ShouldBeTrue(); + GitConfigReadRetry.IsTransientFileAccessError(Error, lineMustContain: "unable to access").ShouldBeFalse(); + } + + [TestCase] + public void InvokeResultStopsAtTheRequestedAttemptLimit() + { + MockTracer tracer = new MockTracer(); + int reads = 0; + int delays = 0; + + int result = GitConfigReadRetry.InvokeResult( + "gvfs.test", + () => ++reads, + (int _) => true, + tracer, + delay => ++delays, + maxAttempts: 2); + + result.ShouldEqual(2); + reads.ShouldEqual(2); + delays.ShouldEqual(1); + } + + [TestCase] + public void InvokeResultDoesNotRetryWhenOneAttemptIsAllowed() + { + int reads = 0; + int delays = 0; + + GitConfigReadRetry.InvokeResult( + "gvfs.test", + () => ++reads, + (int _) => true, + new MockTracer(), + delay => ++delays, + maxAttempts: 1); + + reads.ShouldEqual(1); + delays.ShouldEqual(0); + } + + [TestCase] + public void InvokeResultUsesExponentialDelays() + { + List delays = new List(); + + GitConfigReadRetry.InvokeResult( + "gvfs.test", + () => 0, + (int _) => true, + new MockTracer(), + delays.Add); + + delays.ShouldMatchInOrder( + TimeSpan.FromMilliseconds(20), + TimeSpan.FromMilliseconds(40), + TimeSpan.FromMilliseconds(80), + TimeSpan.FromMilliseconds(160)); + } + + [TestCase] + public void InvokeCatchingRethrowsTheOriginalExceptionAfterTheLastAttempt() + { + int reads = 0; + InvalidOperationException lastException = null; + + Assert.Throws( + () => GitConfigReadRetry.InvokeCatching( + "gvfs.test", + () => + { + lastException = new InvalidOperationException("attempt " + ++reads); + throw lastException; + }, + (Exception _) => true, + new MockTracer(), + delay => { }, + maxAttempts: 3)); + + reads.ShouldEqual(3); + lastException.Message.ShouldEqual("attempt 3"); + } + + [TestCase] + public void TracedConfigNameDoesNotContainUrlCredentials() + { + MockTracer tracer = new MockTracer(); + + GitConfigReadRetry.InvokeResult( + "gvfs.user:token@example.com.repo.cache-server-url", + () => 0, + (int _) => true, + tracer, + delay => { }, + maxAttempts: 2); + + tracer.RelatedWarningEvents.Count.ShouldEqual(2); + foreach (string warning in tracer.RelatedWarningEvents) + { + warning.Contains("token").ShouldBeFalse(); + warning.ShouldContain("redacted"); + } + } + } +} diff --git a/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs b/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs index 8182f8dfb5..568940d757 100644 --- a/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs @@ -1,7 +1,10 @@ using GVFS.Common.Git; using GVFS.Tests.Should; using GVFS.UnitTests.Mock.Common; +using GVFS.UnitTests.Mock.Git; using NUnit.Framework; +using System; +using System.Collections.Generic; using System.Diagnostics; namespace GVFS.UnitTests.Git @@ -9,6 +12,12 @@ namespace GVFS.UnitTests.Git [TestFixture] public class GitProcessTests { + // What git.exe prints on Windows while another process holds .git\config. + private const string LockedConfigErrors = + "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"; + [TestCase] public void BoundedGitOutputBuffer_KeepsShortOutput() { @@ -114,6 +123,163 @@ public void ResultHasNoErrors() result.StderrContainsErrors().ShouldBeFalse(); } + [TestCase] + public void GetFromLocalConfigRetriesTransientFailureAndReturnsValue() + { + MockTracer tracer = new MockTracer(); + int delayCount = 0; + MockGitProcess process = new MockGitProcess(tracer, delay => ++delayCount); + Queue results = new Queue(); + results.Enqueue(new GitProcess.Result( + string.Empty, + LockedConfigErrors, + GitProcess.Result.GenericFailureCode)); + results.Enqueue(new GitProcess.Result("configured-value\n", string.Empty, GitProcess.Result.SuccessCode)); + process.SetExpectedCommandResult("config --local gvfs.test", () => results.Dequeue()); + + GitProcess.ConfigResult result = process.GetFromLocalConfig("gvfs.test"); + + result.TryParseAsString(out string value, out string error).ShouldBeTrue(error); + value.ShouldEqual("configured-value"); + process.CommandsRun.Count.ShouldEqual(2); + delayCount.ShouldEqual(1); + tracer.RelatedWarningEvents.Count.ShouldEqual(1); + tracer.RelatedWarningEvents[0].ShouldContain("Transient failure while reading git config"); + } + + [TestCase] + public void GetFromLocalConfigDoesNotRetryMissingKey() + { + int delayCount = 0; + MockGitProcess process = new MockGitProcess(new MockTracer(), delay => ++delayCount); + process.SetExpectedCommandResult( + "config --local gvfs.test", + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.GenericFailureCode)); + + GitProcess.ConfigResult result = process.GetFromLocalConfig("gvfs.test"); + + result.TryParseAsString(out string value, out string error).ShouldBeTrue(error); + value.ShouldBeNull(); + process.CommandsRun.Count.ShouldEqual(1); + delayCount.ShouldEqual(0); + } + + [TestCase] + public void GetFromLocalConfigStopsAfterRetryBudget() + { + MockTracer tracer = new MockTracer(); + int delayCount = 0; + MockGitProcess process = new MockGitProcess(tracer, delay => ++delayCount); + process.SetExpectedCommandResult( + "config --local gvfs.test", + () => new GitProcess.Result(string.Empty, LockedConfigErrors, GitProcess.Result.GenericFailureCode)); + + GitProcess.ConfigResult result = process.GetFromLocalConfig("gvfs.test"); + + result.TryParseAsString(out string _, out string error).ShouldBeFalse(); + error.ShouldContain("Permission denied"); + process.CommandsRun.Count.ShouldEqual(GitConfigReadRetry.MaxAttempts); + delayCount.ShouldEqual(GitConfigReadRetry.MaxAttempts - 1); + tracer.RelatedWarningEvents.Count.ShouldEqual(GitConfigReadRetry.MaxAttempts); + tracer.RelatedWarningEvents[GitConfigReadRetry.MaxAttempts - 1].ShouldContain("persisted after all attempts"); + } + + [TestCase] + public void GetFromLocalConfigDoesNotRetryWhenOnlyThePathContainsATransientPhrase() + { + int delayCount = 0; + MockGitProcess process = new MockGitProcess(new MockTracer(), delay => ++delayCount); + process.SetExpectedCommandResult( + "config --local gvfs.test", + () => new GitProcess.Result( + string.Empty, + "fatal: unable to access 'C:/Permission denied/.git/config': No such file or directory", + GitProcess.Result.GenericFailureCode)); + + process.GetFromLocalConfig("gvfs.test").TryParseAsString(out string _, out string _).ShouldBeFalse(); + + process.CommandsRun.Count.ShouldEqual(1); + delayCount.ShouldEqual(0); + } + + [TestCase] + public void GetFromConfigRetriesTransientFailure() + { + MockGitProcess process = this.CreateProcessWithTransientThenSuccess( + "config gvfs.test", + "configured-value\n", + out Func getDelayCount); + + GitProcess.ConfigResult result = process.GetFromConfig("gvfs.test", forceOutsideEnlistment: true); + + result.TryParseAsString(out string value, out string error).ShouldBeTrue(error); + value.ShouldEqual("configured-value"); + getDelayCount().ShouldEqual(1); + } + + [TestCase] + public void GetOriginUrlRetriesTransientFailure() + { + MockGitProcess process = this.CreateProcessWithTransientThenSuccess( + "config --local remote.origin.url", + "https://example.com/repo\n", + out Func getDelayCount); + + GitProcess.ConfigResult result = process.GetOriginUrl(); + + result.TryParseAsString(out string value, out string error).ShouldBeTrue(error); + value.ShouldEqual("https://example.com/repo"); + getDelayCount().ShouldEqual(1); + } + + [TestCase] + public void TryGetAllConfigRetriesTransientFailure() + { + MockGitProcess process = this.CreateProcessWithTransientThenSuccess( + "config --list --local", + "gvfs.test=configured-value\n", + out Func getDelayCount); + + process.TryGetAllConfig(localOnly: true, configSettings: out Dictionary settings).ShouldBeTrue(); + + settings.ContainsKey("gvfs.test").ShouldBeTrue(); + getDelayCount().ShouldEqual(1); + } + + [TestCase] + public void TryGetConfigUrlMatchRetriesTransientFailure() + { + MockGitProcess process = this.CreateProcessWithTransientThenSuccess( + "config --get-urlmatch http https://example.com/repo", + "http.proxy configured-value\n", + out Func getDelayCount); + + process.TryGetConfigUrlMatch("http", "https://example.com/repo", out Dictionary settings).ShouldBeTrue(); + + settings.ContainsKey("http.proxy").ShouldBeTrue(); + getDelayCount().ShouldEqual(1); + } + + [TestCase] + public void GetFromLocalConfigDoesNotRetryNonTransientFailure() + { + int delayCount = 0; + MockGitProcess process = new MockGitProcess(new MockTracer(), delay => ++delayCount); + process.SetExpectedCommandResult( + "config --local gvfs.test", + () => new GitProcess.Result( + string.Empty, + "fatal: bad config line 4 in file .git/config", + GitProcess.Result.GenericFailureCode)); + + GitProcess.ConfigResult result = process.GetFromLocalConfig("gvfs.test"); + + result.TryParseAsString(out string _, out string error).ShouldBeFalse(); + error.ShouldContain("bad config line"); + process.CommandsRun.Count.ShouldEqual(1); + delayCount.ShouldEqual(0); + } + [TestCase] public void ResultHasWarnings() { @@ -402,5 +568,17 @@ public void QuoteGitPath_RoundTripThroughProcess() } } } + + private MockGitProcess CreateProcessWithTransientThenSuccess(string command, string successOutput, out Func getDelayCount) + { + int delayCount = 0; + MockGitProcess process = new MockGitProcess(new MockTracer(), delay => ++delayCount); + Queue results = new Queue(); + results.Enqueue(new GitProcess.Result(string.Empty, LockedConfigErrors, GitProcess.Result.GenericFailureCode)); + results.Enqueue(new GitProcess.Result(successOutput, string.Empty, GitProcess.Result.SuccessCode)); + process.SetExpectedCommandResult(command, () => results.Dequeue()); + getDelayCount = () => delayCount; + return process; + } } } diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs index 2292149b65..c179372a1c 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs @@ -1,5 +1,6 @@ using GVFS.Common; using GVFS.Common.Git; +using GVFS.Common.Tracing; using GVFS.UnitTests.Mock.Git; using System.IO; @@ -39,5 +40,10 @@ public override GitProcess CreateGitProcess() { return this.gitProcess ?? new MockGitProcess(); } + + public override GitProcess CreateGitProcess(ITracer tracer) + { + return this.CreateGitProcess(); + } } } diff --git a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs index cf786a9835..03f229da31 100644 --- a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs +++ b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs @@ -15,7 +15,12 @@ public class MockGitProcess : GitProcess private List expectedCommandInfos = new List(); public MockGitProcess() - : base(new MockGVFSEnlistment()) + : this(NullTracer.Instance, GitConfigReadRetry.Delay) + { + } + + internal MockGitProcess(ITracer tracer, Action configReadDelay) + : base(new MockGVFSEnlistment(), tracer, configReadDelay) { this.CommandsRun = new List(); this.InvocationsRun = new List(); From 69cbeccc4feeee6f147f51e856a3fbc588877a35 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 21 Oct 2026 13:36:18 -0700 Subject: [PATCH 2/3] Read RetryConfig settings with libgit2 RetryConfig started git.exe once for gvfs.max-retries and once for gvfs.timeout-seconds. Read both settings in process with libgit2. Open a transient repository without its object store when the working directory exists. Otherwise, read the global, XDG, system, and ProgramData config, as GitProcess.GetFromConfig does before clone creates the repository. The reads use the GitConfigReadRetry policy. libgit2 does not read GIT_CONFIG_PARAMETERS, GIT_CONFIG_COUNT, GIT_CONFIG_GLOBAL, GIT_CONFIG_SYSTEM, GIT_CONFIG_NOSYSTEM, or GIT_CONFIG. It also does not use GIT_DIR, which git.exe uses to select a repository when no working directory exists. When the environment sets any of these variables, read the settings with git.exe. libgit2 evaluates only the gitdir, gitdir/i, and onbranch includeIf conditions. It skips other conditions, such as hasconfig:remote.*.url, without an error. When the config has such a condition, the libgit2 read fails, and git.exe reads the setting. When libgit2 cannot read a setting for another reason, for example because it rejects the repository's ownership, read the setting with git.exe and trace RetryConfig_ReadWithGit. After a transient file access failure, do not read again with git.exe, because libgit2 already used the retry budget. Move integer setting parsing to GitConfigHelper, so both readers report the same default, minimum, and parse errors. When the LibGit2Repo constructor fails, it releases its libgit2 reference. Suppress finalization in that case, so the finalizer does not release the reference a second time. Assisted-by: Claude Opus 5.5 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/GitConfigHelper.cs | 30 +- GVFS/GVFS.Common/Git/GitProcess.cs | 21 +- GVFS/GVFS.Common/Git/LibGit2Repo.cs | 246 +++++++++++-- GVFS/GVFS.Common/RetryConfig.cs | 104 +++++- .../Common/ConfigReaderTracerTests.cs | 28 +- .../Common/GitConfigHelperTests.cs | 23 ++ .../Common/LibGit2RepoConfigLookupTests.cs | 42 +++ .../GVFS.UnitTests/Common/RetryConfigTests.cs | 325 ++++++++++++++++++ .../Mock/Git/IsolatedLibGit2Config.cs | 55 +++ 9 files changed, 815 insertions(+), 59 deletions(-) create mode 100644 GVFS/GVFS.UnitTests/Mock/Git/IsolatedLibGit2Config.cs diff --git a/GVFS/GVFS.Common/Git/GitConfigHelper.cs b/GVFS/GVFS.Common/Git/GitConfigHelper.cs index eb0b08c879..bd156e0b88 100644 --- a/GVFS/GVFS.Common/Git/GitConfigHelper.cs +++ b/GVFS/GVFS.Common/Git/GitConfigHelper.cs @@ -4,7 +4,7 @@ namespace GVFS.Common.Git { /// - /// Helper methods for git config-style file reading and parsing. + /// Helper methods for reading Git config files and parsing config values. /// public static class GitConfigHelper { @@ -30,6 +30,34 @@ public static bool TrySanitizeConfigFileLine(string fileLine, out string sanitiz return !string.IsNullOrWhiteSpace(sanitizedLine); } + /// + /// Parses an integer setting. A null or blank value selects . + /// + public static bool TryParseIntSetting(string configName, string valueString, int defaultValue, int minValue, out int value, out string error) + { + value = defaultValue; + error = string.Empty; + + if (string.IsNullOrWhiteSpace(valueString)) + { + return true; + } + + if (!int.TryParse(valueString, out value)) + { + error = string.Format("Misconfigured config setting {0}, could not parse value `{1}` as an int", configName, valueString); + return false; + } + + if (value < minValue) + { + error = string.Format("Invalid value {0} for setting {1}, value must be greater than or equal to {2}", value, configName, minValue); + return false; + } + + return true; + } + /// /// Get the settings for a section in a given config file. /// diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index ce96e30f72..615f2cbd0a 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -1451,32 +1451,13 @@ public bool TryParseAsString(out string value, out string error, string defaultV public bool TryParseAsInt(int defaultValue, int minValue, out int value, out string error) { value = defaultValue; - error = string.Empty; if (!this.TryParseAsString(out string valueString, out error)) { return false; } - if (string.IsNullOrWhiteSpace(valueString)) - { - // Use default value - return true; - } - - if (!int.TryParse(valueString, out value)) - { - error = string.Format("Misconfigured config setting {0}, could not parse value `{1}` as an int", this.configName, valueString); - return false; - } - - if (value < minValue) - { - error = string.Format("Invalid value {0} for setting {1}, value must be greater than or equal to {2}", value, this.configName, minValue); - return false; - } - - return true; + return GitConfigHelper.TryParseIntSetting(this.configName, valueString, defaultValue, minValue, out value, out error); } } } diff --git a/GVFS/GVFS.Common/Git/LibGit2Repo.cs b/GVFS/GVFS.Common/Git/LibGit2Repo.cs index d7de0dd5ef..f4d1bda8cc 100644 --- a/GVFS/GVFS.Common/Git/LibGit2Repo.cs +++ b/GVFS/GVFS.Common/Git/LibGit2Repo.cs @@ -1,5 +1,6 @@ using GVFS.Common.Tracing; using System; +using System.Collections; using System.Collections.Generic; using System.IO; using System.Runtime.InteropServices; @@ -27,6 +28,27 @@ namespace GVFS.Common.Git /// public class LibGit2Repo : IDisposable { + // git.exe reads config from these variables, or uses them to select config files. + // libgit2 does not. GIT_DIR selects the repository when no repository path is given. + private static readonly string[] ConfigEnvironmentVariablesLibGit2Ignores = + { + "GIT_CONFIG", + "GIT_CONFIG_COUNT", + "GIT_CONFIG_GLOBAL", + "GIT_CONFIG_NOSYSTEM", + "GIT_CONFIG_PARAMETERS", + "GIT_CONFIG_SYSTEM", + "GIT_DIR", + }; + + // git.exe also evaluates hasconfig:remote.*.url conditions. + private static readonly string[] IncludeConditionsLibGit2Evaluates = + { + "gitdir:", + "gitdir/i:", + "onbranch:", + }; + private bool disposedValue = false; private IntPtr odbHandle = IntPtr.Zero; private readonly Action configReadDelay; @@ -72,6 +94,10 @@ internal LibGit2Repo(ITracer tracer, string repoPath, Action configRea || !CheckSafeDirectoryConfigForCaseSensitivityIssue(tracer, repoPath, out repoHandle)) { ShutdownNative(); + + // The native reference is released, so the finalizer must not release it again. + this.disposedValue = true; + GC.SuppressFinalize(this); throw new InvalidDataException(message); } } @@ -315,6 +341,11 @@ public virtual string GetConfigString(string name) } protected virtual string GetConfigStringOnce(string name) + { + return this.ReadRepoConfigString(name, rejectIncludesLibGit2Ignores: false); + } + + private string ReadRepoConfigString(string name, bool rejectIncludesLibGit2Ignores) { IntPtr configHandle; if (Native.Config.GetConfig(out configHandle, this.RepoHandle) != Native.ResultCode.Success) @@ -323,44 +354,102 @@ protected virtual string GetConfigStringOnce(string name) } try { - // git_config_get_string returns a borrowed pointer whose lifetime is tied to the - // config, so libgit2 only allows it on a snapshot (read-only) config. Calling it on - // the live config returned by git_repository_config fails with "get_string called on - // a live config object". Snapshot the config first, then read the string from it. - IntPtr snapshotHandle; - if (Native.Config.Snapshot(out snapshotHandle, configHandle) != Native.ResultCode.Success) + return GetStringFromSnapshot(configHandle, name, rejectIncludesLibGit2Ignores); + } + finally + { + Native.Config.Free(configHandle); + } + } + + private static string GetStringFromSnapshot(IntPtr configHandle, string name, bool rejectIncludesLibGit2Ignores) + { + // git_config_get_string returns a borrowed pointer whose lifetime is tied to the + // config, so libgit2 only allows it on a snapshot (read-only) config. Calling it on + // the live config returned by git_repository_config fails with "get_string called on + // a live config object". Snapshot the config first, then read the string from it. + IntPtr snapshotHandle; + if (Native.Config.Snapshot(out snapshotHandle, configHandle) != Native.ResultCode.Success) + { + throw new LibGit2Exception($"Failed to snapshot config for '{name}': {Native.GetLastError()}"); + } + + try + { + if (rejectIncludesLibGit2Ignores) + { + string condition = FindIncludeConditionLibGit2Ignores(snapshotHandle); + if (condition != null) + { + throw new LibGit2Exception($"Config has an includeIf condition that libgit2 does not evaluate: {condition}"); + } + } + + // git_config_get_string yields a borrowed pointer owned by the (snapshot) + // config, so it is retrieved as an IntPtr and copied manually. Marshalling it + // directly as an out string would make the interop marshaller free the pointer + // with CoTaskMemFree, corrupting libgit2's heap (mismatched allocator). + IntPtr valuePtr; + Native.ResultCode resultCode = Native.Config.GetString(out valuePtr, snapshotHandle, name); + if (resultCode == Native.ResultCode.NotFound) + { + return null; + } + else if (resultCode != Native.ResultCode.Success) { - throw new LibGit2Exception($"Failed to snapshot config for '{name}': {Native.GetLastError()}"); + throw new LibGit2Exception($"Failed to get config value for '{name}': {Native.GetLastError()}"); } + return valuePtr == IntPtr.Zero ? null : Marshal.PtrToStringUTF8(valuePtr); + } + finally + { + Native.Config.Free(snapshotHandle); + } + } + + /// + /// Returns the first includeIf condition that libgit2 skips but git.exe can apply, or null. + /// + private static string FindIncludeConditionLibGit2Ignores(IntPtr configHandle) + { + const string Prefix = "includeif."; + const string Suffix = ".path"; + + string ignoredCondition = null; + Native.Config.GitConfigMultivarCallback callback = (entryPtr, payload) => + { try { - // git_config_get_string yields a borrowed pointer owned by the (snapshot) - // config, so it is retrieved as an IntPtr and copied manually. Marshalling it - // directly as an out string would make the interop marshaller free the pointer - // with CoTaskMemFree, corrupting libgit2's heap (mismatched allocator). - IntPtr valuePtr; - Native.ResultCode resultCode = Native.Config.GetString(out valuePtr, snapshotHandle, name); - if (resultCode == Native.ResultCode.NotFound) + string entryName = Marshal.PtrToStructure(entryPtr).GetName(); + string condition = entryName.Substring(Prefix.Length, entryName.Length - Prefix.Length - Suffix.Length); + foreach (string evaluated in IncludeConditionsLibGit2Evaluates) { - return null; - } - else if (resultCode != Native.ResultCode.Success) - { - throw new LibGit2Exception($"Failed to get config value for '{name}': {Native.GetLastError()}"); + if (condition.StartsWith(evaluated, StringComparison.Ordinal)) + { + return Native.ResultCode.Success; + } } - return valuePtr == IntPtr.Zero ? null : Marshal.PtrToStringUTF8(valuePtr); + ignoredCondition = condition; } - finally + catch (Exception e) { - Native.Config.Free(snapshotHandle); + // An exception must not cross the native callback boundary. + ignoredCondition = ""; } - } - finally + + return Native.ResultCode.Failure; + }; + + Native.ResultCode resultCode = Native.Config.ForeachMatch(configHandle, @"^includeif\..*\.path$", callback, IntPtr.Zero); + GC.KeepAlive(callback); + if (ignoredCondition == null && resultCode != Native.ResultCode.Success) { - Native.Config.Free(configHandle); + throw new LibGit2Exception($"Failed to enumerate includeIf config: {Native.GetLastError()}"); } + + return ignoredCondition; } public virtual bool? GetConfigBool(string name) @@ -452,6 +541,106 @@ public static bool GetConfigBoolOrDefault(ITracer tracer, string repoPath, strin } } + /// + /// Reads a string config value as git.exe would, without loading the object store. Reads + /// the repo at , or the global, XDG, system, and ProgramData + /// config when is null. is null when + /// the key is unset. Fails when the config has an includeIf condition that libgit2 does not + /// evaluate. Callers must also check . + /// + internal static bool TryGetConfigString(ITracer tracer, string repoPath, string name, out string value, out string error) + { + tracer = tracer ?? NullTracer.Instance; + value = null; + error = null; + + try + { + if (repoPath == null) + { + value = GetGlobalAndSystemConfigString(tracer, name); + } + else + { + using (LibGit2Repo repo = new LibGit2Repo(tracer, repoPath)) + { + value = GitConfigReadRetry.InvokeCatching( + name, + () => repo.ReadRepoConfigString(name, rejectIncludesLibGit2Ignores: true), + GitConfigReadRetry.IsTransientLibGit2Error, + tracer, + repo.configReadDelay, + repo.configReadMaxAttempts); + } + } + + return true; + } + catch (Exception e) when (e is InvalidDataException || e is LibGit2Exception) + { + error = "Error while reading '" + name + "' from config: " + e.Message; + return false; + } + } + + /// + /// True when the process environment selects or supplies Git config that libgit2 does not read. + /// + internal static bool EnvironmentChangesGitConfig() + { + return EnvironmentChangesGitConfig(Environment.GetEnvironmentVariables()); + } + + internal static bool EnvironmentChangesGitConfig(IDictionary environment) + { + foreach (object key in environment.Keys) + { + string name = key as string; + foreach (string variable in ConfigEnvironmentVariablesLibGit2Ignores) + { + if (string.Equals(name, variable, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + } + + return false; + } + + private static string GetGlobalAndSystemConfigString(ITracer tracer, string name) + { + Native.Init(); + try + { + return GitConfigReadRetry.InvokeCatching( + name, + () => + { + if (Native.Config.GetGlobalAndSystemConfig(out IntPtr configHandle) != Native.ResultCode.Success) + { + throw new LibGit2Exception($"Failed to get global and system config handle: {Native.GetLastError()}"); + } + + try + { + return GetStringFromSnapshot(configHandle, name, rejectIncludesLibGit2Ignores: true); + } + finally + { + Native.Config.Free(configHandle); + } + }, + GitConfigReadRetry.IsTransientLibGit2Error, + tracer, + GitConfigReadRetry.Delay); + } + finally + { + Native.Shutdown(); + } + } + public void ForEachMultiVarConfig(string key, MultiVarConfigCallback callback) { List values = GitConfigReadRetry.InvokeCatching( @@ -790,6 +979,13 @@ public static extern ResultCode GetMultivarForeach( GitConfigMultivarCallback callback, IntPtr payload); + [DllImport(Git2NativeLibName, EntryPoint = "git_config_foreach_match")] + public static extern ResultCode ForeachMatch( + IntPtr configHandle, + [MarshalAs(UnmanagedType.LPUTF8Str)] string regex, + GitConfigMultivarCallback callback, + IntPtr payload); + [UnmanagedFunctionPointer(CallingConvention.Cdecl)] public delegate ResultCode GitConfigMultivarCallback( IntPtr entryPtr, diff --git a/GVFS/GVFS.Common/RetryConfig.cs b/GVFS/GVFS.Common/RetryConfig.cs index 1fa7b070f4..628dd7d017 100644 --- a/GVFS/GVFS.Common/RetryConfig.cs +++ b/GVFS/GVFS.Common/RetryConfig.cs @@ -1,6 +1,8 @@ using GVFS.Common.Git; using GVFS.Common.Tracing; using System; +using System.Collections; +using System.IO; using System.Linq; namespace GVFS.Common @@ -17,6 +19,8 @@ public class RetryConfig private static readonly TimeSpan DefaultTimeout = TimeSpan.FromSeconds(DefaultTimeoutSeconds); + internal delegate bool ConfigStringReader(string name, out string value, out string error); + public RetryConfig(int maxRetries = DefaultMaxRetries) : this(maxRetries, DefaultTimeout) { @@ -38,15 +42,88 @@ public int MaxAttempts public static bool TryLoadFromGitConfig(ITracer tracer, Enlistment enlistment, out RetryConfig retryConfig, out string error) { - return TryLoadFromGitConfig(tracer, enlistment.CreateGitProcess(tracer), out retryConfig, out error); + return TryLoadFromGitConfig( + tracer, + CreateConfigReader(tracer, enlistment, Environment.GetEnvironmentVariables()), + out retryConfig, + out error); + } + + internal static bool TryLoadFromGitConfig(ITracer tracer, GitProcess git, out RetryConfig retryConfig, out string error) + { + return TryLoadFromGitConfig( + tracer, + (string name, out string value, out string readError) => git.GetFromConfig(name).TryParseAsString(out value, out readError), + out retryConfig, + out error); + } + + /// + /// Reads the settings in process with libgit2, and with git.exe when libgit2 cannot give + /// the value that git.exe would give. + /// + internal static ConfigStringReader CreateConfigReader(ITracer tracer, Enlistment enlistment, IDictionary environment) + { + GitProcess git = null; + ConfigStringReader readWithGit = (string name, out string value, out string error) => + { + git = git ?? enlistment.CreateGitProcess(tracer); + return git.GetFromConfig(name).TryParseAsString(out value, out error); + }; + + return LibGit2Repo.EnvironmentChangesGitConfig(environment) + ? readWithGit + : WithGitFallback(tracer, CreateLibGit2Reader(tracer, enlistment.WorkingDirectoryBackingRoot), readWithGit); + } + + internal static ConfigStringReader CreateLibGit2Reader(ITracer tracer, string workingDirectoryRoot) + { + // GitProcess.GetFromConfig also reads only the global and system config when the + // working directory does not exist, for example before clone creates it. + string repoPath = Directory.Exists(workingDirectoryRoot) ? workingDirectoryRoot : null; + return (string name, out string value, out string error) => + LibGit2Repo.TryGetConfigString(tracer, repoPath, name, out value, out error); + } + + /// + /// Reads with git.exe when libgit2 fails, for example because it rejects the repository's + /// ownership or skips an includeIf condition. A transient file access failure is not read + /// again, because libgit2 already used the GitConfigReadRetry budget for it. + /// + internal static ConfigStringReader WithGitFallback(ITracer tracer, ConfigStringReader readWithLibGit2, ConfigStringReader readWithGit) + { + return (string name, out string value, out string error) => + { + if (readWithLibGit2(name, out value, out error)) + { + return true; + } + + if (GitConfigReadRetry.IsTransientFileAccessError(error)) + { + return false; + } + + tracer?.RelatedEvent( + EventLevel.Informational, + "RetryConfig_ReadWithGit", + new EventMetadata + { + { "Area", EtwArea }, + { "ConfigName", name }, + { "LibGit2Error", error }, + }); + + return readWithGit(name, out value, out error); + }; } - public static bool TryLoadFromGitConfig(ITracer tracer, GitProcess git, out RetryConfig retryConfig, out string error) + internal static bool TryLoadFromGitConfig(ITracer tracer, ConfigStringReader readConfig, out RetryConfig retryConfig, out string error) { retryConfig = null; int maxRetries; - if (!TryLoadMaxRetries(git, out maxRetries, out error)) + if (!TryLoadMaxRetries(readConfig, out maxRetries, out error)) { if (tracer != null) { @@ -63,7 +140,7 @@ public static bool TryLoadFromGitConfig(ITracer tracer, GitProcess git, out Retr } TimeSpan timeout; - if (!TryLoadTimeout(git, out timeout, out error)) + if (!TryLoadTimeout(readConfig, out timeout, out error)) { if (tracer != null) { @@ -99,10 +176,10 @@ public static bool TryLoadFromGitConfig(ITracer tracer, GitProcess git, out Retr return true; } - private static bool TryLoadMaxRetries(GitProcess git, out int attempts, out string error) + private static bool TryLoadMaxRetries(ConfigStringReader readConfig, out int attempts, out string error) { return TryGetFromGitConfig( - git, + readConfig, GVFSConstants.GitConfig.MaxRetriesConfig, DefaultMaxRetries, MinRetries, @@ -110,12 +187,12 @@ private static bool TryLoadMaxRetries(GitProcess git, out int attempts, out stri out error); } - private static bool TryLoadTimeout(GitProcess git, out TimeSpan timeout, out string error) + private static bool TryLoadTimeout(ConfigStringReader readConfig, out TimeSpan timeout, out string error) { timeout = TimeSpan.FromSeconds(0); int timeoutSeconds; if (!TryGetFromGitConfig( - git, + readConfig, GVFSConstants.GitConfig.TimeoutSecondsConfig, DefaultTimeoutSeconds, 0, @@ -129,10 +206,15 @@ private static bool TryLoadTimeout(GitProcess git, out TimeSpan timeout, out str return true; } - private static bool TryGetFromGitConfig(GitProcess git, string configName, int defaultValue, int minValue, out int value, out string error) + private static bool TryGetFromGitConfig(ConfigStringReader readConfig, string configName, int defaultValue, int minValue, out int value, out string error) { - GitProcess.ConfigResult result = git.GetFromConfig(configName); - return result.TryParseAsInt(defaultValue, minValue, out value, out error); + value = defaultValue; + if (!readConfig(configName, out string valueString, out error)) + { + return false; + } + + return GitConfigHelper.TryParseIntSetting(configName, valueString, defaultValue, minValue, out value, out error); } } } diff --git a/GVFS/GVFS.UnitTests/Common/ConfigReaderTracerTests.cs b/GVFS/GVFS.UnitTests/Common/ConfigReaderTracerTests.cs index a361644279..f31c3c9159 100644 --- a/GVFS/GVFS.UnitTests/Common/ConfigReaderTracerTests.cs +++ b/GVFS/GVFS.UnitTests/Common/ConfigReaderTracerTests.cs @@ -6,6 +6,7 @@ using GVFS.UnitTests.Mock.Common; using GVFS.UnitTests.Mock.Git; using NUnit.Framework; +using System.Collections; using System.Collections.Generic; namespace GVFS.UnitTests.Common @@ -30,17 +31,40 @@ public void CacheServerResolverCreatesItsGitProcessWithTheTracer() } [TestCase] - public void RetryConfigCreatesItsGitProcessWithTheTracer() + public void RetryConfigCreatesItsGitProcessWithTheTracerWhenTheEnvironmentChangesGitConfig() { MockTracer tracer = new MockTracer(); TracerCapturingEnlistment enlistment = this.CreateEnlistment(); - RetryConfig.TryLoadFromGitConfig(tracer, enlistment, out RetryConfig _, out string _); + RetryConfig.TryLoadFromGitConfig( + tracer, + RetryConfig.CreateConfigReader(tracer, enlistment, new Hashtable { { "GIT_CONFIG_COUNT", "1" } }), + out RetryConfig _, + out string _); + // One git.exe process reads both settings. enlistment.CapturedTracers.Count.ShouldEqual(1); enlistment.CapturedTracers[0].ShouldBeSameAs(tracer); } + [TestCase] + public void RetryConfigDoesNotStartGitWhenLibGit2ReadsTheSettings() + { + TracerCapturingEnlistment enlistment = this.CreateEnlistment(); + + // The mock working directory does not exist, so libgit2 reads the empty isolated global config. + using (new IsolatedLibGit2Config()) + { + RetryConfig.TryLoadFromGitConfig( + new MockTracer(), + RetryConfig.CreateConfigReader(new MockTracer(), enlistment, new Hashtable { { "PATH", "C:\\bin" } }), + out RetryConfig _, + out string _); + } + + enlistment.CapturedTracers.Count.ShouldEqual(0); + } + [TestCase] public void GitStatusCacheConfigCreatesItsGitProcessWithTheTracer() { diff --git a/GVFS/GVFS.UnitTests/Common/GitConfigHelperTests.cs b/GVFS/GVFS.UnitTests/Common/GitConfigHelperTests.cs index 6f0a6c1412..4980fce7e5 100644 --- a/GVFS/GVFS.UnitTests/Common/GitConfigHelperTests.cs +++ b/GVFS/GVFS.UnitTests/Common/GitConfigHelperTests.cs @@ -9,6 +9,29 @@ namespace GVFS.UnitTests.Common [TestFixture] public class GitConfigHelperTests { + [TestCase(null, 5)] + [TestCase("", 5)] + [TestCase(" ", 5)] + [TestCase("7", 7)] + [TestCase(" 7 ", 7)] + [TestCase("0", 0)] + public void TryParseIntSettingReturnsTheValueOrTheDefault(string valueString, int expected) + { + GitConfigHelper.TryParseIntSetting("gvfs.test", valueString, 5, 0, out int value, out string error).ShouldBeTrue(error); + value.ShouldEqual(expected); + error.ShouldEqual(string.Empty); + } + + [TestCase("abc", "Misconfigured config setting gvfs.test, could not parse value `abc` as an int")] + [TestCase("1k", "Misconfigured config setting gvfs.test, could not parse value `1k` as an int")] + [TestCase("99999999999", "Misconfigured config setting gvfs.test, could not parse value `99999999999` as an int")] + [TestCase("-1", "Invalid value -1 for setting gvfs.test, value must be greater than or equal to 0")] + public void TryParseIntSettingReportsInvalidValues(string valueString, string expectedError) + { + GitConfigHelper.TryParseIntSetting("gvfs.test", valueString, 5, 0, out int _, out string error).ShouldBeFalse(); + error.ShouldEqual(expectedError); + } + [TestCase] public void SanitizeEmptyString() { diff --git a/GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs b/GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs index c6932cadac..bdd95bc793 100644 --- a/GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs +++ b/GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs @@ -3,6 +3,7 @@ using GVFS.UnitTests.Mock.Common; using NUnit.Framework; using System; +using System.Collections; using System.Collections.Generic; using System.IO; @@ -72,6 +73,47 @@ public void GetConfigBoolOrDefaultOnRepoReturnsDefaultOnInvalidDataExceptionAndL } } + [TestCase("GIT_CONFIG_PARAMETERS")] + [TestCase("GIT_CONFIG_COUNT")] + [TestCase("GIT_CONFIG_GLOBAL")] + [TestCase("GIT_CONFIG_SYSTEM")] + [TestCase("GIT_CONFIG_NOSYSTEM")] + [TestCase("GIT_CONFIG")] + [TestCase("GIT_DIR")] + [TestCase("git_config_parameters")] + public void EnvironmentChangesGitConfigDetectsVariablesThatLibGit2Ignores(string name) + { + Hashtable environment = new Hashtable { { "PATH", "C:\\bin" }, { name, "1" } }; + + LibGit2Repo.EnvironmentChangesGitConfig(environment).ShouldBeTrue(); + } + + [TestCase] + public void EnvironmentChangesGitConfigIgnoresOtherGitVariables() + { + Hashtable environment = new Hashtable + { + { "GIT_TRACE", "1" }, + { "GIT_CONFIG_KEY_0", "gvfs.max-retries" }, + { "GIT_WORK_TREE", "C:\\repo" }, + }; + + LibGit2Repo.EnvironmentChangesGitConfig(environment).ShouldBeFalse(); + } + + [TestCase] + public void TryGetConfigStringReportsAnUnopenableRepository() + { + string missingRepoPath = Path.Combine( + Path.GetTempPath(), + "LibGit2RepoConfigLookupTests_" + Guid.NewGuid().ToString("N")); + + LibGit2Repo.TryGetConfigString(new MockTracer(), missingRepoPath, "gvfs.test", out string value, out string error).ShouldBeFalse(); + + value.ShouldBeNull(); + error.ShouldContain("Error while reading 'gvfs.test' from config: Couldn't open repo at"); + } + [TestCase] public void GetConfigBoolRetriesTransientLibGit2Failure() { diff --git a/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs b/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs index 0f8c45b26d..a46424d056 100644 --- a/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs +++ b/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs @@ -1,10 +1,13 @@ using GVFS.Common; using GVFS.Common.Git; +using GVFS.Common.Tracing; using GVFS.Tests.Should; using GVFS.UnitTests.Mock.Common; using GVFS.UnitTests.Mock.Git; using NUnit.Framework; using System; +using System.Collections.Generic; +using System.IO; namespace GVFS.UnitTests.Common { @@ -107,5 +110,327 @@ public void TryLoadConfigUsesConfiguredValues() config.MaxAttempts.ShouldEqual(config.MaxRetries + 1); config.Timeout.ShouldEqual(TimeSpan.FromSeconds(timeoutSeconds)); } + + [TestCase] + public void TryLoadConfigWithLibGit2UsesRepositoryValues() + { + using (TestRepository repo = new TestRepository("[gvfs]\n\tmax-retries = 7\n\ttimeout-seconds = 31\n")) + { + RetryConfig.TryLoadFromGitConfig(new MockTracer(), RetryConfig.CreateLibGit2Reader(new MockTracer(), repo.WorkingDirectory), out RetryConfig config, out string error) + .ShouldBeTrue(error); + + error.ShouldEqual(string.Empty); + config.MaxRetries.ShouldEqual(7); + config.Timeout.ShouldEqual(TimeSpan.FromSeconds(31)); + } + } + + [TestCase] + public void TryLoadConfigWithLibGit2UsesDefaultValuesWhenEntriesAreBlank() + { + using (TestRepository repo = new TestRepository("[gvfs]\n\tmax-retries =\n\ttimeout-seconds\n")) + { + RetryConfig.TryLoadFromGitConfig(new MockTracer(), RetryConfig.CreateLibGit2Reader(new MockTracer(), repo.WorkingDirectory), out RetryConfig config, out string error) + .ShouldBeTrue(error); + + config.MaxRetries.ShouldEqual(RetryConfig.DefaultMaxRetries); + config.Timeout.ShouldEqual(TimeSpan.FromSeconds(RetryConfig.DefaultTimeoutSeconds)); + } + } + + [TestCase] + public void TryLoadConfigWithLibGit2ReportsTheSameParseErrorAsGit() + { + using (TestRepository repo = new TestRepository("[gvfs]\n\tmax-retries = abc\n")) + { + RetryConfig.TryLoadFromGitConfig(new MockTracer(), RetryConfig.CreateLibGit2Reader(new MockTracer(), repo.WorkingDirectory), out RetryConfig _, out string error) + .ShouldBeFalse(); + + error.ShouldEqual("Misconfigured config setting gvfs.max-retries, could not parse value `abc` as an int"); + } + } + + [TestCase] + public void TryLoadConfigWithLibGit2EnforcesMinimumValuesOnTimeout() + { + using (TestRepository repo = new TestRepository("[gvfs]\n\tmax-retries = 3\n\ttimeout-seconds = -1\n")) + { + MockTracer tracer = new MockTracer(); + RetryConfig.TryLoadFromGitConfig(tracer, RetryConfig.CreateLibGit2Reader(tracer, repo.WorkingDirectory), out RetryConfig _, out string error) + .ShouldBeFalse(); + + error.ShouldEqual("Invalid value -1 for setting gvfs.timeout-seconds, value must be greater than or equal to 0"); + tracer.RelatedErrorEvents.Count.ShouldEqual(1); + tracer.RelatedErrorEvents[0].ShouldContain("TryLoadTimeout failed"); + } + } + + [TestCase] + public void TryLoadConfigWithLibGit2FailsWhenConfigIsCorrupt() + { + using (TestRepository repo = new TestRepository("[gvfs\n")) + { + MockTracer tracer = new MockTracer(); + RetryConfig.TryLoadFromGitConfig(tracer, RetryConfig.CreateLibGit2Reader(tracer, repo.WorkingDirectory), out RetryConfig config, out string error) + .ShouldBeFalse(); + + config.ShouldBeNull(); + error.ShouldContain("Error while reading 'gvfs.max-retries' from config"); + error.ShouldContain("failed to parse config file"); + tracer.RelatedErrorEvents[0].ShouldContain("TryLoadMaxRetries failed"); + } + } + + [TestCase] + public void GitFallbackReadsWithGitWhenLibGit2Fails() + { + MockTracer tracer = new MockTracer(); + List gitReads = new List(); + RetryConfig.ConfigStringReader readConfig = RetryConfig.WithGitFallback( + tracer, + (string name, out string value, out string error) => + { + value = null; + error = "Error while reading '" + name + "' from config: Couldn't open repo"; + return false; + }, + (string name, out string value, out string error) => + { + gitReads.Add(name); + value = "4"; + error = string.Empty; + return true; + }); + + RetryConfig.TryLoadFromGitConfig(tracer, readConfig, out RetryConfig config, out string loadError).ShouldBeTrue(loadError); + + config.MaxRetries.ShouldEqual(4); + config.Timeout.ShouldEqual(TimeSpan.FromSeconds(4)); + gitReads.ShouldMatchInOrder("gvfs.max-retries", "gvfs.timeout-seconds"); + int readWithGitEvent = tracer.RelatedEventNames.IndexOf("RetryConfig_ReadWithGit"); + readWithGitEvent.ShouldEqual(0); + tracer.RelatedEventNames.FindAll(eventName => eventName == "RetryConfig_ReadWithGit").Count.ShouldEqual(2); + EventMetadata metadata = tracer.RelatedEventMetadata[readWithGitEvent]; + metadata["Area"].ShouldEqual("RetryConfig"); + metadata["ConfigName"].ShouldEqual("gvfs.max-retries"); + ((string)metadata["LibGit2Error"]).ShouldContain("Couldn't open repo"); + tracer.RelatedWarningEvents.Count.ShouldEqual(0); + } + + [TestCase] + public void GitFallbackDoesNotReadAgainAfterATransientFailure() + { + int gitReads = 0; + RetryConfig.ConfigStringReader readConfig = RetryConfig.WithGitFallback( + new MockTracer(), + (string name, out string value, out string error) => + { + value = null; + error = "Error while reading '" + name + "' from config: could not open 'C:/repo/.git/config': " + + "The process cannot access the file because it is being used by another process."; + return false; + }, + (string name, out string value, out string error) => + { + ++gitReads; + value = null; + error = null; + return true; + }); + + RetryConfig.TryLoadFromGitConfig(new MockTracer(), readConfig, out RetryConfig _, out string loadError).ShouldBeFalse(); + + gitReads.ShouldEqual(0); + loadError.ShouldContain("being used by another process"); + } + + [TestCase] + public void GitFallbackReportsTheGitErrorWhenBothReadersFail() + { + RetryConfig.ConfigStringReader readConfig = RetryConfig.WithGitFallback( + new MockTracer(), + (string name, out string value, out string error) => + { + value = null; + error = "libgit2 error"; + return false; + }, + (string name, out string value, out string error) => + { + value = null; + error = "git error"; + return false; + }); + + RetryConfig.TryLoadFromGitConfig(new MockTracer(), readConfig, out RetryConfig config, out string loadError).ShouldBeFalse(); + + config.ShouldBeNull(); + loadError.ShouldEqual("git error"); + } + + [TestCase] + public void GitFallbackDoesNotReadWithGitWhenLibGit2Succeeds() + { + MockTracer tracer = new MockTracer(); + int gitReads = 0; + RetryConfig.ConfigStringReader readConfig = RetryConfig.WithGitFallback( + tracer, + (string name, out string value, out string error) => + { + value = null; + error = null; + return true; + }, + (string name, out string value, out string error) => + { + ++gitReads; + value = null; + error = null; + return true; + }); + + RetryConfig.TryLoadFromGitConfig(tracer, readConfig, out RetryConfig config, out string _).ShouldBeTrue(); + + config.MaxRetries.ShouldEqual(RetryConfig.DefaultMaxRetries); + gitReads.ShouldEqual(0); + tracer.RelatedEventNames.Contains("RetryConfig_ReadWithGit").ShouldBeFalse(); + } + + [TestCase] + public void LibGit2ReaderReadsGlobalConfigBeforeTheRepositoryExists() + { + using (IsolatedLibGit2Config machineConfig = new IsolatedLibGit2Config()) + { + File.WriteAllText(machineConfig.GlobalConfigPath, "[gvfs]\n\tmax-retries = 9\n\ttimeout-seconds = 41\n"); + string missingWorkingDirectory = Path.Combine(Path.GetTempPath(), "RetryConfigTests_" + Guid.NewGuid().ToString("N")); + + RetryConfig.TryLoadFromGitConfig(new MockTracer(), RetryConfig.CreateLibGit2Reader(new MockTracer(), missingWorkingDirectory), out RetryConfig config, out string error) + .ShouldBeTrue(error); + + config.MaxRetries.ShouldEqual(9); + config.Timeout.ShouldEqual(TimeSpan.FromSeconds(41)); + } + } + + [TestCase] + public void LibGit2ReaderRejectsAGlobalIncludeConditionItIgnores() + { + using (IsolatedLibGit2Config machineConfig = new IsolatedLibGit2Config()) + { + File.WriteAllText( + machineConfig.GlobalConfigPath, + "[includeIf \"hasconfig:remote.*.url:https://example.com/**\"]\n\tpath = C:/does-not-matter.gitconfig\n"); + string missingWorkingDirectory = Path.Combine(Path.GetTempPath(), "RetryConfigTests_" + Guid.NewGuid().ToString("N")); + + RetryConfig.CreateLibGit2Reader(new MockTracer(), missingWorkingDirectory)("gvfs.max-retries", out string _, out string error) + .ShouldBeFalse(); + + error.ShouldContain("includeIf condition that libgit2 does not evaluate: hasconfig:"); + } + } + + [TestCase] + public void LibGit2AndGitReadTheSameRepositoryValues() + { + string gitPath = GetGitPathOrIgnore(); + using (TestRepository repo = new TestRepository("[gvfs]\n\tmax-retries = 7\n\tmax-retries = 8\n\ttimeout-seconds = 31\n")) + { + RetryConfig.TryLoadFromGitConfig(new MockTracer(), RetryConfig.CreateLibGit2Reader(new MockTracer(), repo.WorkingDirectory), out RetryConfig libGit2Config, out string libGit2Error) + .ShouldBeTrue(libGit2Error); + RetryConfig.TryLoadFromGitConfig(new MockTracer(), new GitProcess(gitPath, repo.WorkingDirectory), out RetryConfig gitConfig, out string gitError) + .ShouldBeTrue(gitError); + + libGit2Config.MaxRetries.ShouldEqual(8); + gitConfig.MaxRetries.ShouldEqual(8); + libGit2Config.Timeout.ShouldEqual(TimeSpan.FromSeconds(31)); + gitConfig.Timeout.ShouldEqual(TimeSpan.FromSeconds(31)); + } + } + + [TestCase] + public void ConfigWithAnIncludeConditionLibGit2IgnoresIsReadWithGit() + { + string gitPath = GetGitPathOrIgnore(); + using (TestRepository repo = new TestRepository(string.Empty)) + { + string includedPath = Path.Combine(repo.WorkingDirectory, "included.gitconfig"); + File.WriteAllText(includedPath, "[gvfs]\n\tmax-retries = 12\n"); + File.AppendAllText( + Path.Combine(repo.WorkingDirectory, ".git", "config"), + "[remote \"origin\"]\n\turl = https://example.com/repo\n" + + "[includeIf \"hasconfig:remote.*.url:https://example.com/**\"]\n\tpath = " + includedPath.Replace('\\', '/') + "\n"); + + MockTracer tracer = new MockTracer(); + RetryConfig.ConfigStringReader readWithLibGit2 = RetryConfig.CreateLibGit2Reader(tracer, repo.WorkingDirectory); + GitProcess git = new GitProcess(gitPath, repo.WorkingDirectory); + RetryConfig.ConfigStringReader readWithGit = (string name, out string value, out string error) => + git.GetFromConfig(name).TryParseAsString(out value, out error); + + readWithLibGit2("gvfs.max-retries", out string _, out string libGit2Error).ShouldBeFalse(); + libGit2Error.ShouldContain("includeIf condition that libgit2 does not evaluate: hasconfig:remote.*.url:https://example.com/**"); + + RetryConfig.TryLoadFromGitConfig(tracer, RetryConfig.WithGitFallback(tracer, readWithLibGit2, readWithGit), out RetryConfig config, out string loadError) + .ShouldBeTrue(loadError); + config.MaxRetries.ShouldEqual(12); + } + } + + [TestCase("gitdir:**/")] + [TestCase("gitdir/i:**/")] + [TestCase("onbranch:main")] + public void LibGit2ReadsConfigWithIncludeConditionsItEvaluates(string condition) + { + using (TestRepository repo = new TestRepository(string.Empty)) + { + string includedPath = Path.Combine(repo.WorkingDirectory, "included.gitconfig"); + File.WriteAllText(includedPath, "[gvfs]\n\tmax-retries = 13\n"); + File.AppendAllText( + Path.Combine(repo.WorkingDirectory, ".git", "config"), + "[includeIf \"" + condition + "\"]\n\tpath = " + includedPath.Replace('\\', '/') + "\n"); + + RetryConfig.CreateLibGit2Reader(new MockTracer(), repo.WorkingDirectory)("gvfs.max-retries", out string value, out string error) + .ShouldBeTrue(error); + value.ShouldEqual("13"); + } + } + + private static string GetGitPathOrIgnore() + { + string gitPath = "C:\\Program Files\\Git\\cmd\\git.exe"; + if (!File.Exists(gitPath)) + { + Assert.Ignore("Git not found at expected path — skipping integration test"); + } + + return gitPath; + } + + // A minimal repository that libgit2 can open without git.exe. libgit2 does not read the + // machine's config while the repository exists. The non-ASCII path checks the UTF-8 + // marshalling of the repository path. + private sealed class TestRepository : IDisposable + { + private readonly IsolatedLibGit2Config machineConfig = new IsolatedLibGit2Config(); + + public TestRepository(string extraConfig) + { + this.WorkingDirectory = Path.Combine(Path.GetTempPath(), "RetryConfigTests_ü_" + Guid.NewGuid().ToString("N")); + string dotGit = Path.Combine(this.WorkingDirectory, ".git"); + Directory.CreateDirectory(Path.Combine(dotGit, "objects")); + Directory.CreateDirectory(Path.Combine(dotGit, "refs", "heads")); + File.WriteAllText(Path.Combine(dotGit, "HEAD"), "ref: refs/heads/main\n"); + File.WriteAllText( + Path.Combine(dotGit, "config"), + "[core]\n\trepositoryformatversion = 0\n\tbare = false\n" + extraConfig); + } + + public string WorkingDirectory { get; } + + public void Dispose() + { + Directory.Delete(this.WorkingDirectory, recursive: true); + this.machineConfig.Dispose(); + } + } } } diff --git a/GVFS/GVFS.UnitTests/Mock/Git/IsolatedLibGit2Config.cs b/GVFS/GVFS.UnitTests/Mock/Git/IsolatedLibGit2Config.cs new file mode 100644 index 0000000000..4c881c8f75 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Mock/Git/IsolatedLibGit2Config.cs @@ -0,0 +1,55 @@ +using GVFS.Common; +using GVFS.Common.Git; +using System; +using System.IO; +using System.Runtime.InteropServices; + +namespace GVFS.UnitTests.Mock.Git +{ + /// + /// Points libgit2's global, XDG, system, and ProgramData config search paths at an empty + /// temporary directory, so a test does not read the machine's config. The search paths last + /// only while libgit2 stays initialized, so this object holds a libgit2 reference until it + /// is disposed. Unit tests run sequentially, so other tests do not observe the change. + /// + public sealed class IsolatedLibGit2Config : IDisposable + { + private const int SetSearchPathOption = 5; + + // GIT_CONFIG_LEVEL_PROGRAMDATA, SYSTEM, XDG, and GLOBAL. + private static readonly int[] ConfigLevels = { 1, 2, 3, 4 }; + + public IsolatedLibGit2Config() + { + this.Directory = Path.Combine(Path.GetTempPath(), "IsolatedLibGit2Config_" + Guid.NewGuid().ToString("N")); + System.IO.Directory.CreateDirectory(this.Directory); + + LibGit2Repo.Native.Init(); + foreach (int level in ConfigLevels) + { + if (SetSearchPath(SetSearchPathOption, level, this.Directory) != 0) + { + throw new InvalidOperationException("git_libgit2_opts failed: " + LibGit2Repo.Native.GetLastError()); + } + } + } + + public string Directory { get; } + + public string GlobalConfigPath => Path.Combine(this.Directory, ".gitconfig"); + + public void Dispose() + { + foreach (int level in ConfigLevels) + { + SetSearchPath(SetSearchPathOption, level, null); + } + + LibGit2Repo.Native.Shutdown(); + System.IO.Directory.Delete(this.Directory, recursive: true); + } + + [DllImport(GVFSConstants.LibGit2LibraryName, EntryPoint = "git_libgit2_opts")] + private static extern int SetSearchPath(int option, int level, [MarshalAs(UnmanagedType.LPUTF8Str)] string path); + } +} From c45955ed318b90f47ee3eea14a5cf49e4cf9515d Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Mon, 19 Oct 2026 14:49:18 -0700 Subject: [PATCH 3/3] Preserve GVFS locks when commands collide with aliases Distinguish built-in hooks from external attempts before alias detection. Keep required locks when a real command takes precedence over an alias. Use the parent Git executable for config queries and suppress recursive hooks. Resolve external names on the inherited Windows PATH without command enumeration. Release external-command locks by typed ownership during post-command. Do not repeat mutable command resolution when deciding whether to release. Report alias-resolution failures and retain the conservative lock. Cover built-in and external collisions, genuine aliases, command spellings, config or executable-lookup failures, and lock-ownership responses. Keep lock-holder lifetimes deterministic in the functional cases. Exercise both hooks with a live Git PID and remove the external between calls. Keep the existing lock exemption policy unchanged. Assisted-by: GPT-6.1 Sol Signed-off-by: Tyrie Vella --- .../GitReadAndGitLockTests.cs | 332 +++++++++++++++ GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs | 2 +- GVFS/GVFS.Hooks/GitCommandLock.cs | 68 +++ GVFS/GVFS.Hooks/GitCommandResolver.cs | 175 ++++++++ GVFS/GVFS.Hooks/KnownGitCommands.cs | 150 ------- GVFS/GVFS.Hooks/Program.cs | 57 ++- GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj | 6 + .../Hooks/GitCommandLockTests.cs | 99 +++++ .../Hooks/GitCommandResolverTests.cs | 396 ++++++++++++++++++ docs/faq.md | 14 + 10 files changed, 1131 insertions(+), 168 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 cb7af7571d..e84242e6f3 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs @@ -4,8 +4,10 @@ using GVFS.Tests.Should; using NUnit.Framework; using System; +using System.Collections.Generic; using System.Diagnostics; using System.IO; +using System.Text; using System.Threading; namespace GVFS.FunctionalTests.Tests.EnlistmentPerFixture @@ -131,5 +133,335 @@ 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\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.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(string suffix) + { + const string CommandName = "gvfs-directory-alias"; + string directory = Path.Combine(this.Enlistment.EnlistmentRoot, nameof(this.DirectoryNamedAfterAnExternalDoesNotHideAnAlias)); + string commandDirectory = Path.Combine(directory, "git-" + CommandName + suffix); + 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"; + + // 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; + 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"); + } + + 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; + 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); + } + + 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 + ")"); + } + + return new ProcessResult(output.ToString(), errors.ToString(), git.ExitCode); + } + finally + { + this.ReleaseLockHolder(holder); + if (gitStarted && !git.WaitForExit(10000)) + { + git.Kill(entireProcessTree: true); + git.WaitForExit(); + } + + if (errorReader != null && (errorReader.ThreadState & System.Threading.ThreadState.Unstarted) == 0) + { + errorReader.Join(10000).ShouldBeTrue("Git stderr did not close during cleanup."); + } + } + } + } + + private void ReleaseLockHolder(Process holder) + { + if (!holder.HasExited) + { + holder.StandardInput.WriteLine("done"); + holder.StandardInput.Close(); + if (!holder.WaitForExit(10000)) + { + holder.Kill(entireProcessTree: true); + holder.WaitForExit(); + Assert.Fail("The lock-holder process did not exit."); + } + } + + holder.ExitCode.ShouldEqual(0, holder.StandardError.ReadToEnd()); + } } } diff --git a/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs b/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs index 48418452d2..db19502c54 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 { diff --git a/GVFS/GVFS.Hooks/GitCommandLock.cs b/GVFS/GVFS.Hooks/GitCommandLock.cs new file mode 100644 index 0000000000..31669c29fd --- /dev/null +++ b/GVFS/GVFS.Hooks/GitCommandLock.cs @@ -0,0 +1,68 @@ +using GVFS.Common.NamedPipes; +using System; + +namespace GVFS.Hooks +{ + internal static class GitCommandLock + { + 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. Run 'gvfs status' to check the mount."; + 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; + 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."; + return false; + } + + ownsLock = response.ResponseData.PID == pid; + return true; + + case NamedPipeMessages.AcquireLock.MountNotReadyResult: + error = "GVFS has not finished initializing. Run 'gvfs status' to check the mount."; + return false; + + case NamedPipeMessages.AcquireLock.UnmountInProgressResult: + error = "GVFS is unmounting."; + return false; + + default: + error = "GVFS returned an unexpected lock-ownership response: " + response.Result; + return false; + } + } + } +} diff --git a/GVFS/GVFS.Hooks/GitCommandResolver.cs b/GVFS/GVFS.Hooks/GitCommandResolver.cs new file mode 100644 index 0000000000..bb8f067e21 --- /dev/null +++ b/GVFS/GVFS.Hooks/GitCommandResolver.cs @@ -0,0 +1,175 @@ +using GVFS.Common; +using System; +using System.ComponentModel; +using System.Diagnostics; +using System.IO; + +namespace GVFS.Hooks +{ + internal static class GitCommandResolver + { + private const int InvalidNameHResult = unchecked((int)0x8007007B); + + public static bool ShouldLock( + string hookCommand, + Func getAlias, + string searchPath, + Func fileExists, + Action reportError) + { + if (!TryIsAlias(hookCommand, getAlias, searchPath, fileExists, out bool isAlias, out string error)) + { + reportError(error + " GVFS will keep the command lock."); + return true; + } + + return !isAlias; + } + + public static bool TryIsAlias( + string hookCommand, + Func getAlias, + string searchPath, + Func fileExists, + 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); + ProcessResult result; + try + { + result = getAlias("alias." + command); + } + catch (Exception ex) when (ex is Win32Exception || ex is InvalidOperationException || + ex is ArgumentException || ex is IOException || ex is UnauthorizedAccessException) + { + error = $"Unable to query Git alias '{command}': {FormatError(ex)}"; + return false; + } + + if (result.ExitCode == 1 && string.IsNullOrEmpty(result.Output)) + { + return true; + } + + if (result.ExitCode != 0) + { + error = $"Unable to read Git alias '{command}' (exit code {result.ExitCode}). {result.Errors}".TrimEnd(); + return false; + } + + if (string.IsNullOrWhiteSpace(result.Output)) + { + return true; + } + + if (string.IsNullOrEmpty(searchPath)) + { + error = $"Unable to resolve Git command '{command}': the inherited PATH is empty."; + return false; + } + + string candidate = null; + try + { + // Git adds its exec-path to PATH before the hook. Probe Windows names directly: + // --list-cmds misses executable names with an uppercase GIT- prefix. + foreach (string directory in searchPath.Split(Path.PathSeparator)) + { + if (directory.Length == 0) + { + continue; + } + + string commandPath = directory.TrimEnd('\\', '/') + "\\" + hookCommand; + if (!hookCommand.EndsWith(".exe", StringComparison.OrdinalIgnoreCase)) + { + candidate = commandPath + ".exe"; + if (fileExists(candidate)) + { + return true; + } + } + + candidate = commandPath; + if (fileExists(candidate)) + { + return true; + } + } + } + catch (Exception ex) when (ex is IOException || ex is UnauthorizedAccessException || ex is ArgumentException) + { + error = $"Unable to resolve Git command '{command}' at '{candidate}': {FormatError(ex)}"; + return false; + } + + isAlias = true; + return true; + } + + public static string GetProcessPath(int processId) + { + using (Process process = Process.GetProcessById(processId)) + { + string path = process.MainModule?.FileName; + if (string.IsNullOrEmpty(path)) + { + throw new InvalidOperationException($"Unable to read the executable path for Git PID {processId}."); + } + + return path; + } + } + + public static ProcessStartInfo CreateAliasQuery(string gitPath, string aliasKey) + { + ProcessStartInfo processInfo = new ProcessStartInfo(gitPath); + processInfo.UseShellExecute = false; + processInfo.RedirectStandardInput = true; + processInfo.RedirectStandardOutput = true; + processInfo.RedirectStandardError = true; + processInfo.ArgumentList.Add("config"); + processInfo.ArgumentList.Add("--get"); + processInfo.ArgumentList.Add(aliasKey); + processInfo.Environment["COMMAND_HOOK_LOCK"] = "true"; + return processInfo; + } + + public static bool FileExists(string path) + { + try + { + return (File.GetAttributes(path) & FileAttributes.Directory) == 0; + } + catch (FileNotFoundException) + { + return false; + } + catch (DirectoryNotFoundException) + { + return false; + } + catch (IOException ex) when (ex.HResult == InvalidNameHResult) + { + // Git also ignores invalid PATH entries, such as directories enclosed in literal quotes. + return false; + } + } + + private static string FormatError(Exception error) + { + return $"{error.GetType().Name} (0x{error.HResult:X8}): {error.Message}"; + } + } +} 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.cs b/GVFS/GVFS.Hooks/Program.cs index 7a595a03c5..1254797817 100644 --- a/GVFS/GVFS.Hooks/Program.cs +++ b/GVFS/GVFS.Hooks/Program.cs @@ -256,7 +256,9 @@ private static void RunLockRequest(string[] args, bool unattended, LockRequestDe { try { - if (ShouldLock(args)) + bool checkOwnership = GetHookType(args) == PostCommandHook && + args[1].StartsWith("git-", StringComparison.Ordinal); + if (checkOwnership || ShouldLock(args)) { using (NamedPipeClient pipeClient = new NamedPipeClient(enlistmentPipename)) { @@ -272,6 +274,30 @@ private static void RunLockRequest(string[] args, bool unattended, LockRequestDe 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); } } @@ -495,13 +521,12 @@ private static bool ShouldLock(string[] args) return false; } - if (!KnownGitCommands.Contains(gitCommand) && - IsAlias(gitCommand)) - { - return false; - } - - return true; + return GitCommandResolver.ShouldLock( + args[1], + aliasKey => ReadGitAlias(args, aliasKey), + Environment.GetEnvironmentVariable("PATH"), + GitCommandResolver.FileExists, + Console.Error.WriteLine); } private static string GetHookType(string[] args) @@ -520,20 +545,18 @@ private static string GetGitCommand(string[] args) return command; } - private static bool IsAlias(string command) + private static ProcessResult ReadGitAlias(string[] args, string aliasKey) { - string aliasKey = "alias." + command; - ProcessResult result = GitConfigReadRetry.InvokeResult( + string gitPath = GitCommandResolver.GetProcessPath(GetParentPid(args)); + return GitConfigReadRetry.InvokeResult( aliasKey, - () => ProcessHelper.Run("git", "config --get " + aliasKey), - (ProcessResult processResult) => - processResult.ExitCode != 0 && - GitConfigReadRetry.IsTransientFileAccessError(processResult.Errors, lineMustContain: "unable to access"), + () => ProcessHelper.Run(GitCommandResolver.CreateAliasQuery(gitPath, aliasKey)), + (ProcessResult result) => + result.ExitCode != 0 && + GitConfigReadRetry.IsTransientFileAccessError(result.Errors, lineMustContain: "unable to access"), NullTracer.Instance, GitConfigReadRetry.Delay, ConfigReadMaxAttempts); - - return !string.IsNullOrEmpty(result.Output); } /// diff --git a/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj b/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj index 0517e82f15..63d45ef81a 100644 --- a/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj +++ b/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj @@ -27,6 +27,12 @@ + + Hooks\GitCommandLock.cs + + + Hooks\GitCommandResolver.cs + Hooks\UnstageCommandParser.cs 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..1923832361 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Hooks/GitCommandResolverTests.cs @@ -0,0 +1,396 @@ +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"; + + [TestCase("stash")] + [TestCase("for-each-repo")] + [TestCase("checkout")] + [TestCase("update-index")] + [TestCase("status")] + public void BuiltInsDoNotQueryAliasesOrSearchPath(string command) + { + GitCommandResolver.TryIsAlias( + command, + key => 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, + key => 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", "checkout")] + [TestCase("git-shell-alias", "!git status")] + [TestCase("git-first", "second")] + [TestCase("git-second", "status")] + [TestCase("git-MiXeD", "status")] + [TestCase("git-git-prefixed", "status")] + public void GenuineAliasesSkipOnlyTheOuterLock(string hookCommand, string alias) + { + int queries = 0; + GitCommandResolver.TryIsAlias( + hookCommand, + key => + { + key.ShouldEqual("alias." + hookCommand.Substring(4)); + ++queries; + return new ProcessResult(alias + "\n", string.Empty, 0); + }, + SearchPath, + path => false, + out bool isAlias, + out string error).ShouldBeTrue(); + + queries.ShouldEqual(1); + isAlias.ShouldBeTrue(); + 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 RealExternalsTakePrecedenceOverAliases(string hookCommand, string executablePath) + { + GitCommandResolver.TryIsAlias( + hookCommand, + key => + { + key.ShouldEqual("alias." + hookCommand.Substring(4)); + return new ProcessResult("!git update-index --refresh\n", string.Empty, 0); + }, + SearchPath, + path => string.Equals(path, executablePath, StringComparison.OrdinalIgnoreCase), + out bool isAlias, + out string error).ShouldBeTrue(); + + isAlias.ShouldBeFalse(); + error.ShouldBeNull(); + } + + [TestCase(1, "")] + [TestCase(0, "")] + [TestCase(0, "\n")] + public void MissingOrEmptyAliasesKeepTheLockWithoutSearchingPath(int exitCode, string output) + { + GitCommandResolver.TryIsAlias( + "git-unknown-command", + key => new ProcessResult(output, string.Empty, exitCode), + SearchPath, + path => throw new AssertionException("A command without an alias must not search PATH."), + out bool isAlias, + out string error).ShouldBeTrue(); + + isAlias.ShouldBeFalse(); + error.ShouldBeNull(); + } + + [TestCase] + public void MissingAliasWithTraceOutputStillKeepsTheLock() + { + GitCommandResolver.TryIsAlias( + "git-unknown-command", + key => new ProcessResult(string.Empty, "trace: built-in: git config --get alias.unknown-command", 1), + SearchPath, + path => throw new AssertionException("A missing alias must not search PATH."), + out bool isAlias, + out string error).ShouldBeTrue(); + + isAlias.ShouldBeFalse(); + error.ShouldBeNull(); + } + + [TestCase(128, "")] + [TestCase(128, "status\n")] + [TestCase(1, "status\n")] + public void FailedConfigReadsNeverGrantAnAliasExemption(int exitCode, string output) + { + GitCommandResolver.TryIsAlias( + "git-co", + key => new ProcessResult(output, "config read failed", exitCode), + SearchPath, + path => throw new AssertionException("A failed config read must not search PATH."), + out bool isAlias, + out string error).ShouldBeFalse(); + + isAlias.ShouldBeFalse(); + error.ShouldContain("Unable to read Git alias 'co'"); + error.ShouldContain("exit code " + exitCode); + error.ShouldContain("config read failed"); + } + + [TestCase(null)] + [TestCase("")] + public void MissingSearchPathProducesADiagnostic(string searchPath) + { + GitCommandResolver.TryIsAlias( + "git-co", + key => new ProcessResult("checkout", string.Empty, 0), + searchPath, + path => false, + out bool isAlias, + out string error).ShouldBeFalse(); + + isAlias.ShouldBeFalse(); + error.ShouldContain("inherited PATH is empty"); + } + + [TestCase(false)] + [TestCase(true)] + [Category(CategoryConstants.ExceptionExpected)] + public void FileLookupFailuresProduceADiagnostic(bool accessDenied) + { + GitCommandResolver.TryIsAlias( + "git-co", + key => new ProcessResult("checkout", string.Empty, 0), + SearchPath, + path => + { + if (accessDenied) + { + throw new UnauthorizedAccessException("access denied"); + } + + throw new IOException("lookup failed"); + }, + out bool isAlias, + out string error).ShouldBeFalse(); + + isAlias.ShouldBeFalse(); + error.ShouldContain("Unable to resolve Git command 'co'"); + error.ShouldContain(accessDenied ? "access denied" : "lookup failed"); + error.ShouldContain(@"mock:\git\libexec\git-co.exe"); + error.ShouldContain("0x"); + error.ShouldContain(accessDenied ? "UnauthorizedAccessException" : "IOException"); + } + + [TestCase] + public void SearchPathUsesRelativeDirectoriesAndSkipsEmptyEntries() + { + List candidates = new List(); + GitCommandResolver.TryIsAlias( + "git-co", + key => new ProcessResult("checkout", string.Empty, 0), + @";relative tools;;mock:\tools;", + path => + { + candidates.Add(path); + return false; + }, + out bool isAlias, + out string error).ShouldBeTrue(); + + candidates.ShouldMatchInOrder( + Path.Combine("relative tools", "git-co.exe"), + Path.Combine("relative tools", "git-co"), + @"mock:\tools\git-co.exe", + @"mock:\tools\git-co"); + isAlias.ShouldBeTrue(); + error.ShouldBeNull(); + } + + [TestCase] + public void ExecutableChangesDoNotUseAStaleCommandCache() + { + bool executableExists = false; + foreach (bool exists in new[] { false, true, false }) + { + executableExists = exists; + GitCommandResolver.TryIsAlias( + "git-co", + key => new ProcessResult("checkout", string.Empty, 0), + SearchPath, + path => executableExists, + out bool isAlias, + out string error).ShouldBeTrue(); + + isAlias.ShouldEqual(!exists); + error.ShouldBeNull(); + } + } + + [TestCase] + public void ExecutableSuffixDoesNotAddASecondExtension() + { + List candidates = new List(); + GitCommandResolver.TryIsAlias( + "git-co.EXE", + key => new ProcessResult("status", string.Empty, 0), + @"mock:\tools", + path => + { + candidates.Add(path); + return false; + }, + out bool isAlias, + out string error).ShouldBeTrue(); + + candidates.ShouldMatchInOrder(@"mock:\tools\git-co.EXE"); + isAlias.ShouldBeTrue(); + error.ShouldBeNull(); + } + + [TestCase] + public void CurrentProcessPathUsesTheSuppliedPid() + { + GitCommandResolver.GetProcessPath(Environment.ProcessId).ShouldEqual(Environment.ProcessPath); + } + + [TestCase] + [Category(CategoryConstants.ExceptionExpected)] + public void InvalidParentPidProducesAQueryFailure() + { + string diagnostic = null; + GitCommandResolver.ShouldLock( + "git-co", + key => + { + GitCommandResolver.GetProcessPath(-1); + throw new AssertionException("An invalid process ID must not return an executable path."); + }, + SearchPath, + path => throw new AssertionException("An invalid parent must not search PATH."), + message => diagnostic = message).ShouldBeTrue(); + + diagnostic.ShouldContain("ArgumentException"); + diagnostic.ShouldContain("GVFS will keep the command lock."); + } + + [TestCase] + [Category(CategoryConstants.ExceptionExpected)] + public void QueryFailuresKeepTheLockWithPreciseDiagnostics() + { + 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"), + }) + { + string diagnostic = null; + GitCommandResolver.ShouldLock( + "git-co", + key => throw failure, + SearchPath, + path => throw new AssertionException("A failed query must not search PATH."), + message => diagnostic = message).ShouldBeTrue(); + + diagnostic.ShouldContain("Unable to query Git alias 'co'"); + diagnostic.ShouldContain(failure.GetType().Name); + diagnostic.ShouldContain(failure.HResult.ToString("X8")); + diagnostic.ShouldContain(failure.Message); + diagnostic.ShouldContain("GVFS will keep the command lock."); + diagnostic.Contains(" at GVFS.").ShouldBeFalse(); + } + } + + [TestCase] + [Category(CategoryConstants.ExceptionExpected)] + public void PathProbeFailuresKeepTheLockInsteadOfAbortingAliases() + { + foreach (Exception failure in new Exception[] + { + new IOException("Unavailable drive"), + new PathTooLongException("Path too long"), + new UnauthorizedAccessException("Access denied"), + new ArgumentException("Invalid path"), + }) + { + string diagnostic = null; + GitCommandResolver.ShouldLock( + "git-co", + key => new ProcessResult("checkout", string.Empty, 0), + SearchPath, + path => throw failure, + message => diagnostic = message).ShouldBeTrue(); + + diagnostic.ShouldContain(@"mock:\git\libexec\git-co.exe"); + diagnostic.ShouldContain(failure.GetType().Name); + diagnostic.ShouldContain("GVFS will keep the command lock."); + } + } + + [TestCase] + public void FailedConfigReadKeepsTheLockWithItsExitCode() + { + string diagnostic = null; + GitCommandResolver.ShouldLock( + "git-co", + key => new ProcessResult("checkout", "config read failed", 128), + SearchPath, + path => throw new AssertionException("A failed query must not search PATH."), + message => diagnostic = message).ShouldBeTrue(); + + diagnostic.ShouldContain("exit code 128"); + diagnostic.ShouldContain("config read failed"); + diagnostic.ShouldContain("GVFS will keep the command lock."); + } + + [TestCase] + public void GenuineAliasExemptionDoesNotReportAnError() + { + GitCommandResolver.ShouldLock( + "git-co", + key => new ProcessResult("checkout", string.Empty, 0), + SearchPath, + path => false, + message => throw new AssertionException(message)).ShouldBeFalse(); + } + + [TestCase] + public void AliasQueryUsesTheParentGitAndSuppressesRecursiveHooks() + { + const string ParentGit = @"mock:\custom git\bin\git.exe"; + const string AliasKey = "alias.git-MiXeD"; + ProcessStartInfo query = GitCommandResolver.CreateAliasQuery(ParentGit, AliasKey); + + query.FileName.ShouldEqual(ParentGit); + query.UseShellExecute.ShouldBeFalse(); + query.RedirectStandardInput.ShouldBeTrue(); + query.RedirectStandardOutput.ShouldBeTrue(); + query.RedirectStandardError.ShouldBeTrue(); + query.ArgumentList.ShouldMatchInOrder("config", "--get", AliasKey); + query.Environment["COMMAND_HOOK_LOCK"].ShouldEqual("true"); + query.Environment["PATH"].ShouldEqual(Environment.GetEnvironmentVariable("PATH")); + query.Environment.TryGetValue("GIT_EXEC_PATH", out string execPath); + execPath.ShouldEqual(Environment.GetEnvironmentVariable("GIT_EXEC_PATH")); + query.Environment.TryGetValue("GIT_CONFIG_PARAMETERS", out string parameters); + parameters.ShouldEqual(Environment.GetEnvironmentVariable("GIT_CONFIG_PARAMETERS")); + } + } +} diff --git a/docs/faq.md b/docs/faq.md index 5026fe6411..36bae31d69 100644 --- a/docs/faq.md +++ b/docs/faq.md @@ -25,6 +25,20 @@ 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? + +When Git selects a built-in command or an external executable, a +same-name alias does not remove the GVFS lock requirement. For example, `alias.stash` +does not change the lock requirement for `git stash`. + +VFS for Git skips the outer lock for a genuine alias. Each command in +the alias follows its normal lock policy. For example, an alias that +runs `checkout` still requires the lock for `checkout`. + +If an alias-config query or executable probe fails, VFS for Git reports +the error and keeps the command lock. The post-command hook releases a +lock only when its Git process owns that lock. + ### Why the name change? This project was formerly known as GVFS (Git Virtual File System). It is