Bug in undo/redo handling in conjunction with history-beginning-search-backward

Bug in undo/redo handling in conjunction with history-beginning-search-backward

[Mikael Magnusson]

zsh -f
% bindkey -e
% bindkey '^U' undo
% bindkey '^Y' redo
% bindkey '^[C' history-beginning-search-backward
at this point, type in "bin" and invoke the
history-beginning-search-backward widget, type a space, then undo and
redo fully a few times, you'll notice the point where the extra "n"
starts popping into the command line.

I think Bart was the last one to poke the undo code, or maybe Oliver?
:) I haven't bisected it (I will if anyone asks me to), but it doesn't
happen in 5.0.7 and it does happen in zsh-5.1-21-ga4f087b (which is
just the other old binary i had lying around).

-- 
Mikael Magnusson

Re: Bug in undo/redo handling in conjunction with history-beginning-search-backward

[Oliver Kiddle]

On 20 May, Mikael Magnusson wrote:
> % bindkey -e
> % bindkey '^U' undo
> % bindkey '^Y' redo
> % bindkey '^[C' history-beginning-search-backward
> at this point, type in "bin" and invoke the
> history-beginning-search-backward widget, type a space, then undo and
> redo fully a few times, you'll notice the point where the extra "n"
> starts popping into the command line.

On first attempt, I couldn't reproduce this. I do get the effect with
repeated undos and no redos, however.

The way history line changes are stored in the undo code differs
markedly from other changes. So the code has ended up with special logic
to handle the history changes.

36131 was when I last touched this and I wasn't really happy with the
state of things then but that was just before a two week holiday. At the
time, I said:
  It might be better to create full undo entries for history changes
  though that would need hacks to ensure that multiple history changes
  are undone in a single step.

If this patch doesn't do the job then that might be a better approach
than playing whack-a-mole with further issues.

Oliver

diff --git a/Src/Zle/zle_utils.c b/Src/Zle/zle_utils.c
index 68794c6..80219a4 100644
--- a/Src/Zle/zle_utils.c
+++ b/Src/Zle/zle_utils.c
@@ -1589,9 +1589,14 @@ undo(char **args)
 	    break;
 	if (prev->changeno <= undo_limitno && !*args)
 	    return 1;
-	if (!unapplychange(prev) && last_change >= 0)
-	    unapplychange(prev);
-	curchange = prev;
+	if (!unapplychange(prev)) {
+	    if (last_change >= 0) {
+		unapplychange(prev);
+		curchange = prev;
+	    }
+	} else {
+	    curchange = prev;
+	}
     } while (last_change >= (zlong)0 || (curchange->flags & CH_PREV));
     setlastline();
     return 0;

Re: Bug in undo/redo handling in conjunction with history-beginning-search-backward

[Bart Schaefer]

On May 25,  3:02am, Oliver Kiddle wrote:
} Subject: Re: Bug in undo/redo handling in conjunction with history-beginni
}
} On first attempt, I couldn't reproduce this. I do get the effect with
} repeated undos and no redos, however.
} 
} The way history line changes are stored in the undo code differs
} markedly from other changes. So the code has ended up with special logic
} to handle the history changes.

I don't think this is directly related to history changes, I can cause a
crash on multiple undo with bracketed-paste-magic having not changed the
history line at all.  See other thread.

Re: Bug in undo/redo handling in conjunction with history-beginning-search-backward

[Oliver Kiddle]

Bart wrote:
> I don't think this is directly related to history changes, I can cause a
> crash on multiple undo with bracketed-paste-magic having not changed the
> history line at all.  See other thread.

Actually it looks like history lines are involved.
bracketed-paste-magic does fc -p which invalidates HISTNO.
After bracketed-paste-magic, dumping the undo structure using a gdb
macro shows the following (| denotes merged changes):

No.	Offset	Delete          Insert         Cursor Hist
 8  	0	                echo https://bu 0 61   803
 6  	0	echo                            5  0     0
 5 |	4	                                4  5   804
 4 |	3	                o               3  4   804
 3 |	2	                h               2  3   804
 2 |	1	                c               1  2   804
 1 |	0	                e               0  1   804

So one undo blanks the whole line; a second undo tries to move to history line
0 - which fails.

fc -p/-P are not part of zle so don't save histline. This also explains
why change 8 has a history line of 803 instead of 804. Use of fc -p
within zle appears to be unique to bracketed-paste-magic. What is the
reason for using it? Are you trying to block history widgets even where
they are listed in active-widgets?

So does anyone have any thoughts on how to handle this. The simplest is
for fc -p/-P to detect zle being active and print an error. Perhaps we
need some module callbacks for pushing and popping history. Zle would
hook the callback, save histline and somehow mark this in the undo
structure.

I attach a patch to the state saving and restoring in
bracketed-paste-magic. Don't expect this to fix anything. I did it to
make things easier to understand. Was there a reason for half the state
saving/restoring to be outside the if statement? Saving BUFFER and
CURSOR is fairly pointless if undo is going to restore it. It'd be good
if this could be tested by someone who does use bracketed-paste-magic,
however.

