cp: fix the result of inodes are not the same when preserve links is flagged #5064
Conversation
src/uu/cp/src/cp.rs
Outdated
| // FIXME: compare sources by the actual file they point to, not their path. (e.g. dir/file == dir/../dir/file in most cases) | ||
| show_warning!("source {} specified more than once", source.quote()); | ||
| } else if preserve_hard_links && source.is_symlink() { | ||
| if let Some(new_source) = seen_sources.get(&source.read_link()?) { |
There was a problem hiding this comment.
please add a comment explaining why you do that
thanks
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
I guess you saw that your change regressed some tests |
|
GNU testsuite comparison: |
Yap I saw |
|
GNU testsuite comparison: |
|
hi @sylvestre
and please review it. |
|
GNU testsuite comparison: |
|
Fails on android, just disable the test on android: |
|
GNU testsuite comparison: |
|
test_cp_issue_5031_case_1 & test_cp_issue_5031_case_2 are failing too :) |
|
GNU testsuite comparison: |
|
Hi @tertsdiepraam about the let found_hard_link = preserve_hardlinks(hard_links, &source_absolute, &dest)?;I think we can do an enhancement in another PR. |
|
Thanks for the fixes! The reason that I want |
|
hmmm ya... that function is some kind of "protecting", "helping" me, I'll try what I can do to fulfill your and my goal. |
|
Just to be clear: I'm not doubting your solution 😄 It looks good, but we shouldn't have two functions doing sort of the same thing. Thanks for taking a look! |
There was a problem hiding this comment.
Excellent! I think this is great now! I love that we're actually reducing the amount of code now (apart from the tests).
I have one more question about the part where you delete existing files, which I probably should have seen earlier, but I only noticed it now.
There are several follow up issues I'd like to open, but that we shouldn't look at now:
- We should clean up the number of parameters we're passing around.
- We should figure out what the issue with Android is 🤔
- We're breaking redox support here. I don't think this is a problem, because we've probably already done that since we don't test Redox in the CI. We should probably get that going first and then start fixing the builds for Redox.
src/uu/cp/src/cp.rs
Outdated
| // Consider the following files: | ||
| // | ||
| // * `src/f` - a regular file | ||
| // * `src/link` - a hard link to `src/f` | ||
| // * `dest/src/f` - a different regular file | ||
| // | ||
| // In this scenario, if we do `cp -a src/ dest/`, it is | ||
| // possible that the order of traversal causes `src/link` | ||
| // to get copied first (to `dest/src/link`). In that case, | ||
| // in order to make sure `dest/src/link` is a hard link to | ||
| // `dest/src/f` and `dest/src/f` has the contents of | ||
| // `src/f`, we delete the existing file to allow the hard | ||
| // linking. | ||
| if file_or_link_exists(dest) && file_or_link_exists(Path::new(new_source)) { | ||
| std::fs::remove_file(dest)?; | ||
| } |
There was a problem hiding this comment.
What if we move this down to below the handle_existing_dest call? Wouldn't that have the same behaviour? We should probably do the proper handling for existing destination files here too right?\
So we'd move the whole preserve hard links branch to below these lines:
if file_or_link_exists(dest) {
handle_existing_dest(source, dest, options, source_in_command_line)?;
}
or wouldn't that work?
There was a problem hiding this comment.
I don't think it will work due to this one (handle_existing_dest function) was protected by the preserve_hardlinks
There was a problem hiding this comment.
What do you mean with by protected? Do you mean that it wasn't called for hard links before? It does do some very specific stuff so it might indeed not work for hard links...
There was a problem hiding this comment.
Sorry for the late response,
I mean I tried to do as you said so like below
if file_or_link_exists(dest) {
handle_existing_dest(source, dest, options, source_in_command_line)?;
}
if options.preserve_hard_links() {
// if we encounter a matching device/inode pair in the source tree
// we can arrange to create a hard link between the corresponding names
// in the destination tree.
if let Some(new_source) = copied_files.get(
&FileInformation::from_path(source, options.dereference(source_in_command_line))
.context(format!("cannot stat {}", source.quote()))?,
) {
std::fs::hard_link(new_source, dest)?;
return Ok(());
};
}We could not do that
due to the handle_existing_dest function only deleting the destination file when the overwrite option was given.
so that function wasn't to act like it's named to deal with every scenario of existing destination As the comment of that function said depending on the options so ya... it does not solve the original problem (if I understand that function correctly)
There was a problem hiding this comment.
But it should honor those options right? For example, if we give -n to (GNU) cp this happens:
❯ exa -Til
8732801 drwxr-xr-x - terts 29 aug 13:35 .
9036332 drwxr-xr-x - terts 29 aug 13:36 ├── dest
9034135 .rw-r--r-- 0 terts 29 aug 13:35 │ ├── f
9034136 .rw-r--r-- 0 terts 29 aug 13:37 │ └── g
9036331 drwxr-xr-x - terts 29 aug 13:34 └── src
9034134 .rw-r--r-- 0 terts 29 aug 13:34 ├── f
9034134 .rw-r--r-- 0 terts 29 aug 13:34 └── g
❯ cp -n --preserve=links src/f src/g dest
cp: not replacing 'dest/f'
cp: not replacing 'dest/g'
Note how src/f and src/g have the same inode so are hardlinks. Seems to me like the treatment of existing files is the same whether we are dealing with a second hard or not.
There was a problem hiding this comment.
Sorry, small correction. The actual case that shows a difference is:
❯ exa -Til 13:46:18
8732801 drwxr-xr-x - terts 29 aug 13:35 .
9036332 drwxr-xr-x - terts 29 aug 13:46 ├── dest
9034136 .rw-r--r-- 0 terts 29 aug 13:45 │ └── g
9036331 drwxr-xr-x - terts 29 aug 13:34 └── src
9034134 .rw-r--r-- 0 terts 29 aug 13:34 ├── f
9034134 .rw-r--r-- 0 terts 29 aug 13:34 └── g
where GNU does this:
❯ cp -n --preserve=links src/f src/g dest
cp: not replacing 'dest/g'
and this PR does this:
❯ cp -n --preserve=links src/f src/g dest
(No output, src/g is copied)
I think this would also be a nice test case to have in our test suite.
There was a problem hiding this comment.
OK, you are right.
let me make some modifications to fulfill that goal. 🙇🏻
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
Hi @tertsdiepraam Please help review while you have time. Thank you 🙇♂️ |
There was a problem hiding this comment.
Thanks so much for sticking with this! Sorry it's taking so many iterations! Could you explain the change in handle_existing_dest? If we change the behaviour for all sorts of files, I'd like to see some commands for GNU cp that I can run to verify this behaviour?
src/uu/cp/src/cp.rs
Outdated
| OverwriteMode::Clobber(ClobberMode::Standard) => { | ||
| // Consider the following files: | ||
| // | ||
| // * `src/f` - a regular file | ||
| // * `src/link` - a hard link to `src/f` | ||
| // * `dest/src/f` - a different regular file | ||
| // | ||
| // In this scenario, if we do `cp -a src/ dest/`, it is | ||
| // possible that the order of traversal causes `src/link` | ||
| // to get copied first (to `dest/src/link`). In that case, | ||
| // in order to make sure `dest/src/link` is a hard link to | ||
| // `dest/src/f` and `dest/src/f` has the contents of | ||
| // `src/f`, we delete the existing file to allow the hard | ||
| // linking. | ||
| if !source_in_command_line { | ||
| fs::remove_file(dest)?; | ||
| } | ||
| } |
There was a problem hiding this comment.
This bit is not specific to hard links, right? So did we just miss it before? If it is only for hard links, should we make an if for it to check? Do you have test cases for this both for hard links and not hard links?
The source_in_command_line boolean also seems weird if that matters? Have you compared this behaviour with recursively copying directories with some hard links?
There was a problem hiding this comment.
I am sorry I used the wrong condition, now I changed 🙇🏻
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
Looks great. Do you know why GNU test: tests/cp/link-preserve.sh doesn't pass ? |
|
GNU testsuite comparison: |
sorry for the late response, I re-run the test and everything seems fine. |
fix #5031