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

Better support for shell aliases #4385

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

stefanhaller
Copy link
Collaborator

  • PR Description

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 calling zsh -ic true takes 600ms, whereas zsh -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).

  • 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

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.
@stefanhaller stefanhaller added the bug Something isn't working label Mar 10, 2025
Copy link

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for 19ac9261 67.86%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (19ac926) Report Missing Report Missing Report Missing
Head commit (2f1873e) 53169 46054 86.62%

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 (#4385) 28 19 67.86%

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.

@jesseduffield
Copy link
Owner

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 wonder if users would actually bother to go and create a bespoke aliases file just so that they can use them in lazygit. It would be good to validate this. I suspect they won't do that, and would prefer to just source ~/.zshrc. I'm not sure how many problems are caused by invoking source ~/.zshrc in a non-interactive context.
  • Would it make more sense to just let users define a command prefix directly like 'source ~/.zshrc && `
  • I'd be okay to have pre-configured custom commands not make use of this aliases file, given that it's so easy to say source ~/.zshrc && in the custom command itself if you need that, and custom commands (I assume) are used more frequently so you'd want them to be fast.

@RBird111
Copy link

@stefanhaller

In my experiments this always printed "0 ms", but that's not a surprise because I could never reproduce the issue on my machine. I'm curious what this prints for you.

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 zsh is still going to be a problem.

Following up on what @jesseduffield said above:

I'm not sure how many problems are caused by invoking source ~/.zshrc in a non-interactive context.

I did some reading and it turns out there's an entire section in zshmisc(1) devoted to this topic (the paragraph starting with "There is a commonly encountered problem..."). The TL;DR is that aliases are expanded as the command is read in, and since the command string is read all at once in this case, the aliases defined in the user supplied aliasesFile will never get expanded. I verified this was the case locally:

+ /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 zsh to cooperate: gist link
Hopefully its useful as a starting point and thank you both for the work you've done on this project!

@stefanhaller
Copy link
Collaborator Author

However, I checked out this PR locally to try it out and it looks like zsh is still going to be a problem.

I could swear I tested this and it worked for me (only after using a newline instead of a ; though). Looks like I dreamed it, I can't get it to work with zsh either now. :-/

I found a workaround though, and I did manage to get it working:

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.

@jesseduffield
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants