10000 ls: Fix display of inodes and add allocation size feature by kimono-koans · Pull Request #3052 · uutils/coreutils · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

ls: Fix display of inodes and add allocation size feature #3052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 41 commits into from
Mar 15, 2022
Merged

ls: Fix display of inodes and add allocation size feature #3052

merged 41 commits into from
Mar 15, 2022

Conversation

kimono-koans
Copy link
Contributor

Only reason the GNU test block size (block-size.sh) appears to be failing is because of issues related to dd not parsing 'conv' correctly (!).

  • Add block size display ("-s", "--size") feature
  • Add custom block size option ("--block-size")
  • Add kibibytes ("-k", "--kibibytes") feature
  • Correct ordering of preferences for block size options
  • Add env var parsing for BLOCK_SIZE, LS_BLOCK_SIZE, POSIXLY_CORRECT
  • Fix how both block size and inode are displayed. Instead of printing through either display_long_item or display_file_name have been moved into a display_more_info function, which could be useful to other "ls" features that print the same whether in Format::Long or not.
  • Tests included to check proper alignment and block size values, and ordering of option preferences
  • Seems like a lot but these are all related options.

@hbina hbina mentioned this pull request Feb 3, 2022
Closed
hbina
hbina reviewed Feb 4, 2022
View reviewed changes
hbina
hbina reviewed Feb 4, 2022
View reviewed changes
hbina
hbina reviewed Feb 4, 2022
View reviewed changes
@hbina
Copy link
Contributor
hbina commented Feb 4, 2022 •
edited
Loading

I think there's some miscalculation somewhere, trying to figure out where,

output is sorted because we currently don't sort by default.

hbina@akarin ~/g/uutils (ls_2)> ls -l --block-size=512 | sort
drwxrwxr-x 2 hbina hbina   8 Dec 24 22:33 examples
drwxrwxr-x 2 hbina hbina   8 Feb  2 21:32 dir2
drwxrwxr-x 2 hbina hbina   8 Feb  2 23:18 dir1
drwxrwxr-x 2 hbina hbina   8 Jan 31 23:43 util
drwxrwxr-x 4 hbina hbina   8 Jan 31 23:20 target
drwxrwxr-x 4 hbina hbina   8 Jan 31 23:43 docs
drwxrwxr-x 6 hbina hbina   8 Dec 24 22:33 src
drwxrwxr-x 6 hbina hbina   8 Jan 31 23:43 tests
-rw-rw-r-- 1 hbina hbina  11 Dec 24 22:33 CODE_OF_CONDUCT.md <=== does not match
-rw-rw-r-- 1 hbina hbina  12 Jan 31 23:43 build.rs <=== does not match
-rw-rw-r-- 1 hbina hbina 131 Feb  2 21:31 Cargo.lock
-rw-rw-r-- 1 hbina hbina  14 Jan 31 23:43 GNUmakefile
-rw-rw-r-- 1 hbina hbina   1 Dec 24 22:33 Makefile
-rw-rw-r-- 1 hbina hbina  21 Dec 24 22:33 Makefile.toml
-rw-rw-r-- 1 hbina hbina  33 Feb  3 21:33 Cargo.toml
-rw-rw-r-- 1 hbina hbina  37 Feb  2 21:31 README.md
-rw-rw-r-- 1 hbina hbina   3 Dec 24 22:33 LICENSE
-rw-rw-r-- 1 hbina hbina   7 Jan 31 23:43 DEVELOPER_INSTRUCTIONS.md
-rw-rw-r-- 1 hbina hbina   8 Jan 31 23:43 CONTRIBUTING.md
total 384
hbina@akarin ~/g/uutils (ls_2)> cargo run --quiet -- ls -l --block-size=512 | sort
drwxrwxr-x 2 hbina hbina    8 Dec 24 22:33 examples
drwxrwxr-x 2 hbina hbina    8 Feb  2 21:32 dir2
drwxrwxr-x 2 hbina hbina    8 Feb  2 23:18 dir1
drwxrwxr-x 2 hbina hbina    8 Jan 31 23:43 util
drwxrwxr-x 4 hbina hbina    8 Jan 31 23:20 target
drwxrwxr-x 4 hbina hbina    8 Jan 31 23:43 docs
drwxrwxr-x 6 hbina hbina    8 Dec 24 22:33 src
drwxrwxr-x 6 hbina hbina    8 Jan 31 23:43 tests
-rw-rw-r-- 1 hbina hbina  136 Feb  2 21:31 Cargo.lock
-rw-rw-r-- 1 hbina hbina   16 Dec 24 22:33 CODE_OF_CONDUCT.md <=== does not match
-rw-rw-r-- 1 hbina hbina   16 Jan 31 23:43 build.rs <=== does not match
-rw-rw-r-- 1 hbina hbina   16 Jan 31 23:43 GNUmakefile
-rw-rw-r-- 1 hbina hbina   24 Dec 24 22:33 Makefile.toml
-rw-rw-r-- 1 hbina hbina   40 Feb  2 21:31 README.md
-rw-rw-r-- 1 hbina hbina   40 Feb  3 21:33 Cargo.toml
-rw-rw-r-- 1 hbina hbina    8 Dec 24 22:33 LICENSE
-rw-rw-r-- 1 hbina hbina    8 Dec 24 22:33 Makefile
-rw-rw-r-- 1 hbina hbina    8 Jan 31 23:43 CONTRIBUTING.md
-rw-rw-r-- 1 hbina hbina    8 Jan 31 23:43 DEVELOPER_INSTRUCTIONS.md
total 384

