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

[Tegar Syahputra]

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

I'm not part of FSF contributor, and wasn't really thinking of contributing
directly. Just giving a heads up that's all.

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

I intern the interactive input because the actual type needed is symbol 
type.
The check was from original, it accept both string and symbol.

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

No, you must check if the package is installed. That will always delete
`old-pkg-desc' even if installation error occurs.

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

Yes, it's to check if the installations was interrupted or if error occur.
I don't think `package-install' gives meaningful return that indicates
package was properly installed. The output it gives could be used but,
I just thought checking the output string may not be reliable or elegant.

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

Yeah, and new-pkg-desc which is a package-desc from package-archive-contents
is implied to be remote thus doesn't include directory.


I'm OP, sorry I didn't configure my email name earlier.