bug#14763: mv directory cross-filesystem where destination exists fails to remove dest with EISDIR

[Pádraig Brady]

On 07/02/2013 01:29 AM, Ken Booth wrote:
> Hi Eric,
> 
> Thanks for the reference to the POSIX standards.
> 
> The reason I wrote the patch the way I did was to emulate Solaris behaviour 
> for a non-empty directory, however I read at
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html
> the sentence "If /new/ names an existing directory, it shall be required to 
> be an empty directory. "
> 
> Therefore I conclude that Solaris is not POSIX compliant.
> 
> After reading the instructions you suggested I hope the following patch is in 
> the correct format, conforms to the requirements, and is also POSIX compliant 
> ...
> 
> From 8b6549f321c06ee81262f58c6d7bd7e9c9092b30 Mon Sep 17 00:00:00 2001
> From: Ken Booth <address@hidden>
> 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;

This looks good, thanks.
Though I don't think there's a need to duplicate the blocks above:
One could minimize to:

-      if (unlink (dst_name) != 0 && errno != ENOENT)
+      if ((S_ISDIR (src_mode) ? rmdir : unlink) (dst_name) != 0
+          && errno != ENOENT)

Though I think unlink at least can be a function like macro,
and so the above could cause compile issues on some platforms.
Therefore I'm adjusting to the following equivalent and will then push:

-      if (unlink (dst_name) != 0 && errno != ENOENT)
+      if ((S_ISDIR (src_mode) ? rmdir (dst_name) : unlink (dst_name)) != 0
+          && errno != ENOENT)

Note we're checking the type of src which is a bit confusing
since we're removing dst.  Now we could check dst as the
overwrite dir with non dir case is checked for earlier (and vice versa).
But I guess this is a double check for this case.
I'll add a comment along these lines.

I'll add a test too.

thanks,
Pádraig.