-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Better support for shell aliases #4385
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
Conversation
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. |
2f1873e
to
6834581
Compare
We are not talking about maintaining a separate file just for lazygit. The idea is to refactor the zshrc file so that it sources a separate file, and that file can then also be used by lazygit. I would totally do that if I had to in order to use shell aliases with lazygit (but it so happens that I have a separate file for this already anyway). Also, having to convert aliases to functions might sound annoying, but it's pretty easy and straightforward, so it may be worth it. Only zsh users need to do it. I added a paragraph to the documentation how to do this (6834581).
That would be a regression though, I'm sure many users would complain about that. In the current release, aliases work for most people, except when they are affected by the hanging problem, which seems to affect only very few people, it seems. It would be bad to take this feature away again with no substitute.
You mean by implementing a terminal inside of lazygit? That sounds like a pretty huge amount of work to me. |
6834581
to
9bfdb8e
Compare
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 preferencesFootnotes
|
@RBird111 Could you have another look at the final state of the branch? I added a documentation section that explains the limitations, would be great if you could proof-read and maybe test again in different shells. |
docs/Config.md
Outdated
|
||
```yml | ||
os: | ||
shellAliasesFile: ~/.my_aliases.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shellAliasesFile: ~/.my_aliases.sh | |
shellFunctionsFile: ~/.my_aliases.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in be4ca3a.
docs/Config.md
Outdated
shellAliasesFile: ~/.my_aliases.sh | ||
``` | ||
|
||
For many people it might work well enough to use their entire shell config file (`~/.bashrc` or `~/.zshrc`) as the `shellAliasesFile`, but these config files typically do a lot more than defining aliases (e.g. initialize the completion system, start an ssh-agent, etc.) and this may unnecessarily delay execution of shell commands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For many people it might work well enough to use their entire shell config file (`~/.bashrc` or `~/.zshrc`) as the `shellAliasesFile`, but these config files typically do a lot more than defining aliases (e.g. initialize the completion system, start an ssh-agent, etc.) and this may unnecessarily delay execution of shell commands. | |
For many people it might work well enough to use their entire shell config file (`~/.bashrc` or `~/.zshrc`) as the `shellFunctionsFile`, but these config files typically do a lot more than defining aliases (e.g. initialize the completion system, start an ssh-agent, etc.) and this may unnecessarily delay execution of shell commands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in be4ca3a.
NewShell(commandStr string) ICmdObj | ||
// Like NewShell, but uses the user's shell rather than "bash", and passes -i to it | ||
NewInteractiveShell(commandStr string) ICmdObj | ||
NewShell(commandStr string, aliasesFile string) ICmdObj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably update the documentation for this method to explain the purpose of the new variable or add a link back to Config.md
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 2b7f56b.
pkg/commands/git_cmd_obj_builder.go
Outdated
|
||
func (self *gitCmdObjBuilder) NewInteractiveShell(cmdStr string) oscommands.ICmdObj { | ||
return self.innerBuilder.NewInteractiveShell(cmdStr).AddEnvVars(defaultEnvVar) | ||
func (self *gitCmdObjBuilder) NewShell(cmdStr string, aliasesFile string) oscommands.ICmdObj { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worthwhile to update the name aliasesFile
to something like shFunctionsFile
to bring it inline with what's used in the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, changed in 2b7f56b.
@stefanhaller I checked out the PR locally and everything works great! I tried my best to break things and I didn't encounter any unexpected or surprising behavior. There are a couple scenarios that I want to bring up: Scenario 1:
Scenario 2: The user, for whatever reason, decides to launch a process in the background Again, I don't think either of these cases are issues, I just thought they were interesting enough to be worth jotting down. So other than a couple small things in the documentation everything looks good! |
@RBird111 Thanks for the thorough review! I agree that the two scenarios you brought up are not something we need to address, but it's good to have them on file for future reference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.
2b7f56b
to
d13306a
Compare
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [jesseduffield/lazygit](https://github.com/jesseduffield/lazygit) | minor | `v0.48.0` -> `v0.50.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>jesseduffield/lazygit (jesseduffield/lazygit)</summary> ### [`v0.50.0`](https://github.com/jesseduffield/lazygit/releases/tag/v0.50.0) [Compare Source](jesseduffield/lazygit@v0.49.0...v0.50.0) <!-- Release notes generated using configuration in .github/release.yml at v0.50.0 --> #### What's Changed ##### Enhancements 🔥 - Continue/abort a conflicted cherry-pick or revert by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4441 - Show todo items for pending cherry-picks and reverts by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4442 - Use "git cherry-pick" for implementing copy/paste of commits by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4443 - Allow reverting a range of commits by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4444 - Section headers for rebase todos and actual commits by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4463 - Focus the main view by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4429 - Auto-forward main branches after fetching by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4493 - Add new command "Move commits to new branch" by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#3876 - Strip the '+' and '-' characters when copying parts of a diff to the clipboard by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4519 - Reduce memory consumption of graph (pipe sets) by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4498 ##### Fixes 🔧 - Fix truncation of branches when scrolling branches panel to the left by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4483 - Fix nvim-remote commands for fish shell by [@​SavingFrame](https://github.com/SavingFrame) in jesseduffield/lazygit#4506 - Disallow creating custom patches when the diff context size is 0 by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4522 - Remove double space between rebase todo and author columns by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4520 ##### Maintenance ⚙️ - Allow closing issues via github actions by [@​jesseduffield](https://github.com/jesseduffield) in jesseduffield/lazygit#4515 ##### Docs 📖 - Add Debian installation instructions alongside Ubuntu by [@​jmkim](https://github.com/jmkim) in jesseduffield/lazygit#4501 - fix wording of random tip by [@​dawedawe](https://github.com/dawedawe) in jesseduffield/lazygit#4488 #### New Contributors - [@​jmkim](https://github.com/jmkim) made their first contribution in jesseduffield/lazygit#4501 - [@​SavingFrame](https://github.com/SavingFrame) made their first contribution in jesseduffield/lazygit#4506 - [@​dawedawe](https://github.com/dawedawe) made their first contribution in jesseduffield/lazygit#4488 **Full Changelog**: jesseduffield/lazygit@v0.49.0...v0.50.0 ### [`v0.49.0`](https://github.com/jesseduffield/lazygit/releases/tag/v0.49.0) [Compare Source](jesseduffield/lazygit@v0.48.0...v0.49.0) <!-- Release notes generated using configuration in .github/release.yml at v0.49.0 --> #### What's Changed ##### Enhancements 🔥 - Support fish when running shell command by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4350 - Add acme editor preset by [@​rakoo](https://github.com/rakoo) in jesseduffield/lazygit#4360 - Support home and end as alternatives to '<' and '>' by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4396 - Drop the git config cache when getting focus by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4376 - Add a "Content of selected file" entry to the copy menu for commit files by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4341 - Add root node in file tree by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4346 - \[FEAT] Add Recursive Bulk Initialize and Update for Submodules by [@​cesarandr](https://github.com/cesarandr) in jesseduffield/lazygit#4259 - Commit without pre-commit hooks action is independent on prefix by [@​kschweiger](https://github.com/kschweiger) in jesseduffield/lazygit#4374 - Let users define custom icons and color for files on the config file by [@​hasecilu](https://github.com/hasecilu) in jesseduffield/lazygit#4395 - Add "Absolute path" item to the file view's copy menu by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4410 - Allow range drop stashes by [@​gaogao-qwq](https://github.com/gaogao-qwq) in jesseduffield/lazygit#4451 - More navigation keybindings for confirmation panel by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4404 - Resolve non-inline merge conflicts by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4431 - Add `runCommand` function to Go template syntax + add support for templates in git `branchPrefix` setting by [@​ruudk](https://github.com/ruudk) in jesseduffield/lazygit#4438 - Show "(hooks disabled)" in title bar of commit message editor by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4467 - Add a command to select all commits of the current branch by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4448 ##### Fixes 🔧 - Use a waiting status for rewording a non-head commit by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4343 - Fix layout of options view for non-english languages by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4359 - Fix changing language while lazygit is running by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4361 - URL encode gitlab brackets to make consistent with branch names by [@​ChrisMcD1](https://github.com/ChrisMcD1) in jesseduffield/lazygit#4336 - Fix commitPrefix setting with empty pattern by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4381 - Commit only tracked files in tracked filter view by [@​parthokunda](https://github.com/parthokunda) in jesseduffield/lazygit#4386 - Revert [#​4313](jesseduffield/lazygit#4313) (Skip post-checkout hook when discarding changes) by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4407 - Enhance support for GPG signed tags by [@​ChrisMcD1](https://github.com/ChrisMcD1) in jesseduffield/lazygit#4394 - Fix checking out a file from a range selection of commits by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4423 - Fix discarding submodule changes in nested folders by [@​brandondong](https://github.com/brandondong) in jesseduffield/lazygit#4317 - Better support for shell aliases by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4385 - Fix hyperlinks in last line of confirmation popups by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4454 - Fix wrong main view content after pressing `e` in a stack of branches by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4450 - fix: update vscode color to logo color by [@​PeterCardenas](https://github.com/PeterCardenas) in jesseduffield/lazygit#4459 - Fix crash with drag selecting and staging by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4480 - Escape special characters in filenames when git-ignoring files by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4475 ##### Maintenance ⚙️ - Fix linter warnings by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4352 - Fix release schedule again by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4364 - Update to go 1.24 by [@​radsaq](https://github.com/radsaq) in jesseduffield/lazygit#4377 - Add an integration test for a config with a negative refspec by [@​radsaq](https://github.com/radsaq) in jesseduffield/lazygit#4382 - Specify a go release minor version by [@​radsaq](https://github.com/radsaq) in jesseduffield/lazygit#4393 - Fix flaky integration test by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4432 - Some code cleanup by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4449 - Bump the minimum required git version to 2.22 by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4439 - Bump go-git, and get rid of github.com/imdario/mergo by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4460 - Skip date check when release worfklow is manually invoked by [@​jesseduffield](https://github.com/jesseduffield) in jesseduffield/lazygit#4484 ##### Docs 📖 - Include empty arrays and maps in config docs by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4413 - Filter out deprecated user config fields from generated Config.md by [@​karimkhaleel](https://github.com/karimkhaleel) in jesseduffield/lazygit#4416 - Corrected interactive rebase keybind example in README.md by [@​NewtonChutney](https://github.com/NewtonChutney) in jesseduffield/lazygit#4426 - Update kidpix link in README to active url by [@​ChrisMcD1](https://github.com/ChrisMcD1) in jesseduffield/lazygit#4425 ##### I18n 🌎 - Update translation files from Crowdin by [@​stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4473 #### New Contributors - [@​rakoo](https://github.com/rakoo) made their first contribution in jesseduffield/lazygit#4360 - [@​radsaq](https://github.com/radsaq) made their first contribution in jesseduffield/lazygit#4377 - [@​cesarandr](https://github.com/cesarandr) made their first contribution in jesseduffield/lazygit#4259 - [@​kschweiger](https://github.com/kschweiger) made their first contribution in jesseduffield/lazygit#4374 - [@​NewtonChutney](https://github.com/NewtonChutney) made their first contribution in jesseduffield/lazygit#4426 - [@​gaogao-qwq](https://github.com/gaogao-qwq) made their first contribution in jesseduffield/lazygit#4451 - [@​ruudk](https://github.com/ruudk) made their first contribution in jesseduffield/lazygit#4438 **Full Changelog**: jesseduffield/lazygit@v0.48.0...v0.49.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MC4xMS4yIiwidXBkYXRlZEluVmVyIjoiNDAuMTEuMiIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
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 ./...
)