Page MenuHomeFreeBSD

sh: fix autocompletion for commands that share name with a directory
ClosedPublic

Authored by pstef on Mar 13 2022, 1:12 PM.

Details

Summary

Provide libedit a special function making it always add a space after
the autocompleted command. The default one adds a slash if the word is
also a name of a directory in the current working directory, but this is
wrong for commands.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pstef requested review of this revision.Mar 13 2022, 1:12 PM
pstef created this revision.
jilles requested changes to this revision.Mar 13 2022, 6:21 PM
jilles added inline comments.
bin/sh/expand.c
365 ↗(On Diff #103804)

Passing EXP_SPLIT, EXP_GLOB or EXP_CASE will generate internal representation that the caller will probably not be able to use. Since the (only) caller does not need any flags, I would prefer not having a flags parameter in tilde_expand() at all.

bin/sh/histedit.c
718

Since both tilde_expand and the parser use STARTSTACKSTR and the like to build strings, this needs some extra code to prevent them from interfering. Otherwise, things could go wrong if completion is done while reading a multi-line string or a here-document.

The grabstackstr()/ungrabstackstr() pair suggested by comments in memalloc.c is not attractive here, since it would slow down all parsing (including of scripts) for an interactive feature. Perhaps it is better to make it such that the code called here will call malloc() for a new stack block.

This revision now requires changes to proceed.Mar 13 2022, 6:21 PM

Drop the flags argument from tilde_expand().

bin/sh/expand.c
365 ↗(On Diff #103804)

I knew the only caller didn't make any use of the flags argument, but I thought it might come in handy in the future and it didn't look like much work to make a caller be able to interpret the internal representation.

But I don't have strong feelings about it, it can be revisited if ever needed.

bin/sh/histedit.c
718

Can you help me understand under what circumstances this interactive-mode-only code could interfere with the parser performance (if we added grabstackstr()) or behavior (STARTSTACKSTR use)? Do you mean those could happen during script execution?

bapt requested changes to this revision.Mar 14 2022, 7:54 AM
bapt added inline comments.
bin/sh/histedit.c
718

the ,0 is a left over?

This revision now requires changes to proceed.Mar 14 2022, 7:54 AM

I missed histedit.c in the previous git add.

pstef marked an inline comment as done.
bin/sh/histedit.c
718

Scenario: start sh, type

<<X
some text
~/

then press Tab.

As it is now, readtoken1() in parser.c builds up the text of the here-document in the stack string, using a local variable (out) to point to the location for the next byte. If the line editing code (called from there via pgetc() and variants) also uses the stack string, it will overwrite this data with the tilde-expanded completion word.

Some other code in this situation uses grabstackstr() and ungrabstackstr() to "allocate" the space, so the tilde expansion will not overwrite it. However, doing that here in the straightforward way would involve a grabstackstr()/ungrabstackstr() pair for most bytes parsed by the shell, greatly slowing down the parser (I have not benchmarked this). Making out a global variable is also likely to lead to a measurable slowdown. An approach that may work is to pierce the abstraction and only grabstackstr()/ungrabstackstr() when pgetc_macro() cannot obtain a byte from the buffer.

Another approach may be to move the problem entirely to the completion code, and have that save and restore the state of the stack string so it does not interfere with the parser. Since it is not known how much space readtoken1() is using in the current block (tracked in the local variable out), the completion code cannot use the block at all. I have not thought out the details of this, but it seems most promising.

Fortunately, the tilde-expanding function from libedit is nicely exported. Too bad this code is duplicated between sh and libedit.

This revision is now accepted and ready to land.Mar 27 2022, 8:55 PM