cp,mv: decouple --update from -f,-i,-n options

* src/copy.h: Change update member from bool to enum.
* src/copy.c: s/interactive == I_ALWAYS_NO/update == UPDATE_NONE_FAIL/;
              s/interactive == I_ALWAYS_SKIP/update == UPDATE_NONE/;
              s/update/update == UPDATE_OLDER/;
* src/install.c: Init with UPDATE_ALL, rather than false.
* src/cp.c: Likewise.  Simply parse -f,-i,-n to x.interactive,
and parse --update to x.update.
* src/mv.c: Likewise.
* tests/cp/cp-i.sh: Add a test case where -n --update -i
honors the --update option, which would previously have been
ignored due to the preceding -n.

diff --git a/src/copy.c b/src/copy.c
index ad61de256b99edb268eb9b003873941fb0a679db..7ffb998f6c480d19a382de486ae46db0fe2e45e7 100644 (file)
--- a/src/copy.c
+++ b/src/copy.c
@@ -2060,8 +2060,8 @@ abandon_move (const struct cp_options *x,
               struct stat const *dst_sb)
 {
   affirm (x->move_mode);
-  return (x->interactive == I_ALWAYS_NO
-          || x->interactive == I_ALWAYS_SKIP
+  return (x->update == UPDATE_NONE
+          || x->update == UPDATE_NONE_FAIL
           || ((x->interactive == I_ASK_USER
                || (x->interactive == I_UNSPECIFIED
                    && x->stdin_tty
@@ -2231,7 +2231,7 @@ 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 && x->interactive != I_ALWAYS_SKIP))
+        || (x->update != UPDATE_NONE && x->update != UPDATE_NONE_FAIL))
     {
       char const *name = rename_errno == 0 ? dst_name : src_name;
       int dirfd = rename_errno == 0 ? dst_dirfd : AT_FDCWD;
@@ -2293,14 +2293,12 @@ copy_internal (char const *src_name, char const *dst_name,
     {
       /* 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
-         doing this when rename_errno == EEXIST and X->interactive is
-         I_ALWAYS_NO or I_ALWAYS_SKIP, something that can happen only
-         with mv in which case x->update must be false which means
-         that even if !NEW_DST the move will be abandoned without
-         looking at DST_SB.  */
+         doing this when rename_errno == EEXIST and not updating,
+         which means that even if !NEW_DST the move will be abandoned
+         without looking at DST_SB.  */
       if (! (rename_errno == EEXIST
-             && (x->interactive == I_ALWAYS_NO
-                 || x->interactive == I_ALWAYS_SKIP)))
+             && (x->update == UPDATE_NONE
+                 || x->update == UPDATE_NONE_FAIL)))
         {
           /* Regular files can be created by writing through symbolic
              links, but other files cannot.  So use stat on the
@@ -2345,7 +2343,7 @@ copy_internal (char const *src_name, char const *dst_name,
           bool return_val = true;
           bool skipped = false;
 
-          if ((x->interactive != I_ALWAYS_NO && x->interactive != I_ALWAYS_SKIP)
+          if ((x->update != UPDATE_NONE && x->update != UPDATE_NONE_FAIL)
               && ! same_file_ok (src_name, &src_sb, dst_dirfd, drelname,
                                  &dst_sb, x, &return_now))
             {
@@ -2354,7 +2352,7 @@ copy_internal (char const *src_name, char const *dst_name,
               return false;
             }
 
-          if (x->update && !S_ISDIR (src_mode))
+          if (x->update == UPDATE_OLDER && !S_ISDIR (src_mode))
             {
               /* When preserving timestamps (but not moving within a file
                  system), don't worry if the destination timestamp is
@@ -2418,27 +2416,27 @@ copy_internal (char const *src_name, char const *dst_name,
                     *rename_succeeded = true;
 
                   skipped = true;
-                  return_val = x->interactive == I_ALWAYS_SKIP;
+                  return_val = x->update == UPDATE_NONE;
                 }
             }
           else
             {
               if (! S_ISDIR (src_mode)
-                  && (x->interactive == I_ALWAYS_NO
-                      || x->interactive == I_ALWAYS_SKIP
+                  && (x->update == UPDATE_NONE
+                      || x->update == UPDATE_NONE_FAIL
                       || (x->interactive == I_ASK_USER
                           && ! overwrite_ok (x, dst_name, dst_dirfd,
                                              dst_relname, &dst_sb))))
                 {
                   skipped = true;
-                  return_val = x->interactive == I_ALWAYS_SKIP;
+                  return_val = x->update == UPDATE_NONE;
                 }
             }
 
 skip:
           if (skipped)
             {
-              if (x->interactive == I_ALWAYS_NO)
+              if (x->update == UPDATE_NONE_FAIL)
                 error (0, 0, _("not replacing %s"), quoteaf (dst_name));
               else if (x->debug)
                 printf (_("skipped %s\n"), quoteaf (dst_name));
@@ -3110,7 +3108,8 @@ skip:
 
       int symlink_err = force_symlinkat (src_link_val, dst_dirfd, dst_relname,
                                          x->unlink_dest_after_failed_open, -1);
-      if (0 < symlink_err && x->update && !new_dst && S_ISLNK (dst_sb.st_mode)
+      if (0 < symlink_err && x->update == UPDATE_OLDER
+          && !new_dst && S_ISLNK (dst_sb.st_mode)
           && dst_sb.st_size == strlen (src_link_val))
         {
           /* See if the destination is already the desired symlink.

diff --git a/src/copy.h b/src/copy.h
index 9da99826a98a4662b37215019bdbf739d618efaf..35068ea5da75711e613d31ff606f91e98ca6fc02 100644 (file)
--- a/src/copy.h
+++ b/src/copy.h
@@ -63,7 +63,7 @@ enum Update_type
   /* Always update..  */
   UPDATE_ALL,
 
-  /* Update if dest older.  */
+  /* Update if (nondirectory) dest has older mtime.  */
   UPDATE_OLDER,
 
   /* Leave existing files.  */
@@ -76,11 +76,10 @@ enum Update_type
 /* This type is used to help mv (via copy.c) distinguish these cases.  */
 enum Interactive
 {
-  I_ALWAYS_YES = 1,
-  I_ALWAYS_NO,       /* Skip and fail.   */
-  I_ALWAYS_SKIP,     /* Skip and ignore. */
-  I_ASK_USER,
-  I_UNSPECIFIED
+  I_UNSPECIFIED,
+  I_ALWAYS_YES,      /* -f.  */
+  I_ALWAYS_SKIP,     /* -n (Skip and ignore).  */
+  I_ASK_USER,        /* -i.  */
 };
 
 /* How to handle symbolic links.  */
@@ -256,9 +255,8 @@ struct cp_options
      Create destination directories as usual. */
   bool symbolic_link;
 
-  /* If true, do not copy a nondirectory that has an existing destination
-     with the same or newer modification time. */
-  bool update;
+  /* Control if destination files are replaced.  */
+  enum Update_type update;
 
   /* If true, display the names of the files before copying them. */
   bool verbose;

diff --git a/src/cp.c b/src/cp.c
index 23c25a9838ac3ada42db302ea705acb3d140f0ee..a0ec067147221d827085c32a0e4b2fb81caccd96 100644 (file)
--- a/src/cp.c
+++ b/src/cp.c
@@ -863,7 +863,7 @@ cp_option_init (struct cp_options *x)
   /* Not used.  */
   x->stdin_tty = false;
 
-  x->update = false;
+  x->update = UPDATE_ALL;
   x->verbose = false;
   x->keep_directory_symlink = false;
 
@@ -984,7 +984,6 @@ main (int argc, char **argv)
   char *target_directory = nullptr;
   bool no_target_directory = false;
   char const *scontext = nullptr;
-  bool no_clobber = false;
 
   initialize_main (&argc, &argv);
   set_program_name (argv[0]);
@@ -1063,9 +1062,7 @@ main (int argc, char **argv)
           break;
 
         case 'i':
-          /* -i overrides -n, but not --update={none,none-fail}.  */
-          if (no_clobber || x.interactive == I_UNSPECIFIED)
-            x.interactive = I_ASK_USER;
+          x.interactive = I_ASK_USER;
           break;
 
         case 'l':
@@ -1078,8 +1075,6 @@ main (int argc, char **argv)
 
         case 'n':
           x.interactive = I_ALWAYS_SKIP;
-          no_clobber = true;
-          x.update = false;
           break;
 
         case 'P':
@@ -1143,36 +1138,10 @@ main (int argc, char **argv)
           break;
 
         case 'u':
-          if (! no_clobber) /* -n > -u */
-            {
-              enum Update_type update_opt = UPDATE_OLDER;
-              if (optarg)
-                update_opt = XARGMATCH ("--update", optarg,
-                                        update_type_string, update_type);
-              if (update_opt == UPDATE_ALL)
-                {
-                  /* Default cp operation.  */
-                  x.update = false;
-                  if (x.interactive != I_ASK_USER)
-                    x.interactive = I_UNSPECIFIED;
-                }
-              else if (update_opt == UPDATE_NONE)
-                {
-                  x.update = false;
-                  x.interactive = I_ALWAYS_SKIP;
-                }
-              else if (update_opt == UPDATE_NONE_FAIL)
-                {
-                  x.update = false;
-                  x.interactive = I_ALWAYS_NO;
-                }
-              else if (update_opt == UPDATE_OLDER)
-                {
-                  x.update = true;
-                  if (x.interactive != I_ASK_USER)
-                    x.interactive = I_UNSPECIFIED;
-                }
-            }
+          x.update = UPDATE_OLDER;
+          if (optarg)
+            x.update = XARGMATCH ("--update", optarg,
+                                  update_type_string, update_type);
           break;
 
         case 'v':
@@ -1236,9 +1205,12 @@ main (int argc, char **argv)
       usage (EXIT_FAILURE);
     }
 
+  if (x.interactive == I_ALWAYS_SKIP)
+    x.update = UPDATE_NONE;
+
   if (make_backups
-      && (x.interactive == I_ALWAYS_SKIP
-          || x.interactive == I_ALWAYS_NO))
+      && (x.update == UPDATE_NONE
+          || x.update == UPDATE_NONE_FAIL))
     {
       error (0, 0,
              _("--backup is mutually exclusive with -n or --update=none-fail"));

diff --git a/src/install.c b/src/install.c
index 5bbf6d5afd80322618be3a0be03f4f702e0778ef..b3b26abdb40dfdc2c71a0bfd9243341003dd775a 100644 (file)
--- a/src/install.c
+++ b/src/install.c
@@ -290,7 +290,7 @@ cp_option_init (struct cp_options *x)
   x->stdin_tty = false;
 
   x->open_dangling_dest_symlink = false;
-  x->update = false;
+  x->update = UPDATE_ALL;
   x->require_preserve_context = false;  /* Not used by install currently.  */
   x->preserve_security_context = false; /* Whether to copy context from src.  */
   x->set_security_context = nullptr; /* Whether to set sys default context.  */

diff --git a/src/mv.c b/src/mv.c
index bbf1e6034874d42f85ca92e5c7dd6253883a1b98..cf1ac56e877b363b0a3767583169a3f2661644e5 100644 (file)
--- a/src/mv.c
+++ b/src/mv.c
@@ -154,7 +154,7 @@ cp_option_init (struct cp_options *x)
   x->stdin_tty = isatty (STDIN_FILENO);
 
   x->open_dangling_dest_symlink = false;
-  x->update = false;
+  x->update = UPDATE_ALL;
   x->verbose = false;
   x->dest_info = nullptr;
   x->src_info = nullptr;
@@ -327,7 +327,6 @@ main (int argc, char **argv)
   int n_files;
   char **file;
   bool selinux_enabled = (0 < is_selinux_enabled ());
-  bool no_clobber = false;
 
   initialize_main (&argc, &argv);
   set_program_name (argv[0]);
@@ -353,23 +352,13 @@ main (int argc, char **argv)
             version_control_string = optarg;
           break;
         case 'f':
-          /* -f overrides -n, or -i, but not --update={none,none-fail}.  */
-          if (no_clobber
-              || x.interactive == I_ASK_USER
-              || x.interactive == I_UNSPECIFIED)
-            x.interactive = I_ALWAYS_YES;
+          x.interactive = I_ALWAYS_YES;
           break;
         case 'i':
-          /* -i overrides -n, or -f, but not --update={none,none-fail}.  */
-          if (no_clobber
-              || x.interactive == I_ALWAYS_YES
-              || x.interactive == I_UNSPECIFIED)
-            x.interactive = I_ASK_USER;
+          x.interactive = I_ASK_USER;
           break;
         case 'n':
           x.interactive = I_ALWAYS_SKIP;
-          no_clobber = true;
-          x.update = false;
           break;
         case DEBUG_OPTION:
           x.debug = x.verbose = true;
@@ -392,38 +381,10 @@ main (int argc, char **argv)
           no_target_directory = true;
           break;
         case 'u':
-          if (! no_clobber)
-            {
-              enum Update_type update_opt = UPDATE_OLDER;
-              if (optarg)
-                update_opt = XARGMATCH ("--update", optarg,
-                                        update_type_string, update_type);
-              if (update_opt == UPDATE_ALL)
-                {
-                  /* Default mv operation.  */
-                  x.update = false;
-                  if (x.interactive != I_ASK_USER
-                      && x.interactive != I_ALWAYS_YES)
-                    x.interactive = I_UNSPECIFIED;
-                }
-              else if (update_opt == UPDATE_NONE)
-                {
-                  x.update = false;
-                  x.interactive = I_ALWAYS_SKIP;
-                }
-              else if (update_opt == UPDATE_NONE_FAIL)
-                {
-                  x.update = false;
-                  x.interactive = I_ALWAYS_NO;
-                }
-              else if (update_opt == UPDATE_OLDER)
-                {
-                  x.update = true;
-                  if (x.interactive != I_ASK_USER
-                      && x.interactive != I_ALWAYS_YES)
-                    x.interactive = I_UNSPECIFIED;
-                }
-            }
+          x.update = UPDATE_OLDER;
+          if (optarg)
+            x.update = XARGMATCH ("--update", optarg,
+                                  update_type_string, update_type);
           break;
         case 'v':
           x.verbose = true;
@@ -533,10 +494,13 @@ main (int argc, char **argv)
     for (int i = 0; i < n_files; i++)
       strip_trailing_slashes (file[i]);
 
+  if (x.interactive == I_ALWAYS_SKIP)
+    x.update = UPDATE_NONE;
+
   if (make_backups
       && (x.exchange
-          || x.interactive == I_ALWAYS_SKIP
-          || x.interactive == I_ALWAYS_NO))
+          || x.update == UPDATE_NONE
+          || x.update == UPDATE_NONE_FAIL))
     {
       error (0, 0,
              _("cannot combine --backup with "

diff --git a/tests/cp/cp-i.sh b/tests/cp/cp-i.sh
index 2fd5abea8fd743bcc2b52bfe65e081f662a2dd2a..2d673a2b09f2f01a7442345c3426bdf80ae4f24c 100755 (executable)
--- a/tests/cp/cp-i.sh
+++ b/tests/cp/cp-i.sh
@@ -88,5 +88,8 @@ compare /dev/null out8 || fail=1
 # Likewise, but coreutils 9.3 - 9.5 incorrectly ignored the update option
 cp -v --update=none -i new old 2>/dev/null >out8 </dev/null || fail=1
 compare /dev/null out8 || fail=1
+# Likewise, but coreutils 9.3 - 9.5 incorrectly ignored the update option
+cp -v -n --update=none -i new old 2>/dev/null >out8 </dev/null || fail=1
+compare /dev/null out8 || fail=1
 
 Exit $fail