bug#14967: 24.3.50; package-delete deletes by trashing?

bug#14967: 24.3.50; package-delete deletes by trashing?

[Sebastian Wiesner]

Currently "package-delete" from package.el is implemented as follows:

(defun package-delete (pkg-desc)
  (let ((dir (package-desc-dir pkg-desc)))
    (if (not (string-prefix-p (file-name-as-directory
                               (expand-file-name package-user-dir))
                              (expand-file-name dir)))
        ;; Don't delete "system" packages.
	(error "Package `%s' is a system package, not deleting"
               (package-desc-full-name pkg-desc))
      (delete-directory dir t t)
      ;; Update package-alist.
      (let* ((name (package-desc-name pkg-desc)))
        (delete pkg-desc (assq name package-alist)))
      (message "Package `%s' deleted." (package-desc-full-name
  pkg-desc)))))

Note that the call to "delete-directory" gives t as the 3rd argument,
thus allowing for deletion by moving to trash depending on
"delete-by-moving-to-trash".

Is that intended behavior?  If so, what is the rationale behind it?

bug#14967: 24.3.50; package-delete deletes by trashing?

[Glenn Morris]

Sebastian Wiesner wrote:

> Note that the call to "delete-directory" gives t as the 3rd argument,
> thus allowing for deletion by moving to trash depending on
> "delete-by-moving-to-trash".
>
> Is that intended behavior?  If so, what is the rationale behind it?

I guess. Don't know. Maybe so you can undo a delete?

What's the bug?

bug#14967: 24.3.50; package-delete deletes by trashing?

[Sebastian Wiesner]

2013/7/30 Glenn Morris <rgm@gnu.org>:
> Sebastian Wiesner wrote:
>
>> Note that the call to "delete-directory" gives t as the 3rd argument,
>> thus allowing for deletion by moving to trash depending on
>> "delete-by-moving-to-trash".
>>
>> Is that intended behavior?  If so, what is the rationale behind it?
>
> I guess. Don't know. Maybe so you can undo a delete?
>
> What's the bug?

Well, obviously I consider this behavior a bug.  Trash is no place for
deleted packages.  I mean, does apt-get remove use the Trash?!

Undoing package removal by simply restoring from Trash does not work
reliably anyway, so what's the point of this, despite cluttering the
user's trash can?

PS: Please excuse the duplicate mail.  I had accidentally only mailed
you directly, and forgot to CC debbugs.  Sorry for any inconvenience.

bug#14967: 24.3.50; package-delete deletes by trashing?

[Stefan Monnier]

> Well, obviously I consider this behavior a bug.  Trash is no place for
> deleted packages.  I mean, does apt-get remove use the Trash?!

I think the reasoning goes as follows:
Emacs uses "trash" when deleting a file upon the user's request (as
opposed to things like temp files used internally).


        Stefan

bug#14967: 24.3.50; package-delete deletes by trashing?

[Sebastian Wiesner]

2013/7/30 Stefan Monnier <monnier@iro.umontreal.ca>:
>> Well, obviously I consider this behavior a bug.  Trash is no place for
>> deleted packages.  I mean, does apt-get remove use the Trash?!
>
> I think the reasoning goes as follows:
> Emacs uses "trash" when deleting a file upon the user's request (as
> opposed to things like temp files used internally).

Emacs doesn't delete “files” here, it deletes “packages”.  Yes,
packages are just a bunch of files in the end, but conceptually a
package is not just a plain bunch of files.  It is a complex opaque
data structure providing Emacs Lisp code, with some complex
management/activation logic behind.

For this reason, a package can't be treated as a plain directory with
files.  For instance, restoring a package from trash doesn't restore
the package for any running Emacs session, simply because "package.el"
doesn't know about it.

Also, "package-delete" may be called in situations other than an
explicit user request.

bug#14967: 24.3.50; package-delete deletes by trashing?

[Stefan Monnier]

> For this reason, a package can't be treated as a plain directory with
> files.

It's pretty close, tho.

> For instance, restoring a package from trash doesn't restore
> the package for any running Emacs session,

I'm not sure what you mean.  Let's say you have a running
Emacs session.  What's the difference between:
- restore the package from trash.
and
- start a new Emacs session, (re)install the package, exit.
AFAIK, the running Emacs session will be affected in exactly the
same way.


        Stefan

bug#14967: 24.3.50; package-delete deletes by trashing?

[npostavs]

tags 14967 notabug
quit

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

>> For this reason, a package can't be treated as a plain directory with
>> files.
>
> It's pretty close, tho.
>
>> For instance, restoring a package from trash doesn't restore
>> the package for any running Emacs session,
>
> I'm not sure what you mean.  Let's say you have a running
> Emacs session.  What's the difference between:
> - restore the package from trash.
> and
> - start a new Emacs session, (re)install the package, exit.
> AFAIK, the running Emacs session will be affected in exactly the
> same way.

