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

From: Jakub Narębski <jnareb@gmail.com>
Date: Jun 19, 2015, 18:47 UTC
Message-ID: <558463B4.3080904@gmail.com>
In-Reply-To: <xmqq7fqza8bo.fsf@gitster.dls.corp.google.com>

W dniu 2015-06-19 o 19:58, Junio C Hamano pisze:

> Charles Bailey <charles@hashpling.org> writes:

[...]

Show 23 quoted lines

>> +		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).

Show 7 quoted lines

> 
>  * 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

Show 11 quoted lines

>  - 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.
>