insert-file-contents and format-decode

insert-file-contents and format-decode

[martin rudalics]

To reproduce:

Open etc/enriched.doc, insert at buffer start the text

Contents

and do M-x revert-buffer.  Admire the guts of enriched.doc.


To explain:

The third element of `format-alist' is a regexp with the following
documentation:

REGEXP  is a regular expression to match against the beginning of the file;
         it should match only files in that format. ...

The regexp for enriched mode is "Content-[Tt]ype:[ \t]*text/enriched".
When reverting the buffer `insert-file-contents' attempts to preserve
"markers pointing to the unchanged parts" and thus not change "Content"
but insert "-[Tt]ype:[ \t]*text/enriched ...." after it.  The subsequent
matching step in `format-decode' fails because `point' is in between
"Content" and "-[Tt]ype:[ \t]*text/enriched" due to

       /* Set point before the inserted characters.  */
       SET_PT_BOTH (temp, same_at_start);

Apparently, the marker preserving optimization was written without
paying attention to `format-decode'.


To fix:

The call to `format-decode' in

       insval = call3 (Qformat_decode,
		      Qnil, make_number (inserted), visit);

should be performed with `point' at the original position and `inserted'
equalling the number of characters that would have been inserted without
the optimization.  After the call `point' may be restored to the
optimized position iff insval == inserted, that is, format-decode has
not modified the length of the "inserted" text (this might still fail in
pathological cases where format-decode removes and adds the same number
of characters but we shouldn't be too concerned about that).

Maybe someone familiar with the optimizations in `insert-file-contents'
could give it a try?


To consider:

The example given above is contrived.  The bug will become annoying when
you wrap the regexp in a comment to avoid, for example, a compiler
error.  In that case, any "real" comment at the beginning of the file
may trigger the bug.

FWIW, the `after-insert-file-functions' call

       insval = call1 (XCAR (p), make_number (inserted));

should be fixed in a similar manner.

Re: insert-file-contents and format-decode

[Richard Stallman]

Can you please write and test a fix for this bug?

Re: insert-file-contents and format-decode

[martin rudalics]

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

 > Can you please write and test a fix for this bug?

I test the attached patch for a week now and didn't encounter any
problems.  In addition to fixing the bug I described it moves the call
of after-change hooks _after_ the call to `after-insert-file-functions'.
The earlier behavior was IMO wrong.  Someone more familiar with the code
of fileio.c should have a look at that though.

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

*** fileio.c.~1.581.~	Sun Jun 10 17:46:54 2007
--- fileio.c	Sun Jun 17 15:08:20 2007
***************
*** 4715,4729 ****
  	  current_buffer->undo_list = Qt;
  	}
  
!       insval = call3 (Qformat_decode,
! 		      Qnil, make_number (inserted), visit);
!       CHECK_NUMBER (insval);
!       inserted = XFASTINT (insval);
  
        if (!NILP (visit))
  	current_buffer->undo_list = empty_undo_list_p ? Qnil : Qt;
      }
  
    /* 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
       "before" the buffer is changed.  */
--- 4715,4789 ----
  	  current_buffer->undo_list = Qt;
  	}
  
!       if (NILP (replace))
! 	{
! 	  insval = call3 (Qformat_decode,
! 			  Qnil, make_number (inserted), visit);
! 	  CHECK_NUMBER (insval);
! 	  inserted = XFASTINT (insval);
! 	}
!       else
! 	{
! 	  /* Suppose replace is non-nil and we succeeded in not replacing the
! 	  beginning or end of the buffer text with the file's contents.  In this
! 	  case we neverthelss have to call format-decode with `point' positioned
! 	  at the beginning of the buffer and `inserted' equalling the number of
! 	  characters in the buffer.  Otherwise, format-decode might fail to
! 	  correctly analyze the beginning or end of the buffer.  Hence we
! 	  temporarily save `point' and `inserted' here and restore `point' iff
! 	  format-decode didn't insert or delete any text.  Otherwise we leave
! 	  `point' at point-min. */
! 	  int opoint = PT;
! 	  int opoint_byte = PT_BYTE;
! 	  int oinserted = ZV - BEGV;
! 	  
! 	  TEMP_SET_PT_BOTH (BEGV, BEGV_BYTE); 
! 	  insval = call3 (Qformat_decode,
! 			  Qnil, make_number (oinserted), visit);
! 	  CHECK_NUMBER (insval);
! 	  if (insval = oinserted)
! 	    SET_PT_BOTH (opoint, opoint_byte);
! 	  inserted = XFASTINT (insval);
! 	}
  
        if (!NILP (visit))
  	current_buffer->undo_list = empty_undo_list_p ? Qnil : Qt;
      }
  
+   p = Vafter_insert_file_functions;
+   while (CONSP (p))
+     {
+       if (NILP (replace))
+ 	{
+ 	  insval = call1 (XCAR (p), make_number (inserted));
+ 	  if (!NILP (insval))
+ 	    {
+ 	      CHECK_NUMBER (insval);
+ 	      inserted = XFASTINT (insval);
+ 	    }
+ 	}
+       else
+ 	{
+ 	  /* For the rationale of this see the comment on format-decode above. */
+ 	  int opoint = PT;
+ 	  int opoint_byte = PT_BYTE;
+ 	  int oinserted = ZV - BEGV;
+ 	  
+ 	  TEMP_SET_PT_BOTH (BEGV, BEGV_BYTE); 
+ 	  insval = call1 (XCAR (p), make_number (oinserted));
+ 	  if (!NILP (insval))
+ 	    {
+ 	      CHECK_NUMBER (insval);
+ 	      if (insval = oinserted)
+ 		SET_PT_BOTH (opoint, opoint_byte);
+ 	      inserted = XFASTINT (insval);
+ 	    }
+ 	}
+       
+       QUIT;
+       p = XCDR (p);
+     }
+ 
    /* 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
       "before" the buffer is changed.  */
