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

From: Rishabh Dave <[email-masked]>
Date: Tue, 5 Apr 2016 20:19:24 +0530

Did fix that - changing function call to GNU style and "suffix_from_env" to
local variable. The corresponding patch is attached.

On Mon, Apr 4, 2016 at 10:36 PM, Paul Eggert <[email-masked]> wrote:

> On 04/04/2016 09:30 AM, Rishabh Dave wrote:
>
>> +  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);
>> +
>>
>
> The usual GNU style is "f (x)" rather than "f(x)" for function calls.
> Also, avoid side effects in an "if"; just have the then-part with a local
> variable "suffix_from_env" and put the subsidary if after that.
>
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-05 20:11:12.096568278 +0530
@@ -268,6 +268,15 @@
   size_t ssize;
   bool simple = true;
 
+
+  /* If simple_backup_suffix is '~', check environment if we have any there. */
+  if (strcmp (simple_backup_suffix, "~") == 0)
+    {
+      char *suffix_from_env;
+      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)
     {