bug#66114: [PATCH] Fix interactive prompt for selecting checkout directory

bug#66114: [PATCH] Fix interactive prompt for selecting checkout directory

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Tags: patch

This patch fixes the interactive prompt in package-vc-checkout. See
commit message for details.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-interactive-prompt-for-selecting-checkout-direct.patch --]
[-- Type: text/patch, Size: 2213 bytes --]

From 70061d76542968575555f247d4715d18ed3c4e75 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 19 Sep 2023 23:58:43 -0700
Subject: [PATCH] Fix interactive prompt for selecting checkout directory

* lisp/emacs-lisp/package-vc.el (package-vc-checkout): Use
read-directory-name instead of read-file-name with
predicate.  Previously, it was impossible to interactively navigate to
a nested subdirectory.
---
 lisp/emacs-lisp/package-vc.el | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index c7a30736e32..29b540d86b8 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -823,12 +823,17 @@ repository.  If REV has the special value
 `:last-release' (interactively, the prefix argument), that stands
 for the last released version of the package."
   (interactive
-   (let* ((name (package-vc--read-package-name "Fetch package source: ")))
-     (list (cadr (assoc name package-archive-contents #'string=))
-           (read-file-name "Clone into new or empty directory: " nil nil t nil
-                           (lambda (dir) (or (not (file-exists-p dir))
-                                             (directory-empty-p dir))))
-           (and current-prefix-arg :last-release))))
+   (let* ((name (package-vc--read-package-name "Fetch package source: "))
+          (desc (cadr (assoc name package-archive-contents #'string=)))
+          (dir (read-directory-name "Clone into new or empty directory: ")))
+     (unless (or (not (file-exists-p dir))
+                 (directory-empty-p dir))
+       (let ((subdir (expand-file-name (package-desc-full-name desc) dir)))
+         (if (and (not (file-exists-p subdir))
+                  (y-or-n-p (format "Create new directory %s ?" subdir)))
+             (setf dir subdir)
+           (user-error "Directory not empty: %S" (expand-file-name dir)))))
+     (list desc dir (and current-prefix-arg :last-release))))
   (package-vc--archives-initialize)
   (let ((pkg-spec (or (package-vc--desc->spec pkg-desc)
                       (and-let* ((extras (package-desc-extras pkg-desc))
-- 
2.41.0

bug#66114: Acknowledgement ([PATCH] Fix interactive prompt for selecting checkout directory)

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

In this updated patch, I didn't not remove a double negative.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-interactive-prompt-for-selecting-checkout-direct.patch --]
[-- Type: text/x-diff, Size: 2209 bytes --]

From cc5d53e59928747b324eef312a4c83266a3bb8c3 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 19 Sep 2023 23:58:43 -0700
Subject: [PATCH] Fix interactive prompt for selecting checkout directory

* lisp/emacs-lisp/package-vc.el (package-vc-checkout): Use
read-directory-name instead of read-file-name with
predicate.  Previously, it was impossible to interactively navigate to
a nested subdirectory.
---
 lisp/emacs-lisp/package-vc.el | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index c7a30736e32..9caa1931af8 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -823,12 +823,17 @@ repository.  If REV has the special value
 `:last-release' (interactively, the prefix argument), that stands
 for the last released version of the package."
   (interactive
-   (let* ((name (package-vc--read-package-name "Fetch package source: ")))
-     (list (cadr (assoc name package-archive-contents #'string=))
-           (read-file-name "Clone into new or empty directory: " nil nil t nil
-                           (lambda (dir) (or (not (file-exists-p dir))
-                                             (directory-empty-p dir))))
-           (and current-prefix-arg :last-release))))
+   (let* ((name (package-vc--read-package-name "Fetch package source: "))
+          (desc (cadr (assoc name package-archive-contents #'string=)))
+          (dir (read-directory-name "Clone into new or empty directory: ")))
+     (when (or (file-exists-p dir)
+               (not (directory-empty-p dir)))
+       (let ((subdir (expand-file-name (package-desc-full-name desc) dir)))
+         (if (and (not (file-exists-p subdir))
+                  (y-or-n-p (format "Create new directory %s ?" subdir)))
+             (setf dir subdir)
+           (user-error "Directory not empty: %S" (expand-file-name dir)))))
+     (list desc dir (and current-prefix-arg :last-release))))
   (package-vc--archives-initialize)
   (let ((pkg-spec (or (package-vc--desc->spec pkg-desc)
                       (and-let* ((extras (package-desc-extras pkg-desc))
-- 
2.41.0

bug#66114: Acknowledgement ([PATCH] Fix interactive prompt for selecting checkout directory)

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Oops! I forgot how contrapositives work for a moment.

Please ignore the previous two patches.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-interactive-prompt-for-selecting-checkout-direct.patch --]
[-- Type: text/x-diff, Size: 2193 bytes --]

From aa3aca16755a8edec8d761e28d4ad0125cc5b7cd Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 19 Sep 2023 23:58:43 -0700
Subject: [PATCH] Fix interactive prompt for selecting checkout directory

* lisp/emacs-lisp/package-vc.el (package-vc-checkout): Use
read-directory-name instead of read-file-name with
predicate.  Previously, it was impossible to interactively navigate to
a nested subdirectory.
---
 lisp/emacs-lisp/package-vc.el | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index c7a30736e32..14014a84448 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -823,12 +823,16 @@ repository.  If REV has the special value
 `:last-release' (interactively, the prefix argument), that stands
 for the last released version of the package."
   (interactive
-   (let* ((name (package-vc--read-package-name "Fetch package source: ")))
-     (list (cadr (assoc name package-archive-contents #'string=))
-           (read-file-name "Clone into new or empty directory: " nil nil t nil
-                           (lambda (dir) (or (not (file-exists-p dir))
-                                             (directory-empty-p dir))))
-           (and current-prefix-arg :last-release))))
+   (let* ((name (package-vc--read-package-name "Fetch package source: "))
+          (desc (cadr (assoc name package-archive-contents #'string=)))
+          (dir (read-directory-name "Clone into new or empty directory: ")))
+     (when (and (file-exists-p dir) (not (directory-empty-p dir)))
+       (let ((subdir (expand-file-name (package-desc-full-name desc) dir)))
+         (if (and (not (file-exists-p subdir))
+                  (y-or-n-p (format "Create new directory %s ?" subdir)))
+             (setf dir subdir)
+           (user-error "Directory not empty: %S" (expand-file-name dir)))))
+     (list desc dir (and current-prefix-arg :last-release))))
   (package-vc--archives-initialize)
   (let ((pkg-spec (or (package-vc--desc->spec pkg-desc)
                       (and-let* ((extras (package-desc-extras pkg-desc))
-- 
2.41.0

bug#66114: [PATCH] Fix interactive prompt for selecting checkout directory

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Tags: patch
>
> This patch fixes the interactive prompt in package-vc-checkout. See
> commit message for details.
>
> From 70061d76542968575555f247d4715d18ed3c4e75 Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Tue, 19 Sep 2023 23:58:43 -0700
> Subject: [PATCH] Fix interactive prompt for selecting checkout directory
>
> * lisp/emacs-lisp/package-vc.el (package-vc-checkout): Use
> read-directory-name instead of read-file-name with
> predicate.  Previously, it was impossible to interactively navigate to
> a nested subdirectory.

I can "navigate" into whatever directory I want, but I don't get completion.

> ---
>  lisp/emacs-lisp/package-vc.el | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index c7a30736e32..29b540d86b8 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -823,12 +823,17 @@ repository.  If REV has the special value
>  `:last-release' (interactively, the prefix argument), that stands
>  for the last released version of the package."
>    (interactive
> -   (let* ((name (package-vc--read-package-name "Fetch package source: ")))
> -     (list (cadr (assoc name package-archive-contents #'string=))
> -           (read-file-name "Clone into new or empty directory: " nil nil t nil
> -                           (lambda (dir) (or (not (file-exists-p dir))
> -                                             (directory-empty-p dir))))
> -           (and current-prefix-arg :last-release))))
> +   (let* ((name (package-vc--read-package-name "Fetch package source: "))
> +          (desc (cadr (assoc name package-archive-contents #'string=)))
> +          (dir (read-directory-name "Clone into new or empty directory: ")))
> +     (unless (or (not (file-exists-p dir))
> +                 (directory-empty-p dir))
> +       (let ((subdir (expand-file-name (package-desc-full-name desc) dir)))
> +         (if (and (not (file-exists-p subdir))
> +                  (y-or-n-p (format "Create new directory %s ?" subdir)))
> +             (setf dir subdir)

I'd prefer to use a setq here.

> +           (user-error "Directory not empty: %S" (expand-file-name dir)))))

This seems like an anti-feature to me, because you need to know what
directory is empty before confirming your choice, and if it is not
empty, you have to restart the entire command again.

> +     (list desc dir (and current-prefix-arg :last-release))))
>    (package-vc--archives-initialize)
>    (let ((pkg-spec (or (package-vc--desc->spec pkg-desc)
>                        (and-let* ((extras (package-desc-extras pkg-desc))

bug#66114: [PATCH] Fix interactive prompt for selecting checkout directory

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Tags: patch
>>
>> This patch fixes the interactive prompt in package-vc-checkout. See
>> commit message for details.
>>
>> From 70061d76542968575555f247d4715d18ed3c4e75 Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Date: Tue, 19 Sep 2023 23:58:43 -0700
>> Subject: [PATCH] Fix interactive prompt for selecting checkout directory
>>
>> * lisp/emacs-lisp/package-vc.el (package-vc-checkout): Use
>> read-directory-name instead of read-file-name with
>> predicate.  Previously, it was impossible to interactively navigate to
>> a nested subdirectory.
>
> I can "navigate" into whatever directory I want, but I don't get completion.

You're right.

>> ---
>>  lisp/emacs-lisp/package-vc.el | 17 +++++++++++------
>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>> index c7a30736e32..29b540d86b8 100644
>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -823,12 +823,17 @@ repository.  If REV has the special value
>>  `:last-release' (interactively, the prefix argument), that stands
>>  for the last released version of the package."
>>    (interactive
>> -   (let* ((name (package-vc--read-package-name "Fetch package source: ")))
>> -     (list (cadr (assoc name package-archive-contents #'string=))
>> -           (read-file-name "Clone into new or empty directory: " nil nil t nil
>> -                           (lambda (dir) (or (not (file-exists-p dir))
>> -                                             (directory-empty-p dir))))
>> -           (and current-prefix-arg :last-release))))
>> +   (let* ((name (package-vc--read-package-name "Fetch package source: "))
>> +          (desc (cadr (assoc name package-archive-contents #'string=)))
>> +          (dir (read-directory-name "Clone into new or empty directory: ")))
>> +     (unless (or (not (file-exists-p dir))
>> +                 (directory-empty-p dir))
>> +       (let ((subdir (expand-file-name (package-desc-full-name desc) dir)))
>> +         (if (and (not (file-exists-p subdir))
>> +                  (y-or-n-p (format "Create new directory %s ?" subdir)))
>> +             (setf dir subdir)
>
> I'd prefer to use a setq here.
>
>> +           (user-error "Directory not empty: %S" (expand-file-name dir)))))
>
> This seems like an anti-feature to me, because you need to know what
> directory is empty before confirming your choice, and if it is not
> empty, you have to restart the entire command again.

Yes, I agree.

IIUC, we want to read from the user a directory which is either
nonexistent or empty.  However, we also want the completions to include
non-empty directories so that the user can easily select a deeply nested
directory.  The current solution does not offer non-empty directories for
completions, meaning that users must manually type in a potentially long
path to a nested empty or nonexistent directory.

Here's a different idea I tried:

(read-file-name "Clone into new or empty directory: " nil nil
                ;; Must match a nonexistent or empty directory
                (lambda (dir) (or (not (file-exists-p dir))
                                  (directory-empty-p dir)))
                nil
                ;; `read-directory-name' accepts no PREDICATE
                ;; argument: hack `read-file-name' instead.
                (lambda (file-name) (file-directory-p file-name)))

but this erroneously returns the default filename, which might be
buffer-file-name or whatever string is currently in the minibuffer.

I would have expected that when MUSTMATCH is a function and it returns
nil, a "[Match required]" message would appear.  However, the behavior of
read-file-name is unspecified when a MUSTMATCH function returns nil:

- a function, which will be called with the input as the
  argument.  If the function returns a non-nil value, the
  minibuffer is exited with that argument as the value.

Is this a bug, a case of under-documentation, or do I misunderstand something?

I'm happy to post this in a separate bug thread if you think it's worth it.

Joseph

>> +     (list desc dir (and current-prefix-arg :last-release))))
>>    (package-vc--archives-initialize)
>>    (let ((pkg-spec (or (package-vc--desc->spec pkg-desc)
>>                        (and-let* ((extras (package-desc-extras pkg-desc))

bug#66114: [PATCH] Fix interactive prompt for selecting checkout directory

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>
>>> Tags: patch
>>>
>>> This patch fixes the interactive prompt in package-vc-checkout. See
>>> commit message for details.
>>>
>>> From 70061d76542968575555f247d4715d18ed3c4e75 Mon Sep 17 00:00:00 2001
>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>> Date: Tue, 19 Sep 2023 23:58:43 -0700
>>> Subject: [PATCH] Fix interactive prompt for selecting checkout directory
>>>
>>> * lisp/emacs-lisp/package-vc.el (package-vc-checkout): Use
>>> read-directory-name instead of read-file-name with
>>> predicate.  Previously, it was impossible to interactively navigate to
>>> a nested subdirectory.
>>
>> I can "navigate" into whatever directory I want, but I don't get completion.
>
> You're right.
>
>>> ---
>>>  lisp/emacs-lisp/package-vc.el | 17 +++++++++++------
>>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>>> index c7a30736e32..29b540d86b8 100644
>>> --- a/lisp/emacs-lisp/package-vc.el
>>> +++ b/lisp/emacs-lisp/package-vc.el
>>> @@ -823,12 +823,17 @@ repository.  If REV has the special value
>>>  `:last-release' (interactively, the prefix argument), that stands
>>>  for the last released version of the package."
>>>    (interactive
>>> -   (let* ((name (package-vc--read-package-name "Fetch package source: ")))
>>> -     (list (cadr (assoc name package-archive-contents #'string=))
>>> -           (read-file-name "Clone into new or empty directory: " nil nil t nil
>>> -                           (lambda (dir) (or (not (file-exists-p dir))
>>> -                                             (directory-empty-p dir))))
>>> -           (and current-prefix-arg :last-release))))
>>> +   (let* ((name (package-vc--read-package-name "Fetch package source: "))
>>> +          (desc (cadr (assoc name package-archive-contents #'string=)))
>>> +          (dir (read-directory-name "Clone into new or empty directory: ")))
>>> +     (unless (or (not (file-exists-p dir))
>>> +                 (directory-empty-p dir))
>>> +       (let ((subdir (expand-file-name (package-desc-full-name desc) dir)))
>>> +         (if (and (not (file-exists-p subdir))
>>> +                  (y-or-n-p (format "Create new directory %s ?" subdir)))
>>> +             (setf dir subdir)
>>
>> I'd prefer to use a setq here.
>>
>>> +           (user-error "Directory not empty: %S" (expand-file-name dir)))))
>>
>> This seems like an anti-feature to me, because you need to know what
>> directory is empty before confirming your choice, and if it is not
>> empty, you have to restart the entire command again.
>
> Yes, I agree.
>
> IIUC, we want to read from the user a directory which is either
> nonexistent or empty.  However, we also want the completions to include
> non-empty directories so that the user can easily select a deeply nested
> directory.  The current solution does not offer non-empty directories for
> completions, meaning that users must manually type in a potentially long
> path to a nested empty or nonexistent directory.

It is funny to me, that I probably never noticed this issue because I
have the habit of using hippie-expand to complete partial file names,
including in the minibuffer :)

> Here's a different idea I tried:
>
> (read-file-name "Clone into new or empty directory: " nil nil
>                 ;; Must match a nonexistent or empty directory
>                 (lambda (dir) (or (not (file-exists-p dir))
>                                   (directory-empty-p dir)))
>                 nil
>                 ;; `read-directory-name' accepts no PREDICATE
>                 ;; argument: hack `read-file-name' instead.
>                 (lambda (file-name) (file-directory-p file-name)))
>
> but this erroneously returns the default filename, which might be
> buffer-file-name or whatever string is currently in the minibuffer.

An issue I am observing, is that it allows selecting non-empty
directories.

> I would have expected that when MUSTMATCH is a function and it returns
> nil, a "[Match required]" message would appear.  

For the record, what completion system are you using.  I don't appear to
see this message at all.

>                                                  However, the behavior of
> read-file-name is unspecified when a MUSTMATCH function returns nil:
>
> - a function, which will be called with the input as the
>   argument.  If the function returns a non-nil value, the
>   minibuffer is exited with that argument as the value.
>
> Is this a bug, a case of under-documentation, or do I misunderstand something?
>
> I'm happy to post this in a separate bug thread if you think it's worth it.

Perhaps that would be worth doing, that way the people who know more
about completion than I do would be more likely to see it and help out.

>
> Joseph
>
>>> +     (list desc dir (and current-prefix-arg :last-release))))
>>>    (package-vc--archives-initialize)
>>>    (let ((pkg-spec (or (package-vc--desc->spec pkg-desc)
>>>                        (and-let* ((extras (package-desc-extras pkg-desc))

-- 
Philip Kaludercic

bug#66114: [PATCH] Fix interactive prompt for selecting checkout directory

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>
>>>> Tags: patch
>>>>
>>>> This patch fixes the interactive prompt in package-vc-checkout.
>>>> See
>>>> commit message for details.
>>>>
>>>> From 70061d76542968575555f247d4715d18ed3c4e75 Mon Sep 17 00:00:00
>>>> 2001
>>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>>> Date: Tue, 19 Sep 2023 23:58:43 -0700
>>>> Subject: [PATCH] Fix interactive prompt for selecting checkout
>>>> directory
>>>>
>>>> * lisp/emacs-lisp/package-vc.el (package-vc-checkout): Use
>>>> read-directory-name instead of read-file-name with
>>>> predicate. Previously, it was impossible to interactively navigate
>>>> to
>>>> a nested subdirectory.
>>>
>>> I can "navigate" into whatever directory I want, but I don't get
>>> completion.
>>
>> You're right.
>>
>>>> ---
>>>>  lisp/emacs-lisp/package-vc.el | 17 +++++++++++------
>>>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>>>> index c7a30736e32..29b540d86b8 100644
>>>> --- a/lisp/emacs-lisp/package-vc.el
>>>> +++ b/lisp/emacs-lisp/package-vc.el
>>>> @@ -823,12 +823,17 @@ repository.  If REV has the special value
>>>>  `:last-release' (interactively, the prefix argument), that stands
>>>>  for the last released version of the package."
>>>>    (interactive
>>>> -   (let* ((name (package-vc--read-package-name "Fetch package source: ")))
>>>> -     (list (cadr (assoc name package-archive-contents #'string=))
>>>> -           (read-file-name "Clone into new or empty directory: " nil nil t nil
>>>> -                           (lambda (dir) (or (not (file-exists-p dir))
>>>> -                                             (directory-empty-p dir))))
>>>> -           (and current-prefix-arg :last-release))))
>>>> +   (let* ((name (package-vc--read-package-name "Fetch package source: "))
>>>> +          (desc (cadr (assoc name package-archive-contents #'string=)))
>>>> +          (dir (read-directory-name "Clone into new or empty directory: ")))
>>>> +     (unless (or (not (file-exists-p dir))
>>>> +                 (directory-empty-p dir))
>>>> +       (let ((subdir (expand-file-name (package-desc-full-name desc) dir)))
>>>> +         (if (and (not (file-exists-p subdir))
>>>> +                  (y-or-n-p (format "Create new directory %s ?" subdir)))
>>>> +             (setf dir subdir)
>>>
>>> I'd prefer to use a setq here.
>>>
>>>> + (user-error "Directory not empty: %S" (expand-file-name dir)))))
>>>
>>> This seems like an anti-feature to me, because you need to know
>>> what
>>> directory is empty before confirming your choice, and if it is not
>>> empty, you have to restart the entire command again.
>>
>> Yes, I agree.
>>
>> IIUC, we want to read from the user a directory which is either
>> nonexistent or empty. However, we also want the completions to
>> include
>> non-empty directories so that the user can easily select a deeply
>> nested
>> directory. The current solution does not offer non-empty directories
>> for
>> completions, meaning that users must manually type in a potentially
>> long
>> path to a nested empty or nonexistent directory.
>
> It is funny to me, that I probably never noticed this issue because I
> have the habit of using hippie-expand to complete partial file names,
> including in the minibuffer :)

Ah, that makes sense :)

>> Here's a different idea I tried:
>>
>> (read-file-name "Clone into new or empty directory: " nil nil
>>                 ;; Must match a nonexistent or empty directory
>>                 (lambda (dir) (or (not (file-exists-p dir))
>>                                   (directory-empty-p dir)))
>>                 nil
>>                 ;; `read-directory-name' accepts no PREDICATE
>>                 ;; argument: hack `read-file-name' instead.
>>                 (lambda (file-name) (file-directory-p file-name)))
>>
>> but this erroneously returns the default filename, which might be
>> buffer-file-name or whatever string is currently in the minibuffer.
>
> An issue I am observing, is that it allows selecting non-empty
> directories.
>
>> I would have expected that when MUSTMATCH is a function and it
>> returns
>> nil, a "[Match required]" message would appear.
>
> For the record, what completion system are you using. I don't appear
> to
> see this message at all.

I also do not see the message, but I expect to see it. I am using
vertico + orderless.

>>                                                  However, the behavior of
>> read-file-name is unspecified when a MUSTMATCH function returns nil:
>>
>> - a function, which will be called with the input as the
>>   argument.  If the function returns a non-nil value, the
>>   minibuffer is exited with that argument as the value.
>>
>> Is this a bug, a case of under-documentation, or do I misunderstand something?
>>
>> I'm happy to post this in a separate bug thread if you think it's worth it.
>
> Perhaps that would be worth doing, that way the people who know more
> about completion than I do would be more likely to see it and help
> out.

Okay, thanks! I'll do that.

>>
>> Joseph
>>
>>>> +     (list desc dir (and current-prefix-arg :last-release))))
>>>>    (package-vc--archives-initialize)
>>>>    (let ((pkg-spec (or (package-vc--desc->spec pkg-desc)
>>>>                        (and-let* ((extras (package-desc-extras pkg-desc))

bug#66114: [PATCH] Fix interactive prompt for selecting checkout directory

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>
>>>                                                  However, the behavior of
>>> read-file-name is unspecified when a MUSTMATCH function returns nil:
>>>
>>> - a function, which will be called with the input as the
>>>   argument.  If the function returns a non-nil value, the
>>>   minibuffer is exited with that argument as the value.
>>>
>>> Is this a bug, a case of under-documentation, or do I misunderstand something?
>>>
>>> I'm happy to post this in a separate bug thread if you think it's worth it.
>>
>> Perhaps that would be worth doing, that way the people who know more
>> about completion than I do would be more likely to see it and help
>> out.
>
> Okay, thanks! I'll do that.

Now that #66187
(<https://yhetil.org/emacs-bugs/87cytlqmqh.fsf@breatheoutbreathe.in/>)
and bug#68815
(<https://yhetil.org/emacs-bugs/8734ufyxoy.fsf@breatheoutbreathe.in/>)
are fixed on emacs-29, I'm revisiting this issue.

With the following patch, the interactive prompt in package-vc-checkout
now offers all directory for completion (empty or not), but only exits
when the minibuffer contains an empty directory or nonexistent filename.

Testing this patch requires checking out emacs-29 after e6c82fe35e3.

Thanks!

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-package-vc-checkout-interactive-prompt-bug-6.patch --]
[-- Type: text/x-diff, Size: 1326 bytes --]

From e6c82fe35e3f5de107020ee206043bd80cbfff27 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 30 Jan 2024 00:52:39 -0800
Subject: [PATCH] Improve package-vc-checkout interactive prompt (bug#66114)

* lisp/emacs-lisp/package-vc.el (package-vc--read-package-name): Use
read-directory-name instead of read-file-name.
---
 lisp/emacs-lisp/package-vc.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index e89ead89d4b..5c5486de290 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -825,8 +825,8 @@ package-vc-checkout
   (interactive
    (let* ((name (package-vc--read-package-name "Fetch package source: ")))
      (list (cadr (assoc name package-archive-contents #'string=))
-           (read-file-name "Clone into new or empty directory: " nil nil t nil
-                           (lambda (dir) (or (not (file-exists-p dir))
+           (read-directory-name "Clone into new or empty directory: " nil nil
+                                (lambda (dir) (or (not (file-exists-p dir))
                                              (directory-empty-p dir))))
            (and current-prefix-arg :last-release))))
   (setf directory (expand-file-name directory))
-- 
2.41.0

bug#66114: [PATCH] Fix interactive prompt for selecting checkout directory

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>
>>>>                                                  However, the behavior of
>>>> read-file-name is unspecified when a MUSTMATCH function returns nil:
>>>>
>>>> - a function, which will be called with the input as the
>>>>   argument.  If the function returns a non-nil value, the
>>>>   minibuffer is exited with that argument as the value.
>>>>
>>>> Is this a bug, a case of under-documentation, or do I misunderstand something?
>>>>
>>>> I'm happy to post this in a separate bug thread if you think it's worth it.
>>>
>>> Perhaps that would be worth doing, that way the people who know more
>>> about completion than I do would be more likely to see it and help
>>> out.
>>
>> Okay, thanks! I'll do that.
>
> Now that #66187
> (<https://yhetil.org/emacs-bugs/87cytlqmqh.fsf@breatheoutbreathe.in/>)
> and bug#68815
> (<https://yhetil.org/emacs-bugs/8734ufyxoy.fsf@breatheoutbreathe.in/>)
> are fixed on emacs-29, I'm revisiting this issue.
>
> With the following patch, the interactive prompt in package-vc-checkout
> now offers all directory for completion (empty or not), but only exits
> when the minibuffer contains an empty directory or nonexistent filename.
>
> Testing this patch requires checking out emacs-29 after e6c82fe35e3.

LGTM, I've pushed it to emacs-29.  Thanks!

> Thanks!
>
> Joseph

bug#66114: [PATCH] Fix interactive prompt for selecting checkout directory

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>
>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>
>>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>>
>>>>>                                                  However, the behavior of
>>>>> read-file-name is unspecified when a MUSTMATCH function returns nil:
>>>>>
>>>>> - a function, which will be called with the input as the
>>>>>   argument.  If the function returns a non-nil value, the
>>>>>   minibuffer is exited with that argument as the value.
>>>>>
>>>>> Is this a bug, a case of under-documentation, or do I misunderstand something?
>>>>>
>>>>> I'm happy to post this in a separate bug thread if you think it's worth it.
>>>>
>>>> Perhaps that would be worth doing, that way the people who know more
>>>> about completion than I do would be more likely to see it and help
>>>> out.
>>>
>>> Okay, thanks! I'll do that.
>>
>> Now that #66187
>> (<https://yhetil.org/emacs-bugs/87cytlqmqh.fsf@breatheoutbreathe.in/>)
>> and bug#68815
>> (<https://yhetil.org/emacs-bugs/8734ufyxoy.fsf@breatheoutbreathe.in/>)
>> are fixed on emacs-29, I'm revisiting this issue.
>>
>> With the following patch, the interactive prompt in package-vc-checkout
>> now offers all directory for completion (empty or not), but only exits
>> when the minibuffer contains an empty directory or nonexistent filename.
>>
>> Testing this patch requires checking out emacs-29 after e6c82fe35e3.
>
> LGTM, I've pushed it to emacs-29.  Thanks!

Thanks!!

Joseph