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

EOF must be sent twice on stdin if no other input is sent #477

Closed
willcassella opened this issue Jan 22, 2019 · 8 comments
Closed

EOF must be sent twice on stdin if no other input is sent #477

willcassella opened this issue Jan 22, 2019 · 8 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@willcassella
Copy link

If you run bat and then immediately press the EOF sequence (Ctrl-D), bat prints the filename 'STDIN' but continues to read from it. You have to press it again to actually close the file.

@sharkdp
Copy link
Owner

sharkdp commented Jan 23, 2019

Hm, interesting. Thank you very much for reporting this.

I guess this is related to the first-line syntax detection.

@sharkdp sharkdp added bug Something isn't working good first issue Good for newcomers labels Jan 23, 2019
@mziter
Copy link

mziter commented Feb 3, 2019

I have found and resolved issue locally. However, I am not sure how to exit immeditately as process::exit(0) will exit immediately and not do any destructuring, etc...

Other two options are 1) Wrap new function in a result, which seems idiomatic, but would require a lot of downstream changes I am sure. 2) Wrap in a new ContentType that would basically get passed on and ignored at some future point.

Would like your thoughts...

@mziter
Copy link

mziter commented Feb 3, 2019

Sorry, just saw this referenced already. :) Will put some comments on pull request.

@sharkdp
Copy link
Owner

sharkdp commented Feb 7, 2019

closed via #492 by @reidwagner

@sharkdp sharkdp closed this as completed Feb 7, 2019
@sharkdp
Copy link
Owner

sharkdp commented Feb 8, 2019

Fixed in v0.10.0.

@reidwagner
Copy link
Contributor

This has been closed - but there was a change in expected behavior, where we no longer get a header and footer for empty input files.

Trying to bring back that behavior while keeping the error logic here seemed to get complex. I think I have a simpler solution all around.

Instead of what's implemented in #492 (that could be reverted), we simply don't call print_file_ranges() if our reader.first_line is empty. That appears to give all the desired functionality, + it prints the STDIN header and footer when we press CTRL-D, which seems more consistent in the first place. e.g in print_file():

if !reader.first_line.is_empty() {
    self.print_file_ranges(printer, writer, reader, &self.config.line_ranges)?;
}

I think maybe the check that first_line.is_empty() could also be removed from read_line since the condition above would work for empty files.

@sharkdp
Copy link
Owner

sharkdp commented Feb 10, 2019

Instead of what's implemented in #492 (that could be reverted), we simply don't call print_file_ranges() if our reader.first_line is empty. That appears to give all the desired functionality, + it prints the STDIN header and footer when we press CTRL-D, which seems more consistent in the first place

Sounds great!

I think maybe the check that first_line.is_empty() could also be removed from read_line since the condition above would work for empty files.

I can't tell, at the moment. We have to make sure to correctly handle (1) empty files (2) files with a single line (3) files with a single line and no \n (4) files with multiple lines (6) files with other encodings (mainly UTF-16 for now). I hope that most of this should be captured by tests, but it might be worth to double-check.

Edit: I have added some additional tests in #502 (not 100% related to this issue)

@reidwagner
Copy link
Contributor

So I opened #504 with the simple change proposed + reverting #492, in case you want to get the fix in soon. As for removing the code from read_line I can double check that before making the change (either added to #504 or another PR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants