Always fall back to creating file symlinks on Windows #1425
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
These are effectively special cases of dangling symlinks. They should be treated the same as ordinary dangling symlinks, but the error kind isn't NotFound for these, so currently they are not created. The new tests should pass once that is fixed. See GitoxideLabs#1420.
When the metadata of a symlink's target cannot be obtained, even if the error is something other than `NotFound`, this falls back to creating file symbolic links. This only affects scenarios where either the checkout would fail entirely or where the symlink would have been treated as a collision and skipped (even though it was not really a collision, since only its target had an error). Other cases are not affected, and all exisitng scenarios where directory symlink would be created will still create directory symlinks. This builds on 31d02a8 (GitoxideLabs#1363) by supporting dangling symlinks even when the target filenames are unusual, such as when the name is invalid or reserved. Windows permits such symlinks to be created, and going ahead and creating the matches the Git behavior. This should also support other errors beisdes `NotFound`. For example, some permissions-related errors, in some cases where traversal or acccess (even to access metadata) are not allowed, would fail to create a symlink. This should address that as well. This works by using `Path::is_dir()` in the standard library, which automatically converts all errors (not just `NotFound`) into `false`. The logic here is thus quite similar to what was already present, just more tolerant, even though the code itself is shorter and simpler. This fixes GitoxideLabs#1420, and also fixes GitoxideLabs#1421.
At least as `std::fs::metdata` currently works on Windows, this entails that symlinks to directories on Windows are created as directory symlinks rather than as file symlinks (since file symlinks cannot always be automatically dereferenced, and accessing metadata via the `stat`-like `std::fs::metadata` function is one situation where that cannot be done). This fixes GitoxideLabs#1422. Note that this issue pertains solely to what the test suite covers; the issue is not a bug in the code under test, and this commit does not modify the code under test.
Generated on an Ubuntu 22.04 LTS system by running all tests with `cargo nextest run --all --no-fail-fast`, verifying that all tests pass, and committing the newly created archive files, which correspond to the new invalid and reserved target symlink tests and the directory symlink test.
This was referenced Jun 26, 2024
Byron
requested changes
Jun 26, 2024
There was a problem hiding this comment.
Thanks a lot for the fix and all the research that went into it.
Your diligence (among other traits) I look up to and attempt to apply similar standards in my own work. My primary school teacher always said one should reach for the impossible to reach the possible :D.
With that said, I have a small request regarding deduplication of tests, but with that done I am looking forward to merging.
This helps to clarify, rather than obscure, their similarities, including the similarity between the old test case and the new ones that cover dangling symlinks whose targets are unusually named. For now, the associated fixtures remain separate.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Jun 30, 2024
These are the changed archives generated by fixture scripts by running `cargo nextest run --all --no-fail-fast` on an Ubuntu 22.04 system. (All tests passed, as expected.) The changed archives divide into two cases. First, there are those that are changed due to the stylistic changes to heredocs in the preceding commit 2641f8b (which change the CRC32 hashes of the scripts and thus cause archives to be regenerated): * gix-index/tests/fixtures/generated-archives/v2_icase_name_clashes.tar * gix-worktree-state/tests/fixtures/generated-archives/make_dangling_symlink.tar * gix/tests/fixtures/generated-archives/make_rev_spec_parse_repos.tar Second, there are those that were already being generated when the tests were run, rather than using the committed archives: * gix-diff/tests/fixtures/generated-archives/make_diff_repo.tar * gix-odb/tests/fixtures/generated-archives/make_repo_multi_index.tar These were being generated every time that `nextest` command was run on Unix-like systems (or at least GNU/Linux and macOS). I don't know why this was happening, but it suggests a bug somewhere. - That oddity precedes 55c635a (GitoxideLabs#1425), I just didn't commit the two archives not related to the changes being made there at that time, since unlike here, that was not a cleanup PR. - It precedes, or mostly precedes, the change in dcab79a (GitoxideLabs#1415). At least the first of those archives, make_diff_repo.tar, already behaved this way in its .tar.xz form before that, as noted in: GitoxideLabs#1361 (comment)
LuaKT
pushed a commit
to LMonitor/gitoxide
that referenced
this pull request
Aug 20, 2024
These are the changed archives generated by fixture scripts by running `cargo nextest run --all --no-fail-fast` on an Ubuntu 22.04 system. (All tests passed, as expected.) The changed archives divide into two cases. First, there are those that are changed due to the stylistic changes to heredocs in the preceding commit 2641f8b (which change the CRC32 hashes of the scripts and thus cause archives to be regenerated): * gix-index/tests/fixtures/generated-archives/v2_icase_name_clashes.tar * gix-worktree-state/tests/fixtures/generated-archives/make_dangling_symlink.tar * gix/tests/fixtures/generated-archives/make_rev_spec_parse_repos.tar Second, there are those that were already being generated when the tests were run, rather than using the committed archives: * gix-diff/tests/fixtures/generated-archives/make_diff_repo.tar * gix-odb/tests/fixtures/generated-archives/make_repo_multi_index.tar These were being generated every time that `nextest` command was run on Unix-like systems (or at least GNU/Linux and macOS). I don't know why this was happening, but it suggests a bug somewhere. - That oddity precedes 55c635a (GitoxideLabs#1425), I just didn't commit the two archives not related to the changes being made there at that time, since unlike here, that was not a cleanup PR. - It precedes, or mostly precedes, the change in dcab79a (GitoxideLabs#1415). At least the first of those archives, make_diff_repo.tar, already behaved this way in its .tar.xz form before that, as noted in: GitoxideLabs#1361 (comment)
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.
Fixes #1420
Fixes #1421
Fixes #1422
Changes
This adds tests that symlinks can specify targets with unusual names that, on Windows, are invalid or reserved (#1420), which I verified locally to fail until changes are made to the code under test; as well as a regression test that symlinks to directories on Windows are created as directory symlinks (#1422; see verification that the test is able to fail).
This also modifies
gix-fs/src/symlink.rs, using the technique proposed in #1420 to convert all errors accessing metadata to guess the proper symlink type, and not justNotFounderrors, intofalse. This falls back to creating file symlinks in more situations, solving #1420 and #1421.This does not contain new tests specifically for #1421. Since the same technique fixed both, I am unsure if that is needed at this time. There may also be various other error conditions that would have prevented checkout or been misinterpreted as collisions that this now addresses by falling back to creating file symlinks, as Git does.
Comparison to Git
There is one subtlety, though not a new one, in comparison to the Git behavior. On Windows, Git repeatedly rechecks if symbolic links that it created as file symlinks should be replaced with directory symlinks. This allows Git to check out the best type of symlink in more situations where ordering would otherwise matter, though it still will check out a file symlink when the target is an existing but dangling directory symlink (thus it is still not intuitive), and I think it may have performance implications. Even if it does not cause performance problems, I think it may need to be implemented very carefully if done for gitoxide, since the facilities in the Rust standard library access a large amount of metadata with multiple Windows API calls plus further fallback calls. (The algorithm Git for Windows uses for this is worst-case quadratic in the number of files involved. If only files from the index were involved, then it would be possible to implement a linear-work alternative by using topological sort. But the files involved need not all be from the index.)
However, that difference between Git and gitoxide in the treatment of symlinks was already present. This leaves that difference unchanged, and merely converts various situations that were previously errors into successful creation of file symlinks.
Manual testing
I have repeated the manual testing described in #1420 and #1421 after installing the version from this feature branch with
cargo install --path ., further verifying the fixes.With the repository that has a symlink to
???:With the repository that has a symlink to
CON:With the repository that has a symlink to a file that, on my system, cannot even have its metadata accessed due to giving a permission error:
In addition, I have further tested that the changes do not break the creation of directory symlinks where that was already working, by cloning this test repository. Unlike the above results, this is the same as before the changes (because it was already working before the changes):
Other considerations
I have checked on another branch, as well as locally, that tests pass even without generated archives.
I have proceeded conservatively with respect to the shell script heredoc style issues raised in #1423, leaving the seemingly unnecessary use in place in
<<-even where in existing tests that are directly relevant to the changes here, while using<<in the new tests. I'd be pleased to change this in either direction; but that could probably be done separately and later.