Skip to content

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

Merged
sylvestre merged 4 commits into
uutils:mainfrom
sylvestre:inter-error
Mar 26, 2023
Merged

Adjust -i behavior for ln, cp & mv #4630
sylvestre merged 4 commits into
uutils:mainfrom
sylvestre:inter-error

Conversation

@sylvestre

@sylvestre sylvestre commented Mar 25, 2023

Copy link
Copy Markdown
Contributor

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)

Just like mv

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

Matches the change upstream
7a69df8
Just like mv & cp

Matches the change upstream
7a69df8
GNU ln uses "replace" instead of "overwrite"

@cakebaker cakebaker left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread src/uu/mv/src/mv.rs
OverwriteMode::Interactive => {
if !prompt_yes!("overwrite {}?", to.quote()) {
return Ok(());
return Err(io::Error::new(io::ErrorKind::Other, ""));

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown

GNU testsuite comparison:

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

@jfinkels jfinkels left a comment

Copy link
Copy Markdown
Collaborator

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