GNU bug report logs - #23153
[PATCH]: For FIXME in cp.c

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 23153 in the body.
You can then email your comments to 23153 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Rishabh Dave <rishabhddave <at> gmail.com>
To: bug-coreutils <at> gnu.org
Subject: [PATCH]: For FIXME in cp.c
Date: Tue, 29 Mar 2016 20:58:01 +0530

[Message part 1 (text/plain, inline)]

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.)

[Message part 2 (text/html, inline)]

[FIXME-calling-getnev.patch (text/x-patch, attachment)]

Message #8 received at 23153 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Rishabh Dave <rishabhddave <at> gmail.com>, 23153 <at> debbugs.gnu.org
Subject: Re: bug#23153: [PATCH]: For FIXME in cp.c
Date: Sun, 3 Apr 2016 10:03:03 +0100

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

Message #11 received at 23153 <at> debbugs.gnu.org (full text, mbox):

From: Rishabh Dave <rishabhddave <at> gmail.com>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 23153 <at> debbugs.gnu.org
Subject: Re: bug#23153: [PATCH]: For FIXME in cp.c
Date: Mon, 4 Apr 2016 22:00:19 +0530

[Message part 1 (text/plain, inline)]

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 <P <at> draigbrady.com> 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
>

[Message part 2 (text/html, inline)]

[FIXME-regarding-getenv.patch (text/x-patch, attachment)]

Message #14 received at 23153 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Rishabh Dave <rishabhddave <at> gmail.com>, Pádraig Brady
 <P <at> draigbrady.com>
Cc: 23153 <at> debbugs.gnu.org
Subject: Re: bug#23153: [PATCH]: For FIXME in cp.c
Date: Mon, 4 Apr 2016 10:06:40 -0700

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.

Message #17 received at 23153 <at> debbugs.gnu.org (full text, mbox):

From: Rishabh Dave <rishabhddave <at> gmail.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 23153 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>
Subject: Re: bug#23153: [PATCH]: For FIXME in cp.c
Date: Tue, 5 Apr 2016 20:19:24 +0530

[Message part 1 (text/plain, inline)]

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 <eggert <at> cs.ucla.edu> 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.
>

[Message part 2 (text/html, inline)]

[FIXME-regarding-getenv.patch (text/x-patch, attachment)]

Message #20 received at 23153 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Rishabh Dave <rishabhddave <at> gmail.com>, bug-gnulib <bug-gnulib <at> gnu.org>
Cc: 23153 <at> debbugs.gnu.org
Subject: Re: bug#23153: [PATCH]: For FIXME in cp.c
Date: Wed, 2 Nov 2016 17:59:34 +0000

[Message part 1 (text/plain, inline)]

unarchive 23153
stop

On 05/04/16 15:49, Rishabh Dave wrote:
> 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 <eggert <at> cs.ucla.edu> 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);
>>> +

Sorry for the delay.

I'll apply the attached variant to gnulib instead.
That has the property that --suffix=~ will still override
an env variable of SIMPLE_BACKUP_SUFFIX.
It also avoids a redundant malloc, and handles empty env variables.

I'll then apply the coreutils patches in your name.

thanks,
Pádraig

[gnulib-backupfile-no-getenv.patch (text/x-patch, attachment)]

Message #25 received at 23153-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Rishabh Dave <rishabhddave <at> gmail.com>
Cc: 23153-done <at> debbugs.gnu.org
Subject: Re: bug#23153: [PATCH]: For FIXME in cp.c
Date: Thu, 3 Nov 2016 00:06:41 +0000

[Message part 1 (text/plain, inline)]

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

Marking this bug as done.

thanks,
Pádraig

[backupfile-cleanup.patch (text/x-patch, attachment)]

Message #28 received at 23153 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: 23153 <at> debbugs.gnu.org, P <at> draigBrady.com, rishabhddave <at> gmail.com
Subject: Re: bug#23153: [PATCH]: For FIXME in cp.c
Date: Thu, 3 Nov 2016 23:44:08 +0100

On 11/03/2016 01:06 AM, Pádraig Brady wrote:
> Subject: [PATCH 1/2] maint: simplify handling of backup --suffix in various
> [...]
> Subject: [PATCH 2/2] maint: refactor printing of backup suffix

Nice one, thanks!

Have a nice day,
Berny

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.