Re: master 65108998b1e: docview: imenu: check return value of 'mutool'

[Eli Zaretskii]

> From: Arash Esbati <arash@gnu.org>
> Cc: "Morgan J. Smith" <Morgan.J.Smith@outlook.com>
> Date: Thu, 27 Jul 2023 20:49:27 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > branch: master
> > commit 65108998b1ee0b42b57d478ba3b51f9040f04cd4
> > Author: Morgan Smith <Morgan.J.Smith@outlook.com>
> > Commit: Eli Zaretskii <eliz@gnu.org>
> >
> >     docview: imenu: check return value of 'mutool'
> >     
> >     While 'mutool' supports many filetypes, 'mutool show' only
> >     supports PDF files.  This would lead to cryptic imenu errors
> >     when opening other
> >     file types (like EPUB) since we would parse the error output.
> >     During my testing this caused 'imenu--index-alist' to have a
> >     value of '(nil).
> >     
> >     * lisp/doc-view.el (doc-view--pdf-outline): Error when 'mutool'
> >     returns an error.  Use 'call-process' to get the return value and
> >     remove the call to 'shell-quote-argument' as 'call-process'
> >     doesn't want any escapes.
> >     (doc-view-mode): Handle possible error from 'doc-view-imenu-setup'.
> >     (doc-view-imenu-enabled): Remove superfluous (and ... t).
> >     (doc-view-imenu-setup): Remove check for mutool already ensured by
> >     'doc-view-imenu-enabled' being non-nil.
> >     (Bug#64516)
> > ---
> >  lisp/doc-view.el | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/lisp/doc-view.el b/lisp/doc-view.el
> > index b14655fb274..847601872f5 100644
> > --- a/lisp/doc-view.el
> > +++ b/lisp/doc-view.el
> > @@ -147,6 +147,8 @@
> >  (require 'filenotify)
> >  (eval-when-compile (require 'subr-x))
> >  
> > +(autoload 'imenu-unavailable-error "imenu")
> > +
> >  ;;;; Customization Options
> >  
> >  (defgroup doc-view nil
> > @@ -214,7 +216,7 @@ are available (see Info node `(emacs)Document View')."
> >    :type 'boolean
> >    :version "30.1")
> >  
> > -(defcustom doc-view-imenu-enabled (and (executable-find "mutool") t)
> > +(defcustom doc-view-imenu-enabled (executable-find "mutool")
> >    "Whether to generate an imenu outline when \"mutool\" is available."
> >    :type 'boolean
> >    :version "29.1")
> 
> I'm not familiar with the code, so my apologies if this comment is off,
> but the change above seems wrong.  With this change,
> `doc-view-imenu-enabled' becomes a string or nil which doesn't match the
> boolean type.  The (and ... t) part wasn't superfluous.

This was okayed by Tassilo, so I'm adding him to this discussion.