bug#72141: 29.4; package-upgrade vs package-load-list

[Philip Kaludercic <philipk@posteo.net>]

Thierry Volpiatto <thievol@posteo.net> writes:

> Hello Philip,
>
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Gladly, then I'd like to try it out it and perhaps write a ERT test.
>
> Patch attached, please review and test it before merging ;-)

Sorry for the delay.

> Thanks.
>
> -- 
> Thierry
>
> From 292e251a383c1fb53cc377cd32f71705e6742f85 Mon Sep 17 00:00:00 2001
> From: Thierry Volpiatto <thievol@posteo.net>
> Date: Sat, 3 Aug 2024 06:07:28 +0200
> Subject: [PATCH] Fix bug#72141, package-upgrade should not include disabled
>  packages
>
> * lisp/emacs-lisp/package.el (package--upgradeable-packages):
> Rewrite with a new optional arg to filter out disabled packages from
> output.
> (package-upgrade, package-upgrade-all): Use it and filter out built-in
> packages from completion according package-install-upgrade-built-in
> value.
> ---
>  lisp/emacs-lisp/package.el | 60 ++++++++++++++++++++++----------------
>  1 file changed, 35 insertions(+), 25 deletions(-)
>
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index 7cae8d68bc0..83996c9d6de 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -2259,11 +2259,15 @@ had been enabled."
>    "Upgrade package NAME if a newer version exists."
>    (interactive
>     (list (completing-read
> -          "Upgrade package: " (package--upgradeable-packages t) nil t)))
> +          "Upgrade package: " (package--upgradeable-packages
> +                               package-install-upgrade-built-in
> +                               'ignore-disabled)
> +          nil t)))
>    (let* ((package (if (symbolp name)
>                        name
>                      (intern name)))
>           (pkg-desc (cadr (assq package package-alist)))
> +         ;; Keep this binding for non-interactive use.
>           (package-install-upgrade-built-in (not pkg-desc)))
>      ;; `pkg-desc' will be nil when the package is an "active built-in".
>      (if (and pkg-desc (package-vc-p pkg-desc))
> @@ -2275,32 +2279,37 @@ had been enabled."
>                         ;; before.  Mark it as installed explicitly.
>                         (and pkg-desc 'dont-select)))))
>  
> -(defun package--upgradeable-packages (&optional include-builtins)
> +(defun package--upgradeable-packages (&optional
> +                                        include-builtins ignore-disabled)
>    ;; Initialize the package system to get the list of package
>    ;; symbols for completion.
>    (package--archives-initialize)
> -  (mapcar
> -   #'car
> -   (seq-filter
> -    (lambda (elt)
> -      (or (let ((available
> -                 (assq (car elt) package-archive-contents)))
> -            (and available
> -                 (or (and
> -                      include-builtins
> -                      (not (package-desc-version (cadr elt))))
> -                     (version-list-<
> -                      (package-desc-version (cadr elt))
> -                      (package-desc-version (cadr available))))))
> -          (package-vc-p (cadr elt))))
> -    (if include-builtins
> -        (append package-alist
> -                (mapcan
> -                 (lambda (elt)
> -                   (when (not (assq (car elt) package-alist))
> -                     (list (list (car elt) (package--from-builtin elt)))))
> -                 package--builtins))
> -      package-alist))))
> +  (let ((pkgs (if include-builtins
> +                  (append package-alist
> +                          (append package-alist
> +                                  (mapcan
> +                                   (lambda (elt)
> +                                     (when (not (assq (car elt) package-alist))
> +                                       (list
> +                                        (list
> +                                         (car elt)
> +                                         (package--from-builtin elt)))))
> +                                   package--builtins)))
> +                package-alist)))
> +    (cl-loop for (sym desc) in pkgs
> +             for available =
> +             (if-let ((av (assq sym package-archive-contents)))
> +                 (if ignore-disabled
> +                     (and (not (package-disabled-p sym cversion)) av) av))
                                                          ^
This loop really confused me.  Specifically               here
If I am not mistaken, cversion might be set by the last iteration, no?
In that case, we are passing some version string unrelated to sym?

I am leaning towards keeping the existing loop, i.e. not rewriting
anything while fixing a bug.  It is probably easier for me to do that.
Do you have any comments on tricks or traps that I should keep in mind?

> +             for cversion = (and available (package-desc-version desc))
> +             when (or (and available
> +                           (or (and include-builtins (not cversion))
> +                               (and cversion
> +                                    (version-list-<
> +                                     cversion
> +                                     (package-desc-version (cadr available))))))
> +                      (package-vc-p desc))
> +             collect sym)))
>  
>  ;;;###autoload
>  (defun package-upgrade-all (&optional query)
> @@ -2315,7 +2324,8 @@ from ELPA by either using `\\[package-upgrade]' or
>  `\\<package-menu-mode-map>\\[package-menu-mark-install]' after `\\[list-packages]'."
>    (interactive (list (not noninteractive)))
>    (package-refresh-contents)
> -  (let ((upgradeable (package--upgradeable-packages)))
> +  (let ((upgradeable (package--upgradeable-packages
> +                      package-install-upgrade-built-in 'ignore-disabled)))
>      (if (not upgradeable)
>          (message "No packages to upgrade")
>        (when (and query

Thierry Volpiatto <thievol@posteo.net> writes:

> Thierry Volpiatto <thievol@posteo.net> writes:
>
>> Hello Philip,
>>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Gladly, then I'd like to try it out it and perhaps write a ERT test.
>>
>> Patch attached, please review and test it before merging ;-)
>>
>> Thanks.
>
> Also, I recently had to read package.el code and I found many loops are
> too complex and/or too difficult to read, here are some:
>

[...]

>
> I have a patch for this, let me know if interested, or perhaps I should
> open a new bug report ?

I have some spare time now, and if I also find some spare energy, I plan
to clean up a number of things in package.el, mainly removing duplicate
logic and making the code more flexible.  I'll start work on a separate
branch, and that's why I am not too enthusiastic about these kinds of
changes on master (for now).

-- 
	Philip Kaludercic on peregrine