Re: [PATCH] config: Use parseopt.

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 14, 2009, 09:35 UTC
Message-ID: <7v1vu1webk.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <7vab8pweod.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 10 quoted lines

>> +	OPT_STRING(0, "get-color", &get_color_name, "name", "find the color configured: [default]"),
>
> get-color is used to get the defined color for a given slot.  Please do not
> rename it to "name" (see the original).
>
>> +	OPT_STRING(0, "get-colorbool", &get_colorbool_name, "name", "find the color setting: [stdout-is-tty]"),
>
> get-colorbool is used to get the boolean setting for a given configuration
> variable.  Please do not rename it to "name" (see the original).
>

By the way, I think it might be more appropriate if you categorize the above two (especially the "colorbool" one) in the "Types" section, as that really is what is happening. Instead of doing the usual "print the value as a string", it does something magical.

> Overall, with the same number of lines, we lost a lot of error checking in
> exchange for an ability to say "git config --remove-sec" instead of "git
> config --remove-section", and "git config --help" may give an easier to
> read message.

And I forgot to mention the "option categorization" merit.

> Given that "git config" is primarily meant for script use, this particular
> round does not look like a good tradeoff to me.

Don't take this too negatively. The tradeoff will improve if there aren't these obvious bugs you can spot without even running the code.