Skip to content

PostIndexChangedHook: skip notification for non-canonical indexes#1940

Merged
tyrielv merged 1 commit into
microsoft:masterfrom
tyrielv:tyrielv/skip-post-index-change-temp-index
Apr 9, 2026
Merged

PostIndexChangedHook: skip notification for non-canonical indexes#1940
tyrielv merged 1 commit into
microsoft:masterfrom
tyrielv:tyrielv/skip-post-index-change-temp-index

Conversation

@tyrielv
Copy link
Copy Markdown
Contributor

@tyrielv tyrielv commented Apr 8, 2026

PostIndexChangedHook: skip notification for non-canonical indexes

Problem

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 post-index-change.exe hook connects to the mount process via named pipe and waits for acknowledgment — a round-trip that takes ~5 seconds for large repos.

Root Cause

Layer File Issue
Git read-cache.cdo_write_locked_index() Fires run_hooks_l("post-index-change", ...) unconditionally — no check for temp vs. real index
GVFS GVFS.PostIndexChangedHook/main.cpp Always connects to mount pipe — no guard for non-canonical indexes

Why COMMAND_HOOK_LOCK doesn't help

The GVFS command lock (pre/post-command in GVFS.Hooks.exe) is a separate mechanism. post-index-change is fired independently by Git's hook runner. Additionally, hook.c's handle_hook_replacement() explicitly states: "We don't skip post-index-change hooks that exist."

Fix

Added IsNonCanonicalIndex() early-exit check in main.cpp:

  1. Read GIT_INDEX_FILE from environment
  2. If not set → this is the default index → proceed normally
  3. If set → compare against $GIT_DIR/index (normalized separators, case-insensitive)
  4. If they differ → exit 0 immediately, no pipe round-trip

Effect: Hook process still spawns (Git-side fires it unconditionally) but exits in <1ms instead of ~5s.

Ideal long-term fix

I think the correct fix would be in Git's do_write_locked_index() (microsoft/git fork, read-cache.c) to skip firing the hook entirely for non-default indexes, avoiding the process spawn altogether. This PR is the GVFS-side mitigation.

Testing

  • ✅ Full solution build (including native C++ projects)
  • ✅ 793/793 unit tests passed
  • Manual verification: set GIT_INDEX_FILE to a temp path → hook exits immediately

@tyrielv tyrielv force-pushed the tyrielv/skip-post-index-change-temp-index branch 3 times, most recently from 0f47bf2 to 0b3255d Compare April 9, 2026 16:05
@tyrielv tyrielv requested a review from KeithIsSleeping April 9, 2026 17:01
@tyrielv tyrielv marked this pull request as ready for review April 9, 2026 17:01
@tyrielv tyrielv enabled auto-merge April 9, 2026 17:04
Comment thread GVFS/GVFS.PostIndexChangedHook/main.cpp Outdated
Comment thread GVFS/GVFS.PostIndexChangedHook/main.cpp
Comment thread GVFS/GVFS.UnitTests/Hooks/PostIndexChangedHookTests.cs
@tyrielv tyrielv force-pushed the tyrielv/skip-post-index-change-temp-index branch from 0b3255d to 4b70348 Compare April 9, 2026 21:13
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 <tyrielv@gmail.com>
@tyrielv tyrielv merged commit 7352617 into microsoft:master Apr 9, 2026
43 of 45 checks passed
@mjcheetham mjcheetham mentioned this pull request May 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants