From 17eb19df560b0fb1086b48ce364aea5fde638e4e Mon Sep 17 00:00:00 2001 From: OfirHaim1 Date: Fri, 19 Jun 2026 00:02:40 +0300 Subject: [PATCH] fix(use,delete,rename): support multiple directories per identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each includeIf block in ~/.gitconfig was keyed by the bare identity name (sentinel '# >>> gitswitch:'), and every use/rename stripped the old block before writing the new one. So binding a SECOND directory to an identity overwrote the first directory's block: config.json recorded both bindings, but ~/.gitconfig held only one, and the un-blocked directory silently fell back to the global identity. Binding two repos to one 'private' identity is a completely normal thing to want, and it broke quietly. Key each block per (identity, directory) instead — sentinel '# >>> gitswitch::' — so multiple directories can share an identity without clobbering. A shared syncBindingBlocks helper rewrites all of an identity's blocks from config.json (the source of truth) and removes the legacy bare-name block, migrating existing users forward on their next use/rename. Also make blocks idempotent: appendBlock now normalises trailing newlines instead of only ever adding them, and stripBlock swallows the trailing blank separator — without this, the repeated strip+re-append in syncBindingBlocks leaked a blank line into ~/.gitconfig on every run. Tests: multi-dir sync (one block per dir, legacy removed, others untouched, idempotent); per-binding name distinct+stable; blocks upsert/remove idempotency. --- internal/blocks/blocks.go | 21 +++++--- internal/blocks/blocks_test.go | 71 ++++++++++++++++++++++++++ internal/cmd/bindingblocks.go | 66 ++++++++++++++++++++++++ internal/cmd/bindingblocks_test.go | 80 ++++++++++++++++++++++++++++++ internal/cmd/delete.go | 9 +++- internal/cmd/rename.go | 30 +++++------ internal/cmd/use.go | 20 +++++--- 7 files changed, 267 insertions(+), 30 deletions(-) create mode 100644 internal/blocks/blocks_test.go create mode 100644 internal/cmd/bindingblocks.go create mode 100644 internal/cmd/bindingblocks_test.go diff --git a/internal/blocks/blocks.go b/internal/blocks/blocks.go index cc86a17..05c4d3a 100644 --- a/internal/blocks/blocks.go +++ b/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,7 +114,11 @@ 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) @@ -120,6 +126,7 @@ func blockPattern(name string, m Marker) *regexp.Regexp { `(?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 ) } diff --git a/internal/blocks/blocks_test.go b/internal/blocks/blocks_test.go new file mode 100644 index 0000000..5ffce31 --- /dev/null +++ b/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) + } +} diff --git a/internal/cmd/bindingblocks.go b/internal/cmd/bindingblocks.go new file mode 100644 index 0000000..b3c2fbc --- /dev/null +++ b/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 ` 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:" only matches the legacy block: the regex + // anchors on the exact name, so it never touches "gitswitch::". + 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 +} diff --git a/internal/cmd/bindingblocks_test.go b/internal/cmd/bindingblocks_test.go new file mode 100644 index 0000000..efba305 --- /dev/null +++ b/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") + } +} diff --git a/internal/cmd/delete.go b/internal/cmd/delete.go index a80ce13..2bf49d6 100644 --- a/internal/cmd/delete.go +++ b/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) } diff --git a/internal/cmd/rename.go b/internal/cmd/rename.go index 33c536f..8e4c8a3 100644 --- a/internal/cmd/rename.go +++ b/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 { diff --git a/internal/cmd/use.go b/internal/cmd/use.go index 6088a4d..c97f6f2 100644 --- a/internal/cmd/use.go +++ b/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)