Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 delta 0.4.5 binary release messes up terminal on Ubuntu 20 04 #463

Closed
salmankhilji opened this issue Dec 24, 2020 · 15 comments · Fixed by #469
Closed

🐛 delta 0.4.5 binary release messes up terminal on Ubuntu 20 04 #463

salmankhilji opened this issue Dec 24, 2020 · 15 comments · Fixed by #469

Comments

@salmankhilji
Copy link

Release 0.4.5, when run on Ubuntu 20 04, messes up the terminal. After running delta, I see the diff output; however, any subsequent typing within the terminal is not echo'd back.

salman@byteevo-scm:~/Scratch$ ls update-pcs*
update-pcs  update-pcs-new
salman@byteevo-scm:~/Scratch$ ./delta -s update-pcs update-pcs-new 

comparing: update-pcs ⟶   update-pcs-new
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

│ 1  │#!/bin/sh                                                │ 1  │#!/bin/sh
│ 2  │for i in $(seq 3); do                                    │ 2  │for i in $(seq 4); do
│8m 3  │    rsync -av -e "ssh -i $HOME/.ssh/id_rsa" $HOME/.local→│ 3  │    rsync -av -e "ssh -i $HOME/.ssh/id_rsa" $HOME/.local→
│ 4  │    rsync -av -e "ssh -i $HOME/.ssh/id_rsa" $HOME/.local→│ 4  │    rsync -av -e "ssh -i $HOME/.ssh/id_rsa" $HOME/.local→
│ 5  │done                                                     │ 5  │done
salman@byteevo-scm:~/Scratch$ 
Command 'lll' not found, did you mean:

  command 'dll' from deb brickos (0.9.0.dfsg-12.2)
  command 'lld' from deb lld (1:10.0-50~exp1)
  command 'lli' from deb llvm-runtime (1:10.0-50~exp1)
  command 'llt' from deb storebackup (3.2.1-1+deb8u1build0.20.04.1)
  command 'llc' from deb llvm (1:10.0-50~exp1)

Try: sudo apt install <deb name>

salman@byteevo-scm:~/Scratch$ delta 0.4.5
salman@byteevo-scm:~/Scratch$ 

As you can see, I intentionally typed a non-existant command lll, which is not echo'd back. Then I typed ./delta --version, which is also not echo'd back. The result of the command is, however, seen in the output above.

@MarcoIeni
Copy link
Contributor

I was able to reproduce it.
By the way with zsh it works.

@dandavison
Copy link
Owner

Hi @MarcoIeni. Do you understand what's happening here? I was not able to reproduce this in a docker container.

@MarcoIeni
Copy link
Contributor

I tried with cargo install git-delta and I have the same problem. So it's not a problem of the released binary only.

I will patiently go back commit by commit to identify where the problem started :)

@dandavison
Copy link
Owner

Thanks very much @MarcoIeni! I'm on MacOS (and can run in Docker also) and cannot reproduce, but I will try to think what might be going wrong. Here are some questions in case they help pinpoint the problem

  • Does it depend on the terminal emulator being used?
  • Does the problem still occur with --paging=never?
  • Does it occur with and without side-by-side?

@MarcoIeni
Copy link
Contributor

  • Does it depend on the terminal emulator being used?

Default ubuntu terminal emulator (gnome-terminal): works with zsh and doesn't work with bash.

  • Does the problem still occur with --paging=never?

no!

  • Does it occur with and without side-by-side?

yes, both cases

@dandavison
Copy link
Owner

dandavison commented Dec 24, 2020

Does the problem still occur with --paging=never?

no!

Interesting! What are your values of the DELTA_PAGER, BAT_PAGER, PAGER and LESS env vars?

@MarcoIeni
Copy link
Contributor

they are empty

image

@MarcoIeni
Copy link
Contributor

image

as you can see with paging=never it works. Otherwise it doesn't echo your input. I don't know why it added an 'l' at the end

@dandavison
Copy link
Owner

OK, if you could determine the first bad commit that would be fantastic.

@MarcoIeni
Copy link
Contributor

MarcoIeni commented Dec 24, 2020

It looks like the problem was introduced in 46eb84b

Also, the problem does not appear if diffs are displayed with a pager (less in my case).

Reproduction guide. Ubuntu 20.04, gnome terminal, bash:

git clone https://github.com/dandavison/delta
cd delta
# the following command will revert to last working state
git checkout a632e3f704d8e5b10326f6d79ad58807f2fa103d
cargo build --release
./target/release/delta file1 file2
# now if you try to type something you should be able to see what you type

# the following command will go to the point where the problem was introduced
git checkout 46eb84bd34f8ff67f749eec8a287d5a3e7a0919a
cargo build --release
./target/release/delta file1 file2
# now if you try to type something you should NOT be able to see what you type

@salmankhilji can you confirm my reprodution guide?

@dandavison
Copy link
Owner

Thanks @MarcoIeni, I think I can see that this is broken on MacOS, even though it isn't messing up my terminal: when I do
delta Cargo.toml /etc/passwd I do not see the full output (I only see the red removed lines from Cargo.toml). But when I do delta --paging=never Cargo.toml /etc/passwd I see the full output (removed and added lines).

I'm guessing I wrote some very bad process management code in 46eb84b and there's a race condition somewhere between delta, diff and less, resulting in some output being truncated.

@dandavison
Copy link
Owner

Thanks very much for debugging @MarcoIeni!

@MarcoIeni
Copy link
Contributor

I am trying to find a solution as well, I don't get why it doesn't work <.<

@MarcoIeni
Copy link
Contributor

MarcoIeni commented Dec 24, 2020

the problem is that you don't have to pass the writer variable to that function.

I am trying to solve the problem against master and doing a PR.

The problem is: that variable is required by tests

dandavison added a commit that referenced this issue Dec 26, 2020
This does not reproduce bug #463 (at least, not reliably).
dandavison added a commit that referenced this issue Dec 26, 2020
dandavison added a commit that referenced this issue Dec 26, 2020
@dandavison
Copy link
Owner

0.5.0 was just released and contains the fix for this issue. Thanks very much @salmankhilji for reporting this with helpful detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants