Subject: Re: bug#23153: [PATCH]: For FIXME in cp.c

From: Pádraig Brady <[email-masked]>
Date: Thu, 3 Nov 2016 00:06:41 +0000

Attached are the two patches I intend to push for this
upon the next gnulib update.

Marking this bug as done.

thanks,
Pádraig
>From 2535447f85c4ca5b7caea0bf144e125267e0fa95 Mon Sep 17 00:00:00 2001
From: Rishabh Dave <[email-masked]>
Date: Wed, 2 Nov 2016 23:43:47 +0000
Subject: [PATCH 1/2] maint: simplify handling of backup --suffix in various
 tools

* src/cp.c (main): Avoid the getenv("SIMPLE_BACKUP_SUFFIX") call,
which is now done if needed in the gnulib backupfile module.
Also avoid the redundant strdup, as we don't modify this suffix.
* src/install.c (main): Likewise.
* src/ln.c (main): Likewise.
* src/mv.c (main): Likewise.
Fixes http://bugs.gnu.org/23153
---
 src/cp.c      | 10 +---------
 src/install.c | 10 +---------
 src/ln.c      | 10 +---------
 src/mv.c      | 10 +---------
 4 files changed, 4 insertions(+), 36 deletions(-)

diff --git a/src/cp.c b/src/cp.c
index b25c9ce..79b93bd 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -922,7 +922,6 @@ main (int argc, char **argv)
   int c;
   bool ok;
   bool make_backups = false;
-  char *backup_suffix_string;
   char *version_control_string = NULL;
   struct cp_options x;
   bool copy_contents = false;
@@ -941,10 +940,6 @@ main (int argc, char **argv)
   selinux_enabled = (0 < is_selinux_enabled ());
   cp_option_init (&x);
 
-  /* FIXME: consider not calling getenv for SIMPLE_BACKUP_SUFFIX unless
-     we'll actually use backup_suffix_string.  */
-  backup_suffix_string = getenv ("SIMPLE_BACKUP_SUFFIX");
-
   while ((c = getopt_long (argc, argv, "abdfHilLnprst:uvxPRS:TZ",
                            long_opts, NULL))
          != -1)
@@ -1123,7 +1118,7 @@ main (int argc, char **argv)
 
         case 'S':
           make_backups = true;
-          backup_suffix_string = optarg;
+          simple_backup_suffix = optarg;
           break;
 
         case_GETOPT_HELP_CHAR;
@@ -1154,9 +1149,6 @@ main (int argc, char **argv)
       usage (EXIT_FAILURE);
     }
 
-  if (backup_suffix_string)
-    simple_backup_suffix = xstrdup (backup_suffix_string);
-
   x.backup_type = (make_backups
                    ? xget_version (_("backup type"),
                                    version_control_string)
diff --git a/src/install.c b/src/install.c
index 9182e50..b4b282a 100644
--- a/src/install.c
+++ b/src/install.c
@@ -807,7 +807,6 @@ main (int argc, char **argv)
   int exit_status = EXIT_SUCCESS;
   const char *specified_mode = NULL;
   bool make_backups = false;
-  char *backup_suffix_string;
   char *version_control_string = NULL;
   bool mkdir_and_install = false;
   struct cp_options x;
@@ -836,10 +835,6 @@ main (int argc, char **argv)
   dir_arg = false;
   umask (0);
 
-  /* FIXME: consider not calling getenv for SIMPLE_BACKUP_SUFFIX unless
-     we'll actually use backup_suffix_string.  */
-  backup_suffix_string = getenv ("SIMPLE_BACKUP_SUFFIX");
-
   while ((optc = getopt_long (argc, argv, "bcCsDdg:m:o:pt:TvS:Z", long_options,
                               NULL)) != -1)
     {
@@ -889,7 +884,7 @@ main (int argc, char **argv)
           break;
         case 'S':
           make_backups = true;
-          backup_suffix_string = optarg;
+          simple_backup_suffix = optarg;
           break;
         case 't':
           if (target_directory)
@@ -961,9 +956,6 @@ main (int argc, char **argv)
              quoteaf (target_directory));
     }
 
