threads / patch / 52840

patch, 4 partsam: provide a replacement for "cat .git/rebase-apply/patch"

Subject: [PATCH 0/4] am: provide a replacement for "cat .git/rebase-apply/patch"

## tl;dr

15 messages between Feb 19, 2020 and Feb 20, 2020. Diffs are folded; open one to read it.

replies: 14people: 4as markdown or json

pbonzini@redhat.com· Feb 19, 2020, 16:13 UTC · lore
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.

This series adds a new mode to "git am --show-current-patch" in order to straighten the suggestion. "--show-current-patch" grows an optional argument, where the default behavior can now also be obtained with "--show-current-patch=raw" and ".git/rebase-apply/patch" can be retrieved with "--show-current-patch=diff".

This requires a little surgery in patches 1 and 2 in order to convert --show-current-patch from OPTION_CMDMODE to OPTION_CALLBACK. After this, the last two patches implement the new syntax and feature.

Thanks,
Paolo
Paolo Bonzini (4):
  parse-options: convert "command mode" to a flag
  am: convert "resume" variable to a struct
  am: support --show-current-patch=raw as a synonym
    for--show-current-patch
  am: support --show-current-patch=diff to retrieve
    .git/rebase-apply/patch
 Documentation/git-am.txt               | 10 +--
 builtin/am.c                           | 96 ++++++++++++++++++++------
 contrib/completion/git-completion.bash |  5 ++
 parse-options.c                        | 20 +++---
 parse-options.h                        |  8 +--
 t/helper/test-parse-options.c          |  2 +
 t/t0040-parse-options.sh               | 18 +++++
 t/t4150-am.sh                          | 20 ++++++
 8 files changed, 140 insertions(+), 39 deletions(-)
-- 
2.21.1
pbonzini@redhat.com· Feb 19, 2020, 16:13 UTC · re: pbonzini@redhat.com · lore

[PATCH 1/4] parse-options: convert "command mode" to a flag

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(-)
Show changes to 4 files +33 −15

parse-options.c, parse-options.h, t/helper/test-parse-options.c, t/t0040-parse-options.sh

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
Eric Sunshine· Feb 19, 2020, 19:11 UTC · re: pbonzini@redhat.com · lore

Re: [PATCH 1/4] parse-options: convert "command mode" to a flag

On Wed, Feb 19, 2020 at 11:15 AM <pbonzini@redhat.com> wrote:
Show 15 quoted lines
> 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
Show 6 quoted lines
> +
> +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.
Johannes Schindelin· Feb 20, 2020, 16:00 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/4] parse-options: convert "command mode" to a flag

Hi Eric,
On Wed, 19 Feb 2020, Eric Sunshine wrote:
Show 21 quoted lines
> 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

Show 11 quoted lines
>
> > +
> > +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.
>
>
Junio C Hamano· Feb 19, 2020, 19:15 UTC · re: pbonzini@redhat.com · lore

Re: [PATCH 1/4] parse-options: convert "command mode" to a flag

pbonzini@redhat.com writes:
Show 7 quoted lines
> 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.

Show 25 quoted lines
> 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.

Show 8 quoted lines
> @@ -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.
Show 49 quoted lines
> 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.
Paolo Bonzini· Feb 19, 2020, 21:05 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/4] parse-options: convert "command mode" to a flag

On 19/02/20 20:15, Junio C Hamano wrote:
Show 12 quoted lines
>> 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.
> 
pbonzini@redhat.com· Feb 19, 2020, 16:13 UTC · re: pbonzini@redhat.com · lore

[PATCH 2/4] am: convert "resume" variable to a struct

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(-)
Show changes to builtin/am.c +18 −14
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
Junio C Hamano· Feb 19, 2020, 19:19 UTC · re: pbonzini@redhat.com · lore

Re: [PATCH 2/4] am: convert "resume" variable to a struct

pbonzini@redhat.com writes:
Show 41 quoted lines
> 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.
Paolo Bonzini· Feb 19, 2020, 21:05 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/4] am: convert "resume" variable to a struct

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
pbonzini@redhat.com· Feb 19, 2020, 16:13 UTC · re: pbonzini@redhat.com · lore

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

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(-)
Show changes to 4 files +70 −7

Documentation/git-am.txt, builtin/am.c, contrib/completion/git-completion.bash, t/t4150-am.sh

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
Eric Sunshine· Feb 19, 2020, 19:34 UTC · re: pbonzini@redhat.com · lore

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

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/
Show 17 quoted lines
> 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.

Show 13 quoted lines
> @@ -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;
> +}
Junio C Hamano· Feb 19, 2020, 20:17 UTC · re: Eric Sunshine · lore

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

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.
Paolo Bonzini· Feb 19, 2020, 20:53 UTC · re: Junio C Hamano · lore

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

On 19/02/20 21:17, Junio C Hamano wrote:
Show 15 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?

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
pbonzini@redhat.com· Feb 19, 2020, 16:13 UTC · re: pbonzini@redhat.com · lore

[PATCH 4/4] am: support --show-current-patch=diff to retrieve .git/rebase-apply/patch

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(-)
Show changes to 4 files +24 −6

Documentation/git-am.txt, builtin/am.c, contrib/completion/git-completion.bash, t/t4150-am.sh

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
Eric Sunshine· Feb 19, 2020, 19:49 UTC · re: pbonzini@redhat.com · lore

Re: [PATCH 4/4] am: support --show-current-patch=diff to retrieve .git/rebase-apply/patch

On Wed, Feb 19, 2020 at 11:15 AM <pbonzini@redhat.com> wrote:
Show 14 quoted lines
> 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)]
Show 5 quoted lines
> @@ -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".

← back to recent threads