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 <[email protected]> > 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.