***************
*** 4734,4752 ****
        update_compositions (PT, PT, CHECK_BORDER);
      }
  
-   p = Vafter_insert_file_functions;
-   while (CONSP (p))
-     {
-       insval = call1 (XCAR (p), make_number (inserted));
-       if (!NILP (insval))
- 	{
- 	  CHECK_NUMBER (insval);
- 	  inserted = XFASTINT (insval);
- 	}
-       QUIT;
-       p = XCDR (p);
-     }
- 
    if (!NILP (visit)
        && current_buffer->modtime == -1)
      {
--- 4794,4799 ----

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

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

Re: insert-file-contents and format-decode

[Richard Stallman]

    I test the attached patch for a week now and didn't encounter any
    problems.  In addition to fixing the bug I described it moves the call
    of after-change hooks _after_ the call to `after-insert-file-functions'.

That seems like a mistake to me.  The `after-insert-file-functions'
are Lisp functions, and if they change the buffer, they will call the
after change hooks for what they did.  Therefore, the calls to the
after change hooks for this function's own direct changes in the
buffer should be done before.

So please install just the patch for the bug.

Re: insert-file-contents and format-decode

[martin rudalics]

 >     I test the attached patch for a week now and didn't encounter any
 >     problems.  In addition to fixing the bug I described it moves the call
 >     of after-change hooks _after_ the call to `after-insert-file-functions'.
 >
 > That seems like a mistake to me.  The `after-insert-file-functions'
 > are Lisp functions, and if they change the buffer, they will call the
 > after change hooks for what they did.  Therefore, the calls to the
 > after change hooks for this function's own direct changes in the
 > buffer should be done before.

(1) The `format-decode' ("round-trip" as the new Elisp manual entry
calls them) based functions are Lisp functions, may change the buffer,
and may call the after-change functions for what they do.  They precede,
within `insert-file-contents', the after-change-functions call.  If a
distinction between round-trip and "piecemeal" functions with respect to
calling after-change-functions is desired it should be documented.

(2) Calling `after-change-functions' from within `format-decode' or
`after-insert-file-functions' seems to me highly risky.  Personally, I'd
never trust any of them if they don't use `inhibit-modification-hooks'.

(3) Not calling `after-change-functions' after performing all functions
in `after-insert-file-functions' may mean, for example, not executing
`font-lock-after-change-function' after inserting the contents of some
file in the current buffer.

I think the only live and safe place to call `after-change-functions' is
after the entire file contents have been decoded, that means, after all
functions in `after-insert-file-functions' have been called.

Re: insert-file-contents and format-decode

[Richard Stallman]

    (1) The `format-decode' ("round-trip" as the new Elisp manual entry
    calls them) based functions are Lisp functions, may change the buffer,
    and may call the after-change functions for what they do.  They precede,
    within `insert-file-contents', the after-change-functions call.

That sounds like a bug to me.  I guess signal_after_change
should be done before `format-decode'.

Does anyone disagree?

Can you test such a change?

Re: insert-file-contents and format-decode

[Richard Stallman]

    (1) The `format-decode' ("round-trip" as the new Elisp manual entry
    calls them) based functions are Lisp functions, may change the buffer,
    and may call the after-change functions for what they do.  They precede,
    within `insert-file-contents', the after-change-functions call.

That sounds like a bug to me.  I guess signal_after_change
should be done before `format-decode'.

Does anyone disagree?

Can you test such a change?

    (2) Calling `after-change-functions' from within `format-decode' or
    `after-insert-file-functions' seems to me highly risky.  Personally, I'd
    never trust any of them if they don't use `inhibit-modification-hooks'.

I won't say you are wrong, but I don't see why that would be so.
Can you explain?

    (3) Not calling `after-change-functions' after performing all functions
    in `after-insert-file-functions' may mean, for example, not executing
    `font-lock-after-change-function' after inserting the contents of some
    file in the current buffer.

No, because if `after-insert-file-functions' change the buffer,
the primitives they use will themselves call signal_after_change.

Re: insert-file-contents and format-decode

[martin rudalics]

 >     (1) The `format-decode' ("round-trip" as the new Elisp manual entry
 >     calls them) based functions are Lisp functions, may change the buffer,
 >     and may call the after-change functions for what they do.  They precede,
 >     within `insert-file-contents', the after-change-functions call.
 >
 > That sounds like a bug to me.  I guess signal_after_change
 > should be done before `format-decode'.
 >
 > Does anyone disagree?

`format-decode' is similar to character decoding and eol conversion.  We
don't run `after-change-functions' for decoding characters or converting
line ends.

 > Can you test such a change?

It would have to be tested against the `after-change-functions' of many
major and minor modes (which holds for testing the present behavior as
well).

 >     (2) Calling `after-change-functions' from within `format-decode' or
 >     `after-insert-file-functions' seems to me highly risky.  Personally, I'd
 >     never trust any of them if they don't use `inhibit-modification-hooks'.
 >
 > I won't say you are wrong, but I don't see why that would be so.
 > Can you explain?

