* Re: bug with completion in quotes
2014-10-11 22:37 bug with completion in quotes Oliver Kiddle
@ 2014-10-12 4:41 ` Bart Schaefer
2014-10-12 17:35 ` Peter Stephenson
2014-10-20 17:22 ` Peter Stephenson
2 siblings, 0 replies; 12+ messages in thread
From: Bart Schaefer @ 2014-10-12 4:41 UTC (permalink / raw)
To: Zsh workers
On Oct 12, 12:37am, Oliver Kiddle wrote:
} Subject: bug with completion in quotes
}
} % : $'<tab>x
} utils.c:5893: BUG: unterminated $' substitution
Hrm. Comments say:
* ... If both GETKEY_DOLLAR_QUOTE
* and GETKEY_UPDATE_OFFSET are present in "how", the string is not
* expected to be terminated (this is used in completion to parse
* a partial $'...'-quoted string) and the length passed back is
* that of the converted string.
And later, right at the point of that DPUTS:
* When called from completion, where we use GETKEY_UPDATE_OFFSET to
* update the index into the metafied editor line, we don't necessarily
* have the end of a $'...' quotation, else we should do.
However, as far as I can tell nothing ever passes GETKEY_UPDATE_OFFSET
in any circumstances, completion or otherwise.
Adding that flag to the call near zle_tricky.c:1733 silences this warning,
but that can't be correct as GETKEY_UPDATE_OFFSET expects to deref the
"misc" parameter which is NULL in that call.
There are also places in utils.c where comments appear to mean that
GETKEY_UPDATE_OFFSET tests need to be added, but there are none. I
think we've uncovered someone's unfinshed work.
} % : $'<tab>x'
} zle_tricky.c:666: BUG: 0 <= wb <= zlemetacs <= we is not true!
Examining in the debugger we have
(gdb) p wb
$2 = 2
(gdb) p zlemetacs
$3 = 1
(gdb) p we
$4 = 3
I think this is an off-by-one error happening because dollar-tick is
two characters being treated as a solitary quote, but I don't know
where to fix it.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bug with completion in quotes
2014-10-11 22:37 bug with completion in quotes Oliver Kiddle
2014-10-12 4:41 ` Bart Schaefer
@ 2014-10-12 17:35 ` Peter Stephenson
2014-10-12 17:51 ` Peter Stephenson
2014-10-20 17:22 ` Peter Stephenson
2 siblings, 1 reply; 12+ messages in thread
From: Peter Stephenson @ 2014-10-12 17:35 UTC (permalink / raw)
To: Zsh workers
On Sun, 12 Oct 2014 00:37:09 +0200
Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> Compiled with --enable-zsh-debug, I get bugs listed when completing
> inside $' style quotes whenever there's stuff after the cursor:
>
> <tab> appears at cursor position
>
> % : $'<tab>x
> utils.c:5893: BUG: unterminated $' substitution
> % : $'<tab>x'
> zle_tricky.c:666: BUG: 0 <= wb <= zlemetacs <= we is not true!
>
> The line containing the first message was added in workers/22026 from
> about 2006.
This code is particularly horrible. 2006 is about the last time I
remember looking at it and giving up. However, most of the really nasty
problems we're with nested quotes e.g. where a quoted argument to a
shell or scripting language itself contained quotes --- which I'm not
sure we whould be trying to do internally anyway. One set of quotes is
just about tractable.
The chief suspect is set_comp_sep() --- it's very brittle because it's
interacting with all sorts of different parts of the shell in ad hoc
ways.
pws
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bug with completion in quotes
2014-10-12 17:35 ` Peter Stephenson
@ 2014-10-12 17:51 ` Peter Stephenson
2014-10-12 18:29 ` Bart Schaefer
0 siblings, 1 reply; 12+ messages in thread
From: Peter Stephenson @ 2014-10-12 17:51 UTC (permalink / raw)
To: Zsh workers
On Sun, 12 Oct 2014 18:35:47 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> On Sun, 12 Oct 2014 00:37:09 +0200
> Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> > Compiled with --enable-zsh-debug, I get bugs listed when completing
> > inside $' style quotes whenever there's stuff after the cursor:
> >
> > <tab> appears at cursor position
> >
> > % : $'<tab>x
> > utils.c:5893: BUG: unterminated $' substitution
> > % : $'<tab>x'
> > zle_tricky.c:666: BUG: 0 <= wb <= zlemetacs <= we is not true!
> >
> > The line containing the first message was added in workers/22026 from
> > about 2006.
>
> The chief suspect is set_comp_sep()
Although that function is a permanent black stain on my soul, before I
send others on wild goose chases I don't think it can be the culprit
here since I think it's *only* used for recursive quotes. I'm now
wondering about get_comp_string() from line 1676 in zle_tricky.c,
including the grumpy comment I would guess is from me, since they usually
are, around line 1686. However, I haven't gone into the nitty gritty.
pws
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bug with completion in quotes
2014-10-12 17:51 ` Peter Stephenson
@ 2014-10-12 18:29 ` Bart Schaefer
2014-10-12 19:44 ` Peter Stephenson
0 siblings, 1 reply; 12+ messages in thread
From: Bart Schaefer @ 2014-10-12 18:29 UTC (permalink / raw)
To: Zsh workers
On Oct 12, 6:51pm, Peter Stephenson wrote:
}
} Although that function is a permanent black stain on my soul, before I
} send others on wild goose chases I don't think it can be the culprit
} here since I think it's *only* used for recursive quotes. I'm now
} wondering about get_comp_string() from line 1676 in zle_tricky.c,
} including the grumpy comment I would guess is from me, since they usually
} are, around line 1686. However, I haven't gone into the nitty gritty.
It's definitely get_comp_string(), probably via getkeystring() in utils.c
See my earlier email.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bug with completion in quotes
2014-10-12 18:29 ` Bart Schaefer
@ 2014-10-12 19:44 ` Peter Stephenson
2014-10-12 20:15 ` Peter Stephenson
0 siblings, 1 reply; 12+ messages in thread
From: Peter Stephenson @ 2014-10-12 19:44 UTC (permalink / raw)
To: Zsh workers
On Sun, 12 Oct 2014 11:29:43 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Oct 12, 6:51pm, Peter Stephenson wrote:
> }
> } Although that function is a permanent black stain on my soul, before I
> } send others on wild goose chases I don't think it can be the culprit
> } here since I think it's *only* used for recursive quotes. I'm now
> } wondering about get_comp_string() from line 1676 in zle_tricky.c,
> } including the grumpy comment I would guess is from me, since they usually
> } are, around line 1686. However, I haven't gone into the nitty gritty.
>
> It's definitely get_comp_string(), probably via getkeystring() in utils.c
>
> See my earlier email.
I think this is zsh-workers/23809, commit
e0a3e74b15fd39b21ef1770e67e2f005321b5fb9, going off at the wrong time.
It was supposed to apply to expanding complete $'...' expressions, but
in this case it's being applied to an uncompleted completion.
It looks like the intended purpose is basically OK since completing
after a $'...' works OK, at least in simple cases. So it may be we
simply need to skip the code added by that change --- simply setting
skipchars to 2 instead looks like it's probably the right thing to do
--- if we're in the middle of it.
I think this is roughly what the "i + (pe - p) < zlemetacs" test is
trying to do at line 1721. Evidently it's not working. Presumably one
of the many ad hoc offsets hasn't been taken account of.
pws
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bug with completion in quotes
2014-10-12 19:44 ` Peter Stephenson
@ 2014-10-12 20:15 ` Peter Stephenson
2014-10-12 20:36 ` Bart Schaefer
0 siblings, 1 reply; 12+ messages in thread
From: Peter Stephenson @ 2014-10-12 20:15 UTC (permalink / raw)
To: Zsh workers
On Sun, 12 Oct 2014 20:44:29 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> On Sun, 12 Oct 2014 11:29:43 -0700
> Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> > On Oct 12, 6:51pm, Peter Stephenson wrote:
> > }
> > } Although that function is a permanent black stain on my soul, before I
> > } send others on wild goose chases I don't think it can be the culprit
> > } here since I think it's *only* used for recursive quotes. I'm now
> > } wondering about get_comp_string() from line 1676 in zle_tricky.c,
> > } including the grumpy comment I would guess is from me, since they usually
> > } are, around line 1686. However, I haven't gone into the nitty gritty.
> >
> > It's definitely get_comp_string(), probably via getkeystring() in utils.c
> >
> > See my earlier email.
>
> I think this is zsh-workers/23809, commit
> e0a3e74b15fd39b21ef1770e67e2f005321b5fb9, going off at the wrong time.
> It was supposed to apply to expanding complete $'...' expressions, but
> in this case it's being applied to an uncompleted completion.
>
> It looks like the intended purpose is basically OK since completing
> after a $'...' works OK, at least in simple cases. So it may be we
> simply need to skip the code added by that change --- simply setting
> skipchars to 2 instead looks like it's probably the right thing to do
> --- if we're in the middle of it.
Sigh. Nope. I think it *is* doing the skipchars == 2 branch but that's
not the right thing to do for some reason. I don't know what is.
pws
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bug with completion in quotes
2014-10-12 20:15 ` Peter Stephenson
@ 2014-10-12 20:36 ` Bart Schaefer
0 siblings, 0 replies; 12+ messages in thread
From: Bart Schaefer @ 2014-10-12 20:36 UTC (permalink / raw)
To: Zsh workers
On Oct 12, 9:15pm, Peter Stephenson wrote:
}
} > I think this is zsh-workers/23809, commit
} > e0a3e74b15fd39b21ef1770e67e2f005321b5fb9, going off at the wrong time.
} > It was supposed to apply to expanding complete $'...' expressions, but
} > in this case it's being applied to an uncompleted completion.
} >
} > It looks like the intended purpose is basically OK since completing
} > after a $'...' works OK, at least in simple cases. So it may be we
} > simply need to skip the code added by that change --- simply setting
} > skipchars to 2 instead looks like it's probably the right thing to do
} > --- if we're in the middle of it.
}
} Sigh. Nope. I think it *is* doing the skipchars == 2 branch but that's
} not the right thing to do for some reason. I don't know what is.
So below I'm talking about the incomplete $'x expression. The case of
the $'x' expression with the wb <= zlemetacs may be different.
I've GDB'd through it and it's definitely taking the branch that begins
(zle_tricky.c) with the comment
/*
* Try and substitute the $'...' expression.
*/
If it took the skipchars = 2 branch it'd never hit the DPUTS that got
this whole thread started.
Comment in utils.c:
* *misc is used for various purposes:
* - If GETKEY_UPDATE_OFFSET is set, it is set on input to some
* mystical completion offset and is updated to a new offset based
* on the converted characters. All Hail the Completion System
* [makes the mystic completion system runic sign in the air].
But nothing ever passes GETKEY_UPDATE_OFFSET. That branch of zle_tricky
looks like the only reasonable place to use that, but I don't know what
mystical offset is meant to be passed in *misc and returned updated.
Maybe &skipchars ? But what value should it start with?
Note that both of these bugs only occur when the completion is at the
START of the $'x' expression (before the "x"). When after the x or
after the whole expression, everything works OK.
So maybe the problem is that getkeystring() is reading too far ahead
and returning the length of the entire $'...' (or the length so far,
in the case of the unclosed quote) and really what we want is only
the length up to the cursor position?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bug with completion in quotes
2014-10-11 22:37 bug with completion in quotes Oliver Kiddle
2014-10-12 4:41 ` Bart Schaefer
2014-10-12 17:35 ` Peter Stephenson
@ 2014-10-20 17:22 ` Peter Stephenson
2014-10-22 14:05 ` Oliver Kiddle
2 siblings, 1 reply; 12+ messages in thread
From: Peter Stephenson @ 2014-10-20 17:22 UTC (permalink / raw)
To: Zsh workers
See if this helps with some of the $'...' problems.
Note that when the cursor is inside $'...' we deliberately don't try to
expand the $'...'. That's not a change. Possibly we could do so, but
we'd need to be careful about where the cursor ended up.
diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index 499c4ae..5fa625a 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -662,8 +662,9 @@ docomplete(int lst)
* NOTE: get_comp_string() calls pushheap(), but not popheap(). */
noerrs = 1;
s = get_comp_string();
- DPUTS(wb < 0 || zlemetacs < wb || zlemetacs > we,
- "BUG: 0 <= wb <= zlemetacs <= we is not true!");
+ DPUTS3(wb < 0 || zlemetacs < wb || zlemetacs > we,
+ "BUG: 0 <= wb (%d) <= zlemetacs (%d) <= we (%d) is not true!",
+ wb, zlemetacs, we);
noerrs = ne;
/* For vi mode, reset the start-of-insertion pointer to the beginning *
* of the word being completed, if it is currently later. Vi itself *
@@ -1720,9 +1721,11 @@ get_comp_string(void)
for (pe = p + 2; *pe && *pe != Snull && i + (pe - p) < zlemetacs;
pe++)
;
- if (!*pe) {
+ if (*pe != Snull) {
/* no terminating Snull, can't substitute */
skipchars = 2;
+ if (*pe)
+ j = 1;
} else {
/*
* Try and substitute the $'...' expression.
@@ -1795,6 +1798,10 @@ get_comp_string(void)
* first clue how the completion system actually works.
*/
skipchars = 2;
+ /*
+ * Also pretend we're in single quotes.
+ */
+ j = 1;
}
}
}
@@ -1817,7 +1824,7 @@ get_comp_string(void)
ocs = zlemetacs;
zlemetacs = i;
foredel(skipchars, CUT_RAW);
- if ((zlemetacs = ocs) > (i -= skipchars))
+ if ((zlemetacs = ocs) > i--)
zlemetacs -= skipchars;
we -= skipchars;
}
pws
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bug with completion in quotes
2014-10-20 17:22 ` Peter Stephenson
@ 2014-10-22 14:05 ` Oliver Kiddle
2014-10-22 14:28 ` Peter Stephenson
0 siblings, 1 reply; 12+ messages in thread
From: Oliver Kiddle @ 2014-10-22 14:05 UTC (permalink / raw)
To: Zsh workers
Peter Stephenson wrote:
>
> Note that when the cursor is inside $'...' we deliberately don't try to
> expand the $'...'. That's not a change. Possibly we could do so, but
> we'd need to be careful about where the cursor ended up.
By the way, I've been using this patch and it seems to work fine.
I don't think we'd want to expand the $'...': I think it's fine as it
is.
Thanks
Oliver
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bug with completion in quotes
2014-10-22 14:05 ` Oliver Kiddle
@ 2014-10-22 14:28 ` Peter Stephenson
2014-10-24 8:51 ` Peter Stephenson
0 siblings, 1 reply; 12+ messages in thread
From: Peter Stephenson @ 2014-10-22 14:28 UTC (permalink / raw)
To: Zsh workers
On Wed, 22 Oct 2014 16:05:01 +0200
Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> Peter Stephenson wrote:
> >
> > Note that when the cursor is inside $'...' we deliberately don't try to
> > expand the $'...'. That's not a change. Possibly we could do so, but
> > we'd need to be careful about where the cursor ended up.
>
> By the way, I've been using this patch and it seems to work fine.
>
> I don't think we'd want to expand the $'...': I think it's fine as it
> is.
Good --- I've been a little tied up (guess which revision control system we've
suddenly decided to move to?) but I'm worried that
- if ((zlemetacs = ocs) > (i -= skipchars))
+ if ((zlemetacs = ocs) > i--)
should actually be
- if ((zlemetacs = ocs) > (i -= skipchars))
+ if ((zlemetacs = ocs) > --i)
In other words, I'm sure that only decrementing once is correct ---
we're deleting characters ahead of the point we're looking at, so the
decrement is simply to cancel out the effect of the ++i in the loop
iterator --- but the --i is more consistent with what was there before.
I'll try and puzzle out the exact meaning of the indices before I commit
this.
pws
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bug with completion in quotes
2014-10-22 14:28 ` Peter Stephenson
@ 2014-10-24 8:51 ` Peter Stephenson
0 siblings, 0 replies; 12+ messages in thread
From: Peter Stephenson @ 2014-10-24 8:51 UTC (permalink / raw)
To: Zsh workers
On Wed, 22 Oct 2014 15:28:02 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> I'm worried that
>
> - if ((zlemetacs = ocs) > (i -= skipchars))
> + if ((zlemetacs = ocs) > i--)
>
> should actually be
>
> - if ((zlemetacs = ocs) > (i -= skipchars))
> + if ((zlemetacs = ocs) > --i)
>
> In other words, I'm sure that only decrementing once is correct ---
> we're deleting characters ahead of the point we're looking at, so the
> decrement is simply to cancel out the effect of the ++i in the loop
> iterator --- but the --i is more consistent with what was there before.
> I'll try and puzzle out the exact meaning of the indices before I commit
> this.
I can't find a case where this seems to make a difference in practice.
If there's an off-by-one error remaining it's a very subtle one.
Given that, I'll commit this with the second change since it looks more
like what's there but with the definite bug removed.
pws
^ permalink raw reply [flat|nested] 12+ messages in thread