zsh-workers
 help / color / mirror / code / Atom feed
* compset -q oddities
@ 2016-09-11  7:30 Daniel Shahaf
  2016-09-12  2:14 ` Bart Schaefer
  0 siblings, 1 reply; 18+ messages in thread
From: Daniel Shahaf @ 2016-09-11  7:30 UTC (permalink / raw)
  To: zsh-workers

I managed to break a few things while composing a reply to 39265:

1.
% _f() { compset -q ; compadd "~~~" } 
% compdef _f f
% f <TAB><TAB>
<becomes>
% f \\\~\\\~\\\~\\\~\\\~\\\~

2. 
% _g() { compset -q } 
% compdef _g g
% g $'\'<TAB>
compcore.c:1657: expecting 'x' at offset 2 of "'x"

3. 
% _h() { repeat 2 compset -q; compadd foo }
% compdef _h h
% h "\$'<TAB>
<becomes>
% h "$'foo<TAB>
utils.c:6826: BUG: unterminated $' substitution



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: compset -q oddities
  2016-09-11  7:30 compset -q oddities Daniel Shahaf
@ 2016-09-12  2:14 ` Bart Schaefer
  2016-09-12 23:06   ` Daniel Shahaf
  0 siblings, 1 reply; 18+ messages in thread
From: Bart Schaefer @ 2016-09-12  2:14 UTC (permalink / raw)
  To: zsh-workers

On Sep 11,  7:30am, Daniel Shahaf wrote:
}
} I managed to break a few things while composing a reply to 39265:

I think mostly what you've discovered is cases where "compset" is not
compensating for pilot error.  "compset -q" does not expect to be
called on an empty word, or on any word that can't be interpreted as
a (possibly partial) command line; so you're getting garbage in /
garbage out instead of garbage in / some kind of failure out.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: compset -q oddities
  2016-09-12  2:14 ` Bart Schaefer
@ 2016-09-12 23:06   ` Daniel Shahaf
  2016-09-13  6:28     ` Bart Schaefer
  0 siblings, 1 reply; 18+ messages in thread
From: Daniel Shahaf @ 2016-09-12 23:06 UTC (permalink / raw)
  To: zsh-workers

Bart Schaefer wrote on Sun, Sep 11, 2016 at 19:14:22 -0700:
> On Sep 11,  7:30am, Daniel Shahaf wrote:
> }
> } I managed to break a few things while composing a reply to 39265:
> 
> I think mostly what you've discovered is cases where "compset" is not
> compensating for pilot error.  "compset -q" does not expect to be
> called on an empty word, or on any word that can't be interpreted as
> a (possibly partial) command line;

Thanks, but I don't understand how any of those examples constitutes
a "pilot error" or an empty word.

In #1, "~~~" is a complete word; completion changed it to "~~~~~~" which
is also a complete word but wasn't a candidate completion.

In #2 and #3, the input is a prefix of a valid command line («g
$'\'foo\''» and «h "$'foo'"» respectively).  #2 does not involve an
empty word; the word there is «'» (a single-byte word).

Thanks again for the explanation.  I'd like to understand what the
expected behaviour here is.  They triggered DPUTS calls so I assumed
they were bugs.

Cheers,

Daniel
(#2 doesn't call compadd, but behaves the same if a call to _nothing is
added to _g.)


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: compset -q oddities
  2016-09-12 23:06   ` Daniel Shahaf
@ 2016-09-13  6:28     ` Bart Schaefer
  2016-09-13 10:21       ` Peter Stephenson
  2016-09-14  3:22       ` compset -q oddities Daniel Shahaf
  0 siblings, 2 replies; 18+ messages in thread
From: Bart Schaefer @ 2016-09-13  6:28 UTC (permalink / raw)
  To: zsh-workers

On Sep 12, 11:06pm, Daniel Shahaf wrote:
} Subject: Re: compset -q oddities
}
} Thanks, but I don't understand how any of those examples constitutes
} a "pilot error" or an empty word.

Some clarification, then:

} In #1, "~~~" is a complete word; completion changed it to "~~~~~~" which
} is also a complete word but wasn't a candidate completion.

You didn't start from ~~~.  You started from an empty word and typed
TAB twice.  ~~~ was never on the line.  I concur that the result of
the second attempt is weird, I would have expected it just to fail.

On the first tab (empty word), the prefix is empty but "compset -q"
causes completion to believe there is quoting on the line (backslash
by default).  The compadd call makes ~~~ a valid completion, which
when added is quoted to \~\~\~.  The effect of compset is then undone,
meaning the backslash-quoting that was presumed removed is restored,
and you end up with \\\~\\\~\\\~.

