moving timestamp preservation to the last step in copy_internal() / copy_reg()

[Mike Frysinger]

a user pointed out that `cp -p` failed to preserve times on a nfs mount:
http://bugs.gentoo.org/132673

this is because changing ownership on a nfs mount updates the ctimes:
touch foo
# stat foo
  File: `foo'
  Size: 0               Blocks: 0          IO Block: 16384  regular empty file
Device: eh/14d  Inode: 584188      Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (  250/ portage)
Access: 2006-05-08 22:34:13.621879904 -0400
Modify: 2006-05-08 22:34:13.621879904 -0400
Change: 2006-05-08 22:34:13.621879904 -0400
# chown 1 foo
# stat foo
  File: `foo'
  Size: 0               Blocks: 0          IO Block: 16384  regular empty file
Device: eh/14d  Inode: 584188      Links: 1
Access: (0644/-rw-r--r--)  Uid: (    1/     bin)   Gid: (  250/ portage)
Access: 2006-05-08 22:34:13.621879904 -0400
Modify: 2006-05-08 22:34:13.621879904 -0400
Change: 2006-05-08 22:34:21.628662688 -0400

i dont know if POSIX has an opinion on the order of operations, but SUSv3 
doesnt care:
http://www.opengroup.org/onlinepubs/009695399/utilities/cp.html
...
-p Duplicate the following characteristics of each source file in the 
corresponding destination file:
...
The order in which the preceding characteristics are duplicated is 
unspecified.

so would there be any real detrimental effects with the attached patch ? 
clearing the setid bits wouldnt be affected by changing of timestamps, so 
doing it after the chown() should be fine ...
-mike

2006-05-08  Mike Frysinger  <address@hidden>

        src/copy.c (copy_reg): Move preserve_timestamps logic last.
        (copy_internal): Likewise.

--- src/copy.c
+++ src/copy.c
@@ -483,23 +483,6 @@ copy_reg (char const *src_name, char con
        }
     }
 
-  if (x->preserve_timestamps)
-    {
-      struct timespec timespec[2];
-      timespec[0] = get_stat_atime (src_sb);
-      timespec[1] = get_stat_mtime (src_sb);
-
-      if (futimens (dest_desc, dst_name, timespec) != 0)
-       {
-         error (0, errno, _("preserving times for %s"), quote (dst_name));
-         if (x->require_preserve)
-           {
-             return_val = false;
-             goto close_src_and_dst_desc;
-           }
-       }
-    }
-
   if (x->preserve_ownership && ! SAME_OWNER_AND_GROUP (*src_sb, sb))
     {
       if (! set_owner (x, dst_name, dest_desc, src_sb->st_uid, src_sb->st_gid))
@@ -523,6 +506,23 @@ copy_reg (char const *src_name, char con
        return_val = false;
     }
 
+  if (x->preserve_timestamps)
+    {
+      struct timespec timespec[2];
+      timespec[0] = get_stat_atime (src_sb);
+      timespec[1] = get_stat_mtime (src_sb);
+
+      if (futimens (dest_desc, dst_name, timespec) != 0)
+       {
+         error (0, errno, _("preserving times for %s"), quote (dst_name));
+         if (x->require_preserve)
+           {
+             return_val = false;
+             goto close_src_and_dst_desc;
+           }
+       }
+    }
+
 close_src_and_dst_desc:
   if (close (dest_desc) < 0)
     {
@@ -1727,24 +1727,9 @@ copy_internal (char const *src_name, cha
      the destination must not be removed.
      FIXME: implement the above. */
 
-  /* Adjust the times (and if possible, ownership) for the copy.
-     chown turns off set[ug]id bits for non-root,
+  /* chown turns off set[ug]id bits for non-root,
      so do the chmod last.  */
 
-  if (x->preserve_timestamps)
-    {
-      struct timespec timespec[2];
-      timespec[0] = get_stat_atime (&src_sb);
-      timespec[1] = get_stat_mtime (&src_sb);
-
-      if (utimens (dst_name, timespec) != 0)
-       {
-         error (0, errno, _("preserving times for %s"), quote (dst_name));
-         if (x->require_preserve)
-           return false;
-       }
-    }
-
   /* Avoid calling chown if we know it's not necessary.  */
   if (x->preserve_ownership
       && (new_dst || !SAME_OWNER_AND_GROUP (src_sb, dst_sb)))
@@ -1777,6 +1762,21 @@ copy_internal (char const *src_name, cha
        }
     }
 
+  /* Adjust the times (and if possible, ownership) for the copy.  */
+  if (x->preserve_timestamps)
+    {
+      struct timespec timespec[2];
+      timespec[0] = get_stat_atime (&src_sb);
+      timespec[1] = get_stat_mtime (&src_sb);
+
+      if (utimens (dst_name, timespec) != 0)
+       {
+         error (0, errno, _("preserving times for %s"), quote (dst_name));
+         if (x->require_preserve)
+           return false;
+       }
+    }
+
   return delayed_ok;
 
 un_backup: