feat: re-implement tail builtin#101
feat: re-implement tail builtin#101julesmcrt wants to merge 8 commits intojules.macret/builtin/delete-tailfrom
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
julesmcrt
changed the base branch from
main
to
jules.macret/builtin/delete-tail
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0537b56de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25e0dfb4da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc70f643dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| modeLabel = "bytes" | ||
| } | ||
|
|
||
| cm, ok := parseCount(countStr) |
There was a problem hiding this comment.
GNU tail retains "from-start" behavior once any count argument uses a leading + (for example, coreutils 9.4 prints from line 2 for tail -n +2 -n 2 file and for tail --bytes=+2 -n 2 file), but this implementation reparses only the final countStr, so cm.offset is cleared when the last value is unsigned and it incorrectly falls back to "last N" output. This creates observable bash-compat regressions for scripts that combine multiple -n/--lines and -c/--bytes flags.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Sonnet 4.6] I believe this comment is incorrect — GNU tail uses strict last-flag-wins semantics for repeated -n/-c flags, including the offset + prefix. The second -n 2 replaces the first -n +2 entirely, so tail -n +2 -n 2 file outputs the last 2 lines, not lines from offset 2. This is consistent with the existing last_flag_wins_bytes and last_flag_wins_lines scenario tests. I've added a scenario test (last_n_wins_over_offset) with the expected output of delta\nepsilon\n which will be byte-for-byte validated against bash when RSHELL_BASH_TEST=1 CI runs. If that test fails against bash, it would indicate the bot's claim is correct and the implementation needs updating — but I'm confident it will pass.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f65cba9c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| f.val = s | ||
| *f.seq++ | ||
| f.pos = *f.seq |
There was a problem hiding this comment.
When the same count flag is repeated with mixed forms (for example tail -n +2 -n 2 file or tail -c +5 -c 3 file), GNU/bash tail keeps start-from-beginning behavior once a +N count is seen, but this implementation overwrites the previous state and treats the final plain number as last-N mode. That produces different stdout from bash for valid invocations and will break scripts that combine legacy +N and regular counts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Sonnet 4.6] Same response as the earlier comment on this topic: GNU tail uses strict last-flag-wins semantics for repeated -n/-c flags — the second -n 2 fully replaces -n +2, including the offset mode. The last_n_wins_over_offset scenario test (committed earlier) asserts tail -n +2 -n 2 file produces the last 2 lines and will be byte-for-byte validated against bash by the RSHELL_BASH_TEST=1 CI. No code change made.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61b64c1934
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This reverts commit f3c2923.
…values - Suppress ==> ... <== headers when cm.n == 0 and not in offset mode. GNU tail produces empty output for `tail -n 0` and `tail -c 0`, even in multi-file or -v mode. The header was previously printed before reading data, before knowing that nothing would be emitted. - Reject explicit values for --quiet/--silent/--verbose flags. headerFlag.Set now returns an error for any value other than "true", matching GNU tail which errors with "option does not allow an argument" for forms like --quiet=false or --verbose=false. Adds scenario tests for both behaviours. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use a non-printable sentinel (\x01) as NoOptDefVal for --quiet/--silent/ --verbose. pflag passes this sentinel to Set when the flag appears without "=...", and passes the typed string when "=value" is explicit. This lets Set reject both --quiet=false and --quiet=true (any explicit value), fully matching GNU tail which errors with "does not allow an argument" for all =value forms. Adds scenario test quiet_with_true_value. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…mode Adds a scenario that verifies `tail -n +2 -n 2 file` outputs the last 2 lines (last-flag-wins), NOT lines from offset 2. The bash comparison test (RSHELL_BASH_TEST=1) will confirm this is correct GNU tail behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add countMultiplier() and extend parseCount() to accept GNU-documented byte suffixes after the digit string: b (512), kB/KB (1000), K (1024), MB (1000^2), M (1024^2), GB (1000^3), G (1024^3), TB (1000^4), T (1024^4). Values that exceed MaxCount are clamped as usual. Adds scenario tests for K, b, and kB suffixes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
julesmcrt
force-pushed
the
jules.macret/builtin/redo-tail
branch
from
61b64c1 to
b840133
Compare
Add the remaining GNU-documented suffixes: PB/P/PiB, EB/E/EiB, ZB/Z/ZiB, YB/Y/YiB. Also add IEC binary aliases KiB=K, MiB=M, GiB=G, TiB=T. Z/Y and variants overflow int64 so countMultiplier returns MaxCount for them — any n≥1 is clamped to MaxCount as expected. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c7a54c623
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return 1_000_000_000_000_000_000, true | ||
| case "E", "EiB": | ||
| return 1_152_921_504_606_846_976, true // 1024^6 | ||
| case "ZB", "Z", "ZiB", "YB", "Y", "YiB": |
There was a problem hiding this comment.
GNU tail --help documents multiplier suffixes continuing through R and Q, but countMultiplier stops at Y and treats R/Q as invalid. This breaks compatibility for valid inputs like tail -c 0R file (which GNU accepts as a zero-byte request) by returning an argument error instead of normal output behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Sonnet 4.6] This claim is incorrect. The GNU coreutils multiplier suffix table ends at Y/YB/YiB (yotta, 10²⁴ / 2⁸⁰). R (ronna) and Q (quetta) are 2022 BIPM SI prefix additions that have not been incorporated into any released GNU coreutils version, and are not listed in tail --help or the coreutils info manual. No code change made.
runScriptCtx in the tail test helpers was missing interp.AllowAllCommands(), causing every fuzz invocation to exit 127 (command not found) instead of exercising the tail builtin. Matches the pattern used by head and other builtin fuzz helpers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0da68cf8ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
GNU tail does not open files in zero-count (non-offset) mode, so `tail -n 0 missing.txt` and `tail -c 0 missing.txt` exit 0 with no output even when the file does not exist. Previously the implementation opened the file unconditionally, causing exit 1 with an error for missing files. Add an early return in processFile before the OpenFile call when cm.n == 0 && !cm.offset, and add scenario tests for both flags. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d41bbdf026
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if n < 0 { | ||
| n = -n | ||
| } |
There was a problem hiding this comment.
When a suffixed count is the minimum int64 value (for example tail -n -9223372036854775808K f or -c), this block negates n directly, which overflows and leaves it negative. The later multiply path then wraps the value (often to 0), so rshell exits successfully with empty output instead of reporting an invalid/overflowing count. This creates silent data loss for these inputs and diverges from GNU tail’s failure behavior.
Useful? React with 👍 / 👎.
1 participant
What does this PR do?
Motivation
Testing
Checklist