Re: undo weirdness with insert-file-contents

[martin rudalics <rudalics@gmx.at>]

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

 > Good, point so we just turn off undo during the dance and then insert
 > the single undo-element (this is only valid for non-REPLACE, of course).

Hopefully for VISIT as well - we should not record deletions when
handling the VISIT&REPLACE case.  I see only three cases here:

(1) undo-list was t => leave it alone.

(2) nochange => restore undo-list to previous value.

(3) otherwise => reset undo-list to nil.

I also added a nochange arm for the non-decoding case - please have a
look.  BTW nochange makes an incompatible change to `revert-buffer': You
can no longer use `revert-buffer' to get rid of the undo list.  I think
this should be documented somehow.  Finally, we should document that
REPLACE doesn't handle differences in text properties and tell users to
write their own `revert-buffer-insert-file-contents-function' to handle
them.

martin, who still considers the non-VISIT&REPLACE case obscure and
cannot see any "example" in `smerge-resolve'.

[-- Attachment #2: fileio.patch --]
[-- Type: text/plain, Size: 6265 bytes --]

*** fileio.c	Wed Feb 27 07:49:02 2008
--- fileio.c	Fri Mar  7 10:16:10 2008
***************
*** 3721,3727 ****
    register int unprocessed;
    int count = SPECPDL_INDEX ();
    struct gcpro gcpro1, gcpro2, gcpro3, gcpro4, gcpro5;
!   Lisp_Object handler, val, insval, orig_filename, old_undo;
    Lisp_Object p;
    int total = 0;
    int not_regular = 0;
--- 3721,3727 ----
    register int unprocessed;
    int count = SPECPDL_INDEX ();
    struct gcpro gcpro1, gcpro2, gcpro3, gcpro4, gcpro5;
!   Lisp_Object handler, val, insval, orig_filename, old_undo_list;
    Lisp_Object p;
    int total = 0;
    int not_regular = 0;
***************
*** 3734,3739 ****
--- 3734,3740 ----
    int read_quit = 0;
    Lisp_Object old_Vdeactivate_mark = Vdeactivate_mark;
    int we_locked_file = 0;
+   int buffer_was_unmodified;
  
    if (current_buffer->base_buffer && ! NILP (visit))
      error ("Cannot do file visiting in an indirect buffer");
***************
*** 3744,3752 ****
    val = Qnil;
    p = Qnil;
    orig_filename = Qnil;
!   old_undo = Qnil;
  
!   GCPRO5 (filename, val, p, orig_filename, old_undo);
  
    CHECK_STRING (filename);
    filename = Fexpand_file_name (filename, Qnil);
--- 3745,3754 ----
    val = Qnil;
    p = Qnil;
    orig_filename = Qnil;
!   old_undo_list = Qnil;
!   buffer_was_unmodified = MODIFF <= SAVE_MODIFF;
  
!   GCPRO5 (filename, val, p, orig_filename, old_undo_list);
  
    CHECK_STRING (filename);
    filename = Fexpand_file_name (filename, Qnil);
***************
*** 3984,3989 ****
--- 3986,3999 ----
        set_coding_system = 1;
      }
  
+   /* Don't record undo information when visiting or not replacing; we
+      restore the undo list manually after the format decode step.  */
+   if (! NILP (visit) || NILP (replace))
+     {
+       old_undo_list = current_buffer->undo_list;
+       current_buffer->undo_list = Qt;
+     }
+ 
    /* If requested, replace the accessible part of the buffer
       with the file contents.  Avoid replacing text at the
       beginning or end of the buffer that matches the file contents;
***************
*** 4067,4074 ****
  	{
  	  emacs_close (fd);
  	  specpdl_ptr--;
! 	  /* Truncate the buffer to the size of the file.  */
! 	  del_range_1 (same_at_start, same_at_end, 0, 0);
  	  goto handled;
  	}
        immediate_quit = 1;
--- 4077,4088 ----
  	{
  	  emacs_close (fd);
  	  specpdl_ptr--;
! 	  if (same_at_start == same_at_end)
! 	    nochange = 1;
! 	  else
! 	    /* Truncate the buffer to the size of the file.  */
! 	    del_range_1 (same_at_start, same_at_end, 0, 0);
! 	  inserted = 0;
  	  goto handled;
  	}
        immediate_quit = 1;
