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.