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

Previous Next

Full log

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Aleksandr Vityazev <avityazev <at> disroot.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 73357 <at> debbugs.gnu.org
Subject: Re: bug#73357: [PATCH] Make vc-clone interactive
Date: Tue, 24 Sep 2024 10:22:31 +0000

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".

> +
>  
>  * 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.

> +         (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.

> +          (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.

> +          (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.

> +          (find-file directory)
> +        directory))))

I'd always return `directory', that seems simpler.

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

-- 
	Philip Kaludercic on siskin

Previous Next

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