People who write after-change functions usually don't think in terms of
raw text.  They think in terms of fully decoded buffer text.

 >     (3) Not calling `after-change-functions' after performing all functions
 >     in `after-insert-file-functions' may mean, for example, not executing
 >     `font-lock-after-change-function' after inserting the contents of some
 >     file in the current buffer.
 >
 > No, because if `after-insert-file-functions' change the buffer,
 > the primitives they use will themselves call signal_after_change.

Then we must tell people to not inhibit such hooks while running the
primitives.

An aside: `format-decode' has

   (let ((mod (buffer-modified-p))
         ...
	  ;; Don't record undo information for the decoding.
         ...
       (set-buffer-modified-p mod))

hence it does not change the buffer-modified state.  It's somehow
unnatural to run `after-change-functions' and keep the buffer-modified
state unchanged at the same time.  The comment about not recording undo
information suggests that someone has been considering (but not
implementing) this.  If it's implemented it would probably have to be
done as in `after-insert-file-set-coding' (very tedious).  And we'd have
to decide and document how `after-insert-file-functions' shall handle
this.  If it's not implemented, `undo' may reveal the raw file contents
which seems worse.

Re: insert-file-contents and format-decode

[Richard Stallman]

    `format-decode' is similar to character decoding and eol conversion.

They are similar in their conceptual roles, I agree.

									  We
    don't run `after-change-functions' for decoding characters or converting
    line ends.

We do, when they operate on text already in the buffer, as for
M-x decode-coding-region.

However, when `insert-file-contents' does decoding, it does not run
the `after-change-functions' for that.  Instead it lumps the decoding
together with the reading of the file, and runs the
`after-change-functions' just once, for the insertion of the decoded
text.

It would be ok to do the same thing for format decoding, but that is
not what the current code does.  What it does now is this" first it
runs the `after-change-functions' for the decoding, then it runs the
`after-change-functions' for the insertion of the text resulting from
decoding.

That is clearly a bug.

Is your suggestion to fix this by disabling the modification hooks in
Finsert_file_contents around the call to `format-decode'?  That sounds
like a good idea.

     > No, because if `after-insert-file-functions' change the buffer,
     > the primitives they use will themselves call signal_after_change.

    Then we must tell people to not inhibit such hooks while running the
    primitives.

Maybe we should, for `after-insert-file-functions'.  Are there
any cases where those functions do inhibit these?

    An aside: `format-decode' has

       (let ((mod (buffer-modified-p))
	     ...
	      ;; Don't record undo information for the decoding.
	     ...
	   (set-buffer-modified-p mod))

Maybe that is a bug.  If you run format-decode not as part of
insertion, I think it should record undo info.  When it is called from
`insert-file-contents', that command should arrange just one undo
entry, for the inserted file contents as finally decoded.

Would you like to try this?

Re: insert-file-contents and format-decode

[martin rudalics]

 > Is your suggestion to fix this by disabling the modification hooks in
 > Finsert_file_contents around the call to `format-decode'?

Either in Finsert_file_contents or within `format-decode'.

I'm not sure what to do with `after-insert-file-functions' though.  The
current documentation suggests that these are handled the same way as
the `format-decode' based functions.  If you want to keep the current
behavior for them, this should be documented throughly.  That means, the
documentation should say that functions in `after-insert-file-functions'
have to take care of narrowing, `buffer-undo-list', after-change hooks,
and the like.

 > If you run format-decode not as part of
 > insertion, I think it should record undo info.  When it is called from
 > `insert-file-contents', that command should arrange just one undo
 > entry, for the inserted file contents as finally decoded.

In general `format-decode' should be run only by `insert-file-contents'
and functions like `format-decode-region' and `format-decode-buffer',
that is, functions in format.el.

 > Would you like to try this?

We'd have to distinguish the calls of `format-decode' by

(1) `insert-file-contents' with `visit-flag' t

(2) `insert-file-contents' with `visit-flag' nil

(3) functions within format.el (and maybe other functions)

