mv: gnu test case `part-hardlink` fix #6632

matrixhead · 2024-08-11T09:29:05Z

fix for #6631

behaviours changed

mv will now preserve links during an inter-device move.
Previously, mv would silently fail to create a backup when no-target-directory was specified and the destination ended with a / This patch resolves that issue.
The progress bar will now work for simple inter-device file moves as well.

src/uu/mv/src/mv.rs

github-actions · 2024-08-11T09:49:27Z

GNU testsuite comparison:

Congrats! The gnu test tests/mv/hard-link-1 is no longer failing!
Congrats! The gnu test tests/mv/part-hardlink is no longer failing!
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

src/uu/mv/src/mv.rs

tests/by-util/test_mv.rs

sylvestre · 2024-08-11T10:58:56Z

Bravo

Congrats! The gnu test tests/mv/hard-link-1 is no longer failing!
Congrats! The gnu test tests/mv/part-hardlink is no longer failing!

github-actions · 2024-08-12T18:48:37Z

GNU testsuite comparison:

Congrats! The gnu test tests/mv/hard-link-1 is no longer failing!
Congrats! The gnu test tests/mv/part-hardlink is no longer failing!

github-actions · 2024-08-12T19:08:22Z

GNU testsuite comparison:

Congrats! The gnu test tests/mv/hard-link-1 is no longer failing!
Congrats! The gnu test tests/mv/part-hardlink is no longer failing!

github-actions · 2024-08-13T04:46:17Z

GNU testsuite comparison:

Congrats! The gnu test tests/mv/hard-link-1 is no longer failing!
Congrats! The gnu test tests/mv/part-hardlink is no longer failing!

src/uu/mv/src/mv.rs

github-actions · 2024-08-14T04:30:58Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/mv/hard-link-1 is no longer failing!
Congrats! The gnu test tests/mv/part-hardlink is no longer failing!

BenWiederhake

Thank you for tackling these issues! However, combining these three behavioral changes seems like a bad idea, given that each of them have delicate consequences.

It seems that get_dir_content is the entirely wrong tool to approach mv, and it causes incorrect behavior.

Finally, it seems you fell into a common trap / bad pattern: Guessing a potential error condition (and not detecting others), then doing an action (and badly handling most error conditions.) This causes unhelpful error messages and unnecessary syscalls.

BenWiederhake · 2024-09-07T19:56:05Z

src/uu/mv/src/mv.rs

+        }
+        // We fall back to copying and then removing.
+        else {


Please move the comment into the else clause. Currently, it looks misleading, as if the if is now complete.

BenWiederhake · 2024-09-07T20:18:49Z

src/uu/mv/src/mv.rs

+    if !from.exists() {
+        if let Some(msg) = from.to_str() {
+            let msg = format!("Path \"{}\" does not exist or you don't have access!", msg);
+            return Err(FsXError::new(FsXErrorKind::NotFound, &msg));
+        }
+        return Err(FsXError::new(
+            FsXErrorKind::NotFound,
+            "Path does not exist or you don't have access!",
+        ));
+    }
+
+    let dir_content = get_dir_content(from)?;


This misses some error conditions, which leads to the following confusing behavior, both on main and on this PR:

$ rm -rf /tmp/foobar/ && cargo run -q --features mv mv -v /root/ /tmp/foobar mv: Permission denied [$? = 1]

Which permission was denied? When accessing which file? Is the permission problem with the source or with the destination? I hope you can see how this error message might be confusing when there's many files being moved/copied.

GNU offers a much more user-friendly error message:

$ rm -rf /tmp/foobar/ && mv -v /root/ /tmp/foobar created directory '/tmp/foobar' mv: cannot access '/root/': Permission denied [$? = 1]

The same holds with the error message on line 770: It doesn't even mention the filename.

Please instead handle errors on the actions that cause them. In this case, handling the error when it happens (instead of trying to guess) completely eliminates the need for the from.exists() call.

BenWiederhake · 2024-09-07T20:47:58Z

src/uu/mv/src/mv.rs

+            if let Some(file_name) = from.file_name().and_then(|file_name| file_name.to_str()) {
+                file_name.to_string()
+            } else {
+                return Err(FsXError::new(
+                    FsXErrorKind::InvalidFileName,
+                    "Invalid file name",
+                ));
+            };


File names are not necessarily valid UTF-8. This code would raise an error if such a file name is encountered. Does that mean non-UTF8-filenames cause an error when enabling progress bars?

We already don't have good handling for non-UTF-8 filenames, but let's try to avoid making the situation worse.

BenWiederhake · 2024-09-07T20:50:01Z

tests/by-util/test_mv.rs

+
+        // Ensure that the folder structure is preserved, files are copied, and their contents remain intact.
+        assert_eq!(
+            read_to_string(other_fs_tempdir.path().join("a/b/d"),).expect("Unable to read other_fs_file"),


Why is there a spurious comma?

Suggested change

read_to_string(other_fs_tempdir.path().join("a/b/d"),).expect("Unable to read other_fs_file"),

read_to_string(other_fs_tempdir.path().join("a/b/d")).expect("Unable to read other_fs_file"),

BenWiederhake · 2024-09-07T20:50:10Z

tests/by-util/test_mv.rs

+            "d"
+        );
+        assert_eq!(
+            read_to_string(other_fs_tempdir.path().join("a/b/c/e"),).expect("Unable to read other_fs_file"),


BenWiederhake · 2024-09-07T20:51:38Z

src/uu/mv/src/mv.rs

+    let result_file_move = if let Some(new_source) = moved_files.get(file_info) {
+        fs::hard_link(new_source, to)?;
+        Ok(file_info.file_size())
+    } else if let Some(progress_bar) = progress_bar {


If you write code for the progress-bar case, then please add a test that exercises the progress-bar case.

BenWiederhake · 2024-09-07T21:08:31Z

src/uu/mv/src/mv.rs

+        ));
+    }
+
+    let dir_content = get_dir_content(from)?;


