Discussion:
ksh autocomplete
Ilya Kaliman
2017-07-01 04:50:12 UTC
Permalink
Hi!

The tab-autocomplete does not seem to work for some strings in ksh.
How to reproduce:

mkdir "a a" "(bbb)" "(c c)"
cd a\ a && mkdir abc{1,2,3} && cd ..
cd \(bbb\) && mkdir abc{1,2,3} && cd ..
cd \(c\ c\) && mkdir abc{1,2,3} && cd ..

type "cd a\ a/" hit tab -> auto-completes to abc, offers abc1 abc2 abc3
type "cd \(bbb\)/" hit tab -> auto-completes to abc, offers abc1 abc2 abc3
type "cd \(c\ c\)/" hit tab -> does not autocomplete; expected - same
as previous.

Thanks,
Ilya
Klemens Nanni
2017-07-01 06:04:33 UTC
Permalink
Post by Ilya Kaliman
The tab-autocomplete does not seem to work for some strings in ksh.
mkdir "a a" "(bbb)" "(c c)"
cd a\ a && mkdir abc{1,2,3} && cd ..
cd \(bbb\) && mkdir abc{1,2,3} && cd ..
cd \(c\ c\) && mkdir abc{1,2,3} && cd ..
type "cd a\ a/" hit tab -> auto-completes to abc, offers abc1 abc2 abc3
type "cd \(bbb\)/" hit tab -> auto-completes to abc, offers abc1 abc2 abc3
type "cd \(c\ c\)/" hit tab -> does not autocomplete; expected - same
as previous.
$ mkdir "(c c)"/abc{1,2,3}
$ cd \(c\ c\)/<Tab>
cd \(c\ c\)/abc<Tab>
abc1/ abc2/ abc3/

Works just fine here.
Anton Lindqvist
2017-07-02 20:46:21 UTC
Permalink
Hi,
Post by Ilya Kaliman
Hi!
The tab-autocomplete does not seem to work for some strings in ksh.
mkdir "a a" "(bbb)" "(c c)"
cd a\ a && mkdir abc{1,2,3} && cd ..
cd \(bbb\) && mkdir abc{1,2,3} && cd ..
cd \(c\ c\) && mkdir abc{1,2,3} && cd ..
type "cd a\ a/" hit tab -> auto-completes to abc, offers abc1 abc2 abc3
type "cd \(bbb\)/" hit tab -> auto-completes to abc, offers abc1 abc2 abc3
type "cd \(c\ c\)/" hit tab -> does not autocomplete; expected - same
as previous.
Nice catch. As I see it, the bug you're seeing happens inside
do_complete() while comparing the length of the input buffer and the
results from the completion. Since the completions are passed through
expand() causing escaped characters to be unescaped to their literal
form while the input buffer remains escaped. You managed to find the
perfect set of input causing the condition to when a completion
succeeded to be invalidated:

olen = strlen("\(c\ c\)/") = 9;
nlen = strlen("(c c)/abc") = 9;

Since `olen == nlen` no completion is performed. Please try out the diff
below in which slashes are discarded when comparing the length. I don't
know if any other character should be discarded as well, if true then it
might be worth passing the input buffer through ksh's own lexer and
parser in order to properly handle special characters, just like in
x_file_glob().

Comments? OK?