On the second tab, one level of quoting is removed by compset to get
\~\~\~ as the prefix.  Again compadd inserts \~\~\~ which does match
the prefix, so it's a valid completion, but somewhere along the line
all the \~ go back to \\\~ and now \~ doesn't match any more and so
instead of being discarded because it's already present in the prefix,
it gets appended and you end up with \\\~\\\~\\\~\\\~\\\~\\\~.

This happens for any compadd containing a tilde, it doesn't have to
be the first character in the word.  So something wonky is going on
with the handlng of tildes.  Actually the same thing happens with a
leading "=" (tilde can be anywhere), so it's quotestring() that is
contributing to the mahem (the large "else if" condition on lines
5824-5836).

} In #2 and #3, the input is a prefix of a valid command line ("g
} $'\'foo\''" and "h "$'foo'"" respectively).

No, that's not what I meant.  The *word* being completed is meant to
be a partial command line, not the whole buffer.  I know that the doc
says "split on spaces into separate words, respecting the usual shell
quoting conventions" but the implementation calls set_comp_sep() which
"splits the current word as if it were a command line".  But read on.

} #2 does not involve an
} empty word; the word there is "'" (a single-byte word).

Also not quite true; the word in both #2 and #3 is an un-closed quoted
string.  The result of that parse is not well-defined.

I also don't know any circumstance in which it would be correct to call
"compset -q" twice in succession as you did in #3.

However, on closer inspection I don't think there's actually anything
wrong with the results if the debug warnings are ignored, unlike the
case with #1.

} Thanks again for the explanation.  I'd like to understand what the
} expected behaviour here is.  They triggered DPUTS calls so I assumed
} they were bugs.

This one --

    compcore.c:1657: expecting 'x' at offset 2 of "'x"

-- seems to be a legit problem with counting bytes when looking for an
unbalanced $'...'.  I don't think fixing that would change the outcome,
that is $'\' --> \' (see quoting converted, above).  I'm not confident
of how to fix it; PWS was last here in workers/22026 (git 34381548).

For this --

    utils.c:6826: BUG: unterminated $' substitution

-- my line numbers seem to be off-by-20 from yours, but its the same
commit as the previous bug, and it's correct about the unterminated
$'...', so it's probably a spurious DPUTS.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: compset -q oddities
  2016-09-13  6:28     ` Bart Schaefer
@ 2016-09-13 10:21       ` Peter Stephenson
  2016-09-14 17:56         ` Bart Schaefer
  2016-09-14  3:22       ` compset -q oddities Daniel Shahaf
  1 sibling, 1 reply; 18+ messages in thread
From: Peter Stephenson @ 2016-09-13 10:21 UTC (permalink / raw)
  To: zsh-workers

On Mon, 12 Sep 2016 23:28:53 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> This one --
>    % _g() { compset -q } 
>    % compdef _g g
>    % g $'\'<TAB>
>    compcore.c:1657: expecting 'x' at offset 2 of "'x"> 
> 
> -- seems to be a legit problem with counting bytes when looking for an
> unbalanced $'...'.  I don't think fixing that would change the outcome,
> that is $'\' --> \' (see quoting converted, above).  I'm not confident
> of how to fix it; PWS was last here in workers/22026 (git 34381548).

The missing offset seems to come because we have an input string
(omitting double quotes) " \\'x" even before we do the double quote
processing.  The \\ goes aways with the double quote processing leaving
the ', which seems to be fine.  It looks like it ought to modify tl to
give the length we set to zlemetall later on, since that comes from the
modified tmp (great names), but it doesn't; however, that doesn't seem
to be the problem.

When we run the lexer over this and get an error, it then ignores that
initial " ", and I think that's what's giving the bad offset.  tokstr is
now "'x " except with ' tokenised (that's not a problem here); we make
efforts to deal with the added final " " but not the fact the previous
initial space has (in technical language) gorn.

I don't know where that initial space is coming from.

I don't know if this means the lexerr stuff is therefore too brittle
about characters it found on an unterminated string to be useful and we
should simply give up in some more well-defined way.

