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

Daniel Mendler <[email protected]> writes:

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

Thank you for the quick response, I've pushed it to emacs-31.

Sean: You marked this bug report as a blocker for Emacs 31.  We have
fixed the main issue, but we can still improve on it in the future.
Should we still close the bug report now or keep it open and de-link it
from the release bug?


>>>>>> Thanks!