Don't read "installation" config from GIT_CONFIG
#1583
Merged
Conversation
This file contains 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 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.
These tests should not pass yet, because we currently do allow `GIT_CONFIG` to be read. Although the optimization that reads `EXEPATH` will sometimes prevent it from being used, it will usually be used, becuase `EXEPATH` is usually not set on Windows (and outside of Windows, that optimization is inapplicable). Furthermore, the tests in `gix-path/src/env/git/tests.rs` bypass that optimization. Two of the three new tests rightly fail. But one of them, `never_from_git_config_env_var`, wrongly passes, due to a big in the test, described in the comments there.
So it fails too, as expected.
When `gix-path` runs `git config -l ...` to obtain the path of a configuration file to regard as being associated with the `git` installation itself, it now no longer allows `GIT_CONFIG` to affect this path. Previously, setting `GIT_CONFIG` would treat it as the path of a `GitInstallation` configuration file. Although it is possible that this may have been used intentionally, it was never documented, did not work reliably on all platforms, and carried significant disadvantages. This is most likely a non-breaking change. The disadvantages of treating a path in `GIT_CONFIG` as the path to a configuration file associated with the `git` installation were: - For `git`, the `GIT_CONFIG` environment variable *only* affects `git config` commands, and not other commands that use configuration variables (which are most `git` commands). But when `gix-path` would obtain a path from `git config -l ...` that came from `GIT_CONFIG`, that configuration file would be used anywhere, and not only `gix config` commands. - Even for `gix config`, it is not at all clear that the `GIT_CONFIG` environment variable ought to be honored, because `gix config` has a different interface from and is not meant to be a drop-in replacement for `git config`, and because this environment variable is only even supported by `git config` for historical reasons. (In `git config`, one should typically use `--file` instead.) - The installation path is generally the path of a high-scoped configuration file, usually the system scope or, with Apple Git on macOS, the "unknown" scope even higher than that (of a file located under `/Library` or `/Applications`). In contrast, the `GIT_CONFIG` environment variable is really command scope, since it is an alternative way of achieving the same goal as `--file` when running `git config`, which `git` supports only for backward compatibility. - On Windows, when `EXEPATH` is set in a way that indentifies a Git for Windows installation directory, which is typically the case in a Git Bash environment (and not otherwise), `GIT_CONFIG` is not used by the public `gix-path` functions that ordinarily get information from calling `git config -l ...`, because this call is not performed and instead the location of a configuration file associated with the installation is inferred from that value. Although this also applies to some other ways the environment affects the behavior of `git config -l ...`, it is much stranger for `GIT_CONFIG`, because whether `GIT_CONFIG` should be used does not intuitively seem like it should be related to what other sources of information are available; the semantics of `GIT_CONFIG` for `git` are to be ignored by most commands, but to always be used commands (`git config`) that do not ignore it. - The effect on finding other files associated with the installation, such as a gitattributes file, may be especially hard to reason about for `GIT_CONFIG`. Such paths are sometimes inferred from the path that `gix-path` reports. In particular, this is how `installation_config_prefix()` works. This change only prevents `GIT_CONFIG` from specifying an installation config file or suppressing the search for it. This does not affect how other `GIT_CONFIG_*` environment variables are used.
Byron
approved these changes
Sep 6, 2024
There was a problem hiding this comment.
Thanks so much, I really appreciate your work (and sometimes feel I don't make that explicit often enough 😅).
There is more information in the commit messages, especially the one prefixed
fix:, eb72d31. This also adds tests related toGIT_CONFIG, in the two commits that precede that, 340ff37 and f8e38d0.
I would only indicate breaking changes if the API breaks - changes in behaviour could qualify, of course, but thus far I didn't encounter them.
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 makes it so
GIT_HIGHEST_SCOPE_CONFIG_PATHnever uses the value of theGIT_CONFIGenvironment variable. Previously it used it becausegit configuses it, treating it as though its value were an operand to--file. But no othergitcommands use it.It was previously used in most situations when present, since it causes
git config -lto list nothing else of any scope (even if very high scoped configuration is available). The main and possibly only significant situation where it was not used was theEXEPATHoptimization on Windows (which in practice only occurs in Git Bash environments). I think this inconsistency--since it never has anything to do with whether a user would wantGIT_CONFIGto have an effect--is one of the most significant reasons to omit it.However, when reviewing this, I recommend considering if possible uses where a user would set
GIT_CONFIGto view configuration from an arbitrary file with commands likegix config--even though it would not work from Git Bash--might be something people are doing. I have prefixed the commit messagefix:but maybe I should have writtenfix!:for this reason? I am not sure. But as far as I know, this is not altering any behavior to become inconsistent with what was previously documented or otherwise stated.There is more information in the commit messages, especially the one prefixed
fix:, eb72d31. This also adds tests related toGIT_CONFIG, in the two commits that precede that, 340ff37 and f8e38d0.I have also unset some other environment variables (that would only occasionally be set in the first place) besides
GIT_CONFIG. This is in the first commit, 01a412f, and it is for a different reason: it is to ensure that even if some version ofgitbehaves weirdly, or if my understanding of its intended behavior is incorrect, then we still get the full effect of settingGIT_DIR. The commit message has more information about that. I mostly included that here rather than in a separate PR to avoid a merge conflict (or decreased code readability if theremove_envcalls were to be merged in an unintuitive order).