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

[Tegar Syahputra <dqs7cp2e@gmail.com>]

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