Re: [PATCH 3/4] am: support --show-current-patch=raw as a synonym for--show-current-patch

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 19, 2020, 20:17 UTC
Message-ID: <xmqqmu9ee3hc.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAPig+cQOZwA3aAzBko-RL8UnW77DuBY-s_-J2D+35Ofn=fFfsg@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 20 quoted lines

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

Show 11 quoted lines

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