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: 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: 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.