bug#81112: 31.0.60; Package.el error when rejecting during review

Philip Kaludercic <[email protected]> writes:

> Philip Kaludercic <[email protected]> writes:
>
>> Sorry for the delay, this got buried in my inbox.
>>
>> Daniel Mendler <[email protected]> writes:
>>
>>> Philip Kaludercic <[email protected]> writes:
>>>
>>>> Daniel Mendler <[email protected]> writes:
>>>>
>>>>> When rejecting packages during package review, only the last rejected
>>>>> package is shown in the error. I would expect to see a list of the
>>>>> packages which have not been installed successfully. The error also does
>>>>> not look nice:
>>>>>
>>>>>   Rejected ‘dicom’, reverting transaction.
>>>>>   Error while upgrading: (error "Package ‘dicom’ not installed")
>>>>>
>>>>> Expected:
>>>>>
>>>>>   Not upgraded: dicom, elfeed
>>>>
>>>> The easy fix would be to adjust the error message to something like:
>>>>
>>>>   Error while upgrading: (error "Aborted transaction: dicom-X.Y, elfeed-M.N")
>>>
>>> To me the way this is printed looks a bit like an internal error. It
>>> should better look like this:
>>>
>>>   Error while upgrading: Aborted transaction: dicom-X.Y, elfeed-M.N
>>>
>>> But then, this is not actually an error, since the upgrades were
>>> rejected intentionally during review, so if you want to polish this, it
>>> could look like this:
>>>
>>>   Upgraded: foo-X.Y, bar-X.Y; Rejected: baz-X.Y
>>
>> If we don't need to look up the version numbers and the issue is /just/
>> with `package-upgrade-all', this part is easy:
>>
>> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
>> index da4642aeb78..78465696e1c 100644
>> --- a/lisp/emacs-lisp/package.el
>> +++ b/lisp/emacs-lisp/package.el
>> @@ -2202,7 +2202,8 @@ package-upgrade-all
>>  `\\<package-menu-mode-map>\\[package-menu-mark-install]' after `\\[list-packages]'."
>>    (interactive (list (not noninteractive)))
>>    (package-refresh-contents)
>> -  (let ((upgradeable (package--upgradeable-packages package-install-upgrade-built-in)))
>> +  (let ((upgradeable (package--upgradeable-packages package-install-upgrade-built-in))
>> +        (upgraded '()))
>>      (if (not upgradeable)
>>          (message "No packages to upgrade")
>>        (when (and query
>> @@ -2214,7 +2215,15 @@ package-upgrade-all
>>          (user-error "Upgrade aborted"))
>>        (dolist (pkg upgradeable)
>>          (with-demoted-errors "Error while upgrading: %S"
>> -          (package-upgrade pkg))))))
>> +          (package-upgrade pkg)
>> +          (push pkg upgraded)))
>> +      (let ((rejected (cl-set-difference upgradable upgraded)))
>> +        (message
>> +         "Upgraded: %s%s"
>> +         (mapconcat #'symbol-name upgraded ", ")
>> +         (if rejected
>> +             (concat "; Rejected: " (mapconcat #'symbol-name rejected ", "))
>> +           ""))))))
>>  
>>  (defun package--dependencies (pkg)
>>    "Return a list of all transitive dependencies of PKG.
>>
>>
>> Note that this doesn't include dependencies!
>>
>> The other message would still get logged, but the last message the user
>> sees is this confirmation.
>
> Ping?  Do you think that this is an acceptable fix for now?

Sounds good!

>>>>> Thanks!