[PATCH] Fix some failing tests in BSD systems

[PATCH] Fix some failing tests in BSD systems

[Daniel Martín]

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


A couple of tests are failing for me on master and a BSD system (macOS):

2 unexpected results:
   FAILED  xref--xref-file-name-display-is-abs
   FAILED  xref--xref-file-name-display-is-relative-to-project-root

An extract of a test failure is

(list-elt 0 (arrays-of-different-length 25 24
"xref-resources//file1.txt" "xref-resources/file1.txt" first-mismatch-at
15))

The reason for the failure is that BSD 'find' sometimes outputs paths
with an extra '/', for example:

find /path/emacs/test/lisp/progmodes/xref-resources/ -type f \( -iname \* \)
/path/emacs/test/lisp/progmodes/xref-resources//file2.txt
/path/emacs/test/lisp/progmodes/xref-resources//file1.txt

(Note the trailing '/' in the first argument to 'find'.)

As I think this differs from GNU find but it's still a valid POSIX path,
I decided to make the tests more flexible.

Could you install the attached patch if you think it does TRT?

Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-some-xref-tests-for-systems-with-BSD-find.patch --]
[-- Type: text/x-patch, Size: 3288 bytes --]

From 6c82722c7ecba28fce619fa4b4fcb3d4425ffa64 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=ADn?= <mardani29@yahoo.es>
Date: Sun, 3 Jan 2021 18:13:07 +0100
Subject: [PATCH] Fix some xref-tests for systems with BSD find

Some versions of BSD find, like those present on macOS systems, may
return a file path with two intermediate
slashes (eg. "path/to//resource").  As this is valid POSIX, just make
the tests more flexible to accommodate for this situation.
---
 test/lisp/progmodes/xref-tests.el | 38 +++++++++++++++++++------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/test/lisp/progmodes/xref-tests.el b/test/lisp/progmodes/xref-tests.el
