undo weirdness with insert-file-contents

undo weirdness with insert-file-contents

[Miles Bader]

I define a "message-setup-hook" which for some reason has the unintended
side-effect of turning off undo in the result message-mode buffer
[buffer-undo-list has a value of t].

I've tracked the problem down to a call to `insert-file-contents' in the
hook function; If I remove the call to insert-file-contents, undo
doesn't get turned off.

I looked at the C code for insert-file-contents, and its handling of the
undo list looks pretty messy, it seems to be saving it and restoring
multiple times (sometimes nested?) using multiple methods of restoring
the old value....

The following cut-down message-setup-hook illustrates the problem:

   (defvar file-to-insert "~/.profile")
   (defun test-message-setup-hook ()
     (save-excursion
       (insert "Foo: bar\n")
       (insert-file-contents file-to-insert)))
   (setq message-setup-hook nil)
   (add-hook 'message-setup-hook 'test-message-setup-hook)

[Substitute an appropriate filename for the value of the file-to-insert
variable; anything is OK as long as it exists and is readable.  I just
used "~/.profile" because it usually exists :-]

To test this, in "emacs -Q", evaluate the above code, and then do
"M-x message-mail RET" to start composing in message-mode.  Insert some
test and then try to undo it -- it should fail, because undo is turned
off in that buffer.

If you then do (setq message-setup-hook nil) and try
"M-x message-mail RET" again, undo should work properly this time.

Anyone know more about the treatment of bufer-undo-list in
insert-file-contents?

Thanks,

-Miles

-- 
Love is a snowmobile racing across the tundra.  Suddenly it flips over,
pinning you underneath.  At night the ice weasels come.  --Nietzsche

Re: undo weirdness with insert-file-contents

[martin rudalics]

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

> I looked at the C code for insert-file-contents, and its handling of the
> undo list looks pretty messy, it seems to be saving it and restoring
> multiple times (sometimes nested?) using multiple methods of restoring
> the old value....

... which is messy because I tried to avoid that intermediate steps of the
decoding process show up in the undo-list.  Please try the attached patch.

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

*** fileio.c.~1.602.~	Thu Feb 14 20:41:44 2008
--- fileio.c	Thu Feb 28 10:41:36 2008
***************
*** 4772,4778 ****
--- 4772,4783 ----
  		/* In the non-visiting case record only the final insertion. */
  		current_buffer->undo_list =
  		  Fcons (Fcons (lbeg, lend), Fcdr (old_undo));
+ 	      eles
+ 		current_buffer->undo_list =
+ 		  Fcons (Fcons (lbeg, lend), old_undo);
  	    }
+ 	  else
+ 	    current_buffer->undo_list = old_undo;
  	}
        else
  	/* If undo_list was Qt before, keep it that way.

Re: undo weirdness with insert-file-contents

[Miles Bader]

martin rudalics <rudalics@gmx.at> writes:
>> I looked at the C code for insert-file-contents, and its handling of the
>> undo list looks pretty messy, it seems to be saving it and restoring
>> multiple times (sometimes nested?) using multiple methods of restoring
>> the old value....
>
> ... which is messy because I tried to avoid that intermediate steps of the
> decoding process show up in the undo-list.  Please try the attached patch.

That avoids the "undo disabled" effect, but the resulting undo list
seems to be screwed up:  hitting undo as the first thing in the
resulting buffer ends up deleting a buffer range that is pretty clearly
bogus (the deleted range is from the first character in the buffer to
part-way through a word).

Not sure what's ending up where, but here's the (head of the) message
buffer as first displayed:

Foo: bar
Blat: Foop
To: 
Subject: 
From: Miles Bader <miles@dhapc248.dev.necel.com>
--text follows this line--

The "Foo: bar\n" was inserted by the (insert...) call inside the hook, the
"Blat: Foop\n" was inserted by the call to insert-file-contents (I made
it point to a file other than ~/.profile :-), and the rest inserted by
message-mode (not sure whether before or after the hook was called).

The value of buffer-undo-list is:

(nil
 (10 . 21)
 (1 . 21)
 (t 0 . 0))

-Miles

-- 
"An atheist doesn't have to be someone who thinks he has a proof that there
can't be a god.  He only has to be someone who believes that the evidence
on the God question is at a similar level to the evidence on the werewolf
question."  [John McCarthy]

Re: undo weirdness with insert-file-contents

[martin rudalics]

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

 > The value of buffer-undo-list is:
 >
 > (nil
 >  (10 . 21)
 >  (1 . 21)
 >  (t 0 . 0))

The (1 . 21) entry is silly and obviously breaks undoing the prior
(insert "Foo: bar\n").  Although EQ (XCAR (tem), lbeg) was not very
intelligent, setting this to (XCAR (tem) == lbeg) doesn't seem to help
either.  Please try again with the attached patch.

GDB doesn't work here currently, so I can't debug this.  But could you
try finding out in `insert-file-contents' (1) why the test in

	      if (CONSP (tem) && INTEGERP (XCAR (tem)) &&
		  INTEGERP (XCDR (tem)) && (XCAR (tem) == lbeg))
		/* In the non-visiting case record only the final insertion. */
		current_buffer->undo_list =
		  Fcons (Fcons (lbeg, lend), Fcdr (old_undo));

