Re: [PATCH 2/3] Move unsigned long option parsing out of pack-objects.c

["Jakub Narębski" <jnareb@gmail.com>]

W dniu 2015-06-19 o 19:58, Junio C Hamano pisze:
> Charles Bailey <charles@hashpling.org> writes: 
[...]
>> +		if (!git_parse_ulong(arg, opt->value))
>> +			return opterror(opt, "expects a numerical value", flags);
> 
> This used to be:
> 
>> -		die(_("unable to parse value '%s' for option %s"),
>> -		    arg, opt->long_name);
> 
> but opterror() talks about which option, so there is no information
> loss by losing "for option %s" from here.  That means there is only
> one difference for pack-objects:
> 
>     $ git pack-objects --max-pack-size=1T
>     fatal: unable to parse value '1T' for option max-pack-size
>     $ ./git pack-objects --max-pack-size=1T
>     error: option `max-pack-size' expects a numerical value
>     usage: git pack-objects --stdout [options...
>     ... 30 more lines omitted ...
> 
> Eh, make that two:
> 
>  * We no longer say what value we did not like.  The user presumably
>    knows what he typed, so this is only a minor loss.

Well, in this case this is not a problem, but for longer commandline
invocation it might be hard to find the exact argument among all the
options (though I don't think there is any integer-accepting option
that can be repeated).
> 
>  * We used to stop without giving "usage", as the error message was
>    specific enough.  We now spew descriptions on other options
>    unrelated to the specific error the user may want to concentrate
>    on.  Perhaps this is a minor regression.
> 
> I wonder if "expects a numerical value" is the best way to say this.

Is "expects numerical value" easier to understand than "unable to
parse value"?
 
> Ponder:
> 
>  - we do not take "4.8"

   - we won't take locale specific "4,8" (for some locales)

   - "4O" is not numerical... "40" is

>  - we do not take "-4".
>  - people may not realize, from "numerical", that we take "5M".
> 
> Except for the minor nits above, I think this is a good change.
> 
> This is a totally unrelated tangent that does not have to be part of
> your series, but we probably should take "4.8M"; I do not think we
> currently do.
> 
> Oh, and perhaps 1T, too.
>