bug in replace-string: widget loses characters

bug in replace-string: widget loses characters

[Moritz Bunkus]

Hey,

zsh 5.0.0 on Arch Linux here. I'm using the replace-string widget and
have the problem that often it loses characters from the current input
when I use it. Not always, but often enough.

In order to demonstrate what I mean, do the following:

* Start zsh with --no-rcs:

sweet-chili% zsh --no-rcs
sweet-chili% zsh --version
zsh 5.0.0 (x86_64-unknown-linux-gnu)

* load the widget and bind it to a key, e.g. Alt-R:

sweet-chili% autoload replace-string
sweet-chili% zle -N replace-regex replace-string
sweet-chili% bindkey '^[r' replace-regex

* Next enter some text with several arguments, don't hit enter. In
this example I've used the following:

sweet-chili% somecmd 64/ubuntu/precise 64/ubuntu/quantal argab

* Hit Alt-R. Replace 64 with 32, and the result will be:

sweet-chili% somecmd 32/ubuntu/precise 32/ubuntu/quantal arga

Note that the "b" is missing at the end. Undo will not restore that
character, but it will turn the "32"s into "64"s again.

* It gets even weirder. Now hit "enter". The command will not be
found, obviously. Go one up in the history displaying the command
again and prefix it with "echo ". The result should look like this:

sweet-chili% echo somecmd 32/ubuntu/precise 32/ubuntu/quantal arga

* Hit Alt-R and replace 32 with 64 again. This time the space between
'echo' and 'somecmd' will be removed resulting in:

sweet-chili% echosomecmd 64/ubuntu/precise 64/ubuntu/quantal arga

Unlike after the first replacement the cursor will be after the "echo"
and not at the end of the line.

This unpredictable behavior makes using the widgets pretty pointless
(or at least highly frustrating), and I do think it qualifies to be
called a bug, doesn't it? ;)

Kind regards,
mosu

Re: bug in replace-string: widget loses characters

[Peter Stephenson]

On 8 October 2012 19:19, Moritz Bunkus <moritz@bunkus.org> wrote:
> zsh 5.0.0 on Arch Linux here. I'm using the replace-string widget and
> have the problem that often it loses characters from the current input
> when I use it. Not always, but often enough.
>
> In order to demonstrate what I mean, do the following:
>
> * Start zsh with --no-rcs:
>
> sweet-chili% zsh --no-rcs
> sweet-chili% zsh --version
> zsh 5.0.0 (x86_64-unknown-linux-gnu)
>
> * load the widget and bind it to a key, e.g. Alt-R:
>
> sweet-chili% autoload replace-string
> sweet-chili% zle -N replace-regex replace-string
> sweet-chili% bindkey '^[r' replace-regex
>
> * Next enter some text with several arguments, don't hit enter. In
> this example I've used the following:
>
> sweet-chili% somecmd 64/ubuntu/precise 64/ubuntu/quantal argab
>
> * Hit Alt-R. Replace 64 with 32, and the result will be:
>
> sweet-chili% somecmd 32/ubuntu/precise 32/ubuntu/quantal arga

