Re: [PATCH] config: Use parseopt.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Junio C Hamano <gitster@xxxxxxxxx> writes:

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux