Skip to content

uucore: unify hard link detection across platforms using FileInformation - #14784

Open
wtcpython wants to merge 1 commit into
uutils:mainfrom
wtcpython:fs-unify-hardlink
Open

wtcpython wants to merge 1 commit into
uutils:mainfrom
wtcpython:fs-unify-hardlink

Conversation

@wtcpython

Copy link
Copy Markdown
Contributor

Hard-link detection currently has separate Unix logic and always returns false on other platforms.

This reuses FileInformation for both hard-link checks, preserving the existing Unix behavior and enabling the same checks on Windows and WASI.

Copilot AI lite review requested due to automatic review settings September 21, 2026 13:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

github-actions Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/date/resolution (fails in this run but passes in the 'main' branch)

Copilot AI review requested due to automatic review settings September 21, 2026 18:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

///
/// * `bool` - Returns `true` if the paths are hard links to the same file, and `false` otherwise.
#[cfg(unix)]
pub fn are_hardlinks_to_same_file(source: &Path, target: &Path) -> bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the unit tests below are still #[cfg(unix)], please enable them on windows too since the function now works there

///
/// * `bool` - Returns `true` if either of above conditions are true, and `false` otherwise.
#[cfg(unix)]
pub fn are_hardlinks_or_one_way_symlink_to_same_file(source: &Path, target: &Path) -> bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on windows from_path actually opens the file, so a file we can't open now silently compares as "not the same" - is that ok for cp/mv?

@@ -790,23 +784,17 @@ pub fn are_hardlinks_to_same_file(_source: &Path, _target: &Path) -> bool {
/// # Returns

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is red (tests unix), could you please check it's not coming from this change? thanks

Copilot AI review requested due to automatic review settings September 21, 2026 23:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings September 22, 2026 03:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI lite review requested due to automatic review settings October 9, 2026 00:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@codspeed

codspeed Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Merging this PR will improve performance by 15.97%

⚠️ 122 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

ℹ️ 1 benchmark has no flame graph

The flamegraph could not be generated because too much time was spent in runtime internals, making the visualization not useful for performance analysis.

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 3 improved benchmarks
✅ 398 untouched benchmarks
⏩ 70 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
⚡ Simulation factor_three_39_bit_primes 544.3 ms 394.7 ms +37.89%
⚡ Simulation factor_five_38_bit_primes 1.9 s 1.8 s +8.84%
⚡ Simulation factor_thirteen_39_bit_primes 9 s 8.6 s +3.91%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.


Comparing wtcpython:fs-unify-hardlink (7506795) with main (fcfb6d8)2

Open in CodSpeed

Footnotes

  1. 70 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 (853c36d) during the generation of this report, so fcfb6d8 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

Copilot AI lite review requested due to automatic review settings October 10, 2026 15:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Comment thread src/uucore/src/lib/features/fs.rs Outdated
}

#[cfg(unix)]
#[cfg(not(target_os = "wasi"))]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gate was widened to include Windows, but it uses Unix-only imports that aren't in scope here. Its body calls tempdir() and unix::fs::symlink(...), but both use std::os::unix; and use tempfile::tempdir; in the test module remain gated behind #[cfg(unix)] only (unchanged by this PR).

Reproduced with cargo check -p uucore --tests --target x86_64-pc-windows-gnu --no-default-features --features fs:

error[E0433]: cannot find module or crate unix in this scope
--> src/uucore/src/lib/features/fs.rs:1400:9
error[E0425]: cannot find function tempdir in this scope
--> src/uucore/src/lib/features/fs.rs:1396:24

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created this #15301 to ensure uucore is checked on Windows.

Copilot AI lite review requested due to automatic review settings October 14, 2026 09:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI lite review requested due to automatic review settings October 14, 2026 09:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants