Add block size display to ls (`ls -l`) by skyronic · Pull Request #2488 · uutils/coreutils

skyronic · 2021-07-09T17:25:22Z

This adds block size display to ls on Unix systems, and displays size on Windows to keep it consistent with the existing behaviour. This works on both long and grid-displays. This still does not implement the -k flag for #2257 but we can add it going forward.

I've tested this on macOS, Windows and FreeBSD.

$ cargo run ls -ls

total 152
 16 -rw-r--r--  1 anirudh staff  5221 Jul  8 21:31 CODE_OF_CONDUCT.md
  8 -rw-r--r--  1 anirudh staff  2513 Jul  8 21:31 CONTRIBUTING.md
120 -rw-r--r--  1 anirudh staff 58234 Jul  8 21:31 Cargo.lock
 32 -rw-r--r--  1 anirudh staff 15208 Jul  8 21:31 Cargo.toml
  8 -rw-r--r--  1 anirudh staff  2296 Jul  8 21:31 DEVELOPER_INSTRUCTIONS.md
 16 -rw-r--r--  1 anirudh staff  7108 Jul  8 22:45 GNUmakefile
  8 -rw-r--r--  1 anirudh staff  1053 Jul  8 21:31 LICENSE
  8 -rw-r--r--  1 anirudh staff    55 Jul  8 21:31 Makefile
 24 -rw-r--r--  1 anirudh staff 10342 Jul  8 21:31 Makefile.toml
 40 -rw-r--r--  1 anirudh staff 18288 Jul  8 21:31 README.md
 16 -rw-r--r--  1 anirudh staff  7283 Jul  8 21:31 build.rs
  8 -rw-r--r--  1 anirudh staff    16 Jul  8 21:31 clippy.toml
  0 drwxr-xr-x 11 anirudh staff   352 Jul  8 21:31 docs
  0 drwxr-xr-x  4 anirudh staff   128 Jul  8 21:31 examples
  0 drwxr-xr-x  6 anirudh staff   192 Jul  8 21:31 src
  0 drwxr-xr-x  6 anirudh staff   192 Jul  8 21:32 target
  0 drwxr-xr-x  7 anirudh staff   224 Jul  8 21:31 tests
  0 drwxr-xr-x 15 anirudh staff   480 Jul  8 21:31 util

$ cargo run ls -s
 16 CODE_OF_CONDUCT.md    8 CONTRIBUTING.md  120 Cargo.lock   32 Cargo.toml    8 DEVELOPER_INSTRUCTIONS.md   16 GNUmakefile    8 LICENSE    8 Makefile   24 Makefile.toml   40 README.md   16 build.rs    8 clippy.toml    0 docs    0 examples    0 src    0 target    0 tests    0 util

I'm aware there's already a PR #2435 but that has since been closed, so I thought I'd open this instead.

tertsdiepraam

Looks good! Some abstractions seem to need a little bit of a rework in general with then new features that have been added (including this one), but rewriting that should not be part of this PR. For this PR, I just have 2 nits.

The terminal of the Windows CI has a default width, which is why those tests fail. You could add -w 0 to give everything its own line and match the expected output.

tertsdiepraam · 2021-07-10T10:37:10Z

src/uu/ls/src/ls.rs

