Subject: Re: bug#70887: In coreutils 9.5, "cp" command does not honor "--interactive" option
From: Pádraig Brady <[email-masked]>
To: Paul Eggert <[email-masked]>, Robert Hill <[email-masked]>, [email-masked]
Date: Mon, 6 Jan 2025 16:04:25 +0000
List-Id: <debbugs-submit.debbugs.gnu.org>

On 12/05/2024 21:31, Pádraig Brady wrote:
> On 12/05/2024 16:06, Paul Eggert wrote:
>> On 2024-05-12 04:49, Pádraig Brady wrote:
>>
>>> @@ -1151,7 +1151,8 @@ main (int argc, char **argv)
>>>                    {
>>>                      /* Default cp operation.  */
>>>                      x.update = false;
>>> -                  x.interactive = I_UNSPECIFIED;
>>> +                  if (x.interactive != I_ASK_USER)
>>> +                    x.interactive = I_UNSPECIFIED;
>>>                    }
>>>                  else if (update_opt == UPDATE_NONE)
>>>                    {
>>> @@ -1166,7 +1167,8 @@ main (int argc, char **argv)
>>>                  else if (update_opt == UPDATE_OLDER)
>>>                    {
>>>                      x.update = true;
>>> -                  x.interactive = I_UNSPECIFIED;
>>> +                  if (x.interactive != I_ASK_USER)
>>> +                    x.interactive = I_UNSPECIFIED;
>>
>> Thanks for looking into this messy area. Here is a comment from another
>> pair of eyes.
>>
>> Could you elaborate a bit more about why these two bits of code change
>> x.interactive at all? That is, why doesn't update_opt simply affect
>> x.update? Why does update_opt bother to override a previous setting of
>> x.interactive to I_ALWAYS_YES, I_ALWAYS_NO, or I_ALWAYS_SKIP?
>>
>> Another way to put it: shouldn't x.update simply reflect the value of
>> the --update option, whereas x.interactive reflects reflects whether -f,
>> -i, -n are used? Although this would require changes to copy.c, it'd
>> make the code easier to follow.
> 
> I agree that some refactoring would be good here.
> At least x.update should be renamed to x.update_older.
> 
> As interactive selection, and file dates all relate
> to selecting which files to update, it's tempting to conflate the settings.
> However you're right that this introduces complexities when
> trying to avoid all inconsistencies. Currently for example:
>     $ cp -v -i --update=none new old  # Won't prompt as expected
>     $ cp -v --update=none -i new old  # Unexpectedly ignores update option
> 
> So yes we should separate these things.
> 
>> Another way to put it: why should, for example, --update=all override a
>> previous -f or (partly) -n but not a previous -i?
> 
> Right -f is significant for mv here (for completeness -f for cp is a separate thing).
> I.e. we need to treat I_ALWAYS_YES specially in mv with the current scheme.
> 
> BTW -n is not overridden by any --update option currently,
> and this change effectively applies the same logic to -i now.

The attached patch set should address this.
Marking this as done.
I'll push the attached tomorrow.

cheers,
Pádraig

From 55728961377715701c1c83c7ae94e60dc404a37e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Mon, 6 Jan 2025 13:01:47 +0000
Subject: [PATCH 1/3] doc: clarify mv -f operation in texinfo

* doc/coreutils.texi (mv invocation): Be less ambiguous,
in that -f is significant for any replacement operation
on the destination, not just unlinking.
---
 doc/coreutils.texi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 9afec5271..585760741 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -10282,7 +10282,7 @@ The program accepts the following options.  Also see @ref{Common options}.
 @opindex -f
 @opindex --force
 @cindex prompts, omitting
-Do not prompt the user before removing a destination file.
+Do not prompt the user before replacing a destination file.
 @macro mvOptsIfn
 If you specify more than one of the @option{-i}, @option{-f}, @option{-n}
 options, only the final one takes effect.
-- 
2.47.1

From a6ab944e19684d0ccd0568fa8c06f71921c1e6c3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Sun, 12 May 2024 12:21:19 +0100
Subject: [PATCH 2/3] cp,mv: ensure -i,f are not overridden by -u

Since coreutils 9.3 we had --update={all,older} override -i.
In coreutils 9.5 this was expanded to -u
(to make it consistent with --update=older).

This patch reinstates things so that -i combines with -u instead.
I.e. have -i be protective, rather than selective (like -u).

The -f option of mv is similarly adjusted in this patch,
so now --update does not override any of -f,-i,-n.

* NEWS: Mention the bug fix.
* src/cp.c (main): Don't have -u disable prompting.
* src/mv.c (main): Likewise.
* tests/cp/cp-i.sh: Add a test case for -i.
* tests/mv/update.sh: Likewise.
* tests/mv/i-3.sh. Add a test case for -f.
Fixes https://bugs.gnu.org/70887
---
 NEWS               |  3 +++
 src/cp.c           | 10 +++++++---
 src/mv.c           | 20 ++++++++++++++++----
 tests/cp/cp-i.sh   | 19 +++++++++++++++++++
 tests/mv/i-3.sh    | 17 +++++++++++++----
 tests/mv/update.sh |  3 +++
 6 files changed, 61 insertions(+), 11 deletions(-)

diff --git a/NEWS b/NEWS
index 331a06358..3e0153610 100644
--- a/NEWS
+++ b/NEWS
@@ -8,6 +8,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   rejected as an invalid option.
   [bug introduced in coreutils-9.5]
 
+  cp,mv --update no longer overrides --interactive or --force.
+  [bug introduced in coreutils-9.3]
+
   ls and printf fix shell quoted output in the edge case of escaped
   first and last characters, and single quotes in the string.
   [bug introduced in coreutils-8.26]
diff --git a/src/cp.c b/src/cp.c
index 215f810bd..23c25a983 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -1063,7 +1063,9 @@ main (int argc, char **argv)
           break;
 
         case 'i':
-          x.interactive = I_ASK_USER;
+          /* -i overrides -n, but not --update={none,none-fail}.  */
+          if (no_clobber || x.interactive == I_UNSPECIFIED)
+            x.interactive = I_ASK_USER;
           break;
 
         case 'l':
@@ -1151,7 +1153,8 @@ main (int argc, char **argv)
                 {
                   /* Default cp operation.  */
                   x.update = false;
-                  x.interactive = I_UNSPECIFIED;
+                  if (x.interactive != I_ASK_USER)
+                    x.interactive = I_UNSPECIFIED;
                 }
               else if (update_opt == UPDATE_NONE)
                 {
@@ -1166,7 +1169,8 @@ main (int argc, char **argv)
               else if (update_opt == UPDATE_OLDER)
                 {
                   x.update = true;
-                  x.interactive = I_UNSPECIFIED;
+                  if (x.interactive != I_ASK_USER)
+                    x.interactive = I_UNSPECIFIED;
                 }
             }
           break;
diff --git a/src/mv.c b/src/mv.c
index 46893a25f..bbf1e6034 100644
--- a/src/mv.c
+++ b/src/mv.c
@@ -353,10 +353,18 @@ main (int argc, char **argv)
             version_control_string = optarg;
           break;
         case 'f':
-          x.interactive = I_ALWAYS_YES;
+          /* -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;
           break;
         case 'i':
-          x.interactive = I_ASK_USER;
+          /* -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;
           break;
         case 'n':
           x.interactive = I_ALWAYS_SKIP;
@@ -394,7 +402,9 @@ main (int argc, char **argv)
                 {
                   /* Default mv operation.  */
                   x.update = false;
-                  x.interactive = I_UNSPECIFIED;
+                  if (x.interactive != I_ASK_USER
+                      && x.interactive != I_ALWAYS_YES)
+                    x.interactive = I_UNSPECIFIED;
                 }
               else if (update_opt == UPDATE_NONE)
                 {
@@ -409,7 +419,9 @@ main (int argc, char **argv)
               else if (update_opt == UPDATE_OLDER)
                 {
                   x.update = true;
-                  x.interactive = I_UNSPECIFIED;
+                  if (x.interactive != I_ASK_USER
+                      && x.interactive != I_ALWAYS_YES)
+                    x.interactive = I_UNSPECIFIED;
                 }
             }
           break;
diff --git a/tests/cp/cp-i.sh b/tests/cp/cp-i.sh
index 08da4b31b..2fd5abea8 100755
--- a/tests/cp/cp-i.sh
+++ b/tests/cp/cp-i.sh
@@ -70,4 +70,23 @@ returns_ 1 cp -bn c d 2>/dev/null || fail=1
 returns_ 1 cp -b --update=none c d 2>/dev/null || fail=1
 returns_ 1 cp -b --update=none-fail c d 2>/dev/null || fail=1
 
+# Verify -i combines with -u,
+echo old > old || framework_failure_
+touch -d yesterday old || framework_failure_
+echo new > new || framework_failure_
+# coreutils 9.3 had --update={all,older} ignore -i
+echo n | returns_ 1 cp -vi --update=older new old 2>/dev/null >out8 || fail=1
+compare /dev/null out8 || fail=1
+echo n | returns_ 1 cp -vi --update=all new old 2>/dev/null >out8 || fail=1
+compare /dev/null out8 || fail=1
+# coreutils 9.5 also had -u ignore -i
+echo n | returns_ 1 cp -vi -u new old 2>/dev/null >out8 || fail=1
+compare /dev/null out8 || fail=1
+# Don't prompt as not updating
+cp -v -i --update=none 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 --update=none -i new old 2>/dev/null >out8 </dev/null || fail=1
+compare /dev/null out8 || fail=1
+
 Exit $fail
diff --git a/tests/mv/i-3.sh b/tests/mv/i-3.sh
index 90fb7cd21..2f70e9ed8 100755
--- a/tests/mv/i-3.sh
+++ b/tests/mv/i-3.sh
@@ -25,8 +25,8 @@ trap '' TTIN # Ignore SIGTTIN
 
 uname -s | grep 'BSD$' && skip_ 'known spurious failure on *BSD'
 
-touch f g h i || framework_failure_
-chmod 0 g i || framework_failure_
+touch f g h i j k || framework_failure_
+chmod 0 g i j k || framework_failure_

 ls /dev/stdin >/dev/null 2>&1 \
@@ -59,11 +59,20 @@ retry_delay_ check_overwrite_prompt .1 7 || { fail=1; cat out; }
 
 cleanup_
 
-mv -f h i > out 2>&1 || fail=1
+# Make sure there was no prompt with -f
+timeout 10 mv -f h i > out 2>&1 || fail=1
 test -f i || fail=1
 test -f h && fail=1
+case "$(cat out)" in
+  '') ;;
+  *) fail=1 ;;
+esac
 
