Package: emacs;
Reported by: Lockywolf <for_emacs_1 <at> lockywolf.net>
Date: Tue, 5 Aug 2025 14:10:03 UTC
Severity: normal
Tags: patch
Found in version 31.0.50
Message #35 received at 79177 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Lockywolf <for_emacs_1 <at> lockywolf.net> Cc: 79177 <at> debbugs.gnu.org Subject: Re: bug#79177: 31.0.50; [PATCH] [v4] Add tests to ispell.el Date: Sat, 09 Aug 2025 16:27:11 +0300
> From: Lockywolf <for_emacs_1 <at> lockywolf.net> > Cc: Lockywolf <for_emacs_1 <at> lockywolf.net>, 79177 <at> debbugs.gnu.org > 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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.