Re: Bug#376749: coreutils: Incorrect error message in mv (due to incorrect behavior)

[Jim Meyering]

Florent Bayle <address@hidden> wrote:
> Package: coreutils
> Version: 5.96-5
> Severity: normal
>
> To be brief :
> address@hidden:/tmp# mkdir -p test1/test test2/test; touch test1/test/file; 
> mv test2/* test1/
> mv: cannot move `test2/test' to a subdirectory of itself, `test1/test'
> address@hidden:/tmp#
>
> It seems to be an incorrect error message, but I think that this bug is
> related to #376743 because with version 5.94-1 of coreutils, I have this
> output :
> address@hidden:/tmp# mkdir -p test1/test test2/test; touch test1/test/file; 
> mv test2/* test1/
> mv: cannot overwrite directory `test1/test'
> address@hidden:/tmp#

Thank you for reporting that.
Note that this is file-system dependent for me.
On tmpfs, reiserfs, and ext3, my results match yours:

    $ rm -rf a b; mkdir -p a/t b/t; touch a/t/f; mv b/t a
    /cu/src/mv: cannot move `b/t' to a subdirectory of itself, `a/t'

But on xfs, I get this:

    $ rm -rf a b; mkdir -p a/t b/t; touch a/t/f; mv b/t a
    ./mv: cannot move `b/t' to `a/t': File exists

Here's where the strace output diverged:

    $ diff -u /t/strace-xfs /t/strace-tmpfs|grep rename
    -rename("b/t", "a/t")                    = -1 EEXIST (File exists)
    +rename("b/t", "a/t")                    = -1 ENOTEMPTY (Directory not 
empty)

This is due in part to the following change, since before then, the code
in question never had to deal with an existing destination directory.

    2006-05-11  Jim Meyering  <address@hidden>

            mv -T DIR EMPTY_DIR no longer fails unconditionally
            * src/copy.c (copy_internal): Don't manually prohibit a move where
            the destination is an existing directory.  Sometimes doing that is
            valid.  Let the rename system call enforce the rules.  That is
            allowed only when the source is a directory and the destination
            directory (to be replaced) is empty.  Reported by Eric Blake.
            * tests/mv/no-target-dir: New file/test for this.
            * tests/mv/Makefile.am (TESTS): Add no-target-dir.
            * NEWS: Mention this.

Since today's problem is due to work-around code whose sole
purpose is to accommodate buggy rename support in old systems,
I'm fixing it by removing the questionable work-around code.
I've included the patch for the trunk below.
Will probably do the same on the branch.

Note that I'm also removing the EIO-handling code.
If anyone cares about SunOS-4.1.4 or Irix 5.3 and still has access
to such a system, please try the following commands in an NFS-mounted
directory, and let me know if you get different results:

    $ rm -rf a; mkdir a; perl -e 'rename "a","a/x" or die "$!\n"'
    Invalid argument

2006-07-05  Jim Meyering  <address@hidden>

        * src/copy.c (copy_internal): Don't work around old NFS clients like
        SunOS-4.1.4 and Irix 5.3 that set errno to values like EIO and
        ENOTEMPTY upon failed rename.  Otherwise, we risk misinterpreting
        a banal failure as a recursive move-into-self failure.
        Reported by Florent Bayle in <http://bugs.debian.org/376749>.

Index: src/copy.c
===================================================================
RCS file: /fetish/cu/src/copy.c,v
retrieving revision 1.200
diff -u -p -r1.200 copy.c
--- src/copy.c  3 Jun 2006 09:04:22 -0000       1.200
+++ src/copy.c  5 Jul 2006 08:38:52 -0000
@@ -1385,18 +1385,7 @@ copy_internal (char const *src_name, cha
 
       /* This happens when attempting to rename a directory to a
         subdirectory of itself.  */
-      if (errno == EINVAL
-
-         /* When src_name is on an NFS file system, some types of
-            clients, e.g., SunOS4.1.4 and IRIX-5.3, set errno to EIO
-            instead.  Testing for this here risks misinterpreting a real
-            I/O error as an attempt to move a directory into itself, so
-            FIXME: consider not doing this.  */
-         || errno == EIO
-
-         /* And with SunOS-4.1.4 client and OpenBSD-2.3 server,
-            we get ENOTEMPTY.  */
-         || errno == ENOTEMPTY)
+      if (errno == EINVAL)
        {
          /* FIXME: this is a little fragile in that it relies on rename(2)
             failing with a specific errno value.  Expect problems on