bug#63757: 29.0.91 order of package paths changed: random old versions of packages in load-path

["Евгений Бойков via Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 10296 bytes --]

Excuse me for extra messages, I've already found a bug in the previous
patch.
I need a review, especially with in-place operations.

move "Prefer VC packages" to `package-process-define-package`
drop `package-disabled-p` and "Prefer builtin packages" comment

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index ba0e3618f28..87c57990705 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -686,7 +686,7 @@ Convert EXP into a `package-desc' object using the

 If there already exists a package by the same name in
 `package-alist', insert this object there such that the packages
-are sorted with the highest version first."
+are sorted with VC packages first and the highest version second."
   (when (eq (car-safe exp) 'define-package)
     (let* ((new-pkg-desc (apply #'package-desc-from-define (cdr exp)))
            (name (package-desc-name new-pkg-desc))
@@ -696,13 +696,29 @@ are sorted with the highest version first."
           ;; If there's no old package, just add this to `package-alist'.
           (push (list name new-pkg-desc) package-alist)
         ;; If there is, insert the new package at the right place in the
list.
-        (while
-            (if (and (cdr old-pkgs)
-                     (version-list-< version
-                                     (package-desc-version (cadr
old-pkgs))))
-                (setq old-pkgs (cdr old-pkgs))
-              (push new-pkg-desc (cdr old-pkgs))
-              nil)))
+        (progn
+          ;; Prefer newer packages
+          (while
+              (if (and (cdr old-pkgs)
+                       (version-list-< version
+                                       (package-desc-version (cadr
old-pkgs))))
+                  (setq old-pkgs (cdr old-pkgs))
+                (push new-pkg-desc (cdr old-pkgs))
+                nil))
+          ;; Prefer VC packages with greater priority than newer packages
+          (let* ((pkg-desc
+                  (apply 'append
+                         (cl-reduce
+                          (lambda (p1 p2)
+                            (if (package-vc-p p1)
+                                (push p1 (cl-first p2))
+                              (push p1 (cl-second p2)))
+                            p2)
+                          (cdr (assq name package-alist))
+                          :initial-value (list nil nil)
+                          :from-end t))))
+            (setf (cdr (assq name package-alist)) pkg-desc)
+            (setq new-pkg-desc (car pkg-desc)))))
       new-pkg-desc)))

 (declare-function package-vc-commit "package-vc" (pkg))
@@ -916,17 +932,7 @@ correspond to previously loaded files."

 (defun package--get-activatable-pkg (pkg-name)
   ;; Is "activatable" a word?
-  (let ((pkg-descs (sort (cdr (assq pkg-name package-alist))
-                         (lambda (p1 p2)
-                           (let ((v1 (package-desc-version p1))
-                                 (v2 (package-desc-version p2)))
-                             (or
-                              ;; Prefer VC packages.
-                              (package-vc-p p1)
-                              (package-vc-p p2)
-                              ;; Prefer builtin packages.
-                              (package-disabled-p p1 v1)
-                              (not (package-disabled-p p2 v2))))))))
+  (let ((pkg-descs (cdr (assq pkg-name package-alist))))
     ;; Check if PACKAGE is available in `package-alist'.
     (while
         (when pkg-descs

On Sun, Jun 4, 2023 at 2:46 PM Евгений Бойков <artscan@list.ru> wrote:

> move "Prefer VC packages" to `package-process-define-package`
> drop `package-disabled-p` and "Prefer builtin packages" comment
>
>
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index ba0e3618f28..65ec07b945a 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -686,7 +686,7 @@ Convert EXP into a `package-desc' object using the
>
>  If there already exists a package by the same name in
>  `package-alist', insert this object there such that the packages
> -are sorted with the highest version first."
> +are sorted with VC packages first and the highest version second."
>    (when (eq (car-safe exp) 'define-package)
>      (let* ((new-pkg-desc (apply #'package-desc-from-define (cdr exp)))
>             (name (package-desc-name new-pkg-desc))
> @@ -696,13 +696,30 @@ are sorted with the highest version first."
>            ;; If there's no old package, just add this to `package-alist'.
>            (push (list name new-pkg-desc) package-alist)
>          ;; If there is, insert the new package at the right place in the
> list.
> -        (while
> -            (if (and (cdr old-pkgs)
> -                     (version-list-< version
> -                                     (package-desc-version (cadr
> old-pkgs))))
> -                (setq old-pkgs (cdr old-pkgs))
> -              (push new-pkg-desc (cdr old-pkgs))
> -              nil)))
> +        (progn
> +          ;; Prefer newer packages
> +          (while
> +              (if (and (cdr old-pkgs)
> +                       (version-list-< version
> +                                       (package-desc-version (cadr
> old-pkgs))))
> +                  (setq old-pkgs (cdr old-pkgs))
> +                (push new-pkg-desc (cdr old-pkgs))
> +                nil))
> +          ;; Prefer VC packages
> +          (let* ((pkg-desc0 (cdr (assq name package-alist)))
> +                 (pkg-desc
> +                  (apply 'append
> +                         (cl-reduce
> +                          (lambda (p1 p2)
> +                            (if (package-vc-p p1)
> +                                (push p1 (cl-first p2))
> +                              (push p1 (cl-second p2)))
> +                            p2)
> +                          pkg-desc0
> +                          :initial-value (list nil nil)
> +                          :from-end t))))
> +            (setf pkg-desc0 pkg-desc)
> +            (setq new-pkg-desc (car pkg-desc)))))
>        new-pkg-desc)))
>
>  (declare-function package-vc-commit "package-vc" (pkg))
> @@ -916,17 +933,7 @@ correspond to previously loaded files."
>
>  (defun package--get-activatable-pkg (pkg-name)
>    ;; Is "activatable" a word?
> -  (let ((pkg-descs (sort (cdr (assq pkg-name package-alist))
> -                         (lambda (p1 p2)
> -                           (let ((v1 (package-desc-version p1))
> -                                 (v2 (package-desc-version p2)))
> -                             (or
> -                              ;; Prefer VC packages.
> -                              (package-vc-p p1)
> -                              (package-vc-p p2)
> -                              ;; Prefer builtin packages.
> -                              (package-disabled-p p1 v1)
> -                              (not (package-disabled-p p2 v2))))))))
> +  (let ((pkg-descs (cdr (assq pkg-name package-alist))))
>      ;; Check if PACKAGE is available in `package-alist'.
>      (while
>          (when pkg-descs
>
>
> On Sun, Jun 4, 2023 at 12:06 AM Stefan Monnier <monnier@iro.umontreal.ca>
> wrote:
>
>> >> Won't this work:
>> >>
>> >> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
>> >> index 01826da273d..7eb185e7042 100644
>> >> --- a/lisp/emacs-lisp/package.el
>> >> +++ b/lisp/emacs-lisp/package.el
>> >> @@ -927,7 +927,9 @@ package--get-activatable-pkg
>> >>                                (package-vc-p p2)
>> >>                                ;; Prefer builtin packages.
>> >>                                (package-disabled-p p1 v1)
>> >> -                              (not (package-disabled-p p2 v2))))))))
>> >> +                              (not (package-disabled-p p2 v2))
>> >> +                              ;; Prever newer packages
>> >> +                              (version-list-< v2 v1)))))))
>> >>      ;; Check if PACKAGE is available in `package-alist'.
>> >>      (while
>> >>          (when pkg-descs
>> >
>> > Ping?
>>
>> The packages should already be sorted by version in `package-alist`, so
>> at best this will paper over the problem.  This said, the `sort` call's
>> predicate looks weird indeed:
>>
>>                          (lambda (p1 p2)
>>                            (let ((v1 (package-desc-version p1))
>>                                  (v2 (package-desc-version p2)))
>>                              (or
>>                               ;; Prefer VC packages.
>>                               (package-vc-p p1)
>>                               (package-vc-p p2)
>>                               ;; Prefer builtin packages.
>>                               (package-disabled-p p1 v1)
>>                               (not (package-disabled-p p2 v2))))))))
>>
>> - If both p1 and p2 are VC, then it will return non-nil, so
>>   they're both "strictly less than" the other :-(
>> - If neither is VC nor disabled, we return non-nil, which again means
>> they're
>>    both "strictly less than" the other.
>> - The comment says "Prefer builtin packages" but the code checks
>>   `package-disabled-p`.
>>
>> And now that I look more carefully, maybe this `sort` is the culprit
>> after all, because it operates directly on the list contained in
>> `packages-alist`, modifying it in-place :-(
>>
>> I think we should *not* sort here: we should instead sort when we
>> populate `package-alist` (like we already do).  So the "Prefer VC" part
>> of the sorting should be moved to `package-process-define-package`
>> (and the `package-disabled-p` can be dropped because it's already taken
>> care of by the loop that follows the above code).
>>
>>
>>         Stefan
>>
>>
>
> --
> __________________________
>
> С уважением,
> Бойков Евгений Алексеевич
> сот. 8-924-202-25-65
> e-mail: artscan@list.ru
>


-- 
__________________________

С уважением,
Бойков Евгений Алексеевич
сот. 8-924-202-25-65
e-mail: artscan@list.ru

[-- Attachment #2: Type: text/html, Size: 13147 bytes --]