*vc-diff* keeps undo information

*vc-diff* keeps undo information

[Bob Rogers]

   Primarily, the "undo" history consists of (erase-buffer) followed by
insertion of diff output.  Even emacs 21.3 allows me to undo/redo
through the complete "history" of diffs.  Is this a feature?!?

   I would argue the contrary.  Besides being wasteful of memory, other
"undo" behavior sometimes gets in the way.  Here's what happens if you
ever create a large diff:

   1.  "emacs -Q"

   2.  Visit a file under version control.

   3.  Make it some 3MB larger, possibly by inserting it into itself
repeatedly.  The goal is to make the resulting vc-diff buffer output at
least 3MB.

   3.  "C-x v =" to make the *vc-diff* buffer appear for the first time.

   4.  "C-x o C-x v =" to replace the *vc-diff* buffer contents.  You
will be prompted with something like:

	Buffer `*vc-diff*' undo info is 3001370 bytes long; discard it? (yes or no) 

This strikes me as an odd question, as I thought I had already
implicitly requested that the buffer be completely trashed by "C-x v =".
(But then, I never knew I could undo these . . . )

   The patch below is sufficient to get rid of undo in *vc-diff*, but is
very much suboptimal, since it happens after all the diffing is done.
Furthermore, it doesn't allow users to undo manual edits made
afterwards, e.g. splitting hunks, which is surely unacceptable.  I'd be
happy to do a better job, but I haven't had a chance to sign copyright
papers, and the result might not be considered "trivial" . . .

   But I expect this issue also affects other applications of diff-mode,
and therefore requires a wider policy decision in any case.

					-- Bob Rogers
					   http://rgrjr.dyndns.org/

------------------------------------------------------------------------
Index: lisp/vc.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/vc.el,v
retrieving revision 1.418
diff -c -r1.418 vc.el
*** lisp/vc.el	13 Apr 2006 13:35:55 -0000	1.418
--- lisp/vc.el	1 Jun 2006 20:38:27 -0000
***************
*** 1789,1794 ****
--- 1789,1795 ----
      ;; buffer should affect the diff command.
      (vc-diff-internal file rev1 rev2))
    (set-buffer "*vc-diff*")
+   (buffer-disable-undo)
    (if (and (zerop (buffer-size))
  	   (not (get-buffer-process (current-buffer))))
        (progn

Re: *vc-diff* keeps undo information

[Kim F. Storm]

Bob Rogers <rogers-emacs@rgrjr.dyndns.org> writes:

>    Primarily, the "undo" history consists of (erase-buffer) followed by
> insertion of diff output.  Even emacs 21.3 allows me to undo/redo
> through the complete "history" of diffs.  Is this a feature?!?

IMO, it would be better to make the diff buffer read-only (and disable undo).

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: *vc-diff* keeps undo information

[Juri Linkov]

>    The patch below is sufficient to get rid of undo in *vc-diff*, but is
> very much suboptimal, since it happens after all the diffing is done.
> Furthermore, it doesn't allow users to undo manual edits made
> afterwards, e.g. splitting hunks, which is surely unacceptable.

I agree, this is unacceptable.  Editing the diff output is useful for e.g.
preparing the patches for sending by mail after manual editing, etc.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: *vc-diff* keeps undo information

[Richard Stallman]

Can someone call buffer-disable-undo in the *vc-diff* buffer
when it is created?


Date: Thu, 1 Jun 2006 17:31:19 -0400
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
From: Bob Rogers <rogers-emacs@rgrjr.dyndns.org>
To: emacs-devel@gnu.org
Subject: *vc-diff* keeps undo information
X-Spam-Status: No, score=0.1 required=5.0 tests=FORGED_RCVD_HELO 
	autolearn=failed version=3.0.4

   Primarily, the "undo" history consists of (erase-buffer) followed by
insertion of diff output.  Even emacs 21.3 allows me to undo/redo
through the complete "history" of diffs.  Is this a feature?!?

   I would argue the contrary.  Besides being wasteful of memory, other
"undo" behavior sometimes gets in the way.  Here's what happens if you
ever create a large diff:

   1.  "emacs -Q"

   2.  Visit a file under version control.

   3.  Make it some 3MB larger, possibly by inserting it into itself
repeatedly.  The goal is to make the resulting vc-diff buffer output at
least 3MB.

   3.  "C-x v =" to make the *vc-diff* buffer appear for the first time.

   4.  "C-x o C-x v =" to replace the *vc-diff* buffer contents.  You
will be prompted with something like:

	Buffer `*vc-diff*' undo info is 3001370 bytes long; discard it? (yes or no) 

