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

From: Rishabh Dave <[email-masked]>
Date: Mon, 4 Apr 2016 22:00:19 +0530

Hello,

Call to getenv(), which is in gnulib's backupfile.c, for cp, mv, ln and
install, is done only if simple_backup_suffix is tilde which would imply we
don't have backup or suffix option chosen while issuing the command. Thus,
only option remaining is to checking the environment.

Sincerely,
Rishabh

On Sun, Apr 3, 2016 at 2:33 PM, Pádraig Brady <[email-masked]> wrote:

> On 29/03/16 16:28, Rishabh Dave wrote:
>
>> 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.)
>>
>>
>
> Thanks for the patch.
> This doesn't address the FIXME though as we still getenv() in the normal
> case.
> To fix that would be more invasive and may need changes to gnulib's
> backupfile module.
> Also any changes here should also be done for mv, ln and install.
>
>
> thanks,
> Pádraig
>
diff -ur Original/coreutils-8.25/lib/backupfile.c Debugging/coreutils-8.25/lib/backupfile.c
--- Original/coreutils-8.25/lib/backupfile.c	2016-01-01 19:15:55.000000000 +0530
+++ Debugging/coreutils-8.25/lib/backupfile.c	2016-04-02 18:46:51.756888000 +0530
@@ -265,9 +265,16 @@
 {
   size_t filelen = strlen (file);
   char *s;
+  char *suffix_from_env;
   size_t ssize;
   bool simple = true;
 
+
+  /* If simple_backup_suffix is '~', check environment if we have any there. */
+  if (strcmp(simple_backup_suffix, "~") == 0)
+    if ((suffix_from_env = getenv("SIMPLE_BACKUP_SUFFIX")) != NULL)
+      simple_backup_suffix = xstrdup (suffix_from_env);
+
   /* Allow room for simple or ".~N~" backups.  The guess must be at
      least sizeof ".~1~", but otherwise will be adjusted as needed.  */
   size_t simple_backup_suffix_size = strlen (simple_backup_suffix) + 1;
diff -ur Original/coreutils-8.25/src/cp.c Debugging/coreutils-8.25/src/cp.c
--- Original/coreutils-8.25/src/cp.c	2016-01-01 19:18:50.000000000 +0530
+++ Debugging/coreutils-8.25/src/cp.c	2016-04-02 18:29:20.284884000 +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)
diff -ur Original/coreutils-8.25/src/install.c Debugging/coreutils-8.25/src/install.c
--- Original/coreutils-8.25/src/install.c	2016-01-03 18:29:44.000000000 +0530
+++ Debugging/coreutils-8.25/src/install.c	2016-04-02 18:31:36.856884000 +0530
@@ -784,7 +784,7 @@
   int exit_status = EXIT_SUCCESS;
   const char *specified_mode = NULL;
   bool make_backups = false;
-  char *backup_suffix_string;
+  char *backup_suffix_string = NULL;
   char *version_control_string = NULL;
   bool mkdir_and_install = false;
   struct cp_options x;
@@ -813,10 +813,6 @@
   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)
     {
diff -ur Original/coreutils-8.25/src/ln.c Debugging/coreutils-8.25/src/ln.c
--- Original/coreutils-8.25/src/ln.c	2016-01-01 19:18:50.000000000 +0530
+++ Debugging/coreutils-8.25/src/ln.c	2016-04-02 18:31:11.764884000 +0530
@@ -464,7 +464,7 @@
   int c;
   bool ok;
   bool make_backups = false;
-  char *backup_suffix_string;
+  char *backup_suffix_string = NULL;
   char *version_control_string = NULL;
   char const *target_directory = NULL;
   bool no_target_directory = false;
@@ -479,10 +479,6 @@
 
   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;
 
diff -ur Original/coreutils-8.25/src/mv.c Debugging/coreutils-8.25/src/mv.c
--- Original/coreutils-8.25/src/mv.c	2016-01-12 17:11:44.000000000 +0530
+++ Debugging/coreutils-8.25/src/mv.c	2016-04-02 18:30:24.832884000 +0530
@@ -346,7 +346,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;
   char *target_directory = NULL;
@@ -368,10 +368,6 @@
   /* 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)
     {