nix: set TEST_GIT_EXECUTABLE_PATH in devShell

[scott2000]

This environment variable is required for running tests on NixOS for me, otherwise the subprocess tests fail.

Checklist

If applicable:

• I have updated CHANGELOG.md
• I have updated the documentation (README.md, docs/, demos/)
• I have updated the config schema (cli/src/config-schema.json)
• I have added tests to cover my changes

[emilazy]

It doesn’t seem like TEST_GIT_EXECUTABLE_PATH should be needed for the actual build at all, since git is already in nativeCheckInputs (though it should probably be checkInputs). nativeCheckInputs is also added to the development shell, so I’m surprised that it’s failing for you. Does which git not work in the shell?

[scott2000]

I'm using nix-direnv, and which git works in the shell, but the tests still fail saying that they can't find git. I think this was discussed in #5228 (which introduced this environment variable). Out of curiosity, does this work for you on the current trunk()?

[thoughtpolice]

FYI, the tests currently work for me on trunk() as of this writing (e58713c), x86_64 Linux, with just cargo nextest run --workspace; my .envrc is nothing more than the line source_env .envrc.recommended

[scott2000]

Hmm that's strange. I'm not sure why I'm seeing something different. I'm also on x86_64 Linux, and I checked out that same commit, and I ran that same command, and my .envrc is identical, but I'm getting different behavior. What does which git return for you? This is what it looks like for me:

$ which git
/nix/store/7mln3k7mmvsnpgp47zr63w9d60pqba9n-git-2.47.1/bin/git
$ cargo nextest run --workspace
...
        FAIL [   0.152s] jj-cli::runner test_git_clone::test_git_clone::spawn_a_git_subprocess_for_remote_calls
──── STDOUT:             jj-cli::runner test_git_clone::test_git_clone::spawn_a_git_subprocess_for_remote_calls

running 1 test
test test_git_clone::test_git_clone::spawn_a_git_subprocess_for_remote_calls ... FAILED

failures:

failures:
    test_git_clone::test_git_clone::spawn_a_git_subprocess_for_remote_calls

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 772 filtered out; finished in 0.14s

──── STDERR:             jj-cli::runner test_git_clone::test_git_clone::spawn_a_git_subprocess_for_remote_calls

thread 'test_git_clone::test_git_clone::spawn_a_git_subprocess_for_remote_calls' panicked at /nix/store/0brrwmpghjjb6ddkhmvp9mbx4bbxzazx-rust-default-1.86.0-nightly-2025-01-24/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5:
Unexpected failure.
code=1
stderr=``````
Fetching into new repo in \"/tmp/jj-test-j2nMnJ/empty\"
Error: Could not execute the git process, found in the OS path \'git\'
Caused by: No such file or directory (os error 2)
```
```
command=`cd "/tmp/jj-test-j2nMnJ" && env -i COLUMNS="100" HOME="/tmp/jj-test-j2nMnJ/home" JJ_CONFIG="/tmp/jj-test-j2nMnJ/config" JJ_EMAIL="[email protected]" JJ_OP_HOSTNAME="host.example.com" JJ_OP_TIMESTAMP="2001-02-03T04:05:07+07:00" JJ_OP_USERNAME="test-username" JJ_RANDOMNESS_SEED="1" JJ_TIMESTAMP="2001-02-03T04:05:07+07:00" JJ_TZ_OFFSET_MINS="660" JJ_USER="Test User" RUST_BACKTRACE="1" SSL_CERT_FILE="/dev/null" "/home/scott/jj/target/debug/jj" "git" "clone" "source" "empty"`
code=1
stdout=""
stderr=```
Fetching into new repo in \"/tmp/jj-test-j2nMnJ/empty\"
Error: Could not execute the git process, found in the OS path \'git\'
Caused by: No such file or directory (os error 2)
```


stack backtrace:
   0: rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::panicking::panic_display
   3: assert_cmd::assert::AssertError::panic::panic_cold_display
   4: assert_cmd::assert::AssertError::panic
   5: core::ops::function::FnOnce::call_once
   6: core::result::Result<T,E>::unwrap_or_else
   7: assert_cmd::assert::Assert::success
   8: runner::common::TestEnvironment::get_ok
   9: runner::common::TestEnvironment::jj_cmd_ok
  10: runner::test_git_clone::test_git_clone
  11: runner::test_git_clone::test_git_clone::spawn_a_git_subprocess_for_remote_calls
  12: runner::test_git_clone::test_git_clone::spawn_a_git_subprocess_for_remote_calls::{{closure}}
  13: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