It might be sensible instead of using tokstr to take the input string
when we encounter an error, but I presume we'd need to know at least the
start of input for the current iteration of the loop containing
ctxtlex() (we do have mechanisms for copying literal input during lexing
for use with the new-improved $(...) parsing but that's largely
orthogonal to what's happening here, where "largely" means "actually I
don't really know what's going on in terms of hierarchy because it's way
too complicated").

Anyway, I suspect it ought to be possible to do better.  I've seen this
case crash intermittently as we're accessing bad memory --- we could
probably at least fix up accesses off the end of the string after the
DPUTS for safety.

However, I'm not sure we want to fiddle with this before a release which
we really ought to be making imminently as this is just one of a series
of incrementally partially fixed problems in this area

(This whole interface is horribly brittle anyway, of course, but it's
unlikely anyone's going to dare to rewrite it any time soon.)

pws


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: compset -q oddities
  2016-09-13  6:28     ` Bart Schaefer
  2016-09-13 10:21       ` Peter Stephenson
@ 2016-09-14  3:22       ` Daniel Shahaf
  2016-09-14  5:20         ` Bart Schaefer
  1 sibling, 1 reply; 18+ messages in thread
From: Daniel Shahaf @ 2016-09-14  3:22 UTC (permalink / raw)
  To: zsh-workers

Bart Schaefer wrote on Mon, Sep 12, 2016 at 23:28:53 -0700:
> On Sep 12, 11:06pm, Daniel Shahaf wrote:
> } Subject: Re: compset -q oddities
> }
> } Thanks, but I don't understand how any of those examples constitutes
> } a "pilot error" or an empty word.
> 
> Some clarification, then:
> 
> } In #1, "~~~" is a complete word; completion changed it to "~~~~~~" which
> } is also a complete word but wasn't a candidate completion.
> 
> You didn't start from ~~~.  You started from an empty word and typed
> TAB twice.  ~~~ was never on the line.  I concur that the result of
> the second attempt is weird, I would have expected it just to fail.
> 

I would have expected the second <TAB> press to do nothing, since that's
what it does if the _f compadd's "xyz" instead of "~~~".

> On the first tab (empty word), the prefix is empty but "compset -q"
> causes completion to believe there is quoting on the line (backslash
> by default).  The compadd call makes ~~~ a valid completion, which
> when added is quoted to \~\~\~.  The effect of compset is then undone,
> meaning the backslash-quoting that was presumed removed is restored,
> and you end up with \\\~\\\~\\\~.
> 

All this is fine.  It would also have been correct for the first level
of quoting not to occur, since «~~~» doesn't need command-line escaping.

> On the second tab, one level of quoting is removed by compset to get
> \~\~\~ as the prefix.  Again compadd inserts \~\~\~ which does match
> the prefix, so it's a valid completion, but somewhere along the line
> all the \~ go back to \\\~ and now \~ doesn't match any more and so
> instead of being discarded because it's already present in the prefix,
> it gets appended and you end up with \\\~\\\~\\\~\\\~\\\~\\\~.
> 
> This happens for any compadd containing a tilde, it doesn't have to
> be the first character in the word.  So something wonky is going on
> with the handlng of tildes.  Actually the same thing happens with a
> leading "=" (tilde can be anywhere), so it's quotestring() that is
> contributing to the mahem (the large "else if" condition on lines
> 5824-5836).

That condition quotes the tilde because isset(EXTENDEDGLOB) is true.
This makes sense as far as quotestring() is concerned, but when called
from completion, this causes the tilde to be quoted even though
${_comp_caller_options[extendedglob]} is unset.

Bypassing this by making _f restore options[extendedglob] before calling
compadd fixes a few cases with «compadd 'a~b'» and with «compadd '~~~'»
followed by «f ~~~<TAB>», but «f <TAB><TAB>» is still wrong: it gives
\\~~~ and then \\\~\~\~\\~~~.

The backslash in \\~~~ is probably added by quotestring() [despite
unsetting extendedglob] because the 'u==s' part of the condition is
true.  Perhaps making quotestring() not add that redundant first
backslash would workaround the issue, i.e., would cause the "wonkiness"
in comp*.c not to get triggered.

> } In #2 and #3, the input is a prefix of a valid command line ("g
> } $'\'foo\''" and "h "$'foo'"" respectively).
> 
> No, that's not what I meant.  The *word* being completed is meant to
> be a partial command line, not the whole buffer.  I know that the doc
> says "split on spaces into separate words, respecting the usual shell
> quoting conventions" but the implementation calls set_comp_sep() which
> "splits the current word as if it were a command line".  But read on.
> 
> } #2 does not involve an
> } empty word; the word there is "'" (a single-byte word).
> 
> Also not quite true; the word in both #2 and #3 is an un-closed quoted
> string.  The result of that parse is not well-defined.
> 

Okay.

> I also don't know any circumstance in which it would be correct to call
> "compset -q" twice in succession as you did in #3.
> 

Something like this:

    sh -c "sh -c '<TAB>

The documentation of compset explicitly mentions that 'compset -q' might
be called recursively.

> However, on closer inspection I don't think there's actually anything
> $'...', so it's probably a spurious DPUTS.

Thanks for the detailed answer.

Daniel


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: compset -q oddities
  2016-09-14  3:22       ` compset -q oddities Daniel Shahaf
@ 2016-09-14  5:20         ` Bart Schaefer
  2016-09-14  6:12           ` Daniel Shahaf
  2016-09-14  8:31           ` Peter Stephenson
  0 siblings, 2 replies; 18+ messages in thread