for example, by abusing the visit-flag which could assume the value t
for (1), the symbol `insert-file-contents-non-visit' for (2), and nil
otherwise.  Then I'd simply steal the corresponding stuff from
`decode-coding-inserted-region' and bind `inhibit-read-only',
`inhibit-point-motion-hooks', and `inhibit-modification-hooks' to t
(unless that's already done in Finsert_file_contents).

Re: insert-file-contents and format-decode

[Richard Stallman]

     > If you run format-decode not as part of
     > insertion, I think it should record undo info.  When it is called from
     > `insert-file-contents', that command should arrange just one undo
     > entry, for the inserted file contents as finally decoded.

    In general `format-decode' should be run only by `insert-file-contents'
    and functions like `format-decode-region' and `format-decode-buffer',
    that is, functions in format.el.

Right.  And these functions should record undo information normally,
except when called from within `insert-file-contents'.

     > Would you like to try this?

    We'd have to distinguish the calls of `format-decode' by

    (1) `insert-file-contents' with `visit-flag' t

In this case, format decoding should make no undo information, just as
inserting the visited file contents makes no undo information.  A
newly visited file starts out with no undo information.

    (2) `insert-file-contents' with `visit-flag' nil

This is the case where it is desirable to make just one undo entry for
the file contents as finally decoded.

    (3) functions within format.el (and maybe other functions)

For this case, they should do nothing special about undo.
The primitives they call should make undo entries normally.

Re: insert-file-contents and format-decode

[martin rudalics]

 >     We'd have to distinguish the calls of `format-decode' by
 >
 >     (1) `insert-file-contents' with `visit-flag' t
 >
 > In this case, format decoding should make no undo information, just as
 > inserting the visited file contents makes no undo information.  A
 > newly visited file starts out with no undo information.
 >
 >     (2) `insert-file-contents' with `visit-flag' nil
 >
 > This is the case where it is desirable to make just one undo entry for
 > the file contents as finally decoded.
 >
 >     (3) functions within format.el (and maybe other functions)
 >
 > For this case, they should do nothing special about undo.
 > The primitives they call should make undo entries normally.

The problem is that I have to communicate this information in my call to
`format-decode'.  Currently we have no way to distinguish cases (2) and
(3) both have `visit-flag' nil.  Either we set `visit-flag' to some
constant (say `insert-file-contents-non-visit') for case (2) or provide
an additional argument to `format-decode'.

Re: insert-file-contents and format-decode

[Richard Stallman]

     >     (2) `insert-file-contents' with `visit-flag' nil
     >
     > This is the case where it is desirable to make just one undo entry for
     > the file contents as finally decoded.
     >
     >     (3) functions within format.el (and maybe other functions)
     >
     > For this case, they should do nothing special about undo.
     > The primitives they call should make undo entries normally.

    The problem is that I have to communicate this information in my call to
    `format-decode'.  Currently we have no way to distinguish cases (2) and
    (3) both have `visit-flag' nil.

In case 2, `insert-file-contents' can turn off undo around the call to
`format-decode', and make a single undo entry later on.  In case 1,
`insert-file-contents' can turn off undo around the call to
`format-decode', and not make any undo entry later on.

Thus, `format-decode' itself should not do anything special about undo
in any of the three cases.

Re: insert-file-contents and format-decode

[martin rudalics]

 >      >     (2) `insert-file-contents' with `visit-flag' nil
 >      >
 >      > This is the case where it is desirable to make just one undo entry for
 >      > the file contents as finally decoded.
 >      >
 >      >     (3) functions within format.el (and maybe other functions)
 >      >
 >      > For this case, they should do nothing special about undo.
 >      > The primitives they call should make undo entries normally.
 >
 >     The problem is that I have to communicate this information in my call to
 >     `format-decode'.  Currently we have no way to distinguish cases (2) and
 >     (3) both have `visit-flag' nil.
 >
 > In case 2, `insert-file-contents' can turn off undo around the call to
 > `format-decode', and make a single undo entry later on.  In case 1,
 > `insert-file-contents' can turn off undo around the call to
 > `format-decode', and not make any undo entry later on.
 >
 > Thus, `format-decode' itself should not do anything special about undo
 > in any of the three cases.

I currently run this in `insert-file-contents' once around the entire
call sequence for `format-decode' and `after-insert-file-functions'.
Do we want undo boundaries before and/or after the insertion?

However, I'm not sure whether handling undo in `insert-file-contents'
contradicts parts of an earlier thread of our discussion:

 >      > Is your suggestion to fix this by disabling the modification hooks in
 >      > Finsert_file_contents around the call to `format-decode'?
 >
 >     Either in Finsert_file_contents or within `format-decode'.
 >
 > I think it is ok to disable them unconditionally inside `format-decode'.
 > Decoding is sufficiently low level that it probably makes no sense
 > to expect them to run these hooks.
 >
 > Then Finsert_file_contents can run the hooks just once for the
 > (decoded) text that is ultimately inserted.

I suppose we do not want to disable the hooks for the "non-insert-file"
case, for example, interactive uses of `format-decode-region'.  Hence,
your suggestion to handle undo in `insert-file-contents' implies that we
have to disable these hooks in `insert-file-contents' too.  If,
alternatively, we wanted to disable them within `format-decode' we'd
have to run them in `format-decode-buffer' and `format-decode-region'
explicitly after calling `format-decode'.

Finally, I'm completely uncertain what to do about `format-insert-file':
That function explicitly prompts for a format and does `format-decode'
after inserting the file and running any `after-insert-file-functions'.
Shall we treat this is as a sequence of

(insert-file-contents filename nil beg end)

(let ((format-alist nil))
   (format-decode-region (point) (+ (point) size) format))

with _two_ `buffer-undo-list' entries and `after-change-functions'
calls?

Re: insert-file-contents and format-decode

[Richard Stallman]

    I currently run this in `insert-file-contents' once around the entire
    call sequence for `format-decode' and `after-insert-file-functions'.

That is the right way.

    Do we want undo boundaries before and/or after the insertion?

An editing primitive such as `Finsert_file_contents' should not make
any undo boundaries.  An undo boundary is made by the main editor loop
and by an explicit call to `undo-boundary'.

    I suppose we do not want to disable the hooks for the "non-insert-file"
    case, for example, interactive uses of `format-decode-region'.  Hence,
    your suggestion to handle undo in `insert-file-contents' implies that we
    have to disable these hooks in `insert-file-contents' too.

Right.

    Finally, I'm completely uncertain what to do about `format-insert-file':
    That function explicitly prompts for a format and does `format-decode'
    after inserting the file and running any `after-insert-file-functions'.
    Shall we treat this is as a sequence of


    (insert-file-contents filename nil beg end)

    (let ((format-alist nil))
       (format-decode-region (point) (+ (point) size) format))

    with _two_ `buffer-undo-list' entries and `after-change-functions'
    calls?

No, I suggest making just one undo entry for the insertion
of the decoded text.  Just as `insert-file-contents' would do.

Re: insert-file-contents and format-decode

[martin rudalics]

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

>     Finally, I'm completely uncertain what to do about `format-insert-file':
>     That function explicitly prompts for a format and does `format-decode'
>     after inserting the file and running any `after-insert-file-functions'.
>     Shall we treat this is as a sequence of
> 
> 
>     (insert-file-contents filename nil beg end)
> 
>     (let ((format-alist nil))
>        (format-decode-region (point) (+ (point) size) format))
> 
>     with _two_ `buffer-undo-list' entries and `after-change-functions'
>     calls?
> 
> No, I suggest making just one undo entry for the insertion
> of the decoded text.  Just as `insert-file-contents' would do.

I tried this in the attached patch.

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

*** format.el	Tue Jan 23 06:40:02 2007
--- format.el	Mon Jul  2 08:54:40 2007
***************
*** 429,441 ****
  	  (fmt (format-read (format "Read file `%s' in format: "
  				    (file-name-nondirectory file)))))
       (list file fmt)))
!   (let (value size)
!     (let ((format-alist nil))
!       (setq value (insert-file-contents filename nil beg end))
!       (setq size (nth 1 value)))
!     (if format
! 	(setq size (format-decode format size)
! 	      value (list (car value) size)))
      value))
  
  (defun format-read (&optional prompt)
--- 429,462 ----
  	  (fmt (format-read (format "Read file `%s' in format: "
  				    (file-name-nondirectory file)))))
       (list file fmt)))
!   (let (value size old-undo)
!     ;; Record only one undo entry for the insertion.  Inhibit point-motion and
!     ;; modification hooks as with `insert-file-contents'.
!     (let ((inhibit-point-motion-hooks t)
! 	  (inhibit-modification-hooks t))
!       ;; Don't bind `buffer-undo-list' to t here to assert that
!       ;; `insert-file-contents' may record whether the buffer was unmodified
!       ;; before.
!       (let ((format-alist nil))
! 	(setq value (insert-file-contents filename nil beg end))
! 	(setq size (nth 1 value)))
!       (when (consp buffer-undo-list)
! 	(let ((head (car buffer-undo-list)))
! 	  (when (and (consp head)
! 		     (equal (car head) (point))
! 		     (equal (cdr head) (+ (point) size)))
! 	    ;; Remove first entry from `buffer-undo-list', we shall insert
! 	    ;; another one below.
! 	    (setq old-undo (cdr buffer-undo-list)))))
!       (when format
! 	(let ((buffer-undo-list t))
! 	  (setq size (format-decode format size)
! 		value (list (car value) size)))
! 	(unless (eq buffer-undo-list t)
! 	  (setq buffer-undo-list
! 		(cons (cons (point) (+ (point) size)) old-undo)))))
!     (unless inhibit-modification-hooks
!       (run-hook-with-args 'after-change-functions (point) (+ (point) size) 0))
      value))
  
  (defun format-read (&optional prompt)

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

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

Re: insert-file-contents and format-decode

[Richard Stallman]

    !   (let (value size old-undo)
    !     ;; Record only one undo entry for the insertion.  Inhibit point-motion and
    !     ;; modification hooks as with `insert-file-contents'.
    !     (let ((inhibit-point-motion-hooks t)
    ! 	  (inhibit-modification-hooks t))
    !       ;; Don't bind `buffer-undo-list' to t here to assert that
    !       ;; `insert-file-contents' may record whether the buffer was unmodified
    !       ;; before.
    !       (let ((format-alist nil))
    ! 	(setq value (insert-file-contents filename nil beg end))
    ! 	(setq size (nth 1 value)))
    !       (when (consp buffer-undo-list)
    ! 	(let ((head (car buffer-undo-list)))
    ! 	  (when (and (consp head)
    ! 		     (equal (car head) (point))
    ! 		     (equal (cdr head) (+ (point) size)))
    ! 	    ;; Remove first entry from `buffer-undo-list', we shall insert
    ! 	    ;; another one below.
    ! 	    (setq old-undo (cdr buffer-undo-list)))))

That pateh is basically good, but it could be simpler.  Instead of
letting insert-file-contents produce an undo entry and taking it off
and taking it apart, it would be better to inhibit undo around
insert-file-contents and generate the undo entry from scratch in all
cases.  When no format decoding is done, VALUE has the size you need.

Re: insert-file-contents and format-decode

[martin rudalics]

 >     !   (let (value size old-undo)
 >     !     ;; Record only one undo entry for the insertion.  Inhibit point-motion and
 >     !     ;; modification hooks as with `insert-file-contents'.
 >     !     (let ((inhibit-point-motion-hooks t)
 >     ! 	  (inhibit-modification-hooks t))
 >     !       ;; Don't bind `buffer-undo-list' to t here to assert that
 >     !       ;; `insert-file-contents' may record whether the buffer was unmodified
 >     !       ;; before.
 >     !       (let ((format-alist nil))
 >     ! 	(setq value (insert-file-contents filename nil beg end))
 >     ! 	(setq size (nth 1 value)))
 >     !       (when (consp buffer-undo-list)
 >     ! 	(let ((head (car buffer-undo-list)))
 >     ! 	  (when (and (consp head)
 >     ! 		     (equal (car head) (point))
 >     ! 		     (equal (cdr head) (+ (point) size)))
 >     ! 	    ;; Remove first entry from `buffer-undo-list', we shall insert
 >     ! 	    ;; another one below.
 >     ! 	    (setq old-undo (cdr buffer-undo-list)))))
 >
 > That pateh is basically good, but it could be simpler.  Instead of
 > letting insert-file-contents produce an undo entry and taking it off
 > and taking it apart, it would be better to inhibit undo around
 > insert-file-contents and generate the undo entry from scratch in all
 > cases.  When no format decoding is done, VALUE has the size you need.

