Subject: bug#62572: cp --no-clobber behavior has changed

From: Pádraig Brady <[email-masked]>
Date: Thu, 6 Apr 2023 15:50:08 +0100

Take 2 attached.

cheers,
Pádraig
From 849aa5658c0fbf1e8d2baec2fc3b01b2ddb23c50 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Sat, 1 Apr 2023 16:27:52 +0100
Subject: [PATCH] cp,mv: add --update=none to always skip existing files

Add --update=none which is equivalent to the --no-clobber behavior
from before coreutils 9.2.  I.e. existing files are unconditionally
skipped, and them not being copied does not affect the exit status.

* src/copy.h [enum Update_type]: A new type to support parameters
to the --update command line option.
[enum Interactive]: Add I_ALWAYS_SKIP.
* src/copy.c: Treat I_ALWAYS_SKIP like I_ALWAYS_NO (-n),
except that we don't fail when skipping.
* src/system.h (emit_update_parameters_note): A new function
to output the description of the new --update parameters.
* src/cp.c (main): Parse --update arguments, ensuring that
-n takes precedence if specified.
(usage): Describe the new option.  Also allude that
-u is related in the -n description.
* src/mv.c: Accept the new --update parameters and
update usage() accordingly.
* doc/coreutils.texi (cp invocation): Describe the new --update
parameters.  Also reference --update from the --no-clobber description.
(mv invocation): Likewise.
* tests/mv/update.sh: Test the new parameters.
* NEWS: Mention the new feature.
Addresses https://bugs.gnu.org/62572
---
 NEWS               |  6 ++++++
 doc/coreutils.texi | 28 ++++++++++++++++++++++--
 src/copy.c         | 15 ++++++++-----
 src/copy.h         | 16 +++++++++++++-
 src/cp.c           | 54 ++++++++++++++++++++++++++++++++++++++++------
 src/mv.c           | 49 ++++++++++++++++++++++++++++++++++++-----
 src/system.h       | 15 +++++++++++++
 tests/mv/update.sh | 47 ++++++++++++++++++++++++++--------------
 8 files changed, 194 insertions(+), 36 deletions(-)

diff --git a/NEWS b/NEWS
index 8f947faed..e4ed291b4 100644
--- a/NEWS
+++ b/NEWS
@@ -27,6 +27,12 @@ GNU coreutils NEWS                                    -*- outline -*-
   wc will now diagnose if any total counts have overflowed.
   [This bug was present in "the beginning".]
 
+** New features
+
+  cp and mv now support --update=none to always skip existing files
+  in the destination, while not affecting the exit status.
+  This is equivalent to the --no-clobber behavior from before v9.2.
+
 
 * Noteworthy changes in release 9.2 (2023-03-20) [stable]
 
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 7852e9f8a..2188922c6 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -9236,9 +9236,9 @@ results in an error message on systems that do not support symbolic links.
 @optNoTargetDirectory
 
 @item -u
-@itemx --update
+@itemx --update[=@var{which}]
 @opindex -u
-@opindex --update
+@opindex --update[=@var{which}]
 @cindex newer files, copying only
 Do not copy a non-directory that has an existing destination with the
 same or newer modification timestamp; instead, silently skip the file
@@ -9254,6 +9254,26 @@ for example), that will take precedence; consequently, depending on the
 order that files are processed from the source, newer files in the destination
 may be replaced, to mirror hard links in the source.
 
+@macro whichUpdate
+@var{which} gives more control over which existing files in the
+destination are replaced, and its value can be one of the following:
+
+@table @samp
+@item all
+This is the default operation when an @option{--update} option is not specified,
+and results in all existing files in the destination being replaced.
+
+@item none
+This is similar to the @option{--no-clobber} option, in that no files in the
+destination are replaced, but also skipping a file does not induce a failure.
+
+@item older
+This is the default operation when @option{--update} is specified, and results
+in files being replaced if they're older than the corresponding source file.
+@end table
+@end macro
+@whichUpdate
+
 @item -v
 @itemx --verbose
 @opindex -v
@@ -10165,6 +10185,8 @@ of its permissions, and fail if the response is not affirmative.
 Do not overwrite an existing file; silently fail instead.
 @mvOptsIfn
 This option is mutually exclusive with @option{-b} or @option{--backup} option.
+See also the @option{--update=none} option which will
+skip existing files but not fail.
 
 @item --no-copy
 @opindex --no-copy
@@ -10188,6 +10210,8 @@ same source and destination.
 This option is ignored if the @option{-n} or @option{--no-clobber}
 option is also specified.
 
+@whichUpdate
+
 @item -v
 @itemx --verbose
 @opindex -v
diff --git a/src/copy.c b/src/copy.c
index a8aa14920..e7e14c150 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2061,6 +2061,7 @@ abandon_move (const struct cp_options *x,
 {
   assert (x->move_mode);
   return (x->interactive == I_ALWAYS_NO
+          || x->interactive == I_ALWAYS_SKIP
           || ((x->interactive == I_ASK_USER
                || (x->interactive == I_UNSPECIFIED
                    && x->stdin_tty
@@ -2234,7 +2235,8 @@ copy_internal (char const *src_name, char const *dst_name,
 
   if (rename_errno == 0
       ? !x->last_file
-      : rename_errno != EEXIST || x->interactive != I_ALWAYS_NO)
+      : rename_errno != EEXIST
+        || (x->interactive != I_ALWAYS_NO && x->interactive != I_ALWAYS_SKIP))
     {
       char const *name = rename_errno == 0 ? dst_name : src_name;
       int dirfd = rename_errno == 0 ? dst_dirfd : AT_FDCWD;
@@ -2288,7 +2290,9 @@ copy_internal (char const *src_name, char const *dst_name,
 
   if (nonexistent_dst <= 0)
     {
-      if (! (rename_errno == EEXIST && x->interactive == I_ALWAYS_NO))
+      if (! (rename_errno == EEXIST
+             && (x->interactive == I_ALWAYS_NO
+                 || x->interactive == I_ALWAYS_SKIP)))
         {
           /* Regular files can be created by writing through symbolic
              links, but other files cannot.  So use stat on the
@@ -2330,7 +2334,7 @@ copy_internal (char const *src_name, char const *dst_name,
         {
           bool return_now = false;
 
-          if (x->interactive != I_ALWAYS_NO
+          if ((x->interactive != I_ALWAYS_NO && x->interactive != I_ALWAYS_SKIP)
               && ! same_file_ok (src_name, &src_sb, dst_dirfd, drelname,
                                  &dst_sb, x, &return_now))
             {
@@ -2400,17 +2404,18 @@ copy_internal (char const *src_name, char const *dst_name,
                      doesn't end up removing the source file.  */
                   if (rename_succeeded)
                     *rename_succeeded = true;
-                  return false;
+                  return x->interactive == I_ALWAYS_SKIP;
                 }
             }
           else
             {
               if (! S_ISDIR (src_mode)
                   && (x->interactive == I_ALWAYS_NO
+                      || x->interactive == I_ALWAYS_SKIP
                       || (x->interactive == I_ASK_USER
                           && ! overwrite_ok (x, dst_name, dst_dirfd,
                                              dst_relname, &dst_sb))))
-                return false;
+                return x->interactive == I_ALWAYS_SKIP;
             }
 
           if (return_now)
diff --git a/src/copy.h b/src/copy.h
index b02aa2bbb..ea5023cdb 100644
--- a/src/copy.h
+++ b/src/copy.h
@@ -57,11 +57,25 @@ enum Reflink_type
   REFLINK_ALWAYS
 };
 
+/* Control how existing destination files are updated.  */
+enum Update_type
+{
+  /* Always update..  */
+  UPDATE_ALL,
+
+  /* Update if dest older.  */
+  UPDATE_OLDER,
+
+  /* Leave existing files.  */
+  UPDATE_NONE,
+};
+
 /* This type is used to help mv (via copy.c) distinguish these cases.  */
 enum Interactive
 {
   I_ALWAYS_YES = 1,
-  I_ALWAYS_NO,
+  I_ALWAYS_NO,       /* Skip and fail.   */
+  I_ALWAYS_SKIP,     /* Skip and ignore. */
   I_ASK_USER,
   I_UNSPECIFIED
 };
diff --git a/src/cp.c b/src/cp.c
index 75ae7de47..488770a0b 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -102,6 +102,16 @@ static enum Reflink_type const reflink_type[] =
 };
 ARGMATCH_VERIFY (reflink_type_string, reflink_type);
 
+static char const *const update_type_string[] =
+{
+  "all", "none", "older", NULL
+};
+static enum Update_type const update_type[] =
+{
+  UPDATE_ALL, UPDATE_NONE, UPDATE_OLDER,
+};
+ARGMATCH_VERIFY (update_type_string, update_type);
+
 static struct option const long_opts[] =
 {
   {"archive", no_argument, NULL, 'a'},
@@ -129,7 +139,7 @@ static struct option const long_opts[] =
   {"suffix", required_argument, NULL, 'S'},
   {"symbolic-link", no_argument, NULL, 's'},
   {"target-directory", required_argument, NULL, 't'},
-  {"update", no_argument, NULL, 'u'},
+  {"update", optional_argument, NULL, 'u'},
   {"verbose", no_argument, NULL, 'v'},
   {GETOPT_SELINUX_CONTEXT_OPTION_DECL},
   {GETOPT_HELP_OPTION_DECL},
@@ -182,8 +192,10 @@ Copy SOURCE to DEST, or multiple SOURCE(s) to DIRECTORY.\n\
   -L, --dereference            always follow symbolic links in SOURCE\n\
 "), stdout);
       fputs (_("\
-  -n, --no-clobber             do not overwrite an existing file (overrides\n\
-                                 a previous -i option)\n\
+  -n, --no-clobber             do not overwrite an existing file (overrides a\n\
+                                 -u or previous -i option). See also --update\n\
+"), stdout);
+      fputs (_("\
   -P, --no-dereference         never follow symbolic links in SOURCE\n\
 "), stdout);
       fputs (_("\
@@ -212,10 +224,14 @@ Copy SOURCE to DEST, or multiple SOURCE(s) to DIRECTORY.\n\
   -T, --no-target-directory    treat DEST as a normal file\n\
 "), stdout);
       fputs (_("\
-  -u, --update                 copy only when the SOURCE file is newer\n\
-                                 than the destination file or when the\n\
-                                 destination file is missing\n\
+  --update[=UPDATE]            control which existing files are updated;\n\
+                                 UPDATE={all,none,older(default)}.  See below\n\
+  -u                           equivalent to --update[=older]\n\
+"), stdout);
+      fputs (_("\
   -v, --verbose                explain what is being done\n\
+"), stdout);
+      fputs (_("\
   -x, --one-file-system        stay on this file system\n\
 "), stdout);
       fputs (_("\
@@ -242,6 +258,7 @@ selected by --sparse=auto.  Specify --sparse=always to create a sparse DEST\n\
 file whenever the SOURCE file contains a long enough sequence of zero bytes.\n\
 Use --sparse=never to inhibit creation of sparse files.\n\
 "), stdout);
+      emit_update_parameters_note ();
       fputs (_("\
 \n\
 When --reflink[=always] is specified, perform a lightweight copy, where the\n\
@@ -1103,7 +1120,30 @@ main (int argc, char **argv)
           break;
 
         case 'u':
-          x.update = true;
+          if (optarg == NULL)
+            x.update = true;
+          else if (x.interactive != I_ALWAYS_NO)  /* -n takes precedence.  */
+            {
+              enum Update_type update_opt;
+              update_opt = XARGMATCH ("--update", optarg,
+                                      update_type_string, update_type);
+              if (update_opt == UPDATE_ALL)
+                {
+                  /* Default cp operation.  */
+                  x.update = false;
+                  x.interactive = I_UNSPECIFIED;
+                }
+              else if (update_opt == UPDATE_NONE)
+                {
+                  x.update = false;
+                  x.interactive = I_ALWAYS_SKIP;
+                }
+              else if (update_opt == UPDATE_OLDER)
+                {
+                  x.update = true;
+                  x.interactive = I_UNSPECIFIED;
+                }
+            }
           break;
 
         case 'v':
diff --git a/src/mv.c b/src/mv.c
index 9cea8dac6..fc2bf77da 100644
--- a/src/mv.c
+++ b/src/mv.c
@@ -24,6 +24,7 @@
 #include <selinux/label.h>
 
 #include "system.h"
+#include "argmatch.h"
 #include "backupfile.h"
 #include "copy.h"
 #include "cp-hash.h"
@@ -53,6 +54,16 @@ enum
   STRIP_TRAILING_SLASHES_OPTION
 };
 
+static char const *const update_type_string[] =
+{
+  "all", "none", "older", NULL
+};
+static enum Update_type const update_type[] =
+{
+  UPDATE_ALL, UPDATE_NONE, UPDATE_OLDER,
+};
+ARGMATCH_VERIFY (update_type_string, update_type);
+
 static struct option const long_options[] =
 {
   {"backup", optional_argument, NULL, 'b'},
@@ -66,7 +77,7 @@ static struct option const long_options[] =
   {"strip-trailing-slashes", no_argument, NULL, STRIP_TRAILING_SLASHES_OPTION},
   {"suffix", required_argument, NULL, 'S'},
   {"target-directory", required_argument, NULL, 't'},
-  {"update", no_argument, NULL, 'u'},
+  {"update", optional_argument, NULL, 'u'},
   {"verbose", no_argument, NULL, 'v'},
   {GETOPT_HELP_OPTION_DECL},
   {GETOPT_VERSION_OPTION_DECL},
@@ -277,15 +288,20 @@ If you specify more than one of -i, -f, -n, only the final one takes effect.\n\
       fputs (_("\
   -t, --target-directory=DIRECTORY  move all SOURCE arguments into DIRECTORY\n\
   -T, --no-target-directory    treat DEST as a normal file\n\
-  -u, --update                 move only when the SOURCE file is newer\n\
-                                 than the destination file or when the\n\
-                                 destination file is missing\n\
+"), stdout);
+      fputs (_("\
+  --update[=UPDATE]            control which existing files are updated;\n\
+                                 UPDATE={all,none,older(default)}.  See below\n\
+  -u                           equivalent to --update[=older]\n\
+"), stdout);
+      fputs (_("\
   -v, --verbose                explain what is being done\n\
   -Z, --context                set SELinux security context of destination\n\
                                  file to default type\n\
 "), stdout);
       fputs (HELP_OPTION_DESCRIPTION, stdout);
       fputs (VERSION_OPTION_DESCRIPTION, stdout);
+      emit_update_parameters_note ();
       emit_backup_suffix_note ();
       emit_ancillary_info (PROGRAM_NAME);
     }
@@ -358,7 +374,30 @@ main (int argc, char **argv)
           no_target_directory = true;
           break;
         case 'u':
-          x.update = true;
+          if (optarg == NULL)
+            x.update = true;
+          else if (x.interactive != I_ALWAYS_NO)  /* -n takes precedence.  */
+            {
+              enum Update_type update_opt;
+              update_opt = XARGMATCH ("--update", optarg,
+                                      update_type_string, update_type);
+              if (update_opt == UPDATE_ALL)
+                {
+                  /* Default mv operation.  */
+                  x.update = false;
+                  x.interactive = I_UNSPECIFIED;
+                }
+              else if (update_opt == UPDATE_NONE)
+                {
+                  x.update = false;
+                  x.interactive = I_ALWAYS_SKIP;
+                }
+              else if (update_opt == UPDATE_OLDER)
+                {
+                  x.update = true;
+                  x.interactive = I_UNSPECIFIED;
+                }
+            }
           break;
         case 'v':
           x.verbose = true;
diff --git a/src/system.h b/src/system.h
index 2aa5d6978..b85897280 100644
--- a/src/system.h
+++ b/src/system.h
@@ -608,6 +608,21 @@ Otherwise, units default to 1024 bytes (or 512 if POSIXLY_CORRECT is set).\n\
 "), program);
 }
 
+static inline void
+emit_update_parameters_note (void)
+{
+  fputs (_("\
+\n\
+UPDATE controls which existing files in the destination are replaced.\n\
+'all' is the default operation when an --update option is not specified,\n\
+and results in all existing files in the destination being replaced.\n\
+'none' is similar to the --no-clobber option, in that no files in the\n\
+destination are replaced, but also skipped files do not induce a failure.\n\
+'older' is the default operation when --update is specified, and results\n\
+in files being replaced if they're older than the corresponding source file.\n\
+"), stdout);
+}
+
 static inline void
 emit_backup_suffix_note (void)
 {
diff --git a/tests/mv/update.sh b/tests/mv/update.sh
index d3ec6120c..ab7309f06 100755
--- a/tests/mv/update.sh
+++ b/tests/mv/update.sh
@@ -19,11 +19,13 @@
 . "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
 print_ver_ cp mv
 
-echo old > old || framework_failure_
-touch -d yesterday old || framework_failure_
-echo new > new || framework_failure_
-
+test_reset() {
+  echo old > old || framework_failure_
+  touch -d yesterday old || framework_failure_
+  echo new > new || framework_failure_
+}
 
+test_reset
 for interactive in '' -i; do
   for cp_or_mv in cp mv; do
     # This is a no-op, with no prompt.
@@ -36,19 +38,32 @@ for interactive in '' -i; do
   done
 done
 
-# This will actually perform the rename.
-mv --update new old || fail=1
-test -f new && fail=1
-case "$(cat old)" in new) ;; *) fail=1 ;; esac
+# These should perform the rename / copy
+for update_option in '--update' '--update=older' '--update=all' \
+ '--update=none --update=all'; do
+  test_reset
+  mv $update_option new old || fail=1
+  test -f new && fail=1
+  case "$(cat old)" in new) ;; *) fail=1 ;; esac
+
+  test_reset
+  cp $update_option new old || fail=1
+  case "$(cat old)" in new) ;; *) fail=1 ;; esac
+  case "$(cat new)" in new) ;; *) fail=1 ;; esac
+done
 
-# Restore initial conditions.
-echo old > old || framework_failure_
-touch -d yesterday old || fail=1
-echo new > new || framework_failure_
+# These should not perform the rename / copy
+for update_option in '--update=none' \
+ '--update=all --update=none'; do
+  test_reset
+  mv $update_option new old || fail=1
+  case "$(cat new)" in new) ;; *) fail=1 ;; esac
+  case "$(cat old)" in old) ;; *) fail=1 ;; esac
 
-# This will actually perform the copy.
-cp --update new old || fail=1
-case "$(cat old)" in new) ;; *) fail=1 ;; esac
-case "$(cat new)" in new) ;; *) fail=1 ;; esac
+  test_reset
+  cp $update_option new old || fail=1
+  case "$(cat new)" in new) ;; *) fail=1 ;; esac
+  case "$(cat old)" in old) ;; *) fail=1 ;; esac
+done
 
 Exit $fail
-- 
2.26.2