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

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?

>>> The more robust solution, would be to declare an exception type that
>>> could include the transaction data, which we could handle in
>>> `package-upgrade-all' and related functions.
>>
>> Sure, but I would only implement such a exception type with metadata if
>> it is worth it at multiple places in package.el.
>>
>>>> If you feel that it is unnecessary complexity to collect the list of
>>>> packages which have not been upgraded, maybe don't show an error at all,
>>>> since rejection during review was intentional?
>>>
>>> No, this is all doable, one just has to think about what the right
>>> approach is a bit.
>>
>> To be clear, this is a minor polishing issue, so don't spend too much
>> time about it.
>
>>>> Thanks!