Omit other high-scoped config in fixtures #1571
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
It doesn't have to have any configuration variables set in the
command scope, though in practice it always should (with versions
of `git` with which `gix-testtools` is compatible) because we are
setting them with `GIT_CONFIG_{COUNT,KEY,VALUE}`, which, like `-c`,
sets configuration variables in the command scope.
But it must not have any configuration variables set in other
scopes. Of course, actual fixture scripts, when run, most often
create Git repositories, in which case there will in practice
almost always be local scope configuration for the script.
The main point here is to make sure we are omitting variables from
the global and system scopes, as we have already been doing (and
testing for), and also omitting them from the other high scopes,
such as the "unknown" scope associated with an Apple Git
installation that is higher than the system scope and unaffected
by `GIT_CONFIG_SYSTEM` but affected by `GIT_CONFIG_NOSYSTEM`.
Like the tests that preceded it, this test creates a new empty
temporary directory to set as the CWD of the test `git` command
configured by `configure_command`. As such, there should be no
local scope configuration, because this directory should be a
subdirectory of a system `/tmp`-like directory.
A `/tmp`-like directory can technically be a Git repository, and
can even contribute configuration if the repository is owned by the
current user or something is keeping `safe.directory` protections
from excluding it.
When the goal is to *get* configuration from scopes higher than the
local scope, it is worth taking steps to prevent this (which for
`gix-path` is achieved by the combination of GitoxideLabs#1523 and GitoxideLabs#1567).
But in the test suite, temporary directories should preferably be
in locations that are only `git` repositories (owned by the curent
user) in the unusual situation that this is desired, and supporting
tooling such as `git` should be at recent enough versions to really
support the usage that the test suite makes of it.
Furthermore, while it is possible to clear the local scope in this
new test -- such as by using, as the command's CWD, a subdirectory
of a directory that is, in the command's environment, prepended to
`GIT_CEILING_DIRECTORIES` -- this would tend to hide problems in
the actual `gix-testtools` code that this is testing.
If any such measure needs to be taken, it would be better done in
code that uses `configure_command` (or in `configure_command`
itself it it is widely needed and generally acceptable), rather
than in tests of `configure_command`.
For these reasons, the test is, at least for now, deliberately
written in such a way that it will fail if the directory we get
from `tempfile::TempDir::new()` is somehow a Git repository.
This commit consolidates the two preceding test cases into this
one. So now there is a single test that `configure-command` both:
- Excludes global and system scope config, as it already did.
- Also excludes other high-scoped config, which it doesn't yet do.
Thus, this new test is expected to fail on most macOS systems
(where `git` is Apple Git and the installation-associated "unknown"
scope configuration file is nonempty), until that is fixed.
In addition to keeping fixture scripts from receiving global and system scope Git configuration variables, as was already done, this also omits configuration variables from high scopes similar to or above the system scope, associated with the Git installation but separate from the system scope. The main and possibly only case where this happens is the "unknown" scope associated with an Apple Git installation on macOS. This is a file usually located under `/Library` or `/Applications`. This is done by using `GIT_CONFIG_NOSYSTEM`, which suppresses both the system scope and this separate "unknown" scope, instead of by settng `GIT_CONFIG_SYSTEM` to a path like `/dev/null`. The latter approach continues to be used to omit global scope config via `GIT_CONFIG_GLOBAL` (as `git` recognized no `GIT_CONFIG_NOGLOBAL`).
Byron
approved these changes
Sep 1, 2024
There was a problem hiding this comment.
Thanks a lot for the additional fix, it's much appreciated!
Even though I didn't test this locally beyond running the command-line with GIT_CONFIG_NOSYSTEM=1 to validate that indeed it won't show any higher scopes, which I assume will translate to CI as well where applicable.
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.
As requested in #1570 (comment), this has the
configure_commandhelper function ingix-testtoolsuseGIT_CONFIG_NOSYSTEMinstead ofGIT_CONFIG_SYSTEMto suppress Git configuration variables in scopes associated with thegitinstallation, even when this includes a scope other than the system scope.This is for the "unknown" scope in Apple Git. I am not aware of other practical cases where this applies, though if they exist then this should help with them too.
Due to having only sporadic and limited access to macOS outside of CI, I have not tested this locally on macOS. Because I think the
gitinstallation on CI runners is always a more recent version set up when customizing the image, I would not expect the consolidated and extended test to fail before the fix on CI.If feasible, I suggest running the test locally on macOS at the first commit (d576b32) to confirm that it fails, and at or after the second commit (a879d22) to confirm that it passes. I do not know any reasonably reliable way to test this in the test suite on other systems; I don't think most builds of Git support such scopes, nor should the test suite actually add, remove, or alter existing configuration files that would affect other programs running on the system.
There is some more information in the commit messages. In particular, I consolidated the two existing tests (from #1570) and extended the resulting single test so that, in addition to effectively covering what was covered before, it tests that no other configuration from files is present in the output of
git config -l --show-originwhenconfigure_commandis used. Whileconfigure_commandadds configuration variables, they are in the command scope, rather than files. Excluding other files will automatically cover the "unknown" scope. It will also automatically cover the local scope. More details about this, including why I made it so even local scope variables will make the test fail, is included in the first commit message.