-
Notifications
You must be signed in to change notification settings - Fork 356
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
Honour the EDITOR
and VISUAL
environment variables in shell configuration
#1057
base: main
Are you sure you want to change the base?
Conversation
It appears the CLA check passed, but please note that I have not agreed to a CLA, as mentioned above, and this is subject to my personal copyright and not my employer's, despite sharing an account. |
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.
Thank you so much for a detailed context on why this change is necessary. Appreciate you taking the time in contributing this PR.
Trailing whitespace is often removed automatically by editors, causing needless diff noise when reviewing. In addition, Git tends to highlight in red in diff output. Remove the trailing whitespace in this file.
The bash language server complains about using a single equals sign with double bracket expressions. Since we're not doing anything complex here that requires a double bracket expression, let's simply use a single bracket expression and double pipe, which is supported by all POSIX shells.
Different shells handle double parentheses differently. bash and zsh have them introduce an arithmetic expression if they're paired, whereas most other shells just treat them as a nested subshell. The bash language server notes that a space is warranted between the parentheses to avoid ambiguity when a nested subshell is wanted, as here, so let's insert to make our intention clear.
By default, Git chooses the editor from the `GIT_EDITOR` environment variable, then `core.editor`, then `VISUAL`, then `EDITOR`, and finally the compiled-in default (usually `vi`, but `editor` on Debian and Ubuntu), taking into account the terminal type if necessary. In this snippet, we check specifically for the Git-specific options, and if they're not set and we're within a VS Code terminal, we force the `GIT_EDITOR` environment variable to use VS Code, overriding the user's settings. Presumably this is because we assume that the user wants to use VS Code if they're running from within its terminal. Unfortunately, this is not necessarily the case. It's not uncommon to use VS Code for connecting to a GitHub Codespace in order to take advantage of port forwarding, or to allow connecting from within the browser, without actually using or wanting to use VS Code for text editing. (The present author knows of multiple Vim users who do exactly this in their professional lives.) If the user specified the `EDITOR` or `VISUAL` environment variables, they told us what editor they wanted to use already: they just told that to us in a way that isn't specific to Git. There's no reason to make them set another configuration specifically for Git when Git will honour their choices already. In addition, it should be noted that `EDITOR` and `VISUAL` can contain arbitrary POSIX shell expressions since they're passed to `sh`, so if the user wants to override their settings inside a VS Code terminal, they can do that by writing something like this: EDITOR='f() { if [ "$TERM_PROGRAM" = vscode ]; then code --wait "$@"; else vim "$@"; fi; };f' This is completely portable and valid, and it allows the user to make this decision for themselves. Thus, there is no regression in functionality if the user wants the current behaviour; they simply must configure their dotfiles accordingly. Respect the user's choice of editor by honouring the `EDITOR` and `VISUAL` environment variables and not overriding `GIT_EDITOR` if they are set.
@bk2204 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Sorry, as I said, I'm not willing to sign the CLA. I'm happy to grant the rights specified under the license, which, considering it's the MIT License, are extremely generous, and I'm even willing to grant this particular code to the public domain if that's necessary, since it's so trivial, but I'm not granting Microsoft carte blanche to anything I might contribute in the scope of this project (including issue comments) in the future. I agree with Ben Balter's position on this. |
Hey @bk2204 👋, I understand your reasons for not being able to sign the CLA, and we are completely fine with it. However, we won't be able to merge this PR as it's blocked due to the CLA signing requirement. Since these changes are important, we propose creating a new PR using your contributed changes. This way, you won't have to sign a CLA, we won't need to force merge anything, and the changes can still be integrated, making everyone happy! @bk2204 Let us know what you think. Thanks! |
That would be fine with me. |
Thanks @bk2204 for the confirmation. @prathameshzarkar9 Can you create a new PR and copy over this PR changes? thanks! |
By default, Git chooses the editor from the
GIT_EDITOR
environment variable, thencore.editor
, thenVISUAL
, thenEDITOR
, and finally the compiled-in default (usuallyvi
, buteditor
on Debian and Ubuntu), taking into account the terminal type if necessary.In this snippet, we check specifically for the Git-specific options, and if they're not set and we're within a VS Code terminal, we force the
GIT_EDITOR
environment variable to use VS Code, overriding the user's settings. Presumably this is because we assume that the user wants to use VS Code if they're running from within its terminal.Unfortunately, this is not necessarily the case. It's not uncommon to use VS Code for connecting to a GitHub Codespace in order to take advantage of port forwarding, or to allow connecting from within the browser, without actually using or wanting to use VS Code for text editing. (The present author knows of multiple Vim users who do exactly this in their professional lives.) If the user specified the
EDITOR
orVISUAL
environment variables, they told us what editor they wanted to use already: they just told that to us in a way that isn't specific to Git. There's no reason to make them set another configuration specifically for Git when Git will honour their choices already.In addition, it should be noted that
EDITOR
andVISUAL
can contain arbitrary POSIX shell expressions since they're passed tosh
, so if the user wants to override their settings inside a VS Code terminal, they can do that by writing something like this:EDITOR='f() { if [ "$TERM_PROGRAM" = vscode ]; then code --wait "$@"; else vim "$@"; fi; };f'
This is completely portable and valid, and it allows the user to make this decision for themselves. Thus, there is no regression in functionality if the user wants the current behaviour; they simply must configure their dotfiles accordingly.
Respect the user's choice of editor by honouring the
EDITOR
andVISUAL
environment variables and not overridingGIT_EDITOR
if they are set.This came to my attention today from a colleague using Codespaces and whose preference in using Vim had been unexpectedly overridden.
In addition, there are some cleanups to fix various shell warnings and whitespace problems as preparatory commits. This PR contains independent, logical, bisectable commits each with their own commit message, and as such is best reviewed commit by commit.
As a note, it's my policy not to sign CLAs for any reason, but I am happy to dedicate my changes to the public domain if that's helpful. If not, I regret that I'll have to close this pull request and instead open an issue explaining the problem instead. I thought sending a patch would be better than just reporting the problem, though.