Hence notabug, I guess?

bug#14967: 24.3.50; package-delete deletes by trashing?

[Alex]

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

>> Well, obviously I consider this behavior a bug.  Trash is no place for
>> deleted packages.  I mean, does apt-get remove use the Trash?!
>
> I think the reasoning goes as follows:
> Emacs uses "trash" when deleting a file upon the user's request (as
> opposed to things like temp files used internally).
>
>
>         Stefan

Could there perhaps be a separate configuration option for deleting
packages versus trashing them? I usually like Emacs using the trash, but
I don't see a reason why packages should go there myself.

bug#14967: 24.3.50; package-delete deletes by trashing?

[npostavs]

severity 14967 wishlist
quit

Alex <agrambot@gmail.com> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> Well, obviously I consider this behavior a bug.  Trash is no place for
>>> deleted packages.  I mean, does apt-get remove use the Trash?!
>>
>> I think the reasoning goes as follows:
>> Emacs uses "trash" when deleting a file upon the user's request (as
>> opposed to things like temp files used internally).
>
> Could there perhaps be a separate configuration option for deleting
> packages versus trashing them? I usually like Emacs using the trash, but
> I don't see a reason why packages should go there myself.

Seems reasonable.

bug#14967: 24.3.50; package-delete deletes by trashing?

[Alex]

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

npostavs@users.sourceforge.net writes:

> severity 14967 wishlist
> quit
>
> Alex <agrambot@gmail.com> writes:
>
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>
>>>> Well, obviously I consider this behavior a bug.  Trash is no place for
>>>> deleted packages.  I mean, does apt-get remove use the Trash?!
>>>
>>> I think the reasoning goes as follows:
>>> Emacs uses "trash" when deleting a file upon the user's request (as
>>> opposed to things like temp files used internally).
>>
>> Could there perhaps be a separate configuration option for deleting
>> packages versus trashing them? I usually like Emacs using the trash, but
>> I don't see a reason why packages should go there myself.
>
> Seems reasonable.

Here's a patch for it.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 2022 bytes --]

From cf3039f10846155fde224a1f6c249799ab7cf41d Mon Sep 17 00:00:00 2001
From: Alexander Gramiak <agrambot@gmail.com>
Date: Sun, 11 Jun 2017 21:12:24 -0600
Subject: [PATCH] Make package-delete conditionally not use the trash

