fix: support fractional profile skips on pipes #1392

Merged

JooaoPedro15 merged 2 commits from fix/profile-skip-pipes into main

2026-11-10 04:26:02 +00:00

JooaoPedro15 commented

2026-11-10 00:16:45 +00:00

(Migrated from github.com)

After #1094, a fractional skip on a pipe reads the whole stream and then fails when it tries to rewind. For nonseekable input with skip, spool the lines to a temporary file while counting them, then apply the existing slice. Regular captures still use the two-pass reader and a pipe with zero skip streams directly.

The regression uses a real pipe and covers CRLF, blank lines, a final line without a newline, and positive and negative skips. The old code fails on the nonzero cases. The game-free suite and a /dev/stdin CLI check passed on macOS.

AI assistance from OpenAI Codex is recorded in the commit trailers.

After #1094, a fractional `skip` on a pipe reads the whole stream and then fails when it tries to rewind. For nonseekable input with `skip`, spool the lines to a temporary file while counting them, then apply the existing slice. Regular captures still use the two-pass reader and a pipe with zero skip streams directly. The regression uses a real pipe and covers CRLF, blank lines, a final line without a newline, and positive and negative skips. The old code fails on the nonzero cases. The game-free suite and a `/dev/stdin` CLI check passed on macOS. AI assistance from OpenAI Codex is recorded in the commit trailers.

NotRexed commented

2026-11-10 03:50:10 +00:00

(Migrated from github.com)

This landed in the v2.0.12 branch (merged there for testing; it reaches main with that release), so closing. Thanks!

This landed in the v2.0.12 branch (merged there for testing; it reaches `main` with that release), so closing. Thanks!

NotRexed commented

2026-11-10 03:54:51 +00:00

(Migrated from github.com)

Reopened: closing this was a mistake on my side. It was tested in the v2.0.12 branch, and it will be merged into main here, so it shows as merged. Sorry for the noise.

hckrman101 (Migrated from github.com) approved these changes

2026-11-10 04:18:14 +00:00

hckrman101 (Migrated from github.com) left a comment

Looks good. I walked through the new pipe branch and the skip arithmetic holds up:

Empty input leaves count at 0, so start is 0 and nothing is yielded.
A final line without a newline is spooled as-is, and the line count matches the regular-file path.
Universal newlines on the read side turn CRLF into \n before the spool, so the two paths see the same physical lines.
Positive, negative and oversized skips go through the same start clamping as before.
f.seek(0) on the spool plus the ExitStack closes everything on both normal exit and early generator close.
Checking S_ISREG before seekable() is the right order for the Windows case you describe.

The test is already picked up by run_all.py's tests/test_*.py glob, and CI is green on both Python jobs. The Windows Vulkan and F2I jobs were still pending when I looked, but nothing here touches them.

Nit: the pipe test patches seekable() to True, which exercises the S_ISREG half of the check but not a genuinely non-seekable pipe. That is fine, since the S_ISREG half is the one that matters on Windows. Thanks for the fix.

Looks good. I walked through the new pipe branch and the skip arithmetic holds up: - Empty input leaves count at 0, so start is 0 and nothing is yielded. - A final line without a newline is spooled as-is, and the line count matches the regular-file path. - Universal newlines on the read side turn CRLF into \n before the spool, so the two paths see the same physical lines. - Positive, negative and oversized skips go through the same start clamping as before. - f.seek(0) on the spool plus the ExitStack closes everything on both normal exit and early generator close. - Checking S_ISREG before seekable() is the right order for the Windows case you describe. The test is already picked up by run_all.py's tests/test_*.py glob, and CI is green on both Python jobs. The Windows Vulkan and F2I jobs were still pending when I looked, but nothing here touches them. Nit: the pipe test patches seekable() to True, which exercises the S_ISREG half of the check but not a genuinely non-seekable pipe. That is fine, since the S_ISREG half is the one that matters on Windows. Thanks for the fix.

Commenting is not possible because the repository is archived.

No reviewers

hckrman101