index eaafc5888c..dd083d655b 100644
--- a/test/lisp/progmodes/xref-tests.el
+++ b/test/lisp/progmodes/xref-tests.el
@@ -99,13 +99,18 @@ xref--buf-pairs-iterator-cleans-up-markers
     (should (null (marker-position (cdr (nth 0 (cdr cons2))))))))
 
 (ert-deftest xref--xref-file-name-display-is-abs ()
-  (let ((xref-file-name-display 'abs))
-    (should (equal (delete-dups
-                    (mapcar 'xref-location-group
-                            (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)")))
-                   (list
-                    (concat xref-tests--data-dir "file1.txt")
-                    (concat xref-tests--data-dir "file2.txt"))))))
+  (let ((xref-file-name-display 'abs)
+        ;; BSD find may add an extra '/' to the path.
+        (expected (list
+                   (concat xref-tests--data-dir "/?file1.txt")
+                   (concat xref-tests--data-dir "/?file2.txt")))
+        (actual (delete-dups
+                 (mapcar 'xref-location-group
+                         (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)")))))
+    (should (and (= (length expected) (length actual))
+                 (cl-every (lambda (e1 e2)
+                             (string-match-p e1 e2))
+                           expected actual)))))
 
 (ert-deftest xref--xref-file-name-display-is-nondirectory ()
   (let ((xref-file-name-display 'nondirectory))
@@ -121,10 +126,15 @@ xref--xref-file-name-display-is-relative-to-project-root
           (file-name-directory (directory-file-name xref-tests--data-dir)))
          (project-find-functions
           #'(lambda (_) (cons 'transient data-parent-dir)))
-        (xref-file-name-display 'project-relative))
-    (should (equal (delete-dups
-                    (mapcar 'xref-location-group
-                            (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)")))
-                   (list
-                    "xref-resources/file1.txt"
-                    "xref-resources/file2.txt")))))
+         (xref-file-name-display 'project-relative)
+         ;; BSD find may add an extra '/' to the path.
+         (expected (list
+                    "xref-resources//?file1.txt"
+                    "xref-resources//?file2.txt"))
+         (actual (delete-dups
+                  (mapcar 'xref-location-group
+                          (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)")))))
+    (should (and (= (length expected) (length actual))
+                 (cl-every (lambda (e1 e2)
+                             (string-match-p e1 e2))
+                           expected actual)))))
-- 
2.28.0

Re: [PATCH] Fix some failing tests in BSD systems

[Eli Zaretskii]

> From: Daniel Martín <mardani29@yahoo.es>
> Date: Sun, 03 Jan 2021 18:16:46 +0100
> 
>  (ert-deftest xref--xref-file-name-display-is-abs ()
> -  (let ((xref-file-name-display 'abs))
> -    (should (equal (delete-dups
> -                    (mapcar 'xref-location-group
> -                            (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)")))
> -                   (list
> -                    (concat xref-tests--data-dir "file1.txt")
> -                    (concat xref-tests--data-dir "file2.txt"))))))
> +  (let ((xref-file-name-display 'abs)
> +        ;; BSD find may add an extra '/' to the path.
> +        (expected (list
> +                   (concat xref-tests--data-dir "/?file1.txt")
> +                   (concat xref-tests--data-dir "/?file2.txt")))

Why are we producing file names by concatenation, instead of calling
expand-file-name?  Wouldn't using the latter solve the problem?  Or
what did I miss?

Re: [PATCH] Fix some failing tests in BSD systems

[Daniel Martín]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Daniel Martín <mardani29@yahoo.es>
>> Date: Sun, 03 Jan 2021 18:16:46 +0100
>> 
>>  (ert-deftest xref--xref-file-name-display-is-abs ()
>> -  (let ((xref-file-name-display 'abs))
>> -    (should (equal (delete-dups
>> -                    (mapcar 'xref-location-group
>> -                            (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)")))
>> -                   (list
>> -                    (concat xref-tests--data-dir "file1.txt")
>> -                    (concat xref-tests--data-dir "file2.txt"))))))
>> +  (let ((xref-file-name-display 'abs)
>> +        ;; BSD find may add an extra '/' to the path.
>> +        (expected (list
>> +                   (concat xref-tests--data-dir "/?file1.txt")
>> +                   (concat xref-tests--data-dir "/?file2.txt")))
>
> Why are we producing file names by concatenation, instead of calling
> expand-file-name?  Wouldn't using the latter solve the problem?  Or
> what did I miss?

Yes.  I've attached another solution in terms of expand-file-name and
file-relative-name.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-some-xref-tests-for-systems-with-BSD-find.patch --]
[-- Type: text/x-patch, Size: 2608 bytes --]

From 4a6eabf615d6b82d4dc570afe801833dd03295ff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=ADn?= <mardani29@yahoo.es>
Date: Sun, 3 Jan 2021 21:49:22 +0100
Subject: [PATCH] Fix some xref-tests for systems with BSD find

Some versions of BSD find, like those present on macOS systems, may
return a file path with two intermediate
slashes (eg. "path/to//resource").  As this is valid POSIX, just make
the tests build their paths in a more portable way.
---
 test/lisp/progmodes/xref-tests.el | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/test/lisp/progmodes/xref-tests.el b/test/lisp/progmodes/xref-tests.el
index eaafc5888c..32cb2960a1 100644
--- a/test/lisp/progmodes/xref-tests.el
+++ b/test/lisp/progmodes/xref-tests.el
@@ -101,11 +101,13 @@ xref--buf-pairs-iterator-cleans-up-markers
 (ert-deftest xref--xref-file-name-display-is-abs ()
   (let ((xref-file-name-display 'abs))
     (should (equal (delete-dups
-                    (mapcar 'xref-location-group
-                            (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)")))
+                    (mapcar
+                     (lambda (e)
+                       (expand-file-name (xref-location-group e)))
+                     (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)")))
                    (list
-                    (concat xref-tests--data-dir "file1.txt")
-                    (concat xref-tests--data-dir "file2.txt"))))))
+                    (expand-file-name "file1.txt" xref-tests--data-dir)
+                    (expand-file-name "file2.txt" xref-tests--data-dir))))))
 
 (ert-deftest xref--xref-file-name-display-is-nondirectory ()
   (let ((xref-file-name-display 'nondirectory))
@@ -121,10 +123,12 @@ xref--xref-file-name-display-is-relative-to-project-root
           (file-name-directory (directory-file-name xref-tests--data-dir)))
          (project-find-functions
           #'(lambda (_) (cons 'transient data-parent-dir)))
-        (xref-file-name-display 'project-relative))
+         (xref-file-name-display 'project-relative))
     (should (equal (delete-dups
-                    (mapcar 'xref-location-group
-                            (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)")))
+                    (mapcar
+                     (lambda (e)
+                       (file-relative-name (xref-location-group e)))
+                     (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)")))
                    (list
                     "xref-resources/file1.txt"
                     "xref-resources/file2.txt")))))
-- 
2.28.0

Re: [PATCH] Fix some failing tests in BSD systems

[Dmitry Gutov]

On 03.01.2021 19:35, Eli Zaretskii wrote:
> Why are we producing file names by concatenation, instead of calling
> expand-file-name?  Wouldn't using the latter solve the problem?  Or
> what did I miss?

While we sometimes do that (expand-file-name is not free, and for 
certain uses its overhead counts), that doesn't seem to be the case here.

The double slash here is produced by an external program, not by 
concatenation.

Re: [PATCH] Fix some failing tests in BSD systems

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 4 Jan 2021 03:41:20 +0200
> 
> On 03.01.2021 19:35, Eli Zaretskii wrote:
> > Why are we producing file names by concatenation, instead of calling
> > expand-file-name?  Wouldn't using the latter solve the problem?  Or
> > what did I miss?
> 
> While we sometimes do that (expand-file-name is not free, and for 
> certain uses its overhead counts), that doesn't seem to be the case here.

Not sure what you mean: I clearly saw calls to concat in the patch,
both in the old and the new code.  What did I miss?

> The double slash here is produced by an external program, not by 
> concatenation.

I understand that much, but expand-file-name removes the double
slashes as part of its job, doesn't it?

Re: [PATCH] Fix some failing tests in BSD systems

[Dmitry Gutov]

On 04.01.2021 05:30, Eli Zaretskii wrote:
>> Cc: emacs-devel@gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Mon, 4 Jan 2021 03:41:20 +0200
>>
>> On 03.01.2021 19:35, Eli Zaretskii wrote:
>>> Why are we producing file names by concatenation, instead of calling
>>> expand-file-name?  Wouldn't using the latter solve the problem?  Or
>>> what did I miss?
>>
>> While we sometimes do that (expand-file-name is not free, and for
>> certain uses its overhead counts), that doesn't seem to be the case here.
> 
> Not sure what you mean: I clearly saw calls to concat in the patch,
> both in the old and the new code.  What did I miss?

The changes you are referring to are both in the test code. There's 
nothing inherently wrong in using concat in such code, as long as we 
test what we want to test.

>> The double slash here is produced by an external program, not by
>> concatenation.
> 
> I understand that much, but expand-file-name removes the double
> slashes as part of its job, doesn't it?

Yes, so?

If you actually meant to suggest an alternative comparison strategy 
(like in Daniel's latest patch), then I think the test becomes 
unnecessarily lax. After all, we want to also check how the string 
"looks" (because the user will see it verbatim), not just what file it 
points at.

Re: [PATCH] Fix some failing tests in BSD systems

[Eli Zaretskii]

> Cc: mardani29@yahoo.es, emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 4 Jan 2021 15:10:40 +0200
> 
> >> On 03.01.2021 19:35, Eli Zaretskii wrote:
> >>> Why are we producing file names by concatenation, instead of calling
> >>> expand-file-name?  Wouldn't using the latter solve the problem?  Or
> >>> what did I miss?
> >>
> >> While we sometimes do that (expand-file-name is not free, and for
> >> certain uses its overhead counts), that doesn't seem to be the case here.
> > 
> > Not sure what you mean: I clearly saw calls to concat in the patch,
> > both in the old and the new code.  What did I miss?
> 
> The changes you are referring to are both in the test code. There's 
> nothing inherently wrong in using concat in such code, as long as we 
> test what we want to test.

There's nothing wrong with that, sure, but you risk false failures due
to issues like the one here.  However, if you are okay with that, I
don't mind.  It just was strange to see such code.

> If you actually meant to suggest an alternative comparison strategy 
> (like in Daniel's latest patch), then I think the test becomes 
> unnecessarily lax. After all, we want to also check how the string 
> "looks" (because the user will see it verbatim), not just what file it 
> points at.

I don't think you can reliably know how the file name will be
formatted by a particular variety of 'find', not IME.  But I'm not
going to argue about this.

Re: [PATCH] Fix some failing tests in BSD systems

[Dmitry Gutov]

On 04.01.2021 17:52, Eli Zaretskii wrote:
>> The changes you are referring to are both in the test code. There's
>> nothing inherently wrong in using concat in such code, as long as we
>> test what we want to test.
> 
> There's nothing wrong with that, sure, but you risk false failures due
> to issues like the one here.  However, if you are okay with that, I
> don't mind.  It just was strange to see such code.

It's a matter of the interpretation. For one, if I saw this behavior 
with GNU find (which is installed on my machine), I'd want to fix it 
rather than just update the test.

No harm in doing that just for BSD find either.

>> If you actually meant to suggest an alternative comparison strategy
>> (like in Daniel's latest patch), then I think the test becomes
>> unnecessarily lax. After all, we want to also check how the string
>> "looks" (because the user will see it verbatim), not just what file it
>> points at.
> 
> I don't think you can reliably know how the file name will be
> formatted by a particular variety of 'find', not IME.  But I'm not
> going to argue about this.

I think we can try.

The fact that we've only seen such a report now is a good indicator.

So, Daniel, how does the patch below work for you? It should also fix 
the same behavior in dired-do-find-regexp.

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index d417382c0d..a87f054146 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -291,7 +291,8 @@ project--files-in-directory
           (localdir (file-local-name (expand-file-name dir)))
           (command (format "%s %s %s -type f %s -print0"
                            find-program
-                          localdir
+                          ;; Omit the / for BSD find's sake.
+                          (directory-file-name localdir)
                            (xref--find-ignores-arguments ignores localdir)
                            (if files
                                (concat (shell-quote-argument "(")
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index d2b5acd555..32efc2807c 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1374,7 +1374,9 @@ xref-matches-in-directory
         ;; do that reliably enough, without creating false negatives?
         (command (xref--rgrep-command (xref--regexp-to-extended regexp)
                                       files
-                                     (file-local-name (expand-file-name 
dir))
+                                     ;; Omit the / for BSD find's sake.
+                                     (directory-file-name
+                                      (file-local-name 
(expand-file-name dir)))
                                       ignores))
         (def default-directory)
         (buf (get-buffer-create " *xref-grep*"))

Re: [PATCH] Fix some failing tests in BSD systems

[Daniel Martín]

Dmitry Gutov <dgutov@yandex.ru> writes:

>
> So, Daniel, how does the patch below work for you? It should also fix
> the same behavior in dired-do-find-regexp.

Thanks, it fixes the test failures for me.  However, I still see the
extra '/' if I follow these steps:

- make TAGS
- emacs -Q
- Visit src/xdisp.c
- M-? in redisplay_internal

Perhaps there are more places that need that patch.

Re: [PATCH] Fix some failing tests in BSD systems

[Dmitry Gutov]

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

On 05.01.2021 00:25, Daniel Martín wrote:
> Thanks, it fixes the test failures for me.  However, I still see the
> extra '/' if I follow these steps:
> 
> - make TAGS
> - emacs -Q
> - Visit src/xdisp.c
> - M-? in redisplay_internal
> 
> Perhaps there are more places that need that patch.

Thanks, that's the third place, and probably the final one.

In the meantime, plot thickens: as pointed out privately by Davis 
Herring, using 'find' on a directory without a trailing slash will fail 
if the directory is a symlink (it will only list the symlink itself).

At this point we could double down on this approach and use the '-H' 
argument (though grep-find-template doesn't make this easy), or return 
to your original patch. Or yet alternatively, paper over this inside the 
xref-location-group defmethod. The two alternatives attached.

So I guess I should ask: was the particular behavior annoying by itself, 
or is it only a problem because of the failing test?

I also wonder whether all macOS users see this, or whether it's maybe 
fixed in some latest version of BSD find. Because it's apparently 
(arguably) a bug: https://unix.stackexchange.com/a/320440/166215

[-- Attachment #2: find-no-doubleslash-h.diff --]
[-- Type: text/x-patch, Size: 2953 bytes --]

diff --git a/lisp/cedet/semantic/symref/grep.el b/lisp/cedet/semantic/symref/grep.el
index 5f9a3fa352..21f075041c 100644
--- a/lisp/cedet/semantic/symref/grep.el
+++ b/lisp/cedet/semantic/symref/grep.el
@@ -119,12 +119,15 @@ semantic-symref-grep-use-template
                    ;; it to the user.  By contrast, here we don't show
                    ;; the output, and the SGR escapes get in the way
                    ;; of parsing the output.
-                   (replace-regexp-in-string "--color=always" ""
-                                             grep-find-template t t)
+                   (replace-regexp-in-string
+                    "<D>" "-H <D>"
+                    (replace-regexp-in-string "--color=always" ""
+                                              grep-find-template t t)
+                    t t)
                  grep-find-template)
                pattern
                filepattern
-               rootdir)))
+               (directory-file-name rootdir))))
     cmd))
 
 (defcustom semantic-symref-grep-shell shell-file-name
diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index d417382c0d..04546d1e4d 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -289,9 +289,9 @@ project--files-in-directory
          ;; expanded and not left for the shell command
          ;; to interpret.
          (localdir (file-local-name (expand-file-name dir)))
-         (command (format "%s %s %s -type f %s -print0"
+         (command (format "%s -H %s %s -type f %s -print0"
                           find-program
-                          localdir
+                          (directory-file-name localdir)
                           (xref--find-ignores-arguments ignores localdir)
                           (if files
                               (concat (shell-quote-argument "(")
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index d2b5acd555..13fc2f09c7 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1374,7 +1374,8 @@ xref-matches-in-directory
        ;; do that reliably enough, without creating false negatives?
        (command (xref--rgrep-command (xref--regexp-to-extended regexp)
                                      files
-                                     (file-local-name (expand-file-name dir))
+                                     (directory-file-name
+                                      (file-local-name (expand-file-name dir)))
                                      ignores))
        (def default-directory)
        (buf (get-buffer-create " *xref-grep*"))
@@ -1543,7 +1544,10 @@ xref--rgrep-command
   ;; `shell-quote-argument' quotes the tilde as well.
   (cl-assert (not (string-match-p "\\`~" dir)))
   (grep-expand-template
-   grep-find-template
+   (replace-regexp-in-string
+    "<D>" "-H <D>"
+    grep-find-template
+    t t)
    regexp
    (concat (shell-quote-argument "(")
            " " find-name-arg " "

[-- Attachment #3: find-no-doubleslash-postprocess.diff --]
[-- Type: text/x-patch, Size: 3355 bytes --]

diff --git a/lisp/cedet/semantic/symref/grep.el b/lisp/cedet/semantic/symref/grep.el
index 5f9a3fa352..9f0ac38ec7 100644
--- a/lisp/cedet/semantic/symref/grep.el
+++ b/lisp/cedet/semantic/symref/grep.el
@@ -168,7 +168,8 @@ semantic-symref-perform-search
       (erase-buffer)
       (setq default-directory rootdir)
       (let ((cmd (semantic-symref-grep-use-template
-                  (file-local-name rootdir) filepattern grepflags greppat)))
+                  (file-name-as-directory (file-local-name rootdir))
+                  filepattern grepflags greppat)))
         (process-file semantic-symref-grep-shell nil b nil
                       shell-command-switch cmd)))
     (setq ans (semantic-symref-parse-tool-output tool b))
diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index d417382c0d..f91246a885 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -291,7 +291,7 @@ project--files-in-directory
          (localdir (file-local-name (expand-file-name dir)))
          (command (format "%s %s %s -type f %s -print0"
                           find-program
-                          localdir
+                          (file-name-as-directory localdir)
                           (xref--find-ignores-arguments ignores localdir)
                           (if files
                               (concat (shell-quote-argument "(")
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index d2b5acd555..bacc5e6f1c 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -161,11 +161,13 @@ xref--project-root-memo
   "Cons mapping `default-directory' value to the search root.")
 
 (cl-defmethod xref-location-group ((l xref-file-location))
+  (let ((file (oref l file)))
+    (unless (eq xref-file-name-display 'abs)
+      (setq file (replace-regexp-in-string "//" "/" file t t)))
   (cl-ecase xref-file-name-display
-    (abs
-     (oref l file))
+    (abs file)
     (nondirectory
-     (file-name-nondirectory (oref l file)))
+     (file-name-nondirectory file))
     (project-relative
      (unless (and xref--project-root-memo
                   (equal (car xref--project-root-memo)
@@ -176,12 +178,12 @@ xref-location-group
                           (let ((pr (project-current)))
                             (and pr (xref--project-root pr)))))
                      (and root (expand-file-name root))))))
-     (let ((file (oref l file))
+     (let ((file file)
            (search-root (cdr xref--project-root-memo)))
        (if (and search-root
                 (string-prefix-p search-root file))
            (substring file (length search-root))
-         file)))))
+         file))))))
 
 (defclass xref-buffer-location (xref-location)
   ((buffer :type buffer :initarg :buffer)
@@ -1374,7 +1376,8 @@ xref-matches-in-directory
        ;; do that reliably enough, without creating false negatives?
        (command (xref--rgrep-command (xref--regexp-to-extended regexp)
                                      files
-                                     (file-local-name (expand-file-name dir))
+                                     (file-name-as-directory
+                                      (file-local-name (expand-file-name dir)))
                                      ignores))
        (def default-directory)
        (buf (get-buffer-create " *xref-grep*"))

Re: [PATCH] Fix some failing tests in BSD systems

[Daniel Martín]

Dmitry Gutov <dgutov@yandex.ru> writes:

>
> In the meantime, plot thickens: as pointed out privately by Davis
> Herring, using 'find' on a directory without a trailing slash will
> fail if the directory is a symlink (it will only list the symlink
> itself).
>
> At this point we could double down on this approach and use the '-H'
> argument (though grep-find-template doesn't make this easy), or return 
> to your original patch. Or yet alternatively, paper over this inside
> the xref-location-group defmethod. The two alternatives attached.
>
> So I guess I should ask: was the particular behavior annoying by
> itself, or is it only a problem because of the failing test?
>
> I also wonder whether all macOS users see this, or whether it's maybe
> fixed in some latest version of BSD find. Because it's apparently 
> (arguably) a bug: https://unix.stackexchange.com/a/320440/166215

I think the safest and simplest route is to just "fix" the tests so they
pass in all systems.  We shouldn't risk introducing regressions just to
work around what POSIX says could be a 'find' bug in some BSD systems
(and maybe only Apple systems, I don't have any other BSD system to test
with).

It only causes a slight cosmetic problem, which should be fixed whenever
they fix the bug, without needing any code change from our side.

Re: [PATCH] Fix some failing tests in BSD systems

[Dmitry Gutov]

On 06.01.2021 11:41, Daniel Martín wrote:

>> So I guess I should ask: was the particular behavior annoying by
>> itself, or is it only a problem because of the failing test?
>>
>> I also wonder whether all macOS users see this, or whether it's maybe
>> fixed in some latest version of BSD find. Because it's apparently
>> (arguably) a bug: https://unix.stackexchange.com/a/320440/166215
> 
> I think the safest and simplest route is to just "fix" the tests so they
> pass in all systems.  We shouldn't risk introducing regressions just to
> work around what POSIX says could be a 'find' bug in some BSD systems
> (and maybe only Apple systems, I don't have any other BSD system to test
> with).

Thanks, that makes sense. We do try to work around problems in widely 
installed software, but so far it seems Apple-only.

I've tried a few major versions of FreeBSD in a VM, going back to 11.4, 
and none have exhibited the problem. Guess we'll do the fix for the 
symlinks problem instead (the easy way), and then wait and see whether 
more reports come complaining about the double-slash issue.

> It only causes a slight cosmetic problem, which should be fixed whenever
> they fix the bug, without needing any code change from our side.

AFAIK Apple tries very hard to avoid updating to the latest versions of 
Unixy tools.

Luckily, there are now several projects that let users install fresh GNU 
versions of said tools (and other command-line programs).

Re: [PATCH] Fix some failing tests in BSD systems

[Stefan Monnier]

>> It only causes a slight cosmetic problem, which should be fixed whenever
>> they fix the bug, without needing any code change from our side.
> AFAIK Apple tries very hard to avoid updating to the latest versions of
> Unixy tools.

AFAIK what they avoid like the plague is GPLv3, so they stick to the
last release of GNU tools that used GPLv2 (and/or switch to tools using
non-copyleft licenses, like their switch from bash to zsh).


        Stefan

Re: [PATCH] Fix some failing tests in BSD systems

[Dmitry Gutov]

On 06.01.2021 20:48, Stefan Monnier wrote:
> AFAIK what they avoid like the plague is GPLv3, so they stick to the
> last release of GNU tools that used GPLv2 (and/or switch to tools using
> non-copyleft licenses, like their switch from bash to zsh).

Right, but if they are using a BSD find, they could upgrade it to some 
recent version (like ones found in FreeBSD 11+). Or switch to it.

Re: [PATCH] Fix some failing tests in BSD systems

[Dmitry Gutov]

Hi Daniel,

On 03.01.2021 19:16, Daniel Martín wrote:
> 
> A couple of tests are failing for me on master and a BSD system (macOS):
> 
> 2 unexpected results:
>     FAILED  xref--xref-file-name-display-is-abs
>     FAILED  xref--xref-file-name-display-is-relative-to-project-root
> 
> An extract of a test failure is
> 
> (list-elt 0 (arrays-of-different-length 25 24
> "xref-resources//file1.txt" "xref-resources/file1.txt" first-mismatch-at
> 15))
> 
> The reason for the failure is that BSD 'find' sometimes outputs paths
> with an extra '/', for example:
> 
> find /path/emacs/test/lisp/progmodes/xref-resources/ -type f \( -iname \* \)
> /path/emacs/test/lisp/progmodes/xref-resources//file2.txt
> /path/emacs/test/lisp/progmodes/xref-resources//file1.txt
> 
> (Note the trailing '/' in the first argument to 'find'.)
> 
> As I think this differs from GNU find but it's still a valid POSIX path,
> I decided to make the tests more flexible.

I don't mind the proposed change to the tests, but if this is the only 
situation when the BSD find behaves differently, we could try to avoid 
it like this:

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index d2b5acd555..019c9a35bc 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1374,7 +1374,8 @@ xref-matches-in-directory
         ;; do that reliably enough, without creating false negatives?
         (command (xref--rgrep-command (xref--regexp-to-extended regexp)
                                       files
-                                     (file-local-name (expand-file-name 
dir))
+                                     (directory-file-name
+                                      (file-local-name 
(expand-file-name dir)))
                                       ignores))
         (def default-directory)
         (buf (get-buffer-create " *xref-grep*"))

Re: [PATCH] Fix some failing tests in BSD systems

[Stefan Monnier]

> I don't mind the proposed change to the tests, but if this is the only
> situation when the BSD find behaves differently, we could try to avoid it
> like this:

It seems the most "frugal" fix, indeed.  But it deserves a comment
explaining the subtle choice.


        Stefan