Merge branch 'master' into fixes-1367

Author: mrexox

internal/command/install.go

@@ -41,10 +41,6 @@ type InstallArgs struct {
 }
 
 func (l *Lefthook) Install(ctx context.Context, args InstallArgs, hooks []string) error {
-	if err := l.ensureHooksPathUnset(args.Force, args.ResetHooksPath); err != nil {
-		return err
-	}
-
 	cfg, err := l.readOrCreateConfig()
 	if err != nil {
 		return err
@@ -70,6 +66,17 @@ func (l *Lefthook) Install(ctx context.Context, args InstallArgs, hooks []string
 		}
 	}
 
+	return l.installHooks(cfg, hooks, args)
+}
+
+// installHooks is the single entry point for creating hook files.
+// It ensures core.hooksPath is checked before any hooks are written,
+// preventing silent overwrites of global hook directories.
+func (l *Lefthook) installHooks(cfg *config.Config, hooks []string, args InstallArgs) error {
+	if err := l.ensureHooksPathUnset(args.Force, args.ResetHooksPath); err != nil {
+		return err
+	}
+
 	return l.createHooksIfNeeded(cfg, hooks, args.Force)
 }
 
@@ -196,7 +203,11 @@ func (l *Lefthook) syncHooks(cfg *config.Config, fetchRemotes bool) (*config.Con
 	}
 
 	// Don't rely on config checksum if remotes were refetched
