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

Gracefully handle (ignore) null bytes in history lines #1790

Merged
merged 1 commit into from Oct 12, 2018

Conversation

Projects
None yet
2 participants
@owst
Contributor

owst commented Oct 9, 2018

Fixes #1789.

Readline is unable to add lines to its history that contain a null byte;
we should therefore avoid saving such lines to the history file, and
ignore any such lines that are already present in the file.

Gracefully handle (ignore) null bytes in history lines
Fixes #1789.

Readline is unable to add lines to its history that contain a null byte;
we should therefore avoid saving such lines to the history file, and
ignore any such lines that are already present in the file.
@@ -34,6 +34,8 @@ def restore_default_behavior
# @return [Integer] The number of lines loaded
def load
@loader.call do |line|
next if invalid_readline_line?(line)

This comment has been minimized.

@kyrylo

kyrylo Oct 10, 2018

Member

I wonder if we should substitute \0 with a blank instead? Like you said in #1789 you typed that by accident, right?

This comment has been minimized.

@owst

owst Oct 10, 2018

Contributor

When you say "substitute \0 with a blank" do you mean "we could delete \0s" or do you mean "we could replace each \0 with a (a space)"? I think you might mean the latter, since in #1789 I said I typed ctrl-space instead of space to generate \0.

I see some issues:

  1. In general a null byte bears no relation to a space - ctrl-space is one keypress sequence to type a literal NULL byte, for me on Linux, the sequence ctrl-v, ctrl-shift-' also enters a NULL byte, and there's likely other ways too
  2. There's no guarantee that the NULL byte appears in a position where adding a space is syntactically valid (e.g. x\0y = 1)
  3. The simple symmetry between "don't save invalid lines" and "don't load existing invalid lines" would be broken

What do you think? I think the behaviour as-is in this PR is simple and likely to be most-often correct vs attempting to modify invalid lines

This comment has been minimized.

@kyrylo

kyrylo Oct 10, 2018

Member

I think you might mean the latter [...]

That's correct. This is what I meant.

Thanks for the analysis, I agree with this decision. I was just wondering about the "what-if" but your points sound very convincing.

@kyrylo

kyrylo approved these changes Oct 12, 2018

@kyrylo kyrylo merged commit 888e5e3 into pry:master Oct 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment