Re: confusing error message from ln

[Paul Eggert]

I installed the following in an attempt to address the issues brought
up in this thread, along with some other problems I noticed when
comparing GNU ln's to FreeBSD ln's behavior.

2005-11-16  Paul Eggert  <address@hidden>

        * NEWS: Improve quality of ln's diagnostics.
        * doc/coreutils.texi (ln invocation): ln -v now outputs lines only for
        successful links.
        * src/ln.c (do_link, usage): Likewise.
        (do_link): Don't use alloca on a buffer of unbounded size.

Index: NEWS
===================================================================
RCS file: /fetish/cu/NEWS,v
retrieving revision 1.345
diff -p -u -r1.345 NEWS
--- NEWS        15 Nov 2005 18:03:26 -0000      1.345
+++ NEWS        16 Nov 2005 22:31:24 -0000
@@ -4,6 +4,10 @@ GNU coreutils NEWS                      
 
 ** Changes in behavior
 
+  ln now uses different (and we hope clearer) diagnostics when it fails.
+  ln -v now acts more like FreeBSD, so it generates output only when
+  successful and the output is easier to parse.
+
   ls now defaults to --time-style='locale', not --time-style='posix-long-iso'.
   However, the 'locale' time style now behaves like 'posix-long-iso'
   if your locale settings appear to be messed up.  This change
Index: doc/coreutils.texi
===================================================================
RCS file: /fetish/cu/doc/coreutils.texi,v
retrieving revision 1.294
diff -p -u -r1.294 coreutils.texi
--- doc/coreutils.texi  8 Nov 2005 20:59:19 -0000       1.294
+++ doc/coreutils.texi  16 Nov 2005 22:31:30 -0000
@@ -7773,7 +7773,7 @@ an error message on systems that do not 
 @itemx --verbose
 @opindex -v
 @opindex --verbose
-Print the name of each file before linking it.
+Print the name of each file after linking it successfully.
 
 @end table
 
Index: src/ln.c
===================================================================
RCS file: /fetish/cu/src/ln.c,v
retrieving revision 1.154
diff -p -u -r1.154 ln.c
--- src/ln.c    5 Nov 2005 09:34:57 -0000       1.154
+++ src/ln.c    16 Nov 2005 22:31:36 -0000
@@ -138,6 +138,8 @@ do_link (const char *source, const char 
   struct stat dest_stats;
   char *dest_backup = NULL;
   bool lstat_ok = false;
+  bool source_is_dir = false;
+  bool ok;
 
   /* Use stat here instead of lstat.
      On SVR4, link does not follow symlinks, so this check disallows
@@ -159,11 +161,15 @@ do_link (const char *source, const char 
                 quote (source));
        }
 
-      if (!hard_dir_link && S_ISDIR (source_stats.st_mode))
+      if (S_ISDIR (source_stats.st_mode))
        {
-         error (0, 0, _("%s: hard link not allowed for directory"),
-                quote (source));
-         return false;
+         source_is_dir = true;
+         if (! hard_dir_link)
+           {
+             error (0, 0, _("%s: hard link not allowed for directory"),
+                    quote (source));
+             return false;
+           }
        }
     }
 
@@ -220,37 +226,22 @@ do_link (const char *source, const char 
 
       if (backup_type != no_backups)
        {
-         char *tmp_backup = find_backup_file_name (dest, backup_type);
-         size_t buf_len = strlen (tmp_backup) + 1;
-         dest_backup = alloca (buf_len);
-         memcpy (dest_backup, tmp_backup, buf_len);
-         free (tmp_backup);
-         if (rename (dest, dest_backup))
+         dest_backup = find_backup_file_name (dest, backup_type);
+         if (rename (dest, dest_backup) != 0)
            {
-             if (errno != ENOENT)
+             int rename_errno = errno;
+             free (dest_backup);
+             dest_backup = NULL;
+             if (rename_errno != ENOENT)
                {
-                 error (0, errno, _("cannot backup %s"), quote (dest));
+                 error (0, rename_errno, _("cannot backup %s"), quote (dest));
                  return false;
                }
-             else
-               dest_backup = NULL;
            }
        }
     }
 
-  if (verbose)
-    {
-      printf ((symbolic_link
-              ? _("create symbolic link %s to %s")
-              : _("create hard link %s to %s")),
-             quote_n (0, dest), quote_n (1, source));
-      if (dest_backup)
-       printf (_(" (backup: %s)"), quote (dest_backup));
-      putchar ('\n');
-    }
-
-  if ((*linkfunc) (source, dest) == 0)
-    return true;
+  ok = (linkfunc (source, dest) == 0);
 
   /* If the attempt to create a link failed and we are removing or
      backing up destinations, unlink the destination and try again.
@@ -270,30 +261,52 @@ do_link (const char *source, const char 
      If we didn't remove DEST in that case, the subsequent LINKFUNC
      call would fail.  */
 
-  if (errno == EEXIST && (remove_existing_files || dest_backup))
+  if (!ok && errno == EEXIST && (remove_existing_files || dest_backup))
     {
       if (unlink (dest) != 0)
        {
          error (0, errno, _("cannot remove %s"), quote (dest));
+         free (dest_backup);
          return false;
        }
 
-      if (linkfunc (source, dest) == 0)
-       return true;
+      ok = (linkfunc (source, dest) == 0);
     }
 
-  error (0, errno,
-        (symbolic_link
-         ? _("creating symbolic link %s to %s")
-         : _("creating hard link %s to %s")),
-        quote_n (0, dest), quote_n (1, source));
-
-  if (dest_backup)
+  if (ok)
     {
-      if (rename (dest_backup, dest))
-       error (0, errno, _("cannot un-backup %s"), quote (dest));
+      if (verbose)
+       {
+         if (dest_backup)
+           printf ("%s ~ ", quote (dest_backup));
+         printf ("%s %c> %s\n", quote_n (0, dest), (symbolic_link ? '-' : '='),
+                 quote_n (1, source));
+       }
+    }
+  else
+    {
+      error (0, errno,
+            (symbolic_link
+             ? (errno != ENAMETOOLONG && *source
+                ? _("creating symbolic link %s")
+                : _("creating symbolic link %s -> %s"))
+             : (errno == EMLINK && !source_is_dir
+                ? _("creating hard link to %.0s%s")
+                : (errno == EDQUOT || errno == EEXIST || errno == ENOSPC
+                   || errno == EROFS)
+                ? _("creating hard link %s")
+                : _("creating hard link %s => %s"))),
+            quote_n (0, dest), quote_n (1, source));
+
+      if (dest_backup)
+       {
+         if (rename (dest_backup, dest) != 0)
+           error (0, errno, _("cannot un-backup %s"), quote (dest));
+       }
     }
-  return false;
+
+  free (dest_backup);
+  return ok;
 }
 
 void
@@ -341,7 +354,7 @@ Mandatory arguments to long options are 
   -t, --target-directory=DIRECTORY  specify the DIRECTORY in which to create\n\
                                 the links\n\
   -T, --no-target-directory   treat LINK_NAME as a normal file\n\
-  -v, --verbose               print name of each file before linking\n\
+  -v, --verbose               print name of each linked file\n\
 "), stdout);
       fputs (HELP_OPTION_DESCRIPTION, stdout);
       fputs (VERSION_OPTION_DESCRIPTION, stdout);