| Message ID | 20200219161352.13562-4-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | am: provide a replacement for "cat .git/rebase-apply/patch" | expand |
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; > +}
Eric Sunshine <sunshine@sunshineco.com> 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.
On 19/02/20 21:17, Junio C Hamano wrote: >>> + 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? We would need them anyway, in order to add a callback to the "command mode" option --show-current-patch. The fact that --show-current-patch is a command mode option is also why I decided against "last one wins". I think it would be counterintuitive that git am --abort --show-current-patch fails, but git am --show-current-patch=diff --show-current-patch=raw succeeds. Another possibility is to have separate options --show-current-message (for .git/rebase-apply/NNNN) and --show-current-diff (for .git/rebase-apply/patch), possibly deprecating --show-current-patch. That would have naturally rejected a command line like git am --show-current-message --show-current-diff (and this one _would_ have removed the need for the first two patches in the series). However, the long common prefix would have prevented using an abbreviated option such as "--show", so I went instead for the optional string argument. I realize now that I should have placed all this in the commit message, sorry about that. Paolo
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt index 11ca61b00b..bafb491ede 100644 --- a/Documentation/git-am.txt +++ b/Documentation/git-am.txt @@ -16,7 +16,7 @@ SYNOPSIS [--exclude=<path>] [--include=<path>] [--reject] [-q | --quiet] [--[no-]scissors] [-S[<keyid>]] [--patch-format=<format>] [(<mbox> | <Maildir>)...] -'git am' (--continue | --skip | --abort | --quit | --show-current-patch) +'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=raw]) DESCRIPTION ----------- @@ -176,7 +176,7 @@ default. You can use `--no-utf8` to override this. Abort the patching operation but keep HEAD and the index untouched. ---show-current-patch:: +--show-current-patch[=raw]:: Show the entire e-mail message "git am" has stopped at, because of conflicts. diff --git a/builtin/am.c b/builtin/am.c index a89e1a96ed..ec4c743556 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -81,6 +81,10 @@ enum signoff_type { SIGNOFF_EXPLICIT /* --signoff was set on the command-line */ }; +enum show_patch_type { + SHOW_PATCH_RAW = 0, +}; + struct am_state { /* state directory path */ char *dir; @@ -2061,7 +2065,7 @@ static void am_abort(struct am_state *state) am_destroy(state); } -static int show_patch(struct am_state *state) +static int show_patch(struct am_state *state, enum show_patch_type sub_mode) { struct strbuf sb = STRBUF_INIT; const char *patch_path; @@ -2078,7 +2082,14 @@ static int show_patch(struct am_state *state) return ret; } - patch_path = am_path(state, msgnum(state)); + switch (sub_mode) { + case SHOW_PATCH_RAW: + patch_path = am_path(state, msgnum(state)); + break; + default: + abort(); + } + len = strbuf_read_file(&sb, patch_path, 0); if (len < 0) die_errno(_("failed to read '%s'"), patch_path); @@ -2130,8 +2141,42 @@ enum resume_type { struct resume_mode { enum resume_type mode; + enum show_patch_type sub_mode; }; +static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset) +{ + int *opt_value = opt->value; + struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode); + + /* + * Please update $__git_showcurrentpatch in git-completion.bash + * when you add new options + */ + const char *valid_modes[] = { + [SHOW_PATCH_RAW] = "raw" + }; + 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); + } + + 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]); + + resume->mode = RESUME_SHOW_PATCH; + resume->sub_mode = new_value; + return 0; +} + static int git_am_config(const char *k, const char *v, void *cb) { int status; @@ -2233,9 +2278,11 @@ int cmd_am(int argc, const char **argv, const char *prefix) OPT_CMDMODE(0, "quit", &resume.mode, N_("abort the patching operation but keep HEAD where it is."), RESUME_QUIT), - OPT_CMDMODE(0, "show-current-patch", &resume.mode, - N_("show the patch being applied."), - RESUME_SHOW_PATCH), + { OPTION_CALLBACK, 0, "show-current-patch", &resume.mode, + "raw", + N_("show the patch being applied"), + PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, + parse_opt_show_current_patch, RESUME_SHOW_PATCH }, OPT_BOOL(0, "committer-date-is-author-date", &state.committer_date_is_author_date, N_("lie about committer date")), @@ -2354,7 +2401,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) am_destroy(&state); break; case RESUME_SHOW_PATCH: - ret = show_patch(&state); + ret = show_patch(&state, resume.sub_mode); break; default: BUG("invalid resume value"); diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 1aac5a56c0..247f34f1fa 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1197,6 +1197,7 @@ __git_count_arguments () __git_whitespacelist="nowarn warn error error-all fix" __git_patchformat="mbox stgit stgit-series hg mboxrd" +__git_showcurrentpatch="raw" __git_am_inprogress_options="--skip --continue --resolved --abort --quit --show-current-patch" _git_am () @@ -1215,6 +1216,10 @@ _git_am () __gitcomp "$__git_patchformat" "" "${cur##--patch-format=}" return ;; + --show-current-patch=*) + __gitcomp "$__git_showcurrentpatch" "" "${cur##--show-current-patch=}" + return + ;; --*) __gitcomp_builtin am "" \ "$__git_am_inprogress_options" diff --git a/t/t4150-am.sh b/t/t4150-am.sh index 4f1e24ecbe..afe456e75e 100755 --- a/t/t4150-am.sh +++ b/t/t4150-am.sh @@ -666,6 +666,16 @@ test_expect_success 'am --show-current-patch' ' test_cmp .git/rebase-apply/0001 actual.patch ' +test_expect_success 'am --show-current-patch=raw' ' + git am --show-current-patch=raw >actual.patch && + test_cmp .git/rebase-apply/0001 actual.patch +' + +test_expect_success 'am accepts repeated --show-current-patch' ' + git am --show-current-patch --show-current-patch=raw >actual.patch && + test_cmp .git/rebase-apply/0001 actual.patch +' + test_expect_success 'am --skip works' ' echo goodbye >expected && git am --skip &&