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

[Eshel Yaron]

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:

v4-0001-Refine-some-package-vc-docstrings.patch
Description: Text Data