diff --git a/src/internal/doctor/check_stale_shims.go b/src/internal/doctor/check_stale_shims.go index 1374399..0202163 100644 --- a/src/internal/doctor/check_stale_shims.go +++ b/src/internal/doctor/check_stale_shims.go @@ -3,6 +3,7 @@ package doctor import ( "fmt" "os" + "path/filepath" goruntime "runtime" "strings" @@ -51,10 +52,12 @@ func (staleShimsCheck) Name() string { return "stale-shims-path" } func (c *staleShimsCheck) Run() Finding { currentShims := c.currentShimsDir() - pathEntries := path.SplitPath(c.pathEnv()) - stale := path.FindStaleShimsEntries(pathEntries, currentShims) + entries := path.ClassifyShimsEntries(path.SplitPath(c.pathEnv()), currentShims) + // The same directory often appears twice, from the System and the User + // PATH; report each once. + stale, others := uniqueDirs(entries.Stale), uniqueDirs(entries.OtherRoots) - if len(stale) == 0 { + if len(stale) == 0 && len(others) == 0 { return Finding{OK: true, Title: "No stale dtvem shims entries in PATH"} } @@ -62,8 +65,18 @@ func (c *staleShimsCheck) Run() Finding { for _, s := range stale { details = append(details, Detail{Key: "Found", Value: s}) } + for _, o := range others { + details = append(details, Detail{Key: "Other install", Value: o}) + } + + if len(stale) == 0 { + return otherRootsFinding(others, details) + } resolution, fix := c.fixOrInstructions(stale, currentShims) + if len(others) > 0 { + resolution += "\nShims directories of other dtvem installs (\"Other install\") still exist and are left in place." + } return Finding{ Severity: SeverityError, Title: fmt.Sprintf("Stale dtvem shims entr%s in PATH", plural(len(stale), "y", "ies")), @@ -73,6 +86,42 @@ func (c *staleShimsCheck) Run() Finding { } } +// uniqueDirs returns dirs without repeats, keeping the first spelling of +// each. Comparison is case-insensitive on Windows. +func uniqueDirs(dirs []string) []string { + seen := make(map[string]bool, len(dirs)) + var unique []string + for _, d := range dirs { + key := filepath.Clean(strings.TrimSpace(d)) + if goruntime.GOOS == constants.OSWindows { + key = strings.ToLower(key) + } + if !seen[key] { + seen[key] = true + unique = append(unique, d) + } + } + return unique +} + +// otherRootsFinding reports PATH entries for another dtvem root's shims +// directory that still exists: a second install, or the default install +// while DTVEM_ROOT points elsewhere. That root may still be in use, so +// doctor never removes these; it only says how to, if the install is gone. +func otherRootsFinding(others []string, details []Detail) Finding { + return Finding{ + Severity: SeverityWarning, + Title: fmt.Sprintf("Another dtvem install's shims director%s in PATH", plural(len(others), "y is", "ies are")), + Details: details, + Resolution: strings.Join([]string{ + "These shims belong to a different dtvem root than the one this dtvem uses (\"Expected\").", + "If you use more than one dtvem root on purpose, nothing needs to change.", + "If that install is no longer used, remove its entry from your PATH (on Windows: your User or", + "System PATH; elsewhere: the export in your shell config), then restart your terminal.", + }, "\n"), + } +} + // fixOrInstructions returns the resolution text and (optionally) a Fix // closure for the current platform. On both platforms we always return // instructions, but only return a Fix when we have a safe in-place diff --git a/src/internal/doctor/check_stale_shims_test.go b/src/internal/doctor/check_stale_shims_test.go index 9b1426d..cd0fe58 100644 --- a/src/internal/doctor/check_stale_shims_test.go +++ b/src/internal/doctor/check_stale_shims_test.go @@ -242,3 +242,92 @@ func TestStaleShimsCheck_FixPropagatesImplError(t *testing.T) { t.Errorf("Fix err = %v, want chain containing %v", err, wantErr) } } + +// existingOtherRootShims creates another dtvem root's shims directory, as a +// second install or the default install would have. +func existingOtherRootShims(t *testing.T) string { + t.Helper() + dir := filepath.Join(t.TempDir(), "other", ".dtvem", "shims") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("creating %s: %v", dir, err) + } + return dir +} + +func TestStaleShimsCheck_OtherRootIsAWarningWithoutFix(t *testing.T) { + _, shims := withDtvemRoot(t) + other := existingOtherRootShims(t) + withPathEnv(t, []string{shims, other, "/usr/local/bin"}) + + got := newStaleShimsCheck().Run() + if got.OK { + t.Fatal("expected a finding for another install's shims directory, got OK") + } + if got.Severity != SeverityWarning { + t.Errorf("Severity = %v, want warning: another root is not an error", got.Severity) + } + if got.Fixable() { + t.Error("finding is fixable; another root's shims must never be removed automatically") + } + + var gotOther bool + for _, d := range got.Details { + if d.Key == "Other install" && d.Value == other { + gotOther = true + } + if d.Key == "Found" { + t.Errorf("Details report %s as stale (Found); it still exists", d.Value) + } + } + if !gotOther { + t.Errorf("Details = %v, want an \"Other install\" entry for %s", got.Details, other) + } + if !strings.Contains(got.Resolution, "no longer used") { + t.Errorf("Resolution %q should explain when to remove the entry", got.Resolution) + } +} + +func TestStaleShimsCheck_RepeatedEntryReportedOnce(t *testing.T) { + _, shims := withDtvemRoot(t) + other := existingOtherRootShims(t) + // Windows puts the System PATH before the User PATH, so a directory + // added to both appears twice. + withPathEnv(t, []string{other, shims, other}) + + got := newStaleShimsCheck().Run() + count := 0 + for _, d := range got.Details { + if d.Key == "Other install" { + count++ + } + } + if count != 1 { + t.Errorf("Details list the other install %d times, want once: %v", count, got.Details) + } +} + +func TestStaleShimsCheck_StaleAndOtherRoot(t *testing.T) { + _, shims := withDtvemRoot(t) + other := existingOtherRootShims(t) + missing := filepath.Join(t.TempDir(), "gone", ".dtvem", "shims") + withPathEnv(t, []string{missing, shims, other}) + + got := newStaleShimsCheck().Run() + if got.OK || got.Severity != SeverityError || !got.Fixable() { + t.Fatalf("Run() OK=%v severity=%v fixable=%v, want a fixable error for the missing directory", got.OK, got.Severity, got.Fixable()) + } + + found := map[string]string{} + for _, d := range got.Details { + found[d.Value] = d.Key + } + if found[missing] != "Found" { + t.Errorf("missing directory reported as %q, want \"Found\"", found[missing]) + } + if found[other] != "Other install" { + t.Errorf("existing other root reported as %q, want \"Other install\"", found[other]) + } + if !strings.Contains(got.Resolution, "left in place") { + t.Errorf("Resolution %q should say other installs' shims are left in place", got.Resolution) + } +} diff --git a/src/internal/path/path.go b/src/internal/path/path.go index 078a347..edfdb95 100644 --- a/src/internal/path/path.go +++ b/src/internal/path/path.go @@ -85,70 +85,101 @@ func IsDtvemShimsPath(path string) bool { return leafEq(parent, "dtvem") || leafEq(parent, ".dtvem") } -// FindStaleShimsEntries scans pathEntries for entries that look like dtvem -// shims directories but do not match currentShimsDir. The returned slice -// preserves the order of appearance in pathEntries and has the original -// (un-cleaned) entry strings, so callers can match them against registry -// or config-file content. -// -// Comparison against currentShimsDir is case-insensitive on Windows. +// FindStaleShimsEntries scans pathEntries for dtvem shims directories that +// are not currentShimsDir and no longer exist. See ClassifyShimsEntries. +// The returned slice preserves order and has the original (un-cleaned) +// entry strings, so callers can match them against registry or +// config-file content. func FindStaleShimsEntries(pathEntries []string, currentShimsDir string) []string { - _, stale := PartitionStaleShimsEntries(pathEntries, currentShimsDir) - return stale + return ClassifyShimsEntries(pathEntries, currentShimsDir).Stale +} + +// FindOtherRootShimsEntries scans pathEntries for dtvem shims directories +// that are not currentShimsDir but still exist: another dtvem root, such +// as a second install or the default install while DTVEM_ROOT points +// elsewhere. Such entries are reported, never removed. The returned slice +// preserves order and has the original entry strings. +func FindOtherRootShimsEntries(pathEntries []string, currentShimsDir string) []string { + return ClassifyShimsEntries(pathEntries, currentShimsDir).OtherRoots } // PartitionStaleShimsEntries splits pathEntries into the entries to keep -// and the stale dtvem-shims entries to remove. Order is preserved in -// both slices, and the strings are the original (un-cleaned) entries so -// callers can match them against registry-stored values verbatim. -// -// An empty string entry is treated as "drop silently" — it lands in -// neither slice, which matches what callers want when rewriting PATH -// (we don't want to preserve stray empty entries that arose from -// trailing separators). -// -// When currentShimsDir is empty the entire input is returned as kept, -// since without a "current" reference we can't decide which dtvem-shims -// entries are stale and which are correct. +// and the stale dtvem-shims entries to remove. See ClassifyShimsEntries; +// another root's existing shims directory is kept. func PartitionStaleShimsEntries(pathEntries []string, currentShimsDir string) (kept, stale []string) { - if currentShimsDir == "" { - kept = make([]string, 0, len(pathEntries)) - for _, entry := range pathEntries { - if strings.TrimSpace(entry) == "" { - continue - } - kept = append(kept, entry) - } - return kept, nil - } - currentClean := filepath.Clean(currentShimsDir) + entries := ClassifyShimsEntries(pathEntries, currentShimsDir) + return entries.Kept, entries.Stale +} + +// ShimsEntries sorts PATH entries by how they relate to the current dtvem +// shims directory. Each slice preserves order and holds the original +// (un-cleaned) entry strings, so callers can match them against +// registry-stored values and config files verbatim. +type ShimsEntries struct { + // Kept holds every entry to keep: anything that isn't a dtvem shims + // directory, the current shims directory, and OtherRoots. + Kept []string + // Stale holds dtvem shims directories, other than the current one, + // that no longer exist: leftovers of a removed or moved install, such + // as a pre-XDG root or the other side of a system/user switch. + Stale []string + // OtherRoots holds dtvem shims directories, other than the current + // one, that still exist. They belong to another dtvem root that may + // still be in use, so they are kept as well. + OtherRoots []string +} +// ClassifyShimsEntries sorts pathEntries against currentShimsDir. Entries +// are compared with environment references expanded (%USERPROFILE% on +// Windows, $HOME elsewhere), so a registry or shell-config spelling of +// the current shims directory is recognized as current. +// +// Empty entries land in no slice, which is what callers want when +// rewriting PATH (stray empties come from trailing separators). When +// currentShimsDir is empty every entry is kept, since without a current +// reference no dtvem shims entry can be judged stale. +func ClassifyShimsEntries(pathEntries []string, currentShimsDir string) ShimsEntries { + var entries ShimsEntries for _, entry := range pathEntries { trimmed := strings.TrimSpace(entry) - if trimmed == "" { - continue - } - if !IsDtvemShimsPath(trimmed) { - kept = append(kept, entry) - continue - } - entryClean := filepath.Clean(trimmed) - matches := entryClean == currentClean - if runtime.GOOS == constants.OSWindows { - matches = strings.EqualFold(entryClean, currentClean) - } - if matches { - kept = append(kept, entry) + switch { + case trimmed == "": continue + case currentShimsDir == "", !IsDtvemShimsPath(expandPathEntry(trimmed)), sameShimsDir(trimmed, currentShimsDir): + entries.Kept = append(entries.Kept, entry) + case shimsDirExists(trimmed): + entries.Kept = append(entries.Kept, entry) + entries.OtherRoots = append(entries.OtherRoots, entry) + default: + entries.Stale = append(entries.Stale, entry) } - stale = append(stale, entry) } - return kept, stale + return entries +} + +// sameShimsDir reports whether two shims directory spellings name the same +// directory once environment references are expanded. Comparison is +// case-insensitive on Windows. +func sameShimsDir(a, b string) bool { + a = filepath.Clean(expandPathEntry(a)) + b = filepath.Clean(expandPathEntry(b)) + if runtime.GOOS == constants.OSWindows { + return strings.EqualFold(a, b) + } + return a == b +} + +// shimsDirExists reports whether a shims directory entry, with environment +// references expanded, exists on disk. +func shimsDirExists(entry string) bool { + info, err := os.Stat(expandPathEntry(entry)) + return err == nil && info.IsDir() } // RemoveStaleShimsFromShellConfig removes "marker + export" blocks -// from a single shell config file when the export references a dtvem -// shims directory that doesn't match currentShimsDir. The file is +// from a single shell config file when the export references a stale +// dtvem shims directory: not currentShimsDir, and no longer on disk. +// Another root's existing shims directory is left alone. The file is // rewritten in place; the returned slice lists the stale paths whose // blocks were dropped, in source order. // @@ -186,8 +217,8 @@ func RemoveStaleShimsFromShellConfig(configFile, currentShimsDir string) ([]stri // removeDtvemMarkerBlocks scans configContent line by line, dropping // each pair of (marker comment, following export line) whose export -// line references a dtvem shims path that doesn't match -// currentShimsDir. Returns the rewritten content and the stale paths +// line references a stale dtvem shims path (see +// extractStaleShimsPath). Returns the rewritten content and the stale paths // that were dropped, in source order. // // Behavior intentionally errs on the side of keeping content: @@ -227,8 +258,9 @@ func removeDtvemMarkerBlocks(configContent, currentShimsDir string) (string, []s // extractStaleShimsPath returns the dtvem shims path referenced in a // PATH-export line if and only if that path is non-empty, looks like a -// dtvem shims dir, and is not currentShimsDir. Returns "" when the -// line isn't a stale-referencing export. +// dtvem shims dir, is not currentShimsDir, and no longer exists (see +// ClassifyShimsEntries). Returns "" when the line isn't a +// stale-referencing export. // // The matcher is permissive across bash/zsh ("export PATH=...") and // fish ("set -gx PATH ...") because the dtvem shims path appears @@ -237,8 +269,6 @@ func removeDtvemMarkerBlocks(configContent, currentShimsDir string) (string, []s // bash export concatenates, and check the first segment against // IsDtvemShimsPath. func extractStaleShimsPath(line, currentShimsDir string) string { - currentClean := filepath.Clean(currentShimsDir) - for _, quoted := range quotedSubstrings(line) { // Bash-style "