From 4b7034866108ea80f695708a9cd768360f8d241d Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 8 Apr 2026 16:40:24 -0700 Subject: [PATCH] PostIndexChangedHook: skip notification for non-canonical indexes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Git fires the post-index-change hook for every index write, including temp indexes created via GIT_INDEX_FILE redirect (e.g. read-tree --index-output, git add with a temp index). The GVFS mount process only needs to know about changes to the real `\$GIT_DIR/index`. When writing a 194MB temp index (2.47M entries), the hook's pipe round-trip to the GVFS mount process adds ~5s of overhead per read-tree/add call — a significant regression for tools that use the temp-index flow. Add an early-exit check in IsNonCanonicalIndex() that resolves both GIT_INDEX_FILE and `\$GIT_DIR/index` to absolute paths via GetFullPathNameA, then compares case-insensitively. If they differ, the hook exits immediately without contacting the mount process. When the environment is unexpected (GIT_DIR absent, path resolution fails), we err on the side of correctness and proceed with the notification rather than silently suppressing it. Unit tests invoke the hook exe with controlled environment variables and WorkingDirectory set to %TEMP% (outside any GVFS mount) to verify: - temp index paths (should skip) - canonical index paths (should NOT skip, exits NotInGVFSEnlistment) - missing GIT_DIR (should NOT skip) - mixed separators and case (normalization via GetFullPathNameA) Note: the ideal long-term fix is in Git's do_write_locked_index() (read-cache.c) to skip firing the hook entirely for non-default indexes, avoiding the process spawn altogether. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella --- GVFS/GVFS.PostIndexChangedHook/main.cpp | 72 ++++++- .../Hooks/PostIndexChangedHookTests.cs | 178 ++++++++++++++++++ 2 files changed, 249 insertions(+), 1 deletion(-) create mode 100644 GVFS/GVFS.UnitTests/Hooks/PostIndexChangedHookTests.cs diff --git a/GVFS/GVFS.PostIndexChangedHook/main.cpp b/GVFS/GVFS.PostIndexChangedHook/main.cpp index 03fb26b29..9b71e2495 100644 --- a/GVFS/GVFS.PostIndexChangedHook/main.cpp +++ b/GVFS/GVFS.PostIndexChangedHook/main.cpp @@ -1,4 +1,4 @@ -#include "stdafx.h" +#include "stdafx.h" #include "common.h" enum PostIndexChangedErrorReturnCode @@ -8,6 +8,66 @@ enum PostIndexChangedErrorReturnCode const int PIPE_BUFFER_SIZE = 1024; +// Returns true if GIT_INDEX_FILE refers to a non-canonical (temp) index. +// The canonical index path is $GIT_DIR/index; anything else is a temp +// index that GVFS doesn't need to be notified about. +// +// GIT_DIR is always set by git.exe itself (via xsetenv in setup.c) before +// any hook runs, so it is reliably present. GIT_INDEX_FILE is only present +// when an external caller (script, build tool, etc.) explicitly exports it +// before invoking git, to redirect index operations to a temp file. +static bool IsNonCanonicalIndex() +{ + char *indexFileEnv = NULL; + size_t indexLen = 0; + _dupenv_s(&indexFileEnv, &indexLen, "GIT_INDEX_FILE"); + + if (indexFileEnv == NULL || indexFileEnv[0] == '\0') + { + free(indexFileEnv); + return false; + } + + char *gitDirEnv = NULL; + size_t gitDirLen = 0; + _dupenv_s(&gitDirEnv, &gitDirLen, "GIT_DIR"); + + if (gitDirEnv == NULL || gitDirEnv[0] == '\0') + { + // GIT_INDEX_FILE is set but GIT_DIR is not — shouldn't happen + // inside a hook (git.exe always sets GIT_DIR), but err on the + // side of correctness: proceed with the notification. + free(indexFileEnv); + free(gitDirEnv); + return false; + } + + // Build the canonical index path: /index + std::string canonical(gitDirEnv); + if (!canonical.empty() && canonical.back() != '\\' && canonical.back() != '/') + canonical += '\\'; + canonical += "index"; + + // Resolve both paths to absolute form so that relative GIT_DIR + // (e.g. ".git") and absolute GIT_INDEX_FILE compare correctly. + char canonicalFull[MAX_PATH]; + char actualFull[MAX_PATH]; + DWORD canonLen = GetFullPathNameA(canonical.c_str(), MAX_PATH, canonicalFull, NULL); + DWORD actualLen = GetFullPathNameA(indexFileEnv, MAX_PATH, actualFull, NULL); + + free(indexFileEnv); + free(gitDirEnv); + + if (canonLen == 0 || canonLen >= MAX_PATH || + actualLen == 0 || actualLen >= MAX_PATH) + { + // Path resolution failed — err on the side of correctness. + return false; + } + + return _stricmp(actualFull, canonicalFull) != 0; +} + int main(int argc, char *argv[]) { if (argc != 3) @@ -15,6 +75,16 @@ int main(int argc, char *argv[]) die(ReturnCode::InvalidArgCount, "Invalid arguments"); } + // Skip notification for non-canonical (temp) index files. + // Git fires post-index-change for every index write, including temp + // indexes created via GIT_INDEX_FILE redirect (e.g. read-tree + // --index-output, git add with a temp index). GVFS only needs to + // know about changes to the real $GIT_DIR/index. + if (IsNonCanonicalIndex()) + { + return 0; + } + if (strcmp(argv[1], "1") && strcmp(argv[1], "0")) { die(PostIndexChangedErrorReturnCode::ErrorPostIndexChangedProtocol, "Invalid value passed for first argument"); diff --git a/GVFS/GVFS.UnitTests/Hooks/PostIndexChangedHookTests.cs b/GVFS/GVFS.UnitTests/Hooks/PostIndexChangedHookTests.cs new file mode 100644 index 000000000..2bf00cc28 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Hooks/PostIndexChangedHookTests.cs @@ -0,0 +1,178 @@ +using NUnit.Framework; +using System; +using System.Diagnostics; +using System.IO; + +namespace GVFS.UnitTests.Hooks +{ + [TestFixture] + public class PostIndexChangedHookTests + { + // Exit code from common.h ReturnCode::NotInGVFSEnlistment. + // The hook dies with this code when it can't find a .gvfs folder. + private const int NotInGVFSEnlistment = 3; + + // The hook exe is built to the same output root as the test runner. + // Walk up from the unit test output dir to find the hook exe under + // the shared build output tree. + private static readonly string HookExePath = FindHookExe(); + + private static string FindHookExe() + { + // Test runner lives at: out\GVFS.UnitTests\bin\Debug\net471\win-x64\ + // Hook exe lives at: out\GVFS.PostIndexChangedHook\bin\x64\Debug\ + string testDir = Path.GetDirectoryName(typeof(PostIndexChangedHookTests).Assembly.Location); + string outDir = Path.GetFullPath(Path.Combine(testDir, "..", "..", "..", "..", "..")); + string hookPath = Path.Combine(outDir, "GVFS.PostIndexChangedHook", "bin", "x64", "Debug", "GVFS.PostIndexChangedHook.exe"); + + // Also check via VFS_OUTDIR if available + if (!File.Exists(hookPath)) + { + string vfsOutDir = Environment.GetEnvironmentVariable("VFS_OUTDIR"); + if (!string.IsNullOrEmpty(vfsOutDir)) + { + hookPath = Path.Combine(vfsOutDir, "GVFS.PostIndexChangedHook", "bin", "x64", "Debug", "GVFS.PostIndexChangedHook.exe"); + } + } + + return hookPath; + } + + [SetUp] + public void EnsureHookExists() + { + if (!File.Exists(HookExePath)) + { + Assert.Ignore($"Hook exe not found at {HookExePath} — build the full solution first."); + } + } + + /// + /// When GIT_INDEX_FILE points to a non-canonical (temp) index, + /// the hook should exit immediately with code 0 without trying + /// to connect to the GVFS pipe. + /// + [TestCase("C:\\repo\\.git\\tmp_index_1234", "C:\\repo\\.git")] + [TestCase("/repo/.git/some_other_index", "/repo/.git")] + [TestCase("D:\\src\\.git\\index.lock", "D:\\src\\.git")] + [TestCase("C:\\tmp\\scratch_index", "C:\\repo\\.git")] + public void SkipsNotification_WhenIndexIsNonCanonical(string indexFile, string gitDir) + { + int exitCode = RunHook(indexFile, gitDir); + Assert.AreEqual(0, exitCode, "Hook should exit 0 (skip) for non-canonical index"); + } + + /// + /// When GIT_INDEX_FILE matches the canonical $GIT_DIR/index, + /// the hook should NOT skip — it should proceed and attempt the + /// pipe connection. Outside a GVFS mount (WorkingDirectory is + /// %TEMP%), the hook fails with NotInGVFSEnlistment, proving + /// the guard did not fire. + /// + [TestCase("C:\\repo\\.git\\index", "C:\\repo\\.git")] + [TestCase("C:\\repo\\.git/index", "C:\\repo\\.git\\")] + public void DoesNotSkip_WhenIndexIsCanonical(string indexFile, string gitDir) + { + int exitCode = RunHook(indexFile, gitDir); + Assert.AreEqual(NotInGVFSEnlistment, exitCode, + "Hook should NOT skip for canonical index (NotInGVFSEnlistment = guard didn't fire)"); + } + + /// + /// When GIT_INDEX_FILE is not set at all, the hook should NOT + /// skip — this is the normal case where git writes the default index. + /// + [Test] + public void DoesNotSkip_WhenGitIndexFileNotSet() + { + int exitCode = RunHook(null, "C:\\repo\\.git"); + Assert.AreEqual(NotInGVFSEnlistment, exitCode, + "Hook should NOT skip when GIT_INDEX_FILE is unset"); + } + + /// + /// When GIT_INDEX_FILE is set but GIT_DIR is empty/missing, + /// the hook should NOT skip — err on the side of correctness + /// when the environment is unexpected. + /// + [TestCase("C:\\repo\\.git\\tmp_index", null)] + [TestCase("C:\\repo\\.git\\tmp_index", "")] + public void DoesNotSkip_WhenGitDirMissing(string indexFile, string gitDir) + { + int exitCode = RunHook(indexFile, gitDir); + Assert.AreEqual(NotInGVFSEnlistment, exitCode, + "Hook should NOT skip when GIT_DIR is absent — err on the side of correctness"); + } + + /// + /// Case-insensitive matching: mixed-case paths that resolve to + /// the canonical index should NOT skip. + /// + [Test] + public void DoesNotSkip_WhenCanonicalPathDiffersOnlyInCase() + { + int exitCode = RunHook("C:\\Repo\\.GIT\\INDEX", "C:\\Repo\\.GIT"); + Assert.AreEqual(NotInGVFSEnlistment, exitCode, + "Case-insensitive canonical match should NOT skip"); + } + + /// + /// Separator normalization: forward vs backslash in canonical + /// path should still match. + /// + [Test] + public void SkipsNotification_ForwardSlashTempIndex() + { + int exitCode = RunHook("C:/repo/.git/tmp_idx", "C:\\repo\\.git"); + Assert.AreEqual(0, exitCode, "Forward-slash temp index should still be detected as non-canonical"); + } + + private int RunHook(string gitIndexFile, string gitDir) + { + ProcessStartInfo psi = new ProcessStartInfo + { + FileName = HookExePath, + Arguments = "1 0", + UseShellExecute = false, + CreateNoWindow = true, + RedirectStandardError = true, + RedirectStandardOutput = true, + + // Run outside any GVFS enlistment so the pipe lookup + // fails predictably with NotInGVFSEnlistment. + WorkingDirectory = Path.GetTempPath(), + }; + + // Set or remove env vars + if (gitIndexFile != null) + { + psi.Environment["GIT_INDEX_FILE"] = gitIndexFile; + } + else + { + psi.Environment.Remove("GIT_INDEX_FILE"); + } + + if (gitDir != null) + { + psi.Environment["GIT_DIR"] = gitDir; + } + else + { + psi.Environment.Remove("GIT_DIR"); + } + + using (Process process = Process.Start(psi)) + { + process.WaitForExit(5000); + if (!process.HasExited) + { + process.Kill(); + Assert.Fail("Hook process timed out (5s) — likely blocked on pipe connect"); + } + + return process.ExitCode; + } + } + } +}