Omit system/global config in fixture scripts even amidst strange filenames #1570
Conversation
|
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. |
Rather than just not having the configuration variable we add. - Keep testing the current failure mode where the unusually named `never_path` files can actually exist. - Also test for paths for those scopes' configuration files, coming from the environment. - Test that the output of showing the full scope's contents is empty, of all values. Note that this includes the system scope but not all scopes higher than the global scope: the "unknown" scope associated with an Apple Git configuration is deliberately not asserted to be cleared here. (`gix-testtools` possibly intentionally does not clear that scope for fixture scripts.)
This covers a possible concern pertaining to a forthcoming minor gix-testtools bugfix. The null device, accessible as `/dev/null` on a Unix-like system, or as `NUL` (and in some other ways) on Windows, is the best candidate to replace the current value of `:` (on Unix) or `-` (on Windows) for `GIT_CONFIG_SYSTEM` and `GIT_CONFIG_GLOBAL` when test fixture scripts are running. This is because the filenames `:` (on Unix) and `-` (on Windows) do not have any special meaning. They are valid but unusual filenames; files of these names most often do not exist, but can, and when they do, their contents are used the populate the system or global scope for test fixtures, which is not the intended behavior. `git` does not treat them as anything but filenames: a `:` is not treated as a separator because those variables are interpreted as single paths rather than lists of paths, and a `-` is not treated to mean standard input (or any other special data source) because it appears in an environment variable rather than as a command-line argument. But `/dev/null` and `NUL` can exist, too. On Unix, where `/dev/null` should be used, the whole point is that `/dev/null` exists. That absolute path is not treated specially. Instead, the device node for the null device is linked there. On Windows, where the null device is named `NUL`, an entry for it with that name does also sort of exist in the filesystem, accessible via the fully qualified device path `\\.\NUL` as well as by some other fully qualified forms such as `\??\NUL`. However, not all interfaces support all path forms, so accessing it in those ways is not necessarily more robust than accessing it with the relative path `NUL`. Yet it is possible for an actual file, not the null device, to exist with the name `NUL` in an arbitrary directory. Such a file can only be referred to using the UNC syntax with a `\\?\` prefixed path (not to be confused with the `\\.\` device namespace prefix, nor the NT `\??\` prefix). When such a file exists, `NUL` itself still does not refer to it; `NUL` still refers to the null device. The additon to the test here (and refactorings to support it) create an acutal file called `NUL` to demonstrate that changing `-` to `NUL` really does avoid getting a configuration file, regardless of what files exist.
Ordinarily such a check would not be justified, but the behavior of `canonicalize()` in giving UNC paths is non-obvious.
This uses the null device, `/dev/null` on Unix-like systems and `NUL` on Windows, as the value of `GIT_CONFIG_SYSTEM` and `GIT_CONFIG_GLOBAL` when `gix-testtols` runs test fixture shell scripts. `/dev/null` is explicitly recommended for this purpose, when setting those environment variables for the purpose of preventing configuration files from being read, in the Git documentation: - https://git-scm.com/docs/git#Documentation/git.txt-codeGITCONFIGGLOBALcode On Windows, `NUL` is an analogue of `/dev/null`. Even in the unusual scenario that a `\\?\` prefixed UNC path is used to create an actual file named `NUL` in the directory the fixture script operates in, the relative path `NUL` still resolves to the null device and not to that file. The previous behavior was to use a value of `:` on Unix-like systems or `-` on Windows. But these were really just unusual but valid paths, such that files of those names could exist in any location. `git` furthermore treats them as paths: a `:` is not special in these environment variables because they hold a single path rather than a list of paths, and a `-` is not special (for example, it does not specify stdin) because it appears in an environment variable rather than a command-line argument. While `:` and `-` are unusual filenames, this code is used in testing, including of edge cases where unusual files may be used. So this change may make the test tools slightly more robust.
Besides the style change done in the previous commit, `clippy` also
identified `File::create_new` is newer than we may wish to rely on:
error: current MSRV (Minimum Supported Rust Version) is `1.76.0` but this item is stable since `1.77.0`
--> tests/tools/src/lib.rs:906:13
|
906 | File::create_new(path)
| ^^^^^^^^^^^^^^^^
This changes that code to use `OpenOptions` instead. This is
simlar to the suggested equivalent code in the `File::create_new`
documentation:
File::options().read(true).write(true).create_new(true).open(...)
But the approach used here differs in that `read(true)` is removed,
since this call is only creating and writing, not reading.
It still does clean up (that is done by RAII; explicitly calling `close()` is just to be able to identify and handle errors). The assertion that it does is not part of what the test is testing, nor is it critical to the correctness of the test itself. I had put it there to catch bugs that might arise during development of the test itself. (In some libraries/frameworks, a recursive deletion on Windows that involves files that need to be accessed with `\\?\` paths does not automatically succeed.)
EliahKagan
force-pushed
the
tools-nulldev
branch
from
dc51d55 to
15bb2e3
Compare
EliahKagan
changed the title
A remaining question is whether high-scoped configuration that is associated with the This distinction between the scopes affected by So far, Setting Setting variables in the local (or worktree) scope, as well as in the command scope with |
|
Thanks so much for interrupting the victory dance, I now think it's still unwarranted.
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 I just tried this on MacOS and this works, leaving only the local scope. Whereas the following will still have the Is this something you'd want to contribute? |
|
Yes. I have opened #1571 for this. |
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_SYSTEMandGIT_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 ingix-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_SYSTEMand/orGIT_CONFIG_GLOBALwhen one wishes to prevent real configuration associated with those scopes from being read:In addition to being more reliable for the reasons described above, this has two more (also minor) advantages:
git configand specifying the scope explicitly with--globalor--systemno longer gives exit code 128 due to an attempt to open a nonexistent file.This PR makes that change. For Windows, where
/dev/nullis not generally a usable path for the null device, it usesNUL.This also adds tests that the system and global scopes are cleared and that attempts to access configuration variables avoid unusual results, for situations:
: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).NULexists 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 asNULstill 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
NULis 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 withNULand joins it to the result of callingcanonicalize(). Thecanonicalize()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
NULis 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 intests/tools/src/lib.rsfrom there to a newly createdtests/tools/src/tests.rsmodule. 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 insidetests/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_DEVICEconstant for the null device path, which is defined the same way as in thegix-pathcrate as of 9917d47 (#1568). But it doesn't necessarily make sense forgix-pathto expose that,gix-testtoolsalso does not already directly declaregix-pathas 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.