This strikes me as an odd question, as I thought I had already
implicitly requested that the buffer be completely trashed by "C-x v =".
(But then, I never knew I could undo these . . . )

   The patch below is sufficient to get rid of undo in *vc-diff*, but is
very much suboptimal, since it happens after all the diffing is done.
Furthermore, it doesn't allow users to undo manual edits made
afterwards, e.g. splitting hunks, which is surely unacceptable.  I'd be
happy to do a better job, but I haven't had a chance to sign copyright
papers, and the result might not be considered "trivial" . . .

   But I expect this issue also affects other applications of diff-mode,
and therefore requires a wider policy decision in any case.

					-- Bob Rogers
					   http://rgrjr.dyndns.org/

------------------------------------------------------------------------
Index: lisp/vc.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/vc.el,v
retrieving revision 1.418
diff -c -r1.418 vc.el
*** lisp/vc.el	13 Apr 2006 13:35:55 -0000	1.418
--- lisp/vc.el	1 Jun 2006 20:38:27 -0000
***************
*** 1789,1794 ****
--- 1789,1795 ----
      ;; buffer should affect the diff command.
      (vc-diff-internal file rev1 rev2))
    (set-buffer "*vc-diff*")
+   (buffer-disable-undo)
    (if (and (zerop (buffer-size))
  	   (not (get-buffer-process (current-buffer))))
        (progn


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: *vc-diff* keeps undo information

[Richard Stallman]

    > Furthermore, it doesn't allow users to undo manual edits made
    > afterwards, e.g. splitting hunks, which is surely unacceptable.

    I agree, this is unacceptable.  Editing the diff output is useful for e.g.
    preparing the patches for sending by mail after manual editing, etc.

That is a good point.  So the thing to do is turn off undo
temporarily while the diff is being received, then turn it on
with an empty undo list.

Re: *vc-diff* keeps undo information

[Thien-Thi Nguyen]

Richard Stallman <rms@gnu.org> writes:

> That is a good point.  So the thing to do is turn off undo
> temporarily while the diff is being received, then turn it on
> with an empty undo list.

the following patch does this.  i am not confident about it
because i had to change several places, and suspect other diff
cases (or other commands) may not behave as desired.

thi

_________________________________
cvs -f diff -c vc.el
Index: vc.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/vc.el,v
retrieving revision 1.418
diff -c -r1.418 vc.el
*** vc.el	13 Apr 2006 13:35:55 -0000	1.418
--- vc.el	3 Jun 2006 12:25:22 -0000
***************
*** 897,903 ****
  The only difference with the default filter is to insert S after markers."
    (with-current-buffer (process-buffer p)
      (save-excursion
!       (let ((inhibit-read-only t))
  	(goto-char (process-mark p))
  	(insert s)
  	(set-marker (process-mark p) (point))))))
--- 897,904 ----
  The only difference with the default filter is to insert S after markers."
    (with-current-buffer (process-buffer p)
      (save-excursion
!       (let ((buffer-undo-list t)
!             (inhibit-read-only t))
  	(goto-char (process-mark p))
  	(insert s)
  	(set-marker (process-mark p) (point))))))