-  if (backup_suffix_string)
-    simple_backup_suffix = xstrdup (backup_suffix_string);
-
   x.backup_type = (make_backups
                    ? xget_version (_("backup type"),
                                    version_control_string)
diff --git a/src/ln.c b/src/ln.c
index 618b03d..0b8eb21 100644
--- a/src/ln.c
+++ b/src/ln.c
@@ -465,7 +465,6 @@ main (int argc, char **argv)
   int c;
   bool ok;
   bool make_backups = false;
-  char *backup_suffix_string;
   char *version_control_string = NULL;
   char const *target_directory = NULL;
   bool no_target_directory = false;
@@ -480,10 +479,6 @@ main (int argc, char **argv)
 
   atexit (close_stdin);
 
-  /* FIXME: consider not calling getenv for SIMPLE_BACKUP_SUFFIX unless
-     we'll actually use backup_suffix_string.  */
-  backup_suffix_string = getenv ("SIMPLE_BACKUP_SUFFIX");
-
   symbolic_link = remove_existing_files = interactive = verbose
     = hard_dir_link = false;
 
@@ -547,7 +542,7 @@ main (int argc, char **argv)
           break;
         case 'S':
           make_backups = true;
-          backup_suffix_string = optarg;
+          simple_backup_suffix = optarg;
           break;
         case_GETOPT_HELP_CHAR;
         case_GETOPT_VERSION_CHAR (PROGRAM_NAME, AUTHORS);
@@ -594,9 +589,6 @@ main (int argc, char **argv)
              quoteaf (file[n_files - 1]));
     }
 
-  if (backup_suffix_string)
-    simple_backup_suffix = xstrdup (backup_suffix_string);
-
   backup_type = (make_backups
                  ? xget_version (_("backup type"), version_control_string)
                  : no_backups);
diff --git a/src/mv.c b/src/mv.c
index 25fa8a4..35b2e92 100644
--- a/src/mv.c
+++ b/src/mv.c
@@ -347,7 +347,6 @@ main (int argc, char **argv)
   int c;
   bool ok;
   bool make_backups = false;
-  char *backup_suffix_string;
   char *version_control_string = NULL;
   struct cp_options x;
   char *target_directory = NULL;
@@ -369,10 +368,6 @@ main (int argc, char **argv)
   /* Try to disable the ability to unlink a directory.  */
   priv_set_remove_linkdir ();
 
