[Emacs-diffs] master fe3676f: (Finsert_file_contents): Keep buffer consistent in non-local exit

[Emacs-diffs] master fe3676f: (Finsert_file_contents): Keep buffer consistent in non-local exit

[Eli Zaretskii]

Is it right to remove decode_coding_unwind?  If set-auto-coding or
find-operation-coding-system signal an error, we will now:

  . leave the buffer unibyte and with undo-list bound to t
  . fail to reset the markers and overlays to their previous state
  . fail to reset intervals

Why is that TRT?  If there's something in your recent changes that
makes those repairs unnecessary, can you point me to what I missed?

Re: [Emacs-diffs] master fe3676f: (Finsert_file_contents): Keep buffer consistent in non-local exit

[Stefan Monnier]

> Is it right to remove decode_coding_unwind?  If set-auto-coding or
> find-operation-coding-system signal an error, we will now:
>
>   . leave the buffer unibyte and with undo-list bound to t

Since the error happens before decoding and even before we have chosen
a coding-system, there aren't many choices in this respect:
1- do as I do now (i.e. stay in unibyte even if the buffer was
   originally in multibyte).
2- erase the text we loaded (i.e. all the text, since the buffer
   started as empty) and revert the buffer to its original multibyteness
   state.
3- decode the text with some "default" coding system.  Note that the
   behavior (1) basically corresponds to a `no-conversion` coding system
   (since such a coding system also forces the buffer to unibyte).
4- keep the old "arbitrary bytes in a multibyte buffer" bug.

I think only (4) is clearly inferior and neither of the other is clearly
superior, so I went with the easier choice, especially since it's
expected to be a very rare corner case occurrence anyway (after all, we
lived with (4) for many years).

W.r.t the undo-list, I don't have a good reason for this choice, it was
just easier.  Since the buffer started as empty, there's a good chance
that the undo list had no interesting info anyway, so I felt like this
is even less important.

>   . fail to reset the markers and overlays to their previous state
>   . fail to reset intervals

These only need to be "reset" in the normal code path because we remove
the text from the buffer (to put it into the gap and then decode it),
but since in this case we don't remove the text, there's no need to do
that (and remember that the buffer was empty to start with so there's
not much "previous state" to restore in this respect).


        Stefan

Re: [Emacs-diffs] master fe3676f: (Finsert_file_contents): Keep buffer consistent in non-local exit

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Wed, 03 Jul 2019 12:43:10 -0400
> 
> 1- do as I do now (i.e. stay in unibyte even if the buffer was
>    originally in multibyte).
> 2- erase the text we loaded (i.e. all the text, since the buffer
>    started as empty) and revert the buffer to its original multibyteness
>    state.
> 3- decode the text with some "default" coding system.  Note that the
>    behavior (1) basically corresponds to a `no-conversion` coding system
>    (since such a coding system also forces the buffer to unibyte).
> 4- keep the old "arbitrary bytes in a multibyte buffer" bug.
> 
> I think only (4) is clearly inferior and neither of the other is clearly
> superior, so I went with the easier choice, especially since it's
> expected to be a very rare corner case occurrence anyway (after all, we
> lived with (4) for many years).

I think  we should do 2.

> W.r.t the undo-list, I don't have a good reason for this choice, it was
> just easier.  Since the buffer started as empty, there's a good chance
> that the undo list had no interesting info anyway, so I felt like this
> is even less important.

I think we should restore undo-list, because (a) it isn't hard, and
(b) someone at some point will complain if we don't.

> >   . fail to reset the markers and overlays to their previous state
> >   . fail to reset intervals
> 
> These only need to be "reset" in the normal code path because we remove
> the text from the buffer (to put it into the gap and then decode it),
> but since in this case we don't remove the text, there's no need to do
> that (and remember that the buffer was empty to start with so there's
> not much "previous state" to restore in this respect).

I don't think I understand this.  Are you saying that empty buffers
cannot have markers and overlays?

Re: [Emacs-diffs] master fe3676f: (Finsert_file_contents): Keep buffer consistent in non-local exit

[Stefan Monnier]

