All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
From: Paul Eggert <eggert@cs.ucla.edu>
To: "Pádraig Brady" <P@draigBrady.com>
Cc: 69532@debbugs.gnu.org, Petr Malat <oss@malat.biz>,
	Rob Landley <rob@landley.net>,
	util-linux <util-linux@vger.kernel.org>,
	Dominique Martinet <asmadeus@codewreck.org>,
	Karel Zak <kzak@redhat.com>
Subject: Re: bug#69532: mv's new -x option should be made orthogonal to -t/-T/default
Date: Sat, 16 Mar 2024 23:10:50 -0700	[thread overview]
Message-ID: <c5622a1c-b59e-4eb3-9d81-111acc1fbddc@cs.ucla.edu> (raw)
In-Reply-To: <636f1247-0de0-2f32-cb04-f6952b6807aa@draigBrady.com>

[-- Attachment #1: Type: text/plain, Size: 1760 bytes --]

On 2024-03-05 06:16, Pádraig Brady wrote:
> I think I'll remove the as yet unreleased mv --swap from coreutils, 
> given that
> util-linux is as widely available as coreutils on GNU/Linux platforms.

Although removing that "mv --swap" implementation was a win, I don't 
think we can simply delegate this to util-linux's exch command. 
Exchanging files via a renameat-like call is not limited to the Linux 
kernel; it's also possible on macOS via renameatx_np with RENAME_SWAP, 
and there have been noises about adding similar things to other 
operating systems.

I just now added support for macOS renameatx_np to Gnulib, so coreutils 
does not need to worry about the macOS details; it can simply use 
renameatu with the Linux flags. See:

https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=af32ee824ee18255839f9812b8ed61aa5257a82b

Even with Linux it's dicey. People may have older util-linux installed 
and so lack the 'exch' utility; this is true for both Fedora 39 and 
Ubuntu 23.10, the current releases. Ubuntu is also odd in that it 
doesn't install all the util-linux utilities as part of the util-linux 
package, so it's not clear what they will do with 'exch'.

So I propose that we implement the idea in coreutils in a better way, 
that interacts more nicely with -t, -T, etc. Also, I suggest using the 
Linuxish name "--exchange" instead of the macOSish name "--swap", and 
(for now at least) not giving the option a single-letter equivalent as I 
expect it to be useful from scripts, not interactively.

After looking at various ways to do it I came up with the attached 
proposed patch. This should work on both GNU/Linux and macOS, if your OS 
is recent enough and the file system supports atomic exchange.

[-- Attachment #2: 0001-mv-new-option-exchange.patch --]
[-- Type: text/x-patch, Size: 12415 bytes --]

From d522aba06107d3532ad6103470727bf9057f8d2c Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 16 Mar 2024 22:50:17 -0700
Subject: [PATCH] mv: new option --exchange

* src/copy.h (struct cp_options): New member 'exchange'.
* src/copy.c (copy_internal): Support the new member.
* src/mv.c (EXCHANGE_OPTION): New constant.
(long_options): Add --exchange.
(usage): Document --exchange.
(main): Support --exchange.
* tests/mv/mv-exchange.sh: New test case.
* tests/local.mk (all_tests): Add it.
---
 NEWS                    |  7 ++++++
 doc/coreutils.texi      | 18 ++++++++++++++
 src/copy.c              | 54 +++++++++++++++++++++++------------------
 src/copy.h              |  4 +++
 src/mv.c                | 16 +++++++++---
 tests/local.mk          |  1 +
 tests/mv/mv-exchange.sh | 41 +++++++++++++++++++++++++++++++
 7 files changed, 114 insertions(+), 27 deletions(-)
 create mode 100755 tests/mv/mv-exchange.sh

diff --git a/NEWS b/NEWS
index f21efc7c0..67bb27ebb 100644
--- a/NEWS
+++ b/NEWS
@@ -81,6 +81,13 @@ GNU coreutils NEWS                                    -*- outline -*-
   and the command exits with failure status if existing files.
   The -n,--no-clobber option is best avoided due to platform differences.
 
+  mv now accepts an --exchange option, which causes the source and
+  destination to be exchanged.  It should be combined with
+  --no-target-directory (-T) if the destination is a directory.
+  The exchange is atomic if source and destination are on a single
+  file system that supports atomic exchange; --exchange is not yet
+  supported in other situations.
+
   od now supports printing IEEE half precision floating point with -t fH,
   or brain 16 bit floating point with -t fB, where supported by the compiler.
 
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index d07ed7e76..c456a03d9 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -10269,6 +10269,24 @@ skip existing files but not fail.
 If a file cannot be renamed because the destination file system differs,
 fail with a diagnostic instead of copying and then removing the file.
 
+@item --exchange
+@opindex --exchange
+Exchange source and destination instead of renaming source to destination.
+Both files must exist; they need not be the same type.
+The exchange is atomic if the source and destination are both in a
+single file system that supports atomic exchange;
+exchanges are not yet supported in other situations.
+
+This option can be used to replace one directory with another, atomically.
+When used this way, it should be combined with
+@code{--no-target-directory} (@option{-T})
+to avoid confusion about the destination location.
+Also, if the two directories might not be on the same file system,
+using @code{--no-copy} will prevent future
+versions of @command{mv} from implementing the exchange by copying.
+For example, you might use @samp{mv -T --exchange --no-copy
+@var{d1} @var{d2}} to exchange the directories @var{d1} and @var{d2}.
+
 @item -u
 @itemx --update
 @opindex -u
diff --git a/src/copy.c b/src/copy.c
index 8d99f8562..e7bf6022f 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2223,9 +2223,11 @@ copy_internal (char const *src_name, char const *dst_name,
     {
       if (rename_errno < 0)
         rename_errno = (renameatu (AT_FDCWD, src_name, dst_dirfd, drelname,
-                                   RENAME_NOREPLACE)
+                                   (x->exchange
+                                    ? RENAME_EXCHANGE : RENAME_NOREPLACE))
                         ? errno : 0);
-      nonexistent_dst = *rename_succeeded = rename_errno == 0;
+      *rename_succeeded = rename_errno == 0;
+      nonexistent_dst = *rename_succeeded && !x->exchange;
     }
 
   if (rename_errno == 0
@@ -2246,7 +2248,7 @@ copy_internal (char const *src_name, char const *dst_name,
 
       src_mode = src_sb.st_mode;
 
-      if (S_ISDIR (src_mode) && !x->recursive)
+      if (S_ISDIR (src_mode) && !x->recursive && !x->exchange)
         {
           error (0, 0, ! x->install_mode /* cp */
                  ? _("-r not specified; omitting directory %s")
@@ -2289,7 +2291,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)
+  if (! new_dst && ! x->exchange)
     {
       /* 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
@@ -2657,7 +2659,7 @@ skip:
      Also, with --recursive, record dev/ino of each command-line directory.
      We'll use that info to detect this problem: cp -R dir dir.  */
 
-  if (rename_errno == 0)
+  if (rename_errno == 0 || x->exchange)
     earlier_file = nullptr;
   else if (x->recursive && S_ISDIR (src_mode))
     {
@@ -2752,7 +2754,7 @@ skip:
 
   if (x->move_mode)
     {
-      if (rename_errno == EEXIST)
+      if (rename_errno == EEXIST && !x->exchange)
         rename_errno = (renameat (AT_FDCWD, src_name, dst_dirfd, drelname) == 0
                         ? 0 : errno);
 
@@ -2781,7 +2783,7 @@ skip:
                  _destination_ dev/ino, since the rename above can't have
                  changed those, and 'mv' always uses lstat.
                  We could limit it further by operating
-                 only on non-directories.  */
+                 only on non-directories when !x->exchange.  */
               record_file (x->dest_info, dst_relname, &src_sb);
             }
 
@@ -2828,7 +2830,7 @@ skip:
          where you'd replace '18' with the integer in parentheses that
          was output from the perl one-liner above.
          If necessary, of course, change '/tmp' to some other directory.  */
-      if (rename_errno != EXDEV || x->no_copy)
+      if (rename_errno != EXDEV || x->no_copy || x->exchange)
         {
           /* There are many ways this can happen due to a race condition.
              When something happens between the initial follow_fstatat and the
@@ -2841,25 +2843,29 @@ skip:
              destination file are made too restrictive, the rename will
              fail.  Etc.  */
           char const *quoted_dst_name = quoteaf_n (1, dst_name);
-          switch (rename_errno)
-            {
-            case EDQUOT: case EEXIST: case EISDIR: case EMLINK:
-            case ENOSPC: case ETXTBSY:
+          if (x->exchange)
+            error (0, rename_errno, _("cannot exchange %s and %s"),
+                   quoteaf_n (0, src_name), quoted_dst_name);
+          else
+            switch (rename_errno)
+              {
+              case EDQUOT: case EEXIST: case EISDIR: case EMLINK:
+              case ENOSPC: case ETXTBSY:
 #if ENOTEMPTY != EEXIST
-            case ENOTEMPTY:
+              case ENOTEMPTY:
 #endif
-              /* The destination must be the problem.  Don't mention
-                 the source as that is more likely to confuse the user
-                 than be helpful.  */
-              error (0, rename_errno, _("cannot overwrite %s"),
-                     quoted_dst_name);
-              break;
+                /* The destination must be the problem.  Don't mention
+                   the source as that is more likely to confuse the user
+                   than be helpful.  */
+                error (0, rename_errno, _("cannot overwrite %s"),
+                       quoted_dst_name);
+                break;
 
-            default:
-              error (0, rename_errno, _("cannot move %s to %s"),
-                     quoteaf_n (0, src_name), quoted_dst_name);
-              break;
-            }
+              default:
+                error (0, rename_errno, _("cannot move %s to %s"),
+                       quoteaf_n (0, src_name), quoted_dst_name);
+                break;
+              }
           forget_created (src_sb.st_ino, src_sb.st_dev);
           return false;
         }
diff --git a/src/copy.h b/src/copy.h
index dfa9435b3..ab89c75fd 100644
--- a/src/copy.h
+++ b/src/copy.h
@@ -155,6 +155,10 @@ struct cp_options
      If that fails and NO_COPY, fail instead of copying.  */
   bool move_mode, no_copy;
 
+  /* Exchange instead of renaming.  Valid only if MOVE_MODE and if
+     BACKUP_TYPE == no_backups.  */
+  bool exchange;
+
   /* If true, install(1) is the caller.  */
   bool install_mode;
 
diff --git a/src/mv.c b/src/mv.c
index 9dc40fe3e..692943a70 100644
--- a/src/mv.c
+++ b/src/mv.c
@@ -48,6 +48,7 @@
 enum
 {
   DEBUG_OPTION = CHAR_MAX + 1,
+  EXCHANGE_OPTION,
   NO_COPY_OPTION,
   STRIP_TRAILING_SLASHES_OPTION
 };
@@ -67,6 +68,7 @@ static struct option const long_options[] =
   {"backup", optional_argument, nullptr, 'b'},
   {"context", no_argument, nullptr, 'Z'},
   {"debug", no_argument, nullptr, DEBUG_OPTION},
+  {"exchange", no_argument, nullptr, EXCHANGE_OPTION},
   {"force", no_argument, nullptr, 'f'},
   {"interactive", no_argument, nullptr, 'i'},
   {"no-clobber", no_argument, nullptr, 'n'},   /* Deprecated.  */
@@ -271,6 +273,9 @@ Rename SOURCE to DEST, or move SOURCE(s) to DIRECTORY.\n\
 "), stdout);
       fputs (_("\
       --debug                  explain how a file is copied.  Implies -v\n\
+"), stdout);
+      fputs (_("\
+      --exchange               exchange source and destination\n\
 "), stdout);
       fputs (_("\
   -f, --force                  do not prompt before overwriting\n\
@@ -361,6 +366,9 @@ main (int argc, char **argv)
         case DEBUG_OPTION:
           x.debug = x.verbose = true;
           break;
+        case EXCHANGE_OPTION:
+          x.exchange = true;
+          break;
         case NO_COPY_OPTION:
           x.no_copy = true;
           break;
@@ -469,7 +477,7 @@ main (int argc, char **argv)
   else
     {
       char const *lastfile = file[n_files - 1];
-      if (n_files == 2)
+      if (n_files == 2 && !x.exchange)
         x.rename_errno = (renameatu (AT_FDCWD, file[0], AT_FDCWD, lastfile,
                                      RENAME_NOREPLACE)
                           ? errno : 0);
@@ -514,11 +522,13 @@ main (int argc, char **argv)
       strip_trailing_slashes (file[i]);
 
   if (make_backups
-      && (x.interactive == I_ALWAYS_SKIP
+      && (x.exchange
+          || x.interactive == I_ALWAYS_SKIP
           || x.interactive == I_ALWAYS_NO))
     {
       error (0, 0,
-             _("--backup is mutually exclusive with -n or --update=none-fail"));
+             _("cannot combine --backup with "
+               "--exchange, -n, or --update=none-fail"));
       usage (EXIT_FAILURE);
     }
 
diff --git a/tests/local.mk b/tests/local.mk
index 7cd1ef7b5..f0ac0386f 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -698,6 +698,7 @@ all_tests =					\
   tests/mv/into-self-3.sh			\
   tests/mv/into-self-4.sh			\
   tests/mv/leak-fd.sh				\
+  tests/mv/mv-exchange.sh			\
   tests/mv/mv-n.sh				\
   tests/mv/mv-special-1.sh			\
   tests/mv/no-copy.sh				\
diff --git a/tests/mv/mv-exchange.sh b/tests/mv/mv-exchange.sh
new file mode 100755
index 000000000..485403a1d
--- /dev/null
+++ b/tests/mv/mv-exchange.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+# Test mv --exchange.
+
+# Copyright (C) 2024 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ mv
+
+
+# Test exchanging files.
+touch a || framework_failure_
+mkdir b || framework_failure_
+if ! mv -T --exchange a b 2>exchange_err; then
+  grep 'not supported' exchange_err || { cat exchange_err; fail=1; }
+else
+  test -d a || fail=1
+  test -f b || fail=1
+fi
+
+# Test wrong number of arguments.
+touch c || framework_failure_
+returns_ 1 mv --exchange a 2>/dev/null || fail=1
+returns_ 1 mv --exchange a b c 2>/dev/null || fail=1
+
+# Both files must exist.
+returns_ 1 mv --exchange a d 2>/dev/null || fail=1
+
+Exit $fail
-- 
2.40.1


  parent reply	other threads:[~2024-03-17  6:18 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <10c814a7-cb68-4fb4-ad8d-f88f286fb0b1@cs.ucla.edu>
     [not found] ` <58281f96-f9c6-4567-e3c9-c6a6cfa6ce27@draigBrady.com>
     [not found]   ` <ZeZqzB4-OzHYfFeQ@codewreck.org>
     [not found]     ` <5914e8b2-48ac-456b-9753-6a7bae7a9bbb@cs.ucla.edu>
2024-03-05 14:16       ` bug#69532: mv's new -x option should be made orthogonal to -t/-T/default Pádraig Brady
2024-03-05 20:41         ` Karel Zak
2024-03-05 22:13         ` Masatake YAMATO
2024-03-17  6:10         ` Paul Eggert [this message]
2024-03-17 11:32           ` Pádraig Brady
2024-03-17 11:40             ` Pádraig Brady
2024-03-20 22:10             ` Paul Eggert
2024-03-20 19:43           ` Bernhard Voelker
2024-03-20 20:56             ` Paul Eggert
2024-03-20 22:53               ` Bernhard Voelker
2024-03-20 23:56                 ` Paul Eggert
2024-03-21 21:45                   ` Bernhard Voelker
2024-03-23  1:44                     ` Paul Eggert
2024-03-23 10:24                       ` Bernhard Voelker
2024-03-22 10:22                 ` Karel Zak
2024-03-23 10:24                   ` Bernhard Voelker
2024-03-21  0:03             ` Rob Landley

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c5622a1c-b59e-4eb3-9d81-111acc1fbddc@cs.ucla.edu \
    --to=eggert@cs.ucla.edu \
    --cc=69532@debbugs.gnu.org \
    --cc=P@draigBrady.com \
    --cc=asmadeus@codewreck.org \
    --cc=kzak@redhat.com \
    --cc=oss@malat.biz \
    --cc=rob@landley.net \
    --cc=util-linux@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.