bug#16406: load prefers directories rather than searching load-path

bug#16406: load prefers directories rather than searching load-path

[Glenn Morris]

Package: emacs
Version: 24.3

From the Emacs source directory, wanting to load the standard info.el library:

./src/emacs -Q -l info

  Cannot open load file: is a directory, /tmp/emacs/trunk/info

It seems dumb for load prefer a directory.

bug#16406: load prefers directories rather than searching load-path

[Stefan Monnier]

> It seems dumb for load prefer a directory.

You have just insulted Emacs.  Pay $50 to each player.


        Stefan "sorry, too much Monopoly lately"

bug#16406: load prefers directories rather than searching load-path

[npostavs]

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

tags 16406 patch
quit

Glenn Morris <rgm@gnu.org> writes:

> Package: emacs
> Version: 24.3
>
> From the Emacs source directory, wanting to load the standard info.el library:
>
> ./src/emacs -Q -l info
>
>   Cannot open load file: is a directory, /tmp/emacs/trunk/info
>
> It seems dumb for load prefer a directory.

Seems simple enough to fix, here is a patch (this also covers #17848
"add suffix search to -l even when directory part in argument"):


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1314 bytes --]

From 38faf8846f7738b533aa29622fd7f78a8c6c81cd Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 21 Aug 2016 10:51:38 -0400
Subject: [PATCH v1] Improvements to --load argument handling

* lisp/startup.el (command-line-1): Search for files with load-suffixes
in default dir (Bug #17484).  Only pass normal files that were found,
since `load' doesn't handle directories (Bug #16406).
---
 lisp/startup.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/startup.el b/lisp/startup.el
index fcdc376..9134604 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -2392,8 +2392,9 @@ command-line-1
                                    (or argval (pop command-line-args-left))))
                             ;; Take file from default dir if it exists there;
                             ;; otherwise let `load' search for it.
-                            (file-ex (expand-file-name file)))
-                       (when (file-exists-p file-ex)
+                            (file-ex (locate-file file (list default-directory)
+                                                  load-suffixes)))
+                       (when (and file-ex (file-regular-p file-ex))
                          (setq file file-ex))
                        (load file nil t)))
 
-- 
2.9.2

bug#16406: load prefers directories rather than searching load-path

[npostavs]

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

npostavs@users.sourceforge.net writes:

>
> Seems simple enough to fix, here is a patch (this also covers #17848
> "add suffix search to -l even when directory part in argument"):

That one broke normal loading.  Here's a new patch.


[-- Attachment #2: patch v2 --]
[-- Type: text/plain, Size: 1429 bytes --]

From 38e48fb4bc3b6f949659d528fcaaa78d903be1db Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 21 Aug 2016 10:51:38 -0400
Subject: [PATCH v2] Improvements to --load argument handling

* lisp/startup.el (command-line-1): Search for files with load-suffixes
in default dir (Bug #17848).  Only pass normal files that were found,
since `load' doesn't handle directories (Bug #16406).
---
 lisp/startup.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/startup.el b/lisp/startup.el
index fcdc376..01e6d85 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -2392,8 +2392,11 @@ command-line-1
                                    (or argval (pop command-line-args-left))))
                             ;; Take file from default dir if it exists there;
                             ;; otherwise let `load' search for it.
-                            (file-ex (expand-file-name file)))
-                       (when (file-exists-p file-ex)
+                            (file-ex (locate-file
+                                      file (list default-directory)
+                                      (append (get-load-suffixes)
+                                              load-file-rep-suffixes))))
+                       (when (and file-ex (file-regular-p file-ex))
                          (setq file file-ex))
                        (load file nil t)))
 
-- 
2.9.3

bug#16406: load prefers directories rather than searching load-path

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Date: Sat, 03 Sep 2016 12:43:29 -0400
> Cc: 16406@debbugs.gnu.org
> 
> > Seems simple enough to fix, here is a patch (this also covers #17848
> > "add suffix search to -l even when directory part in argument"):
> 
> That one broke normal loading.  Here's a new patch.

Instead of doing the search by hand, wouldn't it be better to teach
'load' to ignore directories?  Or do we have a use case where finding
a directory in 'load' would make sense?

IOW, isn't "'load' prefers directories" the actual problem we should
fix, rather than only fixing the startup problem?

I do agree that the file-exists-p test in command-line-1 should be
replaced with a test that doesn't let directories through.  But I
don't understand why using locate-file here is a good idea.

Thanks.

bug#16406: load prefers directories rather than searching load-path

