Thanks for reviewing, I've replied to your comments and attached an updated patch (v4) below. Philip Kaludercic 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: