[WIP] Review Fixes, Stabilty and live testing results#5
Merged
Conversation
- Add vms.termcap.c (fetched from upstream tcsh-org/tcsh); compiles to an empty translation unit on BSD/Linux, but is required by Makefile.in - Add dch-template.in (proper Debian changelog template, adapted from upstream tcsh with mcsh branding) - configure.ac: remove erroneous '##0' strip from PACKAGE_PATCHLEVEL derivation; for version x.y.0 this produced an empty macro, causing compile errors in tc.vers.c - configure.ac: fix TCSH_BASELINE_VERSION AC_DEFINE_UNQUOTED to expand the literal version string rather than the macro name - Commit generated autoconf outputs (configure, aclocal.m4, m4/, acaux/, config.h.in, nls/catgen) so the repo is buildable without requiring autoconf/automake to be installed - Add alacritty.toml (sets mcsh as the terminal shell for testing) 💘 Generated with Crush Assisted-by: Claude Sonnet 4.5 via Crush <crush@charm.land>
The version string format included '[tcsh baseline N.N.N]'; the square brackets are glob metacharacters. set1() in sh.set.c always globs the value vector unless VAR_NOGLOB is set, so on startup fix_version() was calling setcopy(STRversion, ..., VAR_READWRITE) with a string containing '[...]', tglob() flagged it as a pattern, globall() found no matching files, and the shell immediately died with 'No match'. Upstream tcsh's version string never contains glob metacharacters, so this regression was introduced when mcsh added the '[tcsh baseline ...]' annotation. Replace the bracketed annotation with the brace-free form 'tcsh-baseline:N.N.N', consistent with the colon-separated option flags that follow it. 💘 Generated with Crush Assisted-by: Claude Sonnet 4.5 via Crush <crush@charm.land>
- sh.sem.c: restore upstream Dfix() call for all commands including doif/dowhile; mcsh was skipping dollar substitution for expression builtins, breaking variable comparisons like `if ($x > 3)`. Also move bifunc assignment to correct location. - sh.exp.c: restore expr() to upstream's simple `return exp0(vp, 0)`; mcsh had replaced it with a complex copy-and-echo version causing incorrect token advancement. Remove double-expansion path in exp6() that caused Dfix1/globone to run on already-processed tokens. - sh.func.c: fix search() TC_IF nesting level to only increment for `if...then` constructs, not bare `if cmd` forms, matching upstream behaviour. - sh.set.c: guard getn() against empty string (ignored expression arms return STRNULL for short-circuit evaluation), returning 0 instead of ERR_BADNUM. 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
Add ghost-text autosuggestion sourced from command history: - ed.h: add GhostBuf[INBUFSIZE] global for the predicted suffix - ed.chared.c: add predict_from_history() which walks Histlist forward looking for an entry whose prefix matches the current InputBuf; populates GhostBuf with the untyped suffix. Add e_predict_accept() command which, when the cursor is at end-of-line and GhostBuf is non-empty, splices the ghost into InputBuf and clears it; otherwise falls through to e_charfwd. - ed.defns.c: register e_predict_accept as F_PREDICT_ACCEPT (120), bump F_NUM_FNS to 121, add FuncNames entry predict-accept. - ed.decls.h: declare e_predict_accept and predict_from_history. - ed.inputl.c: call predict_from_history() after F_INSERT / F_PREDICT_ACCEPT returns CC_NORM; clear GhostBuf for all other commands. - ed.refresh.c: after cursor is repositioned at end of Refresh(), write ghost chars with ANSI dim, reset, then move cursor back so the visible cursor stays at the insertion point. - ed.screen.c: bind right-arrow to F_PREDICT_ACCEPT by default; falls through to charfwd when no ghost is active so existing behaviour is preserved. 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
Two bugs fixed: - Delayed display: e_insert used RefPlusOne() (fast one-char path) which bypassed Refresh() entirely, so ghost text appeared one keystroke late. Fix: call predict_from_history() in e_insert() before RefPlusOne/Refresh, and extract DrawGhost() as a static helper called by both Refresh() and RefPlusOne() so ghost renders on the fast path too. - Ghost persisted through backspace/movement: GhostBuf was only cleared inside CC_NORM after Refresh() had already fired, so the old ghost was briefly visible. Fix: clear GhostBuf immediately after LastCmd is set (before the switch/Refresh) for any command that is not F_INSERT or F_PREDICT_ACCEPT. 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
Add two new prompt escape sequences to tc.prompt.c:
%g - current git branch name (or 7-char detached HEAD hash).
Outputs nothing outside a git worktree.
%G - same as %g, but appends |STATE when a git operation is in
progress (MERGING, REBASING-i, REBASING, AM, CHERRY-PICKING,
REVERTING, BISECTING).
Detection walks up from $cwd reading .git/HEAD directly - no
subprocesses are spawned. Results are cached per CWD pointer using
the same pattern as %~, so there are no extra filesystem hits per
keystroke. Bare repos and interactive rebase (reads head-name) are
also handled.
Update dot.mcshrc to set rprompt='%S%G%s' as a default example,
showing the branch in standout on the right side of the prompt.
💘 Generated with Crush
Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
acaux/install-sh
- Fix case patterns to use glob (*) so names like "=foo" or "(tmp"
are prefixed with ./ (three sites: -t dst_arg, loop dst_arg, src)
alacritty.toml
- Replace hardcoded /home/orpheus497/... path with bare "mcsh" so
the shell resolves it via PATH
- Comment out pywal import; document optional local override path
configure.ac
- TCSH_BASELINE_VERSION: use TCSH_VERSION M4 macro instead of
duplicated literal "6.24.13"
- PACKAGE_PATCHLEVEL: normalize via printf %d to strip leading zeros
that would produce invalid C integer literals (e.g. "08")
dch-template.in
- Change distribution from "unstable" to UNRELEASED
- Replace placeholder changelog line with substantive release notes
ed.chared.c
- e_predict_accept: write NUL terminator after copy loop so input
buffer invariant is preserved
- predict_from_history: stop ghost copy at \n/\r so no trailing
newline leaks into GhostBuf
- e_digit: call predict_from_history() for numeric inserts so ghost
text updates when digits are typed
ed.defns.c
- predict-accept: change CSAVS catalog id from 122 (collision with
newline-and-hold) to 124 (unused)
ed.inputl.c
- Only call Refresh() when GhostBuf is actually non-empty before
clearing, so stale ghost text is erased from screen
ed.refresh.c (DrawGhost)
- Track prev_ghost_cols statically; erase previous overlay with
spaces+backspaces before drawing new ghost text, fixing stale tail
chars on backspace/navigation
- Use backspace-based cursor return instead of raw ESC[nD sequence
m4/lib-prefix.m4
- Fix "dn;" typo to "dnl" so comment is not leaked into configure
m4/po.m4
- Fix C# DLL error-cleanup to remove the actual failed target
(frobbedlang/DOMAIN.resources.dll) not the unrelated .msg file
- Bump GETTEXT_MACRO_VERSION from 0.22 to 0.23 to match file header
vms.termcap.c
- tgetnum/tgetflag/tgetstr: guard initial ':' scan against OOB read
on malformed entries (while *cp && *cp != ':' instead of *++cp)
- tsetup: change sscanf format to %[^|:] and strcmp for exact match,
preventing "vt" from matching "vt100"
- tsetup: fix continuation-line buffer overflow by computing
remaining capacity and passing it to fgets
- tgetstr octal: parse at most 3 octal digits (no unbounded loop);
fix default/unknown-escape path to write character to **area
- tgoto: enlarge ret[] to 64 bytes; add bounds checks on all *rp++
writes; use snprintf for %d so multi-digit coordinates work
💘 Generated with Crush
Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
Add `set color` to dot.mcshrc so ls-F completion output and filename coloring are active by default. Without this variable set, tw.color.c never activates color_context_lsmF regardless of LS_COLORS/LSCOLORS. Also verified all paste_1.txt review items against current code: - sh.func.c doif expr/int: correct (upstream pattern, intentional) - sh.sem.c Dfix: correct (restored in 1d93e19, intentional) - sh.set.c getn empty: already returns 0 with comment - sh.exp.c exp6 globone: TEXP_NOGLOB guard already present 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
Merge user's .tcshrc configuration into dot.mcshrc, adopting the same section-based architecture and comment style. Key changes: - Full Wayland/GPU env vars, path/editor/pager setup - set color (filetype coloring in tab completion) - set symlinks=chase, histdup=erase, history=10000 - Full keybinding set (arrow history search, Ctrl+Arrow, Home/End) - Complete programmable completion block (cc/make/man/service/ifconfig/tar) - Full alias set (ls -F -G, ll, df, du, .., pd/po/d, objdump, cclean) - Prompt uses native %g/%G git escapes instead of starship - rprompt='%S%G%s' for right-side git branch display - histfile=~/.mcsh_history (separate from tcsh history) 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
Code fixes: - configure.ac: TCSH_BASELINE_VERSION now expands TCSH_VERSION M4 macro instead of hardcoding "TCSH_VERSION" as a string literal - configure.ac: PACKAGE_PATCHLEVEL normalisation uses sed to strip leading zeros, preventing invalid C integer literals (e.g. 08, 09) in patchlevel.h - sh.func.c: doif() local variable widened from int to tcsh_number_t to preserve full expression result before branch decision - vms.termcap.c: octal escape cases '4'-'7' added to backslash-escape switch; continuation digit validation now enforces <= '7' Documentation: - README.md: full rewrite adding new features table (predictive autocomplete, %g/%G git escapes, set color), complete bug-fix table covering all PR3 items, prompt reference, dot.mcshrc section, updated build instructions - ISSUES.md: Phase 8 section documenting all PR3 review items with per-item status (fixed vs. already-correct with rationale) - PLAN.md: changelog entry for 2026-04-21 PR3 review pass All other PR3 findings were confirmed already addressed in the previous session (2026-04-20): install-sh glob patterns, m4 typos, po.m4 DLL cleanup, ed.defns.c catalog ID, ed.inputl.c redraw, ed.refresh.c ghost tracking, ed.chared.c NUL terminator and newline strip, alacritty.toml portability, sh.sem.c Dfix gating, sh.exp.c TEXP_NOGLOB guard, sh.set.c getn empty-string fast-path (intentional for ignored arms). 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
- dot.mcshrc: add `set time = (5 "…")` so commands taking > 5 s report user/sys/real/cpu times, replacing the timing starship provided - [n] in the prompt is %? — last exit status (0 = ok, non-zero = error) 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
Without -l, alacritty spawns mcsh as a plain command (not a login shell), so ~/.mcshrc is not sourced and no prompt/color/time settings apply. Also points to the installed /usr/local/bin/mcsh instead of the dev tree. 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
The threshold in set time is CPU time (user+sys seconds), not wall clock. 5 CPU seconds almost never fires. Changed to 1 so any command burning >= 1 CPU second reports timing. Also note: the shell only times forked commands (external programs), not builtins. set time has no effect on cd, echo, etc. 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
The prusage format loop only did raw xputchar, so \n printed literally. Added backslash-escape handling: \n=newline, \t=tab, \e=ESC, \=backslash. dot.mcshrc: format now uses \n for newline and \e[...m ANSI color codes — real (cyan), user (green), sys (yellow), cpu (magenta). 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
- dot.mcshrc: gate Wayland/GPU vars behind WAYLAND_DISPLAY+/dev/dri check - dot.mcshrc: fix Home/End keybindings (\e[1~, \e[4~, \eOH, \eOF, \e[H, \e[F) - dot.mcshrc: source ~/.mcshrc.local for machine-local overrides - ed.chared.c: reject ghost wholesale if it won't fit in input buffer - ed.chared.c: materialize histline via sprlex() before prefix comparison - ed.refresh.c: tgetstr uses real capbuf; track prev_ghost_start_h for correct erase - tc.prompt.c: gitdir NUL termination; worktree .git file parsing; bare repo snprintf fix - tc.prompt.c: mtime-based cache invalidation (stat HEAD, invalidate on change) - tc.prompt.c: replace raw Strbuf_append1 loops with tprintf_append_mbs for multibyte - vms.termcap.c: bounded sscanf %1023[^|:]; fix last-alias loop; fix sizeof(bp) -> 1024 - vms.termcap.c: fix case '\' -> case '\\' (invalid escape literal) - ISSUES.md: DrawGhost entry corrected to partial mitigation (Display/Vdisplay not used) - PLAN.md: vms.termcap.c retained as POSIX/Android termcap shim (not removed); consistent - README.md: remove ^F from autocomplete accept key (not bound by default) 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
The prev_ghost_start_h approach was wrong: it stored vcursor_h (the virtual-draw offset) not the physical terminal cursor column, so the backspace count to "return to ghost start" was far too large, sending the cursor to column 0. New logic: - When GhostBuf is cleared and Cursor==LastChar (accept path): Refresh already painted the new real content over the ghost area via update_line; no erase needed, just reset prev_ghost_cols. - When Cursor!=LastChar (move-away path): emit spaces then backspaces from the current cursor position to overwrite trailing ghost chars; Refresh will repaint correct content on the next redraw. - Normal redraw with ghost still active: erase from current position then redraw ghost (unchanged). Drops prev_ghost_start_h entirely. 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
dot.mcshrc: - move ~/.mcshrc.local sourcing to end-of-file so user overrides take precedence - guard make completion behind Makefile existence check; redirect stderr - rename alias objdump -> dis to preserve original objdump for normal use - fix VIMINFO -> VIMINIT (correct env var to suppress .viminfo for root) - add comment documenting set time = (0 ...) threshold is intentional ed.refresh.c: - DrawGhost: replace raw backspace-based cursor return with MoveToLine/MoveToChar using tracked prev_ghost_h/prev_ghost_v (physical CursorH/CursorV coordinates) so ghost erase always targets the correct terminal position sh.time.c: - add explicit braces around for-loop body and inner if/else branches to fix ambiguous if/else shape flagged by static analysis tc.prompt.c: - remove no-op snprintf(gitdir, sizeof(gitdir), "%s", gitdir) self-copy vms.termcap.c: - check sscanf return == 1 before strcmp in alias loop - fix rp+tlen pointer arithmetic in case 'd': use integer (rend-rp) comparison alacritty.toml: - comment out pywal import line; change program to "mcsh" (PATH-resolved) ISSUES.md: update alacritty.toml entry to reflect actual current state PLAN.md: clarify VMS section heading — platform discontinued, shim files retained 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
Root cause: DrawGhost was called both from Refresh() (full repaint) and RefPlusOne() (incremental single-char fast path). On the full repaint path update_line already repainted the line cleanly, but DrawGhost still tried to MoveToLine/MoveToChar to the stale prev_ghost_h/prev_ghost_v and overwrite those positions with spaces — clobbering real input that Refresh had just drawn. Fix: add full_repaint flag to DrawGhost(int). - Refresh() calls DrawGhost(1): skip the erase entirely; the line is already clean, just draw the new ghost and record its position. - RefPlusOne() calls DrawGhost(0): erase old ghost tail at the recorded prev_ghost_h/prev_ghost_v before drawing new ghost. Also reset prev_ghost_* whenever ghost is cleared regardless of path. 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
The MoveToLine/MoveToChar approach was wrong. On the RefPlusOne path CursorH already advanced past the newly-typed char, so navigating back to prev_ghost_h put the cursor inside the real input and erased it. Correct model: the ghost always lives exactly at the current cursor position (right after LastChar). On RefPlusOne: - CursorH has already advanced one column (new char was PutPlusOne'd) - the old ghost tail starts precisely at the new CursorH - erase by writing prev_ghost_cols spaces then backspacing back - draw the new ghost from there On full Refresh: line is already clean — skip erase entirely, just draw new ghost with backspace-return (same as the original simple path). Drops MoveToLine/MoveToChar from DrawGhost entirely. 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
Implements Option B: full virtual-display pipeline integration.
No raw ESC bypass — colour flows through the same Display/Vdisplay
model used by every other rendering path.
Architecture end-to-end:
1. syntax_colorize() (ed.syntax.c) runs after every keystroke in
ed.inputl.c — single-pass state machine fills SyntaxColor[]
with SynToken values for each InputBuf character.
2. Draw() (ed.refresh.c) reads SyntaxColor[cp-InputBuf] and sets
the vcurrent_color global (SYN_NORMAL for prompt characters).
3. Vdraw() writes vcurrent_color into VcolorDisplay[v][h] in
parallel with the character write into Vdisplay[v][h].
4. Refresh() main loop copies VcolorDisplay -> ColorDisplay after
the Vdisplay->Display diff, keeping colour arrays in sync.
5. so_write() (ed.screen.c) reads ColorDisplay[CursorV][CursorH]
per character and calls SetSGRColor() before putwraw(). Resets
with SetSGRColor(-1) at end of write.
6. SetSGRColor() tracks cur_sgr to suppress redundant SGR emissions;
resets to -1 whenever SetAttributes() clears all attributes.
Token types: keyword, builtin, cmd_ok, cmd_bad, operator, variable,
dquote, squote, backtick, comment, error (unmatched quote).
LRU command cache (CMD_CACHE_SIZE=32) avoids per-keystroke stat(2)
calls on $PATH for command existence checks.
New files:
ed.syntax.h — SynToken enum, SynColor/SynPalette, SyntaxColor[],
extern declarations
ed.syntax.c — tokeniser, LRU cache, cmd_on_path(), syntax_colorize(),
syntax_clear()
Modified files:
ed.h — VcolorDisplay, ColorDisplay, vcurrent_color externals
ed.screen.c — ReBufferDisplay() allocs colour arrays; SetSGRColor()
new static fn; so_write() reads ColorDisplay per char;
SetAttributes() resets cur_sgr on full attribute clear
ed.refresh.c — Draw() sets vcurrent_color; Vdraw() writes VcolorDisplay;
ClearDisp() zeroes ColorDisplay; Refresh() copies arrays
ed.inputl.c — calls syntax_colorize() after every command dispatch
ed.decls.h — extern syntax_colorize(), syntax_clear()
sh.set.c — update_vars() and dounset hook STRsyntax
tc.const.c — STRsyntax[] constant
Makefile.in — ed.syntax.${SUF} added to EDOBJS
dot.mcshrc — set syntax added after set color
Feature is fully gated on adrof(STRsyntax); no overhead when unset.
Also removes alacritty.toml from tracking (machine-local config).
Docs: PLAN.md Phase 9, ISSUES.md completed section, README feature table.
💘 Generated with Crush
Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
…Display Two bugs causing colours on wrong/random characters: 1. so_write() was reading ColorDisplay[CursorV][CursorH] — the *previous* frame's colour map — while rendering characters from the current Vdisplay. Every character got last keystroke's token, shifted by the display diff. Changed to read VcolorDisplay which is filled by Vdraw() in the current Refresh() pass. 2. VcolorDisplay was never zeroed before each repaint, so deleted characters left stale SynToken values that bled onto the padding spaces written by cpy_pad_spaces(). Added a memset loop at the top of Refresh() before the prompt/input draw pass. 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
CC_NORM falls through without calling Refresh() — it relies on RefPlusOne() for the incremental single-character append path, which never goes through Draw()/Vdraw() and therefore never picks up the new SyntaxColor[] values from syntax_colorize(). When set syntax is active, promote CC_NORM to CC_REFRESH after syntax_colorize() runs so a full Refresh() is always issued and the new token colours reach the screen immediately on every keypress. 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
…f bypass) The core problem: update_line() does a glyph-only diff (*o == *n). When you type a character that changes a word's token class (e.g. 'i' -> 'if' becoming a keyword), the *existing* letters didn't change glyph so the diff skips them entirely -- so_write() never fires for those cells and the new SGR colour never reaches the terminal. Two fixes together: 1. ed.refresh.c: before update_line() on each line, compare ColorDisplay (previous colours) vs VcolorDisplay (new colours). If they differ, memset Display[cur_line] to zero to poison the diff -- forcing update_line() to see every cell as changed and redraw the whole line via so_write() with the new colours. 2. ed.inputl.c: keep CC_NORM -> CC_REFRESH promotion when set syntax is active, so RefPlusOne() (which bypasses Draw/Vdraw entirely) is replaced with a full Refresh() that goes through the colour pipeline. Without this, single-char inserts never reach Refresh() at all. 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
…correctly) Replace the parallel VcolorDisplay/ColorDisplay arrays and Display[] poisoning with the correct approach: pack the 4-bit SynToken directly into bits 0xF0000000 of each Char written to Vdisplay. ATTRIBUTES uses 0x0F000000; QUOTE (0x80000000) is only ever set by the lexer and never written into Vdisplay/Display. So bits 0xF0000000 are completely free in the display layer -- 4 bits covers all 12 tokens. Result: update_line()'s glyph diff (*o == *n) now naturally detects colour-only changes because the packed token makes the Char values differ. Deleted characters become '\0' in the new Vdisplay which differs from the old coloured Char -- backspace correctly erases. No poisoning, no parallel arrays, no stale state, no off-by-one. so_write() extracts the token with SYN_TOK(), calls SetSGRColor(), then strips the token bits with SYN_GLYPH() before putwraw() so the terminal sees only the actual glyph. New macros in ed.syntax.h: SYN_SHIFT / SYN_MASK -- bit field definition SYN_PACK(c, t) -- embed token t into display Char c SYN_TOK(c) -- extract token from display Char c SYN_GLYPH(c) -- strip token bits, get raw glyph Removed: VcolorDisplay, ColorDisplay parallel arrays and all allocation, zeroing, memcmp/memcpy, and poisoning code. 💘 Generated with Crush Assisted-by: Claude Sonnet 4.6 via Crush <crush@charm.land>
Owner
Author
|
All requested review fixes and enhancements for Round 9 have been implemented and verified:
A full regression run (14 tests) passes on FreeBSD 15. Push completed to |
The use of '$> $^' in FreeBSD make caused the 'install' utility to receive the same file twice, leading it to misinterpret the destination filename as a directory. Corrected to use only '$^' (all dependencies).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ed.chared.c`:
- Around line 3884-3891: Remove the dead field f_cache.is_dir from the anonymous
struct definition (the static f_cache) in ed.chared.c since directory-ness is
encoded in f_cache.ghost and predict_file() uses a local is_dir_match; delete
the "int is_dir;" declaration and grep the codebase for any references to
f_cache.is_dir to remove or update them if found, then rebuild to confirm no
references remain.
- Around line 3893-3898: c_cache only gets invalidated when the PATH string
itself changes, so newly installed executables in existing PATH directories or
previously-missing commands cached as "no-match" remain invisible; fix by adding
cache invalidation logic for c_cache: either implement a new
predict_cache_clear() (and call it from dohash() and rehash(), mirroring
syntax_cache_clear() for cmd_cache) or augment c_cache to store
per-PATH-directory mtimes (like f_cache) and invalidate when any directory mtime
differs, ensuring functions/variables to touch are c_cache, cmd_cache,
syntax_cache_clear(), dohash(), rehash(), and predict_cache_clear()/the
mtime-checking code so predictive completion is refreshed appropriately.
In `@ISSUES.md`:
- Around line 451-467: Update the "Remaining open items" section to remove or
amend the stale entry that says the test suite is “not yet initialised” so it
matches the "3. Test suite — initial suite created (`tests/`)" section;
specifically locate the mention of "test suite not yet initialised" under the
Remaining open items heading and either delete it or change it to note that an
initial suite exists (referencing the tests/ directory, 8 scripts, and the
Makefile and the run command `make -C tests MCSH=./mcsh check`) so the issue log
has a single, consistent statement of the test-suite status.
In `@PLAN.md`:
- Around line 280-282: The changelog entries are out of chronological order: the
"2026-04-21 | Phase 4/6 review response (dev4): ..." row currently appears after
the "2026-04-22 | PR `#5` Round 7 ..." row; reorder these rows so dates are
consistent (choose ascending or descending and apply it uniformly), moving the
2026-04-21 entry to the correct position relative to 2026-04-22 (and verify
neighbouring entries like "2026-04-27 | PR `#5` Round 9 ..." remain in sequence).
- Line 281: The table row on line with the PR summary contains unescaped pipe
characters (e.g., in operator examples like `>!`/`>>!`/`>|`) which breaks
Markdown table parsing (MD056); update that row in PLAN.md to escape any `|`
characters as `\|` (or move those operator examples out of the table cell) so
the table columns render correctly, ensuring examples such as `>|`, `>>!`, and
any other `|` tokens are escaped.
In `@README.md`:
- Around line 289-319: Rename the top-level heading "## Known Limitations" to a
non-contradictory title like "## Resolved regressions" (or "Resolved issues")
and update the two subsection titles "Unicode / wide-character regression —
fixed (Round 9)" and "Short-circuit evaluation — fixed (dev4, improved in Round
7)" to remove the word "fixed" duplication (e.g., keep the explanatory suffixes
but ensure they read as resolved items), and ensure the body text still
references ISSUES.md Round 9 and Rounds 6–7; this keeps the README consistent by
clearly separating resolved regressions from active limitations which remain
only in ISSUES.md.
In `@tests/t008_unset_modifiers.sh`:
- Around line 5-9: Update the grep invocation that checks the command output:
when inspecting the variable `out` (from the `$MCSH -f -c 'unset x; echo
"${x:h}"'` invocation) use grep with `-q` to suppress matched-line printing and
remove the redundant `Error|` alternative since `-i` already covers case
variations; this keeps the failure output to a single tidy line while preserving
the same matching semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cec52891-4fa2-4493-ab7b-0752c64dacc3
📒 Files selected for processing (11)
ISSUES.mdPLAN.mdREADME.mddot.mcshrced.chared.ced.syntax.ctc.const.ctests/lib_locale.shtests/run_tests.shtests/t006_function_builtin.shtests/t008_unset_modifiers.sh
This reverts commit d755940.
Verified on FreeBSD 15 that '$^' expands to a single source file, resolving the 'target directory does not exist' error caused by the redundant '$>' variable.
- implement predict_cache_clear() and call it during rehash/path change - remove dead is_dir field from f_cache - documentation cleanup in README, PLAN and ISSUES - reorder PLAN changelog chronologically and escape pipes - update t008 test with more concise grep - fix undeclared function warning in sh.exec.c
Owner
Author
|
All final refinements from the latest review pass (Round 9.1) have been implemented and verified:
Push completed to |
Owner
Author
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to mcsh, including Fish-style predictive autocomplete for file paths and commands, and a comprehensive regression test suite. It addresses critical Unicode regressions inherited from tcsh by correcting byte-vs-character length confusion in mbtowc loops and fixes short-circuit evaluation issues by allowing unset variables to expand to empty strings. Feedback includes concerns regarding the fixed buffer size for PATH caching and suggestions to tighten the syntax highlighting logic for redirection operators to prevent coloring invalid sequences.
| /* Update cache */ | ||
| strncpy(c_cache.prefix, prefix, sizeof(c_cache.prefix) - 1); | ||
| c_cache.prefix[sizeof(c_cache.prefix) - 1] = '\0'; | ||
| strncpy(c_cache.path, pathenv, sizeof(c_cache.path) - 1); |
There was a problem hiding this comment.
The PATH environment variable can exceed 2048 bytes on many modern systems. Truncating it in c_cache.path may lead to incorrect cache misses or, in extremely rare cases of prefix collisions, incorrect hits if the comparison logic isn't careful. Since strcmp is used for the cache check, it will correctly identify that a truncated cache path does not match a longer pathenv, resulting in a safe but persistent cache miss for very long PATH strings. Consider increasing c_cache.path size or using a hash of the PATH string.
Comment on lines
+539
to
541
| if (nc == '>' || nc == '<' || nc == '&' || nc == '-' || | ||
| (opener == '>' && (nc == '!' || nc == '|'))) | ||
| SyntaxColor[++i] = SYN_OPERATOR; |
There was a problem hiding this comment.
The redirection operator coloring logic is still somewhat permissive. It allows sequences like >< or <<< to be colored as operators, which are syntactically invalid in csh. While this is primarily a cosmetic issue in syntax highlighting, a more restrictive check based on the opener and the specific valid combinations (e.g., >>, <<, >&, >&-, >!, >|) would be more precise.
References
- Maintainability and correctness: Syntax highlighting should ideally reflect valid language constructs. (link)
- Swap predict_cmd and predict_file priority - Remove restrictive path prefix check in predict_file - Increase c_cache.path buffer to 4096 bytes - Correct dates and escape pipes in PLAN.md - Rename Known Limitations to Resolved regressions in README.md - Update ISSUES.md metadata and deferred list
Owner
Author
Round 10 Review ResponseAll findings from the latest review pass have been resolved:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ed.chared.c`:
- Around line 4004-4014: The cache check in the f_cache block (using
f_cache.prefix and f_cache.dir) can return stale results for relative paths
because it omits current working directory; update the cache key or the
cache-use condition to incorporate cwd identity: either include the absolute cwd
(e.g., via getcwd()) into the f_cache key or refuse to reuse the cache when
dirpath or prefix are relative (i.e., detect non-absolute lookups and skip the
cached branch). Apply the same change to the other similar cache checks
referenced (around the code paths used by predict_file() and predict_cmd()) so
f_cache.mtime/f_cache.ghost entries are only reused when cwd is part of the key
or when the lookup is absolute.
- Around line 3948-3950: Replace the per-byte casts from editor Char to char and
the byte-by-byte rebuilds of GhostBuf with full-buffer conversions using the
existing wide↔multibyte helper routines: instead of casting each wp[i] & CHAR
into word[i], call the wide-to-multibyte helper (e.g. convert_wide_to_mb or
wide_to_utf8) to transform the whole wp buffer of length wlen into the multibyte
buffer word and null-terminate it; likewise, when reconstructing GhostBuf from a
multibyte suffix, use the multibyte-to-wide helper (e.g. convert_mb_to_wide or
utf8_to_wide) to produce an array of Char rather than assigning code units
one-by-one; apply these changes to the blocks around variables wp, word, wlen
and GhostBuf (the occurrences you noted at ~3948, 4061-4068, 4137-4142,
4212-4218), allocate temp buffers sized for worst-case MB length (MB_LEN_MAX *
wlen + 1) and handle errors/lengths returned by the helpers instead of per-byte
casting.
In `@PLAN.md`:
- Around line 279-282: Update the PLAN.md summary rows to reconcile the new
changelog: change the earlier mention that “#107 was fixed by skipping Dfix() in
sh.sem.c” to reflect the actual short-circuit fix in sh.dol.c (Dgetdol()
returning STRNULL for unset vars) and update the line that says predictive
autocomplete is “always active” to state that predictive autocomplete is now
controllable via the new "set predict" toggle (ed.chared.c
`predict_file`/`predict_cmd` + `set predict`), ensuring the file no longer
contains the conflicting descriptions of the same behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c889b4b5-6df6-4bf2-82c0-7504d23193d8
📒 Files selected for processing (9)
ISSUES.mdPLAN.mdREADME.mded.chared.ced.decls.hnls/Makefile.insh.exec.csh.set.ctests/t008_unset_modifiers.sh
Owner
Author
|
All requested fixes from the latest review pass have been implemented and verified.
The local system has been cleanly re-installed with |
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
PLAN.md (1)
156-156:⚠️ Potential issue | 🟡 MinorPhase 4 Completed table still describes the abandoned
Dfix()/sh.sem.capproach for#107.Line 156 says the fix is “
Dfix()skips expansion for expression-evaluating builtins; expansion deferred until after short-circuit.” — but per the PR objectives and the new Round 6 changelog entry on Line 279, the actual short-circuit fix landed insh.dol.c(Dgetdol()returningSTRNULLfor unset vars in double-quoted contexts). These two descriptions conflict. Please update Line 156 to point atsh.dol.cso the document has a single, current narrative for#107.📝 Suggested edit
-| **#107** (PR) | Medium | `sh.exp.c`, `sh.sem.c` | `$?a && "$a" != ""` throws if `a` is unset. Fix: `Dfix()` skips expansion for expression-evaluating builtins; expansion deferred until after short-circuit. | +| **#107** (PR) | Medium | `sh.dol.c` | `$?a && "$a" != ""` threw `Undefined variable` when `a` was unset. Fix: `Dgetdol()` returns `STRNULL` for unset variables in double-quoted contexts (instead of raising `ERR_UNDVAR`), letting the expression evaluator's short-circuit kick in. `$?var` existence tests are unaffected. Regression test: `tests/t003_shortcircuit.sh`. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PLAN.md` at line 156, The Phase 4 table entry incorrectly describes a discarded Dfix()/sh.sem.c approach for issue `#107`; update the text on Line 156 to reflect the actual fix implemented in sh.dol.c by mentioning Dgetdol() returning STRNULL for unset variables in double-quoted contexts (which enables correct short-circuit behavior for expressions like `$?a && "$a" != ""`) and remove or replace the Dfix()/sh.sem.c wording so the document consistently attributes the change to sh.dol.c and Dgetdol().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ed.chared.c`:
- Around line 4040-4060: predict_file() currently caches only a "no match"
result in f_cache.ghost and misses when the user types a longer prefix, causing
a full opendir/readdir rescan; to fix, extend the cache to track the
longest-prefix that yielded zero matches (e.g. add f_cache.no_match_prefix_len
or store the prefix string alongside f_cache.ghost) and change the lookup logic
in predict_file() to short-circuit when the new prefix starts with that cached
no-match prefix (and dir/cwd/mtime still match), so you return 0 without
rescanning; update the code paths that set f_cache.ghost and where nmatch == 0
to populate this new field and consult it before calling opendir/readdir/stat.
- Around line 4078-4089: The code assumes str2short(match) (wide_match) is
non-NULL and pre-reserves two slots in f_cache.ghost for '/' and NUL; add a
defensive NULL check after calling str2short to bail or handle if wide_match is
NULL, and change the unconditional slash append for is_dir_match to only set
f_cache.ghost[gi++] = '/' when gi <
sizeof(f_cache.ghost)/sizeof(f_cache.ghost[0]) - 1 (so there remains one slot
for the NUL), referencing the symbols str2short, wide_match, f_cache.ghost,
is_dir_match and the gi index to locate the edits.
- Around line 4194-4233: predict_cmd currently skips over PATH components that
exceed dirpath[1024] by advancing p and continuing, which is functionally
correct but causes heavy per-keystroke stat/access syscalls; fix by either (A)
reusing the existing command cache used by ed.syntax.c (expose cmd_cache as an
extern and consult it from predict_cmd to avoid full PATH scans for repeated
prefixes) or (B) short‑circuiting the expensive per-entry stat/access inside
predict_cmd when iterating a directory that is a known PATH directory (maintain
a small set/hash of validated PATH dirs and, if the current dirpath is in that
set, trust readdir results and skip stat/access). Locate predict_cmd and the
loop using variables pathenv, p, dirpath, dp, de, fullpath, and nmatch to
implement one of these optimizations; keep the existing overflow skip (p = q ? q
+ 1 : NULL; continue) logic unchanged.
- Around line 4015-4017: When getcwd(current_cwd, sizeof(current_cwd)) returns
NULL, treat that as an invalid cache state instead of silently using an empty
string: in the block where current_cwd[0] = '\0' set f_cache.valid = 0 (and do
the same mirrored change in predict_cmd where getcwd is checked) so the cache is
invalidated on getcwd failure and won't produce stale cache hits; locate uses of
current_cwd, getcwd, and f_cache.cwd/f_cache.valid to apply the change.
- Around line 3920-3993: predict_file duplicates the same separator set used in
predict_cmd, causing divergence risk; extract the separator test into a small
static helper (e.g. static int is_word_break(int c)) and replace the inline
checks in both predict_file and predict_cmd with calls to is_word_break(c).
Ensure is_word_break handles the current characters (' ', '\t', ';', '|', '&',
'(', ')') and any future additions (e.g. backtick, '<', '>') and update the
while loops that walk wp in predict_file and the equivalent loop in predict_cmd
to call this helper so both functions stay in sync.
- Around line 3884-3899: The c_cache.path buffer can silently truncate an
oversized $PATH causing strcmp(c_cache.path, pathenv) to falsely match; update
the cache struct (c_cache) to also record either the full path length (e.g.,
path_len) or a compact checksum/hash (e.g., uint64_t path_hash) computed from
the entire pathenv at the time of storing, then: when populating c_cache.path
(the strncpy site), set path_len or path_hash from the full pathenv (using
strlen or a stable hash routine) and when comparing/invalidation compare both
the stored length/hash and the truncated buffer string so any change beyond 4
KiB invalidates the cache conservatively; ensure the same scheme is used when
checking cache validity (where strcmp(c_cache.path, pathenv) is used) and when
clearing/setting c_cache.valid.
- Around line 4032-4061: The code currently calls stat(dirpath, &st) after
opendir(dp) which creates a TOCTOU race; change that to call fstat(dirfd(dp),
&st) to read metadata from the already-open directory handle instead. In the
block after opendir(dirpath) replace stat(dirpath, &st) with fstat(dirfd(dp),
&st) and keep the same success/failure handling that sets f_cache.mtime (i.e. if
fstat(...) == 0 set f_cache.mtime = st.st_mtime else set f_cache.mtime = 0).
Ensure dirfd(dp) is used and that error handling remains consistent with the
existing logic around opendir(), f_cache.mtime, and the loop that uses dp and
st.
- Around line 3962-3993: The tilde-expansion code calls getpwnam(user) on every
keystroke for tilde-prefixed input, causing latency; introduce a small cache for
the last looked-up username and its pw result (e.g., static last_user[] and
last_pw pointer or copy of pw->pw_dir) and, before calling getpwnam, compare the
parsed user string to the cached last_user and reuse the cached pw_dir if it
matches; update the cache whenever you successfully call getpwnam and handle
cache misses by calling getpwnam as before, leaving the rest of the expansion
logic (variables user, s, pw and the existing snprintf/strncpy handling)
unchanged.
In `@PLAN.md`:
- Line 282: Update the changelog entry to explicitly list the redirection
operators whose coloring was added in ed.syntax.c so readers can map the note to
the code changes; mention that the PR added coloring for the operators >!, >>!,
>| and >&, and reference ed.syntax.c (the redirection operator handling block)
so future readers can quickly correlate the changelog with the implementation.
- Line 281: Line 281 in PLAN.md contains code spans padded with leading/trailing
spaces (e.g. backtick, space, token, space, backtick) that should be tightened;
open the line where the changelog entry references symbols like sh.dol.c,
tests/run_tests.sh, t006, ed.syntax.c, ed.chared.c and replace instances of
padded code spans (e.g. ` $< `, ` $%unset `) with trimmed spans (e.g. `$<`,
`$%unset`), ensuring every backtick-enclosed token has no extra spaces inside
the backticks; you can locate them by searching for the pattern of a backtick,
whitespace, text, whitespace, backtick and remove the inner spaces around the
token.
---
Duplicate comments:
In `@PLAN.md`:
- Line 156: The Phase 4 table entry incorrectly describes a discarded
Dfix()/sh.sem.c approach for issue `#107`; update the text on Line 156 to reflect
the actual fix implemented in sh.dol.c by mentioning Dgetdol() returning STRNULL
for unset variables in double-quoted contexts (which enables correct
short-circuit behavior for expressions like `$?a && "$a" != ""`) and remove or
replace the Dfix()/sh.sem.c wording so the document consistently attributes the
change to sh.dol.c and Dgetdol().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 20203558-16b2-4bd1-a519-c57b050a36cf
📒 Files selected for processing (2)
PLAN.mded.chared.c
Comment on lines
+4032
to
+4061
| dp = opendir(dirpath); | ||
| if (!dp) | ||
| return 0; | ||
| if (stat(dirpath, &st) == 0) | ||
| f_cache.mtime = st.st_mtime; | ||
| else | ||
| f_cache.mtime = 0; | ||
|
|
||
| while ((de = readdir(dp)) != NULL) { | ||
| const char *name = de->d_name; | ||
| /* skip dot files unless prefix starts with dot */ | ||
| if (name[0] == '.' && (pfxlen == 0 || prefix[0] != '.')) | ||
| continue; | ||
| if (pfxlen > 0 && strncmp(name, prefix, pfxlen) != 0) | ||
| continue; | ||
| if (strcmp(name, prefix) == 0) | ||
| continue; /* exact match — nothing to complete */ | ||
| nmatch++; | ||
| if (nmatch == 1) { | ||
| strncpy(match, name + pfxlen, sizeof(match) - 1); | ||
| match[sizeof(match) - 1] = '\0'; | ||
| /* check if it's a directory to append trailing slash */ | ||
| char fullpath[1024]; | ||
| xsnprintf(fullpath, sizeof(fullpath), "%s%s", dirpath, name); | ||
| is_dir_match = (stat(fullpath, &st) == 0 && S_ISDIR(st.st_mode)); | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| closedir(dp); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is dirfd(3) available on FreeBSD, Linux, macOS, Solaris, and AIX as a POSIX function for retrieving the file descriptor of an open DIR*?
💡 Result:
Yes, dirfd(3) is available on FreeBSD, Linux, macOS, Solaris, and AIX as a POSIX-standardized function for retrieving the file descriptor associated with an open DIR* stream. It was added to POSIX.1-2008 and is part of the current POSIX.1-2024 specification.
Citations:
- 1: https://pubs.opengroup.org/onlinepubs/9799919799.2024edition/functions/dirfd.html
- 2: https://man7.org/linux/man-pages/man3/dirfd.3.html
- 3: https://man7.org/linux/man-pages/man3/dirfd.3p.html
- 4: https://man.freebsd.org/cgi/man.cgi?query=dirfd
- 5: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/dirfd.3.html
- 6: http://docs.oracle.com/cd/E36784_01/html/E36874/dirfd-3c.html
- 7: https://illumos.org/man/3C/dirfd
- 8: https://www.ibm.com/docs/en/zos/3.1.0?topic=functions-dirfd-get-directory-stream-file-descriptor
🏁 Script executed:
#!/bin/bash
# Locate ed.chared.c and verify the code at lines 4032-4061
fd -i 'ed\.chared\.c' | head -5Repository: orpheus497/mcsh
Length of output: 71
🏁 Script executed:
sed -n '4032,4061p' ed.chared.cRepository: orpheus497/mcsh
Length of output: 957
Fix TOCTOU race between opendir() and stat() by using fstat(dirfd(dp), &st) on the open directory handle.
The directory path is opened at line 4032 but then re-stated at line 4035 using the original path. Between these calls, the directory could be replaced (via atomic rename), causing the cached mtime to belong to a different inode than the one enumerated. Use fstat(dirfd(dp), &st) instead to read metadata from the already-open directory handle, eliminating the race.
Suggested fix
dp = opendir(dirpath);
if (!dp)
return 0;
- if (stat(dirpath, &st) == 0)
+ if (fstat(dirfd(dp), &st) == 0)
f_cache.mtime = st.st_mtime;
else
f_cache.mtime = 0;dirfd() is POSIX.1-2008+ standard and widely available on FreeBSD, Linux, macOS, Solaris, and AIX.
🧰 Tools
🪛 Clang (14.0.6)
[note] 4033-4033: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 4035-4035: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 4037-4037: +1, nesting level increased to 1
(clang)
[note] 4040-4040: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 4043-4043: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 4043-4043: +1
(clang)
[note] 4043-4043: +1
(clang)
[note] 4045-4045: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 4045-4045: +1
(clang)
[note] 4047-4047: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 4050-4050: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 4057-4057: +1, nesting level increased to 2
(clang)
[warning] 4033-4033: statement should be inside braces
(readability-braces-around-statements)
[warning] 4035-4035: statement should be inside braces
(readability-braces-around-statements)
[warning] 4037-4037: statement should be inside braces
(readability-braces-around-statements)
[warning] 4043-4043: statement should be inside braces
(readability-braces-around-statements)
[warning] 4045-4045: statement should be inside braces
(readability-braces-around-statements)
[warning] 4047-4047: statement should be inside braces
(readability-braces-around-statements)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ed.chared.c` around lines 4032 - 4061, The code currently calls stat(dirpath,
&st) after opendir(dp) which creates a TOCTOU race; change that to call
fstat(dirfd(dp), &st) to read metadata from the already-open directory handle
instead. In the block after opendir(dirpath) replace stat(dirpath, &st) with
fstat(dirfd(dp), &st) and keep the same success/failure handling that sets
f_cache.mtime (i.e. if fstat(...) == 0 set f_cache.mtime = st.st_mtime else set
f_cache.mtime = 0). Ensure dirfd(dp) is used and that error handling remains
consistent with the existing logic around opendir(), f_cache.mtime, and the loop
that uses dp and st.
Comment on lines
+4040
to
+4060
| while ((de = readdir(dp)) != NULL) { | ||
| const char *name = de->d_name; | ||
| /* skip dot files unless prefix starts with dot */ | ||
| if (name[0] == '.' && (pfxlen == 0 || prefix[0] != '.')) | ||
| continue; | ||
| if (pfxlen > 0 && strncmp(name, prefix, pfxlen) != 0) | ||
| continue; | ||
| if (strcmp(name, prefix) == 0) | ||
| continue; /* exact match — nothing to complete */ | ||
| nmatch++; | ||
| if (nmatch == 1) { | ||
| strncpy(match, name + pfxlen, sizeof(match) - 1); | ||
| match[sizeof(match) - 1] = '\0'; | ||
| /* check if it's a directory to append trailing slash */ | ||
| char fullpath[1024]; | ||
| xsnprintf(fullpath, sizeof(fullpath), "%s%s", dirpath, name); | ||
| is_dir_match = (stat(fullpath, &st) == 0 && S_ISDIR(st.st_mode)); | ||
| } else { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
predict_file() early-exit logic looks correct, but the cached "no match" entry still re-runs opendir() on every keystroke when mtime changes.
When nmatch != 1, the cache is updated (Line 4063-4070) with f_cache.ghost[0] = '\0' (Line 4073). On the next keystroke with the same prefix/dir/cwd, the lookup at Line 4020-4029 will find a match, but the if (f_cache.ghost[0]) test fails so we return 0 without re-scanning. Good.
However: when the user adds another character (so prefix changes), the cache misses regardless of mtime, and we re-scan the whole directory. For deep directories (/usr/bin, etc.) this still drives one full opendir/readdir loop per keystroke during typing. Consider tracking the longest cached prefix so that, when the new prefix extends a previously-empty match (e.g. previous prefix xyz had 0 matches → new prefix xyzw cannot have any), you can short-circuit without rescanning. Optional optimisation, not a blocker.
🧰 Tools
🪛 Clang (14.0.6)
[note] 4040-4040: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 4043-4043: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 4043-4043: +1
(clang)
[note] 4043-4043: +1
(clang)
[note] 4045-4045: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 4045-4045: +1
(clang)
[note] 4047-4047: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 4050-4050: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 4057-4057: +1, nesting level increased to 2
(clang)
[warning] 4043-4043: statement should be inside braces
(readability-braces-around-statements)
[warning] 4045-4045: statement should be inside braces
(readability-braces-around-statements)
[warning] 4047-4047: statement should be inside braces
(readability-braces-around-statements)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ed.chared.c` around lines 4040 - 4060, predict_file() currently caches only a
"no match" result in f_cache.ghost and misses when the user types a longer
prefix, causing a full opendir/readdir rescan; to fix, extend the cache to track
the longest-prefix that yielded zero matches (e.g. add
f_cache.no_match_prefix_len or store the prefix string alongside f_cache.ghost)
and change the lookup logic in predict_file() to short-circuit when the new
prefix starts with that cached no-match prefix (and dir/cwd/mtime still match),
so you return 0 without rescanning; update the code paths that set f_cache.ghost
and where nmatch == 0 to populate this new field and consult it before calling
opendir/readdir/stat.
| | 2026-04-21 | Phase 4/6 review response (dev4): Three flagged weaknesses addressed. Short-circuit fix: `sh.dol.c` Dgetdol() returns STRNULL for unset vars instead of ERR_UNDVAR. Unicode regression documented (no upstream fix available). Test suite created: `tests/` with 7 scripts + Makefile. Gemini inline fixes: `cache_store()` goto removed (two-loop LRU); `GIT_POLL_INTERVAL` named constant. README Known Limitations section added. ISSUES.md Round 6 appended. | | ||
| | 2026-04-21 | Phase 4 upstream sweep: full audit of all tcsh-org/tcsh open + recently closed issues/PRs. Applied: `sh.file.c` 32-bit `wcscoll` cast (#116); `config_f.h` Shift-JIS `<= 4` condition (#115); `sh.h` `AUTOSET_KANJI` CODESET guard removal (#115). Confirmed already-present: #103, #104, #99, #101, #110. Rejected (upstream closed/not merged): #118 FIONREAD, #114 Shift-JIS runtime check. ISSUES.md, PLAN.md, README.md all updated. | | ||
| | 2026-04-21 | PR #5 Round 7 (Copilot + Gemini full review): `sh.dol.c` unset-var modifier fix (call `fixDolMod()` before eatbrac; return 0 for `$#unset`/`$%unset`). `tests/run_tests.sh` portability (glob guard, printf, comment fix). `t006` mktemp portability. `ed.syntax.c` ST_VARIABLE state machine fix ($$/$!/`$<` single-char specials; `>!` / `>>!` / `>\|` operator colouring; remove unused `in_table` var). `ed.chared.c` command+file-aware ghost-text predictor (`predict_file`, `predict_cmd`). New test `t008_unset_modifiers.sh`. ISSUES.md Round 7 appended. | | ||
| | 2026-04-27 | PR #5 Round 9 review response: `ed.syntax.c` redirection coloring fix; `ed.chared.c` caching, `set predict` toggle, `~user` expansion, and empty PATH component handling; test suite hardening (SKIP support, portability, robustness). ISSUES.md Round 9 appended. | |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Round 9 changelog entry is concise but understates which ed.syntax.c redirection-coloring change landed.
“ed.syntax.c redirection coloring fix” is vague. The PR objectives note that >!, >>!, >|, >>& were added to the redirection operator coloring. Consider expanding this entry so future readers can correlate the changelog with the diff in ed.syntax.c:531-547 without grepping commits.
📝 Suggested edit
-| 2026-04-27 | PR `#5` Round 9 review response: `ed.syntax.c` redirection coloring fix; `ed.chared.c` caching, `set predict` toggle, `~user` expansion, and empty PATH component handling; test suite hardening (SKIP support, portability, robustness). ISSUES.md Round 9 appended. |
+| 2026-04-27 | PR `#5` Round 9 review response: `ed.syntax.c` redirection coloring extended to `>!`, `>>!`, `>\|`, `>>&` (noclobber-override / append-noclobber forms); `ed.chared.c` predictive completion gains cwd-aware `f_cache`/`c_cache`, `set predict` gating, `~user`/`~/` tilde expansion, and treats empty `$PATH` components as `.` to match `cmd_on_path()`; test suite hardening (SKIP support via exit 77, mktemp/printf/grep portability, trap-based cleanup). ISSUES.md Round 9 appended. |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | 2026-04-27 | PR #5 Round 9 review response: `ed.syntax.c` redirection coloring fix; `ed.chared.c` caching, `set predict` toggle, `~user` expansion, and empty PATH component handling; test suite hardening (SKIP support, portability, robustness). ISSUES.md Round 9 appended. | | |
| | 2026-04-27 | PR `#5` Round 9 review response: `ed.syntax.c` redirection coloring extended to `>!`, `>>!`, `>\|`, `>>&` (noclobber-override / append-noclobber forms); `ed.chared.c` predictive completion gains cwd-aware `f_cache`/`c_cache`, `set predict` gating, `~user`/`~/` tilde expansion, and treats empty `$PATH` components as `.` to match `cmd_on_path()`; test suite hardening (SKIP support via exit 77, mktemp/printf/grep portability, trap-based cleanup). ISSUES.md Round 9 appended. | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PLAN.md` at line 282, Update the changelog entry to explicitly list the
redirection operators whose coloring was added in ed.syntax.c so readers can map
the note to the code changes; mention that the PR added coloring for the
operators >!, >>!, >| and >&, and reference ed.syntax.c (the redirection
operator handling block) so future readers can quickly correlate the changelog
with the implementation.
Owner
Author
|
All requested code review feedback has been addressed and committed to the
|
Owner
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a Fish-style predictive autocomplete system that supports history, file paths, and command names, featuring a filesystem cache and a user toggle via the set predict variable. It resolves a critical Unicode regression by correctly bounding multi-byte character lookahead with MB_CUR_MAX and fixes short-circuit evaluation issues where unset variables previously triggered errors. The changes also include a new regression test suite, refactored syntax highlighting logic to improve accuracy, and various portability enhancements. Review feedback identifies opportunities to improve the predictive cache's handling of PATH changes, refine command validation for suggestions, and extend syntax highlighting to cover additional special variables and redirection operators.
Comment on lines
+4246
to
+4247
| static char trusted_dirs[16][1024]; | ||
| static int trusted_count = 0; |
There was a problem hiding this comment.
The trusted_dirs and trusted_count variables are declared as static within predict_cmd, which means they persist across calls but cannot be reset by predict_cache_clear(). When the shell's PATH is modified (e.g., via set path or rehash), these variables will contain stale directory information, potentially leading to incorrect command predictions or missed optimizations. These should be moved to file scope and reset in predict_cache_clear().
Comment on lines
+4265
to
+4274
| if (trust_readdir) { | ||
| if (nmatch == 0) { | ||
| strncpy(match, name + pfxlen, sizeof(match) - 1); | ||
| match[sizeof(match) - 1] = '\0'; | ||
| nmatch = 1; | ||
| } else if (strcmp(match, name + pfxlen) != 0) { | ||
| /* Different suffix — truly ambiguous. */ | ||
| nmatch = 2; | ||
| break; | ||
| } |
There was a problem hiding this comment.
The trust_readdir optimization is too aggressive. When a directory is "trusted", the code assumes that any file matching the prefix is a valid command, skipping the stat and access(X_OK) checks. This can lead to incorrect suggestions, such as non-executable files or subdirectories (e.g., suggesting X11 in /usr/bin when typing X). Command prediction should always verify that the match is an executable regular file.
| /* $?var / $#var — single-char modifier prefix; absorb and | ||
| * keep state so the trailing name is also coloured. */ | ||
| SyntaxColor[i] = SYN_VARIABLE; | ||
| } else if (ch == '$' || ch == '!' || ch == '<') { |
There was a problem hiding this comment.
The list of single-character special variables is incomplete. In tcsh, $* and $@ are also valid special variables and should be colored as SYN_VARIABLE followed by a transition to ST_NORMAL.
} else if (ch == '$' || ch == '!' || ch == '<' || ch == '*' || ch == '@') {
Comment on lines
+539
to
541
| if (nc == '>' || nc == '<' || nc == '&' || nc == '-' || | ||
| (opener == '>' && (nc == '!' || nc == '|'))) | ||
| SyntaxColor[++i] = SYN_OPERATOR; |
There was a problem hiding this comment.
The redirection operator coloring is too permissive. It allows mixed operators like >< or <> to be colored as a single operator, which are syntactically invalid in tcsh. The continuation check should ensure that subsequent characters match the opening operator (e.g., >> or <<).
if (nc == opener || nc == '&' || nc == '-' ||
(opener == '>' && (nc == '!' || nc == '|')))
SyntaxColor[++i] = SYN_OPERATOR;
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+23
to
+27
| out=$(LANG="$utf8_locale" LC_ALL="$utf8_locale" "$MCSH" -f "$tmpdir/script.csh" 2>&1) | ||
| case "$out" in | ||
| ok) ;; | ||
| *) echo "FAIL: sourced script Unicode: got '$out'"; exit 1 ;; | ||
| esac |
There was a problem hiding this comment.
The failure message says "sourced script Unicode", but the test runs the script via "$MCSH" -f script (script-file mode, not the source builtin). Updating the message would make failures easier to interpret.
| | **#101** (PR) | Medium | `sh.exp.c` | Signed integer overflow: `@ x = (1 << 63)` raises "Badly formed number". Fix: unsigned arithmetic with overflow detection. | | ||
| | **#110** | Medium | `tc.prompt.c` | `%j` job-count in prompt counted all proclist entries. Fix: counts only live job leaders (`p_procid == p_jobid` && `PRUNNING\|PSTOPPED`). | | ||
| | **#107** (PR) | Medium | `sh.exp.c`, `sh.sem.c` | `$?a && "$a" != ""` throws if `a` is unset. Fix: `Dfix()` skips expansion for expression-evaluating builtins; expansion deferred until after short-circuit. | | ||
| | **#107** (PR) | Medium | `sh.exp.c`, `sh.dol.c` | `$?a && "$a" != ""` throws if `a` is unset. Fix: `Dgetdol()` returns STRNULL for unset variables in double-quoted contexts; expansion deferred until after short-circuit. | |
There was a problem hiding this comment.
This PLAN entry says the unset-var fix only returns STRNULL in double-quoted contexts, but the current sh.dol.c logic expands unset variables to empty string for all expansions (quoted and unquoted) and also changes $#var/$%var behavior. Please update this description to match the implemented semantics (or narrow the code change if the intent was limited to quoted/expression contexts).
Suggested change
| | **#107** (PR) | Medium | `sh.exp.c`, `sh.dol.c` | `$?a && "$a" != ""` throws if `a` is unset. Fix: `Dgetdol()` returns STRNULL for unset variables in double-quoted contexts; expansion deferred until after short-circuit. | | |
| | **#107** (PR) | Medium | `sh.exp.c`, `sh.dol.c` | `$?a && "$a" != ""` throws if `a` is unset. Fix: unset-variable expansion now yields empty/`STRNULL` semantics in `Dgetdol()` broadly (not only in double-quoted contexts), so evaluation can be deferred past short-circuiting; this also changes `$#var`/`$%var` behavior for unset variables. | |
| char match[256]; | ||
| int nmatch = 0; | ||
| char fullpath[2048]; | ||
| char path_str[2048]; |
There was a problem hiding this comment.
path_str is declared but never used, which can trigger compiler warnings and makes this code harder to maintain. Please remove the unused variable (or use it if it’s meant to be part of the PATH caching logic).
Suggested change
| char path_str[2048]; |
Comment on lines
+4245
to
+4275
| int trust_readdir = 0; | ||
| static char trusted_dirs[16][1024]; | ||
| static int trusted_count = 0; | ||
| for (int k = 0; k < trusted_count; k++) { | ||
| if (strcmp(trusted_dirs[k], dirpath) == 0) { | ||
| trust_readdir = 1; | ||
| break; | ||
| } | ||
| } | ||
| if (!trust_readdir && trusted_count < 16) { | ||
| strncpy(trusted_dirs[trusted_count], dirpath, sizeof(trusted_dirs[0]) - 1); | ||
| trusted_dirs[trusted_count][sizeof(trusted_dirs[0]) - 1] = '\0'; | ||
| trusted_count++; | ||
| trust_readdir = 1; | ||
| } | ||
|
|
||
| while ((de = readdir(dp)) != NULL && nmatch <= 1) { | ||
| const char *name = de->d_name; | ||
| if (strncmp(name, prefix, pfxlen) != 0) continue; | ||
| if (strcmp(name, prefix) == 0) continue; | ||
| if (trust_readdir) { | ||
| if (nmatch == 0) { | ||
| strncpy(match, name + pfxlen, sizeof(match) - 1); | ||
| match[sizeof(match) - 1] = '\0'; | ||
| nmatch = 1; | ||
| } else if (strcmp(match, name + pfxlen) != 0) { | ||
| /* Different suffix — truly ambiguous. */ | ||
| nmatch = 2; | ||
| break; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
trust_readdir is always set to 1 for the first 16 distinct $PATH directories (and for any directory already in trusted_dirs), so the fast-path never verifies that the candidate is actually executable/regular. This can make the ghost suggestion propose non-executable entries (or directories) as commands. Consider always validating the final candidate with stat/access(X_OK) (or use d_type when available) even when using the readdir-only ambiguity check.
| modifier handling. This covers the core new features but is not exhaustive. Run with: | ||
|
|
||
| ```sh | ||
| make -C tests MCSH=./mcsh check |
There was a problem hiding this comment.
The documented test command uses make -C tests ... check, but there is no tests/Makefile in the tree (only run_tests.sh and the t*.sh scripts). Either add a tests/Makefile that provides check, or update the docs to use the runner directly (e.g. sh tests/run_tests.sh ./mcsh).
Suggested change
| make -C tests MCSH=./mcsh check | |
| sh tests/run_tests.sh ./mcsh |
Comment on lines
+444
to
+460
| ### 3. Test suite — initial suite created (`tests/`) | ||
|
|
||
| `tests/` directory created with 8 regression scripts and a `Makefile`: | ||
|
|
||
| | Script | What it tests | | ||
| |--------|---------------| | ||
| | `t001_vars.sh` | `$mcsh` and `$tcsh` are set on startup | | ||
| | `t002_overflow.sh` | `@ x = (1 << 31)` yields `2147483648` (unsigned left-shift) | | ||
| | `t003_shortcircuit.sh` | `$?a && "$a" != ""` is silent when `$a` unset | | ||
| | `t004_pipe_to_var.sh` | `echo foo \| set x` assigns `x=foo` | | ||
| | `t005_cd_stack.sh` | `pushd`/`cd -1` navigates directory stack correctly | | ||
| | `t006_function_builtin.sh` | `function` builtin stores and executes body | | ||
| | `t007_arith_rsh.sh` | `@ x = (-8 >> 1)` yields `-4` (signed right-shift) | | ||
| | `t008_unset_modifiers.sh` | `${unset:h}` and `$#unset` don't error when var is unset | | ||
|
|
||
| Run with: `make -C tests MCSH=./mcsh check` | ||
|
|
There was a problem hiding this comment.
This section says tests/ includes a Makefile and recommends make -C tests ... check, but tests/Makefile is not present in the repository (only run_tests.sh exists). Please either add the Makefile (with a check target) or adjust the instructions to match the current runner.
Comment on lines
+48
to
+49
| #define GIT_POLL_INTERVAL 2 /* seconds between filesystem mtime polls */ | ||
|
|
There was a problem hiding this comment.
The PR description mentions the git prompt poll interval being configurable, but this change introduces a compile-time constant (#define GIT_POLL_INTERVAL 2). If runtime configurability is intended, consider wiring this to a set variable / env var (with bounds checking) rather than a fixed macro.
orpheus497
added a commit
that referenced
this pull request
May 1, 2026
- ed.chared.c: Remove unused path_str, restore trust_readdir fast-path with final candidate validation - tc.prompt.c: Make git poll interval configurable via GIT_POLL_INTERVAL env var - tests/t014_unicode_script_source.sh: Correct failure message to 'script file' - PLAN.md: Update #107 description to reflect actual sh.dol.c unset variable semantics - README.md, ISSUES.md: Update test runner instructions to use tests/run_tests.sh - tests/t008_unset_modifiers.sh: Simplify grep check
Owner
Author
|
I've pushed a new commit addressing the latest review feedback:
Please take a look! |
- ed.chared.c: Remove unused path_str, restore trust_readdir fast-path with final candidate validation - tc.prompt.c: Make git poll interval configurable via GIT_POLL_INTERVAL env var - tests/t014_unicode_script_source.sh: Correct failure message to 'script file' - PLAN.md: Update #107 description to reflect actual sh.dol.c unset variable semantics - README.md, ISSUES.md: Update test runner instructions to use tests/run_tests.sh - tests/t008_unset_modifiers.sh: Simplify grep check
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation