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

git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed

* [PATCH v2 0/5] am: provide a replacement for "cat .git/rebase-apply/patch"
@ 2020-02-20 14:15 pbonzini
  2020-02-20 14:15 ` [PATCH v2 1/5] parse-options: add testcases for OPT_CMDMODE() pbonzini
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: pbonzini @ 2020-02-20 14:15 UTC (permalink / raw)
  To: git; +Cc: sunshine

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

v1->v2: - split testcases to a separate patch [Junio]
	- improve commit messages [Junio]
	- fix spacing in designated initializer [Junio]
	- use test_i18ngrep [Eric]
	- replace abort with BUG [Eric]
	- replace "diff|raw" with "(diff|raw)" in docs and help [Eric]
	- improve docs wording [Eric]

Paolo Bonzini (5):
  parse-options: add testcases for OPT_CMDMODE()
  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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/5] parse-options: add testcases for OPT_CMDMODE()
  2020-02-20 14:15 [PATCH v2 0/5] am: provide a replacement for "cat .git/rebase-apply/patch" pbonzini
@ 2020-02-20 14:15 ` pbonzini
  2020-02-20 14:15 ` [PATCH v2 2/5] parse-options: convert "command mode" to a flag pbonzini
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: pbonzini @ 2020-02-20 14:15 UTC (permalink / raw)
  To: git; +Cc: sunshine

From: Paolo Bonzini <pbonzini@redhat.com>

Before modifying the implementation, ensure that general operation of
OPT_CMDMODE() and detection of incompatible options are covered.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
v1->v2: - split testcases to a separate patch [Junio]
	- use test_i18ngrep [Eric]

 t/helper/test-parse-options.c |  2 ++
 t/t0040-parse-options.sh      | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+)

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..3483b72db4 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 &&
+	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 &&
+	test_i18ngrep "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] 6+ messages in thread

* [PATCH v2 2/5] parse-options: convert "command mode" to a flag
  2020-02-20 14:15 [PATCH v2 0/5] am: provide a replacement for "cat .git/rebase-apply/patch" pbonzini
  2020-02-20 14:15 ` [PATCH v2 1/5] parse-options: add testcases for OPT_CMDMODE() pbonzini
@ 2020-02-20 14:15 ` pbonzini
  2020-02-20 14:15 ` [PATCH v2 3/5] am: convert "resume" variable to a struct pbonzini
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: pbonzini @ 2020-02-20 14:15 UTC (permalink / raw)
  To: git; +Cc: sunshine

From: Paolo Bonzini <pbonzini@redhat.com>

OPTION_CMDMODE is essentially OPTION_SET_INT plus an extra check that
the variable had not set before.  In order to allow custom processing
of the option, for example a "command mode" option that also has an
argument, it would be nice to use OPTION_CALLBACK and not have to rewrite
the extra check on incompatible options.  In other words, making the
processing of the option orthogonal to the "only one of these" behavior
provided by OPTION_CMDMODE.

Add a new flag that takes care of the check, and modify OPT_CMDMODE to
use it together with OPTION_SET_INT.  The new flag still requires that the
option value points to an int, but any OPTION_* value can be specified as
long as it does not require a non-int type for opt->value.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
v1->v2: - improve commit message [Junio]

 parse-options.c | 20 +++++++++-----------
 parse-options.h |  8 ++++----
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index a0cef401fc..63d6bab60c 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 }
-- 
2.21.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 3/5] am: convert "resume" variable to a struct
  2020-02-20 14:15 [PATCH v2 0/5] am: provide a replacement for "cat .git/rebase-apply/patch" pbonzini
  2020-02-20 14:15 ` [PATCH v2 1/5] parse-options: add testcases for OPT_CMDMODE() pbonzini
  2020-02-20 14:15 ` [PATCH v2 2/5] parse-options: convert "command mode" to a flag pbonzini
