Skip to content

Fix mv <symlink>/ target for various situations. Align error message with GNU#10060

Open
Vitamin9C wants to merge 2 commits intouutils:mainfrom
Vitamin9C:PR-mv-symlinkslash
Open

Fix mv <symlink>/ target for various situations. Align error message with GNU#10060
Vitamin9C wants to merge 2 commits intouutils:mainfrom
Vitamin9C:PR-mv-symlinkslash

Conversation

@Vitamin9C
Copy link

@Vitamin9C Vitamin9C commented Jan 4, 2026

Description

Resolve problems described in #10026

  • Unaligned error message with GNU
  • Problematic behavior on running mv symlink_foo/ bar (will move symlink destination folder or file to bar)

Changes

  • Added one helper function in uucore::fs (compare other helper functions used in mv like path_ends_with_terminator, are_hardlinks_to_same_file). (Didn't reuse path_ends_with_terminator in Unix implementation for performance, reused it in Windows implementation cuz Windows U16 would worsen the performance anyway)
  • Added a segment in mv.rs to handle symlink_to_file/ and symlink_to_dir/ as mv source correctly and consistently with GNU.
  • Added a new MvError CannotMoveNotADirectory with error code mv-error-cannot-move-not-directory with en-US and fr-FR locales.
  • Added 6 tests under the pattern of mv symlink/ target which would fail before this PR.

@Vitamin9C Vitamin9C force-pushed the PR-mv-symlinkslash branch 2 times, most recently from d15e43f to 341c0f4 Compare January 5, 2026 02:03
@Vitamin9C
Copy link
Author

Vitamin9C commented Jan 5, 2026

The error messages of the failures in CI for win actually matches the error messages yielded with GNU Coreutils on Windows (which is different from GNU Coreutils on my local MacOS). It's just we need to craft these 3 tests for Unix and Windows with different expected values.

# MacOS
 ~/Wo/2026/tests  ls -al                          1 х │ system Node │ 08:09:45
total 0
drwxr-xr-x 6 schen 192 Jan  5 08:09 .
drwxr-xr-x 6 schen 192 Jan  3 13:09 ..
-rw-r--r-- 1 schen   0 Jan  5 08:09 a
drwxr-xr-x 2 schen  64 Jan  5 08:09 bar-dir
drwxr-xr-x 2 schen  64 Jan  5 08:09 foo-dir
lrwxr-xr-x 1 schen   7 Jan  5 08:09 foo-link -> foo-dir
 ~/Wo/2026/tests  gmv foo-link/ a                   ✔ │ system Node │ 08:09:50
gmv: cannot overwrite non-directory 'a' with directory 'foo-link/'
 ~/Wo/2026/tests  gmv foo-link/ foo-dir           1 х │ system Node │ 08:10:26
gmv: cannot move 'foo-link/' to 'foo-dir/foo-link': Not a directory
 ~/Wo/2026/tests  gmv foo-link/ bar-dir           1 х │ system Node │ 08:10:32
gmv: cannot move 'foo-link/' to 'bar-dir/foo-link': Not a directory
# Win
C:\Workspace\Personal\uutils>ls -al
total 8
drw-rw-rw-  4 chens 0    0 2026-01-05 08:07 .
drw-rw-rw-  7 chens 0 4096 2026-01-05 08:03 ..
-rw-rw-rw-  1 chens 0    0 2026-01-05 08:04 a
drw-rw-rw-  2 chens 0    0 2026-01-05 08:07 bar-dir
drw-rw-rw-  2 chens 0    0 2026-01-05 08:04 foo-dir
lr--r--r--  1 chens 0  838 2026-01-05 08:04 foo-link.lnk -> C:/Workspace/Personal/uutils/foo-dir

C:\Workspace\Personal\uutils>mv foo-link\ a
mv: cannot stat `foo-link\\': No such file or directory

C:\Workspace\Personal\uutils>mv foo-link\ foo-dir
mv: cannot stat `foo-link\\': No such file or directory

C:\Workspace\Personal\uutils>mv foo-link/ bar-dir
mv: cannot stat `foo-link/': No such file or directory

@sylvestre
Copy link
Contributor

please replace the screenshots by text

@Vitamin9C
Copy link
Author

Sorry I forgot to fmt before the last commit...... force-pushed again to prevent consuming CI time.

@github-actions
Copy link

github-actions bot commented Jan 5, 2026

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@Vitamin9C Vitamin9C changed the title Fix mv <symlink>/ target for various situations. Align error message … Fix mv <symlink>/ target for various situations. Align error message with GNU Jan 5, 2026
@Vitamin9C
Copy link
Author

Vitamin9C commented Jan 7, 2026

Hi @sylvestre. I wonder if I can do something further to get this PR be improved, reviewed, and merged? It's my first PR to this project and I wish to improve its quality and do better to contribute.

@sylvestre
Copy link
Contributor

sorry, i missed it

}

#[cfg(unix)]
pub fn is_symlink_with_trailing(path: &Path) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add unit tests for this function
including cases like symlink// or /symlink/////

Comment on lines +647 to +648
let len = bytes.len();
let stripped = &bytes[..len - 1];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let len = bytes.len();
let stripped = &bytes[..len - 1];
let stripped = &bytes[..bytes.len() - 1]

if is_symlink_with_trailing(source) {
if !source_is_dir {
return Err(MvError::CannotStatNotADirectory(source.quote().to_string()).into());
} else if target_is_dir {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that when source is a symlink-to-dir with trailing slash and target is NOT a directory and does NOT exist, the code falls through. please verify this matches GNU behavior for 'mv symlink/ nonexistent' and add a test.

@codspeed-hq
Copy link

codspeed-hq bot commented Feb 16, 2026

Merging this PR will degrade performance by 6.7%

❌ 2 regressed benchmarks
✅ 286 untouched benchmarks
⏩ 38 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation unexpand_large_file[10] 397.7 ms 426.3 ms -6.7%
Simulation unexpand_many_lines[100000] 189.7 ms 203.4 ms -6.7%

Comparing Vitamin9C:PR-mv-symlinkslash (1b61bb7) with main (ed05c6e)2

Open in CodSpeed

Footnotes

  1. 38 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on main (bd58575) during the generation of this report, so ed05c6e was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants