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