ls, df: preserve suffix-only block size units - #14390
Open
Socialpranker wants to merge 2 commits into
Open
Conversation
GNU echoes the unit back next to the number when --block-size (or LS_BLOCK_SIZE/BLOCK_SIZE/BLOCKSIZE/DF_BLOCK_SIZE) is given as a bare unit (--block-size=K -> "1K"), but not when a numeric multiplier is given (--block-size=1K or --block-size=1024 -> "1"). uutils printed a bare number in both cases. Adds suffix_from_parsed_block_size and block_size_from_env_with_suffix to uucore's parse_block_size, shared by ls and df. df's BlockSize now carries an optional display suffix. ls tracks the file-size and allocation suffixes separately, since BLOCKSIZE only affects the allocation column (-s) and must not leak into the -l size column.
Contributor
|
could you please make comment #0 a bit shorter ? nobody likes reading long llm output ;) |
sylvestre
reviewed
Sep 5, 2026
Comment on lines
+127
to
+132
| /// The default variant is `Bytes(1024, None)`. | ||
| /// | ||
| /// The second field is the GNU-style display suffix (e.g. `"K"`) to echo | ||
| /// back next to a scaled number, set only when the block size was given as | ||
| /// a suffix-only spec (`--block-size=K`) rather than a numeric one | ||
| /// (`--block-size=1K`, `--block-size=1024`). |
Contributor
There was a problem hiding this comment.
make this comment shorter
sylvestre
reviewed
Sep 5, 2026
| fn resolve_block_sizes_from_env(opt_kb: bool) -> (u64, u64) { | ||
| match parse_block_size::block_size_from_env(&["LS_BLOCK_SIZE", "BLOCK_SIZE"]) { | ||
| parse_block_size::BlockSizeEnv::Found(size) => { | ||
| /// `BLOCKSIZE` only affects the allocation display (`-s`), not file sizes. |
Contributor
There was a problem hiding this comment.
same, this comment is too long
sylvestre
reviewed
Sep 5, 2026
| BlockSizeEnv::NotSet | ||
| } | ||
|
|
||
| /// Extract the display suffix from a block-size spec, if the spec is |
Contributor
There was a problem hiding this comment.
i don't think we need 14 lines of comments
esp when they don't follow rustdoc format or provide an example
sylvestre
reviewed
Sep 5, 2026
| const DEFAULT_FILE_SIZE_BLOCK_SIZE: u64 = 1; | ||
|
|
||
| /// Resolve `(file_size_block_size, block_size)` from environment variables. | ||
| /// The block sizes `ls` displays with, plus the display suffixes that go |
Contributor
There was a problem hiding this comment.
please do a gh stack and move ls in the second commit of the stack
|
GNU testsuite comparison: |
This file contains hidden or 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
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.
ls -l --block-size=Kanddf --block-size=Kprinted a bare number whereGNU echoes the unit back next to it:
GNU's rule: a suffix-only spec (no leading digits —
K,k,KB,KiB,M, ...) echoes the unit next to the scaled number. A spec with aleading number (
--block-size=1K,--block-size=1024,--block-size=2K)does not — it only sets the divisor. The same distinction applies to
LS_BLOCK_SIZE,BLOCK_SIZE,BLOCKSIZE, andDF_BLOCK_SIZE.This was already flagged as a known gap in #14330's description ("The 27
cases that still differ are all suffix-only block sizes... a separate
defect in a different place — the same one #13655 is fixing for
du") andis what #13655 fixes for
du. This PR fixes the same defect forlsanddf.The fix
uucore::parser::parse_block_sizegainssuffix_from_parsed_block_size(extracts the GNU-canonicalized unit from a suffix-only spec,
Noneotherwise) and
block_size_from_env_with_suffix(likeblock_size_from_env, but also returns the suffix from whichever variableresolved the size). Both are shared by
lsanddf;dualready carriesits own copy of this logic in #13655, which touches only
du.rsandtest_du.rsand does not conflict with this change.df'sBlockSize::Bytesgains a second field for the optional suffix,threaded through
read_block_sizeand echoed intable.rswherever ascaled, non-human-readable value is printed.
lsneeded a bit more care:BLOCKSIZEaffects only the allocationcolumn (
-s, and thetotalline), not the-lfile-size column, so asingle suffix field would leak the unit onto the wrong column when
BLOCKSIZEalone was set.Confignow carries the file-size andallocation suffixes separately (
file_size_block_size_suffix,block_size_suffix), matching the existing split betweenfile_size_block_sizeandblock_size.-kclears the allocationsuffix along with the allocation block size, but leaves a file-size
suffix from the environment alone, matching GNU.
Neither
-h/--human-readablenor--siare affected — they do theirown dynamic scaling and print their own units.
How the GNU behavior was established
By running the installed GNU coreutils 9.11 binary (via Homebrew,
gls/gdf) as a black box and diffing its output against uutils — the rulesabove are read off those transcripts. I did not read GNU coreutils
source.
Testing
tests/by-util/test_ls.rs(test_ls_suffix_only_block_size,..._allocation,..._env_block_size,test_ls_suffix_only_blocksize_env_does_not_reach_file_size,..._with_kibibyte_flag,..._not_used_with_human_readable) andtests/by-util/test_df.rs(test_df_suffix_only_block_size,..._env_block_size,..._not_used_with_human_readable,..._ignored_in_posix_portability_mode), plus unit tests for the newuucorehelpers.cargo test --no-default-features --features "feat_os_unix ls" --test tests -- block_size:46 passed (
ls,df,du,dd,truncate,sortblock-size suites,none regressed) + 6 more under a
test_ls_suffix_onlyfilter (namesubstring mismatch in the first run, confirmed separately): all green.
cargo test --no-default-features --features df --test tests -- block_size:20 passed.
cargo fmt --all --check: clean.cargo clippy -p uu_ls -p uu_df -p uucore --all-targets -- -D warnings:clean (run per-crate;
--all-targetsacross all three at once hits anunrelated, pre-existing
uucore::entriestest compile error fromfeature unification on this workspace/toolchain — reproduces identically
on unpatched
main).cargo check --target x86_64-pc-windows-gnu: clean.gls/gdf), 12block-size specs (
K k KB KiB M m MB G 1K 1024 2K 1MB) xls -l, plus-s, env vars (LS_BLOCK_SIZE,BLOCK_SIZE,BLOCKSIZE,DF_BLOCK_SIZE), and the file/allocation-column split: byte-identicalto GNU in every case. One pre-existing, unrelated mismatch found in the
process (
ls -s --block-size=Munder-rounds a partial block) is thedefect ls: round block counts up when scaling to a block size #14330 already fixes; this PR does not touch
ls.rsor thatcode path.
Disclosure
Prepared with AI assistance (Claude Sonnet 5, via Claude Code), per the AI
policy in CONTRIBUTING.md. Every GNU behavior quoted above came from
running the installed binary, not from reading GPL source. All testing
was run locally.