GNU bug report logs - #74604
30.0.92; FR: M-x package-upgrade - offer an option to show a diff on upgrade

Previous Next

To reply to this bug, email your comments to 74604 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.92; FR: M-x package-upgrade - offer an option to show a diff
 on upgrade
Date: Fri, 29 Nov 2024 16:39:27 +0100

This is a feature request for the security wishlist. When upgrading
package it would be good to show a diff between the new and old package
files. Such an option could help performing review casually as part of
the upgrade process and may improve the security of the package
archives. More eyes would look at new package versions. This would make
it harder to inject malicious code either via the source repository or
via attacks on the package archives.

Message #8 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Daniel Mendler <mail <at> daniel-mendler.de>
Cc: 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: 30.0.92; FR: M-x package-upgrade - offer an option
 to show a diff on upgrade
Date: Sun, 01 Dec 2024 22:05:24 +0000

Daniel Mendler <mail <at> daniel-mendler.de> writes:

> This is a feature request for the security wishlist. When upgrading
> package it would be good to show a diff between the new and old package
> files. Such an option could help performing review casually as part of
> the upgrade process and may improve the security of the package
> archives. More eyes would look at new package versions. This would make
> it harder to inject malicious code either via the source repository or
> via attacks on the package archives.

That sounds like a good option to have!  I'll look into adding something
like this via a user option that adjusts how to confirm a package upgrade.

Note that package-vc has something similar with the
`package-vc-log-incoming' command.

Message #11 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Ship Mints <shipmints <at> gmail.com>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Daniel Mendler <mail <at> daniel-mendler.de>, 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: 30.0.92; FR: M-x package-upgrade - offer an option to
 show a diff on upgrade
Date: Sun, 1 Dec 2024 17:47:21 -0500

[Message part 1 (text/plain, inline)]

I like this idea, too. I spend a reasonable amount of time trying to
understand what people have changed and if it will affect me negatively
(the defensive part) or positively (for new features, user options,
deprecations). Showing a source-code diff may be a bit technical for some
users, though. I wonder if there could be either a link to a changelog, or
a way to encourage a changelog convention so one could be displayed for
users prior to a decision to update a package.

-Stephane

On Sun, Dec 1, 2024 at 5:06 PM Philip Kaludercic <philipk <at> posteo.net> wrote:

> Daniel Mendler <mail <at> daniel-mendler.de> writes:
>
> > This is a feature request for the security wishlist. When upgrading
> > package it would be good to show a diff between the new and old package
> > files. Such an option could help performing review casually as part of
> > the upgrade process and may improve the security of the package
> > archives. More eyes would look at new package versions. This would make
> > it harder to inject malicious code either via the source repository or
> > via attacks on the package archives.
>
> That sounds like a good option to have!  I'll look into adding something
> like this via a user option that adjusts how to confirm a package upgrade.
>
> Note that package-vc has something similar with the
> `package-vc-log-incoming' command.
>
>
>
>

[Message part 2 (text/html, inline)]

Message #14 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: 30.0.92; FR: M-x package-upgrade - offer an option
 to show a diff on upgrade
Date: Mon, 02 Dec 2024 00:12:15 +0100

Philip Kaludercic <philipk <at> posteo.net> writes:

> Daniel Mendler <mail <at> daniel-mendler.de> writes:
>
>> This is a feature request for the security wishlist. When upgrading
>> package it would be good to show a diff between the new and old package
>> files. Such an option could help performing review casually as part of
>> the upgrade process and may improve the security of the package
>> archives. More eyes would look at new package versions. This would make
>> it harder to inject malicious code either via the source repository or
>> via attacks on the package archives.
>
> That sounds like a good option to have!  I'll look into adding something
> like this via a user option that adjusts how to confirm a package upgrade.

Thanks! I am happy to test if you have a patch ready.

Daniel

Message #17 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Ship Mints <shipmints <at> gmail.com>
Cc: Daniel Mendler <mail <at> daniel-mendler.de>, 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: 30.0.92; FR: M-x package-upgrade - offer an option
 to show a diff on upgrade
Date: Mon, 02 Dec 2024 08:59:12 +0000

Ship Mints <shipmints <at> gmail.com> writes:

> I like this idea, too. I spend a reasonable amount of time trying to
> understand what people have changed and if it will affect me negatively
> (the defensive part) or positively (for new features, user options,
> deprecations). Showing a source-code diff may be a bit technical for some
> users, though. I wonder if there could be either a link to a changelog, or
> a way to encourage a changelog convention so one could be displayed for
> users prior to a decision to update a package.

Note that packages can distribute this information.  Currently, if a
tarball includes a "news" file, it will be displayed by
`describe-package.  IIRC no package archive generates these right now.
But if we implement a user option like that described above (or below?),
then we can add that as an option as well.

The main issue is that not all package maintainers ensure that there are
changelog/news sources that ELPA could use to provide this information.

> -Stephane
>
> On Sun, Dec 1, 2024 at 5:06 PM Philip Kaludercic <philipk <at> posteo.net> wrote:
>
>> Daniel Mendler <mail <at> daniel-mendler.de> writes:
>>
>> > This is a feature request for the security wishlist. When upgrading
>> > package it would be good to show a diff between the new and old package
>> > files. Such an option could help performing review casually as part of
>> > the upgrade process and may improve the security of the package
>> > archives. More eyes would look at new package versions. This would make
>> > it harder to inject malicious code either via the source repository or
>> > via attacks on the package archives.
>>
>> That sounds like a good option to have!  I'll look into adding something
>> like this via a user option that adjusts how to confirm a package upgrade.
>>
>> Note that package-vc has something similar with the
>> `package-vc-log-incoming' command.
>>
>>
>>
>>

Message #20 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Ship Mints <shipmints <at> gmail.com>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Daniel Mendler <mail <at> daniel-mendler.de>, 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: 30.0.92; FR: M-x package-upgrade - offer an option to
 show a diff on upgrade
Date: Mon, 2 Dec 2024 07:04:24 -0500

[Message part 1 (text/plain, inline)]

Isn't it the case that describe-package works only on installed packages,
not prospectively installed packages? To help determine the value/risk of a
package install or update, I'd think it better to show this in advance.
Daniel's diff suggestion is similar but more technical.

On Mon, Dec 2, 2024 at 3:59 AM Philip Kaludercic <philipk <at> posteo.net> wrote:

> Ship Mints <shipmints <at> gmail.com> writes:
>
> > I like this idea, too. I spend a reasonable amount of time trying to
> > understand what people have changed and if it will affect me negatively
> > (the defensive part) or positively (for new features, user options,
> > deprecations). Showing a source-code diff may be a bit technical for some
> > users, though. I wonder if there could be either a link to a changelog,
> or
> > a way to encourage a changelog convention so one could be displayed for
> > users prior to a decision to update a package.
>
> Note that packages can distribute this information.  Currently, if a
> tarball includes a "news" file, it will be displayed by
> `describe-package.  IIRC no package archive generates these right now.
> But if we implement a user option like that described above (or below?),
> then we can add that as an option as well.
>
> The main issue is that not all package maintainers ensure that there are
> changelog/news sources that ELPA could use to provide this information.
>
> > -Stephane
> >
> > On Sun, Dec 1, 2024 at 5:06 PM Philip Kaludercic <philipk <at> posteo.net>
> wrote:
> >
> >> Daniel Mendler <mail <at> daniel-mendler.de> writes:
> >>
> >> > This is a feature request for the security wishlist. When upgrading
> >> > package it would be good to show a diff between the new and old
> package
> >> > files. Such an option could help performing review casually as part of
> >> > the upgrade process and may improve the security of the package
> >> > archives. More eyes would look at new package versions. This would
> make
> >> > it harder to inject malicious code either via the source repository or
> >> > via attacks on the package archives.
> >>
> >> That sounds like a good option to have!  I'll look into adding something
> >> like this via a user option that adjusts how to confirm a package
> upgrade.
> >>
> >> Note that package-vc has something similar with the
> >> `package-vc-log-incoming' command.
> >>
> >>
> >>
> >>
>

[Message part 2 (text/html, inline)]

Message #23 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Ship Mints <shipmints <at> gmail.com>
Cc: Daniel Mendler <mail <at> daniel-mendler.de>, 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: 30.0.92; FR: M-x package-upgrade - offer an option
 to show a diff on upgrade
Date: Mon, 02 Dec 2024 12:18:06 +0000

Ship Mints <shipmints <at> gmail.com> writes:

> Isn't it the case that describe-package works only on installed packages,
> not prospectively installed packages? To help determine the value/risk of a
> package install or update, I'd think it better to show this in advance.
> Daniel's diff suggestion is similar but more technical.

describe-package (C-h p) works on all packages, but the news feature I
described wouldn't work as it uses a local file.  But that is not a
hard-constraint, we could serve news data as well.

I don't know how much sense it makes to present a diff when installing a
package.  News files are probably also not that interesting.  We could
provide a command like package-vc-checkout that just fetches the package
source and places it somewhere for the user to inspect.

> On Mon, Dec 2, 2024 at 3:59 AM Philip Kaludercic <philipk <at> posteo.net> wrote:
>
>> Ship Mints <shipmints <at> gmail.com> writes:
>>
>> > I like this idea, too. I spend a reasonable amount of time trying to
>> > understand what people have changed and if it will affect me negatively
>> > (the defensive part) or positively (for new features, user options,
>> > deprecations). Showing a source-code diff may be a bit technical for some
>> > users, though. I wonder if there could be either a link to a changelog,
>> or
>> > a way to encourage a changelog convention so one could be displayed for
>> > users prior to a decision to update a package.
>>
>> Note that packages can distribute this information.  Currently, if a
>> tarball includes a "news" file, it will be displayed by
>> `describe-package.  IIRC no package archive generates these right now.
>> But if we implement a user option like that described above (or below?),
>> then we can add that as an option as well.
>>
>> The main issue is that not all package maintainers ensure that there are
>> changelog/news sources that ELPA could use to provide this information.
>>
>> > -Stephane
>> >
>> > On Sun, Dec 1, 2024 at 5:06 PM Philip Kaludercic <philipk <at> posteo.net>
>> wrote:
>> >
>> >> Daniel Mendler <mail <at> daniel-mendler.de> writes:
>> >>
>> >> > This is a feature request for the security wishlist. When upgrading
>> >> > package it would be good to show a diff between the new and old
>> package
>> >> > files. Such an option could help performing review casually as part of
>> >> > the upgrade process and may improve the security of the package
>> >> > archives. More eyes would look at new package versions. This would
>> make
>> >> > it harder to inject malicious code either via the source repository or
>> >> > via attacks on the package archives.
>> >>
>> >> That sounds like a good option to have!  I'll look into adding something
>> >> like this via a user option that adjusts how to confirm a package
>> upgrade.
>> >>
>> >> Note that package-vc has something similar with the
>> >> `package-vc-log-incoming' command.
>> >>
>> >>
>> >>
>> >>
>>

Message #26 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: Ship Mints <shipmints <at> gmail.com>
Cc: Philip Kaludercic <philipk <at> posteo.net>, 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: 30.0.92; FR: M-x package-upgrade - offer an option
 to show a diff on upgrade
Date: Mon, 02 Dec 2024 13:25:22 +0100

Ship Mints <shipmints <at> gmail.com> writes:

> To help determine the value/risk of a
> package install or update, I'd think it better to show this in advance.
> Daniel's diff suggestion is similar but more technical.

I think your idea of adding an option to show the change log is good. It
would be nice to have a `package-upgrade-review' option which could be
set to `nil', `news' or to `diff'.

But I want to emphasize that your suggestion misses the security aspect.
Security is the main reason why I made the proposal. The goal is to make
it easier and more convenient for users (yes, users who are "technical"
and familiar with Elisp) to assess the safety of package upgrades and
possibly report any irregularities to the package archive maintainers.
While packages are commonly reviewed at the time of their inclusion in
package archives, this is often not the case later on.

My proposal does not address or affect the first time installation of a
package. At this time it doesn't make sense to show a "diff" and the
user must first check the package closely anyway.

Daniel

Message #29 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Howard Melman <hmelman <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#74604: 30.0.92;
 FR: M-x package-upgrade - offer an option to show a diff on upgrade
Date: Thu, 05 Dec 2024 17:42:08 -0500

Daniel Mendler via "Bug reports for GNU Emacs, the Swiss
army knife of text editors" <bug-gnu-emacs <at> gnu.org> writes:

> Ship Mints <shipmints <at> gmail.com> writes:
>
>> To help determine the value/risk of a
>> package install or update, I'd think it better to show this in advance.
>> Daniel's diff suggestion is similar but more technical.
>
> I think your idea of adding an option to show the change log is good. It
> would be nice to have a `package-upgrade-review' option which could be
> set to `nil', `news' or to `diff'.

There was a package called paradox which had more features
on the package UI.  It included a command
paradox-commit-list that opened a buffer showing one line
per commit with the commit message and a button that was a
link to the commits diff.  It bolded the commits since the
current installed version to make it easy to see the
changes.  It only worked on github hosted packages.  It
would be great to have this functionality in package.el
particularly if it worked on non-github hosted packages.

-- 

Howard

Message #32 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Ship Mints <shipmints <at> gmail.com>
Cc: Daniel Mendler <mail <at> daniel-mendler.de>,
 Philip Kaludercic <philipk <at> posteo.net>, 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: 30.0.92; FR: M-x package-upgrade - offer an option
 to show a diff on upgrade
Date: Wed, 15 Jan 2025 08:51:08 -0500

>>> This is a feature request for the security wishlist.  When upgrading
>>> package it would be good to show a diff between the new and old package
>>> files.

+1

>>> Such an option could help performing review casually as part of
>>> the upgrade process and may improve the security of the package
>>> archives.  More eyes would look at new package versions.  This would make
>>> it harder to inject malicious code either via the source repository or
>>> via attacks on the package archives.

In addition to improving security it would encourage users to become
familiar with the code, which is very much the driving force behind
a lot of Emacs's design.

>> That sounds like a good option to have!  I'll look into adding something
>> like this via a user option that adjusts how to confirm a package upgrade.

Maybe the UI could be a simple confirmation prompt, where "show diff" is
one of the options.

>> Note that package-vc has something similar with the
>> `package-vc-log-incoming' command.

[ Ideally the two could/should share some aspects (UI-wise or
  implementation-wise).  ]

> Showing a source-code diff may be a bit technical for some users,
> though. I wonder if there could be either a link to a changelog, or
> a way to encourage a changelog convention so one could be displayed
> for users prior to a decision to update a package.

The prompt could offer a choice of "just upgrade / show news /
show diff".

Currently, on the (Non)GNU ELPA side, there *is* a convention for
a changelog file.  This is used to create the "Recent NEWS" part of
release announcements (see
https://lists.gnu.org/archive/html/gnu-emacs-sources/2025-01/msg00027.html
for an example) and the "News" section on the package's web page (See
http://elpa.gnu.org/packages/ellama.html).  But:

- Many packages don't follow it (I try to shame the maintainers, but
  maybe too softly?
  See https://lists.gnu.org/archive/html/gnu-emacs-sources/2025-01/msg00024.html
  for an example).

- There is no convention to relate specific parts of the changelog
  to specific versions, so we just display the first N lines (for email
  announcements, this is arguably the right thing, since we don't know
  what is the reader's current version).

- There is even less of a convention to propagate that changelog info
  through the ELPA protocol (i.e. from elpa.gnu.org to the users's
  machines).

In any case, it sounds everybody likes the idea, so I hope Someone™ will
provide a patch soon!


        Stefan

Message #35 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Philip Kaludercic <philipk <at> posteo.net>, Ship Mints <shipmints <at> gmail.com>,
 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: 30.0.92; FR: M-x package-upgrade - offer an option
 to show a diff on upgrade
Date: Wed, 15 Jan 2025 15:17:13 +0100

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>>>> Such an option could help performing review casually as part of
>>>> the upgrade process and may improve the security of the package
>>>> archives.  More eyes would look at new package versions.  This would make
>>>> it harder to inject malicious code either via the source repository or
>>>> via attacks on the package archives.
>
> In addition to improving security it would encourage users to become
> familiar with the code, which is very much the driving force behind
> a lot of Emacs's design.

Yes, this is the point of the proposal.

>> Showing a source-code diff may be a bit technical for some users,
>> though. I wonder if there could be either a link to a changelog, or
>> a way to encourage a changelog convention so one could be displayed
>> for users prior to a decision to update a package.
>
> The prompt could offer a choice of "just upgrade / show news /
> show diff".

Good idea. I think I would also like to have a customization option
`package-upgrade-diff' where the behavior can be customized, since I
always want to see the diff even for my own packages to check if recent
changes have arrived.

If `package-upgrade-diff' is nil, the confirmation prompt could offer a
key to display the diff. A key could also be reserved to show the change
log in case it is present, but as I mentioned before in this bug report,
displaying the change log is not a security feature and the code as
"driving force" is hidden.

Daniel

Message #38 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Daniel Mendler <mail <at> daniel-mendler.de>, Ship Mints <shipmints <at> gmail.com>,
 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: 30.0.92; FR: M-x package-upgrade - offer an option
 to show a diff on upgrade
Date: Thu, 16 Jan 2025 09:05:03 +0000

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>>> Note that package-vc has something similar with the
>>> `package-vc-log-incoming' command.
>
> [ Ideally the two could/should share some aspects (UI-wise or
>   implementation-wise).  ]

Right now `package-vc-log-incoming' just re-uses `vc-log-incoming', so I
don't know how easy this would be without creating a pseudo-VC backend.

