Package: emacs;
Reported by: Daniel Mendler <mail <at> daniel-mendler.de>
Date: Fri, 29 Nov 2024 15:40:02 UTC
Severity: wishlist
Found in version 30.0.92
View this message in rfc822 format
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: 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)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.