bug#50852: [PATCH] Fix search of the look program.

bug#50852: [PATCH] Fix search of the look program.

[André A. Gomes]

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

Hi Emacs,

Here's a more robust way to handle the existence of the look program by
ispell.  GNU Guix users will be happy.

There's another aspect worth discussing.  The look program doesn't have
the -r flag (as of today), but ispell handles this case (look at
ispell-look-options).  It seems that look was published around 1979.
Perhaps it's time to deprecate this flag?

Thank you.


--
André A. Gomes
"Free Thought, Free World"

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-search-of-the-look-program.patch --]
[-- Type: text/x-patch, Size: 1446 bytes --]

From 5a42784837845fa718bd80fd878c88c09d331a5b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20A=2E=20Gomes?= <andremegafone@gmail.com>
Date: Mon, 27 Sep 2021 21:27:26 +0300
Subject: [PATCH] Fix search of the look program.

Not all distributions follow the FHS standard.  For instance, in GNU
Guix the look program lives at /run/current-system/profile/bin/.

* lisp/textmodes/ispell.el: Fix logic concerning the existence of the
look program.
---
 lisp/textmodes/ispell.el | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index b650ab3871..65da617e07 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -250,16 +250,12 @@ Always stores Fcc copy of message when nil."
 Should probably be \"-Ei\"."
   :type 'string)
 
