Skip to content

Account for (uplift) hardlinks when calculating clean file size - #17485

Open
ranger-ross wants to merge 2 commits into
rust-lang:masterfrom
ranger-ross:better-clean
Open

ranger-ross wants to merge 2 commits into
rust-lang:masterfrom
ranger-ross:better-clean

Conversation

@ranger-ross

Copy link
Copy Markdown
Member

What does this PR try to resolve?

Reported in #15823.

Currently the size we show in cargo clean does not account for hardlinks.
This can lead to large over estimations when there are large files hardlinked during uplifting (like binaries)

I personally have seen Cargo report 2Tb saved when my disk was 1Tb 😆

I briefly considered if we should show both the apparent size and the size on disk, but decided to just stick to showing the size on disk to avoid confusion and feel its want the majority of users will want/expect.

Note: This does not mean the size estimation is now perfectly accurate.
Still missing

  • Hardlinks out out of the target
  • As I understand it, Metadata::len() does not account for account for filesystem specific allocations. (I think MetadataExt::blocks() could handle this better on unix. I didn't do this since testing it would be a bit annoying due to it being filesystem specific, though could be added later)

How to test and review this PR?

You can test this pretty easily testable using du -sh target and comparing that with cargo clean --dry-run.

Testing on ripgrep

❯ du -sh target
382M	target

# cargo 1.98.0
❯ cargo clean --dry-run
     Summary 1407 files, 476.5MiB total
warning: no files deleted due to --dry-run

# This PR
❯ cargo clean --dry-run
     Summary 1407 files, 378.3MiB total
warning: no files deleted due to --dry-run

(note: we under report now due to reasons above, but its much closer to the true size than before)

AI usage disclosure
I used AI during the investigation of this, but all of the code is handwritten.
I also used it to help simplify the test case and make it more idiomatic/match the style of other tests

@rustbot rustbot added Command-clean S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 17, 2026
@rustbot

rustbot commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @Muscraft, @epage, @weihanglo
  • @Muscraft, @epage, @weihanglo expanded to Muscraft, epage, weihanglo
  • Random selection from Muscraft, epage, weihanglo

@ranger-ross

ranger-ross commented Sep 17, 2026

Copy link
Copy Markdown
Member Author

fwiw, I tried meta.blocks() * 512 instead of meta.len() on the ripgrep target-dir from the PR desc

❯ cargo clean --dry-run
     Summary 1407 files, 381.2MiB total

So it seems to be even closer to du's measurement, but I don't know enough about file systems to know if this is a flawed approach.
And we'd still need to need to use meta.len() for Windows

@ranger-ross

ranger-ross commented Sep 17, 2026

Copy link
Copy Markdown
Member Author

Errr, geez didn't realized the windows MetadataExt was nightly only 😓
cc: rust-lang/rust#63010

I guess the PR is still reviewable as the approach should work, I'll just need to use windows-sys directly.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 17, 2026
@epage

epage commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

imo this PR addresses one specific concern and I think that is fully reasonable. We shouldn't block some improvement on trying to make it perfect, especially since each time of improvement likely needs its own decision of whether it is worth it / correct.

Comment thread tests/testsuite/clean.rs Outdated
Comment thread src/ops/cargo_clean.rs
Comment thread src/ops/cargo_clean.rs

@epage epage 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.

Feel free to merge when the comment is added

View changes since this review

@Muscraft

Copy link
Copy Markdown
Member

I think the build-std job failure is related to an issue with the latest nightly.

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

Labels

Command-clean S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants