Subject: Re: bug#79177: 31.0.50; [PATCH] [v4] Add tests to ispell.el

From: Lockywolf <[email-masked]>
Date: Fri, 08 Aug 2025 10:21:37 +0800

Eli Zaretskii <[email-masked]> writes:

>> From: Lockywolf <[email-masked]>
>> Cc: Lockywolf <[email-masked]>,  [email-masked]
>> Date: Thu, 07 Aug 2025 17:59:06 +0800
>> 
>> Eli Zaretskii <[email-masked]> writes:
>> 
>> >> From: Lockywolf <[email-masked]>
>> >> Cc: Lockywolf <[email-masked]>,  [email-masked]
>> >> Date: Thu, 07 Aug 2025 16:07:54 +0800
>> >> 
>> >> >> diff --git a/test/lisp/textmodes/ispell-resources/fake-aspell.bash b/test/lisp/textmodes/ispell-resources/fake-aspell.bash
>> >> >> new file mode 100755
>> >> >> index 00000000000..4406a18a22e
>> >> >> --- /dev/null
>> >> >> +++ b/test/lisp/textmodes/ispell-resources/fake-aspell.bash
>> >> >> @@ -0,0 +1,2 @@
>> >> >> +#!/bin/bash
>> >> >> +printf  '%s\n' "@(#) International Ispell Version 3.1.20 (but really Aspell 0.59.800)"
>> >> >
>> >> > I asked to use Emacs instead of a Bash script?  If that causes
>> >> > problems, let's discuss that, okay?
>> >> 
>> >> Aspell needs to parse the "-vv" command-line argument, and Emacs
>> >> launched in batch mode cannot do that.  Or I did not manage to make it.
>> >
>> > Sorry, I don't understand.  The Bash script just outputs a fixed
>> > string, and Emacs is capable of doing that.  What did you try, and if
>> > it didn''t work, how it failed to work?
>> 
>> No, it does not /just/ output a fixed string. It also _ignores_ the
>> command-line argument "-vv", which, if passed to emacs --batch, produces
>> and error Error: error ("Unknown option ‘-vv’"), and ispell.el does it
>> when initialising the backend.
>
> I feel there's some misunderstanding here.  What I suggested is that
> instead of doing this:
>
>   (let ((fake-aspell-path (expand-file-name
>                            "./fake-aspell.bash"
>                            tests-ispell-data-directory)))
>     (chmod fake-aspell-path 504)
>     (call-process fake-aspell-path nil nil nil)
>
> where fake-aspell.bash does
>
>   printf  '%s\n' "@(#) International Ispell Version 3.1.20 (but really Aspell 0.59.800)"
>
> the code does this (not tested):
>
>   (call-process "emacs" nil nil nil "-Q" "--batch" "--eval" "(princ \\"@(#) International Ispell Version 3.1.20 (but really Aspell 0.59.800)\\")")
>
> For best results, use
>
>   (expand invocation-name invocation-directory)
>
> instead of a literal "emacs".  This makes sure you invoke the same
> Emacs as the one running the test, not some other version.

I am not calling "fake_aspell.bash" with "(call-process fake-aspell-path
nil nil nil)" in the name of printing a string.  I am calling it in
order to make sure that calling it works on the target system.  Read the
test further on, the important lines of the test are
```
(setopt ispell-program-name fake-aspell-path)
(ispell-check-version t)
```
where ispell.el will call "fake_aspell.bash" in the way it likes calling
backend executables.

Maybe I should wrap it in (skip-unless), but I remember it behaving
strangely within a "let".

>> >> >> +    (let ((test-dict "nonexistent")
>> >> >> +          (test-pdict "/tmp/lnonexistent.txt"))
>> >> >                          ^^^^^^^^^^^^^^^^^^^^^^^
>> >> > Likewise here: please use
>> >> >
>> >> >    (expand-file-name "lnonexistent.txt" temporary-file-directory)
>> >> >
>> >> > instead.  (There are several other places in the patch with the same
>> >> > problem.)
>> >> 
>> >> This is not a file path.  This is just a string which looks like a file
>> >> path.  If something tries to access this as if it is a path during the
>> >> execution of the test, this is an error.
>> >
>> > OK, but the file name you use is not an absolute file name on Windows,
>> > so it could cause problems.  Is there a problem with using my
>> > suggestion, even though you don't need a real file name?
>> 
>> ispell's man page says the following:
>> 
>> ```
>>  The  -p  option  is  used  to  specify an alternate personal dictionary file.  If the file name does not begin with "/", $HOME is prefixed.
>> ```
>> 
>> I do not want ispell to do any mangling of the "HOME" directory if this
>> "string which looks like a path" is by chance (if a test malfunctions)
>> passed to ispell binary and it creates a bogus file in the home
>> directory or overwrites something.
>> On windows "temporary-file-directory" probably does not begin with "/".
>
> Yes, but a Windows port of ispell will know that, and detect absolute
> file names as appropriate: by looking at the drive letter, as in
> "d:/foo".  On the contrary, using file names that begin with a slash
> on Windows is likely to fail the test mentioned by the man page.

No, it will not fail.  Citing tree.c from ispell's source:
```c
        /*
        ** Figure out if p is an absolute path name.  Note that beginning
        ** with "./" and "../" is considered an absolute path, since this
        ** still means we can't prepend HOME.
        */
        abspath = IS_SLASH (*p)  ||  strncmp (p, "./", 2) == 0
          ||  strncmp (p, "../", 3) == 0;
#ifdef MSDOS
        /*
        ** DTRT with drive-letter braindamage and with backslashes.
        */
        abspath |= strncmp (p, ".\\", 2) == 0
          || strncmp (p, "..\\", 3) == 0
          || (p[0] != '\0'  &&  p[1] == ':');
#endif
```

That is it, all of it.
There is no #ifdef WINDOWS block, and even the MSDOS block only "adds"
to the absolute path check, so things starting with "/" are always
considered absolute.

In fact, quoting the README (line 258) from the ispell root:

```
Although the ispell maintainer does not support MS-DOS and
Windows, a generous contributor, Eli Zaretskii, has added MS-DOS
support.  You can build ispell for MS-DOS with either EMX/GCC or
DJGPP.  See the file pc/README for compilation instructions.
```

I am surprised that you are asking me this question.

-- 
Your sincerely,
Vladimir Nikishkin (MiEr, lockywolf)
(Laptop)