Re: feature/package-vc has been merged

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: Richard Stallman <rms@gnu.org>,  emacs-devel@gnu.org
> Date: Fri, 04 Nov 2022 18:01:09 +0000
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> >> OK, I've merged master into feature/package+vc, resolving all conflicts
> >> and would be prepared to merge feature/package+vc if there are no
> >> further objections.
> >
> > No objections on my side,
> 
> I have merged the branch onto master, so if anyone wants to fix,
> add/remove or improve anything, feel free to do so.

Thanks.  A few comments about the documentation (you will see that I
fixed some of the issues where I could):

> (defcustom package-vc-repository-store
>   (expand-file-name "emacs/vc-packages" (xdg-data-home))
>   "Directory used by `package-vc--unpack' to store repositories."

The doc string of a user option should not reference an internal
function, it should reference user-visible features.  Would it be
correct to say that this directory is used by package-vc to store
repositories of packages it fetches and/or installs?

> (defun package-vc-ensure-packages ()
>   "Ensure source packages specified in `package-vc-selected-packages'."

This doc string should explain what does this function ensure.
"Ensure source" doesn't clarify that.  I think, but cannot be sure,
this should say something like

  Ensure packages specified in `package-vc-selected-packages' are installed."

> (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:
> 
> - nil, if any package version can be installed;
> - a version string, if that specific revision is to be installed;
> - a property list of the form described in
>   `package-vc-archive-spec-alist', giving a package
>   specification.

There's no variable package-vc-archive-spec-alist.  Did you mean
package-vc--archive-spec-alist instead?  In that case, I think the
format of that list should be spelled out in this doc string, as the
referenced variable is an internal one.

> This user option differs from `package-selected-packages' in that
> it is meant to be specified manually.  You can also use the
> function `package-vc-selected-packages' to apply the changes."

The function package-vc-selected-packages mentioned in the last
sentence doesn't seem to exist.  Also, what does it mean "to apply the
changes" -- apply the changes to what?

> (defvar package-vc--archive-spec-alist nil
>   "List of package specifications for each archive.
> The list maps each package name, as a string, to a plist.
> Valid keys include
> 
>         `:url' (string)

The "Valid keys" part is "out of the blue" here.  Keys of what?  If
that's a reference to the "plist" part preceding it, then we aren't
talking about a plist, we are talking about keyword/value pairs,
right?

> (defun package-vc--version (pkg)
>   "Extract the commit of a development package PKG."

This doc string doesn't seem to match what the function does.

> (defun package-vc--build-documentation (pkg-desc file)
>   "Build documentation FILE for PKG-DESC."
>   (let ((pkg-dir (package-desc-dir pkg-desc)))
>     (when (string-match-p "\\.org\\'" file)
>       (require 'ox)
>       (require 'ox-texinfo)
>       (with-temp-buffer
>         (insert-file-contents file)
>         (setq file (make-temp-file "ox-texinfo-"))
>         (org-export-to-file 'texinfo file)))
>     (call-process "install-info" nil nil nil
>                   file pkg-dir)))

I'm confused by this function.  Does org-export-to-file produce a
.texi file or an Info file?  In any case, the semantics is confusing:
if FILE has the .org extension, the function installs a file whose
name is not FILE, otherwise the function installs FILE.  This seems to
conflate source and destination files in a confusing way.

> (defun package-vc-update (pkg-desc)
>   "Attempt to update the package PKG-DESC."
>   ;; HACK: To run `package-vc--unpack-1' after checking out the new
>   ;; revision, we insert a hook into `vc-post-command-functions', and
>   ;; remove it right after it ran.  To avoid running the hook multiple
>   ;; times or even for the wrong repository (as `vc-pull' is often
>   ;; asynchronous), we extract the relevant arguments using a pseudo
>   ;; filter for `vc-filter-command-function', executed only for the
>   ;; side effect, and store them in the lexical scope.  When the hook
>   ;; is run, we check if the arguments are the same (`eq') as the ones
>   ;; previously extracted, and only in that case will be call
>   ;; `package-vc--unpack-1'.  Ugh...

Shouldn't this use unwind-protect, to make sure the hacked
post-command-hook doesn't leak out?