cp -i --update old new emits pointless prompt (same for mv -i -u)

[Jim Meyering]

When "cp -i --update old new" would do nothing because "new" is
newer than "old", cp would nonetheless prompt for whether it is
ok to overwrite "new".  Then, regardless of the response (because
of the --update option), cp would do nothing.

The following patch eliminates the unnecessary prompt in that case.

diff --git a/ChangeLog b/ChangeLog
index 9219159..3e4bc42 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2007-07-18  Jim Meyering  <address@hidden>
+
+       "cp -i --update older newer" no longer prompts; same for mv
+       * src/copy.c (copy_internal): Perform "update" check before the
+       possible interactive prompt.  Reported by zeno_AT_biyg_DOT_org
+       in <http://bugzilla.redhat.com/248591>
+       * tests/mv/update: Add tests for the above.
+       * NEWS: Mention the bug fix.
+
 2007-07-15  Jim Meyering  <address@hidden>

        ls --color: Don't stat symlinks when neither ORPHAN nor MISSING
diff --git a/NEWS b/NEWS
index 89c20bb..423cf49 100644
--- a/NEWS
+++ b/NEWS
@@ -65,6 +65,8 @@ GNU coreutils NEWS                                    -*- 
outline -*-
   doing nothing.  The behavior of 'cp' is now better documented when
   the destination is a symlink.

+  "cp -i --update older newer" no longer prompts; same for mv
+
   cut now diagnoses a range starting with zero (e.g., -f 0-2) as invalid;
   before, it would treat it as if it started with 1 (-f 1-2).

diff --git a/src/copy.c b/src/copy.c
index b7bf73b..0e549d2 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1210,6 +1210,30 @@ copy_internal (char const *src_name, char const 
*dst_name,
              return false;
            }

+         if (!S_ISDIR (src_mode) && x->update)
+           {
+             /* When preserving time stamps (but not moving within a file
+                system), don't worry if the destination time stamp is
+                less than the source merely because of time stamp
+                truncation.  */
+             int options = ((x->preserve_timestamps
+                             && ! (x->move_mode
+                                   && dst_sb.st_dev == src_sb.st_dev))
+                            ? UTIMECMP_TRUNCATE_SOURCE
+                            : 0);
+
+             if (0 <= utimecmp (dst_name, &dst_sb, &src_sb, options))
+               {
+                 /* We're using --update and the destination is not older
+                    than the source, so do not copy or move.  Pretend the
+                    rename succeeded, so the caller (if it's mv) doesn't
+                    end up removing the source file.  */
+                 if (rename_succeeded)
+                   *rename_succeeded = true;
+                 return true;
+               }
+           }
+
          /* 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
@@ -1302,30 +1326,6 @@ copy_internal (char const *src_name, char const 
*dst_name,
                      return false;
                    }
                }
-
-             if (x->update)
-               {
-                 /* When preserving time stamps (but not moving within a file
-                    system), don't worry if the destination time stamp is
-                    less than the source merely because of time stamp
-                    truncation.  */
-                 int options = ((x->preserve_timestamps
-                                 && ! (x->move_mode
-                                       && dst_sb.st_dev == src_sb.st_dev))
-                                ? UTIMECMP_TRUNCATE_SOURCE
-                                : 0);
-
-                 if (0 <= utimecmp (dst_name, &dst_sb, &src_sb, options))
-                   {
-                     /* We're using --update and the destination is not older
-                        than the source, so do not copy or move.  Pretend the
-                        rename succeeded, so the caller (if it's mv) doesn't
-                        end up removing the source file.  */
-                     if (rename_succeeded)
-                       *rename_succeeded = true;
-                     return true;
-                   }
-               }
            }

          if (x->move_mode)
diff --git a/tests/mv/update b/tests/mv/update
index 0c06024..6c3d149 100755
--- a/tests/mv/update
+++ b/tests/mv/update
@@ -1,7 +1,7 @@
 #!/bin/sh
 # make sure --update works as advertised

-# Copyright (C) 2001, 2004, 2006 Free Software Foundation, Inc.
+# Copyright (C) 2001, 2004, 2006-2007 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
@@ -46,11 +46,16 @@ fi

 fail=0

-for cp_or_mv in cp mv; do
-  # This is a no-op.
-  $cp_or_mv --update old new || fail=1
-  case "`cat new`" in new) ;; *) fail=1 ;; esac
-  case "`cat old`" in old) ;; *) fail=1 ;; esac
+for interactive in '' -i; do
+  for cp_or_mv in cp mv; 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
+    test -s out && fail=1
+    case "`cat new`" in new) ;; *) fail=1 ;; esac
+    case "`cat old`" in old) ;; *) fail=1 ;; esac
+  done
 done

 # This will actually perform the rename.
--
1.5.3.rc1.16.g9d6f