* Re: Bug in ZSH's vi emulation
2016-11-02 16:51 ` Oliver Kiddle
@ 2016-11-02 17:34 ` Peter Stephenson
2016-11-02 18:46 ` Bart Schaefer
` (3 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Peter Stephenson @ 2016-11-02 17:34 UTC (permalink / raw)
To: zsh-workers
On Wed, 02 Nov 2016 17:51:23 +0100
Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> We've also got a separate issue of only lastchar being stuffed into
> vichgbuf so repeating, e.g. gU doesn't work. Why is keybuflen only
> 1 in startvichange? That, along with what the general point of lastchar
> is, has me fairly puzzled.
The thing that gets stuffed into vichgbuf[0] in startvichange() is the
last byte of the binding that caused the change to start, not the range
of the change. So if you issue "c" then the "c" goes into the buffer
here and the target gets handled later. I haven't traced it further
than that, but this would only screw up with atypical bindings for
vi-change, vi-delete, etc. Presumably extracting the full set of bytes
for the command would work.
This wouldn't work with a multibyte binding, for example, e.g. if you
had ¢ on your keyboard and thought it would be a great alternative to c
then only the last byte of the character would be stored. At some
point I think it got fixed up so the keybuf is correct for cases
like this.
pws
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Bug in ZSH's vi emulation
2016-11-02 16:51 ` Oliver Kiddle
2016-11-02 17:34 ` Peter Stephenson
@ 2016-11-02 18:46 ` Bart Schaefer
2016-11-02 23:19 ` Bart Schaefer
` (2 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Bart Schaefer @ 2016-11-02 18:46 UTC (permalink / raw)
To: zsh-workers
On Nov 2, 5:51pm, Oliver Kiddle wrote:
} Subject: Re: Bug in ZSH's vi emulation
}
} Daniel Shahaf wrote:
} > Is it simply a matter of setting vichgbufptr to 0 if getvirange()
} > returns -1?
}
} Not really. If we want a quick fix for 5.3 then either that or checking
} virangeflag at the top of virepeatchange will do the job.
It'd be nice to quash the infinite loop, even if the result isn't
entirely identical to vi behavior.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Bug in ZSH's vi emulation
2016-11-02 16:51 ` Oliver Kiddle
2016-11-02 17:34 ` Peter Stephenson
2016-11-02 18:46 ` Bart Schaefer
@ 2016-11-02 23:19 ` Bart Schaefer
2016-11-03 1:54 ` Daniel Shahaf
2016-11-03 4:12 ` Bart Schaefer
2016-11-03 4:44 ` Bart Schaefer
4 siblings, 1 reply; 16+ messages in thread
From: Bart Schaefer @ 2016-11-02 23:19 UTC (permalink / raw)
To: zsh-workers
On Nov 2, 5:51pm, Oliver Kiddle wrote:
}
} checking virangeflag at the top of virepeatchange will do the job.
Regardless of what else eventually happens, this seems like a wise idea.
diff --git a/Src/Zle/zle_vi.c b/Src/Zle/zle_vi.c
index baa2064..1e0402d 100644
--- a/Src/Zle/zle_vi.c
+++ b/Src/Zle/zle_vi.c
@@ -780,7 +780,7 @@ int
virepeatchange(UNUSED(char **args))
{
/* make sure we have a change to repeat */
- if (!vichgbuf || vichgflag)
+ if (!vichgbuf || vichgflag || virangeflag)
return 1;
/* restore or update the saved count and buffer */
if (zmod.flags & MOD_MULT) {
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Bug in ZSH's vi emulation
2016-11-02 23:19 ` Bart Schaefer
@ 2016-11-03 1:54 ` Daniel Shahaf
0 siblings, 0 replies; 16+ messages in thread
From: Daniel Shahaf @ 2016-11-03 1:54 UTC (permalink / raw)
To: zsh-workers
Bart Schaefer wrote on Wed, Nov 02, 2016 at 16:19:26 -0700:
> On Nov 2, 5:51pm, Oliver Kiddle wrote:
> }
> } checking virangeflag at the top of virepeatchange will do the job.
>
> Regardless of what else eventually happens, this seems like a wise idea.
>
In this case, and given Oliver's comments, I'll table the idea from
39806 to reset vichgbufptr: I only intended that as a stopgap to plug
the infinite loop.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Bug in ZSH's vi emulation
2016-11-02 16:51 ` Oliver Kiddle
` (2 preceding siblings ...)
2016-11-02 23:19 ` Bart Schaefer
@ 2016-11-03 4:12 ` Bart Schaefer
2016-11-03 9:36 ` Peter Stephenson
2016-11-03 4:44 ` Bart Schaefer
4 siblings, 1 reply; 16+ messages in thread
From: Bart Schaefer @ 2016-11-03 4:12 UTC (permalink / raw)
To: zsh-workers
On Nov 2, 5:51pm, Oliver Kiddle wrote:
}
} We've also got a separate issue of only lastchar being stuffed into
} vichgbuf so repeating, e.g. gU doesn't work. Why is keybuflen only
} 1 in startvichange? That, along with what the general point of lastchar
} is, has me fairly puzzled.
It's because of getkeymapcmd():
if(lastlen != keybuflen) {
unmetafy(keybuf + lastlen, &keybuflen); <-- here
ungetbytes(keybuf+lastlen, keybuflen);
if(vichgflag)
vichgbufptr -= keybuflen;
keybuf[lastlen] = 0;
}
Seems like it ought to do the following? Or else use a temporary instead
of stomping on keybuflen.
diff --git a/Src/Zle/zle_keymap.c b/Src/Zle/zle_keymap.c
index 3db4207..b5244b5 100644
--- a/Src/Zle/zle_keymap.c
+++ b/Src/Zle/zle_keymap.c
@@ -1622,7 +1622,7 @@ getkeymapcmd(Keymap km, Thingy *funcp, char **strp)
ungetbytes(keybuf+lastlen, keybuflen);
if(vichgflag)
vichgbufptr -= keybuflen;
- keybuf[lastlen] = 0;
+ keybuf[keybuflen = lastlen] = 0;
}
*funcp = func;
*strp = str;
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Bug in ZSH's vi emulation
2016-11-03 4:12 ` Bart Schaefer
@ 2016-11-03 9:36 ` Peter Stephenson
0 siblings, 0 replies; 16+ messages in thread
From: Peter Stephenson @ 2016-11-03 9:36 UTC (permalink / raw)
To: zsh-workers
On Wed, 02 Nov 2016 21:12:43 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Nov 2, 5:51pm, Oliver Kiddle wrote:
> }
> } We've also got a separate issue of only lastchar being stuffed into
> } vichgbuf so repeating, e.g. gU doesn't work. Why is keybuflen only
> } 1 in startvichange? That, along with what the general point of lastchar
> } is, has me fairly puzzled.
>
> It's because of getkeymapcmd():
>
> if(lastlen != keybuflen) {
> unmetafy(keybuf + lastlen, &keybuflen); <-- here
> ungetbytes(keybuf+lastlen, keybuflen);
> if(vichgflag)
> vichgbufptr -= keybuflen;
> keybuf[lastlen] = 0;
> }
>
> Seems like it ought to do the following? Or else use a temporary instead
> of stomping on keybuflen.
>
> diff --git a/Src/Zle/zle_keymap.c b/Src/Zle/zle_keymap.c
> index 3db4207..b5244b5 100644
> --- a/Src/Zle/zle_keymap.c
> +++ b/Src/Zle/zle_keymap.c
> @@ -1622,7 +1622,7 @@ getkeymapcmd(Keymap km, Thingy *funcp, char **strp)
> ungetbytes(keybuf+lastlen, keybuflen);
> if(vichgflag)
> vichgbufptr -= keybuflen;
> - keybuf[lastlen] = 0;
> + keybuf[keybuflen = lastlen] = 0;
> }
> *funcp = func;
> *strp = str;
>
Yes, I stared at this for a while but I think the intention is to take
the (keybuflen-lastlen) null-terminated characters off the end and make
them available for re-reading as input, leaving only the first lastlen
in the key buffer.
That means keybuflen needs updating at the end anyway, to get rid of
those characters from the key buffer, so use of a temporary is
immaterial.
Comments when this was originally written probably wouldn't have hurt.
pws
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Bug in ZSH's vi emulation
2016-11-02 16:51 ` Oliver Kiddle
` (3 preceding siblings ...)
2016-11-03 4:12 ` Bart Schaefer
@ 2016-11-03 4:44 ` Bart Schaefer
2016-11-03 12:43 ` Oliver Kiddle
4 siblings, 1 reply; 16+ messages in thread
From: Bart Schaefer @ 2016-11-03 4:44 UTC (permalink / raw)
To: zsh-workers
On Nov 2, 5:51pm, Oliver Kiddle wrote:
}
} We've also got a separate issue of only lastchar being stuffed into
} vichgbuf so repeating, e.g. gU doesn't work.
So, given 39813, this seems to work:
diff --git a/Src/Zle/zle_vi.c b/Src/Zle/zle_vi.c
index 1e0402d..bd692af 100644
--- a/Src/Zle/zle_vi.c
+++ b/Src/Zle/zle_vi.c
@@ -91,14 +91,15 @@ startvichange(int im)
lastmod = zmod;
if (vichgbuf)
free(vichgbuf);
- vichgbuf = (char *)zalloc(vichgbufsz = 16);
+ vichgbuf = (char *)zalloc(vichgbufsz = 16 + keybuflen);
if (im == -2) {
vichgbuf[0] =
zlell ? (insmode ? (zlecs < zlell ? 'i' : 'a') : 'R') : 'o';
+ vichgbufptr = 1;
} else {
- vichgbuf[0] = lastchar;
+ strcpy(vichgbuf, keybuf);
+ unmetafy(vichgbuf, &vichgbufptr);
}
- vichgbufptr = 1;
vichgrepeat = 0;
}
}
All X02zlevi tests pass, and I'm even able to do e.g. gUfx to uppercase
everything through the next "x".
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Bug in ZSH's vi emulation
2016-11-03 4:44 ` Bart Schaefer
@ 2016-11-03 12:43 ` Oliver Kiddle
2016-11-03 13:08 ` Bart Schaefer
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Oliver Kiddle @ 2016-11-03 12:43 UTC (permalink / raw)
To: zsh-workers
Bart wrote:
> So, given 39813, this seems to work:
That, along with 39813, looks good. As a further tweak, I had to
make keybuflen not be declared static. It might also be good to
tweak the comment leading up to this function (startvichange) to
reference keybuf instead of lastchar.
> All X02zlevi tests pass, and I'm even able to do e.g. gUfx to uppercase
> everything through the next "x".
Works well in my testing too. We should add a new testcase too such
as that below.
Oliver
diff --git a/Test/X02zlevi.ztst b/Test/X02zlevi.ztst
index ced7030..aa3587e 100644
--- a/Test/X02zlevi.ztst
+++ b/Test/X02zlevi.ztst
@@ -244,6 +244,12 @@
>BUFFER: binging
>CURSOR: 3
+# for vi compatibility, this should repeat the previous change
+ zletest $'worm\erdhd..'
+0:use of vi-repeat as the motion and repeat after a failed change
+>BUFFER: word
+>CURSOR: 2
+
zpty_run 'bindkey "^_" undo'
zletest $'undoc\037e'
0:use of undo in vi insert mode
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Bug in ZSH's vi emulation
2016-11-03 12:43 ` Oliver Kiddle
@ 2016-11-03 13:08 ` Bart Schaefer
2016-11-03 14:55 ` Bart Schaefer
2016-11-03 16:06 ` Bart Schaefer
2 siblings, 0 replies; 16+ messages in thread
From: Bart Schaefer @ 2016-11-03 13:08 UTC (permalink / raw)
To: zsh-workers
On Nov 3, 1:43pm, Oliver Kiddle wrote:
}
} That, along with 39813, looks good. As a further tweak, I had to
} make keybuflen not be declared static.
I snuck that into 39813 ...
} It might also be good to
} tweak the comment leading up to this function (startvichange) to
} reference keybuf instead of lastchar.
Indeed.
} Works well in my testing too. We should add a new testcase too such
} as that below.
Shall I commit that or will you?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Bug in ZSH's vi emulation
2016-11-03 12:43 ` Oliver Kiddle
2016-11-03 13:08 ` Bart Schaefer
@ 2016-11-03 14:55 ` Bart Schaefer
2016-11-03 14:57 ` Bart Schaefer
2016-11-03 16:06 ` Bart Schaefer
2 siblings, 1 reply; 16+ messages in thread
From: Bart Schaefer @ 2016-11-03 14:55 UTC (permalink / raw)
To: zsh-workers
On Nov 3, 1:43pm, Oliver Kiddle wrote:
}
} It might also be good to
} tweak the comment leading up to this function (startvichange) to
} reference keybuf instead of lastchar.
When making this change I also noticed that vichgbuf was not being
nul-terminated in the (im == -2) case.
Updated patch (replaces 39814):
diff --git a/Src/Zle/zle_vi.c b/Src/Zle/zle_vi.c
index 1e0402d..95646a9 100644
--- a/Src/Zle/zle_vi.c
+++ b/Src/Zle/zle_vi.c
@@ -71,7 +71,7 @@ static int inrepeat, vichgrepeat;
* im: >= 0: is an insertmode
* -1: skip setting insert mode
* -2: entering viins at start of editing from clean --- don't use
- * inrepeat or lastchar, synthesise an i to enter insert mode.
+ * inrepeat or keybuf, synthesise an entery to insert mode.
*/
/**/
@@ -91,14 +91,16 @@ startvichange(int im)
lastmod = zmod;
if (vichgbuf)
free(vichgbuf);
- vichgbuf = (char *)zalloc(vichgbufsz = 16);
+ vichgbuf = (char *)zalloc(vichgbufsz = 16 + keybuflen);
if (im == -2) {
vichgbuf[0] =
zlell ? (insmode ? (zlecs < zlell ? 'i' : 'a') : 'R') : 'o';
+ vichgbuf[1] = '\0';
+ vichgbufptr = 1;
} else {
- vichgbuf[0] = lastchar;
+ strcpy(vichgbuf, keybuf);
+ unmetafy(vichgbuf, &vichgbufptr);
}
- vichgbufptr = 1;
vichgrepeat = 0;
}
}
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Bug in ZSH's vi emulation
2016-11-03 12:43 ` Oliver Kiddle
2016-11-03 13:08 ` Bart Schaefer
2016-11-03 14:55 ` Bart Schaefer
@ 2016-11-03 16:06 ` Bart Schaefer
2 siblings, 0 replies; 16+ messages in thread
From: Bart Schaefer @ 2016-11-03 16:06 UTC (permalink / raw)
To: zsh-workers
On Nov 3, 1:43pm, Oliver Kiddle wrote:
}
} Works well in my testing too. We should add a new testcase too such
} as that below.
Revised somewhat:
diff --git a/Test/X02zlevi.ztst b/Test/X02zlevi.ztst
index ced7030..70fef4c 100644
--- a/Test/X02zlevi.ztst
+++ b/Test/X02zlevi.ztst
@@ -244,6 +244,14 @@
>BUFFER: binging
>CURSOR: 3
+ print -u $ZTST_fd 'This test may hang the shell when it fails...'
+ zletest $'worm\erdhd..'
+0:use of vi-repeat as the motion and repeat after a failed change
+>BUFFER: word
+>CURSOR: 2
+F:For vi compatibility, "." should repeat the "rd" change after "d."
+F:Update result to ">BUFFER: wodd" if compatibility is repaired.
+
zpty_run 'bindkey "^_" undo'
zletest $'undoc\037e'
0:use of undo in vi insert mode
^ permalink raw reply [flat|nested] 16+ messages in thread