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 Евгений Бойков 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 > 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