Thierry Volpiatto <thievol@posteo.net> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Philip Kaludercic <philipk@posteo.net>
>>> Cc: Thierry Volpiatto <thievol@posteo.net>,  72141@debbugs.gnu.org
>>> Date: Sun, 28 Jul 2024 11:47:44 +0000
>>> 
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>> 
>>> >> From: Thierry Volpiatto <thievol@posteo.net>
>>> >> Date: Tue, 16 Jul 2024 14:46:37 +0000
>>> >> 
>>> >> 
>>> >> I think there is a bug here, but please verify with following recipe as
>>> >> I don't use widely package installation, at least for myself.  When reading
>>> >> the code I believe it is reproductible as well on emacs-30+.
>>> >> 
>>> >> 1) Install package foo and bar.
>>> >> 2) Disable them in package-load-list ((foo nil) (bar nil) all).
>>> >> 3) Wait some time until foo and/or bar have new versions available.
>>> >> 4) Call package-upgrade-all.  It will call package-upgrade on foo
>>> >> and bar (and possibly others).  When package-upgrade find foo
>>> >> package it will (1) delete it and (2) call package-install which
>>> >> will refuse to install (error) because foo is disabled.
>>> >> 
>>> >> As a result we have lost foo package, it is now uninstalled.
>>> >> Same problem with M-x package-upgrade, foo and bar are listed in
>>> >> completion and made available whereas they are going to fail to
>>> >> upgrade.
>>> >
>>> > Philip, any comments or suggestions?
>>> 
>>> The issue is that we don't install a package if it is disabled.  So
>>> either we allow installing (but don't activate) disabled packages, or we
>>> ignore disabled packages during upgrades.
>>
>> The latter, I'd say.  It makes little sense to upgrade disabled
>> packages.
>
> When I posted initially this bugreport I wrote this (fully not tested):
>
>     (defun package--upgradeable-packages (&optional include-builtins filter-load-list)
>       ;; Initialize the package system to get the list of package
>       ;; symbols for completion.
>       (package--archives-initialize)
>       (let ((pkgs (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))) 
>         (cl-loop for (sym desc) in pkgs
>                  for available = (assq sym package-archive-contents)
>                  when (or (and available
>                                (or (and
>                                     include-builtins
>                                     (not (package-desc-version desc)))
>                                    (version-list-<
>                                     (package-desc-version desc)
>                                     (package-desc-version (cadr available)))
>                                    (and filter-load-list
>                                         (pcase (assq p package-load-list)
>                                           (`(,sym ,val) (or (not (eq val nil))
>                                                             (not (stringp val))))))))
>                           (package-vc-p desc))
>                  collect sym)))
>
> Perhaps package-disabled-p can be used instead of the pcase (I didn't
> know its existence).

Here a version fixing typo and using package-disabled-p (same, still
fully untested)
Note the extra optional arg filter-load-list that allow preserving the initial behavior
if needed (better name?).

(defun package--upgradeable-packages (&optional include-builtins filter-load-list)
  ;; Initialize the package system to get the list of package
  ;; symbols for completion.
  (package--archives-initialize)
  (let ((pkgs (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))) 
    (cl-loop for (sym desc) in pkgs
             for available = (assq sym package-archive-contents)
             for cversion = (and available (package-desc-version desc))
             when (or (and available
                           (or (and
                                include-builtins
                                (not (package-desc-version desc)))
                               (version-list-<
                                cversion
                                (package-desc-version (cadr available)))
                               (and filter-load-list
                                    (package-disabled-p sym cversion))))
                      (package-vc-p desc))
             collect sym)))

Also there is IMO another inconsistency in package-upgrade where the
completion is done inconditionally on packages+builtins and later
package-install-upgrade-built-in is let bounded to prevent
package-install to upgrade built-in in case user chose a built-in!

-- 
Thierry