fails and (2) which value old_undo has here?  Thanks in advance.

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

*** fileio.c.~1.602.~	Thu Feb 14 20:41:44 2008
--- fileio.c	Thu Feb 28 13:59:42 2008
***************
*** 4674,4680 ****
  
        /* 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))
  	{
--- 4674,4679 ----
***************
*** 4768,4774 ****
  	    {
  	      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));
--- 4767,4773 ----
  	    {
  	      Lisp_Object tem = XCAR (old_undo);
  	      if (CONSP (tem) && INTEGERP (XCAR (tem)) &&
! 		  INTEGERP (XCDR (tem)) && (XCAR (tem) == lbeg))
  		/* In the non-visiting case record only the final insertion. */
  		current_buffer->undo_list =
  		  Fcons (Fcons (lbeg, lend), Fcdr (old_undo));

Re: undo weirdness with insert-file-contents

[Stefan Monnier]

> The (1 . 21) entry is silly and obviously breaks undoing the prior
> (insert "Foo: bar\n").  Although EQ (XCAR (tem), lbeg) was not very
> intelligent, setting this to (XCAR (tem) == lbeg) doesn't seem to help
> either.  Please try again with the attached patch.

I recommend you compile your Emacs with -DUSE_LISP_UNION_TYPE so the
compiler will explain to you where you're going wrong: XCAR (tem) is
a Lisp_Object, not an integer, so it cannot be compared with ==, but
only with EQ (which doesn't strike me as particularly non-intelligent in
this instance).


        Stefan

Re: undo weirdness with insert-file-contents

[martin rudalics]

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

> I recommend you compile your Emacs with -DUSE_LISP_UNION_TYPE so the
> compiler will explain to you where you're going wrong: XCAR (tem) is
> a Lisp_Object, not an integer, so it cannot be compared with ==, but
> only with EQ (which doesn't strike me as particularly non-intelligent in
> this instance).

Indeed.  Miles please try to debug with the attached patch instead.

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

