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

[Rishabh Dave]

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 <address@hidden> 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
>

FIXME-regarding-getenv.patch
Description: Text Data