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

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Feb 19, 2020 at 11:15 AM <pbonzini@redhat.com> 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@redhat.com>
> ---
> 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;
> +}