GNU bug report logs - #78855
[PATCH] Expand DIR arg on `package-vc-install-from-checkout'

Previous Next

To reply to this bug, email your comments to 78855 AT debbugs.gnu.org.
There is no need to reopen the bug first.

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

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

From: Roi Martin <jroi.martin <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Expand DIR arg on `package-vc-install-from-checkout'
Date: Sat, 21 Jun 2025 13:51:48 +0200

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

Tags: patch

When `package-vc-install-from-checkout' is called interactively, it asks
users to input a directory.  If users have enabled `fido-mode', when
selecting a directory and pressing RET, they will actually pass
"/path/to/pkg/./" to the function.  Because NAME defaults to the base
name of DIR, it is assigned the value ".".  That is problematic in
several ways.

First, the package would be named ".", which is unexpected.

Second, the following code in `package-vc-install-from-checkout':

  (let* ((name (or name (file-name-base (directory-file-name dir))))
         (pkg-dir (expand-file-name name package-user-dir))
	 ; ...
    (when (file-exists-p pkg-dir)
      (if (yes-or-no-p (format "Overwrite previous checkout for package `%s'?" name))
          (package--delete-directory pkg-dir)
        (error "There already exists a checkout for %s" name)))
    (make-directory pkg-dir t)
    ; ...

would assign `pkg-dir' the value of `package-user-dir', which is wrong.
I.e. it would check for the existence of the wrong directory, it will
try to create the wrong directory, etc.

This patch expands the directory name passed by the user.  So now the
behavior of the command is:

  NAME defaults to the base name of DIR after being expanded.

I don't think there is any case where the old behavior is expected by
users.  So, this change should not break any current use of the
function.  Is this a fair assumption?  Is there any case where the old
behavior can be useful?

        Roi

[0001-Expand-DIR-arg-on-package-vc-install-from-checkout.patch (text/patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Roi Martin <jroi.martin <at> gmail.com>
Cc: 78855 <at> debbugs.gnu.org, philipk <at> posteo.net
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Sat, 21 Jun 2025 15:53:58 +0300

> Cc: Philip Kaludercic <philipk <at> posteo.net>
> From: Roi Martin <jroi.martin <at> gmail.com>
> Date: Sat, 21 Jun 2025 13:51:48 +0200
> 
> When `package-vc-install-from-checkout' is called interactively, it asks
> users to input a directory.  If users have enabled `fido-mode', when
> selecting a directory and pressing RET, they will actually pass
> "/path/to/pkg/./" to the function.  Because NAME defaults to the base
> name of DIR, it is assigned the value ".".  That is problematic in
> several ways.
> 
> First, the package would be named ".", which is unexpected.
> 
> Second, the following code in `package-vc-install-from-checkout':
> 
>   (let* ((name (or name (file-name-base (directory-file-name dir))))
>          (pkg-dir (expand-file-name name package-user-dir))
> 	 ; ...
>     (when (file-exists-p pkg-dir)
>       (if (yes-or-no-p (format "Overwrite previous checkout for package `%s'?" name))
>           (package--delete-directory pkg-dir)
>         (error "There already exists a checkout for %s" name)))
>     (make-directory pkg-dir t)
>     ; ...
> 
> would assign `pkg-dir' the value of `package-user-dir', which is wrong.
> I.e. it would check for the existence of the wrong directory, it will
> try to create the wrong directory, etc.
> 
> This patch expands the directory name passed by the user.

This is the right solution, IMO, but there are a few nits, see below.

