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

Detect the user's shell for running custom commands #2096

Closed
wants to merge 1 commit into from
Closed

Detect the user's shell for running custom commands #2096

wants to merge 1 commit into from

Conversation

mark2185
Copy link
Collaborator

@mark2185 mark2185 commented Aug 7, 2022

This makes it possible to use any shell, and by adding the -i flag it
makes it interactive (running aliases and functions is possible now)

Should close #770 .

Tests are still TODO

This makes it possible to use any shell, and by adding the `-i` flag it
makes it interactive (running aliases and functions is possible now)
Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple thing

Shell: "bash",
ShellArg: "-c",
Shell: getShell(),
ShellArg: "-ic",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does the i flag do and how many shells support it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes the shell interactive which in turn reads startup files (among other things), which allows you to use aliases, functions and variables defined in your e.g. .zshrc.

I tried it with bash, zsh and fish and all of them support the i flag.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow very cool. I wonder if this will slow things down for users that have extensive e.g. zshrc files.

"runtime"
)

func getShell() string {
defaultShell := "bash"
if shell := os.Getenv("SHELL"); shell != defaultShell {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic isn't quite right:
If shell != default shell, we return shell
if shell == default shell, we return default shell which is really just shell

So we're returning shell either way.

I would instead say that if SHELL is blank string, we default to bash

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We presume the shell is bash.

If we read the environment variable SHELL (which actually returns the absolute path, my bad) and it turns out to be zsh (!= bash), we return zsh which... yeah, okay

So we're returning shell either way.

Well, when you put it that way... 😅

@mark2185
Copy link
Collaborator Author

mark2185 commented Sep 24, 2022

cc @Ryooooooga for additional input, on #1642 they said:

If use non-POSIX $SHELL, the string quoting process seems to break.

For example, (when commit.gpgSign=true), bash -c "git commit -m ..." is executed for commit, but fish does not need to escape ` in the commit message.

Absorbing these shell-specific differences can be difficult.

@cfurrow
Copy link

cfurrow commented Dec 8, 2022

An interesting side-effect of applying these changes that I've seen was that upon returning from committing, the lazygit app gets suspended. Here's the example output I see after committing something from within lazygit:

+ /bin/zsh -ic git commit -m "Move shell assignment up"

[pr-2096 3791988d] Move shell assignment up
 1 file changed, 2 insertions(+), 1 deletion(-)

Press enter to return to lazygit[1]  + 82736 suspended (tty input)  bin/lazygit

When I bring the app back to the foreground with fg, it's blank and the current terminal is unresponsive. This "unresponsive" and "blank" behavior is common when using lazygit with Warp, which was why I was testing your PR in the first place to see if it would fix the issues users have seen with lazygit and Warp: warpdotdev/Warp#1511. (it does not fix it).

I don't know if the underlying issue is with the go terminal UI library (gocui?) and Warp, as a root cause is not currently known. I'm simply posting these details to relate the filed issue for Warp with this PR in case someone figures out what's wrong with how Warp handling the lazygit response. Being that Warp does not quite play by the same rules as other terminals and shells.

@mark2185 mark2185 closed this by deleting the head repository Jun 1, 2023
stefanhaller added a commit that referenced this pull request Aug 17, 2024
- **PR Description**

When executing an interactive custom command, use the user's shell
rather than "bash", and pass the -i flag. This makes it possible to use
shell aliases or shell functions which are not available in
non-interactive shells.

In previous attempts to solve this, concerns were brought up: [this
one](#2096 (comment))
is addressed by using the interactive shell only for custom commands but
not anything else. [This
one](#2096 (comment))
is a little dubious and unconfirmed, so I'm not very worried about it.

Supersedes #2096 and #3299. Fixes #770, #899, and #1642.
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 this pull request may close these issues.

custom alias in my ~/.zshrc isn't working with the lazygit's custom command option ':'
3 participants