-# Make sure there was no prompt.
+# Likewise make sure there was no prompt with -f -u
+# coreutils 9.3-9.5 mistakenly did prompt.
+timeout 10 mv -f --update=all j k > out 2>&1 || fail=1
+test -f k || fail=1
+test -f j && fail=1
 case "$(cat out)" in
   '') ;;
   *) fail=1 ;;
diff --git a/tests/mv/update.sh b/tests/mv/update.sh
index b0b4d4acb..cc4214724 100755
--- a/tests/mv/update.sh
+++ b/tests/mv/update.sh
@@ -38,6 +38,9 @@ for interactive in '' -i; do
   done
 done
 
+# This should prompt. coreutils 9.3-9.5 mistakenly did not
+echo n | returns_ 1 mv -vi -u new old >/dev/null 2>&1 || fail=1
+
 # These should accept all options
 for update_option in '--update' '--update=older' '--update=all' \
  '--update=none' '--update=none-fail'; do
-- 
2.47.1

From 1658e470a1d7e421a1ace74352183fa91edfaa88 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Mon, 6 Jan 2025 15:48:02 +0000
Subject: [PATCH 3/3] cp,mv: uncouple --update and -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.
---
 src/copy.c       | 35 ++++++++++++++--------------
 src/copy.h       | 16 ++++++-------
 src/cp.c         | 50 +++++++++-------------------------------
 src/install.c    |  2 +-
 src/mv.c         | 60 ++++++++++--------------------------------------
 tests/cp/cp-i.sh |  3 +++
 6 files changed, 51 insertions(+), 115 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index ad61de256..7ffb998f6 100644
--- 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 9da99826a..c619c8ea6 100644
--- 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 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 23c25a983..a0ec06714 100644
--- 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 5bbf6d5af..b3b26abdb 100644
--- 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 bbf1e6034..cf1ac56e8 100644
--- 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 2fd5abea8..2d673a2b0 100755
--- 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
-- 
2.47.1