bug#63338: 29.0.90; package-vc-install'ing the same package multiple times results in duplication in package-selected-packages

[Jimmy Wong <wyuenho@gmail.com>]

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

This is acceptable to me
On 13 May 2023 at 6:18 PM +0100, Philip Kaludercic <philipk@posteo.net>, wrote:
> ping?
>
> Philip Kaludercic <philipk@posteo.net> writes:
>
> > Jimmy Wong <wyuenho@gmail.com> writes:
> >
> > > I don’t think you should dedup the variable that could have been
> > > modified by something else such as package-install. This may make
> > > debugging harder should package.el itself introduce a bug that
> > > duplicates pacakages in the variable. How about just using good old
> > > add-to-list?
> >
> > The issue is that we want to go through package--save-selected-packages,
> > that is given a new value to assign to `package-selected-packages'.
> > An otherwise, I my understanding is that add-to-list is not conventional
> > in executed code.
> >
> > The alternative is to check for duplicates before invoking the function:
> >
> > diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> > index e9794eac783..b967aaa3d4d 100644
> > --- a/lisp/emacs-lisp/package-vc.el
> > +++ b/lisp/emacs-lisp/package-vc.el
> > @@ -507,9 +507,10 @@ package-vc--unpack-1
> > (package--reload-previously-loaded new-desc)))
> >
> > ;; Mark package as selected
> > - (package--save-selected-packages
> > - (cons (package-desc-name pkg-desc)
> > - package-selected-packages))
> > + (let ((name (package-desc-name pkg-desc)))
> > + (unless (memq name package-selected-packages)
> > + (package--save-selected-packages
> > + (cons name package-selected-packages))))
> > (package--quickstart-maybe-refresh)
> >
> > ;; Confirm that the installation was successful
> >
> >
> >
> > > On 8 May 2023 at 1:03 PM +0100, Eli Zaretskii <eliz@gnu.org>, wrote:
> > > > > Cc: 63338@debbugs.gnu.org
> > > > > From: Philip Kaludercic <philipk@posteo.net>
> > > > > Date: Mon, 08 May 2023 10:36:55 +0000
> > > > >
> > > > > Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:
> > > > >
> > > > > > Reproduction:
> > > > > >
> > > > > > 0. (setq custom-file (const user-emacs-directory "custom.el"))
> > > > > > 1. M-x package-vc-install company
> > > > > > 2. M-x package-vc-install company RET y
> > > > > > 3. C-x C-f ~/.emacs/custom.el
> > > > > > 4. Observe that `company` has been listed twice under
> > > > > > `package-selected-packages`.
> > > > > >
> > > > > > Expectation:
> > > > > >
> > > > > > Installing the same package twice should not result in its duplication
> > > > > > in `package-selected-packages`.
> > > > >
> > > > > An easy fix would be just to ensure that package-selected-packages is
> > > > > always deduplicated before assigning the value:
> > > >
> > > > This is OK for the emacs-29 branch, thanks.

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