On Wed, Feb 19, 2020 at 11:15 AM <pbonzini@xxxxxxxxxx> wrote:
> [...]
> We would like therefore to add a new mode to "git am" that copies
> .git/rebase-merge/patch to stdout. In order to preserve backwards
> compatibility, "git am --show-current-patch"'s behavior as to stay as
s/as to/has to/
> is, and the new functionality will be added as an optional
> argument to --show-current-patch. As a start, add the code to parse
> submodes. For now "raw" is the only valid submode, and it prints
> the full e-mail message just like "git am --show-current-patch".
>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---
> diff --git a/builtin/am.c b/builtin/am.c
> @@ -2078,7 +2082,14 @@ static int show_patch(struct am_state *state)
> - patch_path = am_path(state, msgnum(state));
> + switch (sub_mode) {
> + case SHOW_PATCH_RAW:
> + patch_path = am_path(state, msgnum(state));
> + break;
> + default:
> + abort();
> + }
I expect that this abort() is likely to go away in the next patch, so
it's not such a big deal, but the usual way to indicate that this is
an impossible condition is with BUG() rather than abort(). So, if you
happen to re-roll for some reason, perhaps consider using BUG()
instead.
> @@ -2130,8 +2141,42 @@ enum resume_type {
> +static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)
> +{
> + int new_value = SHOW_PATCH_RAW;
> +
> + if (arg) {
> + for (new_value = 0; new_value < ARRAY_SIZE(valid_modes); new_value++) {
> + if (!strcmp(arg, valid_modes[new_value]))
> + break;
> + }
> + if (new_value >= ARRAY_SIZE(valid_modes))
> + return error(_("Invalid value for --show-current-patch: %s"), arg);
> + }
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' */
}
> + 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.
> + resume->mode = RESUME_SHOW_PATCH;
> + resume->sub_mode = new_value;
> + return 0;
> +}