>> Showing a source-code diff may be a bit technical for some users,
>> though. I wonder if there could be either a link to a changelog, or
>> a way to encourage a changelog convention so one could be displayed
>> for users prior to a decision to update a package.
>
> The prompt could offer a choice of "just upgrade / show news /
> show diff".
>
> Currently, on the (Non)GNU ELPA side, there *is* a convention for
> a changelog file.  This is used to create the "Recent NEWS" part of
> release announcements (see
> https://lists.gnu.org/archive/html/gnu-emacs-sources/2025-01/msg00027.html
> for an example) and the "News" section on the package's web page (See
> http://elpa.gnu.org/packages/ellama.html).  But:
>
> - Many packages don't follow it (I try to shame the maintainers, but
>   maybe too softly?
>   See https://lists.gnu.org/archive/html/gnu-emacs-sources/2025-01/msg00024.html
>   for an example).
>
> - There is no convention to relate specific parts of the changelog
>   to specific versions, so we just display the first N lines (for email
>   announcements, this is arguably the right thing, since we don't know
>   what is the reader's current version).
>
> - There is even less of a convention to propagate that changelog info
>   through the ELPA protocol (i.e. from elpa.gnu.org to the users's
>   machines).

Package.el does show the contents of the "news" file in the
describe-package buffer, but we currently don't generate these on the
elpa side.

> In any case, it sounds everybody likes the idea, so I hope Someone™ will
> provide a patch soon!

I'd be glad to tackle this and a number of other package.el/elpa-related
issues that have been accumulating recently.  (I'm just submitting my
master's thesis in less than two weeks, so I a bit short on time.)

>
>         Stefan

Message #41 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH v1] package.el: Add diff display and confirmation for package
 upgrades (Bug#74604)
Date: Sun, 31 Aug 2025 12:20:39 +0900

Hello,

This patch implements a new feature for package upgrades, as discussed
in Bug#74604.

Summary:
- Shows diffs between the installed and the new version of a package
  before upgrading.
- Lets the user review changes and confirm whether to proceed.
- Supports both regular tarball packages and VC packages.
- New user options:
  - package-show-upgrade-diffs
  - package-upgrade-diff-exclude-packages
  - package-upgrade-diff-exclude-archives
  - package-vc-show-upgrade-diffs
  - package-vc-upgrade-diff-exclude-packages
- Updated documentation (doc/emacs/package.texi).
- Added entry to etc/NEWS.

Testing:
- Built Emacs from the patched branch.
- Verified interactive upgrade of both tarball and VC packages.
- Ran `make check` and confirmed tests pass.

See Bug#74604 for context.

Best regards,
Nobuyuki Kamimoto


---8<---cut here---8<---
From f1793d5c62b194c37a61d8d4d02b655dc8b714c3 Mon Sep 17 00:00:00 2001
From: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>
Date: Sun, 31 Aug 2025 12:08:50 +0900
Subject: [PATCH] Add diff display and confirmation for package upgrades

Show differences between current and new versions before upgrading
packages, allowing users to review changes and choose whether to
proceed. This applies to both regular packages and VC packages.

* lisp/emacs-lisp/package.el (package-show-upgrade-diffs)
(package-upgrade-diff-exclude-packages)
(package-upgrade-diff-exclude-archives): New user options to control
diff display for package upgrades.
(package--extract-tarball-to-temp, package--get-installed-package-dir)
(package--show-package-diff, package--confirm-tarball-upgrade): New
functions to handle diff display and confirmation for tarball upgrades.
(package-upgrade): Check if tarball package upgrade needs confirmation.
(package-menu--perform-transaction): Handle diff confirmation in menu.

* lisp/emacs-lisp/package-vc.el (package-vc-show-upgrade-diffs)
(package-vc-upgrade-diff-exclude-packages): New user options to control
diff display for VC package upgrades.
(package-vc--show-diff, package-vc--confirm-upgrade): New functions to
handle diff display and confirmation for VC package upgrades.
(package-vc-upgrade): Integrate diff confirmation into upgrade process.

* doc/emacs/package.texi (Package Installation): Document the new diff
display functionality for both regular and VC package upgrades,
including the new user options for controlling the behavior.

* etc/NEWS: Add announcement for the new package upgrade diff feature.
---
 doc/emacs/package.texi        |  37 ++++++++
 etc/NEWS                      |  16 ++++
 lisp/emacs-lisp/package-vc.el | 140 ++++++++++++++++++++++-------
 lisp/emacs-lisp/package.el    | 165 ++++++++++++++++++++++++++++++----
 4 files changed, 308 insertions(+), 50 deletions(-)

diff --git a/doc/emacs/package.texi b/doc/emacs/package.texi
index c29beea3b08..1f57910a207 100644
--- a/doc/emacs/package.texi
+++ b/doc/emacs/package.texi
@@ -389,6 +389,25 @@ Package Installation
 use these bulk commands if you want to update only a small number of
 built-in packages.

+@vindex package-show-upgrade-diffs
+@vindex package-upgrade-diff-exclude-packages
+@vindex package-upgrade-diff-exclude-archives
+  By default, Emacs shows a diff of the changes when upgrading
+packages, allowing you to review what has changed between the current
+and new version before proceeding.  This is controlled by the
+@code{package-show-upgrade-diffs} variable.  When non-@code{nil} (the
+default), package upgrades will display the differences and ask for
+confirmation before proceeding.  You can disable this behavior by
+setting @code{package-show-upgrade-diffs} to @code{nil}.
+
+  You can exclude specific packages or package archives from diff
+checking using @code{package-upgrade-diff-exclude-packages} and
+@code{package-upgrade-diff-exclude-archives}.  The first variable
+takes a list of package names (as symbols), while the second takes a
+list of archive names (as strings).  Packages or archives in these
+lists will upgrade without showing diffs, even when
+@code{package-show-upgrade-diffs} is non-@code{nil}.
+
 @cindex package requirements
   A package may @dfn{require} certain other packages to be installed,
 because it relies on functionality provided by them.  When Emacs
@@ -645,6 +664,24 @@ Fetching Package Sources
   Note that currently, built-in packages cannot be upgraded using
 @code{package-vc-install}.

+@vindex package-vc-show-upgrade-diffs
+@vindex package-vc-upgrade-diff-exclude-packages
+  Like regular package upgrades, VC package upgrades can also show
+diffs before proceeding.  This is controlled by the
+@code{package-vc-show-upgrade-diffs} variable, which operates
+independently of @code{package-show-upgrade-diffs}.  When
+non-@code{nil} (the default), upgrading VC packages will display the
+differences between the current and updated versions, allowing you to
+review the changes before confirming the upgrade.
+
+  You can exclude specific VC packages from diff checking by adding
+their names (as symbols) to 
@code{package-vc-upgrade-diff-exclude-packages}.
+Packages in this list will upgrade without showing diffs, even when
+@code{package-vc-show-upgrade-diffs} is non-@code{nil}.  This
+exclusion list is separate from 
@code{package-upgrade-diff-exclude-packages}
+to allow independent control over diff behavior for VC packages versus
+regular packages.
+
 @findex package-report-bug
 @findex package-vc-prepare-patch
   With the source checkout, you might want to reproduce a bug against
diff --git a/etc/NEWS b/etc/NEWS
index 8a139cb03ca..2fdbd1c291f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -74,6 +74,22 @@ done from early-init.el, such as adding to 
'package-directory-list'.
 ** 'prettify-symbols-mode' attempts to ignore undisplayable characters.
 Previously, such characters would be rendered as, e.g., white boxes.

++++
+** Package management now shows diffs before upgrades.
+Package upgrades can now display differences between the current and
+new version before proceeding.  This applies to both regular packages
+and VC packages installed from version control repositories.
+
+The behavior is controlled by these new user options:
+- 'package-show-upgrade-diffs': Enable/disable diff display for regular 
packages
+- 'package-vc-show-upgrade-diffs': Enable/disable diff display for VC 
packages
+- 'package-upgrade-diff-exclude-packages': Exclude specific packages 
from diff checking
+- 'package-upgrade-diff-exclude-archives': Exclude specific archives 
from diff checking
+- 'package-vc-upgrade-diff-exclude-packages': Exclude specific VC 
packages from diff checking
+
+When enabled (the default), users will see a diff buffer showing changes
+and can choose whether to proceed with or cancel the upgrade.
+
 +++
 ** 'standard-display-table' now has more extra slots.
 'standard-display-table' has been extended to allow specifying glyphs
diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 7433fce2d89..da9977b429f 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -83,6 +83,27 @@ package-vc-register-as-project
   :type 'boolean
   :version "30.1")

+(defcustom package-vc-show-upgrade-diffs t
+  "Non-nil means to show diffs before upgrading VC packages.
+This controls whether VC package upgrades show a diff buffer
+comparing the current and updated versions of the package before
+proceeding with the upgrade.  This is independent of the
+`package-show-upgrade-diffs' variable which controls diff
+display for regular package upgrades."
+  :type 'boolean
+  :group 'package-vc
+  :version "31.1")
+
+(defcustom package-vc-upgrade-diff-exclude-packages nil
+  "List of VC package names to exclude from upgrade diff checking.
+This list contains package names (as symbols) for which diff checking
+should be skipped when upgrading VC packages.  This is separate from
+`package-upgrade-diff-exclude-packages' to allow independent control
+of diff exclusions for VC packages versus regular package upgrades."
+  :type '(repeat symbol)
+  :group 'package-vc
+  :version "31.1")
+
 (defvar package-vc-selected-packages) ; pacify byte-compiler

 ;;;###autoload
@@ -729,6 +750,55 @@ package-vc-upgrade-all
 (declare-function vc-dir-prepare-status-buffer "vc-dir"
                   (bname dir backend &optional create-new))

+(defun package-vc--show-diff (old-dir new-dir)
+  "Show diff between OLD-DIR and NEW-DIR package directories.
+Return t if user confirms to continue, nil to cancel."
+  (let ((diff-buffer "*Package VC Diff*"))
+    (with-current-buffer (get-buffer-create diff-buffer)
+      (erase-buffer)
+      (let ((inhibit-read-only t)
+            (exit-status (call-process "diff" nil t nil "-ru" old-dir 
new-dir)))
+        (cond
+         ((eq exit-status 0)
+          (insert "No differences found between current and updated 
package versions.\n"))
+         ((eq exit-status 1)
+          ;; Normal case: differences found, they're already in buffer
+          nil)
+         (t
+          ;; Error case
+          (insert (format "Error running diff (exit status %d).\n" 
exit-status)))))
+      (diff-mode)
+      (read-only-mode 1)
+      (goto-char (point-min)))
+    (display-buffer diff-buffer)
+    (let ((response (yes-or-no-p "Show differences. Continue with 
upgrade? ")))
+      (kill-buffer diff-buffer)
+      response)))
+
+(defun package-vc--confirm-upgrade (pkg-desc)
+  "Show diff for VC package upgrade and ask for confirmation.
+Return t if user wants to proceed, nil otherwise."
+  ;; Check exclusion list first
+  (let ((package-name (package-desc-name pkg-desc)))
+    ;; If package-vc-show-upgrade-diffs is nil, or package is excluded, 
proceed without diffs
+    (if (or (not package-vc-show-upgrade-diffs)
+            (memq package-name package-vc-upgrade-diff-exclude-packages))
+        t  ; Skip diff checking, proceed with upgrade
+      (let* ((pkg-dir (package-desc-dir pkg-desc))
+             (temp-dir (make-temp-file "package-vc-diff-" t))
+             (pkg-spec (package-vc--desc->spec pkg-desc)))
+        ;; Delete temp directory so package-vc--clone can create and 
populate it
+        (delete-directory temp-dir)
+        (unwind-protect
+            (progn
+              ;; Clone the updated version to temp directory
+              (package-vc--clone pkg-desc pkg-spec temp-dir nil)
+              ;; Show diff and get user confirmation
+              (package-vc--show-diff pkg-dir temp-dir))
+          ;; Clean up temp directory
+          (when (file-exists-p temp-dir)
+            (delete-directory temp-dir t)))))))
+
 ;;;###autoload
 (defun package-vc-upgrade (pkg-desc)
   "Upgrade the package described by PKG-DESC from package's VC repository.
@@ -736,40 +806,42 @@ package-vc-upgrade
 This may fail if the local VCS state of the package conflicts
 with the remote repository state."
   (interactive (list (package-vc--read-package-desc "Upgrade VC 
package: " t)))
-  ;; HACK: To run `package-vc--unpack-1' after checking out the new
-  ;; revision, we insert a hook into `vc-post-command-functions', and
-  ;; remove it right after it ran.  To avoid running the hook multiple
-  ;; times or even for the wrong repository (as `vc-pull' is often
-  ;; asynchronous), we extract the relevant arguments using a pseudo
-  ;; filter for `vc-filter-command-function', executed only for the
-  ;; side effect, and store them in the lexical scope.  When the hook
-  ;; is run, we check if the arguments are the same (`eq') as the ones
-  ;; previously extracted, and only in that case will be call
-  ;; `package-vc--unpack-1'.  Ugh...
-  ;;
-  ;; If there is a better way to do this, it should be done.
-  (cl-assert (package-vc-p pkg-desc))
-  (letrec ((pkg-dir (package-desc-dir pkg-desc))
-           (vc-flags)
-           (vc-filter-command-function
-            (lambda (command file-or-list flags)
-              (setq vc-flags flags)
-              (list command file-or-list flags)))
-           (post-upgrade
-            (lambda (_command _file-or-list flags)
-              (when (and (file-equal-p pkg-dir default-directory)
-                         (eq flags vc-flags))
-                (unwind-protect
-                    (with-demoted-errors "Failed to activate: %S"
-                      (package-vc--unpack-1 pkg-desc pkg-dir))
-                  (remove-hook 'vc-post-command-functions 
post-upgrade))))))
-    (add-hook 'vc-post-command-functions post-upgrade)
-    (with-demoted-errors "Failed to fetch: %S"
-      (require 'vc-dir)
-      (with-current-buffer (vc-dir-prepare-status-buffer
-                            (format " *package-vc-dir: %s*" pkg-dir)
-                            pkg-dir (vc-responsible-backend pkg-dir))
-        (vc-pull)))))
+  ;; Check if user wants to see diff and confirm upgrade
+  (when (package-vc--confirm-upgrade pkg-desc)
+    ;; HACK: To run `package-vc--unpack-1' after checking out the new
+    ;; revision, we insert a hook into `vc-post-command-functions', and
+    ;; remove it right after it ran.  To avoid running the hook multiple
+    ;; times or even for the wrong repository (as `vc-pull' is often
+    ;; asynchronous), we extract the relevant arguments using a pseudo
+    ;; filter for `vc-filter-command-function', executed only for the
+    ;; side effect, and store them in the lexical scope.  When the hook
+    ;; is run, we check if the arguments are the same (`eq') as the ones
+    ;; previously extracted, and only in that case will be call
+    ;; `package-vc--unpack-1'.  Ugh...
+    ;;
+    ;; If there is a better way to do this, it should be done.
+    (cl-assert (package-vc-p pkg-desc))
+    (letrec ((pkg-dir (package-desc-dir pkg-desc))
+             (vc-flags)
+             (vc-filter-command-function
+              (lambda (command file-or-list flags)
+                (setq vc-flags flags)
+                (list command file-or-list flags)))
+             (post-upgrade
+              (lambda (_command _file-or-list flags)
+                (when (and (file-equal-p pkg-dir default-directory)
+                           (eq flags vc-flags))
+                  (unwind-protect
+                      (with-demoted-errors "Failed to activate: %S"
+                        (package-vc--unpack-1 pkg-desc pkg-dir))
+                    (remove-hook 'vc-post-command-functions 
post-upgrade))))))
+      (add-hook 'vc-post-command-functions post-upgrade)
+      (with-demoted-errors "Failed to fetch: %S"
+        (require 'vc-dir)
+        (with-current-buffer (vc-dir-prepare-status-buffer
+                              (format " *package-vc-dir: %s*" pkg-dir)
+                              pkg-dir (vc-responsible-backend pkg-dir))
+          (vc-pull))))))

 (defun package-vc--archives-initialize ()
   "Initialize package.el and fetch package specifications."
diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index ba9999c20e6..1d1266b7ede 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -450,6 +450,44 @@ package-archive-column-width
   :type 'natnum
   :version "28.1")

+(defcustom package-show-upgrade-diffs t
+  "Whether to show diffs before upgrading packages.
+When non-nil, package upgrades will display the differences between
+the current and new version before proceeding.  Users can review the
+changes and choose whether to continue with the upgrade.
+When nil, packages upgrade without showing diffs."
+  :type 'boolean
+  :group 'package
+  :version "31.1")
+
+(defcustom package-upgrade-diff-exclude-packages nil
+  "List of package names to exclude from diff checking during upgrades.
+When a package name is in this list, the upgrade will proceed
+without showing diffs, even if `package-show-upgrade-diffs' is non-nil.
+Package names should be specified as symbols.
+
+For example: \\='(emacs magit org-mode)
+
+This allows for fine-grained control over which packages show
+diffs during upgrades."
+  :type '(repeat symbol)
+  :group 'package
+  :version "31.1")
+
+(defcustom package-upgrade-diff-exclude-archives nil
+  "List of package archives to exclude from diff checking during upgrades.
+When a package's archive is in this list, the upgrade will proceed
+without showing diffs, even if `package-show-upgrade-diffs' is non-nil.
+Archive names should be specified as strings.
+
+For example: \\='(\"melpa-stable\" \"nongnu\")
+
+This allows for fine-grained control over which archives show
+diffs during package upgrades."
+  :type '(repeat string)
+  :group 'package
+  :version "31.1")
+
 
 ;;; `package-desc' object definition
 ;; This is the struct used internally to represent packages.
@@ -1019,6 +1057,77 @@ package--alist-to-plist-args

 (declare-function dired-get-marked-files "dired")

+(defun package--extract-tarball-to-temp (buffer pkg-desc temp-dir)
+  "Extract tarball from BUFFER to TEMP-DIR for PKG-DESC.
+Returns the directory where package was extracted."
+  (let ((pkg-dir (expand-file-name (package-desc-full-name pkg-desc) 
temp-dir)))
+    (make-directory temp-dir t)
+    (with-current-buffer buffer
+      (let ((default-directory temp-dir))
+        (package-untar-buffer (package-desc-full-name pkg-desc))))
+    pkg-dir))
+
+(defun package--get-installed-package-dir (pkg-desc)
+  "Get directory of installed PKG-DESC package."
+  (expand-file-name (package-desc-full-name pkg-desc) package-user-dir))
+
+(defun package--show-package-diff (old-dir new-dir)
+  "Show diff between OLD-DIR and NEW-DIR package directories.
+Return t if user confirms to continue, nil to cancel."
+  (let ((diff-buffer "*Package Diff*"))
+    (with-current-buffer (get-buffer-create diff-buffer)
+      (erase-buffer)
+      (let ((inhibit-read-only t)
+            (exit-status (call-process "diff" nil t nil "-ru" old-dir 
new-dir)))
+        (cond
+         ((eq exit-status 0)
+          (insert "No differences found between old and new package 
versions.\n"))
+         ((eq exit-status 1)
+          ;; Normal case: differences found, they're already in buffer
+          nil)
+         (t
+          ;; Error case
+          (insert (format "Error running diff (exit status %d).\n" 
exit-status)))))
+      (diff-mode)
+      (read-only-mode 1)
+      (goto-char (point-min)))
+    (display-buffer diff-buffer)
+    (let ((response (yes-or-no-p "Show differences. Continue with 
upgrade? ")))
+      (kill-buffer diff-buffer)
+      response)))
+
+(defun package--confirm-tarball-upgrade (pkg-desc new-pkg-desc)
+  "Show diff for tarball package upgrade and ask for confirmation.
+Return t if user wants to proceed, nil otherwise."
+  ;; Check exclusion lists first
+  (let ((package-name (package-desc-name new-pkg-desc))
+        (package-archive (package-desc-archive new-pkg-desc)))
+    ;; If package-show-upgrade-diffs is nil, or package/archive is 
excluded, proceed without diffs
+    (if (or (not package-show-upgrade-diffs)
+            (memq package-name package-upgrade-diff-exclude-packages)
+            (member package-archive package-upgrade-diff-exclude-archives))
+        t  ; Skip diff checking, proceed with upgrade
+      (let* ((temp-dir (make-temp-file "package-diff-" t))
+             (old-dir (package--get-installed-package-dir pkg-desc))
+             new-dir)
+        (unwind-protect
+            (progn
+              ;; Download and extract new version to temp directory
+              (let* ((location (package-archive-base new-pkg-desc))
+                     (file (concat (package-desc-full-name new-pkg-desc)
+                                   (package-desc-suffix new-pkg-desc))))
+                (package--with-response-buffer location :file file
+                  (setq new-dir (package--extract-tarball-to-temp
+                                 (current-buffer) new-pkg-desc temp-dir))))
+              ;; Show diff and get user confirmation
+              (if (file-exists-p old-dir)
+                  (package--show-package-diff old-dir new-dir)
+                ;; If no old version exists, just confirm
+                (yes-or-no-p "New package installation. Continue? ")))
+          ;; Clean up temp directory
+          (when (file-exists-p temp-dir)
+            (delete-directory temp-dir t)))))))
+
 (defun package-unpack (pkg-desc)
   "Install the contents of the current buffer as a package."
   (let* ((name (package-desc-name pkg-desc))
@@ -2290,16 +2399,31 @@ package-upgrade
                   (package--upgradeable-packages t) nil t))))
   (cl-check-type name symbol)
   (let* ((pkg-desc (cadr (assq name package-alist)))
-         (package-install-upgrade-built-in (not pkg-desc)))
+         (package-install-upgrade-built-in (not pkg-desc))
+         (new-pkg-desc (cadr (assq name package-archive-contents))))
     ;; `pkg-desc' will be nil when the package is an "active built-in".
     (if (and pkg-desc (package-vc-p pkg-desc))
         (package-vc-upgrade pkg-desc)
-      (when pkg-desc
-        (package-delete pkg-desc 'force 'dont-unselect))
-      (package-install name
-                       ;; An active built-in has never been "selected"
-                       ;; before.  Mark it as installed explicitly.
-                       (and pkg-desc 'dont-select)))))
+      ;; Check if this is a tarball package upgrade that needs diff 
confirmation
+      (if (and pkg-desc new-pkg-desc
+               (eq (package-desc-kind new-pkg-desc) 'tar))
+          ;; For tarball packages, show diff and ask for confirmation
+          (if (package--confirm-tarball-upgrade pkg-desc new-pkg-desc)
+              (progn
+                (package-delete pkg-desc 'force 'dont-unselect)
+                (package-install name
+                                 ;; An active built-in has never been 
"selected"
+                                 ;; before.  Mark it as installed 
explicitly.
+                                 (and pkg-desc 'dont-select)))
+            (message "Package upgrade cancelled"))
+        ;; For non-tarball packages, proceed with normal upgrade
+        (progn
+          (when pkg-desc
+            (package-delete pkg-desc 'force 'dont-unselect))
+          (package-install name
+                           ;; An active built-in has never been "selected"
+                           ;; before.  Mark it as installed explicitly.
+                           (and pkg-desc 'dont-select)))))))

 (defun package--upgradeable-packages (&optional include-builtins)
   ;; Initialize the package system to get the list of package
@@ -2688,16 +2812,16 @@ package-isolate
 in a clean environment."
   (interactive
    (cl-loop for p in (cl-loop for p in (package--alist) append (cdr p))
-        unless (package-built-in-p p)
-        collect (cons (package-desc-full-name p) p) into table
-        finally return
-        (list
+            unless (package-built-in-p p)
+            collect (cons (package-desc-full-name p) p) into table
+            finally return
+            (list
              (cl-loop for c in
                       (completing-read-multiple
                        "Packages to isolate: " table
                        nil t)
-                   collect (alist-get c table nil nil #'string=))
-                  current-prefix-arg)))
+                      collect (alist-get c table nil nil #'string=))
+             current-prefix-arg)))
   (let* ((name (concat "package-isolate-"
                        (mapconcat #'package-desc-full-name packages ",")))
          (all-packages (delete-consecutive-dups
@@ -4172,9 +4296,18 @@ package-menu--perform-transaction
                   (format status-format (incf i)))
             (force-mode-line-update)
             (redisplay 'force)
-            ;; Don't mark as selected, `package-menu-execute' already
-            ;; does that.
-            (package-install pkg 'dont-select))))
+            ;; Check if this is a tarball package upgrade and needs 
confirmation
+            (let* ((pkg-name (package-desc-name pkg))
+                   (old-pkg (cl-find-if (lambda (del-pkg)
+                                          (eq (package-desc-name 
del-pkg) pkg-name))
+                                        delete-list)))
+              (if (and old-pkg (eq (package-desc-kind pkg) 'tar))
+                  ;; This is a tarball upgrade - ask for confirmation
+                  (if (package--confirm-tarball-upgrade old-pkg pkg)
+                      (package-install pkg 'dont-select)
+                    (push (package-desc-full-name pkg) errors))
+                ;; Normal installation
+                (package-install pkg 'dont-select))))))
     (let ((package-menu--transaction-status ":Deleting"))
       (force-mode-line-update)
       (redisplay 'force)
-- 
2.43.0

Message #44 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>
Cc: Daniel Mendler <mail <at> daniel-mendler.de>, 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: [PATCH v1] package.el: Add diff display and
 confirmation for package upgrades (Bug#74604)
Date: Tue, 02 Sep 2025 15:38:42 +0000

Nobuyuki Kamimoto <kamimoto527 <at> gmail.com> writes:

> Hello,
>
> This patch implements a new feature for package upgrades, as discussed
> in Bug#74604.

Thanks!  I have added Daniel to the CCs as he was involved in the
discussion you reference, and I assume is interested in the discussion.

> Summary:
> - Shows diffs between the installed and the new version of a package
>    before upgrading.
> - Lets the user review changes and confirm whether to proceed.
> - Supports both regular tarball packages and VC packages.
> - New user options:
>    - package-show-upgrade-diffs
>    - package-upgrade-diff-exclude-packages
>    - package-upgrade-diff-exclude-archives
>    - package-vc-show-upgrade-diffs
>    - package-vc-upgrade-diff-exclude-packages

This is already implemented in package-vc, but in a different way.  With
`package-vc-log-incoming' you get a log of all the changes that will be
pulled in.  The interface is not exactly the same, but I would rather we
avoid duplicating the functionality in similar but inconsistent ways.

> - Updated documentation (doc/emacs/package.texi).
> - Added entry to etc/NEWS.
>
> Testing:
> - Built Emacs from the patched branch.
> - Verified interactive upgrade of both tarball and VC packages.
> - Ran `make check` and confirmed tests pass.
>
> See Bug#74604 for context.
>
> Best regards,
> Nobuyuki Kamimoto
>
>
> ---8<---cut here---8<---
>  From f1793d5c62b194c37a61d8d4d02b655dc8b714c3 Mon Sep 17 00:00:00 2001
> From: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>
> Date: Sun, 31 Aug 2025 12:08:50 +0900
> Subject: [PATCH] Add diff display and confirmation for package upgrades
>
> Show differences between current and new versions before upgrading
> packages, allowing users to review changes and choose whether to
> proceed. This applies to both regular packages and VC packages.
>
> * lisp/emacs-lisp/package.el (package-show-upgrade-diffs)
> (package-upgrade-diff-exclude-packages)
> (package-upgrade-diff-exclude-archives): New user options to control
> diff display for package upgrades.
> (package--extract-tarball-to-temp, package--get-installed-package-dir)
> (package--show-package-diff, package--confirm-tarball-upgrade): New
> functions to handle diff display and confirmation for tarball upgrades.
> (package-upgrade): Check if tarball package upgrade needs confirmation.
> (package-menu--perform-transaction): Handle diff confirmation in menu.
>
> * lisp/emacs-lisp/package-vc.el (package-vc-show-upgrade-diffs)
> (package-vc-upgrade-diff-exclude-packages): New user options to control
> diff display for VC package upgrades.
> (package-vc--show-diff, package-vc--confirm-upgrade): New functions to
> handle diff display and confirmation for VC package upgrades.
> (package-vc-upgrade): Integrate diff confirmation into upgrade process.
>
> * doc/emacs/package.texi (Package Installation): Document the new diff
> display functionality for both regular and VC package upgrades,
> including the new user options for controlling the behavior.
>
> * etc/NEWS: Add announcement for the new package upgrade diff feature.
> ---
>   doc/emacs/package.texi        |  37 ++++++++
>   etc/NEWS                      |  16 ++++
>   lisp/emacs-lisp/package-vc.el | 140 ++++++++++++++++++++++-------
>   lisp/emacs-lisp/package.el    | 165 ++++++++++++++++++++++++++++++----
>   4 files changed, 308 insertions(+), 50 deletions(-)

Just a general comment, your message appears to have a number of
non-breaking whitespaces (0xA0).  Can you try to attach your patch in
the future, to avoid these kinds of problems in the future?

>
> diff --git a/doc/emacs/package.texi b/doc/emacs/package.texi
> index c29beea3b08..1f57910a207 100644
> --- a/doc/emacs/package.texi
> +++ b/doc/emacs/package.texi
> @@ -389,6 +389,25 @@ Package Installation
>   use these bulk commands if you want to update only a small number of
>   built-in packages.
>
> +@vindex package-show-upgrade-diffs
> +@vindex package-upgrade-diff-exclude-packages
> +@vindex package-upgrade-diff-exclude-archives
> +  By default, Emacs shows a diff of the changes when upgrading
> +packages, allowing you to review what has changed between the current
> +and new version before proceeding.  This is controlled by the
> +@code{package-show-upgrade-diffs} variable.  When non-@code{nil} (the
                                     ^
                                     user-option

> +default), package upgrades will display the differences and ask for
> +confirmation before proceeding.  You can disable this behavior by
> +setting @code{package-show-upgrade-diffs} to @code{nil}.
> +
> +  You can exclude specific packages or package archives from diff
> +checking using @code{package-upgrade-diff-exclude-packages} and
> +@code{package-upgrade-diff-exclude-archives}.  The first variable
> +takes a list of package names (as symbols), while the second takes a
> +list of archive names (as strings).  Packages or archives in these
> +lists will upgrade without showing diffs, even when
> +@code{package-show-upgrade-diffs} is non-@code{nil}.
> +
>   @cindex package requirements
>     A package may @dfn{require} certain other packages to be installed,
>   because it relies on functionality provided by them.  When Emacs
> @@ -645,6 +664,24 @@ Fetching Package Sources
>     Note that currently, built-in packages cannot be upgraded using
>   @code{package-vc-install}.
>
> +@vindex package-vc-show-upgrade-diffs
> +@vindex package-vc-upgrade-diff-exclude-packages
> +  Like regular package upgrades, VC package upgrades can also show
> +diffs before proceeding.  This is controlled by the
> +@code{package-vc-show-upgrade-diffs} variable, which operates
> +independently of @code{package-show-upgrade-diffs}.  When
> +non-@code{nil} (the default), upgrading VC packages will display the
> +differences between the current and updated versions, allowing you to
> +review the changes before confirming the upgrade.
> +
> +  You can exclude specific VC packages from diff checking by adding
> +their names (as symbols) to 
> @code{package-vc-upgrade-diff-exclude-packages}.
> +Packages in this list will upgrade without showing diffs, even when
> +@code{package-vc-show-upgrade-diffs} is non-@code{nil}.  This
> +exclusion list is separate from 
> @code{package-upgrade-diff-exclude-packages}
> +to allow independent control over diff behavior for VC packages versus
> +regular packages.
> +
>   @findex package-report-bug
>   @findex package-vc-prepare-patch
>     With the source checkout, you might want to reproduce a bug against
>
> diff --git a/etc/NEWS b/etc/NEWS
> index 8a139cb03ca..2fdbd1c291f 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -74,6 +74,22 @@ done from early-init.el, such as adding to 
> 'package-directory-list'.
>   ** 'prettify-symbols-mode' attempts to ignore undisplayable characters.
>   Previously, such characters would be rendered as, e.g., white boxes.
>
> ++++
> +** Package management now shows diffs before upgrades.
> +Package upgrades can now display differences between the current and
> +new version before proceeding.  This applies to both regular packages
> +and VC packages installed from version control repositories.
> +
> +The behavior is controlled by these new user options:
> +- 'package-show-upgrade-diffs': Enable/disable diff display for regular 
> packages
> +- 'package-vc-show-upgrade-diffs': Enable/disable diff display for VC 
> packages
> +- 'package-upgrade-diff-exclude-packages': Exclude specific packages 
> from diff checking
> +- 'package-upgrade-diff-exclude-archives': Exclude specific archives 
> from diff checking
> +- 'package-vc-upgrade-diff-exclude-packages': Exclude specific VC 
> packages from diff checking
> +
> +When enabled (the default), users will see a diff buffer showing changes
> +and can choose whether to proceed with or cancel the upgrade.

This description is probably a bit too detailed for a NEWS entry.  I
would not go into the new user options is that great of a detail.

Also, if we decide to enable this by default (which I am not certain of
right now), the news entry should be more clear in what the change is
that will affect all users: For instance, it is not "can now display"
but "will now display".

> +
>   +++
>   ** 'standard-display-table' now has more extra slots.
>   'standard-display-table' has been extended to allow specifying glyphs
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index 7433fce2d89..da9977b429f 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -83,6 +83,27 @@ package-vc-register-as-project
>     :type 'boolean
>     :version "30.1")
>
> +(defcustom package-vc-show-upgrade-diffs t
> +  "Non-nil means to show diffs before upgrading VC packages.
> +This controls whether VC package upgrades show a diff buffer
> +comparing the current and updated versions of the package before
> +proceeding with the upgrade.  This is independent of the
> +`package-show-upgrade-diffs' variable which controls diff
> +display for regular package upgrades."
> +  :type 'boolean
> +  :group 'package-vc

(General: You don't need to specify the group, `defcustom' will by
default re-use the last group defined at macro-expansion time.)

> +  :version "31.1")
> +
> +(defcustom package-vc-upgrade-diff-exclude-packages nil
> +  "List of VC package names to exclude from upgrade diff checking.
> +This list contains package names (as symbols) for which diff checking
> +should be skipped when upgrading VC packages.  This is separate from
> +`package-upgrade-diff-exclude-packages' to allow independent control
> +of diff exclusions for VC packages versus regular package upgrades."
> +  :type '(repeat symbol)
> +  :group 'package-vc
> +  :version "31.1")
> +
>   (defvar package-vc-selected-packages) ; pacify byte-compiler
>
>   ;;;###autoload
> @@ -729,6 +750,55 @@ package-vc-upgrade-all
>   (declare-function vc-dir-prepare-status-buffer "vc-dir"
>                     (bname dir backend &optional create-new))
>
> +(defun package-vc--show-diff (old-dir new-dir)
> +  "Show diff between OLD-DIR and NEW-DIR package directories.
> +Return t if user confirms to continue, nil to cancel."
> +  (let ((diff-buffer "*Package VC Diff*"))
> +    (with-current-buffer (get-buffer-create diff-buffer)
> +      (erase-buffer)
> +      (let ((inhibit-read-only t)
> +            (exit-status (call-process "diff" nil t nil "-ru" old-dir 
> new-dir)))
> +        (cond
> +         ((eq exit-status 0)
> +          (insert "No differences found between current and updated 
> package versions.\n"))
> +         ((eq exit-status 1)
> +          ;; Normal case: differences found, they're already in buffer
> +          nil)
> +         (t
> +          ;; Error case
> +          (insert (format "Error running diff (exit status %d).\n" 
> exit-status)))))
> +      (diff-mode)
> +      (read-only-mode 1)
> +      (goto-char (point-min)))
> +    (display-buffer diff-buffer)
> +    (let ((response (yes-or-no-p "Show differences. Continue with 
> upgrade? ")))
> +      (kill-buffer diff-buffer)
> +      response)))
> +
> +(defun package-vc--confirm-upgrade (pkg-desc)
> +  "Show diff for VC package upgrade and ask for confirmation.
> +Return t if user wants to proceed, nil otherwise."
> +  ;; Check exclusion list first
> +  (let ((package-name (package-desc-name pkg-desc)))
> +    ;; If package-vc-show-upgrade-diffs is nil, or package is excluded, 
> proceed without diffs
> +    (if (or (not package-vc-show-upgrade-diffs)
> +            (memq package-name package-vc-upgrade-diff-exclude-packages))
> +        t  ; Skip diff checking, proceed with upgrade
> +      (let* ((pkg-dir (package-desc-dir pkg-desc))
> +             (temp-dir (make-temp-file "package-vc-diff-" t))
> +             (pkg-spec (package-vc--desc->spec pkg-desc)))
> +        ;; Delete temp directory so package-vc--clone can create and 
> populate it
> +        (delete-directory temp-dir)
> +        (unwind-protect
> +            (progn
> +              ;; Clone the updated version to temp directory
> +              (package-vc--clone pkg-desc pkg-spec temp-dir nil)
> +              ;; Show diff and get user confirmation
> +              (package-vc--show-diff pkg-dir temp-dir))
> +          ;; Clean up temp directory
> +          (when (file-exists-p temp-dir)
> +            (delete-directory temp-dir t)))))))
> +
>   ;;;###autoload
>   (defun package-vc-upgrade (pkg-desc)
>     "Upgrade the package described by PKG-DESC from package's VC repository.
> @@ -736,40 +806,42 @@ package-vc-upgrade
>   This may fail if the local VCS state of the package conflicts
>   with the remote repository state."
>     (interactive (list (package-vc--read-package-desc "Upgrade VC 
> package: " t)))
> -  ;; HACK: To run `package-vc--unpack-1' after checking out the new
> -  ;; revision, we insert a hook into `vc-post-command-functions', and
> -  ;; remove it right after it ran.  To avoid running the hook multiple
> -  ;; times or even for the wrong repository (as `vc-pull' is often
> -  ;; asynchronous), we extract the relevant arguments using a pseudo
> -  ;; filter for `vc-filter-command-function', executed only for the
> -  ;; side effect, and store them in the lexical scope.  When the hook
> -  ;; is run, we check if the arguments are the same (`eq') as the ones
> -  ;; previously extracted, and only in that case will be call
> -  ;; `package-vc--unpack-1'.  Ugh...
> -  ;;
> -  ;; If there is a better way to do this, it should be done.
> -  (cl-assert (package-vc-p pkg-desc))
> -  (letrec ((pkg-dir (package-desc-dir pkg-desc))
> -           (vc-flags)
> -           (vc-filter-command-function
> -            (lambda (command file-or-list flags)
> -              (setq vc-flags flags)
> -              (list command file-or-list flags)))
> -           (post-upgrade
> -            (lambda (_command _file-or-list flags)
> -              (when (and (file-equal-p pkg-dir default-directory)
> -                         (eq flags vc-flags))
> -                (unwind-protect
> -                    (with-demoted-errors "Failed to activate: %S"
> -                      (package-vc--unpack-1 pkg-desc pkg-dir))
> -                  (remove-hook 'vc-post-command-functions 
> post-upgrade))))))
> -    (add-hook 'vc-post-command-functions post-upgrade)
> -    (with-demoted-errors "Failed to fetch: %S"
> -      (require 'vc-dir)
> -      (with-current-buffer (vc-dir-prepare-status-buffer
> -                            (format " *package-vc-dir: %s*" pkg-dir)
> -                            pkg-dir (vc-responsible-backend pkg-dir))
> -        (vc-pull)))))
> +  ;; Check if user wants to see diff and confirm upgrade
> +  (when (package-vc--confirm-upgrade pkg-desc)
> +    ;; HACK: To run `package-vc--unpack-1' after checking out the new
> +    ;; revision, we insert a hook into `vc-post-command-functions', and
> +    ;; remove it right after it ran.  To avoid running the hook multiple
> +    ;; times or even for the wrong repository (as `vc-pull' is often
> +    ;; asynchronous), we extract the relevant arguments using a pseudo
> +    ;; filter for `vc-filter-command-function', executed only for the
> +    ;; side effect, and store them in the lexical scope.  When the hook
> +    ;; is run, we check if the arguments are the same (`eq') as the ones
> +    ;; previously extracted, and only in that case will be call
> +    ;; `package-vc--unpack-1'.  Ugh...
> +    ;;
> +    ;; If there is a better way to do this, it should be done.
> +    (cl-assert (package-vc-p pkg-desc))
> +    (letrec ((pkg-dir (package-desc-dir pkg-desc))
> +             (vc-flags)
> +             (vc-filter-command-function
> +              (lambda (command file-or-list flags)
> +                (setq vc-flags flags)
> +                (list command file-or-list flags)))
> +             (post-upgrade
> +              (lambda (_command _file-or-list flags)
> +                (when (and (file-equal-p pkg-dir default-directory)
> +                           (eq flags vc-flags))
> +                  (unwind-protect
> +                      (with-demoted-errors "Failed to activate: %S"
> +                        (package-vc--unpack-1 pkg-desc pkg-dir))
> +                    (remove-hook 'vc-post-command-functions 
> post-upgrade))))))
> +      (add-hook 'vc-post-command-functions post-upgrade)
> +      (with-demoted-errors "Failed to fetch: %S"
> +        (require 'vc-dir)
> +        (with-current-buffer (vc-dir-prepare-status-buffer
> +                              (format " *package-vc-dir: %s*" pkg-dir)
> +                              pkg-dir (vc-responsible-backend pkg-dir))
> +          (vc-pull))))))

I hope it is fine that I will not comment on this file any more, because
I am not convinced of the approach (among other things having to make a
fresh clone of the entire repository every time the package is being
upgraded, which for larger packages like auctex can take a while).  That
is not to say that I am not open to UI suggestions based on
`package-vc-log-incoming', if you want to suggest anything like that.

>
>   (defun package-vc--archives-initialize ()
>     "Initialize package.el and fetch package specifications."
>
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index ba9999c20e6..1d1266b7ede 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -450,6 +450,44 @@ package-archive-column-width
>     :type 'natnum
>     :version "28.1")
>
> +(defcustom package-show-upgrade-diffs t
> +  "Whether to show diffs before upgrading packages.
> +When non-nil, package upgrades will display the differences between
> +the current and new version before proceeding.  Users can review the
> +changes and choose whether to continue with the upgrade.
> +When nil, packages upgrade without showing diffs."
> +  :type 'boolean
> +  :group 'package
> +  :version "31.1")
> +
> +(defcustom package-upgrade-diff-exclude-packages nil
> +  "List of package names to exclude from diff checking during upgrades.
> +When a package name is in this list, the upgrade will proceed
> +without showing diffs, even if `package-show-upgrade-diffs' is non-nil.
> +Package names should be specified as symbols.
> +
> +For example: \\='(emacs magit org-mode)
> +
> +This allows for fine-grained control over which packages show
> +diffs during upgrades."
> +  :type '(repeat symbol)
> +  :group 'package
> +  :version "31.1")
> +
> +(defcustom package-upgrade-diff-exclude-archives nil
> +  "List of package archives to exclude from diff checking during upgrades.
> +When a package's archive is in this list, the upgrade will proceed
> +without showing diffs, even if `package-show-upgrade-diffs' is non-nil.
> +Archive names should be specified as strings.
> +
> +For example: \\='(\"melpa-stable\" \"nongnu\")
> +
> +This allows for fine-grained control over which archives show
> +diffs during package upgrades."
> +  :type '(repeat string)
> +  :group 'package
> +  :version "31.1")

I wonder if instead of having multiple user options, it might be cleaner
to replace `package-show-upgrade-diffs' with a non-boolean user option
that can select the packages to trust/mistrust, sort of like how
`buffer-match-p' can select buffers.  So the user option instead a list
consisting of

  (package foo)
  (archive "bar")

entries or t, if you want to mistrust everything.

Also, shouldn't we also be able to configure what files to diff?  

Also also, IIRC another discussion from bug#74604 was to allow just
prompting the user what packages they want to upgrade.  Perhaps we
should orient ourselves on `save-some-buffers' and query the user for
each package if they want to upgrade it, keep it as is (perhaps even
remember this for the future) and/or show a diff.

> +
>   
>   ;;; `package-desc' object definition
>   ;; This is the struct used internally to represent packages.
> @@ -1019,6 +1057,77 @@ package--alist-to-plist-args
>
>   (declare-function dired-get-marked-files "dired")

> +(defun package--extract-tarball-to-temp (buffer pkg-desc temp-dir)
> +  "Extract tarball from BUFFER to TEMP-DIR for PKG-DESC.
> +Returns the directory where package was extracted."
> +  (let ((pkg-dir (expand-file-name (package-desc-full-name pkg-desc) 
> temp-dir)))
> +    (make-directory temp-dir t)
> +    (with-current-buffer buffer
> +      (let ((default-directory temp-dir))
> +        (package-untar-buffer (package-desc-full-name pkg-desc))))
> +    pkg-dir))
> +
> +(defun package--get-installed-package-dir (pkg-desc)
> +  "Get directory of installed PKG-DESC package."
> +  (expand-file-name (package-desc-full-name pkg-desc) package-user-dir))
> +
> +(defun package--show-package-diff (old-dir new-dir)
> +  "Show diff between OLD-DIR and NEW-DIR package directories.
> +Return t if user confirms to continue, nil to cancel."
> +  (let ((diff-buffer "*Package Diff*"))
> +    (with-current-buffer (get-buffer-create diff-buffer)
> +      (erase-buffer)
> +      (let ((inhibit-read-only t)
> +            (exit-status (call-process "diff" nil t nil "-ru" old-dir 
                                                           ^
                                                           generally, I
                                                           think it is
                                                           better to use
                                                           --long-options
                                                           to make the
                                                           code more readable.

> new-dir)))
> +        (cond
> +         ((eq exit-status 0)
> +          (insert "No differences found between old and new package 
> versions.\n"))
> +         ((eq exit-status 1)
> +          ;; Normal case: differences found, they're already in buffer
> +          nil)
> +         (t
> +          ;; Error case
> +          (insert (format "Error running diff (exit status %d).\n" 
> exit-status)))))
> +      (diff-mode)

Is there a reason you don't just use the `diff' function with NO-ASYNC
set to a non-nil value?

> +      (read-only-mode 1)

I think it is interesting to consider not setting the buffer to be
read-only by default, as it allows the user to manipulate the diff.
This also relates to the fact that the user might have made local
adjustment that they might want to keep between upgrades...

> +      (goto-char (point-min)))
> +    (display-buffer diff-buffer)
> +    (let ((response (yes-or-no-p "Show differences. Continue with
                                    ^
                                    What do you mean by show differences
                                    here?  It sounds imperative, but I
                                    am not sure if that is what you mean.
> upgrade? ")))
> +      (kill-buffer diff-buffer)
> +      response)))

You can use `prog1' here to avoid storing `response'.

> +
> +(defun package--confirm-tarball-upgrade (pkg-desc new-pkg-desc)
> +  "Show diff for tarball package upgrade and ask for confirmation.
> +Return t if user wants to proceed, nil otherwise."
> +  ;; Check exclusion lists first
> +  (let ((package-name (package-desc-name new-pkg-desc))
> +        (package-archive (package-desc-archive new-pkg-desc)))
> +    ;; If package-show-upgrade-diffs is nil, or package/archive is 
> excluded, proceed without diffs
> +    (if (or (not package-show-upgrade-diffs)
> +            (memq package-name package-upgrade-diff-exclude-packages)
> +            (member package-archive package-upgrade-diff-exclude-archives))
> +        t  ; Skip diff checking, proceed with upgrade
> +      (let* ((temp-dir (make-temp-file "package-diff-" t))
> +             (old-dir (package--get-installed-package-dir pkg-desc))
> +             new-dir)
> +        (unwind-protect
> +            (progn
> +              ;; Download and extract new version to temp directory
> +              (let* ((location (package-archive-base new-pkg-desc))
> +                     (file (concat (package-desc-full-name new-pkg-desc)
> +                                   (package-desc-suffix new-pkg-desc))))
> +                (package--with-response-buffer location :file file
> +                  (setq new-dir (package--extract-tarball-to-temp
> +                                 (current-buffer) new-pkg-desc temp-dir))))
> +              ;; Show diff and get user confirmation
> +              (if (file-exists-p old-dir)
> +                  (package--show-package-diff old-dir new-dir)
> +                ;; If no old version exists, just confirm
> +                (yes-or-no-p "New package installation. Continue? ")))
> +          ;; Clean up temp directory
> +          (when (file-exists-p temp-dir)
> +            (delete-directory temp-dir t)))))))

My preferred approach to solving this would be if we could split
`package-install-from-archive' or `package-unpack' into two, allowing us
to install the new package without compiling, generating autoloads, and
then to resume that part later on.  This would require some refactoring,
but would allow us to express the upgrade procedure as

1. Fetch the new source but not process it,

2. Compute the diff and present it to the user,

3. If they are fine with the changes, delete old package and finish
   initialising the package we just downloaded,

4. Otherwise we remove and forget about the code we had just downloaded
   and keep everything as is.

I imagine that this approach should keep the upgrade procedure more
uniform and setting aside changes caused by re-indenting code we factor
out of existing functionality, result in a simpler upgrade-logic.

> +
>   (defun package-unpack (pkg-desc)
>     "Install the contents of the current buffer as a package."
>     (let* ((name (package-desc-name pkg-desc))
> @@ -2290,16 +2399,31 @@ package-upgrade
>                     (package--upgradeable-packages t) nil t))))
>     (cl-check-type name symbol)
>     (let* ((pkg-desc (cadr (assq name package-alist)))
> -         (package-install-upgrade-built-in (not pkg-desc)))
> +         (package-install-upgrade-built-in (not pkg-desc))
> +         (new-pkg-desc (cadr (assq name package-archive-contents))))
>       ;; `pkg-desc' will be nil when the package is an "active built-in".
>       (if (and pkg-desc (package-vc-p pkg-desc))
>           (package-vc-upgrade pkg-desc)
> -      (when pkg-desc
> -        (package-delete pkg-desc 'force 'dont-unselect))
> -      (package-install name
> -                       ;; An active built-in has never been "selected"
> -                       ;; before.  Mark it as installed explicitly.
> -                       (and pkg-desc 'dont-select)))))
> +      ;; Check if this is a tarball package upgrade that needs diff 
> confirmation
> +      (if (and pkg-desc new-pkg-desc
> +               (eq (package-desc-kind new-pkg-desc) 'tar))
> +          ;; For tarball packages, show diff and ask for confirmation
> +          (if (package--confirm-tarball-upgrade pkg-desc new-pkg-desc)
> +              (progn
> +                (package-delete pkg-desc 'force 'dont-unselect)
> +                (package-install name
> +                                 ;; An active built-in has never been 
> "selected"
> +                                 ;; before.  Mark it as installed 
> explicitly.
> +                                 (and pkg-desc 'dont-select)))
> +            (message "Package upgrade cancelled"))
> +        ;; For non-tarball packages, proceed with normal upgrade
> +        (progn
> +          (when pkg-desc
> +            (package-delete pkg-desc 'force 'dont-unselect))
> +          (package-install name
> +                           ;; An active built-in has never been "selected"
> +                           ;; before.  Mark it as installed explicitly.
> +                           (and pkg-desc 'dont-select)))))))

Why shouldn't we prompt the user with a diff for non-tarball upgrades?  

>
>   (defun package--upgradeable-packages (&optional include-builtins)
>     ;; Initialize the package system to get the list of package
> @@ -2688,16 +2812,16 @@ package-isolate
>   in a clean environment."
>     (interactive
>      (cl-loop for p in (cl-loop for p in (package--alist) append (cdr p))
> -        unless (package-built-in-p p)
> -        collect (cons (package-desc-full-name p) p) into table
> -        finally return
> -        (list
> +            unless (package-built-in-p p)
> +            collect (cons (package-desc-full-name p) p) into table
> +            finally return
> +            (list
>                (cl-loop for c in
>                         (completing-read-multiple
>                          "Packages to isolate: " table
>                          nil t)
> -                   collect (alist-get c table nil nil #'string=))
> -                  current-prefix-arg)))
> +                      collect (alist-get c table nil nil #'string=))
> +             current-prefix-arg)))

These are unrelated changes, right?

>     (let* ((name (concat "package-isolate-"
>                          (mapconcat #'package-desc-full-name packages ",")))
>            (all-packages (delete-consecutive-dups
> @@ -4172,9 +4296,18 @@ package-menu--perform-transaction
>                     (format status-format (incf i)))
>               (force-mode-line-update)
>               (redisplay 'force)
> -            ;; Don't mark as selected, `package-menu-execute' already
> -            ;; does that.
> -            (package-install pkg 'dont-select))))
> +            ;; Check if this is a tarball package upgrade and needs 
> confirmation
> +            (let* ((pkg-name (package-desc-name pkg))
> +                   (old-pkg (cl-find-if (lambda (del-pkg)
> +                                          (eq (package-desc-name 
> del-pkg) pkg-name))
> +                                        delete-list)))
> +              (if (and old-pkg (eq (package-desc-kind pkg) 'tar))
> +                  ;; This is a tarball upgrade - ask for confirmation
> +                  (if (package--confirm-tarball-upgrade old-pkg pkg)
> +                      (package-install pkg 'dont-select)
> +                    (push (package-desc-full-name pkg) errors))
> +                ;; Normal installation
> +                (package-install pkg 'dont-select))))))
>       (let ((package-menu--transaction-status ":Deleting"))
>         (force-mode-line-update)
>         (redisplay 'force)

Final request, I have just pushed a branch called feature/package-revamp
where among other thing I want to split package.el into multiple smaller
files and have the package-menu re-use the package-upgrade logic.  If
everything goes as intended, this will be merged into master at some
point.  It would be great if you could try to re-base your work on that
branch, or at least keep it in mind.

-- 
Philip Kaludercic

Message #47 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>, 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: [PATCH v1] package.el: Add diff display and
 confirmation for package upgrades (Bug#74604)
Date: Tue, 02 Sep 2025 21:46:54 +0200

Philip Kaludercic <philipk <at> posteo.net> writes:

> Nobuyuki Kamimoto <kamimoto527 <at> gmail.com> writes:
>
>> Hello,
>>
>> This patch implements a new feature for package upgrades, as discussed
>> in Bug#74604.
>
> Thanks!  I have added Daniel to the CCs as he was involved in the
> discussion you reference, and I assume is interested in the discussion.

Philip, thanks for adding me. I wonder why I haven't received the
message earlier on, given that I've created bug#74604. I much appreciate
the work on this feature.

>> Summary:
>> - Shows diffs between the installed and the new version of a package
>>    before upgrading.
>> - Lets the user review changes and confirm whether to proceed.
>> - Supports both regular tarball packages and VC packages.
>> - New user options:
>>    - package-show-upgrade-diffs
>>    - package-upgrade-diff-exclude-packages
>>    - package-upgrade-diff-exclude-archives
>>    - package-vc-show-upgrade-diffs
>>    - package-vc-upgrade-diff-exclude-packages
>
> This is already implemented in package-vc, but in a different way.  With
> `package-vc-log-incoming' you get a log of all the changes that will be
> pulled in.  The interface is not exactly the same, but I would rather we
> avoid duplicating the functionality in similar but inconsistent ways.

I don't see how the package-vc-log-incoming functionality could be
reused here. package-vc-log-incoming simply calls vc-log-incoming, but
this new feature does not rely vc?

>> - Updated documentation (doc/emacs/package.texi).
>> - Added entry to etc/NEWS.
>>
>> Testing:
>> - Built Emacs from the patched branch.
>> - Verified interactive upgrade of both tarball and VC packages.
>> - Ran `make check` and confirmed tests pass.
>>
>> See Bug#74604 for context.
>>
>> Best regards,
>> Nobuyuki Kamimoto
>>
>>
>> ---8<---cut here---8<---
>>  From f1793d5c62b194c37a61d8d4d02b655dc8b714c3 Mon Sep 17 00:00:00 2001
>> From: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>
>> Date: Sun, 31 Aug 2025 12:08:50 +0900
>> Subject: [PATCH] Add diff display and confirmation for package upgrades
>>
>> Show differences between current and new versions before upgrading
>> packages, allowing users to review changes and choose whether to
>> proceed. This applies to both regular packages and VC packages.
>>
>> * lisp/emacs-lisp/package.el (package-show-upgrade-diffs)
>> (package-upgrade-diff-exclude-packages)
>> (package-upgrade-diff-exclude-archives): New user options to control
>> diff display for package upgrades.
>> (package--extract-tarball-to-temp, package--get-installed-package-dir)
>> (package--show-package-diff, package--confirm-tarball-upgrade): New
>> functions to handle diff display and confirmation for tarball upgrades.
>> (package-upgrade): Check if tarball package upgrade needs confirmation.
>> (package-menu--perform-transaction): Handle diff confirmation in menu.
>>
>> * lisp/emacs-lisp/package-vc.el (package-vc-show-upgrade-diffs)
>> (package-vc-upgrade-diff-exclude-packages): New user options to control
>> diff display for VC package upgrades.
>> (package-vc--show-diff, package-vc--confirm-upgrade): New functions to
>> handle diff display and confirmation for VC package upgrades.
>> (package-vc-upgrade): Integrate diff confirmation into upgrade process.
>>
>> * doc/emacs/package.texi (Package Installation): Document the new diff
>> display functionality for both regular and VC package upgrades,
>> including the new user options for controlling the behavior.
>>
>> * etc/NEWS: Add announcement for the new package upgrade diff feature.
>> ---
>>   doc/emacs/package.texi        |  37 ++++++++
>>   etc/NEWS                      |  16 ++++
>>   lisp/emacs-lisp/package-vc.el | 140 ++++++++++++++++++++++-------
>>   lisp/emacs-lisp/package.el    | 165 ++++++++++++++++++++++++++++++----
>>   4 files changed, 308 insertions(+), 50 deletions(-)
>
> Just a general comment, your message appears to have a number of
> non-breaking whitespaces (0xA0).  Can you try to attach your patch in
> the future, to avoid these kinds of problems in the future?
>
>>
>> diff --git a/doc/emacs/package.texi b/doc/emacs/package.texi
>> index c29beea3b08..1f57910a207 100644
>> --- a/doc/emacs/package.texi
>> +++ b/doc/emacs/package.texi
>> @@ -389,6 +389,25 @@ Package Installation
>>   use these bulk commands if you want to update only a small number of
>>   built-in packages.
>>
>> +@vindex package-show-upgrade-diffs
>> +@vindex package-upgrade-diff-exclude-packages
>> +@vindex package-upgrade-diff-exclude-archives
>> +  By default, Emacs shows a diff of the changes when upgrading
>> +packages, allowing you to review what has changed between the current
>> +and new version before proceeding.  This is controlled by the
>> +@code{package-show-upgrade-diffs} variable.  When non-@code{nil} (the
>                                      ^
>                                      user-option
>
>> +default), package upgrades will display the differences and ask for
>> +confirmation before proceeding.  You can disable this behavior by
>> +setting @code{package-show-upgrade-diffs} to @code{nil}.
>> +
>> +  You can exclude specific packages or package archives from diff
>> +checking using @code{package-upgrade-diff-exclude-packages} and
>> +@code{package-upgrade-diff-exclude-archives}.  The first variable
>> +takes a list of package names (as symbols), while the second takes a
>> +list of archive names (as strings).  Packages or archives in these
>> +lists will upgrade without showing diffs, even when
>> +@code{package-show-upgrade-diffs} is non-@code{nil}.
>> +
>>   @cindex package requirements
>>     A package may @dfn{require} certain other packages to be installed,
>>   because it relies on functionality provided by them.  When Emacs
>> @@ -645,6 +664,24 @@ Fetching Package Sources
>>     Note that currently, built-in packages cannot be upgraded using
>>   @code{package-vc-install}.
>>
>> +@vindex package-vc-show-upgrade-diffs
>> +@vindex package-vc-upgrade-diff-exclude-packages
>> +  Like regular package upgrades, VC package upgrades can also show
>> +diffs before proceeding.  This is controlled by the
>> +@code{package-vc-show-upgrade-diffs} variable, which operates
>> +independently of @code{package-show-upgrade-diffs}.  When
>> +non-@code{nil} (the default), upgrading VC packages will display the
>> +differences between the current and updated versions, allowing you to
>> +review the changes before confirming the upgrade.
>> +
>> +  You can exclude specific VC packages from diff checking by adding
>> +their names (as symbols) to 
>> @code{package-vc-upgrade-diff-exclude-packages}.
>> +Packages in this list will upgrade without showing diffs, even when
>> +@code{package-vc-show-upgrade-diffs} is non-@code{nil}.  This
>> +exclusion list is separate from 
>> @code{package-upgrade-diff-exclude-packages}
>> +to allow independent control over diff behavior for VC packages versus
>> +regular packages.
>> +
>>   @findex package-report-bug
>>   @findex package-vc-prepare-patch
>>     With the source checkout, you might want to reproduce a bug against
>>
>> diff --git a/etc/NEWS b/etc/NEWS
>> index 8a139cb03ca..2fdbd1c291f 100644
>> --- a/etc/NEWS
>> +++ b/etc/NEWS
>> @@ -74,6 +74,22 @@ done from early-init.el, such as adding to 
>> 'package-directory-list'.
>>   ** 'prettify-symbols-mode' attempts to ignore undisplayable characters.
>>   Previously, such characters would be rendered as, e.g., white boxes.
>>
>> ++++
>> +** Package management now shows diffs before upgrades.
>> +Package upgrades can now display differences between the current and
>> +new version before proceeding.  This applies to both regular packages
>> +and VC packages installed from version control repositories.
>> +
>> +The behavior is controlled by these new user options:
>> +- 'package-show-upgrade-diffs': Enable/disable diff display for regular 
>> packages
>> +- 'package-vc-show-upgrade-diffs': Enable/disable diff display for VC 
>> packages
>> +- 'package-upgrade-diff-exclude-packages': Exclude specific packages 
>> from diff checking
>> +- 'package-upgrade-diff-exclude-archives': Exclude specific archives 
>> from diff checking
>> +- 'package-vc-upgrade-diff-exclude-packages': Exclude specific VC 
>> packages from diff checking
>> +
>> +When enabled (the default), users will see a diff buffer showing changes
>> +and can choose whether to proceed with or cancel the upgrade.
>
> This description is probably a bit too detailed for a NEWS entry.  I
> would not go into the new user options is that great of a detail.
>
> Also, if we decide to enable this by default (which I am not certain of
> right now), the news entry should be more clear in what the change is
> that will affect all users: For instance, it is not "can now display"
> but "will now display".

Yes, the feature should not be enabled by default, only via user option.
We could consider adding an option to the package installation
confirmation. Instead of pressing y and n, the user could press d to see
the diff.

>> +
>>   +++
>>   ** 'standard-display-table' now has more extra slots.
>>   'standard-display-table' has been extended to allow specifying glyphs
>>
>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>> index 7433fce2d89..da9977b429f 100644
>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -83,6 +83,27 @@ package-vc-register-as-project
>>     :type 'boolean
>>     :version "30.1")
>>
>> +(defcustom package-vc-show-upgrade-diffs t
>> +  "Non-nil means to show diffs before upgrading VC packages.
>> +This controls whether VC package upgrades show a diff buffer
>> +comparing the current and updated versions of the package before
>> +proceeding with the upgrade.  This is independent of the
>> +`package-show-upgrade-diffs' variable which controls diff
>> +display for regular package upgrades."
>> +  :type 'boolean
>> +  :group 'package-vc
>
> (General: You don't need to specify the group, `defcustom' will by
> default re-use the last group defined at macro-expansion time.)
>
>> +  :version "31.1")
>> +
>> +(defcustom package-vc-upgrade-diff-exclude-packages nil
>> +  "List of VC package names to exclude from upgrade diff checking.
>> +This list contains package names (as symbols) for which diff checking
>> +should be skipped when upgrading VC packages.  This is separate from
>> +`package-upgrade-diff-exclude-packages' to allow independent control
>> +of diff exclusions for VC packages versus regular package upgrades."
>> +  :type '(repeat symbol)
>> +  :group 'package-vc
>> +  :version "31.1")
>> +
>>   (defvar package-vc-selected-packages) ; pacify byte-compiler
>>
>>   ;;;###autoload
>> @@ -729,6 +750,55 @@ package-vc-upgrade-all
>>   (declare-function vc-dir-prepare-status-buffer "vc-dir"
>>                     (bname dir backend &optional create-new))
>>
>> +(defun package-vc--show-diff (old-dir new-dir)
>> +  "Show diff between OLD-DIR and NEW-DIR package directories.
>> +Return t if user confirms to continue, nil to cancel."
>> +  (let ((diff-buffer "*Package VC Diff*"))
>> +    (with-current-buffer (get-buffer-create diff-buffer)
>> +      (erase-buffer)
>> +      (let ((inhibit-read-only t)
>> +            (exit-status (call-process "diff" nil t nil "-ru" old-dir 
>> new-dir)))
>> +        (cond
>> +         ((eq exit-status 0)
>> +          (insert "No differences found between current and updated 
>> package versions.\n"))
>> +         ((eq exit-status 1)
>> +          ;; Normal case: differences found, they're already in buffer
>> +          nil)
>> +         (t
>> +          ;; Error case
>> +          (insert (format "Error running diff (exit status %d).\n" 
>> exit-status)))))
>> +      (diff-mode)
>> +      (read-only-mode 1)
>> +      (goto-char (point-min)))
>> +    (display-buffer diff-buffer)
>> +    (let ((response (yes-or-no-p "Show differences. Continue with 
>> upgrade? ")))
>> +      (kill-buffer diff-buffer)
>> +      response)))
>> +
>> +(defun package-vc--confirm-upgrade (pkg-desc)
>> +  "Show diff for VC package upgrade and ask for confirmation.
>> +Return t if user wants to proceed, nil otherwise."
>> +  ;; Check exclusion list first
>> +  (let ((package-name (package-desc-name pkg-desc)))
>> +    ;; If package-vc-show-upgrade-diffs is nil, or package is excluded, 
>> proceed without diffs
>> +    (if (or (not package-vc-show-upgrade-diffs)
>> +            (memq package-name package-vc-upgrade-diff-exclude-packages))
>> +        t  ; Skip diff checking, proceed with upgrade
>> +      (let* ((pkg-dir (package-desc-dir pkg-desc))
>> +             (temp-dir (make-temp-file "package-vc-diff-" t))
>> +             (pkg-spec (package-vc--desc->spec pkg-desc)))
>> +        ;; Delete temp directory so package-vc--clone can create and 
>> populate it
>> +        (delete-directory temp-dir)
>> +        (unwind-protect
>> +            (progn
>> +              ;; Clone the updated version to temp directory
>> +              (package-vc--clone pkg-desc pkg-spec temp-dir nil)
>> +              ;; Show diff and get user confirmation
>> +              (package-vc--show-diff pkg-dir temp-dir))
>> +          ;; Clean up temp directory
>> +          (when (file-exists-p temp-dir)
>> +            (delete-directory temp-dir t)))))))
>> +
>>   ;;;###autoload
>>   (defun package-vc-upgrade (pkg-desc)
>>     "Upgrade the package described by PKG-DESC from package's VC repository.
>> @@ -736,40 +806,42 @@ package-vc-upgrade
>>   This may fail if the local VCS state of the package conflicts
>>   with the remote repository state."
>>     (interactive (list (package-vc--read-package-desc "Upgrade VC 
>> package: " t)))
>> -  ;; HACK: To run `package-vc--unpack-1' after checking out the new
>> -  ;; revision, we insert a hook into `vc-post-command-functions', and
>> -  ;; remove it right after it ran.  To avoid running the hook multiple
>> -  ;; times or even for the wrong repository (as `vc-pull' is often
>> -  ;; asynchronous), we extract the relevant arguments using a pseudo
>> -  ;; filter for `vc-filter-command-function', executed only for the
>> -  ;; side effect, and store them in the lexical scope.  When the hook
>> -  ;; is run, we check if the arguments are the same (`eq') as the ones
>> -  ;; previously extracted, and only in that case will be call
>> -  ;; `package-vc--unpack-1'.  Ugh...
>> -  ;;
>> -  ;; If there is a better way to do this, it should be done.
>> -  (cl-assert (package-vc-p pkg-desc))
>> -  (letrec ((pkg-dir (package-desc-dir pkg-desc))
>> -           (vc-flags)
>> -           (vc-filter-command-function
>> -            (lambda (command file-or-list flags)
>> -              (setq vc-flags flags)
>> -              (list command file-or-list flags)))
>> -           (post-upgrade
>> -            (lambda (_command _file-or-list flags)
>> -              (when (and (file-equal-p pkg-dir default-directory)
>> -                         (eq flags vc-flags))
>> -                (unwind-protect
>> -                    (with-demoted-errors "Failed to activate: %S"
>> -                      (package-vc--unpack-1 pkg-desc pkg-dir))
>> -                  (remove-hook 'vc-post-command-functions 
>> post-upgrade))))))
>> -    (add-hook 'vc-post-command-functions post-upgrade)
>> -    (with-demoted-errors "Failed to fetch: %S"
>> -      (require 'vc-dir)
>> -      (with-current-buffer (vc-dir-prepare-status-buffer
>> -                            (format " *package-vc-dir: %s*" pkg-dir)
>> -                            pkg-dir (vc-responsible-backend pkg-dir))
>> -        (vc-pull)))))
>> +  ;; Check if user wants to see diff and confirm upgrade
>> +  (when (package-vc--confirm-upgrade pkg-desc)
>> +    ;; HACK: To run `package-vc--unpack-1' after checking out the new
>> +    ;; revision, we insert a hook into `vc-post-command-functions', and
>> +    ;; remove it right after it ran.  To avoid running the hook multiple
>> +    ;; times or even for the wrong repository (as `vc-pull' is often
>> +    ;; asynchronous), we extract the relevant arguments using a pseudo
>> +    ;; filter for `vc-filter-command-function', executed only for the
>> +    ;; side effect, and store them in the lexical scope.  When the hook
>> +    ;; is run, we check if the arguments are the same (`eq') as the ones
>> +    ;; previously extracted, and only in that case will be call
>> +    ;; `package-vc--unpack-1'.  Ugh...
>> +    ;;
>> +    ;; If there is a better way to do this, it should be done.
>> +    (cl-assert (package-vc-p pkg-desc))
>> +    (letrec ((pkg-dir (package-desc-dir pkg-desc))
>> +             (vc-flags)
>> +             (vc-filter-command-function
>> +              (lambda (command file-or-list flags)
>> +                (setq vc-flags flags)
>> +                (list command file-or-list flags)))
>> +             (post-upgrade
>> +              (lambda (_command _file-or-list flags)
>> +                (when (and (file-equal-p pkg-dir default-directory)
>> +                           (eq flags vc-flags))
>> +                  (unwind-protect
>> +                      (with-demoted-errors "Failed to activate: %S"
>> +                        (package-vc--unpack-1 pkg-desc pkg-dir))
>> +                    (remove-hook 'vc-post-command-functions 
>> post-upgrade))))))
>> +      (add-hook 'vc-post-command-functions post-upgrade)
>> +      (with-demoted-errors "Failed to fetch: %S"
>> +        (require 'vc-dir)
>> +        (with-current-buffer (vc-dir-prepare-status-buffer
>> +                              (format " *package-vc-dir: %s*" pkg-dir)
>> +                              pkg-dir (vc-responsible-backend pkg-dir))
>> +          (vc-pull))))))
>
> I hope it is fine that I will not comment on this file any more, because
> I am not convinced of the approach (among other things having to make a
> fresh clone of the entire repository every time the package is being
> upgraded, which for larger packages like auctex can take a while).  That
> is not to say that I am not open to UI suggestions based on
> `package-vc-log-incoming', if you want to suggest anything like that.

Do I understand correctly that for package-vc we don't need a new
addition at all since one can use package-vc-log-incoming?

>>
>>   (defun package-vc--archives-initialize ()
>>     "Initialize package.el and fetch package specifications."
>>
>> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
>> index ba9999c20e6..1d1266b7ede 100644
>> --- a/lisp/emacs-lisp/package.el
>> +++ b/lisp/emacs-lisp/package.el
>> @@ -450,6 +450,44 @@ package-archive-column-width
>>     :type 'natnum
>>     :version "28.1")
>>
>> +(defcustom package-show-upgrade-diffs t
>> +  "Whether to show diffs before upgrading packages.
>> +When non-nil, package upgrades will display the differences between
>> +the current and new version before proceeding.  Users can review the
>> +changes and choose whether to continue with the upgrade.
>> +When nil, packages upgrade without showing diffs."
>> +  :type 'boolean
>> +  :group 'package
>> +  :version "31.1")
>> +
>> +(defcustom package-upgrade-diff-exclude-packages nil
>> +  "List of package names to exclude from diff checking during upgrades.
>> +When a package name is in this list, the upgrade will proceed
>> +without showing diffs, even if `package-show-upgrade-diffs' is non-nil.
>> +Package names should be specified as symbols.
>> +
>> +For example: \\='(emacs magit org-mode)
>> +
>> +This allows for fine-grained control over which packages show
>> +diffs during upgrades."
>> +  :type '(repeat symbol)
>> +  :group 'package
>> +  :version "31.1")
>> +
>> +(defcustom package-upgrade-diff-exclude-archives nil
>> +  "List of package archives to exclude from diff checking during upgrades.
>> +When a package's archive is in this list, the upgrade will proceed
>> +without showing diffs, even if `package-show-upgrade-diffs' is non-nil.
>> +Archive names should be specified as strings.
>> +
>> +For example: \\='(\"melpa-stable\" \"nongnu\")
>> +
>> +This allows for fine-grained control over which archives show
>> +diffs during package upgrades."
>> +  :type '(repeat string)
>> +  :group 'package
>> +  :version "31.1")
>
> I wonder if instead of having multiple user options, it might be cleaner
> to replace `package-show-upgrade-diffs' with a non-boolean user option
> that can select the packages to trust/mistrust, sort of like how
> `buffer-match-p' can select buffers.  So the user option instead a list
> consisting of
>
>   (package foo)
>   (archive "bar")
>
> entries or t, if you want to mistrust everything.

Sounds good to me.

> Also, shouldn't we also be able to configure what files to diff?

Such an option might be good to have, but my preference would be to
always see the whole diff, even if potentially "harmless" files are
added. I would also mistrust everything. So from my perspective, more
rigid and secure defaults would work too.

> Also also, IIRC another discussion from bug#74604 was to allow just
> prompting the user what packages they want to upgrade.  Perhaps we
> should orient ourselves on `save-some-buffers' and query the user for
> each package if they want to upgrade it, keep it as is (perhaps even
> remember this for the future) and/or show a diff.
>
>> +
>>   
>>   ;;; `package-desc' object definition
>>   ;; This is the struct used internally to represent packages.
>> @@ -1019,6 +1057,77 @@ package--alist-to-plist-args
>>
>>   (declare-function dired-get-marked-files "dired")
>
>> +(defun package--extract-tarball-to-temp (buffer pkg-desc temp-dir)
>> +  "Extract tarball from BUFFER to TEMP-DIR for PKG-DESC.
>> +Returns the directory where package was extracted."
>> +  (let ((pkg-dir (expand-file-name (package-desc-full-name pkg-desc) 
>> temp-dir)))
>> +    (make-directory temp-dir t)
>> +    (with-current-buffer buffer
>> +      (let ((default-directory temp-dir))
>> +        (package-untar-buffer (package-desc-full-name pkg-desc))))
>> +    pkg-dir))
>> +
>> +(defun package--get-installed-package-dir (pkg-desc)
>> +  "Get directory of installed PKG-DESC package."
>> +  (expand-file-name (package-desc-full-name pkg-desc) package-user-dir))
>> +
>> +(defun package--show-package-diff (old-dir new-dir)
>> +  "Show diff between OLD-DIR and NEW-DIR package directories.
>> +Return t if user confirms to continue, nil to cancel."
>> +  (let ((diff-buffer "*Package Diff*"))
>> +    (with-current-buffer (get-buffer-create diff-buffer)
>> +      (erase-buffer)
>> +      (let ((inhibit-read-only t)
>> +            (exit-status (call-process "diff" nil t nil "-ru" old-dir 
>                                                            ^
>                                                            generally, I
>                                                            think it is
>                                                            better to use
>                                                            --long-options
>                                                            to make the
>                                                            code more readable.
>
>> new-dir)))
>> +        (cond
>> +         ((eq exit-status 0)
>> +          (insert "No differences found between old and new package 
>> versions.\n"))
>> +         ((eq exit-status 1)
>> +          ;; Normal case: differences found, they're already in buffer
>> +          nil)
>> +         (t
>> +          ;; Error case
>> +          (insert (format "Error running diff (exit status %d).\n" 
>> exit-status)))))
>> +      (diff-mode)
>
> Is there a reason you don't just use the `diff' function with NO-ASYNC
> set to a non-nil value?
>
>> +      (read-only-mode 1)
>
> I think it is interesting to consider not setting the buffer to be
> read-only by default, as it allows the user to manipulate the diff.
> This also relates to the fact that the user might have made local
> adjustment that they might want to keep between upgrades...

Isn't this orthogonal to the problem here - if you want to make local
changes, you should install the package from source or via package-vc.
If you install a package via the standard mechanism, the package should
be installed in the form provided by the archive and therefore a
read-only diff should be fine.

>> +      (goto-char (point-min)))
>> +    (display-buffer diff-buffer)
>> +    (let ((response (yes-or-no-p "Show differences. Continue with
>                                     ^
>                                     What do you mean by show differences
>                                     here?  It sounds imperative, but I
>                                     am not sure if that is what you mean.
>> upgrade? ")))
>> +      (kill-buffer diff-buffer)
>> +      response)))
>
> You can use `prog1' here to avoid storing `response'.
>
>> +
>> +(defun package--confirm-tarball-upgrade (pkg-desc new-pkg-desc)
>> +  "Show diff for tarball package upgrade and ask for confirmation.
>> +Return t if user wants to proceed, nil otherwise."
>> +  ;; Check exclusion lists first
>> +  (let ((package-name (package-desc-name new-pkg-desc))
>> +        (package-archive (package-desc-archive new-pkg-desc)))
>> +    ;; If package-show-upgrade-diffs is nil, or package/archive is 
>> excluded, proceed without diffs
>> +    (if (or (not package-show-upgrade-diffs)
>> +            (memq package-name package-upgrade-diff-exclude-packages)
>> +            (member package-archive package-upgrade-diff-exclude-archives))
>> +        t  ; Skip diff checking, proceed with upgrade
>> +      (let* ((temp-dir (make-temp-file "package-diff-" t))
>> +             (old-dir (package--get-installed-package-dir pkg-desc))
>> +             new-dir)
>> +        (unwind-protect
>> +            (progn
>> +              ;; Download and extract new version to temp directory
>> +              (let* ((location (package-archive-base new-pkg-desc))
>> +                     (file (concat (package-desc-full-name new-pkg-desc)
>> +                                   (package-desc-suffix new-pkg-desc))))
>> +                (package--with-response-buffer location :file file
>> +                  (setq new-dir (package--extract-tarball-to-temp
>> +                                 (current-buffer) new-pkg-desc temp-dir))))
>> +              ;; Show diff and get user confirmation
>> +              (if (file-exists-p old-dir)
>> +                  (package--show-package-diff old-dir new-dir)
>> +                ;; If no old version exists, just confirm
>> +                (yes-or-no-p "New package installation. Continue? ")))
>> +          ;; Clean up temp directory
>> +          (when (file-exists-p temp-dir)
>> +            (delete-directory temp-dir t)))))))
>
> My preferred approach to solving this would be if we could split
> `package-install-from-archive' or `package-unpack' into two, allowing us
> to install the new package without compiling, generating autoloads, and
> then to resume that part later on.  This would require some refactoring,
> but would allow us to express the upgrade procedure as
>
> 1. Fetch the new source but not process it,
>
> 2. Compute the diff and present it to the user,
>
> 3. If they are fine with the changes, delete old package and finish
>    initialising the package we just downloaded,
>
> 4. Otherwise we remove and forget about the code we had just downloaded
>    and keep everything as is.
>
> I imagine that this approach should keep the upgrade procedure more
> uniform and setting aside changes caused by re-indenting code we factor
> out of existing functionality, result in a simpler upgrade-logic.

I see your point but I would generally be careful with performing "half"
of the installation before showing the diff. Until the user has reviewed
the diff, the new package should be considered malicious so it might be
better if it does not yet end up in the ~/.config/emacs/elpa/ directory.
Maybe a separate review directory could be used, e.g.,
~/.config/emacs/package-review/ and then the package could be moved to
the final destination in the end.

Generally regarding rollback, package.el is not robust right now (there
is another bug about that). The original package can stay deleted if
installation fails. This can happen due to multiple reasons - package
archive metadata outdated, package tarball not available, some other
failure during installation. In these case, Emacs can end up in a broken
state with a missing package.

>> +
>>   (defun package-unpack (pkg-desc)
>>     "Install the contents of the current buffer as a package."
>>     (let* ((name (package-desc-name pkg-desc))
>> @@ -2290,16 +2399,31 @@ package-upgrade
>>                     (package--upgradeable-packages t) nil t))))
>>     (cl-check-type name symbol)
>>     (let* ((pkg-desc (cadr (assq name package-alist)))
>> -         (package-install-upgrade-built-in (not pkg-desc)))
>> +         (package-install-upgrade-built-in (not pkg-desc))
>> +         (new-pkg-desc (cadr (assq name package-archive-contents))))
>>       ;; `pkg-desc' will be nil when the package is an "active built-in".
>>       (if (and pkg-desc (package-vc-p pkg-desc))
>>           (package-vc-upgrade pkg-desc)
>> -      (when pkg-desc
>> -        (package-delete pkg-desc 'force 'dont-unselect))
>> -      (package-install name
>> -                       ;; An active built-in has never been "selected"
>> -                       ;; before.  Mark it as installed explicitly.
>> -                       (and pkg-desc 'dont-select)))))
>> +      ;; Check if this is a tarball package upgrade that needs diff 
>> confirmation
>> +      (if (and pkg-desc new-pkg-desc
>> +               (eq (package-desc-kind new-pkg-desc) 'tar))
>> +          ;; For tarball packages, show diff and ask for confirmation
>> +          (if (package--confirm-tarball-upgrade pkg-desc new-pkg-desc)
>> +              (progn
>> +                (package-delete pkg-desc 'force 'dont-unselect)
>> +                (package-install name
>> +                                 ;; An active built-in has never been 
>> "selected"
>> +                                 ;; before.  Mark it as installed 
>> explicitly.
>> +                                 (and pkg-desc 'dont-select)))
>> +            (message "Package upgrade cancelled"))
>> +        ;; For non-tarball packages, proceed with normal upgrade
>> +        (progn
>> +          (when pkg-desc
>> +            (package-delete pkg-desc 'force 'dont-unselect))
>> +          (package-install name
>> +                           ;; An active built-in has never been "selected"
>> +                           ;; before.  Mark it as installed explicitly.
>> +                           (and pkg-desc 'dont-select)))))))
>
> Why shouldn't we prompt the user with a diff for non-tarball upgrades?  

+1

>>   (defun package--upgradeable-packages (&optional include-builtins)
>>     ;; Initialize the package system to get the list of package
>> @@ -2688,16 +2812,16 @@ package-isolate
>>   in a clean environment."
>>     (interactive
>>      (cl-loop for p in (cl-loop for p in (package--alist) append (cdr p))
>> -        unless (package-built-in-p p)
>> -        collect (cons (package-desc-full-name p) p) into table
>> -        finally return
>> -        (list
>> +            unless (package-built-in-p p)
>> +            collect (cons (package-desc-full-name p) p) into table
>> +            finally return
>> +            (list
>>                (cl-loop for c in
>>                         (completing-read-multiple
>>                          "Packages to isolate: " table
>>                          nil t)
>> -                   collect (alist-get c table nil nil #'string=))
>> -                  current-prefix-arg)))
>> +                      collect (alist-get c table nil nil #'string=))
>> +             current-prefix-arg)))
>
> These are unrelated changes, right?
>
>>     (let* ((name (concat "package-isolate-"
>>                          (mapconcat #'package-desc-full-name packages ",")))
>>            (all-packages (delete-consecutive-dups
>> @@ -4172,9 +4296,18 @@ package-menu--perform-transaction
>>                     (format status-format (incf i)))
>>               (force-mode-line-update)
>>               (redisplay 'force)
>> -            ;; Don't mark as selected, `package-menu-execute' already
>> -            ;; does that.
>> -            (package-install pkg 'dont-select))))
>> +            ;; Check if this is a tarball package upgrade and needs 
>> confirmation
>> +            (let* ((pkg-name (package-desc-name pkg))
>> +                   (old-pkg (cl-find-if (lambda (del-pkg)
>> +                                          (eq (package-desc-name 
>> del-pkg) pkg-name))
>> +                                        delete-list)))
>> +              (if (and old-pkg (eq (package-desc-kind pkg) 'tar))
>> +                  ;; This is a tarball upgrade - ask for confirmation
>> +                  (if (package--confirm-tarball-upgrade old-pkg pkg)
>> +                      (package-install pkg 'dont-select)
>> +                    (push (package-desc-full-name pkg) errors))
>> +                ;; Normal installation
>> +                (package-install pkg 'dont-select))))))
>>       (let ((package-menu--transaction-status ":Deleting"))
>>         (force-mode-line-update)
>>         (redisplay 'force)
>
> Final request, I have just pushed a branch called feature/package-revamp
> where among other thing I want to split package.el into multiple smaller
> files and have the package-menu re-use the package-upgrade logic.  If
> everything goes as intended, this will be merged into master at some
> point.  It would be great if you could try to re-base your work on that
> branch, or at least keep it in mind.

Message #50 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Daniel Mendler <mail <at> daniel-mendler.de>
Cc: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>, 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: [PATCH v1] package.el: Add diff display and
 confirmation for package upgrades (Bug#74604)
Date: Tue, 02 Sep 2025 23:08:54 +0000

Daniel Mendler <mail <at> daniel-mendler.de> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>> Nobuyuki Kamimoto <kamimoto527 <at> gmail.com> writes:
>>
>>> Hello,
>>>
>>> This patch implements a new feature for package upgrades, as discussed
>>> in Bug#74604.
>>
>> Thanks!  I have added Daniel to the CCs as he was involved in the
>> discussion you reference, and I assume is interested in the discussion.
>
> Philip, thanks for adding me. I wonder why I haven't received the
> message earlier on, given that I've created bug#74604. I much appreciate
> the work on this feature.

I am not sure either...

>>> Summary:
>>> - Shows diffs between the installed and the new version of a package
>>>    before upgrading.
>>> - Lets the user review changes and confirm whether to proceed.
>>> - Supports both regular tarball packages and VC packages.
>>> - New user options:
>>>    - package-show-upgrade-diffs
>>>    - package-upgrade-diff-exclude-packages
>>>    - package-upgrade-diff-exclude-archives
>>>    - package-vc-show-upgrade-diffs
>>>    - package-vc-upgrade-diff-exclude-packages
>>
>> This is already implemented in package-vc, but in a different way.  With
>> `package-vc-log-incoming' you get a log of all the changes that will be
>> pulled in.  The interface is not exactly the same, but I would rather we
>> avoid duplicating the functionality in similar but inconsistent ways.
>
> I don't see how the package-vc-log-incoming functionality could be
> reused here. package-vc-log-incoming simply calls vc-log-incoming, but
> this new feature does not rely vc?

To be clear, I was referencing the last two user options, as I hopefully
clarify below.


[...]

>>> ++++
>>> +** Package management now shows diffs before upgrades.
>>> +Package upgrades can now display differences between the current and
>>> +new version before proceeding.  This applies to both regular packages
>>> +and VC packages installed from version control repositories.
>>> +
>>> +The behavior is controlled by these new user options:
>>> +- 'package-show-upgrade-diffs': Enable/disable diff display for regular 
>>> packages
>>> +- 'package-vc-show-upgrade-diffs': Enable/disable diff display for VC 
>>> packages
>>> +- 'package-upgrade-diff-exclude-packages': Exclude specific packages 
>>> from diff checking
>>> +- 'package-upgrade-diff-exclude-archives': Exclude specific archives 
>>> from diff checking
>>> +- 'package-vc-upgrade-diff-exclude-packages': Exclude specific VC 
>>> packages from diff checking
>>> +
>>> +When enabled (the default), users will see a diff buffer showing changes
>>> +and can choose whether to proceed with or cancel the upgrade.
>>
>> This description is probably a bit too detailed for a NEWS entry.  I
>> would not go into the new user options is that great of a detail.
>>
>> Also, if we decide to enable this by default (which I am not certain of
>> right now), the news entry should be more clear in what the change is
>> that will affect all users: For instance, it is not "can now display"
>> but "will now display".
>
> Yes, the feature should not be enabled by default, only via user option.
> We could consider adding an option to the package installation
> confirmation. Instead of pressing y and n, the user could press d to see
> the diff.

1+

Another key like "n" or "c" to show the news/changes might also be
useful to have.  Perhaps the configuration we want is actually just to
specify what packages can be updated without any checks, and what
packages should be "approved".

[...]

>>
>> I hope it is fine that I will not comment on this file any more, because
>> I am not convinced of the approach (among other things having to make a
>> fresh clone of the entire repository every time the package is being
>> upgraded, which for larger packages like auctex can take a while).  That
>> is not to say that I am not open to UI suggestions based on
>> `package-vc-log-incoming', if you want to suggest anything like that.
>
> Do I understand correctly that for package-vc we don't need a new
> addition at all since one can use package-vc-log-incoming?

Right, that is at least my current understanding.  We can combine
`vc-log-incoming' with an upgrade procedure.  The interface will
probably not be the same, also because I don't think that
batch-upgrading VC packages is good practice.


[...]

>>> +This allows for fine-grained control over which archives show
>>> +diffs during package upgrades."
>>> +  :type '(repeat string)
>>> +  :group 'package
>>> +  :version "31.1")
>>
>> I wonder if instead of having multiple user options, it might be cleaner
>> to replace `package-show-upgrade-diffs' with a non-boolean user option
>> that can select the packages to trust/mistrust, sort of like how
>> `buffer-match-p' can select buffers.  So the user option instead a list
>> consisting of
>>
>>   (package foo)
>>   (archive "bar")
>>
>> entries or t, if you want to mistrust everything.
>
> Sounds good to me.
>
>> Also, shouldn't we also be able to configure what files to diff?
>
> Such an option might be good to have, but my preference would be to
> always see the whole diff, even if potentially "harmless" files are
> added. I would also mistrust everything. So from my perspective, more
> rigid and secure defaults would work too.

I agree.

[...]

>>> +      (read-only-mode 1)
>>
>> I think it is interesting to consider not setting the buffer to be
>> read-only by default, as it allows the user to manipulate the diff.
>> This also relates to the fact that the user might have made local
>> adjustment that they might want to keep between upgrades...
>
> Isn't this orthogonal to the problem here - if you want to make local
> changes, you should install the package from source or via package-vc.
> If you install a package via the standard mechanism, the package should
> be installed in the form provided by the archive and therefore a
> read-only diff should be fine.

You are right, this is just a general thought I haven't thought out
myself that we can ignore for now.

>>> +      (goto-char (point-min)))
>>> +    (display-buffer diff-buffer)
>>> +    (let ((response (yes-or-no-p "Show differences. Continue with
>>                                     ^
>>                                     What do you mean by show differences
>>                                     here?  It sounds imperative, but I
>>                                     am not sure if that is what you mean.
>>> upgrade? ")))
>>> +      (kill-buffer diff-buffer)
>>> +      response)))
>>
>> You can use `prog1' here to avoid storing `response'.
>>
>>> +
>>> +(defun package--confirm-tarball-upgrade (pkg-desc new-pkg-desc)
>>> +  "Show diff for tarball package upgrade and ask for confirmation.
>>> +Return t if user wants to proceed, nil otherwise."
>>> +  ;; Check exclusion lists first
>>> +  (let ((package-name (package-desc-name new-pkg-desc))
>>> +        (package-archive (package-desc-archive new-pkg-desc)))
>>> +    ;; If package-show-upgrade-diffs is nil, or package/archive is 
>>> excluded, proceed without diffs
>>> +    (if (or (not package-show-upgrade-diffs)
>>> +            (memq package-name package-upgrade-diff-exclude-packages)
>>> +            (member package-archive package-upgrade-diff-exclude-archives))
>>> +        t  ; Skip diff checking, proceed with upgrade
>>> +      (let* ((temp-dir (make-temp-file "package-diff-" t))
>>> +             (old-dir (package--get-installed-package-dir pkg-desc))
>>> +             new-dir)
>>> +        (unwind-protect
>>> +            (progn
>>> +              ;; Download and extract new version to temp directory
>>> +              (let* ((location (package-archive-base new-pkg-desc))
>>> +                     (file (concat (package-desc-full-name new-pkg-desc)
>>> +                                   (package-desc-suffix new-pkg-desc))))
>>> +                (package--with-response-buffer location :file file
>>> +                  (setq new-dir (package--extract-tarball-to-temp
>>> +                                 (current-buffer) new-pkg-desc temp-dir))))
>>> +              ;; Show diff and get user confirmation
>>> +              (if (file-exists-p old-dir)
>>> +                  (package--show-package-diff old-dir new-dir)
>>> +                ;; If no old version exists, just confirm
>>> +                (yes-or-no-p "New package installation. Continue? ")))
>>> +          ;; Clean up temp directory
>>> +          (when (file-exists-p temp-dir)
>>> +            (delete-directory temp-dir t)))))))
>>
>> My preferred approach to solving this would be if we could split
>> `package-install-from-archive' or `package-unpack' into two, allowing us
>> to install the new package without compiling, generating autoloads, and
>> then to resume that part later on.  This would require some refactoring,
>> but would allow us to express the upgrade procedure as
>>
>> 1. Fetch the new source but not process it,
>>
>> 2. Compute the diff and present it to the user,
>>
>> 3. If they are fine with the changes, delete old package and finish
>>    initialising the package we just downloaded,
>>
>> 4. Otherwise we remove and forget about the code we had just downloaded
>>    and keep everything as is.
>>
>> I imagine that this approach should keep the upgrade procedure more
>> uniform and setting aside changes caused by re-indenting code we factor
>> out of existing functionality, result in a simpler upgrade-logic.
>
> I see your point but I would generally be careful with performing "half"
> of the installation before showing the diff. Until the user has reviewed
> the diff, the new package should be considered malicious so it might be
> better if it does not yet end up in the ~/.config/emacs/elpa/ directory.
> Maybe a separate review directory could be used, e.g.,
> ~/.config/emacs/package-review/ and then the package could be moved to
> the final destination in the end.

You are right, that is important to keep in mind.  My main focus here is
that we should avoid downloading the package twice.  FWIW it should be
fine if the package is first cloned into /tmp/ and then copied over to
~/.config/emacs/elpa/ after being approved, or is your point with
.../package-review/ that the list of not-yet-reviewed should stay
persistent?

> Generally regarding rollback, package.el is not robust right now (there
> is another bug about that). The original package can stay deleted if
> installation fails. This can happen due to multiple reasons - package
> archive metadata outdated, package tarball not available, some other
> failure during installation. In these case, Emacs can end up in a broken
> state with a missing package.

That is true and something I want to improve upon independently of this
bug report (but we shouldn't make it worse either).

[...]

Message #53 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>, 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: [PATCH v1] package.el: Add diff display and
 confirmation for package upgrades (Bug#74604)
Date: Wed, 03 Sep 2025 11:24:37 +0200

>>> Also, if we decide to enable this by default (which I am not certain of
>>> right now), the news entry should be more clear in what the change is
>>> that will affect all users: For instance, it is not "can now display"
>>> but "will now display".
>>
>> Yes, the feature should not be enabled by default, only via user option.
>> We could consider adding an option to the package installation
>> confirmation. Instead of pressing y and n, the user could press d to see
>> the diff.
>
> 1+
>
> Another key like "n" or "c" to show the news/changes might also be
> useful to have.  Perhaps the configuration we want is actually just to
> specify what packages can be updated without any checks, and what
> packages should be "approved".

Yes, but as I mentioned I would use "paranoid" settings (display the
diff always for everything). To clarify the motivation - this would
offer some protection against all kinds of attacks, e.g., even a
manipulated package archive or build process, even if the package itself
and the git does not look suspicious. If anything suspicious stands out
during upgrade (or the diff is particularly large), a closer
investigation could be done on the repository directly. I hope if a
couple of experienced users enable this feature we would get robust
sanity checking of the entire supply chain, at least for widely
installed packages, but these are the ones which would affect most of
the users.

>>> I hope it is fine that I will not comment on this file any more, because
>>> I am not convinced of the approach (among other things having to make a
>>> fresh clone of the entire repository every time the package is being
>>> upgraded, which for larger packages like auctex can take a while).  That
>>> is not to say that I am not open to UI suggestions based on
>>> `package-vc-log-incoming', if you want to suggest anything like that.
>>
>> Do I understand correctly that for package-vc we don't need a new
>> addition at all since one can use package-vc-log-incoming?
>
> Right, that is at least my current understanding.  We can combine
> `vc-log-incoming' with an upgrade procedure.  The interface will
> probably not be the same, also because I don't think that
> batch-upgrading VC packages is good practice.

Agree.

>>> I imagine that this approach should keep the upgrade procedure more
>>> uniform and setting aside changes caused by re-indenting code we factor
>>> out of existing functionality, result in a simpler upgrade-logic.
>>
>> I see your point but I would generally be careful with performing "half"
>> of the installation before showing the diff. Until the user has reviewed
>> the diff, the new package should be considered malicious so it might be
>> better if it does not yet end up in the ~/.config/emacs/elpa/ directory.
>> Maybe a separate review directory could be used, e.g.,
>> ~/.config/emacs/package-review/ and then the package could be moved to
>> the final destination in the end.
>
> You are right, that is important to keep in mind.  My main focus here is
> that we should avoid downloading the package twice.  FWIW it should be
> fine if the package is first cloned into /tmp/ and then copied over to
> ~/.config/emacs/elpa/ after being approved, or is your point with
> .../package-review/ that the list of not-yet-reviewed should stay
> persistent?

Absolutely agree that we should avoid downloading and unpacking twice.
My idea was to remove the package after review and I proposed a
directory close to the emacs/elpa/ directory since then the directory
can be renamed. /tmp could be on a different FS, so it would involve
copying, but not that it matters. I hadn't thought about this, but maybe
it is not such a bad idea if the non-approved package stays around for
inspection in a separate directory. Otoh this would increase the risk
for accidental execution, in particular with macro expansion and code
execution and if we generally trust the content of the ~/.config/emacs/
directory, as could be configured by the trusted-content variable.

>> Generally regarding rollback, package.el is not robust right now (there
>> is another bug about that). The original package can stay deleted if
>> installation fails. This can happen due to multiple reasons - package
>> archive metadata outdated, package tarball not available, some other
>> failure during installation. In these case, Emacs can end up in a broken
>> state with a missing package.
>
> That is true and something I want to improve upon independently of this
> bug report (but we shouldn't make it worse either).

Thanks. Indeed :)

Daniel

Message #56 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>
To: Philip Kaludercic <philipk <at> posteo.net>,
 Daniel Mendler <mail <at> daniel-mendler.de>
Cc: 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: [PATCH v1] package.el: Add diff display and
 confirmation for package upgrades (Bug#74604)
Date: Sat, 13 Sep 2025 09:01:33 +0900

[Message part 1 (text/plain, inline)]

Hello Philip and Daniel,

Thank you for your detailed feedback on the initial patch. You raised 
excellent points about avoiding
duplication of existing VC functionality and improving the overall approach.

I've completely revised the implementation based on your suggestions:

Key Changes

For VC Packages:
- Now uses existing package-vc-log-incoming and vc-root-diff-incoming 
functions instead of creating custom
clone-based diff functionality
- Eliminates the expensive full repository cloning approach
- The 'd' option in the interactive prompt now shows both log and source 
diff using standard VC functions

Unified Configuration:
- Replaced multiple defcustom options with a single 
package-upgrade-confirmation-policy that accepts:
  - t (default): Show diffs and confirm for all packages
  - nil: Auto-upgrade without confirmation
  - List format like ((package magit) (archive "melpa")) for 
fine-grained control

Interactive Prompt System:
- Implements y/n/d/c interface (Yes/No/Diff/Changelog/Quit)
- Consistent between tarball and VC packages
- Uses existing VC infrastructure for VC packages

Addressing Your Concerns

- No VC functionality duplication: Now leverages existing 
vc-log-incoming/vc-root-diff-incoming
- Performance: No more expensive repository cloning for VC packages
- Configuration simplicity: Single unified option instead of multiple 
variables
- Standards compliance: Uses standard Emacs diff and VC functionality

Regarding feature/package-revamp

I'd be happy to test and adapt this work for the feature/package-revamp 
branch. Would you prefer me to:
1. Rebase this current patch against that branch, or
2. Wait for the branch to be merged and then adapt?

The revised patch is attached to avoid formatting issues with 
non-breaking spaces.

I believe this approach better aligns with Emacs conventions while 
providing the user interface improvements
discussed in Bug#74604.

Best regards,
Nobuyuki Kamimoto

On 2025/09/03 8:08, Philip Kaludercic wrote:
> Daniel Mendler <mail <at> daniel-mendler.de> writes:
>
>> Philip Kaludercic <philipk <at> posteo.net> writes:
>>
>>> Nobuyuki Kamimoto <kamimoto527 <at> gmail.com> writes:
>>>
>>>> Hello,
>>>>
>>>> This patch implements a new feature for package upgrades, as discussed
>>>> in Bug#74604.
>>> Thanks!  I have added Daniel to the CCs as he was involved in the
>>> discussion you reference, and I assume is interested in the discussion.
>> Philip, thanks for adding me. I wonder why I haven't received the
>> message earlier on, given that I've created bug#74604. I much appreciate
>> the work on this feature.
> I am not sure either...
>
>>>> Summary:
>>>> - Shows diffs between the installed and the new version of a package
>>>>     before upgrading.
>>>> - Lets the user review changes and confirm whether to proceed.
>>>> - Supports both regular tarball packages and VC packages.
>>>> - New user options:
>>>>     - package-show-upgrade-diffs
>>>>     - package-upgrade-diff-exclude-packages
>>>>     - package-upgrade-diff-exclude-archives
>>>>     - package-vc-show-upgrade-diffs
>>>>     - package-vc-upgrade-diff-exclude-packages
>>> This is already implemented in package-vc, but in a different way.  With
>>> `package-vc-log-incoming' you get a log of all the changes that will be
>>> pulled in.  The interface is not exactly the same, but I would rather we
>>> avoid duplicating the functionality in similar but inconsistent ways.
>> I don't see how the package-vc-log-incoming functionality could be
>> reused here. package-vc-log-incoming simply calls vc-log-incoming, but
>> this new feature does not rely vc?
> To be clear, I was referencing the last two user options, as I hopefully
> clarify below.
>
>
> [...]
>
>>>> ++++
>>>> +** Package management now shows diffs before upgrades.
>>>> +Package upgrades can now display differences between the current and
>>>> +new version before proceeding.  This applies to both regular packages
>>>> +and VC packages installed from version control repositories.
>>>> +
>>>> +The behavior is controlled by these new user options:
>>>> +- 'package-show-upgrade-diffs': Enable/disable diff display for regular
>>>> packages
>>>> +- 'package-vc-show-upgrade-diffs': Enable/disable diff display for VC
>>>> packages
>>>> +- 'package-upgrade-diff-exclude-packages': Exclude specific packages
>>>> from diff checking
>>>> +- 'package-upgrade-diff-exclude-archives': Exclude specific archives
>>>> from diff checking
>>>> +- 'package-vc-upgrade-diff-exclude-packages': Exclude specific VC
>>>> packages from diff checking
>>>> +
>>>> +When enabled (the default), users will see a diff buffer showing changes
>>>> +and can choose whether to proceed with or cancel the upgrade.
>>> This description is probably a bit too detailed for a NEWS entry.  I
>>> would not go into the new user options is that great of a detail.
>>>
>>> Also, if we decide to enable this by default (which I am not certain of
>>> right now), the news entry should be more clear in what the change is
>>> that will affect all users: For instance, it is not "can now display"
>>> but "will now display".
>> Yes, the feature should not be enabled by default, only via user option.
>> We could consider adding an option to the package installation
>> confirmation. Instead of pressing y and n, the user could press d to see
>> the diff.
> 1+
>
> Another key like "n" or "c" to show the news/changes might also be
> useful to have.  Perhaps the configuration we want is actually just to
> specify what packages can be updated without any checks, and what
> packages should be "approved".
>
> [...]
>
>>> I hope it is fine that I will not comment on this file any more, because
>>> I am not convinced of the approach (among other things having to make a
>>> fresh clone of the entire repository every time the package is being
>>> upgraded, which for larger packages like auctex can take a while).  That
>>> is not to say that I am not open to UI suggestions based on
>>> `package-vc-log-incoming', if you want to suggest anything like that.
>> Do I understand correctly that for package-vc we don't need a new
>> addition at all since one can use package-vc-log-incoming?
> Right, that is at least my current understanding.  We can combine
> `vc-log-incoming' with an upgrade procedure.  The interface will
> probably not be the same, also because I don't think that
> batch-upgrading VC packages is good practice.
>
>
> [...]
>
>>>> +This allows for fine-grained control over which archives show
>>>> +diffs during package upgrades."
>>>> +  :type '(repeat string)
>>>> +  :group 'package
>>>> +  :version "31.1")
>>> I wonder if instead of having multiple user options, it might be cleaner
>>> to replace `package-show-upgrade-diffs' with a non-boolean user option
>>> that can select the packages to trust/mistrust, sort of like how
>>> `buffer-match-p' can select buffers.  So the user option instead a list
>>> consisting of
>>>
>>>    (package foo)
>>>    (archive "bar")
>>>
>>> entries or t, if you want to mistrust everything.
>> Sounds good to me.
>>
>>> Also, shouldn't we also be able to configure what files to diff?
>> Such an option might be good to have, but my preference would be to
>> always see the whole diff, even if potentially "harmless" files are
>> added. I would also mistrust everything. So from my perspective, more
>> rigid and secure defaults would work too.
> I agree.
>
> [...]
>
>>>> +      (read-only-mode 1)
>>> I think it is interesting to consider not setting the buffer to be
>>> read-only by default, as it allows the user to manipulate the diff.
>>> This also relates to the fact that the user might have made local
>>> adjustment that they might want to keep between upgrades...
>> Isn't this orthogonal to the problem here - if you want to make local
>> changes, you should install the package from source or via package-vc.
>> If you install a package via the standard mechanism, the package should
>> be installed in the form provided by the archive and therefore a
>> read-only diff should be fine.
> You are right, this is just a general thought I haven't thought out
> myself that we can ignore for now.
>
>>>> +      (goto-char (point-min)))
>>>> +    (display-buffer diff-buffer)
>>>> +    (let ((response (yes-or-no-p "Show differences. Continue with
>>>                                      ^
>>>                                      What do you mean by show differences
>>>                                      here?  It sounds imperative, but I
>>>                                      am not sure if that is what you mean.
>>>> upgrade? ")))
>>>> +      (kill-buffer diff-buffer)
>>>> +      response)))
>>> You can use `prog1' here to avoid storing `response'.
>>>
>>>> +
>>>> +(defun package--confirm-tarball-upgrade (pkg-desc new-pkg-desc)
>>>> +  "Show diff for tarball package upgrade and ask for confirmation.
>>>> +Return t if user wants to proceed, nil otherwise."
>>>> +  ;; Check exclusion lists first
>>>> +  (let ((package-name (package-desc-name new-pkg-desc))
>>>> +        (package-archive (package-desc-archive new-pkg-desc)))
>>>> +    ;; If package-show-upgrade-diffs is nil, or package/archive is
>>>> excluded, proceed without diffs
>>>> +    (if (or (not package-show-upgrade-diffs)
>>>> +            (memq package-name package-upgrade-diff-exclude-packages)
>>>> +            (member package-archive package-upgrade-diff-exclude-archives))
>>>> +        t  ; Skip diff checking, proceed with upgrade
>>>> +      (let* ((temp-dir (make-temp-file "package-diff-" t))
>>>> +             (old-dir (package--get-installed-package-dir pkg-desc))
>>>> +             new-dir)
>>>> +        (unwind-protect
>>>> +            (progn
>>>> +              ;; Download and extract new version to temp directory
>>>> +              (let* ((location (package-archive-base new-pkg-desc))
>>>> +                     (file (concat (package-desc-full-name new-pkg-desc)
>>>> +                                   (package-desc-suffix new-pkg-desc))))
>>>> +                (package--with-response-buffer location :file file
>>>> +                  (setq new-dir (package--extract-tarball-to-temp
>>>> +                                 (current-buffer) new-pkg-desc temp-dir))))
>>>> +              ;; Show diff and get user confirmation
>>>> +              (if (file-exists-p old-dir)
>>>> +                  (package--show-package-diff old-dir new-dir)
>>>> +                ;; If no old version exists, just confirm
>>>> +                (yes-or-no-p "New package installation. Continue? ")))
>>>> +          ;; Clean up temp directory
>>>> +          (when (file-exists-p temp-dir)
>>>> +            (delete-directory temp-dir t)))))))
>>> My preferred approach to solving this would be if we could split
>>> `package-install-from-archive' or `package-unpack' into two, allowing us
>>> to install the new package without compiling, generating autoloads, and
>>> then to resume that part later on.  This would require some refactoring,
>>> but would allow us to express the upgrade procedure as
>>>
>>> 1. Fetch the new source but not process it,
>>>
>>> 2. Compute the diff and present it to the user,
>>>
>>> 3. If they are fine with the changes, delete old package and finish
>>>     initialising the package we just downloaded,
>>>
>>> 4. Otherwise we remove and forget about the code we had just downloaded
>>>     and keep everything as is.
>>>
>>> I imagine that this approach should keep the upgrade procedure more
>>> uniform and setting aside changes caused by re-indenting code we factor
>>> out of existing functionality, result in a simpler upgrade-logic.
>> I see your point but I would generally be careful with performing "half"
>> of the installation before showing the diff. Until the user has reviewed
>> the diff, the new package should be considered malicious so it might be
>> better if it does not yet end up in the ~/.config/emacs/elpa/ directory.
>> Maybe a separate review directory could be used, e.g.,
>> ~/.config/emacs/package-review/ and then the package could be moved to
>> the final destination in the end.
> You are right, that is important to keep in mind.  My main focus here is
> that we should avoid downloading the package twice.  FWIW it should be
> fine if the package is first cloned into /tmp/ and then copied over to
> ~/.config/emacs/elpa/ after being approved, or is your point with
> .../package-review/ that the list of not-yet-reviewed should stay
> persistent?
>
>> Generally regarding rollback, package.el is not robust right now (there
>> is another bug about that). The original package can stay deleted if
>> installation fails. This can happen due to multiple reasons - package
>> archive metadata outdated, package tarball not available, some other
>> failure during installation. In these case, Emacs can end up in a broken
>> state with a missing package.
> That is true and something I want to improve upon independently of this
> bug report (but we shouldn't make it worse either).
>
> [...]

[0001-Enhance-package-upgrade-UI-with-interactive-y-n-d-c-.patch (text/plain, attachment)]

Message #59 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>
Cc: mail <at> daniel-mendler.de, philipk <at> posteo.net, 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: [PATCH v1] package.el: Add diff display and
 confirmation for package upgrades (Bug#74604)
Date: Sat, 13 Sep 2025 10:08:57 +0300

> Cc: 74604 <at> debbugs.gnu.org
> Date: Sat, 13 Sep 2025 09:01:33 +0900
> From: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>
> 
> +(defcustom package-upgrade-confirmation-policy t

The default value sounds problematic to me.  For starters, it changes
previous behavior, which already should have a good reason.  Moreover,
IME most people don't care about the changes, and consider any display
that insists of showing them, let alone wants a confirmation, an
annoyance.  Consider the case that a user upgrades many packages at
once, for example.

So my suggestion is to leave the default at nil, preserving previous
behavior.  This option is a very good feature, but only for users who
are actually interested in reviewing the changes when they upgrade.

> +Possible values:
> +
> +\\=`t\\=' - Default
> +    Show diffs and prompt for all package upgrades.
> +
> +\\=`nil\\='
> +    Automatically upgrade all packages without showing diffs.

We don't quote t and nil in doc strings, so please don't.

> +  :type '(choice
> +          (const :tag "Always show diffs" t)
> +          (const :tag "Never show diffs" nil)
> +          (repeat :tag "Specific packages/archives to confirm"

If we are going to request confirmation, the first two tags should
also mention that.  As written, the first two tags only talk about
showing the diffs and never mention that showing changes also requires
a confirmation.

Thanks.

Message #62 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Stéphane Marks <shipmints <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: mail <at> daniel-mendler.de, philipk <at> posteo.net,
 Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>, 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: [PATCH v1] package.el: Add diff display and
 confirmation for package upgrades (Bug#74604)
Date: Sat, 13 Sep 2025 07:34:13 -0400

[Message part 1 (text/plain, inline)]

On Sat, Sep 13, 2025 at 3:10 AM Eli Zaretskii <eliz <at> gnu.org> wrote:

> > Cc: 74604 <at> debbugs.gnu.org
> > Date: Sat, 13 Sep 2025 09:01:33 +0900
> > From: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>
> >
> > +(defcustom package-upgrade-confirmation-policy t
>
> The default value sounds problematic to me.  For starters, it changes
> previous behavior, which already should have a good reason.  Moreover,
> IME most people don't care about the changes, and consider any display
> that insists of showing them, let alone wants a confirmation, an
> annoyance.  Consider the case that a user upgrades many packages at
> once, for example.
>
> So my suggestion is to leave the default at nil, preserving previous
> behavior.  This option is a very good feature, but only for users who
> are actually interested in reviewing the changes when they upgrade.
>
> > +Possible values:
> > +
> > +\\=`t\\=' - Default
> > +    Show diffs and prompt for all package upgrades.
> > +
> > +\\=`nil\\='
> > +    Automatically upgrade all packages without showing diffs.
>
> We don't quote t and nil in doc strings, so please don't.
>
> > +  :type '(choice
> > +          (const :tag "Always show diffs" t)
> > +          (const :tag "Never show diffs" nil)
> > +          (repeat :tag "Specific packages/archives to confirm"
>
> If we are going to request confirmation, the first two tags should
> also mention that.  As written, the first two tags only talk about
> showing the diffs and never mention that showing changes also requires
> a confirmation.
>

I think I'd prefer a whitelist of packages I choose to trust vs. a
blacklist of those I don't.  I'm guessing that for the people who want this
feature (like me), their blacklist would be much longer than the whitelist,
and we will not want to maintain that blacklist by hand.  Maintaining the
whitelist will be easy, with so few packages to not warrant a review.

[Message part 2 (text/html, inline)]

Message #65 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Stéphane Marks <shipmints <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: mail <at> daniel-mendler.de, philipk <at> posteo.net,
 Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>, 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: [PATCH v1] package.el: Add diff display and
 confirmation for package upgrades (Bug#74604)
Date: Sat, 13 Sep 2025 08:17:18 -0400

[Message part 1 (text/plain, inline)]

On Sat, Sep 13, 2025 at 7:34 AM Stéphane Marks <shipmints <at> gmail.com> wrote:

> On Sat, Sep 13, 2025 at 3:10 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
>> > Cc: 74604 <at> debbugs.gnu.org
>> > Date: Sat, 13 Sep 2025 09:01:33 +0900
>> > From: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>
>> >
>> > +(defcustom package-upgrade-confirmation-policy t
>>
>> The default value sounds problematic to me.  For starters, it changes
>> previous behavior, which already should have a good reason.  Moreover,
>> IME most people don't care about the changes, and consider any display
>> that insists of showing them, let alone wants a confirmation, an
>> annoyance.  Consider the case that a user upgrades many packages at
>> once, for example.
>>
>> So my suggestion is to leave the default at nil, preserving previous
>> behavior.  This option is a very good feature, but only for users who
>> are actually interested in reviewing the changes when they upgrade.
>>
>> > +Possible values:
>> > +
>> > +\\=`t\\=' - Default
>> > +    Show diffs and prompt for all package upgrades.
>> > +
>> > +\\=`nil\\='
>> > +    Automatically upgrade all packages without showing diffs.
>>
>> We don't quote t and nil in doc strings, so please don't.
>>
>> > +  :type '(choice
>> > +          (const :tag "Always show diffs" t)
>> > +          (const :tag "Never show diffs" nil)
>> > +          (repeat :tag "Specific packages/archives to confirm"
>>
>> If we are going to request confirmation, the first two tags should
>> also mention that.  As written, the first two tags only talk about
>> showing the diffs and never mention that showing changes also requires
>> a confirmation.
>>
>
> I think I'd prefer a whitelist of packages I choose to trust vs. a
> blacklist of those I don't.  I'm guessing that for the people who want this
> feature (like me), their blacklist would be much longer than the whitelist,
> and we will not want to maintain that blacklist by hand.  Maintaining the
> whitelist will be easy, with so few packages to not warrant a review.
>

An option to automatically whitelist built-ins might also be useful, as
those are "core."

[Message part 2 (text/html, inline)]

Message #68 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>
Cc: Daniel Mendler <mail <at> daniel-mendler.de>, 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: [PATCH v1] package.el: Add diff display and
 confirmation for package upgrades (Bug#74604)
Date: Sat, 13 Sep 2025 12:17:31 +0000

Nobuyuki Kamimoto <kamimoto527 <at> gmail.com> writes:

> Hello Philip and Daniel,
>
> Thank you for your detailed feedback on the initial patch. You raised
> excellent points about avoiding
> duplication of existing VC functionality and improving the overall approach.

(Not to start a unrelated discussion, but parts of your message sound
like they might have been written by generative AI.  I don't mind that,
but it might be an issue if you used GenAI to prepare the patch as well,
as TTBOMK is not yet a resolved matter for the GNU project.  Related to
that, have you signed the FSF copyright assignment?)

[...]

>
> From a41c934b9c39e99e9aaf379519f0d66a71b39966 Mon Sep 17 00:00:00 2001
> From: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>
> Date: Fri, 12 Sep 2025 21:12:53 +0900
> Subject: [PATCH] Enhance package upgrade UI with interactive y/n/d/c prompts
>
> Add interactive confirmation system for package upgrades with options for:
> - Yes/No upgrade decisions
> - Diff display between package versions
> - Changelog viewing with file exclusion patterns
> - Configurable confirmation policy per package/archive
>
> Includes comprehensive diff utilities for tarball packages and VC packages,
> with proper error handling and user-friendly display buffers.

It would be nice if you could rewrite the commit message to align with
the style described in the CONTRIBUTE file (see the section "Commit
messages").  The main thing here is that you explicitly annotate how
which functions have been changed in which files.

> ---
>  doc/emacs/package.texi          |  42 ++++
>  etc/NEWS                        |  10 +
>  lisp/package/package-core.el    | 158 ++++++++++++++

How much of these changes really have to be part of package-core?  The
idea behind the ongoing refactoring is to keep that file as small as
possible, so that it contains just what is necessary to activate
packages on startup.

>  lisp/package/package-install.el | 356 +++++++++++++++++++++++++++++++-
>  lisp/package/package-menu.el    |  48 +++--
>  lisp/package/package-vc.el      | 218 +++++++++++++++----
>  6 files changed, 774 insertions(+), 58 deletions(-)
>
> diff --git a/doc/emacs/package.texi b/doc/emacs/package.texi
> index 5aa9f9a74bf..3b2a59dda23 100644
> --- a/doc/emacs/package.texi
> +++ b/doc/emacs/package.texi
> @@ -389,6 +389,38 @@ Package Installation
>  use these bulk commands if you want to update only a small number of
>  built-in packages.
>  
> +@vindex package-upgrade-confirmation-policy
> +  By default, Emacs shows a diff of the changes when upgrading
> +packages, allowing you to review what has changed between the current
> +and new version before proceeding.  This is controlled by the
> +@code{package-upgrade-confirmation-policy} user option.  When set to @code{t} (the
> +default), package upgrades will display the differences and ask for
> +confirmation before proceeding.  You can disable this behavior by
> +setting @code{package-upgrade-confirmation-policy} to @code{nil} to upgrade
> +all packages automatically without showing diffs.
> +
> +  You can also specify which packages or archives require confirmation by
> +setting @code{package-upgrade-confirmation-policy} to a list of specific
> +packages and archives.  Only the packages and archives in the list will show
> +confirmation prompts, while all others will upgrade automatically.
> +
> +@noindent
> +Examples of list-based configuration:
> +
> +@example
> +;; Only confirm magit and helm upgrades, auto-upgrade everything else
> +(setq package-upgrade-confirmation-policy
> +      '((package magit) (package helm)))
> +
> +;; Only confirm packages from melpa archive, auto-upgrade others
> +(setq package-upgrade-confirmation-policy
> +      '((archive "melpa")))
> +
> +;; Mixed: confirm org package and all melpa-stable packages
> +(setq package-upgrade-confirmation-policy
> +      '((package org) (archive "melpa-stable")))
> +@end example
> +
>  @cindex package requirements
>    A package may @dfn{require} certain other packages to be installed,
>  because it relies on functionality provided by them.  When Emacs
> @@ -655,6 +687,16 @@ Fetching Package Sources
>    Note that currently, built-in packages cannot be upgraded using
>  @code{package-vc-install}.
>  
> +  Like regular package upgrades, VC package upgrades can also show
> +diffs before proceeding.  This behavior is controlled by the same
> +@code{package-upgrade-confirmation-policy} user option that controls regular
> +package upgrades.  When set to @code{t} (the default), upgrading VC
> +packages will display the differences between the current and updated
> +versions, allowing you to review the changes before confirming the upgrade.
> +
> +  VC packages use the same confirmation policy and file exclusion patterns
> +as regular packages, ensuring consistent behavior across all package types.
> +
>  @findex package-report-bug
>  @findex package-vc-prepare-patch
>    With the source checkout, you might want to reproduce a bug against
> diff --git a/etc/NEWS b/etc/NEWS
> index ac8e56326bf..904592577a0 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -74,6 +74,16 @@ done from early-init.el, such as adding to 'package-directory-list'.
>  ** 'prettify-symbols-mode' attempts to ignore undisplayable characters.
>  Previously, such characters would be rendered as, e.g., white boxes.
>  
> ++++
> +** Package management now shows diffs before upgrades.
> +Package upgrades will now display differences between the current and
> +new version before proceeding.  This applies to both regular packages
> +and VC packages installed from version control repositories.
> +
> +Users will see a diff buffer showing changes and can choose whether to
> +proceed with or cancel the upgrade.  This behavior can be controlled
> +through the 'package-upgrade-confirmation-policy' user option.
> +
>  +++
>  ** 'standard-display-table' now has more extra slots.
>  'standard-display-table' has been extended to allow specifying glyphs
> diff --git a/lisp/package/package-core.el b/lisp/package/package-core.el
> index e52654aa53d..b0b1831e1ed 100644
> --- a/lisp/package/package-core.el
> +++ b/lisp/package/package-core.el
> @@ -284,6 +284,164 @@ package-selected-packages
>    :version "25.1"
>    :type '(repeat symbol))
>  
> +;; Interactive upgrade prompt constants
> +(defconst package-upgrade-interactive-commands
> +  '((?y . upgrade)
> +    (?n . cancel)
> +    (?d . show-diff)
> +    (?c . show-changelog)
> +    (?q . quit))
> +  "Interactive commands for package upgrade confirmation.")
> +
> +(defconst package-upgrade-prompt-message
> +  "Upgrade %s? (y)es, (n)o, (d)iff, (c)hangelog, (q)uit: "
> +  "Prompt message format for package upgrade confirmation.")
> +
> +(defconst package-diff-context-lines 3
> +  "Number of context lines to show in package and changelog diffs.")

I don't think that we need these declarations as constants?

> +(defconst package-changelog-max-size (* 1024 1024)
> +  "Maximum size in bytes for changelog files to process.")
> +
> +(defcustom package-upgrade-confirmation-policy t

As Eli said, the default is probably too invasive.

> +  "Policy for confirming packages during upgrades.
> +This determines which packages require user confirmation before upgrading.
> +
> +Possible values:
> +
> +\\=`t\\=' - Default
> +    Show diffs and prompt for all package upgrades.
> +
> +\\=`nil\\='
> +    Automatically upgrade all packages without showing diffs.
> +
> +A list of specific packages/archives to confirm
> +    Only the packages and archives listed will show confirmation prompts.
> +    All others will be upgraded automatically.
> +
> +    List format:
> +    (package PACKAGE-SYMBOL)  - Show diff for this specific package

Do you think that it would also be acceptable to have symbols
interpreted as synonyms to (package ...)?

> +    (archive ARCHIVE-NAME)    - Show diff for all packages in this archive
> +
> +Examples:
> +    \\='((package magit) (package helm))
> +        - Only confirm magit and helm upgrades
> +        - All other packages upgrade automatically
> +
> +    \\='((archive \"melpa\") (package org))
> +        - Confirm all melpa archive packages
> +        - Confirm org package upgrades
> +        - All other packages upgrade automatically
> +
> +    \\='((package magit))
> +        - Only confirm magit upgrades
> +        - All other packages upgrade automatically"
> +  :type '(choice
> +          (const :tag "Always show diffs" t)
> +          (const :tag "Never show diffs" nil)
> +          (repeat :tag "Specific packages/archives to confirm"
> +                  (choice
> +                   (list :tag "Package rule" (const package) symbol)
> +                   (list :tag "Archive rule" (const archive) string))))
> +  :version "31.1")
> +
> +(defun package--match-rule-p (rule package-name package-archive)

I think it would be cleaner to pass the package-desc object here, than
to have to extract the information before invoking the function.

> +  "Check if RULE matches PACKAGE-NAME and PACKAGE-ARCHIVE."
> +  (pcase rule
> +    (`(package ,name) (eq name package-name))
> +    (`(archive ,archive) (string= archive package-archive))
> +    (_ nil)))
> +
> +(defun package--should-show-diff-p (pkg-desc)
> +  "Check if diff should be shown for PKG-DESC based on confirmation policy."
> +  (let ((package-name (package-desc-name pkg-desc))
> +        (package-archive (package-desc-archive pkg-desc))
> +        (policy package-upgrade-confirmation-policy))
> +    (cond

Feel free to use `pcase' here as well, to avoid the trivial binding.

> +     ((eq policy t) t)
> +     ((eq policy nil) nil)

This is redundant, as if the policy is nil, (listp nil) is true, and
(seq-some ... nil) is always nil.

> +     ((listp policy)
> +      ;; List mode: only show diff for packages/archives in the list
> +      (seq-some (lambda (rule)
> +                  (package--match-rule-p rule package-name package-archive))

As the function is used only once, I would actually even inline it here.

> +                policy))
> +     (t t))))
> +
> +(defcustom package-changelog-patterns
> +  '("CHANGELOG*" "NEWS*" "ChangeLog*" "CHANGES*" "HISTORY*")
> +  "File patterns to match changelog files."
> +  :type '(repeat string)
> +  :version "31.1")
> +
> +(defvar package-changelog-file-regex
> +  (concat "\\(" (mapconcat (lambda (pattern)
> +                           (replace-regexp-in-string "\\*" ".*" pattern))
> +                         package-changelog-patterns "\\|") "\\)")
> +  "Regular expression to match changelog files based on patterns.")

I think it would be better to check for a designated "news" file, the
way that `describe-package-1' does.

> +;; Changelog file utilities
> +(declare-function diff-no-select "diff" (old new &optional switches no-async))
> +
> +(defun package--find-changelog-files (directory)
> +  "Find all changelog files in DIRECTORY using regular expression matching.
> +Returns a list of paths to all matching changelog files found, or nil if none."
> +  (when (and directory (stringp directory) (file-directory-p directory))
> +    (condition-case err
> +        (let ((files (directory-files directory t "^[^.]" t))
> +              (candidates nil))
> +          ;; Collect all matching files
> +          (dolist (file files)
> +            (when (and (file-regular-p file)
> +                       (file-readable-p file)
> +                       (let ((basename (file-name-nondirectory file)))
> +                         (string-match-p package-changelog-file-regex
> +                                         (downcase basename))))

You can also just bind `case-fold-search' to nil, that way you don't
have to allocate a new string.

> +              (push file candidates)))
> +          ;; Return all candidates sorted by name for consistent ordering
> +          (sort candidates #'string<))
> +      (file-error
> +       (message "File access error searching for changelog files in %s: %s"
> +                directory (error-message-string err))
> +       nil)
> +      (error
> +       (message "Error searching for changelog files in %s: %s"
> +                directory (error-message-string err))
> +       nil))))

I think that you can simplify this using `with-demoted-errors'.

> +
> +(defun package--diff-available-p ()
> +  "Check if diff functionality is available.
> +Since we use built-in Emacs diff functions, this always returns t."
> +  (require 'diff)
> +  t)

Why do we need this?

> +(defun package--diff-changelog-files (old-file new-file)
> +  "Generate unified diff between OLD-FILE and NEW-FILE.
> +Returns diff output as string, or nil if diff is not available."
> +  (when (and old-file new-file
> +             (file-exists-p old-file)
> +             (file-exists-p new-file)
> +             (package--diff-available-p))
> +    (let ((old-size (nth 7 (file-attributes old-file)))
> +          (new-size (nth 7 (file-attributes new-file))))
> +      ;; Check file size limits
> +      (when (and (< old-size package-changelog-max-size)
> +                 (< new-size package-changelog-max-size))
> +        (let ((diff-buffer (diff-no-select old-file new-file
> +                                           (format "--unified=%d" package-diff-context-lines) t)))
> +          (when diff-buffer
> +            (with-current-buffer diff-buffer
> +              (let ((diff-output (buffer-string)))
> +                (kill-buffer diff-buffer)
> +                (when (> (length diff-output) 0)
> +                  diff-output)))))))))
> +
> +(defun package--format-changelog-diff ()
> +  "Format and highlight diff output in current buffer.
> +Uses diff-mode features for syntax highlighting."
> +  (when (fboundp 'diff-mode)
> +    (diff-mode)
> +    (font-lock-ensure)))
> +
>  ;; Pseudo fields.
>  (defun package-version-join (vlist)
>    "Return the version string corresponding to the list VLIST.
> diff --git a/lisp/package/package-install.el b/lisp/package/package-install.el
> index 8401a7769b7..34468c109b5 100644
> --- a/lisp/package/package-install.el
> +++ b/lisp/package/package-install.el
> @@ -380,6 +380,7 @@ package-install
>        (message "`%s' is already installed" name))))
>  
>  (declare-function package-vc-upgrade "package-vc" (pkg))
> +(declare-function diff-no-select "diff" (old new &optional switches no-async))
>  
>  ;;;###autoload
>  (defun package-upgrade (name)
> @@ -392,16 +393,26 @@ package-upgrade
>                    (package--upgradeable-packages t) nil t))))
>    (cl-check-type name symbol)
>    (let* ((pkg-desc (cadr (assq name package-alist)))
> -         (package-install-upgrade-built-in (not pkg-desc)))
> +         (package-install-upgrade-built-in (not pkg-desc))
> +         (new-pkg-desc (cadr (assq name package-archive-contents))))
>      ;; `pkg-desc' will be nil when the package is an "active built-in".
>      (if (and pkg-desc (package-vc-p pkg-desc))
>          (package-vc-upgrade pkg-desc)
> -      (when pkg-desc
> -        (package-delete pkg-desc 'force 'dont-unselect))
> -      (package-install name
> -                       ;; An active built-in has never been "selected"
> -                       ;; before.  Mark it as installed explicitly.
> -                       (and pkg-desc 'dont-select)))))
> +      ;; Check if this is a tarball package upgrade that needs diff confirmation
> +      (if (and pkg-desc new-pkg-desc
> +               (eq (package-desc-kind new-pkg-desc) 'tar))
> +          ;; For tarball packages, show diff and ask for confirmation
> +          ;; The function now handles the complete upgrade process internally
> +          (unless (package--confirm-tarball-upgrade pkg-desc new-pkg-desc)
> +            (message "Package upgrade cancelled"))
> +        ;; For non-tarball packages, proceed with normal upgrade
> +        (progn

You can drop the `progn' here, Elisp's if has an implicit trailing progn.

> +          (when pkg-desc
> +            (package-delete pkg-desc 'force 'dont-unselect))
> +          (package-install name
> +                           ;; An active built-in has never been "selected"
> +                           ;; before.  Mark it as installed explicitly.
> +                           (and pkg-desc 'dont-select)))))))
>  
>  (defun package--upgradeable-packages (&optional include-builtins)
>    ;; Initialize the package system to get the list of package
> @@ -1108,5 +1119,336 @@ package-recompile-all
>      (with-demoted-errors "Error while recompiling: %S"
>        (package-recompile pkg-desc))))
>  
> +;; Package upgrade diff utilities
> +
> +(defun package--safe-insert-file (file &optional error-msg)
> +  "Safely insert FILE contents, showing ERROR-MSG on failure."
> +  (condition-case err

Same comment here as above.

> +      (when (and file (file-readable-p file))
> +        (insert-file-contents file))
> +    (file-error
> +     (insert (or error-msg
> +                 (format "File access error: %s" (error-message-string err)))))

How can this happen, if we have established prior to
`insert-file-contents' that the file is readable -- excluding the
possibility of corrupted permissions due the involvement of multiple
users or race conditions?

> +    (error
> +     (insert (or error-msg
> +                 (format "Error reading file: %s" (error-message-string err)))))))
> +
> +(defun package--show-changelog (old-dir new-dir)
> +  "Show diff of all changelog files between OLD-DIR and NEW-DIR.
> +Displays unified diff showing what changed in all matched changelog files.
> +Returns t if any changelog diff was displayed, nil otherwise."
> +  (let ((old-changelogs (when old-dir (package--find-changelog-files old-dir)))
> +        (new-changelogs (when new-dir (package--find-changelog-files new-dir)))
> +        (displayed-any nil))
> +    (cond
> +     ;; Both directories have changelog files
> +     ((or old-changelogs new-changelogs)
> +      (setq displayed-any (package--display-all-changelog-diffs old-changelogs new-changelogs)))
> +     ;; No changelog files found in either directory
> +     (t
> +      (message "No changelog files found")
> +      nil))
> +    displayed-any))
> +
> +(defun package--display-all-changelog-diffs (old-files new-files)
> +  "Display diffs for all changelog files between OLD-FILES and NEW-FILES.
> +Returns t if any diff was displayed, nil otherwise."
> +  (let ((buffer-name "*Package Changelog Diff*")
> +        (displayed-any nil)
> +        (unique-filenames nil))
> +    ;; Kill existing buffer if it exists
> +    (when (get-buffer buffer-name)
> +      (kill-buffer buffer-name))
> +
> +    ;; Get unique filenames from both lists
> +    (setq unique-filenames
> +          (delete-dups
> +           (append (mapcar #'file-name-nondirectory old-files)
> +                   (mapcar #'file-name-nondirectory new-files))))
> +
> +    (with-current-buffer (get-buffer-create buffer-name)
> +      (let ((inhibit-read-only t))
> +        (erase-buffer)
> +        (insert "Changelog differences between old and new versions:\n")
> +        (insert (make-string 55 ?=))
> +        (insert "\n\n")
> +
> +        ;; Process each unique filename
> +        (dolist (filename unique-filenames)
> +          (let ((old-file (car (seq-filter (lambda (f)
> +                                             (string= filename (file-name-nondirectory f)))
> +                                           old-files)))
> +                (new-file (car (seq-filter (lambda (f)
> +                                             (string= filename (file-name-nondirectory f)))
> +                                           new-files))))
> +            (cond
> +             ;; Both old and new versions exist: show diff
> +             ((and old-file new-file)
> +              (package--insert-file-diff filename old-file new-file)
> +              (setq displayed-any t))
> +             ;; Only new file exists: show as new addition
> +             ((and (not old-file) new-file)
> +              (package--insert-new-file filename new-file)
> +              (setq displayed-any t))
> +             ;; Only old file exists: show as removal
> +             ((and old-file (not new-file))
> +              (insert (format "--- %s (REMOVED in new version) ---\n\n" filename))
> +              (setq displayed-any t)))))
> +
> +        (if displayed-any
> +            (progn
> +              (package--format-changelog-diff)
> +              (goto-char (point-min))
> +              (read-only-mode 1))
> +          (insert "No changelog differences found.\n")
> +          (read-only-mode 1)))
> +
> +      (display-buffer buffer-name)
> +      displayed-any)))

IMO it might be nicer if we could use an existing interface like
tabulated-list-mode.  But just generating a plain, recursive diff would
also be fine.

> +(defun package--insert-file-diff (filename old-file new-file)
> +  "Insert diff content for a single file into current buffer."
> +  (insert (format "--- %s ---\n" filename))
> +  (let ((diff-output (package--diff-changelog-files old-file new-file)))
> +    (if diff-output
> +        (insert diff-output)
> +      ;; Fallback if diff is unavailable
> +      (insert "Diff unavailable. Showing full new content:\n")
> +      (package--safe-insert-file new-file))
> +    (insert "\n\n")))
> +
> +(defun package--insert-new-file (filename new-file)
> +  "Insert new file content into current buffer."
> +  (insert (format "--- %s (NEW in this version) ---\n" filename))
> +  (package--safe-insert-file new-file)
> +  (insert "\n\n"))
> +
> +(defun package--show-package-diff (old-dir new-dir pkg-desc)
> +  "Show diff between OLD-DIR and NEW-DIR package directories for PKG-DESC.
> +This function only displays the diff without prompting for user confirmation."
> +  ;; Ensure diff is loaded and ready before proceeding
> +  (require 'diff)
> +  ;; Ensure diff feature is fully loaded on first run
> +  (unless (featurep 'diff)
> +    (sit-for 0.1))

require is not asynchronous!  This is not an issue.

> +  ;; Additional safety: ensure diff functions are available
> +  (unless (fboundp 'diff-mode)
> +    (autoload 'diff-mode "diff" "Diff major mode" t))

This is als not necessary, diff-mode is part of Emacs and you can rely
on the above require to do the right thing™.

> +  (condition-case outer-err
> +      (let ((diff-buffer-name "*Package Diff*"))
> +        ;; Kill existing buffer to avoid read-only issues
> +        (when (get-buffer diff-buffer-name)
> +          (kill-buffer diff-buffer-name))
> +
> +        ;; Check if directories exist before proceeding
> +        (unless (and (file-directory-p old-dir) (file-directory-p new-dir))
> +          (error "One or both directories do not exist: %s, %s" old-dir new-dir))
> +
> +        (condition-case diff-err
> +            ;; Use built-in diff-no-select instead of external commands

Why?

> +            (let ((diff-buffer (diff-no-select old-dir new-dir "--unified" t)))
> +              (if diff-buffer
> +                  (progn
> +                    ;; Safely configure diff buffer before renaming
> +                    (with-current-buffer diff-buffer
> +                      ;; Ensure diff-mode is properly initialized first
> +                      (when (fboundp 'diff-mode)
> +                        (diff-mode))
> +                      ;; Configure buffer for stable scrolling
> +                      (setq buffer-read-only t)
> +                      (setq truncate-lines nil)  ; Allow line wrapping
> +                      ;; Disable potentially problematic diff features
> +                      (when (boundp 'diff-refine-hunk)
> +                        (set (make-local-variable 'diff-refine-hunk) nil))

Why disable this?  This is a useful feature when reading the file.

> +                      (goto-char (point-min))
> +                      ;; Now safely rename the buffer
> +                      (rename-buffer diff-buffer-name t))
> +                    ;; Display the properly configured buffer
> +                    (display-buffer diff-buffer-name))
> +                ;; No differences found
> +                (with-current-buffer (get-buffer-create diff-buffer-name)
> +                  (let ((inhibit-read-only t))
> +                    (erase-buffer)
> +                    (insert (format "No significant differences found between old and new versions of %s.\n"
> +                                    (package-desc-name pkg-desc)))
> +                    (goto-char (point-min))
> +                    (read-only-mode 1))
> +                  (display-buffer diff-buffer-name))))
> +          (error
> +           ;; If diff fails, create a simple error message
> +           (with-current-buffer (get-buffer-create diff-buffer-name)
> +             (let ((inhibit-read-only t))
> +               (erase-buffer)
> +               (insert (format "Error running diff operation: %s\n\n" (error-message-string diff-err)))
> +               (insert "This may be due to:\n")
> +               (insert "- File access permissions\n")
> +               (insert "- Directory structure issues\n")
> +               (insert "- Memory limitations for large diffs\n")
> +               (read-only-mode 1))
> +             (display-buffer diff-buffer-name))))
> +        ;; Return t to indicate success
> +        t)
> +    (error
> +     (message "Error showing package diff: %s (Retry may work)" (error-message-string outer-err))
> +     ;; Return nil to indicate failure but allow retry
> +     nil)))
> +
> +(defun package--handle-interactive-command (command pkg-desc old-dir new-dir)
> +  "Handle interactive COMMAND for PKG-DESC upgrade."
> +  (pcase command
> +    ('upgrade 'upgrade)

You don't need to quote upgrade here?

> +    ('cancel 'cancel)
> +    ('show-diff
> +     (condition-case err
> +         (if (and old-dir new-dir)
> +             (progn
> +               (let ((result (package--show-package-diff old-dir new-dir pkg-desc)))
> +                 (if result
> +                     (message "Diff displayed. Press y to upgrade, n to cancel, c for changelog.")
> +                   (message "Diff display failed. Please try again or use changelog (c)."))))
> +           (message "Package directories not available for diff."))
> +       (error
> +        (message "Error displaying diff: %s (Try again with 'd' or use changelog with 'c')"
> +                 (error-message-string err))))
> +     ;; Always return nil to prevent process termination
> +     nil)
> +    ('show-changelog
> +     (condition-case err
> +         (if (package--show-changelog old-dir new-dir)
> +             (message "Changelog displayed. Press y to upgrade, n to cancel, d for full diff.")
> +           (message "No changelog differences found. Press y to upgrade, n to cancel."))
> +       (error
> +        (message "Error displaying changelog: %s" (error-message-string err))))
> +     nil)
> +    ('quit 'quit)
> +    (_
> +     (message "Invalid choice. Use: y=upgrade, n=cancel, d=diff, c=changelog, q=quit")
> +     (sit-for 1.5)
> +     nil)))
> +
> +(defun package--upgrade-interactive-prompt (pkg-desc &optional old-dir new-dir)
> +  "Interactive prompt for package upgrade confirmation."
> +  (let ((package-name (package-desc-name pkg-desc))
> +        (prompt-choices (mapcar #'car package-upgrade-interactive-commands)))
> +    (catch 'result
> +      (while t
> +        (condition-case err
> +            (let* ((choice (read-char-choice
> +                            (format package-upgrade-prompt-message package-name)
> +                            prompt-choices))
> +                   (command (cdr (assq choice package-upgrade-interactive-commands)))
> +                   (result (package--handle-interactive-command command pkg-desc old-dir new-dir)))
> +              (when result
> +                (throw 'result result)))
> +          (quit
> +           (message "Package upgrade cancelled by user")
> +           (throw 'result 'cancel))
> +          (error
> +           (message "Error during input: %s" (error-message-string err))
> +           (sit-for 1)))))))

You have reimplemented `read-multiple-choice'...

> +
> +(defun package--get-installed-package-dir (pkg-desc)
> +  "Get directory of installed PKG-DESC package."
> +  (expand-file-name (package-desc-full-name pkg-desc) package-user-dir))
> +
> +(defun package-extract (pkg-desc)
> +  "Extract package PKG-DESC files without generating autoloads or descriptors.
> +Only performs file extraction based on package kind.  Returns the package
> +directory path where files were extracted."
> +  (let* ((name (package-desc-name pkg-desc))
> +         (dirname (package-desc-full-name pkg-desc))
> +         (pkg-dir (expand-file-name dirname package-user-dir)))
> +    ;; Extract files based on package kind
> +    (pcase (package-desc-kind pkg-desc)
> +      ('dir
> +       (make-directory pkg-dir t)
> +       (let ((file-list
> +              (or (and (derived-mode-p 'dired-mode)
> +                       (dired-get-marked-files))
> +                  (directory-files-recursively default-directory "" nil))))
> +         (dolist (source-file file-list)
> +           (let ((target (expand-file-name
> +                          (file-relative-name source-file default-directory)
> +                          pkg-dir)))
> +             (make-directory (file-name-directory target) t)
> +             (copy-file source-file target t)))
> +         ;; Now that the files have been installed, this package is
> +         ;; indistinguishable from a `tar' or a `single'. Let's make
> +         ;; things simple by ensuring we're one of them.
> +         (setf (package-desc-kind pkg-desc)
> +               (if (length> file-list 1) 'tar 'single))))
> +      ('tar
> +       (make-directory package-user-dir t)
> +       (let* ((default-directory (file-name-as-directory package-user-dir)))
> +         (package-untar-buffer dirname)))
> +      ('single
> +       (let ((el-file (expand-file-name (format "%s.el" name) pkg-dir)))
> +         (make-directory pkg-dir t)
> +         (package--write-file-no-coding el-file)))
> +      (kind (error "Unknown package kind: %S" kind)))
> +    ;; Return the directory path (no autoloads generation yet)
> +    pkg-dir))
> +
> +(defun package--download-and-extract (new-pkg-desc)
> +  "Download and extract NEW-PKG-DESC. Return extraction directory."
> +  (let ((location (package-archive-base new-pkg-desc))
> +        (file (concat (package-desc-full-name new-pkg-desc)
> +                      (package-desc-suffix new-pkg-desc))))
> +    (package--with-response-buffer location :file file
> +      (package-extract new-pkg-desc))))
> +
> +(defun package--confirm-upgrade (_pkg-desc new-pkg-desc old-dir new-dir)
> +  "Ask user to confirm upgrade from _PKG-DESC to NEW-PKG-DESC."
> +  (if (file-exists-p old-dir)
> +      (let ((result (package--upgrade-interactive-prompt new-pkg-desc old-dir new-dir)))
> +        (cond
> +         ((eq result 'upgrade) t)
> +         ((eq result 'quit) (user-error "Package upgrade cancelled by user"))
> +         (t nil)))
> +    (yes-or-no-p (format "New package installation: %s. Continue? "
> +                         (package-desc-name new-pkg-desc)))))
> +
> +(defun package--complete-upgrade (pkg-desc new-pkg-desc new-dir)
> +  "Complete the upgrade process from PKG-DESC to NEW-PKG-DESC."
> +  (when pkg-desc
> +    (package-delete pkg-desc 'force 'dont-unselect))
> +  (when (and new-dir (file-directory-p new-dir))
> +    (delete-directory new-dir t))
> +  (package-install-from-archive new-pkg-desc))
> +
> +(defun package--confirm-tarball-upgrade (pkg-desc new-pkg-desc)
> +  "Confirm and execute tarball package upgrade.
> +
> +This function handles the complete tarball upgrade workflow:
> +1. Check if diff display is required based on trust policy
> +2. Download and extract new package for comparison
> +3. Show diff and get user confirmation
> +4. Complete upgrade or clean up on cancellation
> +
> +Args:
> +  PKG-DESC: Current installed package descriptor
> +  NEW-PKG-DESC: New package descriptor to upgrade to
> +
> +Returns:
> +  t if upgrade was confirmed and completed, nil otherwise."
> +  (if (package--should-show-diff-p new-pkg-desc)
> +      (let* ((old-dir (package--get-installed-package-dir pkg-desc))
> +             (new-dir nil)
> +             (confirmed nil))
> +        (unwind-protect
> +            (progn
> +              (setq new-dir (package--download-and-extract new-pkg-desc))
> +              (setq confirmed (package--confirm-upgrade pkg-desc new-pkg-desc old-dir new-dir))
> +              (when confirmed
> +                (package--complete-upgrade pkg-desc new-pkg-desc new-dir)))
> +          ;; Cleanup: remove temporary extraction directory if upgrade was cancelled
> +          (when (and new-dir (not confirmed) (file-exists-p new-dir))
> +            (ignore-errors (delete-directory new-dir t))))
> +        confirmed)
> +    ;; Trust policy says skip diff - proceed directly with upgrade
> +    (progn
> +      (package--complete-upgrade pkg-desc new-pkg-desc nil)
> +      t)))
> +
>  (provide 'package-install)
>  ;;; package-install.el ends here
> diff --git a/lisp/package/package-menu.el b/lisp/package/package-menu.el
> index c57086112c4..3ffdb940554 100644
> --- a/lisp/package/package-menu.el
> +++ b/lisp/package/package-menu.el
> @@ -653,22 +653,38 @@ package-menu--perform-transaction
>                    (format status-format (incf i)))
>              (force-mode-line-update)
>              (redisplay 'force)
> -            ;; Don't mark as selected, `package-menu-execute' already
> -            ;; does that.
> -            (package-install pkg 'dont-select))))
> -    (let ((package-menu--transaction-status ":Deleting"))
> -      (force-mode-line-update)
> -      (redisplay 'force)
> -      (dolist (elt (package--sort-by-dependence delete-list))
> -        (condition-case-unless-debug err
> -            (let ((inhibit-message (or inhibit-message package-menu-async)))
> -              (package-delete elt nil 'nosave))
> -          (error
> -           (push (package-desc-full-name elt) errors)
> -           (message "Error trying to delete `%s': %s"
> -                    (package-desc-full-name elt)
> -                    (error-message-string err))))))
> -    errors))
> +            ;; Check if this is a package upgrade and needs confirmation
> +            (let* ((pkg-name (package-desc-name pkg))
> +                   (old-pkg (cl-find-if (lambda (del-pkg)
> +                                          (eq (package-desc-name del-pkg) pkg-name))
> +                                        delete-list)))
> +              (cond
> +               ;; Tarball package upgrade - use tarball confirmation
> +               ((and old-pkg (eq (package-desc-kind pkg) 'tar))
> +                (if (package--confirm-tarball-upgrade old-pkg pkg)
> +                    (package-install pkg 'dont-select)
> +                  (push (package-desc-full-name pkg) errors)))
> +               ;; VC package upgrade - use VC confirmation
> +               ((and old-pkg (package-vc-p old-pkg))
> +                (if (package-vc--confirm-upgrade old-pkg)
> +                    (package-install pkg 'dont-select)
> +                  (push (package-desc-full-name pkg) errors)))
> +               ;; Normal installation or upgrade
> +               (t
> +                (package-install pkg 'dont-select))))))
> +      (let ((package-menu--transaction-status ":Deleting"))
> +        (force-mode-line-update)
> +        (redisplay 'force)
> +        (dolist (elt (package--sort-by-dependence delete-list))
> +          (condition-case-unless-debug err
> +              (let ((inhibit-message (or inhibit-message package-menu-async)))
> +                (package-delete elt nil 'nosave))
> +            (error
> +             (push (package-desc-full-name elt) errors)
> +             (message "Error trying to delete `%s': %s"
> +                      (package-desc-full-name elt)
> +                      (error-message-string err))))))
> +      errors)))
>  
>  (defun package--update-selected-packages (add remove)
>    "Update the `package-selected-packages' list according to ADD and REMOVE.
> diff --git a/lisp/package/package-vc.el b/lisp/package/package-vc.el
> index 03767b99729..3579c9d69b5 100644
> --- a/lisp/package/package-vc.el
> +++ b/lisp/package/package-vc.el
> @@ -53,6 +53,7 @@
>  (require 'package-elpa)
>  (require 'lisp-mnt)
>  (require 'vc)
> +(require 'vc-git)

Err, package-vc is intentionally vc agnostic, so we shouldn't depend on
a specific backend, unless we are providing a speedup (in which case it
would be better to implement the feature more generally in vc-git).

>  (require 'seq)
>  
>  (defgroup package-vc nil
> @@ -737,6 +738,8 @@ package-vc-upgrade-all
>  
>  (declare-function vc-dir-prepare-status-buffer "vc-dir"
>                    (bname dir backend &optional create-new))
> +(declare-function vc-diff-incoming "vc" (&optional remote-location fileset))
> +(declare-function vc-log-incoming "vc" (&optional remote-location))

You shouldn't need this, as we are requiring vc above!

>  
>  ;;;###autoload
>  (defun package-vc-upgrade (pkg-desc)
> @@ -745,40 +748,42 @@ package-vc-upgrade
>  This may fail if the local VCS state of the package conflicts
>  with the remote repository state."
>    (interactive (list (package-vc--read-package-desc "Upgrade VC package: " t)))
> -  ;; HACK: To run `package-vc--unpack-1' after checking out the new
> -  ;; revision, we insert a hook into `vc-post-command-functions', and
> -  ;; remove it right after it ran.  To avoid running the hook multiple
> -  ;; times or even for the wrong repository (as `vc-pull' is often
> -  ;; asynchronous), we extract the relevant arguments using a pseudo
> -  ;; filter for `vc-filter-command-function', executed only for the
> -  ;; side effect, and store them in the lexical scope.  When the hook
> -  ;; is run, we check if the arguments are the same (`eq') as the ones
> -  ;; previously extracted, and only in that case will be call
> -  ;; `package-vc--unpack-1'.  Ugh...
> -  ;;
> -  ;; If there is a better way to do this, it should be done.
> -  (cl-assert (package-vc-p pkg-desc))
> -  (letrec ((pkg-dir (package-desc-dir pkg-desc))
> -           (vc-flags)
> -           (vc-filter-command-function
> -            (lambda (command file-or-list flags)
> -              (setq vc-flags flags)
> -              (list command file-or-list flags)))
> -           (post-upgrade
> -            (lambda (_command _file-or-list flags)
> -              (when (and (file-equal-p pkg-dir default-directory)
> -                         (eq flags vc-flags))
> -                (unwind-protect
> -                    (with-demoted-errors "Failed to activate: %S"
> -                      (package-vc--unpack-1 pkg-desc pkg-dir))
> -                  (remove-hook 'vc-post-command-functions post-upgrade))))))
> -    (add-hook 'vc-post-command-functions post-upgrade)
> -    (with-demoted-errors "Failed to fetch: %S"
> -      (require 'vc-dir)
> -      (with-current-buffer (vc-dir-prepare-status-buffer
> -                            (format " *package-vc-dir: %s*" pkg-dir)
> -                            pkg-dir (vc-responsible-backend pkg-dir))
> -        (vc-pull)))))
> +  ;; Check if user wants to see diff and confirm upgrade
> +  (when (package-vc--confirm-upgrade pkg-desc)
> +    ;; HACK: To run `package-vc--unpack-1' after checking out the new
> +    ;; revision, we insert a hook into `vc-post-command-functions', and
> +    ;; remove it right after it ran.  To avoid running the hook multiple
> +    ;; times or even for the wrong repository (as `vc-pull' is often
> +    ;; asynchronous), we extract the relevant arguments using a pseudo
> +    ;; filter for `vc-filter-command-function', executed only for the
> +    ;; side effect, and store them in the lexical scope.  When the hook
> +    ;; is run, we check if the arguments are the same (`eq') as the ones
> +    ;; previously extracted, and only in that case will be call
> +    ;; `package-vc--unpack-1'.  Ugh...
> +    ;;
> +    ;; If there is a better way to do this, it should be done.
> +    (cl-assert (package-vc-p pkg-desc))

This assertion should remain at the beginning of the function.

> +    (letrec ((pkg-dir (package-desc-dir pkg-desc))
> +             (vc-flags)
> +             (vc-filter-command-function
> +              (lambda (command file-or-list flags)
> +                (setq vc-flags flags)
> +                (list command file-or-list flags)))
> +             (post-upgrade
> +              (lambda (_command _file-or-list flags)
> +                (when (and (file-equal-p pkg-dir default-directory)
> +                           (eq flags vc-flags))
> +                  (unwind-protect
> +                      (with-demoted-errors "Failed to activate: %S"
> +                        (package-vc--unpack-1 pkg-desc pkg-dir))
> +                    (remove-hook 'vc-post-command-functions post-upgrade))))))
> +      (add-hook 'vc-post-command-functions post-upgrade)
> +      (with-demoted-errors "Failed to fetch: %S"
> +        (require 'vc-dir)
> +        (with-current-buffer (vc-dir-prepare-status-buffer
> +                              (format " *package-vc-dir: %s*" pkg-dir)
> +                              pkg-dir (vc-responsible-backend pkg-dir))
> +          (vc-pull))))))
>  
>  (defun package-vc--archives-initialize ()
>    "Initialize package.el and fetch package specifications."
> @@ -997,7 +1002,150 @@ package-vc-log-incoming
>    (interactive
>     (list (package-vc--read-package-desc "Incoming log for package: " t)))
>    (let ((default-directory (package-desc-dir pkg-desc)))
> -    (call-interactively #'vc-log-incoming)))
> +    ;; Call vc-log-incoming directly without interactive prompting
> +    (vc-log-incoming nil)))
> +
> +;; Package upgrade diff utilities for VC packages
> +
> +(defun package-vc--show-changelog (pkg-desc from-rev to-rev)
> +  "Show diff of all changelog files between FROM-REV and TO-REV for PKG-DESC.
> +Returns t if any changelog diff was displayed, nil otherwise."
> +  (let* ((pkg-dir (package-desc-dir pkg-desc))
> +         (default-directory pkg-dir)
> +         (changelog-files (package--find-changelog-files pkg-dir)))
> +    (if changelog-files
> +        (package-vc--display-all-changelog-diffs changelog-files from-rev to-rev)
> +      (message "No changelog files found in repository")
> +      nil)))
> +
> +(defun package-vc--display-all-changelog-diffs (changelog-files from-rev to-rev)
> +  "Display git diff for all CHANGELOG-FILES between FROM-REV and TO-REV."
> +  (let ((diff-buffer-name "*Package VC Changelog Diff*")
> +        (displayed-any nil))
> +    ;; Kill existing buffer
> +    (when (get-buffer diff-buffer-name)
> +      (kill-buffer diff-buffer-name))
> +
> +    (with-current-buffer (get-buffer-create diff-buffer-name)
> +      (let ((inhibit-read-only t))
> +        (erase-buffer)
> +        (insert (format "Changelog differences between %s and %s:\n" from-rev to-rev))
> +        (insert (make-string 60 ?=))
> +        (insert "\n\n")
> +
> +        ;; Process each changelog file
> +        (dolist (changelog-file changelog-files)
> +          (let ((filename (file-name-nondirectory changelog-file)))
> +            (insert (format "--- %s ---\n" filename))
> +            (condition-case err
> +                (let ((diff-output
> +                       (with-output-to-string
> +                         (with-current-buffer standard-output
> +                           (vc-git-command

This is not acceptable.

> +                            (current-buffer) 0 changelog-file
> +                            "diff" from-rev to-rev
> +                            (format "--unified=%d" package-diff-context-lines)
> +                            "--" (file-relative-name changelog-file))))))
> +                  (if (> (length diff-output) 0)
> +                      (progn
> +                        (insert diff-output)
> +                        (setq displayed-any t))
> +                    (insert "No changes found in this file.\n")))
> +              (error
> +               (insert (format "Error running git diff: %s" (error-message-string err)))))
> +            (insert "\n\n")))
> +
> +        (if displayed-any
> +            (progn
> +              (package--format-changelog-diff)
> +              (goto-char (point-min))
> +              (read-only-mode 1))
> +          (insert "No changelog changes found between the specified revisions.\n")
> +          (read-only-mode 1)))
> +
> +      (display-buffer diff-buffer-name)
> +      displayed-any)))
> +
> +(defun package-vc--upgrade-interactive-prompt (pkg-desc)
> +  "Interactive prompt for VC package upgrade confirmation.
> +PKG-DESC is the VC package descriptor.
> +Returns \\='upgrade to proceed with upgrade, \\='cancel to abort."
> +  (condition-case err
> +      (let ((package-name (package-desc-name pkg-desc))
> +            (prompt-choices (mapcar #'car package-upgrade-interactive-commands))
> +            (result nil)
> +            (pkg-dir (package-desc-dir pkg-desc))
> +            (current-rev nil)
> +            (target-rev nil))
> +
> +        ;; Initialize revisions for changelog comparison
> +        (condition-case _
> +            (let ((default-directory pkg-dir))
> +              (setq current-rev (string-trim (shell-command-to-string "git rev-parse HEAD")))
> +              (setq target-rev "origin/HEAD"))
> +          (error
> +           (setq current-rev "HEAD~1")
> +           (setq target-rev "HEAD")))
> +
> +        (while (not (memq result '(upgrade cancel quit)))
> +          (condition-case input-err
> +              (let ((choice (read-char-choice
> +                             (format package-upgrade-prompt-message package-name)
> +                             prompt-choices)))
> +                (pcase (cdr (assq choice package-upgrade-interactive-commands))
> +                  ('upgrade
> +                   (setq result 'upgrade))
> +                  ('cancel
> +                   (setq result 'cancel))
> +                  ('quit
> +                   (setq result 'quit))
> +                  ('show-diff
> +                   ;; For VC packages, show both log and diff using separate standard VC functions
> +                   (condition-case log-err
> +                       (let ((default-directory (package-desc-dir pkg-desc)))
> +                         ;; Show incoming log
> +                         (vc-log-incoming nil)
> +                         ;; Also show incoming diff
> +                         (vc-root-diff-incoming nil))
> +                     (error
> +                      (message "Error showing incoming changes: %s" (error-message-string log-err)))))
> +                  ('show-changelog
> +                   (condition-case log-err
> +                       (package-vc--show-changelog pkg-desc current-rev target-rev)
> +                     (error
> +                      (message "Error showing changelog: %s" (error-message-string log-err)))))
> +                  (_
> +                   ;; Invalid choice, continue loop
> +                   (message "Invalid choice. Please press y, n, d, c, or q.")
> +                   (sit-for 1))))
> +            (quit
> +             ;; Handle user interruption (C-g)
> +             (setq result 'cancel))
> +            (error
> +             (message "Error during input: %s" (error-message-string input-err))
> +             (sit-for 1))))
> +
> +        result)
> +    (error
> +     (message "Critical error in interactive prompt for %s: %s"
> +              (package-desc-name pkg-desc) (error-message-string err))
> +     'cancel)))
> +
> +(defun package-vc--confirm-upgrade (pkg-desc)
> +  "Show incoming changes for VC package upgrade and ask for confirmation.
> +PKG-DESC is the package descriptor to upgrade.
> +Return t if user wants to proceed, nil otherwise."
> +  ;; Check trust policy
> +  (let ((should-show-diff (package--should-show-diff-p pkg-desc)))
> +    (if should-show-diff
> +        ;; Use interactive prompt for upgrade confirmation
> +        (let ((result (package-vc--upgrade-interactive-prompt pkg-desc)))
> +          (cond
> +           ((eq result 'upgrade) t)
> +           ((eq result 'quit) (user-error "Package upgrade cancelled by user"))
> +           (t nil)))
> +      ;; Auto-upgrade without diff
> +      t)))

I am sorry to say, but I am not convinced by the current approach.  My
suggestion would be to first focus on upgrading tarball packages, and
then we can consider the matter again for vc-packages.

>  (provide 'package-vc)
>  ;;; package-vc.el ends here

Message #71 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Daniel Mendler <mail <at> daniel-mendler.de>, 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: [PATCH v1] package.el: Add diff display and
 confirmation for package upgrades (Bug#74604)
Date: Sat, 13 Sep 2025 22:05:56 +0900

Thank you for reviewing my patch.
I will go through your comments and apply the suggested changes.

> (Not to start a unrelated discussion, but parts of your message sound
> like they might have been written by generative AI.  I don't mind that,
> but it might be an issue if you used GenAI to prepare the patch as well,
> as TTBOMK is not yet a resolved matter for the GNU project.  Related to
> that, have you signed the FSF copyright assignment?)

I am aware of the complexities surrounding FSF copyright from what I 
have seen online,
so I write the actual code by hand. (However, I do ask for advice on 
refactoring methods and best practices.)
Since English is not my native language, I write commit messages and 
emails in my native language, then translate them with AI and 
double-check them using Google Translate. As a result, some nuances or 
expressions may not always be appropriate.
Would such a development workflow be acceptable?

In addition, I have already received the copyright assignment form for 
the FSF and plan to complete and submit it.



On 2025/09/13 21:17, Philip Kaludercic wrote:
> Nobuyuki Kamimoto <kamimoto527 <at> gmail.com> writes:
>
>> Hello Philip and Daniel,
>>
>> Thank you for your detailed feedback on the initial patch. You raised
>> excellent points about avoiding
>> duplication of existing VC functionality and improving the overall approach.
> (Not to start a unrelated discussion, but parts of your message sound
> like they might have been written by generative AI.  I don't mind that,
> but it might be an issue if you used GenAI to prepare the patch as well,
> as TTBOMK is not yet a resolved matter for the GNU project.  Related to
> that, have you signed the FSF copyright assignment?)
>
> [...]
>
>>  From a41c934b9c39e99e9aaf379519f0d66a71b39966 Mon Sep 17 00:00:00 2001
>> From: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>
>> Date: Fri, 12 Sep 2025 21:12:53 +0900
>> Subject: [PATCH] Enhance package upgrade UI with interactive y/n/d/c prompts
>>
>> Add interactive confirmation system for package upgrades with options for:
>> - Yes/No upgrade decisions
>> - Diff display between package versions
>> - Changelog viewing with file exclusion patterns
>> - Configurable confirmation policy per package/archive
>>
>> Includes comprehensive diff utilities for tarball packages and VC packages,
>> with proper error handling and user-friendly display buffers.
> It would be nice if you could rewrite the commit message to align with
> the style described in the CONTRIBUTE file (see the section "Commit
> messages").  The main thing here is that you explicitly annotate how
> which functions have been changed in which files.
>
>> ---
>>   doc/emacs/package.texi          |  42 ++++
>>   etc/NEWS                        |  10 +
>>   lisp/package/package-core.el    | 158 ++++++++++++++
> How much of these changes really have to be part of package-core?  The
> idea behind the ongoing refactoring is to keep that file as small as
> possible, so that it contains just what is necessary to activate
> packages on startup.
>
>>   lisp/package/package-install.el | 356 +++++++++++++++++++++++++++++++-
>>   lisp/package/package-menu.el    |  48 +++--
>>   lisp/package/package-vc.el      | 218 +++++++++++++++----
>>   6 files changed, 774 insertions(+), 58 deletions(-)
>>
>> diff --git a/doc/emacs/package.texi b/doc/emacs/package.texi
>> index 5aa9f9a74bf..3b2a59dda23 100644
>> --- a/doc/emacs/package.texi
>> +++ b/doc/emacs/package.texi
>> @@ -389,6 +389,38 @@ Package Installation
>>   use these bulk commands if you want to update only a small number of
>>   built-in packages.
>>   
>> +@vindex package-upgrade-confirmation-policy
>> +  By default, Emacs shows a diff of the changes when upgrading
>> +packages, allowing you to review what has changed between the current
>> +and new version before proceeding.  This is controlled by the
>> +@code{package-upgrade-confirmation-policy} user option.  When set to @code{t} (the
>> +default), package upgrades will display the differences and ask for
>> +confirmation before proceeding.  You can disable this behavior by
>> +setting @code{package-upgrade-confirmation-policy} to @code{nil} to upgrade
>> +all packages automatically without showing diffs.
>> +
>> +  You can also specify which packages or archives require confirmation by
>> +setting @code{package-upgrade-confirmation-policy} to a list of specific
>> +packages and archives.  Only the packages and archives in the list will show
>> +confirmation prompts, while all others will upgrade automatically.
>> +
>> +@noindent
>> +Examples of list-based configuration:
>> +
>> +@example
>> +;; Only confirm magit and helm upgrades, auto-upgrade everything else
>> +(setq package-upgrade-confirmation-policy
>> +      '((package magit) (package helm)))
>> +
>> +;; Only confirm packages from melpa archive, auto-upgrade others
>> +(setq package-upgrade-confirmation-policy
>> +      '((archive "melpa")))
>> +
>> +;; Mixed: confirm org package and all melpa-stable packages
>> +(setq package-upgrade-confirmation-policy
>> +      '((package org) (archive "melpa-stable")))
>> +@end example
>> +
>>   @cindex package requirements
>>     A package may @dfn{require} certain other packages to be installed,
>>   because it relies on functionality provided by them.  When Emacs
>> @@ -655,6 +687,16 @@ Fetching Package Sources
>>     Note that currently, built-in packages cannot be upgraded using
>>   @code{package-vc-install}.
>>   
>> +  Like regular package upgrades, VC package upgrades can also show
>> +diffs before proceeding.  This behavior is controlled by the same
>> +@code{package-upgrade-confirmation-policy} user option that controls regular
>> +package upgrades.  When set to @code{t} (the default), upgrading VC
>> +packages will display the differences between the current and updated
>> +versions, allowing you to review the changes before confirming the upgrade.
>> +
>> +  VC packages use the same confirmation policy and file exclusion patterns
>> +as regular packages, ensuring consistent behavior across all package types.
>> +
>>   @findex package-report-bug
>>   @findex package-vc-prepare-patch
>>     With the source checkout, you might want to reproduce a bug against
>> diff --git a/etc/NEWS b/etc/NEWS
>> index ac8e56326bf..904592577a0 100644
>> --- a/etc/NEWS
>> +++ b/etc/NEWS
>> @@ -74,6 +74,16 @@ done from early-init.el, such as adding to 'package-directory-list'.
>>   ** 'prettify-symbols-mode' attempts to ignore undisplayable characters.
>>   Previously, such characters would be rendered as, e.g., white boxes.
>>   
>> ++++
>> +** Package management now shows diffs before upgrades.
>> +Package upgrades will now display differences between the current and
>> +new version before proceeding.  This applies to both regular packages
>> +and VC packages installed from version control repositories.
>> +
>> +Users will see a diff buffer showing changes and can choose whether to
>> +proceed with or cancel the upgrade.  This behavior can be controlled
>> +through the 'package-upgrade-confirmation-policy' user option.
>> +
>>   +++
>>   ** 'standard-display-table' now has more extra slots.
>>   'standard-display-table' has been extended to allow specifying glyphs
>> diff --git a/lisp/package/package-core.el b/lisp/package/package-core.el
>> index e52654aa53d..b0b1831e1ed 100644
>> --- a/lisp/package/package-core.el
>> +++ b/lisp/package/package-core.el
>> @@ -284,6 +284,164 @@ package-selected-packages
>>     :version "25.1"
>>     :type '(repeat symbol))
>>   
>> +;; Interactive upgrade prompt constants
>> +(defconst package-upgrade-interactive-commands
>> +  '((?y . upgrade)
>> +    (?n . cancel)
>> +    (?d . show-diff)
>> +    (?c . show-changelog)
>> +    (?q . quit))
>> +  "Interactive commands for package upgrade confirmation.")
>> +
>> +(defconst package-upgrade-prompt-message
>> +  "Upgrade %s? (y)es, (n)o, (d)iff, (c)hangelog, (q)uit: "
>> +  "Prompt message format for package upgrade confirmation.")
>> +
>> +(defconst package-diff-context-lines 3
>> +  "Number of context lines to show in package and changelog diffs.")
> I don't think that we need these declarations as constants?
>
>> +(defconst package-changelog-max-size (* 1024 1024)
>> +  "Maximum size in bytes for changelog files to process.")
>> +
>> +(defcustom package-upgrade-confirmation-policy t
> As Eli said, the default is probably too invasive.
>
>> +  "Policy for confirming packages during upgrades.
>> +This determines which packages require user confirmation before upgrading.
>> +
>> +Possible values:
>> +
>> +\\=`t\\=' - Default
>> +    Show diffs and prompt for all package upgrades.
>> +
>> +\\=`nil\\='
>> +    Automatically upgrade all packages without showing diffs.
>> +
>> +A list of specific packages/archives to confirm
>> +    Only the packages and archives listed will show confirmation prompts.
>> +    All others will be upgraded automatically.
>> +
>> +    List format:
>> +    (package PACKAGE-SYMBOL)  - Show diff for this specific package
> Do you think that it would also be acceptable to have symbols
> interpreted as synonyms to (package ...)?
>
>> +    (archive ARCHIVE-NAME)    - Show diff for all packages in this archive
>> +
>> +Examples:
>> +    \\='((package magit) (package helm))
>> +        - Only confirm magit and helm upgrades
>> +        - All other packages upgrade automatically
>> +
>> +    \\='((archive \"melpa\") (package org))
>> +        - Confirm all melpa archive packages
>> +        - Confirm org package upgrades
>> +        - All other packages upgrade automatically
>> +
>> +    \\='((package magit))
>> +        - Only confirm magit upgrades
>> +        - All other packages upgrade automatically"
>> +  :type '(choice
>> +          (const :tag "Always show diffs" t)
>> +          (const :tag "Never show diffs" nil)
>> +          (repeat :tag "Specific packages/archives to confirm"
>> +                  (choice
>> +                   (list :tag "Package rule" (const package) symbol)
>> +                   (list :tag "Archive rule" (const archive) string))))
>> +  :version "31.1")
>> +
>> +(defun package--match-rule-p (rule package-name package-archive)
> I think it would be cleaner to pass the package-desc object here, than
> to have to extract the information before invoking the function.
>
>> +  "Check if RULE matches PACKAGE-NAME and PACKAGE-ARCHIVE."
>> +  (pcase rule
>> +    (`(package ,name) (eq name package-name))
>> +    (`(archive ,archive) (string= archive package-archive))
>> +    (_ nil)))
>> +
>> +(defun package--should-show-diff-p (pkg-desc)
>> +  "Check if diff should be shown for PKG-DESC based on confirmation policy."
>> +  (let ((package-name (package-desc-name pkg-desc))
>> +        (package-archive (package-desc-archive pkg-desc))
>> +        (policy package-upgrade-confirmation-policy))
>> +    (cond
> Feel free to use `pcase' here as well, to avoid the trivial binding.
>
>> +     ((eq policy t) t)
>> +     ((eq policy nil) nil)
> This is redundant, as if the policy is nil, (listp nil) is true, and
> (seq-some ... nil) is always nil.
>
>> +     ((listp policy)
>> +      ;; List mode: only show diff for packages/archives in the list
>> +      (seq-some (lambda (rule)
>> +                  (package--match-rule-p rule package-name package-archive))
> As the function is used only once, I would actually even inline it here.
>
>> +                policy))
>> +     (t t))))
>> +
>> +(defcustom package-changelog-patterns
>> +  '("CHANGELOG*" "NEWS*" "ChangeLog*" "CHANGES*" "HISTORY*")
>> +  "File patterns to match changelog files."
>> +  :type '(repeat string)
>> +  :version "31.1")
>> +
>> +(defvar package-changelog-file-regex
>> +  (concat "\\(" (mapconcat (lambda (pattern)
>> +                           (replace-regexp-in-string "\\*" ".*" pattern))
>> +                         package-changelog-patterns "\\|") "\\)")
>> +  "Regular expression to match changelog files based on patterns.")
> I think it would be better to check for a designated "news" file, the
> way that `describe-package-1' does.
>
>> +;; Changelog file utilities
>> +(declare-function diff-no-select "diff" (old new &optional switches no-async))
>> +
>> +(defun package--find-changelog-files (directory)
>> +  "Find all changelog files in DIRECTORY using regular expression matching.
>> +Returns a list of paths to all matching changelog files found, or nil if none."
>> +  (when (and directory (stringp directory) (file-directory-p directory))
>> +    (condition-case err
>> +        (let ((files (directory-files directory t "^[^.]" t))
>> +              (candidates nil))
>> +          ;; Collect all matching files
>> +          (dolist (file files)
>> +            (when (and (file-regular-p file)
>> +                       (file-readable-p file)
>> +                       (let ((basename (file-name-nondirectory file)))
>> +                         (string-match-p package-changelog-file-regex
>> +                                         (downcase basename))))
> You can also just bind `case-fold-search' to nil, that way you don't
> have to allocate a new string.
>
>> +              (push file candidates)))
>> +          ;; Return all candidates sorted by name for consistent ordering
>> +          (sort candidates #'string<))
>> +      (file-error
>> +       (message "File access error searching for changelog files in %s: %s"
>> +                directory (error-message-string err))
>> +       nil)
>> +      (error
>> +       (message "Error searching for changelog files in %s: %s"
>> +                directory (error-message-string err))
>> +       nil))))
> I think that you can simplify this using `with-demoted-errors'.
>
>> +
>> +(defun package--diff-available-p ()
>> +  "Check if diff functionality is available.
>> +Since we use built-in Emacs diff functions, this always returns t."
>> +  (require 'diff)
>> +  t)
> Why do we need this?
>
>> +(defun package--diff-changelog-files (old-file new-file)
>> +  "Generate unified diff between OLD-FILE and NEW-FILE.
>> +Returns diff output as string, or nil if diff is not available."
>> +  (when (and old-file new-file
>> +             (file-exists-p old-file)
>> +             (file-exists-p new-file)
>> +             (package--diff-available-p))
>> +    (let ((old-size (nth 7 (file-attributes old-file)))
>> +          (new-size (nth 7 (file-attributes new-file))))
>> +      ;; Check file size limits
>> +      (when (and (< old-size package-changelog-max-size)
>> +                 (< new-size package-changelog-max-size))
>> +        (let ((diff-buffer (diff-no-select old-file new-file
>> +                                           (format "--unified=%d" package-diff-context-lines) t)))
>> +          (when diff-buffer
>> +            (with-current-buffer diff-buffer
>> +              (let ((diff-output (buffer-string)))
>> +                (kill-buffer diff-buffer)
>> +                (when (> (length diff-output) 0)
>> +                  diff-output)))))))))
>> +
>> +(defun package--format-changelog-diff ()
>> +  "Format and highlight diff output in current buffer.
>> +Uses diff-mode features for syntax highlighting."
>> +  (when (fboundp 'diff-mode)
>> +    (diff-mode)
>> +    (font-lock-ensure)))
>> +
>>   ;; Pseudo fields.
>>   (defun package-version-join (vlist)
>>     "Return the version string corresponding to the list VLIST.
>> diff --git a/lisp/package/package-install.el b/lisp/package/package-install.el
>> index 8401a7769b7..34468c109b5 100644
>> --- a/lisp/package/package-install.el
>> +++ b/lisp/package/package-install.el
>> @@ -380,6 +380,7 @@ package-install
>>         (message "`%s' is already installed" name))))
>>   
>>   (declare-function package-vc-upgrade "package-vc" (pkg))
>> +(declare-function diff-no-select "diff" (old new &optional switches no-async))
>>   
>>   ;;;###autoload
>>   (defun package-upgrade (name)
>> @@ -392,16 +393,26 @@ package-upgrade
>>                     (package--upgradeable-packages t) nil t))))
>>     (cl-check-type name symbol)
>>     (let* ((pkg-desc (cadr (assq name package-alist)))
>> -         (package-install-upgrade-built-in (not pkg-desc)))
>> +         (package-install-upgrade-built-in (not pkg-desc))
>> +         (new-pkg-desc (cadr (assq name package-archive-contents))))
>>       ;; `pkg-desc' will be nil when the package is an "active built-in".
>>       (if (and pkg-desc (package-vc-p pkg-desc))
>>           (package-vc-upgrade pkg-desc)
>> -      (when pkg-desc
>> -        (package-delete pkg-desc 'force 'dont-unselect))
>> -      (package-install name
>> -                       ;; An active built-in has never been "selected"
>> -                       ;; before.  Mark it as installed explicitly.
>> -                       (and pkg-desc 'dont-select)))))
>> +      ;; Check if this is a tarball package upgrade that needs diff confirmation
>> +      (if (and pkg-desc new-pkg-desc
>> +               (eq (package-desc-kind new-pkg-desc) 'tar))
>> +          ;; For tarball packages, show diff and ask for confirmation
>> +          ;; The function now handles the complete upgrade process internally
>> +          (unless (package--confirm-tarball-upgrade pkg-desc new-pkg-desc)
>> +            (message "Package upgrade cancelled"))
>> +        ;; For non-tarball packages, proceed with normal upgrade
>> +        (progn
> You can drop the `progn' here, Elisp's if has an implicit trailing progn.
>
>> +          (when pkg-desc
>> +            (package-delete pkg-desc 'force 'dont-unselect))
>> +          (package-install name
>> +                           ;; An active built-in has never been "selected"
>> +                           ;; before.  Mark it as installed explicitly.
>> +                           (and pkg-desc 'dont-select)))))))
>>   
>>   (defun package--upgradeable-packages (&optional include-builtins)
>>     ;; Initialize the package system to get the list of package
>> @@ -1108,5 +1119,336 @@ package-recompile-all
>>       (with-demoted-errors "Error while recompiling: %S"
>>         (package-recompile pkg-desc))))
>>   
>> +;; Package upgrade diff utilities
>> +
>> +(defun package--safe-insert-file (file &optional error-msg)
>> +  "Safely insert FILE contents, showing ERROR-MSG on failure."
>> +  (condition-case err
> Same comment here as above.
>
>> +      (when (and file (file-readable-p file))
>> +        (insert-file-contents file))
>> +    (file-error
>> +     (insert (or error-msg
>> +                 (format "File access error: %s" (error-message-string err)))))
> How can this happen, if we have established prior to
> `insert-file-contents' that the file is readable -- excluding the
> possibility of corrupted permissions due the involvement of multiple
> users or race conditions?
>
>> +    (error
>> +     (insert (or error-msg
>> +                 (format "Error reading file: %s" (error-message-string err)))))))
>> +
>> +(defun package--show-changelog (old-dir new-dir)
>> +  "Show diff of all changelog files between OLD-DIR and NEW-DIR.
>> +Displays unified diff showing what changed in all matched changelog files.
>> +Returns t if any changelog diff was displayed, nil otherwise."
>> +  (let ((old-changelogs (when old-dir (package--find-changelog-files old-dir)))
>> +        (new-changelogs (when new-dir (package--find-changelog-files new-dir)))
>> +        (displayed-any nil))
>> +    (cond
>> +     ;; Both directories have changelog files
>> +     ((or old-changelogs new-changelogs)
>> +      (setq displayed-any (package--display-all-changelog-diffs old-changelogs new-changelogs)))
>> +     ;; No changelog files found in either directory
>> +     (t
>> +      (message "No changelog files found")
>> +      nil))
>> +    displayed-any))
>> +
>> +(defun package--display-all-changelog-diffs (old-files new-files)
>> +  "Display diffs for all changelog files between OLD-FILES and NEW-FILES.
>> +Returns t if any diff was displayed, nil otherwise."
>> +  (let ((buffer-name "*Package Changelog Diff*")
>> +        (displayed-any nil)
>> +        (unique-filenames nil))
>> +    ;; Kill existing buffer if it exists
>> +    (when (get-buffer buffer-name)
>> +      (kill-buffer buffer-name))
>> +
>> +    ;; Get unique filenames from both lists
>> +    (setq unique-filenames
>> +          (delete-dups
>> +           (append (mapcar #'file-name-nondirectory old-files)
>> +                   (mapcar #'file-name-nondirectory new-files))))
>> +
>> +    (with-current-buffer (get-buffer-create buffer-name)
>> +      (let ((inhibit-read-only t))
>> +        (erase-buffer)
>> +        (insert "Changelog differences between old and new versions:\n")
>> +        (insert (make-string 55 ?=))
>> +        (insert "\n\n")
>> +
>> +        ;; Process each unique filename
>> +        (dolist (filename unique-filenames)
>> +          (let ((old-file (car (seq-filter (lambda (f)
>> +                                             (string= filename (file-name-nondirectory f)))
>> +                                           old-files)))
>> +                (new-file (car (seq-filter (lambda (f)
>> +                                             (string= filename (file-name-nondirectory f)))
>> +                                           new-files))))
>> +            (cond
>> +             ;; Both old and new versions exist: show diff
>> +             ((and old-file new-file)
>> +              (package--insert-file-diff filename old-file new-file)
>> +              (setq displayed-any t))
>> +             ;; Only new file exists: show as new addition
>> +             ((and (not old-file) new-file)
>> +              (package--insert-new-file filename new-file)
>> +              (setq displayed-any t))
>> +             ;; Only old file exists: show as removal
>> +             ((and old-file (not new-file))
>> +              (insert (format "--- %s (REMOVED in new version) ---\n\n" filename))
>> +              (setq displayed-any t)))))
>> +
>> +        (if displayed-any
>> +            (progn
>> +              (package--format-changelog-diff)
>> +              (goto-char (point-min))
>> +              (read-only-mode 1))
>> +          (insert "No changelog differences found.\n")
>> +          (read-only-mode 1)))
>> +
>> +      (display-buffer buffer-name)
>> +      displayed-any)))
> IMO it might be nicer if we could use an existing interface like
> tabulated-list-mode.  But just generating a plain, recursive diff would
> also be fine.
>
>> +(defun package--insert-file-diff (filename old-file new-file)
>> +  "Insert diff content for a single file into current buffer."
>> +  (insert (format "--- %s ---\n" filename))
>> +  (let ((diff-output (package--diff-changelog-files old-file new-file)))
>> +    (if diff-output
>> +        (insert diff-output)
>> +      ;; Fallback if diff is unavailable
>> +      (insert "Diff unavailable. Showing full new content:\n")
>> +      (package--safe-insert-file new-file))
>> +    (insert "\n\n")))
>> +
>> +(defun package--insert-new-file (filename new-file)
>> +  "Insert new file content into current buffer."
>> +  (insert (format "--- %s (NEW in this version) ---\n" filename))
>> +  (package--safe-insert-file new-file)
>> +  (insert "\n\n"))
>> +
>> +(defun package--show-package-diff (old-dir new-dir pkg-desc)
>> +  "Show diff between OLD-DIR and NEW-DIR package directories for PKG-DESC.
>> +This function only displays the diff without prompting for user confirmation."
>> +  ;; Ensure diff is loaded and ready before proceeding
>> +  (require 'diff)
>> +  ;; Ensure diff feature is fully loaded on first run
>> +  (unless (featurep 'diff)
>> +    (sit-for 0.1))
> require is not asynchronous!  This is not an issue.
>
>> +  ;; Additional safety: ensure diff functions are available
>> +  (unless (fboundp 'diff-mode)
>> +    (autoload 'diff-mode "diff" "Diff major mode" t))
> This is als not necessary, diff-mode is part of Emacs and you can rely
> on the above require to do the right thing™.
>
>> +  (condition-case outer-err
>> +      (let ((diff-buffer-name "*Package Diff*"))
>> +        ;; Kill existing buffer to avoid read-only issues
>> +        (when (get-buffer diff-buffer-name)
>> +          (kill-buffer diff-buffer-name))
>> +
>> +        ;; Check if directories exist before proceeding
>> +        (unless (and (file-directory-p old-dir) (file-directory-p new-dir))
>> +          (error "One or both directories do not exist: %s, %s" old-dir new-dir))
>> +
>> +        (condition-case diff-err
>> +            ;; Use built-in diff-no-select instead of external commands
> Why?
>
>> +            (let ((diff-buffer (diff-no-select old-dir new-dir "--unified" t)))
>> +              (if diff-buffer
>> +                  (progn
>> +                    ;; Safely configure diff buffer before renaming
>> +                    (with-current-buffer diff-buffer
>> +                      ;; Ensure diff-mode is properly initialized first
>> +                      (when (fboundp 'diff-mode)
>> +                        (diff-mode))
>> +                      ;; Configure buffer for stable scrolling
>> +                      (setq buffer-read-only t)
>> +                      (setq truncate-lines nil)  ; Allow line wrapping
>> +                      ;; Disable potentially problematic diff features
>> +                      (when (boundp 'diff-refine-hunk)
>> +                        (set (make-local-variable 'diff-refine-hunk) nil))
> Why disable this?  This is a useful feature when reading the file.
>
>> +                      (goto-char (point-min))
>> +                      ;; Now safely rename the buffer
>> +                      (rename-buffer diff-buffer-name t))
>> +                    ;; Display the properly configured buffer
>> +                    (display-buffer diff-buffer-name))
>> +                ;; No differences found
>> +                (with-current-buffer (get-buffer-create diff-buffer-name)
>> +                  (let ((inhibit-read-only t))
>> +                    (erase-buffer)
>> +                    (insert (format "No significant differences found between old and new versions of %s.\n"
>> +                                    (package-desc-name pkg-desc)))
>> +                    (goto-char (point-min))
>> +                    (read-only-mode 1))
>> +                  (display-buffer diff-buffer-name))))
>> +          (error
>> +           ;; If diff fails, create a simple error message
>> +           (with-current-buffer (get-buffer-create diff-buffer-name)
>> +             (let ((inhibit-read-only t))
>> +               (erase-buffer)
>> +               (insert (format "Error running diff operation: %s\n\n" (error-message-string diff-err)))
>> +               (insert "This may be due to:\n")
>> +               (insert "- File access permissions\n")
>> +               (insert "- Directory structure issues\n")
>> +               (insert "- Memory limitations for large diffs\n")
>> +               (read-only-mode 1))
>> +             (display-buffer diff-buffer-name))))
>> +        ;; Return t to indicate success
>> +        t)
>> +    (error
>> +     (message "Error showing package diff: %s (Retry may work)" (error-message-string outer-err))
>> +     ;; Return nil to indicate failure but allow retry
>> +     nil)))
>> +
>> +(defun package--handle-interactive-command (command pkg-desc old-dir new-dir)
>> +  "Handle interactive COMMAND for PKG-DESC upgrade."
>> +  (pcase command
>> +    ('upgrade 'upgrade)
> You don't need to quote upgrade here?
>
>> +    ('cancel 'cancel)
>> +    ('show-diff
>> +     (condition-case err
>> +         (if (and old-dir new-dir)
>> +             (progn
>> +               (let ((result (package--show-package-diff old-dir new-dir pkg-desc)))
>> +                 (if result
>> +                     (message "Diff displayed. Press y to upgrade, n to cancel, c for changelog.")
>> +                   (message "Diff display failed. Please try again or use changelog (c)."))))
>> +           (message "Package directories not available for diff."))
>> +       (error
>> +        (message "Error displaying diff: %s (Try again with 'd' or use changelog with 'c')"
>> +                 (error-message-string err))))
>> +     ;; Always return nil to prevent process termination
>> +     nil)
>> +    ('show-changelog
>> +     (condition-case err
>> +         (if (package--show-changelog old-dir new-dir)
>> +             (message "Changelog displayed. Press y to upgrade, n to cancel, d for full diff.")
>> +           (message "No changelog differences found. Press y to upgrade, n to cancel."))
>> +       (error
>> +        (message "Error displaying changelog: %s" (error-message-string err))))
>> +     nil)
>> +    ('quit 'quit)
>> +    (_
>> +     (message "Invalid choice. Use: y=upgrade, n=cancel, d=diff, c=changelog, q=quit")
>> +     (sit-for 1.5)
>> +     nil)))
>> +
>> +(defun package--upgrade-interactive-prompt (pkg-desc &optional old-dir new-dir)
>> +  "Interactive prompt for package upgrade confirmation."
>> +  (let ((package-name (package-desc-name pkg-desc))
>> +        (prompt-choices (mapcar #'car package-upgrade-interactive-commands)))
>> +    (catch 'result
>> +      (while t
>> +        (condition-case err
>> +            (let* ((choice (read-char-choice
>> +                            (format package-upgrade-prompt-message package-name)
>> +                            prompt-choices))
>> +                   (command (cdr (assq choice package-upgrade-interactive-commands)))
>> +                   (result (package--handle-interactive-command command pkg-desc old-dir new-dir)))
>> +              (when result
>> +                (throw 'result result)))
>> +          (quit
>> +           (message "Package upgrade cancelled by user")
>> +           (throw 'result 'cancel))
>> +          (error
>> +           (message "Error during input: %s" (error-message-string err))
>> +           (sit-for 1)))))))
> You have reimplemented `read-multiple-choice'...
>
>> +
>> +(defun package--get-installed-package-dir (pkg-desc)
>> +  "Get directory of installed PKG-DESC package."
>> +  (expand-file-name (package-desc-full-name pkg-desc) package-user-dir))
>> +
>> +(defun package-extract (pkg-desc)
>> +  "Extract package PKG-DESC files without generating autoloads or descriptors.
>> +Only performs file extraction based on package kind.  Returns the package
>> +directory path where files were extracted."
>> +  (let* ((name (package-desc-name pkg-desc))
>> +         (dirname (package-desc-full-name pkg-desc))
>> +         (pkg-dir (expand-file-name dirname package-user-dir)))
>> +    ;; Extract files based on package kind
>> +    (pcase (package-desc-kind pkg-desc)
>> +      ('dir
>> +       (make-directory pkg-dir t)
>> +       (let ((file-list
>> +              (or (and (derived-mode-p 'dired-mode)
>> +                       (dired-get-marked-files))
>> +                  (directory-files-recursively default-directory "" nil))))
>> +         (dolist (source-file file-list)
>> +           (let ((target (expand-file-name
>> +                          (file-relative-name source-file default-directory)
>> +                          pkg-dir)))
>> +             (make-directory (file-name-directory target) t)
>> +             (copy-file source-file target t)))
>> +         ;; Now that the files have been installed, this package is
>> +         ;; indistinguishable from a `tar' or a `single'. Let's make
>> +         ;; things simple by ensuring we're one of them.
>> +         (setf (package-desc-kind pkg-desc)
>> +               (if (length> file-list 1) 'tar 'single))))
>> +      ('tar
>> +       (make-directory package-user-dir t)
>> +       (let* ((default-directory (file-name-as-directory package-user-dir)))
>> +         (package-untar-buffer dirname)))
>> +      ('single
>> +       (let ((el-file (expand-file-name (format "%s.el" name) pkg-dir)))
>> +         (make-directory pkg-dir t)
>> +         (package--write-file-no-coding el-file)))
>> +      (kind (error "Unknown package kind: %S" kind)))
>> +    ;; Return the directory path (no autoloads generation yet)
>> +    pkg-dir))
>> +
>> +(defun package--download-and-extract (new-pkg-desc)
>> +  "Download and extract NEW-PKG-DESC. Return extraction directory."
>> +  (let ((location (package-archive-base new-pkg-desc))
>> +        (file (concat (package-desc-full-name new-pkg-desc)
>> +                      (package-desc-suffix new-pkg-desc))))
>> +    (package--with-response-buffer location :file file
>> +      (package-extract new-pkg-desc))))
>> +
>> +(defun package--confirm-upgrade (_pkg-desc new-pkg-desc old-dir new-dir)
>> +  "Ask user to confirm upgrade from _PKG-DESC to NEW-PKG-DESC."
>> +  (if (file-exists-p old-dir)
>> +      (let ((result (package--upgrade-interactive-prompt new-pkg-desc old-dir new-dir)))
>> +        (cond
>> +         ((eq result 'upgrade) t)
>> +         ((eq result 'quit) (user-error "Package upgrade cancelled by user"))
>> +         (t nil)))
>> +    (yes-or-no-p (format "New package installation: %s. Continue? "
>> +                         (package-desc-name new-pkg-desc)))))
>> +
>> +(defun package--complete-upgrade (pkg-desc new-pkg-desc new-dir)
>> +  "Complete the upgrade process from PKG-DESC to NEW-PKG-DESC."
>> +  (when pkg-desc
>> +    (package-delete pkg-desc 'force 'dont-unselect))
>> +  (when (and new-dir (file-directory-p new-dir))
>> +    (delete-directory new-dir t))
>> +  (package-install-from-archive new-pkg-desc))
>> +
>> +(defun package--confirm-tarball-upgrade (pkg-desc new-pkg-desc)
>> +  "Confirm and execute tarball package upgrade.
>> +
>> +This function handles the complete tarball upgrade workflow:
>> +1. Check if diff display is required based on trust policy
>> +2. Download and extract new package for comparison
>> +3. Show diff and get user confirmation
>> +4. Complete upgrade or clean up on cancellation
>> +
>> +Args:
>> +  PKG-DESC: Current installed package descriptor
>> +  NEW-PKG-DESC: New package descriptor to upgrade to
>> +
>> +Returns:
>> +  t if upgrade was confirmed and completed, nil otherwise."
>> +  (if (package--should-show-diff-p new-pkg-desc)
>> +      (let* ((old-dir (package--get-installed-package-dir pkg-desc))
>> +             (new-dir nil)
>> +             (confirmed nil))
>> +        (unwind-protect
>> +            (progn
>> +              (setq new-dir (package--download-and-extract new-pkg-desc))
>> +              (setq confirmed (package--confirm-upgrade pkg-desc new-pkg-desc old-dir new-dir))
>> +              (when confirmed
>> +                (package--complete-upgrade pkg-desc new-pkg-desc new-dir)))
>> +          ;; Cleanup: remove temporary extraction directory if upgrade was cancelled
>> +          (when (and new-dir (not confirmed) (file-exists-p new-dir))
>> +            (ignore-errors (delete-directory new-dir t))))
>> +        confirmed)
>> +    ;; Trust policy says skip diff - proceed directly with upgrade
>> +    (progn
>> +      (package--complete-upgrade pkg-desc new-pkg-desc nil)
>> +      t)))
>> +
>>   (provide 'package-install)
>>   ;;; package-install.el ends here
>> diff --git a/lisp/package/package-menu.el b/lisp/package/package-menu.el
>> index c57086112c4..3ffdb940554 100644
>> --- a/lisp/package/package-menu.el
>> +++ b/lisp/package/package-menu.el
>> @@ -653,22 +653,38 @@ package-menu--perform-transaction
>>                     (format status-format (incf i)))
>>               (force-mode-line-update)
>>               (redisplay 'force)
>> -            ;; Don't mark as selected, `package-menu-execute' already
>> -            ;; does that.
>> -            (package-install pkg 'dont-select))))
>> -    (let ((package-menu--transaction-status ":Deleting"))
>> -      (force-mode-line-update)
>> -      (redisplay 'force)
>> -      (dolist (elt (package--sort-by-dependence delete-list))
>> -        (condition-case-unless-debug err
>> -            (let ((inhibit-message (or inhibit-message package-menu-async)))
>> -              (package-delete elt nil 'nosave))
>> -          (error
>> -           (push (package-desc-full-name elt) errors)
>> -           (message "Error trying to delete `%s': %s"
>> -                    (package-desc-full-name elt)
>> -                    (error-message-string err))))))
>> -    errors))
>> +            ;; Check if this is a package upgrade and needs confirmation
>> +            (let* ((pkg-name (package-desc-name pkg))
>> +                   (old-pkg (cl-find-if (lambda (del-pkg)
>> +                                          (eq (package-desc-name del-pkg) pkg-name))
>> +                                        delete-list)))
>> +              (cond
>> +               ;; Tarball package upgrade - use tarball confirmation
>> +               ((and old-pkg (eq (package-desc-kind pkg) 'tar))
>> +                (if (package--confirm-tarball-upgrade old-pkg pkg)
>> +                    (package-install pkg 'dont-select)
>> +                  (push (package-desc-full-name pkg) errors)))
>> +               ;; VC package upgrade - use VC confirmation
>> +               ((and old-pkg (package-vc-p old-pkg))
>> +                (if (package-vc--confirm-upgrade old-pkg)
>> +                    (package-install pkg 'dont-select)
>> +                  (push (package-desc-full-name pkg) errors)))
>> +               ;; Normal installation or upgrade
>> +               (t
>> +                (package-install pkg 'dont-select))))))
>> +      (let ((package-menu--transaction-status ":Deleting"))
>> +        (force-mode-line-update)
>> +        (redisplay 'force)
>> +        (dolist (elt (package--sort-by-dependence delete-list))
>> +          (condition-case-unless-debug err
>> +              (let ((inhibit-message (or inhibit-message package-menu-async)))
>> +                (package-delete elt nil 'nosave))
>> +            (error
>> +             (push (package-desc-full-name elt) errors)
>> +             (message "Error trying to delete `%s': %s"
>> +                      (package-desc-full-name elt)
>> +                      (error-message-string err))))))
>> +      errors)))
>>   
>>   (defun package--update-selected-packages (add remove)
>>     "Update the `package-selected-packages' list according to ADD and REMOVE.
>> diff --git a/lisp/package/package-vc.el b/lisp/package/package-vc.el
>> index 03767b99729..3579c9d69b5 100644
>> --- a/lisp/package/package-vc.el
>> +++ b/lisp/package/package-vc.el
>> @@ -53,6 +53,7 @@
>>   (require 'package-elpa)
>>   (require 'lisp-mnt)
>>   (require 'vc)
>> +(require 'vc-git)
> Err, package-vc is intentionally vc agnostic, so we shouldn't depend on
> a specific backend, unless we are providing a speedup (in which case it
> would be better to implement the feature more generally in vc-git).
>
>>   (require 'seq)
>>   
>>   (defgroup package-vc nil
>> @@ -737,6 +738,8 @@ package-vc-upgrade-all
>>   
>>   (declare-function vc-dir-prepare-status-buffer "vc-dir"
>>                     (bname dir backend &optional create-new))
>> +(declare-function vc-diff-incoming "vc" (&optional remote-location fileset))
>> +(declare-function vc-log-incoming "vc" (&optional remote-location))
> You shouldn't need this, as we are requiring vc above!
>
>>   
>>   ;;;###autoload
>>   (defun package-vc-upgrade (pkg-desc)
>> @@ -745,40 +748,42 @@ package-vc-upgrade
>>   This may fail if the local VCS state of the package conflicts
>>   with the remote repository state."
>>     (interactive (list (package-vc--read-package-desc "Upgrade VC package: " t)))
>> -  ;; HACK: To run `package-vc--unpack-1' after checking out the new
>> -  ;; revision, we insert a hook into `vc-post-command-functions', and
>> -  ;; remove it right after it ran.  To avoid running the hook multiple
>> -  ;; times or even for the wrong repository (as `vc-pull' is often
>> -  ;; asynchronous), we extract the relevant arguments using a pseudo
>> -  ;; filter for `vc-filter-command-function', executed only for the
>> -  ;; side effect, and store them in the lexical scope.  When the hook
>> -  ;; is run, we check if the arguments are the same (`eq') as the ones
>> -  ;; previously extracted, and only in that case will be call
>> -  ;; `package-vc--unpack-1'.  Ugh...
>> -  ;;
>> -  ;; If there is a better way to do this, it should be done.
>> -  (cl-assert (package-vc-p pkg-desc))
>> -  (letrec ((pkg-dir (package-desc-dir pkg-desc))
>> -           (vc-flags)
>> -           (vc-filter-command-function
>> -            (lambda (command file-or-list flags)
>> -              (setq vc-flags flags)
>> -              (list command file-or-list flags)))
>> -           (post-upgrade
>> -            (lambda (_command _file-or-list flags)
>> -              (when (and (file-equal-p pkg-dir default-directory)
>> -                         (eq flags vc-flags))
>> -                (unwind-protect
>> -                    (with-demoted-errors "Failed to activate: %S"
>> -                      (package-vc--unpack-1 pkg-desc pkg-dir))
>> -                  (remove-hook 'vc-post-command-functions post-upgrade))))))
>> -    (add-hook 'vc-post-command-functions post-upgrade)
>> -    (with-demoted-errors "Failed to fetch: %S"
>> -      (require 'vc-dir)
>> -      (with-current-buffer (vc-dir-prepare-status-buffer
>> -                            (format " *package-vc-dir: %s*" pkg-dir)
>> -                            pkg-dir (vc-responsible-backend pkg-dir))
>> -        (vc-pull)))))
>> +  ;; Check if user wants to see diff and confirm upgrade
>> +  (when (package-vc--confirm-upgrade pkg-desc)
>> +    ;; HACK: To run `package-vc--unpack-1' after checking out the new
>> +    ;; revision, we insert a hook into `vc-post-command-functions', and
>> +    ;; remove it right after it ran.  To avoid running the hook multiple
>> +    ;; times or even for the wrong repository (as `vc-pull' is often
>> +    ;; asynchronous), we extract the relevant arguments using a pseudo
>> +    ;; filter for `vc-filter-command-function', executed only for the
>> +    ;; side effect, and store them in the lexical scope.  When the hook
>> +    ;; is run, we check if the arguments are the same (`eq') as the ones
>> +    ;; previously extracted, and only in that case will be call
>> +    ;; `package-vc--unpack-1'.  Ugh...
>> +    ;;
>> +    ;; If there is a better way to do this, it should be done.
>> +    (cl-assert (package-vc-p pkg-desc))
> This assertion should remain at the beginning of the function.
>
>> +    (letrec ((pkg-dir (package-desc-dir pkg-desc))
>> +             (vc-flags)
>> +             (vc-filter-command-function
>> +              (lambda (command file-or-list flags)
>> +                (setq vc-flags flags)
>> +                (list command file-or-list flags)))
>> +             (post-upgrade
>> +              (lambda (_command _file-or-list flags)
>> +                (when (and (file-equal-p pkg-dir default-directory)
>> +                           (eq flags vc-flags))
>> +                  (unwind-protect
>> +                      (with-demoted-errors "Failed to activate: %S"
>> +                        (package-vc--unpack-1 pkg-desc pkg-dir))
>> +                    (remove-hook 'vc-post-command-functions post-upgrade))))))
>> +      (add-hook 'vc-post-command-functions post-upgrade)
>> +      (with-demoted-errors "Failed to fetch: %S"
>> +        (require 'vc-dir)
>> +        (with-current-buffer (vc-dir-prepare-status-buffer
>> +                              (format " *package-vc-dir: %s*" pkg-dir)
>> +                              pkg-dir (vc-responsible-backend pkg-dir))
>> +          (vc-pull))))))
>>   
>>   (defun package-vc--archives-initialize ()
>>     "Initialize package.el and fetch package specifications."
>> @@ -997,7 +1002,150 @@ package-vc-log-incoming
>>     (interactive
>>      (list (package-vc--read-package-desc "Incoming log for package: " t)))
>>     (let ((default-directory (package-desc-dir pkg-desc)))
>> -    (call-interactively #'vc-log-incoming)))
>> +    ;; Call vc-log-incoming directly without interactive prompting
>> +    (vc-log-incoming nil)))
>> +
>> +;; Package upgrade diff utilities for VC packages
>> +
>> +(defun package-vc--show-changelog (pkg-desc from-rev to-rev)
>> +  "Show diff of all changelog files between FROM-REV and TO-REV for PKG-DESC.
>> +Returns t if any changelog diff was displayed, nil otherwise."
>> +  (let* ((pkg-dir (package-desc-dir pkg-desc))
>> +         (default-directory pkg-dir)
>> +         (changelog-files (package--find-changelog-files pkg-dir)))
>> +    (if changelog-files
>> +        (package-vc--display-all-changelog-diffs changelog-files from-rev to-rev)
>> +      (message "No changelog files found in repository")
>> +      nil)))
>> +
>> +(defun package-vc--display-all-changelog-diffs (changelog-files from-rev to-rev)
>> +  "Display git diff for all CHANGELOG-FILES between FROM-REV and TO-REV."
>> +  (let ((diff-buffer-name "*Package VC Changelog Diff*")
>> +        (displayed-any nil))
>> +    ;; Kill existing buffer
>> +    (when (get-buffer diff-buffer-name)
>> +      (kill-buffer diff-buffer-name))
>> +
>> +    (with-current-buffer (get-buffer-create diff-buffer-name)
>> +      (let ((inhibit-read-only t))
>> +        (erase-buffer)
>> +        (insert (format "Changelog differences between %s and %s:\n" from-rev to-rev))
>> +        (insert (make-string 60 ?=))
>> +        (insert "\n\n")
>> +
>> +        ;; Process each changelog file
>> +        (dolist (changelog-file changelog-files)
>> +          (let ((filename (file-name-nondirectory changelog-file)))
>> +            (insert (format "--- %s ---\n" filename))
>> +            (condition-case err
>> +                (let ((diff-output
>> +                       (with-output-to-string
>> +                         (with-current-buffer standard-output
>> +                           (vc-git-command
> This is not acceptable.
>
>> +                            (current-buffer) 0 changelog-file
>> +                            "diff" from-rev to-rev
>> +                            (format "--unified=%d" package-diff-context-lines)
>> +                            "--" (file-relative-name changelog-file))))))
>> +                  (if (> (length diff-output) 0)
>> +                      (progn
>> +                        (insert diff-output)
>> +                        (setq displayed-any t))
>> +                    (insert "No changes found in this file.\n")))
>> +              (error
>> +               (insert (format "Error running git diff: %s" (error-message-string err)))))
>> +            (insert "\n\n")))
>> +
>> +        (if displayed-any
>> +            (progn
>> +              (package--format-changelog-diff)
>> +              (goto-char (point-min))
>> +              (read-only-mode 1))
>> +          (insert "No changelog changes found between the specified revisions.\n")
>> +          (read-only-mode 1)))
>> +
>> +      (display-buffer diff-buffer-name)
>> +      displayed-any)))
>> +
>> +(defun package-vc--upgrade-interactive-prompt (pkg-desc)
>> +  "Interactive prompt for VC package upgrade confirmation.
>> +PKG-DESC is the VC package descriptor.
>> +Returns \\='upgrade to proceed with upgrade, \\='cancel to abort."
>> +  (condition-case err
>> +      (let ((package-name (package-desc-name pkg-desc))
>> +            (prompt-choices (mapcar #'car package-upgrade-interactive-commands))
>> +            (result nil)
>> +            (pkg-dir (package-desc-dir pkg-desc))
>> +            (current-rev nil)
>> +            (target-rev nil))
>> +
>> +        ;; Initialize revisions for changelog comparison
>> +        (condition-case _
>> +            (let ((default-directory pkg-dir))
>> +              (setq current-rev (string-trim (shell-command-to-string "git rev-parse HEAD")))
>> +              (setq target-rev "origin/HEAD"))
>> +          (error
>> +           (setq current-rev "HEAD~1")
>> +           (setq target-rev "HEAD")))
>> +
>> +        (while (not (memq result '(upgrade cancel quit)))
>> +          (condition-case input-err
>> +              (let ((choice (read-char-choice
>> +                             (format package-upgrade-prompt-message package-name)
>> +                             prompt-choices)))
>> +                (pcase (cdr (assq choice package-upgrade-interactive-commands))
>> +                  ('upgrade
>> +                   (setq result 'upgrade))
>> +                  ('cancel
>> +                   (setq result 'cancel))
>> +                  ('quit
>> +                   (setq result 'quit))
>> +                  ('show-diff
>> +                   ;; For VC packages, show both log and diff using separate standard VC functions
>> +                   (condition-case log-err
>> +                       (let ((default-directory (package-desc-dir pkg-desc)))
>> +                         ;; Show incoming log
>> +                         (vc-log-incoming nil)
>> +                         ;; Also show incoming diff
>> +                         (vc-root-diff-incoming nil))
>> +                     (error
>> +                      (message "Error showing incoming changes: %s" (error-message-string log-err)))))
>> +                  ('show-changelog
>> +                   (condition-case log-err
>> +                       (package-vc--show-changelog pkg-desc current-rev target-rev)
>> +                     (error
>> +                      (message "Error showing changelog: %s" (error-message-string log-err)))))
>> +                  (_
>> +                   ;; Invalid choice, continue loop
>> +                   (message "Invalid choice. Please press y, n, d, c, or q.")
>> +                   (sit-for 1))))
>> +            (quit
>> +             ;; Handle user interruption (C-g)
>> +             (setq result 'cancel))
>> +            (error
>> +             (message "Error during input: %s" (error-message-string input-err))
>> +             (sit-for 1))))
>> +
>> +        result)
>> +    (error
>> +     (message "Critical error in interactive prompt for %s: %s"
>> +              (package-desc-name pkg-desc) (error-message-string err))
>> +     'cancel)))
>> +
>> +(defun package-vc--confirm-upgrade (pkg-desc)
>> +  "Show incoming changes for VC package upgrade and ask for confirmation.
>> +PKG-DESC is the package descriptor to upgrade.
>> +Return t if user wants to proceed, nil otherwise."
>> +  ;; Check trust policy
>> +  (let ((should-show-diff (package--should-show-diff-p pkg-desc)))
>> +    (if should-show-diff
>> +        ;; Use interactive prompt for upgrade confirmation
>> +        (let ((result (package-vc--upgrade-interactive-prompt pkg-desc)))
>> +          (cond
>> +           ((eq result 'upgrade) t)
>> +           ((eq result 'quit) (user-error "Package upgrade cancelled by user"))
>> +           (t nil)))
>> +      ;; Auto-upgrade without diff
>> +      t)))
> I am sorry to say, but I am not convinced by the current approach.  My
> suggestion would be to first focus on upgrading tarball packages, and
> then we can consider the matter again for vc-packages.
>
>>   (provide 'package-vc)
>>   ;;; package-vc.el ends here

Message #74 received at 74604 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Nobuyuki Kamimoto <kamimoto527 <at> gmail.com>
Cc: Daniel Mendler <mail <at> daniel-mendler.de>, 74604 <at> debbugs.gnu.org
Subject: Re: bug#74604: [PATCH v1] package.el: Add diff display and
 confirmation for package upgrades (Bug#74604)
Date: Sat, 13 Sep 2025 22:13:57 +0000

Nobuyuki Kamimoto <kamimoto527 <at> gmail.com> writes:

> Thank you for reviewing my patch.
> I will go through your comments and apply the suggested changes.
>
>> (Not to start a unrelated discussion, but parts of your message sound
>> like they might have been written by generative AI.  I don't mind that,
>> but it might be an issue if you used GenAI to prepare the patch as well,
>> as TTBOMK is not yet a resolved matter for the GNU project.  Related to
>> that, have you signed the FSF copyright assignment?)
>
> I am aware of the complexities surrounding FSF copyright from what I
> have seen online,
> so I write the actual code by hand. (However, I do ask for advice on
> refactoring methods and best practices.)
> Since English is not my native language, I write commit messages and
> emails in my native language, then translate them with AI and
> double-check them using Google Translate. As a result, some nuances or
> expressions may not always be appropriate.
> Would such a development workflow be acceptable?

I cannot claim that nobody will complain, but I have no issue with you
using generative AI as a spell/grammar checker.  The only thing were we
have to watch out is with generated code.

But feel free to ask question here (or if you don't want it to end up on
the mailing list I'd gladly give you my input as well in personal
correspondence) if you want to learn more about best practice.  As you
might have seen in my review of your last patch, you ended up writing a
lot more code than is actually necessary, and I feel that humans are
still a bit better at recognising patterns like these in niche languages
like Elisp :)

> In addition, I have already received the copyright assignment form for
> the FSF and plan to complete and submit it.

OK, so we will have to wait on that anyway.  But we can continue the
discussion here so that the one doesn't block the other, and vice versa.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.