Edit: Fixed this with this change. Basically the size shown is not the number of blocks allocated, but the minimum number of blocks required for the file size.

diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs
index 48486865..967eecbe 100644
--- a/src/uu/ls/src/ls.rs
+++ b/src/uu/ls/src/ls.rs
@@ -1805,10 +1805,17 @@ fn pad_right(string: &str, count: usize) -> String {
 }
 
 fn customize_block_size(size: u64, config: &Config) -> u64 {
-    if config.block_size.is_some() {
-        size / config.block_size.unwrap() as u64
+    let block_size = config.block_size.unwrap_or(DEFAULT_BLOCK_SIZE) as u64;
+    size / block_size
+}
+
+fn customize_file_size(size: u64, config: &Config) -> u64 {
+    let block_size = config.block_size.unwrap_or(DEFAULT_BLOCK_SIZE) as u64;
+    let lower_bound = size / block_size;
+    if lower_bound * block_size < size {
+        lower_bound + 1
     } else {
-        size / DEFAULT_BLOCK_SIZE as u64
+        lower_bound
     }
 }
 
@@ -1930,7 +1937,7 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter<Stdout
             mut longest_size_len,
             mut longest_major_len,
             mut longest_minor_len,
-        ) = (1, 1, 1, 1, 1, 1, 1);
+        ) = (1, 1, 1, 1, 1, 0, 0);
 
         #[cfg(not(unix))]
         let (
@@ -2079,6 +2086,22 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter<Stdout
     }
 }
 
+fn get_file_size(md: &Metadata) -> u64 {
+    /* GNU ls will display sizes in terms of block size
+       md.len() will differ from this value when the file has some holes
+    */
+    #[cfg(unix)]
+    {
+        md.size()
+    }
+
+    #[cfg(not(unix))]
+    {
+        // no way to get block size for windows, fall-back to file size
+        md.len()
+    }
+}
+
 fn get_block_size(md: &Metadata) -> u64 {
     /* GNU ls will display sizes in terms of block size
        md.len() will differ from this value when the file has some holes
@@ -2515,7 +2538,7 @@ fn display_size_or_rdev(metadata: &Metadata, config: &Config) -> SizeOrDeviceId
     }
     if config.block_size.is_some() {
         SizeOrDeviceId::Size(display_size(
-            customize_block_size(get_block_size(metadata), config),
+            customize_file_size(get_file_size(metadata), config),
             config,
         ))
     } else {

@kimono-koans
Copy link
Contributor Author

I think there's some miscalculation somewhere, trying to figure out where,

Okay, this is interesting, but I think it may be outside the scope of this PR, as it addresses the non-block size given in the LongFormat option. I've noticed similar behavior too.

Anyway, I think this might be better placed in a separate PR with some tests attached, but appreciate your work. Thank you.

@kimono-koans
Copy link
Contributor Author

I think there's some miscalculation somewhere, trying to figure out where,

If you prepare a PR, let me know. I'd be pleased to review. One suggestion -- I would generic-ize the operation of rounding up to make clear what you are doing/not doing, with something like: https://doc.rust-lang.org/std/primitive.u64.html#method.unstable_div_ceil

@hbina
Copy link
Contributor
hbina commented Feb 5, 2022

I'll create the PR after yours are merged because its based on that.

@kimono-koans
Copy link
Contributor Author

@sylvestre @tertsdiepraam Don't mean to rush a review but CICD won't tell me which test is failing. Perhaps I'm missing something. I see:

Warning: Congrats! The gnu test tests/cp/copy-FMR is now passing!
Warning: Congrats! The gnu test tests/misc/shuf-reservoir is now passing!
Warning: Congrats! The gnu test tests/misc/sort-stale-thread-mem is now passing!
Warning: Congrats! The gnu test tests/misc/tty-eof is now passing!

But no indication which test is newly failing per:

Warning: Changes from main: PASS -1 / FAIL -4 / ERROR +0 / SKIP +5

Any assistance you could provide would be appreciated. Thanks!

@kimono-koans
Copy link
Contributor Author

@sylvestre @tertsdiepraam I'd like to request a review, when it is convenient. Thank you.

tertsdiepraam
tertsdiepraam requested changes Feb 26, 2022
View reviewed changes
Copy link
Member
@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

I think it's correct, but it needs some cleanup or documentation in a few places. Nice work!

@kimono-koans
Copy link
Contributor Author
kimono-koans commented Mar 1, 2022 •
edited
Loading

I think it's correct, but it needs some cleanup or documentation in a few places. Nice work!

I think this may be ready to merge but I think it also needs your all clear, when it is convenient. Thank you for your review. It really needed some more TLC and your comments very much helped.

@kimono-koans
Copy link
Contributor Author

@hbina FYI I added your suggestion with a test. Thanks!

tertsdiepraam
tertsdiepraam approved these changes Mar 5, 2022
View reviewed changes
Copy link
Member
@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Thank you! It looks great now! I found one last thing to change and then it can get merged.

@sylvestre sylvestre merged commit fa6af85 into uutils:main Mar 15, 2022
@kimono-koans kimono-koans deleted the ls_2 branch April 4, 2022 19:15
@sylvestre sylvestre mentioned this pull request Apr 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hbina hbina hbina left review comments

@tertsdiepraam tertsdiepraam tertsdiepraam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kimono-koans @hbina @tertsdiepraam @sylvestre
0