bug#52508: [PATCH] Option for vc-delete-file to keep file on disk

bug#52508: [PATCH] Option for vc-delete-file to keep file on disk

[Ashwin Kafle]

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

Hello,
I've created the attached patch to have vc-delete file to keep files on
disk using a prefix argument. I've only tested it for vc-git.

I've already signed the copyright papers.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Option-for-vc-delete-file-to-keep-file-on-disk.patch --]
[-- Type: text/x-diff, Size: 4801 bytes --]

From 8bfbc34afa760c4d526fa15d8dddc78f2be78a81 Mon Sep 17 00:00:00 2001
From: Ashwin Kafle <ashwin@ashwink.com.np>
Date: Wed, 15 Dec 2021 15:25:04 +0545
Subject: [PATCH] Option for vc-delete-file to keep file on disk

Add a prefix argument on vc-delete-file to keep affected
file on disk and keep the current buffer intact. This option
relies on the backends to not delete files themselves.

* doc/emacs/vc1-xtra.texi: Document the change.
* lisp/vc/vc-git.el: Make git leave files on disk.
* lisp/vc/vc.el: Change vc-delete-file to accept optional prefix argument.
---
 doc/emacs/vc1-xtra.texi |  3 ++-
 etc/NEWS                |  4 ++++
 lisp/vc/vc-git.el       |  4 ++--
 lisp/vc/vc.el           | 20 +++++++++++---------
 4 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/doc/emacs/vc1-xtra.texi b/doc/emacs/vc1-xtra.texi
index 4cd00cba6c..2cf69583b3 100644
--- a/doc/emacs/vc1-xtra.texi
+++ b/doc/emacs/vc1-xtra.texi
@@ -122,7 +122,8 @@ VC Delete/Rename
   If you wish to delete a version-controlled file, use the command
 @kbd{M-x vc-delete-file}.  This prompts for the file name, and deletes
 it via the version control system.  The file is removed from the
-working tree, and in the VC Directory buffer
+working tree, and in the VC Directory buffer. If you give a prefix argument,
+the file is not deleted from disk.
 @iftex
 (@pxref{VC Directory Mode,,, emacs, the Emacs Manual}),
 @end iftex
diff --git a/etc/NEWS b/etc/NEWS
index 8d83b2a7e3..2469060a3d 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -372,6 +372,10 @@ info node.  This command only works for the Emacs and Emacs Lisp manuals.
 
 ** vc
 
+*** 'C-x v x' accepts a prefix argument to keep file on disk
+Previously 'C-x v x' always deleted the selected file. Now if you give it
+prefix argument, it will keep the buffer and file on disk intact.
+Currently this is only implemented for vc-git.
 ---
 *** 'C-x v v' on an unregistered file will now use the most specific backend.
 Previously, if you had an SVN-covered "~/" directory, and a Git-covered
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 5c6a39aec9..69ef216529 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1569,8 +1569,8 @@ vc-git-next-revision
     (or (vc-git-symbolic-commit next-rev) next-rev)))
 
 (defun vc-git-delete-file (file)
-  (vc-git-command nil 0 (vc-git--literal-pathspec file) "rm" "-f" "--"))
-
+  (vc-git-command nil 0 (vc-git--literal-pathspec file) "rm" "-f" "--cached" "--"))
+)
 (defun vc-git-rename-file (old new)
   (vc-git-command nil 0 (list old new) "mv" "-f" "--"))
 
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 64f752f248..43fc0e787e 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2970,14 +2970,13 @@ vc-transfer-file
       (vc-checkin file new-backend comment (stringp comment)))))
 
 ;;;###autoload
-(defun vc-delete-file (file)
+(defun vc-delete-file (file &optional keep-file)
   "Delete file and mark it as such in the version control system.
 If called interactively, read FILE, defaulting to the current
-buffer's file name if it's under version control."
-  (interactive (list (read-file-name "VC delete file: " nil
-                                     (when (vc-backend buffer-file-name)
-                                       buffer-file-name)
-                                     t)))
+buffer's file name if it's under version control.
+If a prefix argument is given (optional argument KEEP-FILE) then
+don't delete the file from the disk"
+  (interactive "f\nP")
   (setq file (expand-file-name file))
   (let ((buf (get-file-buffer file))
         (backend (vc-backend file)))
@@ -2999,19 +2998,22 @@ vc-delete-file
     (unless (or (file-directory-p file) (null make-backup-files)
                 (not (file-exists-p file)))
       (with-current-buffer (or buf (find-file-noselect file))
-	(let ((backup-inhibited nil))
+	(let ((backup-inhibited nil)
+              ;; if you don't set this, then for some reason, the file is never brought back
+              (backup-by-copying t))
 	  (backup-buffer))))
     ;; Bind `default-directory' so that the command that the backend
     ;; runs to remove the file is invoked in the correct context.
     (let ((default-directory (file-name-directory file)))
       (vc-call-backend backend 'delete-file file))
     ;; If the backend hasn't deleted the file itself, let's do it for him.
-    (when (file-exists-p file) (delete-file file))
+    (when (and (file-exists-p file) (null keep-file))
+      (delete-file file))
     ;; Forget what VC knew about the file.
     (vc-file-clearprops file)
     ;; Make sure the buffer is deleted and the *vc-dir* buffers are
     ;; updated after this.
-    (vc-resynch-buffer file nil t)))
+    (vc-resynch-buffer file keep-file t)))
 
 ;;;###autoload
 (defun vc-rename-file (old new)
-- 
2.34.1

bug#52508: [PATCH] Option for vc-delete-file to keep file on disk

[Eli Zaretskii]

> From: Ashwin Kafle <ashwin@ashwink.com.np>
> Date: Wed, 15 Dec 2021 10:23:42 +0000
> Cc: Ashwin Kafle <ashwin@ashwink.com.np>, Dmitry Gutov <dgutov@yandex.ru>
> 
> I've created the attached patch to have vc-delete file to keep files on
> disk using a prefix argument. I've only tested it for vc-git.

Thanks, sounds useful.

A few minor comments below, mainly to the documentation parts:

> * doc/emacs/vc1-xtra.texi: Document the change.
> * lisp/vc/vc-git.el: Make git leave files on disk.
> * lisp/vc/vc.el: Change vc-delete-file to accept optional prefix argument.

Our conventions are to use the ChangeLog style of these entries, which
means they should state the function names where the changes are made,
not just the file names.

> --- a/doc/emacs/vc1-xtra.texi
> +++ b/doc/emacs/vc1-xtra.texi
> @@ -122,7 +122,8 @@ VC Delete/Rename
>    If you wish to delete a version-controlled file, use the command
>  @kbd{M-x vc-delete-file}.  This prompts for the file name, and deletes
>  it via the version control system.  The file is removed from the
> -working tree, and in the VC Directory buffer
> +working tree, and in the VC Directory buffer. If you give a prefix argument,
                                               ^^
We leave 2 spaces between sentences, per US English conventions.

> +*** 'C-x v x' accepts a prefix argument to keep file on disk

Period at the end of the heading, please.

> +Previously 'C-x v x' always deleted the selected file. Now if you give it
> +prefix argument, it will keep the buffer and file on disk intact.

Two spaces between sentences.

> -(defun vc-delete-file (file)
> +(defun vc-delete-file (file &optional keep-file)
>    "Delete file and mark it as such in the version control system.
>  If called interactively, read FILE, defaulting to the current
> -buffer's file name if it's under version control."
> -  (interactive (list (read-file-name "VC delete file: " nil
> -                                     (when (vc-backend buffer-file-name)
> -                                       buffer-file-name)
> -                                     t)))
> +buffer's file name if it's under version control.
> +If a prefix argument is given (optional argument KEEP-FILE) then
> +don't delete the file from the disk"

Period missing at the end of the last sentence.

> -	(let ((backup-inhibited nil))
> +	(let ((backup-inhibited nil)
> +              ;; if you don't set this, then for some reason, the file is never brought back
> +              (backup-by-copying t))

Wouldn't it be better to understand why this mystery happens?

Also, please start the comment with a capital letter and end it with a
period.

bug#52508: [PATCH] Option for vc-delete-file to keep file on disk

[Ashwin Kafle]

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

Eli Zaretskii <eliz@gnu.org> writes:


[...]
>> I've created the attached patch to have vc-delete file to keep files on
>> disk using a prefix argument. I've only tested it for vc-git.
>
> Thanks, sounds useful.
>
> A few minor comments below, mainly to the documentation parts:
[...]

Thanks, now fixed and attached.

>
>> -	(let ((backup-inhibited nil))
>> +	(let ((backup-inhibited nil)
>> + ;; if you don't set this, then for some reason, the file is never
>> brought back
>> +              (backup-by-copying t))
>
> Wouldn't it be better to understand why this mystery happens?

Yeah, a backup function should bring back the original file.  That is
indeed weird.  But I can't read much elisp yet to be of help in here.

In this case, backup-by-copying seems to be the better method for
backups since the intention is to not touch the original files at all.
Reading the docstring of backup-buffer, backup-by-rename doesn't look
fit for this use case.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Option-for-vc-delete-file-to-keep-file-on-disk.patch --]
[-- Type: text/x-diff, Size: 4902 bytes --]

From b8e5b8463f15a53e5fb6ddac5447005d4133606b Mon Sep 17 00:00:00 2001
From: Ashwin Kafle <ashwin@ashwink.com.np>
Date: Wed, 15 Dec 2021 23:49:47 +0545
Subject: [PATCH] Option for vc-delete-file to keep file on disk

Add a prefix argument on vc-delete-file to keep affected
file on disk and keep the current buffer intact.  This option
relies on the backends to not delete files themselves.

* doc/emacs/vc1-xtra.texi: Document the change.
* lisp/vc/vc-git.el (vc-git-delete-file): Make git leave files on disk.
* lisp/vc/vc.el (vc-delete-file): Change vc-delete-file to accept
optional prefix argument.
---
 doc/emacs/vc1-xtra.texi |  3 ++-
 etc/NEWS                |  4 ++++
 lisp/vc/vc-git.el       |  4 ++--
 lisp/vc/vc.el           | 18 ++++++++++++------
 4 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/doc/emacs/vc1-xtra.texi b/doc/emacs/vc1-xtra.texi
index 4cd00cba6c..99e5eef7c1 100644
--- a/doc/emacs/vc1-xtra.texi
+++ b/doc/emacs/vc1-xtra.texi
@@ -122,7 +122,8 @@ VC Delete/Rename
   If you wish to delete a version-controlled file, use the command
 @kbd{M-x vc-delete-file}.  This prompts for the file name, and deletes
 it via the version control system.  The file is removed from the
-working tree, and in the VC Directory buffer
+working tree, and in the VC Directory buffer.  If you give a prefix argument,
+the file is not deleted from disk.
 @iftex
 (@pxref{VC Directory Mode,,, emacs, the Emacs Manual}),
 @end iftex
diff --git a/etc/NEWS b/etc/NEWS
index 8d83b2a7e3..af358aaedb 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -372,6 +372,10 @@ info node.  This command only works for the Emacs and Emacs Lisp manuals.
 
 ** vc
 
+*** 'C-x v x' accepts a prefix argument to keep file on disk.
+Previously 'C-x v x' always deleted the selected file.  Now if you give it
+prefix argument, it will keep the buffer and file on disk intact.
+Currently this is only implemented for vc-git.
 ---
 *** 'C-x v v' on an unregistered file will now use the most specific backend.
 Previously, if you had an SVN-covered "~/" directory, and a Git-covered
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 5c6a39aec9..69ef216529 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1569,8 +1569,8 @@ vc-git-next-revision
     (or (vc-git-symbolic-commit next-rev) next-rev)))
 
 (defun vc-git-delete-file (file)
-  (vc-git-command nil 0 (vc-git--literal-pathspec file) "rm" "-f" "--"))
-
+  (vc-git-command nil 0 (vc-git--literal-pathspec file) "rm" "-f" "--cached" "--"))
+)
 (defun vc-git-rename-file (old new)
   (vc-git-command nil 0 (list old new) "mv" "-f" "--"))
 
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 64f752f248..a4d390fea8 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2970,14 +2970,17 @@ vc-transfer-file
       (vc-checkin file new-backend comment (stringp comment)))))
 
 ;;;###autoload
-(defun vc-delete-file (file)
+(defun vc-delete-file (file &optional keep-file)
   "Delete file and mark it as such in the version control system.
 If called interactively, read FILE, defaulting to the current
-buffer's file name if it's under version control."
+buffer's file name if it's under version control.
+If a prefix argument is given (optional argument KEEP-FILE) then
+don't delete the file from the disk."
   (interactive (list (read-file-name "VC delete file: " nil
                                      (when (vc-backend buffer-file-name)
                                        buffer-file-name)
-                                     t)))
+                                     t)
+                     current-prefix-arg))
   (setq file (expand-file-name file))
   (let ((buf (get-file-buffer file))
         (backend (vc-backend file)))
@@ -2999,19 +3002,22 @@ vc-delete-file
     (unless (or (file-directory-p file) (null make-backup-files)
                 (not (file-exists-p file)))
       (with-current-buffer (or buf (find-file-noselect file))
-	(let ((backup-inhibited nil))
+	(let ((backup-inhibited nil)
+              ;; If you don't set this, then for some reason, the file is never brought back.
+              (backup-by-copying t))
 	  (backup-buffer))))
     ;; Bind `default-directory' so that the command that the backend
     ;; runs to remove the file is invoked in the correct context.
     (let ((default-directory (file-name-directory file)))
       (vc-call-backend backend 'delete-file file))
     ;; If the backend hasn't deleted the file itself, let's do it for him.
-    (when (file-exists-p file) (delete-file file))
+    (when (and (file-exists-p file) (null keep-file))
+      (delete-file file))
     ;; Forget what VC knew about the file.
     (vc-file-clearprops file)
     ;; Make sure the buffer is deleted and the *vc-dir* buffers are
     ;; updated after this.
-    (vc-resynch-buffer file nil t)))
+    (vc-resynch-buffer file keep-file t)))
 
 ;;;###autoload
 (defun vc-rename-file (old new)
-- 
2.34.1

bug#52508: [PATCH] Option for vc-delete-file to keep file on disk

[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. ]]]

  > I've created the attached patch to have vc-delete file to keep files on
  > disk using a prefix argument. I've only tested it for vc-git.

This may seem shocking, but what about changing vc-delete-file
to ALWAYS preserve the local file?  In other words, to change ONLY the repo?

That's the operation that vc-delete-file is necessary for.
If you do in fact want to delete the local file as well, that's easy to do.

This would have the advantage of one less switch to remember.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#52508: [PATCH] Option for vc-delete-file to keep file on disk

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Date: Fri, 17 Dec 2021 23:41:15 -0500
> Cc: ashwin@ashwink.com.np, 52508@debbugs.gnu.org, dgutov@yandex.ru
> 
>   > I've created the attached patch to have vc-delete file to keep files on
>   > disk using a prefix argument. I've only tested it for vc-git.
> 
> This may seem shocking, but what about changing vc-delete-file
> to ALWAYS preserve the local file?  In other words, to change ONLY the repo?

I think it would be unexpected.  VCS deletion usually means you delete
both the file on the local disk and tell the VCS to delete it from the
repository.

bug#52508: [PATCH] Option for vc-delete-file to keep file on disk

[Ashwin Kafle]

Richard Stallman <rms@gnu.org> writes:

> [[[ 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. ]]]
>
>   > I've created the attached patch to have vc-delete file to keep files on
>   > disk using a prefix argument. I've only tested it for vc-git.
>
> This may seem shocking, but what about changing vc-delete-file
> to ALWAYS preserve the local file?  In other words, to change ONLY the repo?
>
> That's the operation that vc-delete-file is necessary for.
> If you do in fact want to delete the local file as well, that's easy to do.
>
> This would have the advantage of one less switch to remember.

I had the same initial thought ;)

But, vc-delete-file is used by a lot of people and it's probably muscle
memory for them.  I didn't want people to be affected much by this patch.

Also, there's a new revision of this patch under bug#52507 that will
make sure that the local file is always preserved even if the VCS itself
deletes it.

After i understand what Juri is saying, i'll likely make a
new patch removing the change from vc-git.el


Thank you, Stallman, for all the hard work you've done and continue to do.

bug#52508: [PATCH] Option for vc-delete-file to keep file on disk

[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. ]]]

  > But, vc-delete-file is used by a lot of people and it's probably muscle
  > memory for them.  I didn't want people to be affected much by this patch.

I'm convinced.  Happy hacking.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#52508: [PATCH] Option for vc-delete-file to keep file on disk

[Lars Ingebrigtsen]

Reading this bug report, it's unclear what the conclusion is.  I agree
that it would be useful to have a command to remove a file from the VC
without deleting it.  But I don't think the proposed implementation is
safe, because it only works with git -- and in other VCs, doing `C-u M-x
vc-delete-file' would continue to delete the file, contrary to what the
doc string says.

Dmitry suggested to alter the interface, but that makes for awkward
compatibility.

Perhaps this should be a new command instead?  That'd be less confusing,
I think.  `M-x vc-remove-file', for instance.