Fix issue #3308

[eldios]

• PR Description

It seems that setting TERM=dumb breaks compatibility with curses
subprocess in Go, especially with pinentry-curses

This fixes #3308

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• Docs (specifically docs/Config.md) have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[eldios]

possibly also fixed #1146 if it's still an issue (can't repro on my systems).

[stefanhaller]

This doesn't look right to me. I don't think we want to set TERM to linux on platforms other than Linux.

But then it also seems like this is only a hacky workaround for a problem that needs to be solved in a very different way. If I understand the circumstances of this bug correctly, then what happens here is that pinentry-curses tries to take over the terminal at a moment where lazygit didn't suspend itself to the background. This is bad, we don't want two programs to fight for the UI at the same time. But actually it sounds like user error to me, because you are setting overrideGpg to true under circumstances where you shouldn't.

Or am I misunderstanding something here?

[eldios]

I tried both with and without the overrideGpg setting and the error of broken curses always show up with the TERM=dumb setup.

also, setting TERM=linux doesn't mean you're actually running Linux, it's just specifying a collection of TERM capabilities that are (usually) supported by Linux terminals.
Anyway if that leads to confusion, any other TERM definition that supports enough capabilities and is not as "stripped-down" as dumb would work. (ie term-256-colors).

You should be able to find an alternative collection by following what's in your man terminfo page.

But seeing this issue in other ways, this is creating a huge disservice to people as we're not able to sign via nor use GPG-agents for SSH as it breaks the lazygit interface when it asks for the password via pinentry-curses and I don't see any drawback in setting TERM to a more capable value over dumb.

[stefanhaller]

Look, I understand that this is an important issue for you, and I'm all for fixing it (the right way). The fix you are proposing is just not the right one; let's look for a proper fix instead.

Lazygit already has support for gpg-signing commits. When it sees that you have commit.gpgsign in your config, it suspends itself to the background when you commit so that you can type in your passphrase or whatever is needed (I have no experience with gpg myself, so my understanding is a bit sketchy here). It would be good if you could confirm that this works with pinentry-curses.

Now, it sounds like you are getting the error when you push, is that right? Do you have push.gpgSign in your git config? If that's the problem, then we should fix it by handling it in the same way as we do for committing.

[eldios]

Totally agree in finding the right solution, and I still believe the problem IS the TERM variable. But back to your questions first.

unfortunately neither commit, pull or push work when a password is requested via pinentry-curses. The observed problem is that the UI gets all jumbled and broken as depicted in the image shown in #3308

The process is what you mentioned, so that when you need to type the password for some actions (commit, pull, push) that passes through the GPG agent (either for signing of when used in SSH mode) it goes in background but then the screen gets all scrabbled.

The reason I believe the problem is with the TERM variable is that before going to background you set the variable to a dumb terminal which doesn't support some special terminal messages (like clear the screen and such) that are widely used by curses and thus creates the issue observed.

So while I understand and share your idea of fixing it a "better way", I think that forcing a change in that variable (by hard-coding it to dumb) IS the actual issue.

In fact I simply commented out that variable hard-coding and it also worked as intended.
So maybe a good solution could be to not change that variable and leaving it as is.

Why are you forcing it to dumb btw? in the code it seems like a nice-to-have rather than a strict need

[stefanhaller]

OK, I see. Well yes, in that case we should totally remove the code that sets TERM. I don't see a reason to set it to dumb either, and the value that the user has set it to in their shell is likely the most appropriate one.

However, I'm surprised that lazygit suspends itself to the background when you push; I don't see any code that does this, at a quick glance. I thought we'd have to add it.

[eldios]

okay, TERM variable set removed.

[eldios]

However, I'm surprised that lazygit suspends itself to the background when you push; I don't see any code that does this, at a quick glance. I thought we'd have to add it.

yes sorry, I should have mentioned that here I was referring to push via SSH and relying on pinentry-curses to get the password to pass to GPG-agent set in SSH-mode.

[stefanhaller]

okay, TERM variable set removed.

Thanks. Can you please squash the commits and rebase on master? (Please never merge master into your branch, it creates a confusing git history that we don't want here. Always rebase onto master instead.)

yes sorry, I should have mentioned that here I was referring to push via SSH and relying on pinentry-curses to get the password to pass to GPG-agent set in SSH-mode.

OK, but does it suspend to the background? I suspect it doesn't, and maybe it's hard to tell when pinentry-curses takes over the screen. I'm just trying to figure out if we need to create a followup issue to fix this for people who are using push.gpgSign, and something like pinentry-tty that doesn't take over the screen. But maybe that's an uncommon enough usecase that we don't have to care too much.

[eldios]

I think we should be good now.

OK, but does it suspend to the background? I suspect it doesn't, and maybe it's hard to tell when pinentry-curses takes over the screen. I'm just trying to figure out if we need to create a followup issue to fix this for people who are using push.gpgSign, and something like pinentry-tty that doesn't take over the screen. But maybe that's an uncommon enough usecase that we don't have to care too much.

I guess so, otherwise it wouldn't be able to be resumed, also it stays alive in the process table as suspended, from what I can see.

Unfortunately I can't make the -tty counterpart to work as pinentry keeps reverting to the curses version, but I guess it should be all working, for the few people that uses it :)

[cavanaug]

fwiw. Thank you for fixing this, it was driving me mental...