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

From: [email-masked] (GNU bug Tracking System)
Date: Thu, 03 Nov 2016 00:07:02 +0000

Your bug report

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

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
23153: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23153
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#23153: [PATCH]: For FIXME in cp.c
From: Pádraig Brady <[email-masked]>
To: Rishabh Dave <[email-masked]>
Cc: [email-masked]
Date: Thu, 3 Nov 2016 00:06:41 +0000
List-Id: <debbugs-submit.debbugs.gnu.org>

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

Subject: [PATCH]: For FIXME in cp.c
From: Rishabh Dave <[email-masked]>
To: [email-masked]
Date: Tue, 29 Mar 2016 20:58:01 +0530
List-Id: <debbugs-submit.debbugs.gnu.org>

Hello,

I have wrote the attached patch for following FIXME in file src/cp.c -

/* FIXME: consider not calling getenv for SIMPLE_BACKUP_SUFFIX unless
     we'll actually use backup_suffix_string.  */
  backup_suffix_string = getenv ("SIMPLE_BACKUP_SUFFIX");

Since we use backup_suffix_string to duplicate it into
simple_backup_suffix, I brought the getenv() call there, that too, only if
required (as simple_backup_suffix already stores tilde already).

I did 'diff -ur'  directly against original cp.c (named cp-original.c,
then) to create the patch. I tested patch using -b, --backup and --suffix
option of c. Version I have used is latest one on savannah.gnu.org -
coreuitls-8.25.

There was cppi (didn't know what it does) at the bottom of coreutil's
download page. After reading it's README, I concluded that it is not to be
considered while debugging/fixing coreutils. Hopefully, I was correct in
doing so.

If incorrect, please do correct me in any case. :)

There was one little doubt, maybe bug, after doing '--backups=numbered' it
becomes impossible to have suffixed backups (using --suffix or -b) until we
do '--backup=simple' explicitly. Is this supposed to be? I tried with both
altered and unaltered version of cp.
(And should I have or should create/d a new separate thread? I wasn't sure.)
--- cp-original.c	2016-01-01 19:18:50.000000000 +0530
+++ cp-edited.c	2016-03-29 20:13:47.274362100 +0530
@@ -920,7 +920,7 @@
   int c;
   bool ok;
   bool make_backups = false;
-  char *backup_suffix_string;
+  char *backup_suffix_string = NULL;
   char *version_control_string = NULL;
   struct cp_options x;
   bool copy_contents = false;
@@ -939,10 +939,6 @@
   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)
@@ -1151,6 +1147,11 @@
       usage (EXIT_FAILURE);
     }
 
+  /* If backup_suffix_string is still null (i.e. no --suffix or --backup = 
+     numbered | existing), try to get it from environment. */
+  if (!backup_suffix_string)
+    backup_suffix_string = getenv ("SIMPLE_BACKUP_SUFFIX");
+
   if (backup_suffix_string)
     simple_backup_suffix = xstrdup (backup_suffix_string);