Repository navigation
fix: panics on redirect to /dev/full - #261
toggysmith wants to merge 2 commits into
Conversation
This fixes several panics that would occur when the stdout was redirected to /dev/full. This also introduces a new macro for this purpose, named `safe_println!()`, that does not panic in this situation, which can presumably be reused elsewhere this issue may reoccur in the future.
|
This resolves #165 . |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #261 +/- ##
===========================
===========================
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // For the full copyright and license information, please view the LICENSE-* | ||
| // files that was distributed with this source code. | ||
|
|
||
| use crate::safe_println; |
There was a problem hiding this comment.
please add tests in tests/integration.rs for diff and cmp writing to /dev/full (exit code + stderr)
| ); | ||
| } | ||
| println!(); | ||
| safe_println!(); |
There was a problem hiding this comment.
this only works because the print! above is line-buffered
and cmp --help > /dev/full still panics (line 217)
| @@ -91,7 +91,7 @@ pub fn main(opts: Peekable<ArgsOs>) -> ExitCode { | |||
| params.to.to_string_lossy() | |||
There was a problem hiding this comment.
what about this one and maybe_report_identical_files? diff -q a b > /dev/full still panics.
same for -y, side_diff still unwraps.
| ); | ||
| } else { | ||
| io::stdout().write_all(&result).unwrap(); | ||
| let _ = io::stdout().write_all(&result); |
There was a problem hiding this comment.
ignoring the error isn't right either
GNU diff prints a write error and exits with 2 here, we would exit 0/1 silently
could you please propagate the error instead?
| }}; | ||
| } | ||
|
|
||
| /// A safe version of println!() that does not panic if stdout is redirected to /dev/full |
There was a problem hiding this comment.
a println! that swallows errors will hide real failures, no?
i would rather return the io::Result and handle it in the caller
| println!("Usage: {name} [function [arguments...]]\n"); | ||
| println!("Currently defined functions:\n"); | ||
| println!(" cmp, diff\n"); | ||
| safe_println!("{name} {VERSION} (multi-call binary)\n"); |
There was a problem hiding this comment.
this changes the output: println!("...\n") printed two newlines, is safe_println! doing the same?
please add a test either way
This fixes several panics that would occur when the stdout was redirected to /dev/full. This also introduces a new macro for this purpose, named
safe_println!(), that does not panic in this situation, which can presumably be reused elsewhere this issue may reoccur in the future.