bug#64785: Package: coreutils Version: 8.32-4+b1 program=mv

[Nir Oren]

I agree with Paul suggestion for an error message.
In any case, Coreutils 5.93 message was better than the current one

On Sat, Jul 22, 2023 at 8:37 PM Paul Eggert  wrote:

> On 2023-07-22 03:19, Pádraig Brady wrote:
> > Given the subtleties in this area,
> > I'd be reluctant to adjust diagnostics here.
>
> I looked into this a bit more. Given "mv dir e" where e/dir is an
> existing nonempty directory, 7th Edition Unix fails this way:
>
>mv: e/dir exists
>
> Solaris 10 mv fails this way:
>
>mv: cannot rename dir to e/dir: File exists
>
> Coreutils 5.93 fails this way:
>
>mv: cannot overwrite directory `e/dir'
>
> Current coreutils fails this way:
>
>mv: cannot move 'dir' to 'e/dir': Directory not empty
>
> macOS fails this way:
>
>mv: rename dir to e/dir: Directory not empty
>
> If you ask me, none of these are all that good. Current coreutils and
> macOS give a confusing "Directory not empty" message (which directory?).
> Solaris 10 has a similar confusion. 7th Edition and coreutils 5.93 don't
> print strerror (errno).
>
> 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.
>

bug#64785: Package: coreutils Version: 8.32-4+b1 program=mv

[Paul Eggert]

Thanks to both of you. I installed the attached into Savannah master 
coreutils. It implements the suggestion, except that later I noticed 
that EMLINK and ETXTBSY can join the throng.


At some point it might make sense to scan for other direct or indirect 
calls to renameat, renameat2, and linkat, where the diagnostic could be 
improved if it's known to refer to the destination.From 3cb862ce5f10db392cc8e6907dd9d888acfa2a30 Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Sat, 22 Jul 2023 13:39:17 -0700
Subject: [PATCH] mv: better diagnostic for 'mv dir x' failure

Problem reported by Nir Oren .
* src/copy.c (copy_internal): Use a more-specific diagnostic when
a rename fails due to a problem that must be due to the
destination, avoiding user confusion in cases like 'mv dir x'
where x is a nonempty directory.
* tests/mv/dir2dir.sh: Adjust to match.
---
 NEWS|  7 +++
 src/copy.c  | 20 +---
 tests/mv/dir2dir.sh |  2 +-
 3 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/NEWS b/NEWS
index 7d2ca7f6b..bd0c59eb5 100644
--- a/NEWS
+++ b/NEWS
@@ -56,6 +56,13 @@ GNU coreutils NEWS-*- outline -*-
   short option is reserved to better support emulation of the standalone
   checksum utilities with cksum.
 
+  'mv dir x' now complains differently if x/dir is a nonempty directory.
+  Previously it said "mv: cannot move 'dir' to 'x/dir': Directory not empty",
+  where it was unclear whether 'dir' or 'x/dir' was the problem.
+  Now it says "mv: cannot overwrite 'x/dir': Directory not empty".
+  Similarly for other renames where the destination must be the problem.
+  [problem introduced in coreutils-6.0]
+
 ** Improvements
 
   cp, mv, and install now avoid copy_file_range on linux kernels before 5.3
diff --git a/src/copy.c b/src/copy.c
index ddc9eab30..90eebf6bc 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2840,9 +2840,23 @@ skip:
  If the permissions on the directory containing the source or
  destination file are made too restrictive, the rename will
  fail.  Etc.  */
-  error (0, rename_errno,
- _("cannot move %s to %s"),
- quoteaf_n (0, src_name), quoteaf_n (1, dst_name));
+  char const *quoted_dst_name = quoteaf_n (1, dst_name);
+  switch (rename_errno)
+{
+case EDQUOT: case EEXIST: case EISDIR: case EMLINK:
+case ENOSPC: case ENOTEMPTY: case ETXTBSY:
+  /* The destination must be the problem.  Don't mention
+ the source as that is more likely to confuse the user
+ than be helpful.  */
+  error (0, rename_errno, _("cannot overwrite %s"),
+ quoted_dst_name);
+  break;
+
+default:
+  error (0, rename_errno, _("cannot move %s to %s"),
+ quoteaf_n (0, src_name), quoted_dst_name);
+  break;
+}
   forget_created (src_sb.st_ino, src_sb.st_dev);
   return false;
 }
diff --git a/tests/mv/dir2dir.sh b/tests/mv/dir2dir.sh
index ea99eca56..da6a518e5 100755
--- a/tests/mv/dir2dir.sh
+++ b/tests/mv/dir2dir.sh
@@ -34,7 +34,7 @@ sed 's/: File exists/: Directory not empty/'o1;mv o1 out
 sed 's/: Device or resource busy/: Directory not empty/'o1;mv o1 out
 
 cat <<\EOF > exp || framework_failure_
-mv: cannot move 'b/t' to 'a/t': Directory not empty
+mv: cannot overwrite 'a/t': Directory not empty
 EOF
 
 compare exp out || fail=1
-- 
2.39.2

bug#64785: Package: coreutils Version: 8.32-4+b1 program=mv

[Pádraig Brady]

On 22/07/2023 18:37, Paul Eggert wrote:

On 2023-07-22 03:19, Pádraig Brady wrote:

Given the subtleties in this area,
I'd be reluctant to adjust diagnostics here.


I looked into this a bit more. Given "mv dir e" where e/dir is an
existing nonempty directory, 7th Edition Unix fails this way:

mv: e/dir exists

Solaris 10 mv fails this way:

mv: cannot rename dir to e/dir: File exists

Coreutils 5.93 fails this way:

mv: cannot overwrite directory `e/dir'

