bug#65386: [PATCH] ; Refine some 'package-vc' docstrings

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

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

Philip Kaludercic <philipk@posteo.net> writes:

>>>>  (defcustom package-vc-default-backend 'Git
>>>> -  "Default VC backend used when cloning a package repository.
>>>> -If no repository type was specified or could be guessed by
>>>> -`package-vc-heuristic-alist', this is the default VC backend
>>>> -used as fallback.  The value must be a member of
>>>> -`vc-handled-backends' and the named backend must implement
>>>> -the `clone' function."
>>>> -  :type `(choice ,@(mapcar (lambda (b) (list 'const b))
>>>> -                           vc-handled-backends))
>>>> +  "Default VC backend to use for cloning package repositories.
>>>> +`package-vc-install' uses this backend when you specify neither
>>>> +the backend nor a repository URL that's recognized via
>>>> +`package-vc-heuristic-alist'.
>>>> +
>>>> +The value must be a member of `vc-handled-backends' that supports
>>>> +the `clone' VC function."
>>>> +  :type `(choice ,@(seq-keep
>>>> +                    (lambda (be)
>>>> +                      (when (or (vc-find-backend-function be 'clone)
>>>> +                                (alist-get 'clone (get be 'vc-functions)))
>>>> +                        (list 'const be)))
>>>> +                    vc-handled-backends))
>>>
>>> This is good, but shouldn't we do something like this in a separate
>>> patch?  No hard opinions on that though.
>>
>> I thought it was a small enough change to include along with the doc
>> improvements, but if you prefer I can update the `:type` in a separate
>> patch.  BTW, this should probably be applied also to
>> `package-vc-heuristic-alist` either way.
>
> In that case I think it would be better to prepare a separate patch.

Alright, I've extracted the `:type` refinement to a separate patch.  See
below.

> Also, would it make sense to determine this at compile-time?  On the
> other hand, if a VC backend is installed later on from ELPA, we would
> want the custom type to reflect this.

Yes, I couldn't find a way to defer computing the set of candidates to
"customization type", I'm not sure if that even makes total sense.  I
think it's not that crucial since someone adding a VC backend and
immediately trying to customize these options seems to me like a very
minor edge case, and we had the same issue prior to my patch anyhow.

> The patch looks fine to me, but considering my history with
> documentation I think it is best to let a few others take a look at your
> revision as well.

No problem, and thanks.

> Other than that, the plan is to pull out the
> `package-vc-default-backend' custom type and rename
> `package-vc-allow-side-effects', right?

Yes, I'm attaching three patches:

1. an updated (v5) doc-only patch,
2. a follow-up for refining the `defcustom` types as discussed above, and
3. a patch renaming `package-vc-allow-side-effects` to `package-vc-allow-build-commands`.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v5-0001-Refine-some-package-vc-docstrings.patch --]
[-- Type: text/x-patch, Size: 13114 bytes --]

From c9ec26056b9d49b96706843fdcb845566c875b25 Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Fri, 18 Aug 2023 21:59:10 +0200
Subject: [PATCH v5] ; Refine some 'package-vc' docstrings

* lisp/emacs-lisp/package-vc.el (package-vc-heuristic-alist)
(package-vc-install-dependencies, package-vc-upgrade)
(package-vc-install, package-vc-install-from-checkout)
(package-vc-prepare-patch, package-vc-upgrade-all)
(package-vc-allow-side-effects)
(package-vc-default-backend): Refine docstrings.
---
 lisp/emacs-lisp/package-vc.el | 167 +++++++++++++++++++++-------------
 1 file changed, 102 insertions(+), 65 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index db8b41aee6a..a3762d252b0 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -94,7 +94,14 @@ package-vc-heuristic-alist
                  (+ (or alnum "-" "." "_")) (? "/")))
           eos)
      . Bzr))
-  "Heuristic mapping URL regular expressions to VC backends."
+  "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 (choice :tag "VC Backend"
                                     ,@(mapcar (lambda (b) `(const ,b))
@@ -102,12 +109,13 @@ package-vc-heuristic-alist
   :version "29.1")
 
 (defcustom package-vc-default-backend 'Git
-  "Default VC backend used when cloning a package repository.
-If no repository type was specified or could be guessed by
-`package-vc-heuristic-alist', this is the default VC backend
-used as fallback.  The value must be a member of
-`vc-handled-backends' and the named backend must implement
-the `clone' function."
+  "Default VC backend to use for cloning package repositories.
+`package-vc-install' uses this backend when you specify neither
+the backend nor a repository URL that's recognized via
+`package-vc-heuristic-alist'.
+
+The value must be a member of `vc-handled-backends' that supports
+the `clone' VC function."
   :type `(choice ,@(mapcar (lambda (b) (list 'const b))
                            vc-handled-backends))
   :version "29.1")
@@ -140,20 +148,21 @@ package-vc-install-selected-packages
                (package-desc-create :name name :kind 'vc))
            spec)))))))
 