-(defcustom ispell-look-command
-  (cond ((file-exists-p "/bin/look") "/bin/look")
-	((file-exists-p "/usr/local/bin/look") "/usr/local/bin/look")
-	((file-exists-p "/usr/bin/look") "/usr/bin/look")
-	(t "look"))
+(defcustom ispell-look-command (executable-find "look")
   "Name of the look command for search processes.
 This must be an absolute file name."
   :type 'file)
 
-(defcustom ispell-look-p (file-exists-p ispell-look-command)
+(defcustom ispell-look-p ispell-look-command
   "Non-nil means use `look' rather than `grep'.
 Default is based on whether `look' seems to be available."
   :type 'boolean)
-- 
2.33.0

bug#50852: [PATCH] Fix search of the look program.

[Stefan Kangas]

André A. Gomes <andremegafone@gmail.com> writes:

> Here's a more robust way to handle the existence of the look program by
> ispell.  GNU Guix users will be happy.

LGTM.  I do wonder if this shouldn't be done in many more places...

> There's another aspect worth discussing.  The look program doesn't have
> the -r flag (as of today), but ispell handles this case (look at
> ispell-look-options).  It seems that look was published around 1979.
> Perhaps it's time to deprecate this flag?

Does that apply to all versions of look or just the GNU one?

bug#50852: [PATCH] Fix search of the look program.

[Lars Ingebrigtsen]

André A. Gomes <andremegafone@gmail.com> writes:

> Here's a more robust way to handle the existence of the look program by
> ispell.  GNU Guix users will be happy.

Thanks; applied to Emacs 28 (but I made the ispell-look-p change more
defensive, in case somebody has set ispell-look-command in their init
file).

> There's another aspect worth discussing.  The look program doesn't have
> the -r flag (as of today), but ispell handles this case (look at
> ispell-look-options).  It seems that look was published around 1979.
> Perhaps it's time to deprecate this flag?

Do you know when the "-r" flag disappeared?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50852: [PATCH] Fix search of the look program.

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Mon, 27 Sep 2021 16:56:14 -0700
> Cc: 50852@debbugs.gnu.org
> 
> André A. Gomes <andremegafone@gmail.com> writes:
> 
> > Here's a more robust way to handle the existence of the look program by
> > ispell.  GNU Guix users will be happy.
> 
> LGTM.  I do wonder if this shouldn't be done in many more places...

Careful: that will subtly change the behavior.  For starters,
executable-find looks in exec-path, which includes Emacs-specific
directories.  It also has its own ideas about which files are
executable and whether to try some extensions.  And finally,
executable-find could return nil, whereas the original value was never
nil, which could cause some code to signal an error, and requires the
user to redefine the value when a program was originally absent, but
is then installed without restarting Emacs.

I'm not saying these differences are for the worse, but they need to
be carefully considered when making such "innocent" changes, as they
could introduce subtle bugs and misfeatures, at least potentially.

In this case, why not simply have the value as "look" with no leading
directories?  If the program is installed, it will be found when it's
invoked, and if it isn't installed, the user gets an error message at
that time.

bug#50852: [PATCH] Fix search of the look program.

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> In this case, why not simply have the value as "look" with no leading
> directories?  If the program is installed, it will be found when it's
> invoked, and if it isn't installed, the user gets an error message at
> that time.

That's a good point.  And the ispell-look-p variable is itself rather
odd -- why have both ispell-look-command and that variable (with a
non-standard name)?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50852: [PATCH] Fix search of the look program.

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Stefan Kangas <stefan@marxist.se>,  andremegafone@gmail.com,
>   50852@debbugs.gnu.org
> Date: Tue, 28 Sep 2021 08:11:18 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > In this case, why not simply have the value as "look" with no leading
> > directories?  If the program is installed, it will be found when it's
> > invoked, and if it isn't installed, the user gets an error message at
> > that time.
> 
> That's a good point.  And the ispell-look-p variable is itself rather
> odd -- why have both ispell-look-command and that variable (with a
> non-standard name)?

Right.  The existence of "look" should IMO be tested as part of
ispell-lookup-words, not when the package loads.  Then the command
could decide whether to use "look" or Grep at that time, and the need
for the ispell-look-p variable would disappear.

bug#50852: [PATCH] Fix search of the look program.

[André A. Gomes]

Stefan Kangas <stefan@marxist.se> writes:

>> There's another aspect worth discussing.  The look program doesn't have
>> the -r flag (as of today), but ispell handles this case (look at
>> ispell-look-options).  It seems that look was published around 1979.
>> Perhaps it's time to deprecate this flag?
>
> Does that apply to all versions of look or just the GNU one?

I don't think there's a GNU one, since it's not part of the GNU
coreutils.

Here's what the man page say:

--8<---------------cut here---------------start------------->8---
The look utility appeared in Version 7 AT&T Unix.

The look command is part of the util-linux  package  and  is  available
       from https://www.kernel.org/pub/linux/utils/util-linux/.
--8<---------------cut here---------------end--------------->8---

From the first sentence I inferred that it appeared in 1979.

I had a look at the "look" program both on my GNU/Linux machine and on a
modern Mac laptop.  Both have the same "look", none have the "-r" flag.


-- 
André A. Gomes
"Free Thought, Free World"

bug#50852: [PATCH] Fix search of the look program.

[André A. Gomes]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Cc: Stefan Kangas <stefan@marxist.se>,  andremegafone@gmail.com,
>>   50852@debbugs.gnu.org
>> Date: Tue, 28 Sep 2021 08:11:18 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > In this case, why not simply have the value as "look" with no leading
>> > directories?  If the program is installed, it will be found when it's
>> > invoked, and if it isn't installed, the user gets an error message at
>> > that time.
>> 
>> That's a good point.  And the ispell-look-p variable is itself rather
>> odd -- why have both ispell-look-command and that variable (with a
>> non-standard name)?
>
> Right.  The existence of "look" should IMO be tested as part of
> ispell-lookup-words, not when the package loads.  Then the command
> could decide whether to use "look" or Grep at that time, and the need
> for the ispell-look-p variable would disappear.

You're both right.  I was actually silly.  I can prepare a patch
following these ideas.


-- 
André A. Gomes
"Free Thought, Free World"

bug#50852: [PATCH] Fix search of the look program.

[André A. Gomes]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> André A. Gomes <andremegafone@gmail.com> writes:
>
>> Here's a more robust way to handle the existence of the look program by
>> ispell.  GNU Guix users will be happy.
>
> Thanks; applied to Emacs 28 (but I made the ispell-look-p change more
> defensive, in case somebody has set ispell-look-command in their init
> file).

Makes sense.  I was naive.

>> There's another aspect worth discussing.  The look program doesn't have
>> the -r flag (as of today), but ispell handles this case (look at
>> ispell-look-options).  It seems that look was published around 1979.
>> Perhaps it's time to deprecate this flag?
>
> Do you know when the "-r" flag disappeared?

That's a good question.  As mentioned above, look is the same in both
MacOS and GNU/Linux.

The -r flag stands for regexp.  It seems odd that a binary search
program like look would support regexp.

My intuition tells me that once upon a time there was a programme named
look whose behaviour resembles that of grep.  But what does a kid like
me know about Unix and the dinosaurs anyway? :)

I'd deprecate support for this odd flag.  But ispell-lookup-words should
definitely use look, by default, instead of grep.   


-- 
André A. Gomes
"Free Thought, Free World"

bug#50852: [PATCH] Fix search of the look program.

[Eli Zaretskii]

> From: André A. Gomes <andremegafone@gmail.com>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  stefan@marxist.se,
>   50852@debbugs.gnu.org
> Date: Tue, 28 Sep 2021 11:41:38 +0300
> 
> I can prepare a patch following these ideas.

I think it will be appreciated, thanks.

bug#50852: [PATCH] Fix search of the look program.

[Stefan Kangas]

André A. Gomes <andremegafone@gmail.com> writes:

> I don't think there's a GNU one, since it's not part of the GNU
> coreutils.
>
> Here's what the man page say:
>
> --8<---------------cut here---------------start------------->8---
> The look utility appeared in Version 7 AT&T Unix.
>
> The look command is part of the util-linux  package  and  is  available
>        from https://www.kernel.org/pub/linux/utils/util-linux/.
> --8<---------------cut here---------------end--------------->8---
>
> From the first sentence I inferred that it appeared in 1979.
>
> I had a look at the "look" program both on my GNU/Linux machine and on a
> modern Mac laptop.  Both have the same "look", none have the "-r" flag.

Maybe it's better to make all that stuff obsolete, indeed.

But I do wonder why the "-r" parameter was added.  It seems like it
depends on the `ispell-have-new-look' variable, added in the initial
revision in 1994 (commit 007852e00dd8).

bug#50852: [PATCH] Fix search of the look program.

[Stefan Kangas]

André A. Gomes <andremegafone@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Lars Ingebrigtsen <larsi@gnus.org>
>>> Cc: Stefan Kangas <stefan@marxist.se>,  andremegafone@gmail.com,
>>>   50852@debbugs.gnu.org
>>> Date: Tue, 28 Sep 2021 08:11:18 +0200
>>>
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>
>>> > In this case, why not simply have the value as "look" with no leading
>>> > directories?  If the program is installed, it will be found when it's
>>> > invoked, and if it isn't installed, the user gets an error message at
>>> > that time.
>>>
>>> That's a good point.  And the ispell-look-p variable is itself rather
>>> odd -- why have both ispell-look-command and that variable (with a
>>> non-standard name)?
>>
>> Right.  The existence of "look" should IMO be tested as part of
>> ispell-lookup-words, not when the package loads.  Then the command
>> could decide whether to use "look" or Grep at that time, and the need
>> for the ispell-look-p variable would disappear.
>
> You're both right.  I was actually silly.  I can prepare a patch
> following these ideas.

It seems like we are still waiting for this patch.  So here's a friendly
reminder to take a look at this when you find some time.  :-)

Thanks in advance.

bug#50852: [PATCH] Fix search of the look program.

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Maybe it's better to make all that stuff obsolete, indeed.

I've now done so in Emacs 29.