GNU bug report logs - #27798
Documentation of locate-dominating-file is wrong

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 27798 in the body.
You can then email your comments to 27798 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Clément Pit--Claudel <clement.pitclaudel <at> live.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Documentation of locate-dominating-file is wrong
Date: Sun, 23 Jul 2017 11:49:36 +0200

[Message part 1 (text/plain, inline)]

The docs of locate-dominating-file say this:

  (locate-dominating-file FILE NAME)

  Look up the directory hierarchy from FILE for a directory containing NAME.
  Stop at the first parent directory containing a file NAME,
  and return the directory.  Return nil if not found.
  Instead of a string, NAME can also be a predicate taking one argument
  (a directory) and returning a non-nil value if that directory is the one for
  which we’re looking.

This part is wrong, because locate-dominating-file also accepts directories:

  Look up the directory hierarchy from FILE

This part is wrong, because the predicate is called with the initial file name, too:

  NAME can also be a predicate taking one argument (a directory)

Indeed, the following form:

  (locate-dominating-file "/usr/local/bin/emacs" (lambda (x) (ignore (message "%s" x))))

prints this:

  /usr/local/bin/emacs
  /usr/local/bin/
  /usr/local/
  /usr/
  /

(not the file name passed into the first call.

I think the fix is to update the docs in both places, as there might be callers relying on the existing behavior.  It's important to also document that passing a directory name is OK, though, because that's the only way to use locate-dominating-file reliably with a directory-only predicate — otherwise, the predicate needs to handle both files and folders.

Cheers,
Clément.

[signature.asc (application/pgp-signature, attachment)]

Message #8 received at 27798 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Clément Pit--Claudel <clement.pitclaudel <at> live.com>
Cc: 27798 <at> debbugs.gnu.org
Subject: Re: bug#27798: Documentation of locate-dominating-file is wrong
Date: Sun, 23 Jul 2017 17:31:51 +0300

> From: Clément Pit--Claudel <clement.pitclaudel <at> live.com>
> Date: Sun, 23 Jul 2017 11:49:36 +0200
> 
>   (locate-dominating-file FILE NAME)
> 
>   Look up the directory hierarchy from FILE for a directory containing NAME.
>   Stop at the first parent directory containing a file NAME,
>   and return the directory.  Return nil if not found.
>   Instead of a string, NAME can also be a predicate taking one argument
>   (a directory) and returning a non-nil value if that directory is the one for
>   which we’re looking.
> 
> This part is wrong, because locate-dominating-file also accepts directories:
> 
>   Look up the directory hierarchy from FILE

Actually, FILE _must_ be a directory, because the function does this:

      (setq try (if (stringp name)
                    (file-exists-p (expand-file-name name file))
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^

It's possible that "directory hierarchy from FILE" doesn't convey that
clearly enough, in which case we could add

  FILE should be a directory.

> This part is wrong, because the predicate is called with the initial file name, too:
> 
>   NAME can also be a predicate taking one argument (a directory)

Why you say that this is wrong?  The doc string never said anything to
the contrary.

If we change the first sentence to say this:

  Starting from FILE, look up directory hierarchy for directory containing NAME.

will that address the second issue?

Thanks.

Message #11 received at 27798 <at> debbugs.gnu.org (full text, mbox):

From: Clément Pit--Claudel <clement.pitclaudel <at> live.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 27798 <at> debbugs.gnu.org
Subject: Re: bug#27798: Documentation of locate-dominating-file is wrong
Date: Sun, 23 Jul 2017 16:52:51 +0200

[Message part 1 (text/plain, inline)]

On 2017-07-23 16:31, Eli Zaretskii wrote:>> This part is wrong, because locate-dominating-file also accepts directories:
>>
>>   Look up the directory hierarchy from FILE
> 
> Actually, FILE _must_ be a directory, because the function does this:
> 
>       (setq try (if (stringp name)
>                     (file-exists-p (expand-file-name name file))
>                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^

Ouch.  This is a problem, because I'm not the only one who assumed that this had to be a file and not a directory.  There are instances of locate-dominating-file being called with file-name in vc, trampver, yasnippet, company, flycheck, proof-general, etc.  Github finds 35k matches for (locate-dominating-file buffer-file-name) (https://github.com/search?q=%28locate-dominating-file+buffer-file-name&type=Code&utf8=%E2%9C%93).

> It's possible that "directory hierarchy from FILE" doesn't convey that
> clearly enough, in which case we could add
> 
>   FILE should be a directory.

Yes, this would be great.  In fact, I think we should rename the argument to DIRECTORY, if it's a directory.
But I'm not sure what we should do about all the existing callers…

>> This part is wrong, because the predicate is called with the initial file name, too:
>>
>>   NAME can also be a predicate taking one argument (a directory)
> 
> Why you say that this is wrong?  The doc string never said anything to
> the contrary.

The docstring says "one argument (a directory)", but locate-dominating-file (when called with a file name, not a directory name) passes that file name to NAME (thus calling it with one argument that's not a directory).

I understand now that this isn't a valid use of locate-dominating-file, however, so that point is moot.  I was under the impression from the docstring that locate-dominating-file had to be called with a file name, not a directory name.

Thanks for the explanations!
Clément.

[signature.asc (application/pgp-signature, attachment)]

Message #14 received at 27798 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Clément Pit--Claudel <clement.pitclaudel <at> live.com>
Cc: 27798 <at> debbugs.gnu.org
Subject: Re: bug#27798: Documentation of locate-dominating-file is wrong
Date: Sun, 23 Jul 2017 18:20:20 +0300

> Cc: 27798 <at> debbugs.gnu.org
> From: Clément Pit--Claudel <clement.pitclaudel <at> live.com>
> Date: Sun, 23 Jul 2017 16:52:51 +0200
> 
> > Actually, FILE _must_ be a directory, because the function does this:
> > 
> >       (setq try (if (stringp name)
> >                     (file-exists-p (expand-file-name name file))
> >                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Ouch.  This is a problem, because I'm not the only one who assumed that this had to be a file and not a directory.  There are instances of locate-dominating-file being called with file-name in vc, trampver, yasnippet, company, flycheck, proof-general, etc.  Github finds 35k matches for (locate-dominating-file buffer-file-name) (https://github.com/search?q=%28locate-dominating-file+buffer-file-name&type=Code&utf8=%E2%9C%93).

buffer-file-name could be a directory, couldn't it?

But anyway, calling this function with a non-directory file just
wastes one iteration through the loop, so perhaps the situation is not
as bad as my original response made it sound.

> > It's possible that "directory hierarchy from FILE" doesn't convey that
> > clearly enough, in which case we could add
> > 
> >   FILE should be a directory.
> 
> Yes, this would be great.

It would probably be more accurate if we said

  FILE can be a directory.

Message #17 received at 27798 <at> debbugs.gnu.org (full text, mbox):

From: Clément Pit--Claudel <clement.pitclaudel <at> live.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 27798 <at> debbugs.gnu.org
Subject: Re: bug#27798: Documentation of locate-dominating-file is wrong
Date: Sun, 23 Jul 2017 18:08:23 +0200

[Message part 1 (text/plain, inline)]

On 2017-07-23 17:20, Eli Zaretskii wrote:
> It would probably be more accurate if we said
> 
>   FILE can be a directory.

Sounds good.  In that case we also need to adjust the part of the docstring about NAME (when FILE is not a directory and NAME is a lambda, NAME is called with something that's not a directory — namely FILE).

Clément.

[signature.asc (application/pgp-signature, attachment)]

Message #22 received at 27798-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Clément Pit--Claudel <clement.pitclaudel <at> live.com>
Cc: 27798-done <at> debbugs.gnu.org
Subject: Re: bug#27798: Documentation of locate-dominating-file is wrong
Date: Fri, 28 Jul 2017 12:39:10 +0300

> Cc: 27798 <at> debbugs.gnu.org
> From: Clément Pit--Claudel <clement.pitclaudel <at> live.com>
> Date: Sun, 23 Jul 2017 18:08:23 +0200
> 
> On 2017-07-23 17:20, Eli Zaretskii wrote:
> > It would probably be more accurate if we said
> > 
> >   FILE can be a directory.
> 
> Sounds good.  In that case we also need to adjust the part of the docstring about NAME (when FILE is not a directory and NAME is a lambda, NAME is called with something that's not a directory — namely FILE).

Done.

Message #25 received at 27798-done <at> debbugs.gnu.org (full text, mbox):

From: Clément Pit--Claudel <clement.pitclaudel <at> live.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 27798-done <at> debbugs.gnu.org
Subject: Re: bug#27798: Documentation of locate-dominating-file is wrong
Date: Fri, 28 Jul 2017 11:59:19 +0200

[Message part 1 (text/plain, inline)]

On 2017-07-28 11:39, Eli Zaretskii wrote:
> Done.

Thanks!

[signature.asc (application/pgp-signature, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.