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.

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

Previous Next

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