get_dir_content does not return sufficient information. GNU mv copies/moves as many files/directories as it can, even if it first encounter some files where it has insufficient permissions:

$ mv -v source/ /tmp created directory '/tmp/source' created directory '/tmp/source/d1' mv: cannot access 'source/d1': Permission denied created directory '/tmp/source/d3' mv: cannot access 'source/d3': Permission denied copied 'source/f2' -> '/tmp/source/f2' [$? = 1] $ ls source/ # source has NOT been deleted! d1 d3 f2 $ ls /tmp/source/ d1 d3 f2

However, get_dir_content bails completely if there are any permission issues:

$ cargo run -q --features mv mv -v source/ /tmp mv: cannot move 'source/' to '/tmp/source': Permission denied [$? = 1] $ ls /tmp/source/ ls: cannot access '/tmp/source/': No such file or directory [$? = 2]

And the final nail in the coffin is that get_dir_content insists to first read the entire filesystem structure, which uses up potentially vast amounts of memory, and gives ample opportunity for all kinds of TOCTOU races: https://docs.rs/fs_extra/latest/fs_extra/dir/struct.DirContent.html

Therefore, we should avoid using get_dir_content entirely. I'm not even sure it can be used correctly.

Yes, you're right. Instead of using get_dir_content, we could follow the approach used in uu_cp, which involves walking through the directory, copying files, and creating folders as we go.

BenWiederhake

Whoops, I meant to set the "Request changes" flag. See above for my reasoning. In particular, get_dir_content must go.

May I suggest to split the tree changes into two or three PRs?

github-actions · 2024-09-08T11:36:52Z

GNU testsuite comparison:

GNU test failed: tests/timeout/timeout. tests/timeout/timeout is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/mv/hard-link-1 is no longer failing!
Congrats! The gnu test tests/mv/part-hardlink is no longer failing!

github-actions · 2024-09-11T08:13:46Z

GNU testsuite comparison:

GNU test failed: tests/mv/backup-dir. tests/mv/backup-dir is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

github-actions · 2024-09-11T08:49:03Z

GNU testsuite comparison:

GNU test failed: tests/mv/backup-dir. tests/mv/backup-dir is passing on 'main'. Maybe you have to rebase?

matrixhead · 2024-09-12T06:22:17Z

@BenWiederhake I cleaned up this PR. Now, it focuses solely on the directory copying part. I'll work on verbose output and error reporting next. For now, could you let me know if this approach looks okay?

github-actions · 2024-09-12T06:39:31Z

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

src/uu/mv/src/mv.rs

github-actions · 2024-09-12T09:44:23Z

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

github-actions · 2024-09-17T10:49:51Z

GNU testsuite comparison:

Skip an intermittent issue tests/rm/rm1 (fails in this run but passes in the 'main' branch)
GNU test failed: tests/timeout/timeout. tests/timeout/timeout is passing on 'main'. Maybe you have to rebase?

github-actions · 2024-09-19T17:01:44Z

GNU testsuite comparison:

GNU test failed: tests/timeout/timeout. tests/timeout/timeout is passing on 'main'. Maybe you have to rebase?

github-actions · 2024-09-22T10:46:02Z

GNU testsuite comparison:

GNU test failed: tests/mv/perm-1. tests/mv/perm-1 is passing on 'main'. Maybe you have to rebase?

Co-authored-by: Sylvestre Ledru <sylvestre@debian.org>

sylvestre reviewed Aug 11, 2024

View reviewed changes

src/uu/mv/src/mv.rs Outdated Show resolved Hide resolved

sylvestre requested a review from BenWiederhake August 11, 2024 09:31

sylvestre reviewed Aug 11, 2024

