Re: numfmt (=print 'human' sizes) updates

[Pádraig Brady]

On 12/14/2012 08:53 AM, Pádraig Brady wrote:
> On 12/14/2012 05:38 AM, Assaf Gordon wrote:
> > Hello,
> >
> > Attached is a slightly improved patch - minor code changes, and many more tests.
> > Line coverage is 98%, and branch coverage is now >93% , and most of the 
> > non-covered branches are simply unreachable (I'm checking the reachable ones).
> >
> > The comments below still apply.

It's looking like a really useful cohesive command.

I verified all the new tests pass and
syntax check is fine too.
Your attention to detail is _much_ appreciated.

I'll note here some _future_ TODO items, which
don't need to be added for the initial version:
  1. Parse numbers containing grouping separators.
  2. Have --fields support multiple values.

I did some basic perf testing and verified no leaks with:
$ yes "1 2" | pv > /dev/null
^C09MB 0:00:03 [ 102MB/s] [       <=>                                           
          ]
$ yes "1 2" | (ulimit -v 20000; src/numfmt --field 2 --to=iec --from=auto) | pv 
> /dev/null
^C.9MB 0:00:11 [6.32MB/s] [                           <=>                       
          ]
$ yes "1 2" | head -n10 | valgrind src/numfmt --field 2 --to=iec --from=auto 
2>&1 | grep leaks
==23446== All heap blocks were freed -- no leaks are possible

I notice the "UNIT options:" seciont is mis-formatted by help2man.

I notice use of fprintf (stderr, ...) where error (0, 0, ...) should be used.

indent -nut src/numfmt.c makes lots of changes, most valid.


So on to some usage...

This shows auto padding works, with before and after displayed through tee.

$ df | head -n3 | tee >(src/numfmt --header --field=2 --to=iec)
Filesystem           1K-blocks      Used Available Use% Mounted on
rootfs                13102208  12544324    424816  97% /
udev                   1454732         0   1454732   0% /dev
Filesystem           1K-blocks      Used Available Use% Mounted on
rootfs                     13M  12544324    424816  97% /
udev                      1.4M         0   1454732   0% /dev

This shows padding increases the field width as required in edge cases:

$ src/df /home | tee >(src/numfmt --header --field=2 --from-unit=1024 
--to-unit=1 --group)
Filesystem     1K-blocks     Used Available Use% Mounted on
/dev/sdb1      100791728 71565732  24105996  75% /home
Filesystem     1K-blocks     Used Available Use% Mounted on
/dev/sdb1 103,210,729,472 71565732  24105996  75% /home

I notice the unit on both --to=iec and --to=si is the same.
I was thinking that --to=iec would add a 'i' to distinguish?

$ /bin/ls -l | head -n4 | src/numfmt --to=iec --field=5
total 7252
-rw-rw-r--.  1 padraig padraig     92K Aug 23  2011 ABOUT-NLS
-rw-rw-r--.  1 padraig padraig     49K Dec 21 16:22 aclocal.m4
-rw-rw-r--.  1 padraig padraig    3.6K Dec 21 16:15 AUTHORS

I notice we exit immediately with invalid numbers.
How about printing the warning, outputting the unparsable number as is
and continuing?

$ printf "%s\n" 1 one 2 | src/numfmt
1
src/numfmt: invalid number: 'one'


As suggested missing fields are ignored now...

$ echo -ne "1\n2 2\n" | src/numfmt --field=2
1
2 2

...and we get warnings with --debug. However they mix awkwardly with
the output. Perhaps delay the warning until line causing the warning is 
displayed.

$ echo -ne "1\n2 2\n" | src/numfmt --field=2 --debug
src/numfmt: no conversion option specified
1src/numfmt: input line is too short, no numbers found to convert in field 2

2 2

--devdebug should probably renamed to ---devdebug

I'm starting to think the original idea of having a --format option
would be more a more concise, well known and extendible interface
rather than having --padding, --grouping, --base, ...
It wouldn't have to be passed directly to printf, and could
be parsed and preprocessed something like we do in seq(1).

thanks!
Pádraig.