Index: emacs.c
===================================================================
RCS file: /cvs/src/bin/ksh/emacs.c,v
retrieving revision 1.70
diff -u -p -r1.70 emacs.c
--- emacs.c 25 Jun 2017 17:28:39 -0000 1.70
+++ emacs.c 2 Jul 2017 20:43:00 -0000
@@ -1754,6 +1754,7 @@ do_complete(int flags, /* XCF_{COMMAND,F
char **words;
int nwords;
int start, end, nlen, olen;
+ int i, ndiscard;
int is_command;
int completed = 0;

@@ -1773,9 +1774,13 @@ do_complete(int flags, /* XCF_{COMMAND,F
}

olen = end - start;
+ ndiscard = 0;
+ for (i = start; i < end; i++)
+ if (xbuf[i] == '\\')
+ ndiscard++;
nlen = x_longest_prefix(nwords, words);
/* complete */
- if (nwords == 1 || nlen > olen) {
+ if (nwords == 1 || nlen > olen - ndiscard) {
x_goto(xbuf + start);
x_delete(olen, false);
x_escape(words[0], nlen, x_do_ins);
Klemens Nanni
2017-07-02 21:36:39 UTC
Permalink
Post by Anton Lindqvist
Hi,
Post by Ilya Kaliman
Hi!
The tab-autocomplete does not seem to work for some strings in ksh.
mkdir "a a" "(bbb)" "(c c)"
cd a\ a && mkdir abc{1,2,3} && cd ..
cd \(bbb\) && mkdir abc{1,2,3} && cd ..
cd \(c\ c\) && mkdir abc{1,2,3} && cd ..
type "cd a\ a/" hit tab -> auto-completes to abc, offers abc1 abc2 abc3
type "cd \(bbb\)/" hit tab -> auto-completes to abc, offers abc1 abc2 abc3
type "cd \(c\ c\)/" hit tab -> does not autocomplete; expected - same
as previous.
Nice catch. As I see it, the bug you're seeing happens inside
do_complete() while comparing the length of the input buffer and the
results from the completion. Since the completions are passed through
expand() causing escaped characters to be unescaped to their literal
form while the input buffer remains escaped. You managed to find the
perfect set of input causing the condition to when a completion
olen = strlen("\(c\ c\)/") = 9;
nlen = strlen("(c c)/abc") = 9;
Since `olen == nlen` no completion is performed. Please try out the diff
below in which slashes are discarded when comparing the length. I don't
know if any other character should be discarded as well, if true then it
might be worth passing the input buffer through ksh's own lexer and
parser in order to properly handle special characters, just like in
x_file_glob().
So I tried reproducing this in both vi and emacs mode but with no avail,
here's the typescript showing successfull auto-completion for directory
names in "(c c)" (^U is used to clear the line):

Script started on Sun Jul 2 23:24:06 2017
$ mkdir -p "(c c)"/abc{1,2,3}
$ cd \(c\ c\)/abc
abc1/ abc2/ abc3/
$ cd \(c\ c\)/abc $ $ set -o emacs
$ cd \(c\ c\)/abc
abc1/ abc2/ abc3/
$ cd \(c\ c\)/abc 
$ ^D

Script done on Sun Jul 2 23:25:13 2017

What are you guys doing differently that triggers this bug?
Ilya Kaliman
2017-07-03 20:41:37 UTC
Permalink
Hi,

The patch fixes this for me. OK.

Ilya
Post by Anton Lindqvist
Hi,
Post by Ilya Kaliman
Hi!
The tab-autocomplete does not seem to work for some strings in ksh.
mkdir "a a" "(bbb)" "(c c)"
cd a\ a && mkdir abc{1,2,3} && cd ..
cd \(bbb\) && mkdir abc{1,2,3} && cd ..
cd \(c\ c\) && mkdir abc{1,2,3} && cd ..
type "cd a\ a/" hit tab -> auto-completes to abc, offers abc1 abc2 abc3
type "cd \(bbb\)/" hit tab -> auto-completes to abc, offers abc1 abc2 abc3
type "cd \(c\ c\)/" hit tab -> does not autocomplete; expected - same
as previous.
Nice catch. As I see it, the bug you're seeing happens inside
do_complete() while comparing the length of the input buffer and the
results from the completion. Since the completions are passed through
expand() causing escaped characters to be unescaped to their literal
form while the input buffer remains escaped. You managed to find the
perfect set of input causing the condition to when a completion
olen = strlen("\(c\ c\)/") = 9;
nlen = strlen("(c c)/abc") = 9;
Since `olen == nlen` no completion is performed. Please try out the diff
below in which slashes are discarded when comparing the length. I don't
know if any other character should be discarded as well, if true then it
might be worth passing the input buffer through ksh's own lexer and
parser in order to properly handle special characters, just like in
x_file_glob().
Comments? OK?
Index: emacs.c
===================================================================
RCS file: /cvs/src/bin/ksh/emacs.c,v
retrieving revision 1.70
diff -u -p -r1.70 emacs.c
--- emacs.c 25 Jun 2017 17:28:39 -0000 1.70
+++ emacs.c 2 Jul 2017 20:43:00 -0000
@@ -1754,6 +1754,7 @@ do_complete(int flags, /* XCF_{COMMAND,F
char **words;
int nwords;
int start, end, nlen, olen;
+ int i, ndiscard;
int is_command;
int completed = 0;
@@ -1773,9 +1774,13 @@ do_complete(int flags, /* XCF_{COMMAND,F
}
olen = end - start;
+ ndiscard = 0;
+ for (i = start; i < end; i++)
+ if (xbuf[i] == '\\')
+ ndiscard++;
nlen = x_longest_prefix(nwords, words);
/* complete */
- if (nwords == 1 || nlen > olen) {
+ if (nwords == 1 || nlen > olen - ndiscard) {
x_goto(xbuf + start);
x_delete(olen, false);
x_escape(words[0], nlen, x_do_ins);
Anton Lindqvist
2017-07-05 06:34:10 UTC
Permalink
Ping, looking for OKs.
Post by Anton Lindqvist
Hi,
Post by Ilya Kaliman
Hi!
The tab-autocomplete does not seem to work for some strings in ksh.
mkdir "a a" "(bbb)" "(c c)"
cd a\ a && mkdir abc{1,2,3} && cd ..
cd \(bbb\) && mkdir abc{1,2,3} && cd ..
cd \(c\ c\) && mkdir abc{1,2,3} && cd ..
type "cd a\ a/" hit tab -> auto-completes to abc, offers abc1 abc2 abc3
type "cd \(bbb\)/" hit tab -> auto-completes to abc, offers abc1 abc2 abc3
type "cd \(c\ c\)/" hit tab -> does not autocomplete; expected - same
as previous.
Nice catch. As I see it, the bug you're seeing happens inside
do_complete() while comparing the length of the input buffer and the
results from the completion. Since the completions are passed through
expand() causing escaped characters to be unescaped to their literal
form while the input buffer remains escaped. You managed to find the
perfect set of input causing the condition to when a completion
olen = strlen("\(c\ c\)/") = 9;
nlen = strlen("(c c)/abc") = 9;
Since `olen == nlen` no completion is performed. Please try out the diff
below in which slashes are discarded when comparing the length. I don't
know if any other character should be discarded as well, if true then it
might be worth passing the input buffer through ksh's own lexer and
parser in order to properly handle special characters, just like in
x_file_glob().
Comments? OK?
Index: emacs.c
===================================================================
RCS file: /cvs/src/bin/ksh/emacs.c,v
retrieving revision 1.70
diff -u -p -r1.70 emacs.c
--- emacs.c 25 Jun 2017 17:28:39 -0000 1.70
+++ emacs.c 2 Jul 2017 20:43:00 -0000
@@ -1754,6 +1754,7 @@ do_complete(int flags, /* XCF_{COMMAND,F
char **words;
int nwords;
int start, end, nlen, olen;
+ int i, ndiscard;
int is_command;
int completed = 0;
@@ -1773,9 +1774,13 @@ do_complete(int flags, /* XCF_{COMMAND,F
}
olen = end - start;
+ ndiscard = 0;
+ for (i = start; i < end; i++)
+ if (xbuf[i] == '\\')
+ ndiscard++;
nlen = x_longest_prefix(nwords, words);
/* complete */
- if (nwords == 1 || nlen > olen) {
+ if (nwords == 1 || nlen > olen - ndiscard) {
x_goto(xbuf + start);
x_delete(olen, false);
x_escape(words[0], nlen, x_do_ins);
Ingo Schwarze
2017-07-05 12:59:48 UTC
Permalink
Hi,
Post by Anton Lindqvist
Ping, looking for OKs.
I'm not going to even read the patch.

I consider autocompletion vastly overengineered and my opinion
is that it ought to be radically simplified. For example,
never do autocompletion if there is any character in the word
that is neither alnum nor '/', '.', or '_'. Yes, that implies
never doing autocompletion in words containing non-ASCII
characters.

In my opinion, autocompletion is a feature where KISS is even
more important than elsewhere. I *REALLY* don't want to end
up in a position where something as bells-and-whistle-like
and as utterly unimportant as autocompletion causes any serious
bug, least of all in a program as critical as the shell.

That said, if others want to OK it, i won't oppose a commit.

If somebody sends diffs to simplify autocompletion code,
i *will* read them.

Yours,
Ingo
Ingo Schwarze
2017-07-05 13:05:06 UTC
Permalink
In particular, never attempt autocompletion if there is any shell
meta-character in the vicinity.

Loading...