Test suite does not assert directory symlink creation

[EliahKagan]

Current behavior 😯

On Windows, which has separate file and directory symlinks, gitoxide falls back to creating file symlinks when the target is not found, since 31d02a8 (#1363), to support dangling symlinks (#1354). I wish to create file symlinks in even more situations, to fix #1420 and #1421. But this runs the risk of accidentally converting situations where directory symlinks are currently being created into situations where file symlinks are created, if I make a mistake.

On a feature branch which should never be merged, I tried deliberately breaking symlink creation to only ever create file symlinks on Windows. Enabling CI on that branch reveals that this breaking causes no existing tests to fail. I have also produced this effect locally.

Ordinarily I would just open a PR that adds a test. I intend to open a PR that fixes #1420 and hopefully also #1421, and to include a test that a symlink to a directory is created as a directory symlink on Windows. I plan to include it there so that it validates that my fix there does not break this functionality. I am also opening an issue for this, for two reasons:

1. If, for simplicity, the new test is not wanted in that PR, then it can be removed and the need for it will still not be forgotten.
2. It is not obvious how this test should be done, or how thoroughly it should be done. Rather than have that PR be the only place to discuss it, I'm opening this issue so that possible concerns with the approach I plan to take can be easily raised.

On that second point, the approach I am thinking of taking is actually not to check the created symlink type at all, but instead to write an operating-system agnostic test that will fail on Windows if the wrong type of symlink is created, testing the actual functionality of the symlink.

Some experiments I have done reveal that obtaining metadata with std::fs::metadata will fail to traverse through the wrong type of symlink to access metadata on the target. (This is not for lack of trying. Unlike symlink_metadata, which is lstat-like, the metadata function is stat-like and attempts to obtain metadata of the ultimate target of the symlink.) Therefore, a test can simply attempt to do that, and also assert that the target is a directory; together, that should provide both validation and insight into what goes wrong if there is a regression.

The advantage of that approach is that it tests the desired behavior that symlinks to directories are, when possible, created in a way that actually works, on all systems, including Windows where it happens to be easy to get that wrong.

The disadvantage is that I am not sure the inability to traverse through the wrong type of symlink to obtain metadata is a guaranteed part of the behavior of the metadata function in the Rust standard library.

If this approach is inadequate, then it could either be complimented or supplanted by another test that uses Windows-specific functions to check the actual type.

Expected behavior 🤔

I think at least one test in the test suite should fail if gix clone on Windows always creates symlinks as file symbolic links and never as directory symbolic links even if the target is a directory that already exists.

To be clear, I don't believe gitoxide currently has that or any bug like it. This issue is solely about what the test suite checks for. The expected behavior I'm describing is the behavior of helping to keep me from creating such a bug when changing the code that decides what type of symlink to create on Windows.

Git behavior

I have not researched if, or how, Git's own test suite checks for this.

Whether or not the approach I suggested above and plan to implement soon is used, I don't anticipate any large difficulties extending the test suite to cover this. However, if I encounter problems, I may research that for comparison. I can also look into it if there is interest, separate from any need.

Steps to reproduce 🕹

I recommend just checking the linked experimental branch and test results above.

However, because I intend to eventually delete that branch (after this issue is closed, but the issue will still exist), here's a diff that introduces the breakage that this issue is about the test suite not yet failing on:

diff --git a/gix-fs/src/symlink.rs b/gix-fs/src/symlink.rs
index ce639c48f..a7571c0c6 100644
--- a/gix-fs/src/symlink.rs
+++ b/gix-fs/src/symlink.rs
@@ -33,24 +33,12 @@ pub fn remove(path: &Path) -> io::Result<()> {

 /// Create a new symlink at `link` which points to `original`.
 ///
-/// Note that if a symlink target (the `original`) isn't present on disk, it's assumed to be a
-/// file, creating a dangling file symlink. This is similar to a dangling symlink on Unix,
-/// which doesn't have to care about the target type though.
+/// This temporarily broken implementation is to test the test suite.
+/// In real life, we should sometimes create directory symlinks!
+/// This change should not be merged and should not appear in any releases!!
 #[cfg(windows)]
 pub fn create(original: &Path, link: &Path) -> io::Result<()> {
-    use std::os::windows::fs::{symlink_dir, symlink_file};
-    // TODO: figure out if links to links count as files or whatever they point at
-    let orig_abs = link.parent().expect("dir for link").join(original);
-    let is_dir = match std::fs::metadata(orig_abs) {
-        Ok(m) => m.is_dir(),
-        Err(err) if err.kind() == io::ErrorKind::NotFound => false,
-        Err(err) => return Err(err),
-    };
-    if is_dir {
-        symlink_dir(original, link)
-    } else {
-        symlink_file(original, link)
-    }
+    std::os::windows::fs::symlink_file(original, link)
 }

 /// Return true if `err` indicates that a file collision happened, i.e. a symlink couldn't be created as the `link`

I emphasize that this diff is included for posterity and not as a request that anyone use it.

[Byron]

Thanks a lot for bringing this up!

Indeed I skimped on this as Windows testing is a chore for me, but that's the only reason. Thus, having at least one test for directory handling is definitely appreciated. My stance is generally to keep it simple, but if details can be added easily to improve coverage, then I don't see why they shouldn't be added.

I leave it up to you entirely how to structure PRs about this, a single PR might be easiest to handle though.

[EliahKagan]

As noted elsewhere (but I think mentioning it here may make things clearer), I ended up including a test as called for here -- though the repository it uses is very simple, consisting of a single symlink whose target is . -- in #1425. So I would be inclined to regard that as fixing this as well.