bug#50630: 28.0.50; list-directory shows free space for current directory, not the specified one

bug#50630: 28.0.50; list-directory shows free space for current directory, not the specified one

[John Cummings]

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

Hello, long time emacs user; first time reporter. Apologies if I do anything wrong here.

The verbose output for list-directory always shows the free space for the current directory of the buffer where it was called, not the target directory that is supplied as an argument.

This may have been first introduced in 849051903e7, while fixing this report:
https://lists.gnu.org/archive/html/bug-gnu-emacs/2002-03/msg00251.html
The result was that, while the *Directory* output buffer's current directory was set correctly, setting it last meant that the free space calculation always took place in "." rather than the argument to list-directory.

Here are commands/output to demonstrate. Note that the free space changes for the same directory argument when I change the current directory, since the two different directories are on different disks.


  (eval-expression (pwd) t)
  "Directory ~/emacs/"

  (eval-expression (list-directory "~" t) t)
  "/home/emacsdev/"

  (insert-buffer-substring "*Directory*")
  Directory /home/emacsdev
  total used in directory 0 available 15.5 GiB
  lrwxrwxrwx 1 /emacsdev /emacsdev 26 Sep 16 17:45 emacs -> /media/ubuntu/turkey/emacs
  lrwxrwxrwx 1 /emacsdev /emacsdev 34 Sep 16 17:44 emacs-project -> /media/ubuntu/turkey/emacs-project


  (cd "/")

  (eval-expression (pwd) t)
  "Directory /"

  (eval-expression (list-directory "~" t) t)
  "/home/emacsdev/"

  (insert-buffer-substring "*Directory*")
  Directory /home/emacsdev
  total used in directory 0 available 7.7 GiB
  lrwxrwxrwx 1 /emacsdev /emacsdev 26 Sep 16 17:45 emacs -> /media/ubuntu/turkey/emacs
  lrwxrwxrwx 1 /emacsdev /emacsdev 34 Sep 16 17:44 emacs-project -> /media/ubuntu/turkey/emacs-project

Changing the call to get-free-disk-space seemed to work in the attached patch, but I haven't attempted to dig deeper and check for any regressions. I would be glad to do that, but would need more time to research and would probably ask for more help to do it.

Thanks for everything!



In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.20, cairo version 1.16.0)
 of 2021-09-10 built on ubuntu
Repository revision: ceb60225bacc7650b5e52032c0c33b9d67f9a6d7
Repository branch: master
System Description: Ubuntu 20.04.1 LTS

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG
LIBSELINUX LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG SECCOMP SOUND
THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM XPM GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg rfc6068 epg-config gnus-util rmail
rmail-loaddefs auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json map time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils subr-x pcase
misearch multi-isearch vc-git diff-mode easy-mmode vc-dispatcher
bug-reference find-func cl-extra shortdoc text-property-search seq
mule-diag thingatpt help-fns radix-tree help-mode cl-loaddefs cl-lib
term/xterm xterm byte-opt gv bytecomp byte-compile cconv iso-transl
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget hashtable-print-readable backquote threads dbusbind
inotify dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 99674 10407)
 (symbols 48 8011 1)
 (strings 32 25795 958)
 (string-bytes 1 846999)
 (vectors 16 13457)
 (vector-slots 8 152638 6184)
 (floats 8 90 749)
 (intervals 56 1088 62)
 (buffers 992 16))



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-text-verbose-dir-fix.patch --]
[-- Type: text/x-patch; name=0001-text-verbose-dir-fix.patch, Size: 770 bytes --]

From 87dbf73c209e70382db64c2d196b761d656df409 Mon Sep 17 00:00:00 2001
From: emacs dev <noone@example.com>
Date: Thu, 16 Sep 2021 18:49:43 -0400
Subject: [PATCH] text verbose dir fix

---
 lisp/files.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/files.el b/lisp/files.el
index 7e4bdab507..ef0c4ac458 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -7608,7 +7608,7 @@ insert-directory
 		  ;; Replace "total" with "total used in directory" to
 		  ;; avoid confusion.
 		  (replace-match "total used in directory" nil nil nil 1)
-		  (let ((available (get-free-disk-space ".")))
+		  (let ((available (get-free-disk-space file)))
 		    (when available
 		      (end-of-line)
 		      (insert " available " available))))))))))
-- 
2.25.1

bug#50630: 28.0.50; list-directory shows free space for current directory, not the specified one

[Eli Zaretskii]

> Date: Thu, 16 Sep 2021 23:00:50 +0000
> From: John Cummings <john@rootabega.net>
> 
> Hello, long time emacs user; first time reporter. Apologies if I do anything wrong here.

Thanks.  In the future, please make sure Git uses your actual email
address, as without that we need to apply the patch by hand.  Also, we
request a commit log message formatted according to ChangeLog rules;
see CONTRIBUTE for the details.  (I made the necessary changes for you
this time.)

Also, if you intend to contribute in the future, I recommend starting
your legal paperwork rolling for assigning copyright to the FSF.  If
you agree, I will send you the form to fill and the instructions to go
with it.

> Changing the call to get-free-disk-space seemed to work in the attached patch, but I haven't attempted to dig deeper and check for any regressions. I would be glad to do that, but would need more time to research and would probably ask for more help to do it.

The patch makes sense to me, so I installed it, and I'm closing this
bug.

Thanks.

bug#50630: 28.0.50; list-directory shows free space for current directory, not the specified one

[John Cummings]

On Friday, September 17th, 2021 at 3:07 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> Thanks. In the future,
...
>
> Also, if you intend to contribute in the future, I recommend starting
>
> your legal paperwork rolling for assigning copyright to the FSF. If
>
> you agree, I will send you the form to fill and the instructions to go
>
> with it.

That sounds great, please do send me the form.

Sorry, I expected to have to do some more research and change a few other things; just intended that patch to be a proof of concept. Thank you for correcting it for me. I will do some deeper research as well as follow up with a test.

Thanks!

bug#50630: 28.0.50; list-directory shows free space for current directory, not the specified one

[Eli Zaretskii]

> Date: Fri, 17 Sep 2021 10:24:30 +0000
> From: John Cummings <john@rootabega.net>
> Cc: 50630-done@debbugs.gnu.org
> 
> On Friday, September 17th, 2021 at 3:07 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> Sorry, I expected to have to do some more research and change a few other things; just intended that patch to be a proof of concept. Thank you for correcting it for me. I will do some deeper research as well as follow up with a test.