It's complicated because I may have to create the (t HIGH . LOW) entry
to indicate that the buffer previously had "unmodified" status.  With my
solution I let `insert-file-contents' take care of that automatically.
With your solution I would have to create it manually and care about the
modification time myself.  Is it worth the trouble?

Re: insert-file-contents and format-decode

[Richard Stallman]

    It's complicated because I may have to create the (t HIGH . LOW) entry
    to indicate that the buffer previously had "unmodified" status.  With my
    solution I let `insert-file-contents' take care of that automatically.
    With your solution I would have to create it manually and care about the
    modification time myself.  Is it worth the trouble?

Maybe you are right.  If the other method is not substantially simpler
then your method is fine.

Re: insert-file-contents and format-decode

[Richard Stallman]

     > Is your suggestion to fix this by disabling the modification hooks in
     > Finsert_file_contents around the call to `format-decode'?

    Either in Finsert_file_contents or within `format-decode'.

I think it is ok to disable them unconditionally inside `format-decode'.
Decoding is sufficiently low level that it probably makes no sense
to expect them to run these hooks.

Then Finsert_file_contents can run the hooks just once for the
(decoded) text that is ultimately inserted.

    I'm not sure what to do with `after-insert-file-functions' though.  The
    current documentation suggests that these are handled the same way as
    the `format-decode' based functions.  If you want to keep the current
    behavior for them, this should be documented throughly.  That means, the
    documentation should say that functions in `after-insert-file-functions'
    have to take care of narrowing, `buffer-undo-list', after-change hooks,
    and the like.

With the current plan, they don't have to deal with undo or change hooks.
How do they have "take care of" narrowing?

Re: insert-file-contents and format-decode

[martin rudalics]

 >      > Is your suggestion to fix this by disabling the modification hooks in
 >      > Finsert_file_contents around the call to `format-decode'?
 >
 >     Either in Finsert_file_contents or within `format-decode'.
 >
 > I think it is ok to disable them unconditionally inside `format-decode'.
 > Decoding is sufficiently low level that it probably makes no sense
 > to expect them to run these hooks.
 >
 > Then Finsert_file_contents can run the hooks just once for the
 > (decoded) text that is ultimately inserted.

Agreed.

 >
 >     I'm not sure what to do with `after-insert-file-functions' though.  The
 >     current documentation suggests that these are handled the same way as
 >     the `format-decode' based functions.  If you want to keep the current
 >     behavior for them, this should be documented throughly.  That means, the
 >     documentation should say that functions in `after-insert-file-functions'
 >     have to take care of narrowing, `buffer-undo-list', after-change hooks,
 >     and the like.
 >
 > With the current plan, they don't have to deal with undo or change
 >     hooks.

We have a plan for dealing with functions called by `format-decode'.  We
do not have a plan yet for dealing with `after-insert-file-functions'.
Shall we treat functions there the same way we treat functions called by
`format-decode'?  If so we would have to deal with this right in
`Finsert_file_contents'.  Or shall we keep things as they are?  In this
case the documentation should say the things mentioned above.

 > How do they have "take care of" narrowing?

When I insert the contents of a file with `visit-flag' nil the buffer
should be reasonably narrowed to work only on the inserted text as in
`decode-coding-inserted-region'.  Currently, neither `format-alist' nor
`after-insert-file-functions' handling provides such a service.  The
functions there are supposed to do the narrowing themselves.

Re: insert-file-contents and format-decode

[Richard Stallman]

    We have a plan for dealing with functions called by `format-decode'.  We
    do not have a plan yet for dealing with `after-insert-file-functions'.
    Shall we treat functions there the same way we treat functions called by
    `format-decode'?

If it's good for one, it's probably good for the other.  Consistency
between the two features is also good.

Do you see any reason NOT to do so?

    When I insert the contents of a file with `visit-flag' nil the buffer
    should be reasonably narrowed to work only on the inserted text as in
    `decode-coding-inserted-region'.  Currently, neither `format-alist' nor
    `after-insert-file-functions' handling provides such a service.  The
    functions there are supposed to do the narrowing themselves.

Perhaps it would be convenient to narrow around the call to
the `after-insert-file-functions'.  This would not contradict the
established calling convention.

If we want to narrow around format conversion functions, the place
to do that is inside format.el.

Re: insert-file-contents and format-decode

[martin rudalics]

 >     We have a plan for dealing with functions called by `format-decode'.  We
 >     do not have a plan yet for dealing with `after-insert-file-functions'.
 >     Shall we treat functions there the same way we treat functions called by
 >     `format-decode'?
 >
 > If it's good for one, it's probably good for the other.  Consistency
 > between the two features is also good.
 >
 > Do you see any reason NOT to do so?

We could introduce one: Keep `format-decode' for practical use and
`after-insert-file-functions' for applications that want to fine-tune
every single step of the decoding process.

 >     When I insert the contents of a file with `visit-flag' nil the buffer
 >     should be reasonably narrowed to work only on the inserted text as in
 >     `decode-coding-inserted-region'.  Currently, neither `format-alist' nor
 >     `after-insert-file-functions' handling provides such a service.  The
 >     functions there are supposed to do the narrowing themselves.
 >
 > Perhaps it would be convenient to narrow around the call to
 > the `after-insert-file-functions'.  This would not contradict the
 > established calling convention.
 >
 > If we want to narrow around format conversion functions, the place
 > to do that is inside format.el.

