Subject: bug#62572: cp --no-clobber behavior has changed

From: Pádraig Brady <[email-masked]>
Date: Sun, 2 Apr 2023 11:10:19 +0100

On 01/04/2023 23:44, Paul Eggert wrote:
> On 2023-04-01 08:44, Pádraig Brady wrote:
> 
>> OK first stab at --update=none support is attached.
> 
> Thanks, some comments:
> 
> 
>> +  /* Always Overwrite.  */
>> +  UPDATE_OVERWRITE,
> 
> Might be better to call this UPDATE_ALL as it doesn't overwrite if you
> use cp -l or -s or (in some cases) --preserve=links.

+1

>> +static char const *const update_type_string[] =
>> +{
>> +  "none", "older", NULL
>> +};
> 
> Perhaps there should also be an --update=all, which is the same as no
> update option at all? It would presumably override previous --update
> suboptions.

+1

>>     -n, --no-clobber             do not overwrite an existing file (overrides\n\
>> -                                 a previous -i option)\n\
>> +                                 a -u or previous -i option)\n\
> 
> Shouldn't -n override only previous -u or --update options? I thought
> these options were processed left to right, with later overriding earlier.

Well I was just keeping the existing global treatment of -n wrt -u, as per:
https://github.com/coreutils/coreutils/commit/7e244891b
-n does seem like more of a global "make sure you don't overwrite anything" setting
(which is analogous to the shell "noclobber" option BTW which also induces a failure when triggered).
I'll think a bit more about this.

>> +  -u                           only update when the SOURCE file is newer\n\
>>                                    than the destination file or when the\n\
>>                                    destination file is missing\n\
>> +  --update[=UPDATE_MODE]       like -u, but support skipping existing files;\n\
>> +                                 UPDATE_MODE={none,older(default)}\n\
> 
> This might be clearer if we describe --update first, and then say that
> -u is equivalent to plain --update.

+1

>> +  if (make_backups && x.interactive == I_ALWAYS_SKIP)
>> +    {
>> +      error (0, 0,
>> +             _("options --backup and --update=none are mutually exclusive"));
>> +      usage (EXIT_FAILURE);
>> +    }
> 
> Why is this needed? --backup and -u are not mutually exclusive now.
> Admittedly --backup is useless when -u is given, but it seems a bit
> harsh to error out.

Yes --backup is redundant with -n or --update=none.
We already disallowed --backup with -n,
so I was just being consistent with disallowing --backup with --update=none.
I'll remove this new restriction.

thanks for the review,
Pádraig