Philip Kaludercic <philipk@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 ;-)
>
> 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?

Yes, indeed, this is an error, thanks to catch it.

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

No problem, it is probably better to not introduce a new bug for now.

> Do you have any comments on tricks or traps that I should keep in
> mind?

Yes the other changes are about built-in packages that are shown in
completion, and refused later by package-install.

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

Here.

>> 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).

Great, no problem, as long as it is cleaned up that's good ;-)

Thanks.

-- 
Thierry