[Toybox] [PATCH] Fix human_readable HR_SI.

[enh]

On Sun, Sep 6, 2015 at 6:09 PM, Rob Landley <rob at landley.net> wrote:
> On 09/06/2015 02:23 AM, enh wrote:
>> Fix human_readable HR_SI.
>
> For a definition of "fix" that means "decimal is no longer the default
> without a flag".

that was how it was before until you accidentally flipped it :-)

i did it that way because (a) BSD does it that way round and (b) 1024
is more common than 1000 in existing tools.

> So this SI standards body thing is decimal units...?
> That's non-obvious to me.

yeah, which is why i suggested we change the name. i think HR_SI made
sense to me at the time because i was thinking of coreutils' --si
(which is like -h but /1000), but BSD has HR_DIVISOR_1000.

>> The sense of HR_SI got flipped (because the tests were all backwards).
>> With this, we get the same results as coreutils in the directories
>> I've tried, with the exception of cases where we give "better" results
>> by rounding to nearest rather than towards infinity.
>>
>> Also extend the tests a little.
>>
>> Maybe we should rename HR_SI to HR_1000, since "SI" is clearly
>> confusing? (coreutils uses --si to mean this, but I suspect it goes
>> largely unused.)
>
> Yeah, HR_1000 sounds like a better name.
>
>> diff --git a/lib/lib.c b/lib/lib.c
>> index 27305ec..f5ed52f 100644
>> --- a/lib/lib.c
>> +++ b/lib/lib.c
>> @@ -870,7 +870,7 @@ void names_to_pid(char **names, int
>> (*callback)(pid_t pid, char *name))
>>  int human_readable(char *buf, unsigned long long num, int style)
>>  {
>>    unsigned long long snap = 0;
>> -  int len, unit, divisor = (style&HR_SI) ? 1024 : 1000;
>> +  int len, unit, divisor = (style&HR_SI) ? 1000 : 1024;
>
> While we're at it the HR_B thing needs changing, and let's change the
> name (which affects df)...
>
>
>> -testing "human_readable" "test_human_readable 123456789" "123M\n" "" ""
>> -testing "human_readable -b" "test_human_readable -b 123456789" "123MB\n" "" ""
>> -testing "human_readable -s" "test_human_readable -s 123456789" "123 M\n" "" ""
>> -testing "human_readable -i" "test_human_readable -i 5675" "5.5k\n" "" ""
>> +testing "human_readable l 1024" "test_human_readable 123456789" "118M\n" "" ""
>> +testing "human_readable l 1000" "test_human_readable -i 123456789"
>
> What do the "l" and "s" mean in this context? (Previous one showed the
> flags...)

sorry, "large" and "small". i was trying to stay <80 columns.

>> "123M\n" "" ""
>> +testing "human_readable s 1024" "test_human_readable 5675" "5.5K\n" "" ""
>> +testing "human_readable s 1000" "test_human_readable -i 5675" "5.6k\n" "" ""
>> +
>> +# An example input where we give a better result than coreutils.
>> +# 267350/1024=261.08. We say 261K and coreutils says 262K.
>> +testing "human_readable" "test_human_readable 267350" "261K\n" "" ""
>> +
>> +testing "human_readable -b" "test_human_readable -b 123456789" "118MB\n" "" ""
>
> There was an earlier argument that HR_B meant use "B" instead of no
> metric for bytes. Not _always_ add a B?

oh, yeah. what you've done seems right.

> I changed it to do that, but if it's wrong...
>
>> +testing "human_readable -s" "test_human_readable -s 123456789" "118 M\n" "" ""
>> +testing "human_readable -bs" "test_human_readable -bs 123456789" "118
>> MB\n" "" ""
>
> Fun corner case: If we ask for a space but not HR_B we get a trailing
> space but no units. Is that right?

it's what BSD does. that's one reason why i flipped the sense of the
toybox flag compared to BSD's, and dd -- the only user of HR_SPACE --
asks for HR_SPACE|HR_B so it's fine, and you don't have to remember to
do anything in the usual case. (it may be that HR_SPACE and HR_B
should be the same flag, but there wasn't an obvious name for that.)

> Rob

btw, you broke the build by accidentally including this change:

-int yesno(char *prompt, int def);
+int yesno(int def);

-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.

 1441594983.0