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

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Tags: patch

Hi,

This patch includes improvement suggestions for the docstrings of some
of the functions and user options in package-vc.el.



In GNU Emacs 30.0.50 (build 21, x86_64-apple-darwin22.5.0, NS
 appkit-2299.60 Version 13.4 (Build 22F66)) of 2023-08-19 built on
 dazzs-mbp.home
Repository revision: a85f31c4a4d37f5d39a99e79cc32be172d49603f
Repository branch: master
Windowing system distributor 'Apple', version 10.3.2299
System Description:  macOS 13.4

Configured using:
 'configure 'CFLAGS=-g0 -O3' --with-native-compilation --with-json
 --with-imagemagick --with-tree-sitter --enable-link-time-optimization'


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

From bc3cacdeb9f30c5f54aa92975835a3cfb8ff526d Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Fri, 18 Aug 2023 21:59:10 +0200
Subject: [PATCH] ; 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): Refine docstrings.
(package-vc-default-backend): Refine docstring and custom type.
---
 lisp/emacs-lisp/package-vc.el | 149 ++++++++++++++++++++--------------
 1 file changed, 88 insertions(+), 61 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index db8b41aee6a..29d09958e83 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -94,7 +94,10 @@ package-vc-heuristic-alist
                  (+ (or alnum "-" "." "_")) (? "/")))
           eos)
      . Bzr))
