Re: feature/package-vc has been merged

[Philip Kaludercic <philipk@posteo.net>]

Eli Zaretskii <eliz@gnu.org> writes:

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

You are right, I forgot about this when renaming package-vc-unpack to
package-vc--unpack.  How about

  "Directory used to store clone repositories.  
                                                
This directory is only used for packages with a special `:lisp-dir'
entry in their package specification (for details on package
specifications see `package-vc-selected-packages').  Repositories for
packages like these are cloned into the directory specified here, and
their `:lisp-dir' is then linked back into the user elpa directory."

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

My intention was for it to be read like "Ensure (source packages)
specified in ...".  But yes, the phrasing is inelegant.

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

It is a bit long, but checkdoc doesn't appear to complain.

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

You are right (same mistake as before), the documentation for package
specifications should be moved from the internal variable to either
'package-vc-selected-packages' or some other place.  It might not be bad
to document it in package.texi either?  Or would that be too Lisp-y for
the Emacs manual?

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

It was supposed to be `package-vc-ensure-packages'.  How about
rephrasing that last sentence into "By calling the function
`package-vc-selected-packages", the packages specified in this list can
also be installed.

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

Yes, these are plist keys.  Should we say something like "Valid keys for
these plists include:".  But as I have said above, the documentation for
package specifications (which is what the plist is) should be documented
somewhere else.

>>   "Extract the commit of a development package PKG."
>
> This doc string doesn't seem to match what the function does.

I was confused at first, as I was assuming you were talking about
`package-vc-commit', but it appears I accidentally copied the docstring
into `package-vc--version'.  A better documentation string here would be

     "Return the version number for the source package PKG."

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

I can expand the documentation here.

"file" is the input, either a .texi or an .org file.  If it is an .org
file, we want to convert it to .texi first (which is what the (when ...)
block does), and write the output into a temporary file.  The temporary
file is used as an input to install-info, that builds the .info file.

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

The issue is that vc-pull *can be* asynchronous, so wrapping it in a
`unwind-protect' would un-modify the hook too soon.  But you are right
that if vc-pull were to fail, the hook would remain inside of
`vc-post-command-functions' for too long.

As I said in that comment

   "If there is a better way to do this, it should be done."