Good news! Your problem is now officially classified as "weird".
(To be honest, "weird and interesting" is the one to go for,
but we're half way there.)

I think it's a problem with "undo".  After the function has read the
original and replacement strings from the area under the command line,
it's supposed to undo all that, so that you don't see it as part of the
normal undo history (undo should take you back through the replacement,
then immediately back through any changes you made before invoking the
replace widget).  It looks like that undo can go one change too far,
which means I've got the counting of undo changes wrong somewhere.

I didn't really properly get to grips with the undo system,
so this isn't that surprising. I suppose it's no use hoping
anybody else is going to understand it.  I'll take a look
later.

Anyway, it's not a problem with the function itself; you could work
around it by removing the "zle undo" line, which will expose the
previous unhelpful undo behaviour of the replacement widgets but
should mean the replacement itself works OK.

pws

Re: bug in replace-string: widget loses characters

[Moritz Bunkus]

Hey,

your explanation matches exactly what I'm seeing. I wrote:

>> zsh 5.0.0 on Arch Linux here. I'm using the replace-string widget and
>> have the problem that often it loses characters from the current input
>> when I use it. Not always, but often enough.

It even accounts for the "not always" part: I often type a trailing
space at the end of a line (mostly automatically because I'm not
really thinking about whether or not there will be an additional
argument). If that was the last action before invoking replace-string
the space will get removed, but I actually don't realize it has been.
Hence me thinking that replace-string would only lose characters
occasionally instead of all the time.

I'll deactivate the undo part for that widget as you've suggested.

Thanks for looking into it.

Kind regards,
mosu

Re: bug in replace-string: widget loses characters

[Peter Stephenson]

On 9 October 2012 12:22, Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> I think it's a problem with "undo".  After the function has read the
> original and replacement strings from the area under the command line,
> it's supposed to undo all that, so that you don't see it as part of the
> normal undo history (undo should take you back through the replacement,
> then immediately back through any changes you made before invoking the
> replace widget).  It looks like that undo can go one change too far,
> which means I've got the counting of undo changes wrong somewhere.
>
> I didn't really properly get to grips with the undo system,
> so this isn't that surprising. I suppose it's no use hoping
> anybody else is going to understand it.  I'll take a look
> later.

I think this fix should be robust.  It increments the undo change number
at the point where we pass it as the value of the variable.  This looks
like a cop out, but it does absolutely guarantee the separation of
changes before and after that point regardless of where the wires were
attached to the fence posts.  The zlong range should be big enough to
cope.

The one side effect is that $UNDO_CHANGE_NO is different each time it's
read.  I could fix that if anyone cared, but if it helped you could sort
of look on it as a feature: you are guaranteed that all recorded change
points are unique and monotonically increasing whether or not the line
itself was changed.

Index: Src/Zle/zle_utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/zle_utils.c,v
retrieving revision 1.63
diff -p -u -r1.63 zle_utils.c
--- Src/Zle/zle_utils.c	29 Mar 2012 20:31:33 -0000	1.63
+++ Src/Zle/zle_utils.c	9 Oct 2012 14:24:35 -0000
@@ -1520,23 +1520,25 @@ setlastline(void)
 int
 undo(char **args)
 {
-    zlong last_change = (zlong)0;
+    zlong last_change;

     if (*args)
-    {
 	last_change = zstrtol(*args, NULL, 0);
-    }
+    else
+	last_change = (zlong)-1;

     handleundo();
     do {
-	if(!curchange->prev)
+	struct change *prev = curchange->prev;
+	if(!prev)
 	    return 1;
-	if (unapplychange(curchange->prev))
-	    curchange = curchange->prev;
+	if (prev->changeno < last_change)
+	    break;
+	if (unapplychange(prev))
+	    curchange = prev;
 	else
 	    break;
-    } while (*args ? curchange->changeno != last_change :
-	     (curchange->flags & CH_PREV));
+    } while (last_change >= (zlong)0 || (curchange->flags & CH_PREV));
     setlastline();
     return 0;
 }
@@ -1660,6 +1662,11 @@ zlecallhook(char *name, char *arg)
 zlong
 get_undo_current_change(UNUSED(Param pm))
 {
-    return undo_changeno;
+    /*
+     * Increment the number in case a change is in progress;
+     * we don't want to back off what's already been done when
+     * we return to this change number.  This eliminates any
+     * problem about the point where a change is numbered.
+     */
+    return ++undo_changeno;
 }
-

pws

Re: bug in replace-string: widget loses characters

[Bart Schaefer]

On Oct 9,  3:34pm, Peter Stephenson wrote:
}
} The one side effect is that $UNDO_CHANGE_NO is different each time it's
} read.  I could fix that if anyone cared, but if it helped you could sort
} of look on it as a feature: you are guaranteed that all recorded change
} points are unique and monotonically increasing whether or not the line
} itself was changed.

I think this is OK, but I suppose there might be a case where one wanted
to be able to detect whether any changes had occurred between state X
and state Y, e.g. previously

	local current_undo=$UNDO_CHANGE_NO
	# ... some stuff potentially happens ...
	if (( current_undo == UNDO_CHANGE_NO ))
	then
	  # nothing happened?
	fi

