bug#73357: [PATCH] Make vc-clone interactive

[Aleksandr Vityazev via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

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

On 2024-10-24 19:26, Sean Whitton wrote:

Hello,

> Hello,
>
> On Thu 24 Oct 2024 at 10:43am GMT, Philip Kaludercic wrote:
>
>> Aleksandr Vityazev <avityazev@disroot.org> writes:
>>
>> [...]
>>
>>>>> V4 patches:
>>>>
>>>> Thanks.
>>>>
>>>> Dmitry, any comments, or should I install this?
>>>
>>> Just a gentle ping, any news on this bug?
>>
>> FWIW as the vc-clone author, I think we can apply it, but Dmitry is the
>> VC maintainer so he should have the last word.
>
> I'm the new VC maintainer.
>
> Aleksandr, thank you for this.  Some comments on v4:
>
> - The commit message of the first patch doesn't completely follow the
>   guidelines in CONTRIBUTE.  I think M-q will fix it.

fixed
>   - I also find the ... thing hard to read because it's separated by
>     other changes.  Would you mind just writing out the changes twice?

fixed
> - vc-heuristic-alist should probaby have ':version 31.1'

fixed
> - Inserting vc-guess-backend right at the top doesn't seem right.  There
>   is already a section "Code for deducing what fileset and backend to
>   assume".
Moved to suggested section

> - I think that vc-guess-backend should be called vc-guess-url-backend or
>   similar.  'vc-guess-backend' is too generic.

Renamed to vc-guess-url-backend

> - I'm not really convinced by the OPEN-DIR argument.  You specifically
>   say that it's for scripting purposes, but then, the script can just
>   call find-file :)  Is there some reason why it's better as an
>   argument?

I don't have a strong opinion on this. I originally proposed this:

(when (file-directory-p directory)
  (if (called-interactively-p 'interactive)
      (find-file directory)
    directory))

The OPEN-DIR argument was suggested by Philip, and I agreed with him,
since the option is also good.  I'm fine with both options, I'll do as
you say.

V5 patches:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] Move package-vc-heuristic-alist and related to vc.el --]
[-- Type: text/x-patch, Size: 10640 bytes --]

From 7072d2002a4b0852ba6d133f1f4728fc7d25ccc6 Mon Sep 17 00:00:00 2001
Message-ID: <7072d2002a4b0852ba6d133f1f4728fc7d25ccc6.1729772961.git.avityazev@disroot.org>
From: Aleksandr Vityazev <avityazev@disroot.org>
Date: Thu, 24 Oct 2024 15:11:44 +0300
Subject: [PATCH] Move package-vc-heuristic-alist and related to vc.el

* lisp/emacs-lisp/package-vc (package-vc--backend-type)
(package-vc-heuristic-alist, package-vc--guess-backend): Rename
to vc-backend-type, vc-heuristic-alist, vc-guess-backend
accordingly and move into lisp/vc/vc.el.
(package-vc-heuristic-alist): Make obsolete.
(package-vc-default-backend): Set type to vc-backend-type.
(package-vc--clone, package-vc--read-package-name)
(package-vc-install, package-vc-checkout): Use vc-guess-backend.
* lisp/vc/vc (vc-backend-type, vc-heuristic-alist)
(vc-guess-backend): New defconst, defcustom and defun
accordingly.  Rename and move here from
lisp/emacs-lisp/package-vc.el.
---
 lisp/emacs-lisp/package-vc.el | 77 ++++-------------------------------
 lisp/vc/vc.el                 | 62 ++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 68 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 894bc9c8c37..6ea55c1baef 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -63,61 +63,9 @@ package-vc
 (defconst package-vc--elpa-packages-version 1
   "Version number of the package specification format understood by package-vc.")
 
