copy: go back to failing 'cp --backup a~ a'

Suggested by Kamil Dudka in:
http://lists.gnu.org/archive/html/coreutils/2017-07/msg00072.html
* NEWS: Document the changed nature of the fix.
* doc/coreutils.texi, tests/cp/backup-is-src.sh:
* tests/mv/backup-is-src.sh: Revert previous change.
* src/copy.c (source_is_dst_backup): New function.
(copy_internal): Use it.  Fail instead of falling back on numbered
backups when it looks like the backup will overwrite the source.
Although this reintroduces a race, it's more compatible with
previous behavior.

NEWS

@@ -15,10 +15,10 @@ GNU coreutils NEWS                                    -*- outline -*-
   later, the races are still present on other platforms.
   [the bug dates back to the initial implementation]
 
-  cp, install, ln, and mv now use a numbered instead of a simple
-  backup if copying a backup file to what might be its original.
+  cp, install, ln, and mv no longer lose data when asked to copy a
+  backup file to its original via a differently-spelled file name.
   E.g., 'rm -f a a~; : > a; echo data > a~; cp --backup=simple a~ ./a'
-  now makes a numbered backup file instead of losing the data.
+  now fails instead of losing the data.
   [the bug dates back to the initial implementation]
 
   cp, install, ln, and mv now ignore nonsensical backup suffixes.

doc/coreutils.texi

@@ -865,19 +865,17 @@ Never make backups.
 @opindex numbered @r{backup method}
 Always make numbered backups.
 
-@item simple
-@itemx never
-@opindex simple @r{backup method}
-Make simple backups, except make a numbered backup if the source might
-be a simple backup of the destination; this avoids losing source data.
-Please note @samp{never} is not to be confused with @samp{none}.
-
 @item existing
 @itemx nil
 @opindex existing @r{backup method}
-Make numbered backups of files that already have them, or if the source
-might be a simple backup of the destination.
-Otherwise, make simple backups.
+Make numbered backups of files that already have them, simple backups
+of the others.
+
+@item simple
+@itemx never
+@opindex simple @r{backup method}
+Always make simple backups.  Please note @samp{never} is not to be
+confused with @samp{none}.
 
 @end table
 

src/copy.c

@@ -1806,6 +1806,29 @@ should_dereference (const struct cp_options *x, bool command_line_arg)
              && command_line_arg);
 }
 
