diff --git a/docs/content/faq.md b/docs/content/faq.md index f58050f1e1..b0e8845a7b 100644 --- a/docs/content/faq.md +++ b/docs/content/faq.md @@ -195,8 +195,8 @@ Use `-D` to force-delete branches with unmerged changes. Use `--no-delete-branch - `wt remove` — besides the target worktree, two cleanup mechanisms run. The removed worktree's own `git fsmonitor--daemon` (git's per-worktree filesystem watcher under `core.fsmonitor=true`, which would leak once its worktree is gone) is sent `git fsmonitor--daemon stop`, then force-terminated (`SIGTERM`, then `SIGKILL`) via the PID resolved from its IPC socket if it didn't exit. A background sweep then deletes `.git/wt/trash/` entries older than 24 hours (directories orphaned when a previous background removal was interrupted) and terminates fsmonitor daemons whose worktree no longer exists (orphans from `git worktree remove`, `rm -rf`, or a crashed `wt`) - `wt config state clear` — removes all worktrunk data from `.git/` (config keys, caches, markers, hints, variables, logs, stale trash) -- `wt config shell install` — when migrating an integration to a new location, removes the worktrunk-managed file left at the old one: fish `conf.d/wt.fish` (now `functions/wt.fish`) and nushell wrappers stranded under `/vendor/autoload` (now `/vendor/autoload`). Recognition is the same as for uninstall below, so a file of the user's own sitting at that path is left in place -- `wt config shell uninstall` — removes integration lines from bash/zsh/PowerShell rc files, and deletes worktrunk's wrapper and completion files (fish `functions/`, `conf.d/`, and `completions/`; nushell `vendor/autoload`). Files are recognized by worktrunk's own content markers, whatever binary name they were installed under; files without the markers are left alone. An rc file belongs to the user, so a line qualifies only where it runs the init command: one that merely mentions it, inside a comment, an `echo`, or an alias body, stays. Every line uninstall does take is printed, before removal and again after +- `wt config shell install` — when migrating an integration to a new location, removes the file left at the old one: fish `conf.d/wt.fish` (now `functions/wt.fish`) and nushell wrappers stranded under `/vendor/autoload` (now `/vendor/autoload`). The old path is where worktrunk's own wrapper lived and is named after the command being installed, so it's taken back whole without reading it — a `conf.d/wt.fish` left in place would be sourced at startup and shadow the new wrapper anyway. Only that exact filename is touched, and each removal is printed +- `wt config shell uninstall` — removes integration lines from bash/zsh/PowerShell rc files, and deletes worktrunk's wrapper and completion files (fish `functions/`, `conf.d/`, and `completions/`; nushell `vendor/autoload`). Uninstall takes no command name, so it lists those directories and recognizes files by worktrunk's own content markers, whatever binary name they were installed under; files without the markers are left alone. An rc file belongs to the user, so a line qualifies only where it runs the init command: one that merely mentions it, inside a comment, an `echo`, or an alias body, stays. Every line uninstall does take is printed, before removal and again after See [What files does Worktrunk create?](#what-files-does-worktrunk-create) for details. diff --git a/plugins/worktrunk/skills/worktrunk/reference/faq.md b/plugins/worktrunk/skills/worktrunk/reference/faq.md index fffae01e4f..63517fb487 100644 --- a/plugins/worktrunk/skills/worktrunk/reference/faq.md +++ b/plugins/worktrunk/skills/worktrunk/reference/faq.md @@ -190,8 +190,8 @@ Use `-D` to force-delete branches with unmerged changes. Use `--no-delete-branch - `wt remove` — besides the target worktree, two cleanup mechanisms run. The removed worktree's own `git fsmonitor--daemon` (git's per-worktree filesystem watcher under `core.fsmonitor=true`, which would leak once its worktree is gone) is sent `git fsmonitor--daemon stop`, then force-terminated (`SIGTERM`, then `SIGKILL`) via the PID resolved from its IPC socket if it didn't exit. A background sweep then deletes `.git/wt/trash/` entries older than 24 hours (directories orphaned when a previous background removal was interrupted) and terminates fsmonitor daemons whose worktree no longer exists (orphans from `git worktree remove`, `rm -rf`, or a crashed `wt`) - `wt config state clear` — removes all worktrunk data from `.git/` (config keys, caches, markers, hints, variables, logs, stale trash) -- `wt config shell install` — when migrating an integration to a new location, removes the worktrunk-managed file left at the old one: fish `conf.d/wt.fish` (now `functions/wt.fish`) and nushell wrappers stranded under `/vendor/autoload` (now `/vendor/autoload`). Recognition is the same as for uninstall below, so a file of the user's own sitting at that path is left in place -- `wt config shell uninstall` — removes integration lines from bash/zsh/PowerShell rc files, and deletes worktrunk's wrapper and completion files (fish `functions/`, `conf.d/`, and `completions/`; nushell `vendor/autoload`). Files are recognized by worktrunk's own content markers, whatever binary name they were installed under; files without the markers are left alone. An rc file belongs to the user, so a line qualifies only where it runs the init command: one that merely mentions it, inside a comment, an `echo`, or an alias body, stays. Every line uninstall does take is printed, before removal and again after +- `wt config shell install` — when migrating an integration to a new location, removes the file left at the old one: fish `conf.d/wt.fish` (now `functions/wt.fish`) and nushell wrappers stranded under `/vendor/autoload` (now `/vendor/autoload`). The old path is where worktrunk's own wrapper lived and is named after the command being installed, so it's taken back whole without reading it — a `conf.d/wt.fish` left in place would be sourced at startup and shadow the new wrapper anyway. Only that exact filename is touched, and each removal is printed +- `wt config shell uninstall` — removes integration lines from bash/zsh/PowerShell rc files, and deletes worktrunk's wrapper and completion files (fish `functions/`, `conf.d/`, and `completions/`; nushell `vendor/autoload`). Uninstall takes no command name, so it lists those directories and recognizes files by worktrunk's own content markers, whatever binary name they were installed under; files without the markers are left alone. An rc file belongs to the user, so a line qualifies only where it runs the init command: one that merely mentions it, inside a comment, an `echo`, or an alias body, stays. Every line uninstall does take is printed, before removal and again after See [What files does Worktrunk create?](#what-files-does-worktrunk-create) for details. diff --git a/skills/worktrunk/reference/faq.md b/skills/worktrunk/reference/faq.md index fffae01e4f..63517fb487 100644 --- a/skills/worktrunk/reference/faq.md +++ b/skills/worktrunk/reference/faq.md @@ -190,8 +190,8 @@ Use `-D` to force-delete branches with unmerged changes. Use `--no-delete-branch - `wt remove` — besides the target worktree, two cleanup mechanisms run. The removed worktree's own `git fsmonitor--daemon` (git's per-worktree filesystem watcher under `core.fsmonitor=true`, which would leak once its worktree is gone) is sent `git fsmonitor--daemon stop`, then force-terminated (`SIGTERM`, then `SIGKILL`) via the PID resolved from its IPC socket if it didn't exit. A background sweep then deletes `.git/wt/trash/` entries older than 24 hours (directories orphaned when a previous background removal was interrupted) and terminates fsmonitor daemons whose worktree no longer exists (orphans from `git worktree remove`, `rm -rf`, or a crashed `wt`) - `wt config state clear` — removes all worktrunk data from `.git/` (config keys, caches, markers, hints, variables, logs, stale trash) -- `wt config shell install` — when migrating an integration to a new location, removes the worktrunk-managed file left at the old one: fish `conf.d/wt.fish` (now `functions/wt.fish`) and nushell wrappers stranded under `/vendor/autoload` (now `/vendor/autoload`). Recognition is the same as for uninstall below, so a file of the user's own sitting at that path is left in place -- `wt config shell uninstall` — removes integration lines from bash/zsh/PowerShell rc files, and deletes worktrunk's wrapper and completion files (fish `functions/`, `conf.d/`, and `completions/`; nushell `vendor/autoload`). Files are recognized by worktrunk's own content markers, whatever binary name they were installed under; files without the markers are left alone. An rc file belongs to the user, so a line qualifies only where it runs the init command: one that merely mentions it, inside a comment, an `echo`, or an alias body, stays. Every line uninstall does take is printed, before removal and again after +- `wt config shell install` — when migrating an integration to a new location, removes the file left at the old one: fish `conf.d/wt.fish` (now `functions/wt.fish`) and nushell wrappers stranded under `/vendor/autoload` (now `/vendor/autoload`). The old path is where worktrunk's own wrapper lived and is named after the command being installed, so it's taken back whole without reading it — a `conf.d/wt.fish` left in place would be sourced at startup and shadow the new wrapper anyway. Only that exact filename is touched, and each removal is printed +- `wt config shell uninstall` — removes integration lines from bash/zsh/PowerShell rc files, and deletes worktrunk's wrapper and completion files (fish `functions/`, `conf.d/`, and `completions/`; nushell `vendor/autoload`). Uninstall takes no command name, so it lists those directories and recognizes files by worktrunk's own content markers, whatever binary name they were installed under; files without the markers are left alone. An rc file belongs to the user, so a line qualifies only where it runs the init command: one that merely mentions it, inside a comment, an `echo`, or an alias body, stays. Every line uninstall does take is printed, before removal and again after See [What files does Worktrunk create?](#what-files-does-worktrunk-create) for details. diff --git a/src/commands/configure_shell.rs b/src/commands/configure_shell.rs index 366215fab2..649d7aebcf 100644 --- a/src/commands/configure_shell.rs +++ b/src/commands/configure_shell.rs @@ -132,9 +132,15 @@ const WRAPPER_MARKER: &str = "worktrunk shell integration for"; /// The header every fish completion file worktrunk has shipped opens with. const COMPLETION_MARKER: &str = "# worktrunk completions for"; -/// Whether a whole file is one worktrunk generated, and so is worktrunk's to -/// delete — by `uninstall`, and by the install-time cleanup of the legacy fish -/// `conf.d` wrapper. +/// Whether a whole file is one worktrunk generated, and so is `uninstall`'s to +/// delete. +/// +/// This is the one place ownership is read out of a file's contents, because +/// it's the one place worktrunk doesn't know the name. `uninstall` takes no +/// `--cmd`, so it lists the shell-owned directories and has to tell worktrunk's +/// `{cmd}.fish` from the user's own files sitting beside it. Wherever the +/// command name *is* known — install, and the legacy-location cleanups that +/// accompany it — the path names the file worktrunk owns and nothing reads it. /// /// The answer is independent of the binary name embedded in the file, so a /// wrapper installed as `wt.fish`, `git-wt.fish`, or `git-wt.nu` is recognized @@ -145,10 +151,10 @@ const COMPLETION_MARKER: &str = "# worktrunk completions for"; /// (per the rc-file line detector, keeping one definition of "an integration /// line"). /// -/// Both callers walk directories the user owns, so the question is asked of -/// every line rather than of the file as a blob. A user's own `wt.fish` that -/// runs `wt config shell init` amid other code — or merely mentions it in a -/// comment — survives; a whole-file substring test would delete it. +/// The directories walked are the user's, so the question is asked of every +/// line rather than of the file as a blob. A user's own `wt.fish` that runs +/// `wt config shell init` amid other code — or merely mentions it in a comment +/// — survives; a whole-file substring test would delete it. fn is_worktrunk_managed_content(content: &str) -> bool { if content.contains(WRAPPER_MARKER) { return true; @@ -160,21 +166,18 @@ fn is_worktrunk_managed_content(content: &str) -> bool { .all(shell::is_shell_integration_line_for_uninstall_any_cmd) } -/// Check if a Nushell wrapper file is worktrunk-managed. +/// Take back the fish wrapper's legacy `conf.d` location after installing to +/// `functions/`. /// -/// The Nushell wrapper is a complete autoload file (not a `source` line), so it -/// carries no `config shell init` marker — the header comment is the whole -/// signal that the file is ours to remove during stranded-file cleanup -/// (issue #2878). -fn is_worktrunk_managed_nushell(content: &str) -> bool { - content.contains(&format!("{WRAPPER_MARKER} nushell")) -} - -/// Clean up legacy fish conf.d file after installing to functions/ +/// Fish integration used to install to `~/.config/fish/conf.d/{cmd}.fish`, +/// which loads before Homebrew's PATH setup in `config.fish` (issue #566); +/// installs now write `functions/{cmd}.fish`, autoloaded on first use. /// -/// Previously, fish shell integration was installed to `~/.config/fish/conf.d/{cmd}.fish`. -/// This caused issues with Homebrew PATH setup (see issue #566). We now install to -/// `functions/{cmd}.fish` instead. This function removes the legacy file if it exists. +/// The path names the command being installed, so it's worktrunk's and the file +/// goes whole — the contents are never read. Leaving it would also break the +/// install it accompanies: `conf.d` is sourced at startup, so a `function +/// {cmd}` defined there is already loaded by the time fish would autoload +/// `functions/{cmd}.fish`, and the stale wrapper wins every time. /// /// Returns the paths of files that were cleaned up, each paired with `Shell::Fish`. fn cleanup_legacy_fish_conf_d(configured: &[ConfigureResult], cmd: &str) -> Vec<(Shell, PathBuf)> { @@ -198,16 +201,6 @@ fn cleanup_legacy_fish_conf_d(configured: &[ConfigureResult], cmd: &str) -> Vec< return cleaned; } - // Only remove if the file contains worktrunk integration markers - // to avoid deleting user's custom wt.fish that isn't from worktrunk - let Ok(content) = fs::read_to_string(&legacy_path) else { - return cleaned; - }; - - if !is_worktrunk_managed_content(&content) { - return cleaned; - } - match fs::remove_file(&legacy_path) { Ok(()) => { cleaned.push((Shell::Fish, legacy_path)); @@ -231,9 +224,13 @@ fn cleanup_legacy_fish_conf_d(configured: &[ConfigureResult], cmd: &str) -> Vec< /// /// Older worktrunk installed the wrapper under `/vendor/autoload`, /// which Nushell never autoloads (issue #2878). After installing to the correct -/// vendor-autoload dir (`/vendor/autoload`), this removes any -/// worktrunk-managed wrapper left at the other candidate paths so a stale, -/// never-loaded copy isn't left behind. +/// vendor-autoload dir (`/vendor/autoload`), this removes the wrapper +/// left at the other candidate paths so a stale, never-loaded copy isn't left +/// behind. +/// +/// Every path considered is one worktrunk itself computes for this command name +/// (`config_paths`), so — as with the fish `conf.d` cleanup — the path settles +/// ownership and the file is removed whole, unread. /// /// Returns the paths removed, each paired with `Shell::Nushell`. fn cleanup_stranded_nushell(configured: &[ConfigureResult], cmd: &str) -> Vec<(Shell, PathBuf)> { @@ -254,14 +251,6 @@ fn cleanup_stranded_nushell(configured: &[ConfigureResult], cmd: &str) -> Vec<(S if &path == canonical || !path.exists() { continue; } - // Only remove files that are clearly worktrunk's, to avoid deleting a - // user's own `wt.nu`. - let Ok(content) = fs::read_to_string(&path) else { - continue; - }; - if !is_worktrunk_managed_nushell(&content) { - continue; - } match fs::remove_file(&path) { Ok(()) => cleaned.push((Shell::Nushell, path)), Err(e) => { @@ -1633,12 +1622,12 @@ mod tests { assert!(is_worktrunk_managed_content(content), "{content}"); } - // Not ours: both callers walk directories the user owns, so a file that - // only mentions the command must survive — as must a user's own file - // that runs the init line amid other code, which a per-line any-match - // would delete whole. The second and third also carry the init command - // and a `| source` somewhere, which is all a whole-file substring test - // asks before deleting. + // Not ours: the uninstall scan walks directories the user owns and has + // only the contents to go on, so a file that merely mentions the command + // must survive — as must a user's own file that runs the init line amid + // other code, which a per-line any-match would delete whole. The second + // and third also carry the init command and a `| source` somewhere, + // which is all a whole-file substring test asks before deleting. for content in [ "function notes\n echo run wt config shell init fish to set up\nend\n", "# reminder: wt config shell init fish\nfunction helpers\n cat ~/.aliases | source\nend\n", diff --git a/tests/integration_tests/configure_shell.rs b/tests/integration_tests/configure_shell.rs index 30e9f3bcd6..f0aa51c55c 100644 --- a/tests/integration_tests/configure_shell.rs +++ b/tests/integration_tests/configure_shell.rs @@ -408,20 +408,23 @@ fn test_configure_shell_fish_legacy_conf_d_cleanup(repo: TestRepo, temp_home: Te ); } -/// Installing fish integration leaves a user's own `conf.d/wt.fish` alone. +/// Installing fish integration reclaims `conf.d/{cmd}.fish` whatever it holds. /// -/// The legacy cleanup deletes a whole file the user never asked it to touch, so -/// it has to recognize worktrunk's own wrapper rather than any file that -/// mentions the init command. This one names it in a comment and pipes -/// something unrelated to `source` — both substrings a whole-file test would -/// take as proof the file was worktrunk's. +/// The path names the command being installed, so it's worktrunk's; the file's +/// contents don't enter into it. Leaving this one would break the install +/// besides — `conf.d` is sourced at startup, so a `function wt` defined there +/// is already loaded when fish would otherwise autoload `functions/wt.fish`. #[rstest] -fn test_configure_shell_fish_preserves_user_conf_d_file(repo: TestRepo, temp_home: TempDir) { +fn test_configure_shell_fish_reclaims_conf_d_path(repo: TestRepo, temp_home: TempDir) { let conf_d = temp_home.path().join(".config/fish/conf.d"); fs::create_dir_all(&conf_d).unwrap(); - let user_file = conf_d.join("wt.fish"); - let user_content = "# reminder: set up with wt config shell init fish\nfunction wt_helpers\n cat ~/.aliases | source\nend\n"; - fs::write(&user_file, user_content).unwrap(); + let stale = conf_d.join("wt.fish"); + // No worktrunk header, and nothing a content test would recognize. + fs::write(&stale, "function wt\n command wt-old $argv\nend\n").unwrap(); + // A neighbour under another name is untouched: only `{cmd}.fish` is ours. + let neighbour = conf_d.join("aliases.fish"); + let neighbour_content = "# reminder: wt config shell init fish\nalias ll 'ls -l'\n"; + fs::write(&neighbour, neighbour_content).unwrap(); let mut cmd = wt_command(); repo.configure_wt_cmd(&mut cmd); @@ -437,16 +440,22 @@ fn test_configure_shell_fish_preserves_user_conf_d_file(repo: TestRepo, temp_hom .path() .join(".config/fish/functions/wt.fish") .exists(), - "install should still write functions/wt.fish" + "install should write functions/wt.fish" ); assert!( - user_file.exists(), - "install deleted the user's own conf.d/wt.fish" + !stale.exists(), + "install should reclaim conf.d/wt.fish: {stale:?}" ); assert_eq!( - fs::read_to_string(&user_file).unwrap(), - user_content, - "user's own conf.d/wt.fish must survive install untouched" + fs::read_to_string(&neighbour).unwrap(), + neighbour_content, + "a conf.d file under another name is not worktrunk's" + ); + // The removal is reported, not silent. + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("deprecated"), + "install should report the conf.d cleanup:\n{stderr}" ); } @@ -2774,21 +2783,23 @@ fn test_nushell_install_cleans_stranded_legacy(repo: TestRepo, temp_home: TempDi ); } -/// Data safety: install must NOT delete a `wt.nu` at a legacy location that -/// isn't worktrunk-managed (no worktrunk header) — it could be the user's own -/// file. Only files carrying the worktrunk header are cleaned up (issue #2878). +/// The stranded-file cleanup is scoped by path, so it reclaims `{cmd}.nu` at a +/// legacy autoload dir whatever the file holds — and touches nothing else in +/// that directory (issue #2878). #[rstest] -fn test_nushell_install_keeps_unmanaged_legacy_file(repo: TestRepo, temp_home: TempDir) { +fn test_nushell_install_reclaims_only_the_command_name(repo: TestRepo, temp_home: TempDir) { let home = canonical_temp_home(&temp_home); let autoload = home.join(".local/share/nushell/vendor/autoload"); - // A user-authored wt.nu at the legacy config-dir location — no worktrunk - // header, so it must be left untouched. let legacy_dir = home.join(".config/nushell/vendor/autoload"); fs::create_dir_all(&legacy_dir).unwrap(); + // No worktrunk header: the path is what makes it worktrunk's. let legacy = legacy_dir.join("wt.nu"); - let user_content = "# my own wt helper\ndef wt [] { echo hi }\n"; - fs::write(&legacy, user_content).unwrap(); + fs::write(&legacy, "def wt [] { echo hi }\n").unwrap(); + // A neighbour under another name stays put. + let neighbour = legacy_dir.join("helpers.nu"); + let neighbour_content = "def hi [] { echo hi }\n"; + fs::write(&neighbour, neighbour_content).unwrap(); let mut cmd = wt_command(); repo.configure_wt_cmd(&mut cmd); @@ -2805,16 +2816,15 @@ fn test_nushell_install_keeps_unmanaged_legacy_file(repo: TestRepo, temp_home: T String::from_utf8_lossy(&output.stderr) ); - // Canonical wrapper written, user's legacy file preserved verbatim. assert!(autoload.join("wt.nu").exists(), "canonical wrapper missing"); assert!( - legacy.exists(), - "unmanaged legacy wt.nu must be preserved: {legacy:?}" + !legacy.exists(), + "install should reclaim the legacy wt.nu: {legacy:?}" ); assert_eq!( - fs::read_to_string(&legacy).unwrap(), - user_content, - "unmanaged legacy file must be left unchanged" + fs::read_to_string(&neighbour).unwrap(), + neighbour_content, + "a legacy-dir file under another name is not worktrunk's" ); }