-(defconst package-vc--backend-type
-  `(choice :convert-widget
-           ,(lambda (widget)
-              (let (opts)
-                (dolist (be vc-handled-backends)
-                  (when (or (vc-find-backend-function be 'clone)
-                            (alist-get 'clone (get be 'vc-functions)))
-                    (push (widget-convert (list 'const be)) opts)))
-                (widget-put widget :args opts))
-              widget))
-  "The type of VC backends that support cloning package VCS repositories.")
-
-(defcustom package-vc-heuristic-alist
-  `((,(rx bos "http" (? "s") "://"
-          (or (: (? "www.") "github.com"
-                 "/" (+ (or alnum "-" "." "_"))
-                 "/" (+ (or alnum "-" "." "_")))
-              (: "codeberg.org"
-                 "/" (+ (or alnum "-" "." "_"))
-                 "/" (+ (or alnum "-" "." "_")))
-              (: (? "www.") "gitlab" (+ "." (+ alnum))
-                 "/" (+ (or alnum "-" "." "_"))
-                 "/" (+ (or alnum "-" "." "_")))
-              (: "git.sr.ht"
-                 "/~" (+ (or alnum "-" "." "_"))
-                 "/" (+ (or alnum "-" "." "_")))
-              (: "git." (or "savannah" "sv") "." (? "non") "gnu.org/"
-                 (or "r" "git") "/"
-                 (+ (or alnum "-" "." "_")) (? "/")))
-          (or (? "/") ".git") eos)
-     . Git)
-    (,(rx bos "http" (? "s") "://"
-          (or (: "hg.sr.ht"
-                 "/~" (+ (or alnum "-" "." "_"))
-                 "/" (+ (or alnum "-" "." "_")))
-              (: "hg." (or "savannah" "sv") "." (? "non") "gnu.org/hgweb/"
-                 (+ (or alnum "-" "." "_")) (? "/")))
-          eos)
-     . Hg)
-    (,(rx bos "http" (? "s") "://"
-          (or (: "bzr." (or "savannah" "sv") "." (? "non") "gnu.org/r/"
-                 (+ (or alnum "-" "." "_")) (? "/")))
-          eos)
-     . Bzr))
-  "Alist mapping repository URLs to VC backends.
-`package-vc-install' consults this alist to determine the VC
-backend from the repository URL when you call it without
-specifying a backend.  Each element of the alist has the form
-\(URL-REGEXP . BACKEND).  `package-vc-install' will use BACKEND of
-the first association for which the URL of the repository matches
-the URL-REGEXP of the association.  If no match is found,
-`package-vc-install' uses `package-vc-default-backend' instead."
-  :type `(alist :key-type (regexp :tag "Regular expression matching URLs")
-                :value-type ,package-vc--backend-type)
-  :version "29.1")
+(define-obsolete-variable-alias
+  'package-vc-heuristic-alist
+  'vc-heuristic-alist "31.1")
 
 (defcustom package-vc-default-backend 'Git
   "Default VC backend to use for cloning package repositories.
@@ -127,7 +75,7 @@ package-vc-default-backend
 
 The value must be a member of `vc-handled-backends' that supports
 the `clone' VC function."
-  :type package-vc--backend-type
+  :type vc-backend-type
   :version "29.1")
 
 (defcustom package-vc-register-as-project t
@@ -626,13 +574,6 @@ package-vc--unpack-1
                  "")))
     t))
 
-(defun package-vc--guess-backend (url)
-  "Guess the VC backend for URL.
-This function will internally query `package-vc-heuristic-alist'
-and return nil if it cannot reasonably guess."
-  (and url (alist-get url package-vc-heuristic-alist
-                      nil nil #'string-match-p)))
-
 (declare-function project-remember-projects-under "project" (dir &optional recursive))
 
 (defun package-vc--clone (pkg-desc pkg-spec dir rev)
@@ -646,7 +587,7 @@ package-vc--clone
     (unless (file-exists-p dir)
       (make-directory (file-name-directory dir) t)
       (let ((backend (or (plist-get pkg-spec :vc-backend)
-                         (package-vc--guess-backend url)
+                         (vc-guess-url-backend url)
                          (plist-get (alist-get (package-desc-archive pkg-desc)
                                                package-vc--archive-data-alist
                                                nil nil #'string=)
@@ -753,7 +694,7 @@ package-vc--read-package-name
                            ;; pointing towards a repository, and use that as a backup
                            (and-let* ((extras (package-desc-extras (cadr pkg)))
                                       (url (alist-get :url extras))
-                                      ((package-vc--guess-backend url)))))))
+                                      ((vc-guess-url-backend url)))))))
                    (not allow-url)))
 
 (defun package-vc--read-package-desc (prompt &optional installed)
@@ -917,7 +858,7 @@ package-vc-install
      (cdr package)
      rev))
    ((and-let* (((stringp package))
-               (backend (or backend (package-vc--guess-backend package))))
+               (backend (or backend (vc-guess-url-backend package))))
       (package-vc--unpack
        (package-desc-create
         :name (or name (intern (file-name-base package)))
@@ -930,7 +871,7 @@ package-vc-install
        (or (package-vc--desc->spec (cadr desc))
            (and-let* ((extras (package-desc-extras (cadr desc)))
                       (url (alist-get :url extras))
-                      (backend (package-vc--guess-backend url)))
+                      (backend (vc-guess-url-backend url)))
              (list :vc-backend backend :url url))
            (user-error "Package `%s' has no VC data" package))
        rev)))
