On 19/03/2021 10:50, Kyle Meyer wrote: > Maxim Nikulin writes: > A few comments in addition to Eli's advice to drop the > (eq system-type 'gnu/linux) condition... Feel free to commit the change suggested in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=44824#82 instead of this patch. >> +(defun org--error-process-sentinel (proc event) >> + "Show a message if process failed (exited with non-zero code >> +or killed by a signal. Pass the function as :SENTINEL argument > > Please rework the first sentence so that it fits on the first line, > though I'd be in favor dropping the function and using a lambda in the > make-process call. My impression is that org-open-file function is already too long and complex. Another reason to use standalone function is that I am unsure if elisp compiler and interpreter are smart enough to reuse single instance of lambda. I was afraid that every opened file caused creation of new sentinel possibly with a closure containing chain of stack frames. On the other hand even in worst case memory footprint is negligible in comparison to any GUI viewer. >> + (unless (string-match "finished" event) > > There's no need for substring matching, right? So it could be > > (equal event "finished\n") I was surprised by final "\n" that is not always suitable and I was in doubts concerning its stability. I would prefer something like (starts-with event "finished") Certainly match-data is not necessary, so even match-string-p is better. > (when (and (memq (process-status proc) '(exit signal)) > (/= (process-exit-status proc) 0)) Thank you, I was too lazy to implement such kind of check myself. Certainly this variant is better. I hope, I have addressed other your comments in the updated patch.