mv: Change error message

[PThorpe92]

Issue: mv: error message "Directory not empty" is confusing #5102

Error message changed to: "A directory with the same name exists at destination"

[sylvestre]

Could you please add a test to make sure we don't regress? thanks :)

[PThorpe92]

A test that this is the intended behavior?

 ~/Projects  mkdir -p proj/temp                                                                                                                             Thu 20 Jul 2023 10:49:42 AM EDT
 ~/Projects  mkdir -p proj2/temp                                                                                                                                Thu 20 Jul 2023 10:49:49 AM EDT
 ~/Projects  touch proj/temp/foo                                                                                                                                Thu 20 Jul 2023 10:50:00 AM EDT
 ~/Projects  touch proj2/temp/bar                                                                                                                               Thu 20 Jul 2023 10:50:09 AM EDT
 ~/Projects  ./uutils/target/debug/mv proj/temp proj2                                                                                                           Thu 20 Jul 2023 10:50:14 AM EDT
./uutils/target/debug/mv: cannot move 'proj/temp' to 'proj2/temp': A directory with the same name exists at destination

[github-actions[bot]]

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm2 is no longer failing!
GNU test failed: tests/mv/dir2dir. tests/mv/dir2dir is passing on 'main'. Maybe you have to rebase?

[sylvestre]

yeah and it seems that you regressed one of the GNU test. if it makes sense, don't hesitate to update the error message in the GNU test like we are doing in https://github.com/uutils/coreutils/blob/main/util/build-gnu.sh#L168

[PThorpe92]

Edit: Ok I thought I was missing something.. Looks like that took care of the GNU test.

looks like the only failed test was an emulator issue

 Error: Timeout waiting for emulator to boot.

[github-actions[bot]]

GNU testsuite comparison:

Skipping an intermittent issue tests/tail-2/inotify-dir-recreate

[sylvestre]

is it ready to be merged? sorry for the latency :(

[PThorpe92]

After looking at the issue again. Can I confirm that

 mv: cannot overwrite 'e/dir': Directory not empty

is the preferred message, before I change it? That seemed to be the verdict in the GNU thread.

also, @tertsdiepraam sorry this may not be the best place for this, but you will need to approve this anyway. just a reminder from @eza for you to publish uutils-term-grid to crates.io, when you get a chance ;)

[tertsdiepraam]

What GNU thread are you referring to? If GNU decides on some behaviour, we can probably match it.

also, @tertsdiepraam sorry this may not be the best place for this, but you will need to approve this anyway. just a reminder from @eza for you to publish uutils-term-grid to crates.io, when you get a chance ;)

Ah of course! I'll try to make that a priority this weekend.

[PThorpe92]

The referenced thread in the issue

https://lists.gnu.org/archive/html/bug-coreutils/2023-07/msg00030.html

If I can get a thumbs up, I'll change it to: mv: cannot overwrite 'e/dir': Directory not empty (the agreed upon msg in the thread). Just wanted to double check with you on the final decision. Personally I think it's a much better message. Also, at some point there will no longer be the need for the sed command, although I'm not sure exactly how long it will take upstream.

Ah of course! I'll try to make that a priority this weekend.

Thanks :) Not a huge rush but we would definitely like to make the switch before the upcoming 1.0 release. We really appreciate it :heart:

[tertsdiepraam]

Yeah I like message in the GNU thread! Let's go for that!

[PThorpe92]

I know it looks like unnecessary code duplication, but I could not get the show! macro to work without repeating

   match multi_progress {
          Some(ref pb) => pb.suspend(|| show!(e)),
          None => show!(e),
    };

in both branches. If there is a better way to accomplish this, or if you would prefer it be a match statement, please let me know.

Note: that added semicolon in tests/by-util/test_split was just a random clippy suggestion.

[github-actions[bot]]

GNU testsuite comparison:

Congrats! The gnu test tests/mv/dir2dir is no longer failing!

[PThorpe92]

Sorry for the noise, would you like me to rebase that into one commit?

[sylvestre]

don't bother, we can squash them

[github-actions[bot]]

GNU testsuite comparison:

Congrats! The gnu test tests/mv/dir2dir is no longer failing!

[PThorpe92]

@tertsdiepraam with the work-around, did you still want a note about moving to io::Error::DirectoryNotEmpty when it becomes avail in stable Rust?

[tertsdiepraam]

Yeah I think that's a good idea. Using thay llt would still be cleaner than the workaround I think.

[PThorpe92]

Should be all set. Nushell PR is ready, when this is merged. (based it off my main branch, so these changes are included)

[tertsdiepraam]

Looks good! I think you might have missed this comment of mine:

The GNU list also mentions the following error codes: EDQUOT, EISDIR, ENOSPC, EEXIST, ENOTEMPTY, EMLINK and ETXTBSY, which I think they all made special cases. We could do the same.

I think it would be nice if we handle all of these, like ENOTEMPTY, just like GNU does.

[PThorpe92]

Ok I re-read the GNU list.

All things considered, how about if we go back to something like
coreutils 5.93, except output strerror (errno) too? That is, something
like this:

    mv: cannot overwrite 'e/dir': Directory not empty

This focuses the user on the problem, avoiding confusion from the
irrelevant source file name. We'd use this format if renameat fails with
an errno that means the problem must be with the destination, and stick
with the current format otherwise. Affected errno values would be
EDQUOT, EISDIR, ENOSPC, EEXIST, ENOTEMPTY.  (EMLINK and ETXTBSY are added later)

Meaning you would like the output changed where the diagnostic could be improved if it's known to refer to the destination. ? For the other equivalent MvErrors

[tertsdiepraam]

Yeah it looks like that's what GNU did.

[PThorpe92]

Ok, but not only in the circumstances where it's being overwritten? It sounded like what they were referring to was removing the source file from the error message, in all the messages where the issue is @ destination. I'll see how many places this applies. It will require some more sed commands in build_gnu.sh because I don't believe this is the current upstream behavior yet.

[PThorpe92]

@tertsdiepraam Verifying an example of what you would like changed.

mv: cannot move '../test1/file' to 'test1/file': not replacing 'test1/file'

It refers to the destination. Sorry I haven't had time to really dig into this. Were they explicit in what error message they would use? Did they intend on using

cannot overwrite {}: {error_code}

so in the case of the message above, it would instead be:

cannot overwrite 'test1/file': not replacing 'test1/file'

Just confirming, because looking for every occurrence, and then some of those will also have to match on more libc error values, which I noticed will act different in tests (when multiple files are sent and different overwrite modes,etc), will definitely end up being a non-trivial change. Is there any behavior you can think of that GNU might have been referring to where that message wouldn't be appropriate?

also, I am going to remove these changes from the nushell PR and submit that, because this might take longer than expected.

[tertsdiepraam]

I think what thye did was change the message the same way that you did, but not just for ENOTEMPTY, but for all other error variants as well. I don't think they hanged any errors elsewhere. Does that clarify it?

[github-actions[bot]]

GNU testsuite comparison:

Congrats! The gnu test tests/mv/dir2dir is no longer failing!

[github-actions[bot]]

GNU testsuite comparison:

Congrats! The gnu test tests/mv/dir2dir is no longer failing!

[github-actions[bot]]

GNU testsuite comparison:

Congrats! The gnu test tests/mv/dir2dir is no longer failing!

[github-actions[bot]]

GNU testsuite comparison:

GNU test failed: tests/mv/dir2dir. tests/mv/dir2dir is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/tail/inotify-dir-recreate