Oliver

diff --git a/Functions/Zle/bracketed-paste-magic b/Functions/Zle/bracketed-paste-magic
index cafe18e..00cdb92 100644
--- a/Functions/Zle/bracketed-paste-magic
+++ b/Functions/Zle/bracketed-paste-magic
@@ -145,27 +145,26 @@ bracketed-paste-magic() {
 	done
     fi
 
-    # Save context, create a clean slate for the paste
-    integer bpm_mark=$MARK bpm_cursor=$CURSOR bpm_region=$REGION_ACTIVE
-    integer bpm_numeric=${NUMERIC:-1}
-    local bpm_buffer=$BUFFER
-    fc -p -a /dev/null 0 0
-    BUFFER=
-
     zstyle -a :bracketed-paste-magic inactive-keys bpm_inactive
     if zstyle -s :bracketed-paste-magic active-widgets bpm_active '|'; then
-        # There are active widgets.  Reprocess $PASTED as keystrokes.
-	NUMERIC=1
-	zle -U - $PASTED
-
+	# Save context, create a clean slate for the paste
+	integer bpm_mark=$MARK bpm_region=$REGION_ACTIVE
+	integer bpm_numeric=${NUMERIC:-1}
+	integer bpm_limit=$UNDO_LIMIT_NO bpm_undo=$UNDO_CHANGE_NO
+	BUFFER=
+	CURSOR=1
+	zle .split-undo
+	UNDO_LIMIT_NO=$UNDO_CHANGE_NO
+	fc -p -a /dev/null 0 0
 	if [[ $bmp_keymap = vicmd ]]; then
 	    zle -K viins
 	fi
 
+	# There are active widgets.  Reprocess $PASTED as keystrokes.
+	NUMERIC=1
+	zle -U - $PASTED
+
 	# Just in case there are active undo widgets
-	zle .split-undo
-	integer bpm_limit=$UNDO_LIMIT_NO bpm_undo=$UNDO_CHANGE_NO
-	UNDO_LIMIT_NO=$UNDO_CHANGE_NO
 
 	while [[ -n $PASTED ]] && zle .read-command; do
 	    PASTED=${PASTED#$KEYS}
@@ -183,21 +182,16 @@ bracketed-paste-magic() {
 	done
 	PASTED=$BUFFER
 
-	# Reset the undo state
+	# Restore state
+	zle -K $bpm_keymap
+	fc -P
+	MARK=$bpm_mark
+	REGION_ACTIVE=$bpm_region
+	NUMERIC=$bpm_numeric
 	zle .undo $bpm_undo
 	UNDO_LIMIT_NO=$bpm_limit
-
-	zle -K $bpm_keymap
     fi
 
-    # Restore state
-    BUFFER=$bpm_buffer
-    MARK=$bpm_mark
-    CURSOR=$bpm_cursor
-    REGION_ACTIVE=$bpm_region
-    NUMERIC=$bpm_numeric
-    fc -P
-
     # PASTED has been updated, run the paste-finish functions
     if zstyle -a :bracketed-paste-magic paste-finish bpm_hooks; then
 	for bpm_func in $bpm_hooks; do

Re: Bug in undo/redo handling in conjunction with history-beginning-search-backward

[Bart Schaefer]

On Jun 2,  5:12pm, Oliver Kiddle wrote:
}
} bracketed-paste-magic does fc -p which invalidates HISTNO.
} 
} So one undo blanks the whole line; a second undo tries to move to
} history line 0 - which fails.

Oof.

} fc -p/-P are not part of zle so don't save histline. This also explains
} why change 8 has a history line of 803 instead of 804. Use of fc -p
} within zle appears to be unique to bracketed-paste-magic.

Functions/Misc/sticky-note also uses it within ZLE, and has for years.

} What is the
} reason for using it? Are you trying to block history widgets even where
} they are listed in active-widgets?

That's the reason in bracketed-paste-magic; in sticky-note it differs.
History motions should be able to move around within the pasted text
(up-line-or-history) but not out of the paste into the actual history.

In sticky-note, an alternate history is substituted and the intention
is to let you move around and interact within it.

} So does anyone have any thoughts on how to handle this. The simplest is
} for fc -p/-P to detect zle being active and print an error.

That would be unfortunate.

} Perhaps we need some module callbacks for pushing and popping history.
} Zle would hook the callback, save histline and somehow mark this in
} the undo structure.

Either that, or perhaps there's some way for undo to detect that the
history has changed since the last time an undo point was recorded?
I'm not familiar with how the history push/pop works internally.

} I attach a patch to the state saving and restoring in
} bracketed-paste-magic. Don't expect this to fix anything. I did it to
} make things easier to understand. Was there a reason for half the state
} saving/restoring to be outside the if statement?

Basically (a) I first implemented it without using split-undo and (b) I
didn't trust undo to save and restore everything correctly for the context
needed by the paste-finish functions.