bug#36831: Enhance directory move. (was Re: bug#36831: enhance 'directory not empty' message)

2019-08-02 Thread Assaf Gordon

Hello,

On 2019-08-02 9:56 p.m., L A Walsh wrote:

On 2019/08/02 19:47, Assaf Gordon wrote:

Can new merging features be added to 'mv'? yes.
But it seems to me these would be better suited for 'higher level'
programs (e.g. a GUI file manager).

---
But neither the person who posted the original bug on this
nor I are using a GUI, we are running 'mv' GUI, we use the cmd line on linux, 
so that wouldn't
be of any use.


The original post was about the error *message*, asking to make it 
clearer. That is the topic of this thread (and the previous patch) -

so let's leave them at that.


I see you started a new thread ( https://bugs.gnu.org/36901 ),
so I'll reply there.






bug#36831: Enhance directory move. (was Re: bug#36831: enhance 'directory not empty' message)

2019-08-02 Thread L A Walsh



On 2019/08/02 19:47, Assaf Gordon wrote:
> Can new merging features be added to 'mv'? yes.
> But it seems to me these would be better suited for 'higher level'
> programs (e.g. a GUI file manager).
---
But neither the person who posted the original bug on this
nor I are using a GUI, we are running 'mv' GUI, we use the cmd line on linux, 
so that wouldn't
be of any use.

If the command was named 'ren', then I'd expect it to be dummer,
but 'mv'/move seem like it should be able to move files from
one dir into another.

But you say posix wants it to perform as a rename?
I know, create a 're' command (or 'rn') for rename, and have
it do what 'mv' would do.  Maybe posix would realize it would
be better to have re/rn behave like rename, and 'mv' to
behave it was moving something.

So if I have:
mkdir A B
touch A/foo B/fee

So when I look at the system call on linux for rename:
   oldpath can specify a directory.  In this case, newpath must either not
   exist, or it must specify an empty directory.
(complying with POSIX_C_SOURCE >= 200809L)

So move should give an error: Nope:

mv A B
> tree B
B
├── A
│   └── foo
└── fee

1 directory, 2 files

So mv is violating POSIX - it didn't do the rename, but moved
A under B and neither dir had to be empty.

Saying it has to follow POSIX when it doesn't appear to, seems
a bit contradictory?














bug#36831: Enhance directory move. (was Re: bug#36831: enhance 'directory not empty' message)

2019-08-02 Thread Assaf Gordon
Hello,

On Fri, Aug 02, 2019 at 02:41:31AM -0700, L A Walsh wrote:
> On 2019/07/28 23:28, Assaf Gordon wrote:
> >
> >
> > $ mkdir A B B/A
> > $ touch A/bar B/A/foo
> > $ mv A B
> > mv: cannot move 'A' to 'B/A': Directory not empty
> >
> > And the reason (as you've found out) is that the target directory 'B/A'
> > is not empty (has the 'foo' file in it).
> > Had this been allowed, moving 'A' to 'B/A' would result in the 'foo'
> > file disappearing.
> >   
> ---
> Why must foo disappear?
> 
> Microsoft Windows handles this situation by telling the user that
> the target directory already exists and giving the option to *MERGE*
> the directories.
> 
> If you attempt to move a file into a directory that already contains
> a file by the same name, it pops up another notice asking [...]

Certainly, GUI programs (and more 'feature-rich' programs than 'mv')
offer many "merging" options.

I'm sure Midnight-Commander, KDE/Doplhine, XFCE/Thunar, Gnome/Nautilus
and many other free software GUI file managers have some "merging"
capabilities.

But 'mv' is more basic and does not have this capability.
Partly that is because it adheres to the POSIX standards, which
mandates:
"3. The mv utility shall perform actions equivalent to the
rename() function [...]"
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/mv.html

Some rsync options (--remove-source-files) can mimick 'mv' with merging,
but then they are more like "copy+delete" than actual "rename/move".


Can new merging features be added to 'mv'? yes.
But it seems to me these would be better suited for 'higher level'
programs (e.g. a GUI file manager).

regards,
 - assaf





bug#36831: Enhance directory move. (was Re: bug#36831: enhance 'directory not empty' message)

2019-08-02 Thread L A Walsh
On 2019/07/28 23:28, Assaf Gordon wrote:
>
>
> $ mkdir A B B/A
> $ touch A/bar B/A/foo
> $ mv A B
> mv: cannot move 'A' to 'B/A': Directory not empty
>
> And the reason (as you've found out) is that the target directory 'B/A'
> is not empty (has the 'foo' file in it).
> Had this been allowed, moving 'A' to 'B/A' would result in the 'foo'
> file disappearing.
>   
---
Why must foo disappear?

Microsoft Windows handles this situation by telling the user that
the target directory already exists and giving the option to *MERGE*
the directories.

If you attempt to move a file into a directory that already contains
a file by the same name, it pops up another notice asking if you want
to replace the old with the new, keep it as it is (file won't be moved and
in this case, old directory would still exist with the unmoved files left
in it), or keep both with the new file getting a name variant (like
foo.exe -> foo_1.exe, keeping the extension the same so as to not mess
up the identity of the contents.

But in all the cases, a file with a non-conflicting name wouldn't
disappear from the target.










bug#36831: enhance 'directory not empty' message

2019-08-01 Thread Assaf Gordon
On Thu, Aug 01, 2019 at 03:58:51PM -0700, Paul Eggert wrote:
> Thanks, that's better, but we're still missing some opportunities for 
> improvement.
> 
> > mv: cannot move 'A' to 'B/A': Target directory not empty
> 
> This should be "Destination" not "Target". 
[...] 
> You meant "mv" not "rm".
[...]
> > +static char*
> Space before "*".
[...]
> > +strerror_target (int e)
> Change name to "strerror_dest"
[...] 
> This function should return NULL instead of aborting when the errno value is
> inapplicable. That way, its callers need not hardcode which errno values it
> handles.

Thanks for the review and suggestions - attached an updated patch.

> Come to think of it, the same improvement should be made to ln, cp, install
> and shred. Basically, to any program that uses 'rename' or 'link' or similar
> syscalls, and which reports an error if the syscall fails.

OK, I will work on that next.

-assaf
>From 8dc6158a6fde668e55312b5fb69384f438b7e55a Mon Sep 17 00:00:00 2001
From: Assaf Gordon 
Date: Mon, 29 Jul 2019 00:23:20 -0600
Subject: [PATCH] mv: improve error messages when destination directory is at
 fault

Suggested by Alex Mantel  in
https://bugs.gnu.org/36831 .

$ mkdir A B B/A
$ touch A/bar B/A/foo

Before:

$ mv A B
mv: cannot move 'A' to 'B/A': Directory not empty

After:

$ mv A B
mv: cannot move 'A' to 'B/A': Destination directory not empty

The following errors are handled:
EDQUOT, EEXIST, ENOTEMPTY, EISDIR, ENOSPC, ETXTBSY.

* src/copy.c (copy_internal): Print custom messages for errors
that explicitly fault the destination directory.
(strerror_dest): New function, return custom, translatable error
messages for errors relating to 'destination' component.
* tests/mv/dir2dir.sh: Adjust expected error message.
* NEWS: Mention change.
---
 NEWS|  6 +
 src/copy.c  | 53 ++---
 tests/mv/dir2dir.sh |  8 ---
 3 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/NEWS b/NEWS
index fd0543351..3d80665ae 100644
--- a/NEWS
+++ b/NEWS
@@ -44,6 +44,12 @@ GNU coreutils NEWS-*- 
outline -*-
   stat(1) also supports a new --cached= option to control cache
   coherency of file system attributes, useful on network file systems.
 
+** Improvements
+
+  mv now prints clearer error messages when a failure relates to the
+  destination directory (e.g., "Destination directory is not empty" instead
+  of "Directory not empty").
+
 
 * Noteworthy changes in release 8.31 (2019-03-10) [stable]
 
diff --git a/src/copy.c b/src/copy.c
index 65cf65895..602c8307b 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1867,6 +1867,44 @@ source_is_dst_backup (char const *srcbase, struct stat 
const *src_st,
   return dst_back_status == 0 && SAME_INODE (*src_st, dst_back_sb);
 }
 
+/* Return custom error messages replacing the default libc's
+   messages. These messages explicity fault the destination component
+   in the error.
+
+   Return NULL if E (errno value) is not handled (and by implication
+   should use the system's default text for the error message).  */
+static char *
+strerror_dest (int e)
+{
+  /* TRANSLATORS: These strings should mimick libc's standard
+ error messages (from strerror(3)), but explicitly mention
+ the fault is with the destination directory. */
+  switch (errno)
+{
+case EDQUOT:
+  return _("Disk quota exceeded on destination device");
+case EEXIST:
+case ENOTEMPTY:
+  return _("Destination directory not empty");
+case EISDIR:
+  return _("Tried to overwrite a directory with a file");
+case ENOSPC:
+  return _("No space left on destination device");
+case ETXTBSY:
+  /* NOTE: The error is "Text file busy" - but "text" in that context
+ refers to "text segment" of an executable file (as opposed to
+ "data segment" and "BSS segment").
+
+ This error message is meant for users, and 'text file' can be easily
+ confused with an actual text file (i.e., one containing only ASCII
+ characters. Thus, say 'executable' instead of 'text'.*/
+  return _("Destination executable file is busy");
+default:
+  return NULL;
+}
+}
+
+
 /* Copy the file SRC_NAME to the file DST_NAME.  The files may be of
any type.  NEW_DST should be true if the file DST_NAME cannot
exist because its parent directory was just created; NEW_DST should
@@ -2477,9 +2515,18 @@ copy_internal (char const *src_name, char const 
*dst_name,
  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));
+
+  const char *custom_err_msg = strerror_dest (rename_errno);
+  if (custom_err_msg)
+error (0, 0,
+  

bug#36831: enhance 'directory not empty' message

2019-08-01 Thread Assaf Gordon
Hello,

On Wed, Jul 31, 2019 at 08:03:45PM -0700, Paul Eggert wrote:
> Assaf Gordon wrote:
> > An explicit error explicitly saying "cannot move", and mention the source 
> > and
> > destination, and also "blames" the target directory seems the most
> > user-friendly and least ambiguous.
> 
> Sure, but that handles only the ENOTEMPTY/EEXIST case. How would you handle
> the EDQUOT, EISDIR, and ENOSPC cases? Will you invent a separate diagnostic
> for each case, or just treat them as in my proposed patch? I assume the
> latter, but either way I'd like to see a patch that handles these properly
> too. Also, please handle ETXTBUSY while you're at it (sorry, I missed that
> one).
> 
> > For the second and third cases,
> > "No space" and "Quota exceeded" seem to me to always relate to the
> > destination, and I don't think users get confused about those
> > (other opinions of course welcomed).
> 
> What's obvious to experts like us is not always obvious to users. If users
> get confused by the current diagnostic for ENOTEMPTY/EEXIST, I don't see why
> they wouldn't also get confused for ETXTBUSY etc.
> 
> > Your patch also added "EISDIR", for which rename(2) says:
> >  "newpath is an existing directory, but oldpath is not a directory."
> > 
> > But I don't think this error can happen with gnu mv.
> 
> It can, as a result of a race condition if some other process is mutating
> the file system while 'mv' is running. Admittedly unlikely, but we might as
> well improve this errno value while we're improving the others.

All good points.

Please see attached updated version.

It does add explicit error string for each error code, but I hope the
implementation is reasonable and easy to maintain and translate.

-assaf
>From 8ee71b24d74d7cfe81f151de430d38935cf04675 Mon Sep 17 00:00:00 2001
From: Assaf Gordon 
Date: Mon, 29 Jul 2019 00:23:20 -0600
Subject: [PATCH] mv: improve error messages when target directory is at fault

Suggested by Alex Mantel  in
https://bugs.gnu.org/36831 .

$ mkdir A B B/A
$ touch A/bar B/A/foo

Before:

$ mv A B
mv: cannot move 'A' to 'B/A': Directory not empty

After:

$ mv A B
mv: cannot move 'A' to 'B/A': Target directory not empty

The following errors are handled:
EDQUOT, EEXIST, ENOTEMPTY, EISDIR, ENOSPC, ETXTBSY.

* src/copy.c (copy_internal): Print custom messages for errors
that explicitly fault the target directory.
(strerror_target): New function, return custom and translatable error
messages.
* tests/mv/dir2dir.sh: Adjust expected error message.
* NEWS: Mention change.
---
 NEWS|  6 +
 src/copy.c  | 56 ++---
 tests/mv/dir2dir.sh |  6 ++---
 3 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/NEWS b/NEWS
index fd0543351..4ec4d0df0 100644
--- a/NEWS
+++ b/NEWS
@@ -44,6 +44,12 @@ GNU coreutils NEWS-*- 
outline -*-
   stat(1) also supports a new --cached= option to control cache
   coherency of file system attributes, useful on network file systems.
 
+** Improvements
+
+  rm now prints clearer error messages when a failure relates to the
+  target directory (e.g., "Target directory is not empty" instead of
+  "Directory not empty").
+
 
 * Noteworthy changes in release 8.31 (2019-03-10) [stable]
 
diff --git a/src/copy.c b/src/copy.c
index 65cf65895..9cf02ad9c 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1867,6 +1867,38 @@ source_is_dst_backup (char const *srcbase, struct stat 
const *src_st,
   return dst_back_status == 0 && SAME_INODE (*src_st, dst_back_sb);
 }
 
+static char*
+strerror_target (int e)
+{
+  /* TRANSLATORS: These strings should mimick libc's standard
+ error messages (from strerror(3)), but explicitly mention
+ the fault is with the target directory. */
+  switch (errno)
+{
+case EDQUOT:
+  return _("Disk quota exceeded on target device");
+case EEXIST:
+case ENOTEMPTY:
+  return _("Target directory not empty");
+case EISDIR:
+  return _("Tried to overwrite a directory with a file");
+case ENOSPC:
+  return _("No space left on target device");
+case ETXTBSY:
+  /* NOTE: The error is "Text file busy" - but "text" in that context
+ refers to "text segment" of an executable file (as opposed to
+ "data segment" and "BSS segment").
+
+ This error message is meant for users, and 'text file' can be easily
+ confused with an actual text file (i.e., one containing only ASCII
+ characters. Thus, say 'executable' instead of 'text'.*/
+  return _("Target executable file is busy");
+default:
+  assert (0);
+}
+}
+
+
 /* Copy the file SRC_NAME to the file DST_NAME.  The files may be of
any type.  NEW_DST should be true if the file DST_NAME cannot
exist because its parent directory was just created; NEW_DST should
@@ -2477,9 +2509,27 @@ copy_internal (char const *src_name, char const 
*dst_name,
  If the permissions 

bug#36831: enhance 'directory not empty' message

2019-08-01 Thread Erik Auerswald
Hi,

On Wed, Jul 31, 2019 at 04:05:05PM -0600, Assaf Gordon wrote:
> On Mon, Jul 29, 2019 at 06:50:46PM -0500, Paul Eggert wrote:
> > On 7/29/19 1:28 AM, Assaf Gordon wrote:
> > > +  if (rename_errno == ENOTEMPTY || rename_errno == EEXIST)
> > > +{
> > > +  error (0, 0, _("cannot move %s to %s: Target directory not 
> > > empty"),
> > > + quoteaf_n (0, src_name), quoteaf_n (1, dst_name));
> > 
> > Although this is an improvement, it is not general enough, as other errno
> > values are relevant only for the destination. Better would be to have a
> > special case for errno values that matter only for the destination, and use
> > the existing code for errno values where we don't know whether the problem
> > is the source or the destination. Something like the attached, say.
> 
> > +case EDQUOT: case EEXIST: case EISDIR: case ENOSPC: case 
> > ENOTEMPTY:
> > +  error (0, rename_errno, "%s", quotearg_colon (dst_name));
> > +  break;
> > +
> 
> [...]
> An explicit error explicitly saying "cannot move", and mention the source and
> destination, and also "blames" the target directory seems the most
> user-friendly and least ambiguous.

I agree with this reasoning and prefer Assaf's error message improvement.

Thanks,
Erik
-- 
If you're willing to restrict the flexibility of your approach,
you can almost always do something better.
-- John Carmack





bug#36831: enhance 'directory not empty' message

2019-07-31 Thread Paul Eggert

Assaf Gordon wrote:

An explicit error explicitly saying "cannot move", and mention the source and
destination, and also "blames" the target directory seems the most
user-friendly and least ambiguous.


Sure, but that handles only the ENOTEMPTY/EEXIST case. How would you handle the 
EDQUOT, EISDIR, and ENOSPC cases? Will you invent a separate diagnostic for each 
case, or just treat them as in my proposed patch? I assume the latter, but 
either way I'd like to see a patch that handles these properly too. Also, please 
handle ETXTBUSY while you're at it (sorry, I missed that one).



For the second and third cases,
"No space" and "Quota exceeded" seem to me to always relate to the
destination, and I don't think users get confused about those
(other opinions of course welcomed).


What's obvious to experts like us is not always obvious to users. If users get 
confused by the current diagnostic for ENOTEMPTY/EEXIST, I don't see why they 
wouldn't also get confused for ETXTBUSY etc.



Your patch also added "EISDIR", for which rename(2) says:
 "newpath is an existing directory, but oldpath is not a directory."

But I don't think this error can happen with gnu mv.


It can, as a result of a race condition if some other process is mutating the 
file system while 'mv' is running. Admittedly unlikely, but we might as well 
improve this errno value while we're improving the others.






bug#36831: enhance 'directory not empty' message

2019-07-31 Thread Bernhard Voelker
On 8/1/19 12:05 AM, Assaf Gordon wrote:
> Happy to hear your opinion,

+1
Including the full context about an error is essential:
- what did the tool try to do (at that point)?
- what was the problem? (e.g. ENOTEMPTY)
- where was the problem: src or dst?

Thanks & have a nice day,
Berny





bug#36831: enhance 'directory not empty' message

2019-07-31 Thread Assaf Gordon
Hello Paul,

On Mon, Jul 29, 2019 at 06:50:46PM -0500, Paul Eggert wrote:
> On 7/29/19 1:28 AM, Assaf Gordon wrote:
> > +  if (rename_errno == ENOTEMPTY || rename_errno == EEXIST)
> > +{
> > +  error (0, 0, _("cannot move %s to %s: Target directory not 
> > empty"),
> > + quoteaf_n (0, src_name), quoteaf_n (1, dst_name));
> 
> Although this is an improvement, it is not general enough, as other errno
> values are relevant only for the destination. Better would be to have a
> special case for errno values that matter only for the destination, and use
> the existing code for errno values where we don't know whether the problem
> is the source or the destination. Something like the attached, say.

> +case EDQUOT: case EEXIST: case EISDIR: case ENOSPC: case 
> ENOTEMPTY:
> +  error (0, rename_errno, "%s", quotearg_colon (dst_name));
> +  break;
> +

Thanks for the review.

At the risk of bikeshedding, I'd like to argue for the prior method.
While it is not general enough, I think it provides a clearer error message.

For example, with the more general implementation the errors would be:

  $ mv A B
  mv: B/A: Directory not empty

  $ mv A B
  mv: B/A: No space left on device

  $ mv A B
  mv: B/A: Quota exceeded

In the first case,
I think this error is potentially more confusing than
before: while it doesn't mention the source directory, it also doesn't
say "cannot move" - so it is only implied it is an error (an
inexperienced user might dismiss this as a warning).

Also, it could be that there will be a source directory named very similarly
to the destination directory, and from a quick glace it would not be easy to
understand what happened.

An explicit error explicitly saying "cannot move", and mention the source and
destination, and also "blames" the target directory seems the most
user-friendly and least ambiguous.

---

For the second and third cases,
"No space" and "Quota exceeded" seem to me to always relate to the
destination, and I don't think users get confused about those
(other opinions of course welcomed).

---

Your patch also added "EISDIR", for which rename(2) says:
 "newpath is an existing directory, but oldpath is not a directory."

But I don't think this error can happen with gnu mv.
If we try to move a file onto a directory, we get:

  $ mkdir C C/D ; touch D
  $ mv D C
  mv: cannot overwrite directory 'C/D' with non-directory

And this case is specifically handled in copy.c line 2131, before
calling rename(2)  (and also this is an example of a custom error
message instead of using stock libc messages).

---

Happy to hear your opinion,
 - assaf







bug#36831: enhance 'directory not empty' message

2019-07-29 Thread Paul Eggert

On 7/29/19 1:28 AM, Assaf Gordon wrote:

+  if (rename_errno == ENOTEMPTY || rename_errno == EEXIST)
+{
+  error (0, 0, _("cannot move %s to %s: Target directory not empty"),
+ quoteaf_n (0, src_name), quoteaf_n (1, dst_name));


Although this is an improvement, it is not general enough, as other 
errno values are relevant only for the destination. Better would be to 
have a special case for errno values that matter only for the 
destination, and use the existing code for errno values where we don't 
know whether the problem is the source or the destination. Something 
like the attached, say.



diff --git a/src/copy.c b/src/copy.c
index 65cf65895..b1e4557e4 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2477,9 +2477,18 @@ copy_internal (char const *src_name, char const *dst_name,
  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));
+  switch (errno)
+{
+case EDQUOT: case EEXIST: case EISDIR: case ENOSPC: case ENOTEMPTY:
+  error (0, rename_errno, "%s", quotearg_colon (dst_name));
+  break;
+
+default:
+  error (0, rename_errno,
+ _("cannot move %s to %s"),
+ quoteaf_n (0, src_name), quoteaf_n (1, dst_name));
+  break;
+}
   forget_created (src_sb.st_ino, src_sb.st_dev);
   return false;
 }


bug#36831: enhance 'directory not empty' message

2019-07-29 Thread Jim Meyering
On Sun, Jul 28, 2019 at 11:29 PM Assaf Gordon  wrote:
...
> What do others think? If this is a desired improvement, I'll finish the
> patch with news/tests/etc.
...
> [PATCH] mv: improve ENOTEMPTY/EEXIST error message
>
> Suggested by Alex Mantel  in
> https://bugs.gnu.org/36831 .
>
> $ mkdir A B B/A
> $ touch A/bar B/A/foo
>
> Before:
>
> $ mv A B
> mv: cannot move 'A' to 'B/A': Directory not empty
>
> After:
>
> $ mv A B
> mv: cannot move 'A' to 'B/A': Target directory not empty
>
> * src/copy.c (copy_internal): Add special handling for ENOTEMPTY/EEXIST.
> TODO: NEWS, tests.

I like it. Thank you.





bug#36831: enhance 'directory not empty' message

2019-07-29 Thread Assaf Gordon
Hello,

On Sun, Jul 28, 2019 at 08:58:59PM +0200, Alex Mantel wrote:
[...] 
> Ah, the target directory does exist! Hmm... But i'd like the message to be
> like:
> 
>    $ mv thing/ ../things
>    mv: cannot move 'thing' to '../things/things': Targetdirectory not empty
> 
>   ^ this little thing here,
>     it explains everyting.
> 
> Change text from 'Directory not empty' to 'Targetdirectory not empty'.

Thanks for the report.

To clarify, the scenario is:

$ mkdir A B B/A
$ touch A/bar B/A/foo
$ mv A B
mv: cannot move 'A' to 'B/A': Directory not empty

And the reason (as you've found out) is that the target directory 'B/A'
is not empty (has the 'foo' file in it).
Had this been allowed, moving 'A' to 'B/A' would result in the 'foo'
file disappearing.

---

How is a user expecting to know this error is about that target
directory?

There is a bit of a trade-off here between user-friendliness (especially
for non-technical user) and more technical knowledge.
If we go one step 'lower' to the programming interface, almost all
sources mention this is about the 'target' directory not being empty:

POSIX's says:
https://pubs.opengroup.org/onlinepubs/009695399/functions/rename.html
[EEXIST] or [ENOTEMPTY]
The link named by new is a directory that is not an empty directory.

Linux's rename(2) manual page says:
ENOTEMPTY or EEXIST
newpath is a nonempty directory, that is, contains entries
other than "." and "..".

FreeBSD's rename(2) manual page says:
[ENOTEMPTY]The to argument is a directory and is not empty.

AIX rename(2) manual page says:
 ENOTEMPTY
   The ToPath parameter specifies an existing directory that is
   not empty.


So there is some merit in claiming this helpful piece of information is
lost when the error message is reported to the user.

---

In GNU coreutils this error message originates from 'copy.c' line 2480:
https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/copy.c#n2480

error (0, rename_errno,
  _("cannot move %s to %s"),
  quoteaf_n (0, src_name), quoteaf_n (1, dst_name));

And herein lies the (technical) problem: The actual message "Directory
not empty" is not in the source code - it is a system error message
that corresponds to the value of 'rename_errno' variable
(ENOTEMPTY/EEXIST). It originates from GLibc (or another libc).

So there is no trivial way to change the error message in coreutils.

Attached a patch to add special handling for this error.

---

What do others think? If this is a desired improvement, I'll finish the
patch with news/tests/etc.


regards,
 - assaf
>From 430b30104234db719bf15e6fc681a62312c7124f Mon Sep 17 00:00:00 2001
From: Assaf Gordon 
Date: Mon, 29 Jul 2019 00:23:20 -0600
Subject: [PATCH] mv: improve ENOTEMPTY/EEXIST error message

Suggested by Alex Mantel  in
https://bugs.gnu.org/36831 .

$ mkdir A B B/A
$ touch A/bar B/A/foo

Before:

$ mv A B
mv: cannot move 'A' to 'B/A': Directory not empty

After:

$ mv A B
mv: cannot move 'A' to 'B/A': Target directory not empty

* src/copy.c (copy_internal): Add special handling for ENOTEMPTY/EEXIST.
TODO: NEWS, tests.
---
 src/copy.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/copy.c b/src/copy.c
index 65cf65895..a5af570bf 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2450,6 +2450,14 @@ copy_internal (char const *src_name, char const 
*dst_name,
   return true;
 }
 
+  if (rename_errno == ENOTEMPTY || rename_errno == EEXIST)
+{
+  error (0, 0, _("cannot move %s to %s: Target directory not empty"),
+ quoteaf_n (0, src_name), quoteaf_n (1, dst_name));
+  forget_created (src_sb.st_ino, src_sb.st_dev);
+  return false;
+}
+
   /* WARNING: there probably exist systems for which an inter-device
  rename fails with a value of errno not handled here.
  If/as those are reported, add them to the condition below.
-- 
2.11.0



bug#36831: enhance 'directory not empty' message

2019-07-28 Thread Alex Mantel

i couldn't use move:

    $ mv thing/ ../things
    mv: cannot move 'thing' to '../things/things': Directory not empty

An i can not move it. i do not understand why. I have to google and find
at stackoverflow:

    Though its man page doesn't document it, mv will refuse to rename a
    directory to another directory if the target directory contains files.
    This is a good thing in your case because you turn out to want to
    merge the content of the source into the target, which mv will not do.

https://askubuntu.com/questions/269775/mv-directory-not-empty

Ah, the target directory does exist! Hmm... But i'd like the message to 
be like:


   $ mv thing/ ../things
   mv: cannot move 'thing' to '../things/things': Targetdirectory not empty

  ^ this little thing here,
    it explains everyting.

Change text from 'Directory not empty' to 'Targetdirectory not empty'.