@@ -958,7 +899,7 @@ package-vc-checkout
   (let ((pkg-spec (or (package-vc--desc->spec pkg-desc)
                       (and-let* ((extras (package-desc-extras pkg-desc))
                                  (url (alist-get :url extras))
-                                 (backend (package-vc--guess-backend url)))
+                                 (backend (vc-guess-url-backend url)))
                         (list :vc-backend backend :url url))
                       (user-error "Package `%s' has no VC data"
                                   (package-desc-name pkg-desc)))))
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 6498b8522fd..8a8eb6fcfc7 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -944,6 +944,61 @@ vc-allow-rewriting-published-history
                  (const :tag "Allow without prompting" t))
   :version "31.1")
 
+(defconst vc-backend-type
+  `(choice :convert-widget
+     ,(lambda (widget)
+        (let (opts)
+          (dolist (be vc-handled-backends)
+            (when (or (vc-find-backend-function be 'clone)
+                      (alist-get 'clone (get be 'vc-functions)))
+              (push (widget-convert (list 'const be)) opts)))
+          (widget-put widget :args opts))
+        widget))
+  "The type of VC backends that support cloning VCS repositories.")
+
+(defcustom vc-heuristic-alist
+  `((,(rx bos "http" (? "s") "://"
+          (or (: (? "www.") "github.com"
+               "/" (+ (or alnum "-" "." "_"))
+               "/" (+ (or alnum "-" "." "_")))
+              (: "codeberg.org"
+               "/" (+ (or alnum "-" "." "_"))
+               "/" (+ (or alnum "-" "." "_")))
+              (: (? "www.") "gitlab" (+ "." (+ alnum))
+               "/" (+ (or alnum "-" "." "_"))
+               "/" (+ (or alnum "-" "." "_")))
+              (: "git.sr.ht"
+               "/~" (+ (or alnum "-" "." "_"))
+               "/" (+ (or alnum "-" "." "_")))
+              (: "git." (or "savannah" "sv") "." (? "non") "gnu.org/"
+               (or "r" "git") "/"
+               (+ (or alnum "-" "." "_")) (? "/")))
+          (or (? "/") ".git") eos)
+     . Git)
+    (,(rx bos "http" (? "s") "://"
+          (or (: "hg.sr.ht"
+               "/~" (+ (or alnum "-" "." "_"))
+               "/" (+ (or alnum "-" "." "_")))
+              (: "hg." (or "savannah" "sv") "." (? "non") "gnu.org/hgweb/"
+               (+ (or alnum "-" "." "_")) (? "/")))
+          eos)
+     . Hg)
+    (,(rx bos "http" (? "s") "://"
+          (or (: "bzr." (or "savannah" "sv") "." (? "non") "gnu.org/r/"
+               (+ (or alnum "-" "." "_")) (? "/")))
+          eos)
+     . Bzr))
+  "Alist mapping repository URLs to VC backends.
+`vc-clone' consults this alist to determine the VC
+backend from the repository URL when you call it without
+specifying a backend.  Each element of the alist has the form
+\(URL-REGEXP . BACKEND).  `vc-clone' will use BACKEND of
+the first association for which the URL of the repository matches
+the URL-REGEXP of the association."
+  :type `(alist :key-type (regexp :tag "Regular expression matching URLs")
+                :value-type ,vc-backend-type)
+  :version "31.1")
+
 \f
 ;; File property caching
 
@@ -1033,6 +1088,13 @@ vc-backend-for-registration
 	(vc-call-backend bk 'create-repo))
       (throw 'found bk))))
 
+(defun vc-guess-url-backend (url)
+  "Guess the VC backend for URL.
+This function will internally query `vc-heuristic-alist'
+and return nil if it cannot reasonably guess."
+  (and url (alist-get url vc-heuristic-alist
+                      nil nil #'string-match-p)))
+
 ;;;###autoload
 (defun vc-responsible-backend (file &optional no-error)
   "Return the name of a backend system that is responsible for FILE.
-- 
2.46.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: [PATCH] Make vc-clone interactive --]
[-- Type: text/x-patch, Size: 4787 bytes --]

From a64134c73086f5067d1f360dad0ab48267750fd1 Mon Sep 17 00:00:00 2001
Message-ID: <a64134c73086f5067d1f360dad0ab48267750fd1.1729772961.git.avityazev@disroot.org>
From: Aleksandr Vityazev <avityazev@disroot.org>
Date: Thu, 24 Oct 2024 15:19:34 +0300
Subject: [PATCH] Make vc-clone interactive

* lisp/vc/vc.el (vc-clone): Make interactive.  Add optional
argument OPEN-DIR. Mention these changes in the doc string.
(vc--remotes-history): New defvar.
* etc/NEWS: Announce these changes.
---
 etc/NEWS      | 10 ++++++++++
 lisp/vc/vc.el | 54 +++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 49 insertions(+), 15 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 64e4f22b9d3..12053fffc57 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -614,6 +614,16 @@ even though Emacs thinks it is dangerous.
 
 So far, this applies only to using 'e' from Log View mode for Git.
 
+*** 'vc-clone' is now an interactive command.
+When called interactively, 'vc-clone' now prompts for the remote
+repository address, the backend for cloning, if it has not been
+determined automatically according to the URL, and the directory to
+clone the repository into.
+
+*** 'vc-clone' now accepts an optional argument OPEN-DIR.
+When the argument is non-nil, the function switches to a buffer visiting
+directory to which the repository was cloned.
+
 \f
 * New Modes and Packages in Emacs 31.1
 
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 8a8eb6fcfc7..975f7f40b15 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -3876,7 +3876,9 @@ vc-check-headers
   (interactive)
   (vc-call-backend (vc-backend buffer-file-name) 'check-headers))
 
-(defun vc-clone (remote &optional backend directory rev)
+(defvar vc--remotes-history)
+
+(defun vc-clone (remote &optional backend directory rev open-dir)
   "Clone repository REMOTE using version-control BACKEND, into DIRECTORY.
 If successful, return the string with the directory of the checkout;
 otherwise return nil.
@@ -3886,20 +3888,42 @@ 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 OPEN-DIR is non-nil, switches to a buffer visiting DIRECTORY to
+which the repository was cloned.  It would be useful in scripts, but not
+in regular code.
+If called interactively, prompt for REMOTE, DIRECTORY and BACKEND,
+if BACKEND has not been automatically determined according to the REMOTE
+URL, in the minibuffer."
+  (interactive
+   (let* ((url (read-string "Remote: " nil 'vc--remotes-history))
+          (backend (or (vc-guess-url-backend url)
+                       (intern (completing-read
+                                "Backend: " vc-handled-backends nil t)))))
+     (list url backend
+           (read-directory-name
+            "Clone into new or empty directory: " nil nil
+            (lambda (dir) (or (not (file-exists-p dir))
+                              (directory-empty-p dir))))
+           nil t)))
+  (let* ((directory (expand-file-name (or directory default-directory)))
+         (backend (or backend (vc-guess-url-backend remote)))
+         (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))))))))
+    (when (file-directory-p directory)
+      (when open-dir
+        (find-file directory))
+      directory)))
 
 (declare-function log-view-current-tag "log-view" (&optional pos))
 (defun vc-default-last-change (_backend file line)
-- 
2.46.0


[-- Attachment #4: Type: text/plain, Size: 38 bytes --]


-- 
Best regards,
Aleksandr Vityazev