***************
*** 4281,4290 ****
        if (bufpos == inserted)
  	{
  	  specpdl_ptr--;
- 	  /* Truncate the buffer to the size of the file.  */
  	  if (same_at_start == same_at_end)
  	    nochange = 1;
  	  else
  	    del_range_byte (same_at_start, same_at_end, 0);
  	  inserted = 0;
  
--- 4295,4304 ----
        if (bufpos == inserted)
  	{
  	  specpdl_ptr--;
  	  if (same_at_start == same_at_end)
  	    nochange = 1;
  	  else
+ 	    /* Truncate the buffer to the size of the file.  */
  	    del_range_byte (same_at_start, same_at_end, 0);
  	  inserted = 0;
  
***************
*** 4632,4640 ****
  
    if (!NILP (visit))
      {
-       if (!EQ (current_buffer->undo_list, Qt) && !nochange)
- 	current_buffer->undo_list = Qnil;
- 
        if (NILP (handler))
  	{
  	  current_buffer->modtime = st.st_mtime;
--- 4646,4651 ----
***************
*** 4679,4688 ****
        specbind (Qinhibit_point_motion_hooks, Qt);
        specbind (Qinhibit_modification_hooks, Qt);
  
-       /* Save old undo list and don't record undo for decoding. */
-       old_undo = current_buffer->undo_list;
-       current_buffer->undo_list = Qt;
- 
        if (NILP (replace))
  	{
  	  insval = call3 (Qformat_decode,
--- 4690,4695 ----
***************
*** 4766,4793 ****
  	  p = XCDR (p);
  	}
  
!       if (NILP (visit))
  	{
! 	  Lisp_Object lbeg, lend;
! 	  XSETINT (lbeg, PT);
! 	  XSETINT (lend, PT + inserted);
! 	  if (CONSP (old_undo))
  	    {
! 	      Lisp_Object tem = XCAR (old_undo);
! 	      if (CONSP (tem) && INTEGERP (XCAR (tem)) &&
! 		  INTEGERP (XCDR (tem)) && EQ (XCAR (tem), lbeg))
! 		/* In the non-visiting case record only the final insertion. */
! 		current_buffer->undo_list =
! 		  Fcons (Fcons (lbeg, lend), Fcdr (old_undo));
  	    }
  	}
-       else
- 	/* If undo_list was Qt before, keep it that way.
- 	   Otherwise start with an empty undo_list. */
- 	current_buffer->undo_list = EQ (old_undo, Qt) ? Qt : Qnil;
- 
        unbind_to (count, Qnil);
      }
  
    /* Call after-change hooks for the inserted text, aside from the case
       of normal visiting (not with REPLACE), which is done in a new buffer
--- 4773,4813 ----
  	  p = XCDR (p);
  	}
  
!       if (! NILP (visit))
  	{
! 	  if (! EQ (old_undo_list, Qt))
! 	    /* Reset undo list for visiting case.  */
! 	    current_buffer->undo_list =  Qnil;
! 	}
!       else if (NILP (replace) && ! EQ (old_undo_list, Qt))
! 	/* For the non-visiting, non-replacing case record the last
! 	   insertion only.  This avoids that intermediate steps get
! 	   recorded and show up during undo.  */
! 	{
! 	  current_buffer->undo_list = old_undo_list;
! 	  if (inserted > 0)
  	    {
! 	      if (buffer_was_unmodified)
! 		/* If the buffer was unmodified when this function was
! 		   called record a first buffer change (this is *not*
! 		   done by the record_insert below since the buffer is
! 		   already modified).  */
! 		record_first_change ();
! 	      record_insert (PT, inserted);
  	    }
  	}
        unbind_to (count, Qnil);
      }
+   else if (! NILP (visit))
+     {
+       if (! EQ (old_undo_list, Qt))
+ 	/* For the visiting case restore old undo list iff replacing and
+ 	   nothing changed.  Otherwise start with empty undo list.  */
+ 	current_buffer->undo_list = nochange ? old_undo_list : Qnil;
+     }
+   else if (NILP (replace))
+     /* Restore old undo list in non-visiting, non-replacing case.  */
+     current_buffer->undo_list = old_undo_list;
  
    /* Call after-change hooks for the inserted text, aside from the case
       of normal visiting (not with REPLACE), which is done in a new buffer