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

From: Charles Bailey <charles@hashpling.org>
Date: Jun 20, 2015, 16:51 UTC
Message-ID: <20150620165138.GA27488@hashpling.org>
In-Reply-To: <xmqq7fqza8bo.fsf@gitster.dls.corp.google.com>

On Fri, Jun 19, 2015 at 10:58:51AM -0700, Junio C Hamano wrote:

> Charles Bailey <charles@hashpling.org> writes:
> 
> Please place it immediately after INTEGER, as they are conceptually
> siblings---group similar things together.

Sorry, this is a bad habit from working on projects where changing the value of existing enum identifiers cause bad things.

Show 27 quoted lines

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

I was aware that I was changing the error reporting for max-pack-size and window-memory but thought that by going with the existing behaviour of OPT_INTEGER I'd be going with a more established pattern.

These observations also seem to apply to OPT_INTEGER handling. Would this be something that we'd want to fix too?

Currently git package-objects --depth=5.5 prints:

    error: option `depth' expects a numerical value
    usage: git pack-objects --stdout [options...
    [... many more lines omitted ...]

Obviously, changing this to skip the full usage report would affect many existing commands.

Also, I preserved the PARSE_OPT_NONEG flag for OPT_ULONG but would this ever not make sense for an OPT_INTEGER option?