* bug#27798: Documentation of locate-dominating-file is wrong
@ 2017-07-23 9:49 Clément Pit--Claudel
2017-07-23 14:31 ` Eli Zaretskii
0 siblings, 1 reply; 7+ messages in thread
From: Clément Pit--Claudel @ 2017-07-23 9:49 UTC (permalink / raw)
To: 27798
[-- Attachment #1.1: Type: text/plain, Size: 1395 bytes --]
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.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#27798: Documentation of locate-dominating-file is wrong
2017-07-23 9:49 bug#27798: Documentation of locate-dominating-file is wrong Clément Pit--Claudel
@ 2017-07-23 14:31 ` Eli Zaretskii
2017-07-23 14:52 ` Clément Pit--Claudel
0 siblings, 1 reply; 7+ messages in thread
From: Eli Zaretskii @ 2017-07-23 14:31 UTC (permalink / raw)
To: Clément Pit--Claudel; +Cc: 27798
> From: Clément Pit--Claudel <clement.pitclaudel@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.
^ permalink raw reply [flat|nested] 7+ messages in thread* bug#27798: Documentation of locate-dominating-file is wrong
2017-07-23 14:31 ` Eli Zaretskii
@ 2017-07-23 14:52 ` Clément Pit--Claudel
2017-07-23 15:20 ` Eli Zaretskii
0 siblings, 1 reply; 7+ messages in thread
From: Clément Pit--Claudel @ 2017-07-23 14:52 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 27798
[-- Attachment #1.1: Type: text/plain, Size: 1964 bytes --]
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.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#27798: Documentation of locate-dominating-file is wrong
2017-07-23 14:52 ` Clément Pit--Claudel
@ 2017-07-23 15:20 ` Eli Zaretskii
2017-07-23 16:08 ` Clément Pit--Claudel
0 siblings, 1 reply; 7+ messages in thread
From: Eli Zaretskii @ 2017-07-23 15:20 UTC (permalink / raw)
To: Clément Pit--Claudel; +Cc: 27798
> Cc: 27798@debbugs.gnu.org
> From: Clément Pit--Claudel <clement.pitclaudel@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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-07-28 9:59 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-23 9:49 bug#27798: Documentation of locate-dominating-file is wrong Clément Pit--Claudel
2017-07-23 14:31 ` Eli Zaretskii
2017-07-23 14:52 ` Clément Pit--Claudel
2017-07-23 15:20 ` Eli Zaretskii
2017-07-23 16:08 ` Clément Pit--Claudel
2017-07-28 9:39 ` Eli Zaretskii
2017-07-28 9:59 ` Clément Pit--Claudel
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).