Fix hang when returning from shell command

[stefanhaller]

• PR Description

In #3793 we changed the execution of shell commands to use an interactive shell (-i), because this allows users to use aliases or shell functions, which is a nice convenience.

Since then, however, many users have reported problems with lazygit not coming back to the foreground after executing a shell command. Some users report that appending "; exit" to the end of the command line solves this. I don't really understand what the cause of this problem was, or why appending "; exit" solves it, but if it helps, let's do it.

Fixes #3903, #3923, #3937.

• 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)
• If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for ec410b21	✅ 60.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (ec410b2)	Report Missing	Report Missing	Report Missing
Head commit (f9c37d3)	51443	44424	86.36%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#4126)	15	9	60.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

Footnotes

1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

[rayluo]

Some users report that appending "; exit" to the end of the command line solves this. I don't really understand what the cause of this problem was, or why appending "; exit" solves it, but if it helps, let's do it.

I am one of those users commented in #3937. I don't know why or how that ; exit work, either. I tested your branch (in a github codespace, for what it's worth), but the new change does not seem to work.

@rayluo ➜ /workspaces/lazygit (fix-hang-on-shell-command) $ ./lazygit 

+ /bin/bash -i -c ls; exit $?

cmd  CODE-OF-CONDUCT.md  CONTRIBUTING.md  demo  Dockerfile  docs  go.mod  go.sum  lazygit  LICENSE  main.go  Makefile  pkg  README.md  schema  scripts  test  vendor

Press enter to return to lazygit
[1]+  Stopped                 ./lazygit
@rayluo ➜ /workspaces/lazygit (fix-hang-on-shell-command) $ fg
./lazygit


+ /bin/bash -i -c ls; exit $?

cmd  CODE-OF-CONDUCT.md  CONTRIBUTING.md  demo  Dockerfile  docs  go.mod  go.sum  lazygit  LICENSE  main.go  Makefile  pkg  README.md  schema  scripts  test  vendor

Press enter to return to lazygit
[1]+  Stopped                 ./lazygit
@rayluo ➜ /workspaces/lazygit (fix-hang-on-shell-command) $ fg
./lazygit

If I manually add yet another "; exit" suffix in my command line, it won't hang.

+ /bin/bash -i -c ls -l; exit; exit $?

......
exit

Press enter to return to lazygit

[yam-liu]

@rayluo It seems the difference is $?. Can you remove it from your codespace and test it again?

[rayluo]

@rayluo It seems the difference is $?. Can you remove it from your codespace and test it again?

I don't know why $? is needed but it turns out it is irrelevant.

The solution is to quote the entire command line. @stefanhaller , please review (and test?) #4129 and merge it to your PR, and then I think your PR will be good to go.

[jesseduffield]

LGTM pending @rayluo 's approval

[stefanhaller]

@rayluo What a stupid mistake, of course the exit needs to be included in the call to quotedCommandString. Thanks for pointing this out, I included the fix here, so you can close your PR. I'm actually unsure why it even worked at all before, but whatever.

The reason why we want to include the $? is so that if a command fails, you 1) get an error message in lazygit, and 2) if you have promptToReturnFromSubprocess set to false, it still prompts if the command fails, so that you can read the error output in the console.

[rayluo]

... the exit needs to be included in the call to quotedCommandString. Thanks for pointing this out, I included the fix here, so you can close your PR.

OK, that would also mean I will need to wait for next time to get my name shown up in the Lazygit contributors list, but whatever. :-)

Just don't forget to squash your current commit, because its "fixup" keyword probably causes a CI/CD test failure.

The reason why we want to include the $? is so that if a command fails, you 1) get an error message in lazygit, and 2) if you have promptToReturnFromSubprocess set to false, it still prompts if the command fails, so that you can read the error output in the console.

Good to know. Thanks for your attention to details!

[stefanhaller]

Just don't forget to squash your current commit, because its "fixup" keyword probably causes a CI/CD test failure.

Don't worry, I use fixup commits all the time, they are an integral part of my daily workflow, so I know quite well what I have to do with them. 😄

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for ec410b21	✅ 60.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (ec410b2)	Report Missing	Report Missing	Report Missing
Head commit (5fac40c)	51443	44422	86.35%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#4126)	15	9	60.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

Footnotes

1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