I already changed my mind: I now think instead that `format-decode' and
`after-insert-file-functions' _should_ be able to detect whether the
insertion is done at the beginning of the current buffer or any other
position (simply by comparing the `begin' argument with `point-min'
wthout any need to widen the buffer).

The only clean way to handle this would be using a temporary buffer for
decoding.

Re: insert-file-contents and format-decode

[Richard Stallman]

     >     We have a plan for dealing with functions called by `format-decode'.  We
     >     do not have a plan yet for dealing with `after-insert-file-functions'.
     >     Shall we treat functions there the same way we treat functions called by
     >     `format-decode'?
     >
     > If it's good for one, it's probably good for the other.  Consistency
     > between the two features is also good.
     >
     > Do you see any reason NOT to do so?

    We could introduce one: Keep `format-decode' for practical use and
    `after-insert-file-functions' for applications that want to fine-tune
    every single step of the decoding process.

Incoherence is a minus, not a plus.  If there is no specific reason
to treat them differentlky, let's treat them the same!

    I already changed my mind: I now think instead that `format-decode' and
    `after-insert-file-functions' _should_ be able to detect whether the
    insertion is done at the beginning of the current buffer or any other
    position (simply by comparing the `begin' argument with `point-min'
    wthout any need to widen the buffer).

If you think that's useful, then let's not insert a save-restriction.

So the only remaining change to be done is to inhibit execution of
certain hooks in `format-decode' and around the calls to
`after-insert-file-functions'.

Would you like to implement this?

Re: insert-file-contents and format-decode

[martin rudalics]

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

> So the only remaining change to be done is to inhibit execution of
> certain hooks in `format-decode' and around the calls to
> `after-insert-file-functions'.

Please have a look at the attached patch (my acquaintance with C code
is marginal).  Handa-san could you please check whether such a patch
would introduce conflicts with the unicode-2 branch.


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

*** fileio.c	Mon Jul  2 07:27:56 2007
--- fileio.c	Mon Jul  2 08:23:20 2007
***************
*** 3720,3727 ****
    register int how_much;
    register int unprocessed;
    int count = SPECPDL_INDEX ();
!   struct gcpro gcpro1, gcpro2, gcpro3, gcpro4;
!   Lisp_Object handler, val, insval, orig_filename;
    Lisp_Object p;
    int total = 0;
    int not_regular = 0;
--- 3720,3727 ----
    register int how_much;
    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;
***************
*** 3744,3751 ****
    val = Qnil;
    p = Qnil;
    orig_filename = Qnil;
  
!   GCPRO4 (filename, val, p, orig_filename);
  
    CHECK_STRING (filename);
    filename = Fexpand_file_name (filename, Qnil);
--- 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);
***************
*** 4704,4727 ****
    /* Decode file format */
    if (inserted > 0)
      {
!       int empty_undo_list_p = 0;
! 
!       /* If we're anyway going to discard undo information, don't
! 	 record it in the first place.  The buffer's undo list at this
! 	 point is either nil or t when visiting a file.  */
!       if (!NILP (visit))
  	{
! 	  empty_undo_list_p = NILP (current_buffer->undo_list);
! 	  current_buffer->undo_list = Qt;
  	}
  
!       insval = call3 (Qformat_decode,
! 		      Qnil, make_number (inserted), visit);
!       CHECK_NUMBER (insval);
!       inserted = XFASTINT (insval);
! 
!       if (!NILP (visit))
! 	current_buffer->undo_list = empty_undo_list_p ? Qnil : Qt;
      }
  
    /* Call after-change hooks for the inserted text, aside from the case
--- 4705,4806 ----
    /* Decode file format */
    if (inserted > 0)
      {
!       /* Don't run point motion or modification hooks when decoding. */
!       int count = SPECPDL_INDEX ();
!       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,
! 			  Qnil, make_number (inserted), visit);
! 	  CHECK_NUMBER (insval);
! 	  inserted = XFASTINT (insval);
! 	}
!       else
! 	{
! 	  /* Suppose replace is non-nil and we succeeded in not replacing the
! 	  beginning or end of the buffer text with the file's contents.  In this
! 	  case we neverthelss have to call format-decode with `point' positioned
! 	  at the beginning of the buffer and `inserted' equalling the number of
! 	  characters in the buffer.  Otherwise, format-decode might fail to
! 	  correctly analyze the beginning or end of the buffer.  Hence we
! 	  temporarily save `point' and `inserted' here and restore `point' iff
! 	  format-decode didn't insert or delete any text.  Otherwise we leave
! 	  `point' at point-min. */
! 	  int opoint = PT;
! 	  int opoint_byte = PT_BYTE;
! 	  int oinserted = ZV - BEGV;
! 	  
! 	  TEMP_SET_PT_BOTH (BEGV, BEGV_BYTE); 
! 	  insval = call3 (Qformat_decode,
! 			  Qnil, make_number (oinserted), visit);
! 	  CHECK_NUMBER (insval);
! 	  if (insval = oinserted)
! 	    SET_PT_BOTH (opoint, opoint_byte);
! 	  inserted = XFASTINT (insval);
  	}
  
