Info-insert-dir

Info-insert-dir

[martin rudalics]

I fail to understand the following code in `Info-insert-dir':

     (let ((dirs (if Info-additional-directory-list
		    (append Info-directory-list
			    Info-additional-directory-list)
		  Info-directory-list))
       ...
       ;; Search the directory list for the directory file.
       (while dirs
           ....
	  (unless (cdr dirs)
	    (set (make-local-variable 'Info-dir-contents-directory)
		 (file-name-as-directory (car dirs))))
	  (setq dirs (cdr dirs))))

According to the doc-string of `Info-directory-list'

"the directory of Info files that come with Emacs
is put last (so that local Info files override standard ones)"

Apparently that's what the "unless ..." form relies upon to produce a
default-directory.  When `Info-additional-directory-list' is non-nil the
"append ..." form appends that list and the "unless ..." form will
return the last directory in `Info-additional-directory-list'.  If that
directory does not exist, the value of default-directory is nil.

Re: Info-insert-dir

[Richard Stallman]

    My last statement was incorrect.  The value of `default-directory' in
    the *info* buffer is not nil but a directory that doesn't exist.

This nonexistent directory came from `Info-additional-directory-list',
right?  So I think that if this is a bug, the bug would be at the
earlier stage, where the value of `Info-additional-directory-list' was
set up.  Do you disagree?

      More disconcerting,
    however, is that any part of Emacs that relies on a valid, existing
    default directory won't work reliably.

You can set `default-directory' to any string, so Emacs facilities
should not assume it is an existing directory unless it part of
their definition to need that.

Therefore, those other features such as spell checking should be
changed not to depend on this.  As far as I know, ispell and aspell
have no need to use the default directory.  Indeed one call-process
call in ispell.el already handles the case of nonexistent default
directory.

Does this patch make it work?


