Page MenuHomeFreeBSD

ls -h: humanize the total as well
ClosedPublic

Authored by pstef on Sun, Jan 5, 10:39 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Jan 16, 4:13 PM
Unknown Object (File)
Thu, Jan 16, 2:35 PM
Unknown Object (File)
Tue, Jan 14, 7:26 AM
Unknown Object (File)
Sun, Jan 12, 12:21 PM
Unknown Object (File)
Sun, Jan 12, 1:02 AM
Unknown Object (File)
Sat, Jan 11, 12:56 PM
Unknown Object (File)
Fri, Jan 10, 2:53 PM
Unknown Object (File)
Fri, Jan 10, 9:07 AM

Details

Summary

Before this change, the total printed on the first line was always in
blocks.

Now the long format with -h will print the "humanized number" instead.

ls -sh never provided sizes in anything other than blocks, total or
individual, and this commit doesn't change that.

The total number of blocks is a sum of fts_statp->st_blocks, so it's
multiplied by 512 and not by blocksize.

$ ls -lh /usr/bin | head -n 2
total 442 MB
-r-xr-xr-x 1 root wheel 70B Dec 20 21:06 CC

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

pstef requested review of this revision.Sun, Jan 5, 10:39 PM
pstef edited the summary of this revision. (Show Details)
ziaee requested changes to this revision.Wed, Jan 8, 3:05 AM
ziaee added a subscriber: ziaee.

Hi! This is a really interesting proposal! I always wished it did this, but also thought there was almost certainly some excellent reason why it hasn't. Since this is the foundation of userspace generations of scripts are built from, and thy Holy POLA of FreeBSD, I'm going to CC community leaders and grandmasters. Please forgive the disruption if this is undesirable. Thanks!

bin/ls/ls.1
503โ€“504

I'm not sure this should be done, but if we are going to do it. please integrate the documentation into the existing documentation like so.

542โ€“544

I think this looks tacked on. See above for my suggestion how to integrate it into the doc.

This revision now requires changes to proceed.Wed, Jan 8, 3:05 AM

Scripts shouldn't be using -h.

Scripts shouldn't be using -h.

(... unless what they do is pretty up something for the user, without actually understanding the bits they process.)

I think this change makes good sense, and it is arguably an oversight that it wasn't done that way when -l learned about -h

glebius added a subscriber: glebius.

I agree that scripts shouldn't use -h.

I agree with @phk that this was probably an oversight. I'm not sure I'd include blocks under -h -- why not just total 268503 without -h and total 131M with -h?

I agree with @phk that this was probably an oversight. I'm not sure I'd include blocks under -h -- why not just total 268503 without -h and total 131M with -h?

I wanted to keep the information that was always there, in case someone got used to it.

I wanted to keep the information that was always there, in case someone got used to it.

Just as a data point, GNU ls doesn't print the number of blocks with -h. Which doesn't mean it is bad to print it, especially given that scripts should not use -h. No personal preference here.

Just as a data point, GNU ls doesn't print the number of blocks with -h. Which doesn't mean it is bad to print it, especially given that scripts should not use -h. No personal preference here.

I know that it doesn't, but FreeBSD does and I assume it always did. I don't want to take away it, just add the "humanized" form.

It seems reasonable to me to switch to (rather than add) the humanized form for -h which is what's already done for the files in the list. "xMB in y blocks" also gives me the impression that we're presenting two different quantities (e.g. that 1M could be in some arbitrary number of blocks) - it would probably make more sense to show it as "total %s (%lu blocks)" if keeping both.

It seems reasonable to me to switch to (rather than add) the humanized form for -h which is what's already done for the files in the list. "xMB in y blocks" also gives me the impression that we're presenting two different quantities (e.g. that 1M could be in some arbitrary number of blocks) - it would probably make more sense to show it as "total %s (%lu blocks)" if keeping both.

