{"thread":{"id":"49817","subject":"[PATCH 0/1] rebase: understand -C again, refactor","startedAt":"2018-11-13T12:38:27Z","lastAt":"2018-11-20T19:56:02Z","messageCount":26,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Phillip Wood","Johannes Schindelin","Ævar Arnfjörð Bjarmason","Stefan Beller","Elijah Newren","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"363146","messageId":"pull.76.git.gitgitgadget@gmail.com","threadId":"49817","inReplyTo":null,"subject":"[PATCH 0/1] rebase: understand -C again, refactor","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-13T12:38:24Z","receivedAt":"2018-11-13T12:38:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Phillip Wood reported a problem where the built-in rebase did not understand\noptions like -C1, i.e. it did not expect the option argument.\n\nWhile investigating how to address this best, I stumbled upon \nOPT_PASSTHRU_ARGV (which I was so far happily unaware of).\n\nInstead of just fixing the -C bug, I decided to simply convert all of the\noptions intended for git am (or, eventually, for git apply). This happens to\nfix that bug, and does so much more: it simplifies the entire logic (and\nremoves more lines than it adds).\n\nJohannes Schindelin (1):\n rebase: really just passthru the `git am` options\n\n builtin/rebase.c | 98 +++++++++++++++++-------------------------------\n 1 file changed, 35 insertions(+), 63 deletions(-)\n\n\nbase-commit: 8858448bb49332d353febc078ce4a3abcc962efe\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-76%2Fdscho%2Frebase-Cn-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-76/dscho/rebase-Cn-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/76\n-- \ngitgitgadget\n"},{"id":"363147","messageId":"dc36a450680b1854abbb9bb06180001ce68c3f3b.1542112703.git.gitgitgadget@gmail.com","threadId":"49817","inReplyTo":"pull.76.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] rebase: really just passthru the `git am` options","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-13T12:38:26Z","receivedAt":"2018-11-13T12:38:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nCurrently, we parse the options intended for `git am` as if we wanted to\nhandle them in `git rebase`, and then reconstruct them painstakingly to\ndefine the `git_am_opt` variable.\n\nHowever, there is a much better way (that I was unaware of, at the time\nwhen I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.\nIt is intended for exactly this use case, where command-line options\nwant to be parsed into a separate `argv_array`.\n\nLet's use this feature.\n\nIncidentally, this also allows us to address a bug discovered by Phillip\nWood, where the built-in rebase failed to understand that the `-C`\noption takes an optional argument.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 98 +++++++++++++++++-------------------------------\n 1 file changed, 35 insertions(+), 63 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 0ee06aa363..96ffa80b71 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -87,7 +87,7 @@ struct rebase_options {\n \t\tREBASE_FORCE = 1<<3,\n \t\tREBASE_INTERACTIVE_EXPLICIT = 1<<4,\n \t} flags;\n-\tstruct strbuf git_am_opt;\n+\tstruct argv_array git_am_opts;\n \tconst char *action;\n \tint signoff;\n \tint allow_rerere_autoupdate;\n@@ -339,7 +339,7 @@ N_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n static int run_specific_rebase(struct rebase_options *opts)\n {\n \tconst char *argv[] = { NULL, NULL };\n-\tstruct strbuf script_snippet = STRBUF_INIT;\n+\tstruct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;\n \tint status;\n \tconst char *backend, *backend_func;\n \n@@ -433,7 +433,9 @@ static int run_specific_rebase(struct rebase_options *opts)\n \t\toid_to_hex(&opts->restrict_revision->object.oid) : NULL);\n \tadd_var(&script_snippet, \"GIT_QUIET\",\n \t\topts->flags & REBASE_NO_QUIET ? \"\" : \"t\");\n-\tadd_var(&script_snippet, \"git_am_opt\", opts->git_am_opt.buf);\n+\tsq_quote_argv_pretty(&buf, opts->git_am_opts.argv);\n+\tadd_var(&script_snippet, \"git_am_opt\", buf.buf);\n+\tstrbuf_release(&buf);\n \tadd_var(&script_snippet, \"verbose\",\n \t\topts->flags & REBASE_VERBOSE ? \"t\" : \"\");\n \tadd_var(&script_snippet, \"diffstat\",\n@@ -756,7 +758,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tstruct rebase_options options = {\n \t\t.type = REBASE_UNSPECIFIED,\n \t\t.flags = REBASE_NO_QUIET,\n-\t\t.git_am_opt = STRBUF_INIT,\n+\t\t.git_am_opts = ARGV_ARRAY_INIT,\n \t\t.allow_rerere_autoupdate = -1,\n \t\t.allow_empty_message = 1,\n \t\t.git_format_patch_opt = STRBUF_INIT,\n@@ -777,12 +779,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tACTION_EDIT_TODO,\n \t\tACTION_SHOW_CURRENT_PATCH,\n \t} action = NO_ACTION;\n-\tint committer_date_is_author_date = 0;\n-\tint ignore_date = 0;\n-\tint ignore_whitespace = 0;\n \tconst char *gpg_sign = NULL;\n-\tint opt_c = -1;\n-\tstruct string_list whitespace = STRING_LIST_INIT_NODUP;\n \tstruct string_list exec = STRING_LIST_INIT_NODUP;\n \tconst char *rebase_merges = NULL;\n \tint fork_point = -1;\n@@ -804,15 +801,20 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t{OPTION_NEGBIT, 'n', \"no-stat\", &options.flags, NULL,\n \t\t\tN_(\"do not show diffstat of what changed upstream\"),\n \t\t\tPARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },\n-\t\tOPT_BOOL(0, \"ignore-whitespace\", &ignore_whitespace,\n-\t\t\t N_(\"passed to 'git apply'\")),\n \t\tOPT_BOOL(0, \"signoff\", &options.signoff,\n \t\t\t N_(\"add a Signed-off-by: line to each commit\")),\n-\t\tOPT_BOOL(0, \"committer-date-is-author-date\",\n-\t\t\t &committer_date_is_author_date,\n-\t\t\t N_(\"passed to 'git am'\")),\n-\t\tOPT_BOOL(0, \"ignore-date\", &ignore_date,\n-\t\t\t N_(\"passed to 'git am'\")),\n+\t\tOPT_PASSTHRU_ARGV(0, \"ignore-whitespace\", &options.git_am_opts,\n+\t\t\t\t NULL, N_(\"passed to 'git am'\"),\n+\t\t\t\t PARSE_OPT_NOARG),\n+\t\tOPT_PASSTHRU_ARGV(0, \"committer-date-is-author-date\",\n+\t\t\t\t &options.git_am_opts, NULL,\n+\t\t\t\t N_(\"passed to 'git am'\"), PARSE_OPT_NOARG),\n+\t\tOPT_PASSTHRU_ARGV(0, \"ignore-date\", &options.git_am_opts, NULL,\n+\t\t\t\t N_(\"passed to 'git am'\"), PARSE_OPT_NOARG),\n+\t\tOPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_(\"n\"),\n+\t\t\t\t N_(\"passed to 'git apply'\"), 0),\n+\t\tOPT_PASSTHRU_ARGV(0, \"whitespace\", &options.git_am_opts,\n+\t\t\t\t N_(\"action\"), N_(\"passed to 'git apply'\"), 0),\n \t\tOPT_BIT('f', \"force-rebase\", &options.flags,\n \t\t\tN_(\"cherry-pick all commits, even if unchanged\"),\n \t\t\tREBASE_FORCE),\n@@ -856,10 +858,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &gpg_sign, N_(\"key-id\"),\n \t\t\tN_(\"GPG-sign commits\"),\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n-\t\tOPT_STRING_LIST(0, \"whitespace\", &whitespace,\n-\t\t\t\tN_(\"whitespace\"), N_(\"passed to 'git apply'\")),\n-\t\tOPT_SET_INT('C', NULL, &opt_c, N_(\"passed to 'git apply'\"),\n-\t\t\t REBASE_AM),\n \t\tOPT_BOOL(0, \"autostash\", &options.autostash,\n \t\t\t N_(\"automatically stash/stash pop before and after\")),\n \t\tOPT_STRING_LIST('x', \"exec\", &exec, N_(\"exec\"),\n@@ -884,6 +882,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"rebase all reachable commits up to the root(s)\")),\n \t\tOPT_END(),\n \t};\n+\tint i;\n \n \t/*\n \t * NEEDSWORK: Once the builtin rebase has been tested enough\n@@ -1064,22 +1063,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t state_dir_base, cmd_live_rebase, buf.buf);\n \t}\n \n-\tif (!(options.flags & REBASE_NO_QUIET))\n-\t\tstrbuf_addstr(&options.git_am_opt, \" -q\");\n-\n-\tif (committer_date_is_author_date) {\n-\t\tstrbuf_addstr(&options.git_am_opt,\n-\t\t\t \" --committer-date-is-author-date\");\n-\t\toptions.flags |= REBASE_FORCE;\n+\tfor (i = 0; i < options.git_am_opts.argc; i++) {\n+\t\tconst char *option = options.git_am_opts.argv[i];\n+\t\tif (!strcmp(option, \"--committer-date-is-author-date\") ||\n+\t\t !strcmp(option, \"--ignore-date\") ||\n+\t\t !strcmp(option, \"--whitespace=fix\") ||\n+\t\t !strcmp(option, \"--whitespace=strip\"))\n+\t\t\toptions.flags |= REBASE_FORCE;\n \t}\n \n-\tif (ignore_whitespace)\n-\t\tstrbuf_addstr(&options.git_am_opt, \" --ignore-whitespace\");\n-\n-\tif (ignore_date) {\n-\t\tstrbuf_addstr(&options.git_am_opt, \" --ignore-date\");\n-\t\toptions.flags |= REBASE_FORCE;\n-\t}\n+\tif (!(options.flags & REBASE_NO_QUIET))\n+\t\targv_array_push(&options.git_am_opts, \"-q\");\n \n \tif (options.keep_empty)\n \t\timply_interactive(&options, \"--keep-empty\");\n@@ -1089,23 +1083,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\n \t}\n \n-\tif (opt_c >= 0)\n-\t\tstrbuf_addf(&options.git_am_opt, \" -C%d\", opt_c);\n-\n-\tif (whitespace.nr) {\n-\t\tint i;\n-\n-\t\tfor (i = 0; i < whitespace.nr; i++) {\n-\t\t\tconst char *item = whitespace.items[i].string;\n-\n-\t\t\tstrbuf_addf(&options.git_am_opt, \" --whitespace=%s\",\n-\t\t\t\t item);\n-\n-\t\t\tif ((!strcmp(item, \"fix\")) || (!strcmp(item, \"strip\")))\n-\t\t\t\toptions.flags |= REBASE_FORCE;\n-\t\t}\n-\t}\n-\n \tif (exec.nr) {\n \t\tint i;\n \n@@ -1181,23 +1158,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tbreak;\n \t}\n \n-\tif (options.git_am_opt.len) {\n-\t\tconst char *p;\n-\n+\tif (options.git_am_opts.argc) {\n \t\t/* all am options except -q are compatible only with --am */\n-\t\tstrbuf_reset(&buf);\n-\t\tstrbuf_addbuf(&buf, &options.git_am_opt);\n-\t\tstrbuf_addch(&buf, ' ');\n-\t\twhile ((p = strstr(buf.buf, \" -q \")))\n-\t\t\tstrbuf_splice(&buf, p - buf.buf, 4, \" \", 1);\n-\t\tstrbuf_trim(&buf);\n+\t\tfor (i = options.git_am_opts.argc - 1; i >= 0; i--)\n+\t\t\tif (strcmp(options.git_am_opts.argv[i], \"-q\"))\n+\t\t\t\tbreak;\n \n-\t\tif (is_interactive(&options) && buf.len)\n+\t\tif (is_interactive(&options) && i >= 0)\n \t\t\tdie(_(\"error: cannot combine interactive options \"\n \t\t\t \"(--interactive, --exec, --rebase-merges, \"\n \t\t\t \"--preserve-merges, --keep-empty, --root + \"\n \t\t\t \"--onto) with am options (%s)\"), buf.buf);\n-\t\tif (options.type == REBASE_MERGE && buf.len)\n+\t\tif (options.type == REBASE_MERGE && i >= 0)\n \t\t\tdie(_(\"error: cannot combine merge options (--merge, \"\n \t\t\t \"--strategy, --strategy-option) with am options \"\n \t\t\t \"(%s)\"), buf.buf);\n@@ -1207,7 +1179,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tif (options.type == REBASE_PRESERVE_MERGES)\n \t\t\tdie(\"cannot combine '--signoff' with \"\n \t\t\t \"'--preserve-merges'\");\n-\t\tstrbuf_addstr(&options.git_am_opt, \" --signoff\");\n+\t\targv_array_push(&options.git_am_opts, \"--signoff\");\n \t\toptions.flags |= REBASE_FORCE;\n \t}\n \n-- \ngitgitgadget\n"},{"id":"363150","messageId":"xmqqlg5xunzh.fsf@gitster-ct.c.googlers.com","threadId":"49817","inReplyTo":"dc36a450680b1854abbb9bb06180001ce68c3f3b.1542112703.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] rebase: really just passthru the `git am` options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-13T13:05:22Z","receivedAt":"2018-11-13T13:05:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> However, there is a much better way (that I was unaware of, at the time\n> when I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.\n> It is intended for exactly this use case, where command-line options\n> want to be parsed into a separate `argv_array`.\n>\n> Let's use this feature.\n>\n> Incidentally, this also allows us to address a bug discovered by Phillip\n> Wood, where the built-in rebase failed to understand that the `-C`\n> option takes an optional argument.\n\nThe resulting code does show what is going on more clearly. I\nwonder if we can do the same for -S as well? It needs to be passed\nto other backends, so I guess it is not such a good idea.\n\n> -\t\t.git_am_opt = STRBUF_INIT,\n> +\t\t.git_am_opts = ARGV_ARRAY_INIT,\n\nYes, this is much nicer.\n\n> @@ -856,10 +858,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &gpg_sign, N_(\"key-id\"),\n> \t\t\tN_(\"GPG-sign commits\"),\n> \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n> -\t\tOPT_STRING_LIST(0, \"whitespace\", &whitespace,\n> -\t\t\t\tN_(\"whitespace\"), N_(\"passed to 'git apply'\")),\n> -\t\tOPT_SET_INT('C', NULL, &opt_c, N_(\"passed to 'git apply'\"),\n> -\t\t\t REBASE_AM),\n> \t\tOPT_BOOL(0, \"autostash\", &options.autostash,\n> \t\t\t N_(\"automatically stash/stash pop before and after\")),\n> \t\tOPT_STRING_LIST('x', \"exec\", &exec, N_(\"exec\"),\n> @@ -884,6 +882,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\t N_(\"rebase all reachable commits up to the root(s)\")),\n> \t\tOPT_END(),\n> \t};\n> +\tint i;\n> \n> +\tfor (i = 0; i < options.git_am_opts.argc; i++) {\n\nYes, this is very nice way to first collect and then inspect them\nseparately.\n\n> +\t\tconst char *option = options.git_am_opts.argv[i];\n> +\t\tif (!strcmp(option, \"--committer-date-is-author-date\") ||\n> +\t\t !strcmp(option, \"--ignore-date\") ||\n> +\t\t !strcmp(option, \"--whitespace=fix\") ||\n> +\t\t !strcmp(option, \"--whitespace=strip\"))\n> +\t\t\toptions.flags |= REBASE_FORCE;\n> \t}\n\nOverall very nicely done. Perhaps we'll see a test or two from\nPhilip?\n\nThanks.\n"},{"id":"363160","messageId":"00d9b0e4-dde5-c96d-76d7-42fb9ac393f8@talktalk.net","threadId":"49817","inReplyTo":"dc36a450680b1854abbb9bb06180001ce68c3f3b.1542112703.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] rebase: really just passthru the `git am` options","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-13T15:05:36Z","receivedAt":"2018-11-13T15:05:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nThanks for looking at this. Unfortunately using OPT_PASSTHRU_ARGV seems \nto break the error reporting\n\nRunning\n bin/wrappers/git rebase --onto @^^^^ @^^ -Cbad\n\nGives\n git encountered an error while preparing the patches to replay\n these revisions:\n\n \n67f673aa4a580b9e407b1ca505abf1f50510ec47...7c3e01a708856885e60bf4051586970e65dd326c\n\n As a result, git cannot rebase them.\n\nIf I do\n\n bin/wrappers/git rebase @^^ -Cbad\n\nI get no error, it just tells me that it does not need to rebase (which \nis true)\n\nBest Wishes\n\nPhillip\n\n\nOn 13/11/2018 12:38, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> Currently, we parse the options intended for `git am` as if we wanted to\n> handle them in `git rebase`, and then reconstruct them painstakingly to\n> define the `git_am_opt` variable.\n> \n> However, there is a much better way (that I was unaware of, at the time\n> when I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.\n> It is intended for exactly this use case, where command-line options\n> want to be parsed into a separate `argv_array`.\n> \n> Let's use this feature.\n> \n> Incidentally, this also allows us to address a bug discovered by Phillip\n> Wood, where the built-in rebase failed to understand that the `-C`\n> option takes an optional argument.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/rebase.c | 98 +++++++++++++++++-------------------------------\n> 1 file changed, 35 insertions(+), 63 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 0ee06aa363..96ffa80b71 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -87,7 +87,7 @@ struct rebase_options {\n> \t\tREBASE_FORCE = 1<<3,\n> \t\tREBASE_INTERACTIVE_EXPLICIT = 1<<4,\n> \t} flags;\n> -\tstruct strbuf git_am_opt;\n> +\tstruct argv_array git_am_opts;\n> \tconst char *action;\n> \tint signoff;\n> \tint allow_rerere_autoupdate;\n> @@ -339,7 +339,7 @@ N_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n> static int run_specific_rebase(struct rebase_options *opts)\n> {\n> \tconst char *argv[] = { NULL, NULL };\n> -\tstruct strbuf script_snippet = STRBUF_INIT;\n> +\tstruct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;\n> \tint status;\n> \tconst char *backend, *backend_func;\n> \n> @@ -433,7 +433,9 @@ static int run_specific_rebase(struct rebase_options *opts)\n> \t\toid_to_hex(&opts->restrict_revision->object.oid) : NULL);\n> \tadd_var(&script_snippet, \"GIT_QUIET\",\n> \t\topts->flags & REBASE_NO_QUIET ? \"\" : \"t\");\n> -\tadd_var(&script_snippet, \"git_am_opt\", opts->git_am_opt.buf);\n> +\tsq_quote_argv_pretty(&buf, opts->git_am_opts.argv);\n> +\tadd_var(&script_snippet, \"git_am_opt\", buf.buf);\n> +\tstrbuf_release(&buf);\n> \tadd_var(&script_snippet, \"verbose\",\n> \t\topts->flags & REBASE_VERBOSE ? \"t\" : \"\");\n> \tadd_var(&script_snippet, \"diffstat\",\n> @@ -756,7 +758,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \tstruct rebase_options options = {\n> \t\t.type = REBASE_UNSPECIFIED,\n> \t\t.flags = REBASE_NO_QUIET,\n> -\t\t.git_am_opt = STRBUF_INIT,\n> +\t\t.git_am_opts = ARGV_ARRAY_INIT,\n> \t\t.allow_rerere_autoupdate = -1,\n> \t\t.allow_empty_message = 1,\n> \t\t.git_format_patch_opt = STRBUF_INIT,\n> @@ -777,12 +779,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\tACTION_EDIT_TODO,\n> \t\tACTION_SHOW_CURRENT_PATCH,\n> \t} action = NO_ACTION;\n> -\tint committer_date_is_author_date = 0;\n> -\tint ignore_date = 0;\n> -\tint ignore_whitespace = 0;\n> \tconst char *gpg_sign = NULL;\n> -\tint opt_c = -1;\n> -\tstruct string_list whitespace = STRING_LIST_INIT_NODUP;\n> \tstruct string_list exec = STRING_LIST_INIT_NODUP;\n> \tconst char *rebase_merges = NULL;\n> \tint fork_point = -1;\n> @@ -804,15 +801,20 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t{OPTION_NEGBIT, 'n', \"no-stat\", &options.flags, NULL,\n> \t\t\tN_(\"do not show diffstat of what changed upstream\"),\n> \t\t\tPARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },\n> -\t\tOPT_BOOL(0, \"ignore-whitespace\", &ignore_whitespace,\n> -\t\t\t N_(\"passed to 'git apply'\")),\n> \t\tOPT_BOOL(0, \"signoff\", &options.signoff,\n> \t\t\t N_(\"add a Signed-off-by: line to each commit\")),\n> -\t\tOPT_BOOL(0, \"committer-date-is-author-date\",\n> -\t\t\t &committer_date_is_author_date,\n> -\t\t\t N_(\"passed to 'git am'\")),\n> -\t\tOPT_BOOL(0, \"ignore-date\", &ignore_date,\n> -\t\t\t N_(\"passed to 'git am'\")),\n> +\t\tOPT_PASSTHRU_ARGV(0, \"ignore-whitespace\", &options.git_am_opts,\n> +\t\t\t\t NULL, N_(\"passed to 'git am'\"),\n> +\t\t\t\t PARSE_OPT_NOARG),\n> +\t\tOPT_PASSTHRU_ARGV(0, \"committer-date-is-author-date\",\n> +\t\t\t\t &options.git_am_opts, NULL,\n> +\t\t\t\t N_(\"passed to 'git am'\"), PARSE_OPT_NOARG),\n> +\t\tOPT_PASSTHRU_ARGV(0, \"ignore-date\", &options.git_am_opts, NULL,\n> +\t\t\t\t N_(\"passed to 'git am'\"), PARSE_OPT_NOARG),\n> +\t\tOPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_(\"n\"),\n> +\t\t\t\t N_(\"passed to 'git apply'\"), 0),\n> +\t\tOPT_PASSTHRU_ARGV(0, \"whitespace\", &options.git_am_opts,\n> +\t\t\t\t N_(\"action\"), N_(\"passed to 'git apply'\"), 0),\n> \t\tOPT_BIT('f', \"force-rebase\", &options.flags,\n> \t\t\tN_(\"cherry-pick all commits, even if unchanged\"),\n> \t\t\tREBASE_FORCE),\n> @@ -856,10 +858,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &gpg_sign, N_(\"key-id\"),\n> \t\t\tN_(\"GPG-sign commits\"),\n> \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n> -\t\tOPT_STRING_LIST(0, \"whitespace\", &whitespace,\n> -\t\t\t\tN_(\"whitespace\"), N_(\"passed to 'git apply'\")),\n> -\t\tOPT_SET_INT('C', NULL, &opt_c, N_(\"passed to 'git apply'\"),\n> -\t\t\t REBASE_AM),\n> \t\tOPT_BOOL(0, \"autostash\", &options.autostash,\n> \t\t\t N_(\"automatically stash/stash pop before and after\")),\n> \t\tOPT_STRING_LIST('x', \"exec\", &exec, N_(\"exec\"),\n> @@ -884,6 +882,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\t N_(\"rebase all reachable commits up to the root(s)\")),\n> \t\tOPT_END(),\n> \t};\n> +\tint i;\n> \n> \t/*\n> \t * NEEDSWORK: Once the builtin rebase has been tested enough\n> @@ -1064,22 +1063,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t state_dir_base, cmd_live_rebase, buf.buf);\n> \t}\n> \n> -\tif (!(options.flags & REBASE_NO_QUIET))\n> -\t\tstrbuf_addstr(&options.git_am_opt, \" -q\");\n> -\n> -\tif (committer_date_is_author_date) {\n> -\t\tstrbuf_addstr(&options.git_am_opt,\n> -\t\t\t \" --committer-date-is-author-date\");\n> -\t\toptions.flags |= REBASE_FORCE;\n> +\tfor (i = 0; i < options.git_am_opts.argc; i++) {\n> +\t\tconst char *option = options.git_am_opts.argv[i];\n> +\t\tif (!strcmp(option, \"--committer-date-is-author-date\") ||\n> +\t\t !strcmp(option, \"--ignore-date\") ||\n> +\t\t !strcmp(option, \"--whitespace=fix\") ||\n> +\t\t !strcmp(option, \"--whitespace=strip\"))\n> +\t\t\toptions.flags |= REBASE_FORCE;\n> \t}\n> \n> -\tif (ignore_whitespace)\n> -\t\tstrbuf_addstr(&options.git_am_opt, \" --ignore-whitespace\");\n> -\n> -\tif (ignore_date) {\n> -\t\tstrbuf_addstr(&options.git_am_opt, \" --ignore-date\");\n> -\t\toptions.flags |= REBASE_FORCE;\n> -\t}\n> +\tif (!(options.flags & REBASE_NO_QUIET))\n> +\t\targv_array_push(&options.git_am_opts, \"-q\");\n> \n> \tif (options.keep_empty)\n> \t\timply_interactive(&options, \"--keep-empty\");\n> @@ -1089,23 +1083,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\n> \t}\n> \n> -\tif (opt_c >= 0)\n> -\t\tstrbuf_addf(&options.git_am_opt, \" -C%d\", opt_c);\n> -\n> -\tif (whitespace.nr) {\n> -\t\tint i;\n> -\n> -\t\tfor (i = 0; i < whitespace.nr; i++) {\n> -\t\t\tconst char *item = whitespace.items[i].string;\n> -\n> -\t\t\tstrbuf_addf(&options.git_am_opt, \" --whitespace=%s\",\n> -\t\t\t\t item);\n> -\n> -\t\t\tif ((!strcmp(item, \"fix\")) || (!strcmp(item, \"strip\")))\n> -\t\t\t\toptions.flags |= REBASE_FORCE;\n> -\t\t}\n> -\t}\n> -\n> \tif (exec.nr) {\n> \t\tint i;\n> \n> @@ -1181,23 +1158,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\tbreak;\n> \t}\n> \n> -\tif (options.git_am_opt.len) {\n> -\t\tconst char *p;\n> -\n> +\tif (options.git_am_opts.argc) {\n> \t\t/* all am options except -q are compatible only with --am */\n> -\t\tstrbuf_reset(&buf);\n> -\t\tstrbuf_addbuf(&buf, &options.git_am_opt);\n> -\t\tstrbuf_addch(&buf, ' ');\n> -\t\twhile ((p = strstr(buf.buf, \" -q \")))\n> -\t\t\tstrbuf_splice(&buf, p - buf.buf, 4, \" \", 1);\n> -\t\tstrbuf_trim(&buf);\n> +\t\tfor (i = options.git_am_opts.argc - 1; i >= 0; i--)\n> +\t\t\tif (strcmp(options.git_am_opts.argv[i], \"-q\"))\n> +\t\t\t\tbreak;\n> \n> -\t\tif (is_interactive(&options) && buf.len)\n> +\t\tif (is_interactive(&options) && i >= 0)\n> \t\t\tdie(_(\"error: cannot combine interactive options \"\n> \t\t\t \"(--interactive, --exec, --rebase-merges, \"\n> \t\t\t \"--preserve-merges, --keep-empty, --root + \"\n> \t\t\t \"--onto) with am options (%s)\"), buf.buf);\n> -\t\tif (options.type == REBASE_MERGE && buf.len)\n> +\t\tif (options.type == REBASE_MERGE && i >= 0)\n> \t\t\tdie(_(\"error: cannot combine merge options (--merge, \"\n> \t\t\t \"--strategy, --strategy-option) with am options \"\n> \t\t\t \"(%s)\"), buf.buf);\n> @@ -1207,7 +1179,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\tif (options.type == REBASE_PRESERVE_MERGES)\n> \t\t\tdie(\"cannot combine '--signoff' with \"\n> \t\t\t \"'--preserve-merges'\");\n> -\t\tstrbuf_addstr(&options.git_am_opt, \" --signoff\");\n> +\t\targv_array_push(&options.git_am_opts, \"--signoff\");\n> \t\toptions.flags |= REBASE_FORCE;\n> \t}\n> \n> \n\n"},{"id":"363200","messageId":"nycvar.QRO.7.76.6.1811132019440.39@tvgsbejvaqbjf.bet","threadId":"49817","inReplyTo":"00d9b0e4-dde5-c96d-76d7-42fb9ac393f8@talktalk.net","subject":"Re: [PATCH 1/1] rebase: really just passthru the `git am` options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-13T19:21:19Z","receivedAt":"2018-11-13T19:21:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Tue, 13 Nov 2018, Phillip Wood wrote:\n\n> Thanks for looking at this. Unfortunately using OPT_PASSTHRU_ARGV seems to\n> break the error reporting\n> \n> Running\n> bin/wrappers/git rebase --onto @^^^^ @^^ -Cbad\n> \n> Gives\n> git encountered an error while preparing the patches to replay\n> these revisions:\n> \n> \n> 67f673aa4a580b9e407b1ca505abf1f50510ec47...7c3e01a708856885e60bf4051586970e65dd326c\n> \n> As a result, git cannot rebase them.\n> \n> If I do\n> \n> bin/wrappers/git rebase @^^ -Cbad\n> \n> I get no error, it just tells me that it does not need to rebase (which is\n> true)\n\nHmm. Isn't this the same behavior as with the scripted version? Also: are\nwe sure that we want to allow options to come *after* the ``\nargument?\n\nCiao,\nDscho\n\n> Best Wishes\n> \n> Phillip\n> \n> \n> On 13/11/2018 12:38, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> > \n> > Currently, we parse the options intended for `git am` as if we wanted to\n> > handle them in `git rebase`, and then reconstruct them painstakingly to\n> > define the `git_am_opt` variable.\n> > \n> > However, there is a much better way (that I was unaware of, at the time\n> > when I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.\n> > It is intended for exactly this use case, where command-line options\n> > want to be parsed into a separate `argv_array`.\n> > \n> > Let's use this feature.\n> > \n> > Incidentally, this also allows us to address a bug discovered by Phillip\n> > Wood, where the built-in rebase failed to understand that the `-C`\n> > option takes an optional argument.\n> > \n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > builtin/rebase.c | 98 +++++++++++++++++-------------------------------\n> > 1 file changed, 35 insertions(+), 63 deletions(-)\n> > \n> > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > index 0ee06aa363..96ffa80b71 100644\n> > --- a/builtin/rebase.c\n> > +++ b/builtin/rebase.c\n> > @@ -87,7 +87,7 @@ struct rebase_options {\n> > REBASE_FORCE = 1<<3,\n> > REBASE_INTERACTIVE_EXPLICIT = 1<<4,\n> > \t} flags;\n> > -\tstruct strbuf git_am_opt;\n> > +\tstruct argv_array git_am_opts;\n> > const char *action;\n> > int signoff;\n> > int allow_rerere_autoupdate;\n> > @@ -339,7 +339,7 @@ N_(\"Resolve all conflicts manually, mark them as\n> > resolved with\\n\"\n> > static int run_specific_rebase(struct rebase_options *opts)\n> > {\n> > \tconst char *argv[] = { NULL, NULL };\n> > -\tstruct strbuf script_snippet = STRBUF_INIT;\n> > +\tstruct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;\n> > int status;\n> > const char *backend, *backend_func;\n> > @@ -433,7 +433,9 @@ static int run_specific_rebase(struct rebase_options\n> > *opts)\n> > \toid_to_hex(&opts->restrict_revision->object.oid) : NULL);\n> > add_var(&script_snippet, \"GIT_QUIET\",\n> > \t\topts->flags & REBASE_NO_QUIET ? \"\" : \"t\");\n> > -\tadd_var(&script_snippet, \"git_am_opt\", opts->git_am_opt.buf);\n> > +\tsq_quote_argv_pretty(&buf, opts->git_am_opts.argv);\n> > +\tadd_var(&script_snippet, \"git_am_opt\", buf.buf);\n> > +\tstrbuf_release(&buf);\n> > add_var(&script_snippet, \"verbose\",\n> > \topts->flags & REBASE_VERBOSE ? \"t\" : \"\");\n> > \tadd_var(&script_snippet, \"diffstat\",\n> > @@ -756,7 +758,7 @@ int cmd_rebase(int argc, const char **argv, const char\n> > *prefix)\n> > struct rebase_options options = {\n> > .type = REBASE_UNSPECIFIED,\n> > .flags = REBASE_NO_QUIET,\n> > -\t\t.git_am_opt = STRBUF_INIT,\n> > +\t\t.git_am_opts = ARGV_ARRAY_INIT,\n> > .allow_rerere_autoupdate = -1,\n> > .allow_empty_message = 1,\n> > .git_format_patch_opt = STRBUF_INIT,\n> > @@ -777,12 +779,7 @@ int cmd_rebase(int argc, const char **argv, const\n> > char *prefix)\n> > ACTION_EDIT_TODO,\n> > ACTION_SHOW_CURRENT_PATCH,\n> > \t} action = NO_ACTION;\n> > -\tint committer_date_is_author_date = 0;\n> > -\tint ignore_date = 0;\n> > -\tint ignore_whitespace = 0;\n> > \tconst char *gpg_sign = NULL;\n> > -\tint opt_c = -1;\n> > -\tstruct string_list whitespace = STRING_LIST_INIT_NODUP;\n> > struct string_list exec = STRING_LIST_INIT_NODUP;\n> > const char *rebase_merges = NULL;\n> > int fork_point = -1;\n> > @@ -804,15 +801,20 @@ int cmd_rebase(int argc, const char **argv, const\n> > char *prefix)\n> > {OPTION_NEGBIT, 'n', \"no-stat\", &options.flags, NULL,\n> > N_(\"do not show diffstat of what changed upstream\"),\n> > PARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },\n> > -\t\tOPT_BOOL(0, \"ignore-whitespace\", &ignore_whitespace,\n> > -\t\t\t N_(\"passed to 'git apply'\")),\n> > OPT_BOOL(0, \"signoff\", &options.signoff,\n> > \t\t\t N_(\"add a Signed-off-by: line to each commit\")),\n> > -\t\tOPT_BOOL(0, \"committer-date-is-author-date\",\n> > -\t\t\t &committer_date_is_author_date,\n> > -\t\t\t N_(\"passed to 'git am'\")),\n> > -\t\tOPT_BOOL(0, \"ignore-date\", &ignore_date,\n> > -\t\t\t N_(\"passed to 'git am'\")),\n> > +\t\tOPT_PASSTHRU_ARGV(0, \"ignore-whitespace\",\n> > &options.git_am_opts,\n> > +\t\t\t\t NULL, N_(\"passed to 'git am'\"),\n> > +\t\t\t\t PARSE_OPT_NOARG),\n> > +\t\tOPT_PASSTHRU_ARGV(0, \"committer-date-is-author-date\",\n> > +\t\t\t\t &options.git_am_opts, NULL,\n> > +\t\t\t\t N_(\"passed to 'git am'\"),\n> > PARSE_OPT_NOARG),\n> > +\t\tOPT_PASSTHRU_ARGV(0, \"ignore-date\", &options.git_am_opts,\n> > NULL,\n> > +\t\t\t\t N_(\"passed to 'git am'\"),\n> > PARSE_OPT_NOARG),\n> > +\t\tOPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_(\"n\"),\n> > +\t\t\t\t N_(\"passed to 'git apply'\"), 0),\n> > +\t\tOPT_PASSTHRU_ARGV(0, \"whitespace\", &options.git_am_opts,\n> > +\t\t\t\t N_(\"action\"), N_(\"passed to 'git apply'\"),\n> > 0),\n> > OPT_BIT('f', \"force-rebase\", &options.flags,\n> > N_(\"cherry-pick all commits, even if unchanged\"),\n> > REBASE_FORCE),\n> > @@ -856,10 +858,6 @@ int cmd_rebase(int argc, const char **argv, const\n> > char *prefix)\n> > { OPTION_STRING, 'S', \"gpg-sign\", &gpg_sign, N_(\"key-id\"),\n> > N_(\"GPG-sign commits\"),\n> > PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n> > -\t\tOPT_STRING_LIST(0, \"whitespace\", &whitespace,\n> > -\t\t\t\tN_(\"whitespace\"), N_(\"passed to 'git\n> > apply'\")),\n> > -\t\tOPT_SET_INT('C', NULL, &opt_c, N_(\"passed to 'git apply'\"),\n> > -\t\t\t REBASE_AM),\n> > OPT_BOOL(0, \"autostash\", &options.autostash,\n> > \t N_(\"automatically stash/stash pop before and after\")),\n> > \t\tOPT_STRING_LIST('x', \"exec\", &exec, N_(\"exec\"),\n> > @@ -884,6 +882,7 @@ int cmd_rebase(int argc, const char **argv, const char\n> > *prefix)\n> > \t N_(\"rebase all reachable commits up to the root(s)\")),\n> > \tOPT_END(),\n> > \t};\n> > +\tint i;\n> > \n> > /*\n> > \t * NEEDSWORK: Once the builtin rebase has been tested enough\n> > @@ -1064,22 +1063,17 @@ int cmd_rebase(int argc, const char **argv, const\n> > char *prefix)\n> > \t state_dir_base, cmd_live_rebase, buf.buf);\n> > }\n> > -\tif (!(options.flags & REBASE_NO_QUIET))\n> > -\t\tstrbuf_addstr(&options.git_am_opt, \" -q\");\n> > -\n> > -\tif (committer_date_is_author_date) {\n> > -\t\tstrbuf_addstr(&options.git_am_opt,\n> > -\t\t\t \" --committer-date-is-author-date\");\n> > -\t\toptions.flags |= REBASE_FORCE;\n> > +\tfor (i = 0; i < options.git_am_opts.argc; i++) {\n> > +\t\tconst char *option = options.git_am_opts.argv[i];\n> > +\t\tif (!strcmp(option, \"--committer-date-is-author-date\") ||\n> > +\t\t !strcmp(option, \"--ignore-date\") ||\n> > +\t\t !strcmp(option, \"--whitespace=fix\") ||\n> > +\t\t !strcmp(option, \"--whitespace=strip\"))\n> > +\t\t\toptions.flags |= REBASE_FORCE;\n> > }\n> > -\tif (ignore_whitespace)\n> > -\t\tstrbuf_addstr(&options.git_am_opt, \" --ignore-whitespace\");\n> > -\n> > -\tif (ignore_date) {\n> > -\t\tstrbuf_addstr(&options.git_am_opt, \" --ignore-date\");\n> > -\t\toptions.flags |= REBASE_FORCE;\n> > -\t}\n> > +\tif (!(options.flags & REBASE_NO_QUIET))\n> > +\t\targv_array_push(&options.git_am_opts, \"-q\");\n> > \n> > if (options.keep_empty)\n> > \t\timply_interactive(&options, \"--keep-empty\");\n> > @@ -1089,23 +1083,6 @@ int cmd_rebase(int argc, const char **argv, const\n> > char *prefix)\n> > \toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\n> > }\n> > -\tif (opt_c >= 0)\n> > -\t\tstrbuf_addf(&options.git_am_opt, \" -C%d\", opt_c);\n> > -\n> > -\tif (whitespace.nr) {\n> > -\t\tint i;\n> > -\n> > -\t\tfor (i = 0; i < whitespace.nr; i++) {\n> > -\t\t\tconst char *item = whitespace.items[i].string;\n> > -\n> > -\t\t\tstrbuf_addf(&options.git_am_opt, \" --whitespace=%s\",\n> > -\t\t\t\t item);\n> > -\n> > -\t\t\tif ((!strcmp(item, \"fix\")) || (!strcmp(item,\n> > \"strip\")))\n> > -\t\t\t\toptions.flags |= REBASE_FORCE;\n> > -\t\t}\n> > -\t}\n> > -\n> > if (exec.nr) {\n> > int i;\n> > @@ -1181,23 +1158,18 @@ int cmd_rebase(int argc, const char **argv,\n> > const char *prefix)\n> > \tbreak;\n> > }\n> > -\tif (options.git_am_opt.len) {\n> > -\t\tconst char *p;\n> > -\n> > +\tif (options.git_am_opts.argc) {\n> > \t\t/* all am options except -q are compatible only with --am */\n> > -\t\tstrbuf_reset(&buf);\n> > -\t\tstrbuf_addbuf(&buf, &options.git_am_opt);\n> > -\t\tstrbuf_addch(&buf, ' ');\n> > -\t\twhile ((p = strstr(buf.buf, \" -q \")))\n> > -\t\t\tstrbuf_splice(&buf, p - buf.buf, 4, \" \", 1);\n> > -\t\tstrbuf_trim(&buf);\n> > +\t\tfor (i = options.git_am_opts.argc - 1; i >= 0; i--)\n> > +\t\t\tif (strcmp(options.git_am_opts.argv[i], \"-q\"))\n> > +\t\t\t\tbreak;\n> > -\t\tif (is_interactive(&options) && buf.len)\n> > +\t\tif (is_interactive(&options) && i >= 0)\n> > die(_(\"error: cannot combine interactive options \"\n> > \"(--interactive, --exec, --rebase-merges, \"\n> > \"--preserve-merges, --keep-empty, --root + \"\n> > \"--onto) with am options (%s)\"), buf.buf);\n> > -\t\tif (options.type == REBASE_MERGE && buf.len)\n> > +\t\tif (options.type == REBASE_MERGE && i >= 0)\n> > die(_(\"error: cannot combine merge options (--merge, \"\n> > \"--strategy, --strategy-option) with am options \"\n> > \"(%s)\"), buf.buf);\n> > @@ -1207,7 +1179,7 @@ int cmd_rebase(int argc, const char **argv, const\n> > char *prefix)\n> > if (options.type == REBASE_PRESERVE_MERGES)\n> > die(\"cannot combine '--signoff' with \"\n> > \t\t\t \"'--preserve-merges'\");\n> > -\t\tstrbuf_addstr(&options.git_am_opt, \" --signoff\");\n> > +\t\targv_array_push(&options.git_am_opts, \"--signoff\");\n> > \toptions.flags |= REBASE_FORCE;\n> > }\n> > \n> > \n> \n> \n"},{"id":"363212","messageId":"1b8461d1-6cb7-6622-94d2-44c27623236d@talktalk.net","threadId":"49817","inReplyTo":"nycvar.QRO.7.76.6.1811132019440.39@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] rebase: really just passthru the `git am` options","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-13T19:58:55Z","receivedAt":"2018-11-13T19:59:02Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nOn 13/11/2018 19:21, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Tue, 13 Nov 2018, Phillip Wood wrote:\n> \n>> Thanks for looking at this. Unfortunately using OPT_PASSTHRU_ARGV seems to\n>> break the error reporting\n>>\n>> Running\n>> bin/wrappers/git rebase --onto @^^^^ @^^ -Cbad\n>>\n>> Gives\n>> git encountered an error while preparing the patches to replay\n>> these revisions:\n>>\n>>\n>> 67f673aa4a580b9e407b1ca505abf1f50510ec47...7c3e01a708856885e60bf4051586970e65dd326c\n>>\n>> As a result, git cannot rebase them.\n>>\n\ngit 2.19.1 gives\n\nFirst, rewinding head to replay your work on top of it...\nApplying: Ninth batch for 2.20\nerror: switch `C' expects a numerical value\n\nSo it has a clear message as to what the error is, this patch regresses \nthat. It would be better if rebase detected the error before starting \nthough.\n\n>> If I do\n>>\n>> bin/wrappers/git rebase @^^ -Cbad\n>>\n>> I get no error, it just tells me that it does not need to rebase (which is\n>> true)\n> \n> Hmm. Isn't this the same behavior as with the scripted version?\n\nAh you're right the script does not check if the option argument is valid.\n\n> Also: are\n> we sure that we want to allow options to come *after* the ``\n> argument?\n\nMaybe not but the scripted version does. I'm not sure if that is a good \nidea or not.\n\nBest Wishes\n\nPhillip\n\n> Ciao,\n> Dscho\n> \n>> Best Wishes\n>>\n>> Phillip\n>>\n>>\n>> On 13/11/2018 12:38, Johannes Schindelin via GitGitGadget wrote:\n>>> From: Johannes Schindelin \n>>>\n>>> Currently, we parse the options intended for `git am` as if we wanted to\n>>> handle them in `git rebase`, and then reconstruct them painstakingly to\n>>> define the `git_am_opt` variable.\n>>>\n>>> However, there is a much better way (that I was unaware of, at the time\n>>> when I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.\n>>> It is intended for exactly this use case, where command-line options\n>>> want to be parsed into a separate `argv_array`.\n>>>\n>>> Let's use this feature.\n>>>\n>>> Incidentally, this also allows us to address a bug discovered by Phillip\n>>> Wood, where the built-in rebase failed to understand that the `-C`\n>>> option takes an optional argument.\n>>>\n>>> Signed-off-by: Johannes Schindelin \n>>> ---\n>>> builtin/rebase.c | 98 +++++++++++++++++-------------------------------\n>>> 1 file changed, 35 insertions(+), 63 deletions(-)\n>>>\n>>> diff --git a/builtin/rebase.c b/builtin/rebase.c\n>>> index 0ee06aa363..96ffa80b71 100644\n>>> --- a/builtin/rebase.c\n>>> +++ b/builtin/rebase.c\n>>> @@ -87,7 +87,7 @@ struct rebase_options {\n>>> REBASE_FORCE = 1<<3,\n>>> REBASE_INTERACTIVE_EXPLICIT = 1<<4,\n>>> \t} flags;\n>>> -\tstruct strbuf git_am_opt;\n>>> +\tstruct argv_array git_am_opts;\n>>> const char *action;\n>>> int signoff;\n>>> int allow_rerere_autoupdate;\n>>> @@ -339,7 +339,7 @@ N_(\"Resolve all conflicts manually, mark them as\n>>> resolved with\\n\"\n>>> static int run_specific_rebase(struct rebase_options *opts)\n>>> {\n>>> \tconst char *argv[] = { NULL, NULL };\n>>> -\tstruct strbuf script_snippet = STRBUF_INIT;\n>>> +\tstruct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;\n>>> int status;\n>>> const char *backend, *backend_func;\n>>> @@ -433,7 +433,9 @@ static int run_specific_rebase(struct rebase_options\n>>> *opts)\n>>> \toid_to_hex(&opts->restrict_revision->object.oid) : NULL);\n>>> add_var(&script_snippet, \"GIT_QUIET\",\n>>> \t\topts->flags & REBASE_NO_QUIET ? \"\" : \"t\");\n>>> -\tadd_var(&script_snippet, \"git_am_opt\", opts->git_am_opt.buf);\n>>> +\tsq_quote_argv_pretty(&buf, opts->git_am_opts.argv);\n>>> +\tadd_var(&script_snippet, \"git_am_opt\", buf.buf);\n>>> +\tstrbuf_release(&buf);\n>>> add_var(&script_snippet, \"verbose\",\n>>> \topts->flags & REBASE_VERBOSE ? \"t\" : \"\");\n>>> \tadd_var(&script_snippet, \"diffstat\",\n>>> @@ -756,7 +758,7 @@ int cmd_rebase(int argc, const char **argv, const char\n>>> *prefix)\n>>> struct rebase_options options = {\n>>> .type = REBASE_UNSPECIFIED,\n>>> .flags = REBASE_NO_QUIET,\n>>> -\t\t.git_am_opt = STRBUF_INIT,\n>>> +\t\t.git_am_opts = ARGV_ARRAY_INIT,\n>>> .allow_rerere_autoupdate = -1,\n>>> .allow_empty_message = 1,\n>>> .git_format_patch_opt = STRBUF_INIT,\n>>> @@ -777,12 +779,7 @@ int cmd_rebase(int argc, const char **argv, const\n>>> char *prefix)\n>>> ACTION_EDIT_TODO,\n>>> ACTION_SHOW_CURRENT_PATCH,\n>>> \t} action = NO_ACTION;\n>>> -\tint committer_date_is_author_date = 0;\n>>> -\tint ignore_date = 0;\n>>> -\tint ignore_whitespace = 0;\n>>> \tconst char *gpg_sign = NULL;\n>>> -\tint opt_c = -1;\n>>> -\tstruct string_list whitespace = STRING_LIST_INIT_NODUP;\n>>> struct string_list exec = STRING_LIST_INIT_NODUP;\n>>> const char *rebase_merges = NULL;\n>>> int fork_point = -1;\n>>> @@ -804,15 +801,20 @@ int cmd_rebase(int argc, const char **argv, const\n>>> char *prefix)\n>>> {OPTION_NEGBIT, 'n', \"no-stat\", &options.flags, NULL,\n>>> N_(\"do not show diffstat of what changed upstream\"),\n>>> PARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },\n>>> -\t\tOPT_BOOL(0, \"ignore-whitespace\", &ignore_whitespace,\n>>> -\t\t\t N_(\"passed to 'git apply'\")),\n>>> OPT_BOOL(0, \"signoff\", &options.signoff,\n>>> \t\t\t N_(\"add a Signed-off-by: line to each commit\")),\n>>> -\t\tOPT_BOOL(0, \"committer-date-is-author-date\",\n>>> -\t\t\t &committer_date_is_author_date,\n>>> -\t\t\t N_(\"passed to 'git am'\")),\n>>> -\t\tOPT_BOOL(0, \"ignore-date\", &ignore_date,\n>>> -\t\t\t N_(\"passed to 'git am'\")),\n>>> +\t\tOPT_PASSTHRU_ARGV(0, \"ignore-whitespace\",\n>>> &options.git_am_opts,\n>>> +\t\t\t\t NULL, N_(\"passed to 'git am'\"),\n>>> +\t\t\t\t PARSE_OPT_NOARG),\n>>> +\t\tOPT_PASSTHRU_ARGV(0, \"committer-date-is-author-date\",\n>>> +\t\t\t\t &options.git_am_opts, NULL,\n>>> +\t\t\t\t N_(\"passed to 'git am'\"),\n>>> PARSE_OPT_NOARG),\n>>> +\t\tOPT_PASSTHRU_ARGV(0, \"ignore-date\", &options.git_am_opts,\n>>> NULL,\n>>> +\t\t\t\t N_(\"passed to 'git am'\"),\n>>> PARSE_OPT_NOARG),\n>>> +\t\tOPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_(\"n\"),\n>>> +\t\t\t\t N_(\"passed to 'git apply'\"), 0),\n>>> +\t\tOPT_PASSTHRU_ARGV(0, \"whitespace\", &options.git_am_opts,\n>>> +\t\t\t\t N_(\"action\"), N_(\"passed to 'git apply'\"),\n>>> 0),\n>>> OPT_BIT('f', \"force-rebase\", &options.flags,\n>>> N_(\"cherry-pick all commits, even if unchanged\"),\n>>> REBASE_FORCE),\n>>> @@ -856,10 +858,6 @@ int cmd_rebase(int argc, const char **argv, const\n>>> char *prefix)\n>>> { OPTION_STRING, 'S', \"gpg-sign\", &gpg_sign, N_(\"key-id\"),\n>>> N_(\"GPG-sign commits\"),\n>>> PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n>>> -\t\tOPT_STRING_LIST(0, \"whitespace\", &whitespace,\n>>> -\t\t\t\tN_(\"whitespace\"), N_(\"passed to 'git\n>>> apply'\")),\n>>> -\t\tOPT_SET_INT('C', NULL, &opt_c, N_(\"passed to 'git apply'\"),\n>>> -\t\t\t REBASE_AM),\n>>> OPT_BOOL(0, \"autostash\", &options.autostash,\n>>> \t N_(\"automatically stash/stash pop before and after\")),\n>>> \t\tOPT_STRING_LIST('x', \"exec\", &exec, N_(\"exec\"),\n>>> @@ -884,6 +882,7 @@ int cmd_rebase(int argc, const char **argv, const char\n>>> *prefix)\n>>> \t N_(\"rebase all reachable commits up to the root(s)\")),\n>>> \tOPT_END(),\n>>> \t};\n>>> +\tint i;\n>>> \n>>> /*\n>>> \t * NEEDSWORK: Once the builtin rebase has been tested enough\n>>> @@ -1064,22 +1063,17 @@ int cmd_rebase(int argc, const char **argv, const\n>>> char *prefix)\n>>> \t state_dir_base, cmd_live_rebase, buf.buf);\n>>> }\n>>> -\tif (!(options.flags & REBASE_NO_QUIET))\n>>> -\t\tstrbuf_addstr(&options.git_am_opt, \" -q\");\n>>> -\n>>> -\tif (committer_date_is_author_date) {\n>>> -\t\tstrbuf_addstr(&options.git_am_opt,\n>>> -\t\t\t \" --committer-date-is-author-date\");\n>>> -\t\toptions.flags |= REBASE_FORCE;\n>>> +\tfor (i = 0; i < options.git_am_opts.argc; i++) {\n>>> +\t\tconst char *option = options.git_am_opts.argv[i];\n>>> +\t\tif (!strcmp(option, \"--committer-date-is-author-date\") ||\n>>> +\t\t !strcmp(option, \"--ignore-date\") ||\n>>> +\t\t !strcmp(option, \"--whitespace=fix\") ||\n>>> +\t\t !strcmp(option, \"--whitespace=strip\"))\n>>> +\t\t\toptions.flags |= REBASE_FORCE;\n>>> }\n>>> -\tif (ignore_whitespace)\n>>> -\t\tstrbuf_addstr(&options.git_am_opt, \" --ignore-whitespace\");\n>>> -\n>>> -\tif (ignore_date) {\n>>> -\t\tstrbuf_addstr(&options.git_am_opt, \" --ignore-date\");\n>>> -\t\toptions.flags |= REBASE_FORCE;\n>>> -\t}\n>>> +\tif (!(options.flags & REBASE_NO_QUIET))\n>>> +\t\targv_array_push(&options.git_am_opts, \"-q\");\n>>> \n>>> if (options.keep_empty)\n>>> \t\timply_interactive(&options, \"--keep-empty\");\n>>> @@ -1089,23 +1083,6 @@ int cmd_rebase(int argc, const char **argv, const\n>>> char *prefix)\n>>> \toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\n>>> }\n>>> -\tif (opt_c >= 0)\n>>> -\t\tstrbuf_addf(&options.git_am_opt, \" -C%d\", opt_c);\n>>> -\n>>> -\tif (whitespace.nr) {\n>>> -\t\tint i;\n>>> -\n>>> -\t\tfor (i = 0; i < whitespace.nr; i++) {\n>>> -\t\t\tconst char *item = whitespace.items[i].string;\n>>> -\n>>> -\t\t\tstrbuf_addf(&options.git_am_opt, \" --whitespace=%s\",\n>>> -\t\t\t\t item);\n>>> -\n>>> -\t\t\tif ((!strcmp(item, \"fix\")) || (!strcmp(item,\n>>> \"strip\")))\n>>> -\t\t\t\toptions.flags |= REBASE_FORCE;\n>>> -\t\t}\n>>> -\t}\n>>> -\n>>> if (exec.nr) {\n>>> int i;\n>>> @@ -1181,23 +1158,18 @@ int cmd_rebase(int argc, const char **argv,\n>>> const char *prefix)\n>>> \tbreak;\n>>> }\n>>> -\tif (options.git_am_opt.len) {\n>>> -\t\tconst char *p;\n>>> -\n>>> +\tif (options.git_am_opts.argc) {\n>>> \t\t/* all am options except -q are compatible only with --am */\n>>> -\t\tstrbuf_reset(&buf);\n>>> -\t\tstrbuf_addbuf(&buf, &options.git_am_opt);\n>>> -\t\tstrbuf_addch(&buf, ' ');\n>>> -\t\twhile ((p = strstr(buf.buf, \" -q \")))\n>>> -\t\t\tstrbuf_splice(&buf, p - buf.buf, 4, \" \", 1);\n>>> -\t\tstrbuf_trim(&buf);\n>>> +\t\tfor (i = options.git_am_opts.argc - 1; i >= 0; i--)\n>>> +\t\t\tif (strcmp(options.git_am_opts.argv[i], \"-q\"))\n>>> +\t\t\t\tbreak;\n>>> -\t\tif (is_interactive(&options) && buf.len)\n>>> +\t\tif (is_interactive(&options) && i >= 0)\n>>> die(_(\"error: cannot combine interactive options \"\n>>> \"(--interactive, --exec, --rebase-merges, \"\n>>> \"--preserve-merges, --keep-empty, --root + \"\n>>> \"--onto) with am options (%s)\"), buf.buf);\n>>> -\t\tif (options.type == REBASE_MERGE && buf.len)\n>>> +\t\tif (options.type == REBASE_MERGE && i >= 0)\n>>> die(_(\"error: cannot combine merge options (--merge, \"\n>>> \"--strategy, --strategy-option) with am options \"\n>>> \"(%s)\"), buf.buf);\n>>> @@ -1207,7 +1179,7 @@ int cmd_rebase(int argc, const char **argv, const\n>>> char *prefix)\n>>> if (options.type == REBASE_PRESERVE_MERGES)\n>>> die(\"cannot combine '--signoff' with \"\n>>> \t\t\t \"'--preserve-merges'\");\n>>> -\t\tstrbuf_addstr(&options.git_am_opt, \" --signoff\");\n>>> +\t\targv_array_push(&options.git_am_opts, \"--signoff\");\n>>> \toptions.flags |= REBASE_FORCE;\n>>> }\n>>> \n>>>\n>>\n>>\n\n"},{"id":"363221","messageId":"87y39w1wc2.fsf@evledraar.gmail.com","threadId":"49817","inReplyTo":"1b8461d1-6cb7-6622-94d2-44c27623236d@talktalk.net","subject":"rebase-in-C stability for 2.20","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-13T21:50:05Z","receivedAt":"2018-11-13T21:50:50Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 13 2018, Phillip Wood wrote:\n\n> Hi Johannes\n>\n> On 13/11/2018 19:21, Johannes Schindelin wrote:\n>> Hi Phillip,\n>>\n>> On Tue, 13 Nov 2018, Phillip Wood wrote:\n>>\n>>> Thanks for looking at this. Unfortunately using OPT_PASSTHRU_ARGV seems to\n>>> break the error reporting\n>>>\n>>> Running\n>>> bin/wrappers/git rebase --onto @^^^^ @^^ -Cbad\n>>>\n>>> Gives\n>>> git encountered an error while preparing the patches to replay\n>>> these revisions:\n>>>\n>>>\n>>> 67f673aa4a580b9e407b1ca505abf1f50510ec47...7c3e01a708856885e60bf4051586970e65dd326c\n>>>\n>>> As a result, git cannot rebase them.\n>>>\n>\n> git 2.19.1 gives\n>\n> First, rewinding head to replay your work on top of it...\n> Applying: Ninth batch for 2.20\n> error: switch `C' expects a numerical value\n>\n> So it has a clear message as to what the error is, this patch\n> regresses that. It would be better if rebase detected the error before\n> starting though.\n>\n>>> If I do\n>>>\n>>> bin/wrappers/git rebase @^^ -Cbad\n>>>\n>>> I get no error, it just tells me that it does not need to rebase (which is\n>>> true)\n>>\n>> Hmm. Isn't this the same behavior as with the scripted version?\n>\n> Ah you're right the script does not check if the option argument is valid.\n>\n>> Also: are\n>> we sure that we want to allow options to come *after* the ``\n>> argument?\n>\n> Maybe not but the scripted version does. I'm not sure if that is a\n> good idea or not.\n\nAccording to Junio's calendar we're now 2 days from 2.20-rc0. We have\nthe js/rebase-autostash-detach-fix bug I reported sitting in \"pu\" still,\nand then this.\n\nI see we still have rebase.useBuiltin in the code as an escape hatch,\nbut it's undocumented.\n\nGiven that we're still finding regressions bugs in the rebase-in-C\nversion should we be considering reverting 5541bd5b8f (\"rebase: default\nto using the builtin rebase\", 2018-08-08)?\n\nI love the feature, but fear that the current list of known regressions\nserve as a canary for a larger list which we'd discover if we held off\nfor another major release (and would re-enable rebase.useBuiltin=true in\nmaster right after 2.20 is out the door).\n\nBut maybe I'm being overly paranoid. What do those more familiar with\nthis think?\n"},{"id":"363235","messageId":"CAGZ79kYX4vNMYgzitY1D8Bu7o=2oY6Aiq0d7KQdCTaC5eu=RYg@mail.gmail.com","threadId":"49817","inReplyTo":"87y39w1wc2.fsf@evledraar.gmail.com","subject":"Re: rebase-in-C stability for 2.20","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-14T00:07:33Z","receivedAt":"2018-11-14T00:07:58Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> But maybe I'm being overly paranoid. What do those more familiar with\n> this think?\n\nI am not too worried,\n* as rebase is a main porcelain, that is even hard to use in a script.\n so any failures are not deep down in some automation,\n but when found exposed quickly (and hopefully reported).\n* 5541bd5b8f was merged to next a month ago; internally we\n distribute the next branch to Googlers (on a weekly basis)\n and we have not had any bug reports regarding rebase.\n (Maybe our environment is too strict for the wide range\n of bugs reported)\n* Johannes reported that the rebase is used in GfW\n https://public-inbox.org/git/nycvar.QRO.7.76.6.1808241320540.73@tvgsbejvaqbjf.bet/\n https://github.com/git-for-windows/git/pull/1800\n and from my cursory reading it is part of\n 2.19.windows, which has a large user base.\n\n> (and would re-enable rebase.useBuiltin=true in\n> master right after 2.20 is out the door).\n\nThat would be fine with me as well, but I'd rather\ndocument rebase.useBuiltin instead of flip-flopping\nthe switch around the release.\n\nHave there been any fixes that are only in\nthe C version (has the shell version already bitrotted)?\n\nStefan\n"},{"id":"363273","messageId":"CABPp-BGOq2jAp79qU-XYkCv18WLBdw7huKsmBYTJcS2e_jzvFw@mail.gmail.com","threadId":"49817","inReplyTo":"87y39w1wc2.fsf@evledraar.gmail.com","subject":"Re: rebase-in-C stability for 2.20","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-11-14T00:36:11Z","receivedAt":"2018-11-14T00:36:26Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Nov 13, 2018 at 1:52 PM Ævar Arnfjörð Bjarmason\n wrote:\n> According to Junio's calendar we're now 2 days from 2.20-rc0. We have\n> the js/rebase-autostash-detach-fix bug I reported sitting in \"pu\" still,\n> and then this.\n>\n> I see we still have rebase.useBuiltin in the code as an escape hatch,\n> but it's undocumented.\n>\n> Given that we're still finding regressions bugs in the rebase-in-C\n> version should we be considering reverting 5541bd5b8f (\"rebase: default\n> to using the builtin rebase\", 2018-08-08)?\n\nThat date feels slightly misleading; it has a commit date of Oct 11,\nand only merged to master less than two weeks ago. Given how big the\ntwo different rebase in C series were, I'd expect a couple small\nthings to fall out after it hit master, which is what appears to be\nhappening.\n\n> I love the feature, but fear that the current list of known regressions\n> serve as a canary for a larger list which we'd discover if we held off\n> for another major release (and would re-enable rebase.useBuiltin=true in\n> master right after 2.20 is out the door).\n>\n> But maybe I'm being overly paranoid. What do those more familiar with\n> this think?\n\nI probably don't qualify as more familar, but giving my $0.02\nanyway... I'm happy that setting rebase.useBuiltin=false by default\nexists an escape hatch if things don't settle down as we get closer to\nthe release, but I'd rather wait until further into the RC cycle\nbefore going that route, personally.\n"},{"id":"363291","messageId":"xmqq8t1wqqd7.fsf@gitster-ct.c.googlers.com","threadId":"49817","inReplyTo":"87y39w1wc2.fsf@evledraar.gmail.com","subject":"Re: rebase-in-C stability for 2.20","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-14T03:39:48Z","receivedAt":"2018-11-14T03:39:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> According to Junio's calendar we're now 2 days from 2.20-rc0. We have\n> the js/rebase-autostash-detach-fix bug I reported sitting in \"pu\" still,\n> and then this.\n>\n> I see we still have rebase.useBuiltin in the code as an escape hatch,\n> but it's undocumented.\n>\n> Given that we're still finding regressions bugs in the rebase-in-C\n> version should we be considering reverting 5541bd5b8f (\"rebase: default\n> to using the builtin rebase\", 2018-08-08)?\n>\n> I love the feature, but fear that the current list of known regressions\n> serve as a canary for a larger list which we'd discover if we held off\n> for another major release (and would re-enable rebase.useBuiltin=true in\n> master right after 2.20 is out the door).\n>\n> But maybe I'm being overly paranoid. What do those more familiar with\n> this think?\n\nI was hoping that having it early in GfW 2.19 would have smoked out\nall the remaining issues, but it seems that those building from the\nsource and testing have usage patterns different enough to find more\nissues. This is a normal part of the development process, and\nhopefully the remaining bugs are minor and can be flushed out in the\n-rc testing period---this kind of thing is the whole reason why we\ncode-freeze and test rcs.\n\nIt unfortunately is too late to depend on the rebase.useBuiltin as\nan escape hatch, as 'master', 'next', or anywhere else had the\n\"rebase and rebase -i in C\" series merged without it set to false\nand used in any seriousness. Quite honestly, I think we can trust\nthe rest of the \"rebase and rebase -i in C\" series much more than\nits \"use the scripted one even though we have built-in one\" part.\n\nSo if we have to seriously consider holding back, we may be better\noff ripping the whole thing out. I do not offhand know how involved\nsuch a reversion would be, though, and I am *NOT* looking forward to\nhaving do such a major surgery right before the release.\n"},{"id":"363329","messageId":"20181114072909.GD19904@sigill.intra.peff.net","threadId":"49817","inReplyTo":"pull.76.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/1] rebase: understand -C again, refactor","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-14T07:29:09Z","receivedAt":"2018-11-14T07:29:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2018 at 04:38:24AM -0800, Johannes Schindelin via GitGitGadget wrote:\n\n> Phillip Wood reported a problem where the built-in rebase did not understand\n> options like -C1, i.e. it did not expect the option argument.\n> \n> While investigating how to address this best, I stumbled upon \n> OPT_PASSTHRU_ARGV (which I was so far happily unaware of).\n\nI was unaware of it, too. Looking at the OPT_PASSTHRU and its ARGV\ncounterpart, I think the original intent was that you'd pass through\nnormal last-one-wins individual options with OPT_PASSTHRU, and then\nlist-like options with OPT_PASSTHRU_ARGV. But here you've used the\nlatter to pass sets of individual last-one-wins options.\n\nThat said, I think what you've done here is way simpler and more\nreadable than using a bunch of OPT_PASSTHRUs would have been. And even\nif it was not the original intent of the ARGV variant, I can't see any\nreason to avoid doing it this way.\n\n-Peff\n"},{"id":"363336","messageId":"20181114090144.31412-1-avarab@gmail.com","threadId":"49817","inReplyTo":"CAGZ79kYX4vNMYgzitY1D8Bu7o=2oY6Aiq0d7KQdCTaC5eu=RYg@mail.gmail.com","subject":"[PATCH 0/2] rebase.useBuiltin doc & test mode","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-14T09:01:42Z","receivedAt":"2018-11-14T09:01:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, Nov 14 2018, Stefan Beller wrote:\n\n>> But maybe I'm being overly paranoid. What do those more familiar with\n>> this think?\n>\n> I am not too worried,\n> * as rebase is a main porcelain, that is even hard to use in a script.\n> so any failures are not deep down in some automation,\n> but when found exposed quickly (and hopefully reported).\n> * 5541bd5b8f was merged to next a month ago; internally we\n> distribute the next branch to Googlers (on a weekly basis)\n> and we have not had any bug reports regarding rebase.\n> (Maybe our environment is too strict for the wide range\n> of bugs reported)\n\nI do the same at Booking.com (although at a more ad-hoc schedule) and\ngot the report whose fix is now sitting in \"pu\" noted upthread.\n\nI fear that these sorts of corporate environments, both Google's and\nBooking's, end up testing a relatively narrow featureset. Most people\nhave similar enough workflows, e.g. just using \"git pull --rebase\",\nI'd be surprised if we have more than 2-3 internal users who ever use\nthe --onto option for example.\n\n> * Johannes reported that the rebase is used in GfW\n> https://public-inbox.org/git/nycvar.QRO.7.76.6.1808241320540.73@tvgsbejvaqbjf.bet/\n> https://github.com/git-for-windows/git/pull/1800\n> and from my cursory reading it is part of\n> 2.19.windows, which has a large user base.\n>\n>> (and would re-enable rebase.useBuiltin=true in\n>> master right after 2.20 is out the door).\n>\n> That would be fine with me as well, but I'd rather\n> document rebase.useBuiltin instead of flip-flopping\n> the switch around the release.\n>\n> Have there been any fixes that are only in\n> the C version (has the shell version already bitrotted)?\n\nThat's a good question, one which I don't think we knew the answer to\nbefore the following patches. As it turns out no, we still run the\ntests without failures with GIT_TEST_REBASE_USE_BUILTIN=false.\n\nÆvar Arnfjörð Bjarmason (2):\n rebase doc: document rebase.useBuiltin\n tests: add a special setup where rebase.useBuiltin is off\n\n Documentation/config/rebase.txt | 14 ++++++++++++++\n builtin/rebase.c | 5 ++++-\n t/README | 3 +++\n 3 files changed, 21 insertions(+), 1 deletion(-)\n\n-- \n2.19.1.1182.g4ecb1133ce\n\n"},{"id":"363337","messageId":"20181114090144.31412-2-avarab@gmail.com","threadId":"49817","inReplyTo":"CAGZ79kYX4vNMYgzitY1D8Bu7o=2oY6Aiq0d7KQdCTaC5eu=RYg@mail.gmail.com","subject":"[PATCH 1/2] rebase doc: document rebase.useBuiltin","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-14T09:01:43Z","receivedAt":"2018-11-14T09:01:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The rebase.useBuiltin variable introduced in 55071ea248 (\"rebase:\nstart implementing it as a builtin\", 2018-08-07) was turned on by\ndefault in 5541bd5b8f (\"rebase: default to using the builtin rebase\",\n2018-08-08), but had no documentation.\n\nLet's document it so that users who run into any stability issues with\nthe C rewrite know there's an escape hatch[1], and make it clear that\nneeding to turn off builtin rebase means you've found a bug in git.\n\n1. https://public-inbox.org/git/87y39w1wc2.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config/rebase.txt | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt\nindex 42e1ba7575..f079bf6b7e 100644\n--- a/Documentation/config/rebase.txt\n+++ b/Documentation/config/rebase.txt\n@@ -1,3 +1,17 @@\n+rebase.useBuiltin::\n+\tSet to `false` to use the legacy shellscript implementation of\n+\tlinkgit:git-rebase[1]. Is `true` by default, which means use\n+\tthe built-in rewrite of it in C.\n++\n+The C rewrite is first included with Git version 2.20. This option\n+serves an an escape hatch to re-enable the legacy version in case any\n+bugs are found in the rewrite. This option and the shellscript version\n+of linkgit:git-rebase[1] will be removed in some future release.\n++\n+If you find some reason to set this option to `false` other than\n+one-off testing you should report the behavior difference as a bug in\n+git.\n+\n rebase.stat::\n \tWhether to show a diffstat of what changed upstream since the last\n \trebase. False by default.\n-- \n2.19.1.1182.g4ecb1133ce\n\n"},{"id":"363338","messageId":"20181114090144.31412-3-avarab@gmail.com","threadId":"49817","inReplyTo":"CAGZ79kYX4vNMYgzitY1D8Bu7o=2oY6Aiq0d7KQdCTaC5eu=RYg@mail.gmail.com","subject":"[PATCH 2/2] tests: add a special setup where rebase.useBuiltin is off","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-14T09:01:44Z","receivedAt":"2018-11-14T09:02:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a GIT_TEST_REBASE_USE_BUILTIN=false test mode which is equivalent\nto running with rebase.useBuiltin=false. This is needed to spot that\nwe're not introducing any regressions in the legacy rebase version\nwhile we're carrying both it and the new builtin version.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/rebase.c | 5 ++++-\n t/README | 3 +++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 0ee06aa363..68ad8c1149 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -48,7 +48,10 @@ static int use_builtin_rebase(void)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf out = STRBUF_INIT;\n-\tint ret;\n+\tint ret, env = git_env_bool(\"GIT_TEST_REBASE_USE_BUILTIN\", -1);\n+\n+\tif (env != -1)\n+\t\treturn env;\n \n \targv_array_pushl(&cp.args,\n \t\t\t \"config\", \"--bool\", \"rebase.usebuiltin\", NULL);\ndiff --git a/t/README b/t/README\nindex 242497455f..c719e08414 100644\n--- a/t/README\n+++ b/t/README\n@@ -348,6 +348,9 @@ GIT_TEST_MULTI_PACK_INDEX=, when true, forces the multi-pack-\n index to be written after every 'git repack' command, and overrides the\n 'core.multiPackIndex' setting to true.\n \n+GIT_TEST_REBASE_USE_BUILTIN, when false, disables the builtin\n+version of git-rebase. See 'rebase.useBuiltin' in git-config(1).\n+\n Naming Tests\n ------------\n \n-- \n2.19.1.1182.g4ecb1133ce\n\n"},{"id":"363365","messageId":"nycvar.QRO.7.76.6.1811141506100.39@tvgsbejvaqbjf.bet","threadId":"49817","inReplyTo":"20181114090144.31412-1-avarab@gmail.com","subject":"Re: [PATCH 0/2] rebase.useBuiltin doc & test mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-14T14:07:20Z","receivedAt":"2018-11-14T14:07:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Wed, 14 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> On Wed, Nov 14 2018, Stefan Beller wrote:\n> \n> >> But maybe I'm being overly paranoid. What do those more familiar with\n> >> this think?\n> >\n> > I am not too worried,\n> > * as rebase is a main porcelain, that is even hard to use in a script.\n> > so any failures are not deep down in some automation,\n> > but when found exposed quickly (and hopefully reported).\n> > * 5541bd5b8f was merged to next a month ago; internally we\n> > distribute the next branch to Googlers (on a weekly basis)\n> > and we have not had any bug reports regarding rebase.\n> > (Maybe our environment is too strict for the wide range\n> > of bugs reported)\n> \n> I do the same at Booking.com (although at a more ad-hoc schedule) and\n> got the report whose fix is now sitting in \"pu\" noted upthread.\n> \n> I fear that these sorts of corporate environments, both Google's and\n> Booking's, end up testing a relatively narrow featureset. Most people\n> have similar enough workflows, e.g. just using \"git pull --rebase\",\n> I'd be surprised if we have more than 2-3 internal users who ever use\n> the --onto option for example.\n> \n> > * Johannes reported that the rebase is used in GfW\n> > https://public-inbox.org/git/nycvar.QRO.7.76.6.1808241320540.73@tvgsbejvaqbjf.bet/\n> > https://github.com/git-for-windows/git/pull/1800\n> > and from my cursory reading it is part of\n> > 2.19.windows, which has a large user base.\n> >\n> >> (and would re-enable rebase.useBuiltin=true in\n> >> master right after 2.20 is out the door).\n> >\n> > That would be fine with me as well, but I'd rather\n> > document rebase.useBuiltin instead of flip-flopping\n> > the switch around the release.\n> >\n> > Have there been any fixes that are only in\n> > the C version (has the shell version already bitrotted)?\n> \n> That's a good question, one which I don't think we knew the answer to\n> before the following patches.\n\nI pay close attention to `git rebase` bug reports and patches (obviously),\nand there have not been any changes going into the built-in rebase/rebase\n-i that necessitated changes also in the scripted version.\n\nCiao,\nDscho\n\n> As it turns out no, we still run the tests without failures with\n> GIT_TEST_REBASE_USE_BUILTIN=false.\n> \n> Ævar Arnfjörð Bjarmason (2):\n> rebase doc: document rebase.useBuiltin\n> tests: add a special setup where rebase.useBuiltin is off\n> \n> Documentation/config/rebase.txt | 14 ++++++++++++++\n> builtin/rebase.c | 5 ++++-\n> t/README | 3 +++\n> 3 files changed, 21 insertions(+), 1 deletion(-)\n> \n> -- \n> 2.19.1.1182.g4ecb1133ce\n> \n> "},{"id":"363366","messageId":"nycvar.QRO.7.76.6.1811141508130.39@tvgsbejvaqbjf.bet","threadId":"49817","inReplyTo":"1b8461d1-6cb7-6622-94d2-44c27623236d@talktalk.net","subject":"Re: [PATCH 1/1] rebase: really just passthru the `git am` options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-14T14:22:26Z","receivedAt":"2018-11-14T14:22:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Tue, 13 Nov 2018, Phillip Wood wrote:\n\n> On 13/11/2018 19:21, Johannes Schindelin wrote:\n> > Hi Phillip,\n> > \n> > On Tue, 13 Nov 2018, Phillip Wood wrote:\n> > \n> > > Thanks for looking at this. Unfortunately using OPT_PASSTHRU_ARGV seems to\n> > > break the error reporting\n> > >\n> > > Running\n> > > bin/wrappers/git rebase --onto @^^^^ @^^ -Cbad\n> > >\n> > > Gives\n> > > git encountered an error while preparing the patches to replay\n> > > these revisions:\n> > >\n> > >\n> > > 67f673aa4a580b9e407b1ca505abf1f50510ec47...7c3e01a708856885e60bf4051586970e65dd326c\n> > >\n> > > As a result, git cannot rebase them.\n> > >\n> \n> git 2.19.1 gives\n> \n> First, rewinding head to replay your work on top of it...\n> Applying: Ninth batch for 2.20\n> error: switch `C' expects a numerical value\n> \n> So it has a clear message as to what the error is, this patch regresses that.\n> It would be better if rebase detected the error before starting though.\n\nI agree that that would make most sense: why start something when you can\nknow that it will fail later...\n\nLet me try to add that test case that Junio wanted, and some early error\nhandling.\n\n> > > If I do\n> > >\n> > > bin/wrappers/git rebase @^^ -Cbad\n> > >\n> > > I get no error, it just tells me that it does not need to rebase (which is\n> > > true)\n> > \n> > Hmm. Isn't this the same behavior as with the scripted version?\n> \n> Ah you're right the script does not check if the option argument is valid.\n> \n> > Also: are\n> > we sure that we want to allow options to come *after* the ``\n> > argument?\n> \n> Maybe not but the scripted version does. I'm not sure if that is a good idea\n> or not.\n\nThat behavior was never documented, though, was it?\n\nCiao,\nDscho\n\n> \n> Best Wishes\n> \n> Phillip\n> \n> > Ciao,\n> > Dscho\n> > \n> > > Best Wishes\n> > >\n> > > Phillip\n> > >\n> > >\n> > > On 13/11/2018 12:38, Johannes Schindelin via GitGitGadget wrote:\n> > > > From: Johannes Schindelin \n> > > >\n> > > > Currently, we parse the options intended for `git am` as if we wanted to\n> > > > handle them in `git rebase`, and then reconstruct them painstakingly to\n> > > > define the `git_am_opt` variable.\n> > > >\n> > > > However, there is a much better way (that I was unaware of, at the time\n> > > > when I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.\n> > > > It is intended for exactly this use case, where command-line options\n> > > > want to be parsed into a separate `argv_array`.\n> > > >\n> > > > Let's use this feature.\n> > > >\n> > > > Incidentally, this also allows us to address a bug discovered by Phillip\n> > > > Wood, where the built-in rebase failed to understand that the `-C`\n> > > > option takes an optional argument.\n> > > >\n> > > > Signed-off-by: Johannes Schindelin \n> > > > ---\n> > > > builtin/rebase.c | 98\n> > > > +++++++++++++++++-------------------------------\n> > > > 1 file changed, 35 insertions(+), 63 deletions(-)\n> > > >\n> > > > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > > > index 0ee06aa363..96ffa80b71 100644\n> > > > --- a/builtin/rebase.c\n> > > > +++ b/builtin/rebase.c\n> > > > @@ -87,7 +87,7 @@ struct rebase_options {\n> > > > REBASE_FORCE = 1<<3,\n> > > > REBASE_INTERACTIVE_EXPLICIT = 1<<4,\n> > > > \t} flags;\n> > > > -\tstruct strbuf git_am_opt;\n> > > > +\tstruct argv_array git_am_opts;\n> > > > const char *action;\n> > > > int signoff;\n> > > > int allow_rerere_autoupdate;\n> > > > @@ -339,7 +339,7 @@ N_(\"Resolve all conflicts manually, mark them as\n> > > > resolved with\\n\"\n> > > > static int run_specific_rebase(struct rebase_options *opts)\n> > > > {\n> > > > \tconst char *argv[] = { NULL, NULL };\n> > > > -\tstruct strbuf script_snippet = STRBUF_INIT;\n> > > > +\tstruct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;\n> > > > int status;\n> > > > const char *backend, *backend_func;\n> > > > @@ -433,7 +433,9 @@ static int run_specific_rebase(struct\n> > > > rebase_options\n> > > > *opts)\n> > > > \toid_to_hex(&opts->restrict_revision->object.oid) : NULL);\n> > > > add_var(&script_snippet, \"GIT_QUIET\",\n> > > > \t\topts->flags & REBASE_NO_QUIET ? \"\" : \"t\");\n> > > > -\tadd_var(&script_snippet, \"git_am_opt\", opts->git_am_opt.buf);\n> > > > +\tsq_quote_argv_pretty(&buf, opts->git_am_opts.argv);\n> > > > +\tadd_var(&script_snippet, \"git_am_opt\", buf.buf);\n> > > > +\tstrbuf_release(&buf);\n> > > > add_var(&script_snippet, \"verbose\",\n> > > > \topts->flags & REBASE_VERBOSE ? \"t\" : \"\");\n> > > > \tadd_var(&script_snippet, \"diffstat\",\n> > > > @@ -756,7 +758,7 @@ int cmd_rebase(int argc, const char **argv, const\n> > > > char\n> > > > *prefix)\n> > > > struct rebase_options options = {\n> > > > .type = REBASE_UNSPECIFIED,\n> > > > .flags = REBASE_NO_QUIET,\n> > > > -\t\t.git_am_opt = STRBUF_INIT,\n> > > > +\t\t.git_am_opts = ARGV_ARRAY_INIT,\n> > > > .allow_rerere_autoupdate = -1,\n> > > > .allow_empty_message = 1,\n> > > > .git_format_patch_opt = STRBUF_INIT,\n> > > > @@ -777,12 +779,7 @@ int cmd_rebase(int argc, const char **argv, const\n> > > > char *prefix)\n> > > > ACTION_EDIT_TODO,\n> > > > ACTION_SHOW_CURRENT_PATCH,\n> > > > \t} action = NO_ACTION;\n> > > > -\tint committer_date_is_author_date = 0;\n> > > > -\tint ignore_date = 0;\n> > > > -\tint ignore_whitespace = 0;\n> > > > \tconst char *gpg_sign = NULL;\n> > > > -\tint opt_c = -1;\n> > > > -\tstruct string_list whitespace = STRING_LIST_INIT_NODUP;\n> > > > struct string_list exec = STRING_LIST_INIT_NODUP;\n> > > > const char *rebase_merges = NULL;\n> > > > int fork_point = -1;\n> > > > @@ -804,15 +801,20 @@ int cmd_rebase(int argc, const char **argv, const\n> > > > char *prefix)\n> > > > {OPTION_NEGBIT, 'n', \"no-stat\", &options.flags, NULL,\n> > > > N_(\"do not show diffstat of what changed upstream\"),\n> > > > PARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },\n> > > > -\t\tOPT_BOOL(0, \"ignore-whitespace\", &ignore_whitespace,\n> > > > -\t\t\t N_(\"passed to 'git apply'\")),\n> > > > OPT_BOOL(0, \"signoff\", &options.signoff,\n> > > > \t\t\t N_(\"add a Signed-off-by: line to each\n> > > > commit\")),\n> > > > -\t\tOPT_BOOL(0, \"committer-date-is-author-date\",\n> > > > -\t\t\t &committer_date_is_author_date,\n> > > > -\t\t\t N_(\"passed to 'git am'\")),\n> > > > -\t\tOPT_BOOL(0, \"ignore-date\", &ignore_date,\n> > > > -\t\t\t N_(\"passed to 'git am'\")),\n> > > > +\t\tOPT_PASSTHRU_ARGV(0, \"ignore-whitespace\",\n> > > > &options.git_am_opts,\n> > > > +\t\t\t\t NULL, N_(\"passed to 'git am'\"),\n> > > > +\t\t\t\t PARSE_OPT_NOARG),\n> > > > +\t\tOPT_PASSTHRU_ARGV(0, \"committer-date-is-author-date\",\n> > > > +\t\t\t\t &options.git_am_opts, NULL,\n> > > > +\t\t\t\t N_(\"passed to 'git am'\"),\n> > > > PARSE_OPT_NOARG),\n> > > > +\t\tOPT_PASSTHRU_ARGV(0, \"ignore-date\", &options.git_am_opts,\n> > > > NULL,\n> > > > +\t\t\t\t N_(\"passed to 'git am'\"),\n> > > > PARSE_OPT_NOARG),\n> > > > +\t\tOPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_(\"n\"),\n> > > > +\t\t\t\t N_(\"passed to 'git apply'\"), 0),\n> > > > +\t\tOPT_PASSTHRU_ARGV(0, \"whitespace\", &options.git_am_opts,\n> > > > +\t\t\t\t N_(\"action\"), N_(\"passed to 'git apply'\"),\n> > > > 0),\n> > > > OPT_BIT('f', \"force-rebase\", &options.flags,\n> > > > N_(\"cherry-pick all commits, even if unchanged\"),\n> > > > REBASE_FORCE),\n> > > > @@ -856,10 +858,6 @@ int cmd_rebase(int argc, const char **argv, const\n> > > > char *prefix)\n> > > > { OPTION_STRING, 'S', \"gpg-sign\", &gpg_sign, N_(\"key-id\"),\n> > > > N_(\"GPG-sign commits\"),\n> > > > PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n> > > > -\t\tOPT_STRING_LIST(0, \"whitespace\", &whitespace,\n> > > > -\t\t\t\tN_(\"whitespace\"), N_(\"passed to 'git\n> > > > apply'\")),\n> > > > -\t\tOPT_SET_INT('C', NULL, &opt_c, N_(\"passed to 'git apply'\"),\n> > > > -\t\t\t REBASE_AM),\n> > > > OPT_BOOL(0, \"autostash\", &options.autostash,\n> > > > \t N_(\"automatically stash/stash pop before and after\")),\n> > > > \t\tOPT_STRING_LIST('x', \"exec\", &exec, N_(\"exec\"),\n> > > > @@ -884,6 +882,7 @@ int cmd_rebase(int argc, const char **argv, const\n> > > > char\n> > > > *prefix)\n> > > > \t N_(\"rebase all reachable commits up to the root(s)\")),\n> > > > \tOPT_END(),\n> > > > \t};\n> > > > +\tint i;\n> > > > \n> > > > /*\n> > > > \t * NEEDSWORK: Once the builtin rebase has been tested enough\n> > > > @@ -1064,22 +1063,17 @@ int cmd_rebase(int argc, const char **argv,\n> > > > const\n> > > > char *prefix)\n> > > > \t state_dir_base, cmd_live_rebase, buf.buf);\n> > > > }\n> > > > -\tif (!(options.flags & REBASE_NO_QUIET))\n> > > > -\t\tstrbuf_addstr(&options.git_am_opt, \" -q\");\n> > > > -\n> > > > -\tif (committer_date_is_author_date) {\n> > > > -\t\tstrbuf_addstr(&options.git_am_opt,\n> > > > -\t\t\t \" --committer-date-is-author-date\");\n> > > > -\t\toptions.flags |= REBASE_FORCE;\n> > > > +\tfor (i = 0; i < options.git_am_opts.argc; i++) {\n> > > > +\t\tconst char *option = options.git_am_opts.argv[i];\n> > > > +\t\tif (!strcmp(option, \"--committer-date-is-author-date\") ||\n> > > > +\t\t !strcmp(option, \"--ignore-date\") ||\n> > > > +\t\t !strcmp(option, \"--whitespace=fix\") ||\n> > > > +\t\t !strcmp(option, \"--whitespace=strip\"))\n> > > > +\t\t\toptions.flags |= REBASE_FORCE;\n> > > > }\n> > > > -\tif (ignore_whitespace)\n> > > > -\t\tstrbuf_addstr(&options.git_am_opt, \" --ignore-whitespace\");\n> > > > -\n> > > > -\tif (ignore_date) {\n> > > > -\t\tstrbuf_addstr(&options.git_am_opt, \" --ignore-date\");\n> > > > -\t\toptions.flags |= REBASE_FORCE;\n> > > > -\t}\n> > > > +\tif (!(options.flags & REBASE_NO_QUIET))\n> > > > +\t\targv_array_push(&options.git_am_opts, \"-q\");\n> > > > \n> > > > if (options.keep_empty)\n> > > > \t\timply_interactive(&options, \"--keep-empty\");\n> > > > @@ -1089,23 +1083,6 @@ int cmd_rebase(int argc, const char **argv, const\n> > > > char *prefix)\n> > > > \toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\n> > > > }\n> > > > -\tif (opt_c >= 0)\n> > > > -\t\tstrbuf_addf(&options.git_am_opt, \" -C%d\", opt_c);\n> > > > -\n> > > > -\tif (whitespace.nr) {\n> > > > -\t\tint i;\n> > > > -\n> > > > -\t\tfor (i = 0; i < whitespace.nr; i++) {\n> > > > -\t\t\tconst char *item = whitespace.items[i].string;\n> > > > -\n> > > > -\t\t\tstrbuf_addf(&options.git_am_opt, \" --whitespace=%s\",\n> > > > -\t\t\t\t item);\n> > > > -\n> > > > -\t\t\tif ((!strcmp(item, \"fix\")) || (!strcmp(item,\n> > > > \"strip\")))\n> > > > -\t\t\t\toptions.flags |= REBASE_FORCE;\n> > > > -\t\t}\n> > > > -\t}\n> > > > -\n> > > > if (exec.nr) {\n> > > > int i;\n> > > > @@ -1181,23 +1158,18 @@ int cmd_rebase(int argc, const char **argv,\n> > > > const char *prefix)\n> > > > \tbreak;\n> > > > }\n> > > > -\tif (options.git_am_opt.len) {\n> > > > -\t\tconst char *p;\n> > > > -\n> > > > +\tif (options.git_am_opts.argc) {\n> > > > \t\t/* all am options except -q are compatible only with\n> > > > --am */\n> > > > -\t\tstrbuf_reset(&buf);\n> > > > -\t\tstrbuf_addbuf(&buf, &options.git_am_opt);\n> > > > -\t\tstrbuf_addch(&buf, ' ');\n> > > > -\t\twhile ((p = strstr(buf.buf, \" -q \")))\n> > > > -\t\t\tstrbuf_splice(&buf, p - buf.buf, 4, \" \", 1);\n> > > > -\t\tstrbuf_trim(&buf);\n> > > > +\t\tfor (i = options.git_am_opts.argc - 1; i >= 0; i--)\n> > > > +\t\t\tif (strcmp(options.git_am_opts.argv[i], \"-q\"))\n> > > > +\t\t\t\tbreak;\n> > > > -\t\tif (is_interactive(&options) && buf.len)\n> > > > +\t\tif (is_interactive(&options) && i >= 0)\n> > > > die(_(\"error: cannot combine interactive options \"\n> > > > \"(--interactive, --exec, --rebase-merges, \"\n> > > > \"--preserve-merges, --keep-empty, --root + \"\n> > > > \"--onto) with am options (%s)\"), buf.buf);\n> > > > -\t\tif (options.type == REBASE_MERGE && buf.len)\n> > > > +\t\tif (options.type == REBASE_MERGE && i >= 0)\n> > > > die(_(\"error: cannot combine merge options (--merge, \"\n> > > > \"--strategy, --strategy-option) with am options \"\n> > > > \"(%s)\"), buf.buf);\n> > > > @@ -1207,7 +1179,7 @@ int cmd_rebase(int argc, const char **argv, const\n> > > > char *prefix)\n> > > > if (options.type == REBASE_PRESERVE_MERGES)\n> > > > die(\"cannot combine '--signoff' with \"\n> > > > \t\t\t \"'--preserve-merges'\");\n> > > > -\t\tstrbuf_addstr(&options.git_am_opt, \" --signoff\");\n> > > > +\t\targv_array_push(&options.git_am_opts, \"--signoff\");\n> > > > \toptions.flags |= REBASE_FORCE;\n> > > > }\n> > > > \n> > > >\n> > >\n> > >\n> \n> \n> \n"},{"id":"363367","messageId":"nycvar.QRO.7.76.6.1811141525080.39@tvgsbejvaqbjf.bet","threadId":"49817","inReplyTo":"20181114072909.GD19904@sigill.intra.peff.net","subject":"Re: [PATCH 0/1] rebase: understand -C again, refactor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-14T14:28:35Z","receivedAt":"2018-11-14T14:28:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 14 Nov 2018, Jeff King wrote:\n\n> On Tue, Nov 13, 2018 at 04:38:24AM -0800, Johannes Schindelin via GitGitGadget wrote:\n> \n> > Phillip Wood reported a problem where the built-in rebase did not understand\n> > options like -C1, i.e. it did not expect the option argument.\n> > \n> > While investigating how to address this best, I stumbled upon \n> > OPT_PASSTHRU_ARGV (which I was so far happily unaware of).\n> \n> I was unaware of it, too.\n\nThanks, that makes me feel better ;-)\n\n> Looking at the OPT_PASSTHRU and its ARGV counterpart, I think the\n> original intent was that you'd pass through normal last-one-wins\n> individual options with OPT_PASSTHRU, and then list-like options with\n> OPT_PASSTHRU_ARGV. But here you've used the latter to pass sets of\n> individual last-one-wins options.\n> \n> That said, I think what you've done here is way simpler and more\n> readable than using a bunch of OPT_PASSTHRUs would have been. And even\n> if it was not the original intent of the ARGV variant, I can't see any\n> reason to avoid doing it this way.\n\nThank you, that makes me feel *even* better ;-)\n\nTogether with the extra-early error checking for incorrect -C and\n--whitespace options, I think we're in a much better place now.\n\nCiao,\nDscho\n"},{"id":"363376","messageId":"pull.76.v2.git.gitgitgadget@gmail.com","threadId":"49817","inReplyTo":"pull.76.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] rebase: understand -C again, refactor","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-14T16:25:28Z","receivedAt":"2018-11-14T16:25:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Phillip Wood reported a problem where the built-in rebase did not understand\noptions like -C1, i.e. it did not expect the option argument.\n\nWhile investigating how to address this best, I stumbled upon \nOPT_PASSTHRU_ARGV (which I was so far happily unaware of).\n\nInstead of just fixing the -C bug, I decided to simply convert all of the\noptions intended for git am (or, eventually, for git apply). This happens to\nfix that bug, and does so much more: it simplifies the entire logic (and\nremoves more lines than it adds).\n\nChange since v1:\n\n * Introduce early parameter validation for the options passed through to \n git am.\n\nJohannes Schindelin (2):\n rebase: really just passthru the `git am` options\n rebase: validate -C and --whitespace= parameters early\n\n builtin/rebase.c | 108 ++++++++++++++++----------------------\n t/t3406-rebase-message.sh | 7 +++\n 2 files changed, 52 insertions(+), 63 deletions(-)\n\n\nbase-commit: 8858448bb49332d353febc078ce4a3abcc962efe\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-76%2Fdscho%2Frebase-Cn-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-76/dscho/rebase-Cn-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/76\n\nRange-diff vs v1:\n\n 1: dc36a45068 = 1: dc36a45068 rebase: really just passthru the `git am` options\n -: ---------- > 2: 4c2ba52766 rebase: validate -C and --whitespace= parameters early\n\n-- \ngitgitgadget\n"},{"id":"363378","messageId":"dc36a450680b1854abbb9bb06180001ce68c3f3b.1542212726.git.gitgitgadget@gmail.com","threadId":"49817","inReplyTo":"pull.76.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] rebase: really just passthru the `git am` options","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-14T16:25:29Z","receivedAt":"2018-11-14T16:25:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nCurrently, we parse the options intended for `git am` as if we wanted to\nhandle them in `git rebase`, and then reconstruct them painstakingly to\ndefine the `git_am_opt` variable.\n\nHowever, there is a much better way (that I was unaware of, at the time\nwhen I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.\nIt is intended for exactly this use case, where command-line options\nwant to be parsed into a separate `argv_array`.\n\nLet's use this feature.\n\nIncidentally, this also allows us to address a bug discovered by Phillip\nWood, where the built-in rebase failed to understand that the `-C`\noption takes an optional argument.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 98 +++++++++++++++++-------------------------------\n 1 file changed, 35 insertions(+), 63 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 0ee06aa363..96ffa80b71 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -87,7 +87,7 @@ struct rebase_options {\n \t\tREBASE_FORCE = 1<<3,\n \t\tREBASE_INTERACTIVE_EXPLICIT = 1<<4,\n \t} flags;\n-\tstruct strbuf git_am_opt;\n+\tstruct argv_array git_am_opts;\n \tconst char *action;\n \tint signoff;\n \tint allow_rerere_autoupdate;\n@@ -339,7 +339,7 @@ N_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n static int run_specific_rebase(struct rebase_options *opts)\n {\n \tconst char *argv[] = { NULL, NULL };\n-\tstruct strbuf script_snippet = STRBUF_INIT;\n+\tstruct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;\n \tint status;\n \tconst char *backend, *backend_func;\n \n@@ -433,7 +433,9 @@ static int run_specific_rebase(struct rebase_options *opts)\n \t\toid_to_hex(&opts->restrict_revision->object.oid) : NULL);\n \tadd_var(&script_snippet, \"GIT_QUIET\",\n \t\topts->flags & REBASE_NO_QUIET ? \"\" : \"t\");\n-\tadd_var(&script_snippet, \"git_am_opt\", opts->git_am_opt.buf);\n+\tsq_quote_argv_pretty(&buf, opts->git_am_opts.argv);\n+\tadd_var(&script_snippet, \"git_am_opt\", buf.buf);\n+\tstrbuf_release(&buf);\n \tadd_var(&script_snippet, \"verbose\",\n \t\topts->flags & REBASE_VERBOSE ? \"t\" : \"\");\n \tadd_var(&script_snippet, \"diffstat\",\n@@ -756,7 +758,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tstruct rebase_options options = {\n \t\t.type = REBASE_UNSPECIFIED,\n \t\t.flags = REBASE_NO_QUIET,\n-\t\t.git_am_opt = STRBUF_INIT,\n+\t\t.git_am_opts = ARGV_ARRAY_INIT,\n \t\t.allow_rerere_autoupdate = -1,\n \t\t.allow_empty_message = 1,\n \t\t.git_format_patch_opt = STRBUF_INIT,\n@@ -777,12 +779,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tACTION_EDIT_TODO,\n \t\tACTION_SHOW_CURRENT_PATCH,\n \t} action = NO_ACTION;\n-\tint committer_date_is_author_date = 0;\n-\tint ignore_date = 0;\n-\tint ignore_whitespace = 0;\n \tconst char *gpg_sign = NULL;\n-\tint opt_c = -1;\n-\tstruct string_list whitespace = STRING_LIST_INIT_NODUP;\n \tstruct string_list exec = STRING_LIST_INIT_NODUP;\n \tconst char *rebase_merges = NULL;\n \tint fork_point = -1;\n@@ -804,15 +801,20 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t{OPTION_NEGBIT, 'n', \"no-stat\", &options.flags, NULL,\n \t\t\tN_(\"do not show diffstat of what changed upstream\"),\n \t\t\tPARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },\n-\t\tOPT_BOOL(0, \"ignore-whitespace\", &ignore_whitespace,\n-\t\t\t N_(\"passed to 'git apply'\")),\n \t\tOPT_BOOL(0, \"signoff\", &options.signoff,\n \t\t\t N_(\"add a Signed-off-by: line to each commit\")),\n-\t\tOPT_BOOL(0, \"committer-date-is-author-date\",\n-\t\t\t &committer_date_is_author_date,\n-\t\t\t N_(\"passed to 'git am'\")),\n-\t\tOPT_BOOL(0, \"ignore-date\", &ignore_date,\n-\t\t\t N_(\"passed to 'git am'\")),\n+\t\tOPT_PASSTHRU_ARGV(0, \"ignore-whitespace\", &options.git_am_opts,\n+\t\t\t\t NULL, N_(\"passed to 'git am'\"),\n+\t\t\t\t PARSE_OPT_NOARG),\n+\t\tOPT_PASSTHRU_ARGV(0, \"committer-date-is-author-date\",\n+\t\t\t\t &options.git_am_opts, NULL,\n+\t\t\t\t N_(\"passed to 'git am'\"), PARSE_OPT_NOARG),\n+\t\tOPT_PASSTHRU_ARGV(0, \"ignore-date\", &options.git_am_opts, NULL,\n+\t\t\t\t N_(\"passed to 'git am'\"), PARSE_OPT_NOARG),\n+\t\tOPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_(\"n\"),\n+\t\t\t\t N_(\"passed to 'git apply'\"), 0),\n+\t\tOPT_PASSTHRU_ARGV(0, \"whitespace\", &options.git_am_opts,\n+\t\t\t\t N_(\"action\"), N_(\"passed to 'git apply'\"), 0),\n \t\tOPT_BIT('f', \"force-rebase\", &options.flags,\n \t\t\tN_(\"cherry-pick all commits, even if unchanged\"),\n \t\t\tREBASE_FORCE),\n@@ -856,10 +858,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &gpg_sign, N_(\"key-id\"),\n \t\t\tN_(\"GPG-sign commits\"),\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n-\t\tOPT_STRING_LIST(0, \"whitespace\", &whitespace,\n-\t\t\t\tN_(\"whitespace\"), N_(\"passed to 'git apply'\")),\n-\t\tOPT_SET_INT('C', NULL, &opt_c, N_(\"passed to 'git apply'\"),\n-\t\t\t REBASE_AM),\n \t\tOPT_BOOL(0, \"autostash\", &options.autostash,\n \t\t\t N_(\"automatically stash/stash pop before and after\")),\n \t\tOPT_STRING_LIST('x', \"exec\", &exec, N_(\"exec\"),\n@@ -884,6 +882,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"rebase all reachable commits up to the root(s)\")),\n \t\tOPT_END(),\n \t};\n+\tint i;\n \n \t/*\n \t * NEEDSWORK: Once the builtin rebase has been tested enough\n@@ -1064,22 +1063,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t state_dir_base, cmd_live_rebase, buf.buf);\n \t}\n \n-\tif (!(options.flags & REBASE_NO_QUIET))\n-\t\tstrbuf_addstr(&options.git_am_opt, \" -q\");\n-\n-\tif (committer_date_is_author_date) {\n-\t\tstrbuf_addstr(&options.git_am_opt,\n-\t\t\t \" --committer-date-is-author-date\");\n-\t\toptions.flags |= REBASE_FORCE;\n+\tfor (i = 0; i < options.git_am_opts.argc; i++) {\n+\t\tconst char *option = options.git_am_opts.argv[i];\n+\t\tif (!strcmp(option, \"--committer-date-is-author-date\") ||\n+\t\t !strcmp(option, \"--ignore-date\") ||\n+\t\t !strcmp(option, \"--whitespace=fix\") ||\n+\t\t !strcmp(option, \"--whitespace=strip\"))\n+\t\t\toptions.flags |= REBASE_FORCE;\n \t}\n \n-\tif (ignore_whitespace)\n-\t\tstrbuf_addstr(&options.git_am_opt, \" --ignore-whitespace\");\n-\n-\tif (ignore_date) {\n-\t\tstrbuf_addstr(&options.git_am_opt, \" --ignore-date\");\n-\t\toptions.flags |= REBASE_FORCE;\n-\t}\n+\tif (!(options.flags & REBASE_NO_QUIET))\n+\t\targv_array_push(&options.git_am_opts, \"-q\");\n \n \tif (options.keep_empty)\n \t\timply_interactive(&options, \"--keep-empty\");\n@@ -1089,23 +1083,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\n \t}\n \n-\tif (opt_c >= 0)\n-\t\tstrbuf_addf(&options.git_am_opt, \" -C%d\", opt_c);\n-\n-\tif (whitespace.nr) {\n-\t\tint i;\n-\n-\t\tfor (i = 0; i < whitespace.nr; i++) {\n-\t\t\tconst char *item = whitespace.items[i].string;\n-\n-\t\t\tstrbuf_addf(&options.git_am_opt, \" --whitespace=%s\",\n-\t\t\t\t item);\n-\n-\t\t\tif ((!strcmp(item, \"fix\")) || (!strcmp(item, \"strip\")))\n-\t\t\t\toptions.flags |= REBASE_FORCE;\n-\t\t}\n-\t}\n-\n \tif (exec.nr) {\n \t\tint i;\n \n@@ -1181,23 +1158,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tbreak;\n \t}\n \n-\tif (options.git_am_opt.len) {\n-\t\tconst char *p;\n-\n+\tif (options.git_am_opts.argc) {\n \t\t/* all am options except -q are compatible only with --am */\n-\t\tstrbuf_reset(&buf);\n-\t\tstrbuf_addbuf(&buf, &options.git_am_opt);\n-\t\tstrbuf_addch(&buf, ' ');\n-\t\twhile ((p = strstr(buf.buf, \" -q \")))\n-\t\t\tstrbuf_splice(&buf, p - buf.buf, 4, \" \", 1);\n-\t\tstrbuf_trim(&buf);\n+\t\tfor (i = options.git_am_opts.argc - 1; i >= 0; i--)\n+\t\t\tif (strcmp(options.git_am_opts.argv[i], \"-q\"))\n+\t\t\t\tbreak;\n \n-\t\tif (is_interactive(&options) && buf.len)\n+\t\tif (is_interactive(&options) && i >= 0)\n \t\t\tdie(_(\"error: cannot combine interactive options \"\n \t\t\t \"(--interactive, --exec, --rebase-merges, \"\n \t\t\t \"--preserve-merges, --keep-empty, --root + \"\n \t\t\t \"--onto) with am options (%s)\"), buf.buf);\n-\t\tif (options.type == REBASE_MERGE && buf.len)\n+\t\tif (options.type == REBASE_MERGE && i >= 0)\n \t\t\tdie(_(\"error: cannot combine merge options (--merge, \"\n \t\t\t \"--strategy, --strategy-option) with am options \"\n \t\t\t \"(%s)\"), buf.buf);\n@@ -1207,7 +1179,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tif (options.type == REBASE_PRESERVE_MERGES)\n \t\t\tdie(\"cannot combine '--signoff' with \"\n \t\t\t \"'--preserve-merges'\");\n-\t\tstrbuf_addstr(&options.git_am_opt, \" --signoff\");\n+\t\targv_array_push(&options.git_am_opts, \"--signoff\");\n \t\toptions.flags |= REBASE_FORCE;\n \t}\n \n-- \ngitgitgadget\n\n"},{"id":"363377","messageId":"4c2ba5276636097a1c3bb2207c7537831e9079bc.1542212726.git.gitgitgadget@gmail.com","threadId":"49817","inReplyTo":"pull.76.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] rebase: validate -C and --whitespace= parameters early","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-14T16:25:31Z","receivedAt":"2018-11-14T16:25:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIt is a good idea to error out early upon seeing, say, `-Cbad`, rather\nthan starting the rebase only to have the `--am` backend complain later.\n\nLet's do this.\n\nThe only options accepting parameters which we pass through to `git am`\n(which may, or may not, forward them to `git apply`) are `-C` and\n`--whitespace`. The other options we pass through do not accept\nparameters, so we do not have to validate them here.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 12 +++++++++++-\n t/t3406-rebase-message.sh | 7 +++++++\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 96ffa80b71..571cf899d5 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t}\n \n \tfor (i = 0; i < options.git_am_opts.argc; i++) {\n-\t\tconst char *option = options.git_am_opts.argv[i];\n+\t\tconst char *option = options.git_am_opts.argv[i], *p;\n \t\tif (!strcmp(option, \"--committer-date-is-author-date\") ||\n \t\t !strcmp(option, \"--ignore-date\") ||\n \t\t !strcmp(option, \"--whitespace=fix\") ||\n \t\t !strcmp(option, \"--whitespace=strip\"))\n \t\t\toptions.flags |= REBASE_FORCE;\n+\t\telse if (skip_prefix(option, \"-C\", &p)) {\n+\t\t\twhile (*p)\n+\t\t\t\tif (!isdigit(*(p++)))\n+\t\t\t\t\tdie(_(\"switch `C' expects a \"\n+\t\t\t\t\t \"numerical value\"));\n+\t\t} else if (skip_prefix(option, \"--whitespace=\", &p)) {\n+\t\t\tif (*p && strcmp(p, \"warn\") && strcmp(p, \"nowarn\") &&\n+\t\t\t strcmp(p, \"error\") && strcmp(p, \"error-all\"))\n+\t\t\t\tdie(\"Invalid whitespace option: '%s'\", p);\n+\t\t}\n \t}\n \n \tif (!(options.flags & REBASE_NO_QUIET))\ndiff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\nindex 0392e36d23..2c79eed4fe 100755\n--- a/t/t3406-rebase-message.sh\n+++ b/t/t3406-rebase-message.sh\n@@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the invalid ref' '\n \ttest_i18ngrep \"invalid-ref\" err\n '\n \n+test_expect_success 'error out early upon -C or --whitespace=' '\n+\ttest_must_fail git rebase -Cnot-a-number HEAD 2>err &&\n+\ttest_i18ngrep \"numerical value\" err &&\n+\ttest_must_fail git rebase --whitespace=bad HEAD 2>err &&\n+\ttest_i18ngrep \"Invalid whitespace option\" err\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"363385","messageId":"ce8e2f7c-cded-0c9d-1306-330711a29021@talktalk.net","threadId":"49817","inReplyTo":"4c2ba5276636097a1c3bb2207c7537831e9079bc.1542212726.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] rebase: validate -C and --whitespace= parameters early","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-14T16:37:54Z","receivedAt":"2018-11-14T16:37:58Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nThanks for doing this, I think this patch is good. I've not checked the \nfirst patch as I think it is the same as before judging from the \ncovering letter.\n\nBest Wishes\n\nPhillip\n\nOn 14/11/2018 16:25, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> It is a good idea to error out early upon seeing, say, `-Cbad`, rather\n> than starting the rebase only to have the `--am` backend complain later.\n> \n> Let's do this.\n> \n> The only options accepting parameters which we pass through to `git am`\n> (which may, or may not, forward them to `git apply`) are `-C` and\n> `--whitespace`. The other options we pass through do not accept\n> parameters, so we do not have to validate them here.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/rebase.c | 12 +++++++++++-\n> t/t3406-rebase-message.sh | 7 +++++++\n> 2 files changed, 18 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 96ffa80b71..571cf899d5 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t}\n> \n> \tfor (i = 0; i < options.git_am_opts.argc; i++) {\n> -\t\tconst char *option = options.git_am_opts.argv[i];\n> +\t\tconst char *option = options.git_am_opts.argv[i], *p;\n> \t\tif (!strcmp(option, \"--committer-date-is-author-date\") ||\n> \t\t !strcmp(option, \"--ignore-date\") ||\n> \t\t !strcmp(option, \"--whitespace=fix\") ||\n> \t\t !strcmp(option, \"--whitespace=strip\"))\n> \t\t\toptions.flags |= REBASE_FORCE;\n> +\t\telse if (skip_prefix(option, \"-C\", &p)) {\n> +\t\t\twhile (*p)\n> +\t\t\t\tif (!isdigit(*(p++)))\n> +\t\t\t\t\tdie(_(\"switch `C' expects a \"\n> +\t\t\t\t\t \"numerical value\"));\n> +\t\t} else if (skip_prefix(option, \"--whitespace=\", &p)) {\n> +\t\t\tif (*p && strcmp(p, \"warn\") && strcmp(p, \"nowarn\") &&\n> +\t\t\t strcmp(p, \"error\") && strcmp(p, \"error-all\"))\n> +\t\t\t\tdie(\"Invalid whitespace option: '%s'\", p);\n> +\t\t}\n> \t}\n> \n> \tif (!(options.flags & REBASE_NO_QUIET))\n> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\n> index 0392e36d23..2c79eed4fe 100755\n> --- a/t/t3406-rebase-message.sh\n> +++ b/t/t3406-rebase-message.sh\n> @@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the invalid ref' '\n> \ttest_i18ngrep \"invalid-ref\" err\n> '\n> \n> +test_expect_success 'error out early upon -C or --whitespace=' '\n> +\ttest_must_fail git rebase -Cnot-a-number HEAD 2>err &&\n> +\ttest_i18ngrep \"numerical value\" err &&\n> +\ttest_must_fail git rebase --whitespace=bad HEAD 2>err &&\n> +\ttest_i18ngrep \"Invalid whitespace option\" err\n> +'\n> +\n> test_done\n> \n\n"},{"id":"363395","messageId":"nycvar.QRO.7.76.6.1811142224110.39@tvgsbejvaqbjf.bet","threadId":"49817","inReplyTo":"ce8e2f7c-cded-0c9d-1306-330711a29021@talktalk.net","subject":"Re: [PATCH v2 2/2] rebase: validate -C and --whitespace= parameters early","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-14T21:24:55Z","receivedAt":"2018-11-14T21:25:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Wed, 14 Nov 2018, Phillip Wood wrote:\n\n> Thanks for doing this, I think this patch is good.\n\nThanks!\n\n> I've not checked the first patch as I think it is the same as before\n> judging from the covering letter.\n\nIndeed, that's what the range-diff said. Sorry for not stating this\nexplicitly.\n\nThank you for your review,\nDscho\n\n> \n> Best Wishes\n> \n> Phillip\n> \n> On 14/11/2018 16:25, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> > \n> > It is a good idea to error out early upon seeing, say, `-Cbad`, rather\n> > than starting the rebase only to have the `--am` backend complain later.\n> > \n> > Let's do this.\n> > \n> > The only options accepting parameters which we pass through to `git am`\n> > (which may, or may not, forward them to `git apply`) are `-C` and\n> > `--whitespace`. The other options we pass through do not accept\n> > parameters, so we do not have to validate them here.\n> > \n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > builtin/rebase.c | 12 +++++++++++-\n> > t/t3406-rebase-message.sh | 7 +++++++\n> > 2 files changed, 18 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > index 96ffa80b71..571cf899d5 100644\n> > --- a/builtin/rebase.c\n> > +++ b/builtin/rebase.c\n> > @@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv,\n> > const char *prefix)\n> > }\n> > \n> > \tfor (i = 0; i < options.git_am_opts.argc; i++) {\n> > -\t\tconst char *option = options.git_am_opts.argv[i];\n> > +\t\tconst char *option = options.git_am_opts.argv[i], *p;\n> > if (!strcmp(option, \"--committer-date-is-author-date\") ||\n> > !strcmp(option, \"--ignore-date\") ||\n> > !strcmp(option, \"--whitespace=fix\") ||\n> > !strcmp(option, \"--whitespace=strip\"))\n> > \t\t\toptions.flags |= REBASE_FORCE;\n> > +\t\telse if (skip_prefix(option, \"-C\", &p)) {\n> > +\t\t\twhile (*p)\n> > +\t\t\t\tif (!isdigit(*(p++)))\n> > +\t\t\t\t\tdie(_(\"switch `C' expects a \"\n> > +\t\t\t\t\t \"numerical value\"));\n> > +\t\t} else if (skip_prefix(option, \"--whitespace=\", &p)) {\n> > +\t\t\tif (*p && strcmp(p, \"warn\") && strcmp(p, \"nowarn\")\n> > &&\n> > +\t\t\t strcmp(p, \"error\") && strcmp(p, \"error-all\"))\n> > +\t\t\t\tdie(\"Invalid whitespace option: '%s'\", p);\n> > +\t\t}\n> > }\n> > \n> > \tif (!(options.flags & REBASE_NO_QUIET))\n> > diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\n> > index 0392e36d23..2c79eed4fe 100755\n> > --- a/t/t3406-rebase-message.sh\n> > +++ b/t/t3406-rebase-message.sh\n> > @@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the\n> > invalid ref' '\n> > \ttest_i18ngrep \"invalid-ref\" err\n> > '\n> > \n> > +test_expect_success 'error out early upon -C or --whitespace='\n> > '\n> > +\ttest_must_fail git rebase -Cnot-a-number HEAD 2>err &&\n> > +\ttest_i18ngrep \"numerical value\" err &&\n> > +\ttest_must_fail git rebase --whitespace=bad HEAD 2>err &&\n> > +\ttest_i18ngrep \"Invalid whitespace option\" err\n> > +'\n> > +\n> > test_done\n> > \n> \n> \n"},{"id":"363632","messageId":"874lcd1bub.fsf@evledraar.gmail.com","threadId":"49817","inReplyTo":"4c2ba5276636097a1c3bb2207c7537831e9079bc.1542212726.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] rebase: validate -C and --whitespace= parameters early","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-19T12:38:36Z","receivedAt":"2018-11-19T12:38:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 14 2018, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> It is a good idea to error out early upon seeing, say, `-Cbad`, rather\n> than starting the rebase only to have the `--am` backend complain later.\n>\n> Let's do this.\n>\n> The only options accepting parameters which we pass through to `git am`\n> (which may, or may not, forward them to `git apply`) are `-C` and\n> `--whitespace`. The other options we pass through do not accept\n> parameters, so we do not have to validate them here.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/rebase.c | 12 +++++++++++-\n> t/t3406-rebase-message.sh | 7 +++++++\n> 2 files changed, 18 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 96ffa80b71..571cf899d5 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t}\n>\n> \tfor (i = 0; i < options.git_am_opts.argc; i++) {\n> -\t\tconst char *option = options.git_am_opts.argv[i];\n> +\t\tconst char *option = options.git_am_opts.argv[i], *p;\n> \t\tif (!strcmp(option, \"--committer-date-is-author-date\") ||\n> \t\t !strcmp(option, \"--ignore-date\") ||\n> \t\t !strcmp(option, \"--whitespace=fix\") ||\n> \t\t !strcmp(option, \"--whitespace=strip\"))\n> \t\t\toptions.flags |= REBASE_FORCE;\n> +\t\telse if (skip_prefix(option, \"-C\", &p)) {\n> +\t\t\twhile (*p)\n> +\t\t\t\tif (!isdigit(*(p++)))\n> +\t\t\t\t\tdie(_(\"switch `C' expects a \"\n> +\t\t\t\t\t \"numerical value\"));\n> +\t\t} else if (skip_prefix(option, \"--whitespace=\", &p)) {\n> +\t\t\tif (*p && strcmp(p, \"warn\") && strcmp(p, \"nowarn\") &&\n> +\t\t\t strcmp(p, \"error\") && strcmp(p, \"error-all\"))\n> +\t\t\t\tdie(\"Invalid whitespace option: '%s'\", p);\n> +\t\t}\n> \t}\n>\n> \tif (!(options.flags & REBASE_NO_QUIET))\n> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\n> index 0392e36d23..2c79eed4fe 100755\n> --- a/t/t3406-rebase-message.sh\n> +++ b/t/t3406-rebase-message.sh\n> @@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the invalid ref' '\n> \ttest_i18ngrep \"invalid-ref\" err\n> '\n>\n> +test_expect_success 'error out early upon -C or --whitespace=' '\n> +\ttest_must_fail git rebase -Cnot-a-number HEAD 2>err &&\n> +\ttest_i18ngrep \"numerical value\" err &&\n> +\ttest_must_fail git rebase --whitespace=bad HEAD 2>err &&\n> +\ttest_i18ngrep \"Invalid whitespace option\" err\n> +'\n> +\n\nThe combination of this gitster/js/rebase-am-options and my\ngitster/ab/rebase-in-c-escape-hatch breaks tests under\nGIT_TEST_REBASE_USE_BUILTIN=false for obvious reasons. The C version is\nnow more strict.\n"},{"id":"363724","messageId":"nycvar.QRO.7.76.6.1811201157170.41@tvgsbejvaqbjf.bet","threadId":"49817","inReplyTo":"874lcd1bub.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 2/2] rebase: validate -C and --whitespace= parameters early","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-20T10:58:36Z","receivedAt":"2018-11-20T10:58:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Mon, 19 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> \n> On Wed, Nov 14 2018, Johannes Schindelin via GitGitGadget wrote:\n> \n> > From: Johannes Schindelin \n> >\n> > It is a good idea to error out early upon seeing, say, `-Cbad`, rather\n> > than starting the rebase only to have the `--am` backend complain later.\n> >\n> > Let's do this.\n> >\n> > The only options accepting parameters which we pass through to `git am`\n> > (which may, or may not, forward them to `git apply`) are `-C` and\n> > `--whitespace`. The other options we pass through do not accept\n> > parameters, so we do not have to validate them here.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > builtin/rebase.c | 12 +++++++++++-\n> > t/t3406-rebase-message.sh | 7 +++++++\n> > 2 files changed, 18 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > index 96ffa80b71..571cf899d5 100644\n> > --- a/builtin/rebase.c\n> > +++ b/builtin/rebase.c\n> > @@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> > \t}\n> >\n> > \tfor (i = 0; i < options.git_am_opts.argc; i++) {\n> > -\t\tconst char *option = options.git_am_opts.argv[i];\n> > +\t\tconst char *option = options.git_am_opts.argv[i], *p;\n> > \t\tif (!strcmp(option, \"--committer-date-is-author-date\") ||\n> > \t\t !strcmp(option, \"--ignore-date\") ||\n> > \t\t !strcmp(option, \"--whitespace=fix\") ||\n> > \t\t !strcmp(option, \"--whitespace=strip\"))\n> > \t\t\toptions.flags |= REBASE_FORCE;\n> > +\t\telse if (skip_prefix(option, \"-C\", &p)) {\n> > +\t\t\twhile (*p)\n> > +\t\t\t\tif (!isdigit(*(p++)))\n> > +\t\t\t\t\tdie(_(\"switch `C' expects a \"\n> > +\t\t\t\t\t \"numerical value\"));\n> > +\t\t} else if (skip_prefix(option, \"--whitespace=\", &p)) {\n> > +\t\t\tif (*p && strcmp(p, \"warn\") && strcmp(p, \"nowarn\") &&\n> > +\t\t\t strcmp(p, \"error\") && strcmp(p, \"error-all\"))\n> > +\t\t\t\tdie(\"Invalid whitespace option: '%s'\", p);\n> > +\t\t}\n> > \t}\n> >\n> > \tif (!(options.flags & REBASE_NO_QUIET))\n> > diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\n> > index 0392e36d23..2c79eed4fe 100755\n> > --- a/t/t3406-rebase-message.sh\n> > +++ b/t/t3406-rebase-message.sh\n> > @@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the invalid ref' '\n> > \ttest_i18ngrep \"invalid-ref\" err\n> > '\n> >\n> > +test_expect_success 'error out early upon -C or --whitespace=' '\n> > +\ttest_must_fail git rebase -Cnot-a-number HEAD 2>err &&\n> > +\ttest_i18ngrep \"numerical value\" err &&\n> > +\ttest_must_fail git rebase --whitespace=bad HEAD 2>err &&\n> > +\ttest_i18ngrep \"Invalid whitespace option\" err\n> > +'\n> > +\n> \n> The combination of this gitster/js/rebase-am-options and my\n> gitster/ab/rebase-in-c-escape-hatch breaks tests under\n> GIT_TEST_REBASE_USE_BUILTIN=false for obvious reasons. The C version is\n> now more strict.\n\nMaybe you can concoct a prereq for this test? Something like\n\ntest_lazy_prereq BUILTIN_REBASE '\n\ttest true = \"${GIT_TEST_REBASE_USE_BUILTIN:-true}\"\n'\n\nCiao,\nDscho"},{"id":"363729","messageId":"20181120114208.14251-1-avarab@gmail.com","threadId":"49817","inReplyTo":"nycvar.QRO.7.76.6.1811201157170.41@tvgsbejvaqbjf.bet","subject":"[PATCH] rebase: mark a test as failing with rebase.useBuiltin=false","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-20T11:42:08Z","receivedAt":"2018-11-20T11:42:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Mark a test added in 04519d7201 (\"rebase: validate -C and\n--whitespace= parameters early\", 2018-11-14) as only succeeding\nwith the builtin version of rebase. It would be nice if the\nshellscript version had the same fix, but it's on its way out, and the\nauthor is not interested in fixing it[1].\n\nThis makes the entire test suite pass again with the\nGIT_TEST_REBASE_USE_BUILTIN=false mode added in my 62c23938fa (\"tests:\nadd a special setup where rebase.useBuiltin is off\", 2018-11-14).\n\n1. https://public-inbox.org/git/nycvar.QRO.7.76.6.1811201157170.41@tvgsbejvaqbjf.bet/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nOn Tue, Nov 20 2018, Johannes Schindelin wrote:\n\n> [...]\n> Maybe you can concoct a prereq for this test? Something like\n>\n> test_lazy_prereq BUILTIN_REBASE '\n> test true = \"${GIT_TEST_REBASE_USE_BUILTIN:-true}\"\n> '\n\nIt's better to just mark the test as needing the prereq turned\noff. E.g. this is what we do for the split index tests & now for the\ngettext tests. That way we always run the test, but just indicate that\nit relies on GIT_TEST_REBASE_USE_BUILTIN being unset.\n\n t/t3406-rebase-message.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\nindex 38bd876cab..77e5bbb3d5 100755\n--- a/t/t3406-rebase-message.sh\n+++ b/t/t3406-rebase-message.sh\n@@ -84,7 +84,8 @@ test_expect_success 'rebase --onto outputs the invalid ref' '\n \ttest_i18ngrep \"invalid-ref\" err\n '\n \n-test_expect_success 'error out early upon -C or --whitespace=' '\n+test_expect_success 'builtin rebase: error out early upon -C or --whitespace=' '\n+\tsane_unset GIT_TEST_REBASE_USE_BUILTIN &&\n \ttest_must_fail git rebase -Cnot-a-number HEAD 2>err &&\n \ttest_i18ngrep \"numerical value\" err &&\n \ttest_must_fail git rebase --whitespace=bad HEAD 2>err &&\n-- \n2.20.0.rc0.387.gc7a69e6b6c\n\n"},{"id":"363775","messageId":"nycvar.QRO.7.76.6.1811202029110.41@tvgsbejvaqbjf.bet","threadId":"49817","inReplyTo":"20181120114208.14251-1-avarab@gmail.com","subject":"Re: [PATCH] rebase: mark a test as failing with rebase.useBuiltin=false","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-20T19:55:35Z","receivedAt":"2018-11-20T19:56:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Tue, 20 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> Mark a test added in 04519d7201 (\"rebase: validate -C and\n> --whitespace= parameters early\", 2018-11-14) as only succeeding\n> with the builtin version of rebase. It would be nice if the\n> shellscript version had the same fix, but it's on its way out, and the\n> author is not interested in fixing it[1].\n\nIt's not that I am not interested in fixing it. It's more like I hoped\nthat I could work on Git for Windows v2.20.0-rc0 and rely on you to fix\nthis bug.\n\nNow that Git for Windows v2.20.0-rc0 is out (see\nhttps://github.com/git-for-windows/git/releases/tag/v2.20.0-rc0.windows.1),\nexpect a patch soon (see https://github.com/gitgitgadget/git/pull/86).\n\nCiao,\nDscho\n\n> \n> This makes the entire test suite pass again with the\n> GIT_TEST_REBASE_USE_BUILTIN=false mode added in my 62c23938fa (\"tests:\n> add a special setup where rebase.useBuiltin is off\", 2018-11-14).\n> \n> 1. https://public-inbox.org/git/nycvar.QRO.7.76.6.1811201157170.41@tvgsbejvaqbjf.bet/\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> \n> On Tue, Nov 20 2018, Johannes Schindelin wrote:\n> \n> > [...]\n> > Maybe you can concoct a prereq for this test? Something like\n> >\n> > test_lazy_prereq BUILTIN_REBASE '\n> > test true = \"${GIT_TEST_REBASE_USE_BUILTIN:-true}\"\n> > '\n> \n> It's better to just mark the test as needing the prereq turned\n> off. E.g. this is what we do for the split index tests & now for the\n> gettext tests. That way we always run the test, but just indicate that\n> it relies on GIT_TEST_REBASE_USE_BUILTIN being unset.\n> \n> t/t3406-rebase-message.sh | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\n> index 38bd876cab..77e5bbb3d5 100755\n> --- a/t/t3406-rebase-message.sh\n> +++ b/t/t3406-rebase-message.sh\n> @@ -84,7 +84,8 @@ test_expect_success 'rebase --onto outputs the invalid ref' '\n> \ttest_i18ngrep \"invalid-ref\" err\n> '\n> \n> -test_expect_success 'error out early upon -C or --whitespace=' '\n> +test_expect_success 'builtin rebase: error out early upon -C or --whitespace=' '\n> +\tsane_unset GIT_TEST_REBASE_USE_BUILTIN &&\n> \ttest_must_fail git rebase -Cnot-a-number HEAD 2>err &&\n> \ttest_i18ngrep \"numerical value\" err &&\n> \ttest_must_fail git rebase --whitespace=bad HEAD 2>err &&\n> -- \n> 2.20.0.rc0.387.gc7a69e6b6c\n> \n> "}]}