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