Subject: Re: bug#69532: mv's new -x option should be made orthogonal to -t/-T/default

From: Paul Eggert <[email-masked]>
Date: Fri, 22 Mar 2024 18:44:39 -0700

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 <[email-masked]>
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 <[email-masked]>
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