gosh: clean up autocomplete, courtesy claude.ai#3487
gosh: clean up autocomplete, courtesy claude.ai#3487rminnich wants to merge 1 commit intou-root:mainfrom
Conversation
The autocomplete with bubbletea leads to lots of display probelms, making it almost unusable, esp. in qemu. This changes the completer to use liner, which is an older package. It does not appear to be maintained, so if we really want it, I suggest we pull the package into u-root. Liner is very small. This is an experiment. But we have to do something. Bubbletea is unusable. Signed-off-by: Ronald G. Minnich <rminnich@gmail.com>
rminnich
requested review from
10000TB,
abarisani,
andrewsun2898,
binjip978,
brho,
Copilot and
orangecms
|
note that it fails the autocomplete test. Consider this a WIP. |
There was a problem hiding this comment.
Pull request overview
This PR replaces the bubbletea-based autocomplete implementation with the liner library to resolve display issues, particularly in qemu environments. The change modifies the interactive shell implementation to use a simpler, more stable input library.
Changes:
- Switched from bubbline to liner library for interactive input handling
- Rewrote the
runInteractivefunction with multi-line input buffering - Added new helper functions
getPathCommands()andcompleteFiles()for tab completion
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "github.com/knz/bubbline/complete" | ||
| "github.com/knz/bubbline/computil" | ||
| "github.com/knz/bubbline/editline" |
There was a problem hiding this comment.
These bubbline imports are no longer used after switching to liner. The code now uses liner for interactive input instead of bubbline. These unused imports should be removed.
| // getPathCommands returns a list of all executable commands in PATH | ||
| func getPathCommands() []string { | ||
| pathEnv := os.Getenv("PATH") | ||
| if pathEnv == "" { | ||
| return nil | ||
| } | ||
|
|
||
| paths := filepath.SplitList(pathEnv) | ||
| commandSet := make(map[string]bool) | ||
|
|
||
| for _, dir := range paths { | ||
| entries, err := os.ReadDir(dir) | ||
| if err != nil { | ||
| continue | ||
| default: | ||
| } | ||
|
|
||
| if line != "" { | ||
| if err := input.AddHistory(line); err != nil { | ||
| fmt.Fprintf(stdout, "unable to add %s to history: %v\n", line, err) | ||
| for _, entry := range entries { | ||
| if entry.IsDir() { | ||
| continue | ||
| } | ||
|
|
||
| // Check if executable | ||
| info, err := entry.Info() | ||
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| mode := info.Mode() | ||
| if mode&0111 != 0 { // Has any execute bit set | ||
| commandSet[entry.Name()] = true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if err := parser.Stmts(strings.NewReader(line), func(stmt *syntax.Stmt) bool { | ||
| if parser.Incomplete() { | ||
| fmt.Fprintf(stdout, "-> ") | ||
| return true | ||
| // Convert set to sorted slice | ||
| commands := make([]string, 0, len(commandSet)) | ||
| for cmd := range commandSet { | ||
| commands = append(commands, cmd) | ||
| } | ||
|
|
||
| return commands | ||
| } | ||
|
|
||
| // completeFiles returns file/directory completions for the given prefix | ||
| func completeFiles(prefix string) []string { | ||
| // Handle empty prefix - complete in current directory | ||
| if prefix == "" { | ||
| prefix = "./" | ||
| } | ||
|
|
||
| // Expand home directory | ||
| if strings.HasPrefix(prefix, "~/") { | ||
| home, err := os.UserHomeDir() | ||
| if err == nil { | ||
| prefix = filepath.Join(home, prefix[2:]) | ||
| } | ||
| } | ||
|
|
||
| // Get directory and file prefix | ||
| dir := filepath.Dir(prefix) | ||
| filePrefix := filepath.Base(prefix) | ||
|
|
||
| // If prefix ends with /, we want to complete in that directory | ||
| if strings.HasSuffix(prefix, "/") { | ||
| dir = prefix | ||
| filePrefix = "" | ||
| } | ||
|
|
||
| // Read directory entries | ||
| entries, err := os.ReadDir(dir) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
|
|
||
| var matches []string | ||
| for _, entry := range entries { | ||
| name := entry.Name() | ||
|
|
||
| // Skip hidden files unless explicitly requested | ||
| if !strings.HasPrefix(filePrefix, ".") && strings.HasPrefix(name, ".") { | ||
| continue | ||
| } | ||
|
|
||
| // Check if name matches prefix | ||
| if strings.HasPrefix(name, filePrefix) { | ||
| // Build full path | ||
| var fullPath string | ||
| if dir == "." { | ||
| fullPath = name | ||
| } else { | ||
| fullPath = filepath.Join(dir, name) | ||
| } | ||
|
|
||
| runErr = runner.Run(context.Background(), stmt) | ||
| return !runner.Exited() | ||
| }); err != nil { | ||
| fmt.Fprintf(stderr, "error: %s\n", err.Error()) | ||
| // Add trailing slash for directories | ||
| if entry.IsDir() { | ||
| fullPath += "/" | ||
| } | ||
|
|
||
| matches = append(matches, fullPath) | ||
| } | ||
| } | ||
| exit: | ||
| return nil | ||
|
|
||
| return matches | ||
| } |
There was a problem hiding this comment.
The getPathCommands and completeFiles functions duplicate existing functionality already available in completer_common.go (commandCompleter and filepathCompleter). This creates code duplication and maintenance burden. Consider reusing the existing functions instead of reimplementing them.
| // Set up completion with both commands and files | ||
| line.SetCompleter(func(input string) []string { | ||
| // Find the position where we should insert completions | ||
| words := strings.Fields(input) |
There was a problem hiding this comment.
Using strings.Fields to parse shell input is problematic because it splits on all whitespace and doesn't handle quoted strings. For example, 'echo "hello world"' would be split into 3 words instead of 2. This will cause incorrect completion behavior when users have quoted arguments. Consider using a proper shell tokenizer or the syntax parser to determine word boundaries.
|
|
||
| //``` | ||
| // | ||
| //## Key Changes | ||
| // | ||
| //1. **`getPathCommands()`** - Scans all directories in `$PATH` and finds executable files | ||
| //2. **Caches commands** - Only scans PATH once at startup for performance | ||
| //3. **Checks execute permissions** - Only includes files with execute bit set | ||
| //4. **Deduplicates** - Uses a map to handle commands that appear in multiple PATH directories | ||
| // | ||
| //## Features | ||
| // | ||
| //- Completes commands from `$PATH` for the first argument | ||
| //- Completes files/directories for subsequent arguments or paths with `/` | ||
| //- Fast completion (PATH scanned only once) | ||
| //- Works with any command in your PATH | ||
| // | ||
| //Example: | ||
| //``` | ||
| //$ gre<TAB> → completes to grep, grep-changelog, etc. | ||
| //$ /usr/bi<TAB> → completes files in /usr/bin/ | ||
| //$ ls Do<TAB> → completes to Documents/, Downloads/, etc. |
There was a problem hiding this comment.
These lines of comment markup should not be part of the source code. They appear to be leftover documentation or notes that were accidentally included. Remove lines 325-345 as they are not valid Go code and will likely cause compilation errors.
| //``` | |
| // | |
| //## Key Changes | |
| // | |
| //1. **`getPathCommands()`** - Scans all directories in `$PATH` and finds executable files | |
| //2. **Caches commands** - Only scans PATH once at startup for performance | |
| //3. **Checks execute permissions** - Only includes files with execute bit set | |
| //4. **Deduplicates** - Uses a map to handle commands that appear in multiple PATH directories | |
| // | |
| //## Features | |
| // | |
| //- Completes commands from `$PATH` for the first argument | |
| //- Completes files/directories for subsequent arguments or paths with `/` | |
| //- Fast completion (PATH scanned only once) | |
| //- Works with any command in your PATH | |
| // | |
| //Example: | |
| //``` | |
| //$ gre<TAB> → completes to grep, grep-changelog, etc. | |
| //$ /usr/bi<TAB> → completes files in /usr/bin/ | |
| //$ ls Do<TAB> → completes to Documents/, Downloads/, etc. |
| line.SetCompleter(func(input string) []string { | ||
| // Find the position where we should insert completions | ||
| words := strings.Fields(input) | ||
| if len(words) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| var runErr error | ||
| // Get the last word being typed | ||
| lastWord := words[len(words)-1] | ||
|
|
||
| // Calculate prefix (everything before the last word) | ||
| lastWordStart := strings.LastIndex(input, lastWord) | ||
| prefix := "" | ||
| if lastWordStart > 0 { | ||
| prefix = input[:lastWordStart] | ||
| } | ||
|
|
||
| var matches []string | ||
|
|
||
| // If it's the first word and doesn't contain /, complete with commands from PATH | ||
| if len(words) == 1 && !strings.Contains(lastWord, "/") { | ||
| for _, cmd := range pathCommands { | ||
| if strings.HasPrefix(cmd, lastWord) { | ||
| matches = append(matches, prefix+cmd) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // The following code is used to intercept SIGINT signals. | ||
| // Calling signal.Ignore wouldn't work as child prcesses inherit this trait. | ||
| // We only want to catch SIGINTs that are propagated from a child, | ||
| // the child itself should get the signal as per usual. | ||
| // Always try file completion for paths | ||
| fileMatches := completeFiles(lastWord) | ||
| for _, match := range fileMatches { | ||
| matches = append(matches, prefix+match) | ||
| } | ||
|
|
||
| return matches | ||
| }) |
There was a problem hiding this comment.
The completion flag is no longer checked or used. The new implementation always sets up a completer regardless of the flag value, breaking the intended behavior where users could disable completion. Either check the flag before calling SetCompleter or remove the flag if it's no longer needed.
| // Handle empty prefix - complete in current directory | ||
| if prefix == "" { | ||
| prefix = "./" | ||
| } | ||
|
|
||
| // Expand home directory | ||
| if strings.HasPrefix(prefix, "~/") { | ||
| home, err := os.UserHomeDir() | ||
| if err == nil { | ||
| prefix = filepath.Join(home, prefix[2:]) | ||
| } | ||
| } | ||
|
|
||
| // Get directory and file prefix | ||
| dir := filepath.Dir(prefix) | ||
| filePrefix := filepath.Base(prefix) | ||
|
|
||
| // If prefix ends with /, we want to complete in that directory | ||
| if strings.HasSuffix(prefix, "/") { | ||
| dir = prefix | ||
| filePrefix = "" |
There was a problem hiding this comment.
When prefix is empty, setting it to "./" will cause completions to always include the "./" prefix, which may not match user expectations. For example, if the user types nothing and presses tab, they'll see "./file" instead of just "file". The existing filepathCompleter in completer_common.go handles this case better.
| // Handle empty prefix - complete in current directory | |
| if prefix == "" { | |
| prefix = "./" | |
| } | |
| // Expand home directory | |
| if strings.HasPrefix(prefix, "~/") { | |
| home, err := os.UserHomeDir() | |
| if err == nil { | |
| prefix = filepath.Join(home, prefix[2:]) | |
| } | |
| } | |
| // Get directory and file prefix | |
| dir := filepath.Dir(prefix) | |
| filePrefix := filepath.Base(prefix) | |
| // If prefix ends with /, we want to complete in that directory | |
| if strings.HasSuffix(prefix, "/") { | |
| dir = prefix | |
| filePrefix = "" | |
| var dir string | |
| var filePrefix string | |
| if prefix == "" { | |
| // Empty prefix: complete in current directory with no initial filename prefix. | |
| dir = "." | |
| filePrefix = "" | |
| } else { | |
| // Expand home directory | |
| if strings.HasPrefix(prefix, "~/") { | |
| home, err := os.UserHomeDir() | |
| if err == nil { | |
| prefix = filepath.Join(home, prefix[2:]) | |
| } | |
| } | |
| // Get directory and file prefix | |
| dir = filepath.Dir(prefix) | |
| filePrefix = filepath.Base(prefix) | |
| // If prefix ends with /, we want to complete in that directory | |
| if strings.HasSuffix(prefix, "/") { | |
| dir = prefix | |
| filePrefix = "" | |
| } |
| lastWord := words[len(words)-1] | ||
|
|
||
| // Calculate prefix (everything before the last word) | ||
| lastWordStart := strings.LastIndex(input, lastWord) |
There was a problem hiding this comment.
Using strings.LastIndex to find the position of the last word is unreliable when the same word appears multiple times in the input. For example, in "echo test test", when completing the second "test", this would find the first occurrence instead. This will cause the prefix to be calculated incorrectly.
| } | ||
| // Set up liner | ||
| line := liner.NewLiner() | ||
| defer line.Close() |
There was a problem hiding this comment.
History persistence is not implemented with liner. The old bubbline code loaded and auto-saved history using HistFile, but this functionality was removed. Users will lose their command history between sessions. Consider adding line.ReadHistory(HistFile) after creating the liner instance and line.WriteHistory(HistFile) before returning or in a defer statement.
| defer line.Close() | |
| _ = line.ReadHistory(HistFile) | |
| defer func() { | |
| _ = line.WriteHistory(HistFile) | |
| line.Close() | |
| }() |
| // Find the position where we should insert completions | ||
| words := strings.Fields(input) | ||
| if len(words) == 0 { | ||
| return nil |
There was a problem hiding this comment.
When the input is empty or contains only whitespace, strings.Fields returns an empty slice, but the user might want to complete commands from PATH. The current implementation returns nil for empty input. Consider providing command completions even when input is empty to improve user experience.
| return nil | |
| // When the input is empty or whitespace, offer command completions from PATH. | |
| return append([]string(nil), pathCommands...) |
| for _, dir := range paths { | ||
| entries, err := os.ReadDir(dir) | ||
| if err != nil { | ||
| continue | ||
| default: | ||
| } | ||
|
|
||
| if line != "" { | ||
| if err := input.AddHistory(line); err != nil { | ||
| fmt.Fprintf(stdout, "unable to add %s to history: %v\n", line, err) | ||
| for _, entry := range entries { | ||
| if entry.IsDir() { | ||
| continue | ||
| } | ||
|
|
||
| // Check if executable | ||
| info, err := entry.Info() | ||
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| mode := info.Mode() | ||
| if mode&0111 != 0 { // Has any execute bit set | ||
| commandSet[entry.Name()] = true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Calling os.ReadDir for every directory in PATH and then calling entry.Info() for each file is inefficient compared to the existing commandCompleter implementation which uses filepath.WalkDir. Additionally, this function doesn't handle subdirectories, meaning it won't find executables in PATH directories that contain subdirectories. The existing commandCompleter implementation is more robust.
|
I'm enjoying watching copilot critique Claude's code :-) |
see if claude has any good responses to copilot's critiques! |
|
before I go any further I want to see how people feel about removing bubbline. Having tried to use it, and having dealt with complaints from users, I see no downside. |
|
git@github.com:reeflective/readline.git (looks simpler and hence possibly preferred) |
2 participants
The autocomplete with bubbletea leads to lots of display probelms, making it almost unusable, esp. in qemu.
This changes the completer to use liner, which is an older package. It does not appear to be maintained, so if we really want it, I suggest we pull the package into u-root. Liner is very small.
This is an experiment. But we have to do something. Bubbletea is unusable.