Unset other env vars related to GIT_DIR for fixtures
#1581
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This removes other environment variables that have an effect conceptually related to `GIT_DIR` even when `GIT_DIR` is not set. Most of them change where `git` will look for files that are ordinarily in a repository's `.git` directory. In contrast, `GIT_WORK_TREE` changes where the working tree is found. These would rarely be set in the environment in which the tests are run, but it makes sense to unset them for the same reason as unsetting `GIT_DIR`, which is already done. The new `remove_env` calls are roughly in the order in which the variables they unset are listed in git(1). This deliberately does not attempt to unset every possible environment variable that git(1) documents as affecting its behavior. This is for four reasons: - Some variables may be set on the test machine without envisioning this usage, but should still be kept, such as those that cause more or less traversal than usual to be done. For example, if `GIT_CEILING_DIRECTORIES` or even `GIT_DISCOVERY_ACROSS_FILESYSTEM` are set, it may be for a good reason. - Some variables will have no default unless other variables that are being modified here are changed again after the changes here. In particular, `GIT_CONFIG_SYSTEM` only has an effect if `GIT_CONFIG_NOSYSTEM` is not set. We set `GIT_CONFIG_NOSYSTEM` to `1`, so if it is unset then a fixture script has unset it, in which case it is presumably intended that `GIT_CONFIG_SYSTEM` have some effect (if the fixture script doesn't change/unset it). - Some variables are useful for extra debugging and make sense to set when running the test fixtures under foreseeable conditions. For example, the effects of all `GIT_TRACE*` variables are intentionally preserved. - For a few variables, such as `GIT_DEFAULT_HASH`, it is unlikely that they would be wanted in the test environment, but even more unlikely that they would be set in that environment without the intention of experimenting with their effect on fixtures. However, this is not to say that all environment variables that would make sense to remove have necessarily been removed. The removed variables here differ from those removed for the `git` invocation in `gix-path/src/env/git/mod.rs` for two reasons: - That is a single `git` invocation for a specific command, so the environment variables that ought to affect it must be kept, and others can be removed. But here, arbitrary fixtures need to work, and they provide almost all of their own environment as needed. - Setting an unusual value of `GIT_DIR` there that `git` cannot take to be a usable repository also prevents the variables that override `GIT_DIR` for specific files from being used. (But maybe those should be unset there anyway, for clarity?)
EliahKagan
force-pushed
the
fixture-env
branch
from
d7ae44f to
9d4dd12
Compare
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Sep 6, 2024
This has the `git config -l ...` invocation `gix-path` uses to find a configuration file to treat as associated with the `git` installation remove more environment variables related to `GIT_DIR` besides `GIT_COMMON_DIR`. The new removals probably don't make a difference, because they are only used if `GIT_DIR` is either unset or set to a location that actually has a Git repository (that `git` is willing to use). Unsetting these others is thus more to make clear that they have no effect than to prevent them from having an effect, though various versions and downstream releases of `git` exist, so it's narrowly possible that this is also avoiding a problem somewhere, I guess. This change is motivated by my realization in 9d4dd12 (GitoxideLabs#1581) that it could be slightly beneficial. Other differences between the variables unset here and in `gix-testtools` remain due to the differences described in that commit message.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Sep 6, 2024
This has the `git config -l ...` invocation `gix-path` uses to find a configuration file to treat as associated with the `git` installation remove more environment variables related to `GIT_DIR` besides `GIT_COMMON_DIR`. The new removals probably don't make a difference, because they are only used if `GIT_DIR` is either unset or set to a location that actually has a Git repository (that `git` is willing to use). Unsetting these others is thus more to make clear that they have no effect than to prevent them from having an effect, though various versions and downstream releases of `git` exist, so it's narrowly possible that this is also avoiding a problem somewhere, I guess. This change is motivated by my realization in 9d4dd12 (GitoxideLabs#1581) that it could be slightly beneficial. Other differences between the variables unset here and in `gix-testtools` remain due to the differences described in that commit message.
Byron
approved these changes
Sep 6, 2024
There was a problem hiding this comment.
This is great, thanks a lot!
On another note, it's interesting to wonder if, or to what extend, gitoxide should respect such environment variables. If they are undocumented, the answer is not at all, but otherwise they would need to be given some consideration.
Facilities exist to control which environment variables gitoxide uses, and methods exist to respect environment variables specifically, and maybe some of that can be extended should there be any need.
|
I've noticed this PR has been open for a while since being approved. Was auto-merge intended to have picked it up? |
|
Strange, I completely missed this one (and your message) :/. |
|
This could happen if you accidentally unsubscribed from notifications on this PR specifically, which I have found is easy to do on the Android app. But that wouldn't have an effect on auto-merge. All pushes here (including force pushes) substantially preceded the approving review, so I don't know what would cause that. But it may be that I am misunderstanding the circumstances required for auto-merge to take effect. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes other environment variables that have an effect conceptually related to
GIT_DIReven whenGIT_DIRis not set. Most of them change wheregitwill look for files that are ordinarily in a repository's.gitdirectory. In contrast,GIT_WORK_TREEchanges where the working tree is found.Significant further details are in the commit message, in case they are of interest now or in the future. I did not add tests because the likelihood that there is a mistake here that new tests would find is low, and because tests that go beyond effectively repeating the changes to the code under test would effectively run into the concerns described in #1577 or #1578.