-  /* FIXME: consider not calling getenv for SIMPLE_BACKUP_SUFFIX unless
-     we'll actually use backup_suffix_string.  */
-  backup_suffix_string = getenv ("SIMPLE_BACKUP_SUFFIX");
-
   while ((c = getopt_long (argc, argv, "bfint:uvS:TZ", long_options, NULL))
          != -1)
     {
@@ -421,7 +416,7 @@ main (int argc, char **argv)
           break;
         case 'S':
           make_backups = true;
-          backup_suffix_string = optarg;
+          simple_backup_suffix = optarg;
           break;
         case 'Z':
           /* As a performance enhancement, don't even bother trying
@@ -481,9 +476,6 @@ main (int argc, char **argv)
       usage (EXIT_FAILURE);
     }
 
-  if (backup_suffix_string)
-    simple_backup_suffix = xstrdup (backup_suffix_string);
-
   x.backup_type = (make_backups
                    ? xget_version (_("backup type"),
                                    version_control_string)
-- 
2.5.5

>From 8e348bce903bf71518fc9ecbb7b0e20ea321b751 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Wed, 2 Nov 2016 23:56:53 +0000
Subject: [PATCH 2/2] maint: refactor printing of backup suffix --help

* src/system.h (emit_backup_suffix_note): A new function to
output the backup suffix info.  The strings are unchanged,
so translations are not impacted.
* src/cp.c (usage): Use the new function.
* src/ln.c (usage): Likewise.
* src/mv.c (usage): Likewise.
* src/install.c (usage): Likewise.
---
 src/cp.c      | 14 +-------------
 src/install.c | 14 +-------------
 src/ln.c      | 14 +-------------
 src/mv.c      | 14 +-------------
 src/system.h  | 18 ++++++++++++++++++
 5 files changed, 22 insertions(+), 52 deletions(-)

diff --git a/src/cp.c b/src/cp.c
index 79b93bd..97a868a 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -250,19 +250,7 @@ When --reflink[=always] is specified, perform a lightweight copy, where the\n\
 data blocks are copied only when modified.  If this is not possible the copy\n\
 fails, or if --reflink=auto is specified, fall back to a standard copy.\n\
 "), stdout);
-      fputs (_("\
-\n\
-The backup suffix is '~', unless set with --suffix or SIMPLE_BACKUP_SUFFIX.\n\
-The version control method may be selected via the --backup option or through\n\
-the VERSION_CONTROL environment variable.  Here are the values:\n\
-\n\
-"), stdout);
-      fputs (_("\
-  none, off       never make backups (even if --backup is given)\n\
-  numbered, t     make numbered backups\n\
-  existing, nil   numbered if numbered backups exist, simple otherwise\n\
-  simple, never   always make simple backups\n\
-"), stdout);
+      emit_backup_suffix_note ();
       fputs (_("\
 \n\
 As a special case, cp makes a backup of SOURCE when the force and backup\n\
diff --git a/src/install.c b/src/install.c
index b4b282a..4fa4bb3 100644
--- a/src/install.c
+++ b/src/install.c
@@ -681,19 +681,7 @@ In the 4th form, create all components of the given DIRECTORY(ies).\n\
 
       fputs (HELP_OPTION_DESCRIPTION, stdout);
       fputs (VERSION_OPTION_DESCRIPTION, stdout);
-      fputs (_("\
-\n\
-The backup suffix is '~', unless set with --suffix or SIMPLE_BACKUP_SUFFIX.\n\
-The version control method may be selected via the --backup option or through\n\
-the VERSION_CONTROL environment variable.  Here are the values:\n\
-\n\
-"), stdout);
-      fputs (_("\
-  none, off       never make backups (even if --backup is given)\n\
-  numbered, t     make numbered backups\n\
-  existing, nil   numbered if numbered backups exist, simple otherwise\n\
-  simple, never   always make simple backups\n\
-"), stdout);
+      emit_backup_suffix_note ();
       emit_ancillary_info (PROGRAM_NAME);
     }
   exit (status);
diff --git a/src/ln.c b/src/ln.c
index 0b8eb21..2a56dc9 100644
--- a/src/ln.c
+++ b/src/ln.c
@@ -436,19 +436,7 @@ interpreted in relation to its parent directory.\n\
 "), stdout);
       fputs (HELP_OPTION_DESCRIPTION, stdout);
       fputs (VERSION_OPTION_DESCRIPTION, stdout);
-      fputs (_("\
-\n\
-The backup suffix is '~', unless set with --suffix or SIMPLE_BACKUP_SUFFIX.\n\
-The version control method may be selected via the --backup option or through\n\
-the VERSION_CONTROL environment variable.  Here are the values:\n\
-\n\
-"), stdout);
-      fputs (_("\
-  none, off       never make backups (even if --backup is given)\n\
-  numbered, t     make numbered backups\n\
-  existing, nil   numbered if numbered backups exist, simple otherwise\n\
-  simple, never   always make simple backups\n\
-"), stdout);
+      emit_backup_suffix_note ();
       printf (_("\
 \n\
 Using -s ignores -L and -P.  Otherwise, the last option specified controls\n\
diff --git a/src/mv.c b/src/mv.c
index 35b2e92..6a3d0d2 100644
--- a/src/mv.c
+++ b/src/mv.c
@@ -323,19 +323,7 @@ If you specify more than one of -i, -f, -n, only the final one takes effect.\n\
 "), stdout);
       fputs (HELP_OPTION_DESCRIPTION, stdout);
       fputs (VERSION_OPTION_DESCRIPTION, stdout);
-      fputs (_("\
-\n\
-The backup suffix is '~', unless set with --suffix or SIMPLE_BACKUP_SUFFIX.\n\
-The version control method may be selected via the --backup option or through\n\
-the VERSION_CONTROL environment variable.  Here are the values:\n\
-\n\
-"), stdout);
-      fputs (_("\
-  none, off       never make backups (even if --backup is given)\n\
-  numbered, t     make numbered backups\n\
-  existing, nil   numbered if numbered backups exist, simple otherwise\n\
-  simple, never   always make simple backups\n\
-"), stdout);
+      emit_backup_suffix_note ();
       emit_ancillary_info (PROGRAM_NAME);
     }
   exit (status);
diff --git a/src/system.h b/src/system.h
index 1b7a0fb..e82dce4 100644
--- a/src/system.h
+++ b/src/system.h
@@ -609,6 +609,24 @@ Otherwise, units default to 1024 bytes (or 512 if POSIXLY_CORRECT is set).\n\
 }
 
 static inline void
+emit_backup_suffix_note (void)
+{
+  fputs (_("\
+\n\
+The backup suffix is '~', unless set with --suffix or SIMPLE_BACKUP_SUFFIX.\n\
+The version control method may be selected via the --backup option or through\n\
+the VERSION_CONTROL environment variable.  Here are the values:\n\
+\n\
+"), stdout);
+  fputs (_("\
+  none, off       never make backups (even if --backup is given)\n\
+  numbered, t     make numbered backups\n\
+  existing, nil   numbered if numbered backups exist, simple otherwise\n\
+  simple, never   always make simple backups\n\
+"), stdout);
+}
+
+static inline void
 emit_ancillary_info (char const *program)
 {
   struct infomap { char const *program; char const *node; } const infomap[] = {
-- 
2.5.5