...

I'm using fish shell, but I doubt that would matter.

[thoughtpolice]

My git is the exact same as yours, but that tipped me off, and I suspect it's probably because for some reason the test executes /usr/bin/git by default on my machine. I ran execsnoop.bt with bpftrace in a separate terminal while I ran only that exact test, and you can usee it use /usr/bin and /usr/lib for this:

austin@GANON:~/src/jj$ sudo execsnoop.bt | grep git
1879       761051 atuin history start -- sudo execsnoop.bt | grep git
1891       761055 grep --color=auto git
6240       761110 atuin history start -- cargo nextest run --workspace -E 'test(test_git_clone::test_git_clone::spawn_a_git_subprocess_for_remote_calls)'
6253       761113 cargo nextest run --workspace -E test(test_git_clone::test_git_clone::spawn_a_git_subprocess_for_remote_calls)
6260       761113 /nix/store/bzl8g2f5winnj0257aqjl9yfdfrb8sd4-cargo-nextest-0.9.87/bin/cargo-nextest nextest run --workspace -E test(test_git_clone::test_git_clone::spawn_a_git_subprocess_for_remote_calls)
6682       761172 /proc/self/exe __double-spawn -- /home/austin/src/jj/target/debug/deps/runner-3b6986b46e449623 --exact test_git_clone::test_git_clone::spawn_a_git_subprocess_for_remote_calls --nocapture
6683       761172 /home/austin/src/jj/target/debug/deps/runner-3b6986b46e449623 --exact test_git_clone::test_git_clone::spawn_a_git_subprocess_for_remote_calls --nocapture
6706       761174 /home/austin/src/jj/target/debug/jj git clone source empty
6767       761178 git --bare --git-dir /tmp/jj-test-tyBYlT/empty/.jj/repo/store/git/ fetch --prune -- origin +refs/heads/*:refs/remotes/origin/*
6767       761178 git --bare --git-dir /tmp/jj-test-tyBYlT/empty/.jj/repo/store/git/ fetch --prune -- origin +refs/heads/*:refs/remotes/origin/*
6768       761179 /bin/sh -c git-upload-pack '/tmp/jj-test-tyBYlT/source' git-upload-pack '/tmp/jj-test-tyBYlT/source'
6768       761180 git-upload-pack /tmp/jj-test-tyBYlT/source
6770       761181 /usr/lib/git-core/git maintenance run --auto --no-quiet

So I think this fix is ultimately correct and should be merged. Let me guess: NixOS user?

[emilazy]

But why is it defaulting to /usr/bin? It should be looking at the $PATH. If it’s wrong in the tests, won’t it be wrong in production too?

[scott2000]

So I think this fix is ultimately correct and should be merged. Let me guess: NixOS user?

Yep :)

But why is it defaulting to /usr/bin? It should be looking at the $PATH. If it’s wrong in the tests, won’t it be wrong in production too?

I'm thinking that when $PATH isn't set, it defaults to searching common directories such as /usr/bin. Like when I run the jj binary locally, it succeeds, and I can see that it's calling the correct binary in strace (the one from $PATH). Maybe if we had set $PATH to some dummy value instead, then it would have failed in the tests before this change even when /usr/bin/git is present?

[emilazy]

Oh, do the tests unset $PATH? That would be the missing piece if so. But isn’t that kind of inconvenient for everyone, not just Nix users?

[scott2000]

Oh, do the tests unset $PATH? That would be the missing piece if so. But isn’t that kind of inconvenient for everyone, not just Nix users?

Yes, I think it's unset to ensure consistency while running tests (except on Windows it's sometimes still set due to something with MinGW). I think the reason it hasn't been inconvenient is because when $PATH is unset, it defaults to looking in the standard places anyway, so people running on non-NixOS machines wouldn't notice any issue usually.

I tested setting $PATH to a dummy value in 29f983e and it causes the tests to fail when $TEST_GIT_EXECUTABLE_PATH isn't set, since it no longer defaults to /usr/bin/git.

Maybe we shouldn't be clearing $PATH at all?

[arxanas]

As @scott2000 mentioned on Discord, this might be reasonable for setting a default search path on Linux: https://github.com/torvalds/linux/blob/405057718a1f9074133979a9f2ff0c9fa4a19948/arch/um/os-Linux/execvp.c#L54

FYI: git-branchless makes everyone set environment variables to run tests in any environment: https://github.com/arxanas/git-branchless/wiki/Runbook#running-tests-locally. Granted, it's more important because it integrates with Git more tightly and is more sensitive to version changes.

Also FYI: be aware that Git has its own GIT_EXEC_PATH environment variable that it'll search to find its own helper utilities. Usually, Git does a good job of finding them even with no configuration, but IIRC I've seen issues in CI where it failed to find stuff like git-remote-https at runtime. Now that we shell out for more things, you might see it.

[emilazy]

I think either we leave the inherited $PATH from the environment when running tests – it shouldn’t matter except when we spawn subprocesses, which is exactly when we want to inherit from the testing environment, and anyway making tests hermetic requires a more elaborate setup than just clearing a few variables – or we should set it to something that never exists anywhere so that it can be maximally isolated and everyone gets to suffer equally :)

[arxanas]

I didn't find anything obvious in the git-branchless commit history which strongly supports that we ought to clear out the PATH in all cases, but here's some related issues that it might be worth taking a look at while we're on the topic:

• fix(hooks): different PATH's name on windows and linux arxanas/git-branchless#1267: need to use Path instead of PATH on Windows. Recall that Windows environment variables are case-insensitive, which makes bugs nefarious instead of obvious.
• fix: make tests independent of system locale arxanas/git-branchless#1400: need to set LC_ALL=C to avoid Git printing out messages in other languages
  • In particular, we may need to do this for runtime correctness rather than just test correctness if we're parsing Git output 👀, cc @bsdinis
• Fix tests and execution on Windows arxanas/git-branchless#20: many Windows-related CI + environment issues, which could be a good resource to consult if we see any weird Windows CI issues in the future

---

This doesn't specifically support any argument, but for future reference, here's the sanitized PATH for git-branchless tests:

https://github.com/arxanas/git-branchless/blob/ca7bddba1e64a2c470af1128ca87281d9af53501/git-branchless-lib/src/testing.rs#L154-L171

And the full sanitized environment:

https://github.com/arxanas/git-branchless/blob/ca7bddba1e64a2c470af1128ca87281d9af53501/git-branchless-lib/src/testing.rs#L173-L206

[bsdinis]

Just to add some context here:

• the env var was added mirroring what git-branchless was doing
• the tests clear the PATH, so both Nix and Windows were failing to find the git executable
• for some reason, Ubuntu and MacOS were working, maybe because of this default searching on /usr/bin??
• I agree it's overall somewhat silly

[bsdinis]

In particular, we may need to do this for runtime correctness rather than just test correctness if we're parsing Git output 👀, cc @bsdinis

Opa, that's a doozy. I think we can just set LC_ALL=C in jj_cmd at cli/tests/common/mod.rc for the CLI tests.
But now that I think of it, we rely on parsing git output, so that should probably just be done directly when we spawn git.
It shouldn't be too big a problem (i.e., if jj implements locale aware stuff it can then just print the errors in the correct locale). The only possible problem would be with the remote: messages we aim to forward to the client (but I'm not even sure they are locale aware)

[scott2000]

I made a PR (#5468) 5468 to keep the $PATH variable (and also $Path on Windows), similar to @arxanas's handling in git-branchless. I didn't add LC_ALL=C or keep $GIT_EXEC_PATH yet, since we haven't run into any issues with them so far.

[scott2000]

Closing this PR as it is superseded by #5468.

[arxanas]

I made a PR (#5468) 5468 to keep the $PATH variable (and also $Path on Windows), similar to @arxanas's handling in git-branchless. I didn't add LC_ALL=C or keep $GIT_EXEC_PATH yet, since we haven't run into any issues with them so far.

@scott2000 sorry if I caused any churn here (such as for Path); I hadn't looked too carefully at the issues to see if they properly applied to jj 😅.

[scott2000]

sorry if I caused any churn here (such as for Path); I hadn't looked too carefully at the issues to see if they properly applied to jj 😅.

No worries! The discussion in that issue was actually really helpful since I couldn't find any good info online about whether std::env::var actually was case insensitive or not, and I don't have a Windows machine to test it out on.