Re: [PATCH v2 3/8] config: Use parseopt.

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 17, 2009, 02:24 UTC
Message-ID: <7vbpt1by1y.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1234832094-15541-3-git-send-email-felipe.contreras@gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

> +	if (HAS_MULTI_BITS(actions)) {
> +		error("only one action at a time.");
> +		usage_with_options(builtin_config_usage, builtin_config_options);

My initial reaction was:

	Can we easily say "--get and --getall are mutually incompatible"?

and knowing that it would take much more code, the second reaction was:

	Does the user know what we mean by "action"?

Since the answer to this is "Yes, the usage message comes from parseopt and there is a clear categorization", I think the message is good enough.

What happens when the user says "config --get --get-colorbool user.name"? Is it an error? Is it diagnosed as an error?

It probably is easy to fix it by defining two bits of fake actions and do:

	if (get_color_slot)
        	actions |= ACTION_GET_COLOR;
	if (get_colorbool_slot)
        	actions |= ACTION_GET_COLORBOOL;

immediately before this HAS_MULTI_BITS check.

I know I suggested these to are type-like, but I realize that these two are better categorized as actions tied to a specific type (color), as you had in your earlier round.

Show 5 quoted lines

> +	if (actions == 0)
> +		switch (argc) {
> +		case 1: actions |= ACTION_GET; break;
> +		case 2: actions |= ACTION_ADD; break;
> +		case 3: actions |= ACTION_REPLACE_ALL; break;

Straight assignment, not ORing-in please. It wastes a few seconds from the reader wondering what other bits in the variable "actions" are used for things other than ACTION_* (the answer is none).

Similarly, later conditions:

> +	if (actions & ACTION_LIST) {

would read better if they used equality == checks.