Some users would like to trash other files, but not old package
files (Bug#14967).

* lisp/emacs-lisp/package.el: Add `package-trash-old-packages'.
* etc/NEWS: Document it.
---
 etc/NEWS                   | 3 +++
 lisp/emacs-lisp/package.el | 7 ++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index 7972511f7a..fa1e5c1312 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -743,6 +743,9 @@ header's value.
 where the GnuPG home directory (used for signature verification) is
 located and whether GnuPG's option "--homedir" is used or not.
 
+*** The new variable 'package-trash-old-packages' has been added to
+control whether or not 'package-delete' trashes old packages files.
+
 ** Tramp
 
 +++
diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index bebfd18d7a..5173a32e33 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2071,6 +2071,11 @@ package--newest-p
   (equal (cadr (assq (package-desc-name pkg) package-alist))
          pkg))
 
+(defvar package-trash-old-packages t
+  "If non-nil, `package-delete' may trash packages.
+`delete-by-moving-to-trash' must be non-nil for this variable to
+have an effect.")
+
 (defun package-delete (pkg-desc &optional force nosave)
   "Delete package PKG-DESC.
 
@@ -2127,7 +2132,7 @@ package-delete
                   (package-desc-name pkg-used-elsewhere-by)))
           (t
            (add-hook 'post-command-hook #'package-menu--post-refresh)
-           (delete-directory dir t t)
+           (delete-directory dir t package-trash-old-packages)
            ;; Remove NAME-VERSION.signed and NAME-readme.txt files.
            (dolist (suffix '(".signed" "readme.txt"))
              (let* ((version (package-version-join (package-desc-version pkg-desc)))
-- 
2.11.0

bug#14967: 24.3.50; package-delete deletes by trashing?

[Eli Zaretskii]

> From: Alex <agrambot@gmail.com>
> Date: Sun, 11 Jun 2017 21:26:20 -0600
> Cc: Sebastian Wiesner <lunaryorn@gmail.com>,
> 	Stefan Monnier <monnier@iro.umontreal.ca>, 14967@debbugs.gnu.org
> 
> Here's a patch for it.

Thanks.

> +*** The new variable 'package-trash-old-packages' has been added to
> +control whether or not 'package-delete' trashes old packages files.

This should say something about the default behavior.

> +(defvar package-trash-old-packages t
> +  "If non-nil, `package-delete' may trash packages.

Why "may"?

> +`delete-by-moving-to-trash' must be non-nil for this variable to
> +have an effect.")

I think it would be better to make this defcustom.

bug#14967: 24.3.50; package-delete deletes by trashing?

[Glenn Morris]

Does anyone actually want deleted packages to go to trash?

Yours-concerned-about-yet-another-option-ly,

bug#14967: 24.3.50; package-delete deletes by trashing?

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: Alex <agrambot@gmail.com>,  lunaryorn@gmail.com,  monnier@iro.umontreal.ca,  14967@debbugs.gnu.org,  npostavs@users.sourceforge.net
> Date: Mon, 12 Jun 2017 12:03:05 -0400
> 
> 
> Does anyone actually want deleted packages to go to trash?

Do you or did you ever use the trash facility?  Because there are
people who never ever actually delete files, only move them to trash.
(No, I'm not one of them, but I hardly matter in this case.)

bug#14967: 24.3.50; package-delete deletes by trashing?

[Glenn Morris]

Eli Zaretskii wrote:

> Do you or did you ever use the trash facility?

No. But the people who do and commented in this report don't want it to
apply to packages. So I think it's worth thinking, before adding a new
option, if anyone actually wants the current behaviour.

bug#14967: 24.3.50; package-delete deletes by trashing?

[Stefan Monnier]

>> Do you or did you ever use the trash facility?
> No. But the people who do and commented in this report don't want it to
> apply to packages. So I think it's worth thinking, before adding a new
> option, if anyone actually wants the current behaviour.

I'm the kind of user who'd rather alias "rm" to "rm -f" than to "rm -i"
and then rely on automated backups.  And since the "trash" system is not
an automated backup in my view, I indeed have no idea if there are users
who'd want package.el to use the trash.

And I agree it's worth asking.


        Stefan

bug#14967: 24.3.50; package-delete deletes by trashing?

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: agrambot@gmail.com,  lunaryorn@gmail.com,  monnier@iro.umontreal.ca,  14967@debbugs.gnu.org,  npostavs@users.sourceforge.net
> Date: Mon, 12 Jun 2017 12:56:08 -0400
> 
> Eli Zaretskii wrote:
> 
> > Do you or did you ever use the trash facility?
> 
> No. But the people who do and commented in this report don't want it to
> apply to packages. So I think it's worth thinking, before adding a new
> option, if anyone actually wants the current behaviour.

Fine with me.  All I said was that if we do add a variable, it should
be a defcustom; defvar makes little sense to me.

bug#14967: 24.3.50; package-delete deletes by trashing?

[Stefan Monnier]

>> No. But the people who do and commented in this report don't want it to
>> apply to packages. So I think it's worth thinking, before adding a new
>> option, if anyone actually wants the current behaviour.
> Fine with me.  All I said was that if we do add a variable, it should
> be a defcustom; defvar makes little sense to me.

Maybe the most effective way to "ask" users is to just change the code
so it doesn't use the trash system (mentioning it in NEWS, of course).
And then wait to see if someone complains.


        Stefan

bug#14967: 24.3.50; package-delete deletes by trashing?

[Alex]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> +(defvar package-trash-old-packages t
>> +  "If non-nil, `package-delete' may trash packages.
>
> Why "may"?

Since after this patch, `package-delete' may trash packages, or may not,
depending on the value of `delete-by-moving-to-trash'.

>> +`delete-by-moving-to-trash' must be non-nil for this variable to
>> +have an effect.")
>
> I think it would be better to make this defcustom.

Right, I forgot to do that. I updated and attached a new patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: trash2 --]
[-- Type: text/x-patch, Size: 2065 bytes --]

From ee33b113826efb6130dfe2d9c568b8e4ddc80eb2 Mon Sep 17 00:00:00 2001
From: Alexander Gramiak <agrambot@gmail.com>
Date: Sun, 11 Jun 2017 21:12:24 -0600
Subject: [PATCH] Make package-delete conditionally not use the trash

Some users would like to trash other files, but not old package
files (Bug#14967).

* lisp/emacs-lisp/package.el: Add `package-trash-old-packages'.
---
 etc/NEWS                   | 4 ++++
 lisp/emacs-lisp/package.el | 9 ++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index 7972511f7a..ee99d114a7 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -743,6 +743,10 @@ header's value.
 where the GnuPG home directory (used for signature verification) is
 located and whether GnuPG's option "--homedir" is used or not.
 
+*** The new variable 'package-trash-old-packages' has been added to
+control whether or not 'package-delete' trashes old packages
+files. The default behaviour is to trash old packages.
+
 ** Tramp
 
 +++
diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index bebfd18d7a..c92ce19e6c 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -368,6 +368,13 @@ package-menu-async
   :type 'boolean
   :version "25.1")
 
