bug#21523: 25.0.50; Undo with active region adds extra text

bug#21523: 25.0.50; Undo with active region adds extra text

[Drew Adams]

The Subject line is not very clear.  Here is a recipe.  You decide
whether the behavior is correct or a bug.  FWIW, this behavior is at
least as old as Emacs 20 (with transient-mark-mode on).

emacs -Q

In buffer *scratch*, put point before "that" on the last line.
`M-c', to capitalize "That".
`C-SPC C-e', to select the text after "That", up to eol.
(The region does not contain the word "That".)

`C-_' to undo the last change within the region.
The word "that" is inserted, giving this:

;; then enter the text in Thatthat file=A1=AFs own buffer.

This seems disconcerting, at least.  (Same behavior for `M-u' etc.)

(I can see the explanation of why this happens coming as a reply.  As I
say, you can decide whether this behavior is OK as is.)


In GNU Emacs 25.0.50.1 (i686-pc-mingw32)
 of 2015-09-05
Bzr revision: 2330ca33a97867f2ea1123bcf7bfe5cfcc030b36
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --host=3Di686-pc-mingw32 --enable-checking=3Dyes,glyphs'

bug#21523: 25.0.50; Undo with active region adds extra text

[Stefan Monnier]

> `C-_' to undo the last change within the region.
> The word "that" is inserted, giving this:
>   ;; then enter the text in Thatthat file's own buffer.
> This seems disconcerting, at least.  (Same behavior for `M-u' etc.)

Indeed, that's wrong.  I haven't looked in detail of why this happens,
but I can guess that it's because the previous change is represented as
"insert That" and "remove that", and when we undo them, the exact
location of "remove that" ends up right at the region boundary, making
it unclear whether it should be considered as "inside" or "outside".

Not sure how best to fix it.  Maybe such replacements should have
a special status in the undo-list, so they aren't considered as
two independent changes (remove+insert) but as a single one.

Or maybe the "undo-in-region" should only ever undo complete steps
(i.e. everything between two undo boundaries), so if any part of an undo
step affects text outside of the region, then the whole step is skipped.


        Stefan

bug#21523: 25.0.50; Undo with active region adds extra text

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > Or maybe the "undo-in-region" should only ever undo complete steps
  > (i.e. everything between two undo boundaries), so if any part of an undo
  > step affects text outside of the region, then the whole step is skipped.

The undo commands are supposed to undo a complete change
as one unit.  If in this case it fails to do that, that would seem
to be a bug, right?

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.

bug#21523: 25.0.50; Undo with active region adds extra text

[Stefan Monnier]

>> Or maybe the "undo-in-region" should only ever undo complete steps
>> (i.e. everything between two undo boundaries), so if any part of an undo
>> step affects text outside of the region, then the whole step is skipped.

> The undo commands are supposed to undo a complete change
> as one unit.  If in this case it fails to do that, that would seem
> to be a bug, right?

Yes, that's what I was also trying to say.  I think that'd be the
better fix.


        Stefan

bug#21523: 25.0.50; Undo with active region adds extra text

[Lars Ingebrigtsen]

Drew Adams <drew.adams@oracle.com> writes:

> The Subject line is not very clear.  Here is a recipe.  You decide
> whether the behavior is correct or a bug.  FWIW, this behavior is at
> least as old as Emacs 20 (with transient-mark-mode on).
>
> emacs -Q
>
> In buffer *scratch*, put point before "that" on the last line.
> `M-c', to capitalize "That".
> `C-SPC C-e', to select the text after "That", up to eol.
> (The region does not contain the word "That".)
>
> `C-_' to undo the last change within the region.
> The word "that" is inserted, giving this:
>
> ;; then enter the text in Thatthat file=A1=AFs own buffer.

This bug is still present in Emacs 28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#21523: 25.0.50; Undo with active region adds extra text

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 16 Aug 2021 14:39:15 +0200
> Cc: 21523@debbugs.gnu.org
> 
> Drew Adams <drew.adams@oracle.com> writes:
> 
> > The Subject line is not very clear.  Here is a recipe.  You decide
> > whether the behavior is correct or a bug.  FWIW, this behavior is at
> > least as old as Emacs 20 (with transient-mark-mode on).
> >
> > emacs -Q
> >
> > In buffer *scratch*, put point before "that" on the last line.
> > `M-c', to capitalize "That".
> > `C-SPC C-e', to select the text after "That", up to eol.
> > (The region does not contain the word "That".)
> >
> > `C-_' to undo the last change within the region.
> > The word "that" is inserted, giving this:
> >
> > ;; then enter the text in Thatthat file=A1=AFs own buffer.
> 
> This bug is still present in Emacs 28.

