cp -i and mv -i incompatibility with POSIX

[Paul Eggert]

Here's a patch that fixes an incompability of cp -i and mv -i with
POSIX.  POSIX requires a prompt fairly early on, so that the user can
bail out from an operation they know will fail.  But coreutils
sometimes looks ahead, sees that the operation will fail, and reports
an error message before prompting.

This incompatibility has been around for a while (at least since 5.97)
so it's not a recent regression.  Since it's not a big deal I thought
I'd like a 2nd opinion before installing it; perhaps it ought to wait
until after the next stable release.

2006-09-19  Paul Eggert  <address@hidden>

        * src/copy.c (copy_internal): With -i, prompt even if the source
        is a directory and the destination is not.  This is required by
        POSIX and gives the user a chance to bail out before failing.
        * tests/cp/Makefile.am (TESTS): Add cp-i.
        * tests/cp/cp-i: New file.
        * tests/mv/Makefile.am (TESTS): Add i-5.
        * tests/mv/i-5: New file.

Index: src/copy.c
===================================================================
RCS file: /fetish/cu/src/copy.c,v
retrieving revision 1.211
diff -p -u -r1.211 copy.c
--- src/copy.c  3 Sep 2006 02:53:16 -0000       1.211
+++ src/copy.c  19 Sep 2006 21:28:56 -0000
@@ -1036,31 +1036,52 @@ copy_internal (char const *src_name, cha
             that it is XSTAT'able.  */
          bool return_now;
          bool unlink_src;
-         bool ok = same_file_ok (src_name, &src_sb, dst_name, &dst_sb,
-                                 x, &return_now, &unlink_src);
-         if (unlink_src)
+
+         if (! same_file_ok (src_name, &src_sb, dst_name, &dst_sb,
+                             x, &return_now, &unlink_src))
+           {
+             error (0, 0, _("%s and %s are the same file"),
+                    quote_n (0, src_name), quote_n (1, dst_name));
+             return false;
+           }
+
+         /* When there is an existing destination file, we may end up
+            returning early, and hence not copying/moving the file.
+            This may be due to an interactive `negative' reply to the
+            prompt about the existing file.  It may also be due to the
+            use of the --reply=no option.
+
+            cp and mv treat -i and -f differently.  */
+         if (x->move_mode)
            {
-             if (!abandon_move (x, dst_name, &dst_sb)
-                 && unlink (src_name) != 0)
+             if (abandon_move (x, dst_name, &dst_sb)
+                 || (unlink_src && unlink (src_name) == 0))
+               {
+                 /* Pretend the rename succeeded, so the caller (mv)
+                    doesn't end up removing the source file.  */
+                 if (rename_succeeded)
+                   *rename_succeeded = true;
+                 return true;
+               }
+             if (unlink_src)
                {
                  error (0, errno, _("cannot remove %s"), quote (src_name));
                  return false;
                }
-             /* Tell the caller that there's no need to remove src_name.  */
-             if (rename_succeeded)
-               *rename_succeeded = true;
+           }
+         else
+           {
+             if (! S_ISDIR (src_mode)
+                 && (x->interactive == I_ALWAYS_NO
+                     || (x->interactive == I_ASK_USER
+                         && (overwrite_prompt (dst_name, &dst_sb), 1)
+                         && ! yesno ())))
+               return true;
            }
 
          if (return_now)
            return true;
 
-         if (! ok)
-           {
-             error (0, 0, _("%s and %s are the same file"),
-                    quote_n (0, src_name), quote_n (1, dst_name));
-             return false;
-           }
-
          if (!S_ISDIR (dst_sb.st_mode))
            {
              if (S_ISDIR (src_type))
@@ -1140,37 +1161,6 @@ copy_internal (char const *src_name, cha
                }
            }
 
-         /* When there is an existing destination file, we may end up
-            returning early, and hence not copying/moving the file.
-            This may be due to an interactive `negative' reply to the
-            prompt about the existing file.  It may also be due to the
-            use of the --reply=no option.  */
-         if (!S_ISDIR (src_type))
-           {
-             /* cp and mv treat -i and -f differently.  */
-             if (x->move_mode)
-               {
-                 if (abandon_move (x, dst_name, &dst_sb))
-                   {
-                     /* Pretend the rename succeeded, so the caller (mv)
-                        doesn't end up removing the source file.  */
-                     if (rename_succeeded)
-                       *rename_succeeded = true;
-                     return true;
-                   }
-               }
-             else
-               {
-                 if (x->interactive == I_ALWAYS_NO
-                     || (x->interactive == I_ASK_USER
-                         && (overwrite_prompt (dst_name, &dst_sb), 1)
-                         && ! yesno ()))
-                   {
-                     return true;
-                   }
-               }
-           }
-
          if (x->move_mode)
            {
              /* Don't allow user to move a directory onto a non-directory.  */
Index: tests/cp/Makefile.am
===================================================================
RCS file: /fetish/cu/tests/cp/Makefile.am,v
retrieving revision 1.38
diff -p -u -r1.38 Makefile.am
--- tests/cp/Makefile.am        8 Sep 2006 17:08:54 -0000       1.38
+++ tests/cp/Makefile.am        19 Sep 2006 21:28:56 -0000
@@ -29,7 +29,7 @@ TESTS = \
   preserve-2 r-vs-symlink link-preserve \
   backup-1 no-deref-link1 no-deref-link2 no-deref-link3 backup-is-src \
   same-file cp-mv-backup symlink-slash slink-2-slink fail-perm dir-slash \
-  perm cp-HL special-bits link dir-rm-dest cp-parents deref-slink \
+  perm cp-HL cp-i special-bits link dir-rm-dest cp-parents deref-slink \
   dir-vs-file into-self
 EXTRA_DIST = $(TESTS) trailing-slash
 TESTS_ENVIRONMENT = \
Index: tests/mv/Makefile.am
===================================================================
RCS file: /fetish/cu/tests/mv/Makefile.am,v
retrieving revision 1.51
diff -p -u -r1.51 Makefile.am
--- tests/mv/Makefile.am        26 Aug 2006 17:13:50 -0000      1.51
+++ tests/mv/Makefile.am        19 Sep 2006 21:28:56 -0000
@@ -36,7 +36,7 @@ TESTS = \
   perm-1 \
   i-link-no \
   part-fail \
-  dup-source childproof i-4 update i-2 mv-special-1 \
+  dup-source childproof i-4 i-5 update i-2 mv-special-1 \
   into-self into-self-2 into-self-3 into-self-4 \
   backup-is-src \
   i-1 hard-link-1 force partition-perm to-symlink dir-file diag \
--- /dev/null   2005-09-24 22:00:15.000000000 -0700
+++ tests/cp/cp-i       2006-09-19 14:00:24.000000000 -0700
@@ -0,0 +1,51 @@
+#!/bin/sh
+# Test whether cp -i prompts in the right place.
+
+# Copyright (C) 2006 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+# 02110-1301, USA.
+
+if test "$VERBOSE" = yes; then
+  set -x
+  cp --version
+fi
+
+. $srcdir/../envvar-check
+. $srcdir/../lang-default
+
+pwd=`pwd`
+t0=`echo "$0"|sed 's,.*/,,'`.tmp; tmp=$t0/$$
+trap 'status=$?; cd $pwd; chmod -R u+rwx $t0; rm -rf $t0 && exit $status' 0
+trap '(exit $?); exit $?' 1 2 13 15
+
+framework_failure=0
+mkdir -p $tmp || framework_failure=1
+cd $tmp || framework_failure=1
+
+mkdir -p a b/a/c || framework_failure=1
+touch a/c || framework_failure=1
+
+if test $framework_failure = 1; then
+  echo "$0: failure in testing framework" 1>&2
+  (exit 1); exit 1
+fi
+
+fail=0
+
+# coreutils 6.2 cp would neglect to prompt in this case.
+echo n | cp -iR a b 2>/dev/null || fail=1
+
+(exit $fail); exit $fail
--- /dev/null   2005-09-24 22:00:15.000000000 -0700
+++ tests/mv/i-5        2006-09-19 14:04:12.000000000 -0700
@@ -0,0 +1,51 @@
+#!/bin/sh
+# Make sure `mv -i dir file' prompts before failing.
+
+# Copyright (C) 2006 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+# 02110-1301, USA.
+
+if test "$VERBOSE" = yes; then
+  set -x
+  mv --version
+fi
+
+. $srcdir/../envvar-check
+. $srcdir/../lang-default
+
+pwd=`pwd`
+t0=`echo "$0"|sed 's,.*/,,'`.tmp; tmp=$t0/$$
+trap 'status=$?; cd $pwd; chmod -R u+rwx $t0; rm -rf $t0 && exit $status' 0
+trap '(exit $?); exit $?' 1 2 13 15
+
+framework_failure=0
+mkdir -p $tmp || framework_failure=1
+cd $tmp || framework_failure=1
+
+mkdir a || framework_failure=1
+touch b || framework_failure=1
+
+if test $framework_failure = 1; then
+  echo 'failure in testing framework'
+  exit 1
+fi
+
+fail=0
+
+# coreutils 6.2 mv would neglect to prompt in this case.
+echo n | mv -i a b 2>/dev/null || fail=1
+
+(exit $fail); exit $fail