Skip to content

Conversation

@nekluu
Copy link

@nekluu nekluu commented Jan 17, 2026

Description

This PR addresses a TODO in copy_direntry regarding error handling during directory traversal.

Currently, if a file is missing (e.g., deleted by another process) during a copy operation, cp terminates immediately. This change allows cp to show a warning and continue with the remaining files when a NotFound error occurs, consistent with how PermissionDenied is handled.

Changes

  • Added a match arm for io::ErrorKind::NotFound in copy_direntry.
  • Ensured the process continues instead of returning an Err early.

Related Issue

Addresses the TODO comment in src/uu/cp/src/copydir.rs.

@Ecordonnier
Copy link
Collaborator

"Related Issue
Addresses the TODO comment in src/uu/cp/src/cp.rs."

-> what do you mean? The TODO this PR addresses is in copydir.rs, not in cp.rs.

Also since this PR addresses the TODO, can the TODO now be removed?

@Ecordonnier Ecordonnier self-assigned this Jan 18, 2026
@github-actions
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/shuf/shuf-reservoir (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/sort/sort-stale-thread-mem (passes in this run but fails in the 'main' branch)

@codspeed-hq
Copy link

codspeed-hq bot commented Jan 18, 2026

Merging this PR will degrade performance by 26.25%

❌ 7 regressed benchmarks
✅ 275 untouched benchmarks
⏩ 38 skipped benchmarks1

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

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Memory sort_mixed_data[500000] 22.5 MB 27.2 MB -17.15%
Memory sort_ascii_only[500000] 22.2 MB 28.3 MB -21.74%
Memory sort_long_line[160000] 724.4 KB 982.3 KB -26.25%
Memory sort_accented_data[500000] 22.1 MB 28.3 MB -21.78%
Memory sort_key_field[500000] 45.8 MB 51.8 MB -11.46%
Memory cp_large_file[16] 107.6 KB 116.9 KB -8%
Simulation cp_large_file[16] 369.8 µs 383.3 µs -3.52%

Comparing nekluu:main (9ad27a2) with main (799de29)

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.

@nekluu
Copy link
Author

nekluu commented Jan 18, 2026

"Related Issue Addresses the TODO comment in src/uu/cp/src/cp.rs."

-> what do you mean? The TODO this PR addresses is in copydir.rs, not in cp.rs.

Also since this PR addresses the TODO, can the TODO now be removed?

I apologize, I wrote the file path incorrectly. My mistake. I keep the TODO because I thought you might want to support other error types in the future. If you think no further error support is needed, we can remove the TODO.

e,
"{}",
translate!(
"cp-error-cannot-stat-no-such-file",
Copy link
Collaborator

Choose a reason for hiding this comment

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

"cp-error-cannot-stat-no-such-file" is not defined in the translation file. It would need to be added (see truncate as example):

asteba@asteba-MS-7C75:~/dev/coreutils/src/uu$ rg "cannot-stat-no-such-file" .
./cp/src/copydir.rs
326:                                "cp-error-cannot-stat-no-such-file",

./truncate/locales/fr-FR.ftl
33:truncate-error-cannot-stat-no-such-file = impossible d'obtenir les informations de { $filename } : Aucun fichier ou répertoire de ce type

./truncate/locales/en-US.ftl
33:truncate-error-cannot-stat-no-such-file = cannot stat { $filename }: No such file or directory

./truncate/src/truncate.rs
253:                    translate!("truncate-error-cannot-stat-no-such-file", "filename" => reference_path.quote()),

"source" => entry.source_relative.quote()
),
));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually the TODO and the comment that this function should terminate if there is any kind of error except certain errors seems wrong to me. This does not match the behavior of GNU coreutils and the POSIX specification.

This was added originally in b89e8e5 / #3973

I think the function should continue on any errors to match the behavior of GNU coreutils, or am I missing something?

See the specification:
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/cp.html
""When a failure occurs during the copying of a file hierarchy, cp is required to attempt to copy files that are on the same level in the hierarchy or above the file where the failure occurred. It is unspecified if cp shall attempt to copy files below the file where the failure occurred (which cannot succeed in any case)."

And multiple places in the DESCRIPTION say:

"cp shall write a diagnostic message to standard error, do nothing more with source_file, and go on to any remaining files.""

@Ecordonnier
Copy link
Collaborator

Ecordonnier commented Jan 18, 2026

Suggestion:

                 // In case we do --archive, we might copy the symlink
                 // before the file itself
             } else {
-                // At this point, `path` is just a plain old file.
-                // Terminate this function immediately if there is any
-                // kind of error *except* a "permission denied" error.
+                // POSIX requires: "When a failure occurs during the copying of a file
+                // hierarchy, cp is required to attempt to copy files that are on the
+                // same level in the hierarchy or above the file where the failure occurred."
+                // (POSIX.1-2017, cp utility, RATIONALE)
+               //
+               // Therefore, show errors but continue to copy remaining files in the directory.
-                // TODO What other kinds of errors, if any, should
-                // cause us to continue walking the directory?
                 match err {
                     CpError::IoErrContext(e, _) if e.kind() == io::ErrorKind::PermissionDenied => {
                         show!(uio_error!(
@@ -328,7 +330,10 @@ fn copy_direntry(
                             ),
                         ));
                     }
-                    e => return Err(e),
+                    e => {
+                        // Any other error: show it but continue with remaining files
+                        show_error!("{e}");
+                    }
                 }
             }
         }

@Ecordonnier
Copy link
Collaborator

Also can you please add a test (something like the code below) which triggers the NotFound error?

#[test]
fn test_copy_dir_with_dangling_symlink() {
    let (at, mut ucmd) = at_and_ucmd!();
    
    at.mkdir("src");
    at.touch("src/file1.txt");
    at.touch("src/file2.txt");
    
    // Create a dangling symlink (points to non-existent file)
    at.symlink_file("nonexistent", "src/dangling");
    
    // Copy with -L (follow/dereference symlinks) and -R (recursive)
    ucmd.args(&["-L", "-R", "src", "dst"])
        .fails()
        .stderr_contains("cannot stat")
        .stderr_contains("src/dangling")
        .stderr_contains("No such file or directory");
    
    // Other files should still be copied
    assert!(at.file_exists("dst/file1.txt"));
    assert!(at.file_exists("dst/file2.txt"));
    assert!(!at.file_exists("dst/dangling"));
}

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