Is there another way to detect this?  It's probably not a common thing
to care about so perhaps not worth a lot of effort.

Re: bug in replace-string: widget loses characters

[Peter Stephenson]

On Tue, 09 Oct 2012 08:24:25 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Oct 9,  3:34pm, Peter Stephenson wrote:
> }
> } The one side effect is that $UNDO_CHANGE_NO is different each time
> it's } read.  I could fix that if anyone cared, but if it helped you
> could sort } of look on it as a feature: you are guaranteed that all
> recorded change } points are unique and monotonically increasing
> whether or not the line } itself was changed.
> 
> I think this is OK, but I suppose there might be a case where one
> wanted to be able to detect whether any changes had occurred between
> state X and state Y, e.g. previously
> 
> 	local current_undo=$UNDO_CHANGE_NO
> 	# ... some stuff potentially happens ...
> 	if (( current_undo == UNDO_CHANGE_NO ))
> 	then
> 	  # nothing happened?
> 	fi
> 
> Is there another way to detect this?  It's probably not a common thing
> to care about so perhaps not worth a lot of effort.

Well, with the current set-up you'd be guaranteed that the second call
would be an increment of 1 from the first.  You need to be careful how
many times you call UNDO_CHANGE_NO, though, so it's not very robust.

If it seems a preferable way of doing it, it's straightforward to add a
flag saying who last incremented the number, so it only gets incremented
the first time.  The change numbers for the edits are hidden, so that
ought to do the trick.

-- 
Peter Stephenson <p.stephenson@samsung.com>       Consultant, Software
Tel: +44 (0)1223 434724              Samsung Cambridge Solution Centre
St John's House, St John's Innovation Park,
Cowley Road, Cambridge, CB4 0ZT, UK

Re: bug in replace-string: widget loses characters

[Peter Stephenson]

On 9 October 2012 16:44, Peter Stephenson <p.stephenson@samsung.com> wrote:
> If it seems a preferable way of doing it, it's straightforward to add a
> flag saying who last incremented the number, so it only gets incremented
> the first time.  The change numbers for the edits are hidden, so that
> ought to do the trick.

Might as well avoid making unnecessary changes, I suppose.

Index: Src/Zle/zle_utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/zle_utils.c,v
retrieving revision 1.64
diff -p -u -r1.64 zle_utils.c
--- Src/Zle/zle_utils.c	9 Oct 2012 14:57:16 -0000	1.64
+++ Src/Zle/zle_utils.c	9 Oct 2012 16:09:22 -0000
@@ -1363,6 +1363,10 @@ static struct change *nextchanges, *endn

 static zlong undo_changeno;

+/* If non-zero, the last increment to undo_changeno was for the variable */
+
+static int undo_set_by_variable;
+
 /**/
 void
 initundo(void)
@@ -1373,6 +1377,7 @@ initundo(void)
     curchange->del = curchange->ins = NULL;
     curchange->dell = curchange->insl = 0;
     curchange->changeno = undo_changeno = 0;
+    undo_set_by_variable = 0;
     lastline = zalloc((lastlinesz = linesz) * ZLE_CHAR_SIZE);
     ZS_memcpy(lastline, zleline, (lastll = zlell));
     lastcs = zlecs;
@@ -1498,6 +1503,7 @@ mkundoent(void)
 	ch->prev = NULL;
     }
     ch->changeno = ++undo_changeno;
+    undo_set_by_variable = 0;
     endnextchanges = ch;
 }

@@ -1662,6 +1668,11 @@ zlecallhook(char *name, char *arg)
 zlong
 get_undo_current_change(UNUSED(Param pm))
 {
+    if (undo_set_by_variable) {
+	/* We were the last to increment this, doesn't need another one. */
+	return undo_changeno;
+    }
+    undo_set_by_variable = 1;
     /*
      * Increment the number in case a change is in progress;
      * we don't want to back off what's already been done when

pws