!       /* For consistency with format-decode call these now iff inserted > 0
! 	 (martin 2007-06-28) */
!       p = Vafter_insert_file_functions;
!       while (CONSP (p))
! 	{
! 	  if (NILP (replace))
! 	    {
! 	      insval = call1 (XCAR (p), make_number (inserted));
! 	      if (!NILP (insval))
! 		{
! 		  CHECK_NUMBER (insval);
! 		  inserted = XFASTINT (insval);
! 		}
! 	    }
! 	  else
! 	    {
! 	      /* For the rationale of this see the comment on format-decode above. */
! 	      int opoint = PT;
! 	      int opoint_byte = PT_BYTE;
! 	      int oinserted = ZV - BEGV;
! 	      
! 	      TEMP_SET_PT_BOTH (BEGV, BEGV_BYTE); 
! 	      insval = call1 (XCAR (p), make_number (oinserted));
! 	      if (!NILP (insval))
! 		{
! 		  CHECK_NUMBER (insval);
! 		  if (insval = oinserted)
! 		    SET_PT_BOTH (opoint, opoint_byte);
! 		  inserted = XFASTINT (insval);
! 		}
! 	    }
! 	  
! 	  QUIT;
! 	  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)) && (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
! 	/* In the visiting case restore the previous value. */
! 	current_buffer->undo_list = old_undo;
!       
!       unbind_to (count, Qnil);
      }
  
    /* Call after-change hooks for the inserted text, aside from the case
***************
*** 4734,4752 ****
        update_compositions (PT, PT, CHECK_BORDER);
      }
  
-   p = Vafter_insert_file_functions;
-   while (CONSP (p))
-     {
-       insval = call1 (XCAR (p), make_number (inserted));
-       if (!NILP (insval))
- 	{
- 	  CHECK_NUMBER (insval);
- 	  inserted = XFASTINT (insval);
- 	}
-       QUIT;
-       p = XCDR (p);
-     }
- 
    if (!NILP (visit)
        && current_buffer->modtime == -1)
      {
--- 4813,4818 ----


[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

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

Re: insert-file-contents and format-decode

[Richard Stallman]

    ! 	  /* Suppose replace is non-nil and we succeeded in not replacing the
    ! 	  beginning or end of the buffer text with the file's contents.  In this
    ! 	  case we neverthelss have to call format-decode with `point' positioned
    ! 	  at the beginning of the buffer

I think that's a bug.  Point has to be at the beginning of the
inserted text.  If you put it at the beginning of the buffer,
the decoding functions have no way to determine what text
was just inserted, so they can't possibly do the job.

The same goes for the Vafter_insert_file_functions.
It is the only way to make that case work.

If that conflicts with documentation, then the documentation
has to be corrected.

    !       else
    ! 	/* In the visiting case restore the previous value. */
    ! 	current_buffer->undo_list = old_undo;

When visiting a file, you should set undo_list to t
if it was t before, otherwise to nil.

Re: insert-file-contents and format-decode

[martin rudalics]

 >     ! 	  /* Suppose replace is non-nil and we succeeded in not replacing the
 >     ! 	  beginning or end of the buffer text with the file's contents.  In this
 >     ! 	  case we neverthelss have to call format-decode with `point' positioned
 >     ! 	  at the beginning of the buffer
 >
 > I think that's a bug.  Point has to be at the beginning of the
 > inserted text.  If you put it at the beginning of the buffer,
 > the decoding functions have no way to determine what text
 > was just inserted, so they can't possibly do the job.

I don't understand: Fixing this was my primary intention (where
"beginning of the buffer" should read "beginning of the accessible
portion of the buffer" though in practice this is needed only for
`revert-buffer').  That's what the "replace" stuff is all about.  In the
particular case the decoding functions should be fooled into believing
that more text was inserted from the file.

 > The same goes for the Vafter_insert_file_functions.
 > It is the only way to make that case work.

These are done for the replace non-nil case only.  Both `format-decode'
and `after-insert-file-functions' were inherently broken in that case.

 > If that conflicts with documentation, then the documentation
 > has to be corrected.

There's no specific documentation on that.  `format-decode' and
`after-insert-file-functions' simply ceased to work correctly after the
`insert-file-contents' optimizations for the replace non-nil case were
installed.

 >
 >     !       else
 >     ! 	/* In the visiting case restore the previous value. */
 >     ! 	current_buffer->undo_list = old_undo;
 >
 > When visiting a file, you should set undo_list to t
 > if it was t before, otherwise to nil.
 >

I believed an earlier comment here that read as

       /* If we're anyway going to discard undo information, don't
	 record it in the first place.  The buffer's undo list at this
	 point is either nil or t when visiting a file.  */

Hence my

 >     ! 	current_buffer->undo_list = old_undo;

should have done that automatically.  Is it better to rewrite as:

       if (NILP (visit))
	{
	  ...
	}
       else if (old_undo == Qt)
	/* If undo_list was Qt before, keep it that way. */
	current_buffer->undo_list = Qt;
       else
	/* Otherwise start with an empty undo_list. */
	current_buffer->undo_list = Qnil;

Re: insert-file-contents and format-decode

[Richard Stallman]

I think your arguments are valid, so would you please install
your patch in the trunk?

Please install the description in etc/NEWS at the same time.

Please forgive my delay.