From: Bart Schaefer @ 2016-09-14  5:20 UTC (permalink / raw)
  To: zsh-workers

On Sep 14,  3:22am, Daniel Shahaf wrote:
} Subject: Re: compset -q oddities
}
} Bart Schaefer wrote on Mon, Sep 12, 2016 at 23:28:53 -0700:
} > You didn't start from ~~~.  You started from an empty word and typed
} > TAB twice.  ~~~ was never on the line.  I concur that the result of
} > the second attempt is weird, I would have expected it just to fail.
} 
} I would have expected the second <TAB> press to do nothing

If the first tab had correctly produced one of ~~~ or \~\~\~ then I
would have expected the second tab to do nothing, but given what the
first tab incorrectly burped out, it should have failed.

Also now that I think of it, there's only one match with that compadd,
so it should have appended a trailing space and the second tab should
have been in an entirely new (also empty) word.

} > On the first tab (empty word), the prefix is empty but "compset -q"
} > causes completion to believe there is quoting on the line (backslash
} > by default).  The compadd call makes ~~~ a valid completion, which
} > when added is quoted to \~\~\~.  The effect of compset is then undone,
} > meaning the backslash-quoting that was presumed removed is restored,
} > and you end up with \\\~\\\~\\\~.
} 
} All this is fine.

Well, no, not really.  "compset -q" should make note that there were
no quotes when it was called, and therefore not restore any quote on
the way back out.  There's a comment above compcore.c:307

    /*
     * It looks like we may need to do stuff with backslashes even
     * if instring is QT_NONE.
     */

Peter, that was you in workers/22819, can you explain?

} It would also have been correct for the first level of quoting not to
} occur, since "~~~" doesn't need command-line escaping.

As noted later, this is because of extendedglob.  Examining each tilde
in isolation, quotestring() has to assume the string may be interpreted
as an X~Y pattern, so it quotes all tildes.

} This makes sense as far as quotestring() is concerned, but when called
} from completion, this causes the tilde to be quoted even though
} ${_comp_caller_options[extendedglob]} is unset.

Indeed.  It might make sense for the internals to save the top-level
option state on entry and implicitly re-assert it during compadd.

} The backslash in \\~~~ is probably added by quotestring() [despite
} unsetting extendedglob] because the 'u==s' part of the condition

That's exactly what happens, and is also why a leading = gets quoted.

} Perhaps making quotestring() not add that redundant first
} backslash would workaround the issue

The problem isn't really with the backslash being added there, it's
somewhere later on when the prefix is being compared to the candidate
match and one side of gets too much (? different?) quoting before the
comparison is made.  I haven't figured out where that is, yet (and am
not going to try too hard, honestly).


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: compset -q oddities
  2016-09-14  5:20         ` Bart Schaefer
@ 2016-09-14  6:12           ` Daniel Shahaf
  2016-09-14 14:59             ` Bart Schaefer
  2016-09-14  8:31           ` Peter Stephenson
  1 sibling, 1 reply; 18+ messages in thread
From: Daniel Shahaf @ 2016-09-14  6:12 UTC (permalink / raw)
  To: zsh-workers

Bart Schaefer wrote on Tue, Sep 13, 2016 at 22:20:29 -0700:
> On Sep 14,  3:22am, Daniel Shahaf wrote:
> } Subject: Re: compset -q oddities
> }
> } Bart Schaefer wrote on Mon, Sep 12, 2016 at 23:28:53 -0700:
> } > You didn't start from ~~~.  You started from an empty word and typed
> } > TAB twice.  ~~~ was never on the line.  I concur that the result of
> } > the second attempt is weird, I would have expected it just to fail.
> } 
> } I would have expected the second <TAB> press to do nothing
> 
> If the first tab had correctly produced one of ~~~ or \~\~\~ then I
> would have expected the second tab to do nothing, but given what the
> first tab incorrectly burped out, it should have failed.

Why should «f \\\~\\\~\\\~<TAB>» fail?

«ssh \git.code<TAB>» completes to "ssh git.code.sf.net" despite the
redundant backslash.

By analogy, when 'compset -q' is _not_ involved, if "~~~" is a match
then «\~\~\~<TAB>» should complete to it.  (Although it might in fact
complete to \~\~\~ because of the quotestring() issue discussed below.)

Since a \~\~\~ word matches a '~~~' completion when 'compset -q' is not
involved, therefore a \\\~\\\~\\\~ word should match a '~~~' completion
when 'compset -q' _is_ in effect.

Essentially, 'compset -q' "looks through" one level of quoting, and
whatever is happening in the «ssh \git.code<TAB>» case looks through the
second level of quoting.

Makes sense?

> Also now that I think of it, there's only one match with that compadd,
> so it should have appended a trailing space and the second tab should
> have been in an entirely new (also empty) word.

