diff --git a/cmd/install.go b/cmd/install.go index fb84f24..3aada30 100644 --- a/cmd/install.go +++ b/cmd/install.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "runtime" + "sort" "github.com/spf13/cobra" @@ -190,8 +191,12 @@ func runInstallReconcile(mgr *tool.Manager, cfg *config.Config) error { return nil } -// isUpToDate checks whether a tool is already installed at the target version. -// Returns true (and prints a warning) if the installed version matches, false otherwise. +// isUpToDate reports whether the installed copy already matches the +// target version AND the manifest's asset spec. The spec check covers +// the case where the user added or renamed a bin/man/completion entry +// after the tool was already installed at the current release tag — +// without it, `gh tool install` would print "up to date" and never +// create the new symlinks. func isUpToDate(mgr *tool.Manager, t config.Tool) bool { name := t.Name() state := mgr.ReadState(name) @@ -208,10 +213,38 @@ func isUpToDate(mgr *tool.Manager, t config.Tool) bool { targetTag = latest } - if state.Tag == targetTag { - fmt.Printf("%s %s up to date (%s)\n", ui.IconBullet, name, targetTag) - return true + if state.Tag != targetTag { + return false + } + if !stringSlicesEqual(state.Bin, t.Bin) || + !stringSlicesEqual(state.Man, t.Man) || + !stringSlicesEqual(state.Completions, t.Completions) { + return false } - return false + fmt.Printf("%s %s up to date (%s)\n", ui.IconBullet, name, targetTag) + return true +} + +// stringSlicesEqual compares two slices as unordered sets, treating +// nil and empty as equivalent. Used by isUpToDate to detect manifest +// asset-spec changes that require a reinstall regardless of whether +// the release tag moved. +func stringSlicesEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + if len(a) == 0 { + return true + } + ac := append([]string(nil), a...) + bc := append([]string(nil), b...) + sort.Strings(ac) + sort.Strings(bc) + for i := range ac { + if ac[i] != bc[i] { + return false + } + } + return true } diff --git a/cmd/install_test.go b/cmd/install_test.go new file mode 100644 index 0000000..40422d9 --- /dev/null +++ b/cmd/install_test.go @@ -0,0 +1,107 @@ +package cmd + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/BurntSushi/toml" + + "github.com/ascarter/gh-tool/internal/config" + "github.com/ascarter/gh-tool/internal/paths" + "github.com/ascarter/gh-tool/internal/tool" +) + +func TestStringSlicesEqual(t *testing.T) { + cases := []struct { + a, b []string + want bool + }{ + {nil, nil, true}, + {nil, []string{}, true}, + {[]string{"x"}, []string{"x"}, true}, + {[]string{"a", "b"}, []string{"b", "a"}, true}, // unordered + {[]string{"x"}, []string{"y"}, false}, + {[]string{"x", "y"}, []string{"x"}, false}, + } + for _, c := range cases { + if got := stringSlicesEqual(c.a, c.b); got != c.want { + t.Errorf("stringSlicesEqual(%v, %v) = %v, want %v", c.a, c.b, got, c.want) + } + } +} + +// Regression: when a tool is already installed at the manifest's target +// tag but the manifest's bin/man/completions spec has changed (entries +// added or removed), isUpToDate must return false so install runs and +// actualizes the new spec. The previous implementation only compared +// the tag and would report "up to date" forever. +func TestIsUpToDateDetectsManifestSpecChange(t *testing.T) { + root := t.TempDir() + dirs := paths.Dirs{ + Config: filepath.Join(root, "config"), + Data: filepath.Join(root, "data"), + State: filepath.Join(root, "state"), + Cache: filepath.Join(root, "cache"), + } + if err := dirs.EnsureDirs(); err != nil { + t.Fatalf("EnsureDirs: %v", err) + } + mgr := tool.NewManager(dirs) + + // Persist a state file as if the tool had been installed at tag v1 + // with bin=[fzf] only. + state := tool.InstalledState{ + Repo: "junegunn/fzf", + Tag: "v1", + Pattern: "fzf-v1.tar.gz", + Bin: []string{"fzf"}, + InstalledAt: time.Now().UTC().Format(time.RFC3339), + } + if err := writeStateForTest(dirs, "fzf", state); err != nil { + t.Fatalf("write state: %v", err) + } + + // Same tag, bin spec unchanged → up to date. + tmatch := config.Tool{Repo: "junegunn/fzf", Tag: "v1", Bin: []string{"fzf"}} + if !isUpToDate(mgr, tmatch) { + t.Errorf("isUpToDate(matching spec)=false, want true") + } + + // Same tag, manifest gained completions entry → must NOT be up to + // date so install runs and creates the new completion symlinks. + tadded := config.Tool{Repo: "junegunn/fzf", Tag: "v1", Bin: []string{"fzf"}, Completions: []string{"shell/completion.bash"}} + if isUpToDate(mgr, tadded) { + t.Errorf("isUpToDate(spec gained completions)=true, want false") + } + + // Same tag, bin renamed → must NOT be up to date. + trenamed := config.Tool{Repo: "junegunn/fzf", Tag: "v1", Bin: []string{"fzf-bin:fzf"}} + if isUpToDate(mgr, trenamed) { + t.Errorf("isUpToDate(renamed bin)=true, want false") + } + + // Different tag → false regardless of spec. We pin Tag to bypass + // the LatestTag network call. + tnewtag := config.Tool{Repo: "junegunn/fzf", Tag: "v2", Bin: []string{"fzf"}} + if isUpToDate(mgr, tnewtag) { + t.Errorf("isUpToDate(different tag)=true, want false") + } +} + +// writeStateForTest seeds an InstalledState toml file at the path +// Manager.ReadState looks at. Avoids exporting Manager.writeState. +func writeStateForTest(dirs paths.Dirs, name string, state tool.InstalledState) error { + path := dirs.StateFile(name) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return err + } + f, err := os.Create(path) + if err != nil { + return err + } + defer f.Close() + return toml.NewEncoder(f).Encode(state) +} + diff --git a/internal/tool/tool.go b/internal/tool/tool.go index 68a4c46..e86eb2f 100644 --- a/internal/tool/tool.go +++ b/internal/tool/tool.go @@ -164,7 +164,15 @@ func (m *Manager) installFromAsset(t config.Tool, assetPath, tag, resolvedPatter m.verifyAttestation(name, t.Repo, assetPath) } - // Clean previous install + // Reap any prior install's symlinks before wiping the tool dir. The + // new install's createSymlinks below will only place links the + // current manifest entry calls for, so without this reap a renamed + // or removed bin/man/completion entry would leave a dangling + // symlink behind. Safe to call when there is no prior install: + // removeToolSymlinks only removes links whose targets resolve into + // the tool's ToolDir. + m.removeToolSymlinks(name) + toolDir := m.Dirs.ToolDir(name) _ = os.RemoveAll(toolDir) diff --git a/internal/tool/tool_test.go b/internal/tool/tool_test.go index 862f756..492a793 100644 --- a/internal/tool/tool_test.go +++ b/internal/tool/tool_test.go @@ -289,6 +289,64 @@ func TestRemoveToolSymlinksRelativeTarget(t *testing.T) { } } +// Regression: installFromAsset must reap the tool's existing symlinks +// before re-extracting, otherwise links from a previous install whose +// bin/man/completion spec changed (rename or removal) dangle pointing +// into the now-deleted ToolDir. +func TestInstallFromAssetReapsStaleSymlinks(t *testing.T) { + root := t.TempDir() + dirs := paths.Dirs{ + Config: filepath.Join(root, "config"), + Data: filepath.Join(root, "data"), + State: filepath.Join(root, "state"), + Cache: filepath.Join(root, "cache"), + } + if err := dirs.EnsureDirs(); err != nil { + t.Fatalf("EnsureDirs: %v", err) + } + mgr := NewManager(dirs) + + name := "mytool" + toolDir := dirs.ToolDir(name) + if err := os.MkdirAll(toolDir, 0o755); err != nil { + t.Fatal(err) + } + + // Simulate a prior install: a symlink in BinDir pointing at a file + // inside the (about-to-be-replaced) ToolDir. The new install's + // manifest will not reference this file. + staleSrc := filepath.Join(toolDir, "old-name") + if err := os.WriteFile(staleSrc, []byte{}, 0o755); err != nil { + t.Fatal(err) + } + staleLink := filepath.Join(dirs.BinDir(), "old-name") + if err := os.Symlink(staleSrc, staleLink); err != nil { + t.Fatalf("symlink: %v", err) + } + + // Stage a fake "downloaded asset" — a bare binary at a known path — + // and use InstallFromAsset (the public wrapper) with a Tool whose + // only bin is the new name. + asset := filepath.Join(root, "new-name") + if err := os.WriteFile(asset, []byte{}, 0o755); err != nil { + t.Fatal(err) + } + tool := config.Tool{Repo: "ex/mytool", Pattern: "new-name", Bin: []string{"new-name"}} + if err := mgr.InstallFromAsset(tool, asset, "v1", false); err != nil { + t.Fatalf("InstallFromAsset: %v", err) + } + + // The stale link must be gone (the bug was that it dangled). + if _, err := os.Lstat(staleLink); !os.IsNotExist(err) { + t.Errorf("expected stale symlink %s to be removed by reinstall; err=%v", staleLink, err) + } + // The new link must exist and resolve to the new binary in ToolDir. + newLink := filepath.Join(dirs.BinDir(), "new-name") + if _, err := os.Lstat(newLink); err != nil { + t.Errorf("new bin symlink missing: %v", err) + } +} + func TestInstalledStateRoundTrip(t *testing.T) { root := t.TempDir() dirs := paths.Dirs{