+// - blocks
+fn get_max_sizes (items: &[PathData], config: &Config) -> (usize, usize, usize, u64) {
+    let (mut max_links, mut max_width, mut max_blocks) = (1, 1, 1);
+        let mut total_size = 0;


Please fix this indentation

tertsdiepraam · 2021-07-10T10:44:53Z

src/uu/ls/src/ls.rs

+    if config.format != Format::Long && config.blocks {
+        if let Some(md) = path.md() {
+            width += max_blocks + 1;
+            name.insert_str(0, &format!("{} ", pad_left(get_display_size(md).to_string(), max_blocks)));


Maybe we can make pad_left generic over types implementing Display? I think it also makes sense to put the width += max_blocks + 1; outside of the if-else.

skyronic · 2021-07-13T00:38:45Z

Thanks @tertsdiepraam! I'm yet to update the tests but have the other items worked out.

skyronic · 2021-07-15T14:50:26Z

Hey @tertsdiepraam do let me know if there's anything else to change here, thanks!

tertsdiepraam · 2021-07-15T17:52:43Z

I wanted to merge this, but decided to quickly run this locally and got values that are off by a factor of 2. The total is also missing from the short output.

❯ ls -s
total 192
 4 benches     60 Cargo.lock    8 CODE_OF_CONDUCT.md          4 docs          4 LICENSE        20 README.md   4 tests
12 boink.path  16 Cargo.toml    4 CONTRIBUTING.md             4 examples      4 Makefile        4 src         4 util
 8 build.rs     4 clippy.toml   4 DEVELOPER_INSTRUCTIONS.md   8 GNUmakefile  12 Makefile.toml   4 target

❯ uu_ls -s
 16 CODE_OF_CONDUCT.md   32 Cargo.toml                   8 LICENSE         40 README.md    16 build.rs       8 examples    8 tests
  8 CONTRIBUTING.md       8 DEVELOPER_INSTRUCTIONS.md    8 Makefile         8 benches       8 clippy.toml    8 src         8 util
120 Cargo.lock           16 GNUmakefile                 24 Makefile.toml   24 boink.path    8 docs           8 target

From the ls manual:

Normally the disk allocation is printed in units of 1024 bytes, but this can be overridden (see Block size).

From the Rust docs:

Returns the number of blocks allocated to the file, in 512-byte units.

Looks good otherwise and I can merge this if you can fix this :)

skyronic · 2021-07-16T19:16:17Z

Sounds good, @tertsdiepraam

I tested this on macOS but I'll test it on other platforms. I'm probably going to spend a bit more time and honor the BLOCKSIZE env variable as well if possible!

skyronic · 2021-07-23T15:08:35Z

Hey, unfortunately due to some personal issues, I am unable to continue work on this. I apologize... If anyone wants to take over and fix the issues, or write something else from scratch, please go ahead! Thank you for your time @tertsdiepraam

tertsdiepraam · 2021-07-23T16:10:22Z

I hope you are doing well. I think this PR is too good to throw away, so I'll continue this when I have the time (in about a week). In the meantime, someone else could pick this up as well. Thanks for your work on this PR!

skyronic · 2021-07-23T18:34:04Z

Thank you, @tertsdiepraam! Things just got a bit busy here, but hope to contribute more a few weeks down the line.

Feel free to make any changes and even rebase the commits as you see fit! Have a good weekend.

tertsdiepraam · 2021-07-23T22:12:24Z

Alright! Good luck and have a good weekend!

skyronic added 2 commits July 9, 2021 13:21

Implement "ls -s" and "ls -ls"

0f1c71c

Add support for windows and tests

b0c5006

sylvestre requested a review from tertsdiepraam July 10, 2021 08:38

tertsdiepraam approved these changes Jul 10, 2021

View reviewed changes

Formatting and test fixes

b0936cc

skyronic added 2 commits July 12, 2021 20:45

Force -w=0 for ls -s unit test

080cae1

Style and clippy fixes

e14e885

skyronic closed this Jul 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add block size display to ls (`ls -l`)#2488

Add block size display to ls (`ls -l`)#2488
skyronic wants to merge 5 commits intouutils:masterfrom
skyronic:ls-block-size

skyronic commented Jul 9, 2021

Uh oh!

tertsdiepraam left a comment •

edited

Loading

Uh oh!

tertsdiepraam Jul 10, 2021

Uh oh!

tertsdiepraam Jul 10, 2021

Uh oh!

skyronic commented Jul 13, 2021

Uh oh!

skyronic commented Jul 15, 2021

Uh oh!

tertsdiepraam commented Jul 15, 2021

Uh oh!

skyronic commented Jul 16, 2021

Uh oh!

skyronic commented Jul 23, 2021

Uh oh!

tertsdiepraam commented Jul 23, 2021

Uh oh!

skyronic commented Jul 23, 2021

Uh oh!

tertsdiepraam commented Jul 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

skyronic commented Jul 9, 2021

Uh oh!

tertsdiepraam left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tertsdiepraam Jul 10, 2021

Choose a reason for hiding this comment

Uh oh!

tertsdiepraam Jul 10, 2021

Choose a reason for hiding this comment

Uh oh!

skyronic commented Jul 13, 2021

Uh oh!

skyronic commented Jul 15, 2021

Uh oh!

tertsdiepraam commented Jul 15, 2021

Uh oh!

skyronic commented Jul 16, 2021

Uh oh!

skyronic commented Jul 23, 2021

Uh oh!

tertsdiepraam commented Jul 23, 2021

Uh oh!

skyronic commented Jul 23, 2021

Uh oh!

tertsdiepraam commented Jul 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tertsdiepraam left a comment •

edited

Loading