From 2bd62ef5fea039967f08e0e08994e42114f28861 Mon Sep 17 00:00:00 2001 From: "Calvin A. Allen" Date: Fri, 23 Oct 2026 10:47:38 -0400 Subject: [PATCH] fix(doctor): stop removing other dtvem installs' shims from PATH MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every PATH entry that looked like a dtvem shims directory (…/dtvem/shims or …/.dtvem/shims) but wasn't the current root's was treated as stale, without checking whether it was. With a second install, or DTVEM_ROOT pointing away from the default install, the default install's real shims directory was reported as an error and removed by `dtvem doctor --fix` (also under --yes): from the User PATH on Windows, and from the shell config on Unix. `dtvem init` on Windows dropped it silently too. A non-current shims entry is now stale only when its directory no longer exists. A directory that still exists belongs to another dtvem root, which may still be in use: it is kept everywhere and only reported. - path.ClassifyShimsEntries sorts entries into kept, stale and other roots. PartitionStaleShimsEntries and FindStaleShimsEntries wrap it, so the doctor check, the Windows registry fix, the Unix shell-config fix and dtvem init all follow the same rule. - Entries are judged with environment references expanded (%VAR% on Windows, $VAR elsewhere). A registry spelling of the current shims directory, such as %USERPROFILE%\.dtvem\shims, used to compare unequal to the expanded current directory and was removed as stale; it is now recognized as current. - doctor reports other roots' shims as a warning with manual instructions and no fix; stale entries stay a fixable error. A directory listed in both the System and User PATH is reported once. - dtvem init on Windows keeps other roots' entries and warns about them. --- src/internal/doctor/check_stale_shims.go | 55 ++++++- src/internal/doctor/check_stale_shims_test.go | 89 +++++++++++ src/internal/path/path.go | 150 +++++++++++------- src/internal/path/path_test.go | 98 ++++++++++++ src/internal/path/path_unix.go | 6 + src/internal/path/path_windows.go | 8 +- src/internal/path/registry_windows.go | 46 ++++-- src/internal/path/registry_windows_test.go | 86 +++++++++- 8 files changed, 456 insertions(+), 82 deletions(-) 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 ":$PATH" — keep the part before the first // colon. Fish-style "" — the whole quoted string is the @@ -248,14 +278,18 @@ func extractStaleShimsPath(line, currentShimsDir string) string { candidate = candidate[:idx] } candidate = strings.TrimSpace(candidate) - if !IsDtvemShimsPath(candidate) { + if !IsDtvemShimsPath(expandPathEntry(candidate)) { continue } - if filepath.Clean(candidate) == currentClean { + if sameShimsDir(candidate, currentShimsDir) { // References the current shims dir — that's the active // export we want to leave in place. continue } + if shimsDirExists(candidate) { + // Another dtvem root's shims, which may still be in use. + continue + } return candidate } return "" diff --git a/src/internal/path/path_test.go b/src/internal/path/path_test.go index 2fbfc1a..b4df74b 100644 --- a/src/internal/path/path_test.go +++ b/src/internal/path/path_test.go @@ -991,3 +991,101 @@ func stringSliceEqual(a, b []string) bool { } return true } + +// shimsDirOfOtherRoot creates a dtvem shims directory for another root +// and returns its path. +func shimsDirOfOtherRoot(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 TestClassifyShimsEntries(t *testing.T) { + root := t.TempDir() + current := filepath.Join(root, "current", ".dtvem", "shims") + other := shimsDirOfOtherRoot(t) + missing := filepath.Join(root, "gone", ".dtvem", "shims") + plain := filepath.Join(root, "tools") + + got := ClassifyShimsEntries([]string{plain, other, "", missing, current}, current) + + if want := []string{plain, other, current}; strings.Join(got.Kept, "|") != strings.Join(want, "|") { + t.Errorf("Kept = %v, want %v", got.Kept, want) + } + if want := []string{missing}; strings.Join(got.Stale, "|") != strings.Join(want, "|") { + t.Errorf("Stale = %v, want %v", got.Stale, want) + } + if want := []string{other}; strings.Join(got.OtherRoots, "|") != strings.Join(want, "|") { + t.Errorf("OtherRoots = %v, want %v", got.OtherRoots, want) + } + + if stale := FindStaleShimsEntries([]string{other, missing}, current); len(stale) != 1 || stale[0] != missing { + t.Errorf("FindStaleShimsEntries() = %v, want only the missing directory", stale) + } + if others := FindOtherRootShimsEntries([]string{other, missing}, current); len(others) != 1 || others[0] != other { + t.Errorf("FindOtherRootShimsEntries() = %v, want only the existing directory", others) + } +} + +// envSpelling returns dir written with an environment reference to its +// parent, the way a registry value or shell config might spell it. +func envSpelling(t *testing.T, dir string) string { + t.Helper() + t.Setenv("DTVEM_TEST_PARENT", filepath.Dir(dir)) + if runtime.GOOS == constants.OSWindows { + return `%DTVEM_TEST_PARENT%\` + filepath.Base(dir) + } + return "$DTVEM_TEST_PARENT/" + filepath.Base(dir) +} + +func TestClassifyShimsEntries_EnvironmentSpelling(t *testing.T) { + current := filepath.Join(t.TempDir(), "current", ".dtvem", "shims") + spelled := envSpelling(t, current) + + // The current shims dir spelled with an environment reference is the + // current dir, not a stale entry, even though it doesn't exist on disk. + got := ClassifyShimsEntries([]string{spelled}, current) + if len(got.Stale) != 0 || len(got.OtherRoots) != 0 || len(got.Kept) != 1 { + t.Errorf("ClassifyShimsEntries(%q) = %+v, want it kept as the current dir", spelled, got) + } +} + +func TestClassifyShimsEntries_EnvironmentSpellingOfOtherRoot(t *testing.T) { + current := filepath.Join(t.TempDir(), "current", ".dtvem", "shims") + other := shimsDirOfOtherRoot(t) + spelled := envSpelling(t, other) + + got := ClassifyShimsEntries([]string{spelled}, current) + if len(got.OtherRoots) != 1 || len(got.Stale) != 0 { + t.Errorf("ClassifyShimsEntries(%q) = %+v, want it reported as another root", spelled, got) + } +} + +func TestRemoveDtvemMarkerBlocks_KeepsOtherRoots(t *testing.T) { + if runtime.GOOS == constants.OSWindows { + // Shell-config exports are a Unix concern, and the ":$PATH" + // parsing can't take a Windows temp path with a drive colon. + t.Skip("shell config PATH exports are Unix-only") + } + current := filepath.Join(t.TempDir(), "current", "dtvem", "shims") + other := shimsDirOfOtherRoot(t) + missing := filepath.Join(t.TempDir(), "gone", ".dtvem", "shims") + input := strings.Join([]string{ + "# Added by dtvem", + `export PATH="` + other + `:$PATH"`, + "# Added by dtvem", + `export PATH="` + missing + `:$PATH"`, + }, "\n") + + out, removed := removeDtvemMarkerBlocks(input, current) + + if len(removed) != 1 || removed[0] != missing { + t.Errorf("removed = %v, want only %s", removed, missing) + } + if want := "# Added by dtvem\n" + `export PATH="` + other + `:$PATH"`; out != want { + t.Errorf("output = %q, want the other root's block kept:\n%q", out, want) + } +} diff --git a/src/internal/path/path_unix.go b/src/internal/path/path_unix.go index d9979b5..c91ce3b 100644 --- a/src/internal/path/path_unix.go +++ b/src/internal/path/path_unix.go @@ -190,3 +190,9 @@ func RegistryPaths() ([]RegistryPath, error) { func RepairUserPathType() error { return fmt.Errorf("the PATH registry only exists on Windows") } + +// expandPathEntry expands environment references such as $HOME in a PATH +// entry, as the shell would. +func expandPathEntry(entry string) string { + return os.ExpandEnv(entry) +} diff --git a/src/internal/path/path_windows.go b/src/internal/path/path_windows.go index 55d79d6..051c78f 100644 --- a/src/internal/path/path_windows.go +++ b/src/internal/path/path_windows.go @@ -365,7 +365,7 @@ func modifyPath(root registry.Key, keyPath, scope, shimsDir, action string) erro } defer func() { _ = key.Close() }() - removedStale, err := prependShimsDir(key, scope, shimsDir) + removedStale, otherRoots, err := prependShimsDir(key, scope, shimsDir) if err != nil { return err } @@ -376,6 +376,12 @@ func modifyPath(root registry.Key, keyPath, scope, shimsDir, action string) erro for _, stale := range removedStale { ui.Success("Removed stale dtvem shims entry from %s PATH: %s", scope, stale) } + for _, other := range otherRoots { + ui.Warning("Left another dtvem install's shims on your %s PATH: %s", scope, other) + } + if len(otherRoots) > 0 { + ui.Info("If that install is no longer used, remove the entry from your %s PATH.", scope) + } if action == pathActionMove { ui.Success("Moved %s to the beginning of your %s PATH", shimsDir, scope) diff --git a/src/internal/path/registry_windows.go b/src/internal/path/registry_windows.go index 04d0a6b..3cf236a 100644 --- a/src/internal/path/registry_windows.go +++ b/src/internal/path/registry_windows.go @@ -53,34 +53,48 @@ func hasEnvReference(value string) bool { return envReferencePattern.MatchString(value) } +// expandPathEntry expands %VAR% references in a PATH entry, as Windows does +// for a REG_EXPAND_SZ value. An entry that can't be expanded is returned as +// is. +func expandPathEntry(entry string) string { + expanded, err := registry.ExpandString(entry) + if err != nil { + return entry + } + return expanded +} + // prependShimsDir puts shimsDir first on the PATH under key, dropping any -// other copy of it and any stale dtvem shims entries, and returns the -// stale entries it removed. scope names the PATH ("User" or "System") in -// errors. -func prependShimsDir(key pathKey, scope, shimsDir string) ([]string, error) { +// other spelling of it and any stale dtvem shims entries (see +// ClassifyShimsEntries). It returns the stale entries it removed, and +// other dtvem roots' existing shims entries, which it keeps. scope names +// the PATH ("User" or "System") in errors. +func prependShimsDir(key pathKey, scope, shimsDir string) (removedStale, otherRoots []string, err error) { current, valType, err := readPathValue(key) if err != nil { - return nil, fmt.Errorf("failed to read %s PATH: %w", scope, err) + return nil, nil, fmt.Errorf("failed to read %s PATH: %w", scope, err) } + classified := ClassifyShimsEntries(strings.Split(current, ";"), shimsDir) entries := []string{shimsDir} - var removedStale []string - for _, p := range strings.Split(current, ";") { - trimmed := strings.TrimSpace(p) - switch { - case trimmed == "", strings.EqualFold(trimmed, shimsDir): - continue - case IsDtvemShimsPath(trimmed): - removedStale = append(removedStale, trimmed) - default: + for _, e := range classified.Kept { + if trimmed := strings.TrimSpace(e); !sameShimsDir(trimmed, shimsDir) { entries = append(entries, trimmed) } } if err := writePathValue(key, strings.Join(entries, ";"), valType); err != nil { - return nil, fmt.Errorf("failed to update %s PATH in registry: %w", scope, err) + return nil, nil, fmt.Errorf("failed to update %s PATH in registry: %w", scope, err) + } + return trimAll(classified.Stale), trimAll(classified.OtherRoots), nil +} + +func trimAll(entries []string) []string { + trimmed := make([]string, len(entries)) + for i, e := range entries { + trimmed[i] = strings.TrimSpace(e) } - return removedStale, nil + return trimmed } // removeStaleShims removes stale dtvem shims entries from the PATH under diff --git a/src/internal/path/registry_windows_test.go b/src/internal/path/registry_windows_test.go index 82ec95a..26e54a2 100644 --- a/src/internal/path/registry_windows_test.go +++ b/src/internal/path/registry_windows_test.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "reflect" "strings" "testing" @@ -94,7 +95,7 @@ func TestPrependShimsDir(t *testing.T) { valType: registry.EXPAND_SZ, } - removed, err := prependShimsDir(key, "User", shims) + removed, others, err := prependShimsDir(key, "User", shims) if err != nil { t.Fatalf("prependShimsDir() error: %v", err) } @@ -109,6 +110,60 @@ func TestPrependShimsDir(t *testing.T) { if !reflect.DeepEqual(removed, []string{`C:\old\.dtvem\shims`}) { t.Errorf("removed = %v, want the stale shims entry", removed) } + if len(others) != 0 { + t.Errorf("others = %v, want none", others) + } +} + +// otherRootShims creates a dtvem shims directory for another root and +// returns its path. +func otherRootShims(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 TestPrependShimsDir_KeepsOtherRoots(t *testing.T) { + const shims = `C:\Users\me\.dtvem\shims` + other := otherRootShims(t) + key := &fakePathKey{value: `C:\tools;` + other + `;C:\gone\.dtvem\shims`, valType: registry.EXPAND_SZ} + + removed, others, err := prependShimsDir(key, "User", shims) + if err != nil { + t.Fatalf("prependShimsDir() error: %v", err) + } + + if want := shims + `;C:\tools;` + other; key.value != want { + t.Errorf("PATH = %q, want %q", key.value, want) + } + if !reflect.DeepEqual(others, []string{other}) { + t.Errorf("others = %v, want [%s]", others, other) + } + if !reflect.DeepEqual(removed, []string{`C:\gone\.dtvem\shims`}) { + t.Errorf("removed = %v, want the missing shims entry", removed) + } +} + +func TestPrependShimsDir_ExpandedSpellingOfShimsDir(t *testing.T) { + t.Setenv("USERPROFILE", `C:\Users\me`) + key := &fakePathKey{value: `%USERPROFILE%\.dtvem\shims;C:\tools`, valType: registry.EXPAND_SZ} + + removed, others, err := prependShimsDir(key, "User", `C:\Users\me\.dtvem\shims`) + if err != nil { + t.Fatalf("prependShimsDir() error: %v", err) + } + + // The %USERPROFILE% spelling is the current shims dir: moved to the + // front once, not kept as a second entry or reported as stale. + if want := `C:\Users\me\.dtvem\shims;C:\tools`; key.value != want { + t.Errorf("PATH = %q, want %q", key.value, want) + } + if len(removed) != 0 || len(others) != 0 { + t.Errorf("removed = %v, others = %v; want neither", removed, others) + } } func TestPrependShimsDir_KeepsType(t *testing.T) { @@ -125,7 +180,7 @@ func TestPrependShimsDir_KeepsType(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if _, err := prependShimsDir(tt.key, "User", shims); err != nil { + if _, _, err := prependShimsDir(tt.key, "User", shims); err != nil { t.Fatalf("prependShimsDir() error: %v", err) } if tt.key.value != tt.value || tt.key.valType != tt.want { @@ -138,7 +193,7 @@ func TestPrependShimsDir_KeepsType(t *testing.T) { func TestPrependShimsDir_ReadError(t *testing.T) { key := &fakePathKey{readErr: errors.New("access denied")} - _, err := prependShimsDir(key, "System", `C:\shims`) + _, _, err := prependShimsDir(key, "System", `C:\shims`) if err == nil || !strings.Contains(err.Error(), "System PATH") { t.Errorf("prependShimsDir() error = %v, want a System PATH read error", err) } @@ -168,6 +223,29 @@ func TestRemoveStaleShims(t *testing.T) { } } +func TestRemoveStaleShims_KeepsOtherRootsAndCurrentSpelling(t *testing.T) { + t.Setenv("USERPROFILE", `C:\Users\me`) + other := otherRootShims(t) + key := &fakePathKey{ + value: `%USERPROFILE%\.dtvem\shims;` + other + `;C:\gone\.dtvem\shims;C:\tools`, + valType: registry.EXPAND_SZ, + } + + removed, err := removeStaleShims(key, `C:\Users\me\.dtvem\shims`) + if err != nil { + t.Fatalf("removeStaleShims() error: %v", err) + } + + // Only the missing directory goes. The %USERPROFILE% spelling of the + // current shims dir and the other root's existing shims dir stay. + if want := `%USERPROFILE%\.dtvem\shims;` + other + `;C:\tools`; key.value != want { + t.Errorf("PATH = %q, want %q", key.value, want) + } + if !reflect.DeepEqual(removed, []string{`C:\gone\.dtvem\shims`}) { + t.Errorf("removed = %v, want only the missing directory", removed) + } +} + func TestRemoveStaleShims_NothingToRemove(t *testing.T) { key := &fakePathKey{value: `C:\tools;%USERPROFILE%\bin`, valType: registry.SZ} @@ -208,7 +286,7 @@ func TestPrependShimsDir_Registry(t *testing.T) { if err := key.SetExpandStringValue("Path", `%SystemRoot%\system32;C:\tools`); err != nil { t.Fatalf("seeding scratch Path: %v", err) } - if _, err := prependShimsDir(key, "User", `C:\Users\me\.dtvem\shims`); err != nil { + if _, _, err := prependShimsDir(key, "User", `C:\Users\me\.dtvem\shims`); err != nil { t.Fatalf("prependShimsDir() error: %v", err) }