-(defcustom package-vc-selected-packages '()
-  "List of packages that must be installed.
-Each member of the list is of the form (NAME . SPEC), where NAME
-is a symbol designating the package and SPEC is one of:
+
+(defcustom package-vc-selected-packages nil
+  "List of packages to install from their VCS repositories.
+Each element is of the form (NAME . SPEC), where NAME is a symbol
+designating the package and SPEC is one of:
 
 - nil, if any package version can be installed;
 - a version string, if that specific revision is to be installed;
-- a property list, describing a package specification.  For more
-  details, please consult the subsection \"Specifying Package
-  Sources\" in the Info node `(emacs)Fetching Package Sources'.
+- a property list, describing a package specification.  For possible
+  values, see the subsection \"Specifying Package Sources\" in the
+  Info node `(emacs)Fetching Package Sources'.
 
-This user option will be automatically updated to store package
-specifications for packages that are not specified in any
-archive."
+The command `package-vc-install' updates the value of this user
+option to store package specifications for packages that are not
+specified in any archive."
   :type '(alist :tag "List of packages you want to be installed"
                 :key-type (symbol :tag "Package")
                 :value-type
@@ -345,19 +354,26 @@ package-vc--generate-description-file
        nil pkg-file nil 'silent))))
 
 (defcustom package-vc-allow-side-effects nil
-  "Whether to process :make and :shell-command spec arguments.
+  "Whether to run extra build commands when installing VC packages.
+
+Some packages specify \"make\" targets or other shell commands
+that should run prior to building the package, by including the
+:make or :shell-command keywords in their specification.  By
+default, Emacs ignores these keywords when installing and
+upgrading VC packages, but if the value is a list of package
+names (symbols), the build commands will be run for those
+packages.  If the value is t, always respect :make and
+:shell-command keywords.
 
 It may be necessary to run :make and :shell-command arguments in
 order to initialize a package or build its documentation, but
 please be careful when changing this option, as installing and
 updating a package can run potentially harmful code.
 
-When set to a list of symbols (packages), run commands for only
-packages in the list.  When nil, never run commands.  Otherwise
-when non-nil, run commands for any package with :make or
-:shell-command specified.
-
-Package specs are loaded from trusted package archives."
+This applies to package specifications that come from your
+configured package archives, as well as from entries in
+`package-vc-selected-packages' and specifications that you give
+to `package-vc-install' directly."
   :type '(choice (const :tag "Run for all packages" t)
                  (repeat :tag "Run only for selected packages" (symbol :tag "Package name"))
                  (const :tag "Never run" nil))
@@ -414,15 +430,15 @@ package-vc--build-documentation
     (when clean-up
       (delete-file file))))
 
