On 2023-01-30 13:35, Pádraig Brady wrote:
This is a good observation.
Also the current "skip" functionality of coreutils cp -n
is already catered for with the --update option.
For consistency, there too the exit status should reflect whether the cp
action was done.
I installed the attached patch to implement this; comments welcome. In
the meantime I'm boldly closing the bug report.From 7a69df88999bedd8e9fccf9f3dfa9ac6907fab66 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@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