-	return cfg, l.createHooksIfNeeded(cfg, hooks, false)
+	if err := l.installHooks(cfg, hooks, InstallArgs{}); err != nil {
+		log.Warnf("Skipping hook sync: %s", err)
+		return cfg, nil
+	}
+	return cfg, nil
 }
 
 func (l *Lefthook) shouldRefetch(remote *config.Remote) bool {
@@ -276,23 +287,7 @@ func (l *Lefthook) createHooksIfNeeded(cfg *config.Config, hooks []string, force
 		return fmt.Errorf("could not create hooks dir: %w", err)
 	}
 
-	rootsMap := make(map[string]struct{})
-	for _, hook := range cfg.Hooks {
-		for _, command := range hook.Commands {
-			if len(command.Root) > 0 {
-				root := strings.Trim(command.Root, "/")
-				if _, ok := rootsMap[root]; !ok {
-					rootsMap[root] = struct{}{}
-				}
-			}
-		}
-
-		collectAllJobRoots(rootsMap, hook.Jobs)
-	}
-	roots := make([]string, 0, len(rootsMap))
-	for root := range rootsMap {
-		roots = append(roots, root)
-	}
+	roots := collectRoots(cfg)
 
 	hookNames := make([]string, 0, len(cfg.Hooks)+1)
 	for hook := range cfg.Hooks {
@@ -348,6 +343,23 @@ func (l *Lefthook) createHooksIfNeeded(cfg *config.Config, hooks []string, force
 	return nil
 }
 
+func collectRoots(cfg *config.Config) []string {
+	rootsMap := make(map[string]struct{})
+	for _, hook := range cfg.Hooks {
+		for _, command := range hook.Commands {
+			if len(command.Root) > 0 {
+				rootsMap[strings.Trim(command.Root, "/")] = struct{}{}
+			}
+		}
+		collectAllJobRoots(rootsMap, hook.Jobs)
+	}
+	roots := make([]string, 0, len(rootsMap))
+	for root := range rootsMap {
+		roots = append(roots, root)
+	}
+	return roots
+}
+
 func collectAllJobRoots(roots map[string]struct{}, jobs []*config.Job) {
 	for _, job := range jobs {
 		if len(job.Root) > 0 {

internal/command/install_test.go

@@ -317,14 +317,14 @@ remotes:
 		t.Run(fmt.Sprintf("%d: %s", n, tt.name), func(t *testing.T) {
 			assert := assert.New(t)
 
-			// Prepend git config commands required by getHooksPathConfig() in install.go.
-			// These commands are always called at the start of Install() to detect core.hooksPath conflicts.
+			// Append git config commands required by ensureHooksPathUnset() via installHooks().
+			// These commands are called after remote syncing, when hooks are about to be created.
 			gitCmds := tt.git
 			if len(gitCmds) == 0 || gitCmds[0].Command != "git config --local core.hooksPath" {
-				gitCmds = append([]cmdtest.Out{
-					{Command: "git config --local core.hooksPath"},
-					{Command: "git config --global core.hooksPath"},
-				}, gitCmds...)
+				gitCmds = append(gitCmds,
+					cmdtest.Out{Command: "git config --local core.hooksPath"},
+					cmdtest.Out{Command: "git config --global core.hooksPath"},
+				)
 			}
 
 			repo := gittest.NewRepositoryBuilder().
@@ -506,6 +506,52 @@ commit-msg:
 				hookPath(config.GhostHookName),
 			},
 		},
+		{
+			// Reproduces the bug: when core.hooksPath is set globally (e.g. for
+			// credential leak detection), syncHooks should skip hook creation
+			// rather than overwriting the user's global hooks directory.
+			// See: reproduce-bug.sh
+			name: "unsynchronized with global core.hooksPath set",
+			config: `
+pre-commit:
+  commands:
+    tests:
+      run: yarn test
+`,
+			checksum: "00000000f706df65f379a9ff5ce0119b 1555894311\n",
+			git: []cmdtest.Out{
+				{Command: "git config --local core.hooksPath"},
+				{Command: "git config --global core.hooksPath", Output: "/home/user/.config/git/hooks"},
+			},
+			wantExist: []string{
+				configPath,
+			},
+			wantNotExist: []string{
+				hookPath("pre-commit"),
+				hookPath(config.GhostHookName),
+			},
+		},
+		{
+			name: "unsynchronized with local core.hooksPath set",
+			config: `
+pre-commit:
+  commands:
+    tests:
+      run: yarn test
+`,
+			checksum: "00000000f706df65f379a9ff5ce0119b 1555894311\n",
+			git: []cmdtest.Out{
+				{Command: "git config --local core.hooksPath", Output: ".custom-hooks"},
+				{Command: "git config --global core.hooksPath"},
+			},
+			wantExist: []string{
+				configPath,
+			},
+			wantNotExist: []string{
+				hookPath("pre-commit"),
+				hookPath(config.GhostHookName),
+			},
+		},
 		{
 			name: "with unfetched remote",
 			config: `
@@ -566,7 +612,18 @@ remotes:
 		t.Run(fmt.Sprintf("%d: %s", n, tt.name), func(t *testing.T) {
 			assert := assert.New(t)
 
-			repo := gittest.NewRepositoryBuilder().Root(root).Fs(fs).Cmd(cmdtest.NewOrdered(t, tt.git)).Build()
+			// Append git config commands required by ensureHooksPathUnset() via installHooks().
+			// These commands are called when syncHooks needs to create/update hooks,
+			// which happens after any remote fetching.
+			gitCmds := tt.git
+			if len(gitCmds) == 0 || gitCmds[0].Command != "git config --local core.hooksPath" {
+				gitCmds = append(gitCmds,
+					cmdtest.Out{Command: "git config --local core.hooksPath"},
+					cmdtest.Out{Command: "git config --global core.hooksPath"},
+				)
+			}
+
+			repo := gittest.NewRepositoryBuilder().Root(root).Fs(fs).Cmd(cmdtest.NewOrdered(t, gitCmds)).Build()
 			lefthook := &Lefthook{
 				fs:   fs,
 				repo: repo,

internal/config/load.go

@@ -385,29 +385,10 @@ func addHook(name string, main, secondary *koanf.Koanf, c *Config) error {
 		default:
 		}
 
-		var destJobs, srcJobs []any
-		switch jobs := dest["jobs"].(type) {
-		case []any:
-			destJobs = jobs
-		default:
-		}
-		switch jobs := src["jobs"].(type) {
-		case []any:
-			srcJobs = jobs
-		default:
-		}
-
-		var destSetup, srcSetup []any
-		switch setup := dest["setup"].(type) {
-		case []any:
-			destSetup = setup
-		default:
-		}
-		switch setup := src["setup"].(type) {
-		case []any:
-			srcSetup = setup
-		default:
-		}
+		destJobs := anySlice(dest, "jobs")
+		srcJobs := anySlice(src, "jobs")
+		destSetup := anySlice(dest, "setup")
+		srcSetup := anySlice(src, "setup")
 
 		destJobs = mergeJobsSlice(srcJobs, destJobs)
 		destSetup = slices.Concat(srcSetup, destSetup)
@@ -497,57 +478,10 @@ func (k koanfProvider) ReadBytes() ([]byte, error) {
 // `jobs` settings get overwritten by name or get appended to the end.
 // `setup` always get prepended.
 func mergeHooks(src, dest map[string]any) error {
-	srcJobs := make(map[string][]any)
-	for name, maybeHook := range src {
-		switch hook := maybeHook.(type) {
-		case map[string]any:
-			switch jobs := hook["jobs"].(type) {
-			case []any:
-				srcJobs[name] = jobs
-			default:
-			}
-		default:
-		}
-	}
-
-	destJobs := make(map[string][]any)
-	for name, maybeHook := range dest {
-		switch hook := maybeHook.(type) {
-		case map[string]any:
-			switch jobs := hook["jobs"].(type) {
-			case []any:
-				destJobs[name] = jobs
-			default:
-			}
-		default:
-		}
-	}
-
-	srcSetup := make(map[string][]any)
-	for name, maybeHook := range src {
-		switch hook := maybeHook.(type) {
-		case map[string]any:
-			switch setup := hook["setup"].(type) {
-			case []any:
-				srcSetup[name] = setup
-			default:
-			}
-		default:
-		}
-	}
-
-	destSetup := make(map[string][]any)
-	for name, maybeHook := range dest {
-		switch hook := maybeHook.(type) {
-		case map[string]any:
-			switch setup := hook["setup"].(type) {
-			case []any:
-				destSetup[name] = setup
-			default:
-			}
-		default:
-		}
-	}
+	srcJobs := extractHookSlices(src, "jobs")
+	destJobs := extractHookSlices(dest, "jobs")
+	srcSetup := extractHookSlices(src, "setup")
+	destSetup := extractHookSlices(dest, "setup")
 
 	if (len(srcJobs) == 0 || len(destJobs) == 0) && (len(srcSetup) == 0 || len(destSetup) == 0) {
 		maps.Merge(src, dest)
@@ -599,10 +533,30 @@ func mergeHooks(src, dest map[string]any) error {
 	return nil
 }
 
+// anySlice extracts a []any value from a string-keyed map, returning nil if absent or wrong type.
+func anySlice(m map[string]any, key string) []any {
+	v, _ := m[key].([]any)
+	return v
+}
+
+// extractHookSlices builds a name→[]any map from a hooks map for the given field key.
+func extractHookSlices(hooks map[string]any, key string) map[string][]any {
+	result := make(map[string][]any)
+	for name, maybeHook := range hooks {
+		if hook, ok := maybeHook.(map[string]any); ok {
+			if v, ok := hook[key].([]any); ok {
+				result[name] = v
+			}
+		}
+	}
+	return result
+}
+
 func mergeJobsSlice(src, dest []any) []any {
 	mergeable := make(map[string]map[string]any)
 	result := make([]any, 0, len(dest))
 
+	// Pass 1: index dest jobs by name and preserve order.
 	for _, maybeJob := range dest {
 		switch destJob := maybeJob.(type) {
 		case map[string]any:
@@ -617,6 +571,7 @@ func mergeJobsSlice(src, dest []any) []any {
 		}
 	}
 
+	// Pass 2: merge src jobs into dest by name, or append unnamed ones.
 	for _, maybeJob := range src {
 		switch srcJob := maybeJob.(type) {
 		case map[string]any:

internal/config/skip_checker.go

@@ -8,16 +8,16 @@ import (
 	"github.com/evilmartians/lefthook/v2/internal/system"
 )
 
-type skipChecker struct {
+type SkipChecker struct {
 	exec *commandExecutor
 }
 
-func NewSkipChecker(cmd system.Command) *skipChecker {
-	return &skipChecker{&commandExecutor{cmd}}
+func NewSkipChecker(cmd system.Command) *SkipChecker {
+	return &SkipChecker{&commandExecutor{cmd}}
 }
 
 // Check returns the result of applying a skip/only setting which can be a branch, git state, shell command, etc.
-func (sc *skipChecker) Check(state func() git.State, skip any, only any) bool {
+func (sc *SkipChecker) Check(state func() git.State, skip any, only any) bool {
 	if skip == nil && only == nil {
 		return false
 	}
@@ -35,7 +35,7 @@ func (sc *skipChecker) Check(state func() git.State, skip any, only any) bool {
 	return false
 }
 
-func (sc *skipChecker) matches(state func() git.State, value any) bool {
+func (sc *SkipChecker) matches(state func() git.State, value any) bool {
 	switch typedValue := value.(type) {
 	case bool:
 		return typedValue
@@ -47,7 +47,7 @@ func (sc *skipChecker) matches(state func() git.State, value any) bool {
 	return false
 }
 
-func (sc *skipChecker) matchesSlices(gitState func() git.State, slice []any) bool {
+func (sc *SkipChecker) matchesSlices(gitState func() git.State, slice []any) bool {
 	for _, state := range slice {
 		switch typedState := state.(type) {
 		case string:
@@ -68,7 +68,7 @@ func (sc *skipChecker) matchesSlices(gitState func() git.State, slice []any) boo
 	return false
 }
 
-func (sc *skipChecker) matchesRef(state func() git.State, typedState map[string]any) bool {
+func (sc *SkipChecker) matchesRef(state func() git.State, typedState map[string]any) bool {
 	ref, ok := typedState["ref"].(string)
 	if !ok {
 		return false
@@ -84,7 +84,7 @@ func (sc *skipChecker) matchesRef(state func() git.State, typedState map[string]
 	return g.Match(branch)
 }
 
-func (sc *skipChecker) matchesCommands(typedState map[string]any) bool {
+func (sc *SkipChecker) matchesCommands(typedState map[string]any) bool {
 	commandLine, ok := typedState["run"].(string)
 	if !ok {
 		return false