fix(use,delete,rename): support multiple directories per identity

internal/blocks/blocks.go

@@ -99,11 +99,13 @@ func stripBlock(content, name string, m Marker) string {
 }
 
 func appendBlock(content, name, body string, m Marker) string {
-	if content != "" && !strings.HasSuffix(content, "\n") {
-		content += "\n"
-	}
-	if content != "" && !strings.HasSuffix(content, "\n\n") {
-		content += "\n"
+	// Normalise to exactly one blank line of separation before the block.
+	// Collapsing trailing newlines (rather than only ever adding them) is
+	// what keeps repeated strip+append cycles idempotent — otherwise every
+	// Upsert after a Remove would leak another blank line into the file.
+	content = strings.TrimRight(content, "\n")
+	if content != "" {
+		content += "\n\n"
 	}
 	open := fmt.Sprintf("%s >>> %s%s", m.CommentPrefix, m.OpenWord, name)
 	closeLine := fmt.Sprintf("%s <<< %s%s", m.CommentPrefix, m.OpenWord, name)
@@ -112,14 +114,19 @@ func appendBlock(content, name, body string, m Marker) string {
 }
 
 // blockPattern matches a full sentinel-wrapped block, including its
-// trailing newline if present. The (?s) flag lets `.` cross newlines.
+// trailing newline and any blank lines that follow it. Swallowing the
+// trailing blank separator is what keeps remove/re-add idempotent —
+// otherwise each strip would strand the blank line appendBlock inserted.
+// The (?s) flag lets `.` cross newlines; the trailing group matches only
+// blank lines, so it never eats into the next block or real config.
 func blockPattern(name string, m Marker) *regexp.Regexp {
 	cp := regexp.QuoteMeta(m.CommentPrefix)
 	ow := regexp.QuoteMeta(m.OpenWord + name)
 	return regexp.MustCompile(
 		`(?s)` +
 			`[ \t]*` + cp + `[ \t]*>>>[ \t]*` + ow + `[ \t]*\n` + // open line
 			`.*?` + // body, non-greedy
-			`[ \t]*` + cp + `[ \t]*<<<[ \t]*` + ow + `[ \t]*\n?`, // close line
+			`[ \t]*` + cp + `[ \t]*<<<[ \t]*` + ow + `[ \t]*\n?` + // close line
+			`(?:[ \t]*\n)*`, // any blank lines after the block
 	)
 }

internal/blocks/blocks_test.go

@@ -0,0 +1,71 @@
+package blocks
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// TestUpsertIdempotentAcrossManyBlocks guards the appendBlock fix: adding
+// several blocks and then re-adding them must not keep leaking blank lines
+// into the file (the bug that made multi-directory bindings grow
+// ~/.gitconfig on every `gitswitch use`).
+func TestUpsertIdempotentAcrossManyBlocks(t *testing.T) {
+	f := filepath.Join(t.TempDir(), "gitconfig")
+	if err := os.WriteFile(f, []byte("[user]\n    name = base\n"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	write := func() {
+		for _, n := range []string{"a", "b", "c"} {
+			if err := Upsert(f, n, "[x]\n    k = "+n+"\n", 0o600); err != nil {
+				t.Fatal(err)
+			}
+		}
+	}
+
+	write()
+	first, _ := os.ReadFile(f)
+	for i := 0; i < 3; i++ {
+		write()
+	}
+	again, _ := os.ReadFile(f)
+
+	if string(first) != string(again) {
+		t.Errorf("Upsert not idempotent.\nfirst:\n%q\nafter re-runs:\n%q", first, again)
+	}
+	// User content preserved; no run of 3+ newlines (i.e. >1 blank line).
+	if !strings.Contains(string(again), "name = base") {
+		t.Errorf("user content lost:\n%s", again)
+	}
+	if strings.Contains(string(again), "\n\n\n") {
+		t.Errorf("blank lines leaked:\n%q", again)
+	}
+}
+
+// TestRemoveOneOfManyKeepsOthers verifies removing a block leaves the
+// others intact and doesn't strand blank lines.
+func TestRemoveOneOfManyKeepsOthers(t *testing.T) {
+	f := filepath.Join(t.TempDir(), "gitconfig")
+	_ = os.WriteFile(f, []byte(""), 0o600)
+	for _, n := range []string{"a", "b", "c"} {
+		if err := Upsert(f, n, "body-"+n+"\n", 0o600); err != nil {
+			t.Fatal(err)
+		}
+	}
+	if err := Remove(f, "b", 0o600); err != nil {
+		t.Fatal(err)
+	}
+	out, _ := os.ReadFile(f)
+	s := string(out)
+	if strings.Contains(s, "gitswitch:b") || strings.Contains(s, "body-b") {
+		t.Errorf("block b should be gone:\n%s", s)
+	}
+	if !strings.Contains(s, "body-a") || !strings.Contains(s, "body-c") {
+		t.Errorf("blocks a and c should remain:\n%s", s)
+	}
+	if strings.Contains(s, "\n\n\n") {
+		t.Errorf("blank lines leaked after remove:\n%q", s)
+	}
+}

internal/cmd/bindingblocks.go

@@ -0,0 +1,66 @@
+package cmd
+
+import (
+	"fmt"
+	"hash/fnv"
+	"path/filepath"
+
+	"github.com/target-ops/gitswitch/internal/blocks"
+	"github.com/target-ops/gitswitch/internal/identity"
+)
+
+// bindingBlockName is the sentinel name for the includeIf block of a
+// single (identity, directory) binding. Keying per binding — rather than
+// per identity — is what lets several directories share one identity
+// without their includeIf blocks clobbering each other in ~/.gitconfig.
+// Older gitswitch keyed blocks by the bare identity name, so a second
+// `use <id> <dir>` overwrote the first directory's block. The directory
+// is folded into a short stable hash so the sentinel stays comment- and
+// regex-safe and bounded in length.
+func bindingBlockName(name, dir string) string {
+	h := fnv.New32a()
+	_, _ = h.Write([]byte(filepath.Clean(dir)))
+	return fmt.Sprintf("%s:%08x", name, h.Sum32())
+}
+
+// syncBindingBlocks rewrites ~/.gitconfig (at path) so there is exactly
+// one includeIf block per directory currently bound to `name` in cfg. It
+// first removes the legacy single-block-per-identity form (keyed by the
+// bare identity name) written by older gitswitch versions, migrating the
+// user forward on the next `use`/`rename`. Idempotent: re-running with
+// the same cfg yields the same file.
+//
+// Note: this only adds/refreshes blocks for directories still bound to
+// `name`; callers that drop a binding must remove that binding's block
+// (bindingBlockName) themselves before calling this.
+func syncBindingBlocks(cfg *identity.Config, name, path string) error {
+	// First remove the legacy bare-name block and every current
+	// per-binding block, THEN re-append them. Stripping all up front
+	// (rather than upserting each in place) keeps the result order-stable
+	// and idempotent — repeated in-place upserts would otherwise reshuffle
+	// blocks and leak blank lines into ~/.gitconfig.
+	//
+	// Removing "gitswitch:<name>" only matches the legacy block: the regex
+	// anchors on the exact name, so it never touches "gitswitch:<name>:<hash>".
+	if err := blocks.Remove(path, name, 0o600); err != nil {
+		return err
+	}
+	for _, b := range cfg.Bindings {
+		if b.Identity != name {
+			continue
+		}
+		if err := blocks.Remove(path, bindingBlockName(name, b.Directory), 0o600); err != nil {
+			return err
+		}
+	}
+	for _, b := range cfg.Bindings {
+		if b.Identity != name {
+			continue
+		}
+		body := buildIncludeIfBlock(b.Directory, identity.GitconfigPath(name))
+		if err := blocks.Upsert(path, bindingBlockName(name, b.Directory), body, 0o600); err != nil {
+			return err
+		}
+	}
+	return nil
+}

internal/cmd/bindingblocks_test.go

@@ -0,0 +1,80 @@
+package cmd
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/target-ops/gitswitch/internal/identity"
+)
+
+// TestSyncBindingBlocksMultipleDirsOneIdentity is the regression test for
+// the bug where two directories bound to the same identity clobbered each
+// other's includeIf block. After sync, ~/.gitconfig must hold one block
+// per bound directory, the legacy bare-name block must be gone, and other
+// identities must be untouched.
+func TestSyncBindingBlocksMultipleDirsOneIdentity(t *testing.T) {
+	gc := filepath.Join(t.TempDir(), ".gitconfig")
+	// A legacy single-block-per-identity entry, as older gitswitch wrote.
+	legacy := "# >>> gitswitch:private\n" +
+		"[includeIf \"gitdir:/old/dir/\"]\n    path = /x/private.gitconfig\n" +
+		"# <<< gitswitch:private\n"
+	if err := os.WriteFile(gc, []byte(legacy), 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	cfg := &identity.Config{Bindings: []identity.Binding{
+		{Directory: "/work/a", Identity: "private"},
+		{Directory: "/work/b", Identity: "private"},
+		{Directory: "/work/c", Identity: "other"},
+	}}
+	if err := syncBindingBlocks(cfg, "private", gc); err != nil {
+		t.Fatal(err)
+	}
+
+	out, _ := os.ReadFile(gc)
+	s := string(out)
+
+	if !strings.Contains(s, "gitdir:/work/a/") || !strings.Contains(s, "gitdir:/work/b/") {
+		t.Errorf("both private dirs should have includeIf blocks:\n%s", s)
+	}
+	if n := strings.Count(s, ">>> gitswitch:private:"); n != 2 {
+		t.Errorf("expected 2 per-binding blocks, got %d:\n%s", n, s)
+	}
+	// Legacy bare block (open line "gitswitch:private" with no :hash) gone.
+	if strings.Contains(s, ">>> gitswitch:private\n") {
+		t.Errorf("legacy bare block should be removed:\n%s", s)
+	}
+	if strings.Contains(s, "gitdir:/old/dir/") {
+		t.Errorf("legacy block contents should be gone:\n%s", s)
+	}
+	// Unrelated identity must not be added.
+	if strings.Contains(s, "gitdir:/work/c/") {
+		t.Errorf("should not touch the 'other' identity:\n%s", s)
+	}
+
+	// Idempotent: a second sync yields identical content.
+	if err := syncBindingBlocks(cfg, "private", gc); err != nil {
+		t.Fatal(err)
+	}
+	out2, _ := os.ReadFile(gc)
+	if string(out2) != s {
+		t.Errorf("syncBindingBlocks is not idempotent:\nfirst:\n%s\nsecond:\n%s", s, out2)
+	}
+}
+
+func TestBindingBlockNameDistinctAndStable(t *testing.T) {
+	a := bindingBlockName("private", "/work/a")
+	b := bindingBlockName("private", "/work/b")
+	if a == b {
+		t.Errorf("distinct dirs should yield distinct block names, both = %q", a)
+	}
+	if !strings.HasPrefix(a, "private:") {
+		t.Errorf("block name should be prefixed by identity name: %q", a)
+	}
+	// Stable across equivalent paths (trailing slash is cleaned away).
+	if a != bindingBlockName("private", "/work/a/") {
+		t.Errorf("block name should be stable across equivalent paths")
+	}
+}

internal/cmd/delete.go

@@ -107,8 +107,15 @@ func runDelete(name string, assumeYes bool) error {
 		}
 	}
 
-	// 1. strip the includeIf block from ~/.gitconfig (no-op if not present)
+	// 1. strip the includeIf blocks from ~/.gitconfig — one per bound
+	// directory, plus the legacy bare-name block from older versions
+	// (all no-ops if not present).
 	gitconfigPath := filepath.Join(os.Getenv("HOME"), ".gitconfig")
+	for _, b := range refBindings {
+		if err := blocks.Remove(gitconfigPath, bindingBlockName(name, b.Directory), 0o600); err != nil {
+			return fmt.Errorf("strip includeIf block from ~/.gitconfig: %w", err)
+		}
+	}
 	if err := blocks.Remove(gitconfigPath, name, 0o600); err != nil {
 		return fmt.Errorf("strip includeIf block from ~/.gitconfig: %w", err)
 	}

internal/cmd/rename.go

@@ -70,12 +70,18 @@ func runRename(oldName, newName string) error {
 		}
 	}
 
-	// 2. Update the includeIf block in ~/.gitconfig — strip the old,
-	//    re-add under the new name pointing at the new path. Done by
-	//    finding any binding for the old name and re-applying it
-	//    under the new identity post-rename (we do that after JSON
-	//    update so we have the new identity in scope).
+	// 2. Strip the old includeIf blocks from ~/.gitconfig — one per
+	//    bound directory, plus the legacy bare-name block. We do this
+	//    before mutating the JSON, while bindings still reference the
+	//    old name.
 	gitconfigPath := filepath.Join(os.Getenv("HOME"), ".gitconfig")
+	for _, b := range cfg.Bindings {
+		if b.Identity == oldName {
+			if err := blocks.Remove(gitconfigPath, bindingBlockName(oldName, b.Directory), 0o600); err != nil {
+				return fmt.Errorf("strip old includeIf block: %w", err)
+			}
+		}
+	}
 	if err := blocks.Remove(gitconfigPath, oldName, 0o600); err != nil {
 		return fmt.Errorf("strip old includeIf block: %w", err)
 	}
@@ -92,16 +98,10 @@ func runRename(oldName, newName string) error {
 		}
 	}
 
-	// 4. Re-add the includeIf block under the new name for every dir
-	//    that's still bound to it.
-	for _, b := range cfg.Bindings {
-		if b.Identity != newName {
-			continue
-		}
-		body := buildIncludeIfBlock(b.Directory, identity.GitconfigPath(newName))
-		if err := blocks.Upsert(gitconfigPath, newName, body, 0o600); err != nil {
-			return fmt.Errorf("re-add includeIf block: %w", err)
-		}
+	// 4. Re-add the includeIf blocks under the new name, one per dir
+	//    still bound to it.
+	if err := syncBindingBlocks(cfg, newName, gitconfigPath); err != nil {
+		return fmt.Errorf("re-add includeIf block: %w", err)
 	}
 
 	if err := identity.Save(cfg); err != nil {

internal/cmd/use.go

@@ -97,15 +97,15 @@ func runUse(args []string, unbind bool) error {
 		return nil
 	}
 
-	// 2. Update ~/.gitconfig with an idempotent includeIf block.
+	// 2. Record the binding, then (re)write ~/.gitconfig with one
+	// includeIf block per directory bound to this identity. Keying the
+	// blocks per binding (not per identity) lets several directories
+	// share one identity without clobbering each other.
 	gitconfigPath := filepath.Join(os.Getenv("HOME"), ".gitconfig")
-	blockBody := buildIncludeIfBlock(dir, identity.GitconfigPath(name))
-	if err := blocks.Upsert(gitconfigPath, name, blockBody, 0o600); err != nil {
+	addBinding(cfg, name, dir)
+	if err := syncBindingBlocks(cfg, name, gitconfigPath); err != nil {
 		return fmt.Errorf("update ~/.gitconfig: %w", err)
 	}
-
-	// 3. Record the binding.
-	addBinding(cfg, name, dir)
 	if err := identity.Save(cfg); err != nil {
 		return err
 	}
@@ -235,7 +235,13 @@ func removeBinding(c *identity.Config, name, dir string) error {
 		if b.Directory == dir && b.Identity == name {
 			c.Bindings = append(c.Bindings[:i], c.Bindings[i+1:]...)
 			gitconfigPath := filepath.Join(os.Getenv("HOME"), ".gitconfig")
-			return blocks.Remove(gitconfigPath, name, 0o600)
+			// Drop this binding's own block, then rebuild the blocks for
+			// the identity's remaining directories (which also migrates
+			// any legacy bare-name block).
+			if err := blocks.Remove(gitconfigPath, bindingBlockName(name, dir), 0o600); err != nil {
+				return err
+			}
+			return syncBindingBlocks(c, name, gitconfigPath)
 		}
 	}
 	return fmt.Errorf("%s is not bound to %s", name, dir)