From 7a69df88999bedd8e9fccf9f3dfa9ac6907fab66 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 31 Jan 2023 08:46:21 -0800
Subject: [PATCH] cp,ln,mv: when skipping exit with nonzero status

* NEWS, doc/coreutils.texi: Document this.
* src/copy.c (copy_internal):
* src/ln.c (do_link): Return false when skipping action due to
--interactive or --no-clobber.
* tests/cp/cp-i.sh, tests/cp/preserve-link.sh:
* tests/cp/slink-2-slink.sh, tests/mv/i-1.pl, tests/mv/i-5.sh:
* tests/mv/mv-n.sh, tests/mv/update.sh:
Adjust expectations of exit status to match revised behavior.
---
 NEWS                      |  5 +++++
 doc/coreutils.texi        | 24 +++++++++++++-----------
 src/copy.c                |  6 +++---
 src/ln.c                  |  2 +-
 tests/cp/cp-i.sh          | 10 +++++-----
 tests/cp/preserve-link.sh |  2 +-
 tests/cp/slink-2-slink.sh |  4 ++--
 tests/mv/i-1.pl           |  2 +-
 tests/mv/i-5.sh           |  2 +-
 tests/mv/mv-n.sh          |  8 ++++----
 tests/mv/update.sh        |  3 ++-
 11 files changed, 38 insertions(+), 30 deletions(-)

diff --git a/NEWS b/NEWS
index d714b8f3b..c2d3a42ec 100644
--- a/NEWS
+++ b/NEWS
@@ -69,6 +69,11 @@ GNU coreutils NEWS                                    -*- outline -*-
   'cp --reflink=always A B' no longer leaves behind a newly created
   empty file B merely because copy-on-write clones are not supported.
 
+  'cp -n' and 'mv -n' now exit with nonzero status if they skip their
+  action because the destination exists, and likewise for 'cp -i',
+  'ln -i', and 'mv -i' when the user declines.  (POSIX specifies this
+  for 'cp -i' and 'mv -i'.)
+
   cp, mv, and install again read in multiples of the reported block size,
   to support unusual devices that may have this constraint.
   [behavior inadvertently changed in coreutils-7.2]
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index c7494ba47..da73503ed 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -8924,7 +8924,8 @@ via recursive traversal.
 @opindex -i
 @opindex --interactive
 When copying a file other than a directory, prompt whether to
-overwrite an existing destination file.  The @option{-i} option overrides
+overwrite an existing destination file, and fail if the response
+is not affirmative.  The @option{-i} option overrides
 a previous @option{-n} option.
 
 @item -l
@@ -8946,7 +8947,7 @@ a regular file in the destination tree.
 @itemx --no-clobber
 @opindex -n
 @opindex --no-clobber
-Do not overwrite an existing file; silently do nothing instead.
+Do not overwrite an existing file; silently fail instead.
 This option overrides a previous
 @option{-i} option.  This option is mutually exclusive with @option{-b} or
 @option{--backup} option.
@@ -9190,7 +9191,8 @@ results in an error message on systems that do not support symbolic links.
 @opindex --update
 @cindex newer files, copying only
 Do not copy a non-directory that has an existing destination with the
-same or newer modification timestamp.  If timestamps are being preserved,
+same or newer modification timestamp; silently fail instead.
+If timestamps are being preserved,
 the comparison is to the source timestamp truncated to the
 resolutions of the destination file system and of the system calls
 used to update timestamps; this avoids duplicate work if several
@@ -10098,8 +10100,7 @@ options, only the final one takes effect.
 @opindex --interactive
 @cindex prompts, forcing
 Prompt whether to overwrite each existing destination file, regardless
-of its permissions.
-If the response is not affirmative, the file is skipped.
+of its permissions, and fail if the response is not affirmative.
 @mvOptsIfn
 
 @item -n
@@ -10107,7 +10108,7 @@ If the response is not affirmative, the file is skipped.
 @opindex -n
 @opindex --no-clobber
 @cindex prompts, omitting
-Do not overwrite an existing file; silently do nothing instead.
+Do not overwrite an existing file; silently fail instead.
 @mvOptsIfn
 This option is mutually exclusive with @option{-b} or @option{--backup} option.
 
@@ -10123,7 +10124,7 @@ fail with a diagnostic instead of copying and then removing the file.
 @opindex --update
 @cindex newer files, moving only
 Do not move a non-directory that has an existing destination with the
-same or newer modification timestamp.
+same or newer modification timestamp; silently fail instead.
 If the move is across file system boundaries, the comparison is to the
 source timestamp truncated to the resolutions of the destination file
 system and of the system calls used to update timestamps; this avoids
@@ -10216,7 +10217,7 @@ Ignore any previous @option{--interactive} (@option{-i}) option.
 @item -i
 @opindex -i
 Prompt whether to remove each file.