View reviewed changes

src/uu/mv/src/mv.rs Outdated Show resolved Hide resolved

sylvestre reviewed Aug 11, 2024

View reviewed changes

tests/by-util/test_mv.rs Show resolved Hide resolved

matrixhead force-pushed the part-hardlink branch from f32325e to 1da64ed Compare August 12, 2024 18:47

matrixhead force-pushed the part-hardlink branch from 1da64ed to 2c7bee1 Compare August 13, 2024 04:26

matrixhead requested a review from sylvestre August 13, 2024 04:47

sylvestre reviewed Aug 13, 2024

View reviewed changes

src/uu/mv/src/mv.rs Outdated Show resolved Hide resolved

matrixhead requested a review from sylvestre August 15, 2024 09:03

BenWiederhake mentioned this pull request Sep 7, 2024

mv: fails to create dir when missing read permissions #6684

Open

BenWiederhake reviewed Sep 7, 2024

View reviewed changes

BenWiederhake requested changes Sep 7, 2024

View reviewed changes

sylvestre force-pushed the part-hardlink branch from 4b9e64e to 666f8cb Compare September 8, 2024 11:17

matrixhead force-pushed the part-hardlink branch from 666f8cb to 119ce64 Compare September 11, 2024 07:47

sylvestre force-pushed the part-hardlink branch from 119ce64 to e8c9ffc Compare September 11, 2024 08:22

matrixhead marked this pull request as draft September 11, 2024 17:47

matrixhead force-pushed the part-hardlink branch from 3754a06 to 338ed28 Compare September 12, 2024 06:12

sylvestre reviewed Sep 12, 2024

View reviewed changes

src/uu/mv/src/mv.rs Outdated Show resolved Hide resolved

matrixhead and others added 8 commits September 22, 2024 12:58

mv: copy dir

11d5ae2

mv: gnu test backup-dir regression fix

7aa5983

minor changes

6148541

Update src/uu/mv/src/mv.rs

2fc1f76

Co-authored-by: Sylvestre Ledru <sylvestre@debian.org>

changed to MAIN_SEPARATOR

5aa3ece

added verbose context type to print verbose message

2472d9b

mv: added dir copy error messages and types

07e16c9

mv: tests and minor changes

dd06b70

sylvestre force-pushed the part-hardlink branch from 102a27d to dd06b70 Compare September 22, 2024 10:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mv: gnu test case `part-hardlink` fix #6632

mv: gnu test case `part-hardlink` fix #6632

matrixhead commented Aug 11, 2024

github-actions bot commented Aug 11, 2024

sylvestre commented Aug 11, 2024

github-actions bot commented Aug 12, 2024

github-actions bot commented Aug 12, 2024

github-actions bot commented Aug 13, 2024

github-actions bot commented Aug 14, 2024

BenWiederhake left a comment

BenWiederhake Sep 7, 2024

BenWiederhake Sep 7, 2024

BenWiederhake Sep 7, 2024

BenWiederhake Sep 7, 2024

BenWiederhake Sep 7, 2024

BenWiederhake Sep 7, 2024

BenWiederhake Sep 7, 2024

matrixhead Sep 8, 2024

BenWiederhake left a comment

github-actions bot commented Sep 8, 2024

github-actions bot commented Sep 11, 2024

github-actions bot commented Sep 11, 2024

matrixhead commented Sep 12, 2024

github-actions bot commented Sep 12, 2024

github-actions bot commented Sep 12, 2024

github-actions bot commented Sep 17, 2024

github-actions bot commented Sep 19, 2024

github-actions bot commented Sep 22, 2024

	read_to_string(other_fs_tempdir.path().join("a/b/d"),).expect("Unable to read other_fs_file"),
	read_to_string(other_fs_tempdir.path().join("a/b/d")).expect("Unable to read other_fs_file"),

mv: gnu test case part-hardlink fix #6632

Are you sure you want to change the base?

mv: gnu test case part-hardlink fix #6632

Conversation

matrixhead commented Aug 11, 2024

github-actions bot commented Aug 11, 2024

sylvestre commented Aug 11, 2024

github-actions bot commented Aug 12, 2024

github-actions bot commented Aug 12, 2024

github-actions bot commented Aug 13, 2024

github-actions bot commented Aug 14, 2024

BenWiederhake left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenWiederhake left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 8, 2024

github-actions bot commented Sep 11, 2024

github-actions bot commented Sep 11, 2024

matrixhead commented Sep 12, 2024

github-actions bot commented Sep 12, 2024

github-actions bot commented Sep 12, 2024

github-actions bot commented Sep 17, 2024

github-actions bot commented Sep 19, 2024

github-actions bot commented Sep 22, 2024

mv: gnu test case `part-hardlink` fix #6632

mv: gnu test case `part-hardlink` fix #6632