On 3/21/24 14:45, Bernhard Voelker wrote:
On 3/21/24 00:56, Paul Eggert wrote:
On 3/20/24 15:53, Bernhard Voelker wrote:
Yes, that's the expected behavior for this contrived case. Just as one
would get odd behavior if one did the same thing without --exchange.
There's another which is not consistent with/without --exchange:
$ src/mv -v a a
src/mv: 'a' and 'a' are the same file
$ src/mv -v --exchange a a
renamed 'a' -> 'a'
RENAME_EXCHANGE is allowed (but useless?) for 1 file.
Yes, thanks, --exchange should act more like non --exchange there.
BTW: shouldn't the -v diagnostic better say "exchanged 'a' <-> 'a'"
because that's what happened?
Good suggestion.
It seems that -i is skipped:
$ src/mv -iv --exchange a b
renamed 'a' -> 'b'
Yes, I suppose -i should be treated similarly too.
I installed the attached patches to do the above. (Basically, the
problem was that my earlier patches were too ambitious; these patches
scale things back to avoid some optimizations so that mv --exchange is
more like ordinary mv.)
The first patch simplifies the code (and fixes a diagnostic to be more
useful) without otherwise changing behavior; it's more of a refactoring.
The second patch does the real work.
Thanks again.
From ff42adb55e99226f55a7b141316ee2a7b84a4857 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@xxxxxxxxxxx>
Date: Fri, 22 Mar 2024 12:02:41 -0700
Subject: [PATCH 1/2] cp,ln,mv: improve dir vs nondir diagnostics
* src/copy.c (copy_internal): Simplify logic for copying
from directory to non-directory or vice versa, and always
diagnose with both source and destination file names.
---
src/copy.c | 88 ++++++++++++++++--------------------------------------
1 file changed, 26 insertions(+), 62 deletions(-)
diff --git a/src/copy.c b/src/copy.c
index e7bf6022f..8b4a29692 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2451,72 +2451,36 @@ skip:
if (return_now)
return return_val;
- if (!S_ISDIR (dst_sb.st_mode))
+ /* Copying a directory onto a non-directory, or vice versa,
+ is ok only with --backup. */
+ if (!S_ISDIR (src_mode) != !S_ISDIR (dst_sb.st_mode)
+ && x->backup_type == no_backups)
{
- if (S_ISDIR (src_mode))
- {
- if (x->move_mode && x->backup_type != no_backups)
- {
- /* Moving a directory onto an existing
- non-directory is ok only with --backup. */
- }
- else
- {
- error (0, 0,
- _("cannot overwrite non-directory %s with directory %s"),
- quoteaf_n (0, dst_name), quoteaf_n (1, src_name));
- return false;
- }
- }
-
- /* Don't let the user destroy their data, even if they try hard:
- This mv command must fail (likewise for cp):
- rm -rf a b c; mkdir a b c; touch a/f b/f; mv a/f b/f c
- Otherwise, the contents of b/f would be lost.
- In the case of 'cp', b/f would be lost if the user simulated
- a move using cp and rm.
- Note that it works fine if you use --backup=numbered. */
- if (command_line_arg
- && x->backup_type != numbered_backups
- && seen_file (x->dest_info, dst_relname, &dst_sb))
- {
- error (0, 0,
- _("will not overwrite just-created %s with %s"),
- quoteaf_n (0, dst_name), quoteaf_n (1, src_name));
- return false;
- }
- }
-
- if (!S_ISDIR (src_mode))
- {
- if (S_ISDIR (dst_sb.st_mode))
- {
- if (x->move_mode && x->backup_type != no_backups)
- {
- /* Moving a non-directory onto an existing
- directory is ok only with --backup. */
- }
- else
- {
- error (0, 0,
- _("cannot overwrite directory %s with non-directory"),
- quoteaf (dst_name));
- return false;
- }
- }
+ error (0, 0,
+ _(S_ISDIR (src_mode)
+ ? ("cannot overwrite non-directory %s "
+ "with directory %s")
+ : ("cannot overwrite directory %s "
+ "with non-directory %s")),
+ quoteaf_n (0, dst_name), quoteaf_n (1, src_name));
+ return false;
}
- if (x->move_mode)
+ /* Don't let the user destroy their data, even if they try hard:
+ This mv command must fail (likewise for cp):
+ rm -rf a b c; mkdir a b c; touch a/f b/f; mv a/f b/f c
+ Otherwise, the contents of b/f would be lost.
+ In the case of 'cp', b/f would be lost if the user simulated
+ a move using cp and rm.
+ Note that it works fine if you use --backup=numbered. */
+ if (!S_ISDIR (dst_sb.st_mode) && command_line_arg
+ && x->backup_type != numbered_backups
+ && seen_file (x->dest_info, dst_relname, &dst_sb))
{
- /* Don't allow user to move a directory onto a non-directory. */
- if (S_ISDIR (src_sb.st_mode) && !S_ISDIR (dst_sb.st_mode)
- && x->backup_type == no_backups)
- {
- error (0, 0,
- _("cannot move directory onto non-directory: %s -> %s"),
- quotef_n (0, src_name), quotef_n (0, dst_name));
- return false;
- }
+ error (0, 0,
+ _("will not overwrite just-created %s with %s"),
+ quoteaf_n (0, dst_name), quoteaf_n (1, src_name));
+ return false;
}
char const *srcbase;
--
2.44.0
From 5a1d00e4504204dc4c84eb641abb961e8074a218 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@xxxxxxxxxxx>
Date: Fri, 22 Mar 2024 18:38:08 -0700
Subject: [PATCH 2/2] mv: treat --exchange more like non-exchange
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Also, improve quality of diagnostics.
Problems/suggestions by Bernhard Voelker in
<https://bugs.gnu.org/69532#82>.
* src/copy.c (emit_verbose): New arg FORMAT. All uses changed,
to improve quality of diagnostics when --exchange is used.
(copy_internal): Don’t try to optimize --exchange so much; this
simplifies the code and keeps it closer to the non --exchange case.
---
src/copy.c | 48 +++++++++++++++++++++++-------------------------
1 file changed, 23 insertions(+), 25 deletions(-)
diff --git a/src/copy.c b/src/copy.c
index 8b4a29692..817d5b13b 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2075,9 +2075,10 @@ abandon_move (const struct cp_options *x,
If BACKUP_DST_NAME is non-null, then also indicate that it is
the name of a backup file. */
static void
-emit_verbose (char const *src, char const *dst, char const *backup_dst_name)
+emit_verbose (char const *format, char const *src, char const *dst,
+ char const *backup_dst_name)
{
- printf ("%s -> %s", quoteaf_n (0, src), quoteaf_n (1, dst));
+ printf (format, quoteaf_n (0, src), quoteaf_n (1, dst));
if (backup_dst_name)
printf (_(" (backup: %s)"), quoteaf (backup_dst_name));
putchar ('\n');
@@ -2219,15 +2220,13 @@ copy_internal (char const *src_name, char const *dst_name,
*copy_into_self = false;
int rename_errno = x->rename_errno;
- if (x->move_mode)
+ if (x->move_mode && !x->exchange)
{
if (rename_errno < 0)
rename_errno = (renameatu (AT_FDCWD, src_name, dst_dirfd, drelname,
- (x->exchange
- ? RENAME_EXCHANGE : RENAME_NOREPLACE))
+ RENAME_NOREPLACE)
? errno : 0);
- *rename_succeeded = rename_errno == 0;
- nonexistent_dst = *rename_succeeded && !x->exchange;
+ nonexistent_dst = *rename_succeeded = rename_errno == 0;
}
if (rename_errno == 0
@@ -2248,7 +2247,7 @@ copy_internal (char const *src_name, char const *dst_name,
src_mode = src_sb.st_mode;
- if (S_ISDIR (src_mode) && !x->recursive && !x->exchange)
+ if (S_ISDIR (src_mode) && !x->recursive)
{
error (0, 0, ! x->install_mode /* cp */
? _("-r not specified; omitting directory %s")
@@ -2291,7 +2290,7 @@ copy_internal (char const *src_name, char const *dst_name,
treated the same as nonexistent files. */
bool new_dst = 0 < nonexistent_dst;
- if (! new_dst && ! x->exchange)
+ if (! new_dst)
{
/* Normally, fill in DST_SB or set NEW_DST so that later code
can use DST_SB if NEW_DST is false. However, don't bother
@@ -2452,9 +2451,9 @@ skip:
return return_val;
/* Copying a directory onto a non-directory, or vice versa,
- is ok only with --backup. */
+ is ok only with --backup or --exchange. */
if (!S_ISDIR (src_mode) != !S_ISDIR (dst_sb.st_mode)
- && x->backup_type == no_backups)
+ && x->backup_type == no_backups && !x->exchange)
{
error (0, 0,
_(S_ISDIR (src_mode)
@@ -2472,9 +2471,9 @@ skip:
Otherwise, the contents of b/f would be lost.
In the case of 'cp', b/f would be lost if the user simulated
a move using cp and rm.
- Note that it works fine if you use --backup=numbered. */
+ Nothing is lost if you use --backup=numbered or --exchange. */
if (!S_ISDIR (dst_sb.st_mode) && command_line_arg
- && x->backup_type != numbered_backups
+ && x->backup_type != numbered_backups && !x->exchange
&& seen_file (x->dest_info, dst_relname, &dst_sb))
{
error (0, 0,
@@ -2591,7 +2590,7 @@ skip:
sure we'll create a directory. Also don't announce yet when moving
so we can distinguish renames versus copies. */
if (x->verbose && !x->move_mode && !S_ISDIR (src_mode))
- emit_verbose (src_name, dst_name, dst_backup);
+ emit_verbose ("%s -> %s", src_name, dst_name, dst_backup);
/* Associate the destination file name with the source device and inode
so that if we encounter a matching dev/ino pair in the source tree
@@ -2718,17 +2717,19 @@ skip:
if (x->move_mode)
{
- if (rename_errno == EEXIST && !x->exchange)
- rename_errno = (renameat (AT_FDCWD, src_name, dst_dirfd, drelname) == 0
+ if (rename_errno == EEXIST)
+ rename_errno = ((renameatu (AT_FDCWD, src_name, dst_dirfd, drelname,
+ x->exchange ? RENAME_EXCHANGE : 0)
+ == 0)
? 0 : errno);
if (rename_errno == 0)
{
if (x->verbose)
- {
- printf (_("renamed "));
- emit_verbose (src_name, dst_name, dst_backup);
- }
+ emit_verbose (_(x->exchange
+ ? "exchanged %s <-> %s"
+ : "renamed %s -> %s"),
+ src_name, dst_name, dst_backup);
if (x->set_security_context)
{
@@ -2853,10 +2854,7 @@ skip:
}
if (x->verbose && !S_ISDIR (src_mode))
- {
- printf (_("copied "));
- emit_verbose (src_name, dst_name, dst_backup);
- }
+ emit_verbose (_("copied %s -> %s"), src_name, dst_name, dst_backup);
new_dst = true;
}
@@ -2956,7 +2954,7 @@ skip:
if (x->move_mode)
printf (_("created directory %s\n"), quoteaf (dst_name));
else
- emit_verbose (src_name, dst_name, nullptr);
+ emit_verbose ("%s -> %s", src_name, dst_name, nullptr);
}
}
else
--
2.44.0
