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

Previous Next

Full log

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

Previous Next

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