bug#27798: Documentation of locate-dominating-file is wrong

bug#27798: Documentation of locate-dominating-file is wrong

[Clément Pit--Claudel]

[-- 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 --]

bug#27798: Documentation of locate-dominating-file is wrong

[Eli Zaretskii]

> 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.

bug#27798: Documentation of locate-dominating-file is wrong

[Clément Pit--Claudel]

[-- 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 --]

bug#27798: Documentation of locate-dominating-file is wrong

[Eli Zaretskii]

> 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.

bug#27798: Documentation of locate-dominating-file is wrong

[Clément Pit--Claudel]

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

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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#27798: Documentation of locate-dominating-file is wrong

[Eli Zaretskii]

> Cc: 27798@debbugs.gnu.org
> From: Clément Pit--Claudel <clement.pitclaudel@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.

bug#27798: Documentation of locate-dominating-file is wrong

[Clément Pit--Claudel]

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

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

Thanks!


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]