*** fileio.c.~1.602.~	Thu Feb 14 20:41:44 2008
--- fileio.c	Thu Feb 28 20:21:18 2008
***************
*** 4674,4680 ****
  
        /* 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))
  	{
--- 4674,4679 ----

Re: undo weirdness with insert-file-contents

[Miles Bader]

martin rudalics <rudalics@gmx.at> writes:
> Indeed.  Miles please try to debug with the attached patch instead.

Should that patch replace the one you sent yesterday, or is it in
addition to it?  [sorry I can't actually test it until tomorrow.]

Thanks,

-Miles

-- 
White, adj. and n. Black.

Re: undo weirdness with insert-file-contents

[martin rudalics]

 > Should that patch replace the one you sent yesterday, or is it in
 > addition to it?  [sorry I can't actually test it until tomorrow.]

It should replace it.

Re: undo weirdness with insert-file-contents

[Stefan Monnier]

>> I recommend you compile your Emacs with -DUSE_LISP_UNION_TYPE so the
>> compiler will explain to you where you're going wrong: XCAR (tem) is
>> a Lisp_Object, not an integer, so it cannot be compared with ==, but
>> only with EQ (which doesn't strike me as particularly non-intelligent in
>> this instance).

> Indeed.  Miles please try to debug with the attached patch instead.

> *** fileio.c.~1.602.~	Thu Feb 14 20:41:44 2008
> --- fileio.c	Thu Feb 28 20:21:18 2008
> ***************
> *** 4674,4680 ****
  
>         /* Save old undo list and don't record undo for decoding. */
>         old_undo = current_buffer->undo_list;
> -       current_buffer->undo_list = Qt;
  
But this patch is clearly problematic, if one believe the comment: it
would cause the generation of undo info during the decoding step.
I understand that this info will be thrown away later, but we don't want
to pay the corresponding price.


        Stefan

Re: undo weirdness with insert-file-contents

[martin rudalics]

 >>-       current_buffer->undo_list = Qt;
 >
 > But this patch is clearly problematic, if one believe the comment: it
 > would cause the generation of undo info during the decoding step.
 > I understand that this info will be thrown away later, but we don't want
 > to pay the corresponding price.

The comment is mine, don't trust it.  Emacs 22 records undo information
all through the decoding step.  That's what I wanted to avoid.  But I'm
afraid two insertions get collapsed into one by the undo recording
mechanism and I'm comparing the wrong starting positions.  I don't yet
understand how this happens.

Re: undo weirdness with insert-file-contents

[Glenn Morris]

Miles Bader wrote:

> I define a "message-setup-hook" which for some reason has the unintended
> side-effect of turning off undo in the result message-mode buffer
> [buffer-undo-list has a value of t].

Here's a simpler example of the same thing (?):

http://lists.gnu.org/archive/html/emacs-devel/2008-02/msg02302.html

  From:   Bill Wohler
  Subject:     23.0.60; Disabled undo
  Date:        Sat, 23 Feb 2008 22:07:26 -0800

Re: undo weirdness with insert-file-contents

[martin rudalics]

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

>>I define a "message-setup-hook" which for some reason has the unintended
>>side-effect of turning off undo in the result message-mode buffer
>>[buffer-undo-list has a value of t].
> 
> 
> Here's a simpler example of the same thing (?):

Thanks for noting.

> http://lists.gnu.org/archive/html/emacs-devel/2008-02/msg02302.html
> 
>   From:   Bill Wohler
>   Subject:     23.0.60; Disabled undo
>   Date:        Sat, 23 Feb 2008 22:07:26 -0800

Bill, please try whether the attached patch works.

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

