mv: hard link check #4697

haciek · 2023-03-31T13:08:30Z

Related to issue #4613
This pull request checks if files are hard linked.

Current behavior:

$ rm -f link
$ touch origin
$ echo content > origin
$ ln origin link
$ ./target/debug/coreutils mv origin link
mv: 'origin' and 'link' are the same file

Previously this didn't generate an error.
Let me know if there is anything I can change/improve. Thanks!

github-actions · 2023-03-31T13:59:26Z

GNU testsuite comparison:

Congrats! The gnu test tests/mv/force is no longer failing!
Congrats! The gnu test tests/mv/i-4 is no longer failing!

sylvestre · 2023-03-31T16:16:13Z

nice, could you please add a test ? thanks

haciek · 2023-03-31T16:34:10Z

nice, could you please add a test ? thanks

Sure, I'll try to work on that soon.

sylvestre · 2023-03-31T17:44:27Z

src/uu/mv/src/mv.rs

+ let mut is_hard_link = false;
+
+ if target.symlink_metadata().is_ok() {
+ let source_inode = source.symlink_metadata().unwrap().ino();


could you please avoid an unwrap here? Maybe manage the error ?

github-actions · 2023-04-01T11:18:58Z

GNU testsuite comparison:

Congrats! The gnu test tests/mv/force is no longer failing!
Congrats! The gnu test tests/mv/i-4 is no longer failing!

sylvestre · 2023-04-01T11:28:54Z

src/uu/mv/src/mv.rs

+ // Hard links are not allowed for directories
+ if !target_metadata.is_dir() {
+ let target_inode = target_metadata.ino();
+ let source_inode = match source.symlink_metadata() {


what about something like ?

let source_inode = source.symlink_metadata().map_err(|_| MvError::NoSuchFile(source.quote().to_string()).into())?;

sylvestre · 2023-04-01T11:31:20Z

src/uu/mv/src/mv.rs

+ if source_inode.eq(&target_inode) {
+ is_hard_link = true;
+ }


Suggested change

if source_inode.eq(&target_inode) {

is_hard_link = true;

}

is_hard_link = source_inode.ino() == target_inode;

sylvestre · 2023-04-01T11:33:00Z

fails on windows:


error[E0599]: no method named `ino` found for struct `Metadata` in the current scope
   --> src\uu\mv\src/mv.rs:267:56
    |
267 |                     let target_inode = target_metadata.ino();
    |                                                        ^^^ method not found in `Metadata`

haciek · 2023-04-01T12:19:08Z

fails on windows:


error[E0599]: no method named `ino` found for struct `Metadata` in the current scope
   --> src\uu\mv\src/mv.rs:267:56
    |
267 |                     let target_inode = target_metadata.ino();
    |                                                        ^^^ method not found in `Metadata`

I'm not really good with windows, should I make the check for hard links run on Unix only or is there a better way to handle this error?

github-actions · 2023-04-05T19:40:35Z

GNU testsuite comparison:

Congrats! The gnu test tests/mv/force is no longer failing!
Congrats! The gnu test tests/mv/i-4 is no longer failing!
GNU test failed: tests/mv/i-1. tests/mv/i-1 is passing on 'main'. Maybe you have to rebase?

github-actions · 2023-04-05T20:50:29Z

GNU testsuite comparison:

Congrats! The gnu test tests/mv/force is no longer failing!
Congrats! The gnu test tests/mv/i-4 is no longer failing!
GNU test failed: tests/mv/i-1. tests/mv/i-1 is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/rm/rm1. tests/rm/rm1 is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

github-actions · 2023-04-06T07:46:34Z

GNU testsuite comparison:

Congrats! The gnu test tests/mv/force is no longer failing!
Congrats! The gnu test tests/mv/i-4 is no longer failing!
GNU test failed: tests/mv/i-1. tests/mv/i-1 is passing on 'main'. Maybe you have to rebase?

tertsdiepraam · 2023-04-07T10:20:09Z

src/uu/mv/src/mv.rs

@@ -420,7 +445,7 @@ fn rename(
 OverwriteMode::NoClobber => return Ok(()),
 OverwriteMode::Interactive => {
 if !prompt_yes!("overwrite {}?", to.quote()) {
- return Err(io::Error::new(io::ErrorKind::Other, ""));
+ return Ok(());


I think this is the change that makes the CI fail. We have a mechanism to turn a Result into an exit code. Ok will be 0 and an error usually 1. You can get around this with set_exit_code maybe. Or you could add a new variant Skipped (or something similar) to the MvError enum and return that here.

tertsdiepraam · 2023-04-07T10:58:56Z

src/uu/mv/src/mv.rs

@@ -1,4 +1,3 @@
-// This file is part of the uutils coreutils package.


Please don't remove this

cakebaker · 2023-05-09T14:20:51Z

Closing this PR because the hard link check has been implemented as part of #4831 in the meantime.

Thanks anyway for your PR :)

sylvestre reviewed Mar 31, 2023

View reviewed changes

sylvestre reviewed Apr 1, 2023

View reviewed changes

haciek marked this pull request as draft April 1, 2023 12:31

haciek marked this pull request as ready for review April 5, 2023 18:43

haciek and others added 4 commits April 5, 2023 21:55

mv: hard link check

d9e3594

mv: test same file hard link

8cdbb00

mv: fix err handling

a37b860

mv: fix hard link check on windows

f468b24

sylvestre force-pushed the mv_symlink_fix branch from 47a54ec to f468b24 Compare April 5, 2023 19:55

tertsdiepraam reviewed Apr 7, 2023

View reviewed changes

src/uu/mv/src/mv.rs

@@ -1,4 +1,3 @@

// This file is part of the uutils coreutils package.

Copy link

Member

tertsdiepraam Apr 7, 2023

Choose a reason for hiding this comment

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

Please don't remove this

cakebaker closed this May 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mv: hard link check #4697

mv: hard link check #4697

haciek commented Mar 31, 2023

github-actions bot commented Mar 31, 2023

sylvestre commented Mar 31, 2023

haciek commented Mar 31, 2023

sylvestre Mar 31, 2023

github-actions bot commented Apr 1, 2023

sylvestre Apr 1, 2023

sylvestre Apr 1, 2023

sylvestre commented Apr 1, 2023

haciek commented Apr 1, 2023

github-actions bot commented Apr 5, 2023

github-actions bot commented Apr 5, 2023

github-actions bot commented Apr 6, 2023

tertsdiepraam Apr 7, 2023

tertsdiepraam Apr 7, 2023

cakebaker commented May 9, 2023

		@@ -1,4 +1,3 @@
		// This file is part of the uutils coreutils package.

mv: hard link check #4697

mv: hard link check #4697

Conversation

haciek commented Mar 31, 2023

github-actions bot commented Mar 31, 2023

sylvestre commented Mar 31, 2023

haciek commented Mar 31, 2023

sylvestre Mar 31, 2023

Choose a reason for hiding this comment

github-actions bot commented Apr 1, 2023

sylvestre Apr 1, 2023

Choose a reason for hiding this comment

sylvestre Apr 1, 2023

Choose a reason for hiding this comment

sylvestre commented Apr 1, 2023

haciek commented Apr 1, 2023

github-actions bot commented Apr 5, 2023

github-actions bot commented Apr 5, 2023

github-actions bot commented Apr 6, 2023

tertsdiepraam Apr 7, 2023

Choose a reason for hiding this comment

tertsdiepraam Apr 7, 2023

Choose a reason for hiding this comment

cakebaker commented May 9, 2023