On 24/07/13 18:14, Pádraig Brady wrote:
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.
Hi Pádraig,
I thought about the fact that we're only checking src and decided its
the correct behaviour, as we intend to get an error if dst is the wrong
type. I should have included a comment to that effect, just to make it
clear that we dont want to check dst.
So the code as you've written it should stand without an extra check,
just a comment.
Thanks for approving my first patch.
Regards,
Ken.
