Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Removing symlink ending with / removes the original path #13275

Closed
kubouch opened this issue Jun 30, 2024 · 6 comments · Fixed by #14667
Closed

Removing symlink ending with / removes the original path #13275

kubouch opened this issue Jun 30, 2024 · 6 comments · Fixed by #14667
Assignees
@hjetmundsen
Labels
🐛 bug Something isn't working file-system Related to commands and core nushell behavior around the file system
Milestone
v0.102.0

Comments

@kubouch
Copy link
Contributor

kubouch commented Jun 30, 2024 •
edited
Loading

Describe the bug

Removing a symlink of a directory, if ending with /, removes not the symlink, but the existing directory.

This is especially troublesome since autocompletion automatically appends the trailing /.

How to reproduce

> mkdir foo
> ln -s foo foo-link
> rm foo-<tab>  # autocompletes to "foo-link"
> ls -l
# lists only foo-link

Expected behavior

Two options (not mutually exclusive):

  • Both rm foo-link and rm foo-link/ should remove the symlink, not the original path. Such a subtle difference as a trailing slash should not have this severe impact on semantics.
  • Autocompletion shouldn't append / to symlinks pointing at directories.

Screenshots

No response

Configuration

key value
version 0.95.1
major 0
minor 95
patch 1
branch main
commit_hash 153b45b
build_os linux-x86_64
build_target x86_64-unknown-linux-gnu
rust_version rustc 1.77.2 (25ef9e3d8 2024-04-09)
rust_channel 1.77.2-x86_64-unknown-linux-gnu
cargo_version cargo 1.77.2 (e52e36006 2024-03-26)
build_time 2024-06-29 10:28:36 +03:00
build_rust_channel release
allocator mimalloc
features default, sqlite, system-clipboard, trash
installed_plugins explore, formats 0.95.1, gstat 0.95.1, highlight 1.2.0+0.95.0, inc 0.95.1, polars 0.95.1, query 0.95.1

Additional context

No response
@kubouch kubouch added 🐛 bug Something isn't working file-system Related to commands and core nushell behavior around the file system labels Jun 30, 2024
@hqsz
Copy link
Contributor

hqsz commented Jul 6, 2024

With docs, the command description of rm is Remove files and directories.
So this means it behaves like the rm or rmdir command depending on the situation.

With normal shell (e.g : bash, zsh)

> mkdir foo
> ln -s foo foo-link
> rm foo-link/
rm: foo-link/: is a directory 
# denied
> rmdir foo-link/
> ls -l
lrwxr-xr-x@  1 hqsz  staff     3 Jul  6 13:20 foo-link -> foo
# same as nushell

I don't think this is a bug because it works similarly to how other shells define it.
I would be very grateful if you could let me know your additional thoughts!

@kubouch
Copy link
Contributor Author

kubouch commented Jul 6, 2024

I see, rm foo-link/ seems to match the traditional semantic of other shells, which is a good argument, though, I still think it's a footgun. At the very least, though, the autocompletion shouldn't append / at the end of symlinks.

@MatrixManAtYrService
Copy link

MatrixManAtYrService commented Sep 21, 2024 •
edited
Loading

Another case where the tab-completed trailing / is problematic:

❯  mkdir bar
❯  ln -s bar foo
❯  readlink fo▏   # press tab here
❯  readlink foo/  # you'll get this, which gives no output, since the linked-to dir is not itself a link

❯  readlink foo   # this would be better (it's what bash tab completion would do)
bar

@hjetmundsen hjetmundsen mentioned this issue Dec 23, 2024
Merged
@hjetmundsen
Copy link
Contributor

I've opened a pull request with changes to fix this! Can I be added as the Assignee for this issue?

kubouch pushed a commit that referenced this issue Dec 27, 2024
@hjetmundsen
Remove trailing slash from symlink completion (issue #13275) (#14667)
76bbd41 
# Description
These changes fix #13275 where a slash is appended to completions of
symlinks pointing to directories.

# User-Facing Changes
The `/` character will no longer be appended to completions of symlinks.

Co-authored-by: Henry Jetmundsen <jet@henrys-mbp-2.lan>
@github-actions github-actions bot added this to the v0.102.0 milestone Dec 27, 2024
@kuchta
Copy link

kuchta commented Dec 27, 2024

@hq @kubouch But it seems rm don't match the behavior of legacy shells which by default refuse to remove directories which basically eliminates this issue...

@kubouch
Copy link
Contributor Author

kubouch commented Dec 27, 2024

Nushell also refuses to remove a non-empty directory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hjetmundsen hjetmundsen

Labels
🐛 bug Something isn't working file-system Related to commands and core nushell behavior around the file system
Projects
None yet
Milestone
v0.102.0
Development

Successfully merging a pull request may close this issue.

Remove trailing slash from symlink completion (issue #13275) hjetmundsen/nushell
5 participants
@kuchta @MatrixManAtYrService @kubouch @hjetmundsen @hqsz