>> 1- do as I do now (i.e. stay in unibyte even if the buffer was
>>    originally in multibyte).
>> 2- erase the text we loaded (i.e. all the text, since the buffer
>>    started as empty) and revert the buffer to its original multibyteness
>>    state.
>> 3- decode the text with some "default" coding system.  Note that the
>>    behavior (1) basically corresponds to a `no-conversion` coding system
>>    (since such a coding system also forces the buffer to unibyte).
>> 4- keep the old "arbitrary bytes in a multibyte buffer" bug.
>> 
>> I think only (4) is clearly inferior and neither of the other is clearly
>> superior, so I went with the easier choice, especially since it's
>> expected to be a very rare corner case occurrence anyway (after all, we
>> lived with (4) for many years).
>
> I think  we should do 2.

I think as a user, after an error, I'd rather see the text decoded with
`raw-text-unix` (which is what we get now) than no text at all.
Also until now, the behavior has been to show the text we loaded, so
this would be a more significant change.

What would be the benefit of making the buffer empty?

>> W.r.t the undo-list, I don't have a good reason for this choice, it was
>> just easier.  Since the buffer started as empty, there's a good chance
>> that the undo list had no interesting info anyway, so I felt like this
>> is even less important.
>
> I think we should restore undo-list, because (a) it isn't hard, and
> (b) someone at some point will complain if we don't.

Fair enough, I'll do that.

>> >   . fail to reset the markers and overlays to their previous state
>> >   . fail to reset intervals
>> 
>> These only need to be "reset" in the normal code path because we remove
>> the text from the buffer (to put it into the gap and then decode it),
>> but since in this case we don't remove the text, there's no need to do
>> that (and remember that the buffer was empty to start with so there's
>> not much "previous state" to restore in this respect).
> I don't think I understand this.  Are you saying that empty buffers
> cannot have markers and overlays?

No I'm saying that all the markers were at BOB and it's perfectly
correct to have them moved elsewhere after the insertion of text in
the buffer.

The resetting of markers in the normal codepath is required for
correctness reasons (without it, we can end up with markers pointing
outside of the actual buffer text, for example), but in the non-local
exit case it's not needed and would only throw away information.

But if you prefer, I can move that back to the unwind function (along
with the undo-list resetting), of course.


        Stefan

Re: [Emacs-diffs] master fe3676f: (Finsert_file_contents): Keep buffer consistent in non-local exit

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 03 Jul 2019 13:48:27 -0400
> 
> > I think  we should do 2.
> 
> I think as a user, after an error, I'd rather see the text decoded with
> `raw-text-unix` (which is what we get now) than no text at all.
> Also until now, the behavior has been to show the text we loaded, so
> this would be a more significant change.
> 
> What would be the benefit of making the buffer empty?

I think our usual practice is to roll back any failed operations,
leaving things as they were before the problematic transaction
started.

> > I think we should restore undo-list, because (a) it isn't hard, and
> > (b) someone at some point will complain if we don't.
> 
> Fair enough, I'll do that.
> 
> >> >   . fail to reset the markers and overlays to their previous state
> >> >   . fail to reset intervals
> >> 
> >> These only need to be "reset" in the normal code path because we remove
> >> the text from the buffer (to put it into the gap and then decode it),
> >> but since in this case we don't remove the text, there's no need to do
> >> that (and remember that the buffer was empty to start with so there's
> >> not much "previous state" to restore in this respect).
> > I don't think I understand this.  Are you saying that empty buffers
> > cannot have markers and overlays?
> 
> No I'm saying that all the markers were at BOB and it's perfectly
> correct to have them moved elsewhere after the insertion of text in
> the buffer.
> 
> The resetting of markers in the normal codepath is required for
> correctness reasons (without it, we can end up with markers pointing
> outside of the actual buffer text, for example), but in the non-local
> exit case it's not needed and would only throw away information.
> 
> But if you prefer, I can move that back to the unwind function (along
> with the undo-list resetting), of course.

To reset the undo-list, you already need an unwind function, right?

If you are sure that markers and overlays will not be affected by
insertion that was aborted half-way, then the unwind function doesn't
need to do anything with them.  I couldn't convince myself that none
of these will be affected.