-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Better support for shell aliases #4385
base: master
Are you sure you want to change the base?
Conversation
There is a section at the end with deprecated settings, and a comment saying "The following configs are all deprecated". The clipboard-related settings were accidentally added to that section; they are not deprecated, so move them up to before that section.
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more Footnotes
|
To make sure I understand this issue: interactive shells cause problems because sometimes the subprocess (potentially) hands control back to the wrong process, and because they're slow to load. We want to lose the interactivity but keep the ability to alias. So we want to remove interactivity from custom commands and allow users to specify an aliases file explicitly to get aliases. Is that right? I agree to remove interactivity from custom commands. Thinking out loud about the implementation:
|
I applied your diff from #4320 and ended up with "0ms" as well so it looks like there's something else going on. But since custom commands will no longer be run in interactive shells, the whole things probably moot. 🤷 For what it's worth, I agree with your reasoning and I like your proposed solution. However, I checked out this PR locally to try it out and it looks like Following up on what @jesseduffield said above:
I did some reading and it turns out there's an entire section in + /usr/bin/zsh -c source ~/.zshrc
alias l
Hello from .zshrc
l='eza -la --icons --group-directories-first'
Press enter to return to lazygit
+ /usr/bin/zsh -c source ~/.zshrc
l
Hello from .zshrc
zsh:2: command not found: l I found a workaround though, and I did manage to get it working: + /usr/bin/zsh -c source ~/.zshrc
eval l
Hello from .zshrc
drwxr-xr-x - rburd 2 Mar 12:22 .devcontainer
drwxr-xr-x - rburd 10 Mar 19:46 .git
drwxr-xr-x - rburd 2 Mar 12:22 .github
... Here's a diff with what I added to get |
I could swear I tested this and it worked for me (only after using a newline instead of a
As far as I understand, the workaround from that SO answer is meant only for the case where the alias is the only thing on the command line (it does say "In general you should properly quote all arguments to eval"). This is not the case in lazygit's shell prompt, users may type arbitrary command lines where aliases only occur in the middle, and I don't understand the implications of eval'ing the whole thing enough to feel comfortable with this approach. The official recommendation is to use functions instead of aliases. I think this is reasonable advice, but it does mean users have to do even more work to their shell startup files before they can use them with lazygit. What a mess. |
Indeed. I really can't imagine lazygit users going to the effort of creating a separate file to define functions for aliases they already have defined in their zshrc files. At the moment I'm leaning towards just not supporting aliases or functions at all. Or having some other keybinding that just switches to a full interactive shell session. |
In version 0.45.0 we started to use an interactive shell for running shell commands (see #4159). The idea was that this allows users to use their aliases and shell functions in lazygit without having to do any additional configuration.
Unfortunately, this hasn't worked out well. For some users this resulted in lazygit hanging in the background upon trying to return from the shell command; we tried various fixes for this (see #4126, #4159, and #4350), but some users still have this problem (e.g. #4320).
Also, starting an interactive shell can be a lot slower than starting a non-interactive one, depending on how much happens in the
.bashrc
or.zshrc
file. For example, on my machine callingzsh -ic true
takes 600ms, whereaszsh -c true
takes less than 2ms. This is too high of a price to pay for using shell aliases, especially when all users have to pay it, even those who don't care about using their aliases in lazygit.This PR reverts all commits related to interactive shells, and instead introduces a different approach: we let users specify a shell aliases file that will be sourced before running a command. The downside is that it doesn't work transparently out of the box, but requires configuration, and it may also require that users restructure their shell startup file(s) if they currently only have a single big one. The advantage is that only users who actually want to use aliases or functions are affected, and that we can now use this mechanism not only for shell commands, but also for custom commands and for calling the editor (some users have asked for this in the past).
go generate ./...
)