Exit on first EOF from stdin

[reidwagner]

Addresses #477.

[mziter]

per docs, process::exit:

Note that because this function never returns, and that it terminates the process, no destructors on the current stack or any other thread's stack will be run.

Not sure if this is an issue or not?

[reidwagner]

Thanks for bringing this up! I was hoping process::exit was sufficient for this simple case, but I can see the argument for a stricter exit sequence as well.

This is also how a BrokenPipe error is handled, so there is a precedent.

I'd like to know the maintainer's thoughts as well :).

[sharkdp]

Thank you for working on this.

Yes, returning an Err(..) with a custom error would definitely be the preferred option here, instead of calling process::exit directly.

Using process::exit is probably even wrong here if there are multiple input files...

Speaking of wrong behavior: it would be great if we could add integration/regression tests for this (see tests/integration_tests.rs)

[reidwagner]

Okay, thanks for the input. I'll look into implementing this and include integration tests as well.

[willcassella]

I agree with sharkdp that Err is preferable to exit. But an interesting note is that when using exit you don't actually need a Result, since exit returns never which is implicitly convertible to anything (since the conversion never actually happens since exit... exits).

[reidwagner]

@willcassella I added the Result here to handle read_until()?. Do you think I should have left it as is, discarding the error?

[sharkdp]

@willcassella I added the Result here to handle read_until()?. Do you think I should have left it as is, discarding the error?

I think it should be fine to handle the error with ? here - I originally thought it would cause problems with broken pipes (when the input file only has one line without line ending), but it returns Some(..) in this case.

[reidwagner]

Okay, I created a new ErrorKind variant and essentially just ignore it - would any extra actions need to be taken? The behavior is now correct with multiple input files, e.g. bat foo.txt - foo.txt.

I haven't figured out how to emulate a CTRL-D for an integration test yet.

[sharkdp]

Instead of matching on ErrorKind::ImmediateEOF here and in handle_error, could we also move the handle_error call into the _ => branch?

match error {
    Error(ErrorKind::ImmediateEOF, _) => (),
    _ => {
        handle_error(&error);
        no_errors = false
    },
}

[reidwagner]

Sure - that did seem redundant. I backed it up even further and just matched the Result against the Err(Error(ErrorKind::ImmediateEOF, _)), if that works for you.

[sharkdp]

I haven't figured out how to emulate a CTRL-D for an integration test yet.

You are right, that's probably very tricky. I'd like to have some tests where we check that bat empty test == test, bat test empty == test, bat empty empty == empty, bat test empty test == test + test etc., but that's not really part of this issue (they would have caught the problem with process::exit, though).

[reidwagner]

Makes sense. I can write that up.

[sharkdp]

Thank you for the update!

[willcassella]

I added the Result here to handle read_until()?

Sorry I didn't notice that, I got distracted by how cool never is :)

Thanks for fixing this!