[PATCH 3/4] git.el: Check for existing buffers on revert.

[PATCH 3/4] git.el: Check for existing buffers on revert.

[Alexandre Julliard]

Refuse to revert a file if it is modified in an existing buffer but
not saved. On success, revert the buffers that contains the files that
have been reverted.

Signed-off-by: Alexandre Julliard <julliard@winehq.org>
---
 contrib/emacs/git.el |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/contrib/emacs/git.el b/contrib/emacs/git.el
index 5519ed1..e1058b9 100644
--- a/contrib/emacs/git.el
+++ b/contrib/emacs/git.el
@@ -1033,11 +1033,19 @@ Return the list of files that haven't been handled."
           ('deleted (push (git-fileinfo->name info) modified))
           ('unmerged (push (git-fileinfo->name info) modified))
           ('modified (push (git-fileinfo->name info) modified))))
+      ;; check if a buffer contains one of the files and isn't saved
+      (dolist (file (append added modified))
+        (let ((buffer (get-file-buffer file)))
+          (when (and buffer (buffer-modified-p buffer))
+            (error "Buffer %s is modified. Please kill or save modified buffers before reverting." (buffer-name buffer)))))
       (when added
         (apply #'git-call-process-env nil nil "update-index" "--force-remove" "--" added))
       (when modified
         (apply #'git-call-process-env nil nil "checkout" "HEAD" modified))
       (git-update-status-files (append added modified) 'uptodate)
+      (dolist (file (append added modified))
+        (let ((buffer (get-file-buffer file)))
+          (when buffer (with-current-buffer buffer (revert-buffer t t t)))))
       (git-success-message "Reverted" (git-get-filenames files)))))
 
 (defun git-resolve-file ()
-- 
1.5.4.38.g0d380

-- 
Alexandre Julliard
julliard@winehq.org

Re: [PATCH 3/4] git.el: Check for existing buffers on revert.

[Sergei Organov]

Alexandre Julliard <julliard@winehq.org> writes:
> Refuse to revert a file if it is modified in an existing buffer but
> not saved.

What's the point? What if I do want to have modified buffer and still
revert the on-disk file? Why git-revert cares to the level of
prohibiting this?

Besides, it's inconsistent with the rest of Emacs, I think, as in
similar situations Emacs usually allows to either save the buffer(s), do
not save the buffer(s) and continue, or abort operation (I suppose using
(save-some-buffers) call, though I didn't check). See, for example, how
(compile) behaves when some of buffers are not saved.

In fact I believe the way PCL-CVS handles this, and that was implemented
in my earlier patch, is superior compared to this patch. An addition of
save-some-buffers call won't hurt either, but IMHO is not very useful in
the specific case of git-revert.

BTW, what definitely lacks (save-some-buffers) call is git-commit, as it
silently commits on-disk state of a file when corresponding buffer is
modified.

> On success, revert the buffers that contains the files that have been
> reverted.

This part is indeed very handy.

-- Sergei Organov.

Re: [PATCH 3/4] git.el: Check for existing buffers on revert.

[Alexandre Julliard]

Sergei Organov <osv@javad.com> writes:

> What's the point? What if I do want to have modified buffer and still
> revert the on-disk file? Why git-revert cares to the level of
> prohibiting this?
>
> Besides, it's inconsistent with the rest of Emacs, I think, as in
> similar situations Emacs usually allows to either save the buffer(s), do
> not save the buffer(s) and continue, or abort operation (I suppose using
> (save-some-buffers) call, though I didn't check). See, for example, how
> (compile) behaves when some of buffers are not saved.

It's modeled on the vc-revert behavior, but yes, it could also prompt
whether to discard changes; prompting to save doesn't make sense if you
are about to throw away the changes. I think reverting the file on disk
without changing the buffer is confusing: either you want to discard the
changes, and you want to discard the buffer changes too, or you want to
keep your changes, and reverting the file on disk doesn't make sense
since the revert will be undone as soon as you save the buffer.

> In fact I believe the way PCL-CVS handles this, and that was implemented
> in my earlier patch, is superior compared to this patch. An addition of
> save-some-buffers call won't hurt either, but IMHO is not very useful in
> the specific case of git-revert.
>
> BTW, what definitely lacks (save-some-buffers) call is git-commit, as it
> silently commits on-disk state of a file when corresponding buffer is
> modified.

This could certainly be done, though it would have to be smarter than
a simple (save-some-buffers), I find it very annoying when compile
prompts to save a bunch of unrelated files.

-- 
Alexandre Julliard
julliard@winehq.org

Re: [PATCH 3/4] git.el: Check for existing buffers on revert.

[Sergei Organov]

Alexandre Julliard <julliard@winehq.org> writes:

> Sergei Organov <osv@javad.com> writes:
>
>> What's the point? What if I do want to have modified buffer and still
>> revert the on-disk file? Why git-revert cares to the level of
>> prohibiting this?
>>
>> Besides, it's inconsistent with the rest of Emacs, I think, as in
>> similar situations Emacs usually allows to either save the buffer(s), do
>> not save the buffer(s) and continue, or abort operation (I suppose using
>> (save-some-buffers) call, though I didn't check). See, for example, how
>> (compile) behaves when some of buffers are not saved.
>
> It's modeled on the vc-revert behavior, but yes, it could also prompt
> whether to discard changes; prompting to save doesn't make sense if you
> are about to throw away the changes.

Yes, and I think that prompting makes more sense at the time we try to
actually revert buffer from disk. Besides this allows to eliminate the
first check for modified buffers altogether.

And I'd put this (revert some buffers with prompt) into a separate
function as I foresee a need for such a function when, say,
git-checkout, or any other command that changes working files will be
implemented.

> I think reverting the file on disk without changing the buffer is
> confusing: either you want to discard the changes, and you want to
> discard the buffer changes too, or you want to keep your changes, and
> reverting the file on disk doesn't make sense since the revert will be
> undone as soon as you save the buffer.

I think that prompting is the best solution. It won't be a frequent
situation, so prompting won't be annoying. Though this will differ both
from VC and PCVS behavior.

BTW, there is another related issue: what to do with buffers for which their
files disappear (are removed). AFAIK PCVS doesn't close such buffers,
that according to the above logic is confusing as well.

>> In fact I believe the way PCL-CVS handles this, and that was implemented
>> in my earlier patch, is superior compared to this patch. An addition of
>> save-some-buffers call won't hurt either, but IMHO is not very useful in
>> the specific case of git-revert.
>>
>> BTW, what definitely lacks (save-some-buffers) call is git-commit, as it
>> silently commits on-disk state of a file when corresponding buffer is
>> modified.
>
> This could certainly be done, though it would have to be smarter than
> a simple (save-some-buffers), I find it very annoying when compile
> prompts to save a bunch of unrelated files.

[And so do I, but unlike git-commit, compile has no clue of what files
are relevant (though it does lack a method to ignore buffers by, say,
regexp of buffer name).]

I agree git-commit could be made smarter. Fortunately, save-some-buffers
has /pred/ argument that allows for arbitrary filtering of buffers to be
considered.

-- Sergei.

Re: [PATCH 3/4] git.el: Check for existing buffers on revert.

[Tommy Thorn]

Sergei Organov wrote:
> BTW, there is another related issue: what to do with buffers for which their
> files disappear (are removed). AFAIK PCVS doesn't close such buffers,
> that according to the above logic is confusing as well.
>   

Please don't revert such buffers! More than once have I been saved from 
a blunder by the fact that Emacs still had a copy of the accidentally 
erase/overwritten file. The normal Emacs behaviour is to prompt upon 
editing whether to edit the buffer since the file has changed.

Thanks
Tommy