>  (defun package-vc-install-from-checkout (dir &optional name)
>    "Install the package NAME from its source directory DIR.
> -NAME defaults to the base name of DIR.
> +NAME defaults to the base name of DIR after being expanded.

This part of the change is not necessary: such fine implementation
details should not be in the doc string.

>    (interactive (let* ((dir (read-directory-name "Directory: "))
> -                      (base (file-name-base (directory-file-name dir))))
> +                      (base (file-name-base (directory-file-name (expand-file-name dir)))))
>                   (list dir (and current-prefix-arg
>                                  (read-string
>                                   (format-prompt "Package name" base)
>                                   nil nil base)))))
>    (package-vc--archives-initialize)
> -  (let* ((name (or name (file-name-base (directory-file-name dir))))
> +  (let* ((dir (expand-file-name dir))
> +         (name (or name (file-name-base (directory-file-name dir))))

Can we avoid this duplicate call to expand-file-name in the
interactive invocation?

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

From: Roi Martin <jroi.martin <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78855 <at> debbugs.gnu.org, philipk <at> posteo.net
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Sat, 21 Jun 2025 20:28:17 +0200

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

Eli Zaretskii <eliz <at> gnu.org> writes:

>>    (interactive (let* ((dir (read-directory-name "Directory: "))
>> -                      (base (file-name-base (directory-file-name dir))))
>> +                      (base (file-name-base (directory-file-name (expand-file-name dir)))))
>>                   (list dir (and current-prefix-arg
>>                                  (read-string
>>                                   (format-prompt "Package name" base)
>>                                   nil nil base)))))
>>    (package-vc--archives-initialize)
>> -  (let* ((name (or name (file-name-base (directory-file-name dir))))
>> +  (let* ((dir (expand-file-name dir))
>> +         (name (or name (file-name-base (directory-file-name dir))))
>
> Can we avoid this duplicate call to expand-file-name in the
> interactive invocation?

I don't think so.  If I'm not wrong, `interactive' must be located at
top-level and be the first form in the body (not counting `declare').
Additionally, we also want to expand the directory name when the command
is called from Emacs Lisp.  So, I don't see how to deduplicate that
call.

In fact,

  (file-name-base (directory-file-name ...))

was already duplicated.  Maybe because of the same reason?  Any hint to
avoid this?

In any case, I've attached a new version of the patch that fixes the doc
string issue and adds the bug number.

        Roi

[0001-Expand-DIR-arg-on-package-vc-install-from-checkout.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Roi Martin <jroi.martin <at> gmail.com>
Cc: 78855 <at> debbugs.gnu.org, philipk <at> posteo.net
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Sat, 21 Jun 2025 22:09:51 +0300

> From: Roi Martin <jroi.martin <at> gmail.com>
> Cc: 78855 <at> debbugs.gnu.org, philipk <at> posteo.net
> Date: Sat, 21 Jun 2025 20:28:17 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >>    (interactive (let* ((dir (read-directory-name "Directory: "))
> >> -                      (base (file-name-base (directory-file-name dir))))
> >> +                      (base (file-name-base (directory-file-name (expand-file-name dir)))))
> >>                   (list dir (and current-prefix-arg
> >>                                  (read-string
> >>                                   (format-prompt "Package name" base)
> >>                                   nil nil base)))))
> >>    (package-vc--archives-initialize)
> >> -  (let* ((name (or name (file-name-base (directory-file-name dir))))
> >> +  (let* ((dir (expand-file-name dir))
> >> +         (name (or name (file-name-base (directory-file-name dir))))
> >
> > Can we avoid this duplicate call to expand-file-name in the
> > interactive invocation?
> 
> I don't think so.  If I'm not wrong, `interactive' must be located at
> top-level and be the first form in the body (not counting `declare').
> Additionally, we also want to expand the directory name when the command
> is called from Emacs Lisp.  So, I don't see how to deduplicate that
> call.

But there are ways to know if the call was interactive, right?

> In fact,
> 
>   (file-name-base (directory-file-name ...))
> 
> was already duplicated.  Maybe because of the same reason?  Any hint to
> avoid this?

It would be good to avoid that as well.  However, while file-name-base
is a very simple function that just transforms its string argument,
expand-file-name is much more complex, and can be expensive,
especially if the file name is remote.

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

From: Roi Martin <jroi.martin <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78855 <at> debbugs.gnu.org, philipk <at> posteo.net
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Sun, 22 Jun 2025 14:19:04 +0200

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

Eli Zaretskii <eliz <at> gnu.org> writes:

>> >>    (interactive (let* ((dir (read-directory-name "Directory: "))
>> >> -                      (base (file-name-base (directory-file-name dir))))
>> >> +                      (base (file-name-base (directory-file-name (expand-file-name dir)))))
>> >>                   (list dir (and current-prefix-arg
>> >>                                  (read-string
>> >>                                   (format-prompt "Package name" base)
>> >>                                   nil nil base)))))
>> >>    (package-vc--archives-initialize)
>> >> -  (let* ((name (or name (file-name-base (directory-file-name dir))))
>> >> +  (let* ((dir (expand-file-name dir))
>> >> +         (name (or name (file-name-base (directory-file-name dir))))
>> >
>> > Can we avoid this duplicate call to expand-file-name in the
>> > interactive invocation?
>> 
>> I don't think so.  If I'm not wrong, `interactive' must be located at
>> top-level and be the first form in the body (not counting `declare').
>> Additionally, we also want to expand the directory name when the command
>> is called from Emacs Lisp.  So, I don't see how to deduplicate that
>> call.
>
> But there are ways to know if the call was interactive, right?

Sorry, I thought you meant simplifying the code, not avoiding the second
call at runtime.  I've attached a new version of the patch.

        Roi

[0001-Expand-DIR-arg-on-package-vc-install-from-checkout.patch (text/x-patch, attachment)]

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Roi Martin <jroi.martin <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 78855 <at> debbugs.gnu.org
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Thu, 26 Jun 2025 20:41:17 +0000

Roi Martin <jroi.martin <at> gmail.com> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>>> >>    (interactive (let* ((dir (read-directory-name "Directory: "))
>>> >> -                      (base (file-name-base (directory-file-name dir))))
>>> >> +                      (base (file-name-base (directory-file-name (expand-file-name dir)))))
>>> >>                   (list dir (and current-prefix-arg
>>> >>                                  (read-string
>>> >>                                   (format-prompt "Package name" base)
>>> >>                                   nil nil base)))))
>>> >>    (package-vc--archives-initialize)
>>> >> -  (let* ((name (or name (file-name-base (directory-file-name dir))))
>>> >> +  (let* ((dir (expand-file-name dir))
>>> >> +         (name (or name (file-name-base (directory-file-name dir))))
>>> >
>>> > Can we avoid this duplicate call to expand-file-name in the
>>> > interactive invocation?
>>> 
>>> I don't think so.  If I'm not wrong, `interactive' must be located at
>>> top-level and be the first form in the body (not counting `declare').
>>> Additionally, we also want to expand the directory name when the command
>>> is called from Emacs Lisp.  So, I don't see how to deduplicate that
>>> call.
>>
>> But there are ways to know if the call was interactive, right?
>
> Sorry, I thought you meant simplifying the code, not avoiding the second
> call at runtime.  I've attached a new version of the patch.
>
>         Roi
>
> From 16610a7da2f947e043db7ed95525091712ac38ff Mon Sep 17 00:00:00 2001
> From: Roi Martin <jroi.martin <at> gmail.com>
> Date: Sat, 21 Jun 2025 10:28:45 +0200
> Subject: [PATCH] Expand DIR arg on `package-vc-install-from-checkout'
>
> Expand the directory name passed to the
> `package-vc-install-from-checkout' function so that paths like
> "/path/to/pkg/./" are converted to "/path/to/pkg/".  This ensures that
> the package name defaults to "pkg" instead of "." (Bug#78855).
> * lisp/emacs-lisp/package-vc.el (package-vc-install-from-checkout):
> Expand the provided directory name.
> ---
>  lisp/emacs-lisp/package-vc.el | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index babc0b715245..11e5af884d0a 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -920,17 +920,18 @@ package-vc-install-from-checkout
>  under version control, typically one created by `package-vc-checkout'.
>  If invoked interactively with a prefix argument, prompt the user
>  for the NAME of the package to set up."
> -  (interactive (let* ((dir (read-directory-name "Directory: "))
> +  (interactive (let* ((dir (expand-file-name (read-directory-name "Directory: ")))
>                        (base (file-name-base (directory-file-name dir))))
>                   (list dir (and current-prefix-arg
>                                  (read-string
>                                   (format-prompt "Package name" base)
>                                   nil nil base)))))
>    (package-vc--archives-initialize)
> -  (let* ((name (or name (file-name-base (directory-file-name dir))))
> +  (let* ((dir (if (called-interactively-p 'any) dir (expand-file-name dir)))

I do not get the advantage of this approach?  Even if we ignore that
`called-interactively-p' is tricky, the result is that whatever `dir' is
bound to after this point has passed through `expand-file-name'?  So
isn't the first patch the simpler solution, with the same effect?

> +         (name (or name (file-name-base (directory-file-name dir))))
>           (pkg-dir (expand-file-name name package-user-dir))
>           (package-vc-selected-packages
> -          (cons (list name :lisp-dir (expand-file-name dir))
> +          (cons (list name :lisp-dir dir)
>                  package-vc-selected-packages)))
>      (when (file-exists-p pkg-dir)
>        (if (yes-or-no-p (format "Overwrite previous checkout for package `%s'?" name))

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

From: Roi Martin <jroi.martin <at> gmail.com>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 78855 <at> debbugs.gnu.org
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Thu, 26 Jun 2025 23:40:03 +0200

Philip Kaludercic <philipk <at> posteo.net> writes:

>> From 16610a7da2f947e043db7ed95525091712ac38ff Mon Sep 17 00:00:00 2001
>> From: Roi Martin <jroi.martin <at> gmail.com>
>> Date: Sat, 21 Jun 2025 10:28:45 +0200
>> Subject: [PATCH] Expand DIR arg on `package-vc-install-from-checkout'
>>
>> Expand the directory name passed to the
>> `package-vc-install-from-checkout' function so that paths like
>> "/path/to/pkg/./" are converted to "/path/to/pkg/".  This ensures that
>> the package name defaults to "pkg" instead of "." (Bug#78855).
>> * lisp/emacs-lisp/package-vc.el (package-vc-install-from-checkout):
>> Expand the provided directory name.
>> ---
>>  lisp/emacs-lisp/package-vc.el | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>> index babc0b715245..11e5af884d0a 100644
>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -920,17 +920,18 @@ package-vc-install-from-checkout
>>  under version control, typically one created by `package-vc-checkout'.
>>  If invoked interactively with a prefix argument, prompt the user
>>  for the NAME of the package to set up."
>> -  (interactive (let* ((dir (read-directory-name "Directory: "))
>> +  (interactive (let* ((dir (expand-file-name (read-directory-name "Directory: ")))
>>                        (base (file-name-base (directory-file-name dir))))
>>                   (list dir (and current-prefix-arg
>>                                  (read-string
>>                                   (format-prompt "Package name" base)
>>                                   nil nil base)))))
>>    (package-vc--archives-initialize)
>> -  (let* ((name (or name (file-name-base (directory-file-name dir))))
>> +  (let* ((dir (if (called-interactively-p 'any) dir (expand-file-name dir)))
>
> I do not get the advantage of this approach?  Even if we ignore that
> `called-interactively-p' is tricky, the result is that whatever `dir' is
> bound to after this point has passed through `expand-file-name'?  So
> isn't the first patch the simpler solution, with the same effect?

Eli's point is:

  expand-file-name is much more complex, and can be expensive,
  especially if the file name is remote.

With this approach we avoid calling `expand-file-name' twice.

        Roi

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 78855 <at> debbugs.gnu.org, jroi.martin <at> gmail.com
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Fri, 27 Jun 2025 09:13:06 +0300

> From: Philip Kaludercic <philipk <at> posteo.net>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  78855 <at> debbugs.gnu.org
> Date: Thu, 26 Jun 2025 20:41:17 +0000
> 
> Roi Martin <jroi.martin <at> gmail.com> writes:
> 
> > Eli Zaretskii <eliz <at> gnu.org> writes:
> >
> >>> > Can we avoid this duplicate call to expand-file-name in the
> >>> > interactive invocation?
> >>> 
> >>> I don't think so.  If I'm not wrong, `interactive' must be located at
> >>> top-level and be the first form in the body (not counting `declare').
> >>> Additionally, we also want to expand the directory name when the command
> >>> is called from Emacs Lisp.  So, I don't see how to deduplicate that
> >>> call.
> >>
> >> But there are ways to know if the call was interactive, right?
> >
> > Sorry, I thought you meant simplifying the code, not avoiding the second
> > call at runtime.  I've attached a new version of the patch.
> >
> >         Roi
> >
> > From 16610a7da2f947e043db7ed95525091712ac38ff Mon Sep 17 00:00:00 2001
> > From: Roi Martin <jroi.martin <at> gmail.com>
> > Date: Sat, 21 Jun 2025 10:28:45 +0200
> > Subject: [PATCH] Expand DIR arg on `package-vc-install-from-checkout'
> >
> > Expand the directory name passed to the
> > `package-vc-install-from-checkout' function so that paths like
> > "/path/to/pkg/./" are converted to "/path/to/pkg/".  This ensures that
> > the package name defaults to "pkg" instead of "." (Bug#78855).
> > * lisp/emacs-lisp/package-vc.el (package-vc-install-from-checkout):
> > Expand the provided directory name.
> > ---
> >  lisp/emacs-lisp/package-vc.el | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> > index babc0b715245..11e5af884d0a 100644
> > --- a/lisp/emacs-lisp/package-vc.el
> > +++ b/lisp/emacs-lisp/package-vc.el
> > @@ -920,17 +920,18 @@ package-vc-install-from-checkout
> >  under version control, typically one created by `package-vc-checkout'.
> >  If invoked interactively with a prefix argument, prompt the user
> >  for the NAME of the package to set up."
> > -  (interactive (let* ((dir (read-directory-name "Directory: "))
> > +  (interactive (let* ((dir (expand-file-name (read-directory-name "Directory: ")))
> >                        (base (file-name-base (directory-file-name dir))))
> >                   (list dir (and current-prefix-arg
> >                                  (read-string
> >                                   (format-prompt "Package name" base)
> >                                   nil nil base)))))
> >    (package-vc--archives-initialize)
> > -  (let* ((name (or name (file-name-base (directory-file-name dir))))
> > +  (let* ((dir (if (called-interactively-p 'any) dir (expand-file-name dir)))
> 
> I do not get the advantage of this approach?  Even if we ignore that
> `called-interactively-p' is tricky, the result is that whatever `dir' is
> bound to after this point has passed through `expand-file-name'?  So
> isn't the first patch the simpler solution, with the same effect?

I would like to avoid running file names through expand-file-name more
than once, because that call could be expensive.  So in this case
performance trumps simplicity, IMO.  Of course, if there's a simpler
way of avoiding duplicate calls to expand-file-name, we should use it,
so if you have a suggestion for such a simpler way, please tell the
details.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Roi Martin <jroi.martin <at> gmail.com>, philipk <at> posteo.net
Cc: 78855 <at> debbugs.gnu.org
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Sat, 28 Jun 2025 12:48:45 +0300

> From: Roi Martin <jroi.martin <at> gmail.com>
> Cc: 78855 <at> debbugs.gnu.org, philipk <at> posteo.net
> Date: Sun, 22 Jun 2025 14:19:04 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> >>    (interactive (let* ((dir (read-directory-name "Directory: "))
> >> >> -                      (base (file-name-base (directory-file-name dir))))
> >> >> +                      (base (file-name-base (directory-file-name (expand-file-name dir)))))
> >> >>                   (list dir (and current-prefix-arg
> >> >>                                  (read-string
> >> >>                                   (format-prompt "Package name" base)
> >> >>                                   nil nil base)))))
> >> >>    (package-vc--archives-initialize)
> >> >> -  (let* ((name (or name (file-name-base (directory-file-name dir))))
> >> >> +  (let* ((dir (expand-file-name dir))
> >> >> +         (name (or name (file-name-base (directory-file-name dir))))
> >> >
> >> > Can we avoid this duplicate call to expand-file-name in the
> >> > interactive invocation?
> >> 
> >> I don't think so.  If I'm not wrong, `interactive' must be located at
> >> top-level and be the first form in the body (not counting `declare').
> >> Additionally, we also want to expand the directory name when the command
> >> is called from Emacs Lisp.  So, I don't see how to deduplicate that
> >> call.
> >
> > But there are ways to know if the call was interactive, right?
> 
> Sorry, I thought you meant simplifying the code, not avoiding the second
> call at runtime.  I've attached a new version of the patch.

Philip, is this okay to install?

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78855 <at> debbugs.gnu.org, jroi.martin <at> gmail.com
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Sat, 28 Jun 2025 13:45:32 +0000

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Philip Kaludercic <philipk <at> posteo.net>
>> Cc: Eli Zaretskii <eliz <at> gnu.org>,  78855 <at> debbugs.gnu.org
>> Date: Thu, 26 Jun 2025 20:41:17 +0000
>> 
>> Roi Martin <jroi.martin <at> gmail.com> writes:
>> 
>> > Eli Zaretskii <eliz <at> gnu.org> writes:
>> >
>> >>> > Can we avoid this duplicate call to expand-file-name in the
>> >>> > interactive invocation?
>> >>> 
>> >>> I don't think so.  If I'm not wrong, `interactive' must be located at
>> >>> top-level and be the first form in the body (not counting `declare').
>> >>> Additionally, we also want to expand the directory name when the command
>> >>> is called from Emacs Lisp.  So, I don't see how to deduplicate that
>> >>> call.
>> >>
>> >> But there are ways to know if the call was interactive, right?
>> >
>> > Sorry, I thought you meant simplifying the code, not avoiding the second
>> > call at runtime.  I've attached a new version of the patch.
>> >
>> >         Roi
>> >
>> > From 16610a7da2f947e043db7ed95525091712ac38ff Mon Sep 17 00:00:00 2001
>> > From: Roi Martin <jroi.martin <at> gmail.com>
>> > Date: Sat, 21 Jun 2025 10:28:45 +0200
>> > Subject: [PATCH] Expand DIR arg on `package-vc-install-from-checkout'
>> >
>> > Expand the directory name passed to the
>> > `package-vc-install-from-checkout' function so that paths like
>> > "/path/to/pkg/./" are converted to "/path/to/pkg/".  This ensures that
>> > the package name defaults to "pkg" instead of "." (Bug#78855).
>> > * lisp/emacs-lisp/package-vc.el (package-vc-install-from-checkout):
>> > Expand the provided directory name.
>> > ---
>> >  lisp/emacs-lisp/package-vc.el | 7 ++++---
>> >  1 file changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>> > index babc0b715245..11e5af884d0a 100644
>> > --- a/lisp/emacs-lisp/package-vc.el
>> > +++ b/lisp/emacs-lisp/package-vc.el
>> > @@ -920,17 +920,18 @@ package-vc-install-from-checkout
>> >  under version control, typically one created by `package-vc-checkout'.
>> >  If invoked interactively with a prefix argument, prompt the user
>> >  for the NAME of the package to set up."
>> > -  (interactive (let* ((dir (read-directory-name "Directory: "))
>> > +  (interactive (let* ((dir (expand-file-name (read-directory-name "Directory: ")))
>> >                        (base (file-name-base (directory-file-name dir))))
>> >                   (list dir (and current-prefix-arg
>> >                                  (read-string
>> >                                   (format-prompt "Package name" base)
>> >                                   nil nil base)))))
>> >    (package-vc--archives-initialize)
>> > -  (let* ((name (or name (file-name-base (directory-file-name dir))))
>> > +  (let* ((dir (if (called-interactively-p 'any) dir (expand-file-name dir)))
>> 
>> I do not get the advantage of this approach?  Even if we ignore that
>> `called-interactively-p' is tricky, the result is that whatever `dir' is
>> bound to after this point has passed through `expand-file-name'?  So
>> isn't the first patch the simpler solution, with the same effect?
>
> I would like to avoid running file names through expand-file-name more
> than once, because that call could be expensive.  So in this case
> performance trumps simplicity, IMO.  Of course, if there's a simpler
> way of avoiding duplicate calls to expand-file-name, we should use it,
> so if you have a suggestion for such a simpler way, please tell the
> details.

I took a closer look at the code, and resolved my confusion.  My only
suggestion would be to use `file-name-absolute-p' instead of
`called-interactively-p', or would this also be too expensive (then
again, seeing as this is an interactive function, I am not under the
impression that the slowdown of invoking the function twice could be
/that/ slow, even with TRAMP involved (which doesn't seem that relevant,
I expect that people are not going to be loading packages over the
network...)).

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 78855 <at> debbugs.gnu.org, jroi.martin <at> gmail.com
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Sat, 28 Jun 2025 17:26:08 +0300

> From: Philip Kaludercic <philipk <at> posteo.net>
> Cc: 78855 <at> debbugs.gnu.org,  jroi.martin <at> gmail.com
> Date: Sat, 28 Jun 2025 13:45:32 +0000
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > I would like to avoid running file names through expand-file-name more
> > than once, because that call could be expensive.  So in this case
> > performance trumps simplicity, IMO.  Of course, if there's a simpler
> > way of avoiding duplicate calls to expand-file-name, we should use it,
> > so if you have a suggestion for such a simpler way, please tell the
> > details.
> 
> I took a closer look at the code, and resolved my confusion.  My only
> suggestion would be to use `file-name-absolute-p' instead of
> `called-interactively-p'

That doesn't work, because file-name-absolute-p returns non-nil for
~/foo file names.

> (then again, seeing as this is an interactive function, I am not
> under the impression that the slowdown of invoking the function
> twice could be /that/ slow, even with TRAMP involved (which doesn't
> seem that relevant, I expect that people are not going to be loading
> packages over the network...)).

The function can also be invoked non-interactively, though.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: philipk <at> posteo.net
Cc: 78855 <at> debbugs.gnu.org, jroi.martin <at> gmail.com
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Sat, 28 Jun 2025 17:33:27 +0300

> Cc: 78855 <at> debbugs.gnu.org, jroi.martin <at> gmail.com
> Date: Sat, 28 Jun 2025 17:26:08 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > From: Philip Kaludercic <philipk <at> posteo.net>
> > Cc: 78855 <at> debbugs.gnu.org,  jroi.martin <at> gmail.com
> > Date: Sat, 28 Jun 2025 13:45:32 +0000
> > 
> > (then again, seeing as this is an interactive function, I am not
> > under the impression that the slowdown of invoking the function
> > twice could be /that/ slow, even with TRAMP involved (which doesn't
> > seem that relevant, I expect that people are not going to be loading
> > packages over the network...)).
> 
> The function can also be invoked non-interactively, though.

And in addition, users are known to complain about slow responses of
interactive functions.

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78855 <at> debbugs.gnu.org, jroi.martin <at> gmail.com
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Mon, 30 Jun 2025 14:46:40 +0000

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

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Philip Kaludercic <philipk <at> posteo.net>
>> Cc: 78855 <at> debbugs.gnu.org,  jroi.martin <at> gmail.com
>> Date: Sat, 28 Jun 2025 13:45:32 +0000
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> > I would like to avoid running file names through expand-file-name more
>> > than once, because that call could be expensive.  So in this case
>> > performance trumps simplicity, IMO.  Of course, if there's a simpler
>> > way of avoiding duplicate calls to expand-file-name, we should use it,
>> > so if you have a suggestion for such a simpler way, please tell the
>> > details.
>> 
>> I took a closer look at the code, and resolved my confusion.  My only
>> suggestion would be to use `file-name-absolute-p' instead of
>> `called-interactively-p'
>
> That doesn't work, because file-name-absolute-p returns non-nil for
> ~/foo file names.

Considering that we are only interested in the base-name, that should be
fine, unless DIR is "~" -- which usually isn't a package directory, but
I guess it is safer not to make assumptions like these.

>> (then again, seeing as this is an interactive function, I am not
>> under the impression that the slowdown of invoking the function
>> twice could be /that/ slow, even with TRAMP involved (which doesn't
>> seem that relevant, I expect that people are not going to be loading
>> packages over the network...)).
>
> The function can also be invoked non-interactively, though.

True, though I can say that it wasn't my intention and it should have
been annotated that way.  It is too late for that now, in my estimation,
as it could break working code.

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Cc: 78855 <at> debbugs.gnu.org, jroi.martin <at> gmail.com
>> Date: Sat, 28 Jun 2025 17:26:08 +0300
>> From: Eli Zaretskii <eliz <at> gnu.org>
>> 
>> > From: Philip Kaludercic <philipk <at> posteo.net>
>> > Cc: 78855 <at> debbugs.gnu.org,  jroi.martin <at> gmail.com
>> > Date: Sat, 28 Jun 2025 13:45:32 +0000
>> > 
>> > (then again, seeing as this is an interactive function, I am not
>> > under the impression that the slowdown of invoking the function
>> > twice could be /that/ slow, even with TRAMP involved (which doesn't
>> > seem that relevant, I expect that people are not going to be loading
>> > packages over the network...)).
>> 
>> The function can also be invoked non-interactively, though.
>
> And in addition, users are known to complain about slow responses of
> interactive functions.

I remain unconvinced that two local file expansions would be that
computationally intensive, but I adjusted the patch nevertheless to
hopefully address the issue and reduce the number of file expansions.
The main difference is that the command now takes an explicit argument
to indicate interactive invocations, which I find preferable to the
`called-interactively-p'-approach.  Would appreciate comments:

[Message part 2 (text/x-patch, inline)]

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index babc0b71524..37bad5f9ddd 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -913,24 +913,29 @@ package-vc-checkout
     (find-file directory)))
 
 ;;;###autoload
-(defun package-vc-install-from-checkout (dir &optional name)
+(defun package-vc-install-from-checkout (dir &optional name interactive)
   "Install the package NAME from its source directory DIR.
-NAME defaults to the base name of DIR.
-Interactively, prompt the user for DIR, which should be a directory
-under version control, typically one created by `package-vc-checkout'.
-If invoked interactively with a prefix argument, prompt the user
-for the NAME of the package to set up."
-  (interactive (let* ((dir (read-directory-name "Directory: "))
-                      (base (file-name-base (directory-file-name dir))))
+NAME defaults to the base name of DIR.  Interactively, prompt the user
+for DIR, which should be a directory under version control, typically
+one created by `package-vc-checkout'.  If invoked interactively with a
+prefix argument, prompt the user for the NAME of the package to set up.
+If INTERACTIVE is non-nil, then the command will assume that argument
+DIR is in expanded form, and function will not expand DIR again."
+  (interactive (let ((dir (expand-file-name (read-directory-name "Directory: "))))
                  (list dir (and current-prefix-arg
-                                (read-string
-                                 (format-prompt "Package name" base)
-                                 nil nil base)))))
+                                (let ((base (file-name-base
+                                             (directory-file-name
+                                              dir))))
+                                  (read-string
+                                   (format-prompt "Package name" base)
+                                   nil nil base)))
+                       :interactive)))
   (package-vc--archives-initialize)
-  (let* ((name (or name (file-name-base (directory-file-name dir))))
-         (pkg-dir (expand-file-name name package-user-dir))
+  (let* ((dir (if interactive dir (expand-file-name dir))) ;avoid double expansion
+         (name (or name (file-name-base (directory-file-name dir))))
+         (pkg-dir (file-name-concat package-user-dir name))
          (package-vc-selected-packages
-          (cons (list name :lisp-dir (expand-file-name dir))
+          (cons (list name :lisp-dir dir)
                 package-vc-selected-packages)))
     (when (file-exists-p pkg-dir)
       (if (yes-or-no-p (format "Overwrite previous checkout for package `%s'?" name))

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 78855 <at> debbugs.gnu.org, jroi.martin <at> gmail.com
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Mon, 30 Jun 2025 18:43:33 +0300

> From: Philip Kaludercic <philipk <at> posteo.net>
> Cc: 78855 <at> debbugs.gnu.org,  jroi.martin <at> gmail.com
> Date: Mon, 30 Jun 2025 14:46:40 +0000
> 
> I remain unconvinced that two local file expansions would be that
> computationally intensive, but I adjusted the patch nevertheless to
> hopefully address the issue and reduce the number of file expansions.
> The main difference is that the command now takes an explicit argument
> to indicate interactive invocations, which I find preferable to the
> `called-interactively-p'-approach.  Would appreciate comments:

Thanks, LGTM, but I suggest a slight rewording of the doc string:

   "Install the package NAME from its source directory DIR.
 NAME defaults to the base name of DIR.  Interactively, prompt the user
 for DIR, which should be a directory under version control, typically
 one created by `package-vc-checkout'.  If invoked interactively with a
 prefix argument, prompt the user for the NAME of the package to set up.
 If the optional argument INTERACTIVE is non-nil (as happens
 interactively), DIR must be an absolute file name."

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78855 <at> debbugs.gnu.org, jroi.martin <at> gmail.com
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Mon, 30 Jun 2025 21:30:10 +0000

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Philip Kaludercic <philipk <at> posteo.net>
>> Cc: 78855 <at> debbugs.gnu.org,  jroi.martin <at> gmail.com
>> Date: Mon, 30 Jun 2025 14:46:40 +0000
>> 
>> I remain unconvinced that two local file expansions would be that
>> computationally intensive, but I adjusted the patch nevertheless to
>> hopefully address the issue and reduce the number of file expansions.
>> The main difference is that the command now takes an explicit argument
>> to indicate interactive invocations, which I find preferable to the
>> `called-interactively-p'-approach.  Would appreciate comments:
>
> Thanks, LGTM, but I suggest a slight rewording of the doc string:
>
>    "Install the package NAME from its source directory DIR.
>  NAME defaults to the base name of DIR.  Interactively, prompt the user
>  for DIR, which should be a directory under version control, typically
>  one created by `package-vc-checkout'.  If invoked interactively with a
>  prefix argument, prompt the user for the NAME of the package to set up.
>  If the optional argument INTERACTIVE is non-nil (as happens
>  interactively), DIR must be an absolute file name."

Will change that.  Just want to know what Roi thinks of the change, and
if I should commit it under my or their name?

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

From: Roi Martin <jroi.martin <at> gmail.com>
To: Philip Kaludercic <philipk <at> posteo.net>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 78855 <at> debbugs.gnu.org
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Tue, 01 Jul 2025 08:49:41 +0200

Philip Kaludercic <philipk <at> posteo.net> writes:

> Will change that.  Just want to know what Roi thinks of the change, and
> if I should commit it under my or their name?

LGTM.  Please, commit it under your name.  Thanks for giving it a look!

BTW, in the future, I'll follow your approach for detecting interactive
calls.  I saw that the Emacs Lisp reference recommends it, but I was
worried about adding a new optional parameter to the function.

        Roi

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Roi Martin <jroi.martin <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 78855-done <at> debbugs.gnu.org
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Tue, 01 Jul 2025 20:16:27 +0000

Roi Martin <jroi.martin <at> gmail.com> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>> Will change that.  Just want to know what Roi thinks of the change, and
>> if I should commit it under my or their name?
>
> LGTM.  Please, commit it under your name.  Thanks for giving it a look!

OK, but I have listed you under Co-developed-by FWIW.  I'm closing the
bug report now.

> BTW, in the future, I'll follow your approach for detecting interactive
> calls.  I saw that the Emacs Lisp reference recommends it, but I was
> worried about adding a new optional parameter to the function.

1+

>         Roi

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

From: Roi Martin <jroi.martin <at> gmail.com>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 78855-done <at> debbugs.gnu.org
Subject: Re: bug#78855: [PATCH] Expand DIR arg on
 `package-vc-install-from-checkout'
Date: Tue, 01 Jul 2025 22:23:46 +0200

Philip Kaludercic <philipk <at> posteo.net> writes:

> OK, but I have listed you under Co-developed-by FWIW.  I'm closing the
> bug report now.

Thanks! :)

Previous Next

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