ls, df: preserve suffix-only block size units - #14390
Conversation
|
could you please make comment #0 a bit shorter ? nobody likes reading long llm output ;) |
| /// 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`). |
There was a problem hiding this comment.
make this comment shorter
There was a problem hiding this comment.
Cut to 3 lines with the example inline.
| 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. |
There was a problem hiding this comment.
same, this comment is too long
There was a problem hiding this comment.
Cut to 4 lines.
| BlockSizeEnv::NotSet | ||
| } | ||
|
|
||
| /// Extract the display suffix from a block-size spec, if the spec is |
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
There was a problem hiding this comment.
Down to 2 lines plus a rustdoc example.
| 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 |
There was a problem hiding this comment.
please do a gh stack and move ls in the second commit of the stack
There was a problem hiding this comment.
Split: df + the uucore helpers first, ls on top. Each commit builds and tests alone. Happy to split into two PRs if you'd rather have a real stack.
|
GNU testsuite comparison: |
GNU echoes the unit back next to the number when --block-size (or DF_BLOCK_SIZE/BLOCK_SIZE/BLOCKSIZE) 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; df's BlockSize now carries an optional display suffix, which also lets the header keep a full IEC spelling (--block-size=KiB -> "1KiB-blocks").
Same rule as the previous commit, applied to ls: --block-size=K (and LS_BLOCK_SIZE/BLOCK_SIZE/BLOCKSIZE) echoes the unit next to the size, --block-size=1K does not. ls tracks the file-size and allocation suffixes separately, since BLOCKSIZE only affects the allocation column (-s and the total line) and must not leak into the -l size column. -k resets the allocation block size and its unit, leaving a file-size unit from the environment alone, matching GNU.
0d0a1e1 to
b2e2527
Compare
|
Comments trimmed throughout the diff (longest was 14 lines, now 2 + an example), and split into two commits: df + uucore helpers, then ls. Body shortened too. |
Merging this PR will degrade performance by 3.43%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
2 participants
ls -l --block-size=Kanddf --block-size=Kprinted a bare number where GNUechoes the unit back:
1K, not1. A spec with a leading number(
--block-size=1K,--block-size=1024) only sets the divisor and echoesnothing. Same for
LS_BLOCK_SIZE,BLOCK_SIZE,BLOCKSIZE,DF_BLOCK_SIZE.-h/--siare unaffected.This is the gap noted in #14330's description; #13655 fixes the same defect for
du(it touches onlydu.rs, no conflict).Two commits:
df(+ the shareduucorehelpers) first,lson top. Eachbuilds and tests on its own.
Verified byte-for-byte against GNU coreutils 9.11 (
gls/gdf) over 12block-size specs x
ls -l,-s,total, and the env vars. New tests intest_ls.rs,test_df.rs, plus unit tests inuucore.Prepared with AI assistance (Claude Sonnet 5, via Claude Code), per the AI
policy in CONTRIBUTING.md. GNU behavior came from running the binary, not from
reading GPL source.