It looked like TRT to me, but of course more testing will be welcome.

Thanks!

bug#50630: [External] : bug#50630: 28.0.50; list-directory shows free space for current directory, not the specified one

[Drew Adams]

> The patch makes sense to me, so I installed it, and I'm closing this
> bug.

I believe the same change is needed in ls-lisp.el, for function `ls-lisp--insert-directory'.

  (let ((available (get-free-disk-space ".")))

should be:

  (let ((available (get-free-disk-space file)))

(But you might want to check whether FILE is what's needed, and not, say, ORIG-FILE.)

bug#50630: [External] : bug#50630: 28.0.50; list-directory shows free space for current directory, not the specified one

[Eli Zaretskii]

> From: Drew Adams <drew.adams@oracle.com>
> CC: "50630-done@debbugs.gnu.org" <50630-done@debbugs.gnu.org>
> Date: Fri, 17 Sep 2021 17:38:42 +0000
> Accept-Language: en-US
> 
> > The patch makes sense to me, so I installed it, and I'm closing this
> > bug.
> 
> I believe the same change is needed in ls-lisp.el, for function `ls-lisp--insert-directory'.
> 
>   (let ((available (get-free-disk-space ".")))
> 
> should be:
> 
>   (let ((available (get-free-disk-space file)))

Why, do you see the wrong values being reported?

bug#50630: [External] : bug#50630: 28.0.50; list-directory shows free space for current directory, not the specified one

[John Cummings]

On Friday, September 17th, 2021 at 2:04 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Drew Adams drew.adams@oracle.com
> > I believe the same change is needed in ls-lisp.el, for function `ls-lisp--insert-directory'.
> >
> > (let ((available (get-free-disk-space ".")))
> >
> > should be:
> >
> > (let ((available (get-free-disk-space file)))
>
> Why, do you see the wrong values being reported?

The values look wrong to me here. Thanks for the heads up. I've got a working test for files.el and ls-lisp.el, and after I polish it, I'll also look into how to handle FILE vs ORIG-FILE

bug#50630: [External] : bug#50630: 28.0.50; list-directory shows free space for current directory, not the specified one

[Eli Zaretskii]

> Date: Fri, 17 Sep 2021 20:32:53 +0000
> From: John Cummings <john@rootabega.net>
> Cc: Drew Adams <drew.adams@oracle.com>, 50630-done@debbugs.gnu.org
> 
> On Friday, September 17th, 2021 at 2:04 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > > From: Drew Adams drew.adams@oracle.com
> > > I believe the same change is needed in ls-lisp.el, for function `ls-lisp--insert-directory'.
> > >
> > > (let ((available (get-free-disk-space ".")))
> > >
> > > should be:
> > >
> > > (let ((available (get-free-disk-space file)))
> >
> > Why, do you see the wrong values being reported?
> 
> The values look wrong to me here.

In my testing, they reflect the correct volume (because the call is in
the buffer whose default-directory is set to the directory we are
going to display), which is why I installed the changes.  Please tell
how you test, starting from "emacs -Q", because I'd like to look at
this.

> Thanks for the heads up. I've got a working test for files.el and ls-lisp.el, and after I polish it, I'll also look into how to handle FILE vs ORIG-FILE

Thanks.

bug#50630: [External] : bug#50630: 28.0.50; list-directory shows free space for current directory, not the specified one

[John Cummings]

On Saturday, September 18th, 2021 at 1:56 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> > The values look wrong to me here.
>
> In my testing, they reflect the correct volume (because the call is in
> the buffer whose default-directory is set to the directory we are
> going to display), which is why I installed the changes. Please tell
> how you test, starting from "emacs -Q", because I'd like to look at
> this.

Here's a dirty manual way to confirm it. I just chose directories that
I know are on different volumes, but I also confirmed it works with a loopback
filesystem created with something like losetup. This worked, i.e. showed me different
free space, with emacs -Q on a clean build of c1356d3a05a789555c327dd4cb0f444fdf7be205


;;doesn't matter; just use an existing dir
(setq ld-target "~")

;;two dirs on different volumes
(setq ld-working-dirs '("/" "~/emacs/"))


(require 'ls-lisp)
;;make sure to use the ls-lisp functionality
(setq ls-lisp-use-insert-directory-program nil)


;;shows the list-directory output for all working dirs
(defun insert-ld-bug ()
  (dolist (ld-working-dir ld-working-dirs)
    (let ((default-directory ld-working-dir))
      (list-directory ld-target t)
      (insert-buffer-substring "*Directory*")
      (insert "=====\n\n\n"))))

(with-current-buffer-window "ld-bug" nil nil
  (insert-ld-bug))

bug#50630: Confirmation of instances in ls-lisp.el and tramp-sh.el

[John Cummings]

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

I think I've found all the instances of this bug: files.el (already
fixed) and ls-lisp.el (already identified) and tramp-sh.el. I attached
recipes to reproduce against master @
0646c6817139aa905a2f6079fdc82eb4be944de0.  The preceding libraries are
the 3 implementations of insert-directory I found. From what I can
tell, these are usually expected to be called from dired and
list-directory, the former of which ensures default-directory is set
as expected before calling insert-directory, and the latter of which
does not, which is where this was initially reported.

Testing these in a real scenario doesn't show any other interesting
behaviors.  Invoking via dired always does the correct thing, and
invoking via list-directory, or calling insert-directory directly,
always does the wrong thing (i.e. shows the free space of
default-directory instead of the insert-directory file arg) when the
default-directory does not match the file argument. This is true when
either/both of the insert-directory arg or the default-directory are
standard paths or tramp paths.

I think the fixes for the remaining two libraries are going to be as
simple as the previous fix: pass file-system-info the actual file arg
instead of ".". While ensuring default-directory is set as we want
seems to allow "." to work correctly, I haven't found a concrete
advantage to that over just passing the file arg through. (e.g. some
path expansion that only file-system-info can do.) I'm not very
confident that I understand that space fully, though.

Though the fixes may be simple, the tests for even one of the
libraries would be large enough to require my completed copyright
assignment, which is in progress now. The approach I have working now
is just to stub out file-system-info to simulate returning different
free space for different paths and verifying
list-directory/insert-directory shows the matching free space. Anybody
see any big problems with that or have other suggestions?  I also
don't want to duplicate that test for each of the 3 libraries, but I
haven't found a good way to share test utilities like that yet.

[-- Attachment #2: 50630-recipe.txt --]
[-- Type: text/plain, Size: 2381 bytes --]

Recipes to confirm bug 50630
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=50630

These will show two buffers with a directory listing for the same directory, but with different available space depending on the default-directory when executing.

On Windows, I confirmed the bug using two different drive letters for pwd-a and pwd-b. But my main testing was doing all 3 of these on GNU/Linux. On Windows, ls-lisp will probably always be used unless it's disabled.

for lisp/files.el (already fixed):

emacs -Q --eval "\
(progn \
   (setq pwd-a \"/\") \
   (setq pwd-b \"/proc\")  \
   (setq list-dir \"~\") \
   (with-current-buffer-window \"a\" nil nil \
      (cd pwd-a) \
      (insert-directory list-dir \"-l\" nil t) \
      (insert (format \"listing of %s in cwd %s\" list-dir (pwd)))) \
   (with-current-buffer-window \"b\" nil nil \
      (cd pwd-b) \
      (insert-directory list-dir \"-l\" nil t) \
      (insert (format \"listing of %s in cwd %s\" list-dir (pwd)))) \
   (switch-to-buffer \"a\") \
   (switch-to-buffer-other-window \"b\"))"


for lisp/ls-lisp.el:

emacs -Q --eval "\
(progn \
   (setq pwd-a \"/\") \
   (setq pwd-b \"/proc\")  \
   (setq list-dir \"~\") \
   (require 'ls-lisp) \
   (setq ls-lisp-use-insert-directory-program nil) \
   (with-current-buffer-window \"a\" nil nil \
      (cd pwd-a) \
      (insert-directory list-dir \"-l\" nil t) \
      (insert (format \"listing of %s in cwd %s\" list-dir (pwd)))) \
   (with-current-buffer-window \"b\" nil nil \
      (cd pwd-b) \
      (insert-directory list-dir \"-l\" nil t) \
      (insert (format \"listing of %s in cwd %s\" list-dir (pwd)))) \
   (switch-to-buffer \"a\") \
   (switch-to-buffer-other-window \"b\"))"


for lisp/net/tramp-sh.el:

emacs -Q --eval "\
(progn \
   (setq pwd-a \"/\") \
   (setq pwd-b \"/proc\")  \
   (setq list-dir \"/ssh:emacsdev@localhost:~/\") \
   (require 'tramp) \
   (setq ls-lisp-use-insert-directory-program t) \
   (with-current-buffer-window \"a\" nil nil \
      (cd pwd-a) \
      (insert-directory list-dir \"-l\" nil t) \
      (insert (format \"listing of %s in cwd %s\" list-dir (pwd)))) \
   (with-current-buffer-window \"b\" nil nil \
      (cd pwd-b) \
      (insert-directory list-dir \"-l\" nil t) \
      (insert (format \"listing of %s in cwd %s\" list-dir (pwd)))) \
   (switch-to-buffer \"a\") \
   (switch-to-buffer-other-window \"b\"))"

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

[John Cummings]

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

The attached patch adds a regression test for this bug, as well as a
couple other basic functionality tests for insert-directory. This does
not test the other unfixed cases still in ls-lisp.el and tramp-sh.el,
but I thought it prudent to get these tests in before trying to apply
them in other libraries.

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?

I sent in my copyright assignment, but I think it's still being
finalized.

Thank you!

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-tests-for-insert-directory.patch --]
[-- Type: text/x-patch; name=0001-Add-tests-for-insert-directory.patch, Size: 6612 bytes --]

From 213243a73871f365c910a6680a894907a497bbee Mon Sep 17 00:00:00 2001
From: John Cummings <john@rootabega.net>
Date: Fri, 24 Sep 2021 15:36:23 -0400
Subject: [PATCH] Add tests for 'insert-directory'

Add tests for 'insert-directory' base functionality and regression tests for
the issue where free space was reported for the current directory instead of
the target of 'list-directory' (Bug#50630).
* test/lisp/files-tests.el: Add 'insert-directory' tests.
* test/lisp/files-resources/insert-directory/: Create directories and files to
use for testing 'insert-directory'.
---
 .../insert-directory/test_dir/bar             |  0
 .../insert-directory/test_dir/foo             |  0
 .../insert-directory/test_dir_other/bar       |  0
 .../insert-directory/test_dir_other/foo       |  0
 test/lisp/files-tests.el                      | 79 +++++++++++++++++++
 5 files changed, 79 insertions(+)
 create mode 100644 test/lisp/files-resources/insert-directory/test_dir/bar
 create mode 100644 test/lisp/files-resources/insert-directory/test_dir/foo
 create mode 100644 test/lisp/files-resources/insert-directory/test_dir_other/bar
 create mode 100644 test/lisp/files-resources/insert-directory/test_dir_other/foo

diff --git a/test/lisp/files-resources/insert-directory/test_dir/bar b/test/lisp/files-resources/insert-directory/test_dir/bar
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/test/lisp/files-resources/insert-directory/test_dir/foo b/test/lisp/files-resources/insert-directory/test_dir/foo
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/test/lisp/files-resources/insert-directory/test_dir_other/bar b/test/lisp/files-resources/insert-directory/test_dir_other/bar
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/test/lisp/files-resources/insert-directory/test_dir_other/foo b/test/lisp/files-resources/insert-directory/test_dir_other/foo
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index b283a512a4..61c5a13546 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -26,6 +26,7 @@
 (require 'bytecomp) ; `byte-compiler-base-file-name'.
 (require 'dired) ; `dired-uncache'.
 (require 'filenotify) ; `file-notify-add-watch'.
+(require 'map)
 
 ;; Set to t if the local variable was set, `query' if the query was
 ;; triggered.
@@ -1774,6 +1775,84 @@ files-tests-save-buffers-kill-emacs--asks-to-save-buffers
          ;; `save-some-buffers-default-predicate' (i.e. the 2nd element) is ignored.
          (nil save-some-buffers-root ,nb-might-save))))))
 
+;;insert-directory output tests
+(let* ((data-dir "insert-directory")
+       (test-dir (file-name-as-directory
+                  (ert-resource-file
+                   (concat data-dir "/test_dir"))))
+       (test-dir-other (file-name-as-directory
+                        (ert-resource-file
+                         (concat data-dir "/test_dir_other"))))
+       (test-files `(,test-dir "foo" "bar")) ;expected files to be found
+       ;Free space test data for `insert-directory'.
+       ;Meaning: (path free-space-bytes-to-stub expected-free-space-string)
+       (free-data `((,test-dir 10 "available 10 B")
+                    (,test-dir-other 100 "available 100 B")
+                    (:default 999 "available 999 B")))
+       ;;Try to verify that insert-directory will come from files.el,
+       ;;not ls-lisp.el. Windows builds will probably use ls-lisp.el
+       ;;by default, which will invalidate some tests.
+       (insert-directory-program-used (or (not (featurep 'ls-lisp))
+                                          ls-lisp-use-insert-directory-program)))
+
+
+  (defun files-tests--look-up-free-data (path)
+    "Look up free space test data, with a default for unspecified paths."
+    (let ((path (file-name-as-directory path)))
+      (cdr (or (assoc path free-data)
+               (assoc :default free-data)))))
+
+  (defun files-tests--make-file-system-info-stub (&optional static-path)
+    "Return a stub for `file-system-info' using dynamic or static test data.
+If that data should be static, pass STATIC-PATH to choose which
+path's data to use."
+    (lambda (path)
+      (let* ((path (cond (static-path)
+                         ;; file-system-info knows how to handle ".", so we
+                         ;; do the same thing
+                         ((equal "." path) default-directory)
+                         (path)))
+             (return-size
+              (car (files-tests--look-up-free-data path))))
+        (list return-size return-size return-size))))
+
+  (defun files-tests--insert-directory-output (dir &optional verbose)
+    "Run `insert-directory' and return its output."
+    (with-current-buffer-window "files-tests--insert-directory" nil nil
+      (insert-directory dir "-l" nil t)
+      (buffer-substring-no-properties (point-min) (point-max))))
+
+  (ert-deftest files-tests-insert-directory-shows-files ()
+    "Verify `insert-directory' reports the files in the directory."
+    (let* ((test-dir (car test-files))
+           (files (cdr test-files))
+           (output (files-tests--insert-directory-output test-dir)))
+      (dolist (file files)
+        (should (string-match-p file output)))))
+
+  (defun files-tests--insert-directory-shows-given-free (dir &optional info-func)
+    "Run `insert-directory' and verify it reports the correct available space.
+Stub `file-system-info' to ensure the available space is consistent,
+either with the given stub function or a default one using test data."
+    (cl-letf (((symbol-function 'file-system-info)
+               (or info-func
+                   (files-tests--make-file-system-info-stub))))
+      (should (string-match-p (cadr
+                               (files-tests--look-up-free-data dir))
+                              (files-tests--insert-directory-output dir t)))))
+
+  (ert-deftest files-tests-insert-directory-shows-free ()
+    "Test that verbose `insert-directory' shows the correct available space."
+    (files-tests--insert-directory-shows-given-free
+     test-dir
+     (files-tests--make-file-system-info-stub test-dir)))
+
+  (ert-deftest files-tests-bug-50630 ()
+    "Verify verbose `insert-directory' shows free space of the target directory.
+The current directory at call time should not affect the result (Bug#50630)."
+    (skip-unless insert-directory-program-used)
+    (let ((default-directory test-dir-other))
+      (files-tests--insert-directory-shows-given-free test-dir))))
 
 (provide 'files-tests)
 ;;; files-tests.el ends here
-- 
2.25.1

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

[Lars Ingebrigtsen]

John Cummings <john@rootabega.net> writes:

> I sent in my copyright assignment, but I think it's still being
> finalized.

Skimming the patch, it looks good to me, but as you say, we have to wait
until the copyright process is finished.

But this bug report has been closed as fixed, so to avoid your patch
being lost, can you resubmit it with `M-x submit-emacs-patch' so that it
gets its own bug number?

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

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

[Eli Zaretskii]

> 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?

> +       ;;Try to verify that insert-directory will come from files.el,
> +       ;;not ls-lisp.el. Windows builds will probably use ls-lisp.el
> +       ;;by default, which will invalidate some tests.
> +       (insert-directory-program-used (or (not (featurep 'ls-lisp))
> +                                          ls-lisp-use-insert-directory-program)))

I guess this is part of the same question I asked above?  Because I
don't think I understand what you are trying to do here and why.

Thanks.

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

[John Cummings]

Lars Ingebrigtsen <larsi@gnus.org> wrote:

> But this bug report has been closed as fixed, so to avoid your patch
> being lost, can you resubmit it with `M-x submit-emacs-patch' so that it
> gets its own bug number?

I'd be glad to do that, but before I do, let me make sure we're all on
the same page. Earlier in the discussion, it was revealed that this
same bug is present in ls-lisp.el and tramp-sh.el, and I plan to
submit the trivial fixes for those next. (Let me know if the recipes
to confirm those bugs I attached earlier don't work for you.) The
cause of, and fix for, all of them are essentially the same, just
located in 3 different places. Would you prefer to have a bug number
for each of the places where it occurs, or to reopen this one (if
that's something that's done) to hold all the fixes for essentially
the same bug, as well as for the regression tests linked to those
fixes?

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

[John Cummings]

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.  (Let me know if the recipe to confirm the bug is
present in ls-lisp.el attached earlier doesn't work for you.)  So if
ls-lisp was "active" during this test, it could be a false
positive/negative for this test, depending on whether ls-lisp had been
fixed yet. (See my reply to your later question in this message for
what I mean when I say ls-lisp is "active", because just requiring the
library isn't enough.)

On my GNU/Linux system, ls-lisp is not active when running this test
from the Makefile, but I don't want to assume that that is true of
every system. Since Windows GNU Emacs uses ls-lisp.el by default when
running, I thought it might be one of those exceptions at build time
as well. I confirmed that it's also true if someone runs this test
interactively after having activated ls-lisp, regardless of the
system. That is, I activated ls-lisp, eval'd files-tests.el, and ran
ert for that file, and the test failed because it used the buggy
insert-directory from ls-lisp. So even if building a Windows Emacs (or
any system) does not use ls-lisp by default, is there any harm in
having this skip condition (other than potentially confusing the
reader)?

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. If it's NOT something to worry about
during a 'make', would I still need to worry about someone who ran
this test after enabling ls-lisp manually? Perhaps I could fail the
test so the user knows what they did didn't make sense?

Also, note that this is only true for the 50630 regression test. That
may have been the wrong choice. Perhaps it would make sense to run any
test of insert-directory's implementation if and only if ls-lisp.el is
not active?

>> +       ;;Try to verify that insert-directory will come from files.el,
>> +       ;;not ls-lisp.el. Windows builds will probably use ls-lisp.el
>> +       ;;by default, which will invalidate some tests.
>> +       (insert-directory-program-used (or (not (featurep 'ls-lisp))
>> +                                          ls-lisp-use-insert-directory-program)))

> I guess this is part of the same question I asked above?  Because I
> don't think I understand what you are trying to do here and why.

Yes, my logic was that this being non-nil means the test will be using
the Emacs-default insert-directory implementation from files.el, and
not the ls-lisp.el one.  In order to use the ls-lisp one (what I've
referred to as ls-lisp being "active" previously), the ls-lisp library
needs to be loaded, and the variable on the second line needs to be
nil, otherwise ls-lisp will just delegate back to files.el for
insert-directory. 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.

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

[Michael Albinus]

John Cummings <john@rootabega.net> writes:

Hi John,

>> But this bug report has been closed as fixed, so to avoid your patch
>> being lost, can you resubmit it with `M-x submit-emacs-patch' so that it
>> gets its own bug number?
>
> I'd be glad to do that, but before I do, let me make sure we're all on
> the same page. Earlier in the discussion, it was revealed that this
> same bug is present in ls-lisp.el and tramp-sh.el, and I plan to
> submit the trivial fixes for those next. (Let me know if the recipes
> to confirm those bugs I attached earlier don't work for you.) The
> cause of, and fix for, all of them are essentially the same, just
> located in 3 different places. Would you prefer to have a bug number
> for each of the places where it occurs, or to reopen this one (if
> that's something that's done) to hold all the fixes for essentially
> the same bug, as well as for the regression tests linked to those
> fixes?

For Tramp (I'm the maintainer) I don't care whether it is a separate bug
number, or not. If you want to add/extend a regression test for the
Tramp case, you might check whether tramp-test17-insert-directory in
test/lisp/net/tramp-tests.el is a proper place.

Best regards, Michael.

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

[John Cummings]

Michael Albinus <michael.albinus@gmx.de> wrote:

> For Tramp (I'm the maintainer) I don't care whether it is a separate bug
> number, or not.

Thanks Michael, I appreciate the guidance and flexibility. I see how
what looks like a single simple bug could end up lumping together a
bunch of otherwise unrelated maintainers or contributors, so if
separate bugs turns out to be the decision, so be it. I will certainly
keep this in mind if I run into a bug which is larger or more complex,
or can't be fixed quickly enough to make it into the same release.

> If you want to add/extend a regression test for the
> Tramp case, you might check whether tramp-test17-insert-directory in
> test/lisp/net/tramp-tests.el is a proper place.

The test17 test(s) came up on my radar while I was trying to measure
the extent of the bug. I will make sure that I give good consideration
to possibly extending those tests instead of only adding new
ones. From what I remember, even though several existing tramp tests
called insert-directory, they were orthogonal to the free space aspect.

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

[John Cummings]

John Cummings <john@rootabega.net> wrote:

> 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
[snipped long section about WHY I thought the test should be skipped]

Also, I now see how making files-tests.el responsible for
accommodating ls-lisp.el might be backwards. If skipping the tests is
still a good idea, I'll try to learn if there's a more conventional
way to do it, e.g. having files-tests.el look for more general signs
that the insert-directory implementation has been overridden, or
maybe having ls-lisp.el be responsible for asking that that test be
skipped.

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

[Eli Zaretskii]

> 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?

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

> 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.

> 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.

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

[John Cummings]

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.

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

[Eli Zaretskii]

> Date: Sat, 25 Sep 2021 14:29:17 +0000
> From: John Cummings <john@rootabega.net>
> Cc: 50630@debbugs.gnu.org
> 
> > 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.

At the time, I looked at what happened on Windows, and didn't see the
problem, because ls-lisp does

    (get-free-disk-space ".")

and the default-directory of the buffer at that point is already set
to the directory whose listing we will present.

>  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

That recipe invokes insert-directory in some arbitrary way, which is
probably different from what happens when one invokes Dired.  So I'm
not sure we should worry about that.  But anyway, I tried that recipe
on MS-Windows, and I don't see the problem: the values shown on the
first line reflect my home directory, not pwd-a or pwd-b.

>  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.

I suggest that you remove the special treatment of ls-lisp from that
test.  As I say above, I don't expect it to need any special
treatment, I expect it to pass the test without any changes.  And if
it fails, let's take it from there.

In any case, I see no reason to move the ls-lisp specific test of
insert-directory to ls-lisp-tests.el: since we are testing
insert-directory, the natural place for that test is in
files-tests.el, regardless of what platform-specific tricks are at
work behind the scenes.

> I want to avoid situations where someone runs files-tests.el and it
> actually tests the insert-directory implementation from ls-lisp.el.

I see no reason to avoid that.  The entry point is insert-directory,
and that is what's being tested here.

Thanks.

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

[John Cummings]

>>  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

> That recipe invokes insert-directory in some arbitrary way, which is
> probably different from what happens when one invokes Dired.  So I'm
> not sure we should worry about that.  But anyway, I tried that recipe
> on MS-Windows, and I don't see the problem: the values shown on the
> first line reflect my home directory, not pwd-a or pwd-b.

I'm replying only to this part for now, because we need to agree on
the facts in order to do anything else. If what you say is accurate,
then this bug may not really exist as I've described it, but I think
I've done everything reasonable to eliminate that possibility. Are you
sure that you picked values for pwd-a and pwd-b that would have
different amounts of available space on MS-Windows, like two drive
letters on physically separate disks?  But if Windows uses ls-lisp for
this, we should see the same results on any system using ls-lisp, so I
think it's OK to focus on ls-lisp and forget about MS-Windows for the
purposes of this reply.

I tried to supply values for pwd-a and pwd-b that would be under
different mount points on a Unix-like system, / and /proc, but if your
system is different, you may need to pick different Unix-like values
for those, too.

I just built 285f59cbe230701f15d28dfe8036cf2feb9d1d31 (terminal only)
from a brand new git clone on Ubuntu 20,and here are the results of
the recipes. I'll use a "[" character to quote the text I copied off
my terminal:

files.el (this case is already fixed):
 [emacsdev@ubuntu:~/freshemacs/emacs$ src/emacs -Q --eval "\
 [> (progn \
 [>    (setq pwd-a \"/\") \
 [>    (setq pwd-b \"/proc\")  \
 [>    (setq list-dir \"~\") \
 [>    (with-current-buffer-window \"a\" nil nil \
 [>       (cd pwd-a) \
 [>       (insert-directory list-dir \"-l\" nil t) \
 [>       (insert (format \"listing of %s in cwd %s\" list-dir (pwd)))) \
 [>    (with-current-buffer-window \"b\" nil nil \
 [>       (cd pwd-b) \
 [>       (insert-directory list-dir \"-l\" nil t) \
 [>       (insert (format \"listing of %s in cwd %s\" list-dir (pwd)))) \
 [>    (switch-to-buffer \"a\") \
 [>    (switch-to-buffer-other-window \"b\"))"

Resulting Emacs frame:
 [File Edit Options Buffers Tools Help
 [total used in directory 0 available 7.6 GiB
 [lrwxrwxrwx 1 emacsdev emacsdev 26 Sep 24 12:18 emacs -> /media/ubuntu/turkey/emacs
 [lrwxrwxrwx 1 emacsdev emacsdev 31 Sep 25 12:26 freshemacs -> /media/ubuntu/turkey/freshemacs
 [listing of ~ in cwd Directory /
 [
 [
 [
 [
 [
 [
 [
 [-UUU:%%--F1  a              All L1     (Fundamental) -----------------------------------------
 [total used in directory 0 available 7.6 GiB
 [lrwxrwxrwx 1 emacsdev emacsdev 26 Sep 24 12:18 emacs -> /media/ubuntu/turkey/emacs
 [lrwxrwxrwx 1 emacsdev emacsdev 31 Sep 25 12:26 freshemacs -> /media/ubuntu/turkey/freshemacs
 [listing of ~ in cwd Directory /proc/
 [
 [
 [
 [
 [
 [
 [-UUU:%%--F1  b              All L1     (Fundamental) -----------------------------------------
 [



ls-lisp.el (not fixed yet):
 [emacsdev@ubuntu:~/freshemacs/emacs$ src/emacs -Q --eval "\
 [> (progn \
 [>    (setq pwd-a \"/\") \
 [>    (setq pwd-b \"/proc\")  \
 [>    (setq list-dir \"~\") \
 [>    (require 'ls-lisp) \
 [>    (setq ls-lisp-use-insert-directory-program nil) \
 [>    (with-current-buffer-window \"a\" nil nil \
 [>       (cd pwd-a) \
 [>       (insert-directory list-dir \"-l\" nil t) \
 [>       (insert (format \"listing of %s in cwd %s\" list-dir (pwd)))) \
 [>    (with-current-buffer-window \"b\" nil nil \
 [>       (cd pwd-b) \
 [>       (insert-directory list-dir \"-l\" nil t) \
 [>       (insert (format \"listing of %s in cwd %s\" list-dir (pwd)))) \
 [>    (switch-to-buffer \"a\") \
 [>    (switch-to-buffer-other-window \"b\"))"

 Resulting Emacs frame with different available space:
 [File Edit Options Buffers Tools Help
 [total used in directory 1 available 7.6 GiB
 [lrwxrwxrwx  1 emacsdev emacsdev 26 09-24 12:18 emacs -> /media/ubuntu/turkey/emacs
 [lrwxrwxrwx  1 emacsdev emacsdev 31 09-25 12:26 freshemacs -> /media/ubuntu/turkey/freshemacs
 [listing of ~ in cwd Directory /
 [
 [
 [
 [
 [
 [
 [
 [-UUU:%%--F1  a              All L1     (Fundamental) -----------------------------------------
 [total used in directory 1 available 0 B
 [lrwxrwxrwx  1 emacsdev emacsdev 26 09-24 12:18 emacs -> /media/ubuntu/turkey/emacs
 [lrwxrwxrwx  1 emacsdev emacsdev 31 09-25 12:26 freshemacs -> /media/ubuntu/turkey/freshemacs
 [listing of ~ in cwd Directory /proc/
 [
 [
 [
 [
 [
 [
 [-UUU:%%--F1  b              All L1     (Fundamental) -----------------------------------------
 [





tramp-sh.el (not fixed yet):
 [emacsdev@ubuntu:~/freshemacs/emacs$ src/emacs -Q --eval "\
 [> (progn \
 [>    (setq pwd-a \"/\") \
 [>    (setq pwd-b \"/proc\")  \
 [>    (setq list-dir \"/ssh:emacsdev@localhost:~/\") \
 [>    (require 'tramp) \
 [>    (setq ls-lisp-use-insert-directory-program t) \
 [>    (with-current-buffer-window \"a\" nil nil \
 [>       (cd pwd-a) \
 [>       (insert-directory list-dir \"-l\" nil t) \
 [>       (insert (format \"listing of %s in cwd %s\" list-dir (pwd)))) \
 [>    (with-current-buffer-window \"b\" nil nil \
 [>       (cd pwd-b) \
 [>       (insert-directory list-dir \"-l\" nil t) \
 [>       (insert (format \"listing of %s in cwd %s\" list-dir (pwd)))) \
 [>    (switch-to-buffer \"a\") \
 [>    (switch-to-buffer-other-window \"b\"))"

 Resulting Emacs frame with different available space:
 [File Edit Options Buffers Tools Help
 [total used in directory 0 available 7.6 GiB
 [lrwxrwxrwx 1 emacsdev emacsdev 26 Sep 24 12:18 emacs -> /media/ubuntu/turkey/emacs
 [lrwxrwxrwx 1 emacsdev emacsdev 31 Sep 25 12:26 freshemacs -> /media/ubuntu/turkey/freshemacs
 [listing of /ssh:emacsdev@localhost:~/ in cwd Directory /
 [
 [
 [
 [
 [
 [
 [
 [-UUU:%%--F1  a              All L1     (Fundamental) -----------------------------------------
 [total used in directory 0 available 0 B
 [lrwxrwxrwx 1 emacsdev emacsdev 26 Sep 24 12:18 emacs -> /media/ubuntu/turkey/emacs
 [lrwxrwxrwx 1 emacsdev emacsdev 31 Sep 25 12:26 freshemacs -> /media/ubuntu/turkey/freshemacs
 [listing of /ssh:emacsdev@localhost:~/ in cwd Directory /proc/
 [
 [
 [
 [
 [
 [
 [-UUU:%%--F1  b              All L1     (Fundamental) -----------------------------------------
 [

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

[Eli Zaretskii]

> Date: Sat, 25 Sep 2021 17:15:21 +0000
> From: John Cummings <john@rootabega.net>
> Cc: 50630@debbugs.gnu.org
> 
> >>  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
> 
> > That recipe invokes insert-directory in some arbitrary way, which is
> > probably different from what happens when one invokes Dired.  So I'm
> > not sure we should worry about that.  But anyway, I tried that recipe
> > on MS-Windows, and I don't see the problem: the values shown on the
> > first line reflect my home directory, not pwd-a or pwd-b.
> 
> I'm replying only to this part for now, because we need to agree on
> the facts in order to do anything else. If what you say is accurate,
> then this bug may not really exist as I've described it, but I think
> I've done everything reasonable to eliminate that possibility. Are you
> sure that you picked values for pwd-a and pwd-b that would have
> different amounts of available space on MS-Windows, like two drive
> letters on physically separate disks?

No, they were 2 different directories on the same drive.

Maybe I don't understand which part of the totals you want to fix:
there are two numbers there.

> I tried to supply values for pwd-a and pwd-b that would be under
> different mount points on a Unix-like system, / and /proc, but if your
> system is different, you may need to pick different Unix-like values
> for those, too.

I indeed provided different values, but not on different volumes.
What is the significance of a different volume for this purpose?

> I just built 285f59cbe230701f15d28dfe8036cf2feb9d1d31 (terminal only)
> from a brand new git clone on Ubuntu 20,and here are the results of
> the recipes. I'll use a "[" character to quote the text I copied off
> my terminal:

Thanks, but it's hard to interpret that.  What should I be looking at?
Do we agree that listing a given directory should show the same amount
of used and free space, not matter what is the default-directory of
the buffer from which you invoke insert-directory?  Because that's
what I saw after running your recipe.

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

[John Cummings]

> What should I be looking at?

The "available" (or, loosely, "free") space reported.

> Do we agree that listing a given directory should show the same amount
> of used and free space, not matter what is the default-directory of
> the buffer from which you invoke insert-directory? Because that's
> what I saw after running your recipe.

Yes, I agree. If you saw the same amount of free space,
it's because you didn't make pwd-a and pwd-b be directories
on two different volumes that have different amounts of free space.

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

[Eli Zaretskii]

> Date: Sat, 25 Sep 2021 17:37:29 +0000
> From: John Cummings <john@rootabega.net>
> Cc: 50630@debbugs.gnu.org
> 
> > Do we agree that listing a given directory should show the same amount
> > of used and free space, not matter what is the default-directory of
> > the buffer from which you invoke insert-directory? Because that's
> > what I saw after running your recipe.
> 
> Yes, I agree. If you saw the same amount of free space,
> it's because you didn't make pwd-a and pwd-b be directories
> on two different volumes that have different amounts of free space.

OK, so I'm saying that when invoking Dired, I see the same numbers
(both of them) no matter from which buffer I invoked Dired.  Even if
the default-directory of each buffer is on a different drive.

With your recipe, indeed the free space is not always correct.  But
why is that use case interesting?  insert-directory is not a normally
a user-visible function.

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

[John Cummings]

> With your recipe, indeed the free space is not always correct. But
> why is that use case interesting? insert-directory is not a normally
> a user-visible function.

If being a user-visible function is necessary to consider it interesting,
then I reported this as a bug in the verbose `list-directory' output,
which is documented in the manual.

But I don't think it needs to be normally user-visible to be interesting.
Would programmers be welcome to use insert-directory in their code? It would
also be good to fix this so that any future Emacs functionality that used
insert-directory would have the correct output.

The help for that function says:

 insert-directory is a compiled Lisp function in
 ‘/media/ubuntu/turkey/freshemacs/emacs/lisp/files.el’.

 (insert-directory FILE SWITCHES &optional WILDCARD FULL-DIRECTORY-P)

 Insert directory listing for FILE, formatted according to SWITCHES.

I truncated that, but it doesn't say anything about depending on the
current default-directory. If the function performs a listing for FILE,
it shouldn't be possible for it to insert free space that is not correct
for FILE. If it's OK for insert-directory to rely on the current
default-directory, then the FILE argument itself seems unnecessary.

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

[Eli Zaretskii]

> Date: Sat, 25 Sep 2021 18:01:26 +0000
> From: John Cummings <john@rootabega.net>
> Cc: 50630@debbugs.gnu.org
> 
> > With your recipe, indeed the free space is not always correct. But
> > why is that use case interesting? insert-directory is not a normally
> > a user-visible function.
> 
> If being a user-visible function is necessary to consider it interesting,
> then I reported this as a bug in the verbose `list-directory' output,
> which is documented in the manual.

Like I said before: let's pick this up when your test is installed,
and we can run it and debug it if needed.

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

[John Cummings]

Eli Zaretskii <eliz@gnu.org> wrote:

> > > With your recipe, indeed the free space is not always correct. But
> > > why is that use case interesting? insert-directory is not a normally
> > > a user-visible function.
> >
> > If being a user-visible function is necessary to consider it interesting,
> > then I reported this as a bug in the verbose `list-directory' output,
> > which is documented in the manual.
>
> Like I said before: let's pick this up when your test is installed,
> and we can run it and debug it if needed.

I'd appreciate it if you continued this discussion until we can agree there
is actually a bug, or at least something that could be improved.  I don't
think we need the tests to be merged to be able to do that. In fact, I'd say
it's the other way around: if there's a chance that a maintainer might tell
me there is no bug, or that whatever I found is not worth changing, I would
decide not to invest any more time into it.

So are we in agreement about these 3 things below?

1. insert-directory in files.el used to be able to show the incorrect free
   space for its FILE arg. The same function in ls-lisp.el and tramp-sh.el
   continues to be able to show the incorrect free space today.

2. The recipes/instructions I sent will be able to confirm this

3. This should be changed because insert-directory should not be able to
   insert free space info that is not correct for its FILE arg.

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

[Eli Zaretskii]

> Date: Sat, 25 Sep 2021 19:00:14 +0000
> From: John Cummings <john@rootabega.net>
> Cc: 50630@debbugs.gnu.org
> 
> So are we in agreement about these 3 things below?
> 
> 1. insert-directory in files.el used to be able to show the incorrect free
>    space for its FILE arg. The same function in ls-lisp.el and tramp-sh.el
>    continues to be able to show the incorrect free space today.
> 
> 2. The recipes/instructions I sent will be able to confirm this
> 
> 3. This should be changed because insert-directory should not be able to
>    insert free space info that is not correct for its FILE arg.

Yes.  But how exactly to fix this in ls-lisp remains to be seen.  I
think I know how, and I'm asking you to have your tests installed so I
could try my idea conveniently.

Thanks in advance.

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

[John Cummings]

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

Here's the latest patch. If I should send it as a new email to the bug address, or create a new bug like Lars suggested, I can do that.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-tests-for-insert-directory.patch --]
[-- Type: text/x-patch; name=0001-Add-tests-for-insert-directory.patch, Size: 5953 bytes --]

From 0567bf25dc0cb72b95e0bbdef8061c5674134903 Mon Sep 17 00:00:00 2001
From: John Cummings <john@rootabega.net>
Date: Sat, 25 Sep 2021 15:51:42 -0400
Subject: [PATCH] Add tests for 'insert-directory'

Add tests for 'insert-directory' base functionality and regression tests for
the issue where free space was reported for the current directory instead of
the target of 'list-directory' (Bug#50630).
* test/lisp/files-tests.el: Add 'insert-directory' tests.
* test/lisp/files-resources/insert-directory/: Create directories and files to
use for testing 'insert-directory'.
---
 .../insert-directory/test_dir/bar             |  0
 .../insert-directory/test_dir/foo             |  0
 .../insert-directory/test_dir_other/bar       |  0
 .../insert-directory/test_dir_other/foo       |  0
 test/lisp/files-tests.el                      | 72 +++++++++++++++++++
 5 files changed, 72 insertions(+)
 create mode 100644 test/lisp/files-resources/insert-directory/test_dir/bar
 create mode 100644 test/lisp/files-resources/insert-directory/test_dir/foo
 create mode 100644 test/lisp/files-resources/insert-directory/test_dir_other/bar
 create mode 100644 test/lisp/files-resources/insert-directory/test_dir_other/foo

diff --git a/test/lisp/files-resources/insert-directory/test_dir/bar b/test/lisp/files-resources/insert-directory/test_dir/bar
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/test/lisp/files-resources/insert-directory/test_dir/foo b/test/lisp/files-resources/insert-directory/test_dir/foo
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/test/lisp/files-resources/insert-directory/test_dir_other/bar b/test/lisp/files-resources/insert-directory/test_dir_other/bar
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/test/lisp/files-resources/insert-directory/test_dir_other/foo b/test/lisp/files-resources/insert-directory/test_dir_other/foo
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index b283a512a4..83a3aeb5e7 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1774,6 +1774,78 @@ files-tests-save-buffers-kill-emacs--asks-to-save-buffers
          ;; `save-some-buffers-default-predicate' (i.e. the 2nd element) is ignored.
          (nil save-some-buffers-root ,nb-might-save))))))
 
+;;insert-directory output tests
+(let* ((data-dir "insert-directory")
+       (test-dir (file-name-as-directory
+                  (ert-resource-file
+                   (concat data-dir "/test_dir"))))
+       (test-dir-other (file-name-as-directory
+                        (ert-resource-file
+                         (concat data-dir "/test_dir_other"))))
+       (test-files `(,test-dir "foo" "bar")) ;expected files to be found
+       ;Free space test data for `insert-directory'.
+       ;Meaning: (path free-space-bytes-to-stub expected-free-space-string)
+       (free-data `((,test-dir 10 "available 10 B")
+                    (,test-dir-other 100 "available 100 B")
+                    (:default 999 "available 999 B"))))
+
+
+  (defun files-tests--look-up-free-data (path)
+    "Look up free space test data, with a default for unspecified paths."
+    (let ((path (file-name-as-directory path)))
+      (cdr (or (assoc path free-data)
+               (assoc :default free-data)))))
+
+  (defun files-tests--make-file-system-info-stub (&optional static-path)
+    "Return a stub for `file-system-info' using dynamic or static test data.
+If that data should be static, pass STATIC-PATH to choose which
+path's data to use."
+    (lambda (path)
+      (let* ((path (cond (static-path)
+                         ;; file-system-info knows how to handle ".", so we
+                         ;; do the same thing
+                         ((equal "." path) default-directory)
+                         (path)))
+             (return-size
+              (car (files-tests--look-up-free-data path))))
+        (list return-size return-size return-size))))
+
+  (defun files-tests--insert-directory-output (dir &optional verbose)
+    "Run `insert-directory' and return its output."
+    (with-current-buffer-window "files-tests--insert-directory" nil nil
+      (insert-directory dir "-l" nil t)
+      (buffer-substring-no-properties (point-min) (point-max))))
+
+  (ert-deftest files-tests-insert-directory-shows-files ()
+    "Verify `insert-directory' reports the files in the directory."
+    (let* ((test-dir (car test-files))
+           (files (cdr test-files))
+           (output (files-tests--insert-directory-output test-dir)))
+      (dolist (file files)
+        (should (string-match-p file output)))))
+
+  (defun files-tests--insert-directory-shows-given-free (dir &optional info-func)
+    "Run `insert-directory' and verify it reports the correct available space.
+Stub `file-system-info' to ensure the available space is consistent,
+either with the given stub function or a default one using test data."
+    (cl-letf (((symbol-function 'file-system-info)
+               (or info-func
+                   (files-tests--make-file-system-info-stub))))
+      (should (string-match-p (cadr
+                               (files-tests--look-up-free-data dir))
+                              (files-tests--insert-directory-output dir t)))))
+
+  (ert-deftest files-tests-insert-directory-shows-free ()
+    "Test that verbose `insert-directory' shows the correct available space."
+    (files-tests--insert-directory-shows-given-free
+     test-dir
+     (files-tests--make-file-system-info-stub test-dir)))
+
+  (ert-deftest files-tests-bug-50630 ()
+    "Verify verbose `insert-directory' shows free space of the target directory.
+The current directory at call time should not affect the result (Bug#50630)."
+    (let ((default-directory test-dir-other))
+      (files-tests--insert-directory-shows-given-free test-dir))))
 
 (provide 'files-tests)
 ;;; files-tests.el ends here
-- 
2.25.1