-
-
Notifications
You must be signed in to change notification settings - Fork 548
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
fix(shell): fix incorrect timing of child shells #1510
Conversation
When a child shell session is started from another shell session (parent session), the environment variable ATUIN_HISTORY_ID set by the parent session causes Atuin's precmd hook of the child session to be unexpectedly performed before the first call of Atuin's preexec hook. In this patch, we clear ATUIN_HISTORY_ID (possibly set by the parent session) on the startup of the session.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
I'll have a proper dig into this one soon - might be worth pinging @arcuru to check the fish stuff 🙏 |
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.
Looks like a reasonable fix to me, thank you!
As far as I search in the codebase, there does not seem to be a code that reads the environment variable ATUIN_HISTORY_ID from other processes.
Out of scope for this PR, but you're correct. It's only used within the shell integration, and not read from other processes
Thank you! |
Sorry I didn't see this ping until now. The fish change doesn't hurt, but is unnecessary (EDIT: see below). In #1370 I fixed this issue from happening in fish by changing the ATUIN_HISTORY_ID variable to not be exported to child processes. EDIT: Nevermind, the change to fish is necessary. If you, for example, run a fish shell from bash it would inherit that variable from the bash setup, so you'd probably be hitting all the same issues. |
I'm out for a bit, but I'll tag in someone from the Nushell team. |
Just tested the changes for nushell, and it looks like the fix works as intended! ( |
When a child shell session is started from another shell session (let us call this parent session hereafter), the environment variable
ATUIN_HISTORY_ID
set by the parent session causes Atuin's precmd hook of the child session to be unexpectedly performed before the first call of Atuin's preexec hook.For example, let us consider the case to run
bash
inside the terminal to start a new session.In this patch, we clear
ATUIN_HISTORY_ID
(possibly set by the parent session) on the startup of the session. The variableATUIN_HISTORY_ID
doesn't seem to be an environment variable in the fish integration, but we clear it also in the fish integration for consistency.Possible drawback
With this solution, when the user runs
source .bashrc
(oreval "$(atuin init bash)"
) in the command line, theATUIN_HSITORY_ID
forsource .bashrc
is lost and the timing for that command is not recorded. However, the same happens for theexit
command. In the sense thatsource .bashrc
terminates the existingATUIN_SESSION
(and creates another), it is similar toexit
. For this reason, we might not need to care about it more thanexit
.Other possible solutions
However, another possible option would be not to make
ATUIN_HISTORY_ID
the environment variable from the beginning. As far as I search in the codebase, there does not seem to be a code that reads the environment variableATUIN_HISTORY_ID
from other processes. So I do not see the necessity to make the variable an environment variable.Or, with the current
main
, we are effectively measuring the startup time of the child shell. Maybe we can keep the current code if this is the intended one, but then, this seems inconsistent: In this way, only the startup times of the child sessions are measured, but the startup time of the parent session (the login shell started by the terminal) is not measured. Also, this does not work in the current integration with the fish shell because ATUIN_HISTORY_ID doesn't seem to be an environment variable in the fish integration.Review request
In this patch, I touch the integrations with all the shells, but I'm only familiar with Bash. If possible, I'd like reviews from the contributors to the integrations with the other shells: @ellie (for zsh, etc.), @conradludgate (for fish, etc.), @stevenxxiu (for nu), and @sophiajt (for env in nu)