*** fileio.c.~1.602.~	Thu Feb 14 20:41:44 2008
--- fileio.c	Thu Feb 28 20:21:18 2008
***************
*** 4674,4680 ****
  
        /* 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))
  	{
--- 4674,4679 ----

Re: undo weirdness with insert-file-contents

[Glenn Morris]

martin rudalics wrote:

> Bill, please try whether the attached patch works.

Works for me. However the behaviour differs from Emacs 22:

(progn
  (pop-to-buffer "foo")
  (insert-file-contents "/etc/issue")
  (goto-char (point-max))
  (insert-file-contents "/etc/issue"))
M-x undo

now works in both 22 and 23, but in 22 the buffer is marked as not
modified after undo. It 23 it is marked as modified.

Re: undo weirdness with insert-file-contents

[martin rudalics]

 > (progn
 >   (pop-to-buffer "foo")
 >   (insert-file-contents "/etc/issue")
 >   (goto-char (point-max))
 >   (insert-file-contents "/etc/issue"))
 > M-x undo
 >
 > now works in both 22 and 23, but in 22 the buffer is marked as not
 > modified after undo. It 23 it is marked as modified.

... which is wrong, obviously.

Could you try to debug `insert-file-contents' around the lines 4768-4779
of fileio.c

	      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;

to see where the (t 0 . 0) entry gets removed?  Sorry but I currently
can't debug that myself.

Re: undo weirdness with insert-file-contents

[martin rudalics]

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

This is trickier than I thought.  The primary reason of the bug is that
record_insert combines two adjacent insertions thus messing up my
comparison after format decoding wrt the start of the inserted region.
Unless someone has a better idea, I'd handle this by inserting an undo
boundary for `insert-file-contents'.  The second problem is that
`insert-file-contents' does not consider an insertion a buffer change
when the buffer is empty.  This is obviously TRT in the file-visiting
case, but doesn't seem quite right in the non-visiting case.  Both
issues are involved in the buggy behaviors observed.  Please try another
time with the attached patch (I'm afraid it won't be the last).

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

*** fileio.c.~1.602.~	Thu Feb 14 20:41:44 2008
--- fileio.c	Fri Feb 29 23:30:18 2008
***************
*** 3730,3735 ****
--- 3730,3736 ----
    int read_quit = 0;
    Lisp_Object old_Vdeactivate_mark = Vdeactivate_mark;
    int we_locked_file = 0;
+   Lisp_Object original_undo_list = current_buffer->undo_list;
  
    if (current_buffer->base_buffer && ! NILP (visit))
      error ("Cannot do file visiting in an indirect buffer");
***************
*** 4589,4594 ****
--- 4590,4610 ----
  	current_buffer->enable_multibyte_characters = Qnil;
      }
  
+   if (inserted > 0
+       && CONSP (current_buffer->undo_list)
+       && (! NILP (XCAR (current_buffer->undo_list))))
+     /* Insert an undo boundary unless there's one here. */
+     current_buffer->undo_list =
+       Fcons (Qnil, current_buffer->undo_list);
+   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 ();
+     }
+ 
    coding.dst_multibyte = ! NILP (current_buffer->enable_multibyte_characters);
    if (CODING_MAY_REQUIRE_DECODING (&coding)
        && (inserted > 0 || CODING_REQUIRE_FLUSHING (&coding)))

Re: undo weirdness with insert-file-contents

[Stefan Monnier]

> +   if (inserted > 0
> +       && CONSP (current_buffer->undo_list)
> +       && (! NILP (XCAR (current_buffer->undo_list))))
> +     /* Insert an undo boundary unless there's one here. */
> +     current_buffer->undo_list =
> +       Fcons (Qnil, current_buffer->undo_list);

We should use Fundo_boundary here.

> +   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.
Also if the file is empty, is this going to mark the buffer as modified
even though nothing was changed?


        Stefan

Re: undo weirdness with insert-file-contents

[martin rudalics]