That's a bit tricky.  I think it should append an escaped space, e.g.,
«sh -c touc<TAB>» should append <h> <Backslash> <Space>.

Then there are use-cases such as «su -c script-without-arguments.<TAB>»
where one wants to run «su -c script-without-arguments.foobar» (without
further arguments), which make me think that backslash-escaped should be
autoremovable.

Perhaps something like this, for «sh -c ech<TAB>»:

1) <h> <Backslash> <Space> is inserted.

2) If the user types an alphanumeric, the backslash-space is kept (stops
being autoremovable) and the alphanumeric is inserted as the start of
the second argument to 'echo'.

3) If the user types a space, the backslash-space changed to
space (to close the argument to the top-level -c option).

All this assumes the "compset -q"'d argument is backslash-quoted.  It'd
be slightly different if it uses single or double quotes («sh -c
'ech<TAB>»).

> } This makes sense as far as quotestring() is concerned, but when called
> } from completion, this causes the tilde to be quoted even though
> } ${_comp_caller_options[extendedglob]} is unset.
> 
> Indeed.  It might make sense for the internals to save the top-level
> option state on entry and implicitly re-assert it during compadd.
> 

Not only compadd, but all the other comp* builtins too, no?

> } Perhaps making quotestring() not add that redundant first
> } backslash would workaround the issue
> 
> The problem isn't really with the backslash being added there, it's
> somewhere later on when the prefix is being compared to the candidate
> match and one side of gets too much (? different?) quoting before the
> comparison is made.  I haven't figured out where that is, yet (and am
> not going to try too hard, honestly).

Fair enough :-)

Cheers,

Daniel


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: compset -q oddities
  2016-09-14  5:20         ` Bart Schaefer
  2016-09-14  6:12           ` Daniel Shahaf
@ 2016-09-14  8:31           ` Peter Stephenson
  2016-09-14 16:04             ` Bart Schaefer
  1 sibling, 1 reply; 18+ messages in thread
From: Peter Stephenson @ 2016-09-14  8:31 UTC (permalink / raw)
  To: zsh-workers

On Tue, 13 Sep 2016 22:20:29 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Well, no, not really.  "compset -q" should make note that there were
> no quotes when it was called, and therefore not restore any quote on
> the way back out.  There's a comment above compcore.c:307
> 
>     /*
>      * It looks like we may need to do stuff with backslashes even
>      * if instring is QT_NONE.
>      */
> 
> Peter, that was you in workers/22819, can you explain?

I've no idea what case I was looking at ten years ago, and almost
certainly it was just one of many possibilities.  I do know I gave
up completely on improving this further because it was too horrific.

I think the point is related to the fact that in the case of backslashes
we never know there are no quotes.  We just know if there are individual
backslashes present and not what they mean --- but you can still strip
backslashes on user request, which is what you'll do if asked to strip
quotes and you don't find any other sort.

pws


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: compset -q oddities
  2016-09-14  6:12           ` Daniel Shahaf
@ 2016-09-14 14:59             ` Bart Schaefer
  2016-09-14 19:52               ` Oliver Kiddle
  0 siblings, 1 reply; 18+ messages in thread
From: Bart Schaefer @ 2016-09-14 14:59 UTC (permalink / raw)
  To: zsh-workers

On Sep 14,  6:12am, Daniel Shahaf wrote:
} Subject: Re: compset -q oddities
}
} Why should "f \\\~\\\~\\\~<TAB>" fail?
} 
} Essentially, 'compset -q' "looks through" one level of quoting, and
} whatever is happening in the "ssh \git.code<TAB>" case looks through the
} second level of quoting.
} 
} Makes sense?

Yes, except that in \g the backslash really is redundant, whereas with
\~ the intent might be to compare a literal tilde to an expansion tilde
(which is different still from a globbing tilde).

} Bart Schaefer wrote on Tue, Sep 13, 2016 at 22:20:29 -0700:
} > Also now that I think of it, there's only one match with that compadd,
} > so it should have appended a trailing space and the second tab should
} > have been in an entirely new (also empty) word.
} 
} That's a bit tricky.  I think it should append an escaped space, e.g.,
} "sh -c touc<TAB>" should append <h> <Backslash> <Space>.

No, that's *exactly* the kind of DWIM-ing that we abandoned.  If the user
wants the space quoted, then he should start with "sh -c 'touc<TAB>".


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: compset -q oddities
  2016-09-14  8:31           ` Peter Stephenson
