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

From: Eli Zaretskii <[email-masked]>
Date: Sat, 09 Aug 2025 16:27:11 +0300

> From: Lockywolf <[email-masked]>
> Cc: Lockywolf <[email-masked]>,  [email-masked]
> Date: Sat, 09 Aug 2025 19:49:19 +0800
> 
> > So this is intended to fail the version check because fake_aspell
> > script ignores the -vv switch?  If so, why calling Emacs would not do,
> > since Emacs also doesn't support -vv?
> 
> This is intended to succeed, because "fake_aspell.bash", irrespective of
> the options, behaves in the way real aspell behaves when being called
> with "-vv".

If there's no way of using some program that we are sure will be
installed, please use skip-unless to skip the test when Bash is not
available.

> >> >> >> >> +    (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.
> >
> > But it also recognizes the absolute file names that start with a drive
> > letter, which is the full form of an absolute name on Windows.
> 
> No it does not. But even if it did... (see the next paragraph)...
> 
> > So
> > using expand-file-name will not fail the test, would it?  Moreover, we
> > cannot expect all Windows ports of the various spellers to do what
> > Ispell does, and it is IMO wrong to rely on specific details of a
> > specific port in the test suite.  What if in some not-too-distant
> > future this test will be also used for Aspell or Hunspell, which might
> > not consider "/foo/bar" and absolute file name on Windows?
> 
> If you look at the test carefully once again, you will see that it does
> the following
> #+begin_src emacs-lisp
> (insert
>        "hello\n\n\n"
>        "<!-- " ispell-dictionary-keyword test-dict " -->"
>        "<!-- " ispell-pdict-keyword test-pdict " -->")
> #+end_src
> 
> This test runs the test on a buffer, which is such as if it is written
> by a user.  Surely we can expect files having
> "Local IspellPersDict: /tmp/nonexistent.dict" in the footer,
> being _written_ by SOME users, by _opened_on_Windows_ by OTHER users,
> don't you think?  Don't you think that such files are expected to be
> processed gracefully by Emacs, regardless of whether such paths are
> absolute, relative, nonexistent, or even UNC?  Moreover, a user might
> even put something like:
> "Local IspellPersDict: (my-best-personal-dictionary)",
> or
> "Local IspellPersDict: https://cgit.git.savannah.gnu.org/cgit/miscfiles.git/tree/web2"
> (Such structures are not supported, and should be ignored, or warned
> about by ~ispell.el~.)
> This is "user I/O", and tests, at least usually, are not expected to
> receive graceful input, they are expected to make sure that the code
> works reasonably even on garbage input.

Look, when I see "/foo/bar" file names in Emacs sources, it
immediately draws my attention, because such file names cause
portability problems.  So, unless using expand-file-name and/or
temporary-file-directory here is somehow problematic and causes
trouble, please do use them, so that this doesn't stand out, okay?

> >> 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.
> >
> > You always remember every line of code you wrote 20+ years ago?
> 
> > And anyway, the important point here is not to rely on such fine
> > details of the implementation, but instead to go with the general
> > conventions of the target platform.  Because if the application
> > doesn't behave according to those conventions, it's a bug in the
> > application that needs to be fixed there.
> >
> 
> Fair enough, but overall, the convention of Windows (or any systemn) is
> to not fail on text files having some UNIX paths in them.  I can add a
> test having "C:\TMP\nonexistent.pdict", to make sure that all platforms
> are covered, if you'd like.

See above: I just want these Unix-style absolute file name not to
stand out.

Thanks.