bug#79446: 9.7: cp(1): Unnecessary writes / incorrect logging messages

On 21/09/2025 06:30, Paul Eggert wrote:
> On 2025-09-20 14:34, Kye Hunter wrote:
> 
>> I don't
>> think there's any need to look beyond the most intuitive implementation
>> of the --update=older option: If the timestamps on the target are newer
>> than the timestamps on the source, don't do anything.
> 
> We're talking only mtime, right? --update looks only at mtime, not at
> atime or ctime or whatever.
> 
>> I'm only seeing extra calls to chmod being applied to directories
> 
> That's as per the documentation: --update affects only non-directories.
> 
>> /* Avoid calling chown if we know it's not necessary.  */ , so it seems reasonable that the behavior of preserving the mode should be the same.
> 
> Sure, but the problem here is that it appears cp doesn't know whether
> calling chmod is necessary. If deciding whether it's necessary is likely
> more expensive than just doing the chmod, cp might as well just do the
> chmod.
> 
>> Again, to make this more concrete let's consider this code:
>> touch foo
>> cp -l foo bar
>> mkdir qux
>> cp --preserve=links --update=older --target-directory="./qux" *
>> strace cp --preserve=links --update=older --target-directory="./qux" *
>>
>> Here, with the --update=older option, it seems unexpected that ./qux/foo
>> and ./qux/bar are being relinked the second time they are copied from
>> ".", when what the manual describes is "--update[=UPDATE]    control
>> which existing files are updated" and "'older' [...] results in files
>> being replaced if they're older than the corresponding source file."
> 
> --update affects only how non-directories are processed. In this
> particular case, non-directories are indeed skipped; only the directory
> qux is updated. So 'cp' is conforming to its documentation.
> 
> You're right that 'cp' could avoid some relinking if it kept track of
> st_dev and st_ino for all destinations it's already updated. But 'cp'
> would need to spend more RAM to do that, no? That could be a significant
> burden in a recursive copy.
> 
>> Code to show the current behavior of cp for scenario 2:
> 
> Unfortunately I'm not following that scenario or its description (it's
> fairly long....).
> 
>> This line even
>> comes with the helpful comment /* Note we currently replace DST_NAME
>> unconditionally, even if it was a newer separate file.  */ , so actually
>> this behavior is somehow intentional, but I don't see any justification
>> given for this exception to the expected behavior of --update=older or
>> --no-preserve=links.
> 
> Nor do I. It seems a clear bug, or at least a problem. However, there
> clearly was intent here: see Pádraig's commit
> 2aea1828a1aab158f68cccf3eac408203889021e dated 2011-07-27 which puts
> into tests/cp/preserve/link the comment "a separate newer file in dest
> will be overwritten!" indicating that the behavior you subscribe is at
> least surprising.
> 
> I would be inclined to change the behavior to be the way you suggest.
> This would simplify cp and make it a bit faster, and would make its
> behavior easier to explain. However we're close to a release right now
> and we should wait for Pádraig's opinion as he wrote the code (and the
> test case) in question.
> 
> Thanks for helping me think through all this.

Yes this seems a bit hairy to change just before (hopefully tomorrow's) release.
As for the code/test, it's a while ago, but the exclamation mark by me
suggests the code change was a refactor, the behavior was surprising,
but worth adding a test for documentation at least. I.e. the behavior
could be changed IMHO (I've not thought about this much TBH).

cheers,
Padraig

cheers,
Padraig