diff --git a/GVFS/GVFS.Common/Enlistment.cs b/GVFS/GVFS.Common/Enlistment.cs index a9208b0edf..dcc0b58452 100644 --- a/GVFS/GVFS.Common/Enlistment.cs +++ b/GVFS/GVFS.Common/Enlistment.cs @@ -1,5 +1,6 @@ using GVFS.Common.FileSystem; using GVFS.Common.Git; +using GVFS.Common.Tracing; using System; using System.IO; @@ -117,5 +118,10 @@ public virtual GitProcess CreateGitProcess() { return new GitProcess(this); } + + public virtual GitProcess CreateGitProcess(ITracer tracer) + { + return new GitProcess(this, tracer); + } } } diff --git a/GVFS/GVFS.Common/Git/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/GitConfigReadRetry.cs b/GVFS/GVFS.Common/Git/GitConfigReadRetry.cs new file mode 100644 index 0000000000..690df4ce20 --- /dev/null +++ b/GVFS/GVFS.Common/Git/GitConfigReadRetry.cs @@ -0,0 +1,172 @@ +using GVFS.Common.Tracing; +using System; +using System.Threading; + +namespace GVFS.Common.Git +{ + internal static class GitConfigReadRetry + { + internal const int MaxAttempts = 5; + internal const int InitialDelayMilliseconds = 20; + + private static readonly string[] TransientReasons = + { + "Permission denied", + "Device or resource busy", + "being used by another process", + "sharing violation", + }; + + internal static T InvokeResult( + string configName, + Func readConfig, + Func isTransientFailure, + ITracer tracer, + Action delay, + int maxAttempts = MaxAttempts) + { + for (int attempt = 1; ; ++attempt) + { + T result = readConfig(); + if (!isTransientFailure(result)) + { + return result; + } + + if (attempt >= maxAttempts) + { + TraceExhausted(tracer, configName, attempt); + return result; + } + + TimeSpan retryDelay = GetRetryDelay(attempt); + TraceRetry(tracer, configName, attempt, maxAttempts, retryDelay); + delay(retryDelay); + } + } + + internal static T InvokeCatching( + string configName, + Func readConfig, + Func isTransientFailure, + ITracer tracer, + Action delay, + int maxAttempts = MaxAttempts) + { + for (int attempt = 1; ; ++attempt) + { + try + { + return readConfig(); + } + catch (Exception e) when (isTransientFailure(e)) + { + if (attempt >= maxAttempts) + { + TraceExhausted(tracer, configName, attempt); + throw; + } + + TimeSpan retryDelay = GetRetryDelay(attempt); + TraceRetry(tracer, configName, attempt, maxAttempts, retryDelay); + delay(retryDelay); + } + } + } + + internal static bool IsTransientLibGit2Error(Exception exception) + { + return + exception is LibGit2Exception && + IsTransientFileAccessError(exception.Message); + } + + /// + /// Both git.exe and libgit2 report a file failure as ... '<path>': <reason>. + /// Only the reason after the final ': is tested, so a path or key that contains + /// a transient phrase does not match. + /// + internal static bool IsTransientFileAccessError(string error, string lineMustContain = null) + { + if (string.IsNullOrEmpty(error)) + { + return false; + } + + foreach (string line in error.Split('\n')) + { + if (lineMustContain != null && !ContainsOrdinalIgnoreCase(line, lineMustContain)) + { + continue; + } + + int reasonSeparator = line.LastIndexOf("': ", StringComparison.Ordinal); + if (reasonSeparator < 0) + { + continue; + } + + string reason = line.Substring(reasonSeparator + 3); + foreach (string transientReason in TransientReasons) + { + if (ContainsOrdinalIgnoreCase(reason, transientReason)) + { + return true; + } + } + } + + return false; + } + + internal static bool ContainsOrdinalIgnoreCase(string value, string expected) + { + return + value != null && + value.IndexOf(expected, StringComparison.OrdinalIgnoreCase) >= 0; + } + + internal static void Delay(TimeSpan delay) + { + Thread.Sleep(delay); + } + + private static TimeSpan GetRetryDelay(int failedAttempt) + { + return TimeSpan.FromMilliseconds(InitialDelayMilliseconds * (1 << (failedAttempt - 1))); + } + + // A repository URL with embedded credentials can become part of a deprecated cache + // server config key name. + private static string GetTraceableConfigName(string configName) + { + return configName != null && configName.IndexOf('@') >= 0 + ? "" + : configName; + } + + private static void TraceRetry(ITracer tracer, string configName, int failedAttempt, int maxAttempts, TimeSpan retryDelay) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("ConfigName", GetTraceableConfigName(configName)); + metadata.Add("AttemptNumber", failedAttempt); + metadata.Add("MaxAttempts", maxAttempts); + metadata.Add("RetryDelayMilliseconds", retryDelay.TotalMilliseconds); + tracer.RelatedWarning( + metadata, + "Transient failure while reading git config. Retrying.", + Keywords.Telemetry); + } + + private static void TraceExhausted(ITracer tracer, string configName, int attempts) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("ConfigName", GetTraceableConfigName(configName)); + metadata.Add("Attempts", attempts); + tracer.RelatedWarning( + metadata, + "Transient failure while reading git config persisted after all attempts.", + Keywords.Telemetry); + } + } +} diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index df54900b7c..615f2cbd0a 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -56,6 +56,8 @@ public class GitProcess : ICredentialStore private string gitBinPath; private string workingDirectoryRoot; private string dotGitRoot; + private readonly ITracer tracer; + private readonly Action configReadDelay; private Process executingProcess; private bool stopping; @@ -85,11 +87,26 @@ static GitProcess() } public GitProcess(Enlistment enlistment) - : this(enlistment.GitBinPath, enlistment.WorkingDirectoryBackingRoot) + : this(enlistment.GitBinPath, enlistment.WorkingDirectoryBackingRoot, NullTracer.Instance, GitConfigReadRetry.Delay) + { + } + + public GitProcess(Enlistment enlistment, ITracer tracer) + : this(enlistment.GitBinPath, enlistment.WorkingDirectoryBackingRoot, tracer, GitConfigReadRetry.Delay) + { + } + + internal GitProcess(Enlistment enlistment, ITracer tracer, Action configReadDelay) + : this(enlistment.GitBinPath, enlistment.WorkingDirectoryBackingRoot, tracer, configReadDelay) { } public GitProcess(string gitBinPath, string workingDirectoryRoot) + : this(gitBinPath, workingDirectoryRoot, NullTracer.Instance, GitConfigReadRetry.Delay) + { + } + + internal GitProcess(string gitBinPath, string workingDirectoryRoot, ITracer tracer, Action configReadDelay) { if (string.IsNullOrWhiteSpace(gitBinPath)) { @@ -98,6 +115,8 @@ public GitProcess(string gitBinPath, string workingDirectoryRoot) this.gitBinPath = gitBinPath; this.workingDirectoryRoot = workingDirectoryRoot; + this.tracer = tracer ?? NullTracer.Instance; + this.configReadDelay = configReadDelay ?? GitConfigReadRetry.Delay; if (this.workingDirectoryRoot != null) { @@ -127,22 +146,28 @@ public static Result Init(Enlistment enlistment) public static ConfigResult GetFromGlobalConfig(string gitBinPath, string settingName) { - return new ConfigResult( - new GitProcess(gitBinPath, workingDirectoryRoot: null).InvokeGitOutsideEnlistment("config --global " + settingName), + GitProcess git = new GitProcess(gitBinPath, workingDirectoryRoot: null); + return new ConfigResult(git.InvokeConfigRead( + settingName, + () => git.InvokeGitOutsideEnlistment("config --global " + settingName)), settingName); } public static ConfigResult GetFromSystemConfig(string gitBinPath, string settingName) { - return new ConfigResult( - new GitProcess(gitBinPath, workingDirectoryRoot: null).InvokeGitOutsideEnlistment("config --system " + settingName), + GitProcess git = new GitProcess(gitBinPath, workingDirectoryRoot: null); + return new ConfigResult(git.InvokeConfigRead( + settingName, + () => git.InvokeGitOutsideEnlistment("config --system " + settingName)), settingName); } public static ConfigResult GetFromFileConfig(string gitBinPath, string configFile, string settingName) { - return new ConfigResult( - new GitProcess(gitBinPath, workingDirectoryRoot: null).InvokeGitOutsideEnlistment("config --file " + configFile + " " + settingName), + GitProcess git = new GitProcess(gitBinPath, workingDirectoryRoot: null); + return new ConfigResult(git.InvokeConfigRead( + settingName, + () => git.InvokeGitOutsideEnlistment("config --file " + configFile + " " + settingName)), settingName); } @@ -469,7 +494,9 @@ public Result SetInFileConfig(string configFile, string settingName, string valu public bool TryGetConfigUrlMatch(string section, string repositoryUrl, out Dictionary configSettings) { // See GetFromConfig for why pre-command hook is disabled. - Result result = this.InvokeGitAgainstDotGitFolder($"config --get-urlmatch {section} {repositoryUrl}", usePreCommandHook: false); + Result result = this.InvokeConfigRead( + section, + () => this.InvokeGitAgainstDotGitFolder($"config --get-urlmatch {section} {repositoryUrl}", usePreCommandHook: false)); if (result.ExitCodeIsFailure) { configSettings = null; @@ -485,7 +512,11 @@ public bool TryGetAllConfig(bool localOnly, out Dictionary this.InvokeGitAgainstDotGitFolder("config --list " + localParameter, usePreCommandHook: false)), + "--list"); if (result.TryParseAsString(out string output, out string _, string.Empty)) { @@ -515,16 +546,22 @@ public virtual ConfigResult GetFromConfig(string settingName, bool forceOutsideE // mutation happens), and skipping the hook makes us robust to a broken // hook config in the enlistment - which is exactly what we'd be trying // to repair via TryUpdateHooks at mount time. - return + Result result = fileSystem.DirectoryExists(this.workingDirectoryRoot) && !forceOutsideEnlistment - ? new ConfigResult(this.InvokeGitAgainstDotGitFolder(command, usePreCommandHook: false), settingName) - : new ConfigResult(this.InvokeGitOutsideEnlistment(command), settingName); + ? this.InvokeConfigRead(settingName, () => this.InvokeGitAgainstDotGitFolder(command, usePreCommandHook: false)) + : this.InvokeConfigRead(settingName, () => this.InvokeGitOutsideEnlistment(command)); + + return new ConfigResult(result, settingName); } public ConfigResult GetFromLocalConfig(string settingName) { // See GetFromConfig above for why pre-command hook is disabled here. - return new ConfigResult(this.InvokeGitAgainstDotGitFolder("config --local " + settingName, usePreCommandHook: false), settingName); + return new ConfigResult( + this.InvokeConfigRead( + settingName, + () => this.InvokeGitAgainstDotGitFolder("config --local " + settingName, usePreCommandHook: false)), + settingName); } /// @@ -556,7 +593,12 @@ public ConfigResult GetOriginUrl() { /* Disable precommand hook because this config call is used during mounting process * which needs to be able to fix a bad precommand hook configuration. */ - return new ConfigResult(this.InvokeGitAgainstDotGitFolder("config --local remote.origin.url", usePreCommandHook: false), "remote.origin.url"); + const string ConfigName = "remote.origin.url"; + return new ConfigResult( + this.InvokeConfigRead( + ConfigName, + () => this.InvokeGitAgainstDotGitFolder("config --local " + ConfigName, usePreCommandHook: false)), + ConfigName); } public Result DiffTree(string sourceTreeish, string targetTreeish, Action onResult) @@ -1138,6 +1180,23 @@ private static bool CurrentProcessHasVisibleConsoleWindowSafe() } } + private Result InvokeConfigRead(string configName, Func readConfig) + { + return GitConfigReadRetry.InvokeResult( + configName, + readConfig, + IsTransientConfigReadFailure, + this.tracer, + this.configReadDelay); + } + + private static bool IsTransientConfigReadFailure(Result result) + { + return + result.ExitCodeIsFailure && + GitConfigReadRetry.IsTransientFileAccessError(result.Errors, lineMustContain: "unable to access"); + } + private static string ParseValue(string contents, string prefix) { int startIndex = contents.IndexOf(prefix) + prefix.Length; @@ -1392,32 +1451,13 @@ public bool TryParseAsString(out string value, out string error, string defaultV public bool TryParseAsInt(int defaultValue, int minValue, out int value, out string error) { value = defaultValue; - error = string.Empty; if (!this.TryParseAsString(out string valueString, out error)) { return false; } - if (string.IsNullOrWhiteSpace(valueString)) - { - // Use default value - return true; - } - - if (!int.TryParse(valueString, out value)) - { - error = string.Format("Misconfigured config setting {0}, could not parse value `{1}` as an int", this.configName, valueString); - return false; - } - - if (value < minValue) - { - error = string.Format("Invalid value {0} for setting {1}, value must be greater than or equal to {2}", value, this.configName, minValue); - return false; - } - - return true; + return GitConfigHelper.TryParseIntSetting(this.configName, valueString, defaultValue, minValue, out value, out error); } } } diff --git a/GVFS/GVFS.Common/Git/LibGit2Repo.cs b/GVFS/GVFS.Common/Git/LibGit2Repo.cs index 6667dd2c82..f4d1bda8cc 100644 --- a/GVFS/GVFS.Common/Git/LibGit2Repo.cs +++ b/GVFS/GVFS.Common/Git/LibGit2Repo.cs @@ -1,5 +1,6 @@ using GVFS.Common.Tracing; using System; +using System.Collections; using System.Collections.Generic; using System.IO; using System.Runtime.InteropServices; @@ -27,21 +28,65 @@ namespace GVFS.Common.Git /// public class LibGit2Repo : IDisposable { + // git.exe reads config from these variables, or uses them to select config files. + // libgit2 does not. GIT_DIR selects the repository when no repository path is given. + private static readonly string[] ConfigEnvironmentVariablesLibGit2Ignores = + { + "GIT_CONFIG", + "GIT_CONFIG_COUNT", + "GIT_CONFIG_GLOBAL", + "GIT_CONFIG_NOSYSTEM", + "GIT_CONFIG_PARAMETERS", + "GIT_CONFIG_SYSTEM", + "GIT_DIR", + }; + + // git.exe also evaluates hasconfig:remote.*.url conditions. + private static readonly string[] IncludeConditionsLibGit2Evaluates = + { + "gitdir:", + "gitdir/i:", + "onbranch:", + }; + private bool disposedValue = false; private IntPtr odbHandle = IntPtr.Zero; + private readonly Action configReadDelay; + 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); @@ -49,6 +94,10 @@ public LibGit2Repo(ITracer tracer, string repoPath) || !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); } } @@ -62,8 +111,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 +330,22 @@ 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) + { + 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) @@ -282,47 +354,116 @@ public virtual string GetConfigString(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) { - throw new LibGit2Exception($"Failed to snapshot config for '{name}': {Native.GetLastError()}"); + return null; + } + else if (resultCode != Native.ResultCode.Success) + { + 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) - { - return null; - } - else if (resultCode != Native.ResultCode.Success) + string entryName = Marshal.PtrToStructure(entryPtr).GetName(); + string condition = entryName.Substring(Prefix.Length, entryName.Length - Prefix.Length - Suffix.Length); + foreach (string evaluated in IncludeConditionsLibGit2Evaluates) { - 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) + { + 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 +516,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); } @@ -399,15 +541,147 @@ 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( + 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 +689,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 +706,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 +731,8 @@ private static void ForEachMultiVarConfig(IntPtr configHandle, string key, Multi { throw new LibGit2Exception($"Failed to get multivar config for '{key}': {Native.GetLastError()}"); } + + return values; } /// @@ -701,6 +979,13 @@ public static extern ResultCode GetMultivarForeach( GitConfigMultivarCallback callback, IntPtr payload); + [DllImport(Git2NativeLibName, EntryPoint = "git_config_foreach_match")] + public static extern ResultCode ForeachMatch( + IntPtr configHandle, + [MarshalAs(UnmanagedType.LPUTF8Str)] string regex, + GitConfigMultivarCallback callback, + IntPtr payload); + [UnmanagedFunctionPointer(CallingConvention.Cdecl)] public delegate ResultCode GitConfigMultivarCallback( IntPtr entryPtr, diff --git a/GVFS/GVFS.Common/GitStatusCacheConfig.cs b/GVFS/GVFS.Common/GitStatusCacheConfig.cs index 82598fb4ac..415f1e40a1 100644 --- a/GVFS/GVFS.Common/GitStatusCacheConfig.cs +++ b/GVFS/GVFS.Common/GitStatusCacheConfig.cs @@ -25,7 +25,7 @@ public GitStatusCacheConfig(TimeSpan backOffTime) public static bool TryLoadFromGitConfig(ITracer tracer, Enlistment enlistment, out GitStatusCacheConfig gitStatusCacheConfig, out string error) { - return TryLoadFromGitConfig(tracer, new GitProcess(enlistment), out gitStatusCacheConfig, out error); + return TryLoadFromGitConfig(tracer, enlistment.CreateGitProcess(tracer), out gitStatusCacheConfig, out error); } public static bool TryLoadFromGitConfig(ITracer tracer, GitProcess git, out GitStatusCacheConfig gitStatusCacheConfig, out string error) diff --git a/GVFS/GVFS.Common/Http/CacheServerResolver.cs b/GVFS/GVFS.Common/Http/CacheServerResolver.cs index 7a0c6e1a73..34a9c17215 100644 --- a/GVFS/GVFS.Common/Http/CacheServerResolver.cs +++ b/GVFS/GVFS.Common/Http/CacheServerResolver.cs @@ -18,10 +18,10 @@ public CacheServerResolver( this.enlistment = enlistment; } - public static CacheServerInfo GetCacheServerFromConfig(Enlistment enlistment) + public static CacheServerInfo GetCacheServerFromConfig(Enlistment enlistment, ITracer tracer = null) { - GitProcess git = enlistment.CreateGitProcess(); - string url = GetUrlFromConfig(enlistment); + GitProcess git = CreateGitProcess(enlistment, tracer); + string url = GetUrlFromConfig(enlistment, tracer); string prefetchCacheServerUrl = GetEndpointUrlFromConfig(git, GVFSConstants.GitConfig.PrefetchCacheServer); string getCacheServerUrl = GetEndpointUrlFromConfig(git, GVFSConstants.GitConfig.GetCacheServer); string postCacheServerUrl = GetEndpointUrlFromConfig(git, GVFSConstants.GitConfig.PostCacheServer); @@ -36,9 +36,9 @@ public static CacheServerInfo GetCacheServerFromConfig(Enlistment enlistment) sizesCacheServerUrl); } - public static string GetUrlFromConfig(Enlistment enlistment) + public static string GetUrlFromConfig(Enlistment enlistment, ITracer tracer = null) { - GitProcess git = enlistment.CreateGitProcess(); + GitProcess git = CreateGitProcess(enlistment, tracer); // TODO 1057500: Remove support for encoded-repo-url cache config setting return @@ -170,6 +170,13 @@ private static string GetValueFromConfig(GitProcess git, string configName, bool return value; } + private static GitProcess CreateGitProcess(Enlistment enlistment, ITracer tracer) + { + return tracer == null + ? enlistment.CreateGitProcess() + : enlistment.CreateGitProcess(tracer); + } + private static string GetEndpointUrlFromConfig(GitProcess git, string configName) { string url = GetValueFromConfig(git, configName, localOnly: true); diff --git a/GVFS/GVFS.Common/RetryConfig.cs b/GVFS/GVFS.Common/RetryConfig.cs index 4462a18bcc..628dd7d017 100644 --- a/GVFS/GVFS.Common/RetryConfig.cs +++ b/GVFS/GVFS.Common/RetryConfig.cs @@ -1,6 +1,8 @@ using GVFS.Common.Git; using GVFS.Common.Tracing; using System; +using System.Collections; +using System.IO; using System.Linq; namespace GVFS.Common @@ -17,6 +19,8 @@ public class RetryConfig private static readonly TimeSpan DefaultTimeout = TimeSpan.FromSeconds(DefaultTimeoutSeconds); + internal delegate bool ConfigStringReader(string name, out string value, out string error); + public RetryConfig(int maxRetries = DefaultMaxRetries) : this(maxRetries, DefaultTimeout) { @@ -38,15 +42,88 @@ public int MaxAttempts public static bool TryLoadFromGitConfig(ITracer tracer, Enlistment enlistment, out RetryConfig retryConfig, out string error) { - return TryLoadFromGitConfig(tracer, new GitProcess(enlistment), out retryConfig, out error); + return TryLoadFromGitConfig( + tracer, + CreateConfigReader(tracer, enlistment, Environment.GetEnvironmentVariables()), + out retryConfig, + out error); + } + + internal static bool TryLoadFromGitConfig(ITracer tracer, GitProcess git, out RetryConfig retryConfig, out string error) + { + return TryLoadFromGitConfig( + tracer, + (string name, out string value, out string readError) => git.GetFromConfig(name).TryParseAsString(out value, out readError), + out retryConfig, + out error); + } + + /// + /// Reads the settings in process with libgit2, and with git.exe when libgit2 cannot give + /// the value that git.exe would give. + /// + internal static ConfigStringReader CreateConfigReader(ITracer tracer, Enlistment enlistment, IDictionary environment) + { + GitProcess git = null; + ConfigStringReader readWithGit = (string name, out string value, out string error) => + { + git = git ?? enlistment.CreateGitProcess(tracer); + return git.GetFromConfig(name).TryParseAsString(out value, out error); + }; + + return LibGit2Repo.EnvironmentChangesGitConfig(environment) + ? readWithGit + : WithGitFallback(tracer, CreateLibGit2Reader(tracer, enlistment.WorkingDirectoryBackingRoot), readWithGit); + } + + internal static ConfigStringReader CreateLibGit2Reader(ITracer tracer, string workingDirectoryRoot) + { + // GitProcess.GetFromConfig also reads only the global and system config when the + // working directory does not exist, for example before clone creates it. + string repoPath = Directory.Exists(workingDirectoryRoot) ? workingDirectoryRoot : null; + return (string name, out string value, out string error) => + LibGit2Repo.TryGetConfigString(tracer, repoPath, name, out value, out error); + } + + /// + /// Reads with git.exe when libgit2 fails, for example because it rejects the repository's + /// ownership or skips an includeIf condition. A transient file access failure is not read + /// again, because libgit2 already used the GitConfigReadRetry budget for it. + /// + internal static ConfigStringReader WithGitFallback(ITracer tracer, ConfigStringReader readWithLibGit2, ConfigStringReader readWithGit) + { + return (string name, out string value, out string error) => + { + if (readWithLibGit2(name, out value, out error)) + { + return true; + } + + if (GitConfigReadRetry.IsTransientFileAccessError(error)) + { + return false; + } + + tracer?.RelatedEvent( + EventLevel.Informational, + "RetryConfig_ReadWithGit", + new EventMetadata + { + { "Area", EtwArea }, + { "ConfigName", name }, + { "LibGit2Error", error }, + }); + + return readWithGit(name, out value, out error); + }; } - public static bool TryLoadFromGitConfig(ITracer tracer, GitProcess git, out RetryConfig retryConfig, out string error) + internal static bool TryLoadFromGitConfig(ITracer tracer, ConfigStringReader readConfig, out RetryConfig retryConfig, out string error) { retryConfig = null; int maxRetries; - if (!TryLoadMaxRetries(git, out maxRetries, out error)) + if (!TryLoadMaxRetries(readConfig, out maxRetries, out error)) { if (tracer != null) { @@ -63,7 +140,7 @@ public static bool TryLoadFromGitConfig(ITracer tracer, GitProcess git, out Retr } TimeSpan timeout; - if (!TryLoadTimeout(git, out timeout, out error)) + if (!TryLoadTimeout(readConfig, out timeout, out error)) { if (tracer != null) { @@ -99,10 +176,10 @@ public static bool TryLoadFromGitConfig(ITracer tracer, GitProcess git, out Retr return true; } - private static bool TryLoadMaxRetries(GitProcess git, out int attempts, out string error) + private static bool TryLoadMaxRetries(ConfigStringReader readConfig, out int attempts, out string error) { return TryGetFromGitConfig( - git, + readConfig, GVFSConstants.GitConfig.MaxRetriesConfig, DefaultMaxRetries, MinRetries, @@ -110,12 +187,12 @@ private static bool TryLoadMaxRetries(GitProcess git, out int attempts, out stri out error); } - private static bool TryLoadTimeout(GitProcess git, out TimeSpan timeout, out string error) + private static bool TryLoadTimeout(ConfigStringReader readConfig, out TimeSpan timeout, out string error) { timeout = TimeSpan.FromSeconds(0); int timeoutSeconds; if (!TryGetFromGitConfig( - git, + readConfig, GVFSConstants.GitConfig.TimeoutSecondsConfig, DefaultTimeoutSeconds, 0, @@ -129,10 +206,15 @@ private static bool TryLoadTimeout(GitProcess git, out TimeSpan timeout, out str return true; } - private static bool TryGetFromGitConfig(GitProcess git, string configName, int defaultValue, int minValue, out int value, out string error) + private static bool TryGetFromGitConfig(ConfigStringReader readConfig, string configName, int defaultValue, int minValue, out int value, out string error) { - GitProcess.ConfigResult result = git.GetFromConfig(configName); - return result.TryParseAsInt(defaultValue, minValue, out value, out error); + value = defaultValue; + if (!readConfig(configName, out string valueString, out error)) + { + return false; + } + + return GitConfigHelper.TryParseIntSetting(configName, valueString, defaultValue, minValue, out value, out error); } } } diff --git a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs index cb7af7571d..e84242e6f3 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs @@ -4,8 +4,10 @@ using GVFS.Tests.Should; using NUnit.Framework; using System; +using System.Collections.Generic; using System.Diagnostics; using System.IO; +using System.Text; using System.Threading; namespace GVFS.FunctionalTests.Tests.EnlistmentPerFixture @@ -131,5 +133,335 @@ public void OrphanedGVFSLockIsCleanedUp() // an error about "Waiting for GVFS.FunctionalTests.LockHolder" statusWait.Errors.ShouldEqual(string.Empty); } + + [TestCase("stash", "list")] + [TestCase("stash.exe", "list")] + [TestCase("for-each-repo", "--config=gvfs.tests.alias-repositories status --porcelain")] + [TestCase("checkout", "--detach HEAD")] + [Order(9)] + public void BuiltInWithSameNameAliasStillAcquiresLock(string command, string arguments) + { + string aliasName = command.EndsWith(".exe", StringComparison.Ordinal) ? Path.GetFileNameWithoutExtension(command) : command; + string repositoryConfig = command == "for-each-repo" + ? "-c \"gvfs.tests.alias-repositories=" + this.Enlistment.RepoRoot + "\" " + : string.Empty; + ProcessResult result = this.RunWithLockHolder(repositoryConfig + "-c alias." + aliasName + "=version " + command + " " + arguments); + result.ExitCode.ShouldEqual(0); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + + [TestCase("gvfs-lock-outer-alias", "status --porcelain")] + [TestCase("gvfs-lock-outer-alias", "!git status --porcelain")] + [TestCase("gvfs-lock-outer-alias", "gvfs-lock-inner-alias")] + [TestCase("MiXeD-lock-alias", "status --porcelain")] + [TestCase("git-prefixed-lock-alias", "status --porcelain")] + [Order(10)] + public void AliasInnerCommandStillAcquiresLock(string aliasName, string expansion) + { + string command = "-c alias.gvfs-lock-inner-alias=status " + + "-c \"alias." + aliasName + "=" + expansion + "\" " + aliasName; + ProcessResult result = this.RunWithLockHolder(command); + result.ExitCode.ShouldEqual(0); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + + [TestCase] + [Order(11)] + public void ReadOnlyAliasDoesNotAcquireAnOuterLock() + { + ProcessResult result = this.RunWithLockHolder( + "-c \"alias.gvfs-read-only-alias=!git rev-parse --show-toplevel\" gvfs-read-only-alias", + shouldWait: false); + result.ExitCode.ShouldEqual(0); + result.Output.ShouldContain(this.Enlistment.RepoRoot.Replace('\\', '/')); + result.Errors.Contains(ExpectedStatusWaitingText).ShouldBeFalse(); + } + + [TestCase] + [Order(12)] + public void UnknownExternalCommandStillAcquiresLock() + { + ProcessResult result = this.RunWithLockHolder("-c help.autocorrect=never gvfs-unknown-lock-command"); + result.ExitCode.ShouldEqual(1); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + + [TestCase("git-", false, false)] + [TestCase("GIT-", true, false)] + [TestCase("git-", false, true)] + [Order(13)] + public void ExternalWithSameNameAliasStillAcquiresLock(string prefix, bool uppercaseCommand, bool useExecPath) + { + const string CommandName = "gvfs-lock-external"; + string directory = Path.Combine(this.Enlistment.EnlistmentRoot, nameof(this.ExternalWithSameNameAliasStillAcquiresLock)); + Directory.CreateDirectory(directory); + string scriptPath = Path.Combine(directory, prefix + CommandName); + File.WriteAllText(scriptPath, "#!/bin/sh\nexec git status --porcelain\n"); + try + { + Dictionary environment = new Dictionary(); + if (useExecPath) + { + environment["GIT_EXEC_PATH"] = directory; + } + else + { + environment["PATH"] = directory + Path.PathSeparator + Environment.GetEnvironmentVariable("PATH"); + } + + string invocation = uppercaseCommand ? CommandName.ToUpperInvariant() : CommandName; + ProcessResult result = this.RunWithLockHolder("-c alias." + CommandName + "=version " + invocation, environment); + result.ExitCode.ShouldEqual(0); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + finally + { + File.Delete(scriptPath); + Directory.Delete(directory); + } + } + + [TestCase] + [Order(14)] + public void ExternalExecutableWithSameNameAliasStillAcquiresLock() + { + const string CommandName = "gvfs-lock-executable"; + string directory = Path.Combine(this.Enlistment.EnlistmentRoot, nameof(this.ExternalExecutableWithSameNameAliasStillAcquiresLock)); + Directory.CreateDirectory(directory); + string executablePath = Path.Combine(directory, "GIT-" + CommandName + ".EXE"); + File.Copy(Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.System), "cmd.exe"), executablePath); + try + { + Dictionary environment = new Dictionary(); + environment["PATH"] = directory + Path.PathSeparator + Environment.GetEnvironmentVariable("PATH"); + ProcessResult result = this.RunWithLockHolder("-c alias." + CommandName + "=version " + CommandName + " /d /c echo external-command", environment); + result.ExitCode.ShouldEqual(0); + result.Output.ShouldContain("external-command"); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + finally + { + File.Delete(executablePath); + Directory.Delete(directory); + } + } + + [TestCase("")] + [Order(15)] + public void DirectoryNamedAfterAnExternalDoesNotHideAnAlias(string suffix) + { + const string CommandName = "gvfs-directory-alias"; + string directory = Path.Combine(this.Enlistment.EnlistmentRoot, nameof(this.DirectoryNamedAfterAnExternalDoesNotHideAnAlias)); + string commandDirectory = Path.Combine(directory, "git-" + CommandName + suffix); + Directory.CreateDirectory(commandDirectory); + try + { + Dictionary environment = new Dictionary(); + environment["PATH"] = directory + Path.PathSeparator + Environment.GetEnvironmentVariable("PATH"); + ProcessResult result = this.RunWithLockHolder( + "-c alias." + CommandName + "=version " + CommandName, + environment, + shouldWait: false); + result.ExitCode.ShouldEqual(0); + result.Output.ShouldContain("git version"); + } + finally + { + Directory.Delete(commandDirectory); + Directory.Delete(directory); + } + } + + [TestCase] + [Order(16)] + public void PostCommandReleasesOwnedLockAfterExternalDisappears() + { + const string CommandName = "gvfs-lock-race"; + string directory = Path.Combine(this.Enlistment.EnlistmentRoot, nameof(this.PostCommandReleasesOwnedLockAfterExternalDisappears)); + Directory.CreateDirectory(directory); + string executablePath = Path.Combine(directory, "git-" + CommandName); + File.WriteAllText(executablePath, "#!/bin/sh\nexit 99\n"); + string hooksExecutable = Path.Combine(Path.GetDirectoryName(Properties.Settings.Default.PathToGVFS), "GVFS.Hooks.exe"); + Dictionary environment = new Dictionary(); + environment["PATH"] = directory + Path.PathSeparator + Environment.GetEnvironmentVariable("PATH"); + environment["GIT_CONFIG_COUNT"] = "1"; + environment["GIT_CONFIG_KEY_0"] = "alias." + CommandName; + environment["GIT_CONFIG_VALUE_0"] = "checkout --detach HEAD"; + + // Keep the Git PID alive so orphan cleanup cannot hide a skipped release. + using (Process git = new Process()) + { + git.StartInfo = this.CreateGitStartInfo("hash-object --stdin", environment); + git.StartInfo.RedirectStandardInput = true; + bool started = false; + try + { + started = git.Start(); + started.ShouldBeTrue(); + string commandArguments = " git-" + CommandName + " --git-pid=" + git.Id; + ProcessStartInfo hook = this.CreateGitStartInfo("pre-command" + commandArguments, environment); + hook.FileName = hooksExecutable; + ProcessResult preCommand = ProcessHelper.Run(hook, timeoutMs: 30000); + preCommand.ExitCode.ShouldEqual(0, preCommand.Output + preCommand.Errors); + this.Enlistment.Status().ShouldContain("GVFS Lock: Held by git git-" + CommandName + " (PID:" + git.Id + ")"); + + File.Delete(executablePath); + hook.Arguments = "post-command" + commandArguments; + ProcessResult postCommand = ProcessHelper.Run(hook, timeoutMs: 30000); + postCommand.ExitCode.ShouldEqual(0, postCommand.Output + postCommand.Errors); + git.HasExited.ShouldBeFalse(); + this.Enlistment.Status().ShouldContain("GVFS Lock: Free"); + } + finally + { + if (started) + { + git.StandardInput.Close(); + if (!git.WaitForExit(10000)) + { + git.Kill(entireProcessTree: true); + git.WaitForExit(); + Assert.Fail("The parent Git process did not exit."); + } + } + + File.Delete(executablePath); + Directory.Delete(directory); + } + } + } + + [TestCase("\"C:\\invalid quoted path\"")] + [TestCase("C:\\invalid|path")] + [Order(17)] + public void InvalidPathEntryDoesNotPreventAGenuineAlias(string pathEntry) + { + Dictionary environment = new Dictionary(); + environment["PATH"] = pathEntry + Path.PathSeparator + Environment.GetEnvironmentVariable("PATH"); + ProcessResult result = this.RunWithLockHolder( + "-c alias.gvfs-invalid-path-alias=version gvfs-invalid-path-alias", + environment, + shouldWait: false); + result.ExitCode.ShouldEqual(0); + result.Output.ShouldContain("git version"); + } + + private ProcessStartInfo CreateGitStartInfo(string command, Dictionary environment = null) + { + ProcessStartInfo startInfo = new ProcessStartInfo(Properties.Settings.Default.PathToGit, command); + startInfo.WorkingDirectory = this.Enlistment.RepoRoot; + startInfo.UseShellExecute = false; + startInfo.RedirectStandardOutput = true; + startInfo.RedirectStandardError = true; + startInfo.Environment["GIT_TERMINAL_PROMPT"] = "0"; + if (environment != null) + { + foreach (KeyValuePair entry in environment) + { + startInfo.Environment[entry.Key] = entry.Value; + } + } + + return startInfo; + } + + private ProcessResult RunWithLockHolder(string command, Dictionary environment = null, bool shouldWait = true) + { + using (Process holder = new Process()) + using (Process git = new Process()) + using (ManualResetEventSlim waiting = new ManualResetEventSlim(false)) + { + holder.StartInfo = new ProcessStartInfo(GitHelpers.LockHolderCommandPath) + { + WorkingDirectory = this.Enlistment.RepoRoot, + UseShellExecute = false, + RedirectStandardInput = true, + RedirectStandardOutput = true, + RedirectStandardError = true, + }; + holder.Start().ShouldBeTrue(); + bool gitStarted = false; + StringBuilder output = new StringBuilder(); + StringBuilder errors = new StringBuilder(); + Thread errorReader = null; + try + { + this.Enlistment.WaitForLock("GVFS.FunctionalTests.LockHolder (PID:" + holder.Id + ")") + .ShouldBeTrue("The expected lock-holder process did not acquire the lock."); + git.StartInfo = this.CreateGitStartInfo(command, environment); + git.OutputDataReceived += (sender, args) => + { + if (args.Data != null) + { + output.AppendLine(args.Data); + } + }; + errorReader = new Thread(() => + { + int character; + while ((character = git.StandardError.Read()) != -1) + { + errors.Append((char)character); + if (!waiting.IsSet && errors.ToString().Contains(ExpectedStatusWaitingText)) + { + waiting.Set(); + } + } + }); + gitStarted = git.Start(); + gitStarted.ShouldBeTrue(); + git.BeginOutputReadLine(); + errorReader.Start(); + + if (shouldWait) + { + waiting.Wait(AcquireGVFSLockTimeout).ShouldBeTrue("Git did not wait for the lock holder."); + this.ReleaseLockHolder(holder); + } + + git.WaitForExit(30000).ShouldBeTrue("Git did not finish within the test timeout."); + git.WaitForExit(); + errorReader.Join(10000).ShouldBeTrue("Git stderr did not close."); + if (!shouldWait) + { + holder.HasExited.ShouldBeFalse(); + this.Enlistment.Status().ShouldContain("GVFS.FunctionalTests.LockHolder (PID:" + holder.Id + ")"); + } + + return new ProcessResult(output.ToString(), errors.ToString(), git.ExitCode); + } + finally + { + this.ReleaseLockHolder(holder); + if (gitStarted && !git.WaitForExit(10000)) + { + git.Kill(entireProcessTree: true); + git.WaitForExit(); + } + + if (errorReader != null && (errorReader.ThreadState & System.Threading.ThreadState.Unstarted) == 0) + { + errorReader.Join(10000).ShouldBeTrue("Git stderr did not close during cleanup."); + } + } + } + } + + private void ReleaseLockHolder(Process holder) + { + if (!holder.HasExited) + { + holder.StandardInput.WriteLine("done"); + holder.StandardInput.Close(); + if (!holder.WaitForExit(10000)) + { + holder.Kill(entireProcessTree: true); + holder.WaitForExit(); + Assert.Fail("The lock-holder process did not exit."); + } + } + + holder.ExitCode.ShouldEqual(0, holder.StandardError.ReadToEnd()); + } } } diff --git a/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs b/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs index 48418452d2..db19502c54 100644 --- a/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs +++ b/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs @@ -23,7 +23,7 @@ public static class GitHelpers private const string WindowsPathSeparator = "\\"; private const string GitPathSeparator = "/"; - private static string LockHolderCommandPath + public static string LockHolderCommandPath { get { diff --git a/GVFS/GVFS.Hooks/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/GitCommandLock.cs b/GVFS/GVFS.Hooks/GitCommandLock.cs new file mode 100644 index 0000000000..31669c29fd --- /dev/null +++ b/GVFS/GVFS.Hooks/GitCommandLock.cs @@ -0,0 +1,68 @@ +using GVFS.Common.NamedPipes; +using System; + +namespace GVFS.Hooks +{ + internal static class GitCommandLock + { + public static bool TryCheckOwnership( + int pid, + bool isElevated, + string fullCommand, + Func sendRequest, + out bool ownsLock, + out string error) + { + ownsLock = false; + error = null; + NamedPipeMessages.LockRequest request = new NamedPipeMessages.LockRequest( + pid, isElevated, checkAvailabilityOnly: true, fullCommand, gitCommandSessionId: string.Empty); + NamedPipeMessages.Message message = sendRequest(request.CreateMessage(NamedPipeMessages.AcquireLock.AcquireRequest)); + if (message == null) + { + error = "GVFS returned no lock-ownership response. Run 'gvfs status' to check the mount."; + return false; + } + + NamedPipeMessages.AcquireLock.Response response; + try + { + response = new NamedPipeMessages.AcquireLock.Response(message); + } + catch (Exception ex) when (ex is InvalidOperationException || ex is ArgumentException) + { + error = "GVFS returned an invalid lock-ownership response: " + ex.Message; + return false; + } + + switch (response.Result) + { + case NamedPipeMessages.AcquireLock.AvailableResult: + case NamedPipeMessages.AcquireLock.DenyGVFSResult: + return true; + + case NamedPipeMessages.AcquireLock.DenyGitResult: + if (response.ResponseData == null || response.ResponseData.PID <= 0) + { + error = "GVFS returned a lock owner without a valid process ID."; + return false; + } + + ownsLock = response.ResponseData.PID == pid; + return true; + + case NamedPipeMessages.AcquireLock.MountNotReadyResult: + error = "GVFS has not finished initializing. Run 'gvfs status' to check the mount."; + return false; + + case NamedPipeMessages.AcquireLock.UnmountInProgressResult: + error = "GVFS is unmounting."; + return false; + + default: + error = "GVFS returned an unexpected lock-ownership response: " + response.Result; + return false; + } + } + } +} diff --git a/GVFS/GVFS.Hooks/GitCommandResolver.cs b/GVFS/GVFS.Hooks/GitCommandResolver.cs new file mode 100644 index 0000000000..bb8f067e21 --- /dev/null +++ b/GVFS/GVFS.Hooks/GitCommandResolver.cs @@ -0,0 +1,175 @@ +using GVFS.Common; +using System; +using System.ComponentModel; +using System.Diagnostics; +using System.IO; + +namespace GVFS.Hooks +{ + internal static class GitCommandResolver + { + private const int InvalidNameHResult = unchecked((int)0x8007007B); + + public static bool ShouldLock( + string hookCommand, + Func getAlias, + string searchPath, + Func fileExists, + Action reportError) + { + if (!TryIsAlias(hookCommand, getAlias, searchPath, fileExists, out bool isAlias, out string error)) + { + reportError(error + " GVFS will keep the command lock."); + return true; + } + + return !isAlias; + } + + public static bool TryIsAlias( + string hookCommand, + Func getAlias, + string searchPath, + Func fileExists, + out bool isAlias, + out string error) + { + isAlias = false; + error = null; + + // Git supplies canonical, unprefixed names for built-ins, and git- for external attempts. + if (!hookCommand.StartsWith("git-", StringComparison.Ordinal) || + hookCommand.IndexOfAny(new[] { '\\', '/' }) >= 0) + { + return true; + } + + string command = hookCommand.Substring("git-".Length); + ProcessResult result; + try + { + result = getAlias("alias." + command); + } + catch (Exception ex) when (ex is Win32Exception || ex is InvalidOperationException || + ex is ArgumentException || ex is IOException || ex is UnauthorizedAccessException) + { + error = $"Unable to query Git alias '{command}': {FormatError(ex)}"; + return false; + } + + if (result.ExitCode == 1 && string.IsNullOrEmpty(result.Output)) + { + return true; + } + + if (result.ExitCode != 0) + { + error = $"Unable to read Git alias '{command}' (exit code {result.ExitCode}). {result.Errors}".TrimEnd(); + return false; + } + + if (string.IsNullOrWhiteSpace(result.Output)) + { + return true; + } + + if (string.IsNullOrEmpty(searchPath)) + { + error = $"Unable to resolve Git command '{command}': the inherited PATH is empty."; + return false; + } + + string candidate = null; + try + { + // Git adds its exec-path to PATH before the hook. Probe Windows names directly: + // --list-cmds misses executable names with an uppercase GIT- prefix. + foreach (string directory in searchPath.Split(Path.PathSeparator)) + { + if (directory.Length == 0) + { + continue; + } + + string commandPath = directory.TrimEnd('\\', '/') + "\\" + hookCommand; + if (!hookCommand.EndsWith(".exe", StringComparison.OrdinalIgnoreCase)) + { + candidate = commandPath + ".exe"; + if (fileExists(candidate)) + { + return true; + } + } + + candidate = commandPath; + if (fileExists(candidate)) + { + return true; + } + } + } + catch (Exception ex) when (ex is IOException || ex is UnauthorizedAccessException || ex is ArgumentException) + { + error = $"Unable to resolve Git command '{command}' at '{candidate}': {FormatError(ex)}"; + return false; + } + + isAlias = true; + return true; + } + + public static string GetProcessPath(int processId) + { + using (Process process = Process.GetProcessById(processId)) + { + string path = process.MainModule?.FileName; + if (string.IsNullOrEmpty(path)) + { + throw new InvalidOperationException($"Unable to read the executable path for Git PID {processId}."); + } + + return path; + } + } + + public static ProcessStartInfo CreateAliasQuery(string gitPath, string aliasKey) + { + ProcessStartInfo processInfo = new ProcessStartInfo(gitPath); + processInfo.UseShellExecute = false; + processInfo.RedirectStandardInput = true; + processInfo.RedirectStandardOutput = true; + processInfo.RedirectStandardError = true; + processInfo.ArgumentList.Add("config"); + processInfo.ArgumentList.Add("--get"); + processInfo.ArgumentList.Add(aliasKey); + processInfo.Environment["COMMAND_HOOK_LOCK"] = "true"; + return processInfo; + } + + public static bool FileExists(string path) + { + try + { + return (File.GetAttributes(path) & FileAttributes.Directory) == 0; + } + catch (FileNotFoundException) + { + return false; + } + catch (DirectoryNotFoundException) + { + return false; + } + catch (IOException ex) when (ex.HResult == InvalidNameHResult) + { + // Git also ignores invalid PATH entries, such as directories enclosed in literal quotes. + return false; + } + } + + private static string FormatError(Exception error) + { + return $"{error.GetType().Name} (0x{error.HResult:X8}): {error.Message}"; + } + } +} diff --git a/GVFS/GVFS.Hooks/KnownGitCommands.cs b/GVFS/GVFS.Hooks/KnownGitCommands.cs deleted file mode 100644 index a392eace3c..0000000000 --- a/GVFS/GVFS.Hooks/KnownGitCommands.cs +++ /dev/null @@ -1,150 +0,0 @@ -using System.Collections.Generic; - -namespace GVFS.Hooks -{ - internal static class KnownGitCommands - { - private static HashSet knownCommands = new HashSet() - { - "add", - "am", - "annotate", - "apply", - "archive", - "bisect--helper", - "blame", - "branch", - "bundle", - "cat-file", - "check-attr", - "check-ignore", - "check-mailmap", - "check-ref-format", - "checkout", - "checkout-index", - "cherry", - "cherry-pick", - "clean", - "clone", - "column", - "commit", - "commit-tree", - "config", - "count-objects", - "credential", - "describe", - "diff", - "diff-files", - "diff-index", - "diff-tree", - "fast-export", - "fetch", - "fetch-pack", - "fmt-merge-msg", - "for-each-ref", - "format-patch", - "fsck", - "fsck-objects", - "gc", - "get-tar-commit-id", - "grep", - "hash-object", - "help", - "index-pack", - "init", - "init-db", - "interpret-trailers", - "log", - "ls-files", - "ls-remote", - "ls-tree", - "mailinfo", - "mailsplit", - "merge", - "merge-base", - "merge-file", - "merge-index", - "merge-ours", - "merge-recursive", - "merge-recursive-ours", - "merge-recursive-theirs", - "merge-subtree", - "merge-tree", - "mktag", - "mktree", - "mv", - "name-rev", - "notes", - "pack-objects", - "pack-redundant", - "pack-refs", - "patch-id", - "pickaxe", - "prune", - "prune-packed", - "pull", - "push", - "read-tree", - "rebase", - "rebase--helper", - "receive-pack", - "reflog", - "remote", - "remote-ext", - "remote-fd", - "repack", - "replace", - "rerere", - "reset", - "restore", - "rev-list", - "rev-parse", - "revert", - "rm", - "send-pack", - "shortlog", - "show", - "show-branch", - "show-ref", - "stage", - "status", - "stripspace", - "switch", - "symbolic-ref", - "tag", - "unpack-file", - "unpack-objects", - "update-index", - "update-ref", - "update-server-info", - "upload-archive", - "upload-archive--writer", - "var", - "verify-commit", - "verify-pack", - "verify-tag", - "version", - "whatchanged", - "worktree", - "write-tree", - - // Externals - "bisect", - "filter-branch", - "gui", - "merge-octopus", - "merge-one-file", - "merge-resolve", - "mergetool", - "parse-remote", - "quiltimport", - "rebase", - "submodule", - }; - - public static bool Contains(string gitCommand) - { - return knownCommands.Contains(gitCommand); - } - } -} diff --git a/GVFS/GVFS.Hooks/Program.cs b/GVFS/GVFS.Hooks/Program.cs index 940b2cf37d..1254797817 100644 --- a/GVFS/GVFS.Hooks/Program.cs +++ b/GVFS/GVFS.Hooks/Program.cs @@ -22,6 +22,9 @@ public partial class Program private const int PostCommandSpinnerDelayMs = 500; + // Hooks run on the git command path, so config reads retry at most once. + private const int ConfigReadMaxAttempts = 2; + private static string enlistmentRoot; private static string enlistmentPipename; private static string normalizedCurrentDirectory; @@ -171,11 +174,13 @@ private static bool HasShortFlag(string arg, string flag) private static bool ConfigurationAllowsHydrationStatus() { + // See ConfigReadMaxAttempts for why the retry budget is small here. return LibGit2Repo.GetConfigBoolOrDefault( NullTracer.Instance, normalizedCurrentDirectory, GVFSConstants.GitConfig.ShowHydrationStatus, - GVFSConstants.GitConfig.ShowHydrationStatusDefault); + GVFSConstants.GitConfig.ShowHydrationStatusDefault, + ConfigReadMaxAttempts); } /// @@ -251,7 +256,9 @@ private static void RunLockRequest(string[] args, bool unattended, LockRequestDe { try { - if (ShouldLock(args)) + bool checkOwnership = GetHookType(args) == PostCommandHook && + args[1].StartsWith("git-", StringComparison.Ordinal); + if (checkOwnership || ShouldLock(args)) { using (NamedPipeClient pipeClient = new NamedPipeClient(enlistmentPipename)) { @@ -267,6 +274,30 @@ private static void RunLockRequest(string[] args, bool unattended, LockRequestDe ExitWithError("GVFS.Hooks: Unable to find parent git.exe process " + "(PID: " + pid + ")."); } + if (checkOwnership) + { + // Executable and alias config can change after acquisition. + if (!GitCommandLock.TryCheckOwnership( + pid, + GVFSHooksPlatform.IsElevated(), + GenerateFullCommand(args), + request => + { + pipeClient.SendRequest(request); + return pipeClient.ReadResponse(); + }, + out bool ownsLock, + out string error)) + { + ExitWithError(error); + } + + if (!ownsLock) + { + return; + } + } + requestToRun(unattended, args, pid, pipeClient); } } @@ -490,13 +521,12 @@ private static bool ShouldLock(string[] args) return false; } - if (!KnownGitCommands.Contains(gitCommand) && - IsAlias(gitCommand)) - { - return false; - } - - return true; + return GitCommandResolver.ShouldLock( + args[1], + aliasKey => ReadGitAlias(args, aliasKey), + Environment.GetEnvironmentVariable("PATH"), + GitCommandResolver.FileExists, + Console.Error.WriteLine); } private static string GetHookType(string[] args) @@ -515,11 +545,18 @@ private static string GetGitCommand(string[] args) return command; } - private static bool IsAlias(string command) + private static ProcessResult ReadGitAlias(string[] args, string aliasKey) { - ProcessResult result = ProcessHelper.Run("git", "config --get alias." + command); - - return !string.IsNullOrEmpty(result.Output); + string gitPath = GitCommandResolver.GetProcessPath(GetParentPid(args)); + return GitConfigReadRetry.InvokeResult( + aliasKey, + () => ProcessHelper.Run(GitCommandResolver.CreateAliasQuery(gitPath, aliasKey)), + (ProcessResult result) => + result.ExitCode != 0 && + GitConfigReadRetry.IsTransientFileAccessError(result.Errors, lineMustContain: "unable to access"), + NullTracer.Instance, + GitConfigReadRetry.Delay, + ConfigReadMaxAttempts); } /// diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index 0a5c930b5f..4e396efaa0 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -1900,7 +1900,7 @@ private bool TrySetRequiredGitConfigSettings() { Dictionary requiredSettings = RequiredGitConfig.GetRequiredSettings(this.enlistment); - GitProcess git = new GitProcess(this.enlistment); + GitProcess git = this.enlistment.CreateGitProcess(this.tracer); Dictionary existingConfigSettings; if (!git.TryGetAllConfig(localOnly: true, configSettings: out existingConfigSettings)) diff --git a/GVFS/GVFS.Mount/InProcessMountVerb.cs b/GVFS/GVFS.Mount/InProcessMountVerb.cs index 16328f60d6..6bb0cad92e 100644 --- a/GVFS/GVFS.Mount/InProcessMountVerb.cs +++ b/GVFS/GVFS.Mount/InProcessMountVerb.cs @@ -112,7 +112,7 @@ public void Execute() JsonTracer tracer = this.CreateTracer(enlistment, verbosity, keywords); - CacheServerInfo cacheServer = CacheServerResolver.GetCacheServerFromConfig(enlistment); + CacheServerInfo cacheServer = CacheServerResolver.GetCacheServerFromConfig(enlistment, tracer); tracer.WriteStartEvent( enlistment.WorkingDirectoryRoot, diff --git a/GVFS/GVFS.UnitTests/Common/ConfigReaderTracerTests.cs b/GVFS/GVFS.UnitTests/Common/ConfigReaderTracerTests.cs new file mode 100644 index 0000000000..f31c3c9159 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Common/ConfigReaderTracerTests.cs @@ -0,0 +1,106 @@ +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; +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 RetryConfigCreatesItsGitProcessWithTheTracerWhenTheEnvironmentChangesGitConfig() + { + MockTracer tracer = new MockTracer(); + TracerCapturingEnlistment enlistment = this.CreateEnlistment(); + + RetryConfig.TryLoadFromGitConfig( + tracer, + RetryConfig.CreateConfigReader(tracer, enlistment, new Hashtable { { "GIT_CONFIG_COUNT", "1" } }), + out RetryConfig _, + out string _); + + // One git.exe process reads both settings. + enlistment.CapturedTracers.Count.ShouldEqual(1); + enlistment.CapturedTracers[0].ShouldBeSameAs(tracer); + } + + [TestCase] + public void RetryConfigDoesNotStartGitWhenLibGit2ReadsTheSettings() + { + TracerCapturingEnlistment enlistment = this.CreateEnlistment(); + + // The mock working directory does not exist, so libgit2 reads the empty isolated global config. + using (new IsolatedLibGit2Config()) + { + RetryConfig.TryLoadFromGitConfig( + new MockTracer(), + RetryConfig.CreateConfigReader(new MockTracer(), enlistment, new Hashtable { { "PATH", "C:\\bin" } }), + out RetryConfig _, + out string _); + } + + enlistment.CapturedTracers.Count.ShouldEqual(0); + } + + [TestCase] + public void GitStatusCacheConfigCreatesItsGitProcessWithTheTracer() + { + 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/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 843d423b5f..bdd95bc793 100644 --- a/GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs +++ b/GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs @@ -3,6 +3,8 @@ using GVFS.UnitTests.Mock.Common; using NUnit.Framework; using System; +using System.Collections; +using System.Collections.Generic; using System.IO; namespace GVFS.UnitTests.Common @@ -10,6 +12,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 +73,186 @@ public void GetConfigBoolOrDefaultOnRepoReturnsDefaultOnInvalidDataExceptionAndL } } + [TestCase("GIT_CONFIG_PARAMETERS")] + [TestCase("GIT_CONFIG_COUNT")] + [TestCase("GIT_CONFIG_GLOBAL")] + [TestCase("GIT_CONFIG_SYSTEM")] + [TestCase("GIT_CONFIG_NOSYSTEM")] + [TestCase("GIT_CONFIG")] + [TestCase("GIT_DIR")] + [TestCase("git_config_parameters")] + public void EnvironmentChangesGitConfigDetectsVariablesThatLibGit2Ignores(string name) + { + Hashtable environment = new Hashtable { { "PATH", "C:\\bin" }, { name, "1" } }; + + LibGit2Repo.EnvironmentChangesGitConfig(environment).ShouldBeTrue(); + } + + [TestCase] + public void EnvironmentChangesGitConfigIgnoresOtherGitVariables() + { + Hashtable environment = new Hashtable + { + { "GIT_TRACE", "1" }, + { "GIT_CONFIG_KEY_0", "gvfs.max-retries" }, + { "GIT_WORK_TREE", "C:\\repo" }, + }; + + LibGit2Repo.EnvironmentChangesGitConfig(environment).ShouldBeFalse(); + } + + [TestCase] + public void TryGetConfigStringReportsAnUnopenableRepository() + { + string missingRepoPath = Path.Combine( + Path.GetTempPath(), + "LibGit2RepoConfigLookupTests_" + Guid.NewGuid().ToString("N")); + + LibGit2Repo.TryGetConfigString(new MockTracer(), missingRepoPath, "gvfs.test", out string value, out string error).ShouldBeFalse(); + + value.ShouldBeNull(); + error.ShouldContain("Error while reading 'gvfs.test' from config: Couldn't open repo at"); + } + + [TestCase] + public void GetConfigBoolRetriesTransientLibGit2Failure() + { + 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 +285,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 +299,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 +326,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 +357,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/Common/RetryConfigTests.cs b/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs index 0f8c45b26d..a46424d056 100644 --- a/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs +++ b/GVFS/GVFS.UnitTests/Common/RetryConfigTests.cs @@ -1,10 +1,13 @@ using GVFS.Common; using GVFS.Common.Git; +using GVFS.Common.Tracing; using GVFS.Tests.Should; using GVFS.UnitTests.Mock.Common; using GVFS.UnitTests.Mock.Git; using NUnit.Framework; using System; +using System.Collections.Generic; +using System.IO; namespace GVFS.UnitTests.Common { @@ -107,5 +110,327 @@ public void TryLoadConfigUsesConfiguredValues() config.MaxAttempts.ShouldEqual(config.MaxRetries + 1); config.Timeout.ShouldEqual(TimeSpan.FromSeconds(timeoutSeconds)); } + + [TestCase] + public void TryLoadConfigWithLibGit2UsesRepositoryValues() + { + using (TestRepository repo = new TestRepository("[gvfs]\n\tmax-retries = 7\n\ttimeout-seconds = 31\n")) + { + RetryConfig.TryLoadFromGitConfig(new MockTracer(), RetryConfig.CreateLibGit2Reader(new MockTracer(), repo.WorkingDirectory), out RetryConfig config, out string error) + .ShouldBeTrue(error); + + error.ShouldEqual(string.Empty); + config.MaxRetries.ShouldEqual(7); + config.Timeout.ShouldEqual(TimeSpan.FromSeconds(31)); + } + } + + [TestCase] + public void TryLoadConfigWithLibGit2UsesDefaultValuesWhenEntriesAreBlank() + { + using (TestRepository repo = new TestRepository("[gvfs]\n\tmax-retries =\n\ttimeout-seconds\n")) + { + RetryConfig.TryLoadFromGitConfig(new MockTracer(), RetryConfig.CreateLibGit2Reader(new MockTracer(), repo.WorkingDirectory), out RetryConfig config, out string error) + .ShouldBeTrue(error); + + config.MaxRetries.ShouldEqual(RetryConfig.DefaultMaxRetries); + config.Timeout.ShouldEqual(TimeSpan.FromSeconds(RetryConfig.DefaultTimeoutSeconds)); + } + } + + [TestCase] + public void TryLoadConfigWithLibGit2ReportsTheSameParseErrorAsGit() + { + using (TestRepository repo = new TestRepository("[gvfs]\n\tmax-retries = abc\n")) + { + RetryConfig.TryLoadFromGitConfig(new MockTracer(), RetryConfig.CreateLibGit2Reader(new MockTracer(), repo.WorkingDirectory), out RetryConfig _, out string error) + .ShouldBeFalse(); + + error.ShouldEqual("Misconfigured config setting gvfs.max-retries, could not parse value `abc` as an int"); + } + } + + [TestCase] + public void TryLoadConfigWithLibGit2EnforcesMinimumValuesOnTimeout() + { + using (TestRepository repo = new TestRepository("[gvfs]\n\tmax-retries = 3\n\ttimeout-seconds = -1\n")) + { + MockTracer tracer = new MockTracer(); + RetryConfig.TryLoadFromGitConfig(tracer, RetryConfig.CreateLibGit2Reader(tracer, repo.WorkingDirectory), out RetryConfig _, out string error) + .ShouldBeFalse(); + + error.ShouldEqual("Invalid value -1 for setting gvfs.timeout-seconds, value must be greater than or equal to 0"); + tracer.RelatedErrorEvents.Count.ShouldEqual(1); + tracer.RelatedErrorEvents[0].ShouldContain("TryLoadTimeout failed"); + } + } + + [TestCase] + public void TryLoadConfigWithLibGit2FailsWhenConfigIsCorrupt() + { + using (TestRepository repo = new TestRepository("[gvfs\n")) + { + MockTracer tracer = new MockTracer(); + RetryConfig.TryLoadFromGitConfig(tracer, RetryConfig.CreateLibGit2Reader(tracer, repo.WorkingDirectory), out RetryConfig config, out string error) + .ShouldBeFalse(); + + config.ShouldBeNull(); + error.ShouldContain("Error while reading 'gvfs.max-retries' from config"); + error.ShouldContain("failed to parse config file"); + tracer.RelatedErrorEvents[0].ShouldContain("TryLoadMaxRetries failed"); + } + } + + [TestCase] + public void GitFallbackReadsWithGitWhenLibGit2Fails() + { + MockTracer tracer = new MockTracer(); + List gitReads = new List(); + RetryConfig.ConfigStringReader readConfig = RetryConfig.WithGitFallback( + tracer, + (string name, out string value, out string error) => + { + value = null; + error = "Error while reading '" + name + "' from config: Couldn't open repo"; + return false; + }, + (string name, out string value, out string error) => + { + gitReads.Add(name); + value = "4"; + error = string.Empty; + return true; + }); + + RetryConfig.TryLoadFromGitConfig(tracer, readConfig, out RetryConfig config, out string loadError).ShouldBeTrue(loadError); + + config.MaxRetries.ShouldEqual(4); + config.Timeout.ShouldEqual(TimeSpan.FromSeconds(4)); + gitReads.ShouldMatchInOrder("gvfs.max-retries", "gvfs.timeout-seconds"); + int readWithGitEvent = tracer.RelatedEventNames.IndexOf("RetryConfig_ReadWithGit"); + readWithGitEvent.ShouldEqual(0); + tracer.RelatedEventNames.FindAll(eventName => eventName == "RetryConfig_ReadWithGit").Count.ShouldEqual(2); + EventMetadata metadata = tracer.RelatedEventMetadata[readWithGitEvent]; + metadata["Area"].ShouldEqual("RetryConfig"); + metadata["ConfigName"].ShouldEqual("gvfs.max-retries"); + ((string)metadata["LibGit2Error"]).ShouldContain("Couldn't open repo"); + tracer.RelatedWarningEvents.Count.ShouldEqual(0); + } + + [TestCase] + public void GitFallbackDoesNotReadAgainAfterATransientFailure() + { + int gitReads = 0; + RetryConfig.ConfigStringReader readConfig = RetryConfig.WithGitFallback( + new MockTracer(), + (string name, out string value, out string error) => + { + value = null; + error = "Error while reading '" + name + "' from config: could not open 'C:/repo/.git/config': " + + "The process cannot access the file because it is being used by another process."; + return false; + }, + (string name, out string value, out string error) => + { + ++gitReads; + value = null; + error = null; + return true; + }); + + RetryConfig.TryLoadFromGitConfig(new MockTracer(), readConfig, out RetryConfig _, out string loadError).ShouldBeFalse(); + + gitReads.ShouldEqual(0); + loadError.ShouldContain("being used by another process"); + } + + [TestCase] + public void GitFallbackReportsTheGitErrorWhenBothReadersFail() + { + RetryConfig.ConfigStringReader readConfig = RetryConfig.WithGitFallback( + new MockTracer(), + (string name, out string value, out string error) => + { + value = null; + error = "libgit2 error"; + return false; + }, + (string name, out string value, out string error) => + { + value = null; + error = "git error"; + return false; + }); + + RetryConfig.TryLoadFromGitConfig(new MockTracer(), readConfig, out RetryConfig config, out string loadError).ShouldBeFalse(); + + config.ShouldBeNull(); + loadError.ShouldEqual("git error"); + } + + [TestCase] + public void GitFallbackDoesNotReadWithGitWhenLibGit2Succeeds() + { + MockTracer tracer = new MockTracer(); + int gitReads = 0; + RetryConfig.ConfigStringReader readConfig = RetryConfig.WithGitFallback( + tracer, + (string name, out string value, out string error) => + { + value = null; + error = null; + return true; + }, + (string name, out string value, out string error) => + { + ++gitReads; + value = null; + error = null; + return true; + }); + + RetryConfig.TryLoadFromGitConfig(tracer, readConfig, out RetryConfig config, out string _).ShouldBeTrue(); + + config.MaxRetries.ShouldEqual(RetryConfig.DefaultMaxRetries); + gitReads.ShouldEqual(0); + tracer.RelatedEventNames.Contains("RetryConfig_ReadWithGit").ShouldBeFalse(); + } + + [TestCase] + public void LibGit2ReaderReadsGlobalConfigBeforeTheRepositoryExists() + { + using (IsolatedLibGit2Config machineConfig = new IsolatedLibGit2Config()) + { + File.WriteAllText(machineConfig.GlobalConfigPath, "[gvfs]\n\tmax-retries = 9\n\ttimeout-seconds = 41\n"); + string missingWorkingDirectory = Path.Combine(Path.GetTempPath(), "RetryConfigTests_" + Guid.NewGuid().ToString("N")); + + RetryConfig.TryLoadFromGitConfig(new MockTracer(), RetryConfig.CreateLibGit2Reader(new MockTracer(), missingWorkingDirectory), out RetryConfig config, out string error) + .ShouldBeTrue(error); + + config.MaxRetries.ShouldEqual(9); + config.Timeout.ShouldEqual(TimeSpan.FromSeconds(41)); + } + } + + [TestCase] + public void LibGit2ReaderRejectsAGlobalIncludeConditionItIgnores() + { + using (IsolatedLibGit2Config machineConfig = new IsolatedLibGit2Config()) + { + File.WriteAllText( + machineConfig.GlobalConfigPath, + "[includeIf \"hasconfig:remote.*.url:https://example.com/**\"]\n\tpath = C:/does-not-matter.gitconfig\n"); + string missingWorkingDirectory = Path.Combine(Path.GetTempPath(), "RetryConfigTests_" + Guid.NewGuid().ToString("N")); + + RetryConfig.CreateLibGit2Reader(new MockTracer(), missingWorkingDirectory)("gvfs.max-retries", out string _, out string error) + .ShouldBeFalse(); + + error.ShouldContain("includeIf condition that libgit2 does not evaluate: hasconfig:"); + } + } + + [TestCase] + public void LibGit2AndGitReadTheSameRepositoryValues() + { + string gitPath = GetGitPathOrIgnore(); + using (TestRepository repo = new TestRepository("[gvfs]\n\tmax-retries = 7\n\tmax-retries = 8\n\ttimeout-seconds = 31\n")) + { + RetryConfig.TryLoadFromGitConfig(new MockTracer(), RetryConfig.CreateLibGit2Reader(new MockTracer(), repo.WorkingDirectory), out RetryConfig libGit2Config, out string libGit2Error) + .ShouldBeTrue(libGit2Error); + RetryConfig.TryLoadFromGitConfig(new MockTracer(), new GitProcess(gitPath, repo.WorkingDirectory), out RetryConfig gitConfig, out string gitError) + .ShouldBeTrue(gitError); + + libGit2Config.MaxRetries.ShouldEqual(8); + gitConfig.MaxRetries.ShouldEqual(8); + libGit2Config.Timeout.ShouldEqual(TimeSpan.FromSeconds(31)); + gitConfig.Timeout.ShouldEqual(TimeSpan.FromSeconds(31)); + } + } + + [TestCase] + public void ConfigWithAnIncludeConditionLibGit2IgnoresIsReadWithGit() + { + string gitPath = GetGitPathOrIgnore(); + using (TestRepository repo = new TestRepository(string.Empty)) + { + string includedPath = Path.Combine(repo.WorkingDirectory, "included.gitconfig"); + File.WriteAllText(includedPath, "[gvfs]\n\tmax-retries = 12\n"); + File.AppendAllText( + Path.Combine(repo.WorkingDirectory, ".git", "config"), + "[remote \"origin\"]\n\turl = https://example.com/repo\n" + + "[includeIf \"hasconfig:remote.*.url:https://example.com/**\"]\n\tpath = " + includedPath.Replace('\\', '/') + "\n"); + + MockTracer tracer = new MockTracer(); + RetryConfig.ConfigStringReader readWithLibGit2 = RetryConfig.CreateLibGit2Reader(tracer, repo.WorkingDirectory); + GitProcess git = new GitProcess(gitPath, repo.WorkingDirectory); + RetryConfig.ConfigStringReader readWithGit = (string name, out string value, out string error) => + git.GetFromConfig(name).TryParseAsString(out value, out error); + + readWithLibGit2("gvfs.max-retries", out string _, out string libGit2Error).ShouldBeFalse(); + libGit2Error.ShouldContain("includeIf condition that libgit2 does not evaluate: hasconfig:remote.*.url:https://example.com/**"); + + RetryConfig.TryLoadFromGitConfig(tracer, RetryConfig.WithGitFallback(tracer, readWithLibGit2, readWithGit), out RetryConfig config, out string loadError) + .ShouldBeTrue(loadError); + config.MaxRetries.ShouldEqual(12); + } + } + + [TestCase("gitdir:**/")] + [TestCase("gitdir/i:**/")] + [TestCase("onbranch:main")] + public void LibGit2ReadsConfigWithIncludeConditionsItEvaluates(string condition) + { + using (TestRepository repo = new TestRepository(string.Empty)) + { + string includedPath = Path.Combine(repo.WorkingDirectory, "included.gitconfig"); + File.WriteAllText(includedPath, "[gvfs]\n\tmax-retries = 13\n"); + File.AppendAllText( + Path.Combine(repo.WorkingDirectory, ".git", "config"), + "[includeIf \"" + condition + "\"]\n\tpath = " + includedPath.Replace('\\', '/') + "\n"); + + RetryConfig.CreateLibGit2Reader(new MockTracer(), repo.WorkingDirectory)("gvfs.max-retries", out string value, out string error) + .ShouldBeTrue(error); + value.ShouldEqual("13"); + } + } + + private static string GetGitPathOrIgnore() + { + string gitPath = "C:\\Program Files\\Git\\cmd\\git.exe"; + if (!File.Exists(gitPath)) + { + Assert.Ignore("Git not found at expected path — skipping integration test"); + } + + return gitPath; + } + + // A minimal repository that libgit2 can open without git.exe. libgit2 does not read the + // machine's config while the repository exists. The non-ASCII path checks the UTF-8 + // marshalling of the repository path. + private sealed class TestRepository : IDisposable + { + private readonly IsolatedLibGit2Config machineConfig = new IsolatedLibGit2Config(); + + public TestRepository(string extraConfig) + { + this.WorkingDirectory = Path.Combine(Path.GetTempPath(), "RetryConfigTests_ü_" + Guid.NewGuid().ToString("N")); + string dotGit = Path.Combine(this.WorkingDirectory, ".git"); + Directory.CreateDirectory(Path.Combine(dotGit, "objects")); + Directory.CreateDirectory(Path.Combine(dotGit, "refs", "heads")); + File.WriteAllText(Path.Combine(dotGit, "HEAD"), "ref: refs/heads/main\n"); + File.WriteAllText( + Path.Combine(dotGit, "config"), + "[core]\n\trepositoryformatversion = 0\n\tbare = false\n" + extraConfig); + } + + public string WorkingDirectory { get; } + + public void Dispose() + { + Directory.Delete(this.WorkingDirectory, recursive: true); + this.machineConfig.Dispose(); + } + } } } diff --git a/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj b/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj index 0517e82f15..63d45ef81a 100644 --- a/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj +++ b/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj @@ -27,6 +27,12 @@ + + Hooks\GitCommandLock.cs + + + Hooks\GitCommandResolver.cs + Hooks\UnstageCommandParser.cs diff --git a/GVFS/GVFS.UnitTests/Git/GitConfigReadRetryTests.cs b/GVFS/GVFS.UnitTests/Git/GitConfigReadRetryTests.cs new file mode 100644 index 0000000000..682b70bef1 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Git/GitConfigReadRetryTests.cs @@ -0,0 +1,138 @@ +using GVFS.Common.Git; +using GVFS.Tests.Should; +using GVFS.UnitTests.Mock.Common; +using NUnit.Framework; +using System; +using System.Collections.Generic; + +namespace GVFS.UnitTests.Git +{ + [TestFixture] + public class GitConfigReadRetryTests + { + [TestCase("warning: unable to access '.git/config': Permission denied", true)] + [TestCase("warning: unable to access '.git/config': Permission denied\r\nfatal: unknown error occurred while reading the configuration files", true)] + [TestCase("could not open 'C:\\repo\\.git\\config': The process cannot access the file because it is being used by another process.", true)] + [TestCase("error: unable to access 'C:/repo/.git/config': Device or resource busy", true)] + [TestCase("fatal: unable to access 'C:/Permission denied/.git/config': No such file or directory", false)] + [TestCase("error: could not open 'C:/Device or resource busy/config': The system cannot find the file specified.", false)] + [TestCase("Permission denied", false)] + [TestCase("fatal: bad config line 4 in file .git/config", false)] + [TestCase("", false)] + [TestCase(null, false)] + public void IsTransientFileAccessErrorMatchesOnlyTheFailureReason(string error, bool expected) + { + GitConfigReadRetry.IsTransientFileAccessError(error).ShouldEqual(expected); + } + + [TestCase] + public void IsTransientFileAccessErrorHonorsRequiredLinePhrase() + { + const string Error = "warning: could not open 'config': Permission denied"; + + GitConfigReadRetry.IsTransientFileAccessError(Error).ShouldBeTrue(); + GitConfigReadRetry.IsTransientFileAccessError(Error, lineMustContain: "unable to access").ShouldBeFalse(); + } + + [TestCase] + public void InvokeResultStopsAtTheRequestedAttemptLimit() + { + MockTracer tracer = new MockTracer(); + int reads = 0; + int delays = 0; + + int result = GitConfigReadRetry.InvokeResult( + "gvfs.test", + () => ++reads, + (int _) => true, + tracer, + delay => ++delays, + maxAttempts: 2); + + result.ShouldEqual(2); + reads.ShouldEqual(2); + delays.ShouldEqual(1); + } + + [TestCase] + public void InvokeResultDoesNotRetryWhenOneAttemptIsAllowed() + { + int reads = 0; + int delays = 0; + + GitConfigReadRetry.InvokeResult( + "gvfs.test", + () => ++reads, + (int _) => true, + new MockTracer(), + delay => ++delays, + maxAttempts: 1); + + reads.ShouldEqual(1); + delays.ShouldEqual(0); + } + + [TestCase] + public void InvokeResultUsesExponentialDelays() + { + List delays = new List(); + + GitConfigReadRetry.InvokeResult( + "gvfs.test", + () => 0, + (int _) => true, + new MockTracer(), + delays.Add); + + delays.ShouldMatchInOrder( + TimeSpan.FromMilliseconds(20), + TimeSpan.FromMilliseconds(40), + TimeSpan.FromMilliseconds(80), + TimeSpan.FromMilliseconds(160)); + } + + [TestCase] + public void InvokeCatchingRethrowsTheOriginalExceptionAfterTheLastAttempt() + { + int reads = 0; + InvalidOperationException lastException = null; + + Assert.Throws( + () => GitConfigReadRetry.InvokeCatching( + "gvfs.test", + () => + { + lastException = new InvalidOperationException("attempt " + ++reads); + throw lastException; + }, + (Exception _) => true, + new MockTracer(), + delay => { }, + maxAttempts: 3)); + + reads.ShouldEqual(3); + lastException.Message.ShouldEqual("attempt 3"); + } + + [TestCase] + public void TracedConfigNameDoesNotContainUrlCredentials() + { + MockTracer tracer = new MockTracer(); + + GitConfigReadRetry.InvokeResult( + "gvfs.user:token@example.com.repo.cache-server-url", + () => 0, + (int _) => true, + tracer, + delay => { }, + maxAttempts: 2); + + tracer.RelatedWarningEvents.Count.ShouldEqual(2); + foreach (string warning in tracer.RelatedWarningEvents) + { + warning.Contains("token").ShouldBeFalse(); + warning.ShouldContain("redacted"); + } + } + } +} diff --git a/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs b/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs index 8182f8dfb5..568940d757 100644 --- a/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs @@ -1,7 +1,10 @@ using GVFS.Common.Git; using GVFS.Tests.Should; using GVFS.UnitTests.Mock.Common; +using GVFS.UnitTests.Mock.Git; using NUnit.Framework; +using System; +using System.Collections.Generic; using System.Diagnostics; namespace GVFS.UnitTests.Git @@ -9,6 +12,12 @@ namespace GVFS.UnitTests.Git [TestFixture] public class GitProcessTests { + // What git.exe prints on Windows while another process holds .git\config. + private const string LockedConfigErrors = + "warning: unable to access '.git/config': Permission denied\n" + + "warning: unable to access '.git/config': Permission denied\n" + + "fatal: unknown error occurred while reading the configuration files"; + [TestCase] public void BoundedGitOutputBuffer_KeepsShortOutput() { @@ -114,6 +123,163 @@ public void ResultHasNoErrors() result.StderrContainsErrors().ShouldBeFalse(); } + [TestCase] + public void GetFromLocalConfigRetriesTransientFailureAndReturnsValue() + { + MockTracer tracer = new MockTracer(); + int delayCount = 0; + MockGitProcess process = new MockGitProcess(tracer, delay => ++delayCount); + Queue results = new Queue(); + results.Enqueue(new GitProcess.Result( + string.Empty, + LockedConfigErrors, + GitProcess.Result.GenericFailureCode)); + results.Enqueue(new GitProcess.Result("configured-value\n", string.Empty, GitProcess.Result.SuccessCode)); + process.SetExpectedCommandResult("config --local gvfs.test", () => results.Dequeue()); + + GitProcess.ConfigResult result = process.GetFromLocalConfig("gvfs.test"); + + result.TryParseAsString(out string value, out string error).ShouldBeTrue(error); + value.ShouldEqual("configured-value"); + process.CommandsRun.Count.ShouldEqual(2); + delayCount.ShouldEqual(1); + tracer.RelatedWarningEvents.Count.ShouldEqual(1); + tracer.RelatedWarningEvents[0].ShouldContain("Transient failure while reading git config"); + } + + [TestCase] + public void GetFromLocalConfigDoesNotRetryMissingKey() + { + int delayCount = 0; + MockGitProcess process = new MockGitProcess(new MockTracer(), delay => ++delayCount); + process.SetExpectedCommandResult( + "config --local gvfs.test", + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.GenericFailureCode)); + + GitProcess.ConfigResult result = process.GetFromLocalConfig("gvfs.test"); + + result.TryParseAsString(out string value, out string error).ShouldBeTrue(error); + value.ShouldBeNull(); + process.CommandsRun.Count.ShouldEqual(1); + delayCount.ShouldEqual(0); + } + + [TestCase] + public void GetFromLocalConfigStopsAfterRetryBudget() + { + MockTracer tracer = new MockTracer(); + int delayCount = 0; + MockGitProcess process = new MockGitProcess(tracer, delay => ++delayCount); + process.SetExpectedCommandResult( + "config --local gvfs.test", + () => new GitProcess.Result(string.Empty, LockedConfigErrors, GitProcess.Result.GenericFailureCode)); + + GitProcess.ConfigResult result = process.GetFromLocalConfig("gvfs.test"); + + result.TryParseAsString(out string _, out string error).ShouldBeFalse(); + error.ShouldContain("Permission denied"); + process.CommandsRun.Count.ShouldEqual(GitConfigReadRetry.MaxAttempts); + delayCount.ShouldEqual(GitConfigReadRetry.MaxAttempts - 1); + tracer.RelatedWarningEvents.Count.ShouldEqual(GitConfigReadRetry.MaxAttempts); + tracer.RelatedWarningEvents[GitConfigReadRetry.MaxAttempts - 1].ShouldContain("persisted after all attempts"); + } + + [TestCase] + public void GetFromLocalConfigDoesNotRetryWhenOnlyThePathContainsATransientPhrase() + { + int delayCount = 0; + MockGitProcess process = new MockGitProcess(new MockTracer(), delay => ++delayCount); + process.SetExpectedCommandResult( + "config --local gvfs.test", + () => new GitProcess.Result( + string.Empty, + "fatal: unable to access 'C:/Permission denied/.git/config': No such file or directory", + GitProcess.Result.GenericFailureCode)); + + process.GetFromLocalConfig("gvfs.test").TryParseAsString(out string _, out string _).ShouldBeFalse(); + + process.CommandsRun.Count.ShouldEqual(1); + delayCount.ShouldEqual(0); + } + + [TestCase] + public void GetFromConfigRetriesTransientFailure() + { + MockGitProcess process = this.CreateProcessWithTransientThenSuccess( + "config gvfs.test", + "configured-value\n", + out Func getDelayCount); + + GitProcess.ConfigResult result = process.GetFromConfig("gvfs.test", forceOutsideEnlistment: true); + + result.TryParseAsString(out string value, out string error).ShouldBeTrue(error); + value.ShouldEqual("configured-value"); + getDelayCount().ShouldEqual(1); + } + + [TestCase] + public void GetOriginUrlRetriesTransientFailure() + { + MockGitProcess process = this.CreateProcessWithTransientThenSuccess( + "config --local remote.origin.url", + "https://example.com/repo\n", + out Func getDelayCount); + + GitProcess.ConfigResult result = process.GetOriginUrl(); + + result.TryParseAsString(out string value, out string error).ShouldBeTrue(error); + value.ShouldEqual("https://example.com/repo"); + getDelayCount().ShouldEqual(1); + } + + [TestCase] + public void TryGetAllConfigRetriesTransientFailure() + { + MockGitProcess process = this.CreateProcessWithTransientThenSuccess( + "config --list --local", + "gvfs.test=configured-value\n", + out Func getDelayCount); + + process.TryGetAllConfig(localOnly: true, configSettings: out Dictionary settings).ShouldBeTrue(); + + settings.ContainsKey("gvfs.test").ShouldBeTrue(); + getDelayCount().ShouldEqual(1); + } + + [TestCase] + public void TryGetConfigUrlMatchRetriesTransientFailure() + { + MockGitProcess process = this.CreateProcessWithTransientThenSuccess( + "config --get-urlmatch http https://example.com/repo", + "http.proxy configured-value\n", + out Func getDelayCount); + + process.TryGetConfigUrlMatch("http", "https://example.com/repo", out Dictionary settings).ShouldBeTrue(); + + settings.ContainsKey("http.proxy").ShouldBeTrue(); + getDelayCount().ShouldEqual(1); + } + + [TestCase] + public void GetFromLocalConfigDoesNotRetryNonTransientFailure() + { + int delayCount = 0; + MockGitProcess process = new MockGitProcess(new MockTracer(), delay => ++delayCount); + process.SetExpectedCommandResult( + "config --local gvfs.test", + () => new GitProcess.Result( + string.Empty, + "fatal: bad config line 4 in file .git/config", + GitProcess.Result.GenericFailureCode)); + + GitProcess.ConfigResult result = process.GetFromLocalConfig("gvfs.test"); + + result.TryParseAsString(out string _, out string error).ShouldBeFalse(); + error.ShouldContain("bad config line"); + process.CommandsRun.Count.ShouldEqual(1); + delayCount.ShouldEqual(0); + } + [TestCase] public void ResultHasWarnings() { @@ -402,5 +568,17 @@ public void QuoteGitPath_RoundTripThroughProcess() } } } + + private MockGitProcess CreateProcessWithTransientThenSuccess(string command, string successOutput, out Func getDelayCount) + { + int delayCount = 0; + MockGitProcess process = new MockGitProcess(new MockTracer(), delay => ++delayCount); + Queue results = new Queue(); + results.Enqueue(new GitProcess.Result(string.Empty, LockedConfigErrors, GitProcess.Result.GenericFailureCode)); + results.Enqueue(new GitProcess.Result(successOutput, string.Empty, GitProcess.Result.SuccessCode)); + process.SetExpectedCommandResult(command, () => results.Dequeue()); + getDelayCount = () => delayCount; + return process; + } } } diff --git a/GVFS/GVFS.UnitTests/Hooks/GitCommandLockTests.cs b/GVFS/GVFS.UnitTests/Hooks/GitCommandLockTests.cs new file mode 100644 index 0000000000..c1cb79f065 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Hooks/GitCommandLockTests.cs @@ -0,0 +1,99 @@ +using GVFS.Common.NamedPipes; +using GVFS.Hooks; +using GVFS.Tests.Should; +using GVFS.UnitTests.Category; +using NUnit.Framework; + +namespace GVFS.UnitTests.Hooks +{ + [TestFixture] + public class GitCommandLockTests + { + [TestCase(123, true)] + [TestCase(456, false)] + public void OwnershipComparesTheTypedPidWithoutAcquiring(int ownerPid, bool expectedOwner) + { + int requests = 0; + GitCommandLock.TryCheckOwnership( + 123, + isElevated: false, + "git race", + request => + { + ++requests; + request.Header.ShouldEqual("AcquireLock"); + NamedPipeMessages.LockData data = new NamedPipeMessages.LockRequest(request.Body).RequestData; + data.PID.ShouldEqual(123); + data.CheckAvailabilityOnly.ShouldBeTrue(); + data.ParsedCommand.ShouldEqual("git race"); + return new NamedPipeMessages.AcquireLock.Response( + "LockDeniedGit", + new NamedPipeMessages.LockData(ownerPid, false, false, "git original", string.Empty)).CreateMessage(); + }, + out bool ownsLock, + out string error).ShouldBeTrue(); + + requests.ShouldEqual(1); + ownsLock.ShouldEqual(expectedOwner); + error.ShouldBeNull(); + } + + [TestCase("LockAvailable")] + [TestCase("LockDeniedGVFS")] + public void UnownedLocksNeedNoRelease(string result) + { + GitCommandLock.TryCheckOwnership( + 123, false, "git alias", + request => new NamedPipeMessages.Message(result, null), + out bool ownsLock, + out string error).ShouldBeTrue(); + + ownsLock.ShouldBeFalse(); + error.ShouldBeNull(); + } + + [TestCase("MountNotReady", "not finished initializing")] + [TestCase("UnmountInProgress", "unmounting")] + [TestCase("LockAcquired", "unexpected lock-ownership response")] + [TestCase("UnknownResponse", "unexpected lock-ownership response")] + [TestCase("LockDeniedGit", "without a valid process ID")] + public void InvalidOwnershipResponsesProduceADiagnostic(string result, string expectedError) + { + GitCommandLock.TryCheckOwnership( + 123, false, "git alias", + request => new NamedPipeMessages.Message(result, null), + out bool ownsLock, + out string error).ShouldBeFalse(); + + ownsLock.ShouldBeFalse(); + error.ShouldContain(expectedError); + } + + [TestCase] + public void MissingOwnershipResponseProducesADiagnostic() + { + GitCommandLock.TryCheckOwnership( + 123, false, "git alias", request => null, + out bool ownsLock, + out string error).ShouldBeFalse(); + + ownsLock.ShouldBeFalse(); + error.ShouldContain("no lock-ownership response"); + } + + [TestCase("malformed")] + [TestCase("123|false|false|-1|x|0|")] + [Category(CategoryConstants.ExceptionExpected)] + public void MalformedOwnershipResponseProducesADiagnostic(string body) + { + GitCommandLock.TryCheckOwnership( + 123, false, "git alias", + request => new NamedPipeMessages.Message("LockDeniedGit", body), + out bool ownsLock, + out string error).ShouldBeFalse(); + + ownsLock.ShouldBeFalse(); + error.ShouldContain("invalid lock-ownership response"); + } + } +} diff --git a/GVFS/GVFS.UnitTests/Hooks/GitCommandResolverTests.cs b/GVFS/GVFS.UnitTests/Hooks/GitCommandResolverTests.cs new file mode 100644 index 0000000000..1923832361 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Hooks/GitCommandResolverTests.cs @@ -0,0 +1,396 @@ +using GVFS.Common; +using GVFS.Hooks; +using GVFS.Tests.Should; +using GVFS.UnitTests.Category; +using NUnit.Framework; +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.IO; +using Win32Exception = System.ComponentModel.Win32Exception; + +namespace GVFS.UnitTests.Hooks +{ + [TestFixture] + public class GitCommandResolverTests + { + private const string SearchPath = @"mock:\git\libexec;mock:\tools"; + + [TestCase("stash")] + [TestCase("for-each-repo")] + [TestCase("checkout")] + [TestCase("update-index")] + [TestCase("status")] + public void BuiltInsDoNotQueryAliasesOrSearchPath(string command) + { + GitCommandResolver.TryIsAlias( + command, + key => throw new AssertionException("A built-in must not query aliases."), + SearchPath, + path => throw new AssertionException("A built-in must not search PATH."), + out bool isAlias, + out string error).ShouldBeTrue(); + + isAlias.ShouldBeFalse(); + error.ShouldBeNull(); + } + + [TestCase("git-tools/command")] + [TestCase(@"git-tools\command")] + public void ExternalPathsDoNotQueryAliases(string command) + { + GitCommandResolver.TryIsAlias( + command, + key => throw new AssertionException("An external path must not query aliases."), + SearchPath, + path => throw new AssertionException("An external path must not search PATH."), + out bool isAlias, + out string error).ShouldBeTrue(); + + isAlias.ShouldBeFalse(); + error.ShouldBeNull(); + } + + [TestCase("git-co", "checkout")] + [TestCase("git-shell-alias", "!git status")] + [TestCase("git-first", "second")] + [TestCase("git-second", "status")] + [TestCase("git-MiXeD", "status")] + [TestCase("git-git-prefixed", "status")] + public void GenuineAliasesSkipOnlyTheOuterLock(string hookCommand, string alias) + { + int queries = 0; + GitCommandResolver.TryIsAlias( + hookCommand, + key => + { + key.ShouldEqual("alias." + hookCommand.Substring(4)); + ++queries; + return new ProcessResult(alias + "\n", string.Empty, 0); + }, + SearchPath, + path => false, + out bool isAlias, + out string error).ShouldBeTrue(); + + queries.ShouldEqual(1); + isAlias.ShouldBeTrue(); + error.ShouldBeNull(); + } + + [TestCase("git-co", @"mock:\git\libexec\git-co.exe")] + [TestCase("git-co", @"mock:\tools\git-co")] + [TestCase("git-Co", @"mock:\tools\GIT-CO.EXE")] + [TestCase("git-co.EXE", @"mock:\tools\git-co.exe")] + [TestCase("git-git-prefixed", @"mock:\tools\git-git-prefixed")] + public void RealExternalsTakePrecedenceOverAliases(string hookCommand, string executablePath) + { + GitCommandResolver.TryIsAlias( + hookCommand, + key => + { + key.ShouldEqual("alias." + hookCommand.Substring(4)); + return new ProcessResult("!git update-index --refresh\n", string.Empty, 0); + }, + SearchPath, + path => string.Equals(path, executablePath, StringComparison.OrdinalIgnoreCase), + out bool isAlias, + out string error).ShouldBeTrue(); + + isAlias.ShouldBeFalse(); + error.ShouldBeNull(); + } + + [TestCase(1, "")] + [TestCase(0, "")] + [TestCase(0, "\n")] + public void MissingOrEmptyAliasesKeepTheLockWithoutSearchingPath(int exitCode, string output) + { + GitCommandResolver.TryIsAlias( + "git-unknown-command", + key => new ProcessResult(output, string.Empty, exitCode), + SearchPath, + path => throw new AssertionException("A command without an alias must not search PATH."), + out bool isAlias, + out string error).ShouldBeTrue(); + + isAlias.ShouldBeFalse(); + error.ShouldBeNull(); + } + + [TestCase] + public void MissingAliasWithTraceOutputStillKeepsTheLock() + { + GitCommandResolver.TryIsAlias( + "git-unknown-command", + key => new ProcessResult(string.Empty, "trace: built-in: git config --get alias.unknown-command", 1), + SearchPath, + path => throw new AssertionException("A missing alias must not search PATH."), + out bool isAlias, + out string error).ShouldBeTrue(); + + isAlias.ShouldBeFalse(); + error.ShouldBeNull(); + } + + [TestCase(128, "")] + [TestCase(128, "status\n")] + [TestCase(1, "status\n")] + public void FailedConfigReadsNeverGrantAnAliasExemption(int exitCode, string output) + { + GitCommandResolver.TryIsAlias( + "git-co", + key => new ProcessResult(output, "config read failed", exitCode), + SearchPath, + path => throw new AssertionException("A failed config read must not search PATH."), + out bool isAlias, + out string error).ShouldBeFalse(); + + isAlias.ShouldBeFalse(); + error.ShouldContain("Unable to read Git alias 'co'"); + error.ShouldContain("exit code " + exitCode); + error.ShouldContain("config read failed"); + } + + [TestCase(null)] + [TestCase("")] + public void MissingSearchPathProducesADiagnostic(string searchPath) + { + GitCommandResolver.TryIsAlias( + "git-co", + key => new ProcessResult("checkout", string.Empty, 0), + searchPath, + path => false, + out bool isAlias, + out string error).ShouldBeFalse(); + + isAlias.ShouldBeFalse(); + error.ShouldContain("inherited PATH is empty"); + } + + [TestCase(false)] + [TestCase(true)] + [Category(CategoryConstants.ExceptionExpected)] + public void FileLookupFailuresProduceADiagnostic(bool accessDenied) + { + GitCommandResolver.TryIsAlias( + "git-co", + key => new ProcessResult("checkout", string.Empty, 0), + SearchPath, + path => + { + if (accessDenied) + { + throw new UnauthorizedAccessException("access denied"); + } + + throw new IOException("lookup failed"); + }, + out bool isAlias, + out string error).ShouldBeFalse(); + + isAlias.ShouldBeFalse(); + error.ShouldContain("Unable to resolve Git command 'co'"); + error.ShouldContain(accessDenied ? "access denied" : "lookup failed"); + error.ShouldContain(@"mock:\git\libexec\git-co.exe"); + error.ShouldContain("0x"); + error.ShouldContain(accessDenied ? "UnauthorizedAccessException" : "IOException"); + } + + [TestCase] + public void SearchPathUsesRelativeDirectoriesAndSkipsEmptyEntries() + { + List candidates = new List(); + GitCommandResolver.TryIsAlias( + "git-co", + key => new ProcessResult("checkout", string.Empty, 0), + @";relative tools;;mock:\tools;", + path => + { + candidates.Add(path); + return false; + }, + out bool isAlias, + out string error).ShouldBeTrue(); + + candidates.ShouldMatchInOrder( + Path.Combine("relative tools", "git-co.exe"), + Path.Combine("relative tools", "git-co"), + @"mock:\tools\git-co.exe", + @"mock:\tools\git-co"); + isAlias.ShouldBeTrue(); + error.ShouldBeNull(); + } + + [TestCase] + public void ExecutableChangesDoNotUseAStaleCommandCache() + { + bool executableExists = false; + foreach (bool exists in new[] { false, true, false }) + { + executableExists = exists; + GitCommandResolver.TryIsAlias( + "git-co", + key => new ProcessResult("checkout", string.Empty, 0), + SearchPath, + path => executableExists, + out bool isAlias, + out string error).ShouldBeTrue(); + + isAlias.ShouldEqual(!exists); + error.ShouldBeNull(); + } + } + + [TestCase] + public void ExecutableSuffixDoesNotAddASecondExtension() + { + List candidates = new List(); + GitCommandResolver.TryIsAlias( + "git-co.EXE", + key => new ProcessResult("status", string.Empty, 0), + @"mock:\tools", + path => + { + candidates.Add(path); + return false; + }, + out bool isAlias, + out string error).ShouldBeTrue(); + + candidates.ShouldMatchInOrder(@"mock:\tools\git-co.EXE"); + isAlias.ShouldBeTrue(); + error.ShouldBeNull(); + } + + [TestCase] + public void CurrentProcessPathUsesTheSuppliedPid() + { + GitCommandResolver.GetProcessPath(Environment.ProcessId).ShouldEqual(Environment.ProcessPath); + } + + [TestCase] + [Category(CategoryConstants.ExceptionExpected)] + public void InvalidParentPidProducesAQueryFailure() + { + string diagnostic = null; + GitCommandResolver.ShouldLock( + "git-co", + key => + { + GitCommandResolver.GetProcessPath(-1); + throw new AssertionException("An invalid process ID must not return an executable path."); + }, + SearchPath, + path => throw new AssertionException("An invalid parent must not search PATH."), + message => diagnostic = message).ShouldBeTrue(); + + diagnostic.ShouldContain("ArgumentException"); + diagnostic.ShouldContain("GVFS will keep the command lock."); + } + + [TestCase] + [Category(CategoryConstants.ExceptionExpected)] + public void QueryFailuresKeepTheLockWithPreciseDiagnostics() + { + foreach (Exception failure in new Exception[] + { + new Win32Exception(5), + new InvalidOperationException("No executable path"), + new ArgumentException("No parent process"), + new IOException("Read failed"), + new UnauthorizedAccessException("Access denied"), + }) + { + string diagnostic = null; + GitCommandResolver.ShouldLock( + "git-co", + key => throw failure, + SearchPath, + path => throw new AssertionException("A failed query must not search PATH."), + message => diagnostic = message).ShouldBeTrue(); + + diagnostic.ShouldContain("Unable to query Git alias 'co'"); + diagnostic.ShouldContain(failure.GetType().Name); + diagnostic.ShouldContain(failure.HResult.ToString("X8")); + diagnostic.ShouldContain(failure.Message); + diagnostic.ShouldContain("GVFS will keep the command lock."); + diagnostic.Contains(" at GVFS.").ShouldBeFalse(); + } + } + + [TestCase] + [Category(CategoryConstants.ExceptionExpected)] + public void PathProbeFailuresKeepTheLockInsteadOfAbortingAliases() + { + foreach (Exception failure in new Exception[] + { + new IOException("Unavailable drive"), + new PathTooLongException("Path too long"), + new UnauthorizedAccessException("Access denied"), + new ArgumentException("Invalid path"), + }) + { + string diagnostic = null; + GitCommandResolver.ShouldLock( + "git-co", + key => new ProcessResult("checkout", string.Empty, 0), + SearchPath, + path => throw failure, + message => diagnostic = message).ShouldBeTrue(); + + diagnostic.ShouldContain(@"mock:\git\libexec\git-co.exe"); + diagnostic.ShouldContain(failure.GetType().Name); + diagnostic.ShouldContain("GVFS will keep the command lock."); + } + } + + [TestCase] + public void FailedConfigReadKeepsTheLockWithItsExitCode() + { + string diagnostic = null; + GitCommandResolver.ShouldLock( + "git-co", + key => new ProcessResult("checkout", "config read failed", 128), + SearchPath, + path => throw new AssertionException("A failed query must not search PATH."), + message => diagnostic = message).ShouldBeTrue(); + + diagnostic.ShouldContain("exit code 128"); + diagnostic.ShouldContain("config read failed"); + diagnostic.ShouldContain("GVFS will keep the command lock."); + } + + [TestCase] + public void GenuineAliasExemptionDoesNotReportAnError() + { + GitCommandResolver.ShouldLock( + "git-co", + key => new ProcessResult("checkout", string.Empty, 0), + SearchPath, + path => false, + message => throw new AssertionException(message)).ShouldBeFalse(); + } + + [TestCase] + public void AliasQueryUsesTheParentGitAndSuppressesRecursiveHooks() + { + const string ParentGit = @"mock:\custom git\bin\git.exe"; + const string AliasKey = "alias.git-MiXeD"; + ProcessStartInfo query = GitCommandResolver.CreateAliasQuery(ParentGit, AliasKey); + + query.FileName.ShouldEqual(ParentGit); + query.UseShellExecute.ShouldBeFalse(); + query.RedirectStandardInput.ShouldBeTrue(); + query.RedirectStandardOutput.ShouldBeTrue(); + query.RedirectStandardError.ShouldBeTrue(); + query.ArgumentList.ShouldMatchInOrder("config", "--get", AliasKey); + query.Environment["COMMAND_HOOK_LOCK"].ShouldEqual("true"); + query.Environment["PATH"].ShouldEqual(Environment.GetEnvironmentVariable("PATH")); + query.Environment.TryGetValue("GIT_EXEC_PATH", out string execPath); + execPath.ShouldEqual(Environment.GetEnvironmentVariable("GIT_EXEC_PATH")); + query.Environment.TryGetValue("GIT_CONFIG_PARAMETERS", out string parameters); + parameters.ShouldEqual(Environment.GetEnvironmentVariable("GIT_CONFIG_PARAMETERS")); + } + } +} diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs index 2292149b65..c179372a1c 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs @@ -1,5 +1,6 @@ using GVFS.Common; using GVFS.Common.Git; +using GVFS.Common.Tracing; using GVFS.UnitTests.Mock.Git; using System.IO; @@ -39,5 +40,10 @@ public override GitProcess CreateGitProcess() { return this.gitProcess ?? new MockGitProcess(); } + + public override GitProcess CreateGitProcess(ITracer tracer) + { + return this.CreateGitProcess(); + } } } diff --git a/GVFS/GVFS.UnitTests/Mock/Git/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); + } +} diff --git a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs index cf786a9835..03f229da31 100644 --- a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs +++ b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs @@ -15,7 +15,12 @@ public class MockGitProcess : GitProcess private List expectedCommandInfos = new List(); public MockGitProcess() - : base(new MockGVFSEnlistment()) + : this(NullTracer.Instance, GitConfigReadRetry.Delay) + { + } + + internal MockGitProcess(ITracer tracer, Action configReadDelay) + : base(new MockGVFSEnlistment(), tracer, configReadDelay) { this.CommandsRun = new List(); this.InvocationsRun = new List(); diff --git a/docs/faq.md b/docs/faq.md index 5026fe6411..36bae31d69 100644 --- a/docs/faq.md +++ b/docs/faq.md @@ -25,6 +25,20 @@ you can help optimize Git command performance for developers in the repository by limiting the number of files Git needs to consider for many common operations. +### Do Git aliases change command locks? + +When Git selects a built-in command or an external executable, a +same-name alias does not remove the GVFS lock requirement. For example, `alias.stash` +does not change the lock requirement for `git stash`. + +VFS for Git skips the outer lock for a genuine alias. Each command in +the alias follows its normal lock policy. For example, an alias that +runs `checkout` still requires the lock for `checkout`. + +If an alias-config query or executable probe fails, VFS for Git reports +the error and keeps the command lock. The post-command hook releases a +lock only when its Git process owns that lock. + ### Why the name change? This project was formerly known as GVFS (Git Virtual File System). It is