@ 2016-09-14 16:04             ` Bart Schaefer
  0 siblings, 0 replies; 18+ messages in thread
From: Bart Schaefer @ 2016-09-14 16:04 UTC (permalink / raw)
  To: zsh-workers

On Sep 14,  9:31am, Peter Stephenson wrote:
} Subject: Re: compset -q oddities
}
} On Tue, 13 Sep 2016 22:20:29 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > Well, no, not really.  "compset -q" should make note that there were
} > no quotes when it was called, and therefore not restore any quote on
} > the way back out.  There's a comment above compcore.c:307
} > 
} >     /*
} >      * It looks like we may need to do stuff with backslashes even
} >      * if instring is QT_NONE.
} >      */
} 
} I think the point is related to the fact that in the case of backslashes
} we never know there are no quotes.

Aha.  So if we see a word that starts with backslash, instring will be
QT_BACKSLASH, but when instring is QT_NONE, we do not know whether any
other backslashes appear in the middle of the word.

However, as I said in a previous reply:

>> The problem isn't really with the backslash being added there, it's
>> somewhere later on when the prefix is being compared to the candidate
>> match and one side of gets too much (? different?) quoting before the
>> comparison is made.

That later stage [wherever it is, I suspect comp_match()] also expects
that quoting has been manipulated in this way, and so if it has *not*
(I played with compcore.c:307) then strings that should match, do not.

The problem is that we have [at least] three sources of data:  The
literal command line, the (possibly quoting-manipulated) subset of
the command line against which completion is being attempted, and the
candidate matches passed to compadd.  Canonicalizing in a way that
makes it possible to compare any two of the three is very difficult.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: compset -q oddities
  2016-09-13 10:21       ` Peter Stephenson
@ 2016-09-14 17:56         ` Bart Schaefer
  2016-09-15  5:10           ` Daniel Shahaf
  0 siblings, 1 reply; 18+ messages in thread
From: Bart Schaefer @ 2016-09-14 17:56 UTC (permalink / raw)
  To: zsh-workers

On Sep 13, 11:21am, Peter Stephenson wrote:
}
} However, I'm not sure we want to fiddle with this before a release which
} we really ought to be making imminently as this is just one of a series
} of incrementally partially fixed problems in this area

I concur, we can e.g. note these issues in README or somewhere ("the
calling completer may need to test [[ -n $words[CURRENT] ]] before
using `compset -q'" or similar verbiage) and move on for now.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: compset -q oddities
  2016-09-14 14:59             ` Bart Schaefer
@ 2016-09-14 19:52               ` Oliver Kiddle
  2016-09-15  3:08                 ` Bart Schaefer
  0 siblings, 1 reply; 18+ messages in thread
From: Oliver Kiddle @ 2016-09-14 19:52 UTC (permalink / raw)
  To: zsh-workers

Bart wrote:
> } That's a bit tricky.  I think it should append an escaped space, e.g.,
> } "sh -c touc<TAB>" should append <h> <Backslash> <Space>.
>
> No, that's *exactly* the kind of DWIM-ing that we abandoned.  If the user
> wants the space quoted, then he should start with "sh -c 'touc<TAB>".

For this specific case, I don't entirely agree because in theory the
mechanics are in place for that to work.

compset -q appears to disable the default suffix that completions have:
  _foo() {
    compset -q
    compadd one two three
  }
  foo "on<tab>  completes one without a space suffix.
I'm not sure why. Any ideas?

The other problem in this case is that suffixes are not quoted in
general. You're supposed to do that manually with compquote but we only
ever bother to do that in generic helpers. So the following does work as
Daniel wished:
  _foo() {
    suf=' '
    compquote suf
    compadd -S $suf one two three
  }          
  sh -c "foo on<tab>

You might even use ${compstate[quote]:#\\}$suf as the suffix to
close any inner quoting. -P prefixes and -S suffixes are added
unquoted: in contrast to -p/-s. This was probably done so that it
can include the closing quote. Either that or because that's how
compctl originally did things long before completion was complex
enough to worry much about quotes.

Adding calls to compquote in every function that needs a suffix is
not a good idea. With a code refactoring, perhaps the suffix could
be quoted automatically but we'd need an option that makes it easier
to indicate that quoting levels should be closed (accumulating them
as completion functions call each other). And I'm not sure about
prefixes.

In the past, I've looked at the compset code when noticing other odd
cases like those Daniel has mentioned. There's loads of edge cases that
are handled with bits of code that all dig into the parser code.
And the parser is not my favourite bit of zsh code either. Trying to fix
them just involves adding more hacks that are only making it worse. A
proper solution would need more flexible values in compqstack.

One other thing I notice in the code for compset -q is that it is
a completely different function internally from that which does the
initial word split when completion starts. I don't entirely understand
why because logically it ought to be possible to reuse the same
code. It'd be nice if vared could be used without the initial parse:
variable values aren't necessarily shell syntax.

Oliver


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: compset -q oddities
  2016-09-14 19:52               ` Oliver Kiddle
@ 2016-09-15  3:08                 ` Bart Schaefer
  0 siblings, 0 replies; 18+ messages in thread
From: Bart Schaefer @ 2016-09-15  3:08 UTC (permalink / raw)
  To: zsh-workers

On Sep 14,  9:52pm, Oliver Kiddle wrote:
} Subject: Re: compset -q oddities
}
} Bart wrote:
} > } That's a bit tricky.  I think it should append an escaped space, e.g.,
} > } "sh -c touc<TAB>" should append <h> <Backslash> <Space>.
} >
} > No, that's *exactly* the kind of DWIM-ing that we abandoned.  If the user
} > wants the space quoted, then he should start with "sh -c 'touc<TAB>".
} 
} For this specific case, I don't entirely agree because in theory the
} mechanics are in place for that to work.

