Re: Suggestion for package.el: atomic package-upgrade

[Philip Kaludercic]

dqs7cp2e <dqs7cp2e@gmail.com> writes:

> The current `package-upgrade' from package.el delete old package
> before installing the new one. This can be problematic if the user
> interrupt the process or if there is some network problems.
>
> `package-install' allow the same package to be installed if the
> argument is `package-desc' instead symbol representing package name.
> This allow package to be upgraded atomically. Is this a bad idea?

No, I think this is a good idea, but it would be best if you could write
a Git patch and send it to "bug-gnu-emacs@gnu.org" (you can use M-x
submit-emacs-patch).

> diff -u --label \#\<buffer\ package.el.gz\> --label \#\<buffer\ \*scratch\*\> 
> /tmp/buffer-content-4azQGZ /tmp/buffer-content-x8FLpt
> --- #<buffer package.el.gz>
> +++ #<buffer *scratch*>
> @@ -2275,16 +2275,20 @@
>  package using this command, first upgrade the package to a
>  newer version from ELPA by using 
> `\\<package-menu-mode-map>\\[package-menu-mark-install]' after 
> `\\[list-packages]'."
>    (interactive
> -   (list (completing-read
> -          "Upgrade package: " (package--upgradeable-packages) nil t)))
> -  (let* ((package (if (symbolp name)
> -                      name
> -                    (intern name)))
> -         (pkg-desc (cadr (assq package package-alist))))
> -    (if (package-vc-p pkg-desc)
> -        (package-vc-upgrade pkg-desc)
> -      (package-delete pkg-desc 'force 'dont-unselect)
> -      (package-install package 'dont-select))))
> +   (list (intern (completing-read
> +                  "Upgrade package: " (package--upgradeable-packages) nil 
> t))))
> +  (let* ((name (if (symbolp name)
> +                   name
> +                 (intern name)))

If you always intern the package name, you don't need this check
anymore.  On the other hand, you don't need to intern the name, because
of this check, and I think it is better to keep it because that gives
the user more flexibility when invoking the function.

> +         (old-pkg-desc (cadr (assq name package-alist)))
> +         (new-pkg-desc (cadr (assq name package-archive-contents))))
> +    (if (package-vc-p old-pkg-desc)
> +        (package-vc-upgrade old-pkg-desc)
> +      (unwind-protect

I am wondering if unwind-protect is the best approach here, or even
necessary.  Wouldn't something like

--8<---------------cut here---------------start------------->8---
(when (progn (package-install new-pkg-desc 'dont-select) t)
  (package-delete old-pkg-desc 'force 'dont-unselect))
--8<---------------cut here---------------end--------------->8---

have the same effect?  My reasoning is that we are not actually cleaning
anything up in the UNWIND-FORMS, but just checking if the
`package-install' was not interrupted, right?

> +          (package-install new-pkg-desc 'dont-select)
> +        (if (package-installed-p (package-desc-name new-pkg-desc)
> +                                 (package-desc-version new-pkg-desc))

If you check the definition of `package-installed-p', you will find it
does not use MIN-VERSION if the first argument satisfied
`package-desc-p', which makes sense because with a concrete descriptor,
we can unambiguously check if a specific package version has been
installed (the implementation just checks if the expected directory
exists).

Also, perhaps consider using `when' here.  I argue that it looks better
in imperative code where the return-value is not of interest.

> +            (package-delete old-pkg-desc 'force 'dont-unselect))))))
>  
>  (defun package--upgradeable-packages ()
>    ;; Initialize the package system to get the list of package
>
> Diff finished.  Mon Jul 31 08:22:46 2023