Current coreutils fails this way:

mv: cannot move 'dir' to 'e/dir': Directory not empty

macOS fails this way:

mv: rename dir to e/dir: Directory not empty

If you ask me, none of these are all that good. Current coreutils and
macOS give a confusing "Directory not empty" message (which directory?).
Solaris 10 has a similar confusion. 7th Edition and coreutils 5.93 don't
print strerror (errno).

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.


Sounds good.

Thanks for looking into all the details.

Padraig

bug#64785: Package: coreutils Version: 8.32-4+b1 program=mv

[Paul Eggert]

On 2023-07-22 03:19, Pádraig Brady wrote:

Given the subtleties in this area,
I'd be reluctant to adjust diagnostics here.


I looked into this a bit more. Given "mv dir e" where e/dir is an 
existing nonempty directory, 7th Edition Unix fails this way:


  mv: e/dir exists

Solaris 10 mv fails this way:

  mv: cannot rename dir to e/dir: File exists

Coreutils 5.93 fails this way:

  mv: cannot overwrite directory `e/dir'

Current coreutils fails this way:

  mv: cannot move 'dir' to 'e/dir': Directory not empty

macOS fails this way:

  mv: rename dir to e/dir: Directory not empty

If you ask me, none of these are all that good. Current coreutils and 
macOS give a confusing "Directory not empty" message (which directory?). 
Solaris 10 has a similar confusion. 7th Edition and coreutils 5.93 don't 
print strerror (errno).


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.

bug#64785: Package: coreutils Version: 8.32-4+b1 program=mv

[Pádraig Brady]

On 21/07/2023 18:17, Nir Oren wrote:

*mv: error message "Directory not empty" is confusing *

description: when you try to move a directory to a location already
containing a directory with the same name it would just write "mv: cannot
move 'A' to 'B': Directory not empty"

first, this is technically a wrong error message because there is no
requirement that the destination would be empty.
the destination might as well be populated with some content, we just
require that it would not contain a directory with the same name.


Well that's not exactly the case.
mv will replace empty dirs in the destination,
so it is significant if the dir in the destination is empty or not.


second, the error message is confusing because it doesn't state that the
problem is with the destination. One can think that the problem is actually
with the source, and that the source directory has some kind of attribute
that would require it to be empty prior to moving.


I think it's unlikely that users would be confused as
to whether the source needs to be empty.


I would suggest to change the error message to: "*a directory with the same
name already exists at destination*"


More accurately the error would be:
"a populated directory with the same name already exists"

Now would could argue that the above info is implicit in:
"directory not empty"
(which BTW is just the standard error from ENOTEMPTY).

Given the subtleties in this area,
I'd be reluctant to adjust diagnostics here.

thanks,
Pádraig

bug#64785: Package: coreutils Version: 8.32-4+b1 program=mv

[Nir Oren]

*mv: error message "Directory not empty" is confusing *

description: when you try to move a directory to a location already
containing a directory with the same name it would just write "mv: cannot
move 'A' to 'B': Directory not empty"

first, this is technically a wrong error message because there is no
requirement that the destination would be empty.
the destination might as well be populated with some content, we just
require that it would not contain a directory with the same name.

second, the error message is confusing because it doesn't state that the
problem is with the destination. One can think that the problem is actually
with the source, and that the source directory has some kind of attribute
that would require it to be empty prior to moving.

I would suggest to change the error message to: "*a directory with the same
name already exists at destination*"

reproduction:
$:rm -rf /tmp/node_modules
$:mkdir -p proj1/node_modules
$:touch proj1/node_modules/foo
$:mkdir -p proj2/node_modules
$:touch proj2/node_modules/bar
$:mv proj1/node_modules /tmp
$:mv proj2/node_modules /tmp
mv: cannot move 'proj2/node_modules' to '/tmp/node_modules': Directory not
empty