On 2024-09-24 10:22, Philip Kaludercic wrote: > Aleksandr Vityazev 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" >>>> >>>> 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: >> From: Aleksandr Vityazev >> 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: