Regression in bracketed-paste-magic (not in 5.2, but affects 5.3's test releases)

Regression in bracketed-paste-magic (not in 5.2, but affects 5.3's test releases)

[Zhiming Wang]

Commit 20948d0 (38579: Functions/Zle/bracketed-paste-magic: simplify saving and
restoring of state) introduced a regression that results in the ZLE buffer
being repeated when pasting under certain conditions. It was introduced after
5.2 and currently affects 5.2-test-1, 5.2-test-2 and master.

Here's a simple reproducer:

1. Set up environment (.zshrc and history) and open a new session with that
   environment:

       cd "$(mktemp -d /tmp/XXXXXXX)"
       cat >.zshrc <<'EOF'
       HISTFILE=$HOME/history
       autoload -Uz bracketed-paste-magic
       zle -N bracketed-paste bracketed-paste-magic
       EOF
       cat >history <<'EOF'
       echo 1
       echo 2
       EOF
       HOME=$PWD zsh

2. Press the following sequence of keys (no space): ^R echo ^R ^E
   (history-incremental-search-backward, "echo",
   history-incremental-search-backward, end-of-line), then paste something,
   say, "zsh".

At this point, we should expect

    echo 1zsh

in the ZLE buffer. Instead, we see

    echo 1zshecho 1

note the buffer prior to pasting being repeated at the end.

Here's a terminal recording that demonstrates the wrong behavior in action:
https://asciinema.org/a/95254.

Best,
Zhiming

Re: Regression in bracketed-paste-magic (not in 5.2, but affects 5.3's test releases)

[Bart Schaefer]

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

On Dec 6, 2016 6:58 PM, "Zhiming Wang" <zmwangx@gmail.com> wrote:

Commit 20948d0 (38579: Functions/Zle/bracketed-paste-magic: simplify saving
and
restoring of state) introduced a regression that results in the ZLE buffer
being repeated when pasting under certain conditions.

[...]

Here's a simple reproducer:

2. Press the following sequence of keys (no space): ^R echo ^R ^E
   (history-incremental-search-backward, "echo",
   history-incremental-search-backward, end-of-line), then paste something,
   say, "zsh".


This probably indicates a problem with undo-point handling around history
search, which has been a sticky wicket in the past.  The patch you cite
replaced b-p-m's internal handling of $BUFFER with reliance on the undo
mechanism.

 For the 5.3 release the easiest approach is likely to be to back out that
change and re-apply it after the release when it can be more thoroughly
debugged.

Re: Regression in bracketed-paste-magic (not in 5.2, but affects 5.3's test releases)

[Zhiming Wang]

On Dec 6, 2016, at 10:25 PM, Bart Schaefer <schaefer@brasslantern.com> wrote:
> For the 5.3 release the easiest approach is likely to be to back out that
> change and re-apply it after the release when it can be more thoroughly
> debugged.

Sounds good. I have verified that a revert restores sanity, for now.

From c1e93b892d9e444d751a073baa329c25cd996471 Mon Sep 17 00:00:00 2001
From: Zhiming Wang <zmwangx@gmail.com>
Date: Tue, 6 Dec 2016 23:01:33 -0500
Subject: [PATCH] Revert "38579: simplify saving and restoring of state"

This reverts commit 20948d088994dc7b26a26b94926432985fa6863e.
---
 Functions/Zle/bracketed-paste-magic | 44 +++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/Functions/Zle/bracketed-paste-magic b/Functions/Zle/bracketed-paste-magic
index fb584d595..c46f741d5 100644
--- a/Functions/Zle/bracketed-paste-magic
+++ b/Functions/Zle/bracketed-paste-magic
@@ -145,26 +145,27 @@ 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
-	# 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
+        # There are active widgets.  Reprocess $PASTED as keystrokes.
+	NUMERIC=1
+	zle -U - $PASTED
+
 	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}
@@ -182,16 +183,21 @@ bracketed-paste-magic() {
 	done
 	PASTED=$BUFFER
 
-	# Restore state
-	zle -K $bpm_keymap
-	fc -P
-	MARK=$bpm_mark
-	REGION_ACTIVE=$bpm_region
-	NUMERIC=$bpm_numeric
+	# Reset the undo state
 	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
-- 
2.11.0

Re: Regression in bracketed-paste-magic (not in 5.2, but affects 5.3's test releases)

[Oliver Kiddle]

Bart wrote:
> This probably indicates a problem with undo-point handling around history
> search, which has been a sticky wicket in the past.  The patch you cite
> replaced b-p-m's internal handling of $BUFFER with reliance on the undo
> mechanism.

In this particular case, the problem is that fc -p doesn't result
in a call to zle's remember_edits(). remember_edits() updates the
table of history entries for the current line. The edit associated
with doing BUFFER= CURSOR=1 is thus lost. So when it does the
undo, the line is first restored when the history line is reselected
and then the contents are duplicated when the effect of clearing
$BUFFER is undone.

Unfortunately, the ZLE_CMD_SET_HIST_LINE callback happens after this
history line is updated so we can't call remember_edits() from there.
We could simply add another similar hook and call it earlier.

What I would suggest for 5.3 is the patch below. fc -p doesn't
trigger handleundo() either so by moving split-undo above the
clearing of the buffer, we ensure that the edit is missed by both
the undo and history. Referencing $UNDO_CHANGE_NO does force
handleundo() so the downside of this is that UNDO_LIMIT_NO will
allow an undo back to the state before clearing $BUFFER.

If you're wondering why I suggest this rather than adding a hook
to call remember_edits(), it is because, for a proper post-5.3
patch, fc -p wouldn't actually change the current history line. The
only thing that changes is the history number. If history internals
use the Histent to identify history entries then undo wouldn't see
fc -p/-P as a change of history line and we could kill the associated
bugs. I've got some unfinished work on this on a git branch if
someone's interested to play with it. But for now, calling fc -p
in a widget is going to get you undo oddities.

I don't use bracketed-paste-magic so would appreciate if someone
else could do some testing and make the call on what should go into
5.3.

Oliver

diff --git a/Functions/Zle/bracketed-paste-magic b/Functions/Zle/bracketed-paste-magic
index fb584d5..c3cc8b6 100644
--- a/Functions/Zle/bracketed-paste-magic
+++ b/Functions/Zle/bracketed-paste-magic
@@ -151,10 +151,10 @@ bracketed-paste-magic() {
 	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
+	BUFFER=
+	CURSOR=1
 	fc -p -a /dev/null 0 0
 	if [[ $bmp_keymap = vicmd ]]; then
 	    zle -K viins

Re: Regression in bracketed-paste-magic (not in 5.2, but affects 5.3's test releases)

[Zhiming Wang]

On Dec 7, 2016, at 7:39 AM, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> I don't use bracketed-paste-magic so would appreciate if someone
> else could do some testing ...
> 
> diff --git a/Functions/Zle/bracketed-paste-magic b/Functions/Zle/bracketed-paste-magic
> index fb584d5..c3cc8b6 100644
> --- a/Functions/Zle/bracketed-paste-magic
> +++ b/Functions/Zle/bracketed-paste-magic
> @@ -151,10 +151,10 @@ bracketed-paste-magic() {
> 	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
> +	BUFFER=
> +	CURSOR=1
> 	fc -p -a /dev/null 0 0
> 	if [[ $bmp_keymap = vicmd ]]; then
> 	    zle -K viins

I applied the patch and played around for a while; the issue appears to be
fixed.

Re: Regression in bracketed-paste-magic (not in 5.2, but affects 5.3's test releases)

[Bart Schaefer]

On Dec 7,  1:39pm, Oliver Kiddle wrote:
}
} In this particular case, the problem is that fc -p doesn't result
} in a call to zle's remember_edits(). remember_edits() updates the
} table of history entries for the current line. The edit associated
} with doing BUFFER= CURSOR=1 is thus lost. So when it does the
} undo, the line is first restored when the history line is reselected
} and then the contents are duplicated when the effect of clearing
} $BUFFER is undone.

This reminds me of a question I had when 38579 was applied:  What's
the point of assigning CURSOR=1 here?  If BUFFER is empty, the only
valid value of CURSOR is 0 (isn't it?), and the assignment to BUFFER
resets CURSOR implicitly as far as I can tell.

} What I would suggest for 5.3 is the patch below. [...]
} 
} If you're wondering why I suggest this rather than adding a hook
} to call remember_edits(), it is because, for a proper post-5.3
} patch, fc -p wouldn't actually change the current history line.

Now that 5.3.1 is out, I've re-commited bracketed-paste-magic to its
state after 38579 plus the patch in this thread (40118), in spite of
my question above.  We can now begin fiddling with the proper post-5.3
fix to "fc -p".