@ 2020-02-20 14:15 ` pbonzini
  2020-02-20 14:15 ` [PATCH v2 4/5] am: support --show-current-patch=raw as a synonym for--show-current-patch pbonzini
  2020-02-20 14:15 ` [PATCH v2 5/5] am: support --show-current-patch=diff to retrieve .git/rebase-apply/patch pbonzini
  4 siblings, 0 replies; 6+ messages in thread
From: pbonzini @ 2020-02-20 14:15 UTC (permalink / raw)
  To: git; +Cc: sunshine

From: Paolo Bonzini <pbonzini@redhat.com>

This will allow stashing the submode of --show-current-patch from a
callback function.  Using a struct will allow accessing both fields from
outside cmd_am (through container_of).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
v1->v2: - fix spacing in designated initializer [Junio]

 builtin/am.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 8181c2aef3..bd3cda8bec 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] 6+ messages in thread

* [PATCH v2 4/5] am: support --show-current-patch=raw as a synonym for--show-current-patch
  2020-02-20 14:15 [PATCH v2 0/5] am: provide a replacement for "cat .git/rebase-apply/patch" pbonzini
                   ` (2 preceding siblings ...)
  2020-02-20 14:15 ` [PATCH v2 3/5] am: convert "resume" variable to a struct pbonzini
@ 2020-02-20 14:15 ` pbonzini
  2020-02-20 14:15 ` [PATCH v2 5/5] am: support --show-current-patch=diff to retrieve .git/rebase-apply/patch pbonzini
  4 siblings, 0 replies; 6+ messages in thread
From: pbonzini @ 2020-02-20 14:15 UTC (permalink / raw)
  To: git; +Cc: sunshine

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 somewhat 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.  To simplify worktree operations and to avoid that users poke into
.git, it would be better if "git am" also provided a mode that copies
.git/rebase-merge/patch to stdout.

One possibility could be to have completely separate options, introducing
for example --show-current-message (for .git/rebase-apply/NNNN)
and --show-current-diff (for .git/rebase-apply/patch), while possibly
deprecating --show-current-patch.

That would even remove 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".  Therefore, I chose instead to add a string
argument to --show-current-patch.  The new argument is optional, so that
"git am --show-current-patch"'s behavior remains backwards-compatible.

The next choice to make is how to handle multiple --show-current-patch
options.  Right now, something like "git am --abort --show-current-patch"
is rejected, and the previous suggestion would likewise have naturally
rejected a command line like

	git am --show-current-message --show-current-diff

Therefore, I decided to also reject for example

	git am --show-current-patch=diff --show-current-patch=raw

In other words the whole of --show-current-patch=xxx (including the
optional argument) is treated as the command mode.  I found this to be
more consistent and intuitive, even though it differs from the usual
"last one wins" semantics of the git command line.

Add the code to parse submodes based on the above design, where for now
"raw" is the only valid submode.  "raw" prints the full e-mail message
just like "git am --show-current-patch".

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
v1->v2: - improve commit messages [Junio]
	- replace abort with BUG [Eric]

 Documentation/git-am.txt               |  9 ++--
 builtin/am.c                           | 59 +++++++++++++++++++++++---
 contrib/completion/git-completion.bash |  5 +++
 t/t4150-am.sh                          | 10 +++++
 4 files changed, 73 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 11ca61b00b..590b711536 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,9 +176,10 @@ default.   You can use `--no-utf8` to override this.
 	Abort the patching operation but keep HEAD and the index
 	untouched.
 
---show-current-patch::
-	Show the entire e-mail message "git am" has stopped at, because
-	of conflicts.
+--show-current-patch[=raw]::
+	Show the raw contents of the e-mail message at which `git am`
+	has stopped due to conflicts.  The argument must be omitted or
+	`raw`.
 
 DISCUSSION
 ----------
diff --git a/builtin/am.c b/builtin/am.c
index bd3cda8bec..54b04da86d 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:
+		BUG("invalid mode for --show-current-patch");
+	}
+
 	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] 6+ messages in thread

* [PATCH v2 5/5] am: support --show-current-patch=diff to retrieve .git/rebase-apply/patch
  2020-02-20 14:15 [PATCH v2 0/5] am: provide a replacement for "cat .git/rebase-apply/patch" pbonzini
                   ` (3 preceding siblings ...)
  2020-02-20 14:15 ` [PATCH v2 4/5] am: support --show-current-patch=raw as a synonym for--show-current-patch pbonzini
@ 2020-02-20 14:15 ` pbonzini
  4 siblings, 0 replies; 6+ messages in thread
From: pbonzini @ 2020-02-20 14:15 UTC (permalink / raw)
  To: git; +Cc: sunshine

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 somewhat misguided; for example, the output of "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>
---
v1->v2: - replace "diff|raw" with "(diff|raw)" in docs and help [Eric]
	- improve docs wording [Eric]

 Documentation/git-am.txt               | 11 ++++++-----
 builtin/am.c                           |  9 +++++++--
 contrib/completion/git-completion.bash |  2 +-
 t/t4150-am.sh                          | 10 ++++++++++
 4 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 590b711536..ab5754e05d 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[=(diff|raw)])
 
 DESCRIPTION
 -----------
@@ -176,10 +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 raw contents of the e-mail message at which `git am`
-	has stopped due to conflicts.  The argument must be omitted or
-	`raw`.
+--show-current-patch[=(diff|raw)]::
+	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`.
 
 DISCUSSION
 ----------
diff --git a/builtin/am.c b/builtin/am.c
index 54b04da86d..e3dfd93c25 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:
 		BUG("invalid mode for --show-current-patch");
 	}
@@ -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;
@@ -2279,7 +2284,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="diff raw"
 __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] 6+ messages in thread

end of thread, other threads:[~2020-02-20 14:15 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-20 14:15 [PATCH v2 0/5] am: provide a replacement for "cat .git/rebase-apply/patch" pbonzini
2020-02-20 14:15 ` [PATCH v2 1/5] parse-options: add testcases for OPT_CMDMODE() pbonzini
2020-02-20 14:15 ` [PATCH v2 2/5] parse-options: convert "command mode" to a flag pbonzini
2020-02-20 14:15 ` [PATCH v2 3/5] am: convert "resume" variable to a struct pbonzini
2020-02-20 14:15 ` [PATCH v2 4/5] am: support --show-current-patch=raw as a synonym for--show-current-patch pbonzini
2020-02-20 14:15 ` [PATCH v2 5/5] am: support --show-current-patch=diff to retrieve .git/rebase-apply/patch pbonzini

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).