+(defcustom package-trash-old-packages t
+  "If non-nil, `package-delete' may trash packages.
+`delete-by-moving-to-trash' must be non-nil for this variable to
+have an effect."
+  :type 'boolean
+  :version "26.1")
+
 \f
 ;;; `package-desc' object definition
 ;; This is the struct used internally to represent packages.
@@ -2127,7 +2134,7 @@ package-delete
                   (package-desc-name pkg-used-elsewhere-by)))
           (t
            (add-hook 'post-command-hook #'package-menu--post-refresh)
-           (delete-directory dir t t)
+           (delete-directory dir t package-trash-old-packages)
            ;; Remove NAME-VERSION.signed and NAME-readme.txt files.
            (dolist (suffix '(".signed" "readme.txt"))
              (let* ((version (package-version-join (package-desc-version pkg-desc)))
-- 
2.11.0

bug#14967: 24.3.50; package-delete deletes by trashing?

[Alex]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>> No. But the people who do and commented in this report don't want it to
>>> apply to packages. So I think it's worth thinking, before adding a new
>>> option, if anyone actually wants the current behaviour.
>> Fine with me.  All I said was that if we do add a variable, it should
>> be a defcustom; defvar makes little sense to me.
>
> Maybe the most effective way to "ask" users is to just change the code
> so it doesn't use the trash system (mentioning it in NEWS, of course).
> And then wait to see if someone complains.
>
>
>         Stefan

Is that a good idea for new behaviour that results in potential data
loss (from the view of a user that expects `package-delete' to trash
files rather than outright deleting them)? It might be easy to gloss
over an entry in NEWS, after all.

I don't like the default behaviour myself, so I don't personally care
either way.

bug#14967: 24.3.50; package-delete deletes by trashing?

[Glenn Morris]

Alex wrote:

> Is that a good idea for new behaviour that results in potential data
> loss (from the view of a user that expects `package-delete' to trash
> files rather than outright deleting them)? It might be easy to gloss
> over an entry in NEWS, after all.

IMO there's no real potential for data loss, since you can just
reinstall the package from its source. (I assume this is what people do
in practice, rather than un-trashing.) Although "source is no longer
available" is in theory possible, in practice I don't think it's a real
concern.

bug#14967: 24.3.50; package-delete deletes by trashing?

[Alex]

Glenn Morris <rgm@gnu.org> writes:

> Alex wrote:
>
>> Is that a good idea for new behaviour that results in potential data
>> loss (from the view of a user that expects `package-delete' to trash
>> files rather than outright deleting them)? It might be easy to gloss
>> over an entry in NEWS, after all.
>
> IMO there's no real potential for data loss, since you can just
> reinstall the package from its source. (I assume this is what people do
> in practice, rather than un-trashing.) Although "source is no longer
> available" is in theory possible, in practice I don't think it's a real
> concern.

Usually, though someone might have edited packages inside
package-user-dir and might be saved from an accidental deletion by the
trash can (this happened to me once or twice). If this is deemed to be
too uncommon (it probably is), then it probably won't be too bad to just
make package-delete always delete.

bug#14967: 24.3.50; package-delete deletes by trashing?

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  >   And since the "trash" system is not
  > an automated backup in my view, I indeed have no idea if there are users
  > who'd want package.el to use the trash.

Poll the users?

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.

bug#14967: 24.3.50; package-delete deletes by trashing?

[Glenn Morris]

I've applied the following.

IMO if Emacs wants to behave like system package managers (eg yum, apt),
it should (optionally) cache the _source_ when it downloads a package.
Then use that (if appropriate) if asked to (re)install.

commit 65b323e (HEAD, master)
Date:   Mon Jun 19 14:39:25 2017 -0400

    Don't put deleted packages in the trash (bug#14967)
    
    * lisp/emacs-lisp/package.el (package-delete):
    Don't pay attention to delete-by-moving-to-trash.
    
    ; * etc/NEWS: Mention this.

diff --git a/etc/NEWS b/etc/NEWS
index 85d1273..78d37484 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -755,6 +755,9 @@ header's value.
 where the GnuPG home directory (used for signature verification) is
 located and whether GnuPG's option "--homedir" is used or not.
 
+---
+*** Deleting a package no longer respects 'delete-by-moving-to-trash'.
+
 ** Tramp
 
 +++
diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index bebfd18..4245294 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2127,7 +2127,7 @@ If NOSAVE is non-nil, the package is not removed from
                   (package-desc-name pkg-used-elsewhere-by)))
           (t
            (add-hook 'post-command-hook #'package-menu--post-refresh)
-           (delete-directory dir t t)
+           (delete-directory dir t)
            ;; Remove NAME-VERSION.signed and NAME-readme.txt files.
            (dolist (suffix '(".signed" "readme.txt"))
              (let* ((version (package-version-join (package-desc-version pkg-desc)))