My personal preference is only "x MB" as well, but my preference is unimportant here. I can do whichever of the 3 versions or something different still. Just waiting for some consensus.

I agree that -h changes the format of all numbers, So it should just be "x MB".

I vote for just outputting the size in bytes as a single value as well ("x MB")

pstef added inline comments.
bin/ls/ls.1
503โ€“504

The change you suggest duplicates the explanation which is currently given in paragraph 5.

pstef edited the summary of this revision. (Show Details)

Give up on the idea of providing both humanized and dehumanized numbers.

bin/ls/ls.1
307โ€“309

ls -hs is not showing me any bytes, just blocks.

pstef edited the summary of this revision. (Show Details)
bin/ls/ls.1
307โ€“309

It's showing the size in bytes that the blocks represent. You need to update the documentation for -h to mention that it changes the output for -s, it's just a matter of how you say it. It isn't say "1 MB" blocks, it's saying "1MB" bytes if I understand the code change correctly. How would you describe the string "total 1.3MB" to the user?

bin/ls/print.c
216

Why not use HUMANVALSTR_LEN here to match the settings used for printsize?

Maybe "Show the file size in bytes with the following unit suffixes"?

Maybe helpful additional context:
Netbsd manpage says "Modifies the -l and -s options, causing the sizes to be reported in bytes displayed in a human readable format.".
Illumos manpage says "All sizes are scaled to a human readable format, for example, 14K, 234M, 2.7G, or 3.0T. Scaling is done by repetitively dividing by 1024. The last --si or -h option determines the divisor used."

bin/ls/ls.1
307โ€“309

ls -hs shows no units, neither before nor after this proposed change.

bin/ls/print.c
216

HUMANVALSTR_LEN (5) is not enough to print the space and the unit (B). 7 is the minimum here. I don't understand why you'd want to match format from printsize, it has a different purpose (columnar display).

bin/ls/ls.1
307โ€“309

Hmm, isn't the "B" in "1024 KB" a unit?

545

(typo in my original suggestion)

bin/ls/print.c
216

I was just assuming we'd want to be consistent. I think the other case would use 1 MB instead of 1024 KB for example. I don't have a strong opinion either way though.

bin/ls/ls.1
307โ€“309

I'm not seeing any units here:

pstef@freefall:~/empty $ alias ls
ls='ls --color=auto'
pstef@freefall:~/empty $ ls -hs
total 1036
1035 dummy    1 empty
bin/ls/ls.1
307โ€“309

Is that with your change applied? Since you multiply the value by 512 before passing it to humanize_number() it must not be as 1036 isn't a multiple of 512? Also, the example in your change description has total 442 MB which would seem to have a B unit?

jhb added inline comments.
bin/ls/ls.1
307โ€“309

Sigh, somehow I completely misread the description of -s from the commit log and assumed the patch below was changing the behavior for -s.

bin/ls/print.c
212

This condition seems odd btw (old bug). We only call printlong when f_longform is true, so the part after the && here is always true.

The same weirdness exists in printcol which is where the total is output for the -sh case. There the same exact code is duplicated, but the f_longform is always false so the expression should just end with && f_size. Once those two nits are fixed, f_longform could then be static to ls.c.

Maybe worth fixing in a separate commit? I wonder if this total outputting logic used to be in a central place before where it made sense to check all these conditions and then it was duplicated during some refactoring? Looks like that bug goes back to BSD. Hmm, looks to be blind copy and paste of replacing the || subexpression in place of an older f_total helper variable in commits https://svnweb.freebsd.org/csrg?view=revision&revision=52260 and https://svnweb.freebsd.org/csrg?view=revision&revision=52263.

This revision was not accepted when it landed; it landed in state Needs Review.Thu, Jan 16, 4:04 PM
Closed by commit rG82fa7f83b53b: ls -lh: humanize the total (authored by pstef). ยท Explain Why
This revision was automatically updated to reflect the committed changes.