Subject: Re: bug#15173: [cp] --link overrides dereference settings

From: Gian Piero Carrubba <[email-masked]>
Date: Thu, 31 Oct 2013 22:28:47 +0100

* [Thu, Oct 31, 2013 at 02:54:59PM +0100] Gian Piero Carrubba:
>Oh... I was tweaking a bit the last patch posted by Bernhard in order 
>to let it be POSIX compliant (but now I have to add: "for my 
>interpretation of POSIX"), but didn't had the time to complete before 
>going to work.  Will probably finish it up anyway this evening or 
>tomorrow, at this point at least for comparing how far we're currently 
>are from "POSIX".

Here it is.
First of all, sorry for having generated a lot of noise. I completely 
misunderstood the meaning of --no-preserve=links. Having get rid of 
that, the differences in the results of the tests are a lot less.

$ fgrep "DIFFERENCE in Test: " testit.log | sed 's/^.*: //'
cp  -l   filelink ...
cp  -l  --preserve=links filelink ...
cp -L -l   filelink ...
cp -L -l  --preserve=links filelink ...
cp -L -l -R  filelink ...
cp -L -l -R --preserve=links filelink ...
cp -H -l   filelink ...
cp -H -l  --preserve=links filelink ...
cp -H -l -R  filelink ...
cp -H -l -R --preserve=links filelink ...

I think they are all legit (full log and differences summary attached).

* Changes with regard to the last patch posted by Bernhard.

=== Minor changes

- use 'dereference' instead of 'deref' in order to be consistent with 
   'x->dereference' and for passing the "grep test"[0]
- use 'command_line_arg' instead of 'cli_arg' in order to be consistent 
   with an already used variable and for passing the "grep test"
- move up the declaration of bool dereference in order to use it for 
   every invocation of create_hard_link()
- clean up the initialization of 'flags' in create_hard_link()

[0] i.e.: http://jamie-wong.com/2013/07/12/grep-test/

=== Major changes

- dereference by default (i.e.: unless --no-dereference is used) the 
   source files.
   I promise that I will not insist anymore :), but I really think this 
   is important for consistency from a user point of view.

Ciao,
Gian Piero.
Thu Oct 31 21:57:32 CET 2013  Gian Piero Carrubba <[email-masked]>
  * bug-15173
  http://debbugs.gnu.org/cgi/bugreport.cgi?bug=15173
diff -rN -u old-trunk/NEWS new-trunk/NEWS
--- old-trunk/NEWS	2013-10-31 22:24:51.690232614 +0100
+++ new-trunk/NEWS	2013-10-31 22:24:51.694232641 +0100
@@ -70,6 +70,10 @@
 
 ** Changes in behavior
 
+  cp --link now dereferences a symbolic link as source files before creating
+  the hard link in the destination, unless --no-dereference is used too.
+  Previously, it would create a hard link of the symbolic link.
+
   dd status=none now suppresses all non fatal diagnostic messages,
   not just the transfer counts.
 
diff -rN -u old-trunk/src/copy.c new-trunk/src/copy.c
--- old-trunk/src/copy.c	2013-10-31 22:24:51.690232614 +0100
+++ new-trunk/src/copy.c	2013-10-31 22:24:51.698232676 +0100
@@ -1558,18 +1558,23 @@
            _("failed to restore the default file creation context"));
 }
 
-/* Create a hard link DST_NAME to SRC_NAME, honoring the REPLACE and
-   VERBOSE settings.  Return true upon success.  Otherwise, diagnose
-   the failure and return false.
-   If SRC_NAME is a symbolic link it will not be followed.  If the system
-   doesn't support hard links to symbolic links, then DST_NAME will
-   be created as a symbolic link to SRC_NAME.  */
+/* Create a hard link DST_NAME to SRC_NAME, honoring the REPLACE, VERBOSE and
+   DEREFERENCE settings. Return true upon success. Otherwise, diagnose the
+   failure and return false. If SRC_NAME is a symbolic link it will not be
+   followed unless DEREFERENCE is true.
+   If the system doesn't support hard links to symbolic links, then DST_NAME
+   will be created as a symbolic link to SRC_NAME.  */
 static bool
 create_hard_link (char const *src_name, char const *dst_name,
-                  bool replace, bool verbose)
+                  bool replace, bool verbose, bool dereference)
 {
-  /* We want to guarantee that symlinks are not followed.  */
-  bool link_failed = (linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, 0) != 0);
+  /* We want to guarantee that symlinks are not followed, unless requested. */
+  int flags = 0;
+  if (dereference)
+    flags = AT_SYMLINK_FOLLOW;
+
+  bool link_failed = (linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, flags)
+                      != 0);
 
   /* If the link failed because of an existing destination,
      remove that file and then call link again.  */
