chown,chgrp: output the correct ownership in -v messages

* src/chown_core.c (describe_change): Accept the ownership of
the original file and output that when not changing.
This is significant when --from is specified as then
the original and specified ownership may be different.
(user_group_str): A new helper function refactored from
describe_change().
(change_file_owner): Pass the original user and group
strings to describe_change().
* test/chown/basic: Add a test case.
* NEWS: Mention the fix.

3 files changed, 53 insertions, 21 deletions

diff --git a/NEWS b/NEWS
index c4238f42d..2a3f8601a 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,10 @@ GNU coreutils NEWS                                    -*- outline -*-
 
 ** Bug fixes
 
+  chown and chgrp with the -v --from= options, now output the correct owner.
+  I.E. for skipped files, the original ownership is output, not the new one.
+  [bug introduced in sh-utils-2.0g]
+
   printf '%d' '"' no longer accesses out-of-bounds memory in the diagnostic.
   [bug introduced in sh-utils-1.16]
 
diff --git a/src/chown-core.c b/src/chown-core.c
index 82f7341b2..e72aa33b1 100644
--- a/src/chown-core.c
+++ b/src/chown-core.c
@@ -102,17 +102,44 @@ uid_to_name (uid_t uid)
                   : umaxtostr (uid, buf));
 }
 
+/* Allocate a string representing USER and GROUP.  */
+
+static char *
+user_group_str (char const *user, char const *group)
+{
+  char *spec;
+
+  if (user)
+    {
+      if (group)
+        {
+          spec = xmalloc (strlen (user) + 1 + strlen (group) + 1);
+          stpcpy (stpcpy (stpcpy (spec, user), ":"), group);
+        }
+      else
+        {
+          spec = xstrdup (user);
+        }
+    }
+  else
+    {
+      spec = xstrdup (group);
+    }
+
+  return spec;
+}
+
 /* Tell the user how/if the user and group of FILE have been changed.
    If USER is NULL, give the group-oriented messages.
    CHANGED describes what (if anything) has happened. */
 
 static void
 describe_change (const char *file, enum Change_status changed,
+                 char const *old_user, char const *old_group,
                  char const *user, char const *group)
 {
   const char *fmt;
-  char const *spec;
-  char *spec_allocated = NULL;
+  char *spec;
 
   if (changed == CH_NOT_APPLIED)
     {
@@ -121,40 +148,25 @@ describe_change (const char *file, enum Change_status changed,
       return;
     }
 
-  if (user)
-    {
-      if (group)
-        {
-          spec_allocated = xmalloc (strlen (user) + 1 + strlen (group) + 1);
-          stpcpy (stpcpy (stpcpy (spec_allocated, user), ":"), group);
-          spec = spec_allocated;
-        }
-      else
-        {
-          spec = user;
-        }
-    }
-  else
-    {
-      spec = group;
-    }
-
   switch (changed)
     {
     case CH_SUCCEEDED:
       fmt = (user ? _("changed ownership of %s to %s\n")
              : group ? _("changed group of %s to %s\n")
              : _("no change to ownership of %s\n"));
+      spec = user_group_str (user, group);
       break;
     case CH_FAILED:
       fmt = (user ? _("failed to change ownership of %s to %s\n")
              : group ? _("failed to change group of %s to %s\n")
              : _("failed to change ownership of %s\n"));
+      spec = user_group_str (user, group);
       break;
     case CH_NO_CHANGE_REQUESTED:
       fmt = (user ? _("ownership of %s retained as %s\n")
              : group ? _("group of %s retained as %s\n")
              : _("ownership of %s retained\n"));
+      spec = user_group_str (user ? old_user : NULL, group ? old_group : NULL);
       break;
     default:
       abort ();
@@ -162,7 +174,7 @@ describe_change (const char *file, enum Change_status changed,
 
   printf (fmt, quote (file), spec);
 
-  free (spec_allocated);
+  free (spec);
 }
 
 /* Change the owner and/or group of the FILE to UID and/or GID (safely)
@@ -459,8 +471,13 @@ change_file_owner (FTS *fts, FTSENT *ent,
              : !symlink_changed ? CH_NOT_APPLIED
              : !changed ? CH_NO_CHANGE_REQUESTED
              : CH_SUCCEEDED);
+          char *old_usr = file_stats ? uid_to_name (file_stats->st_uid) : NULL;
+          char *old_grp = file_stats ? gid_to_name (file_stats->st_gid) : NULL;
           describe_change (file_full_name, ch_status,
+                           old_usr, old_grp,
                            chopt->user_name, chopt->group_name);
+          free (old_usr);
+          free (old_grp);
         }
     }
 
diff --git a/tests/chown/basic b/tests/chown/basic
index 0614f7090..5626029ae 100755
--- a/tests/chown/basic
+++ b/tests/chown/basic
@@ -27,6 +27,17 @@ chown -R --preserve-root 0:1 f
 # Make sure the owner and group are 0 and 1 respectively.
 set _ `ls -n f`; shift; test "$3:$4" = 0:1 || fail=1
 
+# Make sure the correct diagnostic is output
+# Note we output a name even though an id was specified.
+chown -v --from=42 43 f > out || fail=1
+printf "ownership of \`f' retained as `id -nu`\n" > exp
+compare out exp || fail=1
+
+# Ensure diagnostics work for non existent files.
+chown -v 0 nf > out && fail=1
+printf "failed to change ownership of \`nf' to 0\n" > exp
+compare out exp || fail=1
+
 chown --from=0:1 2:010 f || fail=1
 
 # And now they should be 2 and 10 respectively.