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

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

[Ashwin Kafle]

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.
---
I've already signed the copyright papers.

 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#52507: [PATCH] Option for vc-delete-file to keep file on disk

[Juri Linkov]

forcemerge 52507 52508
thanks

> -  (interactive (list (read-file-name "VC delete file: " nil
> -                                     (when (vc-backend buffer-file-name)
> -                                       buffer-file-name)
> -                                     t)))
> +  (interactive "f\nP")

I wonder why no prompt?  You can add `current-prefix-arg' to the
interactive list to keep the existing prompt.

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

I remember having the same problem while improving `vc-rename-file'.
To solve the problem, it required adding `vc-file-clearprops'.
Maybe it could here as well?

> -    (vc-resynch-buffer file nil t)))
> +    (vc-resynch-buffer file keep-file t)))

It seems vc-resynch-window already uses `vc-file-clearprops'
when `keep-file' is specified, but also on some more conditions.

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

[Ashwin Kafle]

Juri Linkov <juri@linkov.net> writes:

>> -  (interactive (list (read-file-name "VC delete file: " nil
>> -                                     (when (vc-backend buffer-file-name)
>> -                                       buffer-file-name)
>> -                                     t)))
>> +  (interactive "f\nP")
>
> I wonder why no prompt?  You can add `current-prefix-arg' to the
> interactive list to keep the existing prompt.

Thanks, I didn't knew about that. 

>
>> +	(let ((backup-inhibited nil)
>> + ;; if you don't set this, then for some reason, the file is never
>> brought back
>> +              (backup-by-copying t))
>
> I remember having the same problem while improving `vc-rename-file'.
> To solve the problem, it required adding `vc-file-clearprops'.
> Maybe it could here as well?

There was a call to vc-file-clearprops later. I moved that earlier
before backup and that didn't seem to work. 

>
>> -    (vc-resynch-buffer file nil t)))
>> +    (vc-resynch-buffer file keep-file t)))
>
> It seems vc-resynch-window already uses `vc-file-clearprops'
> when `keep-file' is specified, but also on some more conditions.

I'm sorry, but I didn't really understand the above. I don't really know
elisp to be honest.

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

[Juri Linkov]

>>> +	(let ((backup-inhibited nil)
>>> + ;; if you don't set this, then for some reason, the file is never brought back
>>> +              (backup-by-copying t))
>>
>> I remember having the same problem while improving `vc-rename-file'.
>> To solve the problem, it required adding `vc-file-clearprops'.
>> Maybe it could here as well?
>
> There was a call to vc-file-clearprops later. I moved that earlier
> before backup and that didn't seem to work.

Then another question: why backup is needed at all?
When the file is not going to be deleted, then
there is no need to make a backup copy?

>>> -    (vc-resynch-buffer file nil t)))
>>> +    (vc-resynch-buffer file keep-file t)))
>>
>> It seems vc-resynch-window already uses `vc-file-clearprops'
>> when `keep-file' is specified, but also on some more conditions.
>
> I'm sorry, but I didn't really understand the above. I don't really know
> elisp to be honest.

The comment before calling `vc-resynch-buffer' says it's to
make sure the buffer is deleted.  But it seems the buffer
is already deleted at that point?

Also it looks that your another change is dangerous:

 (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" "--")))

because it deletes the file in the staging area
that is not used by vc-git, so there is no way
to commit the deletion using vc commands.

Generally, you have a good idea, but it needs more careful handling
with the existing vc commands.

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

[Ashwin Kafle]

[-- Attachment #1.1: Type: text/plain, Size: 1153 bytes --]

Juri Linkov <juri@linkov.net> writes:

> Then another question: why backup is needed at all?
> When the file is not going to be deleted, then
> there is no need to make a backup copy?

There could be deletion or not depending on the value of keep-file.
Maybe the check for that can be made earlier and backup can be skipped
conditionally.  

> The comment before calling `vc-resynch-buffer' says it's to
> make sure the buffer is deleted.  But it seems the buffer
> is already deleted at that point?

No, the buffer is not deleted at that point. I should've changed that
comment too.  vc-resynch-buffer would've deleted that buffer if the
second arg was nil.

>
> Also it looks that your another change is dangerous:
>
>  (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" "--")))
>
> because it deletes the file in the staging area
> that is not used by vc-git, so there is no way
> to commit the deletion using vc commands.

I think this check in vc-delete-file takes care of that


[-- Attachment #1.2: Type: text/plain, Size: 103 bytes --]

(when (eq state 'edited)
        (error "Please commit or undo your changes before deleting %s" file))

[-- Attachment #1.3: Type: text/plain, Size: 79 bytes --]


-- 
I'd rather just believe that it's done by little elves running around.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Ashwin Kafle]

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

Ashwin Kafle <ashwin@ashwink.com.np> writes:

>> Also it looks that your another change is dangerous:
>>
>>  (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" "--")))
>>
>> because it deletes the file in the staging area
>> that is not used by vc-git, so there is no way
>> to commit the deletion using vc commands.
>
> I think this check in vc-delete-file takes care of that
>
> (when (eq state 'edited)
>         (error "Please commit or undo your changes before deleting %s" file))

Oh, you mean just that single commit can't be done by vc now.  Yeah,
that seems true. Can you think of any solution for that here? 



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Juri Linkov]

>>>  (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" "--")))
>>>
>>> because it deletes the file in the staging area
>>> that is not used by vc-git, so there is no way
>>> to commit the deletion using vc commands.
>> ...
> Oh, you mean just that single commit can't be done by vc now.  Yeah,
> that seems true. Can you think of any solution for that here?

--cached can't be used anyway, because vc commands doesn't use the git index.
Currently, after vc-delete-file, we have the following status in vc-dir:

                         ./
     removed             file1
     unregistered        file1~

So the user can commit the removed file with vc-next-action.
Then after this, the user can manually rename the unregistered backup
by removing ~ from the file name.

So it seems that you want to automate the last part, i.e.
to try automatically rename the file from its backup copy
after all changes were committed?

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

[Dmitry Gutov]

On 16.12.2021 20:01, Juri Linkov wrote:
> --cached can't be used anyway, because vc commands doesn't use the git index.
> Currently, after vc-delete-file, we have the following status in vc-dir:
> 
>                           ./
>       removed             file1
>       unregistered        file1~
> 
> So the user can commit the removed file with vc-next-action.

That's a very good point, I didn't even consider this problem (VC not 
caring about the staging area). Perhaps assuming that the full scenario 
with the original patch is functional.

> Then after this, the user can manually rename the unregistered backup
> by removing ~ from the file name.
> 
> So it seems that you want to automate the last part, i.e.
> to try automatically rename the file from its backup copy
> after all changes were committed?

So a "restore from backup" step indeed could be a solution for this problem.

Or alternatively, if we consider the potential feature which we've been 
talking about (committing a subset of hunks from a file selectively), 
its implementation should have a step which either uses a staging area, 
or adds stuff to it first.

And that step could be the place to enact a change like presently 
discussed (add a deletion to the staging area, and then commit it). That 
deletion would either already be in the staging area (meaning we pick up 
any staged changes for commit, which might be weird), or we would store 
the "intent to remove with --cached" in some buffer-local variable, 
which would be picked up by the new code.

The latter solution would be the "cleaner" one, but the former is one 
that we could have _right now_.

On the plus side, the former also doesn't seem like it's going to 
require changes in the VC API after all.

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

[Juri Linkov]

> Or alternatively, if we consider the potential feature which we've been
> talking about (committing a subset of hunks from a file selectively), its
> implementation should have a step which either uses a staging area, or adds
> stuff to it first.
>
> And that step could be the place to enact a change like presently discussed
> (add a deletion to the staging area, and then commit it). That deletion
> would either already be in the staging area (meaning we pick up any staged
> changes for commit, which might be weird), or we would store the "intent to
> remove with --cached" in some buffer-local variable, which would be picked
> up by the new code.
>
> The latter solution would be the "cleaner" one, but the former is one that
> we could have _right now_.
>
> On the plus side, the former also doesn't seem like it's going to require
> changes in the VC API after all.

The feature of committing a subset of hunks will be performed by one command
'log-edit-done' ('C-c C-c') from the *vc-log* buffer that will run git commands
`git apply --cached` followed by `git commit`.

Doing something similar could mean for example that 'C-u M-x vc-delete-file'
could immediately pop up the *vc-log* buffer waiting for a commit message,
and on 'C-c C-c' will commit only the deleted file.

I doubt that anyone might want to commit the file deletion immediately,
because file deletions usually are committed together with other changes.

But maybe git has a way to mark a file as deleted without actually deleting it?
So `git status --porcelain -z --untracked-files` could return "D" for such file
that still exists, this would be the simplest solution.

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

[Dmitry Gutov]

On 23.12.2021 20:20, Juri Linkov wrote:
> I doubt that anyone might want to commit the file deletion immediately,
> because file deletions usually are committed together with other changes.

Right. That's why I suggested either to have a buffer-local var to store 
the "to be deleted" status, or do that in the staging area first.

> But maybe git has a way to mark a file as deleted without actually deleting it?
> So `git status --porcelain -z --untracked-files` could return "D" for such file
> that still exists, this would be the simplest solution.

'git rm --cached', used in the patch for this issue originally, is 
indeed such command. Unless I misunderstood the question.

$ git rm --cached CONTRIBUTE
rm 'CONTRIBUTE' 
                                  $ git status --porcelain --untracked-files
D  CONTRIBUTE
?? CONTRIBUTE

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

[Juri Linkov]

>> I doubt that anyone might want to commit the file deletion immediately,
>> because file deletions usually are committed together with other changes.
>
> Right. That's why I suggested either to have a buffer-local var to store
> the "to be deleted" status, or do that in the staging area first.
>
>> But maybe git has a way to mark a file as deleted without actually deleting it?
>> So `git status --porcelain -z --untracked-files` could return "D" for such file
>> that still exists, this would be the simplest solution.
>
> 'git rm --cached', used in the patch for this issue originally, is indeed
> such command. Unless I misunderstood the question.
>
> $ git rm --cached CONTRIBUTE
> rm 'CONTRIBUTE'                                   $ git status --porcelain
> --untracked-files
> D  CONTRIBUTE
> ?? CONTRIBUTE

Both "D " and "??" correspond to the 'unregistered' status in vc-dir
according to 'vc-git--git-status-to-vc-state':

  (defun vc-git--git-status-to-vc-state (code-list)
      ...
      ('("D " "??") 'unregistered)

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

[Dmitry Gutov]

On 26.12.2021 20:43, Juri Linkov wrote:
> Both "D " and "??" correspond to the 'unregistered' status in vc-dir
> according to 'vc-git--git-status-to-vc-state':
> 
>    (defun vc-git--git-status-to-vc-state (code-list)
>        ...
>        ('("D " "??") 'unregistered)

"D " corresponds to 'removed', see the third branch of (pcase code-list 
...) inside vc-git--git-status-to-vc-state.

But indeed, when you pass a two-element list to 
vc-git--git-status-to-vc-state, that branch is not taken, and the next 
one (which you quoted) returns 'unregistered'.

That's entirely up to vc-git-state. Your new code can make a different 
decision: the information is all there.

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

[Ashwin Kafle]

Dmitry Gutov <dgutov@yandex.ru> writes:


>> Then after this, the user can manually rename the unregistered backup
>> by removing ~ from the file name.
>> So it seems that you want to automate the last part, i.e.
>> to try automatically rename the file from its backup copy
>> after all changes were committed?
>
> So a "restore from backup" step indeed could be a solution for this problem.

For current vc, restore from backup would only work after a commit which
i don't think is desirable much as you could go days without a commit
and 'C-u C-x v x' saying it will not delete the file would be wrong.

I don't know vc-internals but vc-git using git's --porcelain as juri
said is probably a much better idea.

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

[Ashwin Kafle]

Juri Linkov <juri@linkov.net> writes:

> --cached can't be used anyway, because vc commands doesn't use the git index.

It's not --cached that is the problem.  Even the unpatched
vc-delete-file effectively uses --cached because the file is removed
while backuping and never brought back for git to delete it.

Say you use the current vc-delete-file and then immediately restore it
from backup (before commiting).  VC will show the file as unregistered.
I think this behavior of vc should be fixed instead.

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

[Dmitry Gutov]

On 26.12.2021 17:23, Ashwin Kafle wrote:
> Say you use the current vc-delete-file and then immediately restore it
> from backup (before commiting).  VC will show the file as unregistered.
> I think this behavior of vc should be fixed instead.

What would you have it do instead?

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

[Ashwin Kafle]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 26.12.2021 17:23, Ashwin Kafle wrote:
>> Say you use the current vc-delete-file and then immediately restore it
>> from backup (before commiting).  VC will show the file as unregistered.
>> I think this behavior of vc should be fixed instead.
>
> What would you have it do instead?

I think a better way would be to show two files in vc-dir one saying
unregistered and one saying deleted.  You mark the one saying deleted
then commit that fileset which will not be present after being commited.

That mechanism can also be used for git add -p as you can show staged
file and unsatged file separately. 

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

[Dmitry Gutov]

On 26.12.2021 18:51, Ashwin Kafle wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>> On 26.12.2021 17:23, Ashwin Kafle wrote:
>>> Say you use the current vc-delete-file and then immediately restore it
>>> from backup (before commiting).  VC will show the file as unregistered.
>>> I think this behavior of vc should be fixed instead.
>> What would you have it do instead?
> I think a better way would be to show two files in vc-dir one saying
> unregistered and one saying deleted.  You mark the one saying deleted
> then commit that fileset which will not be present after being commited.

All right.

Well, it seems like it will add more cognitive load in the "common" 
scenario -- where you end up deleting the file you said you want to delete.

And it will be a breaking change in the existing behavior/UI.

> That mechanism can also be used for git add -p as you can show staged
> file and unsatged file separately.

Perhaps it we added a different UI for staging and committing from 
staging area (like in Magit), it could both be presented better and 
avoid bothering the existing users who like the simpler workflow.

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

[Ashwin Kafle]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 26.12.2021 18:51, Ashwin Kafle wrote:
>> Dmitry Gutov<dgutov@yandex.ru>  writes:
>> 
>>> On 26.12.2021 17:23, Ashwin Kafle wrote:
>>>> Say you use the current vc-delete-file and then immediately restore it
>>>> from backup (before commiting).  VC will show the file as unregistered.
>>>> I think this behavior of vc should be fixed instead.
>>> What would you have it do instead?
>> I think a better way would be to show two files in vc-dir one saying
>> unregistered and one saying deleted.  You mark the one saying deleted
>> then commit that fileset which will not be present after being commited.
>
> All right.
>
> Well, it seems like it will add more cognitive load in the "common"
> scenario -- where you end up deleting the file you said you want to
> delete.
>
> And it will be a breaking change in the existing behavior/UI.

If you delete from disk it behaves exactly like how it's doing right
now.  The only difference should be when you delete and immediately
restore from backup and in that case, only vc-dir shows one extra file.
I don't think it brakes any existing behavior.


>
>> That mechanism can also be used for git add -p as you can show staged
>> file and unsatged file separately.
>
> Perhaps it we added a different UI for staging and committing from
> staging area (like in Magit), it could both be presented better and
> avoid bothering the existing users who like the simpler workflow.

Yeah, it's probably a bit tricky for partial adds but for complete file
deletions it should be no different at all.

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

[Dmitry Gutov]

On 26.12.2021 19:12, Ashwin Kafle wrote:
>> All right.
>>
>> Well, it seems like it will add more cognitive load in the "common"
>> scenario -- where you end up deleting the file you said you want to
>> delete.
>>
>> And it will be a breaking change in the existing behavior/UI.
> If you delete from disk it behaves exactly like how it's doing right
> now.  The only difference should be when you delete and immediately
> restore from backup and in that case, only vc-dir shows one extra file.
> I don't think it brakes any existing behavior.

But the file would stay around, right? That would be different.

And I was referring to the VC-Dir interface which would show new 
elements (additional entries for some files).

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

[Ashwin Kafle]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 26.12.2021 19:12, Ashwin Kafle wrote:
>>> All right.
>>>
>>> Well, it seems like it will add more cognitive load in the "common"
>>> scenario -- where you end up deleting the file you said you want to
>>> delete.
>>>
>>> And it will be a breaking change in the existing behavior/UI.
>> If you delete from disk it behaves exactly like how it's doing right
>> now.  The only difference should be when you delete and immediately
>> restore from backup and in that case, only vc-dir shows one extra file.
>> I don't think it brakes any existing behavior.
>
> But the file would stay around, right? That would be different.

Only if you give vc-delete-file a prefix argument, otherwise it'll be
exactly the same.  It will delete even if we use git rm --cached (because
it is checked later if the file exists anymore or not)

>
> And I was referring to the VC-Dir interface which would show new
> elements (additional entries for some files).

Only if the file is deleted in the index and has not been commited yet.

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

[Dmitry Gutov]

On 26.12.2021 20:03, Ashwin Kafle wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> On 26.12.2021 19:12, Ashwin Kafle wrote:
>>>> All right.
>>>>
>>>> Well, it seems like it will add more cognitive load in the "common"
>>>> scenario -- where you end up deleting the file you said you want to
>>>> delete.
>>>>
>>>> And it will be a breaking change in the existing behavior/UI.
>>> If you delete from disk it behaves exactly like how it's doing right
>>> now.  The only difference should be when you delete and immediately
>>> restore from backup and in that case, only vc-dir shows one extra file.
>>> I don't think it brakes any existing behavior.
>>
>> But the file would stay around, right? That would be different.
> 
> Only if you give vc-delete-file a prefix argument, otherwise it'll be
> exactly the same.  It will delete even if we use git rm --cached (because
> it is checked later if the file exists anymore or not)

OK, that seems to make sense. But how would we convey to the user that 
that "removed" (followed by "unregistered") refers to the staging area?

Patch which would implement this in VC-Dir/Git is welcome.

And the next step would be to ensure that such deletions (which keep the 
file on disk) can be committed by vc-next-action.

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

[Ashwin Kafle]

Dmitry Gutov <dgutov@yandex.ru> writes:
>>> But the file would stay around, right? That would be different.
>> Only if you give vc-delete-file a prefix argument, otherwise it'll
>> be
>> exactly the same.  It will delete even if we use git rm --cached (because
>> it is checked later if the file exists anymore or not)
>
> OK, that seems to make sense. But how would we convey to the user that
> that "removed" (followed by "unregistered") refers to the staging
> area?
>

By mentioning it in the manual or perhaps in the docstring of
vc-delete-file?  It should be intuitive enough once you do it.

> 
> Patch which would implement this in VC-Dir/Git is welcome.

Can you please explain to me briefly how vc-dir gets this info?  I will
try at it but if you don't hear from me in a week then you know what
happened ;-)

On a related note, how do you test patches?  Last time i had to manually
C-M-x each and every function that was changed.  Is there a way to tell
emacs to ignore byte compiled files so as a simple restart will apply
new changes?  Any other way is okay with me too. 

>
> And the next step would be to ensure that such deletions (which keep
> the file on disk) can be committed by vc-next-action.
>

If it shows the two files, then you can mark the one saying removed and
vc-next-action can commit it.

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

[Dmitry Gutov]

On 27.12.2021 07:08, Ashwin Kafle wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
>>>> But the file would stay around, right? That would be different.
>>> Only if you give vc-delete-file a prefix argument, otherwise it'll
>>> be
>>> exactly the same.  It will delete even if we use git rm --cached (because
>>> it is checked later if the file exists anymore or not)
>>
>> OK, that seems to make sense. But how would we convey to the user that
>> that "removed" (followed by "unregistered") refers to the staging
>> area?
>>
> 
> By mentioning it in the manual or perhaps in the docstring of
> vc-delete-file?  It should be intuitive enough once you do it.

Ideally the user interface would convey that information without 
requiring the user to read the manual.

>> Patch which would implement this in VC-Dir/Git is welcome.
> 
> Can you please explain to me briefly how vc-dir gets this info?  I will
> try at it but if you don't hear from me in a week then you know what
> happened ;-)

It's fairly involved. There is the vc-git-dir-status-files which fetches 
the status information asynchronously. But it is also updates for the 
individual files when a buffer is saved, if a VC-Dir buffer is already 
open (then the status comes from vc-git-state).

There is also vc-git-dir-printer which renders individual entries.

And finally, there is the ewoc.el package which stores the information 
about the statuses of files which are displayed inside the VC-Dir 
buffer. One should probably verify that it can show different statuses 
for one file name (might be non-trivial to change, or not).

> On a related note, how do you test patches?  Last time i had to manually
> C-M-x each and every function that was changed.

I either use 'M-x eval-buffer' (bound to a key, 'C-c v' in my config) to 
re-evaluate the whole buffer which contains the changed code, or in some 
other cases I run 'make' in the checkout which updates the .elc files, 
and then restart Emacs (or rather launch a second instance for testing, 
while keeping the first one running for editing).

 > Is there a way to tell
 > emacs to ignore byte compiled files so as a simple restart will apply
 > new changes?

   (setq load-prefer-newer t)

should tell Emacs to do exactly that. Though I suppose this will depend 
on the order the packages are loaded -- if the edited file is loaded 
before the part of your init script where this line resides, or -- even 
worse -- is preloaded, this won't have the desired effect.

But otherwise this setting is very handy, and I recommend people turn it 
on. Especially when developing their own packages.

>> And the next step would be to ensure that such deletions (which keep
>> the file on disk) can be committed by vc-next-action.
>>
> 
> If it shows the two files, then you can mark the one saying removed and
> vc-next-action can commit it.

I mean verify that this actually works. As a UI, it sounds workable.

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.

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

[Juri Linkov]

> Say you use the current vc-delete-file and then immediately restore it
> from backup (before commiting).  VC will show the file as unregistered.
> I think this behavior of vc should be fixed instead.

Indeed, often vc causes such a state that requires to resort to
git commands like `git restore`, so vc should be improved in this regard.

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

[Juri Linkov]

> Say you use the current vc-delete-file and then immediately restore it
> from backup (before commiting).  VC will show the file as unregistered.
> I think this behavior of vc should be fixed instead.

Actually, to restore a deleted file there is a command 'vc-revert'.
Obviously it can't be called on a deleted file or its buffer
because it's already gone.  But it can be used on a line
with the tag "removed" in the vc-dir buffer.  There is
no shorter vc-dir binding to this command, so the global keybinding
'C-x v u' could be used on the vc-dir line with the removed file.
This completely reverses the deletion.

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

[Dmitry Gutov]

On 15.12.2021 12:53, Ashwin Kafle wrote:
>   (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" "--"))

This changes the semantics of the 'delete-file' backend action. Kind of 
breaking strict compatibility with third-party backends.

I think it would be better to add an optional argument to it instead 
(e.g. call it KEEP-FILE, just like the new arg to vc-delete-file; or 
KEEP-ON-DISK).

Then git's implementation's signature will look like

   (defun vc-git-delete-file (file &optional keep-on-disk)

and it will decide whether to add '--cached' based on that argument.

No 'delete-file' investigation will be needed as a result.

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

[Ashwin Kafle]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 15.12.2021 12:53, Ashwin Kafle wrote:
>>   (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" "--"))
>
> This changes the semantics of the 'delete-file' backend action. Kind
> of breaking strict compatibility with third-party backends.
>
> I think it would be better to add an optional argument to it instead
> (e.g. call it KEEP-FILE, just like the new arg to vc-delete-file; or
> KEEP-ON-DISK).
>
> Then git's implementation's signature will look like
>
>   (defun vc-git-delete-file (file &optional keep-on-disk)
>
> and it will decide whether to add '--cached' based on that argument.
>
> No 'delete-file' investigation will be needed as a result.

But this would mean that every vc-backend will have to be changed,
immediately.  If we keep things as-is, Existing code either both emacs and
third-party packages will continue to work with no changes.
We can then say in NEWS that all vc backends should always leave files
in disk and let vc-delete-file handle deletion from the disk.

Also, having every vc-backend accept and check keep-on-disk will result
on a lot of duplicate code that can simply be avoided.

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

[Dmitry Gutov]

On 16.12.2021 10:12, Ashwin Kafle wrote:
> But this would mean that every vc-backend will have to be changed,
> immediately.

Not really: since the arg is optional, we can make sure to only add it 
when it's non-nil, and otherwise call the backends with 1 argument.

> Also, having every vc-backend accept and check keep-on-disk will result
> on a lot of duplicate code that can simply be avoided.

That's a valid argument, I suppose. Depends on whether many other 
backends (VCSes) know how to delete files without deleting them on disk.

OTOH, it would be handy to let those that don't declare explicitly their 
inability to do that (by not supporting the second argument).

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

[Ashwin Kafle]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 16.12.2021 10:12, Ashwin Kafle wrote:
>> But this would mean that every vc-backend will have to be changed,
>> immediately.
>
> Not really: since the arg is optional, we can make sure to only add it
> when it's non-nil, and otherwise call the backends with 1 argument.

But then the user will get error about wrong number of arguments if the
backend doesn't support it.

>
>> Also, having every vc-backend accept and check keep-on-disk will result
>> on a lot of duplicate code that can simply be avoided.
>
> That's a valid argument, I suppose. Depends on whether many other
> backends (VCSes) know how to delete files without deleting them on
> disk.
>
> OTOH, it would be handy to let those that don't declare explicitly
> their inability to do that (by not supporting the second argument).

I think it would be better to check if the file exists after calling vc
backends.  If it doesn't and keep-files is non-nil, we can restore from the
backup(which is always happening).  That will make all vc backends
compatible without any change.

If other backends support deletion only from the index, then they should
do that as it preserves file-modes and such.

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

[Dmitry Gutov]

On 16.12.2021 14:26, Ashwin Kafle wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> On 16.12.2021 10:12, Ashwin Kafle wrote:
>>> But this would mean that every vc-backend will have to be changed,
>>> immediately.
>>
>> Not really: since the arg is optional, we can make sure to only add it
>> when it's non-nil, and otherwise call the backends with 1 argument.
> 
> But then the user will get error about wrong number of arguments if the
> backend doesn't support it.

That's better than having the file deleted, I think.

And our implementation can catch this particular error and show a more 
humane message, too.

>>
>>> Also, having every vc-backend accept and check keep-on-disk will result
>>> on a lot of duplicate code that can simply be avoided.
>>
>> That's a valid argument, I suppose. Depends on whether many other
>> backends (VCSes) know how to delete files without deleting them on
>> disk.
>>
>> OTOH, it would be handy to let those that don't declare explicitly
>> their inability to do that (by not supporting the second argument).
> 
> I think it would be better to check if the file exists after calling vc
> backends.  If it doesn't and keep-files is non-nil, we can restore from the
> backup(which is always happening).

Couldn't backups be disabled?

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

[Ashwin Kafle]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

[...]
>> But then the user will get error about wrong number of arguments if
>> the
>> backend doesn't support it.
>
> That's better than having the file deleted, I think.
>
> And our implementation can catch this particular error and show a more
> humane message, too.

With the attached patch, the file will not be deleted when prefix is given.


[...]
>> I think it would be better to check if the file exists after calling
>> vc
>> backends.  If it doesn't and keep-files is non-nil, we can restore from the
>> backup(which is always happening).
>
> Couldn't backups be disabled?

The attached patch changes make-backup-files temporarily.


[-- 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: 5599 bytes --]

From 846a92e9623a05d49d0026a62697a39d0e7690a7 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. Also have it manage backup files in
case the backend delets files when we don't want to.
---
 doc/emacs/vc1-xtra.texi |  3 ++-
 etc/NEWS                |  4 ++++
 lisp/vc/vc-git.el       |  4 ++--
 lisp/vc/vc.el           | 32 +++++++++++++++++++++-----------
 4 files changed, 29 insertions(+), 14 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..8d393282cc 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)))
@@ -2996,22 +2999,29 @@ vc-delete-file
     (unless (y-or-n-p (format "Really want to delete %s? "
 			      (file-name-nondirectory file)))
       (error "Abort!"))
-    (unless (or (file-directory-p file) (null make-backup-files)
-                (not (file-exists-p file)))
+    (when (or (not (file-directory-p file))
+              (file-exists-p file))
       (with-current-buffer (or buf (find-file-noselect file))
-	(let ((backup-inhibited nil))
+	(let ((backup-inhibited nil)
+              (make-backup-files t)
+              ;; 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))
+    ;; If the file dosen't exist when we want to or vice-versa, fix that.
+    (let ((back-file (car (file-backup-file-names file))))
+      (if (file-exists-p file)
+          (if (not keep-file) (delete-file file))
+        (when (and keep-file back-file)
+          (copy-file back-file file t t t t)))
+      (unless make-backup-files (delete-file back-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)))
+    ;; Make sure the *vc-dir* buffers are updated after this.
+    (vc-resynch-buffer file keep-file t)))
 
 ;;;###autoload
 (defun vc-rename-file (old new)
-- 
2.34.1