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

Fix shell-integration-features being ignored with manual shell integration #5048

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

liby
Copy link
Contributor

@liby liby commented Jan 14, 2025

Descriptions

The code was short-circuiting the shell integration setup when shell-integration = none, which prevented the feature environment variables from being set. These environment variables are needed even for manual shell integration to work properly.

Changes

  • Extracted feature environment variables setup into a separate setup_features function

  • Modified the shell integration initialization to ensure features are set up even when shell-integration = none

image

Fixes #5046

@liby liby force-pushed the fix-shell-integration-features-none branch from fcdae70 to f0a83f8 Compare January 14, 2025 08:49
Copy link
Collaborator

@jparise jparise left a comment

Choose a reason for hiding this comment

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

Note that the previous behavior was intentional: when shell integration is disabled, we didn't want to add anything to the shell environment.

@liby
Copy link
Contributor Author

liby commented Jan 14, 2025

Note that the previous behavior was intentional: when shell integration is disabled, we didn't want to add anything to the shell environment.

I apologize for not linking the issue in the PR description. This PR is actually implementing the solution proposed by @mitchellh in #5046, where he stated:

Solution: We currently short circuit shell integration if it is "none" but we should respect the features and pass the proper env vars along even if it is "none"

I'll update the PR description for better tracking.

@jparise
Copy link
Collaborator

jparise commented Jan 14, 2025

Note that the previous behavior was intentional: when shell integration is disabled, we didn't want to add anything to the shell environment.

I apologize for not linking the issue in the PR description. This PR is actually implementing the solution proposed by @mitchellh in #5046, where he stated:

Solution: We currently short circuit shell integration if it is "none" but we should respect the features and pass the proper env vars along even if it is "none"

Got it. I think that makes sense. If someone doesn't want (automatic) shell integration and doesn't want the environment variables, they can still use:

shell-integration = none
shell-integration-features = false

I think the config documentation for shell-integration-features could use a brief update as part of this change, though. It currently says:

Shell integration features to enable if shell integration itself is enabled.

... which reads more ambiguously now that shell-integration = none will result in the environment variables being set and therefore applying to manual shell integration.

@liby
Copy link
Contributor Author

liby commented Jan 14, 2025

I think the config documentation for shell-integration-features could use a brief update as part of this change

Thanks for the feedback! I've updated the documentation for shell-integration-features to make it clearer.

Please let me know if the updated documentation looks good to you.

@liby liby force-pushed the fix-shell-integration-features-none branch from 874621d to 4c3e63f Compare January 14, 2025 13:33
src/config/Config.zig Outdated Show resolved Hide resolved
src/termio/shell_integration.zig Outdated Show resolved Hide resolved
@liby liby force-pushed the fix-shell-integration-features-none branch from 4c3e63f to 3068b94 Compare January 14, 2025 16:09
@liby liby requested a review from jparise January 14, 2025 16:11
Copy link
Collaborator

@jparise jparise left a comment

Choose a reason for hiding this comment

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

Just a couple more suggestions, but looks good to me overall!

src/config/Config.zig Outdated Show resolved Hide resolved
src/termio/shell_integration.zig Outdated Show resolved Hide resolved
@liby liby force-pushed the fix-shell-integration-features-none branch from 3068b94 to f1d792c Compare January 14, 2025 17:41
Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

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

One small but important change and it looks good. Thank you!

src/termio/shell_integration.zig Outdated Show resolved Hide resolved
@liby liby force-pushed the fix-shell-integration-features-none branch from f1d792c to 491af71 Compare January 15, 2025 00:30
@liby liby requested a review from mitchellh January 15, 2025 00:31
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.

shell-integration-features config is ignored on manual shell integration
4 participants