uucore: Implement are_hardlinks_to_same_file on Windows

[lhecker]

Primitively implements hardlink detection using the
existing infos_refer_to_same_file facilities.

This fixes cp --remove-destination and mv --backup.

[Copilot]

🟡 Changes recommended

The Windows implementations currently evaluate both metadata lookups unconditionally, losing the Unix branch’s intentional early-return behavior and causing avoidable I/O in common missing-target scenarios.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Implements Windows support for are_hardlinks_to_same_file (and the related “hardlink-or-one-way-symlink” helper) by leveraging existing FileInformation / infos_refer_to_same_file identity comparisons in uucore’s filesystem feature module.

Changes:

• Add Windows implementations for are_hardlinks_to_same_file and are_hardlinks_or_one_way_symlink_to_same_file using FileInformation::from_path(...).
• Adjust unit tests to exercise these helpers beyond Unix-only, including a new symlink behavior test.
• Minor test cleanup/consistency changes (imports and NamedTempFile usage).

File summaries

File	Description
src/uucore/src/lib/features/fs.rs	Adds Windows hardlink/symlink identity checks via FileInformation, and expands tests to cover the new behavior.

Review details

Suppressed comments (1)

src/uucore/src/lib/features/fs.rs:884

• Same short-circuit issue as above: on Windows both FileInformation::from_path(...) calls are evaluated unconditionally, so you still open the source even when the target lookup fails. The Unix branch avoids that extra work; mirroring it here keeps behavior consistent and reduces unnecessary syscalls.

        infos_refer_to_same_file(
            FileInformation::from_path(source, true),
            FileInformation::from_path(target, false),
        )
    }

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The PR should include/enable Windows end-to-end regression coverage for the claimed cp --remove-destination / mv --backup fixes, and there are also unit-test cleanup issues to address.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/uucore/src/lib/features/fs.rs:1397

• test_are_hardlinks_to_same_file_hard_link creates an additional hardlink path (path2) next to the NamedTempFile, but NamedTempFile only cleans up its own original path. The extra hardlink is left behind in the OS temp directory after the test, which can accumulate across runs. Prefer creating the temp file inside a tempdir() and placing the hardlink inside that directory so everything is cleaned up when the directory is dropped.
  src/uucore/src/lib/features/fs.rs:1401
• The test name test_are_hardlinks_to_same_file_symlink is misleading: it primarily verifies that a symlink is not treated as a hardlink by are_hardlinks_to_same_file, and that are_hardlinks_or_one_way_symlink_to_same_file returns true when the source is a symlink to the target. Renaming the test to reflect the one-way symlink semantics will make failures easier to interpret.

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The updated uucore hardlink/symlink identity unit tests now run on unsupported targets (e.g. non-unix/windows) and will fail without restoring appropriate platform gating.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

src/uucore/src/lib/features/fs.rs:1387

