Bug 980061

Thanks for suggestion, I'll check if this is acceptable with upstream. Please do not set fast flags next time. Maintainer should do that himself.

The first draft of suggested code doesn't work, so here is a second draft

2176c2176
<       if (unlink (dst_name) != 0 && errno != ENOENT)
---
>       if (unlink (dst_name) != 0 && errno != ENOENT && errno != EISDIR)
2267,2269c2267,2274
<               error (0, errno, _("cannot create directory %s"),
<                      quote (dst_name));
<               goto un_backup;
---
>               if (errno == EEXIST)
>                 {
>                   //* some code here to chmod the directory *//
>                 } else {
>                   error (0, errno, _("cannot create directory %s"),
>                          quote (dst_name));
>                   goto un_backup;
>                 }

Needs a bit of work to flesh out chmod'ing the destination directory, but it achieves the same bahaviour as Solaris has.

When the destination directory exists and has contents the source directory merges the directories clobbering existing files.

E.g.
f18# cp /etc/group /test
f18# vi /test/passwd
f18# mkdir /home/test
f18# cp /etc/passwd /home/test
f18# ls -l /home/test /test
/home/test:
total 4
-rw-r--r--. 1 root root 2768 Jul  1 12:54 passwd

/test:
total 8
-rw-r--r--. 1 root root 1162 Jul  1 12:53 group
-rw-r--r--. 1 root root 2781 Jul  1 12:54 passwd
f18# /home/ken/git/fedora/coreutils/src/mv /home/test /
f18# ls -l /home/test /test
ls: cannot access /home/test: No such file or directory
/test:
total 8
-rw-r--r--. 1 root root 1162 Jul  1 12:53 group
-rw-r--r--. 1 root root 2768 Jul  1 12:54 passwd

Now, the question is. Do we want to accept this behaviour as correct for Linux?

Not unlike with upstream - so this should be discussed at bug-coreutils or coreutils - if you want to bring it up there, feel free to do so, otherwise I will raise this question myself - hopefully later this week. Still, 

I think it is not something what should be changed in released RHEL, as it may bring in some nasty consequences for the people who rely on current behaviour.

Final draft for proposed changes is as follows:

src/copy.c
2176c2176
<       if (unlink (dst_name) != 0 && errno != ENOENT)
---
>       if (unlink (dst_name) != 0 && errno != ENOENT && errno != EISDIR)
2267,2269c2267,2278
<               error (0, errno, _("cannot create directory %s"),
<                      quote (dst_name));
<               goto un_backup;
---
>               if (errno == EEXIST)
>                 {
>                   if (lchmod (dst_name, dst_mode & ~omitted_permissions) != 0)
>                     {
>                       error (0, errno, _("setting permissions for %s"),
>                              quote (dst_name));
>                     }
>                 } else {
>                   error (0, errno, _("cannot create directory %s"),
>                          quote (dst_name));
>                   goto un_backup;
>                 }

Completed various tests and this appears to achieve the desired effect.