***************
*** 914,920 ****
      (set (make-local-variable 'vc-parent-buffer-name)
  	 (concat " from " (buffer-name camefrom)))
      (setq default-directory olddir)
!     (let ((inhibit-read-only t))
        (erase-buffer))))
  
  (defun vc-exec-after (code)
--- 915,922 ----
      (set (make-local-variable 'vc-parent-buffer-name)
  	 (concat " from " (buffer-name camefrom)))
      (setq default-directory olddir)
!     (let ((buffer-undo-list t)
!           (inhibit-read-only t))
        (erase-buffer))))
  
  (defun vc-exec-after (code)
***************
*** 1003,1009 ****
  	      (vc-exec-after
  	       `(unless (active-minibuffer-window)
                    (message "Running %s in the background... done" ',command))))
! 	  (setq status (apply 'process-file command nil t nil squeezed))
  	  (when (and (not (eq t okstatus))
                       (or (not (integerp status))
                           (and okstatus (< okstatus status))))
--- 1005,1012 ----
  	      (vc-exec-after
  	       `(unless (active-minibuffer-window)
                    (message "Running %s in the background... done" ',command))))
! 	  (let ((buffer-undo-list t))
!             (setq status (apply 'process-file command nil t nil squeezed)))
  	  (when (and (not (eq t okstatus))
                       (or (not (integerp status))
                           (and okstatus (< okstatus status))))

Re: *vc-diff* keeps undo information

[Bob Rogers]

   From: Thien-Thi Nguyen <ttn@gnu.org>
   Date: 03 Jun 2006 08:30:55 -0400

   Richard Stallman <rms@gnu.org> writes:

   > That is a good point.  So the thing to do is turn off undo
   > temporarily while the diff is being received, then turn it on
   > with an empty undo list.

   the following patch does this.  i am not confident about it
   because i had to change several places, and suspect other diff
   cases (or other commands) may not behave as desired.

   thi

Your patch works for me; thanks.  It also goes beyond solving the
original problem in that it does the same thing for buffers with output
from vc-print-log, etc, which is really nice.

   I found that dired-diff calls diff to do its work, and diff already
does half the job.  The patch below adds the other half.

   As for other modes, pcvs seems to DTRT, and log-view calls
vc-version-diff.  Since emerge and ediff present an indirect interface
to diff output, I assume that we don't need to worry about them.  (But
there are a lot of grep hits to "diff" in the codebase, so there's a
good chance I've missed something.)

					-- Bob

------------------------------------------------------------------------
Index: lisp/diff.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/diff.el,v
retrieving revision 1.59
diff -c -r1.59 diff.el
*** lisp/diff.el	6 Feb 2006 14:33:32 -0000	1.59
--- lisp/diff.el	3 Jun 2006 16:06:13 -0000
***************
*** 69,75 ****
      (goto-char (point-max))
      (insert (format "\nDiff finished%s.  %s\n"
  		    (if (equal 0 code) " (no differences)" "")
! 		    (current-time-string)))))
  
  ;;;###autoload
  (defun diff (old new &optional switches no-async)
--- 69,78 ----
      (goto-char (point-max))
      (insert (format "\nDiff finished%s.  %s\n"
  		    (if (equal 0 code) " (no differences)" "")
! 		    (current-time-string)))
!     ;; undo was turned off by diff; turn it on again to allow the user
!     ;; to split hunks (e.g.).
!     (buffer-enable-undo (current-buffer))))
  
  ;;;###autoload
  (defun diff (old new &optional switches no-async)
***************
*** 118,126 ****
        (display-buffer buf)
        (set-buffer buf)
        (setq buffer-read-only nil)
        (buffer-disable-undo (current-buffer))
        (erase-buffer)
-       (buffer-enable-undo (current-buffer))
        (diff-mode)
        (set (make-local-variable 'revert-buffer-function)
  	   `(lambda (ignore-auto noconfirm)
--- 121,130 ----
        (display-buffer buf)
        (set-buffer buf)
        (setq buffer-read-only nil)
+       ;; recording the erase-buffer and diff content insertion for
+       ;; undo is wasteful.  diff-sentinel turns it back on afterwards.
        (buffer-disable-undo (current-buffer))
        (erase-buffer)
        (diff-mode)
        (set (make-local-variable 'revert-buffer-function)
  	   `(lambda (ignore-auto noconfirm)

Re: *vc-diff* keeps undo information

[Richard Stallman]

Your change looks correct to me.  If no one objects in a few days,
please install it.