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

From: Junio C Hamano <gitster@pobox.com>
To: Charles Bailey <charles@hashpling.org>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 2/3] Move unsigned long option parsing out of pack-objects.c
Date: Sat, 20 Jun 2015 10:47:13 -0700	[thread overview]
Message-ID: <xmqqzj3ujmqm.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <20150620165138.GA27488@hashpling.org> (Charles Bailey's message of "Sat, 20 Jun 2015 17:51:39 +0100")

Charles Bailey <charles@hashpling.org> writes:

> On Fri, Jun 19, 2015 at 10:58:51AM -0700, Junio C Hamano wrote:
>
>> 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.

That is OK.  I just wanted to see that design decision explicitly
recorded in the proposed log message.

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

Interesting.  I get this instead:

    git: 'package-objects' is not a git command. See 'git --help'.

;-)  Jokes aside...

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

Yes and I wouldn't suggest changing that in the same commit that
exposes the human-readable quantity parsing to parse-options API.
That is why I said "Perhaps this is a minor regression".  It is a
change in behaviour, and it may make it slightly worse, but on the
other hand it makes it in line with other types of options, so it
may be OK.

If we wanted to teach commands to omit "usage" when parsing of a
single option failed, we should be doing that consistently for
everybody, not just to pack-objects, and that is outside the scope
of this patch, I would think.

	Side note: Just to make it clear, regarding anything I say
	is "outside the scope of this patch", I am not asking you to
	do them as follow-up patches (as a precondition to accept
	this patch).  For that matter, I am not convinced myself
	that some of them are even worth doing.  And I am not asking
	you _not_ to do these changes, ever, either.  I am just
	asking you _not_ to do any of them in _this_ patch.

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

It depends on what "git cmd --depth=4 --no-depth" should do.  In any
case, changing OPT_INT would be a separate topic outside the scope
of this patch, I think.

My gut feeling is that

    git pack-objects --max-pack-size=20m --no-max-pack-size

should be usable as a way to countermand a pack size limit given
earlier on the command line to make it unlimited, but that is
definitely a separate topic outside the scope of this patch (whose
purpose is to make an existing callback available to other callers).