[npostavs]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: npostavs@users.sourceforge.net
>> Date: Sat, 03 Sep 2016 12:43:29 -0400
>> Cc: 16406@debbugs.gnu.org
>> 
>> > Seems simple enough to fix, here is a patch (this also covers #17848
>> > "add suffix search to -l even when directory part in argument"):
>> 
>> That one broke normal loading.  Here's a new patch.
>
> Instead of doing the search by hand, wouldn't it be better to teach
> 'load' to ignore directories?  Or do we have a use case where finding
> a directory in 'load' would make sense?
>
> IOW, isn't "'load' prefers directories" the actual problem we should
> fix, rather than only fixing the startup problem?
>
> I do agree that the file-exists-p test in command-line-1 should be
> replaced with a test that doesn't let directories through.  But I
> don't understand why using locate-file here is a good idea.

Hmm, I seem to have confused things by trying to combine the fix for
16406 and 17848.  Just replacing the file-exists-p test, as in the patch
below, is enough to fix this bug.  Let's look at 17848 separately.


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1039 bytes --]

From ba8ddc2e039fe982bdfa46519c003808be9ddff3 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 21 Aug 2016 10:51:38 -0400
Subject: [PATCH v3] Don't --load directories

* lisp/startup.el (command-line-1): Only pass expanded FILENAME argument
of --load when it refers to a normal file, since `load' doesn't handle
directories (Bug #16406).
---
 lisp/startup.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/startup.el b/lisp/startup.el
index fcdc376..45beefb 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -2393,7 +2393,7 @@ command-line-1
                             ;; Take file from default dir if it exists there;
                             ;; otherwise let `load' search for it.
                             (file-ex (expand-file-name file)))
-                       (when (file-exists-p file-ex)
+                       (when (and file-ex (file-regular-p file-ex))
                          (setq file file-ex))
                        (load file nil t)))
 
-- 
2.9.3

bug#16406: load prefers directories rather than searching load-path

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: 16406@debbugs.gnu.org,  rgm@gnu.org
> Date: Sat, 03 Sep 2016 14:43:53 -0400
> 
> > I do agree that the file-exists-p test in command-line-1 should be
> > replaced with a test that doesn't let directories through.  But I
> > don't understand why using locate-file here is a good idea.
> 
> Hmm, I seem to have confused things by trying to combine the fix for
> 16406 and 17848.  Just replacing the file-exists-p test, as in the patch
> below, is enough to fix this bug.  Let's look at 17848 separately.
> 
> >From ba8ddc2e039fe982bdfa46519c003808be9ddff3 Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <npostavs@gmail.com>
> Date: Sun, 21 Aug 2016 10:51:38 -0400
> Subject: [PATCH v3] Don't --load directories
> 
> * lisp/startup.el (command-line-1): Only pass expanded FILENAME argument
> of --load when it refers to a normal file, since `load' doesn't handle
> directories (Bug #16406).
> ---
>  lisp/startup.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lisp/startup.el b/lisp/startup.el
> index fcdc376..45beefb 100644
> --- a/lisp/startup.el
> +++ b/lisp/startup.el
> @@ -2393,7 +2393,7 @@ command-line-1
>                              ;; Take file from default dir if it exists there;
>                              ;; otherwise let `load' search for it.
>                              (file-ex (expand-file-name file)))
> -                       (when (file-exists-p file-ex)
> +                       (when (and file-ex (file-regular-p file-ex))
>                           (setq file file-ex))
>                         (load file nil t)))

OK.  But isn't the "and file-ex" test unnecessary?  It wasn't required
for file-exists-p, so why is it for file-regular-p?

Thanks.

bug#16406: load prefers directories rather than searching load-path

