Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unfill and refill crashes #467

Merged
merged 3 commits into from
Sep 15, 2022
Merged

Fix unfill and refill crashes #467

merged 3 commits into from
Sep 15, 2022

Conversation

mgeisler
Copy link
Owner

This fixes two bugs in unfill, both to do with the way we were slicing strings in cases where the input ended with a combination of \r and \n.

Fixes #466.

The crash is actually in `unfill`. After processing the first line, we
would erroneously use the `options.initial_indent` to index into the
second line. This lead to a panic when the second line is shorter than
the first line.
@mgeisler
Copy link
Owner Author

Hi @edio, do you remember why the iterator from #454 only iterated over non-empty lines?

Test that we don't crash on a `\r` following a string of `\n`. The
problem was that we removed both kinds of characters in one code path,
but not in the other. We now use the same `text` in both places and
this fixes the issue.

The fix changed the handling of newlines slightly: we now normalize
the newlines to a single `\n` or `\r\n` depending on what we detect in
the input. The documentation was updated to explicitly say that
passing in multiple paragraphs result in unspecified behavior.
@mgeisler mgeisler merged commit 5457204 into master Sep 15, 2022
@mgeisler mgeisler deleted the fix-refill-crashes branch September 15, 2022 20:33
@github-actions github-actions bot mentioned this pull request Sep 15, 2022
@koiuo
Copy link

koiuo commented Oct 9, 2022

@mgeisler , sorry for the late response.

It's hard to recall now, but I guess it was to allow unfill to operate on multiple paragraphs

paragraph1
text text

pargraph2
text text

would be unfilled into

paragraph1 text text pargraph2 text text

That's the only explanation I can think of right now 🙈

@mgeisler
Copy link
Owner Author

Hi @edio,

Thanks, multiple paragraphs is something we might want to support one day... I think that would name unfill consistent with how fill works.

This was referenced Oct 24, 2022
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.

Bugs with refill
2 participants