Omit system/global config in fixture scripts even amidst strange filenames

EliahKagan commented 4 weeks ago

Since 33be0e0 (#966), configuration from the system and global scopes is cleared in the environment used to run test fixture scripts by setting GIT_CONFIG_SYSTEM and GIT_CONFIG_GLOBAL.

https://github.com/Byron/gitoxide/blob/dd65e7bd0adf7afeec377f47ba67970ae5126fa3/tests/tools/src/lib.rs#L591

https://github.com/Byron/gitoxide/blob/dd65e7bd0adf7afeec377f47ba67970ae5126fa3/tests/tools/src/lib.rs#L602-L603

This is usually effective, because there is usually not a file named : or -. However, these names are not treated specially, and they can exist, in which case configuration variables will be read from them. Because they would have to exist in the CWD of the fixture script, the impact is very limited. However, because tests (and thus their fixtures) often deliberately introduce edge cases, including unusual filenames, it is plausible for this problem to occur unintentionally.

Another factor that could lead to it is if such files are created accidentally, such as by passing - where it is meant to indicate stdin but not interpreted that way, or (though less likely) by using a : somewhere it is intended to be a separator when it is instead treated as a path. In particular, though I am not sure because they may just have been selected as valid but unlikely paths, it seems like those might have been the intentions of those names as they are currently used in gix-testtools. (The reason they do not have those meanings is that the : is in an environment variable that is interpreted as a single path rather than a list of paths, and the - is in an environment variable rather than on the command line.)

git(1) recommends using the null device path for GIT_CONFIG_SYSTEM and/or GIT_CONFIG_GLOBAL when one wishes to prevent real configuration associated with those scopes from being read:

GIT_CONFIG_GLOBAL GIT_CONFIG_SYSTEM

Take the configuration from the given files instead from global or system-level configuration files. If GIT_CONFIG_SYSTEM is set, the system config file defined at build time (usually /etc/gitconfig) will not be read. Likewise, if GIT_CONFIG_GLOBAL is set, neither $HOME/.gitconfig nor $XDG_CONFIG_HOME/git/config will be read. Can be set to /dev/null to skip reading configuration files of the respective level.

In addition to being more reliable for the reasons described above, this has two more (also minor) advantages:

It clearly expresses intent, since it achieves the effect more as the documentation recommends doing it.
Reading the configuration with git config and specifying the scope explicitly with --global or --system no longer gives exit code 128 due to an attempt to open a nonexistent file.

This PR makes that change. For Windows, where /dev/null is not generally a usable path for the null device, it uses NUL.

This also adds tests that the system and global scopes are cleared and that attempts to access configuration variables avoid unusual results, for situations:

Where they were already successfully cleared.
Where weirdly named files (files named : or - depending on platform) would inject configuration for them until the changes here (those tests fail before the bugfix and pass after it, as expected).
When an actual file called NUL exists on Windows in the directory being operated in. This is possible with \\?\ paths, and the main point of testing this is to show that accessing the null device as NUL still works and is unaffected by such a file. Thus this is not just renaming the hypothetical file that would mess things up, but eliminating it.

Regarding that last point, the main goal of explicitly testing with a file called NUL is demonstrative. But there may be a practical benefit, in that it is possible to write code that wrongly opens such a file, if one starts out with NUL and joins it to the result of calling canonicalize(). The canonicalize() functions in Rust return UNC paths on Windows, which may be unexpected. (This, albeit deliberately, is how I made the file in the first place.)

It think testing with a file actually named NUL is not really necessary, and I am unsure if it's worthwhile to keep this or not, because the tests can be shorter without it. But another option, rather than removing that, would be to move these unit tests of private members in tests/tools/src/lib.rs from there to a newly created tests/tools/src/tests.rs module. Or both changes could be made. For now I've left the elaborated tests and also not (yet?) moved them to a different file. (Neither these tests nor anything affected by this PR should be confused with the integration tests introduced in #1560, which are inside tests/tools/tests/.)

Although the topic of this PR is somewhat conceptually related to that of some other recent PRs such as #1567, the small bug this is addressing and the changes it makes are independent of those. This PR introduces and uses a private NULL_DEVICE constant for the null device path, which is defined the same way as in the gix-path crate as of 9917d47 (#1568). But it doesn't necessarily make sense for gix-path to expose that, gix-testtools also does not already directly declare gix-path as a dependency, and I have not thought of a reasonable other gitoxide crate that ought to have, and commit to having, it. So at least for now I think the duplication is acceptable.

EliahKagan commented 4 weeks ago

Although this really is independent of #1569 and all its predecessors, I think the history would be easier to understand if this is rebased anyway, since it looks like it would interact with those other recent changes even though it actually wouldn't. Given that running the tests again is not much of an inconvenience ever since the speedup in #1556, I'm going to go ahead and rebase this onto main.

Edit: Done.

EliahKagan commented 4 weeks ago

I am glad the overrides are now finally done properly.

A remaining question is whether high-scoped configuration that is associated with the git installation, but not in the system scope, should be omitted from the environment in which test fixture scripts run. This is the difference between setting GIT_CONFIG_SYSTEM to /dev/null (or NUL) and setting GIT_CONFIG_NOSYSTEM to 1 (or true, etc.).

This distinction between the scopes affected by GIT_CONFIG_SYSTEM and GIT_CONFIG_NOSYSTEM is the reason gix-path cannot pass --system to git config -l. This relates to the "unknown" scope mentioned https://github.com/Byron/gitoxide/pull/1523#issuecomment-2291515070 and in this code comment introduced in #1567:

https://github.com/Byron/gitoxide/blob/6f128dd6adf9148e859a0cd027ff1c0ba0b619c0/gix-path/src/env/git/mod.rs#L137-L140

So far, gix-testtools does not set GIT_CONFIG_NOSYSTEM and therefore does get the even higher "unknown" scope associated with an Apple Git installation. I suspect that leaving this in may be intentional and I am not sure that it really should be excluded.

Setting GIT_CONFIG_NOSYSTEM to 1 takes precedence over GIT_CONFIG_SYSTEM. So if that were done then a fixture script that doesn't clear GIT_CONFIG_NOSYSTEM but does set GIT_CONFIG_SYSTEM would not achieve the desired effect. I'm not sure how much of a drawback that is, however, since using GIT_CONFIG_SYSTEM to gain configuration variables in a fixture script seems much less likely than using GIT_CONFIG_GLOBAL, which is not suppressed by GIT_CONFIG_NOSYSTEM.

Setting variables in the local (or worktree) scope, as well as in the command scope with git -c or with GIT_CONFIG_{COUNT,KEY,VALUE}, are of course likewise not suppressed by anything to do with installation-level configuration, and these are even more likely ways than GIT_CONFIG_SYSTEM or GIT_CONFIG_GLOBAL for a test fixture script to set configuration for itself.

Byron commented 3 weeks ago

Thanks so much for interrupting the victory dance, I now think it's still unwarranted.

So far, gix-testtools does not set GIT_CONFIG_NOSYSTEM and therefore does get the even higher "unknown" scope associated with an Apple Git installation. I suspect that leaving this in may be intentional and I am not sure that it really should be excluded.

Actually, this sounds like a bug as tests should be isolated to just their own scope, without seeing anything related to the system.

Maybe this is something to try then - instead of setting GIT_CONFIG_SYSTEM to a null-value, one could suppress system scopes and the unknown scope.

I just tried this on MacOS and this works, leaving only the local scope.

> GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_NOSYSTEM=1 git config -l --name-only --show-origin --show-scope
local   file:.git/config        core.repositoryformatversion
local   file:.git/config        core.filemode
local   file:.git/config        core.bare
[..]

Whereas the following will still have the unknown scope like you said:

> GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null git config -l --name-only --show-origin --show-scope
unknown file:/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig    credential.helper
unknown file:/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig    init.defaultbranch
local   file:.git/config        core.repositoryformatversion
local   file:.git/config        core.filemode

Is this something you'd want to contribute?

EliahKagan commented 3 weeks ago

Yes. I have opened #1571 for this.

Byron / gitoxide

Omit system/global config in fixture scripts even amidst strange filenames #1570