Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust -i behavior for ln, cp & mv #4630

Merged
merged 4 commits into from
Mar 26, 2023
Merged

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.

@@ -420,7 +420,7 @@ fn rename(
OverwriteMode::NoClobber => return Ok(()),
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

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