Actually I think you *do* agree -- specifically, that the decision of
whether to quote the space is the responsibility of the completer, not
something that compset -q and compadd should always assume?

} compset -q appears to disable the default suffix that completions have:
} I'm not sure why. Any ideas?

Not offhand.

} Adding calls to compquote in every function that needs a suffix is
} not a good idea. With a code refactoring, perhaps the suffix could
} be quoted automatically but we'd need an option that makes it easier
} to indicate that quoting levels should be closed (accumulating them
} as completion functions call each other). And I'm not sure about
} prefixes.

I think you're just rehashing some of the reasons that we punted this.

} One other thing I notice in the code for compset -q is that it is
} a completely different function internally from that which does the
} initial word split when completion starts.

You're referring to set_comp_sep() ?  I think it's different because
it's simultaneously removing quoting and updating various positions
(pointers/indexes into the buffer), but I could easily be wrong.

} It'd be nice if vared could be used without the initial parse:
} variable values aren't necessarily shell syntax.

There are other cases where it would be helpful to be able to change
the notion of what a "word" is; I think it was Sebastian who wanted
to complete entire lines out of the history.  Trouble is, that means
either wrapper widgets to set up state, or a new class of completion
widgets that get the definition of a word from ... where?


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: compset -q oddities
  2016-09-14 17:56         ` Bart Schaefer
@ 2016-09-15  5:10           ` Daniel Shahaf
  2016-09-16  0:40             ` Bart Schaefer
  0 siblings, 1 reply; 18+ messages in thread
From: Daniel Shahaf @ 2016-09-15  5:10 UTC (permalink / raw)
  To: zsh-workers

Bart Schaefer wrote on Wed, Sep 14, 2016 at 10:56:54 -0700:
> On Sep 13, 11:21am, Peter Stephenson wrote:
> }
> } However, I'm not sure we want to fiddle with this before a release which
> } we really ought to be making imminently as this is just one of a series
> } of incrementally partially fixed problems in this area
> 
> I concur, we can e.g. note these issues in README or somewhere ("the
> calling completer may need to test [[ -n $words[CURRENT] ]] before
> using `compset -q'" or similar verbiage) and move on for now.

I'd write that docs patch but I don't understand what it should say?


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: compset -q oddities
  2016-09-15  5:10           ` Daniel Shahaf
@ 2016-09-16  0:40             ` Bart Schaefer
  2016-09-16  3:05               ` [PATCH] Etc/BUGS: Remove fixed items, add 'compset -q' item from workers/39306 Daniel Shahaf
  0 siblings, 1 reply; 18+ messages in thread
From: Bart Schaefer @ 2016-09-16  0:40 UTC (permalink / raw)
  To: zsh-workers

On Sep 15,  5:10am, Daniel Shahaf wrote:
} Subject: Re: compset -q oddities
}
} Bart Schaefer wrote on Wed, Sep 14, 2016 at 10:56:54 -0700:
} > On Sep 13, 11:21am, Peter Stephenson wrote:
} > }
} > } However, I'm not sure we want to fiddle with this before a release
} > 
} > I concur, we can e.g. note these issues in README or somewhere
} 
} I'd write that docs patch but I don't understand what it should say?

I don't really know what to say either.  It's been the way it is for
more than a decade without anybody noticing, so it could probably go
on this way without any documentation at all.

Or we could just write something in Etc/BUGS, which can simply note
the problem without attempting to explain what to do about it.  Care
to check whether any of the other items in BUGS can be removed?


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH] Etc/BUGS: Remove fixed items, add 'compset -q' item from workers/39306.
  2016-09-16  0:40             ` Bart Schaefer
@ 2016-09-16  3:05               ` Daniel Shahaf
  2016-09-16  5:00                 ` Bart Schaefer
  0 siblings, 1 reply; 18+ messages in thread
From: Daniel Shahaf @ 2016-09-16  3:05 UTC (permalink / raw)
  To: zsh-workers

The first removed hunk is fixed by ZLE_RPROMPT_INDENT.