+/* Return true if the source file with basename SRCBASE and status SRC_ST
+   is likely to be the simple backup file for DST_NAME.  */
+static bool
+source_is_dst_backup (char const *srcbase, struct stat const *src_st,
+                      char const *dst_name)
+{
+  size_t srcbaselen = strlen (srcbase);
+  char const *dstbase = last_component (dst_name);
+  size_t dstbaselen = strlen (dstbase);
+  size_t suffixlen = strlen (simple_backup_suffix);
+  if (! (srcbaselen == dstbaselen + suffixlen
+         && memcmp (srcbase, dstbase, dstbaselen) == 0
+         && STREQ (srcbase + dstbaselen, simple_backup_suffix)))
+    return false;
+  size_t dstlen = strlen (dst_name);
+  char *dst_back = xmalloc (dstlen + suffixlen + 1);
+  strcpy (mempcpy (dst_back, dst_name, dstlen), simple_backup_suffix);
+  struct stat dst_back_sb;
+  int dst_back_status = stat (dst_back, &dst_back_sb);
+  free (dst_back);
+  return dst_back_status == 0 && SAME_INODE (*src_st, dst_back_sb);
+}
+
 /* Copy the file SRC_NAME to the file DST_NAME.  The files may be of
    any type.  NEW_DST should be true if the file DST_NAME cannot
    exist because its parent directory was just created; NEW_DST should
@@ -2081,23 +2104,24 @@ copy_internal (char const *src_name, char const *dst_name,
                  existing hierarchy.  */
               && (x->move_mode || ! S_ISDIR (dst_sb.st_mode)))
             {
-              /* Silently use numbered backups if creating the backup
-                 file might destroy the source file.  Otherwise, the commands:
+              /* Fail if creating the backup file would likely destroy
+                 the source file.  Otherwise, the commands:
                  cd /tmp; rm -f a a~; : > a; echo A > a~; cp --b=simple a~ a
                  would leave two zero-length files: a and a~.  */
-              enum backup_type backup_type = x->backup_type;
-              if (backup_type != numbered_backups)
+              if (x->backup_type != numbered_backups
+                  && source_is_dst_backup (srcbase, &src_sb, dst_name))
                 {
-                  size_t srcbaselen = strlen (srcbase);
-                  char const *dstbase = last_component (dst_name);
-                  size_t dstbaselen = strlen (dstbase);
-                  if (srcbaselen == dstbaselen + strlen (simple_backup_suffix)
-                      && memcmp (srcbase, dstbase, dstbaselen) == 0
-                      && STREQ (srcbase + dstbaselen, simple_backup_suffix))
-                    backup_type = numbered_backups;
+                  const char *fmt;
+                  fmt = (x->move_mode
+                 ? _("backing up %s would destroy source;  %s not moved")
+                 : _("backing up %s would destroy source;  %s not copied"));
+                  error (0, 0, fmt,
+                         quoteaf_n (0, dst_name),
+                         quoteaf_n (1, src_name));
+                  return false;
                 }
 
-              char *tmp_backup = backup_file_rename (dst_name, backup_type);
+              char *tmp_backup = backup_file_rename (dst_name, x->backup_type);
 
               /* FIXME: use fts:
                  Using alloca for a file name that may be arbitrarily

tests/cp/backup-is-src.sh

@@ -20,15 +20,17 @@
 print_ver_ cp
 
 echo a > a || framework_failure_
-cp a a0 || framework_failure_
 echo a-tilde > a~ || framework_failure_
-cp a~ a~0 || framework_failure_
 
-# This cp command should not trash the source.
-cp --b=simple a~ ./a > out 2>&1 || fail=1
+# This cp command should exit nonzero.
+cp --b=simple a~ a > out 2>&1 && fail=1
 
-compare a~0 a || fail=1
-compare a~0 a~ || fail=1
-compare a0 a.~1~ || fail=1
+sed "s,cp:,XXX:," out > out2
+
+cat > exp <<\EOF
+XXX: backing up 'a' would destroy source;  'a~' not copied
+EOF
+
+compare exp out2 || fail=1
 
 Exit $fail

tests/mv/backup-is-src.sh

@@ -22,18 +22,25 @@ cleanup_() { rm -rf "$other_partition_tmpdir"; }
 . "$abs_srcdir/tests/other-fs-tmpdir"
 
 a="$other_partition_tmpdir/a"
-a2="$other_partition_tmpdir/./a~"
+a2="$other_partition_tmpdir/a~"
 
-rm -f "$a" "$a2" a20 || framework_failure_
+rm -f "$a" "$a2" || framework_failure_
 echo a > "$a" || framework_failure_
-cp "$a" a0 || framework_failure_
 echo a2 > "$a2" || framework_failure_
-cp "$a2" a20 || framework_failure_
 
-# This mv command should not trash the source.
-mv --b=simple "$a2" "$a" > out 2>&1 || fail=1
+# This mv command should exit nonzero.
+mv --b=simple "$a2" "$a" > out 2>&1 && fail=1
 
-compare a20 "$a" || fail=1
-compare a0 "$a.~1~" || fail=1
+sed \
+   -e "s,mv:,XXX:," \
+   -e "s,$a,YYY," \
+   -e "s,$a2,ZZZ," \
+  out > out2
+
+cat > exp <<\EOF
+XXX: backing up 'YYY' would destroy source;  'ZZZ' not moved
+EOF
+
+compare exp out2 || fail=1
 
 Exit $fail