Re: undo weirdness with insert-file-contents

[martin rudalics <rudalics@gmx.at>]

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

 >>+     current_buffer->undo_list =
 >>+       Fcons (Qnil, current_buffer->undo_list);
 >
 >
 > We should use Fundo_boundary here.

OK - tho' I'm not sure whether pending_boundary should be considered
here.  Note that I could remove that boundary and also combine the
insertion with a previous one if that's desired.  Personally, I'd prefer
to undo two separate insertions from files separately.  In this context
note that record_insert combines two insertions only if they happen in a
left-to-right way.  IIUC this should be eventually changed when Emacs
goes bi-directional.

 >>+   else if (inserted > 0
 >>+ 	   && (! EQ (original_undo_list, Qt))
 >>+ 	   && (NILP (visit)) && (NILP (replace)))
 >>+     /* Assure that the first insertion is counted as a change. */
 >>+     {
 >>+       current_buffer->undo_list = Qnil;
 >>+       record_first_change ();
 >>+     }
 >
 >
 > You use `else' between the two, but it's not clear why the two should be
 > mutually exclusive.  And please mention `visit' in the comment.

It's hopefully a bit clearer now although I'm not sure whether I need
the empty_undo_list check at all.  BTW, what is the semantics of
`insert-file-contents' with VISIT nil and REPLACE non-nil?

 > Also if the file is empty, is this going to mark the buffer as modified
 > even though nothing was changed?

I'm afraid I don't understand the question - do you mean the case where
inserted equals zero?  I don't handle that.  But maybe I'm missing
something.

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

*** fileio.c.~1.602.~	Thu Feb 14 20:41:44 2008
--- fileio.c	Sun Mar  2 12:03:00 2008
***************
*** 3730,3735 ****
--- 3730,3736 ----
    int read_quit = 0;
    Lisp_Object old_Vdeactivate_mark = Vdeactivate_mark;
    int we_locked_file = 0;
+   int empty_undo_list = NILP (current_buffer->undo_list);
  
    if (current_buffer->base_buffer && ! NILP (visit))
      error ("Cannot do file visiting in an indirect buffer");
***************
*** 4593,4598 ****
--- 4594,4622 ----
    if (CODING_MAY_REQUIRE_DECODING (&coding)
        && (inserted > 0 || CODING_REQUIRE_FLUSHING (&coding)))
      {
+       if (CONSP (current_buffer->undo_list)
+ 	  && ! NILP (XCAR (current_buffer->undo_list)))
+ 	/* If the undo list is non-empty and undo information is
+ 	   recorded, insert an undo boundary unless we already have one.
+ 	   This is needed to avoid that record_insert combines the
+ 	   present insertion with an earlier one and consequently the
+ 	   mechanism fails that compares the start position of the
+ 	   insertion before vs that after the format-decode step.  */
+ 	Fundo_boundary ();
+       else if (empty_undo_list
+ 	       && MODIFF <= SAVE_MODIFF
+ 	       && NILP (visit) && NILP (replace))
+ 	/* Else if the present insertion is the very first change of the
+ 	   current buffer, undo information is recorded, and we are
+ 	   neither visiting a file nor replacing buffer contents, make
+ 	   sure we record the insertion as a first change in the
+ 	   undo-list.  This is needed in order to avoid that undoing the
+ 	   insertion leaves the buffer's modified state set.  */
+ 	{
+ 	  current_buffer->undo_list = Qnil;
+ 	  record_first_change ();
+ 	}
+ 
        move_gap_both (PT, PT_BYTE);
        GAP_SIZE += inserted;
        ZV_BYTE -= inserted;
***************
*** 4604,4611 ****
        coding_system = CODING_ID_NAME (coding.id);
      }
    else if (inserted > 0)
!     adjust_after_insert (PT, PT_BYTE, PT + inserted, PT_BYTE + inserted,
! 			 inserted);
  
    /* Now INSERTED is measured in characters.  */
  
--- 4628,4651 ----
        coding_system = CODING_ID_NAME (coding.id);
      }
    else if (inserted > 0)
!     {
!       if (CONSP (current_buffer->undo_list)
! 	  && ! NILP (XCAR (current_buffer->undo_list)))
! 	/* Insert an undo boundary unless there's one here.  See
! 	   explanation above.  */
! 	Fundo_boundary ();
!       else if (empty_undo_list
! 	       && MODIFF <= SAVE_MODIFF
! 	       && NILP (visit) && NILP (replace))
! 	/* Make sure the first insertion is counted as a change.  See
! 	   explanation above.  */
! 	{
! 	  current_buffer->undo_list = Qnil;
! 	  record_first_change ();
! 	}
!       adjust_after_insert (PT, PT_BYTE, PT + inserted, PT_BYTE + inserted,
! 			   inserted);
!     }
  
    /* Now INSERTED is measured in characters.  */