-  "Heuristic mapping URL regular expressions to VC backends."
+  "Alist mapping regular expressions to VC backends.
+`package-vc-install' tries to match each regular expression in
+this alist to the repository URL you give it, and uses the VC
+backend corresponding to the first matching regular expression."
   :type `(alist :key-type (regexp :tag "Regular expression matching URLs")
                 :value-type (choice :tag "VC Backend"
                                     ,@(mapcar (lambda (b) `(const ,b))
@@ -102,14 +105,19 @@ 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."
-  :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 don't specify a
+backend and give it a URL that doesn't match any regular
+expression in `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))
   :version "29.1")
 
 (defcustom package-vc-register-as-project t
@@ -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 source.
+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.  When
+this option is nil, Emacs ignores these keywords when installing
+and upgrading VC packages.  If it's a list of package
+names (symbols), Emacs only runs these extra build commands for
+those packages.  A non-nil non-list value says to 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))
@@ -415,14 +431,14 @@ package-vc--build-documentation
       (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.
-
-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."
+  "Install missing dependencies according to REQUIREMENTS.
+
+REQUIREMENTS is a list of elements (PACKAGE VERSION-LIST), where
+PACKAGE is a package name and VERSION-LIST is the required
+version of that package.
+
+Return a list of requirements that couldn't be met (or nil, when
+this function successfully installs all given requirements)."
   (let ((to-install '()) (missing '()))
     (cl-labels ((search (pkg)
                   "Attempt to find all dependencies for PKG."
@@ -719,7 +735,7 @@ package-vc--read-package-desc
 
 ;;;###autoload
 (defun package-vc-upgrade-all ()
-  "Attempt to upgrade all installed VC packages."
+  "Upgrade all installed VC packages."
   (interactive)
   (dolist (package package-alist)
     (dolist (pkg-desc (cdr package))
@@ -729,7 +745,7 @@ package-vc-upgrade-all
 
 ;;;###autoload
 (defun package-vc-upgrade (pkg-desc)
-  "Attempt to upgrade the package PKG-DESC."
+  "Upgrade the VC package that PKG-DESC describes."
   (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
@@ -794,32 +810,43 @@ package-vc--release-rev
 (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.
+PACKAGE specifies which package to install, where to find its
+source repository and how to build it.
 
 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 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'.
+
+If you use this function to install a package that you also have
+installed from a package archive, the version this function
+installs is takes precedence."
   (interactive
    (progn
      ;; Initialize the package system to get the list of package
@@ -895,7 +922,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,7 +964,7 @@ 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.
+  "Send patch for REVISIONS to maintainer of the package PKG-DESC using SUBJECT.
 The function uses `vc-prepare-patch', passing SUBJECT and
 REVISIONS directly.  PKG-DESC must be a package description.
 Interactively, prompt for PKG-DESC, SUBJECT, and REVISIONS.  When
-- 
2.41.0

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

[Eli Zaretskii]

> Cc: Philip Kaludercic <philipk@posteo.net>
> Date: Sat, 19 Aug 2023 20:07:07 +0200
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> This patch includes improvement suggestions for the docstrings of some
> of the functions and user options in package-vc.el.

Thanks.  Allow me a few comments:

> @@ -94,7 +94,10 @@ package-vc-heuristic-alist
>                   (+ (or alnum "-" "." "_")) (? "/")))
>            eos)
>       . Bzr))
> -  "Heuristic mapping URL regular expressions to VC backends."
> +  "Alist mapping regular expressions to VC backends.

The first line loses information by omitting the "URL" part, which I
think is important, more important than the "regular expressions"
part.  How about

  Alist mapping repository URLs to VC backends.

> +`package-vc-install' tries to match each regular expression in
> +this alist to the repository URL you give it, and uses the VC
> +backend corresponding to the first matching regular expression."

This tends to describe the implementation, not the purpose.  I would
rephrase

  `package-vc-install' consults this alist to determine the VC backend
  from the repository URL.  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.

>  (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 don't specify a
> +backend and give it a URL that doesn't match any regular
> +expression in `package-vc-heuristic-alist'.

I guess you mean

  `package-vc-install' uses this backend when you specify neither the
  backend nor a repository URL to map 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))
>    :version "29.1")
>  
>  (defcustom package-vc-register-as-project t
> @@ -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 source.

What is the significance of "from source" here?

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

I question the wisdom of having commands modify values of user
options.  I think it is better to have a separate variable whose
initial value is taken from the user option, and then have the
commands add to that separate variable, leaving the user option
intact.

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

The name of this variable is very far from what it means.  Running
commands and "side effects" are barely related to each other.  Please
consider renaming the variable.  How about
package-vc-allow-build-commands?

> +                                                          When
> +this option is nil, Emacs ignores these keywords when installing
> +and upgrading VC packages.  If it's a list of package
> +names (symbols), Emacs only runs these extra build commands for
> +those packages.

"By default, Emacs ignores these keywords [...], but if the value is a
list of package names (symbols), the build commands will be run for
those packages."

>  (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.
> -
> -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."
> +  "Install missing dependencies according to REQUIREMENTS.
> +
> +REQUIREMENTS is a list of elements (PACKAGE VERSION-LIST), where
> +PACKAGE is a package name and VERSION-LIST is the required
> +version of that package.

This seems to indicate that REQUIREMENTS should be renamed to
DEPENDENCIES.

> +Return a list of requirements that couldn't be met (or nil, when
> +this function successfully installs all given requirements)."
                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"Installs requirements"? that's why "dependencies" would be better.

>  (defun package-vc-upgrade (pkg-desc)
> -  "Attempt to upgrade the package PKG-DESC."
> +  "Upgrade the VC package that PKG-DESC describes."

"Upgrade the package described by PKG-DESC from the package's VC repository."

>  (defun package-vc-install (package &optional rev backend name)
>    "Fetch a PACKAGE and set it up for using with Emacs.

"Fetch a package described by PACKAGE and set it up for use with Emacs."

(The point being that PACKAGE is not just a name, it's a description
that can have several forms.)

> -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.
> +PACKAGE specifies which package to install, where to find its
> +source repository and how to build it.
>  
>  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 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'.

I think the doc string should describe PACKAGE before REV.

> +If you use this function to install a package that you also have
> +installed from a package archive, the version this function
> +installs is takes precedence."

"is takes"? something's wrong here.

>  (defun package-vc-prepare-patch (pkg-desc subject revisions)
> -  "Send patch for REVISIONS to maintainer of the package PKG using SUBJECT.
> +  "Send patch for REVISIONS to maintainer of the package PKG-DESC using SUBJECT.

This should mention email, otherwise SUBJECT is confusing.  Maybe use
"Email" instead of "Send".

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Philip Kaludercic <philipk@posteo.net>
>> Date: Sat, 19 Aug 2023 20:07:07 +0200
>> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> This patch includes improvement suggestions for the docstrings of some
>> of the functions and user options in package-vc.el.
>
> Thanks.  Allow me a few comments:
>

Thank you, I've attached an updated patch below.

>> @@ -94,7 +94,10 @@ package-vc-heuristic-alist
>>                   (+ (or alnum "-" "." "_")) (? "/")))
>>            eos)
>>       . Bzr))
>> -  "Heuristic mapping URL regular expressions to VC backends."
>> +  "Alist mapping regular expressions to VC backends.
>
> The first line loses information by omitting the "URL" part, which I
> think is important, more important than the "regular expressions"
> part.  How about
>
>   Alist mapping repository URLs to VC backends.
>

Sure, done.

>> +`package-vc-install' tries to match each regular expression in
>> +this alist to the repository URL you give it, and uses the VC
>> +backend corresponding to the first matching regular expression."
>
> This tends to describe the implementation, not the purpose.  I would
> rephrase
>
>   `package-vc-install' consults this alist to determine the VC backend
>   from the repository URL.  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.
>

Done.

>>  (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 don't specify a
>> +backend and give it a URL that doesn't match any regular
>> +expression in `package-vc-heuristic-alist'.
>
> I guess you mean
>
>   `package-vc-install' uses this backend when you specify neither the
>   backend nor a repository URL to map via `package-vc-heuristic-alist'.
>

I went with a variation of your phrasing that I think is a little more
precise.  LMK what you think.

>> +
>> +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))
>>    :version "29.1")
>>  
>>  (defcustom package-vc-register-as-project t
>> @@ -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 source.
>
> What is the significance of "from source" here?
>

It's just for indicating that we're talking about VC packages
specifically.  Does that make sense?

>> +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."
>
> I question the wisdom of having commands modify values of user
> options.  I think it is better to have a separate variable whose
> initial value is taken from the user option, and then have the
> commands add to that separate variable, leaving the user option
> intact.
>

Yes, this patch only updates the documentation, but I agree that this
behavior may deserve another look as well.

>>  (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.
>
> The name of this variable is very far from what it means.  Running
> commands and "side effects" are barely related to each other.  Please
> consider renaming the variable.  How about
> package-vc-allow-build-commands?
>

I agree.  Philip, any objections to renaming this variable?  I don't
know if people already use it.

>> +                                                          When
>> +this option is nil, Emacs ignores these keywords when installing
>> +and upgrading VC packages.  If it's a list of package
>> +names (symbols), Emacs only runs these extra build commands for
>> +those packages.
>
> "By default, Emacs ignores these keywords [...], but if the value is a
> list of package names (symbols), the build commands will be run for
> those packages."
>

Done.

>>  (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.
>> -
>> -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."
>> +  "Install missing dependencies according to REQUIREMENTS.
>> +
>> +REQUIREMENTS is a list of elements (PACKAGE VERSION-LIST), where
>> +PACKAGE is a package name and VERSION-LIST is the required
>> +version of that package.
>
> This seems to indicate that REQUIREMENTS should be renamed to
> DEPENDENCIES.
>

Alright, I went with DEPS for brevity.

>> +Return a list of requirements that couldn't be met (or nil, when
>> +this function successfully installs all given requirements)."
>                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> "Installs requirements"? that's why "dependencies" would be better.
>

Updated.

>>  (defun package-vc-upgrade (pkg-desc)
>> -  "Attempt to upgrade the package PKG-DESC."
>> +  "Upgrade the VC package that PKG-DESC describes."
>
> "Upgrade the package described by PKG-DESC from the package's VC repository."
>

Done.

>>  (defun package-vc-install (package &optional rev backend name)
>>    "Fetch a PACKAGE and set it up for using with Emacs.
>
> "Fetch a package described by PACKAGE and set it up for use with Emacs."
>
> (The point being that PACKAGE is not just a name, it's a description
> that can have several forms.)
>

Updated.

>> -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.
>> +PACKAGE specifies which package to install, where to find its
>> +source repository and how to build it.
>>  
>>  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 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'.
>
> I think the doc string should describe PACKAGE before REV.
>

Sure, done.

>> +If you use this function to install a package that you also have
>> +installed from a package archive, the version this function
>> +installs is takes precedence."
>
> "is takes"? something's wrong here.
>

Good catch, thanks.  I've removed the "is" in the updated patch.

>>  (defun package-vc-prepare-patch (pkg-desc subject revisions)
>> -  "Send patch for REVISIONS to maintainer of the package PKG using SUBJECT.
>> +  "Send patch for REVISIONS to maintainer of the package PKG-DESC using SUBJECT.
>
> This should mention email, otherwise SUBJECT is confusing.  Maybe use
> "Email" instead of "Send".

Alright, I made a couple more changes to this one.



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

From 213675085fd77246fcbf6ad0cc5134081e4a66d4 Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Fri, 18 Aug 2023 21:59:10 +0200
Subject: [PATCH v2] ; 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): Refine docstrings.
(package-vc-default-backend): Refine docstring and custom type.
---
 lisp/emacs-lisp/package-vc.el | 167 ++++++++++++++++++++--------------
 1 file changed, 100 insertions(+), 67 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index db8b41aee6a..bf0e024ec19 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -94,7 +94,12 @@ 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.  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."
   :type `(alist :key-type (regexp :tag "Regular expression matching URLs")
                 :value-type (choice :tag "VC Backend"
                                     ,@(mapcar (lambda (b) `(const ,b))
@@ -102,14 +107,19 @@ 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."
-  :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))
   :version "29.1")
 
 (defcustom package-vc-register-as-project t
@@ -140,20 +150,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 source.
+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 +356,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.  A non-nil non-list value says to 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 +432,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 +476,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 +737,7 @@ package-vc--read-package-desc
 
 ;;;###autoload
 (defun package-vc-upgrade-all ()
-  "Attempt to upgrade all installed VC packages."
+  "Upgrade all installed VC packages."
   (interactive)
   (dolist (package package-alist)
     (dolist (pkg-desc (cdr package))
@@ -729,7 +747,7 @@ 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."
   (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 +810,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 +924,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 +966,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 the maintainer of the package PKG-DESC.
+
+PKG-DESC is a package description 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

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

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: 65386@debbugs.gnu.org,  philipk@posteo.net
> Date: Sat, 19 Aug 2023 22:42:57 +0200
> 
> >> -(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 source.
> >
> > What is the significance of "from source" here?
> >
> 
> It's just for indicating that we're talking about VC packages
> specifically.  Does that make sense?

Then how about

  List of packages to install from their VCS repositories.

> >> +  "Send patch for REVISIONS to maintainer of the package PKG-DESC using SUBJECT.
> >
> > This should mention email, otherwise SUBJECT is confusing.  Maybe use
> > "Email" instead of "Send".
> 
> Alright, I made a couple more changes to this one.

Now you have lost the reference to SUBJECT.  Was that necessary?  We
generally try to mention all the mandatory arguments in the first line
of the doc string.  The sentence below seems to be squeezable into 79
columns:

  Email patches for REVISIONS to maintainer of package PKG-DESC using SUBJECT.

(Dropping articles is a frequent trick to make the first line fit.)

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hi,

>> > What is the significance of "from source" here?
>>
>> It's just for indicating that we're talking about VC packages
>> specifically.  Does that make sense?
>
> Then how about
>
>   List of packages to install from their VCS repositories.
>

SGTM, updated in the new patch (v3) I've attached below.

>> >> +  "Send patch for REVISIONS to maintainer of the package PKG-DESC using SUBJECT.
>> >
>> > This should mention email, otherwise SUBJECT is confusing.  Maybe use
>> > "Email" instead of "Send".
>>
>> Alright, I made a couple more changes to this one.
>
> Now you have lost the reference to SUBJECT.  Was that necessary?  We
> generally try to mention all the mandatory arguments in the first line
> of the doc string.  The sentence below seems to be squeezable into 79
> columns:
>
>   Email patches for REVISIONS to maintainer of package PKG-DESC using SUBJECT.
>
> (Dropping articles is a frequent trick to make the first line fit.)

Nicely squeezed :) I've adopted your phrasing in the patch below.


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

From 2b977167f8c384d8769b71ad28ca4d4f175503f4 Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Fri, 18 Aug 2023 21:59:10 +0200
Subject: [PATCH v3] ; 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): Refine docstrings.
(package-vc-default-backend): Refine docstring and custom type.
---
 lisp/emacs-lisp/package-vc.el | 167 ++++++++++++++++++++--------------
 1 file changed, 100 insertions(+), 67 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index db8b41aee6a..52a4296e88f 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -94,7 +94,12 @@ 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.  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."
   :type `(alist :key-type (regexp :tag "Regular expression matching URLs")
                 :value-type (choice :tag "VC Backend"
                                     ,@(mapcar (lambda (b) `(const ,b))
@@ -102,14 +107,19 @@ 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."
-  :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))
   :version "29.1")
 
 (defcustom package-vc-register-as-project t
@@ -140,20 +150,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 +356,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.  A non-nil non-list value says to 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 +432,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 +476,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 +737,7 @@ package-vc--read-package-desc
 
 ;;;###autoload
 (defun package-vc-upgrade-all ()
-  "Attempt to upgrade all installed VC packages."
+  "Upgrade all installed VC packages."
   (interactive)
   (dolist (package package-alist)
     (dolist (pkg-desc (cdr package))
@@ -729,7 +747,7 @@ 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."
   (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 +810,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 +924,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 +966,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 description 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

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

[Philip Kaludercic]

Eshel Yaron <me@eshelyaron.com> writes:

> Hi,
>
>>> > What is the significance of "from source" here?
>>>
>>> It's just for indicating that we're talking about VC packages
>>> specifically.  Does that make sense?
>>
>> Then how about
>>
>>   List of packages to install from their VCS repositories.
>>
>
> SGTM, updated in the new patch (v3) I've attached below.
>
>>> >> +  "Send patch for REVISIONS to maintainer of the package PKG-DESC using SUBJECT.
>>> >
>>> > This should mention email, otherwise SUBJECT is confusing.  Maybe use
>>> > "Email" instead of "Send".
>>>
>>> Alright, I made a couple more changes to this one.
>>
>> Now you have lost the reference to SUBJECT.  Was that necessary?  We
>> generally try to mention all the mandatory arguments in the first line
>> of the doc string.  The sentence below seems to be squeezable into 79
>> columns:
>>
>>   Email patches for REVISIONS to maintainer of package PKG-DESC using SUBJECT.
>>
>> (Dropping articles is a frequent trick to make the first line fit.)
>
> Nicely squeezed :) I've adopted your phrasing in the patch below.
>
> From 2b977167f8c384d8769b71ad28ca4d4f175503f4 Mon Sep 17 00:00:00 2001
> From: Eshel Yaron <me@eshelyaron.com>
> Date: Fri, 18 Aug 2023 21:59:10 +0200
> Subject: [PATCH v3] ; 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): Refine docstrings.
> (package-vc-default-backend): Refine docstring and custom type.
> ---
>  lisp/emacs-lisp/package-vc.el | 167 ++++++++++++++++++++--------------
>  1 file changed, 100 insertions(+), 67 deletions(-)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index db8b41aee6a..52a4296e88f 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -94,7 +94,12 @@ 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.  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."

One should mention here, that this is a fallback mechanism, and can be
overwritten by a package specification.  Likewise, I think it is useful
to mention that not matching any entry is also not fatal.  This is what
I had intended to convey using the phrase "heuristic".

>    :type `(alist :key-type (regexp :tag "Regular expression matching URLs")
>                  :value-type (choice :tag "VC Backend"
>                                      ,@(mapcar (lambda (b) `(const ,b))
> @@ -102,14 +107,19 @@ 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."
> -  :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.  Also, it seems this is not
returning all the available VC backends...

>    :version "29.1")
>  
>  (defcustom package-vc-register-as-project t
> @@ -140,20 +150,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."

Sounds good.

>    :type '(alist :tag "List of packages you want to be installed"
>                  :key-type (symbol :tag "Package")
>                  :value-type
> @@ -345,19 +356,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

Perhaps "... as symbols"?

> +packages.  A non-nil non-list value says to always respect :make
> +and :shell-command keywords.

Wait, not any non-nil non-list keyword, just `t' (eq).

>  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 +432,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 +476,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 +737,7 @@ package-vc--read-package-desc
>  
>  ;;;###autoload
>  (defun package-vc-upgrade-all ()
> -  "Attempt to upgrade all installed VC packages."
> +  "Upgrade all installed VC packages."

The attempt here is intentional, since upgrading can fail if it was not
possible to successfully update the local VCS state due to merge
conflicts.  We can mention that there, and emphasise it at other places
as a major downside of VC packages.

>    (interactive)
>    (dolist (package package-alist)
>      (dolist (pkg-desc (cdr package))
> @@ -729,7 +747,7 @@ 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."

Same as above.

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

I wonder if we even have to mention the "use with Emacs"?  What else
could it mean?  The only issue I can think of is if someone hasn't yet
understood that package.el & co. are elisp package managers.

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

AND the package archive hosts elpa-packages.eld, UNLESS a heuristic can
be used to guess the repository.

> +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 +924,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 +966,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 description and SUBJECT is the subject of

I might be mistaken, but I always read pkg-desc as "package descriptor".

> +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)

Thanks a lot, this sounds a lot better.

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Thanks for reviewing, I've replied to your comments and attached an
updated patch (v4) below.

Philip Kaludercic <philipk@posteo.net> writes:

>> @@ -94,7 +94,12 @@ 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.  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."
>
> One should mention here, that this is a fallback mechanism, and can be
> overwritten by a package specification.  Likewise, I think it is useful
> to mention that not matching any entry is also not fatal.  This is what
> I had intended to convey using the phrase "heuristic".
>

Alright, I've added a few words to explain this.

>>    :type `(alist :key-type (regexp :tag "Regular expression matching URLs")
>>                  :value-type (choice :tag "VC Backend"
>>                                      ,@(mapcar (lambda (b) `(const ,b))
>> @@ -102,14 +107,19 @@ 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."
>> -  :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.

> Also, it seems this is not returning all the available VC backends...

Which backend do you see missing?  It should include all VC backends
that support `vc-call-backend` with `clone` as FUNCTION-NAME.  I get
SVN, Bzr, Git and Hg.  Anything missing?  Is there a better way to check
if a VC backend supports cloning for the purposes of `package-vc`?

>>    :version "29.1")
>>
>>  (defcustom package-vc-register-as-project t
>> @@ -140,20 +150,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."
>
> Sounds good.
>
>>    :type '(alist :tag "List of packages you want to be installed"
>>                  :key-type (symbol :tag "Package")
>>                  :value-type
>> @@ -345,19 +356,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
>
> Perhaps "... as symbols"?
>

I tried that but I find it makes the sentence less comprehensible
because it's already quite long, and the parenthesis help with that a
bit.  But I'm fine with either phrasing if you feel strongly about it.

>> +packages.  A non-nil non-list value says to always respect :make
>> +and :shell-command keywords.
>
> Wait, not any non-nil non-list keyword, just `t' (eq).
>

I see, I've adapted this from the original wording that just said "when
non-nil", now fixed.

>>  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 +432,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 +476,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 +737,7 @@ package-vc--read-package-desc
>>
>>  ;;;###autoload
>>  (defun package-vc-upgrade-all ()
>> -  "Attempt to upgrade all installed VC packages."
>> +  "Upgrade all installed VC packages."
>
> The attempt here is intentional, since upgrading can fail if it was not
> possible to successfully update the local VCS state due to merge
> conflicts.  We can mention that there, and emphasise it at other places
> as a major downside of VC packages.
>

Got it.  I don't think it's that useful to include "attempt to" in the
first line, because most commands have some failure modes.  Instead,
I've added a sentence about this possible failure after the first line.

>>    (interactive)
>>    (dolist (package package-alist)
>>      (dolist (pkg-desc (cdr package))
>> @@ -729,7 +747,7 @@ 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."
>
> Same as above.
>

Ditto.

>>    (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 +810,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.
>
> I wonder if we even have to mention the "use with Emacs"?  What else
> could it mean?  The only issue I can think of is if someone hasn't yet
> understood that package.el & co. are elisp package managers.
>

I don't have a strong preference about this one either way.

>> +
>> +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.
>
> AND the package archive hosts elpa-packages.eld, UNLESS a heuristic can
> be used to guess the repository.
>

Do you think we need to include these details in the docstring?  It
feels a bit too nitty gritty so I wonder if we can avoid it.  Are there
common use case in which this extra information is significant?

>> +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 +924,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 +966,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 description and SUBJECT is the subject of
>
> I might be mistaken, but I always read pkg-desc as "package descriptor".
>

Makes sense, updated.

>> +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)
>
> Thanks a lot, this sounds a lot better.

Thank you, here's the updated patch:


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

From 9e2a1e13a3078ac9029ec21b6362808b10bb15c4 Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Fri, 18 Aug 2023 21:59:10 +0200
Subject: [PATCH v4] ; 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): Refine docstrings.
(package-vc-default-backend): Refine docstring and custom type.
---
 lisp/emacs-lisp/package-vc.el | 175 +++++++++++++++++++++-------------
 1 file changed, 108 insertions(+), 67 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index db8b41aee6a..96fe9b386f5 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,14 +109,19 @@ 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."
-  :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))
   :version "29.1")
 
 (defcustom package-vc-register-as-project t
@@ -140,20 +152,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 +358,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 +434,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.
 
-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."
+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.
+
+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 +478,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 +739,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 +752,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 +818,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 +932,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 +974,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

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

[Philip Kaludercic]

Eshel Yaron <me@eshelyaron.com> writes:

> Thanks for reviewing, I've replied to your comments and attached an
> updated patch (v4) below.
>
> Philip Kaludercic <philipk@posteo.net> writes:
>
>>>    :type `(alist :key-type (regexp :tag "Regular expression matching URLs")
>>>                  :value-type (choice :tag "VC Backend"
>>>                                      ,@(mapcar (lambda (b) `(const ,b))
>>> @@ -102,14 +107,19 @@ 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."
>>> -  :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.
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.

>> Also, it seems this is not returning all the available VC backends...
>
> Which backend do you see missing?  It should include all VC backends
> that support `vc-call-backend` with `clone` as FUNCTION-NAME.  I get
> SVN, Bzr, Git and Hg.  Anything missing?  Is there a better way to check
> if a VC backend supports cloning for the purposes of `package-vc`?

Nevermind, I was mistaken in believing there was a vc-cvs-clone
implementation... but for some reason I appear not to be able to find
any reference to the implementation except in a ChangeLog file...
>>> +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
>>
>> Perhaps "... as symbols"?
>>
>
> I tried that but I find it makes the sentence less comprehensible
> because it's already quite long, and the parenthesis help with that a
> bit.  But I'm fine with either phrasing if you feel strongly about it.

No strong opinions here.

>>>  ;;;###autoload
>>>  (defun package-vc-upgrade-all ()
>>> -  "Attempt to upgrade all installed VC packages."
>>> +  "Upgrade all installed VC packages."
>>
>> The attempt here is intentional, since upgrading can fail if it was not
>> possible to successfully update the local VCS state due to merge
>> conflicts.  We can mention that there, and emphasise it at other places
>> as a major downside of VC packages.
>>
>
> Got it.  I don't think it's that useful to include "attempt to" in the
> first line, because most commands have some failure modes.  Instead,
> I've added a sentence about this possible failure after the first line.

Fine with that.

>>>    (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 +810,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.
>>
>> I wonder if we even have to mention the "use with Emacs"?  What else
>> could it mean?  The only issue I can think of is if someone hasn't yet
>> understood that package.el & co. are elisp package managers.
>>
>
> I don't have a strong preference about this one either way.

As long as the docstring remains short enough, it should be fine.

>>> +
>>> +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.
>>
>> AND the package archive hosts elpa-packages.eld, UNLESS a heuristic can
>> be used to guess the repository.
>>
>
> Do you think we need to include these details in the docstring?  It
> feels a bit too nitty gritty so I wonder if we can avoid it.  Are there
> common use case in which this extra information is significant?

The thing is, it should be explained somewhere (and I might have just
forgotten that i was explained at some point).  Perhaps the manual is a
better place to go into these complications.

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.  Other than that, the plan is to pull out the
`package-vc-default-backend' custom type and rename
`package-vc-allow-side-effects', right?

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- 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

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

[Philip Kaludercic]

Eshel Yaron <me@eshelyaron.com> writes:

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

Likely yes, but if we have two mostly equivalent solutions, I think that
flexibility without having to restart Emacs is preferable to performance.

[...]

> 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))

Do you think that these utility functions are useful enough to have in
general, or would it be better to just define a defconst once that
generates the `(choice ...)' object?

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

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> Eshel Yaron <me@eshelyaron.com> writes:
>
>>> 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.
>
> Likely yes, but if we have two mostly equivalent solutions, I think that
> flexibility without having to restart Emacs is preferable to performance.
>

Hmm I'm not sure I completely follow.  Which two solutions do you have
in mind?  I agree that it would be nice have the `:type` updated if the
user adds a relevant VC backend, but I don't think `defcustom` supports
something like that.  How do you propose to achieve that flexibility?

> [...]
>
>> 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))
>
> Do you think that these utility functions are useful enough to have in
> general, or would it be better to just define a defconst once that
> generates the `(choice ...)' object?
>

I think these are nice to have, but I don't mind removing them in favor
of a `defconst` (or a `define-widget` for that matter) if you think
that's better.

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

[Philip Kaludercic]

Eshel Yaron <me@eshelyaron.com> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Eshel Yaron <me@eshelyaron.com> writes:
>>
>>>> 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.
>>
>> Likely yes, but if we have two mostly equivalent solutions, I think that
>> flexibility without having to restart Emacs is preferable to performance.
>>
>
> Hmm I'm not sure I completely follow.  Which two solutions do you have
> in mind?  I agree that it would be nice have the `:type` updated if the
> user adds a relevant VC backend, but I don't think `defcustom` supports
> something like that.  How do you propose to achieve that flexibility?

Sorry, in the end it doesn't matter, I can't quickly think of a way to
define a type flexible enough to automatically update to the
environment, nor does computing a constant value at compile time seem
either worth it.

>> [...]
>>
>>> 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))
>>
>> Do you think that these utility functions are useful enough to have in
>> general, or would it be better to just define a defconst once that
>> generates the `(choice ...)' object?
>
> I think these are nice to have, but I don't mind removing them in favor
> of a `defconst` (or a `define-widget` for that matter) if you think
> that's better.

I think a defconst would be a simpler solution, having functions like
these always bear the danger of being misused, despite being marked as
internal.

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Thanks for reviewing, I've replied to your comments and attached an
updated patch (v4) below.

Philip Kaludercic <philipk@posteo.net> writes:

>> @@ -94,7 +94,12 @@ 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.  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."
>
> One should mention here, that this is a fallback mechanism, and can be
> overwritten by a package specification.  Likewise, I think it is useful
> to mention that not matching any entry is also not fatal.  This is what
> I had intended to convey using the phrase "heuristic".
>

Alright, I've added a few words to explain this.

>>    :type `(alist :key-type (regexp :tag "Regular expression matching URLs")
>>                  :value-type (choice :tag "VC Backend"
>>                                      ,@(mapcar (lambda (b) `(const ,b))
>> @@ -102,14 +107,19 @@ 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."
>> -  :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.

> Also, it seems this is not returning all the available VC backends...

Which backend do you see missing?  It should include all VC backends
that support `vc-call-backend` with `clone` as FUNCTION-NAME.  I get
SVN, Bzr, Git and Hg.  Anything missing?  Is there a better way to check
if a VC backend supports cloning for the purposes of `package-vc`?

>>    :version "29.1")
>>
>>  (defcustom package-vc-register-as-project t
>> @@ -140,20 +150,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."
>
> Sounds good.
>
>>    :type '(alist :tag "List of packages you want to be installed"
>>                  :key-type (symbol :tag "Package")
>>                  :value-type
>> @@ -345,19 +356,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
>
> Perhaps "... as symbols"?
>

I tried that but I find it makes the sentence less comprehensible
because it's already quite long, and the parenthesis help with that a
bit.  But I'm fine with either phrasing if you feel strongly about it.

>> +packages.  A non-nil non-list value says to always respect :make
>> +and :shell-command keywords.
>
> Wait, not any non-nil non-list keyword, just `t' (eq).
>

I see, I've adapted this from the original wording that just said "when
non-nil", now fixed.

>>  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 +432,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 +476,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 +737,7 @@ package-vc--read-package-desc
>>
>>  ;;;###autoload
>>  (defun package-vc-upgrade-all ()
>> -  "Attempt to upgrade all installed VC packages."
>> +  "Upgrade all installed VC packages."
>
> The attempt here is intentional, since upgrading can fail if it was not
> possible to successfully update the local VCS state due to merge
> conflicts.  We can mention that there, and emphasise it at other places
> as a major downside of VC packages.
>

Got it.  I don't think it's that useful to include "attempt to" in the
first line, because most commands have some failure modes.  Instead,
I've added a sentence about this possible failure after the first line.

>>    (interactive)
>>    (dolist (package package-alist)
>>      (dolist (pkg-desc (cdr package))
>> @@ -729,7 +747,7 @@ 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."
>
> Same as above.
>

Ditto.

>>    (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 +810,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.
>
> I wonder if we even have to mention the "use with Emacs"?  What else
> could it mean?  The only issue I can think of is if someone hasn't yet
> understood that package.el & co. are elisp package managers.
>

I don't have a strong preference about this one either way.

>> +
>> +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.
>
> AND the package archive hosts elpa-packages.eld, UNLESS a heuristic can
> be used to guess the repository.
>

Do you think we need to include these details in the docstring?  It
feels a bit too nitty gritty so I wonder if we can avoid it.  Are there
common use case in which this extra information is significant?

>> +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 +924,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 +966,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 description and SUBJECT is the subject of
>
> I might be mistaken, but I always read pkg-desc as "package descriptor".
>

Makes sense, updated.

>> +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)
>
> Thanks a lot, this sounds a lot better.

Thank you, here's the updated patch:


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

From 9e2a1e13a3078ac9029ec21b6362808b10bb15c4 Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Fri, 18 Aug 2023 21:59:10 +0200
Subject: [PATCH v4] ; 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): Refine docstrings.
(package-vc-default-backend): Refine docstring and custom type.
---
 lisp/emacs-lisp/package-vc.el | 175 +++++++++++++++++++++-------------
 1 file changed, 108 insertions(+), 67 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index db8b41aee6a..96fe9b386f5 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,14 +109,19 @@ 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."
-  :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))
   :version "29.1")
 
 (defcustom package-vc-register-as-project t
@@ -140,20 +152,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 +358,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 +434,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.
 
-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."
+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.
+
+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 +478,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 +739,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 +752,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 +818,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 +932,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 +974,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

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

[Philip Kaludercic]

Eshel Yaron <me@eshelyaron.com> writes:


>>>  (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.
>>
>> The name of this variable is very far from what it means.  Running
>> commands and "side effects" are barely related to each other.  Please
>> consider renaming the variable.  How about
>> package-vc-allow-build-commands?
>>
>
> I agree.  Philip, any objections to renaming this variable?  I don't
> know if people already use it.

If we find a better name, I have no objection since the user option was
recently added.

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

[Mauro Aranda]

Eshel Yaron <me@eshelyaron.com> writes:

 > Philip Kaludercic <philipk@posteo.net> writes:
 >
 >> Eshel Yaron <me@eshelyaron.com> writes:
 >>
 >>>> 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.
 >>
 >> Likely yes, but if we have two mostly equivalent solutions, I think that
 >> flexibility without having to restart Emacs is preferable to 
performance.
 >>
 >
 > Hmm I'm not sure I completely follow.  Which two solutions do you have
 > in mind?  I agree that it would be nice have the `:type` updated if the
 > user adds a relevant VC backend, but I don't think `defcustom` supports
 > something like that.  How do you propose to achieve that flexibility?

I haven't watch this thread closely, but do note that creating dynamic
choices is quite possible, if I understand correctly what you're looking
for.

If you can, take a look at the defcustom of completion-styles, in
minibuffer.el.  Its type uses a choice with a specialized
:convert-widget function to keep the choices up-to date.

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

[Philip Kaludercic]

Mauro Aranda <maurooaranda@gmail.com> writes:

> Eshel Yaron <me@eshelyaron.com> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Eshel Yaron <me@eshelyaron.com> writes:
>>>
>>>>> 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.
>>>
>>> Likely yes, but if we have two mostly equivalent solutions, I think that
>>> flexibility without having to restart Emacs is preferable to
>    performance.
>>>
>>
>> Hmm I'm not sure I completely follow.  Which two solutions do you have
>> in mind?  I agree that it would be nice have the `:type` updated if the
>> user adds a relevant VC backend, but I don't think `defcustom` supports
>> something like that.  How do you propose to achieve that flexibility?
>
> I haven't watch this thread closely, but do note that creating dynamic
> choices is quite possible, if I understand correctly what you're looking
> for.
>
> If you can, take a look at the defcustom of completion-styles, in
> minibuffer.el.  Its type uses a choice with a specialized
> :convert-widget function to keep the choices up-to date.

This might be exactly what we are looking for.  It should be possible to
adapt `completion--update-styles-options' pretty much directly to our
example.

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Philip Kaludercic <philipk@posteo.net> writes:

> Mauro Aranda <maurooaranda@gmail.com> writes:
>
>>
>> I haven't watch this thread closely, but do note that creating dynamic
>> choices is quite possible, if I understand correctly what you're looking
>> for.
>>
>> If you can, take a look at the defcustom of completion-styles, in
>> minibuffer.el.  Its type uses a choice with a specialized
>> :convert-widget function to keep the choices up-to date.
>
> This might be exactly what we are looking for.  It should be possible to
> adapt `completion--update-styles-options' pretty much directly to our
> example.

Mauro, thanks for the great tip.  As Philip said, this seems to be
exactly what we need here.  Accordingly, I'm attaching an updated patch for the
`defcustom` types (the two other patches from my previous message are
not affected and remain applicable).


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

From 0550c73148b23135de47229435205914d8080786 Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Sun, 20 Aug 2023 16:20:54 +0200
Subject: [PATCH v2] ; 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', and
compute the list of relevant on demand to keep it fresh.

* lisp/emacs-lisp/package-vc.el (package-vc--cloning-backend-p)
(package-vc--update-backends): New functions.
(package-vc--backend-type): New 'defconst'.
(package-vc-heuristic-alist, package-vc-default-backend): Use it.
---
 lisp/emacs-lisp/package-vc.el | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index a3762d252b0..7c60c55d67f 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -62,6 +62,24 @@ 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--update-backends (widget)
+  "Update WIDGET with VC backends suitable for cloning VCS repositories."
+  (widget-put widget :args
+              (seq-keep (lambda (be)
+                          (when (package-vc--cloning-backend-p be)
+                            (widget-convert (list 'const be))))
+                        vc-handled-backends))
+  widget)
+
+(defconst package-vc--backend-type
+  '(choice :convert-widget package-vc--update-backends)
+  "The type of VC backends that support cloning package VCS repositories.")
+
 (defcustom package-vc-heuristic-alist
   `((,(rx bos "http" (? "s") "://"
           (or (: (? "www.") "github.com"
@@ -103,9 +121,7 @@ package-vc-heuristic-alist
 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))
-                                              vc-handled-backends)))
+                :value-type ,package-vc--backend-type)
   :version "29.1")
 
 (defcustom package-vc-default-backend 'Git
@@ -116,8 +132,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))
+  :type package-vc--backend-type
   :version "29.1")
 
 (defcustom package-vc-register-as-project t
-- 
2.41.0

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

[Philip Kaludercic]

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

Eshel Yaron <me@eshelyaron.com> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Mauro Aranda <maurooaranda@gmail.com> writes:
>>
>>>
>>> I haven't watch this thread closely, but do note that creating dynamic
>>> choices is quite possible, if I understand correctly what you're looking
>>> for.
>>>
>>> If you can, take a look at the defcustom of completion-styles, in
>>> minibuffer.el.  Its type uses a choice with a specialized
>>> :convert-widget function to keep the choices up-to date.
>>
>> This might be exactly what we are looking for.  It should be possible to
>> adapt `completion--update-styles-options' pretty much directly to our
>> example.
>
> Mauro, thanks for the great tip.  As Philip said, this seems to be
> exactly what we need here.  Accordingly, I'm attaching an updated patch for the
> `defcustom` types (the two other patches from my previous message are
> not affected and remain applicable).

I have slightly modified it to remove the helper functions.  As
mentioned before.  Hope you are fine with it:


[-- Attachment #2: Type: text/plain, Size: 1912 bytes --]

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index bf4cad4f319..dfaedbcc84c 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -62,6 +62,18 @@ 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"
@@ -103,9 +115,7 @@ package-vc-heuristic-alist
 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))
-                                              vc-handled-backends)))
+                :value-type ,package-vc--backend-type)
   :version "29.1")
 
 (defcustom package-vc-default-backend 'Git
@@ -116,8 +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))
+  :type package-vc--backend-type
   :version "29.1")
 
 (defcustom package-vc-register-as-project t

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> I have slightly modified it to remove the helper functions.  As
> mentioned before.  Hope you are fine with it:

I am, thanks.

> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> [...]

Best,

Eshel

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Philip,

Philip Kaludercic <philipk@posteo.net> writes:

> close 65386 30.1
> quit

I got a message saying that this bug is now closed, but I don't see that
the relevant patches have been applied.
Just to make sure, is this intentional?

Thanks,

Eshel

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

[Philip Kaludercic]

Eshel Yaron <me@eshelyaron.com> writes:

> Hi Philip,
>
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> close 65386 30.1
>> quit
>
> I got a message saying that this bug is now closed, but I don't see that
> the relevant patches have been applied.
> Just to make sure, is this intentional?

No, that was my mistake.  I had falsely believed, that the patches were
pushed to master, but this was not the case.  It has been done now.
Sorry for the confusion.

> Thanks,
>
> Eshel

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> Eshel Yaron <me@eshelyaron.com> writes:
>
>>
>> I got a message saying that this bug is now closed, but I don't see that
>> the relevant patches have been applied.
>> Just to make sure, is this intentional?
>
> No, that was my mistake.  I had falsely believed, that the patches were
> pushed to master, but this was not the case.  It has been done now.
> Sorry for the confusion.
>

Got it, no worries, and thanks :)