*** ispell.el	17 Feb 2007 15:40:35 -0500	1.208
--- ispell.el	18 Mar 2007 19:27:09 -0500	
***************
*** 895,901 ****
    (let* ((dictionaries
  	  (split-string
  	   (with-temp-buffer
! 	     (call-process ispell-program-name nil t nil "dicts")
  	     (buffer-string))))
  	 ;; Search for the named dictionaries.
  	 (found
--- 895,904 ----
    (let* ((dictionaries
  	  (split-string
  	   (with-temp-buffer
! 	     (let ((default-directory default-directory))
! 	       (unless (file-exists-p default-directory)
! 		 (setq default-directory (expand-file-name "~/")))
! 	       (call-process ispell-program-name nil t nil "dicts"))
  	     (buffer-string))))
  	 ;; Search for the named dictionaries.
  	 (found
***************
*** 928,934 ****
    "Return value of Aspell configuration option KEY.
  Assumes that value contains no whitespace."
    (with-temp-buffer
!     (call-process ispell-program-name nil t nil "config" key)
      (car (split-string (buffer-string)))))
  
  (defun ispell-aspell-find-dictionary (dict-name)
--- 931,940 ----
    "Return value of Aspell configuration option KEY.
  Assumes that value contains no whitespace."
    (with-temp-buffer
!     (let ((default-directory default-directory))
!       (unless (file-exists-p default-directory)
! 	(setq default-directory (expand-file-name "~/")))
!       (call-process ispell-program-name nil t nil "config" key))
      (car (split-string (buffer-string)))))
  
  (defun ispell-aspell-find-dictionary (dict-name)
***************
*** 1499,1509 ****
  	      (set-buffer output-buf)
  	      (erase-buffer)
  	      (set-buffer session-buf)
! 	      (setq status
! 		    (apply 'call-process-region (point-min) (point-max)
! 			   ispell-program-name nil
! 			   output-buf nil
! 			   "-a" "-m" ispell-args))
  	      (set-buffer output-buf)
  	      (goto-char (point-min))
  	      (save-match-data
--- 1505,1518 ----
  	      (set-buffer output-buf)
  	      (erase-buffer)
  	      (set-buffer session-buf)
! 	      (let ((default-directory default-directory))
! 		(unless (file-exists-p default-directory)
! 		  (setq default-directory (expand-file-name "~/")))
! 		(setq status
! 		      (apply 'call-process-region (point-min) (point-max)
! 			     ispell-program-name nil
! 			     output-buf nil
! 			     "-a" "-m" ispell-args)))
  	      (set-buffer output-buf)
  	      (goto-char (point-min))
  	      (save-match-data
***************
*** 2197,2209 ****
  	    (while (search-backward "*" nil t) (insert "."))
  	    (setq word (buffer-string))
  	    (erase-buffer))
! 	  (setq status (apply 'call-process prog nil t nil
! 			      (nconc (if (and args (> (length args) 0))
! 					 (list args)
! 				       (if look-p nil
! 					 (list "-e")))
! 				     (list word)
! 				     (if lookup-dict (list lookup-dict)))))
  	  ;; grep returns status 1 and no output when word not found, which
  	  ;; is a perfectly normal thing.
  	  (if (stringp status)
--- 2206,2221 ----
  	    (while (search-backward "*" nil t) (insert "."))
  	    (setq word (buffer-string))
  	    (erase-buffer))
! 	  (let ((default-directory default-directory))
! 	    (unless (file-exists-p default-directory)
! 	      (setq default-directory (expand-file-name "~/")))
! 	    (setq status (apply 'call-process prog nil t nil
! 				(nconc (if (and args (> (length args) 0))
! 					   (list args)
! 					 (if look-p nil
! 					   (list "-e")))
! 				       (list word)
! 				       (if lookup-dict (list lookup-dict))))))
  	  ;; grep returns status 1 and no output when word not found, which
  	  ;; is a perfectly normal thing.
  	  (if (stringp status)

Re: Info-insert-dir

[martin rudalics]

 > This nonexistent directory came from `Info-additional-directory-list',
 > right?  So I think that if this is a bug, the bug would be at the
 > earlier stage, where the value of `Info-additional-directory-list' was
 > set up.  Do you disagree?

Probably.  It's a customizable variable and `wid-edit' doesn't even
check if the user puts valid directory names in there.  Also, whether a
directory exists at the time of calling `call-process' may depend on a
number of factors - whether the directory is remote, a drive is
mounted...  I think it would be better if `Info-insert-dir' refrained
from using an obscure source such as `Info-additional-directory-list' to
set the default directory of its buffer.

 >       More disconcerting,
 >     however, is that any part of Emacs that relies on a valid, existing
 >     default directory won't work reliably.
 >
 > You can set `default-directory' to any string, so Emacs facilities
 > should not assume it is an existing directory unless it part of
 > their definition to need that.
 >
 > Therefore, those other features such as spell checking should be
 > changed not to depend on this.  As far as I know, ispell and aspell
 > have no need to use the default directory.  Indeed one call-process
 > call in ispell.el already handles the case of nonexistent default
 > directory.

You mean the

       (unless (file-exists-p default-directory)
	(setq default-directory (expand-file-name "~/")))

AFAICT this is the only occurrence of such a test in the entire Emacs
sources.  It might fail if default-directory equals the name of an
existing non-directory file - an unlikely case, though.

 > Does this patch make it work?

I'm not sure.  Before I already wrapped every occurrence of these in a

         (let ((default-directory
		(if (file-directory-p default-directory) default-directory "~/")))

including the `start-process' instance in ispell.el and it worked (BTW
`flyspell' has another instance of `call-process-region').  But isn't it
a bit tedious to search for all occurrences of `call-process' etc?
Couldn't such a check make it to `call-process', `start-process' ...

Re: Info-insert-dir

[Richard Stallman]

    Probably.  It's a customizable variable and `wid-edit' doesn't even
    check if the user puts valid directory names in there.

It shouldn't check that.  We do not try to stop users from putting
nonexistent directory names into Lisp variables.

	   (unless (file-exists-p default-directory)
	    (setq default-directory (expand-file-name "~/")))

    AFAICT this is the only occurrence of such a test in the entire Emacs
    sources.  It might fail if default-directory equals the name of an
    existing non-directory file - an unlikely case, though.

Perhaps it should verify that this is an existing directory.

      But isn't it
    a bit tedious to search for all occurrences of `call-process' etc?
    Couldn't such a check make it to `call-process', `start-process' ...

It would be incorrect to change call-process to change the default
directory on its own.  Often the execution of a command depends on the
current directory, so that if the current directory is nonexistent, an
error is the right thing.


We could make a subroutine which does this and calls call-process.

Re: Info-insert-dir

[martin rudalics]

 >     Probably.  It's a customizable variable and `wid-edit' doesn't even
 >     check if the user puts valid directory names in there.
 >
 > It shouldn't check that.  We do not try to stop users from putting
 > nonexistent directory names into Lisp variables.

I meant `wid-edit' doesn't even check whether it's a "directory name" in
the sense of a filename ending with a slash (`wid-edit' doesn't
discriminate directories and files although it provides separate widgets
for them).

 > It would be incorrect to change call-process to change the default
 > directory on its own.  Often the execution of a command depends on the
 > current directory, so that if the current directory is nonexistent, an
 > error is the right thing.

Agreed.

 > We could make a subroutine which does this and calls call-process.

What about `call-process-region', `start-process', `shell-command'?

Maybe we could introduce a macro like

(defmacro with-valid-default-directory (&rest body)
   "Run BODY with `default-directory' bound to an existing directory."
   (declare (indent 0) (debug t))
   `(let ((default-directory
	   (if (and (stringp default-directory)
		    (file-directory-p default-directory))
	       default-directory
	     (expand-file-name "~/"))))
      ,@body))

Re: Info-insert-dir

[Richard Stallman]

    I meant `wid-edit' doesn't even check whether it's a "directory name" in
    the sense of a filename ending with a slash (`wid-edit' doesn't
    discriminate directories and files although it provides separate widgets
    for them).

I don't think anything bad happens if you use a file name instead of a
directory name.  It is not "quite right", but it is ok.

It would be a pain in the neck for the `directory' widget to reject
directory file names.

It would be a nice little touch for the `directory' widget
to convert directory file names automatically into directory names.
But let's not change that now.

Re: Info-insert-dir

[Richard Stallman]

    What about `call-process-region', `start-process', `shell-command'?

    Maybe we could introduce a macro like

    (defmacro with-valid-default-directory (&rest body)

Is there anything in Emacs except ispell.el that needs to do this?

Re: Info-insert-dir

[martin rudalics]

 >     Maybe we could introduce a macro like
 >
 >     (defmacro with-valid-default-directory (&rest body)
 >
 > Is there anything in Emacs except ispell.el that needs to do this?

flyspell.el and maybe complete.el and man.el.  Most libraries have to
handle this in a special way.  comint.el, for example, also checks the
directory for accessibility.  Moreover, it's not clear whether such a
macro should unwind-protect body in order to restore the non-existing
default-directory after exiting abnormally.  Finally, macros add an
annoying extra indentation level.

Re: Info-insert-dir

[Richard Stallman]

flyspell.el can use the new ispell functions I just added.
I will take care of that.

complete.el does not seem to use call-process,
at least not that I can see.

man.el has handled it in a different way, but it is ot clear that it
has handled the job completely.  Would you like to check?

Re: Info-insert-dir

[martin rudalics]

 > complete.el does not seem to use call-process,
 > at least not that I can see.

`PC-expand-many-files' has this

   (with-current-buffer (generate-new-buffer " *Glob Output*")
     (erase-buffer)
     (when (and (file-name-absolute-p name)
                (not (file-directory-p default-directory)))
       ;; If the current working directory doesn't exist `shell-command'
       ;; signals an error.  So if the file names we're looking for don't
       ;; depend on the working directory, switch to a valid directory first.
       (setq default-directory "/"))
     (shell-command (concat "echo " name) t)

which should be OK (`with-temp-buffer' would be probably simpler).

 > man.el has handled it in a different way, but it is ot clear that it
 > has handled the job completely.  Would you like to check?

- The first two calls are OK:

(defun Man-init-defvars ()
   ...
   (let ((default-directory "/"))
            ...
	   ((eq 0 (call-process Man-sed-command nil nil nil Man-sysv-sed-script))
	   ...
	   ((eq 0 (call-process Man-sed-command nil nil nil Man-berkeley-sed-script))
	   ...

- The next is handled by `with-temp-buffer':

(defun Man-support-local-filenames ()
           ...
           (with-temp-buffer
             (and (equal (condition-case nil
                             (call-process manual-program nil t nil "--help")
                           (error nil))
                  ...

- The final ones should be OK too:

(defun Man-getpage-in-background (topic)
       ...
       (let ((process-environment (copy-sequence process-environment))
             ...
	    ;; Avoid possible error by using a directory that always exists.
	    (default-directory
	      (if (and (file-directory-p default-directory)
		       (not (find-file-name-handler default-directory
						    'file-directory-p)))
		  default-directory
		"/")))
              ...
	     (start-process manual-program buffer
                  ...
		 (call-process shell-file-name nil (list buffer nil) nil
                  ...

Re: Info-insert-dir

[Richard Stallman]

    - The next is handled by `with-temp-buffer':

    (defun Man-support-local-filenames ()
	       ...
	       (with-temp-buffer
		 (and (equal (condition-case nil
				 (call-process manual-program nil t nil "--help")
			       (error nil))

In what sense does the use of `with-temp-buffer' "handle" this issue?
New buffers copy the default directory from the current buffer, and I
don't see any code in `with-temp-buffer' to ensure the default
directory exists.

Re: Info-insert-dir

[martin rudalics]

[-- Attachment #1: Type: text/plain, Size: 267 bytes --]

> In what sense does the use of `with-temp-buffer' "handle" this issue?
> New buffers copy the default directory from the current buffer, and I
> don't see any code in `with-temp-buffer' to ensure the default
> directory exists.

Silly me.  Would the attached do it?

[-- Attachment #2: man.patch --]
[-- Type: text/plain, Size: 889 bytes --]

*** man.el	Tue Jan 23 07:40:06 2007
--- man.el	Fri Mar 23 09:54:48 2007
***************
*** 628,634 ****
      (setq Man-support-local-filenames
            (with-temp-buffer
              (and (equal (condition-case nil
!                             (call-process manual-program nil t nil "--help")
                            (error nil))
                          0)
                   (progn
--- 628,639 ----
      (setq Man-support-local-filenames
            (with-temp-buffer
              (and (equal (condition-case nil
! 			    (let ((default-directory
! 				    (if (and (file-directory-p default-directory)
! 					     (file-readable-p default-directory))
! 					default-directory
! 				      (expand-file-name "~/"))))
! 			      (call-process manual-program nil t nil "--help"))
                            (error nil))
                          0)
                   (progn

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Info-insert-dir

[Richard Stallman]

Would you please install your man.el patch?