From b4e91944687ce8ab60bf6b590b4f0c9de3f06039 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Mon, 21 Sep 2026 16:01:32 -0700 Subject: [PATCH 01/11] Marshal libgit2 P/Invoke strings as UTF-8, not ANSI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit libgit2 treats every string it receives and returns as UTF-8. The LibGit2Repo P/Invoke declarations left CharSet unset, so a bare [DllImport] string parameter defaulted to CharSet.Ansi and encoded through the Windows ANSI code page. On a non-UTF-8 code page an unmappable character (for example a repository path under a non-English Windows user name, such as "C:\Users\...\café_日本語") was dropped to '?', so git_repository_open resolved the wrong path and the constructor threw InvalidDataException. Annotate every string parameter with [MarshalAs(UnmanagedType.LPUTF8Str)]. This covers the repository path (git_repository_open), the revspec (git_revparse_single), the object id string (git_oid_fromstr), and the config keys (git_config_get_string, git_config_get_bool, git_config_get_multivar_foreach). Read the borrowed git_error.message pointer as an IntPtr and copy it with Marshal.PtrToStringUTF8, matching the existing GitConfigEntry pattern, so the interop marshaller never frees libgit2's heap memory. The config value output path already used Marshal.PtrToStringUTF8, so it is unchanged. Add LibGit2NonAsciiPathTests, which opens a real repository at a non-ASCII path through git2.dll and reads a non-ASCII config value. Before the fix both cases fail with "No mapping for the Unicode character exists in the target multi-byte code page"; after the fix they pass. The fixture ignores itself when the active ANSI code page is already UTF-8 (CP65001), where the bug cannot reproduce. Document the UTF-8 marshalling contract on the LibGit2Repo class and in AGENTS.md so future libgit2 bindings do not regress. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- AGENTS.md | 31 +++++ GVFS/GVFS.Common/Git/LibGit2Repo.cs | 47 ++++++-- .../Tests/LibGit2NonAsciiPathTests.cs | 107 ++++++++++++++++++ 3 files changed, 175 insertions(+), 10 deletions(-) create mode 100644 GVFS/GVFS.FunctionalTests/Tests/LibGit2NonAsciiPathTests.cs diff --git a/AGENTS.md b/AGENTS.md index e5dd769f3..2698dd58f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -199,6 +199,37 @@ Default new gates to `false` and gate the **runtime entry point** into a feature, not its build, so the code still compiles and ships (and keeps getting exercised) while its behavior stays off by default. +## libgit2 P/Invoke string marshalling (UTF-8, not ANSI) + +The libgit2 bindings live in `GVFS.Common/Git/LibGit2Repo.cs` under the +`Native` class. **libgit2 treats every string it receives and returns as +UTF-8** — paths, revspecs, config keys and values, and error messages. + +The .NET default for a bare `[DllImport]` string parameter is +`CharSet.Ansi`, which encodes through the Windows ANSI code page and +**silently corrupts non-ASCII input** (a repo path under a non-English user +name, a non-ASCII branch name, etc.) — an unmappable character becomes `?`, +so `git_repository_open` and friends resolve the wrong path or fail. There is +no `[module: DefaultCharSet]` override in `GVFS.Common`, so the ANSI default +applies unless each declaration opts out. + +When you add a libgit2 P/Invoke: + +- Annotate **every** `string` parameter, `out string` parameter, string return + value, and string struct field with + `[MarshalAs(UnmanagedType.LPUTF8Str)]`. +- Do **not** use `CharSet.Unicode` — that marshals UTF-16, which libgit2 does + not accept (it is the same bug in the other direction). +- For a function that returns a **borrowed** pointer owned by libgit2 (e.g. + `git_config_get_string`), marshal it as `IntPtr` and copy with + `Marshal.PtrToStringUTF8(...)`; do not marshal it as `out string`, or the + interop marshaller frees libgit2's heap memory with the wrong allocator. + +Because the mismatch only manifests through the native call, cover new +string-carrying paths with a real-libgit2 test that uses a non-ASCII input +(see `GVFS.FunctionalTests/Tests/LibGit2NonAsciiPathTests.cs`), not a +mock-based unit test. + ## Coding standards See [CONTRIBUTING.md](CONTRIBUTING.md) for StyleCop rules, error-handling diff --git a/GVFS/GVFS.Common/Git/LibGit2Repo.cs b/GVFS/GVFS.Common/Git/LibGit2Repo.cs index b21eb83b9..6667dd2c8 100644 --- a/GVFS/GVFS.Common/Git/LibGit2Repo.cs +++ b/GVFS/GVFS.Common/Git/LibGit2Repo.cs @@ -7,6 +7,24 @@ namespace GVFS.Common.Git { + /// + /// In-process wrapper over libgit2 (git2.dll) for local Git object, tree, blob, and + /// config lookups without spawning git.exe. + /// + /// + /// libgit2 marshalling contract: libgit2 treats EVERY string it receives and returns as + /// UTF-8 (paths, revspecs, config keys and values, error messages). The default marshalling + /// for a bare [DllImport] string parameter is , which encodes + /// through the Windows ANSI code page and silently corrupts (or drops, as '?') any non-ASCII + /// byte — for example a repository path under a non-English Windows user name. When adding a + /// new libgit2 P/Invoke to , annotate every string parameter, + /// out string parameter, string return value, and string struct field with + /// [MarshalAs(UnmanagedType.LPUTF8Str)]. Do NOT use — that + /// marshals UTF-16, which libgit2 does not accept. For a libgit2 function that returns a borrowed + /// pointer owned by libgit2 (for example git_config_get_string), marshal it as + /// and copy with so the + /// interop marshaller does not free libgit2's heap memory. + /// public class LibGit2Repo : IDisposable { private bool disposedValue = false; @@ -572,6 +590,12 @@ protected bool CheckSafeDirectoryConfigForCaseSensitivityIssue(ITracer tracer, s return configuredMatchingDirectory != null && TryOpenRepo(configuredMatchingDirectory, out repoHandle) == Native.ResultCode.Success; } + /// + /// Raw libgit2 P/Invoke declarations. See the marshalling contract on + /// : every string parameter/return/field passed to or from + /// libgit2 MUST be [MarshalAs(UnmanagedType.LPUTF8Str)] (never the ANSI default, + /// never ). + /// public static class Native { public enum ResultCode : int @@ -602,7 +626,7 @@ public static GitOid IntPtrToGitOid(IntPtr oidPtr) public static extern int Shutdown(); [DllImport(Git2NativeLibName, EntryPoint = "git_revparse_single")] - public static extern ResultCode RevParseSingle(out IntPtr objectHandle, IntPtr repoHandle, string oid); + public static extern ResultCode RevParseSingle(out IntPtr objectHandle, IntPtr repoHandle, [MarshalAs(UnmanagedType.LPUTF8Str)] string oid); public static string GetLastError() { @@ -612,7 +636,11 @@ public static string GetLastError() return "Operation was successful"; } - return Marshal.PtrToStructure(ptr).Message; + // git_error.message is a borrowed UTF-8 char* owned by libgit2. Keep it as an + // IntPtr and copy it with Marshal.PtrToStringUTF8 so the interop marshaller never + // frees libgit2's heap memory (same pattern as GitConfigEntry). + GitError error = Marshal.PtrToStructure(ptr); + return error.Message == IntPtr.Zero ? null : Marshal.PtrToStringUTF8(error.Message); } [DllImport(Git2NativeLibName, EntryPoint = "git_error_last")] @@ -621,8 +649,7 @@ public static string GetLastError() [StructLayout(LayoutKind.Sequential)] private struct GitError { - [MarshalAs(UnmanagedType.LPStr)] - public string Message; + public IntPtr Message; public int Klass; } @@ -630,7 +657,7 @@ private struct GitError public static class Repo { [DllImport(Git2NativeLibName, EntryPoint = "git_repository_open")] - public static extern ResultCode Open(out IntPtr repoHandle, string path); + public static extern ResultCode Open(out IntPtr repoHandle, [MarshalAs(UnmanagedType.LPUTF8Str)] string path); [DllImport(Git2NativeLibName, EntryPoint = "git_repository_free")] public static extern void Free(IntPtr repoHandle); @@ -649,7 +676,7 @@ public static class Odb public static extern void Free(IntPtr odbHandle); [DllImport(Git2NativeLibName, EntryPoint = "git_oid_fromstr")] - public static extern ResultCode OidFromStr(out GitOid oid, string str); + public static extern ResultCode OidFromStr(out GitOid oid, [MarshalAs(UnmanagedType.LPUTF8Str)] string str); } public static class Config @@ -664,13 +691,13 @@ public static class Config public static extern ResultCode Snapshot(out IntPtr snapshotConfigHandle, IntPtr configHandle); [DllImport(Git2NativeLibName, EntryPoint = "git_config_get_string")] - public static extern ResultCode GetString(out IntPtr value, IntPtr configHandle, string name); + public static extern ResultCode GetString(out IntPtr value, IntPtr configHandle, [MarshalAs(UnmanagedType.LPUTF8Str)] string name); [DllImport(Git2NativeLibName, EntryPoint = "git_config_get_multivar_foreach")] public static extern ResultCode GetMultivarForeach( IntPtr configHandle, - string name, - string regex, + [MarshalAs(UnmanagedType.LPUTF8Str)] string name, + [MarshalAs(UnmanagedType.LPUTF8Str)] string regex, GitConfigMultivarCallback callback, IntPtr payload); @@ -719,7 +746,7 @@ private static string MarshalUtf8String(IntPtr ptr) } [DllImport(Git2NativeLibName, EntryPoint = "git_config_get_bool")] - public static extern ResultCode GetBool(out bool value, IntPtr configHandle, string name); + public static extern ResultCode GetBool(out bool value, IntPtr configHandle, [MarshalAs(UnmanagedType.LPUTF8Str)] string name); [DllImport(Git2NativeLibName, EntryPoint = "git_config_free")] public static extern void Free(IntPtr configHandle); diff --git a/GVFS/GVFS.FunctionalTests/Tests/LibGit2NonAsciiPathTests.cs b/GVFS/GVFS.FunctionalTests/Tests/LibGit2NonAsciiPathTests.cs new file mode 100644 index 000000000..d641ac677 --- /dev/null +++ b/GVFS/GVFS.FunctionalTests/Tests/LibGit2NonAsciiPathTests.cs @@ -0,0 +1,107 @@ +using GVFS.Common.Git; +using GVFS.Common.Tracing; +using GVFS.FunctionalTests.Tools; +using GVFS.Tests.Should; +using NUnit.Framework; +using System.IO; +using System.Runtime.InteropServices; +using System.Text; +using GitProcess = GVFS.FunctionalTests.Tools.GitProcess; + +namespace GVFS.FunctionalTests.Tests +{ + /// + /// Exercises the real libgit2 (git2.dll) open/read path in against a + /// repository whose path and config value contain non-ASCII characters. This is a regression + /// guard for the string-marshalling bug where the libgit2 P/Invoke declarations used the + /// implicit default instead of UTF-8: + /// on a non-UTF-8 Windows code page the ANSI marshaller drops an unmappable character to '?', + /// so git_repository_open resolves the wrong path and the constructor throws. No + /// mock-based unit test can catch this because it only manifests through the native P/Invoke. + /// + [TestFixture] + public class LibGit2NonAsciiPathTests + { + // "café_日本語_☃" — Latin-1 accent, CJK, and a symbol outside every common Windows ANSI + // code page. Built from \u escapes so the reproduction does not depend on this source + // file's on-disk encoding. + private const string NonAsciiToken = "caf\u00e9_\u65e5\u672c\u8a9e_\u2603"; + + private const string StringConfigKey = "gvfs.functionaltests-nonascii"; + private const string NonAsciiConfigValue = "caf\u00e9-\u65e5\u672c\u8a9e-\u2603"; + + private string repoRoot; + + [OneTimeSetUp] + public void CreateRepo() + { + // The ANSI default marshalling only corrupts non-ASCII characters when the active + // Windows ANSI code page cannot represent them. If the process ANSI code page is + // UTF-8 (CP65001, the "Beta: Use Unicode UTF-8" setting), ANSI marshalling already + // produces correct UTF-8, so the bug cannot reproduce and this guard is inconclusive. + // Log the active code page so a reader of the test results can tell whether this guard + // actually ran (e.g. CP1252 on the standard CI runners) or was skipped as inconclusive. + const uint CP_UTF8 = 65001; + uint activeCodePage = GetACP(); + TestContext.Progress.WriteLine($"{nameof(LibGit2NonAsciiPathTests)}: active ANSI code page = {activeCodePage}"); + if (activeCodePage == CP_UTF8) + { + Assert.Ignore( + $"Active ANSI code page is UTF-8 (CP{CP_UTF8}); ANSI marshalling already produces UTF-8, so the " + + "ANSI-vs-UTF-8 marshalling bug cannot reproduce and this regression guard is inconclusive here. " + + "Run on a non-UTF-8 code page (e.g. CP1252, the standard CI runner default) to exercise it."); + } + + this.repoRoot = Path.Combine( + Path.GetTempPath(), + "GVFS.LibGit2NonAsciiPathTests_" + NonAsciiToken + "_" + Path.GetRandomFileName()); + Directory.CreateDirectory(this.repoRoot); + + GitProcess.Invoke(this.repoRoot, "init"); + GitProcess.Invoke(this.repoRoot, "config user.name \"Functional Test User\""); + GitProcess.Invoke(this.repoRoot, "config user.email \"functional@test.com\""); + + // Write the non-ASCII config value straight to .git/config as UTF-8 (no BOM) so the + // value round-tripped through libgit2 is not confounded by how git.exe would encode a + // command-line argument. + string configPath = Path.Combine(this.repoRoot, ".git", "config"); + File.AppendAllText( + configPath, + $"[gvfs]\n\tfunctionaltests-nonascii = {NonAsciiConfigValue}\n", + new UTF8Encoding(encoderShouldEmitUTF8Identifier: false)); + } + + [OneTimeTearDown] + public void DeleteRepo() + { + if (this.repoRoot != null) + { + RepositoryHelpers.DeleteTestDirectory(this.repoRoot); + } + } + + [TestCase] + public void OpensRepositoryAtNonAsciiPath() + { + // Before the LPUTF8Str marshalling fix, git_repository_open received an ANSI-mangled + // path, failed, and the constructor threw InvalidDataException. + using (LibGit2Repo repo = new LibGit2Repo(NullTracer.Instance, this.repoRoot)) + { + // Reading a known config value proves the RepoHandle is real, not just non-null. + repo.GetConfigString("user.email").ShouldEqual("functional@test.com"); + } + } + + [TestCase] + public void GetConfigStringReturnsNonAsciiValue() + { + using (LibGit2Repo repo = new LibGit2Repo(NullTracer.Instance, this.repoRoot)) + { + repo.GetConfigString(StringConfigKey).ShouldEqual(NonAsciiConfigValue); + } + } + + [DllImport("kernel32.dll")] + private static extern uint GetACP(); + } +} From 096f16a1645a21db76bd21cef7f6c5872e7496d7 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 23 Sep 2026 12:09:12 +0200 Subject: [PATCH 02/11] Update default Microsoft Git version to v2.55.0.vfs.0.10 --- .github/workflows/build.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index a2778fb5a..32706260f 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -24,7 +24,7 @@ permissions: checks: read env: - GIT_VERSION: ${{ github.event.inputs.git_version || 'v2.55.0.vfs.0.8' }} + GIT_VERSION: ${{ github.event.inputs.git_version || 'v2.55.0.vfs.0.10' }} jobs: validate: From b3c837fa4bf95a1a60d9c37de8c9b0ea91241c62 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 23 Sep 2026 14:11:37 -0700 Subject: [PATCH 03/11] docs(agents): clarify functional test dev workflow Document payload publishing, named runner arguments, and local service path resolution. Warn about stale outputs and shared functional-test resources. Assisted-by: GPT-5.6 Sol Signed-off-by: Tyrie Vella --- AGENTS.md | 46 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 2698dd58f..36ab2c0e0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -64,8 +64,8 @@ Skips `dotnet publish`, AOT, native C++ projects, payload assembly, installer. For changes that need the GVFS payload (`gvfs.exe`, hooks, service) but not an installer. `PublishAot=false` skips ilc (~3–4 min saved); `SkipCreateInstaller=true` skips Inno Setup (~95 s saved). -`GVFS.Payload` cascades to its dependencies (GVFS, GVFS.Mount, GVFS.Hooks, -GVFS.Service) via `ProjectReference`. +`GVFS.Payload` only assembles the payload directory. It does not build or +publish the projects that it copies from. > **Prerequisite: the native C++ projects must already be built.** They are > `.vcxproj` (see [Native C++ projects](#native-c-projects-need-msbuild-not-dotnet-build) @@ -78,10 +78,13 @@ GVFS.Service) via `ProjectReference`. ```powershell dotnet publish src\GVFS\GVFS.FunctionalTests\GVFS.FunctionalTests.csproj ` -c Debug /p:PublishAot=false +dotnet publish src\GVFS\GVFS\GVFS.csproj ` + -c Debug /p:PublishAot=false dotnet publish src\GVFS\GVFS.Payload\GVFS.Payload.csproj ` -c Debug /p:PublishAot=false /p:SkipCreateInstaller=true -src\scripts\RunFunctionalTests-Dev.ps1 Debug --test=GVFS.FunctionalTests.Tests... +src\scripts\RunFunctionalTests-Dev.ps1 -Configuration Debug -Arch x64 ` + --test=GVFS.FunctionalTests.Tests... ``` `layout.bat` (invoked by GVFS.Payload) `xcopy`s from each project's `publish\` @@ -89,10 +92,27 @@ or native-output directory — the C# projects do not require AOT, so `PublishAot=false` produces a fully functional test payload. The native hook binaries are copied straight from the vcxproj output. +> **Publish each changed project explicitly.** `GVFS.Payload.csproj` has no +> `ProjectReference` items. Its `CreatePayload` target runs `layout.bat`, which +> copies existing project output. A project that you do not publish can +> contribute stale output from an earlier build. Publish `GVFS.Mount`, +> `GVFS.Hooks`, or `GVFS.Service` explicitly when you change those projects. + `RunFunctionalTests-Dev.ps1` runs functional tests against the build output without requiring admin or a system-wide install. It launches the test -service as a console process. Each invocation gets a unique service name -and data dir, so concurrent runs from different worktrees don't collide. +service as a console process. Each invocation gets a unique service name and +service-data directory. Some fixtures still use shared machine paths or drive +mappings, so do not assume that separate test processes can run concurrently. + +The script sets dev-mode variables. `Settings.Default.Initialize` then resolves +`gvfs.exe` and `GVFS.Service.exe` from `out\GVFS.Payload\...`, not +`C:\Program Files\VFS for Git\`. This path does not require a GVFS +installation. + +> **Pass `-Configuration` and `-Arch` by name.** The script's first two +> positional parameters are `Configuration` and `Arch`. A bare +> `RunFunctionalTests-Dev.ps1 Debug --test=...` binds `--test=...` to `-Arch` +> and fails its `ValidateSet`. ### Path C — Installer build (~5 min — only when you need an installer) @@ -135,12 +155,14 @@ participate in the C# inner-loop paths above. ```powershell # ✅ Correct -& "out\GVFS.UnitTests\bin\...\GVFS.UnitTests.exe" --test "GVFS.UnitTests.Common.WorktreeInfoTests" -src\scripts\RunFunctionalTests-Dev.ps1 Debug --test=GVFS.FunctionalTests.Tests.GVFSVerbTests.UnknownVerb +& "out\GVFS.UnitTests\bin\...\GVFS.UnitTests.exe" --test "GVFS.UnitTests.Common.WorktreeInfoTests" +src\scripts\RunFunctionalTests-Dev.ps1 -Configuration Debug -Arch x64 ` + --test=GVFS.FunctionalTests.Tests.GVFSVerbTests.UnknownVerb # ❌ Wrong — silently runs the entire suite -& "out\GVFS.UnitTests\bin\...\GVFS.UnitTests.exe" --where "class =~ Worktree" -src\scripts\RunFunctionalTests-Dev.ps1 Debug --where "cat == Smoke" +& "out\GVFS.UnitTests\bin\...\GVFS.UnitTests.exe" --where "class =~ Worktree" +src\scripts\RunFunctionalTests-Dev.ps1 -Configuration Debug -Arch x64 ` + --where "cat == Smoke" ``` For unit tests, `--where` is merely annoying (the whole suite runs in @@ -230,6 +252,12 @@ string-carrying paths with a real-libgit2 test that uses a non-ASCII input (see `GVFS.FunctionalTests/Tests/LibGit2NonAsciiPathTests.cs`), not a mock-based unit test. +## Public repository hygiene + +Do not include Microsoft-internal work item IDs, ADO URLs, incident IDs, or +internal service names in repository content or pull requests. Describe the +public problem and fix without requiring internal access. + ## Coding standards See [CONTRIBUTING.md](CONTRIBUTING.md) for StyleCop rules, error-handling From fb3ea43705a17b4516e750e6d65cefc7c2efcb15 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 24 Sep 2026 08:59:51 -0700 Subject: [PATCH 04/11] docs(agents): clarify payload prerequisites Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 36ab2c0e0..861318417 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -67,19 +67,27 @@ an installer. `PublishAot=false` skips ilc (~3–4 min saved); `GVFS.Payload` only assembles the payload directory. It does not build or publish the projects that it copies from. -> **Prerequisite: the native C++ projects must already be built.** They are -> `.vcxproj` (see [Native C++ projects](#native-c-projects-need-msbuild-not-dotnet-build) +> **Prerequisite: the native C++ projects and all managed payload projects must +> already have built outputs.** The native projects are `.vcxproj` (see +> [Native C++ projects](#native-c-projects-need-msbuild-not-dotnet-build) > below) and `dotnet publish` will not build them for you. If you have not > already done a `Build.bat` once in this enlistment, build the native -> projects via VS MSBuild first (or run `Build.bat` once to populate `out\`, -> then iterate with the commands below). After that they are incremental and -> only rebuild when their own sources change. +> projects via VS MSBuild and publish the managed payload projects below +> (or run `Build.bat` once to populate `out\`, then iterate with the commands +> below). After that they are incremental and only rebuild when their own +> sources change. ```powershell dotnet publish src\GVFS\GVFS.FunctionalTests\GVFS.FunctionalTests.csproj ` -c Debug /p:PublishAot=false dotnet publish src\GVFS\GVFS\GVFS.csproj ` -c Debug /p:PublishAot=false +dotnet publish src\GVFS\GVFS.Hooks\GVFS.Hooks.csproj ` + -c Debug /p:PublishAot=false +dotnet publish src\GVFS\GVFS.Mount\GVFS.Mount.csproj ` + -c Debug /p:PublishAot=false +dotnet publish src\GVFS\GVFS.Service\GVFS.Service.csproj ` + -c Debug /p:PublishAot=false dotnet publish src\GVFS\GVFS.Payload\GVFS.Payload.csproj ` -c Debug /p:PublishAot=false /p:SkipCreateInstaller=true @@ -95,8 +103,9 @@ hook binaries are copied straight from the vcxproj output. > **Publish each changed project explicitly.** `GVFS.Payload.csproj` has no > `ProjectReference` items. Its `CreatePayload` target runs `layout.bat`, which > copies existing project output. A project that you do not publish can -> contribute stale output from an earlier build. Publish `GVFS.Mount`, -> `GVFS.Hooks`, or `GVFS.Service` explicitly when you change those projects. +> contribute stale output from an earlier build. Publish all four managed +> payload projects at least once in a new enlistment, and publish a project +> again when you change it. `RunFunctionalTests-Dev.ps1` runs functional tests against the build output without requiring admin or a system-wide install. It launches the test From 51592aa1042175eb5318c157c5d8a1e4142d46f6 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 24 Sep 2026 11:36:10 -0700 Subject: [PATCH 05/11] Retry native C++ project builds on transient MSBuild failure PR validation builds on the arm64 runner intermittently fail with `C3859: Failed to create virtual memory for PCH` while building one of the native hook projects (e.g. GVFS.PostIndexChangedHook). This is a transient cl.exe failure caused by memory pressure on the build host, not a problem with the code being built, and the same build reliably succeeds on a manual re-run. Build.bat now retries each native vcxproj build (GitHooksLoader, GVFS.NativeTests, GVFS.PostIndexChangedHook, GVFS.ReadObjectHook, GVFS.VirtualFileSystemHook) up to 3 times, with a short delay between attempts, before failing the build. This removes the need for a human to notice the failure and manually re-run the pipeline. Assisted-by: GitHub Copilot Signed-off-by: Tyrie Vella --- scripts/Build.bat | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/scripts/Build.bat b/scripts/Build.bat index 592020f59..66d70a1b9 100644 --- a/scripts/Build.bat +++ b/scripts/Build.bat @@ -142,6 +142,13 @@ IF NOT DEFINED VCVARS_BAT ( ECHO INFO: Initializing VC++ env for %ARCH% via "%VCVARS_BAT%" CALL "%VCVARS_BAT%" %ARCH% || GOTO ERROR +REM cl.exe can transiently fail with C3859 "Failed to create virtual memory +REM for PCH" under memory pressure (observed on the arm64 runners, where PCH +REM creation can collide with other concurrent processes for address space). +REM It is not caused by anything in this repo's code and reliably succeeds on +REM a retry, so retry each native project build a few times before failing. +SET NATIVE_BUILD_RETRIES=3 + FOR %%P IN ( "%VFS_SRCDIR%\GVFS\GitHooksLoader\GitHooksLoader.vcxproj" "%VFS_SRCDIR%\GVFS\GVFS.NativeTests\GVFS.NativeTests.vcxproj" @@ -149,14 +156,7 @@ FOR %%P IN ( "%VFS_SRCDIR%\GVFS\GVFS.ReadObjectHook\GVFS.ReadObjectHook.vcxproj" "%VFS_SRCDIR%\GVFS\GVFS.VirtualFileSystemHook\GVFS.VirtualFileSystemHook.vcxproj" ) DO ( - ECHO Building %%~nP... - "%MSBUILD_EXEC%" %%P ^ - /t:Build ^ - /v:%VERBOSITY% ^ - /p:Configuration=%CONFIGURATION% ^ - /p:Platform=%NATIVE_PLATFORM% ^ - /p:VfsArch=%ARCH% ^ - /p:SolutionDir="%VFS_SRCDIR%\\" || GOTO ERROR + CALL :BuildNativeProject %%P || GOTO ERROR ) REM vcvarsall.bat sets Platform= in the environment. MSBuild picks that @@ -201,6 +201,31 @@ FOR %%P IN ( GOTO :EOF +REM Builds a single native vcxproj, retrying on failure up to +REM %NATIVE_BUILD_RETRIES% times. cl.exe's PCH creation (C3859) can fail +REM transiently under memory pressure; a retry is usually enough to recover. +:BuildNativeProject +SET "NATIVE_PROJ=%~1" +SET NATIVE_ATTEMPT=1 +:BuildNativeProject_Retry +ECHO Building %~n1 (attempt %NATIVE_ATTEMPT% of %NATIVE_BUILD_RETRIES%)... +"%MSBUILD_EXEC%" "%NATIVE_PROJ%" ^ + /t:Build ^ + /v:%VERBOSITY% ^ + /p:Configuration=%CONFIGURATION% ^ + /p:Platform=%NATIVE_PLATFORM% ^ + /p:VfsArch=%ARCH% ^ + /p:SolutionDir="%VFS_SRCDIR%\\" +IF NOT ERRORLEVEL 1 EXIT /B 0 +IF %NATIVE_ATTEMPT% GEQ %NATIVE_BUILD_RETRIES% ( + ECHO ERROR: Build of %~n1 failed after %NATIVE_BUILD_RETRIES% attempts. + EXIT /B 1 +) +ECHO WARNING: Build of %~n1 failed on attempt %NATIVE_ATTEMPT%; this can be a transient PCH virtual-memory error ^(C3859^). Retrying in 10 seconds... +TIMEOUT /T 10 /NOBREAK >NUL +SET /A NATIVE_ATTEMPT+=1 +GOTO BuildNativeProject_Retry + :USAGE ECHO usage: %~n0%~x0 [^] [^] [^] [^] ECHO. From 27d8d9a668a91ffd1b50cbc965dac448a4cc833b Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 24 Sep 2026 12:25:23 -0700 Subject: [PATCH 06/11] Only retry native builds on the actual C3859 PCH error The previous commit retried a native vcxproj build on any MSBuild failure, which would also retry (and mask, and waste time on) a real compile error introduced by a code change. Capture each attempt's output and only retry when the log actually contains the transient C3859 "Failed to create virtual memory for PCH" error; any other failure now fails immediately on the first attempt. Assisted-by: GitHub Copilot Signed-off-by: Tyrie Vella --- scripts/Build.bat | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/scripts/Build.bat b/scripts/Build.bat index 66d70a1b9..2cb426056 100644 --- a/scripts/Build.bat +++ b/scripts/Build.bat @@ -202,12 +202,15 @@ FOR %%P IN ( GOTO :EOF REM Builds a single native vcxproj, retrying on failure up to -REM %NATIVE_BUILD_RETRIES% times. cl.exe's PCH creation (C3859) can fail -REM transiently under memory pressure; a retry is usually enough to recover. +REM %NATIVE_BUILD_RETRIES% times, but ONLY when the failure is the known +REM transient cl.exe PCH virtual-memory error (C3859). Any other failure +REM (a real compile error, a bad project change, etc.) fails immediately +REM on the first attempt so it isn't masked or slowed down by retries. :BuildNativeProject SET "NATIVE_PROJ=%~1" SET NATIVE_ATTEMPT=1 :BuildNativeProject_Retry +SET "NATIVE_LOG=%TEMP%\gvfs_native_build_%~n1_%RANDOM%.log" ECHO Building %~n1 (attempt %NATIVE_ATTEMPT% of %NATIVE_BUILD_RETRIES%)... "%MSBUILD_EXEC%" "%NATIVE_PROJ%" ^ /t:Build ^ @@ -215,13 +218,25 @@ ECHO Building %~n1 (attempt %NATIVE_ATTEMPT% of %NATIVE_BUILD_RETRIES%)... /p:Configuration=%CONFIGURATION% ^ /p:Platform=%NATIVE_PLATFORM% ^ /p:VfsArch=%ARCH% ^ - /p:SolutionDir="%VFS_SRCDIR%\\" -IF NOT ERRORLEVEL 1 EXIT /B 0 + /p:SolutionDir="%VFS_SRCDIR%\\" > "%NATIVE_LOG%" 2>&1 +SET NATIVE_BUILD_RESULT=%ERRORLEVEL% +TYPE "%NATIVE_LOG%" +IF %NATIVE_BUILD_RESULT% EQU 0 ( + DEL "%NATIVE_LOG%" >NUL 2>&1 + EXIT /B 0 +) +FINDSTR /C:"C3859" "%NATIVE_LOG%" >NUL +SET NATIVE_IS_TRANSIENT=%ERRORLEVEL% +DEL "%NATIVE_LOG%" >NUL 2>&1 +IF NOT %NATIVE_IS_TRANSIENT% EQU 0 ( + ECHO ERROR: Build of %~n1 failed with a non-transient error; not retrying. + EXIT /B 1 +) IF %NATIVE_ATTEMPT% GEQ %NATIVE_BUILD_RETRIES% ( - ECHO ERROR: Build of %~n1 failed after %NATIVE_BUILD_RETRIES% attempts. + ECHO ERROR: Build of %~n1 failed after %NATIVE_BUILD_RETRIES% attempts, still with C3859. EXIT /B 1 ) -ECHO WARNING: Build of %~n1 failed on attempt %NATIVE_ATTEMPT%; this can be a transient PCH virtual-memory error ^(C3859^). Retrying in 10 seconds... +ECHO WARNING: Build of %~n1 failed on attempt %NATIVE_ATTEMPT% with the transient PCH virtual-memory error ^(C3859^). Retrying in 10 seconds... TIMEOUT /T 10 /NOBREAK >NUL SET /A NATIVE_ATTEMPT+=1 GOTO BuildNativeProject_Retry From b35b9b6f65dec48e27334f8045b5cf4302f93247 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 10 Jun 2026 15:49:03 -0700 Subject: [PATCH 07/11] Add regression tests for reset --mixed skip-worktree on hydrated files In a GVFS enlistment, a hydrated placeholder is on disk but is not in ModifiedPaths, so it still has skip-worktree. A mixed reset does not update the working tree. If git leaves skip-worktree set on a hydrated placeholder whose index entry the reset changes, reset and status do not report the file as modified. The git side of this behavior requires microsoft/git v2.55.0.vfs.0.3 or later. Add two tests: - CorruptionReproTests.ReproResetMixedSkipWorktree is a black-box test. It runs the command sequence that exposed the problem (blame, then reset HEAD~1) and compares the results with the control repo. - ResetMixedTests.ResetMixedClearsSkipWorktreeOnHydratedPlaceholder is a white-box test. It asserts each precondition before the reset: the index entry changes, the file is hydrated, the file is not in ModifiedPaths, and skip-worktree is set. After the reset, it asserts that skip-worktree is cleared and that GVFS adds the file to ModifiedPaths. Also document on GitHelpers.ValidateGitCommand that it compares status output after each command other than status. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella --- .../Tests/GitCommands/CorruptionReproTests.cs | 31 +++++++++- .../Tests/GitCommands/ResetMixedTests.cs | 57 +++++++++++++++++++ GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs | 5 ++ 3 files changed, 92 insertions(+), 1 deletion(-) diff --git a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CorruptionReproTests.cs b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CorruptionReproTests.cs index 9e7eec0bb..4dade3f56 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CorruptionReproTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CorruptionReproTests.cs @@ -12,7 +12,10 @@ namespace GVFS.FunctionalTests.Tests.GitCommands { /// - /// This class is used to reproduce corruption scenarios in the GVFS virtual projection. + /// Reproduces reported scenarios that corrupted the GVFS virtual projection or + /// gave results that did not match a normal git repo. Each test is a black-box + /// regression test: it runs the command sequence that exposed the problem and + /// compares the results with the control repo. /// [Category(Categories.GitCommands)] [TestFixtureSource(typeof(GitRepoTests), nameof(GitRepoTests.ValidateWorkingTree))] @@ -76,6 +79,32 @@ public void ReproCherryPickRestoreCorruption() this.FilesShouldMatchCheckoutOfSourceBranch(); } + /// + /// Regression test for a sequence of commands that a user ran and that + /// showed incorrect behavior: after "git blame" and "git reset --mixed", + /// GVFS did not report a changed file as modified. The control repo did. + /// + /// This is a black-box test. It does not assert why the behavior was wrong. + /// It only runs the same commands and compares the results with the + /// control repo. ResetMixedTests.ResetMixedClearsSkipWorktreeOnHydratedPlaceholder + /// asserts the conditions that cause the problem. + /// + /// The fix requires microsoft/git v2.55.0.vfs.0.3 or later. + /// + /// On the FunctionalTests/20201014 branch, Readme.md is the only file that + /// differs between HEAD and HEAD~1. + /// + [TestCase] + public void ReproResetMixedSkipWorktree() + { + this.ValidateGitCommand("blame Readme.md"); + this.ValidateGitCommand("checkout -b tests/functional/ReproResetMixedSkipWorktree"); + + // Both repos must report Readme.md as modified. + this.ValidateGitCommand("reset HEAD~1"); + this.ValidateGitCommand("ls-files -v Readme.md"); + } + /// /// Reproduction of a reported issue: /// Restoring a file after its parent directory was deleted fails with diff --git a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/ResetMixedTests.cs b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/ResetMixedTests.cs index 0f7740d90..af77ee6e4 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/ResetMixedTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/ResetMixedTests.cs @@ -1,6 +1,9 @@ using GVFS.FunctionalTests.Properties; using GVFS.FunctionalTests.Should; +using GVFS.FunctionalTests.Tools; +using GVFS.Tests.Should; using NUnit.Framework; +using System.IO; namespace GVFS.FunctionalTests.Tests.GitCommands { @@ -30,6 +33,52 @@ public void ResetMixedAfterPrefetch() this.FilesShouldMatchCheckoutOfTargetBranch(); } + /// + /// A mixed reset must clear skip-worktree on a hydrated placeholder whose + /// index entry the reset changes. A hydrated placeholder is on disk but is + /// not in ModifiedPaths, so it still has skip-worktree. The reset does not + /// update the working tree, so the file on disk no longer matches the index. + /// If skip-worktree stays set, git does not compare the file to the index, + /// and reset and status do not report it as modified. + /// + /// Placeholders that are not on disk do not have this problem, because git + /// writes them to disk and clears skip-worktree. For that reason, this test + /// asserts each precondition before the reset. + /// + /// The git side of this behavior requires microsoft/git v2.55.0.vfs.0.3 or later. + /// + [TestCase] + public void ResetMixedClearsSkipWorktreeOnHydratedPlaceholder() + { + string filePath = Path.Combine("Test_ConflictTests", "ModifiedFiles", "ChangeInTarget.txt"); + string gitPath = filePath.Replace(Path.DirectorySeparatorChar, TestConstants.GitPathSeparator); + + // Create local branches for both commits in both repos. + this.ValidateGitCommand("checkout " + GitRepoTests.ConflictSourceBranch); + this.ValidateGitCommand("checkout " + GitRepoTests.ConflictTargetBranch); + + // Precondition: the reset changes the index entry for the file. + GitProcess.InvokeProcess( + this.ControlGitRepo.RootPath, + $"diff --quiet {GitRepoTests.ConflictSourceBranch} {GitRepoTests.ConflictTargetBranch} -- {gitPath}") + .ExitCode.ShouldEqual(1, $"{gitPath} must differ between the reset source and target"); + + // Precondition: the file is a hydrated placeholder. A read hydrates it + // but does not add it to ModifiedPaths, so skip-worktree stays set. + this.Enlistment.GetVirtualPathTo(filePath).ShouldBeAFile(this.FileSystem).WithContents(); + GVFSHelpers.ModifiedPathsShouldNotContain(this.Enlistment, this.FileSystem, gitPath); + this.SkipWorktreeFlagShouldBe(gitPath, expectedFlag: 'S'); + + // The reset output and status must report the file as modified, as in the control repo. + this.ValidateGitCommand("reset --mixed " + GitRepoTests.ConflictSourceBranch); + + // After the reset, skip-worktree is cleared, and GVFS adds the file to + // ModifiedPaths so that later git commands also compare it to the index. + this.SkipWorktreeFlagShouldBe(gitPath, expectedFlag: 'H'); + GVFSHelpers.ModifiedPathsShouldContain(this.Enlistment, this.FileSystem, gitPath); + this.FileContentsShouldMatch(filePath); + } + [TestCase] public void ResetMixedAndCheckoutNewBranch() { @@ -108,5 +157,13 @@ protected override void CreateEnlistment() this.ControlGitRepo.Fetch(GitRepoTests.ConflictTargetBranch); this.ControlGitRepo.Fetch(GitRepoTests.ConflictSourceBranch); } + + private void SkipWorktreeFlagShouldBe(string gitPath, char expectedFlag) + { + // "ls-files -v" prefixes each entry with a tag: "S" means skip-worktree is set, "H" means it is not. + ProcessResult result = GitProcess.InvokeProcess(this.Enlistment.RepoRoot, "ls-files -v -- " + gitPath); + result.ExitCode.ShouldEqual(0, result.Errors); + result.Output.Trim().ShouldEqual($"{expectedFlag} {gitPath}"); + } } } diff --git a/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs b/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs index 8bdc74f17..48418452d 100644 --- a/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs +++ b/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs @@ -130,6 +130,11 @@ private static string FilterMessages( return input; } + /// + /// Runs a git command in the control repo and in the GVFS repo, and asserts + /// that the output and errors match. If the command is not "status", this + /// method then runs "status" in both repos and compares that output too. + /// public static void ValidateGitCommand( GVFSFunctionalTestEnlistment enlistment, ControlGitRepo controlGitRepo, From 6dd6f632dfd8f401fa08080a0d52d50dd5f22eef Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 30 Sep 2026 12:21:28 -0700 Subject: [PATCH 08/11] Add IsSendingAnonymousRequests to VFS.Heartbeat Expose whether outgoing gvfs.exe requests are sent without an Authorization header via the periodic VFS.Heartbeat event, sourced from the enlistment's existing public GitAuthentication.IsAnonymous property. Application Insights shows a fleet failure mode where mounts get stuck unable to download blob sizes or objects (SizesUnavailableException), affecting roughly 6% of active machines with no repro in hand. The suspected cause is an anonymous-auth latch: when the initial anonymous config probe comes back indeterminate, GitAuthentication never clears IsAnonymous, so HttpRequestor permanently omits the Authorization header. Today no heartbeat or telemetry field exposes this state, so the mechanism cannot be confirmed or sized in the field. This change is intentionally scoped away from GitAuthentication.cs and HttpRequestor.cs, which are being reworked by unrelated in-flight fixes for the same bug. The new field is named for the observable effect on the wire (no Authorization header sent) rather than for the internal property, and reads the same way regardless of which build emits it, even though IsAnonymous's default value before the first auth probe completes differs between the current code and the in-flight fix. Assisted-by: Claude Sonnet 5 Signed-off-by: Tyrie Vella --- .../Virtualization/FileSystemCallbacksTests.cs | 7 +++++-- GVFS/GVFS.Virtualization/FileSystemCallbacks.cs | 6 ++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs b/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs index 3a515c3b5..7ab7b13de 100644 --- a/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs +++ b/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs @@ -119,6 +119,7 @@ public void GetMetadataForHeartBeatDoesNotChangeEventLevelWhenNoPlaceholderHaveB metadata.ShouldContain("ModifiedPathsCount", 1); metadata.ShouldContain("FilePlaceholderCount", 0); metadata.ShouldContain(nameof(RepoMetadata.Instance.EnlistmentId), RepoMetadata.Instance.EnlistmentId); + metadata.ShouldContain("IsSendingAnonymousRequests", true); } mockPlaceholderDb.VerifyAll(); @@ -159,7 +160,7 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelToInformationalWhenPlacehol eventLevel.ShouldEqual(EventLevel.Informational); // "ModifiedPathsCount" should be 1 because ".gitattributes" is always present - metadata.Count.ShouldEqual(8); + metadata.Count.ShouldEqual(9); metadata.ContainsKey("FilePlaceholderCreation").ShouldBeTrue(); metadata.TryGetValue("FilePlaceholderCreation", out object fileNestedMetadata); GVFSJsonOptions.Serialize(fileNestedMetadata).ShouldContain("\"ProcessName1\":\"GVFS.UnitTests.exe\""); @@ -169,6 +170,7 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelToInformationalWhenPlacehol metadata.ShouldContain("FolderPlaceholderCount", 0); metadata.ShouldContain(nameof(RepoMetadata.Instance.EnlistmentId), RepoMetadata.Instance.EnlistmentId); metadata.ContainsKey("PhysicalDiskInfo").ShouldBeTrue(); + metadata.ShouldContain("IsSendingAnonymousRequests", true); // Create more placeholders fileSystemCallbacks.OnPlaceholderFileCreated("test.txt", "2222233333444445555566666777778888899999", "GVFS.UnitTests.exe2"); @@ -182,7 +184,7 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelToInformationalWhenPlacehol eventLevel = writeToLogFile2 ? EventLevel.Informational : EventLevel.Verbose; eventLevel.ShouldEqual(EventLevel.Informational); - metadata.Count.ShouldEqual(8); + metadata.Count.ShouldEqual(9); // Only processes that have created placeholders since the last heartbeat should be named metadata.ContainsKey("FilePlaceholderCreation").ShouldBeTrue(); @@ -202,6 +204,7 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelToInformationalWhenPlacehol metadata.ShouldContain("FolderPlaceholderCount", 1); metadata.ShouldContain(nameof(RepoMetadata.Instance.EnlistmentId), RepoMetadata.Instance.EnlistmentId); metadata.ContainsKey("PhysicalDiskInfo").ShouldBeTrue(); + metadata.ShouldContain("IsSendingAnonymousRequests", true); } mockPlaceholderDb.VerifyAll(); diff --git a/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs b/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs index 91e627dd2..c9fc1cd2a 100644 --- a/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs +++ b/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs @@ -292,6 +292,12 @@ public EventMetadata GetAndResetHeartBeatMetadata(out bool logToFile) this.context.Enlistment.WorkingDirectoryBackingRoot, sizeStatsOnly: true)); + // True/false means "HttpRequestor is/isn't attaching an Authorization + // header to outgoing requests" (see HttpRequestor.cs), so a mount stuck + // reporting true for long stretches against a server that requires auth + // is the signature of the anonymous-auth-latch bug this field detects. + metadata.Add("IsSendingAnonymousRequests", this.context.Enlistment.Authentication.IsAnonymous); + return metadata; } From a6c33d23a9e7577937da80f87230b872c70fd78f Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 1 Oct 2026 10:59:42 -0700 Subject: [PATCH 09/11] Show credential prompts above other windows for background mounts Background GVFS processes such as GVFS.Mount.exe run with a hidden console window. When git launches the Git Credential Manager from such a process, the credential manager parents its interactive sign-in prompt to the hidden console. The prompt then has no foreground right and sinks behind other windows, so the user never sees it. Launch git detached from the console (CreateNoWindow=true) for commands that may require authentication when the current process has no visible console window. With no console, git starts the credential manager detached, so the credential manager parents its prompt to a topmost stub window instead of the hidden console. A process with a visible console window (e.g. gvfs.exe run from a classic console) keeps the current behavior, so the credential manager parents the prompt to that window. Detection keys on the console window, not an arbitrary GUI window. A process whose console is hidden, or a non-displayed pseudo-console such as a foreground gvfs.exe run from Windows Terminal, is treated as having no visible window and also detaches git. For the Git Credential Manager this is equivalent or better, because the detached prompt is topmost instead of owned by a hidden host window. Add GVFSPlatform.CurrentProcessHasVisibleConsoleWindow. The Windows implementation checks GetAncestor(GetConsoleWindow(), GA_ROOTOWNER) and IsWindowVisible. Add a mayRequireAuth option to GitProcess.GetGitProcess, which detaches git from a hidden console only when set. Only credential fill can trigger an interactive prompt, so only the url and certificate fills set mayRequireAuth. Credential approve and reject store or erase an already-obtained credential and never prompt, so they keep the default. Every other git command GVFS runs is local and keeps the default, so the per-child conhost cost stays avoided for every non-authenticating call. Log hasVisibleConsoleWindow from TryGetCredential so a credential fill that does not surface a prompt stays diagnosable from telemetry. Guard the native console probe so a binding failure keeps the previous behavior instead of aborting the fill. Add a certificate-fill test for the mayRequireAuth wiring. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/GVFSPlatform.cs | 14 +++ GVFS/GVFS.Common/Git/GitProcess.cs | 57 +++++++-- GVFS/GVFS.Platform.Windows/WindowsPlatform.cs | 30 +++++ .../Git/GitAuthenticationTests.cs | 110 ++++++++++++++++++ .../Mock/Common/MockPlatform.cs | 17 ++- .../GVFS.UnitTests/Mock/Git/MockGitProcess.cs | 11 +- 6 files changed, 223 insertions(+), 16 deletions(-) diff --git a/GVFS/GVFS.Common/GVFSPlatform.cs b/GVFS/GVFS.Common/GVFSPlatform.cs index 5b232628a..964b8d6ac 100644 --- a/GVFS/GVFS.Common/GVFSPlatform.cs +++ b/GVFS/GVFS.Common/GVFSPlatform.cs @@ -65,6 +65,20 @@ public static void Register(GVFSPlatform platform) /// public abstract void PrepareProcessToRunInBackground(); + /// + /// Returns true if the current process owns a visible console window that a child process + /// (e.g. git launching the credential manager) can use as the owner for an interactive prompt. + /// + /// + /// This inspects the current process's console window and its root owner, not arbitrary GUI + /// windows. Background processes such as GVFS.Mount run with a hidden console, so this returns + /// false for them; foreground processes launched from a visible console return true. Credential + /// helpers launched by a process with no visible console window should be started detached from + /// the console, so the credential manager parents its prompt to a topmost stub window instead + /// of the hidden console. + /// + public abstract bool CurrentProcessHasVisibleConsoleWindow(); + public abstract bool IsProcessActive(int processId); /// diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index 43a40ced0..9d0e80ea8 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -273,13 +273,14 @@ public virtual bool TryGetCertificatePassword( using (ITracer activity = tracer.StartActivity("TryGetCertificatePassword", EventLevel.Informational)) { - // See GetFromConfig for why pre-command hook is disabled - // for bootstrap-time git operations. + // See GetFromConfig for why the pre-command hook is disabled for + // bootstrap-time git operations. Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolder( "credential fill", stdin => stdin.Write("protocol=cert\npath=" + certificatePath + "\nusername=\n\n"), parseStdOutLine: null, - usePreCommandHook: false); + usePreCommandHook: false, + mayRequireAuth: true); if (gitCredentialOutput.ExitCodeIsFailure) { @@ -328,14 +329,15 @@ public virtual bool TryGetCredential( using (ITracer activity = tracer.StartActivity(nameof(this.TryGetCredential), EventLevel.Informational)) { - // See GetFromConfig for why pre-command hook is disabled - // for bootstrap-time git operations. + // See GetFromConfig for why the pre-command hook is disabled for + // bootstrap-time git operations. Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolder( GenerateCredentialVerbCommand("fill"), stdin => stdin.Write($"url={repoUrl}\n\n"), parseStdOutLine: null, usePreCommandHook: false, - timeoutMs: timeoutMs); + timeoutMs: timeoutMs, + mayRequireAuth: true); if (gitCredentialOutput.ExitCodeIsFailure) { @@ -361,6 +363,14 @@ public virtual bool TryGetCredential( return false; } + // Record whether the current process had a visible console window. GetGitProcess uses + // this to decide whether to detach git from a hidden console so the credential prompt is + // not left behind other windows; logging it keeps that decision diagnosable from telemetry + // when a prompt fails to appear. + EventMetadata consoleMetadata = new EventMetadata(); + consoleMetadata.Add("hasVisibleConsoleWindow", CurrentProcessHasVisibleConsoleWindowSafe()); + activity.RelatedInfo(consoleMetadata, "Credential fill console window visibility"); + username = ParseValue(gitCredentialOutput.Output, "username="); password = ParseValue(gitCredentialOutput.Output, "password="); @@ -902,7 +912,7 @@ public Result MultiPackIndexRepack(string gitObjectDirectory, string batchSize) return this.InvokeGitAgainstDotGitFolder($"-c pack.threads=1 -c repack.packKeptObjects=true multi-pack-index repack --object-dir=\"{gitObjectDirectory}\" --batch-size={batchSize} --no-progress"); } - public Process GetGitProcess(string command, string workingDirectory, string dotGitDirectory, bool useReadObjectHook, string gitObjectsDirectory, bool usePreCommandHook) + public Process GetGitProcess(string command, string workingDirectory, string dotGitDirectory, bool useReadObjectHook, string gitObjectsDirectory, bool usePreCommandHook, bool mayRequireAuth = false) { ProcessStartInfo processInfo = new ProcessStartInfo(this.gitBinPath); processInfo.WorkingDirectory = workingDirectory; @@ -919,7 +929,13 @@ public Process GetGitProcess(string command, string workingDirectory, string dot // instead), CreateNoWindow must be set to true for that case — otherwise // the non-redirected stream inherits the parent's console handle, which // may be absent when running as a service, causing lost output. - processInfo.CreateNoWindow = false; + // + // When a command may require authentication, git can launch the credential + // manager. Detaching from a hidden console (CreateNoWindow=true) makes that + // prompt appear on top instead of behind other windows. See + // GVFSPlatform.CurrentProcessHasVisibleConsoleWindow for the full rationale. + bool hasVisibleConsoleWindow = !mayRequireAuth || CurrentProcessHasVisibleConsoleWindowSafe(); + processInfo.CreateNoWindow = mayRequireAuth && !hasVisibleConsoleWindow; processInfo.StandardOutputEncoding = UTF8NoBOM; processInfo.StandardErrorEncoding = UTF8NoBOM; @@ -986,7 +1002,8 @@ protected virtual Result InvokeGitImpl( Action parseStdOutLine, int timeoutMs, string gitObjectsDirectory = null, - bool usePreCommandHook = true) + bool usePreCommandHook = true, + bool mayRequireAuth = false) { if (failedToSetEncoding && writeStdIn != null) { @@ -998,7 +1015,7 @@ protected virtual Result InvokeGitImpl( // From https://msdn.microsoft.com/en-us/library/system.diagnostics.process.standardoutput.aspx // To avoid deadlocks, use asynchronous read operations on at least one of the streams. // Do not perform a synchronous read to the end of both redirected streams. - using (this.executingProcess = this.GetGitProcess(command, workingDirectory, dotGitDirectory, useReadObjectHook, gitObjectsDirectory: gitObjectsDirectory, usePreCommandHook: usePreCommandHook)) + using (this.executingProcess = this.GetGitProcess(command, workingDirectory, dotGitDirectory, useReadObjectHook, gitObjectsDirectory: gitObjectsDirectory, usePreCommandHook: usePreCommandHook, mayRequireAuth: mayRequireAuth)) { // Bound how much stdout/stderr we buffer so a pathologically noisy git command // cannot grow these buffers without limit until GVFS.Mount hits an @@ -1086,6 +1103,20 @@ private static string GenerateCredentialVerbCommand(string verb) return $"-c {GitConfigSetting.CredentialUseHttpPath}=true credential {verb}"; } + private static bool CurrentProcessHasVisibleConsoleWindowSafe() + { + try + { + return GVFSPlatform.Instance.CurrentProcessHasVisibleConsoleWindow(); + } + catch (Exception ex) when (ex is DllNotFoundException || ex is EntryPointNotFoundException) + { + // If the native console probe cannot run, assume a visible console so callers keep + // the previous behavior (CreateNoWindow=false) rather than aborting the command. + return true; + } + } + private static string ParseValue(string contents, string prefix) { int startIndex = contents.IndexOf(prefix) + prefix.Length; @@ -1166,7 +1197,8 @@ private Result InvokeGitAgainstDotGitFolder( Action parseStdOutLine, bool usePreCommandHook = true, string gitObjectsDirectory = null, - int timeoutMs = -1) + int timeoutMs = -1, + bool mayRequireAuth = false) { // This git command should not need/use the working directory of the repo. // Run git.exe in Environment.SystemDirectory to ensure the git.exe process @@ -1180,7 +1212,8 @@ private Result InvokeGitAgainstDotGitFolder( parseStdOutLine: parseStdOutLine, timeoutMs: timeoutMs, gitObjectsDirectory: gitObjectsDirectory, - usePreCommandHook: usePreCommandHook); + usePreCommandHook: usePreCommandHook, + mayRequireAuth: mayRequireAuth); } public class Result diff --git a/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs b/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs index 91da42f76..a27c98083 100644 --- a/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs +++ b/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs @@ -11,6 +11,7 @@ using System.IO; using System.IO.Pipes; using System.Linq; +using System.Runtime.InteropServices; using System.Security.AccessControl; using System.Security.Principal; using System.ServiceProcess; @@ -28,6 +29,9 @@ public partial class WindowsPlatform : GVFSPlatform private const string ProfileImagePathRegistryValue = "ProfileImagePath"; private const string AdminProtectionProfilePrefix = "ADMIN_"; + // GetAncestor flag GA_ROOTOWNER (3): walk the owner chain to the root owned window. + private const uint GA_ROOTOWNER = 3; + public WindowsPlatform() : base(underConstruction: new UnderConstructionFlags()) { } @@ -224,6 +228,22 @@ public override void PrepareProcessToRunInBackground() // No additional work required } + public override bool CurrentProcessHasVisibleConsoleWindow() + { + IntPtr consoleWindow = GetConsoleWindow(); + if (consoleWindow == IntPtr.Zero) + { + // The process has no console at all (e.g. a GUI process), so there is no console + // window to own an interactive prompt. + return false; + } + + // GVFS.Mount runs with a hidden console, so its console window exists but its root + // owner is not visible. Only treat the window as usable when its root owner is visible. + IntPtr rootOwner = GetAncestor(consoleWindow, GA_ROOTOWNER); + return rootOwner != IntPtr.Zero && IsWindowVisible(rootOwner); + } + public override NamedPipeServerStream CreatePipeByName(string pipeName) { PipeSecurity security = new PipeSecurity(); @@ -458,6 +478,16 @@ private static object GetValueFromRegistry(RegistryHive registryHive, string key return value; } + [DllImport("kernel32.dll")] + private static extern IntPtr GetConsoleWindow(); + + [DllImport("user32.dll")] + private static extern IntPtr GetAncestor(IntPtr hwnd, uint gaFlags); + + [DllImport("user32.dll")] + [return: MarshalAs(UnmanagedType.Bool)] + private static extern bool IsWindowVisible(IntPtr hWnd); + public class WindowsPlatformConstants : GVFSPlatformConstants { public override string ExecutableExtension diff --git a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs index 25aa675b7..83affa8a3 100644 --- a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs @@ -1,7 +1,9 @@ using System; +using System.Diagnostics; using System.Linq; using System.Threading; using System.Threading.Tasks; +using GVFS.Common; using GVFS.Common.Git; using GVFS.Tests; using GVFS.Tests.Should; @@ -342,6 +344,114 @@ public void TryGetCredentialsWaitsForBackgroundInitializationThenSucceeds() authString.ShouldNotBeNull("A credential string should be returned"); } + [TestCase] + public void CredentialFillIsFlaggedAsMayRequireAuth() + { + // Only credential fill can trigger an interactive sign-in from the credential manager, so + // only it is flagged mayRequireAuth=true. GetGitProcess uses that flag to detach git from a + // hidden console so the prompt is not left behind other windows. + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + dut.TryGetCredentials(tracer, out _, out string error).ShouldEqual(true, "Failed to get credential: " + error); + + gitProcess.MayRequireAuthByCommand[$"{AzureDevOpsUseHttpPathString} credential fill"] + .ShouldEqual(true, "Credential fill should be flagged as possibly requiring auth"); + + // Non-credential git commands never prompt, so they must not be flagged. + gitProcess.MayRequireAuthByCommand["config --get-urlmatch http mock://repoUrl"] + .ShouldEqual(false, "Non-credential git commands should not be flagged as requiring auth"); + } + + [TestCase] + public void CredentialApproveIsNotFlaggedAsMayRequireAuth() + { + // Credential approve stores an already-obtained credential and never prompts. + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + dut.TryGetCredentials(tracer, out string authString, out string error).ShouldEqual(true, "Failed to get credential: " + error); + dut.ApproveCredentials(tracer, authString); + + gitProcess.MayRequireAuthByCommand[$"{AzureDevOpsUseHttpPathString} credential approve"] + .ShouldEqual(false, "Credential approve should not be flagged as requiring auth"); + } + + [TestCase] + public void CredentialRejectIsNotFlaggedAsMayRequireAuth() + { + // Credential reject erases a stored credential and never prompts. + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + dut.TryGetCredentials(tracer, out string authString, out string error).ShouldEqual(true, "Failed to get credential: " + error); + dut.RejectCredentials(tracer, authString); + + gitProcess.MayRequireAuthByCommand[$"{AzureDevOpsUseHttpPathString} credential reject"] + .ShouldEqual(false, "Credential reject should not be flagged as requiring auth"); + } + + [TestCase] + public void CertificateCredentialFillIsFlaggedAsMayRequireAuth() + { + // The certificate credential fill can also trigger an interactive sign-in, so it is flagged + // mayRequireAuth=true like the URL fill. It is covered directly here because the standard + // credential flow does not exercise the certificate path. + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = new MockGitProcess(); + gitProcess.SetExpectedCommandResult( + "credential fill", + () => new GitProcess.Result("password=certpassword\r\n", string.Empty, GitProcess.Result.SuccessCode)); + + gitProcess.TryGetCertificatePassword(tracer, CertificatePath, out string password, out string error) + .ShouldEqual(true, "Failed to get certificate password: " + error); + + gitProcess.MayRequireAuthByCommand["credential fill"] + .ShouldEqual(true, "Certificate credential fill should be flagged as possibly requiring auth"); + } + + [TestCase] + public void GetGitProcessDetachesFromConsoleOnlyWhenAuthMayBeRequiredAndNoVisibleWindow() + { + MockPlatform platform = (MockPlatform)GVFSPlatform.Instance; + bool originalHasVisibleWindow = platform.HasVisibleWindow; + try + { + GitProcess gitProcess = new GitProcess("git.exe", workingDirectoryRoot: null); + + // No visible console + command may require auth => detach so the prompt is topmost. + platform.HasVisibleWindow = false; + using (Process process = gitProcess.GetGitProcess("credential fill", Environment.SystemDirectory, dotGitDirectory: null, useReadObjectHook: false, gitObjectsDirectory: null, usePreCommandHook: false, mayRequireAuth: true)) + { + process.StartInfo.CreateNoWindow.ShouldEqual(true, "Should detach when auth may be required and there is no visible console window"); + } + + // Visible console + command may require auth => keep the console so the prompt parents to it. + platform.HasVisibleWindow = true; + using (Process process = gitProcess.GetGitProcess("credential fill", Environment.SystemDirectory, dotGitDirectory: null, useReadObjectHook: false, gitObjectsDirectory: null, usePreCommandHook: false, mayRequireAuth: true)) + { + process.StartInfo.CreateNoWindow.ShouldEqual(false, "Should keep the console when it is visible"); + } + + // Command cannot require auth => never detach, regardless of console visibility. + platform.HasVisibleWindow = false; + using (Process process = gitProcess.GetGitProcess("rev-parse HEAD", Environment.SystemDirectory, dotGitDirectory: null, useReadObjectHook: false, gitObjectsDirectory: null, usePreCommandHook: true, mayRequireAuth: false)) + { + process.StartInfo.CreateNoWindow.ShouldEqual(false, "Should never detach when auth cannot be required"); + } + } + finally + { + platform.HasVisibleWindow = originalHasVisibleWindow; + } + } + private MockGitProcess GetGitProcess() { MockGitProcess gitProcess = new MockGitProcess(); diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs index 3ee7425de..c3f38b885 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs @@ -21,6 +21,12 @@ public class MockPlatform : GVFSPlatform public string MockCurrentUser { get; set; } + /// + /// Controls the return value of . Defaults to true + /// so tests keep the foreground behavior unless they opt into the background-process case. + /// + public bool HasVisibleWindow { get; set; } = true; + public override IKernelDriver KernelDriver => throw new NotSupportedException(); public override IGitInstallation GitInstallation { get; } = new MockGitInstallation(); @@ -195,9 +201,14 @@ public override Process StartBackgroundVFS4GProcess(ITracer tracer, string progr throw new NotSupportedException(); } - public override void PrepareProcessToRunInBackground() - { - throw new NotSupportedException(); + public override void PrepareProcessToRunInBackground() + { + throw new NotSupportedException(); + } + + public override bool CurrentProcessHasVisibleConsoleWindow() + { + return this.HasVisibleWindow; } public override bool IsGitStatusCacheSupported() diff --git a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs index c9095cc2c..f37cebba8 100644 --- a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs +++ b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs @@ -18,12 +18,19 @@ public MockGitProcess() : base(new MockGVFSEnlistment()) { this.CommandsRun = new List(); + this.MayRequireAuthByCommand = new Dictionary(StringComparer.Ordinal); this.StoredCredentials = new Dictionary(StringComparer.OrdinalIgnoreCase); this.CredentialApprovals = new Dictionary>(); this.CredentialRejections = new Dictionary>(); } public List CommandsRun { get; } + + /// + /// Records the mayRequireAuth value passed to for each command, so + /// tests can assert that only credential-fill commands are flagged as possibly requiring auth. + /// + public Dictionary MayRequireAuthByCommand { get; } public bool ShouldFail { get; set; } public Dictionary StoredCredentials { get; } public Dictionary> CredentialApprovals { get; } @@ -84,9 +91,11 @@ protected override Result InvokeGitImpl( Action parseStdOutLine, int timeoutMs, string gitObjectsDirectory = null, - bool usePrecommandHook = true) + bool usePrecommandHook = true, + bool mayRequireAuth = false) { this.CommandsRun.Add(command); + this.MayRequireAuthByCommand[command] = mayRequireAuth; if (this.ShouldFail) { From d274d5160933fef1624f5089a141ea5ac5013498 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 6 Oct 2026 14:33:04 +0000 Subject: [PATCH 10/11] Bump actions/upload-artifact from 7 to 7 Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 7 to 7. - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](https://github.com/actions/upload-artifact/compare/v7...043fb46d1a93c77aae656e7c1c64a875d1fc6a0a) --- updated-dependencies: - dependency-name: actions/upload-artifact dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- .github/workflows/functional-tests.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/functional-tests.yaml b/.github/workflows/functional-tests.yaml index 7274f9765..110e3c14b 100644 --- a/.github/workflows/functional-tests.yaml +++ b/.github/workflows/functional-tests.yaml @@ -209,7 +209,7 @@ jobs: - name: Upload failure diagnostics (mount dumps + logs) if: always() && steps.skip.outputs.result != 'true' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 continue-on-error: true with: name: ${{ env.ARTIFACT_PREFIX }}FailureDiagnostics_${{ env.FT_MATRIX_NAME }} From 58edd7c8b277f3b289a84bbd0a167fed285d03a0 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 7 Oct 2026 11:12:40 -0700 Subject: [PATCH 11/11] Make credential-fill console-visibility telemetry authoritative and log it on failure Address review feedback on the credential-prompt console-detach change. - Probe CurrentProcessHasVisibleConsoleWindow once per credential-fill call and thread the value through InvokeGitAgainstDotGitFolder, InvokeGitImpl, and GetGitProcess so the logged value and the CreateNoWindow decision are the same probe. GetGitProcess treats a supplied value as authoritative and only probes when none is passed. - Log hasVisibleConsoleWindow on the credential-fill failure path (including the timeout a hidden, unanswered prompt produces), not only on success. The success-path metadata and the certificate-fill success and failure paths log the same value. - Remove the misnamed intermediate local in GetGitProcess; compute CreateNoWindow directly so the && short-circuits the probe when auth cannot be required. - Restore the original pre-command-hook comment wording. - MockPlatform gains ThrowOnConsoleProbe to simulate a native probe that cannot run; add tests for the safe fallback, the caller-supplied authoritative value, and console-visibility telemetry on the URL and certificate credential-fill failure paths. The telemetry tests assert the logged value, not just the key. - MockGitProcess records each git invocation (command, mayRequireAuth, and hasVisibleConsoleWindow) instead of keeping only the last value per command, so the wiring assertions cannot be fooled if a command runs twice with different flags. The failure-telemetry tests also assert the probed console-visibility value threads down to the git invocation. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/GitProcess.cs | 53 +++++++--- .../Git/GitAuthenticationTests.cs | 97 ++++++++++++++++++- .../Mock/Common/MockPlatform.cs | 17 +++- .../GVFS.UnitTests/Mock/Git/MockGitProcess.cs | 46 +++++++-- 4 files changed, 184 insertions(+), 29 deletions(-) diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index 9d0e80ea8..df54900b7 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -273,19 +273,27 @@ public virtual bool TryGetCertificatePassword( using (ITracer activity = tracer.StartActivity("TryGetCertificatePassword", EventLevel.Informational)) { - // See GetFromConfig for why the pre-command hook is disabled for - // bootstrap-time git operations. + // Record whether the current process has a visible console window, using the same value + // GetGitProcess uses to decide whether to detach git from a hidden console. Capturing it + // once here keeps the logged value authoritative and diagnosable on both the success and + // failure paths when a credential prompt fails to appear. + bool hasVisibleConsoleWindow = CurrentProcessHasVisibleConsoleWindowSafe(); + + // See GetFromConfig for why pre-command hook is disabled + // for bootstrap-time git operations. Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolder( "credential fill", stdin => stdin.Write("protocol=cert\npath=" + certificatePath + "\nusername=\n\n"), parseStdOutLine: null, usePreCommandHook: false, - mayRequireAuth: true); + mayRequireAuth: true, + hasVisibleConsoleWindow: hasVisibleConsoleWindow); if (gitCredentialOutput.ExitCodeIsFailure) { EventMetadata errorData = new EventMetadata(); errorData.Add("CertificatePath", certificatePath); + errorData.Add("hasVisibleConsoleWindow", hasVisibleConsoleWindow); tracer.RelatedWarning( errorData, "Git could not get credentials: " + gitCredentialOutput.Errors, @@ -302,7 +310,8 @@ public virtual bool TryGetCertificatePassword( EventMetadata metadata = new EventMetadata { { "Success", success }, - { "CertificatePath", certificatePath } + { "CertificatePath", certificatePath }, + { "hasVisibleConsoleWindow", hasVisibleConsoleWindow } }; if (!success) @@ -329,19 +338,27 @@ public virtual bool TryGetCredential( using (ITracer activity = tracer.StartActivity(nameof(this.TryGetCredential), EventLevel.Informational)) { - // See GetFromConfig for why the pre-command hook is disabled for - // bootstrap-time git operations. + // Record whether the current process has a visible console window, using the same value + // GetGitProcess uses to decide whether to detach git from a hidden console. Capturing it + // once here keeps the logged value authoritative and diagnosable on both the success and + // failure paths (including the timeout that a hidden, unanswered prompt produces). + bool hasVisibleConsoleWindow = CurrentProcessHasVisibleConsoleWindowSafe(); + + // See GetFromConfig for why pre-command hook is disabled + // for bootstrap-time git operations. Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolder( GenerateCredentialVerbCommand("fill"), stdin => stdin.Write($"url={repoUrl}\n\n"), parseStdOutLine: null, usePreCommandHook: false, timeoutMs: timeoutMs, - mayRequireAuth: true); + mayRequireAuth: true, + hasVisibleConsoleWindow: hasVisibleConsoleWindow); if (gitCredentialOutput.ExitCodeIsFailure) { EventMetadata errorData = new EventMetadata(); + errorData.Add("hasVisibleConsoleWindow", hasVisibleConsoleWindow); if (gitCredentialOutput.Errors.StartsWith("Operation timed out")) { @@ -368,7 +385,7 @@ public virtual bool TryGetCredential( // not left behind other windows; logging it keeps that decision diagnosable from telemetry // when a prompt fails to appear. EventMetadata consoleMetadata = new EventMetadata(); - consoleMetadata.Add("hasVisibleConsoleWindow", CurrentProcessHasVisibleConsoleWindowSafe()); + consoleMetadata.Add("hasVisibleConsoleWindow", hasVisibleConsoleWindow); activity.RelatedInfo(consoleMetadata, "Credential fill console window visibility"); username = ParseValue(gitCredentialOutput.Output, "username="); @@ -912,7 +929,7 @@ public Result MultiPackIndexRepack(string gitObjectDirectory, string batchSize) return this.InvokeGitAgainstDotGitFolder($"-c pack.threads=1 -c repack.packKeptObjects=true multi-pack-index repack --object-dir=\"{gitObjectDirectory}\" --batch-size={batchSize} --no-progress"); } - public Process GetGitProcess(string command, string workingDirectory, string dotGitDirectory, bool useReadObjectHook, string gitObjectsDirectory, bool usePreCommandHook, bool mayRequireAuth = false) + public Process GetGitProcess(string command, string workingDirectory, string dotGitDirectory, bool useReadObjectHook, string gitObjectsDirectory, bool usePreCommandHook, bool mayRequireAuth = false, bool? hasVisibleConsoleWindow = null) { ProcessStartInfo processInfo = new ProcessStartInfo(this.gitBinPath); processInfo.WorkingDirectory = workingDirectory; @@ -934,8 +951,11 @@ public Process GetGitProcess(string command, string workingDirectory, string dot // manager. Detaching from a hidden console (CreateNoWindow=true) makes that // prompt appear on top instead of behind other windows. See // GVFSPlatform.CurrentProcessHasVisibleConsoleWindow for the full rationale. - bool hasVisibleConsoleWindow = !mayRequireAuth || CurrentProcessHasVisibleConsoleWindowSafe(); - processInfo.CreateNoWindow = mayRequireAuth && !hasVisibleConsoleWindow; + // A caller that already probed the console visibility passes the result in + // hasVisibleConsoleWindow so the logged and acted-on value stay identical; + // otherwise probe here. The && short-circuits the probe when auth cannot + // be required. + processInfo.CreateNoWindow = mayRequireAuth && !(hasVisibleConsoleWindow ?? CurrentProcessHasVisibleConsoleWindowSafe()); processInfo.StandardOutputEncoding = UTF8NoBOM; processInfo.StandardErrorEncoding = UTF8NoBOM; @@ -1003,7 +1023,8 @@ protected virtual Result InvokeGitImpl( int timeoutMs, string gitObjectsDirectory = null, bool usePreCommandHook = true, - bool mayRequireAuth = false) + bool mayRequireAuth = false, + bool? hasVisibleConsoleWindow = null) { if (failedToSetEncoding && writeStdIn != null) { @@ -1015,7 +1036,7 @@ protected virtual Result InvokeGitImpl( // From https://msdn.microsoft.com/en-us/library/system.diagnostics.process.standardoutput.aspx // To avoid deadlocks, use asynchronous read operations on at least one of the streams. // Do not perform a synchronous read to the end of both redirected streams. - using (this.executingProcess = this.GetGitProcess(command, workingDirectory, dotGitDirectory, useReadObjectHook, gitObjectsDirectory: gitObjectsDirectory, usePreCommandHook: usePreCommandHook, mayRequireAuth: mayRequireAuth)) + using (this.executingProcess = this.GetGitProcess(command, workingDirectory, dotGitDirectory, useReadObjectHook, gitObjectsDirectory: gitObjectsDirectory, usePreCommandHook: usePreCommandHook, mayRequireAuth: mayRequireAuth, hasVisibleConsoleWindow: hasVisibleConsoleWindow)) { // Bound how much stdout/stderr we buffer so a pathologically noisy git command // cannot grow these buffers without limit until GVFS.Mount hits an @@ -1198,7 +1219,8 @@ private Result InvokeGitAgainstDotGitFolder( bool usePreCommandHook = true, string gitObjectsDirectory = null, int timeoutMs = -1, - bool mayRequireAuth = false) + bool mayRequireAuth = false, + bool? hasVisibleConsoleWindow = null) { // This git command should not need/use the working directory of the repo. // Run git.exe in Environment.SystemDirectory to ensure the git.exe process @@ -1213,7 +1235,8 @@ private Result InvokeGitAgainstDotGitFolder( timeoutMs: timeoutMs, gitObjectsDirectory: gitObjectsDirectory, usePreCommandHook: usePreCommandHook, - mayRequireAuth: mayRequireAuth); + mayRequireAuth: mayRequireAuth, + hasVisibleConsoleWindow: hasVisibleConsoleWindow); } public class Result diff --git a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs index 83affa8a3..682a80537 100644 --- a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs @@ -357,11 +357,11 @@ public void CredentialFillIsFlaggedAsMayRequireAuth() dut.TryGetCredentials(tracer, out _, out string error).ShouldEqual(true, "Failed to get credential: " + error); - gitProcess.MayRequireAuthByCommand[$"{AzureDevOpsUseHttpPathString} credential fill"] + gitProcess.MayRequireAuthFor($"{AzureDevOpsUseHttpPathString} credential fill") .ShouldEqual(true, "Credential fill should be flagged as possibly requiring auth"); // Non-credential git commands never prompt, so they must not be flagged. - gitProcess.MayRequireAuthByCommand["config --get-urlmatch http mock://repoUrl"] + gitProcess.MayRequireAuthFor("config --get-urlmatch http mock://repoUrl") .ShouldEqual(false, "Non-credential git commands should not be flagged as requiring auth"); } @@ -377,7 +377,7 @@ public void CredentialApproveIsNotFlaggedAsMayRequireAuth() dut.TryGetCredentials(tracer, out string authString, out string error).ShouldEqual(true, "Failed to get credential: " + error); dut.ApproveCredentials(tracer, authString); - gitProcess.MayRequireAuthByCommand[$"{AzureDevOpsUseHttpPathString} credential approve"] + gitProcess.MayRequireAuthFor($"{AzureDevOpsUseHttpPathString} credential approve") .ShouldEqual(false, "Credential approve should not be flagged as requiring auth"); } @@ -393,7 +393,7 @@ public void CredentialRejectIsNotFlaggedAsMayRequireAuth() dut.TryGetCredentials(tracer, out string authString, out string error).ShouldEqual(true, "Failed to get credential: " + error); dut.RejectCredentials(tracer, authString); - gitProcess.MayRequireAuthByCommand[$"{AzureDevOpsUseHttpPathString} credential reject"] + gitProcess.MayRequireAuthFor($"{AzureDevOpsUseHttpPathString} credential reject") .ShouldEqual(false, "Credential reject should not be flagged as requiring auth"); } @@ -412,7 +412,7 @@ public void CertificateCredentialFillIsFlaggedAsMayRequireAuth() gitProcess.TryGetCertificatePassword(tracer, CertificatePath, out string password, out string error) .ShouldEqual(true, "Failed to get certificate password: " + error); - gitProcess.MayRequireAuthByCommand["credential fill"] + gitProcess.MayRequireAuthFor("credential fill") .ShouldEqual(true, "Certificate credential fill should be flagged as possibly requiring auth"); } @@ -445,6 +445,93 @@ public void GetGitProcessDetachesFromConsoleOnlyWhenAuthMayBeRequiredAndNoVisibl { process.StartInfo.CreateNoWindow.ShouldEqual(false, "Should never detach when auth cannot be required"); } + + // Native console probe cannot run => safe fallback assumes a visible console, so do not detach. + platform.HasVisibleWindow = false; + platform.ThrowOnConsoleProbe = true; + using (Process process = gitProcess.GetGitProcess("credential fill", Environment.SystemDirectory, dotGitDirectory: null, useReadObjectHook: false, gitObjectsDirectory: null, usePreCommandHook: false, mayRequireAuth: true)) + { + process.StartInfo.CreateNoWindow.ShouldEqual(false, "Should not detach when the console probe cannot run"); + } + + // A caller-supplied console-visibility value is authoritative and bypasses the platform probe + // (the probe would throw if consulted here). + platform.ThrowOnConsoleProbe = true; + platform.HasVisibleWindow = true; + using (Process process = gitProcess.GetGitProcess("credential fill", Environment.SystemDirectory, dotGitDirectory: null, useReadObjectHook: false, gitObjectsDirectory: null, usePreCommandHook: false, mayRequireAuth: true, hasVisibleConsoleWindow: false)) + { + process.StartInfo.CreateNoWindow.ShouldEqual(true, "A caller-supplied false should force detach without probing the platform"); + } + } + finally + { + platform.HasVisibleWindow = originalHasVisibleWindow; + platform.ThrowOnConsoleProbe = false; + } + } + + [TestCase] + public void TryGetCredentialLogsConsoleVisibilityWhenCredentialFillFails() + { + // The console-visibility telemetry must be recorded on the failure path too, because the + // hidden-prompt timeout that this diagnostic exists to explain is itself a failure result. + MockPlatform platform = (MockPlatform)GVFSPlatform.Instance; + bool originalHasVisibleWindow = platform.HasVisibleWindow; + try + { + platform.HasVisibleWindow = false; + + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = new MockGitProcess(); + gitProcess.SetExpectedCommandResult( + $"{AzureDevOpsUseHttpPathString} credential fill", + () => new GitProcess.Result(string.Empty, "Operation timed out waiting for a response", GitProcess.Result.GenericFailureCode)); + + gitProcess.TryGetCredential(tracer, "mock://repoUrl", out string username, out string password, out string error, timeoutMs: 5000) + .ShouldEqual(false, "Credential fill should report failure when git times out"); + + // Assert the logged value, not just the key: the diagnostic is only useful if it records + // the actual console-visibility state (false here) that explains the hidden-prompt timeout. + tracer.RelatedWarningEvents.ShouldContain(e => e.Contains("\"hasVisibleConsoleWindow\":false")); + + // The probed value must thread all the way down to the git invocation that GetGitProcess uses. + gitProcess.InvocationsRun + .Single(invocation => invocation.Command == $"{AzureDevOpsUseHttpPathString} credential fill") + .HasVisibleConsoleWindow.ShouldEqual(false, "The probed console-visibility value must thread down to the git invocation"); + } + finally + { + platform.HasVisibleWindow = originalHasVisibleWindow; + } + } + + [TestCase] + public void TryGetCertificatePasswordLogsConsoleVisibilityWhenCredentialFillFails() + { + // The certificate credential fill stamps the same console-visibility telemetry as the URL fill. + // It is covered separately because the standard credential flow does not exercise the certificate + // path, and the value must be present on the failure path where a hidden prompt times out. + MockPlatform platform = (MockPlatform)GVFSPlatform.Instance; + bool originalHasVisibleWindow = platform.HasVisibleWindow; + try + { + platform.HasVisibleWindow = false; + + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = new MockGitProcess(); + gitProcess.SetExpectedCommandResult( + "credential fill", + () => new GitProcess.Result(string.Empty, "Operation timed out waiting for a response", GitProcess.Result.GenericFailureCode)); + + gitProcess.TryGetCertificatePassword(tracer, CertificatePath, out string password, out string error) + .ShouldEqual(false, "Certificate password fill should report failure when git times out"); + + tracer.RelatedWarningEvents.ShouldContain(e => e.Contains("\"hasVisibleConsoleWindow\":false")); + + // The probed value must thread all the way down to the git invocation that GetGitProcess uses. + gitProcess.InvocationsRun + .Single(invocation => invocation.Command == "credential fill") + .HasVisibleConsoleWindow.ShouldEqual(false, "The probed console-visibility value must thread down to the git invocation"); } finally { diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs index c3f38b885..a5c92b0c9 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs @@ -27,6 +27,12 @@ public class MockPlatform : GVFSPlatform /// public bool HasVisibleWindow { get; set; } = true; + /// + /// When true, throws to simulate a native + /// console probe that cannot run (missing entry point), so tests can cover the safe fallback. + /// + public bool ThrowOnConsoleProbe { get; set; } + public override IKernelDriver KernelDriver => throw new NotSupportedException(); public override IGitInstallation GitInstallation { get; } = new MockGitInstallation(); @@ -201,13 +207,18 @@ public override Process StartBackgroundVFS4GProcess(ITracer tracer, string progr throw new NotSupportedException(); } - public override void PrepareProcessToRunInBackground() - { - throw new NotSupportedException(); + public override void PrepareProcessToRunInBackground() + { + throw new NotSupportedException(); } public override bool CurrentProcessHasVisibleConsoleWindow() { + if (this.ThrowOnConsoleProbe) + { + throw new EntryPointNotFoundException(); + } + return this.HasVisibleWindow; } diff --git a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs index f37cebba8..cf786a983 100644 --- a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs +++ b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs @@ -18,7 +18,7 @@ public MockGitProcess() : base(new MockGVFSEnlistment()) { this.CommandsRun = new List(); - this.MayRequireAuthByCommand = new Dictionary(StringComparer.Ordinal); + this.InvocationsRun = new List(); this.StoredCredentials = new Dictionary(StringComparer.OrdinalIgnoreCase); this.CredentialApprovals = new Dictionary>(); this.CredentialRejections = new Dictionary>(); @@ -27,15 +27,34 @@ public MockGitProcess() public List CommandsRun { get; } /// - /// Records the mayRequireAuth value passed to for each command, so - /// tests can assert that only credential-fill commands are flagged as possibly requiring auth. + /// Records every call into in order, with the mayRequireAuth and + /// hasVisibleConsoleWindow values passed to it. Recording each invocation (rather than keeping only + /// the last value per command) keeps the wiring assertions honest if a command is ever run more than + /// once with different flags. /// - public Dictionary MayRequireAuthByCommand { get; } + public List InvocationsRun { get; } public bool ShouldFail { get; set; } public Dictionary StoredCredentials { get; } public Dictionary> CredentialApprovals { get; } public Dictionary> CredentialRejections { get; } + /// + /// Returns the mayRequireAuth value the given command was invoked with, asserting the command ran and + /// that every invocation of it agreed on the value. + /// + public bool MayRequireAuthFor(string command) + { + List matches = this.InvocationsRun + .Where(invocation => string.Equals(invocation.Command, command, StringComparison.Ordinal)) + .ToList(); + + matches.Count.ShouldBeAtLeast(1, "Command was never run: " + command); + matches.Select(invocation => invocation.MayRequireAuth).Distinct().Count() + .ShouldEqual(1, "Command ran with conflicting mayRequireAuth values: " + command); + + return matches[0].MayRequireAuth; + } + public void SetExpectedCommandResult(string command, Func result, bool matchPrefix = false) { CommandInfo commandInfo = new CommandInfo(command, result, matchPrefix); @@ -92,10 +111,11 @@ protected override Result InvokeGitImpl( int timeoutMs, string gitObjectsDirectory = null, bool usePrecommandHook = true, - bool mayRequireAuth = false) + bool mayRequireAuth = false, + bool? hasVisibleConsoleWindow = null) { this.CommandsRun.Add(command); - this.MayRequireAuthByCommand[command] = mayRequireAuth; + this.InvocationsRun.Add(new GitInvocation(command, mayRequireAuth, hasVisibleConsoleWindow)); if (this.ShouldFail) { @@ -134,6 +154,20 @@ protected override Result InvokeGitImpl( return result; } + public class GitInvocation + { + public GitInvocation(string command, bool mayRequireAuth, bool? hasVisibleConsoleWindow) + { + this.Command = command; + this.MayRequireAuth = mayRequireAuth; + this.HasVisibleConsoleWindow = hasVisibleConsoleWindow; + } + + public string Command { get; } + public bool MayRequireAuth { get; } + public bool? HasVisibleConsoleWindow { get; } + } + public class Credential { public Credential(string username, string password)