Hi Noam, Thanks for the comments. How does this look? From 99e290cec79754d8d92ec6dcf3c58594782a677b Mon Sep 17 00:00:00 2001 From: Kaushal Modi Date: Wed, 5 Apr 2017 17:16:33 -0400 Subject: [PATCH] Check that file argument is a string * lisp/vc/ediff-diff.el (ediff-exec-process): Check that the argument passed to `file-local-copy' is a string (Bug#26378). Also fix the existing comment for this function, and convert it to its doc-string. --- lisp/vc/ediff-diff.el | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/lisp/vc/ediff-diff.el b/lisp/vc/ediff-diff.el index cfa08ef360..3b2a85afdd 100644 --- a/lisp/vc/ediff-diff.el +++ b/lisp/vc/ediff-diff.el @@ -1134,12 +1134,22 @@ ediff-setup-diff-regions3 )) -;; Execute PROGRAM asynchronously, unless OS/2, Windows-*, or DOS, or unless -;; SYNCH is non-nil. BUFFER must be a buffer object, and must be alive. The -;; OPTIONS arg is a list of options to pass to PROGRAM. It may be a blank -;; string. All elements in FILES must be strings. We also delete nil from -;; args. (defun ediff-exec-process (program buffer synch options &rest files) + "Execute the diff PROGRAM. + +The PROGRAM output is sent to BUFFER, which must be a live buffer +object. + +The PROGRAM is executed asynchronously unless `system-type' is +`windows-nt' or `ms-dos', or unless SYNCH is non-nil. + +OPTIONS is a string of space-separated options to pass to PROGRAM. It +may be a blank string. + +FILES is a list of filenames to pass to PROGRAM. This list may +contain a nil element too. + +nil and \"\" elements in OPTIONS and FILES are ignored." (let ((data (match-data)) ;; If this is a buffer job, we are diffing temporary files ;; produced by Emacs with ediff-coding-system-for-write, so @@ -1151,8 +1161,9 @@ ediff-exec-process args) (setq args (append (split-string options) (mapcar (lambda (file) - (file-name-unquote - (or (file-local-copy file) file))) + (when (stringp file) + (file-name-unquote + (or (file-local-copy file) file)))) files))) (setq args (delete "" (delq nil args))) ; delete nil and "" from arguments ;; the --binary option, if present, should be used only for buffer jobs -- 2.11.0 On Wed, Apr 5, 2017 at 7:21 PM wrote: > Now that we're looking at the whole thing, I have a few more comments. > > Kaushal Modi writes: > > > + "Execute the diff PROGRAM. > > + > > +The PROGRAM output is sent to BUFFER, which must be a buffer object, > > +and must be alive. > > It would flow a bit better to say "which must be a live buffer object". > I agree. > > + > > +The PROGRAM is executed asynchronously unless the OS is OS/2, > > +Windows-*, or DOS, or unless SYNCH is non-nil. > > I don't see any reference to OS/2 in the code, so I think we should just > drop it. Makes sense. > And I'm not sure what the "-*" in "Windows-*" means; It means Windows-95, Windows NT, Windows 8, Windows 10, etc. > I think > we should just say "unless `system-type' is `windows-nt' or `ms-dos'". > I am fine with that. > > + > > +OPTIONS is a list of options to pass to PROGRAM. It may be a blank > > +string. > > This seems to be wrong, OPTIONS is not a list. It should say "OPTIONS > is a string of space-separated options to pass to PROGRAM". > You are right; making that change. > > + > > +An element in FILES must be either a string or nil. > > + > > +We also delete nil and \"\" from all arguments." > > I think these last 2 sentences should be combined into something like: > > "FILES is a list of filenames to pass to PROGRAM; nil and \"\" elements > are ignored." > I have tweaked this part slightly (see patch in this email). Based on the code, nil and "" elements will be removed from a concatenated list of OPTIONS and FILES; not just FILES. -- Kaushal Modi