dired-tests.el fails on MS-Windows

dired-tests.el fails on MS-Windows

[Eli Zaretskii]

The failed tests are shown below.  2 others failed originally, but I
fixed them.

Tino, please always add comments to the tests explaining the idea
behind all the tricks and juggling.  Otherwise, it is very hard to
understand why the test is doing what it's doing and how it is doing
that.  Same request for ediff-ptch-tests.el, which also fails for me.

Than ks.

dired-tests.log:

  Running 12 tests (2017-08-01 17:44:38+0300)
     passed   1/12  dired-autoload
  Marking matching files...
  Checking d:/gnu/git/emacs/trunk/test/bug22694/test
  1 matching file marked.
     passed   2/12  dired-test-bug22694
  Copy: 1 of 1
  Copy: 1 file
  Copy: 1 of 1
  Copy: 1 file
  Test dired-test-bug25609 backtrace:
    signal(ert-test-failed (((should (file-exists-p target)) :form (file
    ert-fail(((should (file-exists-p target)) :form (file-exists-p "c:/D
    (if (unwind-protect (setq value-17 (apply fn-15 args-16)) (setq form
    (let (form-description-19) (if (unwind-protect (setq value-17 (apply
    (let ((value-17 'ert-form-evaluation-aborted-18)) (let (form-descrip
    (let ((fn-15 (function file-exists-p)) (args-16 (list target))) (let
    (progn (let ((fn-15 (function file-exists-p)) (args-16 (list target)
    (unwind-protect (progn (let ((fn-15 (function file-exists-p)) (args-
    (let* ((from (make-temp-file "foo" 'dir)) (to (make-temp-file "bar" 
    (closure (t) nil (let* ((from (make-temp-file "foo" 'dir)) (to (make
    ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
    ert-run-test(#s(ert-test :name dired-test-bug25609 :documentation "T
    ert-run-or-rerun-test(#s(ert--stats :selector t :tests [#s(ert-test 
    ert-run-tests(t #f(compiled-function (event-type &rest event-args) #
    ert-run-tests-batch(nil)
    ert-run-tests-batch-and-exit(nil)
    eval((ert-run-tests-batch-and-exit nil))
    command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/dired-tests.el" "--e
    command-line()
    normal-top-level()
  Test dired-test-bug25609 condition:
      (ert-test-failed
       ((should
	 (file-exists-p target))
	:form
	(file-exists-p "c:/DOCUME~1/Zaretzky/LOCALS~1/Temp/bar6828Ler/foo6828WPJ")
	:value nil))
     FAILED   3/12  dired-test-bug25609
     passed   4/12  dired-test-bug27243-01
     passed   5/12  dired-test-bug27243-02
     passed   6/12  dired-test-bug27243-03
  Test dired-test-bug27631 backtrace:
    signal(error ("em-ls is not a currently loaded feature"))
    error("%s is not a currently loaded feature" "em-ls")
    unload-feature(em-ls force)
    (unwind-protect (progn (make-directory dir1) (make-directory dir2) (
    (let* ((dir (make-temp-file "bug27631" 'dir)) (dir1 (expand-file-nam
    (closure (t) nil (let* ((dir (make-temp-file "bug27631" 'dir)) (dir1
    ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
    ert-run-test(#s(ert-test :name dired-test-bug27631 :documentation "T
    ert-run-or-rerun-test(#s(ert--stats :selector t :tests [#s(ert-test 
    ert-run-tests(t #f(compiled-function (event-type &rest event-args) #
    ert-run-tests-batch(nil)
    ert-run-tests-batch-and-exit(nil)
    eval((ert-run-tests-batch-and-exit nil))
    command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/dired-tests.el" "--e
    command-line()
    normal-top-level()
  Test dired-test-bug27631 condition:
      (error "em-ls is not a currently loaded feature")
     FAILED   7/12  dired-test-bug27631
     passed   8/12  dired-test-bug27693
     failed   9/12  dired-test-bug27762
     passed  10/12  dired-test-bug27817
     passed  11/12  dired-test-bug27843
     passed  12/12  dired-test-bug7131

  Ran 12 tests, 10 results as expected, 2 unexpected (2017-08-01 17:44:46+0300)
  1 expected failures

  2 unexpected results:
     FAILED  dired-test-bug25609
     FAILED  dired-test-bug27631

ediff-ptch-tests.log:

  Running 2 tests (2017-08-01 18:21:40+0300)
     passed  1/2  ediff-ptch-test-bug25010
  Test ediff-ptch-test-bug26084 backtrace:
  Test ediff-ptch-test-bug26084 condition:
      (wrong-type-argument stringp nil)
     FAILED  2/2  ediff-ptch-test-bug26084

  Ran 2 tests, 1 results as expected, 1 unexpected (2017-08-01 18:21:40+0300)

  1 unexpected results:
     FAILED  ediff-ptch-test-bug26084

Re: dired-tests.el fails on MS-Windows

[Tino Calancha]

Eli Zaretskii <eliz@gnu.org> writes:

> The failed tests are shown below.  2 others failed originally, but I
> fixed them.
>
> Tino, please always add comments to the tests explaining the idea
> behind all the tricks and juggling.  Otherwise, it is very hard to
> understand why the test is doing what it's doing and how it is doing
> that.  Same request for ediff-ptch-tests.el, which also fails for me.
OK.  I agree they looks not very digestive tests.

> dired-tests.log:
>   Test dired-test-bug25609 condition:
>       (ert-test-failed
>        ((should
> 	 (file-exists-p target))
> 	:form
> 	(file-exists-p "c:/DOCUME~1/Zaretzky/LOCALS~1/Temp/bar6828Ler/foo6828WPJ")
> 	:value nil))
Could you check the following?
emacs -Q
;; eval following form:
(require 'nadvice)
(let* ((from (make-temp-file "foo" 'dir))
       (to (make-temp-file "bar" 'dir))
       (target (expand-file-name (file-name-nondirectory from) to))
       (nested (expand-file-name (file-name-nondirectory from) target))
       (dired-dwim-target t)
       (dired-recursive-copies 'always) ; Don't prompt me.
       buffers)
  (advice-add 'dired-query ; Don't ask confirmation to overwrite a file.
              :override
              (lambda (_sym _prompt &rest _args) (setq dired-query t))
              '((name . "advice-dired-query")))
  (advice-add 'completing-read ; Just return init.
              :override
              (lambda (_prompt _coll &optional _pred _match init _hist _def _inherit _keymap)
                init)
              '((name . "advice-completing-read")))
  (push (dired to) buffers)
  (push (dired-other-window temporary-file-directory) buffers)
  (dired-goto-file from)
  (dired-do-copy)
  (dired-do-copy); Again.
  (unwind-protect
      (progn
        (list (file-exists-p target) (file-exists-p nested)))
    (dolist (buf buffers)
      (when (buffer-live-p buf) (kill-buffer buf)))
    (delete-directory from 'recursive)
    (delete-directory to 'recursive)
    (advice-remove 'dired-query "advice-dired-query")
    (advice-remove 'completing-read "advice-completing-read")))
;; The normal result is: '(t nil).  If you get '(nil t) means
;; the bug is not fixed in your platform.  Other values must be
;; wrong assumptions from my side that are not true in your box.


>   Test dired-test-bug27631 backtrace:
>     signal(error ("em-ls is not a currently loaded feature"))
>     error("%s is not a currently loaded feature" "em-ls")
>     unload-feature(em-ls force)
>     (unwind-protect (progn (make-directory dir1) (make-directory dir2) (
>     (let* ((dir (make-temp-file "bug27631" 'dir)) (dir1 (expand-file-nam
>     (closure (t) nil (let* ((dir (make-temp-file "bug27631" 'dir)) (dir1
>     ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
>     ert-run-test(#s(ert-test :name dired-test-bug27631 :documentation "T
>     ert-run-or-rerun-test(#s(ert--stats :selector t :tests [#s(ert-test 
>     ert-run-tests(t #f(compiled-function (event-type &rest event-args) #
>     ert-run-tests-batch(nil)
>     ert-run-tests-batch-and-exit(nil)
>     eval((ert-run-tests-batch-and-exit nil))
>     command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/dired-tests.el" "--e
>     command-line()
>     normal-top-level()
>   Test dired-test-bug27631 condition:
>       (error "em-ls is not a currently loaded feature")
I don't get this error, but the idea of require
those libs and unload them looks ugly.
We could move those parts to:
test/lisp/ls-lisp.el (yes i called wrongly: must be ls-lisp-tests.el)
test/lisp/eshell/eshell-tests.el
and then, we don't need to unload anything.

> ediff-ptch-tests.log:
>
>   Running 2 tests (2017-08-01 18:21:40+0300)
>      passed  1/2  ediff-ptch-test-bug25010
>   Test ediff-ptch-test-bug26084 backtrace:
>   Test ediff-ptch-test-bug26084 condition:
>       (wrong-type-argument stringp nil)
>      FAILED  2/2  ediff-ptch-test-bug26084
I think this test for Bug#26084 is more complicated than the
fix of the bug itself.  It has also problems because the different
idiosyncrasy respect to "-b" option for different versions of "patch".
Delete it?
Skipped it unless in a GNU system?

Re: dired-tests.el fails on MS-Windows

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Cc: emacs-devel@gnu.org, tino.calancha@gmail.com
> Date: Wed, 02 Aug 2017 02:02:32 +0900
> 
> > dired-tests.log:
> >   Test dired-test-bug25609 condition:
> >       (ert-test-failed
> >        ((should
> > 	 (file-exists-p target))
> > 	:form
> > 	(file-exists-p "c:/DOCUME~1/Zaretzky/LOCALS~1/Temp/bar6828Ler/foo6828WPJ")
> > 	:value nil))
> Could you check the following?

I could, but I don't understand the purpose.  This form is almost
identical to what's in dired-tests.el, and I already established that
the failure is indeed because 'target' doesn't exist at that moment.
I just didn't dig deep enough to understand why, because I didn't
really understand what the code wants to do, e.g. why it calls
dired-do-copy twice, and more importantly why 'target' is supposed to
exist after all that.

What I see here is that at the point where file-exists-p is called,
there are two directories: /bla/blah/foNNNNNN and /bla/bla/barKKKKKK,
but not /bla/bla/fooNNNNN/barKKKKKK, as I think the code expects.

maybe if you could explain the idea behind the code I could think of a
reason why it doesn't work here.

> >   Test dired-test-bug27631 backtrace:
> >     signal(error ("em-ls is not a currently loaded feature"))
> >     error("%s is not a currently loaded feature" "em-ls")
> >     unload-feature(em-ls force)
> >     (unwind-protect (progn (make-directory dir1) (make-directory dir2) (
> >     (let* ((dir (make-temp-file "bug27631" 'dir)) (dir1 (expand-file-nam
> >     (closure (t) nil (let* ((dir (make-temp-file "bug27631" 'dir)) (dir1
> >     ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
> >     ert-run-test(#s(ert-test :name dired-test-bug27631 :documentation "T
> >     ert-run-or-rerun-test(#s(ert--stats :selector t :tests [#s(ert-test 
> >     ert-run-tests(t #f(compiled-function (event-type &rest event-args) #
> >     ert-run-tests-batch(nil)
> >     ert-run-tests-batch-and-exit(nil)
> >     eval((ert-run-tests-batch-and-exit nil))
> >     command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/dired-tests.el" "--e
> >     command-line()
> >     normal-top-level()
> >   Test dired-test-bug27631 condition:
> >       (error "em-ls is not a currently loaded feature")
> I don't get this error, but the idea of require
> those libs and unload them looks ugly.

I think we need to understand why the problem happens before we decide
how to proceed.

> > ediff-ptch-tests.log:
> >
> >   Running 2 tests (2017-08-01 18:21:40+0300)
> >      passed  1/2  ediff-ptch-test-bug25010
> >   Test ediff-ptch-test-bug26084 backtrace:
> >   Test ediff-ptch-test-bug26084 condition:
> >       (wrong-type-argument stringp nil)
> >      FAILED  2/2  ediff-ptch-test-bug26084
> I think this test for Bug#26084 is more complicated than the
> fix of the bug itself.  It has also problems because the different
> idiosyncrasy respect to "-b" option for different versions of "patch".
> Delete it?

I don't know.  What does it try to test?

> Skipped it unless in a GNU system?

Only if there's no better way.  The Patch invocation definitely needs
the --binary switch on Windows, though.  But the failure above is not
about that, it's about something else, because directory-files returns
an empty list.  Something prevents Patch from creating backup files.

Re: dired-tests.el fails on MS-Windows

[Fabrice Popineau]

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

2017-08-01 21:04 GMT+02:00 Eli Zaretskii <eliz@gnu.org>:

>
> Only if there's no better way.  The Patch invocation definitely needs
> the --binary switch on Windows, though.  But the failure above is not
> about that, it's about something else, because directory-files returns
> an empty list.  Something prevents Patch from creating backup files.
>
>
When I add the '--binary' option to patch, the test passes.
Again, windows 10, mingw64.



-- 
Fabrice

[-- Attachment #2: Type: text/html, Size: 1073 bytes --]

Re: dired-tests.el fails on MS-Windows

[Tino Calancha]

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



On Tue, 1 Aug 2017, Fabrice Popineau wrote:

> 
> 
> 2017-08-01 21:04 GMT+02:00 Eli Zaretskii <eliz@gnu.org>:
>
>       Only if there's no better way.  The Patch invocation definitely needs
>       the --binary switch on Windows, though.  But the failure above is not
>       about that, it's about something else, because directory-files returns
>       an empty list.  Something prevents Patch from creating backup files.
> 
> 
> When I add the '--binary' option to patch, the test passes.
> Again, windows 10, mingw64.
Fabrice, Eli
does the following work in your environments?

--8<-----------------------------cut here---------------start------------->8---
commit 16fd5bf68538240b7a601e0975bdd92f0521b7e5
Author: Tino Calancha <tino.calancha@gmail.com>
Date:   Wed Aug 2 15:37:11 2017 +0900

     Fix an ediff test which fails in MS-Windows

     https://lists.gnu.org/archive/html/emacs-devel/2017-08/msg00018.html
     * test/lisp/vc/ediff-ptch-tests.el (ediff-ptch-test-bug26084):
     Add comments to explain the test logic.
     Pass '--binary' option to 'patch' program in windows environments.
     Check explicitely that a backup is created before compare file contents.

diff --git a/test/lisp/vc/ediff-ptch-tests.el b/test/lisp/vc/ediff-ptch-tests.el
index 387786ced0..6fbc1b0a8b 100644
--- a/test/lisp/vc/ediff-ptch-tests.el
+++ b/test/lisp/vc/ediff-ptch-tests.el
@@ -66,41 +66,55 @@
        (write-region nil nil bar nil 'silent))
      (call-process git-program nil `(:file ,patch) nil "diff")
      (call-process git-program nil nil nil "reset" "--hard" "HEAD")
+    ;; Visit the diff file i.e., patch; extract from it the parts
+    ;; affecting just each of the files: store in patch-bar the part
+    ;; affecting 'bar', and in patch-qux the part affecting 'qux'.
      (find-file patch)
      (unwind-protect
          (let* ((info
                  (progn (ediff-map-patch-buffer (current-buffer)) ediff-patch-map))
-               (patch1
+               (patch-bar
                  (buffer-substring-no-properties
                   (car (nth 3 (car info)))
                   (car (nth 4 (car info)))))
-               (patch2
+               (patch-qux
                  (buffer-substring-no-properties
                   (car (nth 3 (cadr info)))
                   (car (nth 4 (cadr info))))))
            ;; Apply both patches.
-          (dolist (x (list (cons patch1 bar) (cons patch2 qux)))
+          (dolist (x (list (cons patch-bar bar) (cons patch-qux qux)))
              (with-temp-buffer
-              (insert (car x))
-              (call-process-region (point-min)
-                                   (point-max)
-                                   ediff-patch-program
-                                   nil nil nil
-                                   "-b" (cdr x))))
-          ;; Check backup files were saved correctly.
+              ;; Some windows variants require the option '--binary'
+              ;; in order to 'patch' create backup files.
+              (let ((opts (format "--backup%s"
+                                  (if (memq system-type '(windows-nt ms-dos))
+                                      " --binary" ""))))
+                (insert (car x))
+                (call-process-region (point-min)
+                                     (point-max)
+                                     ediff-patch-program
+                                     nil nil nil
+                                     opts (cdr x)))))
+          ;; Check backup files were saved correctly; in Bug#26084 some
+          ;; of the backup files are overwritten with the actual content
+          ;; of the updated file.  To ensure that the bug is fixed we just
+          ;; need to check that every backup file produced has different
+          ;; content that the current updated file.
            (dolist (x (list qux bar))
              (let ((backup
                     (car
                      (directory-files
                       tmpdir 'full
                       (concat (file-name-nondirectory x) ".")))))
-              (should-not
-               (string= (with-temp-buffer
-                          (insert-file-contents x)
-                          (buffer-string))
-                        (with-temp-buffer
-                          (insert-file-contents backup)
-                          (buffer-string))))))
+              ;; Compare files only if the backup has being created.
+              (when backup
+                (should-not
+                 (string= (with-temp-buffer
+                            (insert-file-contents x)
+                            (buffer-string))
+                          (with-temp-buffer
+                            (insert-file-contents backup)
+                            (buffer-string)))))))
            (delete-directory tmpdir 'recursive)
            (delete-file patch)))))

--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 26.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
  of 2017-08-02 built
Repository revision: 0fd6de9cb444d6cc553ea67815ccfb7a923012a2

Re: dired-tests.el fails on MS-Windows

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Wed, 2 Aug 2017 15:44:59 +0900 (JST)
> cc: Eli Zaretskii <eliz@gnu.org>, Tino Calancha <tino.calancha@gmail.com>, 
>     Emacs developers <emacs-devel@gnu.org>
> 
> > 2017-08-01 21:04 GMT+02:00 Eli Zaretskii <eliz@gnu.org>:
> >
> >       Only if there's no better way.  The Patch invocation definitely needs
> >       the --binary switch on Windows, though.  But the failure above is not
> >       about that, it's about something else, because directory-files returns
> >       an empty list.  Something prevents Patch from creating backup files.
> > 
> > 
> > When I add the '--binary' option to patch, the test passes.
> > Again, windows 10, mingw64.
> Fabrice, Eli
> does the following work in your environments?

Yes, it passes now.

Thanks!

Re: dired-tests.el fails on MS-Windows

[Fabrice Popineau]

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

I shall say : it almost passes on my side.

I had to apply the following :

diff --git a/test/lisp/dired-tests.el b/test/lisp/dired-tests.el
index 02dbf263b9..88cdbef6ac 100644
--- a/test/lisp/dired-tests.el
+++ b/test/lisp/dired-tests.el
@@ -282,7 +282,7 @@ dired-dwim-target
           (make-directory dir2)
           (with-temp-file (expand-file-name "a.txt" dir1))
           (with-temp-file (expand-file-name "b.txt" dir2))
-          (setq buf (dired (expand-file-name "dir*/*.txt" dir)))
+          (setq buf (dired (cons dir (file-expand-wildcards
"dir*/*.txt"))))
           (dired-toggle-marks)
           (should (cdr (dired-get-marked-files))))
       (delete-directory dir 'recursive)
diff --git a/test/lisp/eshell/em-ls-tests.el
b/test/lisp/eshell/em-ls-tests.el
index 71a555d1ea..cc0e68c47d 100644
--- a/test/lisp/eshell/em-ls-tests.el
+++ b/test/lisp/eshell/em-ls-tests.el
@@ -42,7 +42,7 @@
           (make-directory dir2)
           (with-temp-file (expand-file-name "a.txt" dir1))
           (with-temp-file (expand-file-name "b.txt" dir2))
-          (setq buf (dired (expand-file-name "dir*/*.txt" dir)))
+          (setq buf (dired (cons dir (file-expand-wildcards
"dir*/*.txt"))))
           (dired-toggle-marks)
           (should (cdr (dired-get-marked-files))))
       (customize-set-variable 'eshell-ls-use-in-dired orig)
diff --git a/test/lisp/ls-lisp-tests.el b/test/lisp/ls-lisp-tests.el
index d24b30e5f2..77a02c88dd 100644
--- a/test/lisp/ls-lisp-tests.el
+++ b/test/lisp/ls-lisp-tests.el
@@ -69,7 +69,7 @@
           (make-directory dir2)
           (with-temp-file (expand-file-name "a.txt" dir1))
           (with-temp-file (expand-file-name "b.txt" dir2))
-          (setq buf (dired (expand-file-name "dir*/*.txt" dir)))
+          (setq buf (dired (cons dir (file-expand-wildcards
"dir*/*.txt"))))
           (dired-toggle-marks)
           (should (cdr (dired-get-marked-files))))
       (delete-directory dir 'recursive)

Am I wrong thinking that `expand-file-name' is not supposed to expand
"dir*/*.txt"?

Fabrice


2017-08-04 15:18 GMT+02:00 Eli Zaretskii <eliz@gnu.org>:

> > From: Tino Calancha <tino.calancha@gmail.com>
> > Date: Wed, 2 Aug 2017 15:44:59 +0900 (JST)
> > cc: Eli Zaretskii <eliz@gnu.org>, Tino Calancha <tino.calancha@gmail.com
> >,
> >     Emacs developers <emacs-devel@gnu.org>
> >
> > > 2017-08-01 21:04 GMT+02:00 Eli Zaretskii <eliz@gnu.org>:
> > >
> > >       Only if there's no better way.  The Patch invocation definitely
> needs
> > >       the --binary switch on Windows, though.  But the failure above
> is not
> > >       about that, it's about something else, because directory-files
> returns
> > >       an empty list.  Something prevents Patch from creating backup
> files.
> > >
> > >
> > > When I add the '--binary' option to patch, the test passes.
> > > Again, windows 10, mingw64.
> > Fabrice, Eli
> > does the following work in your environments?
>
> Yes, it passes now.
>
> Thanks!
>

[-- Attachment #2: Type: text/html, Size: 4455 bytes --]

Re: dired-tests.el fails on MS-Windows

[Tino Calancha]

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



On Fri, 4 Aug 2017, Fabrice Popineau wrote:

>            (with-temp-file (expand-file-name "b.txt" dir2))
> -          (setq buf (dired (expand-file-name "dir*/*.txt" dir)))
> +          (setq buf (dired (cons dir (file-expand-wildcards "dir*/*.txt"))))
>            (dired-toggle-marks)
>            (should (cdr (dired-get-marked-files))))
>        (delete-directory dir 'recursive)
> 
> Am I wrong thinking that `expand-file-name' is not supposed to expand "dir*/*.txt"?
Well we are using expand for 2 different things.

1) Expand filename with directory:
(expand-file-name "lis*/*file" source-directory)
=> "/home/calancha/soft/emacs-master/lis*/*file"
;; Like a concatenation of file + dir.

2) Expand shell wildcards.
(dired (expand-file-name "lis*/*file" source-directory))

Shows a dired buffer with 3 files:
lispref/Makefile
lispintro/Makefile
lisp/Makefile

Re: dired-tests.el fails on MS-Windows

[Fabrice Popineau]

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

2017-08-04 15:44 GMT+02:00 Tino Calancha <tino.calancha@gmail.com>:

>
>
> On Fri, 4 Aug 2017, Fabrice Popineau wrote:
>
>            (with-temp-file (expand-file-name "b.txt" dir2))
>> -          (setq buf (dired (expand-file-name "dir*/*.txt" dir)))
>> +          (setq buf (dired (cons dir (file-expand-wildcards
>> "dir*/*.txt"))))
>>            (dired-toggle-marks)
>>            (should (cdr (dired-get-marked-files))))
>>        (delete-directory dir 'recursive)
>>
>> Am I wrong thinking that `expand-file-name' is not supposed to expand
>> "dir*/*.txt"?
>>
> Well we are using expand for 2 different things.
>
> 2) Expand shell wildcards.
> (dired (expand-file-name "lis*/*file" source-directory))
>
> This doesn't work here.

I have :

c:/tmp/dir1/foo.txt
c:/tmp/dir2/bar.txt

and:

(expand-file-name "dir*/*.txt" "c:/tmp/")
"c:/tmp/dir*/*.txt"

Moreover, as far as I can read it, `expand-file-name' documentation says
nothing about
expanding wildcards.

And last, `dired' maybe fed with a list, but the first argument needs to be
the directory
and the next ones, the files you want in the dired buffer, so I doubt that
`dired' will accomodate
the return value(s?) of `expand-file-name'?

-> In other words: I don't understand how this should work :-)



Fabrice


Shows a dired buffer with 3 files:
> lispref/Makefile
> lispintro/Makefile
> lisp/Makefile
>

[-- Attachment #2: Type: text/html, Size: 2506 bytes --]

Re: dired-tests.el fails on MS-Windows

[Eli Zaretskii]

> From: Fabrice Popineau <fabrice.popineau@gmail.com>
> Date: Fri, 4 Aug 2017 15:30:49 +0200
> Cc: Tino Calancha <tino.calancha@gmail.com>, Emacs developers <emacs-devel@gnu.org>
> 
> Am I wrong thinking that `expand-file-name' is not supposed to expand "dir*/*.txt"?

You are not wrong.  But the problem is not in expand-file-name: it
wasn't supposed to expand the wildcards.  The problem is in dired:
when it uses the 'ls' command, the wildcards are expanded by the
shell.

Re: dired-tests.el fails on MS-Windows

[Tino Calancha]

On Tue, 1 Aug 2017, Eli Zaretskii wrote:

>> From: Tino Calancha <tino.calancha@gmail.com>
>> Cc: emacs-devel@gnu.org, tino.calancha@gmail.com
>> Date: Wed, 02 Aug 2017 02:02:32 +0900
>>
>>> dired-tests.log:
>>>   Test dired-test-bug25609 condition:
>>>       (ert-test-failed
>>>        ((should
>>> 	 (file-exists-p target))
>>> 	:form
>>> 	(file-exists-p "c:/DOCUME~1/Zaretzky/LOCALS~1/Temp/bar6828Ler/foo6828WPJ")
>>> 	:value nil))
>> Could you check the following?
>
> I could, but I don't understand the purpose.  This form is almost
> identical to what's in dired-tests.el, and I already established that
> the failure is indeed because 'target' doesn't exist at that moment.
> I just didn't dig deep enough to understand why, because I didn't
> really understand what the code wants to do, e.g. why it calls
> dired-do-copy twice, and more importantly why 'target' is supposed to
> exist after all that.
>
> What I see here is that at the point where file-exists-p is called,
> there are two directories: /bla/blah/foNNNNNN and /bla/bla/barKKKKKK,
> but not /bla/bla/fooNNNNN/barKKKKKK, as I think the code expects.
>
> maybe if you could explain the idea behind the code I could think of a
> reason why it doesn't work here.
I added more comments and sanity checks in commit
db5d38ddb0de83d8f920b7a128fe3fd5156fdf85
(Fix 2 tests that fail in MS-Windows)
Does it work now in Windows?

Re: dired-tests.el fails on MS-Windows

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Fri, 4 Aug 2017 14:21:01 +0900 (JST)
> cc: Tino Calancha <tino.calancha@gmail.com>, 
>     Emacs developers <emacs-devel@gnu.org>
> 
> > maybe if you could explain the idea behind the code I could think of a
> > reason why it doesn't work here.
> I added more comments and sanity checks in commit
> db5d38ddb0de83d8f920b7a128fe3fd5156fdf85
> (Fix 2 tests that fail in MS-Windows)
> Does it work now in Windows?

It didn't, but given the comments I've now succeeded to understand the
idea of dired-test-bug25609, and fixed it.

dired-test-bug27631 still fails, and it fails because of this:

          (setq buf (dired (expand-file-name "dir*/*.txt" dir)))

ls-lisp signals an error here:

   wrong-type-argument listp "dir*/*.txt"

Didn't you add a feature lately that should support this in ls-lisp?
I guess that feature needs to be turned on for this test to pass on
Windows.

Re: dired-tests.el fails on MS-Windows

[Tino Calancha]

On Fri, 4 Aug 2017, Eli Zaretskii wrote:

>> Does it work now in Windows?
>
> It didn't, but given the comments I've now succeeded to understand the
> idea of dired-test-bug25609, and fixed it.
Great! Thank you.

> dired-test-bug27631 still fails, and it fails because of this:
>
>          (setq buf (dired (expand-file-name "dir*/*.txt" dir)))
>
> ls-lisp signals an error here:
>
>   wrong-type-argument listp "dir*/*.txt"
>
> Didn't you add a feature lately that should support this in ls-lisp?
> I guess that feature needs to be turned on for this test to pass on
> Windows.
Yeah, that test must be skipped when Dired is using 'ls' emulation.  We 
have tests for the same bug in ls-lisp-tests.el and em-ls-tests.el.
I've just pushed a fix.

Re: dired-tests.el fails on MS-Windows

[Fabrice Popineau]

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

2017-08-04 15:14 GMT+02:00 Eli Zaretskii <eliz@gnu.org>:

> > From: Tino Calancha <tino.calancha@gmail.com>
> > Date: Fri, 4 Aug 2017 14:21:01 +0900 (JST)
> > cc: Tino Calancha <tino.calancha@gmail.com>,
> >     Emacs developers <emacs-devel@gnu.org>
> >
> > > maybe if you could explain the idea behind the code I could think of a
> > > reason why it doesn't work here.
> > I added more comments and sanity checks in commit
> > db5d38ddb0de83d8f920b7a128fe3fd5156fdf85
> > (Fix 2 tests that fail in MS-Windows)
> > Does it work now in Windows?
>
> It didn't, but given the comments I've now succeeded to understand the
> idea of dired-test-bug25609, and fixed it.
>
> dired-test-bug27631 still fails, and it fails because of this:
>
>           (setq buf (dired (expand-file-name "dir*/*.txt" dir)))
>
> ls-lisp signals an error here:
>
>    wrong-type-argument listp "dir*/*.txt"
>
> Didn't you add a feature lately that should support this in ls-lisp?
> I guess that feature needs to be turned on for this test to pass on
> Windows.
>
>
Actually, the problem seems to be in the
`insert-directory-wildcard-in-dir-p' function
which wrongly splits "c:/tmp/dir*/*.txt" in ("c:/tmp/" . "dir*/*.txt")
instead of
("c:/tmp/dir*/" . "*.txt")


-- 
Fabrice

[-- Attachment #2: Type: text/html, Size: 2245 bytes --]

Re: dired-tests.el fails on MS-Windows

[Fabrice Popineau]

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

2017-08-04 16:23 GMT+02:00 Fabrice Popineau <fabrice.popineau@gmail.com>:

>
>>
> Actually, the problem seems to be in the `insert-directory-wildcard-in-dir-p'
> function
> which wrongly splits "c:/tmp/dir*/*.txt" in ("c:/tmp/" . "dir*/*.txt")
> instead of
> ("c:/tmp/dir*/" . "*.txt")
>
> Forget this (wrong) diagnostic.

The culprit is actually

(let ((default-directory "c:/tmp/"))
  (eshell-extended-glob "dir*/*.txt"))
"dir*/*.txt"

which fails to expand the wildcards (when `file-expand-wildcards' succeeds).


> --
> Fabrice
>

[-- Attachment #2: Type: text/html, Size: 1911 bytes --]

Re: dired-tests.el fails on MS-Windows

[Tino Calancha]

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



On Fri, 4 Aug 2017, Fabrice Popineau wrote:

> 
> 
> 2017-08-04 16:23 GMT+02:00 Fabrice Popineau <fabrice.popineau@gmail.com>:
> 
> 
> Actually, the problem seems to be in the `insert-directory-wildcard-in-dir-p' function
> which wrongly splits "c:/tmp/dir*/*.txt" in ("c:/tmp/" . "dir*/*.txt") instead of
> ("c:/tmp/dir*/" . "*.txt")
> 
> Forget this (wrong) diagnostic.
> 
> The culprit is actually 
> 
> (let ((default-directory "c:/tmp/"))
>   (eshell-extended-glob "dir*/*.txt"))
> "dir*/*.txt" 
> 
> which fails to expand the wildcards (when `file-expand-wildcards' succeeds).
Thank you Fabrice,
that's interesing.  I am just wondering if `eshell-extended-glob' gets 
confused with the Windows path, i mean, the disk name 'c:' in front.

Could you check if the following works?
M-x eshell RET
cd "c:/tmp"
ls -l dir*/*.txt

I am also curious if:
M-: (equal temporary-file-directory "c:/tmp/") RET
=> t

Thank you,
Tino

Re: dired-tests.el fails on MS-Windows

[Fabrice Popineau]

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

2017-08-04 16:49 GMT+02:00 Tino Calancha <tino.calancha@gmail.com>:

>
>
> On Fri, 4 Aug 2017, Fabrice Popineau wrote:
>
>
>>
>> 2017-08-04 16:23 GMT+02:00 Fabrice Popineau <fabrice.popineau@gmail.com>:
>>
>>
>> Actually, the problem seems to be in the `insert-directory-wildcard-in-dir-p'
>> function
>> which wrongly splits "c:/tmp/dir*/*.txt" in ("c:/tmp/" . "dir*/*.txt")
>> instead of
>> ("c:/tmp/dir*/" . "*.txt")
>>
>> Forget this (wrong) diagnostic.
>>
>> The culprit is actually
>>
>> (let ((default-directory "c:/tmp/"))
>>   (eshell-extended-glob "dir*/*.txt"))
>> "dir*/*.txt"
>>
>> which fails to expand the wildcards (when `file-expand-wildcards'
>> succeeds).
>>
> Thank you Fabrice,
> that's interesing.  I am just wondering if `eshell-extended-glob' gets
> confused with the Windows path, i mean, the disk name 'c:' in front.
>
> Could you check if the following works?
> M-x eshell RET
> cd "c:/tmp"
> ls -l dir*/*.txt
>
> It says :

c:/tmp $ ls -l dir*/*.txt
dir*/*.txt: No such file or directory


I am also curious if:
> M-: (equal temporary-file-directory "c:/tmp/") RET
> => t
>
> Nope, my temp dir in this case is the Windows temp dir.
("c:/Users/Fabrice/AppData/Roaming/Local/Temp/")

And out of curiosity: what does em-glob that file-expand-wildcards doesn't?
Because replacing the former by the latter would withdraw a depency on
eshell parts in ls-lisp :

diff --git a/lisp/ls-lisp.el b/lisp/ls-lisp.el
index 9a4fc19744..7fe2803271 100644
--- a/lisp/ls-lisp.el
+++ b/lisp/ls-lisp.el
@@ -486,7 +486,7 @@ ls-lisp-insert-directory
 ;; before eshell is compiled.
 ;; So instead we add an autoload call here.
 ;; (https://lists.gnu.org/archive/html/emacs-devel/2017-07/msg01083.html).
-(autoload 'eshell-extended-glob "em-glob")
+;; (autoload 'eshell-extended-glob "em-glob")
 (declare-function dired-read-dir-and-switches "dired" (str))
 (declare-function dired-goto-next-file "dired" ())

@@ -499,7 +499,8 @@ ls-lisp--dired
       (if (not dir-wildcard)
           (funcall orig-fun dir-or-list switches)
         (let* ((default-directory (car dir-wildcard))
-               (files (eshell-extended-glob (cdr dir-wildcard)))
+               ;; (files (eshell-extended-glob (cdr dir-wildcard)))
+               (files (file-expand-wildcards (cdr dir-wildcard)))
                (dir (car dir-wildcard)))
           (if files
               (let ((inhibit-read-only t)

(Obviously: the problem in em-glob as to be fixed in any case)

Fabrice

[-- Attachment #2: Type: text/html, Size: 4066 bytes --]

Re: dired-tests.el fails on MS-Windows

[Tino Calancha]

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



On Fri, 4 Aug 2017, Fabrice Popineau wrote:

> 
> 
> 2017-08-04 16:49 GMT+02:00 Tino Calancha <tino.calancha@gmail.com>:
> 
>
>       On Fri, 4 Aug 2017, Fabrice Popineau wrote:
> 
> 
>
>             2017-08-04 16:23 GMT+02:00 Fabrice Popineau <fabrice.popineau@gmail.com>:
> 
>
>             Actually, the problem seems to be in the `insert-directory-wildcard-in-dir-p' function
>             which wrongly splits "c:/tmp/dir*/*.txt" in ("c:/tmp/" . "dir*/*.txt") instead of
>             ("c:/tmp/dir*/" . "*.txt")
>
>             Forget this (wrong) diagnostic.
>
>             The culprit is actually 
>
>             (let ((default-directory "c:/tmp/"))
>               (eshell-extended-glob "dir*/*.txt"))
>             "dir*/*.txt" 
>
>             which fails to expand the wildcards (when `file-expand-wildcards' succeeds).
>
>       Thank you Fabrice,
>       that's interesing.  I am just wondering if `eshell-extended-glob' gets confused with the Windows path, i mean, the
>       disk name 'c:' in front.
>
>       Could you check if the following works?
>       M-x eshell RET
>       cd "c:/tmp"
>       ls -l dir*/*.txt
> 
> It says : 
> 
> c:/tmp $ ls -l dir*/*.txt
> dir*/*.txt: No such file or directory 
I see.  I think hat uses `eshell-extended-glob' as well, so it seems
you find a bug in that function.   Maybe is better to open a bug
report with this eshell recipe above.
> 
>
>       I am also curious if:
>       M-: (equal temporary-file-directory "c:/tmp/") RET
>       => t
> 
> Nope, my temp dir in this case is the Windows temp dir.
> ("c:/Users/Fabrice/AppData/Roaming/Local/Temp/") 
> 
> And out of curiosity: what does em-glob that file-expand-wildcards doesn't? 
> Because replacing the former by the latter would withdraw a depency on eshell parts in ls-lisp :
I think you are right.  We must use `file-expand-wildcards'.
I think the reason i didn't use this func. is because i didn't know
it existance :-)

Re: dired-tests.el fails on MS-Windows

[Fabrice Popineau]

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

I don't get a failure on dired-test-bug25609 with windows 10 and running
from a mingw64 bash.

I get a failure on  dired-test-bug27631 because "/bin/sh" is hardcoded in
lisp/dired.el and the
place for sh.exe in msys2 is in /usr/bin .

I think that the various options for dired ls tests should be split in
different test files (em-ls for example).

Fabrice


2017-08-01 17:22 GMT+02:00 Eli Zaretskii <eliz@gnu.org>:

> The failed tests are shown below.  2 others failed originally, but I
> fixed them.
>
> Tino, please always add comments to the tests explaining the idea
> behind all the tricks and juggling.  Otherwise, it is very hard to
> understand why the test is doing what it's doing and how it is doing
> that.  Same request for ediff-ptch-tests.el, which also fails for me.
>
> Than ks.
>
> dired-tests.log:
>
>   Running 12 tests (2017-08-01 17:44:38+0300)
>      passed   1/12  dired-autoload
>   Marking matching files...
>   Checking d:/gnu/git/emacs/trunk/test/bug22694/test
>   1 matching file marked.
>      passed   2/12  dired-test-bug22694
>   Copy: 1 of 1
>   Copy: 1 file
>   Copy: 1 of 1
>   Copy: 1 file
>   Test dired-test-bug25609 backtrace:
>     signal(ert-test-failed (((should (file-exists-p target)) :form (file
>     ert-fail(((should (file-exists-p target)) :form (file-exists-p "c:/D
>     (if (unwind-protect (setq value-17 (apply fn-15 args-16)) (setq form
>     (let (form-description-19) (if (unwind-protect (setq value-17 (apply
>     (let ((value-17 'ert-form-evaluation-aborted-18)) (let (form-descrip
>     (let ((fn-15 (function file-exists-p)) (args-16 (list target))) (let
>     (progn (let ((fn-15 (function file-exists-p)) (args-16 (list target)
>     (unwind-protect (progn (let ((fn-15 (function file-exists-p)) (args-
>     (let* ((from (make-temp-file "foo" 'dir)) (to (make-temp-file "bar"
>     (closure (t) nil (let* ((from (make-temp-file "foo" 'dir)) (to (make
>     ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
>     ert-run-test(#s(ert-test :name dired-test-bug25609 :documentation "T
>     ert-run-or-rerun-test(#s(ert--stats :selector t :tests [#s(ert-test
>     ert-run-tests(t #f(compiled-function (event-type &rest event-args) #
>     ert-run-tests-batch(nil)
>     ert-run-tests-batch-and-exit(nil)
>     eval((ert-run-tests-batch-and-exit nil))
>     command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/dired-tests.el" "--e
>     command-line()
>     normal-top-level()
>   Test dired-test-bug25609 condition:
>       (ert-test-failed
>        ((should
>          (file-exists-p target))
>         :form
>         (file-exists-p "c:/DOCUME~1/Zaretzky/LOCALS~
> 1/Temp/bar6828Ler/foo6828WPJ")
>         :value nil))
>      FAILED   3/12  dired-test-bug25609
>      passed   4/12  dired-test-bug27243-01
>      passed   5/12  dired-test-bug27243-02
>      passed   6/12  dired-test-bug27243-03
>   Test dired-test-bug27631 backtrace:
>     signal(error ("em-ls is not a currently loaded feature"))
>     error("%s is not a currently loaded feature" "em-ls")
>     unload-feature(em-ls force)
>     (unwind-protect (progn (make-directory dir1) (make-directory dir2) (
>     (let* ((dir (make-temp-file "bug27631" 'dir)) (dir1 (expand-file-nam
>     (closure (t) nil (let* ((dir (make-temp-file "bug27631" 'dir)) (dir1
>     ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
>     ert-run-test(#s(ert-test :name dired-test-bug27631 :documentation "T
>     ert-run-or-rerun-test(#s(ert--stats :selector t :tests [#s(ert-test
>     ert-run-tests(t #f(compiled-function (event-type &rest event-args) #
>     ert-run-tests-batch(nil)
>     ert-run-tests-batch-and-exit(nil)
>     eval((ert-run-tests-batch-and-exit nil))
>     command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/dired-tests.el" "--e
>     command-line()
>     normal-top-level()
>   Test dired-test-bug27631 condition:
>       (error "em-ls is not a currently loaded feature")
>      FAILED   7/12  dired-test-bug27631
>      passed   8/12  dired-test-bug27693
>      failed   9/12  dired-test-bug27762
>      passed  10/12  dired-test-bug27817
>      passed  11/12  dired-test-bug27843
>      passed  12/12  dired-test-bug7131
>
>   Ran 12 tests, 10 results as expected, 2 unexpected (2017-08-01
> 17:44:46+0300)
>   1 expected failures
>
>   2 unexpected results:
>      FAILED  dired-test-bug25609
>      FAILED  dired-test-bug27631
>
> ediff-ptch-tests.log:
>
>   Running 2 tests (2017-08-01 18:21:40+0300)
>      passed  1/2  ediff-ptch-test-bug25010
>   Test ediff-ptch-test-bug26084 backtrace:
>   Test ediff-ptch-test-bug26084 condition:
>       (wrong-type-argument stringp nil)
>      FAILED  2/2  ediff-ptch-test-bug26084
>
>   Ran 2 tests, 1 results as expected, 1 unexpected (2017-08-01
> 18:21:40+0300)
>
>   1 unexpected results:
>      FAILED  ediff-ptch-test-bug26084
>
>
>


-- 
Fabrice Popineau
-----------------------------
CentraleSupelec
Département Informatique
3, rue Joliot Curie
91192 Gif/Yvette Cedex
Tel direct : +33 (0) 169851950
Standard : +33 (0) 169851212
------------------------------

[-- Attachment #2: Type: text/html, Size: 7277 bytes --]

Re: dired-tests.el fails on MS-Windows

[Tino Calancha]

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



On Tue, 1 Aug 2017, Fabrice Popineau wrote:

> I don't get a failure on dired-test-bug25609 with windows 10 and running from a mingw64 bash.
> I get a failure on  dired-test-bug27631 because "/bin/sh" is hardcoded in lisp/dired.el and the
> place for sh.exe in msys2 is in /usr/bin .
Thank you for the comments and suggestions!

I should used 'executable-find' for a local connection.  For a
tramp connection i don't know how to get the 'sh' location in the
remote host: i just kept '/bin/sh' for them.
Michael?

--8<-----------------------------cut here---------------start------------->8---

commit 76f1ea53c7469f7d4c5b9007633642124ae88c62
Author: Tino Calancha <tino.calancha@gmail.com>
Date:   Wed Aug 2 12:27:42 2017 +0900

     Don't assume /bin/sh as the 'sh' location in the local host

     * lisp/dired.el (dired-insert-directory): Use executable-find in
     a local host.

diff --git a/lisp/dired.el b/lisp/dired.el
index 4f8f615a34..a0e1fe185d 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -1281,7 +1281,8 @@ dired-insert-directory
                 (unless
                     (zerop
                      (process-file
-                     "/bin/sh" nil (current-buffer) nil "-c" script))
+                     (or (and (file-remote-p default-directory) "/bin/sh") (executable-find "sh"))
+                     nil (current-buffer) nil "-c" script))
                   (user-error
                    "%s: No files matching wildcard" (cdr dir-wildcard)))
                 (insert-directory-clean (point) switches)))

--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 26.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
  of 2017-08-02
Repository revision: 0fd6de9cb444d6cc553ea67815ccfb7a923012a2

Re: dired-tests.el fails on MS-Windows

[Michael Albinus]

Tino Calancha <tino.calancha@gmail.com> writes:

Hi Tino.

> I should used 'executable-find' for a local connection.  For a
> tramp connection i don't know how to get the 'sh' location in the
> remote host: i just kept '/bin/sh' for them.
> Michael?

"/bin/sh" is OK. The feature works anyway only for methods defined in
tramp-sh.el.

Alternatively, one could take the value of `explicit-shell-file-name',
which is prepared for connection-local variables. But I doubt, that many
people use it this way already.

Best regards, Michael.

Re: dired-tests.el fails on MS-Windows

[Tino Calancha]

On Wed, 2 Aug 2017, Michael Albinus wrote:

> Tino Calancha <tino.calancha@gmail.com> writes:
>
> Hi Tino.
>
>> I should used 'executable-find' for a local connection.  For a
>> tramp connection i don't know how to get the 'sh' location in the
>> remote host: i just kept '/bin/sh' for them.
>> Michael?
>
> "/bin/sh" is OK. The feature works anyway only for methods defined in
> tramp-sh.el.
>
> Alternatively, one could take the value of `explicit-shell-file-name',
> which is prepared for connection-local variables. But I doubt, that many
> people use it this way already.
Thank you.  I pushed in e82c4f56e6
(Don't assume /bin/sh as the 'sh' location in the local host)
a fix: in the local connection check first for a non-nil
value of explicit-shell-file-name to exists; otherwise it
uses (executable-find "sh").