On Tue, Feb 17, 2009 at 4:24 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Felipe Contreras <felipe.contreras@xxxxxxxxx> 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.
All right, done.
>> + 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.
Cool, I was worried those where not logical to the reader but I
couldn't think of a better solution... this one looks much better!
I've also made the same change for the 'types' variable in a later patch.
--
Felipe Contreras
--
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
