🐛 delta 0.8.3 leaves terminal in an inconsistent state on exit

[abbeyj]

This seems similar to #463, but can still be reproduced with delta 0.8.3. Using CentOS 7, less version 458 from package less-458-9.el7.x86_64.

Run delta with no command line arguments. It will print out the message:

The main way to use delta is to configure it as the pager for git: see https://github.com/dandavison/delta#configuration. You can also use delta to diff two files: `delta file_A file_B`.

It will exit and return to the shell, but the shell prompt will not be drawn.

When using delta --paging=never, the problem does not occur.

There seems to be a race condition Sometimes this problem will occur the first time I run delta, but other times it will work correctly. Running delta 3 or 4 times in a row will usually reproduce the problem.

Even more rarely, the terminal will be left with echo turned off (like stty -echo). This can be reproduced by running delta a bunch of times in a row, e.g.

for i in $(seq 1 100); do delta; done; echo Done

This will run delta 100 times, print out "Done" and then return to the shell. But sometimes echo will be turned off and you will not be able to see what you're typing in your shell.

A possibly related problem happens when using Ctrl+C to exit delta. Run yes | delta so that you are assured of having more than a page of output. At the less prompt, press Ctrl+C. Now delta will exit, and the shell prompt will be drawn. But less --RAW-CONTROL-CHARS --no-init --quit-if-one-screen is still running in the background (reparented to pid 1). The next keystroke is processed by less. Trying a newer version of less (581.2, compiled from source), shows similar symptoms. The command left running in the background is instead less --RAW-CONTROL-CHARS --quit-if-one-screen. And now you're left on the terminal's "alternate screen" until less exits. Then you're returned to the "normal screen". Let me know if you'd prefer that I open a separate bug for this issue.

I think the root cause here might be the same. You launch less but there are some situations where you do not wait for it to exit before exiting yourself. I think the invariant needed here is that once the pager is started you cannot exit until it has exited. I'm not sure of the best way to accomplish that. It seems like any place that you use process::exit in the code is a place where this bug may potentially occur. Any fatal signal would seem to have the same issue. You could perhaps at least work around the Ctrl+C issue by ignoring SIGINT.

[Kr1ss-XD]

Now delta will exit, and the shell prompt will be drawn. But less [...] is still running in the background (reparented to pid 1). The next keystroke is processed by less.

For ref.:

• This did also happen here, as I was reproducing 🐛 delta hangs when comparing certain files #683, after sending SIGINT. (Archlinux, delta master, less 590)

[thiagomg]

It also happened to me.

$ delta
The main way to use delta is to configure it as the pager for git: see https://github.com/dandavison/delta#configuration. You can also use delta to diff two files: `delta file_A file_B`.
$ ls
<redacted>
$ $ $ $ $ $ $ $ 

And then after the next command (ls), the terminal was broken.

[dandavison]

@abbeyj Thanks very much for the careful writeup and for the reminder that there are still problems here. I believe that I've fixed the first bug (running delta with no arguments corrupts terminal) via #685. If you able to build delta from the git repo and confirm, that would be fantastic.

However, the yes | delta ctrl-c bug remains. Perhaps as you suggested, you could copy your analysis there into a new ticket? Edit I believe it's fixed at #686

Incidentally, I was unable to reproduce either of these bugs on MacOS. But I was able to reproduce them in a docker container (based on an ubuntu image).

[Kr1ss-XD]

I believe that I've fixed the first bug (running delta with no arguments corrupts terminal) via #685.

Yes @dandavison - just rebuilt from master, and that one is fixed. 👍

[thiagomg]

I built from source and it worked. Thanks much !!! (I am using Ubuntu 20.04.2 LTS)

[dandavison]

@thiagomg @Kr1ss-XD Great!

And also following @abbeyj's suggestion, #686 makes delta ignore ctrl-c (SIGINT). This seems to fix the yes | delta followed by ctrl-c bug on Ubuntu in a docker container. If anyone would like to test that, the branch is named handle-sigint.

I need to refresh myself on exactly how this works. Perhaps someone could tell me where the following is wrong (@abbeyj ?).

I'm thinking that less itself is ignoring SIGINT, and thus the signal is propagated to the parent delta process, which prior to this change handled it by terminating itself, leaving the less process orphaned but running. So this change makes ctrl-c do nothing when delta output is being paged to less, which is what a less user expects (they use q to exit).

I was initially worried that this change might be annoying when one accidentally sends git log -p to delta with the pager deactivated (i.e. tons of commits flying past), but I believe that similar logic applies: delta now ignores the signal, it is propagated to the parent git process, which does terminate, and thus breaks the pipe to delta and delta exits. (I think I'm slightly unclear why delta exits when this pipe is broken but less does not.)

[Kr1ss-XD]

If anyone would like to test that, the branch is named handle-sigint.

Looks good, indeed.

I'm right now reproducing the situation from #683 again, using a build of the handle-sigint branch. Now, after sending SIGINT or pressing CTRL-C, less stays under the same parent process (which is delta), the git diff process terminates, and the delta-formatted diff output is displayed by less.

EDIT/ Same behavior with yes | delta btw. 👍

[Kr1ss-XD]

I'm thinking that less itself is ignoring SIGINT

FWIW, there's a flag that makes less terminate on SIGINT / CTRL-C : -K or --quit-on-intr. From the manual :

Causes less to exit immediately (with status 2) when an interrupt character (usually ^C) is typed. Normally, an interrupt character causes less to stop whatever it is doing and return to its command prompt. Note that use of this option makes it impossible to return to the command prompt from the "F" command.

Maybe this could help ? I think by the way that when less is in command prompt mode, it actually ignores that signal (given the -K option is not provided). Don't know that for sure though.