[npostavs]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> 
>> Hmm, I seem to have confused things by trying to combine the fix for
>> 16406 and 17848.  Just replacing the file-exists-p test, as in the patch
>> below, is enough to fix this bug.  Let's look at 17848 separately.
>> 
>>                              (file-ex (expand-file-name file)))
>> -                       (when (file-exists-p file-ex)
>> +                       (when (and file-ex (file-regular-p file-ex))
>
> OK.  But isn't the "and file-ex" test unnecessary?  It wasn't required
> for file-exists-p, so why is it for file-regular-p?

Indeed, not required. It's just leftover confusion.


[-- Attachment #2: patch v4 --]
[-- Type: text/plain, Size: 1025 bytes --]

From 72ac37f50068af39aebc200b812cb901e0dcd176 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 21 Aug 2016 10:51:38 -0400
Subject: [PATCH v4] Don't --load directories

* lisp/startup.el (command-line-1): Only pass expanded FILENAME argument
of --load when it refers to a normal file, since `load' doesn't handle
directories (Bug #16406).
---
 lisp/startup.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/startup.el b/lisp/startup.el
index fcdc376..d5225bd 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -2393,7 +2393,7 @@ command-line-1
                             ;; Take file from default dir if it exists there;
                             ;; otherwise let `load' search for it.
                             (file-ex (expand-file-name file)))
-                       (when (file-exists-p file-ex)
+                       (when (file-regular-p file-ex)
                          (setq file file-ex))
                        (load file nil t)))
 
-- 
2.9.3

bug#16406: load prefers directories rather than searching load-path

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: 16406@debbugs.gnu.org,  rgm@gnu.org
> Date: Sat, 03 Sep 2016 15:12:49 -0400
> 
> > OK.  But isn't the "and file-ex" test unnecessary?  It wasn't required
> > for file-exists-p, so why is it for file-regular-p?
> 
> Indeed, not required. It's just leftover confusion.
> 
> >From 72ac37f50068af39aebc200b812cb901e0dcd176 Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <npostavs@gmail.com>
> Date: Sun, 21 Aug 2016 10:51:38 -0400
> Subject: [PATCH v4] Don't --load directories
> 
> * lisp/startup.el (command-line-1): Only pass expanded FILENAME argument
> of --load when it refers to a normal file, since `load' doesn't handle
> directories (Bug #16406).
> ---
>  lisp/startup.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lisp/startup.el b/lisp/startup.el
> index fcdc376..d5225bd 100644
> --- a/lisp/startup.el
> +++ b/lisp/startup.el
> @@ -2393,7 +2393,7 @@ command-line-1
>                              ;; Take file from default dir if it exists there;
>                              ;; otherwise let `load' search for it.
>                              (file-ex (expand-file-name file)))
> -                       (when (file-exists-p file-ex)
> +                       (when (file-regular-p file-ex)
>                           (setq file file-ex))
>                         (load file nil t)))
>  
> -- 
> 2.9.3

Thanks, LGTM.

bug#16406: load prefers directories rather than searching load-path

[npostavs]

tags 16406 fixed
close 16406 25.2
quit

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, LGTM.

Pushed as a08ce41e

bug#17848: #17848 add suffix search to -l even when directory part in argument (WAS: Re: bug#16406: load prefers directories...)

[npostavs]

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

tags 17848 patch
quit

Eli Zaretskii <eliz@gnu.org> writes:
>
> Instead of doing the search by hand, wouldn't it be better to teach
> 'load' to ignore directories?
[...]
>  But I don't understand why using locate-file here is a good idea.

So now in the context of *this* bug, I think `locate-file' is needed.
The current directory needs to be searched for suffixed versions, but we
can't let `load' do that without adding "." to `load-path'.


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1335 bytes --]

From b14d2df532f637fe9fe21a50a8a6066a40720c8b Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 4 Sep 2016 17:57:56 -0400
Subject: [PATCH v1] Add suffix search to --load dir/foo

* lisp/startup.el (command-line-1): Search for files with load-suffixes
in default directory (Bug #17848).
---
 lisp/startup.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/startup.el b/lisp/startup.el
index d5225bd..01e6d85 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -2392,8 +2392,11 @@ command-line-1
                                    (or argval (pop command-line-args-left))))
                             ;; Take file from default dir if it exists there;
                             ;; otherwise let `load' search for it.
-                            (file-ex (expand-file-name file)))
-                       (when (file-regular-p file-ex)
+                            (file-ex (locate-file
+                                      file (list default-directory)
+                                      (append (get-load-suffixes)
+                                              load-file-rep-suffixes))))
+                       (when (and file-ex (file-regular-p file-ex))
                          (setq file file-ex))
                        (load file nil t)))
 
-- 
2.9.3

bug#17848: #17848 add suffix search to -l even when directory part in argument (WAS: Re: bug#16406: load prefers directories...)

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: 17848@debbugs.gnu.org,  rgm@gnu.org
> Date: Sun, 04 Sep 2016 18:06:52 -0400
> 
> So now in the context of *this* bug, I think `locate-file' is needed.
> The current directory needs to be searched for suffixed versions, but we
> can't let `load' do that without adding "." to `load-path'.

I'm not sure.  Wouldn't adding the leading directory to load-path in a
let-binding be a cleaner solution?  IOW, I don't understand the reason
for the "Take file from default dir if it exists there" logic in the
first place -- what are we gaining there?

Thanks.

bug#17848: #17848 add suffix search to -l even when directory part in argument

[npostavs]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: npostavs@users.sourceforge.net
>> Cc: 17848@debbugs.gnu.org,  rgm@gnu.org
>> Date: Sun, 04 Sep 2016 18:06:52 -0400
>> 
>> So now in the context of *this* bug, I think `locate-file' is needed.
>> The current directory needs to be searched for suffixed versions, but we
>> can't let `load' do that without adding "." to `load-path'.
>
> I'm not sure.  Wouldn't adding the leading directory to load-path in a
> let-binding be a cleaner solution?  IOW, I don't understand the reason
> for the "Take file from default dir if it exists there" logic in the
> first place -- what are we gaining there?

If we let-bind `load-path', then this could influence the code that
we're loading.  For more context, I came to this bug from
test/Makefile.in:

    ## We need to use $loadfile because:
    ## i) -L :$srcdir -l basename does not work, because we have files whose
    ## basename duplicates a file in lisp/ (eg eshell.el).
    ## ii) Although -l basename will automatically load .el or .elc,
    ## -l ./basename treats basename as a literal file (it would be nice
    ## to change this; bug#17848 - if that gets done, this can be simplified).

