Skip to content

Conversation

@sylvestre
Copy link
Contributor

@sylvestre sylvestre commented Aug 4, 2020

Real life test case:

set -e

rm -rf dir dir-copy-archive-rust
mkdir dir
echo "a" > dir/1
echo "b" > dir/2
touch -d "2 hours ago" dir/1
chmod 705 dir/*
cd dir
ln -s 1 1.link
ln -s 2 2.link
cd -
cp --archive dir dir-copy-archive
ls -al dir-copy-archive|grep "rwx---r-x"
ls -al dir-copy-archive|grep "1.link -> 1"
./target/debug/coreutils cp --archive -v dir dir-copy-archive-rust
ls -al dir-copy-archive-rust|grep "rwx---r-x"
ls -al dir-copy-archive-rust|grep "1.link -> 1"
ls -al dir-copy-archive-rust|grep "2.link -> 2"

@sylvestre
Copy link
Contributor Author

Made progress, I am ignoring the error when it is a symlink.

@sylvestre sylvestre changed the title WIP feature(cp): implement archive feature(cp): implement archive Sep 26, 2020
@sylvestre sylvestre requested a review from Arcterus September 27, 2020 07:53
@sylvestre sylvestre requested a review from rivy October 25, 2020 16:09
@sylvestre
Copy link
Contributor Author

I am planning to land this Thursday 5st if no objection!

@rivy
Copy link
Member

rivy commented Nov 3, 2020

I haven't really had the time to review this... (busy with COVID uptick at the hospitals).
There are a few style notes/warnings and several CodeCov comments about new, none-test-covered, code.
And I haven't had the chance to actually run/test the new option.
I should be able to get to it over next weekend if that's not too long.

Copy link
Member

@rivy rivy left a comment

Choose a reason for hiding this comment

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

Minor syntax change request, but I'm concerned that the tests are all "linux" gated.

let path_to_check = path_to_new_symlink.to_str().unwrap();
assert_eq!(at.read(&path_to_check), "Hello, World!\n");
}

Copy link
Member

Choose a reason for hiding this comment

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

So, all the tests target only "linux"? ☹️

We definitely need to be testing all platforms. If there is some difficulty in getting them to work on MacOS/Windows, I can offer some time to help at a later date.

And, re-reading the docs, I think target_os = "unix" is incorrect; it should be target_os = "linux".

"unix" is a target_family.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I don't know how to fix these :/

Symlinks are tricky on Windows

@sylvestre sylvestre requested a review from rivy November 29, 2020 22:21
@sylvestre sylvestre changed the title feature(cp): implement archive feature(cp): implement archive & -L Nov 29, 2020
@sylvestre
Copy link
Contributor Author

sylvestre commented Dec 6, 2020

Sorry, I know this isn't perfect but this works and it is improving the overall code coverage.
Landing as it has been around since August

@sylvestre sylvestre merged commit 0cfb83a into uutils:master Dec 6, 2020
@sylvestre sylvestre deleted the archive3 branch December 6, 2020 16:16
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.

3 participants