bug#66542: Fix: locate-dominating-file predicate should receive dir not file

bug#66542: Fix: locate-dominating-file predicate should receive dir not file

[dalanicolai]

[-- Attachment #1.1: Type: text/plain, Size: 1153 bytes --]

Tags: patch

Tags: patch

The docstring of 'locate-dominating-file' mentions that its NAME argument
should be a dir, but currently it simply receives the FILE
argument. Therefore, using the function e.g. with the following predicate
for NAME

(lambda (dir)
(seq-filter (apply-partially #'string-match-p "paint")
(directory-files
dir)))

to check if a directory contains a file regexp-matching 'paint', throws
an error:

(file-error "Opening directory" "Not a directory" "/home/...")

This patch simply wraps the FILE argument in the (funcall NAME FILE) with
a 'file-name-directory' thereby fixing the function.

The commit log entry could read:
FIX: ensure that locate-dominating-file predicate recives dir


In GNU Emacs 29.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.38,
 cairo version 1.17.8) of 2023-08-09 built on
 2a02-a45d-af56-1-666c-72af-583a-b92d.fixed6.kpn.net
Repository revision: 31cef9a4eac01fff5ff4fcb89d7e2b7815e93bad
Repository branch: HEAD
System Description: Fedora Linux 38 (Workstation Edition)

Configured using:
 'configure --with-tree-sitter --with-modules --with-cairo
 --with-native-compilation --with-json --with-pgtk'

[-- Attachment #1.2: Type: text/html, Size: 1506 bytes --]

[-- Attachment #2: locate-dominating-file-patch --]
[-- Type: application/octet-stream, Size: 590 bytes --]

diff --git a/lisp/files.el b/lisp/files.el
index e1421b403bf..19c88e5bc8a 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -1138,7 +1138,7 @@ locate-dominating-file
       (setq try (if (stringp name)
                     (and (file-directory-p file)
                          (file-exists-p (expand-file-name name file)))
-                  (funcall name file)))
+                  (funcall name (file-name-directory file))))
       (cond (try (setq root file))
             ((equal file (setq file (file-name-directory
                                      (directory-file-name file))))

bug#66542: Fix: locate-dominating-file predicate should receive dir not file

[Eli Zaretskii]

> From: dalanicolai <dalanicolai@gmail.com>
> Date: Sat, 14 Oct 2023 17:11:18 +0200
> 
> The docstring of 'locate-dominating-file' mentions that its NAME argument
> should be a dir, but currently it simply receives the FILE
> argument. Therefore, using the function e.g. with the following predicate for NAME
> 
> (lambda (dir)
> (seq-filter (apply-partially #'string-match-p "paint")
> (directory-files
> dir)))
> 
> to check if a directory contains a file regexp-matching 'paint', throws
> an error:
> 
> (file-error "Opening directory" "Not a directory" "/home/...")
> 
> This patch simply wraps the FILE argument in the (funcall NAME FILE) with
> a 'file-name-directory' thereby fixing the function.

Sorry, I don't understand: the return value of file-name-directory is
not identical to its argument when the argument is a directory, so
this patch might change the behavior.  Or what am I missing?

In addition, I don't think I understand the problem you are trying to
solve.  Could you please describe the problem more completely, by
telling when and how was locate-dominating-file called with the name
of a file that is not a directory?

bug#66542: Fix: locate-dominating-file predicate should receive dir not file

[dalanicolai]

[-- Attachment #1.1: Type: text/plain, Size: 2198 bytes --]

You are right (of course :)
So I have attached another patch which only strips the file name (applies
file-name-directory) if the file is not a directory.

If the file is not a directory, then that stripped name should also be
returned by the function, i.e. in the 'cond`, root should be set to the
stripped file name. Therefore, the patched version simply 'checks and sets'
the file name first inside the setq.

Also, now we can remove the check in the first clause of the 'if' (of 'setq
try'), because 'file' is guaranteed to be a directory name, and the
existence of the directory name is already checked by the 'file-exists-p'.

This change does not affect other parts of the function (except that it
speeds it up a little, because it excludes the cycles, that only strip the
file names in case file is a 'real' file.
Indeed, the function should only check for 'parent directories' (not files).

I hope I did not miss anything.


On Sat, 14 Oct 2023 at 17:46, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: dalanicolai <dalanicolai@gmail.com>
> > Date: Sat, 14 Oct 2023 17:11:18 +0200
> >
> > The docstring of 'locate-dominating-file' mentions that its NAME argument
> > should be a dir, but currently it simply receives the FILE
> > argument. Therefore, using the function e.g. with the following
> predicate for NAME
> >
> > (lambda (dir)
> > (seq-filter (apply-partially #'string-match-p "paint")
> > (directory-files
> > dir)))
> >
> > to check if a directory contains a file regexp-matching 'paint', throws
> > an error:
> >
> > (file-error "Opening directory" "Not a directory" "/home/...")
> >
> > This patch simply wraps the FILE argument in the (funcall NAME FILE) with
> > a 'file-name-directory' thereby fixing the function.
>
> Sorry, I don't understand: the return value of file-name-directory is
> not identical to its argument when the argument is a directory, so
> this patch might change the behavior.  Or what am I missing?
>
> In addition, I don't think I understand the problem you are trying to
> solve.  Could you please describe the problem more completely, by
> telling when and how was locate-dominating-file called with the name
> of a file that is not a directory?
>

[-- Attachment #1.2: Type: text/html, Size: 2966 bytes --]

[-- Attachment #2: locate-dominating-file-patch --]
[-- Type: application/octet-stream, Size: 819 bytes --]

diff --git a/lisp/files.el b/lisp/files.el
index e1421b403bf..4e3c9699852 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -1135,9 +1135,11 @@ locate-dominating-file
     (while (not (or root
                     (null file)
                     (string-match locate-dominating-stop-dir-regexp file)))
-      (setq try (if (stringp name)
-                    (and (file-directory-p file)
-                         (file-exists-p (expand-file-name name file)))
+      (setq file (if (file-directory-p file)
+                     file
+                   (file-name-directory file))
+            try (if (stringp name)
+                    (file-exists-p (expand-file-name name file))
                   (funcall name file)))
       (cond (try (setq root file))
             ((equal file (setq file (file-name-directory

bug#66542: Fix: locate-dominating-file predicate should receive dir not file

[Eli Zaretskii]

> From: dalanicolai <dalanicolai@gmail.com>
> Date: Sat, 21 Oct 2023 17:39:17 +0200
> Cc: 66542@debbugs.gnu.org
> 
> You are right (of course :)
> So I have attached another patch which only strips the file name (applies file-name-directory) if the file
> is not a directory.
> 
> If the file is not a directory, then that stripped name should also be returned by the function, i.e. in the
> 'cond`, root should be set to the stripped file name. Therefore, the patched version simply 'checks
> and sets' the file name first inside the setq.
> 
> Also, now we can remove the check in the first clause of the 'if' (of 'setq try'), because 'file' is
> guaranteed to be a directory name, and the existence of the directory name is already checked by
> the 'file-exists-p'.
> 
> This change does not affect other parts of the function (except that it speeds it up a little, because it
> excludes the cycles, that only strip the file names in case file is a 'real' file.
> Indeed, the function should only check for 'parent directories' (not files).

Thanks, installed on master, and closing the bug.

bug#66542: Fix: locate-dominating-file predicate should receive dir not file

[dalanicolai]

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

Thank you Eli!

On Wed, 25 Oct 2023 at 15:15, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: dalanicolai <dalanicolai@gmail.com>
> > Date: Sat, 21 Oct 2023 17:39:17 +0200
> > Cc: 66542@debbugs.gnu.org
> >
> > You are right (of course :)
> > So I have attached another patch which only strips the file name
> (applies file-name-directory) if the file
> > is not a directory.
> >
> > If the file is not a directory, then that stripped name should also be
> returned by the function, i.e. in the
> > 'cond`, root should be set to the stripped file name. Therefore, the
> patched version simply 'checks
> > and sets' the file name first inside the setq.
> >
> > Also, now we can remove the check in the first clause of the 'if' (of
> 'setq try'), because 'file' is
> > guaranteed to be a directory name, and the existence of the directory
> name is already checked by
> > the 'file-exists-p'.
> >
> > This change does not affect other parts of the function (except that it
> speeds it up a little, because it
> > excludes the cycles, that only strip the file names in case file is a
> 'real' file.
> > Indeed, the function should only check for 'parent directories' (not
> files).
>
> Thanks, installed on master, and closing the bug.
>

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