Skip to content

Skip leading newlines before applying the limit in paragraph mode gets - #230

Open
Shaisolaris wants to merge 1 commit into
ruby:masterfrom
Shaisolaris:fix-paragraph-gets-limit
Open

Shaisolaris wants to merge 1 commit into
ruby:masterfrom
Shaisolaris:fix-paragraph-gets-limit

Conversation

@Shaisolaris

Copy link
Copy Markdown

In paragraph mode with a limit, StringIO#gets can return nil while data remains. The leading-newline skip runs inside the window already cut to limit, so when the bytes at the read position within the limit are all newlines, it reports end of data:

io = StringIO.new("a\n\n\n\nb\n\n\nc")
io.gets("", 1) #=> "a"
io.gets("", 1) #=> nil   (expected "b"; File#gets("", 1) returns "b", then "c")

Because of this, each_line("", limit) and readlines("", limit) stop early too.

This change skips the leading newlines against the whole remaining string first, then applies the limit from the new start. When only newlines remain, the position is now moved past them before returning nil, as IO#gets("") does, so eof? is true afterwards. Previously pos stayed put. That is the only change in paragraph mode without a limit. The same change is made in the JRuby implementation.

Tests:

  • test_gets_paragraph_with_limit and test_gets_paragraph_only_newlines fail on master (nil instead of "b"; pos 0 instead of 3) and pass with the change.
  • A generated check, reading 304 inputs with limits 1, 2, 3 and 7 until nil, found data left unread in 944 of 1216 runs on master and 0 with the change.
  • Comparing paragraph-mode results before and after over 4040 input, limit and chomp combinations: without a limit, the only differences are the pos move on newline-only remainders.
  • test/stringio/test_*.rb with the C extension locally on Ruby 3.3: no new failures. The same 3 tests that need the core assertion helpers error with and without the change in my setup.

Not tested: I could not build or run the JRuby extension locally. The Java change mirrors the C one and relies on CI.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant