Skip to content

Conversation

@sylvestre
Copy link
Contributor

@sylvestre sylvestre commented Mar 25, 2023

Mentioned in issue #4627
touch a b && echo "n"|./target/debug/coreutils ln -i a b; echo $?

was 0, it is now 1
See:
coreutils/coreutils@7a69df8

(no problem reading the code here as there isn't much)

Matches the change upstream
7a69df88999bedd8e9fccf9f3dfa9ac6907fab66
Just like mv

Note that cp -i -u won't show the overwrite question

Matches the change upstream
7a69df88999bedd8e9fccf9f3dfa9ac6907fab66
Just like mv & cp

Matches the change upstream
7a69df88999bedd8e9fccf9f3dfa9ac6907fab66
GNU ln uses "replace" instead of "overwrite"
Copy link
Contributor

@cakebaker cakebaker left a comment

Choose a reason for hiding this comment

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

While reviewing I noticed that we use "overwrite" in the prompt of ln instead of "replace" and fixed it.

OverwriteMode::Interactive => {
if !prompt_yes!("overwrite {}?", to.quote()) {
return Ok(());
return Err(io::Error::new(io::ErrorKind::Other, ""));
Copy link
Member

Choose a reason for hiding this comment

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

Can we do something else than constructing an io::Error? This is going to give a weird error message isn't it? For a PR like this, I guess it's OK, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, there isn't any error message :)

$ touch a b && echo "n"|./target/debug/coreutils mv -i a b; echo $?
mv: overwrite 'b'? 
1

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/mv/i-1 is no longer failing!

Copy link
Collaborator

@jfinkels jfinkels left a comment

Choose a reason for hiding this comment

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

makes sense

@sylvestre sylvestre merged commit ef601fa into uutils:main Mar 26, 2023
@sylvestre sylvestre deleted the inter-error branch March 26, 2023 13:39
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.

4 participants