bug#66115: [PATCH] Ensure that directory is expanded in package-vc-checkout

bug#66115: [PATCH] Ensure that directory is expanded in package-vc-checkout

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

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

Tags: patch

Tags: patch

This patch fixes a serious bug which caused package-vc-checkout to fail
entirely when its second argument `directory' was not expanded.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Ensure-that-directory-is-expanded-in-package-vc-chec.patch --]
[-- Type: text/patch, Size: 1015 bytes --]

From 75a6ec74ff5e31edc7592b68e7ad9d1ec26f515c Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Wed, 20 Sep 2023 00:12:10 -0700
Subject: [PATCH] Ensure that directory is expanded in package-vc-checkout

* lisp/emacs-lisp/package-vc.el (package-vc-checkout)
---
 lisp/emacs-lisp/package-vc.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 29b540d86b8..47d8c996ff0 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -834,6 +834,7 @@ for the last released version of the package."
              (setf dir subdir)
            (user-error "Directory not empty: %S" (expand-file-name dir)))))
      (list desc dir (and current-prefix-arg :last-release))))
+  (setf directory (expand-file-name directory))
   (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#66115: [PATCH] Ensure that directory is expanded in package-vc-checkout

[Philip Kaludercic]

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

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Tags: patch
>
> Tags: patch
>
> This patch fixes a serious bug which caused package-vc-checkout to fail
> entirely when its second argument `directory' was not expanded.
>
> From 75a6ec74ff5e31edc7592b68e7ad9d1ec26f515c Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Wed, 20 Sep 2023 00:12:10 -0700
> Subject: [PATCH] Ensure that directory is expanded in package-vc-checkout
>
> * lisp/emacs-lisp/package-vc.el (package-vc-checkout)
> ---
>  lisp/emacs-lisp/package-vc.el | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index 29b540d86b8..47d8c996ff0 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -834,6 +834,7 @@ for the last released version of the package."
>               (setf dir subdir)
>             (user-error "Directory not empty: %S" (expand-file-name dir)))))
>       (list desc dir (and current-prefix-arg :last-release))))
> +  (setf directory (expand-file-name directory))
>    (package-vc--archives-initialize)
>    (let ((pkg-spec (or (package-vc--desc->spec pkg-desc)
>                        (and-let* ((extras (package-desc-extras pkg-desc))

DIRECTORY is passed on to `package-vc--clone' and in turn `vc-clone'.
Wouldn't it be more robust to ensure that `vc-clone' can handle
unexpanded directory names, eg. like this:


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

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 2f4b028bb4a..7f334397a5e 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -3676,8 +3676,7 @@ vc-clone
 checkout.  If BACKEND is nil, iterate through every known backend
 in `vc-handled-backends' until one succeeds.  If REV is non-nil,
 it indicates a specific revision to check out."
-  (unless directory
-    (setq directory default-directory))
+  (setq directory (expand-file-name (or directory default-directory)))
   (if backend
       (progn
         (unless (memq backend vc-handled-backends)

bug#66115: [PATCH] Ensure that directory is expanded in package-vc-checkout

[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
>>
>> Tags: patch
>>
>> This patch fixes a serious bug which caused package-vc-checkout to fail
>> entirely when its second argument `directory' was not expanded.
>>
>> From 75a6ec74ff5e31edc7592b68e7ad9d1ec26f515c Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Date: Wed, 20 Sep 2023 00:12:10 -0700
>> Subject: [PATCH] Ensure that directory is expanded in package-vc-checkout
>>
>> * lisp/emacs-lisp/package-vc.el (package-vc-checkout)
>> ---
>>  lisp/emacs-lisp/package-vc.el | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>> index 29b540d86b8..47d8c996ff0 100644
>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -834,6 +834,7 @@ for the last released version of the package."
>>               (setf dir subdir)
>>             (user-error "Directory not empty: %S" (expand-file-name dir)))))
>>       (list desc dir (and current-prefix-arg :last-release))))
>> +  (setf directory (expand-file-name directory))
>>    (package-vc--archives-initialize)
>>    (let ((pkg-spec (or (package-vc--desc->spec pkg-desc)
>>                        (and-let* ((extras (package-desc-extras pkg-desc))
>
> DIRECTORY is passed on to `package-vc--clone' and in turn `vc-clone'.
> Wouldn't it be more robust to ensure that `vc-clone' can handle
> unexpanded directory names, eg. like this:
>
> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> index 2f4b028bb4a..7f334397a5e 100644
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -3676,8 +3676,7 @@ vc-clone
>  checkout.  If BACKEND is nil, iterate through every known backend
>  in `vc-handled-backends' until one succeeds.  If REV is non-nil,
>  it indicates a specific revision to check out."
> -  (unless directory
> -    (setq directory default-directory))
> +  (setq directory (expand-file-name (or directory default-directory)))
>    (if backend
>        (progn
>          (unless (memq backend vc-handled-backends)

Yes, certainly. Your solution is better.

bug#66115: [PATCH] Ensure that directory is expanded in package-vc-checkout

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>
>>> Tags: patch
>>>
>>> Tags: patch
>>>
>>> This patch fixes a serious bug which caused package-vc-checkout to fail
>>> entirely when its second argument `directory' was not expanded.
>>>
>>> From 75a6ec74ff5e31edc7592b68e7ad9d1ec26f515c Mon Sep 17 00:00:00 2001
>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>> Date: Wed, 20 Sep 2023 00:12:10 -0700
>>> Subject: [PATCH] Ensure that directory is expanded in package-vc-checkout
>>>
>>> * lisp/emacs-lisp/package-vc.el (package-vc-checkout)
>>> ---
>>>  lisp/emacs-lisp/package-vc.el | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>>> index 29b540d86b8..47d8c996ff0 100644
>>> --- a/lisp/emacs-lisp/package-vc.el
>>> +++ b/lisp/emacs-lisp/package-vc.el
>>> @@ -834,6 +834,7 @@ for the last released version of the package."
>>>               (setf dir subdir)
>>>             (user-error "Directory not empty: %S" (expand-file-name dir)))))
>>>       (list desc dir (and current-prefix-arg :last-release))))
>>> +  (setf directory (expand-file-name directory))
>>>    (package-vc--archives-initialize)
>>>    (let ((pkg-spec (or (package-vc--desc->spec pkg-desc)
>>>                        (and-let* ((extras (package-desc-extras pkg-desc))
>>
>> DIRECTORY is passed on to `package-vc--clone' and in turn `vc-clone'.
>> Wouldn't it be more robust to ensure that `vc-clone' can handle
>> unexpanded directory names, eg. like this:
>>
>> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
>> index 2f4b028bb4a..7f334397a5e 100644
>> --- a/lisp/vc/vc.el
>> +++ b/lisp/vc/vc.el
>> @@ -3676,8 +3676,7 @@ vc-clone
>>  checkout.  If BACKEND is nil, iterate through every known backend
>>  in `vc-handled-backends' until one succeeds.  If REV is non-nil,
>>  it indicates a specific revision to check out."
>> -  (unless directory
>> -    (setq directory default-directory))
>> +  (setq directory (expand-file-name (or directory default-directory)))
>>    (if backend
>>        (progn
>>          (unless (memq backend vc-handled-backends)
>
> Yes, certainly. Your solution is better.

OK, closing the issue.

bug#66115: [PATCH] Ensure that directory is expanded in package-vc-checkout

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

It appears that the fix was applied to master. Would it make sense to
apply it to emacs-29?

bug#66115: [PATCH] Ensure that directory is expanded in package-vc-checkout

[Eli Zaretskii]

> Cc: 66115-done@debbugs.gnu.org
> Date: Sat, 23 Sep 2023 22:34:24 -0700
> From:  Joseph Turner via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> It appears that the fix was applied to master. Would it make sense to
> apply it to emacs-29?

The installed change affects much more than the original issue, so I'd
prefer for it to stay on master.

I'm okay with installing the original patch on emacs-29, but be sure
to say "do not merge to master" in the commit log message if you do.

bug#66115: [PATCH] Ensure that directory is expanded in package-vc-checkout

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Cc: 66115-done@debbugs.gnu.org
>>> Date: Sat, 23 Sep 2023 22:34:24 -0700
>>> From:  Joseph Turner via "Bug reports for GNU Emacs,
>>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>>
>>> It appears that the fix was applied to master. Would it make sense to
>>> apply it to emacs-29?
>>
>> The installed change affects much more than the original issue, so I'd
>> prefer for it to stay on master.
>>
>> I'm okay with installing the original patch on emacs-29, but be sure
>> to say "do not merge to master" in the commit log message if you do.
>
> Philip, shall we merge the original patch on emacs-29 with "do not merge
> to master"?

I don't object.

> Thank you!!
>
> Joseph

bug#66115: [PATCH] Ensure that directory is expanded in package-vc-checkout

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

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


Philip Kaludercic <philipk@posteo.net> writes:

> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> Cc: 66115-done@debbugs.gnu.org
>>>> Date: Sat, 23 Sep 2023 22:34:24 -0700
>>>> From:  Joseph Turner via "Bug reports for GNU Emacs,
>>>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>>>
>>>> It appears that the fix was applied to master. Would it make sense to
>>>> apply it to emacs-29?
>>>
>>> The installed change affects much more than the original issue, so I'd
>>> prefer for it to stay on master.
>>>
>>> I'm okay with installing the original patch on emacs-29, but be sure
>>> to say "do not merge to master" in the commit log message if you do.
>>
>> Philip, shall we merge the original patch on emacs-29 with "do not merge
>> to master"?
>
> I don't object.

Would you be willing to merge the attached patch?

Thank you!!

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Ensure-that-directory-is-expanded-in-package-vc-chec.patch --]
[-- Type: text/x-diff, Size: 1015 bytes --]

From 75a6ec74ff5e31edc7592b68e7ad9d1ec26f515c Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Wed, 20 Sep 2023 00:12:10 -0700
Subject: [PATCH] Ensure that directory is expanded in package-vc-checkout

* lisp/emacs-lisp/package-vc.el (package-vc-checkout)
---
 lisp/emacs-lisp/package-vc.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 29b540d86b8..47d8c996ff0 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -834,6 +834,7 @@ for the last released version of the package."
              (setf dir subdir)
            (user-error "Directory not empty: %S" (expand-file-name dir)))))
      (list desc dir (and current-prefix-arg :last-release))))
+  (setf directory (expand-file-name directory))
   (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#66115: [PATCH] Ensure that directory is expanded in package-vc-checkout

[Eli Zaretskii]

> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Cc: Eli Zaretskii <eliz@gnu.org>, 66115@debbugs.gnu.org
> Date: Sun, 19 Nov 2023 14:16:39 -0800
> 
> Philip Kaludercic <philipk@posteo.net> writes:
> 
> > Joseph Turner <joseph@breatheoutbreathe.in> writes:
> >
> >> Eli Zaretskii <eliz@gnu.org> writes:
> >>
> >>>> Cc: 66115-done@debbugs.gnu.org
> >>>> Date: Sat, 23 Sep 2023 22:34:24 -0700
> >>>> From:  Joseph Turner via "Bug reports for GNU Emacs,
> >>>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> >>>>
> >>>> It appears that the fix was applied to master. Would it make sense to
> >>>> apply it to emacs-29?
> >>>
> >>> The installed change affects much more than the original issue, so I'd
> >>> prefer for it to stay on master.
> >>>
> >>> I'm okay with installing the original patch on emacs-29, but be sure
> >>> to say "do not merge to master" in the commit log message if you do.
> >>
> >> Philip, shall we merge the original patch on emacs-29 with "do not merge
> >> to master"?
> >
> > I don't object.
> 
> Would you be willing to merge the attached patch?

I tried, but it doesn't apply to emacs-29.  Would you please submit a
change relative to the emacs-29 branch?

TIA

bug#66115: [PATCH] Ensure that directory is expanded in package-vc-checkout

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

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

Eli Zaretskii <eliz@gnu.org> writes:

> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Would you be willing to merge the attached patch?
>
> I tried, but it doesn't apply to emacs-29.  Would you please submit a
> change relative to the emacs-29 branch?

Sorry about that!  Here you go!

Thanks,

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Ensure-that-directory-is-expanded-in-package-vc-chec.patch --]
[-- Type: text/x-diff, Size: 1053 bytes --]

From 49a94115d7f6bde1618f754265ec3ea8154dd922 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 25 Nov 2023 14:57:45 -0800
Subject: [PATCH] Ensure that directory is expanded in package-vc-checkout

* lisp/emacs-lisp/package-vc.el (package-vc-checkout)

bug#66115
---
 lisp/emacs-lisp/package-vc.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index c7a30736e32..2a5f14b3ee3 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -829,6 +829,7 @@ for the last released version of the package."
                            (lambda (dir) (or (not (file-exists-p dir))
                                              (directory-empty-p dir))))
            (and current-prefix-arg :last-release))))
+  (setf directory (expand-file-name directory))
   (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#66115: [PATCH] Ensure that directory is expanded in package-vc-checkout

[Eli Zaretskii]

> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Cc: philipk@posteo.net, 66115@debbugs.gnu.org
> Date: Sat, 25 Nov 2023 14:59:46 -0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > From: Joseph Turner <joseph@breatheoutbreathe.in>
> >> Would you be willing to merge the attached patch?
> >
> > I tried, but it doesn't apply to emacs-29.  Would you please submit a
> > change relative to the emacs-29 branch?
> 
> Sorry about that!  Here you go!

Thanks, installed on the emacs-29 branch, and closing the bug.

bug#66115: [PATCH] Ensure that directory is expanded in package-vc-checkout

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Cc: philipk@posteo.net, 66115@debbugs.gnu.org
>> Date: Sat, 25 Nov 2023 14:59:46 -0800
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> > From: Joseph Turner <joseph@breatheoutbreathe.in>
>> >> Would you be willing to merge the attached patch?
>> >
>> > I tried, but it doesn't apply to emacs-29.  Would you please submit a
>> > change relative to the emacs-29 branch?
>>
>> Sorry about that!  Here you go!
>
> Thanks, installed on the emacs-29 branch, and closing the bug.

Thank you, Eli!  I made a mistake - I forgot to add "Do not merge into
master" in the commit message.  Do we need to add a commit on the master
branch which reverts this one?

Joseph

bug#66115: [PATCH] Ensure that directory is expanded in package-vc-checkout

[Eli Zaretskii]

> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Cc: philipk@posteo.net, 66115-done@debbugs.gnu.org
> Date: Sun, 26 Nov 2023 12:39:34 -0800
> 
> > Thanks, installed on the emacs-29 branch, and closing the bug.
> 
> Thank you, Eli!  I made a mistake - I forgot to add "Do not merge into
> master" in the commit message.  Do we need to add a commit on the master
> branch which reverts this one?

Yes, after the next merge from emacs-29 to master.

bug#66115: [PATCH] Ensure that directory is expanded in package-vc-checkout

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Cc: philipk@posteo.net, 66115-done@debbugs.gnu.org
>> Date: Sun, 26 Nov 2023 12:39:34 -0800
>>
>> > Thanks, installed on the emacs-29 branch, and closing the bug.
>>
>> Thank you, Eli!  I made a mistake - I forgot to add "Do not merge into
>> master" in the commit message.  Do we need to add a commit on the master
>> branch which reverts this one?
>
> Yes, after the next merge from emacs-29 to master.

I see that you've done this already.  Thank you!