The next two removed hunks work (on my system, anyway) with current master.
---
 Etc/BUGS | 41 +++++++----------------------------------
 1 file changed, 7 insertions(+), 34 deletions(-)

diff --git a/Etc/BUGS b/Etc/BUGS
index 0547271..eeba0fb 100644
--- a/Etc/BUGS
+++ b/Etc/BUGS
@@ -2,40 +2,6 @@
 KNOWN BUGS IN ZSH
 -----------------
 
-On some terminals, display of lines with exactly 80 characters is
-problematic.  zsh assumes that the terminal does not print an extra
-newline in this case, but some terminals (e.g. aixterm) do.
-------------------------------------------------------------------------
-When interrupting code like the following with ^C:
-  while true; do
-    sh -c '...'
-  done
-if the `sh' is executing, zsh does not know that the sh received a ^C and
-continues with the next iteration.  This happens for any program which
-handles the interrupt, then exits after tidying up; it does not happen for
-zsh, which exits directly from the signal handler.  The workaround is to
-use ^Z which forks the shell and makes the loop a separate job, then kill
-the suspended loop.
-------------------------------------------------------------------------
-If you suspend "man", zle seems to get into cooked mode.  It works ok
-for plain "less".
-It is not specific neither to man nor to zsh.
-E.g. call the following program foo:
-#include <sys/wait.h>
-#include <unistd.h>
-
-int main(int argc, char *argv[])
-{
-    int status;
-
-    if (!fork())	/* child */
-	execvp(argv[1], argv + 1);
-    else		/* parent */
-	wait(&status);
-}
-Then if you suspend
-% foo less something
-from zsh/bash, zle/readline gets into cooked mode.
 ------------------------------------------------------------------------
 The pattern %?* matches names beginning with %? instead of names with at
 least two characters beginning with %. This is a hack to allow %?foo job
@@ -46,3 +12,10 @@ the nonomatch and nullglob options.
 ------------------------------------------------------------------------
 It is currently impossible to time builtins.
 ------------------------------------------------------------------------
+'compset -q' has a byte-counting issue, described in workers/39306:
+
+% compdef _f f
+% _f() { compset -q } 
+% f $'\'<TAB>
+compcore.c:1684: expecting 'x' at offset 2 of "'x"
+------------------------------------------------------------------------


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] Etc/BUGS: Remove fixed items, add 'compset -q' item from workers/39306.
  2016-09-16  3:05               ` [PATCH] Etc/BUGS: Remove fixed items, add 'compset -q' item from workers/39306 Daniel Shahaf
@ 2016-09-16  5:00                 ` Bart Schaefer
  0 siblings, 0 replies; 18+ messages in thread
From: Bart Schaefer @ 2016-09-16  5:00 UTC (permalink / raw)
  To: zsh-workers

Thanks for looking at this.

On Sep 16,  3:05am, Daniel Shahaf wrote:
}
}  ------------------------------------------------------------------------
} +'compset -q' has a byte-counting issue, described in workers/39306:
} +
} +% compdef _f f
} +% _f() { compset -q } 
} +% f $'\'<TAB>
} +compcore.c:1684: expecting 'x' at offset 2 of "'x"
} +------------------------------------------------------------------------

True, but not the bug I was thinking of; that one is entirely invisible
unless the shell was configured with --enable-zsh-debug, and fixing it
would not change the end result, so it'll very seldom be seen.

The bug I meant to mention was the multiple-backslashing of tildes and
leading equal signs following "compadd -q", and consequent inability
to complete them properly.  But it's hard to create a real-world
example of that.


^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2016-09-16  5:00 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-11  7:30 compset -q oddities Daniel Shahaf
2016-09-12  2:14 ` Bart Schaefer
2016-09-12 23:06   ` Daniel Shahaf
2016-09-13  6:28     ` Bart Schaefer
2016-09-13 10:21       ` Peter Stephenson
2016-09-14 17:56         ` Bart Schaefer
2016-09-15  5:10           ` Daniel Shahaf
2016-09-16  0:40             ` Bart Schaefer
2016-09-16  3:05               ` [PATCH] Etc/BUGS: Remove fixed items, add 'compset -q' item from workers/39306 Daniel Shahaf
2016-09-16  5:00                 ` Bart Schaefer
2016-09-14  3:22       ` compset -q oddities Daniel Shahaf
2016-09-14  5:20         ` Bart Schaefer
2016-09-14  6:12           ` Daniel Shahaf
2016-09-14 14:59             ` Bart Schaefer
2016-09-14 19:52               ` Oliver Kiddle
2016-09-15  3:08                 ` Bart Schaefer
2016-09-14  8:31           ` Peter Stephenson
2016-09-14 16:04             ` Bart Schaefer

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).