Why is it a bug?  What is the expected "non-buggy" behavior in this
case?

bug#21523: 25.0.50; Undo with active region adds extra text

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> This bug is still present in Emacs 28.
>
> Why is it a bug?  What is the expected "non-buggy" behavior in this
> case?

I'd expect it to either do nothing (since we're undoing in a region, and
the changed text is outside the region), or change the "This" to
"this".  Leaving "Thisthis" in the buffer has to be a bug no matter how
you look at it.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#21523: 25.0.50; Undo with active region adds extra text

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: drew.adams@oracle.com,  21523@debbugs.gnu.org
> Date: Mon, 16 Aug 2021 15:53:55 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> This bug is still present in Emacs 28.
> >
> > Why is it a bug?  What is the expected "non-buggy" behavior in this
> > case?
> 
> I'd expect it to either do nothing (since we're undoing in a region, and
> the changed text is outside the region), or change the "This" to
> "this".  Leaving "Thisthis" in the buffer has to be a bug no matter how
> you look at it.

Is signaling an error okay?  Because then it looks like an off-by-one
error somewhere -- the following slightly modified recipe works as
expected:

  emacs -Q
  C-u 23 M-g c ; go to "text"
  M-c
  C-f          ; the crucial difference!
  C-SPC
  C-e
  C-/
    => user-error: No further undo information for region

So the problem seems to be that in the original recipe the region
starts immediately after the end of the modified portion of text.

bug#21523: 25.0.50; Undo with active region adds extra text

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Is signaling an error okay?

Yup.

> Because then it looks like an off-by-one
> error somewhere -- the following slightly modified recipe works as
> expected:
>
>   emacs -Q
>   C-u 23 M-g c ; go to "text"
>   M-c
>   C-f          ; the crucial difference!
>   C-SPC
>   C-e
>   C-/
>     => user-error: No further undo information for region
>
> So the problem seems to be that in the original recipe the region
> starts immediately after the end of the modified portion of text.

Yup; I'm seeing the same thing, so this does indeed look like a
off-by-one error.  And...  it's in `undo-elt-in-region', I think, called
by (basically) (undo-make-selective-list (mark) (point)).

There's basically no test cases for this function, I think?

Actually, the problem seems to be in undo-adjust-elt, which was
rewritten in 2014 to fix bug#17235.

I've now added a test case (commented out), but I don't quite understand
the logic in undo-adjust-elt...  anybody see something obviously wrong?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#21523: 25.0.50; Undo with active region adds extra text

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Actually, the problem seems to be in undo-adjust-elt, which was
> rewritten in 2014 to fix bug#17235.
>
> I've now added a test case (commented out), but I don't quite understand
> the logic in undo-adjust-elt...  anybody see something obviously wrong?

(defun undo-adjust-elt (elt deltas)

[...]

    ;; (TEXT . POSITION)
    (`(,(and text (pred stringp)) . ,(and pos (pred integerp)))
     (cons text (* (if (< pos 0) -1 1)
                   (undo-adjust-pos (abs pos) deltas))))

The problem seems to be here.  In my test case, this make the ("This"
. 1) entry into a ("This" . 5) entry, which is then included in the
region.  Using < instead if <= works for this particular test case, but
not for undo-test-region-eob.

I've added Barry to the CCs; perhaps he has some insights here.

For reference, this is the test case:

(ert-deftest test-undo-region ()
  (with-temp-buffer
    (insert "This is a test\n")
    (goto-char (point-min))
    (setq buffer-undo-list nil)
    (downcase-word 1)
    (should (= (length (delq nil (undo-make-selective-list 1 9))) 2))
    ;; FIXME: These should give 0, but currently give 1.
    ;;(should (= (length (delq nil (undo-make-selective-list 4 9))) 0))
    ;;(should (= (length (delq nil (undo-make-selective-list 5 9))) 0))
    (should (= (length (delq nil (undo-make-selective-list 6 9))) 0))))


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no