Hi Anton,
Thanks for providing those links to the past discussion.
Anton Lindqvist <an...@openbsd.org> wrote:
> As I would address this, the numbers of arguments passed to the
> completion related routines is already painfully^W long. I would rather
> take a step back and introduce a `struct globstate' (just an example,
> could be renamed) which includes all the necessary parameters. Getting
> such refactoring in place first would make the diff even smaller.
After thinking about this some more, I think the word length is not
really the right thing to check, even if you account for escape
characters. What we really want to know is whether or not the current
word was modified by completing up to a common prefix.
Here is another (slightly contrived) case where completion is not
performed, even when backslashes are not involved:
Say you have a user somelonguser, whose home directory is /a, and
there are two files in that directory, /a/test1 and /a/test2. ksh
will not complete ~somelonguser/t to /a/test since the expanded
word is shorter than the original.
This example works on vi completion for the same reason as before.
I took a stab at changing do_complete to use a similar approach as
vi completion (diff below), and it is a bit simpler (only modifies
emacs.c and net removal). However, I still have some concerns:
* There is a slight behavior change when you have test1 and test2,
and manually type the entire common prefix. Previously, the options
would be displayed on the first <tab>, but now they are only
displayed on the second <tab>.
* The use of a global variable to track whether the word was modified
since the last completion is a bit messy. vi.c contains a bunch
of `expanded = NONE` scattered throughout the file. It is quite
possible that the two I added to emacs.c are not sufficient for
some editing operations.
Here's another idea for a fix that might address those issues:
* After the call to x_cf_glob, save the part of the buffer between
start and end in some temporary location.
* Always replace start through end with the longest common prefix.
* After replacement, compare the escaped word with the saved original
word. If it matches exactly, print the replacement options.
This would avoid the global variable, but I'm not sure how to
determine the new end position after x_escape() since it just calls
x_do_ins() directly.
Though not quite related, I noticed that x_expand does not call
x_free_words before it returns, so it leaks memory in ATEMP. But
since that gets reclaimed after each command, it's probably not
worth worrying about.
> Also, emacs mode has its own regress suite located in
> regress/bin/ksh/edit/emacs.sh. Tricky logic like this should be covered
> in my opinion. Below is a rough diff how to test file name completion.
I'm using OpenBSD's ksh through oksh, which does not contain the
regress tests. I think I'd have to get an OpenBSD VM up and running
in order to contribute a test case.
> Michael, is this something you would like to continue work on?
I would like to see this issue fixed, but now that I have a local
patch that works for me, my motivation is dwindling. I'm happy to
fix any details with my diff, but if there is substantial refactoring
involved in order to merge, then probably not.
I also have no particular attachment to the diffs I wrote. If anyone
has their own idea about a better solution, feel free to go ahead
with that.
diff --git a/emacs.c b/emacs.c
index 2b70992..2ec4fc4 100644
--- a/emacs.c
+++ b/emacs.c
@@ -130,6 +130,7 @@ static int x_arg_set;
static char *macro_args;
static int prompt_skip;
static int prompt_redraw;
+static int completed;
static int x_ins(char *);
static void x_delete(int, int);
@@ -460,6 +461,7 @@ x_ins(char *s)
if (x_do_ins(s, strlen(s)) < 0)
return -1;
+ completed = 0;
/*
* x_zots() may result in a call to x_adjust()
* we want xcp to reflect the new position.
@@ -566,7 +568,7 @@ x_delete(int nc, int push)
for (cp = x_lastcp(); cp > xcp; )
x_bs(*--cp);
- return;
+ completed = 0;
}
static int
@@ -1749,7 +1751,6 @@ do_complete(int flags, /*
XCF_{COMMAND,FILE,COMMAND_FILE} */
int nwords;
int start, end, nlen, olen;
int is_command;
- int completed = 0;
nwords = x_cf_glob(flags, xbuf, xep - xbuf, xcp - xbuf,
&start, &end, &words, &is_command);
@@ -1759,7 +1760,7 @@ do_complete(int flags, /*
XCF_{COMMAND,FILE,COMMAND_FILE} */
return;
}
- if (type == CT_LIST) {
+ if (type == CT_LIST || type == CT_COMPLIST && completed) {
x_print_expansions(nwords, words, is_command);
x_redraw(0);
x_free_words(nwords, words);
@@ -1769,27 +1770,20 @@ do_complete(int flags, /*
XCF_{COMMAND,FILE,COMMAND_FILE} */
olen = end - start;
nlen = x_longest_prefix(nwords, words);
/* complete */
- if (nwords == 1 || nlen > olen) {
- x_goto(xbuf + start);
- x_delete(olen, false);
- x_escape(words[0], nlen, x_do_ins);
- x_adjust();
- completed = 1;
- }
- /* add space if single non-dir match */
- if (nwords == 1 && words[0][nlen - 1] != '/') {
- x_ins(" ");
- completed = 1;
- }
+ x_goto(xbuf + start);
+ x_delete(olen, false);
+ x_escape(words[0], nlen, x_do_ins);
+ x_adjust();
+ completed = 1;
- if (type == CT_COMPLIST && !completed) {
- x_print_expansions(nwords, words, is_command);
- completed = 1;
+ /* add space if single non-dir match */
+ if (nwords == 1) {
+ completed = 0;
+ if (words[0][nlen - 1] != '/')
+ x_ins(" ");
}
- if (completed)
- x_redraw(0);
-
+ x_redraw(0);
x_free_words(nwords, words);
}