-(defun package-vc-install-dependencies (requirements)
-  "Install missing dependencies, and return missing ones.
-The return value will be nil if everything was found, or a list
-of (NAME VERSION) pairs of all packages that couldn't be found.
+(defun package-vc-install-dependencies (deps)
+  "Install missing dependencies according to DEPS.
+
+DEPS is a list of elements (PACKAGE VERSION-LIST), where
+PACKAGE is a package name and VERSION-LIST is the required
+version of that package.
 
-REQUIREMENTS should be a list of additional requirements; each
-element in this list should have the form (PACKAGE VERSION-LIST),
-where PACKAGE is a package name and VERSION-LIST is the required
-version of that package."
+Return a list of dependencies that couldn't be met (or nil, when
+this function successfully installs all given dependencies)."
   (let ((to-install '()) (missing '()))
     (cl-labels ((search (pkg)
                   "Attempt to find all dependencies for PKG."
@@ -458,7 +474,7 @@ package-vc-install-dependencies
                     (or (not desc-a) (not desc-b)
                         (not (depends-on-p desc-b desc-a))
                         (depends-on-p desc-a desc-b)))))
-      (mapc #'search requirements)
+      (mapc #'search deps)
       (cl-callf sort to-install #'version-order)
       (cl-callf seq-uniq to-install #'duplicate-p)
       (cl-callf sort to-install #'dependent-order))
@@ -719,7 +735,10 @@ package-vc--read-package-desc
 
 ;;;###autoload
 (defun package-vc-upgrade-all ()
-  "Attempt to upgrade all installed VC packages."
+  "Upgrade all installed VC packages.
+
+This may fail if the local VCS state of one of the packages
+conflicts with its remote repository state."
   (interactive)
   (dolist (package package-alist)
     (dolist (pkg-desc (cdr package))
@@ -729,7 +748,10 @@ package-vc-upgrade-all
 
 ;;;###autoload
 (defun package-vc-upgrade (pkg-desc)
-  "Attempt to upgrade the package PKG-DESC."
+  "Upgrade the package described by PKG-DESC from package's VC repository.
+
+This may fail if the local VCS state of the package conflicts
+with the remote repository state."
   (interactive (list (package-vc--read-package-desc "Upgrade VC package: " t)))
   ;; HACK: To run `package-vc--unpack-1' after checking out the new
   ;; revision, we insert a hook into `vc-post-command-functions', and
@@ -792,34 +814,45 @@ package-vc--release-rev
 
 ;;;###autoload
 (defun package-vc-install (package &optional rev backend name)
-  "Fetch a PACKAGE and set it up for using with Emacs.
-
-If PACKAGE is a string containing an URL, download the package
-from the repository at that URL; the function will try to guess
-the name of the package from the URL.  This can be overridden by
-passing the optional argument NAME.  If PACKAGE is a cons-cell,
-it should have the form (NAME . SPEC), where NAME is a symbol
-indicating the package name and SPEC is a plist as described in
-`package-vc-selected-packages'.  Otherwise PACKAGE should be a
-symbol whose name is the package name, and the URL for the
-package will be taken from the package's metadata.
+  "Fetch a package described by PACKAGE and set it up for use with Emacs.
+
+PACKAGE specifies which package to install, where to find its
+source repository and how to build it.
+
+If PACKAGE is a symbol, install the package with that name
+according to metadata that package archives provide for it.  This
+is the simplest way to call this function, but it only works if
+the package you want to install is listed in a package archive
+you have configured.
+
+If PACKAGE is a string, it specifies the URL of the package
+repository.  In this case, optional argument BACKEND specifies
+the VC backend to use for cloning the repository; if it's nil,
+this function tries to infer which backend to use according to
+the value of `package-vc-heuristic-alist' and if that fails it
+uses `package-vc-default-backend'.  Optional argument NAME
+specifies the package name in this case; if it's nil, this
+package uses `file-name-base' on the URL to obtain the package
+name, otherwise NAME is the package name as a symbol.
+
+PACKAGE can also be a cons cell (PNAME . SPEC) where PNAME is the
+package name as a symbol, and SPEC is a plist that specifes how
+to fetch and build the package.  For possible values, see the
+subsection \"Specifying Package Sources\" in the Info
+node `(emacs)Fetching Package Sources'.
 
 By default, this function installs the last revision of the
 package available from its repository.  If REV is a string, it
-describes the revision to install, as interpreted by the VC
-backend.  The special value `:last-release' (interactively, the
-prefix argument), will use the commit of the latest release, if
-it exists.  The last release is the latest revision which changed
-the \"Version:\" header of the package's main Lisp file.
-
-Optional argument BACKEND specifies the VC backend to use for cloning
-the package's repository; this is only possible if NAME-OR-URL is a URL,
-a string.  If BACKEND is omitted or nil, the function
-uses `package-vc-heuristic-alist' to guess the backend.
-Note that by default, a VC package will be prioritized over a
-regular package, but it will not remove a VC package.
-
-\(fn PACKAGE &optional REV BACKEND)"
+describes the revision to install, as interpreted by the relevant
+VC backend.  The special value `:last-release' (interactively,
+the prefix argument), says to use the commit of the latest
+release, if it exists.  The last release is the latest revision
+which changed the \"Version:\" header of the package's main Lisp
+file.
+
+If you use this function to install a package that you also have
+installed from a package archive, the version this function
+installs takes precedence."
   (interactive
    (progn
      ;; Initialize the package system to get the list of package
@@ -895,7 +928,7 @@ package-vc-checkout
 
 ;;;###autoload
 (defun package-vc-install-from-checkout (dir name)
-  "Set up the package NAME in DIR by linking it into the ELPA directory.
+  "Install the package NAME from its source directory DIR.
 Interactively, prompt the user for DIR, which should be a directory
 under version control, typically one created by `package-vc-checkout'.
 If invoked interactively with a prefix argument, prompt the user
@@ -937,13 +970,17 @@ package-vc-rebuild
 
 ;;;###autoload
 (defun package-vc-prepare-patch (pkg-desc subject revisions)
-  "Send patch for REVISIONS to maintainer of the package PKG using SUBJECT.
-The function uses `vc-prepare-patch', passing SUBJECT and
-REVISIONS directly.  PKG-DESC must be a package description.
+  "Email patches for REVISIONS to maintainer of package PKG-DESC using SUBJECT.
+
+PKG-DESC is a package descriptor and SUBJECT is the subject of
+the message.
+
 Interactively, prompt for PKG-DESC, SUBJECT, and REVISIONS.  When
 invoked with a numerical prefix argument, use the last N
 revisions.  When invoked interactively in a Log View buffer with
-marked revisions, use those."
+marked revisions, use those.
+
+See also `vc-prepare-patch'."
   (interactive
    (list (package-vc--read-package-desc "Package to prepare a patch for: " t)
          (and (not vc-prepare-patches-separately)
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Refine-defcustom-types-in-package-vc.patch --]
[-- Type: text/x-patch, Size: 2309 bytes --]

From 8a339d3bffe81f80e7e968505c40855d6598b8ac Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Sun, 20 Aug 2023 16:20:54 +0200
Subject: [PATCH] ; Refine 'defcustom' types in 'package-vc'

Only include VC backends that support cloning in the ':type' of
'package-vc-heuristic-alist' and 'package-vc-default-backend'.

* lisp/emacs-lisp/package-vc.el (package-vc--cloning-backend-p)
(package-vc--backends): New functions.
(package-vc-heuristic-alist, package-vc-default-backend): Use it.
---
 lisp/emacs-lisp/package-vc.el | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index a3762d252b0..44a4624e49f 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -62,6 +62,15 @@ package-vc
 (defconst package-vc--elpa-packages-version 1
   "Version number of the package specification format understood by package-vc.")
 
+(defun package-vc--cloning-backend-p (be)
+  "Return non-nil if the VC backend BE supports cloning repositories."
+  (or (vc-find-backend-function be 'clone)
+      (alist-get 'clone (get be 'vc-functions))))
+
+(defun package-vc--backends ()
+  "Return a list of VC backends suitable for cloning package VCS repositories."
+  (seq-filter #'package-vc--cloning-backend-p vc-handled-backends))
+
 (defcustom package-vc-heuristic-alist
   `((,(rx bos "http" (? "s") "://"
           (or (: (? "www.") "github.com"
@@ -105,7 +114,7 @@ package-vc-heuristic-alist
   :type `(alist :key-type (regexp :tag "Regular expression matching URLs")
                 :value-type (choice :tag "VC Backend"
                                     ,@(mapcar (lambda (b) `(const ,b))
-                                              vc-handled-backends)))
+                                              (package-vc--backends))))
   :version "29.1")
 
 (defcustom package-vc-default-backend 'Git
@@ -117,7 +126,7 @@ package-vc-default-backend
 The value must be a member of `vc-handled-backends' that supports
 the `clone' VC function."
   :type `(choice ,@(mapcar (lambda (b) (list 'const b))
-                           vc-handled-backends))
+                           (package-vc--backends)))
   :version "29.1")
 
 (defcustom package-vc-register-as-project t
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0001-Rename-package-vc-allow-side-effects-to-better-fit-i.patch --]
[-- Type: text/x-patch, Size: 3229 bytes --]

From c13cc761439dcaffe8934fc6806ed02c51a7695e Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Sun, 20 Aug 2023 16:29:33 +0200
Subject: [PATCH] Rename 'package-vc-allow-side-effects' to better fit its use

* lisp/emacs-lisp/package-vc.el (package-vc-allow-side-effects):
Rename to 'package-vc-allow-build-commands'.
(package-vc--unpack-1):
* doc/emacs/package.texi (Fetching Package Sources):
* etc/NEWS: Adapt accordingly.
---
 doc/emacs/package.texi        | 8 ++++----
 etc/NEWS                      | 6 +++---
 lisp/emacs-lisp/package-vc.el | 6 +++---
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/doc/emacs/package.texi b/doc/emacs/package.texi
index b294e3d58bd..96ebd35f547 100644
--- a/doc/emacs/package.texi
+++ b/doc/emacs/package.texi
@@ -690,13 +690,13 @@ Fetching Package Sources
 @item :make
 A string or list of strings providing the target or targets defined in
 the repository Makefile which should run before building the Info
-file.  Only takes effect when @code{package-vc-allow-side-effects} is
-non-nil.
+file.  Only takes effect when @code{package-vc-allow-build-commands}
+is non-nil.
 
 @item :shell-command
 A string providing the shell command to run before building the Info
-file.  Only takes effect when @code{package-vc-allow-side-effects} is
-non-@code{nil}.
+file.  Only takes effect when @code{package-vc-allow-build-commands}
+is non-@code{nil}.
 
 @item :vc-backend
 A symbol naming the VC backend to use for downloading a copy of the
diff --git a/etc/NEWS b/etc/NEWS
index 6588299c532..da1406f6831 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -510,9 +510,9 @@ project, that you can quickly select using 'project-switch-project'
 ('C-x p p').
 
 ---
-*** New user option 'package-vc-allow-side-effects'.
-When non-nil, package specifications with side-effects for building
-software will be used when building a package.
+*** New user option 'package-vc-allow-build-commands'.
+Controls for which packages Emacs runs extra build commands when
+installing directly from the package VCS repository.
 
 ---
 *** New command to start an inferior Emacs loading only specific packages.
diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 44a4624e49f..592e989f0f8 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -362,7 +362,7 @@ package-vc--generate-description-file
         "\n")
        nil pkg-file nil 'silent))))
 
-(defcustom package-vc-allow-side-effects nil
+(defcustom package-vc-allow-build-commands nil
   "Whether to run extra build commands when installing VC packages.
 
 Some packages specify \"make\" targets or other shell commands
@@ -549,9 +549,9 @@ package-vc--unpack-1
       (package-vc--generate-description-file pkg-desc pkg-file)
 
       ;; Process :make and :shell-command arguments before building documentation
-      (when (or (eq package-vc-allow-side-effects t)
+      (when (or (eq package-vc-allow-build-commands t)
                 (memq (package-desc-name pkg-desc)
-                      package-vc-allow-side-effects))
+                      package-vc-allow-build-commands))
         (package-vc--make pkg-spec pkg-desc))
 
       ;; Detect a manual
-- 
2.41.0