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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 19, 2015, 17:58 UTC
Message-ID: <xmqq7fqza8bo.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1434705059-2793-3-git-send-email-charles@hashpling.org>

Charles Bailey <charles@hashpling.org> writes:

Show 12 quoted lines

> diff --git a/parse-options.h b/parse-options.h
> index c71e9da..2ddb26f 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -18,7 +18,8 @@ enum parse_opt_type {
>  	OPTION_INTEGER,
>  	OPTION_CALLBACK,
>  	OPTION_LOWLEVEL_CALLBACK,
> -	OPTION_FILENAME
> +	OPTION_FILENAME,
> +	OPTION_ULONG
>  };

Please place it immediately after INTEGER, as they are conceptually siblings---group similar things together.

>  			return opterror(opt, "expects a numerical value", flags);
>  		return 0;
>  
> +	case OPTION_ULONG:

This one is placed right from that point of view ;-)

Show 12 quoted lines

> +		if (unset) {
> +			*(unsigned long *)opt->value = 0;
> +			return 0;
> +		}
> +		if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
> +			*(unsigned long *)opt->value = opt->defval;
> +			return 0;
> +		}
> +		if (get_arg(p, opt, flags, &arg))
> +			return -1;
> +		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.

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

Ponder:

 - we do not take "4.8"
 - 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.