(In reply to Ken Booth from comment #5)
> Final draft for proposed changes is as follows:
> 
> src/copy.c
> 2176c2176
> <       if (unlink (dst_name) != 0 && errno != ENOENT)
> ---
> >       if (unlink (dst_name) != 0 && errno != ENOENT && errno != EISDIR)

Ed script patches are illegible.  Please repost as a context diff.

Wouldn't this be simpler?

diff --git i/src/copy.c w/src/copy.c
index c1c8273..599ed8b 100644
--- i/src/copy.c
+++ w/src/copy.c
@@ -2173,7 +2173,7 @@ copy_internal (char const *src_name, char const *dst_name,
       /* The rename attempt has failed.  Remove any existing destination
          file so that a cross-device 'mv' acts as if it were really using
          the rename syscall.  */
-      if (unlink (dst_name) != 0 && errno != ENOENT)
+      if (remove (dst_name) != 0 && errno != ENOENT)
         {
           error (0, errno,
              _("inter-device move failed: %s to %s; unable to remove target"),

After discussing with Eric Blake on bug-coreutils determined that the above patch is not atomic (using special handling of the unlink error is not recommended practice), and further reading of POSIX standards determines that Solaris is not POSIX compliant.

Therefore propsed a new patch as follows ...

From 8b6549f321c06ee81262f58c6d7bd7e9c9092b30 Mon Sep 17 00:00:00 2001
From: Ken Booth <ken.uk>
Date: Tue, 2 Jul 2013 01:06:32 +0100
Subject: [PATCH] mv: if inter-device move target is directory use rmdir, not
 unlink

---
 src/copy.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index c1c8273..5137f27 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2173,13 +2173,27 @@ copy_internal (char const *src_name, char const *dst_name,
       /* The rename attempt has failed.  Remove any existing destination
          file so that a cross-device 'mv' acts as if it were really using
          the rename syscall.  */
-      if (unlink (dst_name) != 0 && errno != ENOENT)
+      if (S_ISDIR (src_mode))
         {
-          error (0, errno,
-             _("inter-device move failed: %s to %s; unable to remove target"),
-                 quote_n (0, src_name), quote_n (1, dst_name));
-          forget_created (src_sb.st_ino, src_sb.st_dev);
-          return false;
+          if (rmdir (dst_name) != 0 && errno != ENOENT)
+            {
+              error (0, errno,
+                 _("inter-device move failed: %s to %s; unable to remove target directory"),
+                     quote_n (0, src_name), quote_n (1, dst_name));
+              forget_created (src_sb.st_ino, src_sb.st_dev);
+              return false;
+            }
+        }
+      else
+        {
+          if (unlink (dst_name) != 0 && errno != ENOENT)
+            {
+              error (0, errno,
+                 _("inter-device move failed: %s to %s; unable to remove target"),
+                     quote_n (0, src_name), quote_n (1, dst_name));
+              forget_created (src_sb.st_ino, src_sb.st_dev);
+              return false;
+            }
         }
 
       new_dst = true;
-- 
1.8.1.4

Hi Eric,

Oops, just noticed your update. I compiled and tested it and your solution is neater.

I used 20 lines, and you did the same thing in a single line.

Test results give the correct errors:

f18# mkdir /home/test
f18# cp /etc/passwd /home/test
f18# ls -la /home/test /test
/home/test:
total 12
drwxr-xr-x.  2 root root 4096 Jul  2 01:41 .
drwxr-xr-x. 14 root root 4096 Jul  2 01:41 ..
-rw-r--r--.  1 root root 2768 Jul  2 01:41 passwd

/test:
total 12
drwxr-xr-x.  2 root root 4096 Jul  2 00:58 .
dr-xr-xr-x. 25 root root 4096 Jul  2 00:59 ..
-rw-r--r--.  1 root root 2768 Jul  2 00:58 passwd
f18# /home/ken/git/fedora/coreutils/src/mv /home/test /
/home/ken/git/fedora/coreutils/src/mv: inter-device move failed: ‘/home/test’ to ‘/test’; unable to remove target: Directory not empty
f18# rm /test/*
rm: remove regular file ‘/test/passwd’? y
f18# /home/ken/git/fedora/coreutils/src/mv /home/test /
f18# ls -la /home/test /test
ls: cannot access /home/test: No such file or directory
/test:
total 12
drwxr-xr-x.  2 root root 4096 Jul  2 01:41 .
dr-xr-xr-x. 25 root root 4096 Jul  2 01:45 ..
-rw-r--r--.  1 root root 2768 Jul  2 01:41 passwd

(In reply to Ken Booth from comment #9)
> Hi Eric,
> 
> Oops, just noticed your update. I compiled and tested it and your solution
> is neater.
> 
> I used 20 lines, and you did the same thing in a single line.

Ah, but my solution (comment 7) is loosey-goosey when mixing a source file with destination directory, or source directory with destination file; your patch is closer to actual rename() semantics which is what POSIX requires.  At any rate, we'll see what upstream coreutils does about the issue.

Pointer to upstream discussion: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=14763 ... atm. it seems only Ken and Eric discussed the item. As for the question about etiquette in Fedora - please wait for the upstream commit - I'll commit what is used by upstream - before the upstream commit I tend to say we should wait with any actions downstream.

Upstream fix committed:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commit;h=2bdb74ec1

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHSA-2013-1652.html