{"thread":{"id":"52840","subject":"[PATCH 0/4] am: provide a replacement for \"cat .git/rebase-apply/patch\"","startedAt":"2020-02-19T16:14:03Z","lastAt":"2020-02-20T16:01:01Z","messageCount":15,"participants":["pbonzini@redhat.com","Eric Sunshine","Junio C Hamano","Paolo Bonzini","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"392063","messageId":"20200219161352.13562-1-pbonzini@redhat.com","threadId":"52840","inReplyTo":null,"subject":"[PATCH 0/4] am: provide a replacement for \"cat .git/rebase-apply/patch\"","fromName":"","fromEmail":"pbonzini@redhat.com","sentAt":"2020-02-19T16:13:48Z","receivedAt":"2020-02-19T16:14:03Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"From: Paolo Bonzini \n\nWhen \"git am --show-current-patch\" was added in commit 984913a210 (\"am:\nadd --show-current-patch\", 2018-02-12), \"git am\" started recommending it\nas a replacement for .git/rebase-merge/patch. Unfortunately the suggestion\nis misguided; for example, the output \"git am --show-current-patch\" cannot\nbe passed to \"git apply\" if it is encoded as quoted-printable or base64.\n\nThis series adds a new mode to \"git am --show-current-patch\" in order to\nstraighten the suggestion. \"--show-current-patch\" grows an optional\nargument, where the default behavior can now also be obtained with\n\"--show-current-patch=raw\" and \".git/rebase-apply/patch\" can be retrieved\nwith \"--show-current-patch=diff\".\n\nThis requires a little surgery in patches 1 and 2 in order to convert\n--show-current-patch from OPTION_CMDMODE to OPTION_CALLBACK. After this,\nthe last two patches implement the new syntax and feature.\n\nThanks,\n\nPaolo\n\nPaolo Bonzini (4):\n parse-options: convert \"command mode\" to a flag\n am: convert \"resume\" variable to a struct\n am: support --show-current-patch=raw as a synonym\n for--show-current-patch\n am: support --show-current-patch=diff to retrieve\n .git/rebase-apply/patch\n\n Documentation/git-am.txt | 10 +--\n builtin/am.c | 96 ++++++++++++++++++++------\n contrib/completion/git-completion.bash | 5 ++\n parse-options.c | 20 +++---\n parse-options.h | 8 +--\n t/helper/test-parse-options.c | 2 +\n t/t0040-parse-options.sh | 18 +++++\n t/t4150-am.sh | 20 ++++++\n 8 files changed, 140 insertions(+), 39 deletions(-)\n\n-- \n2.21.1\n\n"},{"id":"392064","messageId":"20200219161352.13562-2-pbonzini@redhat.com","threadId":"52840","inReplyTo":"20200219161352.13562-1-pbonzini@redhat.com","subject":"[PATCH 1/4] parse-options: convert \"command mode\" to a flag","fromName":"","fromEmail":"pbonzini@redhat.com","sentAt":"2020-02-19T16:13:49Z","receivedAt":"2020-02-19T16:14:06Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"From: Paolo Bonzini \n\nOPTION_CMDMODE is essentially OPTION_SET_INT plus the extra check\nthat the variable had not set before. In order to allow custom\nprocessing, change it to OPTION_SET_INT plus a new flag that takes\ncare of the check. This works as long as the option value points\nto an int.\n\nAdd testcases while at it.\n\nSigned-off-by: Paolo Bonzini \n---\n parse-options.c | 20 +++++++++-----------\n parse-options.h | 8 ++++----\n t/helper/test-parse-options.c | 2 ++\n t/t0040-parse-options.sh | 18 ++++++++++++++++++\n 4 files changed, 33 insertions(+), 15 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex a0cef401fc..c6e9e2733b 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -61,7 +61,7 @@ static enum parse_opt_result opt_command_mode_error(\n \t */\n \tfor (that = all_opts; that->type != OPTION_END; that++) {\n \t\tif (that == opt ||\n-\t\t that->type != OPTION_CMDMODE ||\n+\t\t !(that->flags & PARSE_OPT_CMDMODE) ||\n \t\t that->value != opt->value ||\n \t\t that->defval != *(int *)opt->value)\n \t\t\tcontinue;\n@@ -95,6 +95,14 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \tif (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG))\n \t\treturn error(_(\"%s takes no value\"), optname(opt, flags));\n \n+\t/*\n+\t * Giving the same mode option twice, although unnecessary,\n+\t * is not a grave error, so let it pass.\n+\t */\n+\tif ((opt->flags & PARSE_OPT_CMDMODE) &&\n+\t *(int *)opt->value && *(int *)opt->value != opt->defval)\n+\t\treturn opt_command_mode_error(opt, all_opts, flags);\n+\n \tswitch (opt->type) {\n \tcase OPTION_LOWLEVEL_CALLBACK:\n \t\treturn opt->ll_callback(p, opt, NULL, unset);\n@@ -130,16 +138,6 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t\t*(int *)opt->value = unset ? 0 : opt->defval;\n \t\treturn 0;\n \n-\tcase OPTION_CMDMODE:\n-\t\t/*\n-\t\t * Giving the same mode option twice, although is unnecessary,\n-\t\t * is not a grave error, so let it pass.\n-\t\t */\n-\t\tif (*(int *)opt->value && *(int *)opt->value != opt->defval)\n-\t\t\treturn opt_command_mode_error(opt, all_opts, flags);\n-\t\t*(int *)opt->value = opt->defval;\n-\t\treturn 0;\n-\n \tcase OPTION_STRING:\n \t\tif (unset)\n \t\t\t*(const char **)opt->value = NULL;\ndiff --git a/parse-options.h b/parse-options.h\nindex 1d60205881..fece5ba628 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -18,7 +18,6 @@ enum parse_opt_type {\n \tOPTION_BITOP,\n \tOPTION_COUNTUP,\n \tOPTION_SET_INT,\n-\tOPTION_CMDMODE,\n \t/* options with arguments (usually) */\n \tOPTION_STRING,\n \tOPTION_INTEGER,\n@@ -47,7 +46,8 @@ enum parse_opt_option_flags {\n \tPARSE_OPT_LITERAL_ARGHELP = 64,\n \tPARSE_OPT_SHELL_EVAL = 256,\n \tPARSE_OPT_NOCOMPLETE = 512,\n-\tPARSE_OPT_COMP_ARG = 1024\n+\tPARSE_OPT_COMP_ARG = 1024,\n+\tPARSE_OPT_CMDMODE = 2048\n };\n \n enum parse_opt_result {\n@@ -168,8 +168,8 @@ struct option {\n #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)\n #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}\n-#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n-\t\t\t\t (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }\n+#define OPT_CMDMODE(s, l, v, h, i) { OPTION_SET_INT, (s), (l), (v), NULL, \\\n+\t\t\t\t (h), PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }\n #define OPT_INTEGER(s, l, v, h) OPT_INTEGER_F(s, l, v, h, 0)\n #define OPT_MAGNITUDE(s, l, v, h) { OPTION_MAGNITUDE, (s), (l), (v), \\\n \t\t\t\t N_(\"n\"), (h), PARSE_OPT_NONEG }\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex af82db06ac..2051ce57db 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -121,6 +121,8 @@ int cmd__parse_options(int argc, const char **argv)\n \t\tOPT_INTEGER('j', NULL, &integer, \"get a integer, too\"),\n \t\tOPT_MAGNITUDE('m', \"magnitude\", &magnitude, \"get a magnitude\"),\n \t\tOPT_SET_INT(0, \"set23\", &integer, \"set integer to 23\", 23),\n+\t\tOPT_CMDMODE(0, \"mode1\", &integer, \"set integer to 1 (cmdmode option)\", 1),\n+\t\tOPT_CMDMODE(0, \"mode2\", &integer, \"set integer to 2 (cmdmode option)\", 2),\n \t\tOPT_CALLBACK('L', \"length\", &integer, \"str\",\n \t\t\t\"get length of \", length_callback),\n \t\tOPT_FILENAME('F', \"file\", &file, \"set file to \"),\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 9d7c7fdaa2..7f4c15a52b 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -23,6 +23,8 @@ usage: test-tool parse-options \n -j get a integer, too\n -m, --magnitude get a magnitude\n --set23 set integer to 23\n+ --mode1 set integer to 1 (cmdmode option)\n+ --mode2 set integer to 2 (cmdmode option)\n -L, --length get length of \n -F, --file set file to \n \n@@ -324,6 +326,22 @@ test_expect_success 'OPT_NEGBIT() works' '\n \ttest-tool parse-options --expect=\"boolean: 6\" -bb --no-neg-or4\n '\n \n+test_expect_success 'OPT_CMDMODE() works' '\n+\ttest-tool parse-options --expect=\"integer: 1\" --mode1\n+'\n+\n+test_expect_success 'OPT_CMDMODE() detects incompatibility' '\n+\ttest_must_fail test-tool parse-options --mode1 --mode2 >output 2>output.err &&\n+\ttest_must_be_empty output &&\n+\tgrep \"incompatible with --mode\" output.err\n+'\n+\n+test_expect_success 'OPT_CMDMODE() detects incompatibility with something else' '\n+\ttest_must_fail test-tool parse-options --set23 --mode2 >output 2>output.err &&\n+\ttest_must_be_empty output &&\n+\tgrep \"incompatible with something else\" output.err\n+'\n+\n test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '\n \ttest-tool parse-options --expect=\"boolean: 6\" + + + + + +\n '\n-- \n2.21.1\n\n\n"},{"id":"392065","messageId":"20200219161352.13562-3-pbonzini@redhat.com","threadId":"52840","inReplyTo":"20200219161352.13562-1-pbonzini@redhat.com","subject":"[PATCH 2/4] am: convert \"resume\" variable to a struct","fromName":"","fromEmail":"pbonzini@redhat.com","sentAt":"2020-02-19T16:13:50Z","receivedAt":"2020-02-19T16:14:06Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"From: Paolo Bonzini \n\nThis will allow stashing the submode of --show-current-patch. Using\na struct will allow accessing both fields from outside cmd_am (through\ncontainer_of).\n\nSigned-off-by: Paolo Bonzini \n---\n builtin/am.c | 32 ++++++++++++++++++--------------\n 1 file changed, 18 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 8181c2aef3..a89e1a96ed 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2118,7 +2118,7 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, int\n \treturn 0;\n }\n \n-enum resume_mode {\n+enum resume_type {\n \tRESUME_FALSE = 0,\n \tRESUME_APPLY,\n \tRESUME_RESOLVED,\n@@ -2128,6 +2128,10 @@ enum resume_mode {\n \tRESUME_SHOW_PATCH\n };\n \n+struct resume_mode {\n+\tenum resume_type mode;\n+};\n+\n static int git_am_config(const char *k, const char *v, void *cb)\n {\n \tint status;\n@@ -2145,7 +2149,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \tint binary = -1;\n \tint keep_cr = -1;\n \tint patch_format = PATCH_FORMAT_UNKNOWN;\n-\tenum resume_mode resume = RESUME_FALSE;\n+\tstruct resume_mode resume = { . mode = RESUME_FALSE };\n \tint in_progress;\n \tint ret = 0;\n \n@@ -2214,22 +2218,22 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_NOARG),\n \t\tOPT_STRING(0, \"resolvemsg\", &state.resolvemsg, NULL,\n \t\t\tN_(\"override error message when patch failure occurs\")),\n-\t\tOPT_CMDMODE(0, \"continue\", &resume,\n+\t\tOPT_CMDMODE(0, \"continue\", &resume.mode,\n \t\t\tN_(\"continue applying patches after resolving a conflict\"),\n \t\t\tRESUME_RESOLVED),\n-\t\tOPT_CMDMODE('r', \"resolved\", &resume,\n+\t\tOPT_CMDMODE('r', \"resolved\", &resume.mode,\n \t\t\tN_(\"synonyms for --continue\"),\n \t\t\tRESUME_RESOLVED),\n-\t\tOPT_CMDMODE(0, \"skip\", &resume,\n+\t\tOPT_CMDMODE(0, \"skip\", &resume.mode,\n \t\t\tN_(\"skip the current patch\"),\n \t\t\tRESUME_SKIP),\n-\t\tOPT_CMDMODE(0, \"abort\", &resume,\n+\t\tOPT_CMDMODE(0, \"abort\", &resume.mode,\n \t\t\tN_(\"restore the original branch and abort the patching operation.\"),\n \t\t\tRESUME_ABORT),\n-\t\tOPT_CMDMODE(0, \"quit\", &resume,\n+\t\tOPT_CMDMODE(0, \"quit\", &resume.mode,\n \t\t\tN_(\"abort the patching operation but keep HEAD where it is.\"),\n \t\t\tRESUME_QUIT),\n-\t\tOPT_CMDMODE(0, \"show-current-patch\", &resume,\n+\t\tOPT_CMDMODE(0, \"show-current-patch\", &resume.mode,\n \t\t\tN_(\"show the patch being applied.\"),\n \t\t\tRESUME_SHOW_PATCH),\n \t\tOPT_BOOL(0, \"committer-date-is-author-date\",\n@@ -2281,12 +2285,12 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t * intend to feed us a patch but wanted to continue\n \t\t * unattended.\n \t\t */\n-\t\tif (argc || (resume == RESUME_FALSE && !isatty(0)))\n+\t\tif (argc || (resume.mode == RESUME_FALSE && !isatty(0)))\n \t\t\tdie(_(\"previous rebase directory %s still exists but mbox given.\"),\n \t\t\t\tstate.dir);\n \n-\t\tif (resume == RESUME_FALSE)\n-\t\t\tresume = RESUME_APPLY;\n+\t\tif (resume.mode == RESUME_FALSE)\n+\t\t\tresume.mode = RESUME_APPLY;\n \n \t\tif (state.signoff == SIGNOFF_EXPLICIT)\n \t\t\tam_append_signoff(&state);\n@@ -2300,7 +2304,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t * stray directories.\n \t\t */\n \t\tif (file_exists(state.dir) && !state.rebasing) {\n-\t\t\tif (resume == RESUME_ABORT || resume == RESUME_QUIT) {\n+\t\t\tif (resume.mode == RESUME_ABORT || resume.mode == RESUME_QUIT) {\n \t\t\t\tam_destroy(&state);\n \t\t\t\tam_state_release(&state);\n \t\t\t\treturn 0;\n@@ -2311,7 +2315,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t\t\tstate.dir);\n \t\t}\n \n-\t\tif (resume)\n+\t\tif (resume.mode)\n \t\t\tdie(_(\"Resolve operation not in progress, we are not resuming.\"));\n \n \t\tfor (i = 0; i < argc; i++) {\n@@ -2329,7 +2333,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\targv_array_clear(&paths);\n \t}\n \n-\tswitch (resume) {\n+\tswitch (resume.mode) {\n \tcase RESUME_FALSE:\n \t\tam_run(&state, 0);\n \t\tbreak;\n-- \n2.21.1\n\n\n"},{"id":"392066","messageId":"20200219161352.13562-4-pbonzini@redhat.com","threadId":"52840","inReplyTo":"20200219161352.13562-1-pbonzini@redhat.com","subject":"[PATCH 3/4] am: support --show-current-patch=raw as a synonym for--show-current-patch","fromName":"","fromEmail":"pbonzini@redhat.com","sentAt":"2020-02-19T16:13:51Z","receivedAt":"2020-02-19T16:14:12Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"From: Paolo Bonzini \n\nWhen \"git am --show-current-patch\" was added in commit 984913a210 (\"am:\nadd --show-current-patch\", 2018-02-12), \"git am\" started recommending it\nas a replacement for .git/rebase-merge/patch. Unfortunately the suggestion\nis misguided, for example the output \"git am --show-current-patch\" cannot\nbe passed to \"git apply\" if it is encoded as quoted-printable or base64.\n\nWe would like therefore to add a new mode to \"git am\" that copies\n.git/rebase-merge/patch to stdout. In order to preserve backwards\ncompatibility, \"git am --show-current-patch\"'s behavior as to stay as\nis, and the new functionality will be added as an optional\nargument to --show-current-patch. As a start, add the code to parse\nsubmodes. For now \"raw\" is the only valid submode, and it prints\nthe full e-mail message just like \"git am --show-current-patch\".\n\nSigned-off-by: Paolo Bonzini \n---\n Documentation/git-am.txt | 4 +-\n builtin/am.c | 59 +++++++++++++++++++++++---\n contrib/completion/git-completion.bash | 5 +++\n t/t4150-am.sh | 10 +++++\n 4 files changed, 70 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\nindex 11ca61b00b..bafb491ede 100644\n--- a/Documentation/git-am.txt\n+++ b/Documentation/git-am.txt\n@@ -16,7 +16,7 @@ SYNOPSIS\n \t [--exclude=] [--include=] [--reject] [-q | --quiet]\n \t [--[no-]scissors] [-S[]] [--patch-format=]\n \t [( | )...]\n-'git am' (--continue | --skip | --abort | --quit | --show-current-patch)\n+'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=raw])\n \n DESCRIPTION\n -----------\n@@ -176,7 +176,7 @@ default. You can use `--no-utf8` to override this.\n \tAbort the patching operation but keep HEAD and the index\n \tuntouched.\n \n---show-current-patch::\n+--show-current-patch[=raw]::\n \tShow the entire e-mail message \"git am\" has stopped at, because\n \tof conflicts.\n \ndiff --git a/builtin/am.c b/builtin/am.c\nindex a89e1a96ed..ec4c743556 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -81,6 +81,10 @@ enum signoff_type {\n \tSIGNOFF_EXPLICIT /* --signoff was set on the command-line */\n };\n \n+enum show_patch_type {\n+\tSHOW_PATCH_RAW = 0,\n+};\n+\n struct am_state {\n \t/* state directory path */\n \tchar *dir;\n@@ -2061,7 +2065,7 @@ static void am_abort(struct am_state *state)\n \tam_destroy(state);\n }\n \n-static int show_patch(struct am_state *state)\n+static int show_patch(struct am_state *state, enum show_patch_type sub_mode)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *patch_path;\n@@ -2078,7 +2082,14 @@ static int show_patch(struct am_state *state)\n \t\treturn ret;\n \t}\n \n-\tpatch_path = am_path(state, msgnum(state));\n+\tswitch (sub_mode) {\n+\tcase SHOW_PATCH_RAW:\n+\t\tpatch_path = am_path(state, msgnum(state));\n+\t\tbreak;\n+\tdefault:\n+\t\tabort();\n+\t}\n+\n \tlen = strbuf_read_file(&sb, patch_path, 0);\n \tif (len < 0)\n \t\tdie_errno(_(\"failed to read '%s'\"), patch_path);\n@@ -2130,8 +2141,42 @@ enum resume_type {\n \n struct resume_mode {\n \tenum resume_type mode;\n+\tenum show_patch_type sub_mode;\n };\n \n+static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)\n+{\n+\tint *opt_value = opt->value;\n+\tstruct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);\n+\n+\t/*\n+\t * Please update $__git_showcurrentpatch in git-completion.bash\n+\t * when you add new options\n+\t */\n+\tconst char *valid_modes[] = {\n+\t\t[SHOW_PATCH_RAW] = \"raw\"\n+\t};\n+\tint new_value = SHOW_PATCH_RAW;\n+\n+\tif (arg) {\n+\t\tfor (new_value = 0; new_value < ARRAY_SIZE(valid_modes); new_value++) {\n+\t\t\tif (!strcmp(arg, valid_modes[new_value]))\n+\t\t\t\tbreak;\n+\t\t}\n+\t\tif (new_value >= ARRAY_SIZE(valid_modes))\n+\t\t\treturn error(_(\"Invalid value for --show-current-patch: %s\"), arg);\n+\t}\n+\n+\tif (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)\n+\t\treturn error(_(\"--show-current-patch=%s is incompatible with \"\n+\t\t\t \"--show-current-patch=%s\"),\n+\t\t\t arg, valid_modes[resume->sub_mode]);\n+\n+\tresume->mode = RESUME_SHOW_PATCH;\n+\tresume->sub_mode = new_value;\n+\treturn 0;\n+}\n+\n static int git_am_config(const char *k, const char *v, void *cb)\n {\n \tint status;\n@@ -2233,9 +2278,11 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\tOPT_CMDMODE(0, \"quit\", &resume.mode,\n \t\t\tN_(\"abort the patching operation but keep HEAD where it is.\"),\n \t\t\tRESUME_QUIT),\n-\t\tOPT_CMDMODE(0, \"show-current-patch\", &resume.mode,\n-\t\t\tN_(\"show the patch being applied.\"),\n-\t\t\tRESUME_SHOW_PATCH),\n+\t\t{ OPTION_CALLBACK, 0, \"show-current-patch\", &resume.mode,\n+\t\t \"raw\",\n+\t\t N_(\"show the patch being applied\"),\n+\t\t PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n+\t\t parse_opt_show_current_patch, RESUME_SHOW_PATCH },\n \t\tOPT_BOOL(0, \"committer-date-is-author-date\",\n \t\t\t&state.committer_date_is_author_date,\n \t\t\tN_(\"lie about committer date\")),\n@@ -2354,7 +2401,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\tam_destroy(&state);\n \t\tbreak;\n \tcase RESUME_SHOW_PATCH:\n-\t\tret = show_patch(&state);\n+\t\tret = show_patch(&state, resume.sub_mode);\n \t\tbreak;\n \tdefault:\n \t\tBUG(\"invalid resume value\");\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 1aac5a56c0..247f34f1fa 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1197,6 +1197,7 @@ __git_count_arguments ()\n \n __git_whitespacelist=\"nowarn warn error error-all fix\"\n __git_patchformat=\"mbox stgit stgit-series hg mboxrd\"\n+__git_showcurrentpatch=\"raw\"\n __git_am_inprogress_options=\"--skip --continue --resolved --abort --quit --show-current-patch\"\n \n _git_am ()\n@@ -1215,6 +1216,10 @@ _git_am ()\n \t\t__gitcomp \"$__git_patchformat\" \"\" \"${cur##--patch-format=}\"\n \t\treturn\n \t\t;;\n+\t--show-current-patch=*)\n+\t\t__gitcomp \"$__git_showcurrentpatch\" \"\" \"${cur##--show-current-patch=}\"\n+\t\treturn\n+\t\t;;\n \t--*)\n \t\t__gitcomp_builtin am \"\" \\\n \t\t\t\"$__git_am_inprogress_options\"\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex 4f1e24ecbe..afe456e75e 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -666,6 +666,16 @@ test_expect_success 'am --show-current-patch' '\n \ttest_cmp .git/rebase-apply/0001 actual.patch\n '\n \n+test_expect_success 'am --show-current-patch=raw' '\n+\tgit am --show-current-patch=raw >actual.patch &&\n+\ttest_cmp .git/rebase-apply/0001 actual.patch\n+'\n+\n+test_expect_success 'am accepts repeated --show-current-patch' '\n+\tgit am --show-current-patch --show-current-patch=raw >actual.patch &&\n+\ttest_cmp .git/rebase-apply/0001 actual.patch\n+'\n+\n test_expect_success 'am --skip works' '\n \techo goodbye >expected &&\n \tgit am --skip &&\n-- \n2.21.1\n\n\n"},{"id":"392067","messageId":"20200219161352.13562-5-pbonzini@redhat.com","threadId":"52840","inReplyTo":"20200219161352.13562-1-pbonzini@redhat.com","subject":"[PATCH 4/4] am: support --show-current-patch=diff to retrieve .git/rebase-apply/patch","fromName":"","fromEmail":"pbonzini@redhat.com","sentAt":"2020-02-19T16:13:52Z","receivedAt":"2020-02-19T16:14:16Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"From: Paolo Bonzini \n\nWhen \"git am --show-current-patch\" was added in commit 984913a210 (\"am:\nadd --show-current-patch\", 2018-02-12), \"git am\" started recommending it\nas a replacement for .git/rebase-merge/patch. Unfortunately the suggestion\nis misguided, for example the output \"git am --show-current-patch\" cannot\nbe passed to \"git apply\" if it is encoded as quoted-printable or base64.\nAdd a new mode to \"git am --show-current-patch\" in order to straighten\nthe suggestion.\n\nReported-by: J. Bruce Fields \nSigned-off-by: Paolo Bonzini \n---\n Documentation/git-am.txt | 10 ++++++----\n builtin/am.c | 7 ++++++-\n contrib/completion/git-completion.bash | 2 +-\n t/t4150-am.sh | 10 ++++++++++\n 4 files changed, 23 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\nindex bafb491ede..363d6ff665 100644\n--- a/Documentation/git-am.txt\n+++ b/Documentation/git-am.txt\n@@ -16,7 +16,7 @@ SYNOPSIS\n \t [--exclude=] [--include=] [--reject] [-q | --quiet]\n \t [--[no-]scissors] [-S[]] [--patch-format=]\n \t [( | )...]\n-'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=raw])\n+'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=raw|diff])\n \n DESCRIPTION\n -----------\n@@ -176,9 +176,11 @@ default. You can use `--no-utf8` to override this.\n \tAbort the patching operation but keep HEAD and the index\n \tuntouched.\n \n---show-current-patch[=raw]::\n-\tShow the entire e-mail message \"git am\" has stopped at, because\n-\tof conflicts.\n+--show-current-patch[=raw|diff]::\n+\tShow the message \"git am\" has stopped at, because of conflicts.\n+\tIf the argument is absent or \"raw\", show the raw contents of\n+\tthe e-mail message. If the argument is \"diff\", show the diff\n+\tportion only.\n \n DISCUSSION\n ----------\ndiff --git a/builtin/am.c b/builtin/am.c\nindex ec4c743556..ad543882ed 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -83,6 +83,7 @@ enum signoff_type {\n \n enum show_patch_type {\n \tSHOW_PATCH_RAW = 0,\n+\tSHOW_PATCH_DIFF = 1,\n };\n \n struct am_state {\n@@ -1767,7 +1768,7 @@ static void am_run(struct am_state *state, int resume)\n \t\t\t\tlinelen(state->msg), state->msg);\n \n \t\t\tif (advice_amworkdir)\n-\t\t\t\tadvise(_(\"Use 'git am --show-current-patch' to see the failed patch\"));\n+\t\t\t\tadvise(_(\"Use 'git am --show-current-patch=diff' to see the failed patch\"));\n \n \t\t\tdie_user_resolve(state);\n \t\t}\n@@ -2086,6 +2087,9 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)\n \tcase SHOW_PATCH_RAW:\n \t\tpatch_path = am_path(state, msgnum(state));\n \t\tbreak;\n+\tcase SHOW_PATCH_DIFF:\n+\t\tpatch_path = am_path(state, \"patch\");\n+\t\tbreak;\n \tdefault:\n \t\tabort();\n \t}\n@@ -2154,6 +2158,7 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar\n \t * when you add new options\n \t */\n \tconst char *valid_modes[] = {\n+\t\t[SHOW_PATCH_DIFF] = \"diff\",\n \t\t[SHOW_PATCH_RAW] = \"raw\"\n \t};\n \tint new_value = SHOW_PATCH_RAW;\n@@ -2284,7 +2288,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"abort the patching operation but keep HEAD where it is.\"),\n \t\t\tRESUME_QUIT),\n \t\t{ OPTION_CALLBACK, 0, \"show-current-patch\", &resume.mode,\n-\t\t \"raw\",\n+\t\t \"diff|raw\",\n \t\t N_(\"show the patch being applied\"),\n \t\t PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n \t\t parse_opt_show_current_patch, RESUME_SHOW_PATCH },\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 247f34f1fa..1151697f01 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1197,7 +1197,7 @@ __git_count_arguments ()\n \n __git_whitespacelist=\"nowarn warn error error-all fix\"\n __git_patchformat=\"mbox stgit stgit-series hg mboxrd\"\n-__git_showcurrentpatch=\"raw\"\n+__git_showcurrentpatch=\"raw diff\"\n __git_am_inprogress_options=\"--skip --continue --resolved --abort --quit --show-current-patch\"\n \n _git_am ()\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex afe456e75e..cb45271457 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -671,11 +671,21 @@ test_expect_success 'am --show-current-patch=raw' '\n \ttest_cmp .git/rebase-apply/0001 actual.patch\n '\n \n+test_expect_success 'am --show-current-patch=diff' '\n+\tgit am --show-current-patch=diff >actual.patch &&\n+\ttest_cmp .git/rebase-apply/patch actual.patch\n+'\n+\n test_expect_success 'am accepts repeated --show-current-patch' '\n \tgit am --show-current-patch --show-current-patch=raw >actual.patch &&\n \ttest_cmp .git/rebase-apply/0001 actual.patch\n '\n \n+test_expect_success 'am detects incompatible --show-current-patch' '\n+\ttest_must_fail git am --show-current-patch=raw --show-current-patch=diff &&\n+\ttest_must_fail git am --show-current-patch --show-current-patch=diff\n+'\n+\n test_expect_success 'am --skip works' '\n \techo goodbye >expected &&\n \tgit am --skip &&\n-- \n2.21.1\n\n"},{"id":"392092","messageId":"CAPig+cQkBKJLW3-W4SS0KX9+Gs2fT-Z-DrvVpcVOLZpFmVBoQA@mail.gmail.com","threadId":"52840","inReplyTo":"20200219161352.13562-2-pbonzini@redhat.com","subject":"Re: [PATCH 1/4] parse-options: convert \"command mode\" to a flag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-19T19:11:24Z","receivedAt":"2020-02-19T19:11:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 19, 2020 at 11:15 AM wrote:\n> OPTION_CMDMODE is essentially OPTION_SET_INT plus the extra check\n> that the variable had not set before. In order to allow custom\n> processing, change it to OPTION_SET_INT plus a new flag that takes\n> care of the check. This works as long as the option value points\n> to an int.\n>\n> Signed-off-by: Paolo Bonzini \n> ---\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> @@ -324,6 +326,22 @@ test_expect_success 'OPT_NEGBIT() works' '\n> +test_expect_success 'OPT_CMDMODE() detects incompatibility' '\n> + test_must_fail test-tool parse-options --mode1 --mode2 >output 2>output.err &&\n> + test_must_be_empty output &&\n> + grep \"incompatible with --mode\" output.err\n> +'\n\nThe error message may have been localized, so use test_i18ngrep()\ninstead of 'grep':\n\n test_i18ngrep \"incompatible with --mode\" output.err\n\n> +\n> +test_expect_success 'OPT_CMDMODE() detects incompatibility with something else' '\n> + test_must_fail test-tool parse-options --set23 --mode2 >output 2>output.err &&\n> + test_must_be_empty output &&\n> + grep \"incompatible with something else\" output.err\n> +'\n\nDitto.\n"},{"id":"392094","messageId":"xmqqzhdee6c6.fsf@gitster-ct.c.googlers.com","threadId":"52840","inReplyTo":"20200219161352.13562-2-pbonzini@redhat.com","subject":"Re: [PATCH 1/4] parse-options: convert \"command mode\" to a flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-19T19:15:53Z","receivedAt":"2020-02-19T19:16:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"pbonzini@redhat.com writes:\n\n> From: Paolo Bonzini \n>\n> OPTION_CMDMODE is essentially OPTION_SET_INT plus the extra check\n> that the variable had not set before. In order to allow custom\n> processing, change it to OPTION_SET_INT plus a new flag that takes\n> care of the check. This works as long as the option value points\n> to an int.\n\nIt is unclear but I am guessing that the purpose of this change is\nto make \"only one of these\" orthgonal to \"the value of this option\nis an int\", in preparation to allow options other than SET_INT to\nalso be combined with \"only one of these\"?\n\nIf my reading is not correct, that would be an indication that the\nabove paragraph does not tell what it wants to to readers. \n\nIt is unclear at this step what other kind of option the flag wants\nto be combined, though.\n\n> diff --git a/parse-options.c b/parse-options.c\n> index a0cef401fc..c6e9e2733b 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -61,7 +61,7 @@ static enum parse_opt_result opt_command_mode_error(\n> \t */\n> \tfor (that = all_opts; that->type != OPTION_END; that++) {\n> \t\tif (that == opt ||\n> -\t\t that->type != OPTION_CMDMODE ||\n> +\t\t !(that->flags & PARSE_OPT_CMDMODE) ||\n> \t\t that->value != opt->value ||\n> \t\t that->defval != *(int *)opt->value)\n> \t\t\tcontinue;\n> @@ -95,6 +95,14 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n> \tif (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG))\n> \t\treturn error(_(\"%s takes no value\"), optname(opt, flags));\n> \n> +\t/*\n> +\t * Giving the same mode option twice, although unnecessary,\n> +\t * is not a grave error, so let it pass.\n> +\t */\n> +\tif ((opt->flags & PARSE_OPT_CMDMODE) &&\n> +\t *(int *)opt->value && *(int *)opt->value != opt->defval)\n> +\t\treturn opt_command_mode_error(opt, all_opts, flags);\n> +\n\n... and when there is no error, we fall through and process it as a\nregular SET_INT, which makes sense.\n\n> @@ -168,8 +168,8 @@ struct option {\n> #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0)\n> #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \\\n> \t\t\t\t (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}\n> -#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n> -\t\t\t\t (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }\n> +#define OPT_CMDMODE(s, l, v, h, i) { OPTION_SET_INT, (s), (l), (v), NULL, \\\n> +\t\t\t\t (h), PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }\n\nOK.\n\n> diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\n> index af82db06ac..2051ce57db 100644\n> --- a/t/helper/test-parse-options.c\n> +++ b/t/helper/test-parse-options.c\n> @@ -121,6 +121,8 @@ int cmd__parse_options(int argc, const char **argv)\n> \t\tOPT_INTEGER('j', NULL, &integer, \"get a integer, too\"),\n> \t\tOPT_MAGNITUDE('m', \"magnitude\", &magnitude, \"get a magnitude\"),\n> \t\tOPT_SET_INT(0, \"set23\", &integer, \"set integer to 23\", 23),\n> +\t\tOPT_CMDMODE(0, \"mode1\", &integer, \"set integer to 1 (cmdmode option)\", 1),\n> +\t\tOPT_CMDMODE(0, \"mode2\", &integer, \"set integer to 2 (cmdmode option)\", 2),\n> \t\tOPT_CALLBACK('L', \"length\", &integer, \"str\",\n> \t\t\t\"get length of \", length_callback),\n> \t\tOPT_FILENAME('F', \"file\", &file, \"set file to \"),\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> index 9d7c7fdaa2..7f4c15a52b 100755\n> --- a/t/t0040-parse-options.sh\n> +++ b/t/t0040-parse-options.sh\n> @@ -23,6 +23,8 @@ usage: test-tool parse-options \n> -j get a integer, too\n> -m, --magnitude get a magnitude\n> --set23 set integer to 23\n> + --mode1 set integer to 1 (cmdmode option)\n> + --mode2 set integer to 2 (cmdmode option)\n> -L, --length get length of \n> -F, --file set file to \n> \n> @@ -324,6 +326,22 @@ test_expect_success 'OPT_NEGBIT() works' '\n> \ttest-tool parse-options --expect=\"boolean: 6\" -bb --no-neg-or4\n> '\n> \n> +test_expect_success 'OPT_CMDMODE() works' '\n> +\ttest-tool parse-options --expect=\"integer: 1\" --mode1\n> +'\n> +\n> +test_expect_success 'OPT_CMDMODE() detects incompatibility' '\n> +\ttest_must_fail test-tool parse-options --mode1 --mode2 >output 2>output.err &&\n> +\ttest_must_be_empty output &&\n> +\tgrep \"incompatible with --mode\" output.err\n> +'\n> +\n> +test_expect_success 'OPT_CMDMODE() detects incompatibility with something else' '\n> +\ttest_must_fail test-tool parse-options --set23 --mode2 >output 2>output.err &&\n> +\ttest_must_be_empty output &&\n> +\tgrep \"incompatible with something else\" output.err\n> +'\n> +\n> test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '\n> \ttest-tool parse-options --expect=\"boolean: 6\" + + + + + +\n> '\n\nWould the updated test-parse-options.c with these three new tests\nwork the same way with or without changes to parse-options.[ch]?\nThat would be a good indication that the change to the code is\n\"upward compatible\".\n\nThanks.\n"},{"id":"392095","messageId":"xmqqv9o2e66q.fsf@gitster-ct.c.googlers.com","threadId":"52840","inReplyTo":"20200219161352.13562-3-pbonzini@redhat.com","subject":"Re: [PATCH 2/4] am: convert \"resume\" variable to a struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-19T19:19:09Z","receivedAt":"2020-02-19T19:19:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"pbonzini@redhat.com writes:\n\n> From: Paolo Bonzini \n>\n> This will allow stashing the submode of --show-current-patch. Using\n> a struct will allow accessing both fields from outside cmd_am (through\n> container_of).\n>\n> Signed-off-by: Paolo Bonzini \n> ---\n> builtin/am.c | 32 ++++++++++++++++++--------------\n> 1 file changed, 18 insertions(+), 14 deletions(-)\n>\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 8181c2aef3..a89e1a96ed 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -2118,7 +2118,7 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, int\n> \treturn 0;\n> }\n> \n> -enum resume_mode {\n> +enum resume_type {\n> \tRESUME_FALSE = 0,\n> \tRESUME_APPLY,\n> \tRESUME_RESOLVED,\n> @@ -2128,6 +2128,10 @@ enum resume_mode {\n> \tRESUME_SHOW_PATCH\n> };\n> \n> +struct resume_mode {\n> +\tenum resume_type mode;\n> +};\n> +\n> static int git_am_config(const char *k, const char *v, void *cb)\n> {\n> \tint status;\n> @@ -2145,7 +2149,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n> \tint binary = -1;\n> \tint keep_cr = -1;\n> \tint patch_format = PATCH_FORMAT_UNKNOWN;\n> -\tenum resume_mode resume = RESUME_FALSE;\n> +\tstruct resume_mode resume = { . mode = RESUME_FALSE };\n\nI do not think it makes a difference to compilers, but it seems that\nexisting code spells this without SP between dot and the field name.\n\n> +\tstruct resume_mode resume = { .mode = RESUME_FALSE };\n\nThe rest of the patch is quite straight-forward update that looks\ncorrect.\n\nThanks.\n"},{"id":"392098","messageId":"CAPig+cQOZwA3aAzBko-RL8UnW77DuBY-s_-J2D+35Ofn=fFfsg@mail.gmail.com","threadId":"52840","inReplyTo":"20200219161352.13562-4-pbonzini@redhat.com","subject":"Re: [PATCH 3/4] am: support --show-current-patch=raw as a synonym for--show-current-patch","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-19T19:34:43Z","receivedAt":"2020-02-19T19:34:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 19, 2020 at 11:15 AM wrote:\n> [...]\n> We would like therefore to add a new mode to \"git am\" that copies\n> .git/rebase-merge/patch to stdout. In order to preserve backwards\n> compatibility, \"git am --show-current-patch\"'s behavior as to stay as\n\ns/as to/has to/\n\n> is, and the new functionality will be added as an optional\n> argument to --show-current-patch. As a start, add the code to parse\n> submodes. For now \"raw\" is the only valid submode, and it prints\n> the full e-mail message just like \"git am --show-current-patch\".\n>\n> Signed-off-by: Paolo Bonzini \n> ---\n> diff --git a/builtin/am.c b/builtin/am.c\n> @@ -2078,7 +2082,14 @@ static int show_patch(struct am_state *state)\n> - patch_path = am_path(state, msgnum(state));\n> + switch (sub_mode) {\n> + case SHOW_PATCH_RAW:\n> + patch_path = am_path(state, msgnum(state));\n> + break;\n> + default:\n> + abort();\n> + }\n\nI expect that this abort() is likely to go away in the next patch, so\nit's not such a big deal, but the usual way to indicate that this is\nan impossible condition is with BUG() rather than abort(). So, if you\nhappen to re-roll for some reason, perhaps consider using BUG()\ninstead.\n\n> @@ -2130,8 +2141,42 @@ enum resume_type {\n> +static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)\n> +{\n> + int new_value = SHOW_PATCH_RAW;\n> +\n> + if (arg) {\n> + for (new_value = 0; new_value < ARRAY_SIZE(valid_modes); new_value++) {\n> + if (!strcmp(arg, valid_modes[new_value]))\n> + break;\n> + }\n> + if (new_value >= ARRAY_SIZE(valid_modes))\n> + return error(_(\"Invalid value for --show-current-patch: %s\"), arg);\n> + }\n\nI think the more typical way of coding this in this project is to\ninitialize 'new_value' to -1. Doing so will make it easier to some day\nadd a configuration value as fallback for when the sub-mode is not\nspecified on the command line. So, it would look something like this:\n\n int submode = -1;\n if (arg) {\n int i;\n for (i = 0; i < ARRAY_SIZE(valid_modes); i++)\n if (!strcmp(arg, valid_modes[i]))\n break;\n if (i >= ARRAY_SIZE(valid_modes))\n return error(_(\"invalid value for --show-current-patch: %s\"), arg);\n submode = i;\n }\n\n /* fall back to config value */\n if (submode < 0) {\n /* check if config value available and assign 'sudmode' */\n }\n\n> + if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)\n> + return error(_(\"--show-current-patch=%s is incompatible with \"\n> + \"--show-current-patch=%s\"),\n> + arg, valid_modes[resume->sub_mode]);\n\nSo, this allows --show-current-patch= to be specified multiple\ntimes but only as long as is the same each time, and errors out\notherwise. That's rather harsh and makes it difficult for someone to\noverride a value specified earlier on the command line (say, coming\nfrom a Git alias). The typical way this is handled is \"last wins\"\nrather than making it an error.\n\n> + resume->mode = RESUME_SHOW_PATCH;\n> + resume->sub_mode = new_value;\n> + return 0;\n> +}\n"},{"id":"392099","messageId":"CAPig+cR2VLDYc_UpbWySFSF49Uo0twVyGRXgVx3Z6w1R04aavg@mail.gmail.com","threadId":"52840","inReplyTo":"20200219161352.13562-5-pbonzini@redhat.com","subject":"Re: [PATCH 4/4] am: support --show-current-patch=diff to retrieve .git/rebase-apply/patch","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-19T19:49:30Z","receivedAt":"2020-02-19T19:49:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 19, 2020 at 11:15 AM wrote:\n> When \"git am --show-current-patch\" was added in commit 984913a210 (\"am:\n> add --show-current-patch\", 2018-02-12), \"git am\" started recommending it\n> as a replacement for .git/rebase-merge/patch. Unfortunately the suggestion\n> is misguided, for example the output \"git am --show-current-patch\" cannot\n> be passed to \"git apply\" if it is encoded as quoted-printable or base64.\n> Add a new mode to \"git am --show-current-patch\" in order to straighten\n> the suggestion.\n>\n> Signed-off-by: Paolo Bonzini \n> ---\n> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\n> @@ -16,7 +16,7 @@ SYNOPSIS\n> -'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=raw])\n> +'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=raw|diff])\n\nMissing parentheses. To be consistent with other documentation, this\nshould be written as:\n\n --show-current-patch[=(raw|diff)]\n\n> @@ -176,9 +176,11 @@ default. You can use `--no-utf8` to override this.\n> ---show-current-patch[=raw]::\n> - Show the entire e-mail message \"git am\" has stopped at, because\n> - of conflicts.\n> +--show-current-patch[=raw|diff]::\n\nDitto: --show-current-patch[=(raw|diff)]::\n\n> + Show the message \"git am\" has stopped at, because of conflicts.\n\nThe weirdly-placed comma is still weird.\n\n> + If the argument is absent or \"raw\", show the raw contents of\n> + the e-mail message. If the argument is \"diff\", show the diff\n> + portion only.\n\nI think the usual term is \"omitted\" rather than \"absent\".\n\nSuggested rewrite:\n\n Show the message at which `git am` has stopped due to conflicts.\n If `raw` is specified, show the raw contents of the e-mail\n message; if `diff`, show the diff portion only. Defaults to `raw`.\n\nThis also simplifies the change if the default ever flips from \"raw\" to \"diff\".\n"},{"id":"392103","messageId":"xmqqmu9ee3hc.fsf@gitster-ct.c.googlers.com","threadId":"52840","inReplyTo":"CAPig+cQOZwA3aAzBko-RL8UnW77DuBY-s_-J2D+35Ofn=fFfsg@mail.gmail.com","subject":"Re: [PATCH 3/4] am: support --show-current-patch=raw as a synonym for--show-current-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-19T20:17:35Z","receivedAt":"2020-02-19T20:17:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> I think the more typical way of coding this in this project is to\n> initialize 'new_value' to -1. Doing so will make it easier to some day\n> add a configuration value as fallback for when the sub-mode is not\n> specified on the command line. So, it would look something like this:\n>\n> int submode = -1;\n> if (arg) {\n> int i;\n> for (i = 0; i < ARRAY_SIZE(valid_modes); i++)\n> if (!strcmp(arg, valid_modes[i]))\n> break;\n> if (i >= ARRAY_SIZE(valid_modes))\n> return error(_(\"invalid value for --show-current-patch: %s\"), arg);\n> submode = i;\n> }\n>\n> /* fall back to config value */\n> if (submode < 0) {\n> /* check if config value available and assign 'sudmode' */\n> }\n\nHmph? Isn't the usual pattern more like this:\n\n\n\tstatic int submode = -1; /* unspecified */\n\n\tint cmd_foo(...)\n\t{\n\t\tgit_config(...); /* this may update submode */\n\t\tparse_options(...); /* this may further update submode */\n\n\t\tif (submode < 0)\n\t\t\tsubmode = ... some default value ...;\n\nto implement \"config gives a custom default, command line overrides,\nbut when there is neither, there is a hard-coded default\"?\n\nOf course, the variable can be initialized to the default value to\nlose the \"-1 /* unspecified */\" bit.\n\n>> + if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)\n>> + return error(_(\"--show-current-patch=%s is incompatible with \"\n>> + \"--show-current-patch=%s\"),\n>> + arg, valid_modes[resume->sub_mode]);\n>\n> So, this allows --show-current-patch= to be specified multiple\n> times but only as long as is the same each time, and errors out\n> otherwise. That's rather harsh and makes it difficult for someone to\n> override a value specified earlier on the command line (say, coming\n> from a Git alias). The typical way this is handled is \"last wins\"\n> rather than making it an error.\n\nYup, the last one wins is something I would have expected. And if\nwe follow that (which is the usual pattern), I suspect that we won't\neven need the first two steps of this series?\n\nThanks for a review.\n"},{"id":"392111","messageId":"217229b8-3a72-55fb-71c6-8ba8ae3ceb0b@redhat.com","threadId":"52840","inReplyTo":"xmqqmu9ee3hc.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/4] am: support --show-current-patch=raw as a synonym for--show-current-patch","fromName":"Paolo Bonzini","fromEmail":"pbonzini@redhat.com","sentAt":"2020-02-19T20:53:37Z","receivedAt":"2020-02-19T20:53:45Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 19/02/20 21:17, Junio C Hamano wrote:\n>>> + if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)\n>>> + return error(_(\"--show-current-patch=%s is incompatible with \"\n>>> + \"--show-current-patch=%s\"),\n>>> + arg, valid_modes[resume->sub_mode]);\n>>\n>> So, this allows --show-current-patch= to be specified multiple\n>> times but only as long as is the same each time, and errors out\n>> otherwise. That's rather harsh and makes it difficult for someone to\n>> override a value specified earlier on the command line (say, coming\n>> from a Git alias). The typical way this is handled is \"last wins\"\n>> rather than making it an error.\n> \n> Yup, the last one wins is something I would have expected. And if\n> we follow that (which is the usual pattern), I suspect that we won't\n> even need the first two steps of this series?\n\nWe would need them anyway, in order to add a callback to the \"command\nmode\" option --show-current-patch.\n\nThe fact that --show-current-patch is a command mode option is also why\nI decided against \"last one wins\". I think it would be counterintuitive\nthat\n\n\tgit am --abort --show-current-patch\n\nfails, but\n\n\tgit am --show-current-patch=diff --show-current-patch=raw\n\nsucceeds.\n\nAnother possibility is to have separate options --show-current-message\n(for .git/rebase-apply/NNNN) and --show-current-diff (for\n.git/rebase-apply/patch), possibly deprecating --show-current-patch.\nThat would have naturally rejected a command line like\n\n\tgit am --show-current-message --show-current-diff\n\n(and this one _would_ have removed the need for the first two patches in\nthe series). However, the long common prefix would have prevented using\nan abbreviated option such as \"--show\", so I went instead for the\noptional string argument.\n\nI realize now that I should have placed all this in the commit message,\nsorry about that.\n\nPaolo\n\n"},{"id":"392113","messageId":"a90410eb-b6e0-3077-0491-511c25a417f2@redhat.com","threadId":"52840","inReplyTo":"xmqqv9o2e66q.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/4] am: convert \"resume\" variable to a struct","fromName":"Paolo Bonzini","fromEmail":"pbonzini@redhat.com","sentAt":"2020-02-19T21:05:34Z","receivedAt":"2020-02-19T21:05:45Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 19/02/20 20:19, Junio C Hamano wrote:\n>> -\tenum resume_mode resume = RESUME_FALSE;\n>> +\tstruct resume_mode resume = { . mode = RESUME_FALSE };\n> I do not think it makes a difference to compilers, but it seems that\n> existing code spells this without SP between dot and the field name.\n\nYes, it's a typo.\n\nPaolo\n\n"},{"id":"392114","messageId":"ea9395db-b39a-358f-df8e-b28907193415@redhat.com","threadId":"52840","inReplyTo":"xmqqzhdee6c6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/4] parse-options: convert \"command mode\" to a flag","fromName":"Paolo Bonzini","fromEmail":"pbonzini@redhat.com","sentAt":"2020-02-19T21:05:40Z","receivedAt":"2020-02-19T21:05:48Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 19/02/20 20:15, Junio C Hamano wrote:\n>> OPTION_CMDMODE is essentially OPTION_SET_INT plus the extra check\n>> that the variable had not set before. In order to allow custom\n>> processing, change it to OPTION_SET_INT plus a new flag that takes\n>> care of the check. This works as long as the option value points\n>> to an int.\n> It is unclear but I am guessing that the purpose of this change is\n> to make \"only one of these\" orthgonal to \"the value of this option\n> is an int\", in preparation to allow options other than SET_INT to\n> also be combined with \"only one of these\"?\n> \n> If my reading is not correct, that would be an indication that the\n> above paragraph does not tell what it wants to to readers. \n\nYour reading and your conclusion are both correct. I'll reword the\ncommit message.\n\nPaolo\n\n> It is unclear at this step what other kind of option the flag wants\n> to be combined, though.\n> \n\n"},{"id":"392174","messageId":"nycvar.QRO.7.76.6.2002201659500.46@tvgsbejvaqbjf.bet","threadId":"52840","inReplyTo":"CAPig+cQkBKJLW3-W4SS0KX9+Gs2fT-Z-DrvVpcVOLZpFmVBoQA@mail.gmail.com","subject":"Re: [PATCH 1/4] parse-options: convert \"command mode\" to a flag","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-20T16:00:48Z","receivedAt":"2020-02-20T16:01:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Wed, 19 Feb 2020, Eric Sunshine wrote:\n\n> On Wed, Feb 19, 2020 at 11:15 AM wrote:\n> > OPTION_CMDMODE is essentially OPTION_SET_INT plus the extra check\n> > that the variable had not set before. In order to allow custom\n> > processing, change it to OPTION_SET_INT plus a new flag that takes\n> > care of the check. This works as long as the option value points\n> > to an int.\n> >\n> > Signed-off-by: Paolo Bonzini \n> > ---\n> > diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> > @@ -324,6 +326,22 @@ test_expect_success 'OPT_NEGBIT() works' '\n> > +test_expect_success 'OPT_CMDMODE() detects incompatibility' '\n> > + test_must_fail test-tool parse-options --mode1 --mode2 >output 2>output.err &&\n> > + test_must_be_empty output &&\n> > + grep \"incompatible with --mode\" output.err\n> > +'\n>\n> The error message may have been localized, so use test_i18ngrep()\n> instead of 'grep':\n>\n> test_i18ngrep \"incompatible with --mode\" output.err\n\nThe error message _is_ localized, causing the GETTEXT_POISON job to fail:\n\nhttps://dev.azure.com/gitgitgadget/git/_build/results?buildId=31113&view=ms.vss-test-web.build-test-results-tab&runId=97704&resultId=102357&paneView=debug\n\nSo yes. It needs to be changed to `test_i18ngrep`.\n\nThanks,\nJohannes\n\n>\n> > +\n> > +test_expect_success 'OPT_CMDMODE() detects incompatibility with something else' '\n> > + test_must_fail test-tool parse-options --set23 --mode2 >output 2>output.err &&\n> > + test_must_be_empty output &&\n> > + grep \"incompatible with something else\" output.err\n> > +'\n>\n> Ditto.\n>\n>\n"}]}