Re: [PATCH] org-macs.el: Do not compare wall time and file modification time

[Max Nikulin <manikulin@gmail.com>]

On 13/05/2022 05:52, Paul Eggert wrote:
> On 5/12/22 09:55, Max Nikulin wrote:
> 
>> +    (unless (file-exists-p file)
>> +      (error "File to tangle does not exist: %s" file))
>> +    (when (file-newer-than-file-p file tangled-file)
>>        (org-babel-tangle-file file ...
> 
> file-newer-than-file-p succeeds only if FILE exists, so in that case 
> it'd be a bit more efficient to avoid testing FILE's existence again, e.g.:
> 
>     (cond
>       ((file-newer-than-file-p file tangled-file)
>        (org-bable-tangle-file file ...))
>       ((not (file-exists-p file))
>        (error "File to tangle does not exist: %s" file)))

My opinion is that performance improvement is negligible while negative 
impact related to code readability is noticeable. Maybe it is just 
because elisp is not my favorite language.

Feel free to commit your variant though, I will not object, but I am not 
going to update my patch in this way as well.

P.S. Since I believe it should be fixed in the bugfix branch, I tried to 
keep changes as minimal as possible.

I am not sure which kind of code I would prefer to see. I do not like 
`cond' with 2 branches. Even nested ifs are a bit better while still to 
"heavy"

     (if (not (file-newer-than-file-p file tangled-file))
         (unless (file-exists-p file)
           (error ...))
       (org-babel-tangle-file ...)) ; intentionally put below to the 
else branch

Maybe I would prefer something like file-target-is-up-to-date-p 
predicate that returns nil if target does not exist. For some reason 
`org-babel-tangle-file' silently ignores missed file neither signalling 
a error nor returning a special value. Delegating error handling to 
`org-babel-tangle-file' would allow to get consistent error messages.

In the previous version of the patch (with inline implementation of file 
modification time comparison) the suggested optimization was quite 
natural, with `file-newer-than-file-p' it is trade off with code 
readability.