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