GNU bug report logs - #73357
[PATCH] Make vc-clone interactive

Previous Next

Full log

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

From: Aleksandr Vityazev <avityazev <at> disroot.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 73357 <at> debbugs.gnu.org
Subject: Re: bug#73357: [PATCH] Make vc-clone interactive
Date: Sun, 29 Sep 2024 21:23:13 +0300

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

On 2024-09-24 10:22, Philip Kaludercic wrote:

> Aleksandr Vityazev <avityazev <at> disroot.org> writes:
>
>> On 2024-09-19 16:36, Eli Zaretskii wrote:
>>
>>>> Date: Thu, 19 Sep 2024 16:18:16 +0300
>>>> From:  Aleksandr Vityazev via "Bug reports for GNU Emacs,
>>>>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>>>> 
>>>> Cloning is used quite often, so I would like to have an interactive
>>>> command. A patch is attached to the email. WDYT?
>>>
>>> Thanks, but making this function a command will take more than just
>>> adding the interactive form.  I think we need at least:
>>>
>>>   . mention that in interactive usage the command prompts for the
>>>     argument (why do we have to prompt for REV, btw?)
>>
>> I clarified in the docstring. We can agree that we shouldn't prompt for REV
>> when cloning interactively, removed.
>>
>>>   . announce the change in NEWS
>> Announced but did not mark the news with +++ or ---.
>>>   . maybe update the user manual
>> maybe if I have to, I'll do it.
>>>   . maybe make the command fall back to 'checkout' method if 'clone'
>>>     is not supported
>>
>> it's worth thinking about, because I can't say for sure right now if
>> it's worth it. And how to do this when vc-checkout requires a file as an
>> argument.
>>
>>>> +    (when (file-directory-p directory)
>>>> +      (if (called-interactively-p 'interactive)
>>>> +          (find-file directory)
>>>> +        directory))))
>>>
>>> This changes the value returned by the function from what it did
>>> before, no?
>>
>> If the function is called from the code, it returns nil or the
>> directory, just like in the previous version. Or am I missing
>> something?
>>
>> V2 patch:
>>
>>>From b302b5c42e01fe0b6f7607128ed660babf55e35a Mon Sep 17 00:00:00 2001
>> Message-ID: <b302b5c42e01fe0b6f7607128ed660babf55e35a.1726762586.git.avityazev <at> disroot.org>
>> From: Aleksandr Vityazev <avityazev <at> disroot.org>
>> Date: Thu, 19 Sep 2024 16:11:31 +0300
>> Subject: [PATCH] Make vc-clone interactive
>>
>> * lisp/vc/vc.el (vc-clone): Make interactive.
>> Mention this in the doc string.
>> (vc--remotes-history): New defvar.
>> * etc/NEWS: Announce it.
>> ---
>>  etc/NEWS      |  7 +++++++
>>  lisp/vc/vc.el | 42 ++++++++++++++++++++++++++++--------------
>>  2 files changed, 35 insertions(+), 14 deletions(-)
>>
>> diff --git a/etc/NEWS b/etc/NEWS
>> index b52ad001a2e..db5f05c823c 100644
>> --- a/etc/NEWS
>> +++ b/etc/NEWS
>> @@ -359,6 +359,13 @@ functionality of the standard 'xref' commands in TeX buffers.  You can
>>  restore the standard 'etags' backend with the 'M-x xref-etags-mode'
>>  toggle.
>>  
>> +** VC
>> +
>> +*** 'vc-clone' is now an interactive command.
>> +When called interactively, 'vc-clone' now prompts for the address of the
>> +remote repository, the backend that will be used for cloning, as well as
>> +the directory where the repository will be cloned.
>
> Try to avoid passive voice, as in "be used" and "be cloned".

Fixed.

>
>> +
>>  
>>  * New Modes and Packages in Emacs 31.1
>>  
>> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
>> index 597a1622f5a..fc964803a02 100644
>> --- a/lisp/vc/vc.el
>> +++ b/lisp/vc/vc.el
>> @@ -3804,6 +3804,8 @@ vc-check-headers
>>    (interactive)
>>    (vc-call-backend (vc-backend buffer-file-name) 'check-headers))
>>  
>> +(defvar vc--remotes-history)
>> +
>>  (defun vc-clone (remote &optional backend directory rev)
>>    "Clone repository REMOTE using version-control BACKEND, into DIRECTORY.
>>  If successful, return the string with the directory of the checkout;
>> @@ -3814,20 +3816,32 @@ vc-clone
>>  If BACKEND is nil or omitted, the function iterates through every known
>>  backend in `vc-handled-backends' until one succeeds to clone REMOTE.
>>  If REV is non-nil, it indicates a specific revision to check out after
>> -cloning; the syntax of REV depends on what BACKEND accepts."
>> -  (setq directory (expand-file-name (or directory default-directory)))
>> -  (if backend
>> -      (progn
>> -        (unless (memq backend vc-handled-backends)
>> -          (error "Unknown VC backend %s" backend))
>> -        (vc-call-backend backend 'clone remote directory rev))
>> -    (catch 'ok
>> -      (dolist (backend vc-handled-backends)
>> -        (ignore-error vc-not-supported
>> -          (when-let ((res (vc-call-backend
>> -                           backend 'clone
>> -                           remote directory rev)))
>> -            (throw 'ok res)))))))
>> +cloning; the syntax of REV depends on what BACKEND accepts.
>> +If called interactively, prompt for REMOTE, BACKEND and DIRECTORY in
>> +the minibuffer."
>> +  (interactive
>> +   (list (read-string "Remote: " nil 'vc--remotes-history)
>> +         (intern (completing-read "Backend: " vc-handled-backends nil t))
>
> We could consider moving `package-vc-heuristic-alist' to vc so as to
> provide some useful default when cloning.

make sense, moved package-vc-heuristic-alist, package-vc--backend-type
and package-vc--guess-backend into vc

>
>> +         (expand-file-name
>
> Here also, I think it would make sense to re-use the same interface as
> `package-vc-checkout' provides, by prompting for a not-yet existing
> directory.

I agree

>
>> +          (read-directory-name "Clone dir: "))))
>> +  (let ((directory (expand-file-name (or directory default-directory))))
>> +    (setq directory
>
> I think binding this in a `let*' expression would look nicer.

also agree

>
>> +          (if backend
>> +              (progn
>> +                (unless (memq backend vc-handled-backends)
>> +                  (error "Unknown VC backend %s" backend))
>> +                (vc-call-backend backend 'clone remote directory rev))
>> +            (catch 'ok
>> +              (dolist (backend vc-handled-backends)
>> +                (ignore-error vc-not-supported
>> +                  (when-let ((res (vc-call-backend
>> +                                   backend 'clone
>> +                                   remote directory rev)))
>> +                    (throw 'ok res)))))))
>> +    (when (file-directory-p directory)
>> +      (if (called-interactively-p 'interactive)
>
> Perhaps we can add a FIND-FILE argument to the end, so that it is also
> possible to open the directory from a script as well.

might be useful, added and documented in doc string.

>
>> +          (find-file directory)
>> +        directory))))
>
> I'd always return `directory', that seems simpler.

Simpler, but it seems logical to switch to a directory when using it
interactively. I left it as it was.

>
>>  
>>  (declare-function log-view-current-tag "log-view" (&optional pos))
>>  (defun vc-default-last-change (_backend file line)
>> -- 
>> 2.46.0

V3 patch: 

[0001-Make-vc-clone-interactive.patch (text/x-patch, attachment)]

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

-- 
Best regards,
Aleksandr Vityazev

Previous Next

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