-If the response is not affirmative, the file is skipped.
+If the response is not affirmative, silently skip the file without failing.
 Ignore any previous @option{--force} (@option{-f}) option.
 Equivalent to @option{--interactive=always}.
 
@@ -10249,7 +10250,7 @@ removal is requested.  Equivalent to @option{-I}.
 @item --one-file-system
 @opindex --one-file-system
 @cindex one file system, restricting @command{rm} to
-When removing a hierarchy recursively, skip any directory that is on a
+When removing a hierarchy recursively, do not remove any directory that is on a
 file system different from that of the corresponding command line argument.
 @cindex bind mount
 This option is useful when removing a build ``chroot'' hierarchy,
@@ -10260,7 +10261,7 @@ unmount @file{/home}.  Then, when you use @command{rm -rf} to remove
 your normally throw-away chroot, that command will remove everything
 under @file{/home}, too.
 Use the @option{--one-file-system} option, and it will
-warn about and skip directories on other file systems.
+diagnose and skip directories on other file systems.
 Of course, this will not save your @file{/home} if it and your
 chroot happen to be on the same file system.
 See also @option{--preserve-root=all} to protect command line arguments
@@ -10802,7 +10803,8 @@ Remove existing destination files.
 @opindex -i
 @opindex --interactive
 @cindex prompting, and @command{ln}
-Prompt whether to remove existing destination files.
+Prompt whether to remove existing destination files,
+and fail if the response is not affirmative.
 
 @item -L
 @itemx --logical
diff --git a/src/copy.c b/src/copy.c
index 99834434f..f236afd2e 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2197,7 +2197,7 @@ copy_internal (char const *src_name, char const *dst_name,
                         }
                     }
 
-                  return true;
+                  return false;
                 }
             }
 
@@ -2216,7 +2216,7 @@ 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 true;
+                  return false;
                 }
             }
           else
@@ -2226,7 +2226,7 @@ copy_internal (char const *src_name, char const *dst_name,
                       || (x->interactive == I_ASK_USER
                           && ! overwrite_ok (x, dst_name, dst_dirfd,
                                              dst_relname, &dst_sb))))
-                return true;
+                return false;
             }
 
           if (return_now)
diff --git a/src/ln.c b/src/ln.c
index 05d89990e..1c3307cac 100644
--- a/src/ln.c
+++ b/src/ln.c
@@ -281,7 +281,7 @@ do_link (char const *source, int destdir_fd, char const *dest_base,
                       if (!yesno ())
                         {
                           free (rel_source);
-                          return true;
+                          return false;
                         }
                     }
 
diff --git a/tests/cp/cp-i.sh b/tests/cp/cp-i.sh
index 6fbc31760..b137bc4a5 100755
--- a/tests/cp/cp-i.sh
+++ b/tests/cp/cp-i.sh
@@ -24,7 +24,7 @@ touch a/c || framework_failure_
 
 
 # coreutils 6.2 cp would neglect to prompt in this case.
-echo n | cp -iR a b 2>/dev/null || fail=1
+echo n | returns_ 1 cp -iR a b 2>/dev/null || fail=1
 
 # test miscellaneous combinations of -f -i -n parameters
 touch c d || framework_failure_
@@ -32,7 +32,7 @@ echo "'c' -> 'd'" > out_copy
 > out_empty
 
 # ask for overwrite, answer no
-echo n | cp -vi  c d 2>/dev/null > out1 || fail=1
+echo n | returns_ 1 cp -vi  c d 2>/dev/null > out1 || fail=1
 compare out1 out_empty || fail=1
 
 # ask for overwrite, answer yes
@@ -44,7 +44,7 @@ echo y | cp -vni c d 2>/dev/null > out3 || fail=1
 compare out3 out_copy  || fail=1
 
 # -n wins over -i
-echo y | cp -vin c d 2>/dev/null > out4 || fail=1
+echo y | returns_ 1 cp -vin c d 2>/dev/null > out4 || fail=1
 compare out4 out_empty || fail=1
 
 # ask for overwrite, answer yes
@@ -52,11 +52,11 @@ echo y | cp -vfi c d 2>/dev/null > out5 || fail=1
 compare out5 out_copy  || fail=1
 
 # do not ask, prevent from overwrite
-echo n | cp -vfn c d 2>/dev/null > out6 || fail=1
+echo n | returns_ 1 cp -vfn c d 2>/dev/null > out6 || fail=1
 compare out6 out_empty || fail=1
 
 # do not ask, prevent from overwrite
-echo n | cp -vnf c d 2>/dev/null > out7 || fail=1
+echo n | returns_ 1 cp -vnf c d 2>/dev/null > out7 || fail=1
 compare out7 out_empty || fail=1
 
 # options --backup and --no-clobber are mutually exclusive
