Panick when reporting a warning without a terminal

[tarsius]

I am running git absorb from Emacs, i.e. without a terminal.

This works, but not if it needs to report an error such as (when running in xterm):

$ git absorb -v -b 0364e89a
May 22 17:23:36.703 WARN No additions staged, try adding something to the \
index., line: 251, module: git_absorb

(By the way the exit-code was 0 but should be non-zero to indicate failure. But that's a second issue.)

Running the same in Emacs results in:

$ git absorb -v -b 0364e89a
thread '<unnamed>' panicked at 'slog::Fuse Drain: Custom { kind: Other, error: "term error: operation not supported by the terminal" }', <::std::macros::panic macros>:5:6
stack backtrace:
   0:     0x5614a3276044 - backtrace::backtrace::libunwind::trace::h90669f559fb267f0
                               at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/libunwind.rs:88
   1:     0x5614a3276044 - backtrace::backtrace::trace_unsynchronized::hffde4e353d8f2f9a
                               at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/mod.rs:66
   2:     0x5614a3276044 - std::sys_common::backtrace::_print_fmt::heaf44068b7eaaa6a
                               at src/libstd/sys_common/backtrace.rs:77
   3:     0x5614a3276044 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h88671019cf081de2
                               at src/libstd/sys_common/backtrace.rs:59
   4:     0x5614a329899c - core::fmt::write::h4e6a29ee6319c9fd
                               at src/libcore/fmt/mod.rs:1052
   5:     0x5614a3273ba7 - std::io::Write::write_fmt::hf06b1c86d898d7d6
                               at src/libstd/io/mod.rs:1426
   6:     0x5614a3278235 - std::sys_common::backtrace::_print::h404ff5f2b50cae09
                               at src/libstd/sys_common/backtrace.rs:62
   7:     0x5614a3278235 - std::sys_common::backtrace::print::hcc4377f1f882322e
                               at src/libstd/sys_common/backtrace.rs:49
   8:     0x5614a3278235 - std::panicking::default_hook::{{closure}}::hc172eff6f35b7f39
                               at src/libstd/panicking.rs:204
   9:     0x5614a3277f21 - std::panicking::default_hook::h7a68887d113f8029
                               at src/libstd/panicking.rs:224
  10:     0x5614a327889a - std::panicking::rust_panic_with_hook::hb7ad5693188bdb00
                               at src/libstd/panicking.rs:472
  11:     0x5614a3278480 - rust_begin_unwind
                               at src/libstd/panicking.rs:380
  12:     0x5614a32783fb - std::panicking::begin_panic_fmt::h3f391831d1286dec
                               at src/libstd/panicking.rs:334
  13:     0x5614a3164d8e - <slog::Fuse<D> as slog::Drain>::log::{{closure}}::hbafbb2994d050578
  14:     0x5614a315e3e1 - std::sys_common::backtrace::__rust_begin_short_backtrace::h39ee7db0fa905189
  15:     0x5614a315eff0 - std::panicking::try::do_call::h7a57d4f8e06a8e51
  16:     0x5614a327f587 - __rust_maybe_catch_panic
                               at src/libpanic_unwind/lib.rs:86
  17:     0x5614a315ff17 - core::ops::function::FnOnce::call_once{{vtable.shim}}::h9834a12dbbfa1158
  18:     0x5614a326d14f - <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once::he3e3bc9932f56404
                               at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/liballoc/boxed.rs:1015
  19:     0x5614a327eac0 - <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once::h0d82364c11057a62
                               at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/liballoc/boxed.rs:1015
  20:     0x5614a327eac0 - std::sys_common::thread::start_thread::h6cf2238254b521b3
                               at src/libstd/sys_common/thread.rs:13
  21:     0x5614a327eac0 - std::sys::unix::thread::Thread::new::thread_start::he70a06005b4d03f8
                               at src/libstd/sys/unix/thread.rs:80
  22:     0x7f21c60b8fa3 - start_thread
  23:     0x7f21c5fcf4cf - clone
  24:                0x0 - <unknown>

It appears that it tries to perform an "operation [that is] not supported by the terminal" to show the warning at the very top. I don't know what that operation is, but am guessing that it is not actually necessary. Could this be changed to just use stdout or errout instead of the "unsupported terminal operation"?

[tummychow]

could you be more specific about "running the same in emacs"? i can't reproduce this with a simple M-!, and i don't use magit. if you have a smaller snippet of elisp i can run, that would be ideal

(0 exit code is intentional, and similar to the behavior of git add with no arguments)

[tarsius]

What Magit does boils down to:

(start-file-process "*git-absorb*" "*git-absorb*" "git" "absorb" "-b" "HEAD~10")

[tummychow]

that repros, thanks. this is clearly some kind of bug in how we're using slog... maybe even an upstream bug with the slog term drain. my first instinct is it was something to do with isatty detection (since the logger uses that to determine whether the output should be colored), but it wasn't an issue when redirecting output to a file, so it's probably something more subtle. i'll take a look over the weekend

[tarsius]

As @marsam noted over at magit/magit#3723 (comment), this can be worked around:

(let ((process-environment process-environment))
  (setenv "TERM" nil) ; value before unsetting was "dumb"
  (start-file-process "*git-absorb*" "*git-absorb*"
                      "git" "absorb" "-b" "HEAD~10"))

[tummychow]

hrm, it seems that what's happening here is:

• emacs sets TERM when executing the process
• we build a slog-term decorator, which calls into the term crate (https://github.com/slog-rs/term/blob/v2.3.0/src/lib.rs#L1249), which sees that TERM=dumb and returns a fancy terminal object based on /usr/share/terminfo/d/dumb
• when we try to log to it, the term crate tries to run something on the terminal and returns an error (panic with operation not supported by the terminal while libterm works fine Stebalien/term#73). what precisely i'm not sure yet - my guess is isatty returns true, but then one of the term formatting functions returns an error that it's not supported because of the terminfo for dumb
• the slog-term drain gets the error and returns it, probably in one of these two places https://github.com/slog-rs/term/blob/v2.3.0/src/lib.rs#L1363-L1386
• and git-absorb fused the drain, so errors turn into panics

we could ignore_res the drain instead of fusing it, but that wouldn't really be an improvement... the logs would simply be silenced with no explanation. we can also forcibly disable color (which avoids the codepaths in the term crate that are returning the error), but that's a crummy ux when you're actually on an interactive terminal. i'll see if i can find the place in slog-term that's returning the error and submit an upstream patch for it to fall back to non-fancy-terminal output instead.

[tummychow]

@tarsius give it another try with 0.6.1? shouldn't need the (setenv "TERM" nil) hack now

[tarsius]

I have tried with 0.6.2 and it did not work for me so far.

But the reason for that might simply be that I don't know how to handle cargo properly. I just ran cargo install git-absorb again and that told me that it was:

   ...
   Compiling slog-term v2.5.0
   Compiling failure v0.1.8
   Compiling git2 v0.9.2
   Compiling git-absorb v0.6.2
    Finished release [optimized] target(s) in 55.77s
   Replacing /home/jonas/.cargo/bin/git-absorb
    Replaced package `git-absorb v0.6.2` with `git-absorb v0.6.2` (executable `git-absorb`)

I am confused about the first quoted line. Shouldn't that somehow mention 1a1f6ecaec82c6ade58806e6b315a1e888d9c2b4 due to what you did in 3e14d55? I.e. I am not sure I am actually using the slog-term "version" that contains the fix.

[tummychow]

huh... this is on the fringes of my understanding as well, but it looks like cargo install ignores patches when installing from crates.io. try cloning this repo and running cargo install --path . from inside it? that seems to respect the patch directive. alternatively, if you're on osx, you could wait for someone to bump the sha of the homebrew formula, which will probably also pick up the patch. sorry for the inconvenience either way

[tarsius]

I can confirm that it works.