• This test uses fs::hard_link(...).unwrap() but is no longer #[cfg(unix)]; on targets where hard links are unsupported/restricted (e.g. WASI), it will panic or fail. Gate it to unix/windows (and exclude Android if necessary).

    #[test]
    fn test_are_hardlinks_to_same_file_hard_link() {

src/uucore/src/lib/features/fs.rs:1372

• This test currently runs on non-unix/windows targets as well, where are_hardlinks_to_same_file always returns false, causing a guaranteed failure. Gate the test to unix/windows (and exclude Android if hard links are not supported there).

    #[test]
    fn test_are_hardlinks_to_same_file_different_files() {

src/uucore/src/lib/features/fs.rs:1400

• This symlink-based test currently runs on targets where neither the unix nor windows symlink creation branch is compiled, leaving path2 uncreated and making the subsequent assertions fail. Gate it to unix/windows (and exclude Android if hard links are not supported there).

    #[test]
    fn test_are_hardlinks_to_same_file_symlink() {

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[github-actions]

GNU testsuite comparison:

Skipping an intermittent issue tests/cut/bounded-memory (passes in this run but fails in the 'main' branch)
Skip an intermittent issue tests/tail/tail-n0f (was skipped on 'main', now failing)

[Copilot]

🔵 Needs a closer look

The PR claims to fix mv --backup on Windows but doesn’t add/enable a Windows mv integration/regression test to verify the behavior end-to-end.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/uucore/src/lib/features/fs.rs:1404

• This test name suggests are_hardlinks_to_same_file should succeed, but the test is primarily about the symlink-resolving behavior of are_hardlinks_or_one_way_symlink_to_same_file. Renaming it would make failures easier to interpret.

src/uucore/src/lib/features/fs.rs:841

• The new Windows implementation is intended to fix mv --backup, but there isn't a Windows integration/regression test exercising the mv backup guard behavior (the existing hardlink/backup tests in tests/by-util/test_mv.rs are still #[cfg(all(unix, ...))]). Adding/enabling a Windows mv --backup test would help ensure this doesn't regress again.

    #[cfg(windows)]
    {
        let Ok(target_metadata) = FileInformation::from_path(target, false) else {
            return false;
        };

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[codspeed-hq]

Merging this PR will not alter performance

✅ 367 untouched benchmarks
⏩ 50 skipped benchmarks¹

---

_{Comparing microsoft:windows/hardlink-identity (a7ae9ea) with main (f604ab9)²}

Footnotes

1. 50 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

2. No successful run was found on main (51ed5dc) during the generation of this report, so f604ab9 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

[Copilot]

🟢 Approval recommended

The functional changes are small and covered by added/expanded tests; the only remaining issue is a misleading test comment.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Address the Windows deletion scenario, WASI test guards, and stale test documentation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

src/uucore/src/lib/features/fs.rs:888

• The newly added public documentation uses the ungrammatical phrase “if either two paths”; please reword it so the two alternatives are unambiguous.

/// Checks if either two paths are hard links to the same file or if the source path is a symbolic link which when fully resolved points to target path

tests/by-util/test_cp.rs:1197

• The adjacent comment still says this test does not work on Windows, but removing this cfg now schedules it on Windows. Please remove or update that stale claim so a Windows failure is not treated as expected.

fn test_cp_arg_remove_destination() {

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved moderate and critical issues affect Windows behavior and test portability.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

src/uucore/src/lib/features/fs.rs:852

• On Windows this calls FileInformation::from_path, which opens each entry with read(true). That requires read access and can fail for an entry that is still metadata-queryable/deletable (for example, an ACL-restricted source or destination), so the helper returns false and mv can miss the same-hardlink check. The existing Windows same_file implementation deliberately opens with access_mode(0) (src/uu/test/src/platform/windows.rs:347-358); use the same no-data-access metadata path here (or expose it through FileInformation) so identity checks do not depend on read permission.

    let Ok(target_metadata) = FileInformation::from_path(target, false) else {
        return false;
    };
    let Ok(source_metadata) = FileInformation::from_path(source, false) else {

src/uucore/src/lib/features/fs.rs:846

• The PR description says this fixes mv --backup, but this helper is skipped whenever opts.backup != BackupMode::None (mv.rs:530-533); the backup-specific guard instead calls backup_would_destroy_source (mv.rs:378-385), which already compares FileInformation directly. As written, adding the Windows hardlink helper does not change the mv --backup path, so either that path needs to be updated or the description should not claim this fix.

pub fn are_hardlinks_to_same_file(source: &Path, target: &Path) -> bool {

tests/by-util/test_cp.rs:1194

• Enabling this test on Windows makes it fail for the existing reason this guard was present: the destination is made read-only, but --remove-destination does not set options.force(), so delete_path leaves the Windows read-only attribute set and fs::remove_file returns access denied. Either make the deletion path clear the attribute for RemoveDestination, or keep this test Windows-gated until that behavior is implemented.

/// from --force. This test originally checked file timestamps, which

tests/by-util/test_mv.rs:554

• not(target_os = "android") is broader than the previous Unix guard: it also compiles this hard-link regression test for WASI and other non-Unix/non-Windows targets, where are_hardlinks_to_same_file is the false stub at fs.rs:859-862. The test then expects mv to reject a hard-link pair and fails; restrict the test to Unix (except Android) or Windows.

#[cfg(not(target_os = "android"))]

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite