walles / riff

A diff filter highlighting which line parts have changed
MIT License
253 stars 5 forks source link

Riff Crashed during `git stash show -p` #16

Closed rjmarwil closed 3 years ago

rjmarwil commented 3 years ago

------------ RIFF CRASHED ------------------- 0: backtrace::backtrace::trace 1: backtrace::capture::Backtrace::new 2: riff::main::{{closure}} 3: std::panicking::rust_panic_with_hook 4: std::panicking::begin_panic_handler::{{closure}} 5: std::sys_common::backtrace::__rust_end_short_backtrace 6: _rust_begin_unwind 7: core::panicking::panic_fmt 8: core::result::unwrap_failed 9: riff::highlight_diff 10: riff::try_pager 11: riff::main 12: std::sys_common::backtrace::__rust_begin_short_backtrace 13: std::rt::lang_start::{{closure}} 14: std::rt::lang_start_internal 15: _main

Riff version: 2.0

rjmarwil commented 3 years ago

Here's a link to the gist which can be used to reproduce by piping into riff https://gist.github.com/rjmarwil/7b19d3d6e9f3b340b28ce4b35e439491

walles commented 3 years ago

Do you think you could try this with riff 2.5?

https://github.com/walles/riff/releases/tag/2.5

I have tried and failed to repro this on macOS (where I develop riff) and in a Ubuntu Docker image using the diff file you provided in your gist.

riff 2.5 has better crash reporting (line numbers included), and at least knowing which line this crashes on would be a good help in trying to track this down.

rjmarwil commented 3 years ago

I was able to reproduce the error using riff 2.5. Here is the crash report:

-v-v-v----------- RIFF CRASHED ---------------v-v-v-

Panic message: <PanicInfo {
    payload: Any,
    message: Some(
        called `Result::unwrap()` on an `Err` value: Os { code: 32, kind: BrokenPipe, message: "Broken pipe" },
    ),
    location: Location {
        file: "src/main.rs",
        line: 93,
        col: 39,
    },
}>

   0: backtrace::backtrace::trace
   1: backtrace::capture::Backtrace::new
   2: riff::main::{{closure}}
   3: std::panicking::rust_panic_with_hook
   4: std::panicking::begin_panic_handler::{{closure}}
   5: std::sys_common::backtrace::__rust_end_short_backtrace
   6: _rust_begin_unwind
   7: core::panicking::panic_fmt
   8: core::option::expect_none_failed
   9: riff::highlight_diff
  10: riff::try_pager
  11: riff::highlight_stream
  12: riff::main
  13: std::sys_common::backtrace::__rust_begin_short_backtrace
  14: std::rt::lang_start::{{closure}}
  15: std::rt::lang_start_internal
  16: _main

Riff version: 2.5

Command line arguments: Args { inner: ["riff"] }

-^-^-^------- END OF RIFF CRASH REPORT -------^-^-^-
walles commented 3 years ago

Thanks for retesting with 2.5 @rjmarwil, this clears things up.

Same as #10, this should definitely be fixed.

walles commented 3 years ago

For context btw, this is what is happening:

  1. You start riff.
  2. riff launches your pager, most likely less
  3. riff starts piping the highlighted diff into your pager
  4. Your pager exits before riff is done piping all the diff (this is fine, you probably just quit it on purpose)
  5. riff receives an EPIPE result from writing to its pipe to the pager, because that pipe just broke (message: "Broken pipe" in the crash report)
  6. riff crashes because it doesn't handle EPIPE properly (at all actually)

So workarounds until this is fixed include:

  1. Switch pagers to a pager that reads all of the pipe at once
  2. Scroll to the end in your pager before exiting it
  3. Just do nothing and ignore this error for now
walles commented 3 years ago

Fixed by e1edb46f19ca3da427d658a232cf1e365bf5b146.

walles commented 3 years ago

Just released version 2.6 with this issue fixed: https://github.com/walles/riff/releases/latest

Thanks for reporting @rjmarwil!

rjmarwil commented 3 years ago

Wow, thank you for fixing so fast @walles! I've tested with version 2.6 and I am no longer receiving an error.

Much appreciated!