* [PATCH 1/4] parse-options: convert "command mode" to a flag
2020-02-19 16:13 [PATCH 0/4] am: provide a replacement for "cat .git/rebase-apply/patch" pbonzini
@ 2020-02-19 16:13 ` pbonzini
2020-02-19 19:11 ` Eric Sunshine
2020-02-19 19:15 ` Junio C Hamano
2020-02-19 16:13 ` [PATCH 2/4] am: convert "resume" variable to a struct pbonzini
` (2 subsequent siblings)
3 siblings, 2 replies; 15+ messages in thread
From: pbonzini @ 2020-02-19 16:13 UTC (permalink / raw)
To: git; +Cc: bfields
From: Paolo Bonzini <pbonzini@redhat.com>
OPTION_CMDMODE is essentially OPTION_SET_INT plus the extra check
that the variable had not set before. In order to allow custom
processing, change it to OPTION_SET_INT plus a new flag that takes
care of the check. This works as long as the option value points
to an int.
Add testcases while at it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
parse-options.c | 20 +++++++++-----------
parse-options.h | 8 ++++----
t/helper/test-parse-options.c | 2 ++
t/t0040-parse-options.sh | 18 ++++++++++++++++++
4 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index a0cef401fc..c6e9e2733b 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -61,7 +61,7 @@ static enum parse_opt_result opt_command_mode_error(
*/
for (that = all_opts; that->type != OPTION_END; that++) {
if (that == opt ||
- that->type != OPTION_CMDMODE ||
+ !(that->flags & PARSE_OPT_CMDMODE) ||
that->value != opt->value ||
that->defval != *(int *)opt->value)
continue;
@@ -95,6 +95,14 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
if (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG))
return error(_("%s takes no value"), optname(opt, flags));
+ /*
+ * Giving the same mode option twice, although unnecessary,
+ * is not a grave error, so let it pass.
+ */
+ if ((opt->flags & PARSE_OPT_CMDMODE) &&
+ *(int *)opt->value && *(int *)opt->value != opt->defval)
+ return opt_command_mode_error(opt, all_opts, flags);
+
switch (opt->type) {
case OPTION_LOWLEVEL_CALLBACK:
return opt->ll_callback(p, opt, NULL, unset);
@@ -130,16 +138,6 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
*(int *)opt->value = unset ? 0 : opt->defval;
return 0;
- case OPTION_CMDMODE:
- /*
- * Giving the same mode option twice, although is unnecessary,
- * is not a grave error, so let it pass.
- */
- if (*(int *)opt->value && *(int *)opt->value != opt->defval)
- return opt_command_mode_error(opt, all_opts, flags);
- *(int *)opt->value = opt->defval;
- return 0;
-
case OPTION_STRING:
if (unset)
*(const char **)opt->value = NULL;
diff --git a/parse-options.h b/parse-options.h
index 1d60205881..fece5ba628 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -18,7 +18,6 @@ enum parse_opt_type {
OPTION_BITOP,
OPTION_COUNTUP,
OPTION_SET_INT,
- OPTION_CMDMODE,
/* options with arguments (usually) */
OPTION_STRING,
OPTION_INTEGER,
@@ -47,7 +46,8 @@ enum parse_opt_option_flags {
PARSE_OPT_LITERAL_ARGHELP = 64,
PARSE_OPT_SHELL_EVAL = 256,
PARSE_OPT_NOCOMPLETE = 512,
- PARSE_OPT_COMP_ARG = 1024
+ PARSE_OPT_COMP_ARG = 1024,
+ PARSE_OPT_CMDMODE = 2048
};
enum parse_opt_result {
@@ -168,8 +168,8 @@ struct option {
#define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)
#define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
(h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}
-#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \
- (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }
+#define OPT_CMDMODE(s, l, v, h, i) { OPTION_SET_INT, (s), (l), (v), NULL, \
+ (h), PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }
#define OPT_INTEGER(s, l, v, h) OPT_INTEGER_F(s, l, v, h, 0)
#define OPT_MAGNITUDE(s, l, v, h) { OPTION_MAGNITUDE, (s), (l), (v), \
N_("n"), (h), PARSE_OPT_NONEG }
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index af82db06ac..2051ce57db 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -121,6 +121,8 @@ int cmd__parse_options(int argc, const char **argv)
OPT_INTEGER('j', NULL, &integer, "get a integer, too"),
OPT_MAGNITUDE('m', "magnitude", &magnitude, "get a magnitude"),
OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23),
+ OPT_CMDMODE(0, "mode1", &integer, "set integer to 1 (cmdmode option)", 1),
+ OPT_CMDMODE(0, "mode2", &integer, "set integer to 2 (cmdmode option)", 2),
OPT_CALLBACK('L', "length", &integer, "str",
"get length of <str>", length_callback),
OPT_FILENAME('F', "file", &file, "set file to <file>"),
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 9d7c7fdaa2..7f4c15a52b 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -23,6 +23,8 @@ usage: test-tool parse-options <options>
-j <n> get a integer, too
-m, --magnitude <n> get a magnitude
--set23 set integer to 23
+ --mode1 set integer to 1 (cmdmode option)
+ --mode2 set integer to 2 (cmdmode option)
-L, --length <str> get length of <str>
-F, --file <file> set file to <file>
@@ -324,6 +326,22 @@ test_expect_success 'OPT_NEGBIT() works' '
test-tool parse-options --expect="boolean: 6" -bb --no-neg-or4
'
+test_expect_success 'OPT_CMDMODE() works' '
+ test-tool parse-options --expect="integer: 1" --mode1
+'
+
+test_expect_success 'OPT_CMDMODE() detects incompatibility' '
+ test_must_fail test-tool parse-options --mode1 --mode2 >output 2>output.err &&
+ test_must_be_empty output &&
+ grep "incompatible with --mode" output.err
+'
+
+test_expect_success 'OPT_CMDMODE() detects incompatibility with something else' '
+ test_must_fail test-tool parse-options --set23 --mode2 >output 2>output.err &&
+ test_must_be_empty output &&
+ grep "incompatible with something else" output.err
+'
+
test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '
test-tool parse-options --expect="boolean: 6" + + + + + +
'
--
2.21.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 1/4] parse-options: convert "command mode" to a flag
2020-02-19 16:13 ` [PATCH 1/4] parse-options: convert "command mode" to a flag pbonzini
@ 2020-02-19 19:11 ` Eric Sunshine
2020-02-20 16:00 ` Johannes Schindelin
2020-02-19 19:15 ` Junio C Hamano
1 sibling, 1 reply; 15+ messages in thread
From: Eric Sunshine @ 2020-02-19 19:11 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Git List, bfields
On Wed, Feb 19, 2020 at 11:15 AM <pbonzini@redhat.com> wrote:
> OPTION_CMDMODE is essentially OPTION_SET_INT plus the extra check
> that the variable had not set before. In order to allow custom
> processing, change it to OPTION_SET_INT plus a new flag that takes
> care of the check. This works as long as the option value points
> to an int.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> @@ -324,6 +326,22 @@ test_expect_success 'OPT_NEGBIT() works' '
> +test_expect_success 'OPT_CMDMODE() detects incompatibility' '
> + test_must_fail test-tool parse-options --mode1 --mode2 >output 2>output.err &&
> + test_must_be_empty output &&
> + grep "incompatible with --mode" output.err
> +'
The error message may have been localized, so use test_i18ngrep()
instead of 'grep':
test_i18ngrep "incompatible with --mode" output.err
> +
> +test_expect_success 'OPT_CMDMODE() detects incompatibility with something else' '
> + test_must_fail test-tool parse-options --set23 --mode2 >output 2>output.err &&
> + test_must_be_empty output &&
> + grep "incompatible with something else" output.err
> +'
Ditto.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/4] parse-options: convert "command mode" to a flag
2020-02-19 19:11 ` Eric Sunshine
@ 2020-02-20 16:00 ` Johannes Schindelin
0 siblings, 0 replies; 15+ messages in thread
From: Johannes Schindelin @ 2020-02-20 16:00 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Paolo Bonzini, Git List, bfields
Hi Eric,
On Wed, 19 Feb 2020, Eric Sunshine wrote:
> On Wed, Feb 19, 2020 at 11:15 AM <pbonzini@redhat.com> wrote:
> > OPTION_CMDMODE is essentially OPTION_SET_INT plus the extra check
> > that the variable had not set before. In order to allow custom
> > processing, change it to OPTION_SET_INT plus a new flag that takes
> > care of the check. This works as long as the option value points
> > to an int.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> > diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> > @@ -324,6 +326,22 @@ test_expect_success 'OPT_NEGBIT() works' '
> > +test_expect_success 'OPT_CMDMODE() detects incompatibility' '
> > + test_must_fail test-tool parse-options --mode1 --mode2 >output 2>output.err &&
> > + test_must_be_empty output &&
> > + grep "incompatible with --mode" output.err
> > +'
>
> The error message may have been localized, so use test_i18ngrep()
> instead of 'grep':
>
> test_i18ngrep "incompatible with --mode" output.err
The error message _is_ localized, causing the GETTEXT_POISON job to fail:
https://dev.azure.com/gitgitgadget/git/_build/results?buildId=31113&view=ms.vss-test-web.build-test-results-tab&runId=97704&resultId=102357&paneView=debug
So yes. It needs to be changed to `test_i18ngrep`.
Thanks,
Johannes
>
> > +
> > +test_expect_success 'OPT_CMDMODE() detects incompatibility with something else' '
> > + test_must_fail test-tool parse-options --set23 --mode2 >output 2>output.err &&
> > + test_must_be_empty output &&
> > + grep "incompatible with something else" output.err
> > +'
>
> Ditto.
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] parse-options: convert "command mode" to a flag
2020-02-19 16:13 ` [PATCH 1/4] parse-options: convert "command mode" to a flag pbonzini
2020-02-19 19:11 ` Eric Sunshine
@ 2020-02-19 19:15 ` Junio C Hamano
2020-02-19 21:05 ` Paolo Bonzini
1 sibling, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2020-02-19 19:15 UTC (permalink / raw)
To: pbonzini; +Cc: git, bfields
pbonzini@redhat.com writes:
> From: Paolo Bonzini <pbonzini@redhat.com>
>
> OPTION_CMDMODE is essentially OPTION_SET_INT plus the extra check
> that the variable had not set before. In order to allow custom
> processing, change it to OPTION_SET_INT plus a new flag that takes
> care of the check. This works as long as the option value points
> to an int.
It is unclear but I am guessing that the purpose of this change is
to make "only one of these" orthgonal to "the value of this option
is an int", in preparation to allow options other than SET_INT to
also be combined with "only one of these"?
If my reading is not correct, that would be an indication that the
above paragraph does not tell what it wants to to readers.
It is unclear at this step what other kind of option the flag wants
to be combined, though.
> diff --git a/parse-options.c b/parse-options.c
> index a0cef401fc..c6e9e2733b 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -61,7 +61,7 @@ static enum parse_opt_result opt_command_mode_error(
> */
> for (that = all_opts; that->type != OPTION_END; that++) {
> if (that == opt ||
> - that->type != OPTION_CMDMODE ||
> + !(that->flags & PARSE_OPT_CMDMODE) ||
> that->value != opt->value ||
> that->defval != *(int *)opt->value)
> continue;
> @@ -95,6 +95,14 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
> if (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG))
> return error(_("%s takes no value"), optname(opt, flags));
>
> + /*
> + * Giving the same mode option twice, although unnecessary,
> + * is not a grave error, so let it pass.
> + */
> + if ((opt->flags & PARSE_OPT_CMDMODE) &&
> + *(int *)opt->value && *(int *)opt->value != opt->defval)
> + return opt_command_mode_error(opt, all_opts, flags);
> +
... and when there is no error, we fall through and process it as a
regular SET_INT, which makes sense.
> @@ -168,8 +168,8 @@ struct option {
> #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)
> #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
> (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}
> -#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \
> - (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }
> +#define OPT_CMDMODE(s, l, v, h, i) { OPTION_SET_INT, (s), (l), (v), NULL, \
> + (h), PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }
OK.
> diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
> index af82db06ac..2051ce57db 100644
> --- a/t/helper/test-parse-options.c
> +++ b/t/helper/test-parse-options.c
> @@ -121,6 +121,8 @@ int cmd__parse_options(int argc, const char **argv)
> OPT_INTEGER('j', NULL, &integer, "get a integer, too"),
> OPT_MAGNITUDE('m', "magnitude", &magnitude, "get a magnitude"),
> OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23),
> + OPT_CMDMODE(0, "mode1", &integer, "set integer to 1 (cmdmode option)", 1),
> + OPT_CMDMODE(0, "mode2", &integer, "set integer to 2 (cmdmode option)", 2),
> OPT_CALLBACK('L', "length", &integer, "str",
> "get length of <str>", length_callback),
> OPT_FILENAME('F', "file", &file, "set file to <file>"),
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index 9d7c7fdaa2..7f4c15a52b 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -23,6 +23,8 @@ usage: test-tool parse-options <options>
> -j <n> get a integer, too
> -m, --magnitude <n> get a magnitude
> --set23 set integer to 23
> + --mode1 set integer to 1 (cmdmode option)
> + --mode2 set integer to 2 (cmdmode option)
> -L, --length <str> get length of <str>
> -F, --file <file> set file to <file>
>
> @@ -324,6 +326,22 @@ test_expect_success 'OPT_NEGBIT() works' '
> test-tool parse-options --expect="boolean: 6" -bb --no-neg-or4
> '
>
> +test_expect_success 'OPT_CMDMODE() works' '
> + test-tool parse-options --expect="integer: 1" --mode1
> +'
> +
> +test_expect_success 'OPT_CMDMODE() detects incompatibility' '
> + test_must_fail test-tool parse-options --mode1 --mode2 >output 2>output.err &&
> + test_must_be_empty output &&
> + grep "incompatible with --mode" output.err
> +'
> +
> +test_expect_success 'OPT_CMDMODE() detects incompatibility with something else' '
> + test_must_fail test-tool parse-options --set23 --mode2 >output 2>output.err &&
> + test_must_be_empty output &&
> + grep "incompatible with something else" output.err
> +'
> +
> test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '
> test-tool parse-options --expect="boolean: 6" + + + + + +
> '
Would the updated test-parse-options.c with these three new tests
work the same way with or without changes to parse-options.[ch]?
That would be a good indication that the change to the code is
"upward compatible".
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/4] parse-options: convert "command mode" to a flag
2020-02-19 19:15 ` Junio C Hamano
@ 2020-02-19 21:05 ` Paolo Bonzini
0 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2020-02-19 21:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, bfields
On 19/02/20 20:15, Junio C Hamano wrote:
>> OPTION_CMDMODE is essentially OPTION_SET_INT plus the extra check
>> that the variable had not set before. In order to allow custom
>> processing, change it to OPTION_SET_INT plus a new flag that takes
>> care of the check. This works as long as the option value points
>> to an int.
> It is unclear but I am guessing that the purpose of this change is
> to make "only one of these" orthgonal to "the value of this option
> is an int", in preparation to allow options other than SET_INT to
> also be combined with "only one of these"?
>
> If my reading is not correct, that would be an indication that the
> above paragraph does not tell what it wants to to readers.
Your reading and your conclusion are both correct. I'll reword the
commit message.
Paolo
> It is unclear at this step what other kind of option the flag wants
> to be combined, though.
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/4] am: convert "resume" variable to a struct
2020-02-19 16:13 [PATCH 0/4] am: provide a replacement for "cat .git/rebase-apply/patch" pbonzini
2020-02-19 16:13 ` [PATCH 1/4] parse-options: convert "command mode" to a flag pbonzini
@ 2020-02-19 16:13 ` pbonzini
2020-02-19 19:19 ` Junio C Hamano
2020-02-19 16:13 ` [PATCH 3/4] am: support --show-current-patch=raw as a synonym for--show-current-patch pbonzini
2020-02-19 16:13 ` [PATCH 4/4] am: support --show-current-patch=diff to retrieve .git/rebase-apply/patch pbonzini
3 siblings, 1 reply; 15+ messages in thread
From: pbonzini @ 2020-02-19 16:13 UTC (permalink / raw)
To: git; +Cc: bfields
From: Paolo Bonzini <pbonzini@redhat.com>
This will allow stashing the submode of --show-current-patch. Using
a struct will allow accessing both fields from outside cmd_am (through
container_of).
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
builtin/am.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 8181c2aef3..a89e1a96ed 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2118,7 +2118,7 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, int
return 0;
}
-enum resume_mode {
+enum resume_type {
RESUME_FALSE = 0,
RESUME_APPLY,
RESUME_RESOLVED,
@@ -2128,6 +2128,10 @@ enum resume_mode {
RESUME_SHOW_PATCH
};
+struct resume_mode {
+ enum resume_type mode;
+};
+
static int git_am_config(const char *k, const char *v, void *cb)
{
int status;
@@ -2145,7 +2149,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
int binary = -1;
int keep_cr = -1;
int patch_format = PATCH_FORMAT_UNKNOWN;
- enum resume_mode resume = RESUME_FALSE;
+ struct resume_mode resume = { . mode = RESUME_FALSE };
int in_progress;
int ret = 0;
@@ -2214,22 +2218,22 @@ int cmd_am(int argc, const char **argv, const char *prefix)
PARSE_OPT_NOARG),
OPT_STRING(0, "resolvemsg", &state.resolvemsg, NULL,
N_("override error message when patch failure occurs")),
- OPT_CMDMODE(0, "continue", &resume,
+ OPT_CMDMODE(0, "continue", &resume.mode,
N_("continue applying patches after resolving a conflict"),
RESUME_RESOLVED),
- OPT_CMDMODE('r', "resolved", &resume,
+ OPT_CMDMODE('r', "resolved", &resume.mode,
N_("synonyms for --continue"),
RESUME_RESOLVED),
- OPT_CMDMODE(0, "skip", &resume,
+ OPT_CMDMODE(0, "skip", &resume.mode,
N_("skip the current patch"),
RESUME_SKIP),
- OPT_CMDMODE(0, "abort", &resume,
+ OPT_CMDMODE(0, "abort", &resume.mode,
N_("restore the original branch and abort the patching operation."),
RESUME_ABORT),
- OPT_CMDMODE(0, "quit", &resume,
+ 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,
+ OPT_CMDMODE(0, "show-current-patch", &resume.mode,
N_("show the patch being applied."),
RESUME_SHOW_PATCH),
OPT_BOOL(0, "committer-date-is-author-date",
@@ -2281,12 +2285,12 @@ int cmd_am(int argc, const char **argv, const char *prefix)
* intend to feed us a patch but wanted to continue
* unattended.
*/
- if (argc || (resume == RESUME_FALSE && !isatty(0)))
+ if (argc || (resume.mode == RESUME_FALSE && !isatty(0)))
die(_("previous rebase directory %s still exists but mbox given."),
state.dir);
- if (resume == RESUME_FALSE)
- resume = RESUME_APPLY;
+ if (resume.mode == RESUME_FALSE)
+ resume.mode = RESUME_APPLY;
if (state.signoff == SIGNOFF_EXPLICIT)
am_append_signoff(&state);
@@ -2300,7 +2304,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
* stray directories.
*/
if (file_exists(state.dir) && !state.rebasing) {
- if (resume == RESUME_ABORT || resume == RESUME_QUIT) {
+ if (resume.mode == RESUME_ABORT || resume.mode == RESUME_QUIT) {
am_destroy(&state);
am_state_release(&state);
return 0;
@@ -2311,7 +2315,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
state.dir);
}
- if (resume)
+ if (resume.mode)
die(_("Resolve operation not in progress, we are not resuming."));
for (i = 0; i < argc; i++) {
@@ -2329,7 +2333,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
argv_array_clear(&paths);
}
- switch (resume) {
+ switch (resume.mode) {
case RESUME_FALSE:
am_run(&state, 0);
break;
--
2.21.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 2/4] am: convert "resume" variable to a struct
2020-02-19 16:13 ` [PATCH 2/4] am: convert "resume" variable to a struct pbonzini
@ 2020-02-19 19:19 ` Junio C Hamano
2020-02-19 21:05 ` Paolo Bonzini
0 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2020-02-19 19:19 UTC (permalink / raw)
To: pbonzini; +Cc: git, bfields
pbonzini@redhat.com writes:
> From: Paolo Bonzini <pbonzini@redhat.com>
>
> This will allow stashing the submode of --show-current-patch. Using
> a struct will allow accessing both fields from outside cmd_am (through
> container_of).
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> builtin/am.c | 32 ++++++++++++++++++--------------
> 1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 8181c2aef3..a89e1a96ed 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2118,7 +2118,7 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, int
> return 0;
> }
>
> -enum resume_mode {
> +enum resume_type {
> RESUME_FALSE = 0,
> RESUME_APPLY,
> RESUME_RESOLVED,
> @@ -2128,6 +2128,10 @@ enum resume_mode {
> RESUME_SHOW_PATCH
> };
>
> +struct resume_mode {
> + enum resume_type mode;
> +};
> +
> static int git_am_config(const char *k, const char *v, void *cb)
> {
> int status;
> @@ -2145,7 +2149,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
> int binary = -1;
> int keep_cr = -1;
> int patch_format = PATCH_FORMAT_UNKNOWN;
> - enum resume_mode resume = RESUME_FALSE;
> + struct resume_mode resume = { . mode = RESUME_FALSE };
I do not think it makes a difference to compilers, but it seems that
existing code spells this without SP between dot and the field name.
> + struct resume_mode resume = { .mode = RESUME_FALSE };
The rest of the patch is quite straight-forward update that looks
correct.
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/4] am: convert "resume" variable to a struct
2020-02-19 19:19 ` Junio C Hamano
@ 2020-02-19 21:05 ` Paolo Bonzini
0 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2020-02-19 21:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, bfields
On 19/02/20 20:19, Junio C Hamano wrote:
>> - enum resume_mode resume = RESUME_FALSE;
>> + struct resume_mode resume = { . mode = RESUME_FALSE };
> I do not think it makes a difference to compilers, but it seems that
> existing code spells this without SP between dot and the field name.
Yes, it's a typo.
Paolo
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/4] am: support --show-current-patch=raw as a synonym for--show-current-patch
2020-02-19 16:13 [PATCH 0/4] am: provide a replacement for "cat .git/rebase-apply/patch" pbonzini
2020-02-19 16:13 ` [PATCH 1/4] parse-options: convert "command mode" to a flag pbonzini
2020-02-19 16:13 ` [PATCH 2/4] am: convert "resume" variable to a struct pbonzini
@ 2020-02-19 16:13 ` pbonzini
2020-02-19 19:34 ` Eric Sunshine
2020-02-19 16:13 ` [PATCH 4/4] am: support --show-current-patch=diff to retrieve .git/rebase-apply/patch pbonzini
3 siblings, 1 reply; 15+ messages in thread
From: pbonzini @ 2020-02-19 16:13 UTC (permalink / raw)
To: git; +Cc: bfields
From: Paolo Bonzini <pbonzini@redhat.com>
When "git am --show-current-patch" was added in commit 984913a210 ("am:
add --show-current-patch", 2018-02-12), "git am" started recommending it
as a replacement for .git/rebase-merge/patch. Unfortunately the suggestion
is misguided, for example the output "git am --show-current-patch" cannot
be passed to "git apply" if it is encoded as quoted-printable or base64.
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
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>
---
Documentation/git-am.txt | 4 +-
builtin/am.c | 59 +++++++++++++++++++++++---
contrib/completion/git-completion.bash | 5 +++
t/t4150-am.sh | 10 +++++
4 files changed, 70 insertions(+), 8 deletions(-)
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 &&
--
2.21.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 3/4] am: support --show-current-patch=raw as a synonym for--show-current-patch
2020-02-19 16:13 ` [PATCH 3/4] am: support --show-current-patch=raw as a synonym for--show-current-patch pbonzini
@ 2020-02-19 19:34 ` Eric Sunshine
2020-02-19 20:17 ` Junio C Hamano
0 siblings, 1 reply; 15+ messages in thread
From: Eric Sunshine @ 2020-02-19 19:34 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Git List, bfields
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;
> +}
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/4] am: support --show-current-patch=raw as a synonym for--show-current-patch
2020-02-19 19:34 ` Eric Sunshine
@ 2020-02-19 20:17 ` Junio C Hamano
2020-02-19 20:53 ` Paolo Bonzini
0 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2020-02-19 20:17 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Paolo Bonzini, Git List, bfields
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.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/4] am: support --show-current-patch=raw as a synonym for--show-current-patch
2020-02-19 20:17 ` Junio C Hamano
@ 2020-02-19 20:53 ` Paolo Bonzini
0 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2020-02-19 20:53 UTC (permalink / raw)
To: Junio C Hamano, Eric Sunshine; +Cc: Git List, bfields
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/4] am: support --show-current-patch=diff to retrieve .git/rebase-apply/patch
2020-02-19 16:13 [PATCH 0/4] am: provide a replacement for "cat .git/rebase-apply/patch" pbonzini
` (2 preceding siblings ...)
2020-02-19 16:13 ` [PATCH 3/4] am: support --show-current-patch=raw as a synonym for--show-current-patch pbonzini
@ 2020-02-19 16:13 ` pbonzini
2020-02-19 19:49 ` Eric Sunshine
3 siblings, 1 reply; 15+ messages in thread
From: pbonzini @ 2020-02-19 16:13 UTC (permalink / raw)
To: git; +Cc: bfields
From: Paolo Bonzini <pbonzini@redhat.com>
When "git am --show-current-patch" was added in commit 984913a210 ("am:
add --show-current-patch", 2018-02-12), "git am" started recommending it
as a replacement for .git/rebase-merge/patch. Unfortunately the suggestion
is misguided, for example the output "git am --show-current-patch" cannot
be passed to "git apply" if it is encoded as quoted-printable or base64.
Add a new mode to "git am --show-current-patch" in order to straighten
the suggestion.
Reported-by: J. Bruce Fields <bfields@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
Documentation/git-am.txt | 10 ++++++----
builtin/am.c | 7 ++++++-
contrib/completion/git-completion.bash | 2 +-
t/t4150-am.sh | 10 ++++++++++
4 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index bafb491ede..363d6ff665 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[=raw])
+'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=raw|diff])
DESCRIPTION
-----------
@@ -176,9 +176,11 @@ default. You can use `--no-utf8` to override this.
Abort the patching operation but keep HEAD and the index
untouched.
---show-current-patch[=raw]::
- Show the entire e-mail message "git am" has stopped at, because
- of conflicts.
+--show-current-patch[=raw|diff]::
+ Show the message "git am" has stopped at, because of conflicts.
+ If the argument is absent or "raw", show the raw contents of
+ the e-mail message. If the argument is "diff", show the diff
+ portion only.
DISCUSSION
----------
diff --git a/builtin/am.c b/builtin/am.c
index ec4c743556..ad543882ed 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -83,6 +83,7 @@ enum signoff_type {
enum show_patch_type {
SHOW_PATCH_RAW = 0,
+ SHOW_PATCH_DIFF = 1,
};
struct am_state {
@@ -1767,7 +1768,7 @@ static void am_run(struct am_state *state, int resume)
linelen(state->msg), state->msg);
if (advice_amworkdir)
- advise(_("Use 'git am --show-current-patch' to see the failed patch"));
+ advise(_("Use 'git am --show-current-patch=diff' to see the failed patch"));
die_user_resolve(state);
}
@@ -2086,6 +2087,9 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
case SHOW_PATCH_RAW:
patch_path = am_path(state, msgnum(state));
break;
+ case SHOW_PATCH_DIFF:
+ patch_path = am_path(state, "patch");
+ break;
default:
abort();
}
@@ -2154,6 +2158,7 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
* when you add new options
*/
const char *valid_modes[] = {
+ [SHOW_PATCH_DIFF] = "diff",
[SHOW_PATCH_RAW] = "raw"
};
int new_value = SHOW_PATCH_RAW;
@@ -2284,7 +2288,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
N_("abort the patching operation but keep HEAD where it is."),
RESUME_QUIT),
{ OPTION_CALLBACK, 0, "show-current-patch", &resume.mode,
- "raw",
+ "diff|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 },
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 247f34f1fa..1151697f01 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1197,7 +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_showcurrentpatch="raw diff"
__git_am_inprogress_options="--skip --continue --resolved --abort --quit --show-current-patch"
_git_am ()
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index afe456e75e..cb45271457 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -671,11 +671,21 @@ test_expect_success 'am --show-current-patch=raw' '
test_cmp .git/rebase-apply/0001 actual.patch
'
+test_expect_success 'am --show-current-patch=diff' '
+ git am --show-current-patch=diff >actual.patch &&
+ test_cmp .git/rebase-apply/patch 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 detects incompatible --show-current-patch' '
+ test_must_fail git am --show-current-patch=raw --show-current-patch=diff &&
+ test_must_fail git am --show-current-patch --show-current-patch=diff
+'
+
test_expect_success 'am --skip works' '
echo goodbye >expected &&
git am --skip &&
--
2.21.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 4/4] am: support --show-current-patch=diff to retrieve .git/rebase-apply/patch
2020-02-19 16:13 ` [PATCH 4/4] am: support --show-current-patch=diff to retrieve .git/rebase-apply/patch pbonzini
@ 2020-02-19 19:49 ` Eric Sunshine
0 siblings, 0 replies; 15+ messages in thread
From: Eric Sunshine @ 2020-02-19 19:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Git List, bfields
On Wed, Feb 19, 2020 at 11:15 AM <pbonzini@redhat.com> wrote:
> When "git am --show-current-patch" was added in commit 984913a210 ("am:
> add --show-current-patch", 2018-02-12), "git am" started recommending it
> as a replacement for .git/rebase-merge/patch. Unfortunately the suggestion
> is misguided, for example the output "git am --show-current-patch" cannot
> be passed to "git apply" if it is encoded as quoted-printable or base64.
> Add a new mode to "git am --show-current-patch" in order to straighten
> the suggestion.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
> @@ -16,7 +16,7 @@ SYNOPSIS
> -'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=raw])
> +'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=raw|diff])
Missing parentheses. To be consistent with other documentation, this
should be written as:
--show-current-patch[=(raw|diff)]
> @@ -176,9 +176,11 @@ default. You can use `--no-utf8` to override this.
> ---show-current-patch[=raw]::
> - Show the entire e-mail message "git am" has stopped at, because
> - of conflicts.
> +--show-current-patch[=raw|diff]::
Ditto: --show-current-patch[=(raw|diff)]::
> + Show the message "git am" has stopped at, because of conflicts.
The weirdly-placed comma is still weird.
> + If the argument is absent or "raw", show the raw contents of
> + the e-mail message. If the argument is "diff", show the diff
> + portion only.
I think the usual term is "omitted" rather than "absent".
Suggested rewrite:
Show the message at which `git am` has stopped due to conflicts.
If `raw` is specified, show the raw contents of the e-mail
message; if `diff`, show the diff portion only. Defaults to `raw`.
This also simplifies the change if the default ever flips from "raw" to "diff".
^ permalink raw reply [flat|nested] 15+ messages in thread