On 12/05/2024 21:31, Pádraig Brady wrote:
> On 12/05/2024 16:06, Paul Eggert wrote:
>> On 2024-05-12 04:49, Pádraig Brady wrote:
>>
>>> @@ -1151,7 +1151,8 @@ main (int argc, char **argv)
>>>                    {
>>>                      /* Default cp operation.  */
>>>                      x.update = false;
>>> -                  x.interactive = I_UNSPECIFIED;
>>> +                  if (x.interactive != I_ASK_USER)
>>> +                    x.interactive = I_UNSPECIFIED;
>>>                    }
>>>                  else if (update_opt == UPDATE_NONE)
>>>                    {
>>> @@ -1166,7 +1167,8 @@ main (int argc, char **argv)
>>>                  else if (update_opt == UPDATE_OLDER)
>>>                    {
>>>                      x.update = true;
>>> -                  x.interactive = I_UNSPECIFIED;
>>> +                  if (x.interactive != I_ASK_USER)
>>> +                    x.interactive = I_UNSPECIFIED;
>>
>> Thanks for looking into this messy area. Here is a comment from another
>> pair of eyes.
>>
>> Could you elaborate a bit more about why these two bits of code change
>> x.interactive at all? That is, why doesn't update_opt simply affect
>> x.update? Why does update_opt bother to override a previous setting of
>> x.interactive to I_ALWAYS_YES, I_ALWAYS_NO, or I_ALWAYS_SKIP?
>>
>> Another way to put it: shouldn't x.update simply reflect the value of
>> the --update option, whereas x.interactive reflects reflects whether -f,
>> -i, -n are used? Although this would require changes to copy.c, it'd
>> make the code easier to follow.
> 
> I agree that some refactoring would be good here.
> At least x.update should be renamed to x.update_older.
> 
> As interactive selection, and file dates all relate
> to selecting which files to update, it's tempting to conflate the settings.
> However you're right that this introduces complexities when
> trying to avoid all inconsistencies. Currently for example:
>     $ cp -v -i --update=none new old  # Won't prompt as expected
>     $ cp -v --update=none -i new old  # Unexpectedly ignores update option
> 
> So yes we should separate these things.
> 
>> Another way to put it: why should, for example, --update=all override a
>> previous -f or (partly) -n but not a previous -i?
> 
> Right -f is significant for mv here (for completeness -f for cp is a separate thing).
> I.e. we need to treat I_ALWAYS_YES specially in mv with the current scheme.
> 
> BTW -n is not overridden by any --update option currently,
> and this change effectively applies the same logic to -i now.

The attached patch set should address this.
Marking this as done.
I'll push the attached tomorrow.

cheers,
Pádraig