bug#50630: [PATCH] Add tests for insert-directory

[John Cummings <john@rootabega.net>]

Eli Zaretskii wrote:

>> Date: Sat, 25 Sep 2021 11:38:08 +0000
>> From: John Cummings <john@rootabega.net>
>> Cc: 50630@debbugs.gnu.org
>>
>> Eli Zaretskii <eliz@gnu.org> wrote:
>>
>> >> Date: Fri, 24 Sep 2021 19:58:25 +0000
>> >> From: John Cummings <john@rootabega.net>
>> >>
>> >> Along those lines, I also attempted to skip the test when ls-lisp
>> >> would be used during files-tests.el, which I predict might happen when
>> >> building on Windows?
>> >
>> > Why did you need to skip those tests?  Can you elaborate?
>>
>> ls-lisp has its own implementation of insert-directory, which
>> duplicates the bug, and which will also duplicate the fix when I
>> submit it soon.
>
> Some context is probably lost here: which bug are you talking about?

50630. A breakdown of the context from other messages in this thread
that've driven my responses so far:

 I found that list-directory in files.el reported the free space of
 default-directory instead of the function's DIRNAME argument. The fix I
 submitted addressed that in insert-directory in files.el, so it
 seems like people are OK with treating this as a problem in
 insert-directory. i.e. the same incorrect free space was reported
 whether you called list-directory or insert-directory.

 I and others found that the same problem was duplicated in ls-lisp.el
 and tramp-sh.el, and I provided instructions to reproduce attached
 earlier in the thread:
 https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-09/msg01813.html
 You can confirm the bug in tramp-sh.el and ls-lisp.el against master
 (currently c17eded545) , and you can confirm the bug in files.el
 against revision a462ccd536 and earlier.

 I've now submitted a regression test for 50630, which I thought would
 be appropriate to include in the 50630 context. I also asked
 elsewhere in the thread whether we shouldn't also handle the instances
 of the bug in ls-lisp.el and tramp-sh.el in that same bug instead of
 creating additional bugs for what is essentially the same small bug
 duplicated a few times. But it's just a question, and if the leaders
 want to have one bug for every library, I'll understand.

> And if that bug will be fixed in ls-lisp, then why do you need to
> single ls-lisp out?

I want to avoid situations where someone runs files-tests.el and it
actually tests the insert-directory implementation from ls-lisp.el. My
plan is to try to reuse, or at worst, copy the testing helpers that I
already submitted for files.el to perform the same test scenario in
ls-lisp-tests.el and in an appropriate place in the tramp tests.
But as I mentioned in another reply, I also understand that making
files-tests.el have to worry about this might not be appropriate, and
if there's another way to handle that, I'd be glad to hear about it
and will be researching that myself, too.


>> I admit I may not correctly or completely understand the way that
>> ls-lisp could relate to a batch ert run at build time, or how that
>> varies between architectures.
>
> Whenever you invoke insert-directory, on MS-Windows it will invoke
> ls-lisp.

That aligns with my reasoning, and so someone running the 50630
regression test for files.el on MS-Windows would actually be testing
ls-lisp.el, and would see inaccurate regression status for files.el.
As of today, the regression test (files-tests-bug-50630) would fail,
and I wouldn't want to have me adding this test be a reason for
any failing MS-Windows builds or other control gates. In the future,
anyone running that test on MS-Windows wouldn't see a failure, but
they still wouldn't have tested the code that files-tests.el is
intended to test.


>> I personally find the variable a little confusing,
>> and think that negating its name and meaning would be more natural,
>> but the documentation does make it clear, even if I have to think
>> about it for a bit every time I set it.
>
> You are talking about ls-lisp-use-insert-directory-program?  Why it is
> confusing?  ls-lisp by default doesn't use any programs, it accesses
> the files' attributes using Emacs Lisp APIs; so the "program" part is
> a reference to the fact that on Posix platforms insert-directory uses
> the 'ls' program instead.

I only mentioned that because I thought it might have been a reason
why it was hard to understand what I was doing with that variable in
files-tests.el. The reason I find it confusing, which could be
personal to me, is that I think it's more natural that a library's
config variables would need to be non-nil when wanting to enable a
feature or function of that library. So a name like
'ls-lisp-use-ls-lisp-insert-directory', and inverting the values,
would have been easier for me to remember, and simpler to express:

  (and (featurep 'ls-lisp)
       ls-lisp-use-ls-lisp-insert-directory)

would mean that we're using the ls-lisp implementation.