diff --git a/tests/cp/preserve-link.sh b/tests/cp/preserve-link.sh
index eb83b0f2b..2adaffd44 100755
--- a/tests/cp/preserve-link.sh
+++ b/tests/cp/preserve-link.sh
@@ -81,7 +81,7 @@ for f in f linkm; do
 
   # Copy all the hard links across.  With cp from coreutils-8.12
   # and prior, it would sometimes mistakenly copy rather than link.
-  cp -au s t || fail=1
+  returns_ 1 cp -au s t || fail=1
 
   same_inode t/s/f t/s/linkm || fail=1
   same_inode t/s/f t/s/linke || fail=1
diff --git a/tests/cp/slink-2-slink.sh b/tests/cp/slink-2-slink.sh
index 00e3feeb3..ece8d393d 100755
--- a/tests/cp/slink-2-slink.sh
+++ b/tests/cp/slink-2-slink.sh
@@ -26,7 +26,7 @@ ln -s file b || framework_failure_
 ln -s no-such-file c || framework_failure_
 ln -s no-such-file d || framework_failure_
 
-cp --update --no-dereference a b || fail=1
-cp --update --no-dereference c d || fail=1
+returns_ 1 cp --update --no-dereference a b || fail=1
+returns_ 1 cp --update --no-dereference c d || fail=1
 
 Exit $fail
diff --git a/tests/mv/i-1.pl b/tests/mv/i-1.pl
index ac4d74640..f2ee0e9cc 100755
--- a/tests/mv/i-1.pl
+++ b/tests/mv/i-1.pl
@@ -32,7 +32,7 @@ my @Tests =
       {IN => {src => "a\n"}}, {IN => {dst => "b\n"}}, '<', {IN => "n\n"},
       {ERR => "mv: overwrite 'dst'? "},
       {POST => sub { -r 'src' or die "test $test_a failed\n"}},
-      {EXIT => 0},
+      {EXIT => 1},
      ],
     );
 
diff --git a/tests/mv/i-5.sh b/tests/mv/i-5.sh
index 66e9cf067..fabb27514 100755
--- a/tests/mv/i-5.sh
+++ b/tests/mv/i-5.sh
@@ -24,6 +24,6 @@ touch b || framework_failure_
 
 
 # coreutils 6.2 mv would neglect to prompt in this case.
-echo n | mv -i a b 2>/dev/null || fail=1
+echo n | returns_ 1 mv -i a b 2>/dev/null || fail=1
 
 Exit $fail
diff --git a/tests/mv/mv-n.sh b/tests/mv/mv-n.sh
index 49db26841..fbf571368 100755
--- a/tests/mv/mv-n.sh
+++ b/tests/mv/mv-n.sh
@@ -27,7 +27,7 @@ echo "renamed 'a' -> 'b'" > out_move
 
 # ask for overwrite, answer no
 touch a b || framework_failure_
-echo n | mv -vi a b 2>/dev/null > out1 || fail=1
+echo n | returns_ 1 mv -vi a b 2>/dev/null > out1 || fail=1
 compare out1 out_empty || fail=1
 
 # ask for overwrite, answer yes
@@ -37,17 +37,17 @@ compare out2 out_move || fail=1
 
 # -n wins (as the last option)
 touch a b || framework_failure_
-echo y | mv -vin a b 2>/dev/null > out3 || fail=1
+echo y | returns_ 1 mv -vin a b 2>/dev/null > out3 || fail=1
 compare out3 out_empty || fail=1
 
 # -n wins (as the last option)
 touch a b || framework_failure_
-echo y | mv -vfn a b 2>/dev/null > out4 || fail=1
+echo y | returns_ 1 mv -vfn a b 2>/dev/null > out4 || fail=1
 compare out4 out_empty || fail=1
 
 # -n wins (as the last option)
 touch a b || framework_failure_
-echo y | mv -vifn a b 2>/dev/null > out5 || fail=1
+echo y | returns_ 1 mv -vifn a b 2>/dev/null > out5 || fail=1
 compare out5 out_empty || fail=1
 
 # options --backup and --no-clobber are mutually exclusive
diff --git a/tests/mv/update.sh b/tests/mv/update.sh
index d3ec6120c..f71297c2b 100755
--- a/tests/mv/update.sh
+++ b/tests/mv/update.sh
@@ -29,7 +29,8 @@ for interactive in '' -i; do
     # This is a no-op, with no prompt.
     # With coreutils-6.9 and earlier, using --update with -i would
     # mistakenly elicit a prompt.
-    $cp_or_mv $interactive --update old new < /dev/null > out 2>&1 || fail=1
+    returns_ 1 $cp_or_mv $interactive --update old new </dev/null >out 2>&1 ||
+      fail=1
     compare /dev/null out || fail=1
     case "$(cat new)" in new) ;; *) fail=1 ;; esac
     case "$(cat old)" in old) ;; *) fail=1 ;; esac
-- 
2.37.2