Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
> I think the more typical way of coding this in this project is to
> initialize 'new_value' to -1. Doing so will make it easier to some day
> add a configuration value as fallback for when the sub-mode is not
> specified on the command line. So, it would look something like this:
>
> int submode = -1;
> if (arg) {
> int i;
> for (i = 0; i < ARRAY_SIZE(valid_modes); i++)
> if (!strcmp(arg, valid_modes[i]))
> break;
> if (i >= ARRAY_SIZE(valid_modes))
> return error(_("invalid value for --show-current-patch: %s"), arg);
> submode = i;
> }
>
> /* fall back to config value */
> if (submode < 0) {
> /* check if config value available and assign 'sudmode' */
> }
Hmph? Isn't the usual pattern more like this:
static int submode = -1; /* unspecified */
int cmd_foo(...)
{
git_config(...); /* this may update submode */
parse_options(...); /* this may further update submode */
if (submode < 0)
submode = ... some default value ...;
to implement "config gives a custom default, command line overrides,
but when there is neither, there is a hard-coded default"?
Of course, the variable can be initialized to the default value to
lose the "-1 /* unspecified */" bit.
>> + if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>> + return error(_("--show-current-patch=%s is incompatible with "
>> + "--show-current-patch=%s"),
>> + arg, valid_modes[resume->sub_mode]);
>
> So, this allows --show-current-patch=<foo> to be specified multiple
> times but only as long as <foo> is the same each time, and errors out
> otherwise. That's rather harsh and makes it difficult for someone to
> override a value specified earlier on the command line (say, coming
> from a Git alias). The typical way this is handled is "last wins"
> rather than making it an error.
Yup, the last one wins is something I would have expected. And if
we follow that (which is the usual pattern), I suspect that we won't
even need the first two steps of this series?
Thanks for a review.
