Cannot edit previous commits in git due to GIT_EDITOR environment variable

jose-pvargas commented 3 years ago

VSCode Version: Version: 1.57.1 (Universal) Commit: 507ce72a4466fbb27b715c3722558bb15afa9f48 Date: 2021-06-17T13:28:32.912Z (2 wks ago) Electron: 12.0.7 Chrome: 89.0.4389.128 Node.js: 14.16.0 V8: 8.9.255.25-electron.0 OS: Darwin x64 20.5.0
Local OS Version: macOS Big Sur 11.4
Name of Dev Container Definition with Issue: mcr.microsoft.com/vscode/devcontainers/java:11 Digest: sha256:5a6c40eb35b50c18ca1c905897dd0926ca517ec6362f4e15fb5aaa250e764cd9

Steps to Reproduce:

Clone a git repository in the dev container running as root (didn't try non-root user)

Try to amend the last commit:

$ git commit --amend
hint: Waiting for your editor to close the file... code or code-insiders is not installed
error: There was a problem with the editor 'code --wait'.
Please supply the message using either -m or -F option.

Workaround:

In my Dockerfile, I added

RUN SNIPPET="unset GIT_EDITOR" \
    && echo $SNIPPET >> /root/.bashrc

to default to the editor that git is configured to use, which was the behavior prior to this commit https://github.com/microsoft/vscode-dev-containers/commit/46fec649780223c234eb27bac3784e588d8627f7

jkeech commented 3 years ago

@Chuxel, shouldn't code or code-insiders always be on the PATH inside a VS Code integrated terminal? I thought that VS Code injects itself on the PATH for all spawned terminals, which should make it safe to set as the default GIT_EDITOR.

The intent of the referenced commit is to make VS Code the default Git editor when interacting with a devcontainer via a VS Code integrated terminal, but the change depends on the VS Code executable to always be on the PATH. The default can be overwritten via git config or tweaking/removing the environment variable. If interacting with Git inside the devcontainer but outside of VS Code (such as via direct SSH into the container or external docker exec outside of VS Code), git should not default to VS Code as the editor, since the GIT_EDITOR environment variable is only set when the TERM_PROGRAM is vscode.

jose-pvargas commented 3 years ago

Thanks for the context @jkeech. To elaborate on the original description, the issue happens, as you correctly assumed, when using the VS Code integrated terminal.

Here is the PATH environment variable:

root ➜ /workspace $ echo $PATH | tr ":" "\n"
/usr/local/share/nvm/current/bin
/usr/local/sdkman/candidates/java/current/bin
/usr/local/openjdk-11/bin
/usr/local/sbin
/usr/local/bin
/usr/sbin
/usr/bin
/sbin
/bin
/usr/local/sdkman/candidates/maven/current/bin
/usr/local/sdkman/candidates/gradle/current/bin
/root/.local/bin

and all the code/code-insiders files in the dev container

root ➜ /workspace $ find / -name "code-insiders"
root ➜ /workspace $ find / -name "code"
/usr/local/bin/code
/root/.vscode-server/extensions/ritwickdey.liveserver-5.6.1/node_modules/upath/build/code
/root/.m2/repository/com/google/code
/vscode/vscode-server/bin/x64/cfa2e218100323074ac1948c885448fdf4de2a7f/bin/code
/vscode/vscode-server/bin/x64/507ce72a4466fbb27b715c3722558bb15afa9f48/bin/code
/vscode/vscode-server/bin/x64/b4c1bd0a9b03c749ea011b06c6d2676c8091a70c/bin/code
/vscode/vscode-server/bin/x64/054a9295330880ed74ceaedda236253b4f39a335/bin/code

/usr/local/bin is in PATH but the code executable returns the same error

# From the VS Code integrated terminal
root ➜ /workspace $ /usr/local/bin/code --wait
code or code-insiders is not installed

jose-pvargas commented 3 years ago

@jkeech From https://git-scm.com/docs/git-var#Documentation/git-var.txt-GITEDITOR

Text editor for use by Git commands. The order of preference is the $GIT_EDITOR environment variable, then core.editor configuration, then $VISUAL, then $EDITOR, and then the default chosen at compile time, which is usually vi.

To use any other editor, a user needs to override the value of, or unset, GIT_EDITOR, as it is given highest precedence. I haven't used VSCode as my git editor, so I can't attest to that experience, but I do use the integrated terminal/vim extensively.

jkeech commented 3 years ago

To use any other editor, a user needs to override the value of, or unset, GIT_EDITOR, as it is given highest precedence. I haven't used VSCode as my git editor, so I can't attest to that experience, but I do use the integrated terminal/vim extensively.

@jose-pvargas, thanks for the correction on the precedence! Yes, it looks like the variable needs to be either unset or have the value changed to override.

It's still not clear to me why code or code-insiders is not on the PATH. We'll want to solve that so the default works well out of the box. There's also more we could do to make it easier to opt-out of using VS Code as the default Git editor for folks that prefer using vim, etc, as the editor. I think it makes sense to treat those as two separate issues.

Do you have a preference on how you'd prefer to opt out of using VS Code as the git editor? One option would be to have a different variable that could be set in the Dockerfile, or configured in remoteEnv/containerEnv in devcontainer.json. If that variable is set, we do not set GIT_EDITOR in the .bashrc.

jose-pvargas commented 3 years ago

I agree that they are two separate issues. Going the environment variable route in devcontainer.json seems like a better experience, as a user wouldn't need to rebuild the Docker image if they want different behavior.

Now that you mentioned it, I didn't think to try and use remoteEnv/containerEnv as a potential workaround. I suspect that it won't work, as bashrc will likely get re-executed if I open a new interactive terminal.

jkeech commented 3 years ago

Now that you mentioned it, I didn't think to try using remoteEnv/containerEnv as a potential workaround. I suspect that it won't work, as bashrc will likely get re-executed if I open a new interactive terminal.

Looking at the current code, I don't think it will work for the exact reason you describe. But it would be easy to add support for a different variable that we check (something like SKIP_GIT_EDITOR=true or CONFIGURE_GIT_EDITOR=false) prior to entering the block that configures GIT_EDITOR in the .bashrc. That variable could be toggled via remoteEnv/containerEnv to change the behavior in the .bashrc.

jkeech commented 3 years ago

I'm not sure exactly how VS Code injects itself on the PATH. I do see it at the beginning of the PATH in a codespace I just created from the mcr.microsoft.com/vscode/devcontainers/java:11 image, which is different than what @jose-pvargas saw.

vscode ➜ /workspaces/simple-server (master ✗) $ which code
/vscode/bin/x64/507ce72a4466fbb27b715c3722558bb15afa9f48/bin/code
vscode ➜ /workspaces/simple-server (master ✗) $ echo $PATH
/vscode/bin/x64/507ce72a4466fbb27b715c3722558bb15afa9f48/bin:/usr/local/share/nvm/current/bin:/usr/local/sdkman/candidates/java/current/bin:/usr/local/openjdk-11/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/local/sdkman/candidates/maven/current/bin:/usr/local/sdkman/candidates/gradle/current/bin:/home/vscode/.local/bin

I do think there's a bug in the /usr/local/bin/code script, but it would only be a problem if that script was not the first code executable on the PATH, in which case, that script shouldn't be invoked. So it's not really the root cause or a big problem in practice. If I run code in my codespace, it works as expected since the VS Code bin version of code is at the front of the PATH. But if I manually run /usr/local/bin/code, it errors:

vscode ➜ /workspaces/simple-server (master ✗) $ /usr/local/bin/code
code or code-insiders is not installed

The script has the following function, which is trying to get all of the code binaries other than itself (/usr/local/bin/code):

get_in_path_except_current() {
    which -a "$1" | grep -A1 "$0" | grep -v "$0"
}

which -a "code" in my codespace gives the following:

vscode ➜ /workspaces/simple-server (master ✗) $ which -a code
/vscode/bin/x64/507ce72a4466fbb27b715c3722558bb15afa9f48/bin/code
/usr/local/bin/code

The grep -A1 "$0" part messes up the logic and causes the final list to be empty. It looks like it's assuming that the /usr/local/bin/code is at the front of the PATH, and then is trying to get the first code after that. In my scenario, there is no code on the PATH after /usr/local/bin/code:

vscode ➜ /workspaces/simple-server (master ✗) $ which -a "code" | grep -A1 "/usr/local/bin/code" | grep -v "usr/local/bin/code"
vscode ➜ /workspaces/simple-server (master ✗) $

It seems like switching the function to be which -a "$1" | grep -v "$0" | head -n 1 behaves as expected by first filtering out the current script and then taking the first line that remains. This allows the script to be in any position in the PATH instead of expecting it to be first:

vscode ➜ /workspaces/simple-server (master ✗) $ which -a "code" | grep -v "usr/local/bin/code"
/vscode/bin/x64/507ce72a4466fbb27b715c3722558bb15afa9f48/bin/code

Fixing this issue will not fix the original PATH issue though, since we need to understand why VS Code was not injected on the PATH in the first place.

jkeech commented 3 years ago

It seems like switching the function to be which -a "$1" | grep -v "$0" | head -n 1 behaves as expected by first filtering out the current script and then taking the first line that remains. This allows the script to be in any position in the PATH instead of expecting it to be first

cc @Chuxel for this, since it looks like that was actually the original implementation, and then it changed in https://github.com/microsoft/vscode-dev-containers/commit/0edc2cccd166ebfa41d4c098b9650962424f82ce#diff-5be493bba96eaf1eaa8b68b49a576aeee4a76f8b473dca10c344486be52f0696L170. I don't have enough context on why it was changed or if anything would break if we switch it back.

Chuxel commented 3 years ago

@jkeech This was changed in https://github.com/microsoft/vscode-dev-containers/pull/616 from @felipecrs (who also wrote the initial script) - that particular commit is due to my having run the "copy-library-scripts" function on another change around the same time.

@felipecrs is there another way we could resolve the problem you were trying to fix in #616? It looks like -A1 is creating an odd problem here.

felipecrs commented 3 years ago

The reason it was added is that I wanted the code shim to only look for other code executables down in the PATH. The use case is: what if there is another shim like that in the PATH -- they would call each other infinitely (which is the case for my dotfiles, as I have such shim set there as well).

My conclusion is the same as from @jkeech:

Fixing this issue will not fix the original PATH issue though, since we need to understand why VS Code was not injected on the PATH in the first place.

felipecrs commented 3 years ago

Remember the goal of the code shim is to act as transparent as possible: if there is another code binary in the PATH first, that's ok, use that and ignore the code shim. It would only be useful when there is no code binary in the PATH (when you are using code-insiders for example).

felipecrs commented 3 years ago

@jose-pvargas I can't reproduce the issue:

Chuxel commented 3 years ago

@jose-pvargas What does which -a code give you? I can't repro either.

The only reason that the .vscode-server path John mentions wouldn't be in the PATH that I can think of is if its being set directly somewhere. Is there anything else to your Dockerfile or devcontainer.json that alters the path? Or dotfiles?

SSH'ing in wouldn't have it in the path, but the GIT_EDITOR should't be set in that case.

jose-pvargas commented 3 years ago

Apologies it's taken me a while to get back to you @Chuxel and @felipecrs.

Here's my attempt at a clean slate using the approach that @felipecrs used. Unfortunately, I still get the same error.

Is there anything else to your Dockerfile or devcontainer.json that alters the path? Or dotfiles?

There isn't anything in the Dockerfile/devcontainer.json that alters the path. Here is my attempt at finding any dot files that may alter the PATH; I didn't find anything suspicious. Please let me know if I should check somewhere else.

root ➜ /workspaces $ find / -type f -iname ".*" -exec grep --with-filename "PATH" {} \;
/usr/local/share/nvm/.travis.yml:    - PATH="$(echo $PATH | sed 's/::/:/')"
/usr/local/share/nvm/.travis.yml:    - PATH="/usr/lib/ccache/:$PATH"
/root/.zshrc:# If you come from bash you might have to change your $PATH.
/root/.zshrc:# export PATH=$HOME/bin:/usr/local/bin:$PATH
/root/.zshrc:# export MANPATH="/usr/local/man:$MANPATH"
/etc/skel/.profile:# set PATH so it includes user's private bin if it exists
/etc/skel/.profile:    PATH="$HOME/bin:$PATH"
/etc/skel/.profile:# set PATH so it includes user's private bin if it exists
/etc/skel/.profile:    PATH="$HOME/.local/bin:$PATH"
/home/vscode/.profile:# set PATH so it includes user's private bin if it exists
/home/vscode/.profile:    PATH="$HOME/bin:$PATH"
/home/vscode/.profile:# set PATH so it includes user's private bin if it exists
/home/vscode/.profile:    PATH="$HOME/.local/bin:$PATH"
/home/vscode/.zshrc:# If you come from bash you might have to change your $PATH.
/home/vscode/.zshrc:# export PATH=$HOME/bin:/usr/local/bin:$PATH
/home/vscode/.zshrc:# export MANPATH="/usr/local/man:$MANPATH"

Here is what which -a code gives me.

root ➜ /workspaces $ which -a code
/usr/local/bin/code

felipecrs commented 3 years ago

It may worth it to check if you didn't change one of these settings:

jose-pvargas commented 3 years ago

Thanks for the pointer @felipecrs! I had the Inherit Env option unselected. With that setting selected, I can edit commit messages via VSCode.

Now with hindsight, I went and looked at the documentation for VSCode's dev containers, and the only references I could find for the Inherit Env option were in the devcontainer.json reference and in the advanced container configuration documentation. Perhaps this VSCode setting should be mentioned somewhere that is more readily visible in the dev container docs? With VSCode now being the default git editor, is there a case when Inherit Env should ever be unselected?

felipecrs commented 3 years ago

So I think one possible fix to the issue would be to set GIT_EDITOR env var using the same approach, so it would only be set to use VS Code when the option is ticket, otherwise the default configured editor for git is placed.

Another approach would be to customize the code shim, which can detect if running in git environment and fallback to another editor automatically if code is not available.

jose-pvargas commented 3 years ago

Seems like the code shim was updated in https://github.com/microsoft/vscode-dev-containers/pull/959 to only use VSCode as the git editor if a git editor is not already set. However, it doesn't fallback to something else if code isn't available.

Chuxel commented 3 years ago

@jose-pvargas This will go to whatever the default editor is since you do not need to set the variable. It's usually nano or vi.

microsoft / vscode-dev-containers

Cannot edit previous commits in git due to GIT_EDITOR environment variable #935