From c83ccaa7a7b8d4255cf59f10449df34b295f8497 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 6 Oct 2026 11:35:10 -0700 Subject: [PATCH 1/7] 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.CommandLocking.cs | 16 +- GVFS/GVFS.Hooks/Program.cs | 4 +- 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 | 176 +++++++++++++ .../Mock/Common/MockGVFSEnlistment.cs | 6 + .../GVFS.UnitTests/Mock/Git/MockGitProcess.cs | 5 + 18 files changed, 1033 insertions(+), 38 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 1faf2f59d7..65d6e46432 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; @@ -123,5 +124,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 ac8a1eb3dc..3475acee28 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -85,6 +85,8 @@ public class GitProcess : ICredentialStore private string gitBinPath; private string workingDirectoryRoot; private string gitDirPath; + private readonly ITracer tracer; + private readonly Action configReadDelay; private Process executingProcess; private bool stopping; @@ -114,11 +116,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)) { @@ -127,6 +144,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) { @@ -167,22 +186,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); } @@ -510,7 +535,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; @@ -526,7 +553,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)) { @@ -556,16 +587,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); } /// @@ -597,7 +634,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) @@ -1284,6 +1326,23 @@ private static bool IsCredentialConfigReadFailure(Result result) return configReadFailed && transientAccessFailure; } + 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 void AddCredentialFillResult(EventMetadata data, Result result) { // Describes the failure without the text of stderr, which can contain a secret. 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 7de0f1a617..568ac7c922 100644 --- a/GVFS/GVFS.Common/RetryConfig.cs +++ b/GVFS/GVFS.Common/RetryConfig.cs @@ -45,7 +45,7 @@ public int MaxAttempts public static RetryConfig LoadFromGitConfig(ITracer tracer, Enlistment enlistment) { ArgumentNullException.ThrowIfNull(enlistment); - return LoadFromGitConfig(tracer, enlistment.CreateGitProcess()); + return LoadFromGitConfig(tracer, enlistment.CreateGitProcess(tracer)); } /// 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.CommandLocking.cs b/GVFS/GVFS.Hooks/Program.CommandLocking.cs index f759ae75e4..d535373325 100644 --- a/GVFS/GVFS.Hooks/Program.CommandLocking.cs +++ b/GVFS/GVFS.Hooks/Program.CommandLocking.cs @@ -1,10 +1,15 @@ using GVFS.Common; +using GVFS.Common.Git; +using GVFS.Common.Tracing; using System.Linq; namespace GVFS.Hooks { public partial class Program { + // Hooks run on the git command path, so config reads retry at most once. + private const int ConfigReadMaxAttempts = 2; + // The lock serializes changes to the enlistment's index and tracked working tree with GVFS projection updates. // Before adding an exemption, check all supported options, subcommands, and child-command paths. // Ref and object writes alone do not require this lock. Index refreshes do. @@ -111,7 +116,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.Hooks/Program.cs b/GVFS/GVFS.Hooks/Program.cs index 49d72af064..3093ab15d9 100644 --- a/GVFS/GVFS.Hooks/Program.cs +++ b/GVFS/GVFS.Hooks/Program.cs @@ -171,11 +171,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); } /// diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index 284b87c83f..f5eae0859f 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -1961,7 +1961,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 9a40a00a7d..cb8364cd3d 100644 --- a/GVFS/GVFS.Mount/InProcessMountVerb.cs +++ b/GVFS/GVFS.Mount/InProcessMountVerb.cs @@ -131,7 +131,7 @@ public void Execute() // leaving behind an empty log with no explanation. try { - 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..d7d3d675a9 --- /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.LoadFromGitConfig(tracer, enlistment); + + 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 85668c6adb..4dd4b0fec4 100644 --- a/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs @@ -4,6 +4,7 @@ using GVFS.UnitTests.Mock.Git; using NUnit.Framework; using System; +using System.Collections.Generic; using System.Diagnostics; using System.IO; using System.Threading; @@ -14,6 +15,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() { @@ -151,6 +158,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() { @@ -479,6 +643,18 @@ 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; + } + private sealed class OutputCallbackGitProcess : GitProcess { public OutputCallbackGitProcess(string gitPath) diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs index 0df52b732e..c98c37ceb8 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; @@ -40,5 +41,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 1f1854c8ad..0d9b39fb39 100644 --- a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs +++ b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs @@ -20,6 +20,11 @@ public MockGitProcess() { } + internal MockGitProcess(ITracer tracer, Action configReadDelay) + : base(new MockGVFSEnlistment(), tracer, configReadDelay) + { + } + public MockGitProcess(Enlistment enlistment) : base(enlistment) { From c84bca15ed90854c4011d3f8012d111a85d63436 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 3 Nov 2026 12:02:12 -0800 Subject: [PATCH 2/7] Read RetryConfig settings with libgit2 RetryConfig started git.exe once for each of gvfs.max-retries, gvfs.timeout-seconds, and gvfs.read-timeout-seconds. Read the 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. When the environment sets any of these variables, read the settings with git.exe. libgit2 also ignores GIT_DIR. Do not read with git.exe for it. Git exports GIT_DIR to hooks in linked worktrees, and GVFS reads repositories through an explicit path. Before clone creates the repository, an inherited GIT_DIR names an unrelated repository, so read only the global and system config. 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. As before, an invalid or unreadable setting uses its default and RetryConfig traces a warning. 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 | 252 ++++++++++++-- GVFS/GVFS.Common/RetryConfig.cs | 95 ++++- .../Common/ConfigReaderTracerTests.cs | 24 +- .../Common/GitConfigHelperTests.cs | 23 ++ .../Common/LibGit2RepoConfigLookupTests.cs | 42 +++ .../GVFS.UnitTests/Common/RetryConfigTests.cs | 325 ++++++++++++++++++ .../Mock/Git/IsolatedLibGit2Config.cs | 55 +++ 9 files changed, 807 insertions(+), 60 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 3475acee28..8398233da8 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -1719,32 +1719,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..a007856d52 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,33 @@ 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. + // + // libgit2 also ignores GIT_DIR, but this list intentionally excludes it. Git exports + // GIT_DIR to hooks in linked worktrees, so GVFS processes that hooks start often inherit + // it. GVFS reads a repository through an explicit path, which GIT_DIR does not change. + // Without a repository path, git.exe can read the local config of the repository that + // GIT_DIR names. That repository is not related to the read, so libgit2 reads only the + // global and system config. + private static readonly string[] ConfigEnvironmentVariablesLibGit2Ignores = + { + "GIT_CONFIG", + "GIT_CONFIG_COUNT", + "GIT_CONFIG_GLOBAL", + "GIT_CONFIG_NOSYSTEM", + "GIT_CONFIG_PARAMETERS", + "GIT_CONFIG_SYSTEM", + }; + + // 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 +100,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 +347,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 +360,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 +547,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 +985,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 568ac7c922..5d755d6948 100644 --- a/GVFS/GVFS.Common/RetryConfig.cs +++ b/GVFS/GVFS.Common/RetryConfig.cs @@ -1,6 +1,7 @@ using GVFS.Common.Git; using GVFS.Common.Tracing; using System; +using System.Collections; using System.ComponentModel; using System.IO; @@ -21,6 +22,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) { @@ -45,21 +48,89 @@ public int MaxAttempts public static RetryConfig LoadFromGitConfig(ITracer tracer, Enlistment enlistment) { ArgumentNullException.ThrowIfNull(enlistment); - return LoadFromGitConfig(tracer, enlistment.CreateGitProcess(tracer)); + return LoadFromGitConfig(tracer, CreateConfigReader(tracer, enlistment, Environment.GetEnvironmentVariables())); } /// - /// Invalid or unreadable retry settings use defaults and emit warnings. + /// Reads the settings with git.exe. Invalid or unreadable retry settings use defaults and + /// emit warnings. /// public static RetryConfig LoadFromGitConfig(ITracer tracer, GitProcess git) { ArgumentNullException.ThrowIfNull(git); + return LoadFromGitConfig( + tracer, + (string name, out string value, out string error) => git.GetFromConfig(name).TryParseAsString(out value, 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) + { + // When the working directory does not exist, for example before clone creates it, + // read only the global and system config. See LibGit2Repo for why GIT_DIR is ignored. + 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); + }; + } + + internal static RetryConfig LoadFromGitConfig(ITracer tracer, ConfigStringReader readConfig) + { int maxRetries = LoadIntOrDefault( - tracer, git, GVFSConstants.GitConfig.MaxRetriesConfig, DefaultMaxRetries, 0, MaxConfiguredRetries); + tracer, readConfig, GVFSConstants.GitConfig.MaxRetriesConfig, DefaultMaxRetries, 0, MaxConfiguredRetries); int timeoutSeconds = LoadIntOrDefault( - tracer, git, GVFSConstants.GitConfig.TimeoutSecondsConfig, DefaultTimeoutSeconds, 1, MaxTimeoutSeconds); + tracer, readConfig, GVFSConstants.GitConfig.TimeoutSecondsConfig, DefaultTimeoutSeconds, 1, MaxTimeoutSeconds); int readTimeoutSeconds = LoadIntOrDefault( - tracer, git, GVFSConstants.GitConfig.ReadTimeoutSecondsConfig, DefaultReadTimeoutSeconds, 0, MaxTimeoutSeconds); + tracer, readConfig, GVFSConstants.GitConfig.ReadTimeoutSecondsConfig, DefaultReadTimeoutSeconds, 0, MaxTimeoutSeconds); RetryConfig retryConfig = new RetryConfig(maxRetries, TimeSpan.FromSeconds(timeoutSeconds)) { ReadTimeout = readTimeoutSeconds == 0 @@ -86,20 +157,20 @@ public static RetryConfig LoadFromGitConfig(ITracer tracer, GitProcess git) } private static int LoadIntOrDefault( - ITracer tracer, GitProcess git, string configName, int defaultValue, int minValue, int maxValue) + ITracer tracer, ConfigStringReader readConfig, string configName, int defaultValue, int minValue, int maxValue) { string error; Exception readException = null; try { - GitProcess.ConfigResult result = git.GetFromConfig(configName); - if (result.TryParseAsInt(defaultValue, minValue, out int value, out error) && value <= maxValue) + if (readConfig(configName, out string valueString, out error) && + GitConfigHelper.TryParseIntSetting(configName, valueString, defaultValue, minValue, out int value, out error)) { - return value; - } + if (value <= maxValue) + { + return value; + } - if (string.IsNullOrEmpty(error)) - { error = $"Invalid value {value} for setting {configName}, value must be between {minValue} and {maxValue}"; } } diff --git a/GVFS/GVFS.UnitTests/Common/ConfigReaderTracerTests.cs b/GVFS/GVFS.UnitTests/Common/ConfigReaderTracerTests.cs index d7d3d675a9..4cff319679 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,36 @@ public void CacheServerResolverCreatesItsGitProcessWithTheTracer() } [TestCase] - public void RetryConfigCreatesItsGitProcessWithTheTracer() + public void RetryConfigCreatesItsGitProcessWithTheTracerWhenTheEnvironmentChangesGitConfig() { MockTracer tracer = new MockTracer(); TracerCapturingEnlistment enlistment = this.CreateEnlistment(); - RetryConfig.LoadFromGitConfig(tracer, enlistment); + RetryConfig.LoadFromGitConfig( + tracer, + RetryConfig.CreateConfigReader(tracer, enlistment, new Hashtable { { "GIT_CONFIG_COUNT", "1" } })); + // One git.exe process reads all 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.LoadFromGitConfig( + new MockTracer(), + RetryConfig.CreateConfigReader(new MockTracer(), enlistment, new Hashtable { { "PATH", "C:\\bin" } })); + } + + 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..7fe8ba5644 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_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_DIR", "C:\\repo\\.git\\worktrees\\wt" }, + { "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 ad619d3547..d91b4a0cca 100644 --- a/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs +++ b/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs @@ -159,6 +159,11 @@ public void CommandOverridesDoNotDiscardLoadedReadTimeout() { using MockTracer tracer = new MockTracer(); MockGitProcess git = CreateGitProcess(); + + // The enlistment's working directory does not exist, so libgit2 reads the isolated + // global config. git.exe reads the same values when the environment changes Git config. + using IsolatedLibGit2Config machineConfig = new IsolatedLibGit2Config(); + File.WriteAllText(machineConfig.GlobalConfigPath, "[gvfs]\n\tmax-retries = 9\n\ttimeout-seconds = 45\n\tread-timeout-seconds = 90\n"); RetryConfig config = new RetryConfigVerb().Load( tracer, new MockGVFSEnlistment(git), TimeSpan.FromMinutes(10)); config.MaxRetries.ShouldEqual(9); @@ -190,6 +195,298 @@ public void CompleteLoaderRejectsMissingDependencies() Assert.Throws(() => RetryConfig.LoadFromGitConfig(null, (Enlistment)null)); } + [TestCase] + public void LibGit2ReaderReadsRepositoryValues() + { + using (TestRepository repo = new TestRepository("[gvfs]\n\tmax-retries = 7\n\ttimeout-seconds = 31\n\tread-timeout-seconds = 0\n")) + { + MockTracer tracer = new MockTracer(); + RetryConfig config = RetryConfig.LoadFromGitConfig(tracer, RetryConfig.CreateLibGit2Reader(tracer, repo.WorkingDirectory)); + + config.MaxRetries.ShouldEqual(7); + config.Timeout.ShouldEqual(TimeSpan.FromSeconds(31)); + config.ReadTimeout.ShouldEqual(System.Threading.Timeout.InfiniteTimeSpan); + tracer.RelatedWarningEvents.Count.ShouldEqual(0); + } + } + + [TestCase] + public void LibGit2ReaderUsesDefaultValuesWhenEntriesAreBlank() + { + using (TestRepository repo = new TestRepository("[gvfs]\n\tmax-retries =\n\ttimeout-seconds\n")) + { + MockTracer tracer = new MockTracer(); + RetryConfig config = RetryConfig.LoadFromGitConfig(tracer, RetryConfig.CreateLibGit2Reader(tracer, repo.WorkingDirectory)); + + config.MaxRetries.ShouldEqual(RetryConfig.DefaultMaxRetries); + config.Timeout.ShouldEqual(TimeSpan.FromSeconds(RetryConfig.DefaultTimeoutSeconds)); + config.ReadTimeout.ShouldEqual(TimeSpan.FromSeconds(RetryConfig.DefaultReadTimeoutSeconds)); + tracer.RelatedWarningEvents.Count.ShouldEqual(0); + } + } + + [TestCase("max-retries = abc", "gvfs.max-retries", "Misconfigured config setting gvfs.max-retries, could not parse value `abc` as an int")] + [TestCase("timeout-seconds = 0", "gvfs.timeout-seconds", "Invalid value 0 for setting gvfs.timeout-seconds, value must be greater than or equal to 1")] + [TestCase("read-timeout-seconds = 2147484", "gvfs.read-timeout-seconds", "Invalid value 2147484 for setting gvfs.read-timeout-seconds, value must be between 0 and 2147483")] + public void LibGit2ReaderReportsTheSameInvalidValueErrorsAsGit(string setting, string configName, string expectedError) + { + using (TestRepository repo = new TestRepository("[gvfs]\n\t" + setting + "\n")) + { + MockTracer tracer = new MockTracer(); + RetryConfig.LoadFromGitConfig(tracer, RetryConfig.CreateLibGit2Reader(tracer, repo.WorkingDirectory)); + + tracer.RelatedWarningEvents.Count.ShouldEqual(1); + tracer.RelatedWarningEvents[0].ShouldContain("\"ConfigName\":\"" + configName + "\"", expectedError.Replace("`", "\\u0060")); + } + } + + [TestCase] + public void LibGit2ReaderUsesDefaultsWhenConfigIsCorrupt() + { + using (TestRepository repo = new TestRepository("[gvfs\n")) + { + MockTracer tracer = new MockTracer(); + RetryConfig config = RetryConfig.LoadFromGitConfig(tracer, RetryConfig.CreateLibGit2Reader(tracer, repo.WorkingDirectory)); + + config.MaxRetries.ShouldEqual(RetryConfig.DefaultMaxRetries); + config.Timeout.ShouldEqual(TimeSpan.FromSeconds(RetryConfig.DefaultTimeoutSeconds)); + List settingWarnings = tracer.RelatedWarningEvents.FindAll(warning => warning.Contains("The retry setting is invalid or unavailable")); + settingWarnings.Count.ShouldEqual(3); + settingWarnings[0].ShouldContain("Error while reading", "failed to parse config file"); + } + } + + [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 config = RetryConfig.LoadFromGitConfig(tracer, readConfig); + + config.MaxRetries.ShouldEqual(4); + config.Timeout.ShouldEqual(TimeSpan.FromSeconds(4)); + config.ReadTimeout.ShouldEqual(TimeSpan.FromSeconds(4)); + gitReads.ShouldMatchInOrder("gvfs.max-retries", "gvfs.timeout-seconds", "gvfs.read-timeout-seconds"); + int readWithGitEvent = tracer.RelatedEventNames.IndexOf("RetryConfig_ReadWithGit"); + readWithGitEvent.ShouldEqual(0); + tracer.RelatedEventNames.FindAll(eventName => eventName == "RetryConfig_ReadWithGit").Count.ShouldEqual(3); + 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() + { + MockTracer tracer = new MockTracer(); + int gitReads = 0; + RetryConfig.ConfigStringReader readConfig = RetryConfig.WithGitFallback( + tracer, + (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 config = RetryConfig.LoadFromGitConfig(tracer, readConfig); + + gitReads.ShouldEqual(0); + config.MaxRetries.ShouldEqual(RetryConfig.DefaultMaxRetries); + tracer.RelatedWarningEvents.Count.ShouldEqual(3); + tracer.RelatedWarningEvents[0].ShouldContain("being used by another process"); + } + + [TestCase] + public void GitFallbackReportsTheGitErrorWhenBothReadersFail() + { + MockTracer tracer = new MockTracer(); + RetryConfig.ConfigStringReader readConfig = RetryConfig.WithGitFallback( + tracer, + (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 config = RetryConfig.LoadFromGitConfig(tracer, readConfig); + + config.MaxRetries.ShouldEqual(RetryConfig.DefaultMaxRetries); + tracer.RelatedWarningEvents.Count.ShouldEqual(3); + tracer.RelatedWarningEvents[0].ShouldContain("\"Error\":\"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 config = RetryConfig.LoadFromGitConfig(tracer, readConfig); + + 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\tread-timeout-seconds = 42\n"); + string missingWorkingDirectory = Path.Combine(Path.GetTempPath(), "RetryConfigTests_" + Guid.NewGuid().ToString("N")); + + MockTracer tracer = new MockTracer(); + RetryConfig config = RetryConfig.LoadFromGitConfig(tracer, RetryConfig.CreateLibGit2Reader(tracer, missingWorkingDirectory)); + + config.MaxRetries.ShouldEqual(9); + config.Timeout.ShouldEqual(TimeSpan.FromSeconds(41)); + config.ReadTimeout.ShouldEqual(TimeSpan.FromSeconds(42)); + tracer.RelatedWarningEvents.Count.ShouldEqual(0); + } + } + + [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\tread-timeout-seconds = 32\n")) + { + RetryConfig libGit2Config = RetryConfig.LoadFromGitConfig(new MockTracer(), RetryConfig.CreateLibGit2Reader(new MockTracer(), repo.WorkingDirectory)); + RetryConfig gitConfig = RetryConfig.LoadFromGitConfig(new MockTracer(), new GitProcess(gitPath, repo.WorkingDirectory)); + + libGit2Config.MaxRetries.ShouldEqual(8); + gitConfig.MaxRetries.ShouldEqual(8); + libGit2Config.Timeout.ShouldEqual(TimeSpan.FromSeconds(31)); + gitConfig.Timeout.ShouldEqual(TimeSpan.FromSeconds(31)); + libGit2Config.ReadTimeout.ShouldEqual(TimeSpan.FromSeconds(32)); + gitConfig.ReadTimeout.ShouldEqual(TimeSpan.FromSeconds(32)); + } + } + + [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 config = RetryConfig.LoadFromGitConfig(tracer, RetryConfig.WithGitFallback(tracer, readWithLibGit2, readWithGit)); + config.MaxRetries.ShouldEqual(12); + tracer.RelatedWarningEvents.Count.ShouldEqual(0); + } + } + + [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; + } private class RetryConfigVerb : GVFSVerb { public override string EnlistmentRootPathParameter { get; set; } @@ -246,5 +543,33 @@ public override ConfigResult GetFromConfig( return base.GetFromConfig(settingName, forceOutsideEnlistment, fileSystem); } } + + // 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 d9aaebc0766b6261d4ae047751c7a8ae21b1d045 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 3 Nov 2026 09:16:33 -0800 Subject: [PATCH 3/7] Use the shared config read retry for credential fill Credential fill had its own retry loop for a transient config read failure. It now uses GitConfigReadRetry, so one policy sets the attempt count, the delays, and the traces for every config read. Credential fill keeps its stricter check: it retries only an empty-output failure in which git reports a transient file access warning and its config read failure, and no other error. Each attempt still gets only the time that remains of the caller's timeout. Add two optional parameters to GitConfigReadRetry.InvokeResult. canWait stops the retries when the caller's time budget cannot cover the next delay. beforeRetry receives each failed result that will be read again. Credential fill uses beforeRetry to write git's error output to the local log. Credential fill now reports retries with the GitConfigReadRetry warnings (ConfigName "credential fill"), not with the CredentialConfigReadFailure event. The transient reasons come from the shared list. Update the credential fill unit tests to assert the shared traces. Add a unit test for canWait and beforeRetry. Assisted-by: Claude Opus 5.5 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/GitConfigReadRetry.cs | 14 +++- GVFS/GVFS.Common/Git/GitProcess.cs | 83 ++++++------------- .../Common/Git/GitProcessCredentialTests.cs | 42 ++++++---- .../Git/GitConfigReadRetryTests.cs | 30 +++++++ 4 files changed, 93 insertions(+), 76 deletions(-) diff --git a/GVFS/GVFS.Common/Git/GitConfigReadRetry.cs b/GVFS/GVFS.Common/Git/GitConfigReadRetry.cs index 690df4ce20..f1b362ede1 100644 --- a/GVFS/GVFS.Common/Git/GitConfigReadRetry.cs +++ b/GVFS/GVFS.Common/Git/GitConfigReadRetry.cs @@ -17,13 +17,20 @@ internal static class GitConfigReadRetry "sharing violation", }; + /// + /// Returns false when the caller's time budget cannot cover the next delay. The read then + /// stops as if it used all attempts. + /// + /// Receives each failed result that will be read again. internal static T InvokeResult( string configName, Func readConfig, Func isTransientFailure, ITracer tracer, Action delay, - int maxAttempts = MaxAttempts) + int maxAttempts = MaxAttempts, + Func canWait = null, + Action beforeRetry = null) { for (int attempt = 1; ; ++attempt) { @@ -33,14 +40,15 @@ internal static T InvokeResult( return result; } - if (attempt >= maxAttempts) + TimeSpan retryDelay = GetRetryDelay(attempt); + if (attempt >= maxAttempts || (canWait != null && !canWait(retryDelay))) { TraceExhausted(tracer, configName, attempt); return result; } - TimeSpan retryDelay = GetRetryDelay(attempt); TraceRetry(tracer, configName, attempt, maxAttempts, retryDelay); + beforeRetry?.Invoke(result); delay(retryDelay); } } diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index 8398233da8..f6287134a3 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -40,8 +40,6 @@ public class GitProcess : ICredentialStore private const string MissingPasswordErrorMessage = "Git credential fill returned no password"; private const int MaxCredentialOutputKeys = 32; private const int MaxCredentialOutputKeyLength = 32; - private const int MaxCredentialConfigReadAttempts = 5; - private const int CredentialConfigReadRetryDelayMs = 20; private static readonly Encoding UTF8NoBOM = new UTF8Encoding(false); @@ -1231,51 +1229,31 @@ private Result InvokeCredentialFill( bool hasVisibleConsoleWindow, int timeoutMs = Timeout.Infinite) { + // Each attempt gets only the time that remains of the caller's timeout. Stopwatch timer = Stopwatch.StartNew(); - int remainingTimeoutMs = timeoutMs; - for (int attempt = 1; ; ++attempt) - { - Result result = this.InvokeGitAgainstDotGitFolder( - command, - stdin => stdin.Write(input), - parseStdOutLine: null, - usePreCommandHook: false, - timeoutMs: remainingTimeoutMs, - mayRequireAuth: true, - hasVisibleConsoleWindow: hasVisibleConsoleWindow); - - if (!IsCredentialConfigReadFailure(result)) - { - return result; - } - - int delayMs = CredentialConfigReadRetryDelayMs << (attempt - 1); - bool willRetry = attempt < MaxCredentialConfigReadAttempts && - (timeoutMs == Timeout.Infinite || timeoutMs - timer.ElapsedMilliseconds > delayMs); - tracer.RelatedEvent( - EventLevel.Warning, - "CredentialConfigReadFailure", - new EventMetadata - { - { "AttemptNumber", attempt }, - { "MaxAttempts", MaxCredentialConfigReadAttempts }, - { "WillRetry", willRetry }, - { "RetryDelayMilliseconds", willRetry ? delayMs : 0 }, - }, - Keywords.Network | Keywords.Telemetry); - - if (!willRetry) - { - return result; - } - - TraceCredentialFillErrorOutput(tracer, result.Errors); - Thread.Sleep(delayMs); - if (timeoutMs != Timeout.Infinite) + bool firstAttempt = true; + return GitConfigReadRetry.InvokeResult( + "credential fill", + () => { - remainingTimeoutMs = (int)Math.Max(0, timeoutMs - timer.ElapsedMilliseconds); - } - } + int attemptTimeoutMs = firstAttempt || timeoutMs == Timeout.Infinite + ? timeoutMs + : (int)Math.Max(0, timeoutMs - timer.ElapsedMilliseconds); + firstAttempt = false; + return this.InvokeGitAgainstDotGitFolder( + command, + stdin => stdin.Write(input), + parseStdOutLine: null, + usePreCommandHook: false, + timeoutMs: attemptTimeoutMs, + mayRequireAuth: true, + hasVisibleConsoleWindow: hasVisibleConsoleWindow); + }, + IsCredentialConfigReadFailure, + tracer, + this.configReadDelay, + canWait: delay => timeoutMs == Timeout.Infinite || timeoutMs - timer.ElapsedMilliseconds > delay.TotalMilliseconds, + beforeRetry: result => TraceCredentialFillErrorOutput(tracer, result.Errors)); } private static bool IsCredentialConfigReadFailure(Result result) @@ -1301,20 +1279,9 @@ private static bool IsCredentialConfigReadFailure(Result result) { configReadFailed = true; } - else if (line.StartsWith("warning: unable to access '", StringComparison.Ordinal)) + else if (line.StartsWith("warning: unable to access '", StringComparison.Ordinal) && + GitConfigReadRetry.IsTransientFileAccessError(line)) { - int separator = line.LastIndexOf("': ", StringComparison.Ordinal); - if (separator < 0) - { - return false; - } - - string reason = line.Substring(separator + 3); - if (reason != "Permission denied" && reason != "Device or resource busy") - { - return false; - } - transientAccessFailure = true; } else diff --git a/GVFS/GVFS.UnitTests/Common/Git/GitProcessCredentialTests.cs b/GVFS/GVFS.UnitTests/Common/Git/GitProcessCredentialTests.cs index c3abf7178e..39899ec0c0 100644 --- a/GVFS/GVFS.UnitTests/Common/Git/GitProcessCredentialTests.cs +++ b/GVFS/GVFS.UnitTests/Common/Git/GitProcessCredentialTests.cs @@ -50,9 +50,11 @@ public void CredentialFillRetriesAConfigReadFailure(bool certificate, string rea password.ShouldEqual(SecretValue); attempts.ShouldEqual(2); gitProcess.InvocationsRun.Select(i => i.TimeoutMs).ShouldMatchInOrder(-1, -1); - tracer.RelatedEventNames.ShouldMatchInOrder("CredentialConfigReadFailure"); - ((bool)tracer.RelatedEventMetadata[0]["WillRetry"]).ShouldBeTrue(); - AssertConfigReadFailureTelemetry(tracer); + List retryTelemetry = ConfigReadRetryTelemetry(tracer); + retryTelemetry.Count.ShouldEqual(1); + retryTelemetry[0]["ConfigName"].ShouldEqual("credential fill"); + retryTelemetry[0]["AttemptNumber"].ShouldEqual(1); + AssertConfigReadRetryTelemetryHasNoStderr(tracer); AssertNoSecret(tracer); } @@ -65,11 +67,13 @@ public void CredentialFillStopsAfterTheConfigRetryBudget() gitProcess.TryGetCredential(tracer, "mock://repoUrl", out _, out _, out string error).ShouldBeFalse(); gitProcess.InvocationsRun.Count.ShouldEqual(5); - tracer.RelatedEventMetadata.Select(e => (bool)e["WillRetry"]).ShouldMatchInOrder(true, true, true, true, false); - tracer.RelatedEventMetadata.Select(e => (int)e["AttemptNumber"]).ShouldMatchInOrder(1, 2, 3, 4, 5); - tracer.RelatedEventMetadata.Select(e => (int)e["MaxAttempts"]).ShouldMatchInOrder(5, 5, 5, 5, 5); - tracer.RelatedEventMetadata.Select(e => (int)e["RetryDelayMilliseconds"]).ShouldMatchInOrder(20, 40, 80, 160, 0); - AssertConfigReadFailureTelemetry(tracer); + List retryTelemetry = ConfigReadRetryTelemetry(tracer); + retryTelemetry.Take(4).Select(e => (int)e["AttemptNumber"]).ShouldMatchInOrder(1, 2, 3, 4); + retryTelemetry.Take(4).Select(e => (double)e["RetryDelayMilliseconds"]).ShouldMatchInOrder(20.0, 40.0, 80.0, 160.0); + retryTelemetry.Take(4).Select(e => (int)e["MaxAttempts"]).ShouldMatchInOrder(5, 5, 5, 5); + retryTelemetry.Count.ShouldEqual(5); + retryTelemetry[4]["Attempts"].ShouldEqual(5); + AssertConfigReadRetryTelemetryHasNoStderr(tracer); AssertErrorMessageHasNoStderr(error); tracer.RelatedWarningEvents.Any(e => e.Contains("Permission denied")).ShouldBeTrue(); } @@ -95,7 +99,7 @@ public void CredentialFillDoesNotRetryOtherFailures(string errors, string output gitProcess.TryGetCredential(tracer, "mock://repoUrl", out _, out _, out _).ShouldBeFalse(); gitProcess.InvocationsRun.Count.ShouldEqual(1); - tracer.RelatedEventNames.ShouldBeEmpty(); + ConfigReadRetryTelemetry(tracer).ShouldBeEmpty(); } [TestCase] @@ -143,7 +147,7 @@ public void CredentialFillDoesNotRetryWhenTheTimeoutCannotCoverTheDelay() gitProcess.TryGetCredential(tracer, "mock://repoUrl", out _, out _, out _, timeoutMs: 1).ShouldBeFalse(); gitProcess.InvocationsRun.Count.ShouldEqual(1); - ((bool)tracer.RelatedEventMetadata.Single()["WillRetry"]).ShouldBeFalse(); + ConfigReadRetryTelemetry(tracer).Single()["Attempts"].ShouldEqual(1); } [TestCase(true, 30000)] @@ -430,13 +434,21 @@ public void TryGetCredentialFailsWhenNoLineStartsWithThePasswordKey(string outpu password.ShouldBeNull(); } - private static void AssertConfigReadFailureTelemetry(MockTracer tracer) + // The GitConfigReadRetry events are the telemetry warnings that name a config read. + private static List ConfigReadRetryTelemetry(MockTracer tracer) { - foreach (EventMetadata metadata in tracer.RelatedEventMetadata) + return tracer.TelemetryWarnings.Where(metadata => metadata.ContainsKey("ConfigName")).ToList(); + } + + private static void AssertConfigReadRetryTelemetryHasNoStderr(MockTracer tracer) + { + foreach (EventMetadata metadata in ConfigReadRetryTelemetry(tracer)) { - CollectionAssert.AreEquivalent( - new[] { "AttemptNumber", "MaxAttempts", "WillRetry", "RetryDelayMilliseconds" }, - metadata.Keys); + foreach (string key in metadata.Keys) + { + new[] { "ConfigName", "AttemptNumber", "MaxAttempts", "RetryDelayMilliseconds", "Attempts", "WarningMessage" } + .ShouldContain(allowed => allowed == key); + } } } diff --git a/GVFS/GVFS.UnitTests/Git/GitConfigReadRetryTests.cs b/GVFS/GVFS.UnitTests/Git/GitConfigReadRetryTests.cs index 682b70bef1..fffe3349cd 100644 --- a/GVFS/GVFS.UnitTests/Git/GitConfigReadRetryTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitConfigReadRetryTests.cs @@ -54,6 +54,36 @@ public void InvokeResultStopsAtTheRequestedAttemptLimit() delays.ShouldEqual(1); } + [TestCase] + public void InvokeResultStopsWhenTheCallerCannotWaitForTheDelay() + { + MockTracer tracer = new MockTracer(); + List requestedWaits = new List(); + List retriedResults = new List(); + int reads = 0; + int delays = 0; + + GitConfigReadRetry.InvokeResult( + "gvfs.test", + () => ++reads, + (int _) => true, + tracer, + delay => ++delays, + canWait: wait => + { + requestedWaits.Add(wait); + return requestedWaits.Count < 3; + }, + beforeRetry: retriedResults.Add); + + reads.ShouldEqual(3); + delays.ShouldEqual(2); + retriedResults.ShouldMatchInOrder(1, 2); + requestedWaits.ShouldMatchInOrder(TimeSpan.FromMilliseconds(20), TimeSpan.FromMilliseconds(40), TimeSpan.FromMilliseconds(80)); + tracer.RelatedWarningEvents.Count.ShouldEqual(3); + tracer.RelatedWarningEvents[2].ShouldContain("persisted after all attempts"); + } + [TestCase] public void InvokeResultDoesNotRetryWhenOneAttemptIsAllowed() { From 573684b0aafa51747a718a2c4f4b62b7f2cefa89 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 3 Nov 2026 11:52:03 -0800 Subject: [PATCH 4/7] Read with git.exe for includes libgit2 evaluates differently The RetryConfig reader trusted three include layouts that libgit2 evaluates differently from git.exe: - onbranch: on a detached HEAD. libgit2 can apply the include, because it does not set the match result in that case. git.exe does not. - A gitdir: or gitdir/i: pattern that starts with a slash. git.exe treats it as a path without a drive. libgit2 matches it anywhere, as if it started with "**". - An include.path or includeIf.*.path that uses %(prefix). git.exe expands it to its installation directory. libgit2 skips the include. The libgit2 read now fails for these layouts, and git.exe reads the setting. libgit2 is still used for gitdir: and gitdir/i: patterns that do not start with a slash. Add unit tests that compare the reader with the installed git.exe for onbranch: on a branch and on a detached HEAD, and for drive-less gitdir: patterns. Add unit tests that check that %(prefix) include paths are read with git.exe. Assisted-by: Claude Opus 5.5 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/LibGit2Repo.cs | 77 +++++++++++++------ GVFS/GVFS.Common/RetryConfig.cs | 4 +- .../GVFS.UnitTests/Common/RetryConfigTests.cs | 58 +++++++++++++- 3 files changed, 109 insertions(+), 30 deletions(-) diff --git a/GVFS/GVFS.Common/Git/LibGit2Repo.cs b/GVFS/GVFS.Common/Git/LibGit2Repo.cs index a007856d52..617217efbf 100644 --- a/GVFS/GVFS.Common/Git/LibGit2Repo.cs +++ b/GVFS/GVFS.Common/Git/LibGit2Repo.cs @@ -47,12 +47,12 @@ public class LibGit2Repo : IDisposable "GIT_CONFIG_SYSTEM", }; - // git.exe also evaluates hasconfig:remote.*.url conditions. - private static readonly string[] IncludeConditionsLibGit2Evaluates = + // Of the includeIf conditions, libgit2 evaluates only these the way git.exe does. It skips + // others, such as hasconfig:remote.*.url, and it can apply onbranch: on a detached HEAD. + private static readonly string[] IncludeConditionsLibGit2EvaluatesLikeGit = { "gitdir:", "gitdir/i:", - "onbranch:", }; private bool disposedValue = false; @@ -384,10 +384,10 @@ private static string GetStringFromSnapshot(IntPtr configHandle, string name, bo { if (rejectIncludesLibGit2Ignores) { - string condition = FindIncludeConditionLibGit2Ignores(snapshotHandle); - if (condition != null) + string include = FindIncludeLibGit2EvaluatesDifferently(snapshotHandle); + if (include != null) { - throw new LibGit2Exception($"Config has an includeIf condition that libgit2 does not evaluate: {condition}"); + throw new LibGit2Exception($"Config has an include that libgit2 does not evaluate like git: {include}"); } } @@ -415,47 +415,74 @@ private static string GetStringFromSnapshot(IntPtr configHandle, string name, bo } /// - /// Returns the first includeIf condition that libgit2 skips but git.exe can apply, or null. + /// Returns the first include ("name = path") that libgit2 can evaluate differently from + /// git.exe, or null. /// - private static string FindIncludeConditionLibGit2Ignores(IntPtr configHandle) + private static string FindIncludeLibGit2EvaluatesDifferently(IntPtr configHandle) { - const string Prefix = "includeif."; + const string ConditionalPrefix = "includeif."; const string Suffix = ".path"; - string ignoredCondition = null; + string differentInclude = null; Native.Config.GitConfigMultivarCallback callback = (entryPtr, payload) => { try { - string entryName = Marshal.PtrToStructure(entryPtr).GetName(); - string condition = entryName.Substring(Prefix.Length, entryName.Length - Prefix.Length - Suffix.Length); - foreach (string evaluated in IncludeConditionsLibGit2Evaluates) + Native.Config.GitConfigEntry entry = Marshal.PtrToStructure(entryPtr); + string entryName = entry.GetName(); + string path = entry.GetValue() ?? string.Empty; + if (IsIncludeLibGit2EvaluatesLikeGit(entryName, path, ConditionalPrefix, Suffix)) { - if (condition.StartsWith(evaluated, StringComparison.Ordinal)) - { - return Native.ResultCode.Success; - } + return Native.ResultCode.Success; } - ignoredCondition = condition; + differentInclude = entryName + " = " + path; } catch (Exception e) { // An exception must not cross the native callback boundary. - ignoredCondition = ""; + differentInclude = ""; } return Native.ResultCode.Failure; }; - Native.ResultCode resultCode = Native.Config.ForeachMatch(configHandle, @"^includeif\..*\.path$", callback, IntPtr.Zero); + Native.ResultCode resultCode = Native.Config.ForeachMatch(configHandle, @"^include(if\..*)?\.path$", callback, IntPtr.Zero); GC.KeepAlive(callback); - if (ignoredCondition == null && resultCode != Native.ResultCode.Success) + if (differentInclude == null && resultCode != Native.ResultCode.Success) + { + throw new LibGit2Exception($"Failed to enumerate include config: {Native.GetLastError()}"); + } + + return differentInclude; + } + + private static bool IsIncludeLibGit2EvaluatesLikeGit(string entryName, string path, string conditionalPrefix, string suffix) + { + // git.exe expands %(prefix) to its installation directory. libgit2 skips the include. + if (path.IndexOf("%(prefix)", StringComparison.Ordinal) >= 0) + { + return false; + } + + if (!entryName.StartsWith(conditionalPrefix, StringComparison.Ordinal)) + { + return true; + } + + string condition = entryName.Substring(conditionalPrefix.Length, entryName.Length - conditionalPrefix.Length - suffix.Length); + foreach (string evaluated in IncludeConditionsLibGit2EvaluatesLikeGit) { - throw new LibGit2Exception($"Failed to enumerate includeIf config: {Native.GetLastError()}"); + if (condition.StartsWith(evaluated, StringComparison.Ordinal)) + { + // git.exe treats a pattern that starts with a slash as a path without a drive. + // libgit2 matches it anywhere, as if it started with "**". + string pattern = condition.Substring(evaluated.Length); + return !pattern.StartsWith("/", StringComparison.Ordinal) && !pattern.StartsWith("\\", StringComparison.Ordinal); + } } - return ignoredCondition; + return false; } public virtual bool? GetConfigBool(string name) @@ -551,8 +578,8 @@ 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 . + /// the key is unset. Fails when the config has an include that libgit2 can evaluate + /// differently from git.exe. Callers must also check . /// internal static bool TryGetConfigString(ITracer tracer, string repoPath, string name, out string value, out string error) { diff --git a/GVFS/GVFS.Common/RetryConfig.cs b/GVFS/GVFS.Common/RetryConfig.cs index 5d755d6948..4751c8cf4f 100644 --- a/GVFS/GVFS.Common/RetryConfig.cs +++ b/GVFS/GVFS.Common/RetryConfig.cs @@ -92,8 +92,8 @@ internal static ConfigStringReader CreateLibGit2Reader(ITracer tracer, string wo /// /// 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. + /// ownership or evaluates an include differently from git.exe. 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) { diff --git a/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs b/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs index d91b4a0cca..6f6c111b09 100644 --- a/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs +++ b/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs @@ -408,7 +408,7 @@ public void LibGit2ReaderRejectsAGlobalIncludeConditionItIgnores() RetryConfig.CreateLibGit2Reader(new MockTracer(), missingWorkingDirectory)("gvfs.max-retries", out string _, out string error) .ShouldBeFalse(); - error.ShouldContain("includeIf condition that libgit2 does not evaluate: hasconfig:"); + error.ShouldContain("libgit2 does not evaluate like git: includeif.hasconfig:"); } } @@ -450,7 +450,7 @@ public void ConfigWithAnIncludeConditionLibGit2IgnoresIsReadWithGit() 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/**"); + libGit2Error.ShouldContain("libgit2 does not evaluate like git: includeif.hasconfig:remote.*.url:https://example.com/**.path"); RetryConfig config = RetryConfig.LoadFromGitConfig(tracer, RetryConfig.WithGitFallback(tracer, readWithLibGit2, readWithGit)); config.MaxRetries.ShouldEqual(12); @@ -458,9 +458,61 @@ public void ConfigWithAnIncludeConditionLibGit2IgnoresIsReadWithGit() } } + // In each layout, libgit2 evaluates the include differently from git.exe. + [TestCase("onbranch:main", false)] + [TestCase("onbranch:main", true)] + [TestCase("gitdir:", false)] + [TestCase("gitdir/i:", false)] + public void IncludeLayoutsLibGit2EvaluatesDifferentlyAreReadWithGit(string condition, bool headOnBranch) + { + string gitPath = GetGitPathOrIgnore(); + using (TestRepository repo = new TestRepository(string.Empty)) + { + if (!headOnBranch) + { + File.WriteAllText(Path.Combine(repo.WorkingDirectory, ".git", "HEAD"), new string('1', 40) + "\n"); + } + + string gitDir = Path.Combine(repo.WorkingDirectory, ".git").Replace('\\', '/') + "/"; + 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.Replace("", gitDir.Substring(2)) + "\"]\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("libgit2 does not evaluate like git"); + + readWithGit("gvfs.max-retries", out string gitValue, out string gitError).ShouldBeTrue(gitError); + RetryConfig config = RetryConfig.LoadFromGitConfig(tracer, RetryConfig.WithGitFallback(tracer, readWithLibGit2, readWithGit)); + config.MaxRetries.ShouldEqual(gitValue == null ? RetryConfig.DefaultMaxRetries : int.Parse(gitValue)); + config.MaxRetries.ShouldEqual(headOnBranch && condition.StartsWith("onbranch:") ? 13 : RetryConfig.DefaultMaxRetries); + tracer.RelatedWarningEvents.Count.ShouldEqual(0); + } + } + + // git.exe expands %(prefix) to its installation directory. libgit2 skips the include. + [TestCase("[include]\n\tpath = %(prefix)/etc/gvfs.gitconfig\n")] + [TestCase("[includeIf \"gitdir:**/\"]\n\tpath = %(prefix)/etc/gvfs.gitconfig\n")] + public void IncludePathsWithAGitPrefixAreReadWithGit(string includeConfig) + { + using (TestRepository repo = new TestRepository(includeConfig)) + { + RetryConfig.CreateLibGit2Reader(new MockTracer(), repo.WorkingDirectory)("gvfs.max-retries", out string _, out string error) + .ShouldBeFalse(); + error.ShouldContain("libgit2 does not evaluate like git", "%(prefix)/etc/gvfs.gitconfig"); + } + } + [TestCase("gitdir:**/")] [TestCase("gitdir/i:**/")] - [TestCase("onbranch:main")] public void LibGit2ReadsConfigWithIncludeConditionsItEvaluates(string condition) { using (TestRepository repo = new TestRepository(string.Empty)) From b07d3da49f80ae9b466e25c8105f3eb6b7dae7e8 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 3 Nov 2026 12:29:01 -0800 Subject: [PATCH 5/7] Check loaded retry settings, not git.exe commands, in authentication test AuthenticationInitializationLoadsAllRetrySettings counted the git config commands that the mock git process ran. RetryConfig now reads the settings with libgit2 unless the environment changes Git config, so the test failed when no GIT_CONFIG_* variable was set. The test now gives the same values to libgit2, through an isolated global config, and to the mock git process. It checks the values in the RetryConfig_LoadedRetryConfig event, so it passes on either read path. Assisted-by: Claude Opus 5.5 Signed-off-by: Tyrie Vella --- GVFS/GVFS.UnitTests/Http/HttpResponseStreamTests.cs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/GVFS/GVFS.UnitTests/Http/HttpResponseStreamTests.cs b/GVFS/GVFS.UnitTests/Http/HttpResponseStreamTests.cs index 2eb95bc11e..1ee094a44f 100644 --- a/GVFS/GVFS.UnitTests/Http/HttpResponseStreamTests.cs +++ b/GVFS/GVFS.UnitTests/Http/HttpResponseStreamTests.cs @@ -638,13 +638,18 @@ public async Task AuthenticationInitializationLoadsAllRetrySettings() GitAuthentication authentication = new GitAuthentication(git, endpoint); MockGVFSEnlistment enlistment = new MockGVFSEnlistment( "mock:\\repo", endpoint, "mock:\\git", git, authentication); + + // libgit2 reads the settings from the isolated global config. When the environment + // changes Git config, git.exe reads the same values from the mock. + using IsolatedLibGit2Config machineConfig = new IsolatedLibGit2Config(); + File.WriteAllText(machineConfig.GlobalConfigPath, "[gvfs]\n\tmax-retries = 0\n\ttimeout-seconds = 2\n\tread-timeout-seconds = 1\n"); try { authentication.TryInitialize(tracer, enlistment, out string error).ShouldEqual(true, error); - foreach (string key in new[] { "gvfs.max-retries", "gvfs.timeout-seconds", "gvfs.read-timeout-seconds" }) - { - git.CommandsRun.Count(command => command == "config " + key).ShouldEqual(1); - } + EventMetadata loaded = tracer.RelatedEventMetadata[tracer.RelatedEventNames.IndexOf("RetryConfig_LoadedRetryConfig")]; + loaded["MaxRetries"].ShouldEqual(0); + loaded["Timeout"].ShouldEqual(TimeSpan.FromSeconds(2)); + loaded["ReadTimeout"].ShouldEqual(TimeSpan.FromSeconds(1)); } finally { From d654acbb0be13b97b496cfa2746cbf46b07dae27 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 3 Nov 2026 12:59:22 -0800 Subject: [PATCH 6/7] Release libgit2 only from repositories that initialized it LibGit2Repo.Dispose freed the repository handle and called git_libgit2_shutdown for every instance. The protected constructors that test doubles use do not call git_libgit2_init, so disposing or finalizing such an instance released a libgit2 reference that another owner held. When the count reached zero, libgit2 released its global state, including the config search paths that IsolatedLibGit2Config sets. Tests that ran later read the machine's global config instead. Because a finalizer could run at any time, the authentication retry-settings test failed only in some CI runs. Record whether the constructor initialized libgit2, and release the repository handle and the libgit2 reference only in that case. Add a unit test that disposes a MockLibGit2Repo while an isolated config is active and checks that a later read still uses the isolated config. Assisted-by: Claude Opus 5.5 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/LibGit2Repo.cs | 12 ++++++++++-- GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs | 17 +++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/GVFS/GVFS.Common/Git/LibGit2Repo.cs b/GVFS/GVFS.Common/Git/LibGit2Repo.cs index 617217efbf..accd73bf28 100644 --- a/GVFS/GVFS.Common/Git/LibGit2Repo.cs +++ b/GVFS/GVFS.Common/Git/LibGit2Repo.cs @@ -56,6 +56,7 @@ public class LibGit2Repo : IDisposable }; private bool disposedValue = false; + private bool initializedNative = false; private IntPtr odbHandle = IntPtr.Zero; private readonly Action configReadDelay; private readonly int configReadMaxAttempts; @@ -74,6 +75,7 @@ internal LibGit2Repo(ITracer tracer, string repoPath, Action configRea this.configReadMaxAttempts = configReadMaxAttempts; InitNative(); + this.initializedNative = true; // Opening the repo parses .git\config, so it is subject to the same transient failures as a config read. IntPtr repoHandle = IntPtr.Zero; @@ -821,8 +823,14 @@ protected virtual void Dispose(bool disposing) this.odbHandle = IntPtr.Zero; } - Native.Repo.Free(this.RepoHandle); - Native.Shutdown(); + // The protected constructors for test doubles do not initialize libgit2, so their + // instances have no libgit2 reference to release. + if (this.initializedNative) + { + Native.Repo.Free(this.RepoHandle); + Native.Shutdown(); + } + this.disposedValue = true; } } diff --git a/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs b/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs index 6f6c111b09..6cc9039f40 100644 --- a/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs +++ b/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs @@ -395,6 +395,23 @@ public void LibGit2ReaderReadsGlobalConfigBeforeTheRepositoryExists() } } + [TestCase] + public void DisposingARepositoryWithoutNativeStateKeepsTheIsolatedConfig() + { + using (IsolatedLibGit2Config machineConfig = new IsolatedLibGit2Config()) + { + File.WriteAllText(machineConfig.GlobalConfigPath, "[gvfs]\n\tmax-retries = 9\n"); + + // This repository never initialized libgit2, so disposing it must not shut libgit2 down. + new MockLibGit2Repo(new MockTracer()).Dispose(); + + string missingWorkingDirectory = Path.Combine(Path.GetTempPath(), "RetryConfigTests_" + Guid.NewGuid().ToString("N")); + RetryConfig.CreateLibGit2Reader(new MockTracer(), missingWorkingDirectory)("gvfs.max-retries", out string value, out string error) + .ShouldBeTrue(error); + value.ShouldEqual("9"); + } + } + [TestCase] public void LibGit2ReaderRejectsAGlobalIncludeConditionItIgnores() { From 600fc4df9634670c821a74eda2da07ecb72e9f67 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 4 Nov 2026 11:02:09 -0800 Subject: [PATCH 7/7] Read with git.exe when libgit2 selects a different user config file git for Windows reads the global config from one home directory: HOME, else HOMEDRIVE and HOMEPATH, else USERPROFILE. libgit2 searches all of these directories and uses the first one that contains .gitconfig. For the XDG file, libgit2 also searches APPDATA and LOCALAPPDATA, which git does not. So when HOME points to a directory without a .gitconfig, libgit2 can read the profile .gitconfig while git reads no global file. The retry settings then differ between the two readers. Before the libgit2 reader returns a value, compare the global and XDG files that libgit2 selects with the files that git for Windows reads. If they differ, fail the libgit2 read, so that RetryConfig reads the value with git.exe. Do not change the libgit2 search paths, because that state is process-wide. Make IsolatedLibGit2Config set HOME and XDG_CONFIG_HOME to match its search paths. Add a unit test that sets HOME to an empty directory and USERPROFILE to a directory with a .gitconfig. The test checks that the libgit2 reader fails, that real git.exe reads no value, and that RetryConfig uses the default value. Assisted-by: Claude Opus 5.5 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/LibGit2Repo.cs | 97 +++++++++++++++++++ .../GVFS.UnitTests/Common/RetryConfigTests.cs | 47 +++++++++ .../Mock/Git/IsolatedLibGit2Config.cs | 32 +++++- 3 files changed, 171 insertions(+), 5 deletions(-) diff --git a/GVFS/GVFS.Common/Git/LibGit2Repo.cs b/GVFS/GVFS.Common/Git/LibGit2Repo.cs index accd73bf28..c3540e55ed 100644 --- a/GVFS/GVFS.Common/Git/LibGit2Repo.cs +++ b/GVFS/GVFS.Common/Git/LibGit2Repo.cs @@ -386,6 +386,8 @@ private static string GetStringFromSnapshot(IntPtr configHandle, string name, bo { if (rejectIncludesLibGit2Ignores) { + ThrowIfUserConfigFilesDifferFromGit(); + string include = FindIncludeLibGit2EvaluatesDifferently(snapshotHandle); if (include != null) { @@ -416,6 +418,84 @@ private static string GetStringFromSnapshot(IntPtr configHandle, string name, bo } } + /// + /// Throws when libgit2 reads a different global or XDG config file than git.exe. git for + /// Windows uses one home directory: HOME, else HOMEDRIVE and HOMEPATH, else USERPROFILE. + /// libgit2 uses the first of these directories that contains the file. libgit2 also looks + /// for the XDG file under APPDATA and LOCALAPPDATA, which git does not. + /// + private static void ThrowIfUserConfigFilesDifferFromGit() + { + string home = GetGitForWindowsHomeDirectory(); + string xdgConfigHome = Environment.GetEnvironmentVariable("XDG_CONFIG_HOME"); + string gitGlobal = home == null ? null : Path.Combine(home, ".gitconfig"); + string gitXdg = !string.IsNullOrEmpty(xdgConfigHome) + ? Path.Combine(xdgConfigHome, "git", "config") + : home == null ? null : Path.Combine(home, ".config", "git", "config"); + + ThrowIfFileDiffersFromGit("global", FindUserConfigFile(isGlobal: true), gitGlobal); + ThrowIfFileDiffersFromGit("XDG", FindUserConfigFile(isGlobal: false), gitXdg); + } + + private static string GetGitForWindowsHomeDirectory() + { + string home = Environment.GetEnvironmentVariable("HOME"); + if (!string.IsNullOrEmpty(home)) + { + return home; + } + + string homeDrive = Environment.GetEnvironmentVariable("HOMEDRIVE"); + string homePath = Environment.GetEnvironmentVariable("HOMEPATH"); + if (!string.IsNullOrEmpty(homeDrive) && !string.IsNullOrEmpty(homePath) && Directory.Exists(homeDrive + homePath)) + { + return homeDrive + homePath; + } + + string userProfile = Environment.GetEnvironmentVariable("USERPROFILE"); + return string.IsNullOrEmpty(userProfile) ? null : userProfile; + } + + private static string FindUserConfigFile(bool isGlobal) + { + Native.Config.GitBuf buffer = default(Native.Config.GitBuf); + try + { + Native.ResultCode resultCode = isGlobal ? Native.Config.FindGlobal(ref buffer) : Native.Config.FindXdg(ref buffer); + if (resultCode == Native.ResultCode.NotFound) + { + return null; + } + + if (resultCode != Native.ResultCode.Success) + { + throw new LibGit2Exception($"Failed to find the {(isGlobal ? "global" : "XDG")} config file: {Native.GetLastError()}"); + } + + return Marshal.PtrToStringUTF8(buffer.Ptr, checked((int)buffer.Size.ToUInt64())); + } + finally + { + Native.Config.DisposeBuf(ref buffer); + } + } + + private static void ThrowIfFileDiffersFromGit(string level, string libGit2Path, string gitPath) + { + if (gitPath != null && !File.Exists(gitPath)) + { + gitPath = null; + } + + libGit2Path = libGit2Path == null ? null : Path.GetFullPath(libGit2Path); + gitPath = gitPath == null ? null : Path.GetFullPath(gitPath); + if (!string.Equals(libGit2Path, gitPath, StringComparison.OrdinalIgnoreCase)) + { + throw new LibGit2Exception( + $"libgit2 reads the {level} config file '{libGit2Path ?? ""}', but git reads '{gitPath ?? ""}'"); + } + } + /// /// Returns the first include ("name = path") that libgit2 can evaluate differently from /// git.exe, or null. @@ -1000,6 +1080,23 @@ public static class Odb public static class Config { + [DllImport(Git2NativeLibName, EntryPoint = "git_config_find_global")] + public static extern ResultCode FindGlobal(ref GitBuf path); + + [DllImport(Git2NativeLibName, EntryPoint = "git_config_find_xdg")] + public static extern ResultCode FindXdg(ref GitBuf path); + + [DllImport(Git2NativeLibName, EntryPoint = "git_buf_dispose")] + public static extern void DisposeBuf(ref GitBuf buffer); + + [StructLayout(LayoutKind.Sequential)] + public struct GitBuf + { + public IntPtr Ptr; + public UIntPtr Reserved; + public UIntPtr Size; + } + [DllImport(Git2NativeLibName, EntryPoint = "git_repository_config")] public static extern ResultCode GetConfig(out IntPtr configHandle, IntPtr repoHandle); diff --git a/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs b/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs index 6cc9039f40..64ab99190f 100644 --- a/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs +++ b/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs @@ -395,6 +395,53 @@ public void LibGit2ReaderReadsGlobalConfigBeforeTheRepositoryExists() } } + [TestCase] + public void UserConfigThatGitDoesNotReadIsReadWithGit() + { + string gitPath = GetGitPathOrIgnore(); + string[] variables = { "HOMEDRIVE", "HOMEPATH", "USERPROFILE" }; + string[] originalValues = Array.ConvertAll(variables, Environment.GetEnvironmentVariable); + using (IsolatedLibGit2Config machineConfig = new IsolatedLibGit2Config()) + { + // git reads only HOME\.gitconfig. libgit2 also falls back to USERPROFILE\.gitconfig. + string emptyHome = Path.Combine(machineConfig.Directory, "home"); + string profile = Path.Combine(machineConfig.Directory, "profile"); + Directory.CreateDirectory(emptyHome); + Directory.CreateDirectory(profile); + File.WriteAllText(Path.Combine(profile, ".gitconfig"), "[gvfs]\n\ttimeout-seconds = 1\n"); + try + { + Environment.SetEnvironmentVariable("HOME", emptyHome); + Environment.SetEnvironmentVariable("HOMEDRIVE", null); + Environment.SetEnvironmentVariable("HOMEPATH", null); + Environment.SetEnvironmentVariable("USERPROFILE", profile); + IsolatedLibGit2Config.ResetUserSearchPathsFromEnvironment(); + + string missingWorkingDirectory = Path.Combine(Path.GetTempPath(), "RetryConfigTests_" + Guid.NewGuid().ToString("N")); + MockTracer tracer = new MockTracer(); + RetryConfig.ConfigStringReader readWithLibGit2 = RetryConfig.CreateLibGit2Reader(tracer, missingWorkingDirectory); + GitProcess git = new GitProcess(gitPath, workingDirectoryRoot: null); + RetryConfig.ConfigStringReader readWithGit = (string name, out string value, out string error) => + git.GetFromConfig(name).TryParseAsString(out value, out error); + + readWithLibGit2("gvfs.timeout-seconds", out string _, out string libGit2Error).ShouldBeFalse(); + libGit2Error.ShouldContain("libgit2 reads the global config file", Path.Combine(profile, ".gitconfig")); + readWithGit("gvfs.timeout-seconds", out string gitValue, out string gitError).ShouldBeTrue(gitError); + gitValue.ShouldBeNull(); + + RetryConfig config = RetryConfig.LoadFromGitConfig(tracer, RetryConfig.WithGitFallback(tracer, readWithLibGit2, readWithGit)); + config.Timeout.ShouldEqual(TimeSpan.FromSeconds(RetryConfig.DefaultTimeoutSeconds)); + } + finally + { + for (int i = 0; i < variables.Length; ++i) + { + Environment.SetEnvironmentVariable(variables[i], originalValues[i]); + } + } + } + } + [TestCase] public void DisposingARepositoryWithoutNativeStateKeepsTheIsolatedConfig() { diff --git a/GVFS/GVFS.UnitTests/Mock/Git/IsolatedLibGit2Config.cs b/GVFS/GVFS.UnitTests/Mock/Git/IsolatedLibGit2Config.cs index 4c881c8f75..49d3c870a0 100644 --- a/GVFS/GVFS.UnitTests/Mock/Git/IsolatedLibGit2Config.cs +++ b/GVFS/GVFS.UnitTests/Mock/Git/IsolatedLibGit2Config.cs @@ -8,26 +8,36 @@ 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. + /// temporary directory, so a test does not read the machine's config. It also sets HOME and + /// XDG_CONFIG_HOME, so git.exe and the libgit2 reader agree on the global and XDG files. 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; + private const int XdgLevel = 3; // GIT_CONFIG_LEVEL_PROGRAMDATA, SYSTEM, XDG, and GLOBAL. - private static readonly int[] ConfigLevels = { 1, 2, 3, 4 }; + private static readonly int[] ConfigLevels = { 1, 2, XdgLevel, 4 }; + + private readonly string originalHome = Environment.GetEnvironmentVariable("HOME"); + private readonly string originalXdgConfigHome = Environment.GetEnvironmentVariable("XDG_CONFIG_HOME"); public IsolatedLibGit2Config() { this.Directory = Path.Combine(Path.GetTempPath(), "IsolatedLibGit2Config_" + Guid.NewGuid().ToString("N")); System.IO.Directory.CreateDirectory(this.Directory); + string xdgConfigHome = Path.Combine(this.Directory, "xdg"); + Environment.SetEnvironmentVariable("HOME", this.Directory); + Environment.SetEnvironmentVariable("XDG_CONFIG_HOME", xdgConfigHome); LibGit2Repo.Native.Init(); foreach (int level in ConfigLevels) { - if (SetSearchPath(SetSearchPathOption, level, this.Directory) != 0) + string path = level == XdgLevel ? Path.Combine(xdgConfigHome, "git") : this.Directory; + if (SetSearchPath(SetSearchPathOption, level, path) != 0) { throw new InvalidOperationException("git_libgit2_opts failed: " + LibGit2Repo.Native.GetLastError()); } @@ -38,6 +48,16 @@ public IsolatedLibGit2Config() public string GlobalConfigPath => Path.Combine(this.Directory, ".gitconfig"); + /// + /// Makes libgit2 find the global and XDG config from the current environment variables. + /// The caller must hold a libgit2 reference. + /// + public static void ResetUserSearchPathsFromEnvironment() + { + SetSearchPath(SetSearchPathOption, XdgLevel, null); + SetSearchPath(SetSearchPathOption, 4, null); + } + public void Dispose() { foreach (int level in ConfigLevels) @@ -46,6 +66,8 @@ public void Dispose() } LibGit2Repo.Native.Shutdown(); + Environment.SetEnvironmentVariable("HOME", this.originalHome); + Environment.SetEnvironmentVariable("XDG_CONFIG_HOME", this.originalXdgConfigHome); System.IO.Directory.Delete(this.Directory, recursive: true); }