bug#17848: #17848 add suffix search to -l even when directory part in argument

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: 17848@debbugs.gnu.org,  rgm@gnu.org
> Date: Mon, 05 Sep 2016 18:59:22 -0400
> 
> > I'm not sure.  Wouldn't adding the leading directory to load-path in a
> > let-binding be a cleaner solution?  IOW, I don't understand the reason
> > for the "Take file from default dir if it exists there" logic in the
> > first place -- what are we gaining there?
> 
> If we let-bind `load-path', then this could influence the code that
> we're loading.  For more context, I came to this bug from
> test/Makefile.in:
> 
>     ## We need to use $loadfile because:
>     ## i) -L :$srcdir -l basename does not work, because we have files whose
>     ## basename duplicates a file in lisp/ (eg eshell.el).
>     ## ii) Although -l basename will automatically load .el or .elc,
>     ## -l ./basename treats basename as a literal file (it would be nice
>     ## to change this; bug#17848 - if that gets done, this can be simplified).

You have a point there, indeed.

However, I don't think the proposed solution, to pass to 'load' the
explicit absolute file name of a file that 'locate-file' found, is TRT
here.  Here's my rationale:

Stepping back a notch, what is the root cause of the reported bug?
The root cause, AFAICT, is that ./foo doesn't exist, and therefore we
pass literally "./foo" to 'load', which then looks for this relative
file name in every directory in load-path, and doesn't try the current
directory.  If we were to pass an absolute file name "/some/where/foo"
to 'load', it wouldn't have looked along load-path, but instead would
try /some/where/foo.elc, /some/where/foo.el, etc. -- exactly as
requested.

The proposed patch indeed passes an absolute file name to 'load', but
it passes the name of the first candidate file found by 'locate-file',
and that could very well be different from what 'load' would have
found, because 'load' has some additional features and logic that
'locate-file' doesn't.

So I think what we need is to see if 'locate-file' finds _any_
candidate at all, and if it does, simply pass to 'load' the result of

  (expand-file-name file)

disregarding the file name returned by 'locate-file'; and we should do
that only if 'file' is not an absolute file name to begin with.  WDYT?

One other thought: should we prefer an exact match, without any
extensions, in this particular case?  IOW, should "-l ./foo" prefer
'foo" even if "foo.el" or "foo.gz" etc. exist in the current
directory?  If we should indeed prefer an exact match, then we might
need to tweak the order of the suffixes to support that.

Thanks.

bug#17848: #17848 add suffix search to -l even when directory part in argument

[npostavs]

tags 17848 - patch
quit

Eli Zaretskii <eliz@gnu.org> writes:

> The proposed patch indeed passes an absolute file name to 'load', but
> it passes the name of the first candidate file found by 'locate-file',
> and that could very well be different from what 'load' would have
> found, because 'load' has some additional features and logic that
> 'locate-file' doesn't.

Actually, thinking more about it, I wonder what's wrong with just doing 

    emacs -l '$PWD/foo'

or even

    emacs -l $PWD/foo

    
So then it's just a question of whether it's worth saving a few
characters on the command line (to write ./foo instead of '$PWD/foo'),
to which I'm inclined to answer "no", especially since implementing it
turns out to be rather fiddly.

>
> One other thought: should we prefer an exact match, without any
> extensions, in this particular case?  IOW, should "-l ./foo" prefer
> 'foo" even if "foo.el" or "foo.gz" etc. exist in the current
> directory?  If we should indeed prefer an exact match, then we might
> need to tweak the order of the suffixes to support that.