-
-
Notifications
You must be signed in to change notification settings - Fork 311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test cross-platform symlink and +x permission expectations on Windows #1652
Conversation
Since GitoxideLabs#1618, which introduced `gix-merge::merge tree::run_baseline` and the functionality it tests, that test has failed on Windows when run with `GIX_TEST_IGNORE_ARCHIVES=1`. This happens because, while `chmod +x` commands run and exit indicating success in MSYS environments such as Git Bash, they do not actually make a change. Multiple cases (all sub-cases of the same faililing test) would fail this way. But the test fails fast, so only one is reported. The first to fail this way is `same-rename-different-mode`, which shows: --- STDERR: gix-merge::merge tree::run_baseline --- failed to extract 'tests\fixtures\generated-archives\tree-baseline.tar': Ignoring archive at 'tests\fixtures\generated-archives\tree-baseline.tar' as GIX_TEST_IGNORE_ARCHIVES is set. thread 'tree::run_baseline' panicked at gix-merge\tests\merge\tree\mod.rs:83:21: assertion `left != right` failed: same-rename-different-mode-A-B-reversed: Git caught up - adjust expectation Git works for the A/B case, but for B/A it forgets to set the executable bit left: Sha1(d75339daa64f3a1d98947623eee7bbb218e6c87d) right: Sha1(d75339daa64f3a1d98947623eee7bbb218e6c87d) This commit fixes that by using `git update-index --chmod=+x` to stage the intended effect of `chmod +x`. The `chmod +x` commands are still run, to avoid non-intended and potentially bug-prone or confusing differences between the metadata on disk and what is staged and commited, on systems where Unix-style permissions are supported on disk and `chmod +x` modifies them. Although only one of the two approaches is needed on any particular system in these tests, deciding based on the operating system is more complicated. It would also be misleading, because the effect of the two kinds of commands is not, in *general*, the same: one operates on files in the working tree, and the other operates on the index. Therefore, both are used, for simplicity and clarity. For readability, adjacent `chmod +x` commands are also combined into one. This way, each place that has both `chmod +x` logic and `git update-index --chmod=+x` logic can have a single command for each, that clearly relate to each other. As noted above, the change in this commit is not sufficient to fix that failing test, because a subsequent assertion still fails. This will be detailed and fixed in a subsequent commit. (This commit also does not yet update generated archives.)
This fixes the second `gix-merge::merge tree::run_baseline` failure in which the baseline fixture `added-file-changed-content-and-mode` produced the wrong results when run on Windows, due to `chmod +x` having no effect (but reporting success) when run in Git Bash. The failure that this fixes, which is visible since the fix for the first baseline case failure was fixed in the preceding commit, is: --- STDERR: gix-merge::merge tree::run_baseline --- failed to extract 'tests\fixtures\generated-archives\tree-baseline.tar': Ignoring archive at 'tests\fixtures\generated-archives\tree-baseline.tar' as GIX_TEST_IGNORE_ARCHIVES is set. thread 'tree::run_baseline' panicked at gix-merge\tests\merge\tree\mod.rs:93:17: assertion failed: `(left == right)`: added-file-changed-content-and-mode-A-B-reversed: tree mismatch: We improve on executable bit handling, but loose on diff quality as we are definitely missing some tweaks [ Conflict { resolution: Ok( OursModifiedTheirsModifiedThenBlobContentMerge { merged_blob: ContentMerge { merged_blob_id: Sha1(7ad0b902fe3f9d6857ca2e02a84f17c3eae607d6), resolution: Conflict, }, }, ), ours: Addition { location: "new", relation: None, entry_mode: EntryMode( 33188, ), id: Sha1(ec046db5d8b05470e472330d75f419c81ed6c9d3), }, theirs: Addition { location: "new", relation: None, entry_mode: EntryMode( 33188, ), id: Sha1(8a1218a1024a212bb3db30becd860315f9f3ac52), }, map: Original, }, ] added-file-changed-content-and-mode-A-B-reversed Diff < left / right > : <297247d >c5c706c ├── a:fd269b6 │ └── x.f:100644:4406528 "original\n1\n2\n3\n4\n5\n" <└── new:100644:7ad0b90 "<<<<<<< B\noriginal\n1\n2\n3\n4\n5\n6\n=======\n1\n2\n3\n4\n5\n>>>>>>> A\n" >└── new:100644:c3e610f "<<<<<<< A\n1\n2\n3\n4\n5\n=======\noriginal\n1\n2\n3\n4\n5\n6\n>>>>>>> B\n" See the previous commit for a general description. The situation here differs because the file, `new`, is newly created at the same time as its mode is set, so it is not already in the index. While staging it and using the same approach used in the previous commit would work, this instead adds `--chmod=+x` to the existing `git add` commands to stage them with the desired permissions initially, since here those commands immediately follow the `chmod +x` commands. To make this work (and so it is clear), this also changes the path argument to refer to the specific file, rather than passing `.`, which had alredy been done in one place. These commands are (and were) only staging a single file, so this is sufficient. It turns out that the baseline cases fixed in the preceding commit and here are the only causes of failure for that test, so the test is now passing, even on Windows with `GIX_TEST_IGNORE_ARCHIVES=1`.
As far as I know, gix-archive has no limitations related to Unix executable bits, on any platform. On Windows, the *filesystem* does not support these, and `chmod +x` commands in fixtures run in Git Bash appear to succeed but actually have no effect. But +x metadata can be staged and committed in Git repositories. When that is done, gix-archive can use those metadata just as it does on a Unix-like system. This fixes the tests to reflect this ability. Changes: 1. Add a `git update-index --chmod=+x` command to the gix-archive `basic.sh` fixture for `dir/subdir/exe`, so that even if the `chmod +x` command has no effect, the executable bit is set. This only affects `dir/subdir/exe`. It does not affect `extra-exe`, since that is never staged. On Windows, `extra-exe` can never have any associated executable mode bits. 2. Update the `basic_usage_internal` test to assert that `dir/subdir/exe` is `EntryKind::BlobExecutable` on all platforms, i.e., no longer `EntryKind::Blob` on Windows. Without this change, the change in (1) causes the test to fail. This also refactors to remove the `expected_exe_mode` constant, since its value is now only used in one place (for `extra-exe`), and to remove `expected_link_mode`, which has unconditionally been another name for `EntryKind::Link` since 93e088a (GitoxideLabs#1444). 3. Update the `basic_usage_tar` test to assert that the mode stored for `prefix/dir/subdir/exe` is 493 (0o755) on all platforms, i.e., no longer 420 (0o644) on Windows. This is analogous to (2), and without this the `basic_usage_tar` test fails due to the changes in (1). As in (2), this includes refactoring: `expected_exe_mode` is removed now that the choice between 420 (0o644) and 493 (0o755) is only made in one place (for `prefix/extra-exe`), and `expected_symlink_type` is removed, since it has unconditionally been another name for `EntryType::Symlink` since 93e088a (GitoxideLabs#1444). For future reference, with (1) but before (2), the failure is: --- STDERR: gix-archive::archive from_tree::basic_usage_internal --- Archive at 'tests\fixtures\generated-archives\basic.tar' not found, creating fixture using script 'basic.sh' thread 'from_tree::basic_usage_internal' panicked at gix-archive\tests\archive.rs:36:13: assertion `left == right` failed left: [(".gitattributes", Blob, Sha1(45c160c35c17ad264b96431cceb9793160396e99)), ("a", Blob, Sha1(45b983be36b73c0788dc9cbcb76cbb80fc7bb057)), ("symlink-to-a", Link, Sha1(2e65efe2a145dda7ee51d1741299f848e5bf752e)), ("dir/b", Blob, Sha1(ab4a98190cf776b43cb0fe57cef231fb93fd07e6)), ("dir/subdir/exe", BlobExecutable, Sha1(e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)), ("extra-file", Blob, Sha1(0000000000000000000000000000000000000000)), ("extra-exe", Blob, Sha1(0000000000000000000000000000000000000000)), ("extra-dir-empty", Tree, Sha1(0000000000000000000000000000000000000000)), ("extra-dir/symlink-to-extra", Link, Sha1(0000000000000000000000000000000000000000))] right: [(".gitattributes", Blob, Sha1(45c160c35c17ad264b96431cceb9793160396e99)), ("a", Blob, Sha1(45b983be36b73c0788dc9cbcb76cbb80fc7bb057)), ("symlink-to-a", Link, Sha1(2e65efe2a145dda7ee51d1741299f848e5bf752e)), ("dir/b", Blob, Sha1(ab4a98190cf776b43cb0fe57cef231fb93fd07e6)), ("dir/subdir/exe", Blob, Sha1(e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)), ("extra-file", Blob, Sha1(0000000000000000000000000000000000000000)), ("extra-exe", Blob, Sha1(0000000000000000000000000000000000000000)), ("extra-dir-empty", Tree, Sha1(0000000000000000000000000000000000000000)), ("extra-dir/symlink-to-extra", Link, Sha1(0000000000000000000000000000000000000000))] And with (1) but before (3), the failure is: --- STDERR: gix-archive::archive from_tree::basic_usage_tar --- thread 'from_tree::basic_usage_tar' panicked at gix-archive\tests\archive.rs:116:13: assertion `left == right` failed left: [("prefix/.gitattributes", Regular, 56, 420), ("prefix/a", Regular, 3, 420), ("prefix/symlink-to-a", Symlink, 0, 420), ("prefix/dir/b", Regular, 3, 420), ("prefix/dir/subdir/exe", Regular, 0, 493), ("prefix/extra-file", Regular, 21, 420), ("prefix/extra-exe", Regular, 0, 420), ("prefix/extra-dir-empty", Directory, 0, 420), ("prefix/extra-dir/symlink-to-extra", Symlink, 0, 420)] right: [("prefix/.gitattributes", Regular, 56, 420), ("prefix/a", Regular, 3, 420), ("prefix/symlink-to-a", Symlink, 0, 420), ("prefix/dir/b", Regular, 3, 420), ("prefix/dir/subdir/exe", Regular, 0, 420), ("prefix/extra-file", Regular, 21, 420), ("prefix/extra-exe", Regular, 0, 420), ("prefix/extra-dir-empty", Directory, 0, 420), ("prefix/extra-dir/symlink-to-extra", Symlink, 0, 420)] note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
This adds `git update-index --chmod=+x` commands in two of the gix-index v2 fixture scripts so that they can set executable bits on Windows, where the plain `chmod +x` commands have no effect.
This adds a `git update-index --chmod=+x` command in the gix-status `status_changed.sh` fixture script before the `git commit` command, so that the commit holds the same mode bits on Windows, where `chmod +x` does not take effect, as on Unix-like systems where it is generally does take effect. This change is done for the purpose of making the fixture be as was intended. But there is no corresponding change to be made to test cases that use it. The tests are asserting different things on Windows and non-Windows systems about changes in the working tree after a commit. The fixture script deliberately does not stage such changes, so for the `chmod +x` and `chmod -x` commands appearing after that in the fixture script, there is no change to the index that can correctly be made to unify the behavior across platforms. Windows doesn't support Unix-style executable permissions in the filesystem, so the state of the working tree still has to be asserted differently on Windows. Although the mode is different in the repo metadata due to this change, it still is not found to disagree with the mode on disk, since the latter information is absent. (Because the files are staged, is implicitly taken to agree with whichever of +x or -x is already recorded in the repo for them.)
For these the current situation is simpler because the tests currently using them do not contain assertions whose correctness varies based on whether the affected files modes' have +x or -x. But it is the intent of the fixtures to record +x modes for some particular files in the test repository, so this modifies the scripts to do so, even on Windows where `chmod +x` does not take effect. This also allows the tests to include a broader range of metadata and, specifically, may verify in some comparisons that having a mode of 0o755 instead of 0o644 does not cause a problem, even though not explicitly asserted in detail.
A few of the gix-worktree-state checkout tests have been checking if the filesystem supports symlinks, while one was skipped (marked ignored) on Windows based on the idea that symlinks would not be created, and also had an assertion that assumed symlinks would not be successfully created. This commit changes such tests so that they run on all platforms, including Windows, and so that, on all platforms, they assert that the filesystem supports symlinks, and assert that expected symlinks are created after attempts to do so. (The reason to assert that the filesystem supports symlinks is so that if this is not detected, either due to a failure or detection or due to the filesystem really not supporting symlinks, the test failures will be clear, rather than having a later assertion fail for unclear reasons.) Since GitoxideLabs#1444, tests involving symlinks are expected to work even on Windows. That PR included changes to the way fixtures were run, and to other parts of the test suite, to cause symlinks to be created in cases where they had previously had not (GitoxideLabs#1443). A number of tests had been assumed not to work due to limitations of Windows, MSYS, or Git: - Although Windows will not allow all users to create symlinks under all configurations, the test suite expects to be run on a Windows system configured to permit this. - Although `ln -s` and similar commands in MSYS environments, including Git Bash, do not by default attempt to create actual symlinks, this does happen when symlink creation is enabled using the `MSYS` environment variable, as done in 0899c2e (GitoxideLabs#1444). (This situation differs from that of Unix-style executable bits, which do not have filesystem support on Windows. While `chmod +x` and `chmod -x` commands do not take effect on Windows, which slightly limits the ability to test such metadata and requires that a number of fixtures set the mode directly in the ndex, with symlinks there is no such inherent restriction. Provided that the `MSYS` environment variable is set to allow it, which gix-testtools takes care of since GitoxideLabs#1444, and that Windows permits the user running the test suite to create symlinks, which is already needed to properly run the test suite on Windows, the same `ln -s` commands in fixture scripts that work on Unix-like systems will also work on Windows.) - Although `git` commands will not check out symlinks as actual symlinks on Windows unless `core.symlinks` is set to `true`, this is not typically required for the way symlinks are used in the gitoixde test suite. Instead, we usually test the presence of expected symlink metadata in repository data structures such as an index and trees, as well as the ability of gitoxide to check out symlinks. (We do not intentionally test the ability to run `ln -s` in Git Bash, but this is needed in order to create a number of the repositories for testing. Having `git` check out symlinks is not typically needed for this.) In addition, since we are requiring that Windows test environments permit the user running the test suite to create symlinks, any failures that arise in the future due to greater sensitivity to `core.symlinks` (see GitoxideLabs#1353 for context) could be worked around by setting that configuration variable for the tests, either in gix-testtools via `GIT_CONFIG_{COUNT,KEY,VALUE}` or in the specifically affected fixture scripts. While GitoxideLabs#1444 updated a number of tests to reflect the ability to create symlinks in fixture scripts and the wish to test them on all platforms including Windows, some tests remain to be updated. This commit covers the gix-worktree-statte checkout tests. This does not cover even their associated fixtures, which can already create symlinks (given the above described conditions), but that should be updated so they can set intended executable permissions (see above on `chmod`). This will be done separately.
This adds `git update-index --chmod=+x` commands to the `make_mixed`, `make_mixed_without_submodules`, and `make_mixed_without_submodules_and_symlinks` fixtures used in gix-worktree-state `tests/state/checkout.rs` tests, so that files are staged and committed with the intended modes. The tests were actually passing before this change, but making it verifies that the tests still work even when the repository is as assumed. (Also, some of those tests are recently enabled or made more expansive on Windows, due to the preceding commit that makes them test symlinks, where applicable, on all platforms.)
As in 470c76e, this updates tests to reflect the ability of Git repositories to represent Unix-style executable permissions in an index and commits, and the ability of gitoxide to operate on this, on all platforms, including Windows where the filesystem itself does not support this kind of executable permissons. Changes: 1. Adds a `git update-index --chmod=+x` command in the basic.sh fixture for gix-worktree-steam, so `dir/subtree/exe` is marked executable in the repository, even on Windows where the filesystem itself does not support Unix-style executable permissions and where, in Git Bash, `chmod +x` has no effect. This does not affect `extra-exe`, which is never staged. Since `extra-exe` is just an extra file in the working tree, it cannot be marked `+x` on Windows. 2. Updates the `paths_and_modes` assertion in `will_provide_all_information_and_respect_export_ignore` to assert the correct mode, with `+x` set, in the staged/committed file `dir/subtree/exe`. The `extra-exe` part of the assertion is unchanged in meaning, but the `expected_exe_mode` would only be used in that one place now, so this removes it and replaces it with the conditional expression for its value based on whether we are using Windows. While doing that refactoring, this also removes the `expected_link_mode` constant, which has unconditionally aliased `EntryKind::Link` since 93e088a (GitoxideLabs#1444). These changes are directly analogous to (1) and (2) in 470c76e. Here, they are made to the gix-worktree-stream tests, while in 470c76e they were made to the gix-archive tests (along with another related change, in (3), for which there is nothing anlogous to be done here). For reference, with *this* commit's change (1) described above but without *this* commit's change (2), the failure is: --- STDERR: gix-worktree-stream::stream from_tree::will_provide_all_information_and_respect_export_ignore --- Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.45s thread 'from_tree::will_provide_all_information_and_respect_export_ignore' panicked at gix-worktree-stream\tests\stream.rs:119:9: assertion `left == right` failed left: [(".gitattributes", Blob, Sha1(45c160c35c17ad264b96431cceb9793160396e99)), ("a", Blob, Sha1(45b983be36b73c0788dc9cbcb76cbb80fc7bb057)), ("bigfile", Blob, Sha1(4995fde49ed64e043977e22539f66a0d372dd129)), ("symlink-to-a", Link, Sha1(2e65efe2a145dda7ee51d1741299f848e5bf752e)), ("dir/.gitattributes", Blob, Sha1(81b9a375276405703e05be6cecf0fc1c8b8eed64)), ("dir/b", Blob, Sha1(ab4a98190cf776b43cb0fe57cef231fb93fd07e6)), ("dir/subdir/exe", BlobExecutable, Sha1(e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)), ("dir/subdir/streamed", Blob, Sha1(08991f58f4de5d85b61c0f87f3ac053c79d0e739)), ("extra-file", Blob, Sha1(0000000000000000000000000000000000000000)), ("extra-bigfile", Blob, Sha1(0000000000000000000000000000000000000000)), ("extra-exe", Blob, Sha1(0000000000000000000000000000000000000000)), ("extra-dir-empty", Tree, Sha1(0000000000000000000000000000000000000000)), ("extra-dir/symlink-to-extra", Link, Sha1(0000000000000000000000000000000000000000))] right: [(".gitattributes", Blob, Sha1(45c160c35c17ad264b96431cceb9793160396e99)), ("a", Blob, Sha1(45b983be36b73c0788dc9cbcb76cbb80fc7bb057)), ("bigfile", Blob, Sha1(4995fde49ed64e043977e22539f66a0d372dd129)), ("symlink-to-a", Link, Sha1(2e65efe2a145dda7ee51d1741299f848e5bf752e)), ("dir/.gitattributes", Blob, Sha1(81b9a375276405703e05be6cecf0fc1c8b8eed64)), ("dir/b", Blob, Sha1(ab4a98190cf776b43cb0fe57cef231fb93fd07e6)), ("dir/subdir/exe", Blob, Sha1(e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)), ("dir/subdir/streamed", Blob, Sha1(08991f58f4de5d85b61c0f87f3ac053c79d0e739)), ("extra-file", Blob, Sha1(0000000000000000000000000000000000000000)), ("extra-bigfile", Blob, Sha1(0000000000000000000000000000000000000000)), ("extra-exe", Blob, Sha1(0000000000000000000000000000000000000000)), ("extra-dir-empty", Tree, Sha1(0000000000000000000000000000000000000000)), ("extra-dir/symlink-to-extra", Link, Sha1(0000000000000000000000000000000000000000))] note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
The make_rev_spec_parse_repos fixture script used `chmod 755` to make a looose object file writable (so it could be corrupted, to test the treatment of corrupted objects). But this also made it executable, which was not needed (on any platforms). This commit changes it from 755 to 644 so that it is still made writable, but not made executable. (It does not start out being executable.) This does not affect the results of any tests.
In the preceding two commits, the make_rev_spec_parse_repos fixture was modified to avoid giving extra executable permissions to a loose object file where they are not needed, and the affected fixture archive was regenerated. Though the permissions change is itself good and causes no problems, the overall change caused two problems, which are corrected here: 1. I had taken the opportunity to follow better practices when running commands in a shell script whose arguments are formed by parameter expansion: adding quoting where splitting and globbing is not intended but could in principle also be indicated; and preceding the argument formed this way with a `--` to designate it clearly as a non-option argument, since `chmod` follows the XBD Utility Syntax Guidelines, which include `--` recognition. While adding quoting was a good change (in this case, just for clarity that no expansions are intended), the way I added `--` created a new problem where none had existed. This is because I wrongly thought of it as separating non-filename arguments from filename arguments, which is incorrect: in `chmod`, a mode argument is neither an option or an operand to an option. Accordingly, only some implementations of `chmod` allow it to be placed after the mode. This commit corrects that by placing it before the mode argument instead, which is portable while still achieving the goal of establishing the argument after it as as never being meant to be interpreted as an option (regardless of whether the system's `chmod` recognizes options after non-option arguments). 2. Due to GitoxideLabs#1622, regenerating `make_rev_spec_parse_repos.tar` with Git 2.47.0 causes revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches to to fail on all systems with all versions of Git, whenever `GIX_TEST_IGNORE_ARCHIVES` is *not* set. This differs from the usual situation where it fails only when that *is* set and only when the available Git is >= 2.47.0. This causes the test to fail in the `test-fast` CI job, since the mitigation in GitoxideLabs#1635 for when the tests are detected to be running on CI deliberately covers only the `GIX_TEST_IGNORE_ARCHIVES` case. In the previous commit, I had regenerated that archive on an Ubuntu 24.04 LTS system with Git 2.47.0 installed from the git-core PPA, causing this problem. This commit regenerates the archive again on a macOS 15.0.1 system with Git 2.39.5 (Apple Git-154), using the command: TZ=UTC cargo nextest run --all --no-fail-fast All tests passed and the archive was successfully remade. I used `TZ=UTC` since I usually regenerate archives on a system whose time zone is configured to be UTC rather than local time, and more specifically because there is an unrelated bug (to be separately reported) causing an unrelated test to fail in some time zones in the two weeks that follow daylight saving time adjustments.
As detailed in d715e4a, most tests involving symlinks have been expected to work even on Windows since GitoxideLabs#1444, but some tests remain to be updated to be run on Windows or to include all symlink-related asertions on Windows. This includes 4 tests in `gix-dir/tests/walk/mod.rs`, which were ignored on Windows even though they are able to pass. This commit enables them on Windows. It also updates the associated fixture scripts to no longer say that symlink test can't run on Windows. But no changes to the fixture scripts are required for the tests to pass. (While doing so, I've made a small change, adding quoting to a here document delimiter to make clear that no expansions are intended to occur in the here document text. But this change is purely for clarity; nothing was broken in connection with that heredoc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much actively improving the portability of the test-suite and gitoxide
, it's very much appreciated. And as always, stunning work and description of the work.
I will do my best to keep this technique in mind as well to avoid silent breakage.
For projects as active as gitoxide, I don't usually include hash cross-references to earlier commits in the same PR in commit messages, since they have to be updated when rebasing, which happens often in feature branches on actively developed projects. However, in this case that seemed helpful in a couple of commits so I did use them.
Thanks for the heads-up. Usually I avoid rewrites, but indeed wouldn't ever perform them when it's clear that references would change.
Even though one line is unclear to me, it's nothing that would prevent a merge.
@@ -53,8 +53,8 @@ git init --bare blob.corrupt | |||
echo bnkxmdwz | git hash-object -w --stdin | |||
oid=$(echo bmwsjxzi | git hash-object -w --stdin) | |||
oidf=objects/$(oid_to_path "$oid") | |||
chmod 755 $oidf | |||
echo broken >$oidf | |||
chmod -- 644 "$oidf" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's very interesting as I don't know what the --
does , nor did I know that this is possible here. It's a bit strange that a mode change is needed at all. Looking at it, it's pretty clear that this test was lifted from the Git test-suite, and maybe the chmod
call isn't needed at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's very interesting as I don't know what the
--
does , nor did I know that this is possible here.
Where supported, a --
argument indicates the end of options: arguments that follow --
are never taken to be options, even if they begin with -
. Not all commands support --
. POSIX requires that chmod
support --
, by requiring that it conform to the XBD Utility Syntax Guidelines, in which support for --
is Guideline 10.
When passing arguments that are produced from parameter expansion that I intend always to be taken as non-option arguments, I prefer to pass --
if it is guaranteed to be supported. This often goes along with quoting the expansion, though the effects are independent. It is also often less important than quoting, and it is more error-prone, because for commands like chmod
that have multiple implementations, even though all implementations must support --
, they do not all need to accept it in all the same positions. On macOS, it must precede the first non-option argument. Since 644
is a non-option argument, it must precede that, even though its actual purpose is to convey unambiguously that "$oidf"
is not an option. See (2) in d74e919.
In this particular case, examination of the surrounding context establishes that neither quoting nor --
are needed for safety. So in this case it is only to unambiguously convey intent.
It's a bit strange that a mode change is needed at all.
The chmod
call is needed because git
creates that loose object file read-only (usually with 0444 permissions, though I think this depends on the umask).
I found it strange when I first looked at it too, and I first tried removing it (before making the change in 8720acb). This caused the fixture to fail: the shell fails to open the file for write to set up the >
redirection when the owner does not have write permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation, I understand now why the chmod
is needed, and why chmod -- <mode> <file>
is good and portable practice. --
is something to remember and try whenever an external program is called for added safety. One might think that after getting calls to ssh
wrong twice I'd know, but one can never try to hammer it in a little more 😅.
Due to filesystem differences, Windows doesn't support all the same operations with symlinks, or with executable permissions, as Unix-like operating systems. However, most Windows-specific limitations in the gitoxide test suite related to symlinks or executable permissions are unnecessary. Some such limitations have already been lifted, while others remain. This pull request mostly alleviates that, lifting limitations in most or all cases where they are not needed. This includes improvements to some of the test code added in #1618.
Symlinks
In fixture scripts, MSYS commands such as
ln -s
create symlinks when run on Windows since 0899c2e (#1444) in which this was configured in theMSYS
environment variable.core.symlinks
defaults tofalse
on Windows, but it only causesgit
not to check out symlinks; it does not affect the ability to represent symlinks in an index or trees, nor the ability of Git to detect symlinks in a working tree or to stage them. Special privileges, or a systemwide setting change, are required to create symlinks on Windows, but the necessary ability has been assumed for the gitoxide test suite since before #1444.Some restrictions in the test suite were lifted or weakened in #1444 itself, while others remained.
Because Windows supports symlinks natively and the necessary preconditions are either provided for or considered prerequisites for running the test suite, this PR lifts symlink-related limitations mostly by removing code from tests that treated Windows differently, though a couple of fixture scripts are also changed to no longer claim that symlink fixtures don't work on Windows.
A small number of limitations remain even with this PR, relating to different capabilities for accessing metadata that affect the behavior of the code under test.
(Separately from that, in principle some operations might not be feasible on Windows due to the non-interchangeability of file and directory symlinks, which makes it so symlinks created before their target may not always work correctly. But I did not find any places in the test suite where this caused a problem, where a fixture script was written in such a way that it could be a problem, or where an attempt was made to test anything that this would keep from working.)
Executable permissions
The kind of executable permissions tracked in Git repositories is Unix-style executable permissions. This is not equivalent to the notion of executable permissions representable in ACLs on Windows, and neither Git nor gitoxide attempt to reinterpret executable permissions from repositories in a way that relates to Windows permissions (nor do I advocate that).
That makes it so that most assertions about how executable permissions affect untracked files, or about the effect of changing a tracked file's executable permissions and not staging the change, do have to be skipped, or to assert a different observation, on Windows. Furthermore, a number of fixtures appear to succeed on Windows because commands like
chmod +x
in an MSYS environment such as Git Bash indicate success when run, even though they do not actually make a change to the filesystem.However, this does not restrict the ability of Git or gitoxide to represent executable permissions--specifically, the distinction between modes 100644 and 100755--in repositories. They can be set in an index, and they are representable in trees, on all platforms.
This is already done a fair amount in fixtures that represent tree objects directly rather than building them up with actual files. But this could be inconvenient and potentially much less readable in some cases. Changing many imperative-style fixtures to use this declarative approach, even when equally good or better, would also require significant rewriting and might lead to the introduction of new test bugs.
Fortunately, this can also be achieved by modifying the mode of an already-staged file with
git update-index --chmod
, or by staging a file with a specific mode withgit add --chmod
. Such techniques had only been used in a couple places in the fixture scripts, but are applicable in a number of others. This PR uses them where applicable, and lifts limitations in the tests that use those fixtures, where the limitation related to staged or committed executable permissions.Motivation: A new failing test
In #1618, the
gix-merge::merge tree::run_baseline
test was added. The new code that it tests works on all platforms, as does the test if it is allowed to use pre-generated archives. But the test fails when run on Windows withGIX_TEST_IGNORE_ARCHIVES=1
.This is newer than, and separate from, any of tests known to fail when run that way that are documented in #1358. The failure is due to the associated
tree-baseline.sh
fixture script's reliance onchmod +x
commands, that on Windows silently report success but have no effect, to set permissions on files that will be staged and committed.One possible approach could have been to mark the affected test as ignored on Windows, as had been done in some other tests previously. Instead, I decided to take the opportunity to make the fixtures work on Windows, to allow both this test and some others that had previously been marked ignored to work on Windows.
The new failure can be seen in this gist, but the information there is incomplete, because it only shows the failure of the
same-rename-different-mode
baseline. Once that is fixed, theadded-file-changed-content-and-mode
baseline fails. Both failures occur in this assertion, in different iterations of the containing loop:gitoxide/gix-merge/tests/merge/tree/mod.rs
Lines 83 to 86 in 3fb989b
See 041bdde and 6faf11a, which apply the associated fixes for both sub-failures, for details.
Other impact
I had hoped that some of the various other improvements included here might manage to fix some of the failures shown in #1358, but that did not happen. But a number of tests that would have failed, some of which were even completely not run on Windows, now run (or run more fully) and pass.
I have run the full test suite with and without
GIX_TEST_IGNORE_ARCHIVES=1
on Windows, Arch Linux, Ubuntu 24.04 LTS, and macOS. (The only runs that especially matter are those with it set on Windows and macOS, since CI does not exercise those combinations.) The changes here do not introduce any new failures.When I regenerated and committed archives, I usually did so on the Ubuntu 24.04 LTS system, on which Git 2.47.0 is installed from the git-core PPA. The one exception is
make_rev_spec_parse_repos.sh
, whose associated archive currently needs to be generated using an earlier version of Git, to avoid triggering #1622 on all platforms on all runs withoutGIX_TEST_IGNORE_ARCHIVES=1
. So I regenerated that one on macOS 15, which had a convenient earlier version. See (2) in d74e919 for details.If rebasing...
For projects as active as gitoxide, I don't usually include hash cross-references to earlier commits in the same PR in commit messages, since they have to be updated when rebasing, which happens often in feature branches on actively developed projects. However, in this case that seemed helpful in a couple of commits so I did use them.
Please do still feel free to rebase this, but if possible I recommend those references be updated when doing so. I would be pleased to do any necessary rebasing, with such commit-message rewords, to avoid creating extra work, if rebasing is needed.