@@ -1582,7 +1587,8 @@
         }
       if (verbose)
         printf (_("removed %s\n"), quote (dst_name));
-      link_failed = (linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, 0) != 0);
+      link_failed = (linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, flags)
+                     != 0);
     }
 
   if (link_failed)
@@ -1595,6 +1601,17 @@
   return true;
 }
 
+/* Return true if the current file should be (tried to be) dereferenced:
+   either for DEREF_ALWAYS or for DEREF_COMMAND_LINE_ARGUMENTS in the case
+   where the current file is a COMMAND_LINE_ARG; otherwise return false. */
+static inline bool _GL_ATTRIBUTE_PURE
+should_dereference (const struct cp_options *x, bool command_line_arg)
+{
+  return x->dereference == DEREF_ALWAYS
+         || (x->dereference == DEREF_COMMAND_LINE_ARGUMENTS
+             && command_line_arg);
+}
+
 /* 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
@@ -1670,6 +1687,8 @@
       record_file (x->src_info, src_name, &src_sb);
     }
 
+  bool dereference = should_dereference (x, command_line_arg);
+
   if (!new_dst)
     {
       /* Regular files can be created by writing through symbolic
@@ -1748,7 +1767,7 @@
                       /* Note we currently replace DST_NAME unconditionally,
                          even if it was a newer separate file.  */
                       if (! create_hard_link (earlier_file, dst_name, true,
-                                              x->verbose))
+                                              x->verbose, dereference))
                         {
                           goto un_backup;
                         }
@@ -2078,7 +2097,8 @@
         }
       else
         {
-          if (! create_hard_link (earlier_file, dst_name, true, x->verbose))
+          if (! create_hard_link (earlier_file, dst_name, true, x->verbose,
+                                  dereference))
             goto un_backup;
 
           return true;
@@ -2389,7 +2409,7 @@
            && !(LINK_FOLLOWS_SYMLINKS && S_ISLNK (src_mode)
                 && x->dereference == DEREF_NEVER))
     {
-      if (! create_hard_link (src_name, dst_name, false, false))
+      if (! create_hard_link (src_name, dst_name, false, false, dereference))
         goto un_backup;
     }
   else if (S_ISREG (src_mode)
diff -rN -u old-trunk/tests/cp/same-file.sh new-trunk/tests/cp/same-file.sh
--- old-trunk/tests/cp/same-file.sh	2013-10-31 22:24:51.690232614 +0100
+++ new-trunk/tests/cp/same-file.sh	2013-10-31 22:24:51.698232676 +0100
@@ -189,9 +189,9 @@
 0 -bf (foo sl1 -> foo sl2 sl2.~1~ -> foo)
 0 -bdf (foo sl1 -> foo sl2 -> foo sl2.~1~ -> foo)
 1 -l [cp: cannot create hard link 'sl2' to 'sl1'] (foo sl1 -> foo sl2 -> foo)
-0 -fl (foo sl1 -> foo sl2 -> foo)
-0 -bl (foo sl1 -> foo sl2 -> foo sl2.~1~ -> foo)
-0 -bfl (foo sl1 -> foo sl2 -> foo sl2.~1~ -> foo)
+0 -fl (foo sl1 -> foo sl2)
+0 -bl (foo sl1 -> foo sl2 sl2.~1~ -> foo)
+0 -bfl (foo sl1 -> foo sl2 sl2.~1~ -> foo)
 
 1 [cp: 'foo' and 'hardlink' are the same file] (foo hardlink)
 1 -d [cp: 'foo' and 'hardlink' are the same file] (foo hardlink)