[-- 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.  */
  

Re: undo weirdness with insert-file-contents

[Stefan Monnier]

> the empty_undo_list check at all.  BTW, what is the semantics of
> `insert-file-contents' with VISIT nil and REPLACE non-nil?

It seems obvious to me that it's similar to "delete + insert".
So I guess I don't understand the question quite right.  I use this kind
of call to insert-file-contents in smerge-resolve, if you want to see
an example.

>> 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?

Yes.

> I don't handle that.

What does that mean?  Are you going to mark the buffer as modified?

> + 	/* 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 ();
> + 	}

Normally record_first_change is called "automatically" elsewhere.
So why is it needed here?

The handling of the undo-list and modified-p status in
insert-file-contents is a real mess.  It'd be good to take a step back
and think about how it *should* work.  E.g. I don't think
insert-file-contents should mess with modified-p: it should behave just
like a normal `insert' in this respect.


        Stefan

Re: undo weirdness with insert-file-contents

[martin rudalics]

 >>the empty_undo_list check at all.  BTW, what is the semantics of
 >>`insert-file-contents' with VISIT nil and REPLACE non-nil?
 >
 > It seems obvious to me that it's similar to "delete + insert".
 > So I guess I don't understand the question quite right.  I use this kind
 > of call to insert-file-contents in smerge-resolve, if you want to see
 > an example.

I asked because from reading the code I sometimes got the impression
that REPLACE => VISIT.

 >>>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?
 >
 > Yes.
 >
 >>I don't handle that.
 >
 > What does that mean?  Are you going to mark the buffer as modified?

Where do I mark a buffer as modified?  What I propose is to insert a
"first change" element in the undo list.  Is it that what you mean?

 > Normally record_first_change is called "automatically" elsewhere.
 > So why is it needed here?

Because it's _not_ done elsewhere.

 > The handling of the undo-list and modified-p status in
 > insert-file-contents is a real mess.

The main reason for the "mess" is that `insert-file-contents' has to
reconcile visiting a file and inserting the contents of some file into
an existing buffer.  In the first case the buffer should appear
unmodified and the undo-list empty after the insertion.  In the second
case the buffer should appear modified and the change recorded in the
undo list.

A second reason stems from the various decoding steps.  Ideally,
decoding should run transparently and only the "final" insertion get
recorded.  For this purpose you have to temporarily switch off undo
recording in the non-visiting case.  Otherwise, undoing changes could
reveal changes done by the decoding routine as can be observed with
Emacs 22.  Moreover, recording changes during decoding might be
expensive.

Finally, functions called by `insert-file-contents', for example,
`decode_coding' and `adjust_after_insert', may modify `buffer-undo-list'
in some way or the other.

 > It'd be good to take a step back
 > and think about how it *should* work.  E.g. I don't think
 > insert-file-contents should mess with modified-p: it should behave just
 > like a normal `insert' in this respect.

Its doc-string says "If second argument VISIT is non-nil, the buffer's
visited filename and last save file modtime are set, and it is marked
unmodified." hence we would have to change that first.

Maybe we could resolve these issues by renaming `insert-file-contents'
to `insert-file-contents-1' and having a new `insert-file-contents'
handle the undo list and the buffer-modified state while calling
`insert-file-contents-1' with undo disabled.

Re: undo weirdness with insert-file-contents

[Stefan Monnier]

>> What does that mean?  Are you going to mark the buffer as modified?

> Where do I mark a buffer as modified?

I don't know.  I just got the impression that your code might cause this
to happen.  If it doesn't happen, that's great.

> What I propose is to insert a "first change" element in the undo list.
> Is it that what you mean?

I'm only asking about the impact of your change, not the way it works.

>> Normally record_first_change is called "automatically" elsewhere.
>> So why is it needed here?
> Because it's _not_ done elsewhere.

Currently it's done at 3 places: record_property_change, record_point
(both of them are in undo.c) and modify_region in insdel.c.
And record_point is called from record_insert.  So normally it all
happens "automatically".

>> The handling of the undo-list and modified-p status in
>> insert-file-contents is a real mess.

> The main reason for the "mess" is that `insert-file-contents' has to
> reconcile visiting a file and inserting the contents of some file into
> an existing buffer.  In the first case the buffer should appear
> unmodified and the undo-list empty after the insertion.

When visiting a file, insert-file-contents is called from some other
piece of elisp which is the one that should take care of setting up the
undo-list and the unmodified status.  It shouldn't be
insert-file-contents's job.

> A second reason stems from the various decoding steps.  Ideally,
> decoding should run transparently and only the "final" insertion get
> recorded.  For this purpose you have to temporarily switch off undo
> recording in the non-visiting case.  Otherwise, undoing changes could
> reveal changes done by the decoding routine as can be observed with
> Emacs 22.  Moreover, recording changes during decoding might be
> expensive.

I understand this part, but it seems there's got to be a simpler way.

BTW, rather than use an undo-boundary, we could simply store the old
undo-list in some local var, then set undo-list to nil, then do the
dance, then nconc the new undo-list and the old undo-list.  Setting the
undo-list to nil temporarily hides the previous undo-list, thus
preventing merging entries.

>> It'd be good to take a step back and think about how it *should*
>> work.  E.g. I don't think insert-file-contents should mess with
>> modified-p: it should behave just like a normal `insert' in
>> this respect.

> Its doc-string says "If second argument VISIT is non-nil, the buffer's
> visited filename and last save file modtime are set, and it is marked
> unmodified." hence we would have to change that first.
> Maybe we could resolve these issues by renaming `insert-file-contents'
> to `insert-file-contents-1' and having a new `insert-file-contents'
> handle the undo list and the buffer-modified state while calling
> `insert-file-contents-1' with undo disabled.

Or rather create a new insert-file-contents-1 which doesn't take any
`visit' argument.


        Stefan

Re: undo weirdness with insert-file-contents

[martin rudalics]

 > Currently it's done at 3 places: record_property_change, record_point
 > (both of them are in undo.c) and modify_region in insdel.c.
 > And record_point is called from record_insert.  So normally it all
 > happens "automatically".

Maybe it doesn't happen because the

   if (MODIFF <= SAVE_MODIFF)

check fails.  Anyway, the behavior reported by Glenn in an earlier
posting (and also observed by me) indicates it doesn't happen.

 >>The main reason for the "mess" is that `insert-file-contents' has to
 >>reconcile visiting a file and inserting the contents of some file into
 >>an existing buffer.  In the first case the buffer should appear
 >>unmodified and the undo-list empty after the insertion.
 >
 > When visiting a file, insert-file-contents is called from some other
 > piece of elisp which is the one that should take care of setting up the
 > undo-list and the unmodified status.  It shouldn't be
 > insert-file-contents's job.

Currently `insert-file-contents' is the only routine knowing 'bout
whether it was able to do something with the REPLACE argument.  That's
also why I asked whether VISIT nil/REPLACE non-nil DTRT.  We always have
the dichotomy that from the user's point of view text is replaced from
BEG to END while in fact text was replaced only from REALBEG >= BEG to
REALEND =< REND.  IIUC, the REPLACE stuff was mainly written to make
reverting smoother.  Since buffer reverting throws the undo list away,
optimizing handling of the latter was hardly a concern.

Also note: We never care about text-properties here - hence strictly
spoken the REPLACE stuff _is_ incorrect.  Suppose I visit a file and
assign some text a face property - the buffer is modified.  Next I
revert the buffer and it appears unmodified although the text property
is still there.  In fact the only change caused by reverting was to make
the buffer appear unmodified and the undo list empty.  I know, with
font-locking there's hardly a way to tell whether a text property is
something that should be considered a "buffer change" or just a "hidden
buffer change" as Alan calls it.  Anyway: When you store text properties
on file the current behavior is inherently wrong :-(

I always wondered whether the REPLACE thingy could have been implemented
easier by comparing the region around `point' before and after the
replacement.  Many routines apply such heuristics (bookmarks, etags,
diff related programs).

 >>A second reason stems from the various decoding steps.  Ideally,
 >>decoding should run transparently and only the "final" insertion get
 >>recorded.  For this purpose you have to temporarily switch off undo
 >>recording in the non-visiting case.  Otherwise, undoing changes could
 >>reveal changes done by the decoding routine as can be observed with
 >>Emacs 22.  Moreover, recording changes during decoding might be
 >>expensive.
 >
 > I understand this part, but it seems there's got to be a simpler way.
 >
 > BTW, rather than use an undo-boundary, we could simply store the old
 > undo-list in some local var, then set undo-list to nil, then do the
 > dance,

... dancing with undo enabled?

 > then nconc the new undo-list and the old undo-list.  Setting the
 > undo-list to nil temporarily hides the previous undo-list, thus
 > preventing merging entries.

But _all_ we really need are the beginning and end of the inserted text.
We can get them easily with undo recording turned off during decoding -
once the problems with VISIT and REPLACE have been resolved.

 > Or rather create a new insert-file-contents-1 which doesn't take any
 > `visit' argument.

... but a REPLACE argument, I suppose.  See the example with BEG and
REALBEG above: Undo from BEG or REALBEG?  Don't change the modified
state when the region is (apparently) identic with that on the file?

Re: undo weirdness with insert-file-contents

[Stefan Monnier]

> I always wondered whether the REPLACE thingy could have been implemented
> easier by comparing the region around `point' before and after the
> replacement.  Many routines apply such heuristics (bookmarks, etags,
> diff related programs).

See my wishlist item in the emacsbugs page ;-)

>>> A second reason stems from the various decoding steps.  Ideally,
>>> decoding should run transparently and only the "final" insertion get
>>> recorded.  For this purpose you have to temporarily switch off undo
>>> recording in the non-visiting case.  Otherwise, undoing changes could
>>> reveal changes done by the decoding routine as can be observed with
>>> Emacs 22.  Moreover, recording changes during decoding might be
>>> expensive.
>> 
>> I understand this part, but it seems there's got to be a simpler way.
>> 
>> BTW, rather than use an undo-boundary, we could simply store the old
>> undo-list in some local var, then set undo-list to nil, then do the
>> dance,

> ... dancing with undo enabled?

>> then nconc the new undo-list and the old undo-list.  Setting the
>> undo-list to nil temporarily hides the previous undo-list, thus
>> preventing merging entries.

> But _all_ we really need are the beginning and end of the inserted text.
> We can get them easily with undo recording turned off during decoding -
> once the problems with VISIT and REPLACE have been resolved.

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).

>> Or rather create a new insert-file-contents-1 which doesn't take any
>> `visit' argument.

> ... but a REPLACE argument, I suppose.  See the example with BEG and
> REALBEG above: Undo from BEG or REALBEG?

The undo-list should record the finer description of the change.


        Stefan

Re: undo weirdness with insert-file-contents

[martin rudalics]

[-- 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

Re: undo weirdness with insert-file-contents

[Stefan Monnier]

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

Ahem... sorry, it's in the part of smerge-resolve that I haven't
yet installed.  Basically what it does is this:

given a 3-way merge conflict, with 3 branches MINE/BASE/OTHER, I do
a narrow-to-region on the conflict, then I save all three branches of
the conflict to separate files and do:

   diff -c -w BASE MINE | patch OTHER >RESULT

and if the patch succeeds I do `insert-file-contents' of RESULT without
VISIT but with REPLACE.

It's indeed somewhat obscure, and indeed, it crashed Emacs when the
Unicode branch was merged (because the code didn't expect REPLACE in
a narrowed buffer).


        Stefan

Re: undo weirdness with insert-file-contents

[martin rudalics]

 > given a 3-way merge conflict, with 3 branches MINE/BASE/OTHER, I do
 > a narrow-to-region on the conflict, then I save all three branches of
 > the conflict to separate files and do:
 >
 >    diff -c -w BASE MINE | patch OTHER >RESULT
 >
 > and if the patch succeeds I do `insert-file-contents' of RESULT without
 > VISIT but with REPLACE.
 >
 > It's indeed somewhat obscure, and indeed, it crashed Emacs when the
 > Unicode branch was merged (because the code didn't expect REPLACE in
 > a narrowed buffer).

With the MINE buffer current, I suppose, to avoid visiting RESULT,
inserting it into MINE, and deleting it afterwards.  As a benefit,
`insert-file-contents' will preserve markers within the conflict region
and store less undo information.  Sounds reasonable.

The examples of non-VISIT&REPLACE I've found in the Emacs sources seem
rather obscure though.

Re: undo weirdness with insert-file-contents

[Bill Wohler]

Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> The handling of the undo-list and modified-p status in
> insert-file-contents is a real mess.  It'd be good to take a step back
> and think about how it *should* work.  E.g. I don't think
> insert-file-contents should mess with modified-p: it should behave just
> like a normal `insert' in this respect.

I agree. The semantics of insert-file-contents seems to be identical to
insert--only the text comes from a file rather than an argument.

I'd therefore be against Martin's suggestion of inserting an undo
boundary since that should be the caller's responsibility (typically,
the command loop). Consider the following:

    (defun bw-foo ()
      (insert "abc")
      (insert "def"))

    (defun bw-bar ()
      (insert-file-contents "/etc/motd")
      (insert-file-contents "/etc/motd"))

A single undo in either case will remove both insertions and reset
buffer-modified-p to nil if that's what it was beforehand.

There is, regrettably, a large difference between these two. Point is
left where insert-file-contents was run rather than at the end of the
inserted string as is the case with insert. Maybe we should consider
fixing this at this time as well. Maybe this is the way it used to work,
or maybe this is the way people expect it to work: I took a quick look
at the MH-E code and many invocations of insert-file-contents are
immediately followed by (goto-char (point-min)) before the inserted file
is processed.

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD

Re: undo weirdness with insert-file-contents

[martin rudalics]

 >>The handling of the undo-list and modified-p status in
 >>insert-file-contents is a real mess.  It'd be good to take a step back
 >>and think about how it *should* work.  E.g. I don't think
 >>insert-file-contents should mess with modified-p: it should behave just
 >>like a normal `insert' in this respect.
 >
 > I agree. The semantics of insert-file-contents seems to be identical to
 > insert--only the text comes from a file rather than an argument.

`insert-file-contents' is used to set up the buffer when visiting a
file.  In that case you hardly want the buffer appear modified after
reading in the file or `point' appear at eob.  Also the whole REPLACE
handling is hardly comparable to what `insert' does (although it's
similar to active region handling with `delete-selection-mode').

 > I'd therefore be against Martin's suggestion of inserting an undo
 > boundary since that should be the caller's responsibility (typically,
 > the command loop). Consider the following:
 >
 >     (defun bw-foo ()
 >       (insert "abc")
 >       (insert "def"))
 >
 >     (defun bw-bar ()
 >       (insert-file-contents "/etc/motd")
 >       (insert-file-contents "/etc/motd"))
 >
 > A single undo in either case will remove both insertions and reset
 > buffer-modified-p to nil if that's what it was beforehand.

I have no opinion here.  But if you insert the second region _before_
the first region you need two undos anway.  Making the behavior depend
on some imaginary left-to-right order is hardly reasonable.  But I agree
that my undo boundary is purely artifical here I should remove it after
it has served its purpose.  A cleaner solution would be to use an extra
variable, say record_insert_no_combine, bind it to Qt in
`insert-file-contents', and have record_insert not combine two adjacent
insertions when this variable is non-nil.  Afterwards I can always
combine the insertions if people want it.

 > There is, regrettably, a large difference between these two. Point is
 > left where insert-file-contents was run rather than at the end of the
 > inserted string as is the case with insert. Maybe we should consider
 > fixing this at this time as well. Maybe this is the way it used to work,
 > or maybe this is the way people expect it to work: I took a quick look
 > at the MH-E code and many invocations of insert-file-contents are
 > immediately followed by (goto-char (point-min)) before the inserted file
 > is processed.

As explained above: The primary use of `insert-file-contents' is to set
up file-visiting buffers not to modify them.

Re: undo weirdness with insert-file-contents

[Bill Wohler]

Glenn Morris <rgm@gnu.org> writes:

> Miles Bader wrote:
>
>> I define a "message-setup-hook" which for some reason has the unintended
>> side-effect of turning off undo in the result message-mode buffer
>> [buffer-undo-list has a value of t].
>
> Here's a simpler example of the same thing (?):
>
> http://lists.gnu.org/archive/html/emacs-devel/2008-02/msg02302.html
>
>   From:   Bill Wohler
>   Subject:     23.0.60; Disabled undo
>   Date:        Sat, 23 Feb 2008 22:07:26 -0800

Thanks, Glenn. Yes, I suspect that it is the same thing. My original
message, however, didn't get quite the same attention as Miles' :-).

Martin, I'll assume that I'd observe the same as Glenn. I'll be happy
to try out new patches.

> now works in both 22 and 23, but in 22 the buffer is marked as not
> modified after undo. It 23 it is marked as modified.

I agree with Martin; the buffer, which is now in its original pristine
state, should not be marked modified.

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD