Skip to content

Commit e8d7647

Browse files
committed
perf(git-hook-sync): resolve the hooks directory once per drift check
driftParts computed githooks.ResolveHooksDir(repoRoot) twice on a cache miss: once inside peekHookSource and again inside preMergeCommitHookDrift. Each call spawns a `git rev-parse --git-path hooks` subprocess, exactly the "never exec a subprocess per item (git rev-parse once did)" anti-pattern docs/development/high-performance-go.md calls out under "Skip work you don't need". Resolve it once in driftParts and thread the result into both helpers. The result is already cached per repoRoot for the process lifetime (driftCache), so this halves one subprocess spawn per repo per process rather than per file — a small but real, unambiguous win with no measurement needed to justify avoiding a redundant fork/exec. TestDriftParts_ResolvesHooksDirOnce substitutes a counting stub for the new resolveHooksDir package var and asserts it is called exactly once per driftParts cache miss. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wWQmrtb8EMbn1hDgkESqE
1 parent 26fc662 commit e8d7647

2 files changed

Lines changed: 50 additions & 8 deletions

File tree

internal/rules/githooksync/rule.go

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,12 @@ const hookMaxReadBytes int64 = 1024 * 1024
2828
// hoisted to avoid re-converting the marker constant to []byte on every call.
2929
var preMergeMarkerBytes = []byte(githooks.PreMergeCommitMarker)
3030

31+
// resolveHooksDir is a package variable so tests can substitute a
32+
// counting stub and assert driftParts resolves the hooks directory
33+
// exactly once per cache miss, instead of once for peekHookSource and
34+
// again for preMergeCommitHookDrift (each a `git rev-parse` subprocess).
35+
var resolveHooksDir = githooks.ResolveHooksDir
36+
3137
func init() {
3238
rule.Register(&Rule{})
3339
}
@@ -181,9 +187,12 @@ const (
181187
)
182188

183189
// peekHookSource reports the current state of the pre-merge-commit
184-
// hook without parsing its contents.
185-
func peekHookSource(repoRoot string) hookSource {
186-
hookPath := filepath.Join(githooks.ResolveHooksDir(repoRoot), "pre-merge-commit")
190+
// hook without parsing its contents. hooksDir is the repo's resolved
191+
// hooks directory (githooks.ResolveHooksDir), passed in so a caller
192+
// that also needs it (driftParts) resolves it once rather than
193+
// spawning a second `git rev-parse` for the same repo.
194+
func peekHookSource(hooksDir string) hookSource {
195+
hookPath := filepath.Join(hooksDir, "pre-merge-commit")
187196
data, err := bytelimit.ReadFileLimited(hookPath, hookMaxReadBytes)
188197
if err != nil {
189198
if os.IsNotExist(err) {
@@ -213,7 +222,8 @@ func (r *Rule) driftParts(repoRoot string) []string {
213222
driftMu.Unlock()
214223

215224
hasDriver := githooks.HasMdsmithMergeDriver(repoRoot)
216-
hookState := peekHookSource(repoRoot)
225+
hooksDir := resolveHooksDir(repoRoot)
226+
hookState := peekHookSource(hooksDir)
217227
// Early-exit when the user has not opted in (no driver) and the hook
218228
// is not mdsmith-managed. hookSourceUnreadable (file too large, bad
219229
// perms) is included in the early-exit: we cannot verify the hook's
@@ -231,7 +241,7 @@ func (r *Rule) driftParts(repoRoot string) []string {
231241
if msg := r.mergeDriverDrift(repoRoot, hasDriver, expectedGlobs); msg != "" {
232242
parts = append(parts, msg)
233243
}
234-
if msg := r.preMergeCommitHookDrift(repoRoot); msg != "" {
244+
if msg := r.preMergeCommitHookDrift(hooksDir); msg != "" {
235245
parts = append(parts, msg)
236246
}
237247

@@ -301,9 +311,12 @@ func describeGlobs(patterns []string) string {
301311
// hook content. Returns an empty string if no hook is installed, the
302312
// hook is not mdsmith-managed, or the content matches. A non-ENOENT
303313
// read error is surfaced rather than silently passing so permission
304-
// or IO failures cannot mask real drift.
305-
func (r *Rule) preMergeCommitHookDrift(repoRoot string) string {
306-
hookPath := filepath.Join(githooks.ResolveHooksDir(repoRoot), "pre-merge-commit")
314+
// or IO failures cannot mask real drift. hooksDir is the repo's
315+
// resolved hooks directory, shared with peekHookSource by driftParts
316+
// so the two checks spawn only one `git rev-parse` per repo instead
317+
// of one each.
318+
func (r *Rule) preMergeCommitHookDrift(hooksDir string) string {
319+
hookPath := filepath.Join(hooksDir, "pre-merge-commit")
307320
data, err := bytelimit.ReadFileLimited(hookPath, hookMaxReadBytes)
308321
if err != nil {
309322
if os.IsNotExist(err) {

internal/rules/githooksync/rule_test.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1071,3 +1071,32 @@ func TestRule_Check_OversizedHookNoDriverSilent(t *testing.T) {
10711071
assert.Empty(t, diags,
10721072
"no driver registered: oversized third-party hook must not produce a diagnostic")
10731073
}
1074+
1075+
// TestDriftParts_ResolvesHooksDirOnce pins that a driftParts cache miss
1076+
// spawns `git rev-parse --git-path hooks` exactly once, shared between
1077+
// peekHookSource and preMergeCommitHookDrift, instead of once for each.
1078+
func TestDriftParts_ResolvesHooksDirOnce(t *testing.T) {
1079+
dir := t.TempDir()
1080+
initRepoWithDriver(t, dir)
1081+
installCanonicalHook(t, dir)
1082+
require.NoError(t, os.WriteFile(filepath.Join(dir, ".gitattributes"),
1083+
[]byte(canonicalManagedBlock()), 0o644))
1084+
1085+
driftMu.Lock()
1086+
delete(driftCache, dir)
1087+
driftMu.Unlock()
1088+
1089+
calls := 0
1090+
orig := resolveHooksDir
1091+
resolveHooksDir = func(repoRoot string) string {
1092+
calls++
1093+
return orig(repoRoot)
1094+
}
1095+
defer func() { resolveHooksDir = orig }()
1096+
1097+
r := &Rule{}
1098+
parts := r.driftParts(dir)
1099+
assert.Empty(t, parts, "canonical repo must report no drift")
1100+
assert.Equal(t, 1, calls,
1101+
"driftParts must resolve the hooks directory once, not once per drift check")
1102+
}

0 commit comments

Comments
 (0)