mv: improve the hardlink support by sylvestre · Pull Request #8296 · uutils/coreutils

sylvestre · 2025-07-01T21:52:27Z

No description provided.

github-actions · 2025-07-01T22:48:35Z

GNU testsuite comparison:

GNU test failed: tests/mv/dup-source. tests/mv/dup-source is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/mv/hard-link-1 is no longer failing!
Congrats! The gnu test tests/mv/part-fail is no longer failing!
Congrats! The gnu test tests/mv/part-hardlink is no longer failing!

cakebaker · 2025-07-02T07:41:29Z

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

Kudos, three passing GNU tests :) Unfortunately, there is also a regression:

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

cakebaker · 2025-07-02T09:00:37Z

tests/by-util/test_mv.rs

+        at.write("f", "file content");
+        at.hard_link("f", "g");


You already test moving a file and a hard link in test_mv_preserves_hardlinks_across_partitions and thus I wouldn't do it again in this test function.

cakebaker · 2025-07-02T09:18:41Z

tests/by-util/test_mv.rs

+        let moved_dir_a_file = other_fs_tempdir.path().join("a/1");
+        let moved_dir_second_file = other_fs_tempdir.path().join("b/1");
+        let moved_dir_a_file_metadata = metadata(&moved_dir_a_file).unwrap();
+        let moved_dir_second_file_metadata = metadata(&moved_dir_second_file).unwrap();


The naming with "a_file" and "second_file" feels a bit odd. Maybe it would be better to use names that contain "a1" and "b1", similar to the naming on lines 2096/2097?

tests/by-util/test_mv.rs

cakebaker · 2025-07-02T13:11:38Z

tests/by-util/test_mv.rs

+#[test]
+fn test_mv_error_handling_graceful_fallback() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.write("file1", "test content");
+
+    at.mkdir("target");
+
+    // Move with verbose output to test error handling messages
+    ucmd.arg("--verbose").arg("file1").arg("target").succeeds();
+
+    assert!(at.file_exists("target/file1"));
+    assert!(!at.file_exists("file1"));
+}


I don't know what the purpose of this test is: the function name and the comment indicate the test is related to some error handling, but there is no error. Currently, it looks like the test function is a duplicate of test_mv_move_file_into_dir.

cakebaker · 2025-07-02T13:15:44Z

tests/by-util/test_mv.rs

+#[test]
+#[cfg(unix)]
+fn test_mv_hardlink_scanning_with_permissions() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    // Create files that we can control permissions on
+    at.write("accessible_file", "content");
+    at.mkdir("target");
+
+    // Test with verbose mode to ensure graceful error handling
+    ucmd.arg("--verbose")
+        .arg("accessible_file")
+        .arg("target")
+        .succeeds();
+
+    assert!(at.file_exists("target/accessible_file"));
+}


This test function also looks like a duplicate of test_mv_move_file_into_dir.

cakebaker · 2025-07-02T13:26:21Z

tests/by-util/test_mv.rs

+#[test]
+#[cfg(unix)]
+fn test_mv_empty_hardlink_groups() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.write("single1", "content1");
+    at.write("single2", "content2");
+    at.mkdir("target");
+
+    // Test with files that have no hardlinks
+    ucmd.arg("--verbose")
+        .arg("single1")
+        .arg("single2")
+        .arg("target")
+        .succeeds();
+
+    assert!(at.file_exists("target/single1"));
+    assert!(at.file_exists("target/single2"));
+}


This test function also looks like a duplicate of test_mv_move_file_into_dir.

cakebaker · 2025-07-02T14:40:18Z

src/uu/mv/src/mv.rs

+                #[cfg(unix)]
+                {
+                    let (mut hardlink_tracker, hardlink_scanner) = create_hardlink_context();
+                    rename(
+                        source,
+                        target,
+                        opts,
+                        None,
+                        Some(&mut hardlink_tracker),
+                        Some(&hardlink_scanner),
                    )
-                })
+                    .map_err_context(|| {
+                        get_message_with_args(
+                            "mv-error-cannot-move",
+                            HashMap::from([
+                                ("source".to_string(), source.quote().to_string()),
+                                ("target".to_string(), target.quote().to_string()),
+                            ]),
+                        )
+                    })
+                }
+                #[cfg(not(unix))]
+                {
+                    rename(source, target, opts, None, None, None).map_err_context(|| {
+                        get_message_with_args(
+                            "mv-error-cannot-move",
+                            HashMap::from([
+                                ("source".to_string(), source.quote().to_string()),
+                                ("target".to_string(), target.quote().to_string()),
+                            ]),
+                        )
+                    })
+                }


I think you could use a similar approach as on lines 659 - 662 and use the same call of rename for both unix and non-unix.

cakebaker · 2025-07-02T14:40:58Z

src/uu/mv/src/mv.rs

+        #[cfg(unix)]
+        {
+            let (mut hardlink_tracker, hardlink_scanner) = create_hardlink_context();
+            rename(
+                source,
+                target,
+                opts,
+                None,
+                Some(&mut hardlink_tracker),
+                Some(&hardlink_scanner),
+            )
+            .map_err(|e| USimpleError::new(1, format!("{e}")))
+        }
+        #[cfg(not(unix))]
+        {
+            rename(source, target, opts, None, None, None)
+                .map_err(|e| USimpleError::new(1, format!("{e}")))
+        }


github-actions · 2025-07-03T13:12:24Z

GNU testsuite comparison:

GNU test failed: tests/mv/dup-source. tests/mv/dup-source is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/mv/hard-link-1 is no longer failing!
Congrats! The gnu test tests/mv/part-fail is no longer failing!
Congrats! The gnu test tests/mv/part-hardlink is no longer failing!

Should fix GNU tests/mv/part-hardlink.sh tests/mv/hard-link-1.sh Co-authored-by: Daniel Hofstetter <daniel.hofstetter@42dh.com>

github-actions · 2025-07-03T14:25:40Z

GNU testsuite comparison:

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