# [PATCH 0/1] rebase: understand -C again, refactor

26 messages from 2018-11-13 to 2018-11-20. Participants: Johannes Schindelin via GitGitGadget, Junio C Hamano, Phillip Wood, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Stefan Beller, Elijah Newren, Jeff King.
Thread: https://gitlist.dev/t/49817

## Johannes Schindelin via GitGitGadget, 2018-11-13 12:38

Subject: [PATCH 0/1] rebase: understand -C again, refactor
Message-ID: <pull.76.git.gitgitgadget@gmail.com>
URL: https://gitlist.dev/e/pull.76.git.gitgitgadget%40gmail.com

```
Phillip Wood reported a problem where the built-in rebase did not understand
options like -C1, i.e. it did not expect the option argument.

While investigating how to address this best, I stumbled upon 
OPT_PASSTHRU_ARGV (which I was so far happily unaware of).

Instead of just fixing the -C<n> bug, I decided to simply convert all of the
options intended for git am (or, eventually, for git apply). This happens to
fix that bug, and does so much more: it simplifies the entire logic (and
removes more lines than it adds).

Johannes Schindelin (1):
  rebase: really just passthru the `git am` options

 builtin/rebase.c | 98 +++++++++++++++++-------------------------------
 1 file changed, 35 insertions(+), 63 deletions(-)


base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-76%2Fdscho%2Frebase-Cn-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-76/dscho/rebase-Cn-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/76
-- 
gitgitgadget

```

## Johannes Schindelin via GitGitGadget, 2018-11-13 12:38

Subject: [PATCH 1/1] rebase: really just passthru the `git am` options
Message-ID: <dc36a450680b1854abbb9bb06180001ce68c3f3b.1542112703.git.gitgitgadget@gmail.com>
URL: https://gitlist.dev/e/dc36a450680b1854abbb9bb06180001ce68c3f3b.1542112703.git.gitgitgadget%40gmail.com
In-Reply-To: <pull.76.git.gitgitgadget@gmail.com>

```
From: Johannes Schindelin <johannes.schindelin@gmx.de>

Currently, we parse the options intended for `git am` as if we wanted to
handle them in `git rebase`, and then reconstruct them painstakingly to
define the `git_am_opt` variable.

However, there is a much better way (that I was unaware of, at the time
when I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.
It is intended for exactly this use case, where command-line options
want to be parsed into a separate `argv_array`.

Let's use this feature.

Incidentally, this also allows us to address a bug discovered by Phillip
Wood, where the built-in rebase failed to understand that the `-C`
option takes an optional argument.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/rebase.c | 98 +++++++++++++++++-------------------------------
 1 file changed, 35 insertions(+), 63 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ee06aa363..96ffa80b71 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -87,7 +87,7 @@ struct rebase_options {
 		REBASE_FORCE = 1<<3,
 		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
 	} flags;
-	struct strbuf git_am_opt;
+	struct argv_array git_am_opts;
 	const char *action;
 	int signoff;
 	int allow_rerere_autoupdate;
@@ -339,7 +339,7 @@ N_("Resolve all conflicts manually, mark them as resolved with\n"
 static int run_specific_rebase(struct rebase_options *opts)
 {
 	const char *argv[] = { NULL, NULL };
-	struct strbuf script_snippet = STRBUF_INIT;
+	struct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;
 	int status;
 	const char *backend, *backend_func;
 
@@ -433,7 +433,9 @@ static int run_specific_rebase(struct rebase_options *opts)
 		oid_to_hex(&opts->restrict_revision->object.oid) : NULL);
 	add_var(&script_snippet, "GIT_QUIET",
 		opts->flags & REBASE_NO_QUIET ? "" : "t");
-	add_var(&script_snippet, "git_am_opt", opts->git_am_opt.buf);
+	sq_quote_argv_pretty(&buf, opts->git_am_opts.argv);
+	add_var(&script_snippet, "git_am_opt", buf.buf);
+	strbuf_release(&buf);
 	add_var(&script_snippet, "verbose",
 		opts->flags & REBASE_VERBOSE ? "t" : "");
 	add_var(&script_snippet, "diffstat",
@@ -756,7 +758,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct rebase_options options = {
 		.type = REBASE_UNSPECIFIED,
 		.flags = REBASE_NO_QUIET,
-		.git_am_opt = STRBUF_INIT,
+		.git_am_opts = ARGV_ARRAY_INIT,
 		.allow_rerere_autoupdate  = -1,
 		.allow_empty_message = 1,
 		.git_format_patch_opt = STRBUF_INIT,
@@ -777,12 +779,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		ACTION_EDIT_TODO,
 		ACTION_SHOW_CURRENT_PATCH,
 	} action = NO_ACTION;
-	int committer_date_is_author_date = 0;
-	int ignore_date = 0;
-	int ignore_whitespace = 0;
 	const char *gpg_sign = NULL;
-	int opt_c = -1;
-	struct string_list whitespace = STRING_LIST_INIT_NODUP;
 	struct string_list exec = STRING_LIST_INIT_NODUP;
 	const char *rebase_merges = NULL;
 	int fork_point = -1;
@@ -804,15 +801,20 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		{OPTION_NEGBIT, 'n', "no-stat", &options.flags, NULL,
 			N_("do not show diffstat of what changed upstream"),
 			PARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },
-		OPT_BOOL(0, "ignore-whitespace", &ignore_whitespace,
-			 N_("passed to 'git apply'")),
 		OPT_BOOL(0, "signoff", &options.signoff,
 			 N_("add a Signed-off-by: line to each commit")),
-		OPT_BOOL(0, "committer-date-is-author-date",
-			 &committer_date_is_author_date,
-			 N_("passed to 'git am'")),
-		OPT_BOOL(0, "ignore-date", &ignore_date,
-			 N_("passed to 'git am'")),
+		OPT_PASSTHRU_ARGV(0, "ignore-whitespace", &options.git_am_opts,
+				  NULL, N_("passed to 'git am'"),
+				  PARSE_OPT_NOARG),
+		OPT_PASSTHRU_ARGV(0, "committer-date-is-author-date",
+				  &options.git_am_opts, NULL,
+				  N_("passed to 'git am'"), PARSE_OPT_NOARG),
+		OPT_PASSTHRU_ARGV(0, "ignore-date", &options.git_am_opts, NULL,
+				  N_("passed to 'git am'"), PARSE_OPT_NOARG),
+		OPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_("n"),
+				  N_("passed to 'git apply'"), 0),
+		OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts,
+				  N_("action"), N_("passed to 'git apply'"), 0),
 		OPT_BIT('f', "force-rebase", &options.flags,
 			N_("cherry-pick all commits, even if unchanged"),
 			REBASE_FORCE),
@@ -856,10 +858,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		{ OPTION_STRING, 'S', "gpg-sign", &gpg_sign, N_("key-id"),
 			N_("GPG-sign commits"),
 			PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
-		OPT_STRING_LIST(0, "whitespace", &whitespace,
-				N_("whitespace"), N_("passed to 'git apply'")),
-		OPT_SET_INT('C', NULL, &opt_c, N_("passed to 'git apply'"),
-			    REBASE_AM),
 		OPT_BOOL(0, "autostash", &options.autostash,
 			 N_("automatically stash/stash pop before and after")),
 		OPT_STRING_LIST('x', "exec", &exec, N_("exec"),
@@ -884,6 +882,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			 N_("rebase all reachable commits up to the root(s)")),
 		OPT_END(),
 	};
+	int i;
 
 	/*
 	 * NEEDSWORK: Once the builtin rebase has been tested enough
@@ -1064,22 +1063,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		    state_dir_base, cmd_live_rebase, buf.buf);
 	}
 
-	if (!(options.flags & REBASE_NO_QUIET))
-		strbuf_addstr(&options.git_am_opt, " -q");
-
-	if (committer_date_is_author_date) {
-		strbuf_addstr(&options.git_am_opt,
-			      " --committer-date-is-author-date");
-		options.flags |= REBASE_FORCE;
+	for (i = 0; i < options.git_am_opts.argc; i++) {
+		const char *option = options.git_am_opts.argv[i];
+		if (!strcmp(option, "--committer-date-is-author-date") ||
+		    !strcmp(option, "--ignore-date") ||
+		    !strcmp(option, "--whitespace=fix") ||
+		    !strcmp(option, "--whitespace=strip"))
+			options.flags |= REBASE_FORCE;
 	}
 
-	if (ignore_whitespace)
-		strbuf_addstr(&options.git_am_opt, " --ignore-whitespace");
-
-	if (ignore_date) {
-		strbuf_addstr(&options.git_am_opt, " --ignore-date");
-		options.flags |= REBASE_FORCE;
-	}
+	if (!(options.flags & REBASE_NO_QUIET))
+		argv_array_push(&options.git_am_opts, "-q");
 
 	if (options.keep_empty)
 		imply_interactive(&options, "--keep-empty");
@@ -1089,23 +1083,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
 	}
 
-	if (opt_c >= 0)
-		strbuf_addf(&options.git_am_opt, " -C%d", opt_c);
-
-	if (whitespace.nr) {
-		int i;
-
-		for (i = 0; i < whitespace.nr; i++) {
-			const char *item = whitespace.items[i].string;
-
-			strbuf_addf(&options.git_am_opt, " --whitespace=%s",
-				    item);
-
-			if ((!strcmp(item, "fix")) || (!strcmp(item, "strip")))
-				options.flags |= REBASE_FORCE;
-		}
-	}
-
 	if (exec.nr) {
 		int i;
 
@@ -1181,23 +1158,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		break;
 	}
 
-	if (options.git_am_opt.len) {
-		const char *p;
-
+	if (options.git_am_opts.argc) {
 		/* all am options except -q are compatible only with --am */
-		strbuf_reset(&buf);
-		strbuf_addbuf(&buf, &options.git_am_opt);
-		strbuf_addch(&buf, ' ');
-		while ((p = strstr(buf.buf, " -q ")))
-			strbuf_splice(&buf, p - buf.buf, 4, " ", 1);
-		strbuf_trim(&buf);
+		for (i = options.git_am_opts.argc - 1; i >= 0; i--)
+			if (strcmp(options.git_am_opts.argv[i], "-q"))
+				break;
 
-		if (is_interactive(&options) && buf.len)
+		if (is_interactive(&options) && i >= 0)
 			die(_("error: cannot combine interactive options "
 			      "(--interactive, --exec, --rebase-merges, "
 			      "--preserve-merges, --keep-empty, --root + "
 			      "--onto) with am options (%s)"), buf.buf);
-		if (options.type == REBASE_MERGE && buf.len)
+		if (options.type == REBASE_MERGE && i >= 0)
 			die(_("error: cannot combine merge options (--merge, "
 			      "--strategy, --strategy-option) with am options "
 			      "(%s)"), buf.buf);
@@ -1207,7 +1179,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		if (options.type == REBASE_PRESERVE_MERGES)
 			die("cannot combine '--signoff' with "
 			    "'--preserve-merges'");
-		strbuf_addstr(&options.git_am_opt, " --signoff");
+		argv_array_push(&options.git_am_opts, "--signoff");
 		options.flags |= REBASE_FORCE;
 	}
 
-- 
gitgitgadget

```

## Junio C Hamano, 2018-11-13 13:05

Subject: Re: [PATCH 1/1] rebase: really just passthru the `git am` options
Message-ID: <xmqqlg5xunzh.fsf@gitster-ct.c.googlers.com>
URL: https://gitlist.dev/e/xmqqlg5xunzh.fsf%40gitster-ct.c.googlers.com
In-Reply-To: <dc36a450680b1854abbb9bb06180001ce68c3f3b.1542112703.git.gitgitgadget@gmail.com>

```
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> However, there is a much better way (that I was unaware of, at the time
> when I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.
> It is intended for exactly this use case, where command-line options
> want to be parsed into a separate `argv_array`.
>
> Let's use this feature.
>
> Incidentally, this also allows us to address a bug discovered by Phillip
> Wood, where the built-in rebase failed to understand that the `-C`
> option takes an optional argument.

The resulting code does show what is going on more clearly.  I
wonder if we can do the same for -S as well?  It needs to be passed
to other backends, so I guess it is not such a good idea.

> -		.git_am_opt = STRBUF_INIT,
> +		.git_am_opts = ARGV_ARRAY_INIT,

Yes, this is much nicer.

> @@ -856,10 +858,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		{ OPTION_STRING, 'S', "gpg-sign", &gpg_sign, N_("key-id"),
>  			N_("GPG-sign commits"),
>  			PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
> -		OPT_STRING_LIST(0, "whitespace", &whitespace,
> -				N_("whitespace"), N_("passed to 'git apply'")),
> -		OPT_SET_INT('C', NULL, &opt_c, N_("passed to 'git apply'"),
> -			    REBASE_AM),
>  		OPT_BOOL(0, "autostash", &options.autostash,
>  			 N_("automatically stash/stash pop before and after")),
>  		OPT_STRING_LIST('x', "exec", &exec, N_("exec"),
> @@ -884,6 +882,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			 N_("rebase all reachable commits up to the root(s)")),
>  		OPT_END(),
>  	};
> +	int i;
>  
> +	for (i = 0; i < options.git_am_opts.argc; i++) {

Yes, this is very nice way to first collect and then inspect them
separately.

> +		const char *option = options.git_am_opts.argv[i];
> +		if (!strcmp(option, "--committer-date-is-author-date") ||
> +		    !strcmp(option, "--ignore-date") ||
> +		    !strcmp(option, "--whitespace=fix") ||
> +		    !strcmp(option, "--whitespace=strip"))
> +			options.flags |= REBASE_FORCE;
>  	}

Overall very nicely done.  Perhaps we'll see a test or two from
Philip?

Thanks.

```

## Phillip Wood, 2018-11-13 15:05

Subject: Re: [PATCH 1/1] rebase: really just passthru the `git am` options
Message-ID: <00d9b0e4-dde5-c96d-76d7-42fb9ac393f8@talktalk.net>
URL: https://gitlist.dev/e/00d9b0e4-dde5-c96d-76d7-42fb9ac393f8%40talktalk.net
In-Reply-To: <dc36a450680b1854abbb9bb06180001ce68c3f3b.1542112703.git.gitgitgadget@gmail.com>

```
Hi Johannes

Thanks for looking at this. Unfortunately using OPT_PASSTHRU_ARGV seems 
to break the error reporting

Running
   bin/wrappers/git rebase --onto @^^^^ @^^ -Cbad

Gives
   git encountered an error while preparing the patches to replay
   these revisions:

 
67f673aa4a580b9e407b1ca505abf1f50510ec47...7c3e01a708856885e60bf4051586970e65dd326c

   As a result, git cannot rebase them.

If I do

   bin/wrappers/git rebase @^^ -Cbad

I get no error, it just tells me that it does not need to rebase (which 
is true)

Best Wishes

Phillip


On 13/11/2018 12:38, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Currently, we parse the options intended for `git am` as if we wanted to
> handle them in `git rebase`, and then reconstruct them painstakingly to
> define the `git_am_opt` variable.
> 
> However, there is a much better way (that I was unaware of, at the time
> when I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.
> It is intended for exactly this use case, where command-line options
> want to be parsed into a separate `argv_array`.
> 
> Let's use this feature.
> 
> Incidentally, this also allows us to address a bug discovered by Phillip
> Wood, where the built-in rebase failed to understand that the `-C`
> option takes an optional argument.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   builtin/rebase.c | 98 +++++++++++++++++-------------------------------
>   1 file changed, 35 insertions(+), 63 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 0ee06aa363..96ffa80b71 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -87,7 +87,7 @@ struct rebase_options {
>   		REBASE_FORCE = 1<<3,
>   		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
>   	} flags;
> -	struct strbuf git_am_opt;
> +	struct argv_array git_am_opts;
>   	const char *action;
>   	int signoff;
>   	int allow_rerere_autoupdate;
> @@ -339,7 +339,7 @@ N_("Resolve all conflicts manually, mark them as resolved with\n"
>   static int run_specific_rebase(struct rebase_options *opts)
>   {
>   	const char *argv[] = { NULL, NULL };
> -	struct strbuf script_snippet = STRBUF_INIT;
> +	struct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;
>   	int status;
>   	const char *backend, *backend_func;
>   
> @@ -433,7 +433,9 @@ static int run_specific_rebase(struct rebase_options *opts)
>   		oid_to_hex(&opts->restrict_revision->object.oid) : NULL);
>   	add_var(&script_snippet, "GIT_QUIET",
>   		opts->flags & REBASE_NO_QUIET ? "" : "t");
> -	add_var(&script_snippet, "git_am_opt", opts->git_am_opt.buf);
> +	sq_quote_argv_pretty(&buf, opts->git_am_opts.argv);
> +	add_var(&script_snippet, "git_am_opt", buf.buf);
> +	strbuf_release(&buf);
>   	add_var(&script_snippet, "verbose",
>   		opts->flags & REBASE_VERBOSE ? "t" : "");
>   	add_var(&script_snippet, "diffstat",
> @@ -756,7 +758,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	struct rebase_options options = {
>   		.type = REBASE_UNSPECIFIED,
>   		.flags = REBASE_NO_QUIET,
> -		.git_am_opt = STRBUF_INIT,
> +		.git_am_opts = ARGV_ARRAY_INIT,
>   		.allow_rerere_autoupdate  = -1,
>   		.allow_empty_message = 1,
>   		.git_format_patch_opt = STRBUF_INIT,
> @@ -777,12 +779,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		ACTION_EDIT_TODO,
>   		ACTION_SHOW_CURRENT_PATCH,
>   	} action = NO_ACTION;
> -	int committer_date_is_author_date = 0;
> -	int ignore_date = 0;
> -	int ignore_whitespace = 0;
>   	const char *gpg_sign = NULL;
> -	int opt_c = -1;
> -	struct string_list whitespace = STRING_LIST_INIT_NODUP;
>   	struct string_list exec = STRING_LIST_INIT_NODUP;
>   	const char *rebase_merges = NULL;
>   	int fork_point = -1;
> @@ -804,15 +801,20 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		{OPTION_NEGBIT, 'n', "no-stat", &options.flags, NULL,
>   			N_("do not show diffstat of what changed upstream"),
>   			PARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },
> -		OPT_BOOL(0, "ignore-whitespace", &ignore_whitespace,
> -			 N_("passed to 'git apply'")),
>   		OPT_BOOL(0, "signoff", &options.signoff,
>   			 N_("add a Signed-off-by: line to each commit")),
> -		OPT_BOOL(0, "committer-date-is-author-date",
> -			 &committer_date_is_author_date,
> -			 N_("passed to 'git am'")),
> -		OPT_BOOL(0, "ignore-date", &ignore_date,
> -			 N_("passed to 'git am'")),
> +		OPT_PASSTHRU_ARGV(0, "ignore-whitespace", &options.git_am_opts,
> +				  NULL, N_("passed to 'git am'"),
> +				  PARSE_OPT_NOARG),
> +		OPT_PASSTHRU_ARGV(0, "committer-date-is-author-date",
> +				  &options.git_am_opts, NULL,
> +				  N_("passed to 'git am'"), PARSE_OPT_NOARG),
> +		OPT_PASSTHRU_ARGV(0, "ignore-date", &options.git_am_opts, NULL,
> +				  N_("passed to 'git am'"), PARSE_OPT_NOARG),
> +		OPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_("n"),
> +				  N_("passed to 'git apply'"), 0),
> +		OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts,
> +				  N_("action"), N_("passed to 'git apply'"), 0),
>   		OPT_BIT('f', "force-rebase", &options.flags,
>   			N_("cherry-pick all commits, even if unchanged"),
>   			REBASE_FORCE),
> @@ -856,10 +858,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		{ OPTION_STRING, 'S', "gpg-sign", &gpg_sign, N_("key-id"),
>   			N_("GPG-sign commits"),
>   			PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
> -		OPT_STRING_LIST(0, "whitespace", &whitespace,
> -				N_("whitespace"), N_("passed to 'git apply'")),
> -		OPT_SET_INT('C', NULL, &opt_c, N_("passed to 'git apply'"),
> -			    REBASE_AM),
>   		OPT_BOOL(0, "autostash", &options.autostash,
>   			 N_("automatically stash/stash pop before and after")),
>   		OPT_STRING_LIST('x', "exec", &exec, N_("exec"),
> @@ -884,6 +882,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   			 N_("rebase all reachable commits up to the root(s)")),
>   		OPT_END(),
>   	};
> +	int i;
>   
>   	/*
>   	 * NEEDSWORK: Once the builtin rebase has been tested enough
> @@ -1064,22 +1063,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		    state_dir_base, cmd_live_rebase, buf.buf);
>   	}
>   
> -	if (!(options.flags & REBASE_NO_QUIET))
> -		strbuf_addstr(&options.git_am_opt, " -q");
> -
> -	if (committer_date_is_author_date) {
> -		strbuf_addstr(&options.git_am_opt,
> -			      " --committer-date-is-author-date");
> -		options.flags |= REBASE_FORCE;
> +	for (i = 0; i < options.git_am_opts.argc; i++) {
> +		const char *option = options.git_am_opts.argv[i];
> +		if (!strcmp(option, "--committer-date-is-author-date") ||
> +		    !strcmp(option, "--ignore-date") ||
> +		    !strcmp(option, "--whitespace=fix") ||
> +		    !strcmp(option, "--whitespace=strip"))
> +			options.flags |= REBASE_FORCE;
>   	}
>   
> -	if (ignore_whitespace)
> -		strbuf_addstr(&options.git_am_opt, " --ignore-whitespace");
> -
> -	if (ignore_date) {
> -		strbuf_addstr(&options.git_am_opt, " --ignore-date");
> -		options.flags |= REBASE_FORCE;
> -	}
> +	if (!(options.flags & REBASE_NO_QUIET))
> +		argv_array_push(&options.git_am_opts, "-q");
>   
>   	if (options.keep_empty)
>   		imply_interactive(&options, "--keep-empty");
> @@ -1089,23 +1083,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
>   	}
>   
> -	if (opt_c >= 0)
> -		strbuf_addf(&options.git_am_opt, " -C%d", opt_c);
> -
> -	if (whitespace.nr) {
> -		int i;
> -
> -		for (i = 0; i < whitespace.nr; i++) {
> -			const char *item = whitespace.items[i].string;
> -
> -			strbuf_addf(&options.git_am_opt, " --whitespace=%s",
> -				    item);
> -
> -			if ((!strcmp(item, "fix")) || (!strcmp(item, "strip")))
> -				options.flags |= REBASE_FORCE;
> -		}
> -	}
> -
>   	if (exec.nr) {
>   		int i;
>   
> @@ -1181,23 +1158,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		break;
>   	}
>   
> -	if (options.git_am_opt.len) {
> -		const char *p;
> -
> +	if (options.git_am_opts.argc) {
>   		/* all am options except -q are compatible only with --am */
> -		strbuf_reset(&buf);
> -		strbuf_addbuf(&buf, &options.git_am_opt);
> -		strbuf_addch(&buf, ' ');
> -		while ((p = strstr(buf.buf, " -q ")))
> -			strbuf_splice(&buf, p - buf.buf, 4, " ", 1);
> -		strbuf_trim(&buf);
> +		for (i = options.git_am_opts.argc - 1; i >= 0; i--)
> +			if (strcmp(options.git_am_opts.argv[i], "-q"))
> +				break;
>   
> -		if (is_interactive(&options) && buf.len)
> +		if (is_interactive(&options) && i >= 0)
>   			die(_("error: cannot combine interactive options "
>   			      "(--interactive, --exec, --rebase-merges, "
>   			      "--preserve-merges, --keep-empty, --root + "
>   			      "--onto) with am options (%s)"), buf.buf);
> -		if (options.type == REBASE_MERGE && buf.len)
> +		if (options.type == REBASE_MERGE && i >= 0)
>   			die(_("error: cannot combine merge options (--merge, "
>   			      "--strategy, --strategy-option) with am options "
>   			      "(%s)"), buf.buf);
> @@ -1207,7 +1179,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		if (options.type == REBASE_PRESERVE_MERGES)
>   			die("cannot combine '--signoff' with "
>   			    "'--preserve-merges'");
> -		strbuf_addstr(&options.git_am_opt, " --signoff");
> +		argv_array_push(&options.git_am_opts, "--signoff");
>   		options.flags |= REBASE_FORCE;
>   	}
>   
> 


```

## Johannes Schindelin, 2018-11-13 19:21

Subject: Re: [PATCH 1/1] rebase: really just passthru the `git am` options
Message-ID: <nycvar.QRO.7.76.6.1811132019440.39@tvgsbejvaqbjf.bet>
URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1811132019440.39%40tvgsbejvaqbjf.bet
In-Reply-To: <00d9b0e4-dde5-c96d-76d7-42fb9ac393f8@talktalk.net>

```
Hi Phillip,

On Tue, 13 Nov 2018, Phillip Wood wrote:

> Thanks for looking at this. Unfortunately using OPT_PASSTHRU_ARGV seems to
> break the error reporting
> 
> Running
>   bin/wrappers/git rebase --onto @^^^^ @^^ -Cbad
> 
> Gives
>   git encountered an error while preparing the patches to replay
>   these revisions:
> 
> 
> 67f673aa4a580b9e407b1ca505abf1f50510ec47...7c3e01a708856885e60bf4051586970e65dd326c
> 
>   As a result, git cannot rebase them.
> 
> If I do
> 
>   bin/wrappers/git rebase @^^ -Cbad
> 
> I get no error, it just tells me that it does not need to rebase (which is
> true)

Hmm. Isn't this the same behavior as with the scripted version? Also: are
we sure that we want to allow options to come *after* the `<upstream>`
argument?

Ciao,
Dscho

> Best Wishes
> 
> Phillip
> 
> 
> On 13/11/2018 12:38, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > 
> > Currently, we parse the options intended for `git am` as if we wanted to
> > handle them in `git rebase`, and then reconstruct them painstakingly to
> > define the `git_am_opt` variable.
> > 
> > However, there is a much better way (that I was unaware of, at the time
> > when I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.
> > It is intended for exactly this use case, where command-line options
> > want to be parsed into a separate `argv_array`.
> > 
> > Let's use this feature.
> > 
> > Incidentally, this also allows us to address a bug discovered by Phillip
> > Wood, where the built-in rebase failed to understand that the `-C`
> > option takes an optional argument.
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >   builtin/rebase.c | 98 +++++++++++++++++-------------------------------
> >   1 file changed, 35 insertions(+), 63 deletions(-)
> > 
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 0ee06aa363..96ffa80b71 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -87,7 +87,7 @@ struct rebase_options {
> >     REBASE_FORCE = 1<<3,
> >     REBASE_INTERACTIVE_EXPLICIT = 1<<4,
> >   	} flags;
> > -	struct strbuf git_am_opt;
> > +	struct argv_array git_am_opts;
> >    const char *action;
> >    int signoff;
> >    int allow_rerere_autoupdate;
> > @@ -339,7 +339,7 @@ N_("Resolve all conflicts manually, mark them as
> > resolved with\n"
> >   static int run_specific_rebase(struct rebase_options *opts)
> >   {
> >   	const char *argv[] = { NULL, NULL };
> > -	struct strbuf script_snippet = STRBUF_INIT;
> > +	struct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;
> >    int status;
> >    const char *backend, *backend_func;
> >   @@ -433,7 +433,9 @@ static int run_specific_rebase(struct rebase_options
> > *opts)
> >    	oid_to_hex(&opts->restrict_revision->object.oid) : NULL);
> >    add_var(&script_snippet, "GIT_QUIET",
> >   		opts->flags & REBASE_NO_QUIET ? "" : "t");
> > -	add_var(&script_snippet, "git_am_opt", opts->git_am_opt.buf);
> > +	sq_quote_argv_pretty(&buf, opts->git_am_opts.argv);
> > +	add_var(&script_snippet, "git_am_opt", buf.buf);
> > +	strbuf_release(&buf);
> >    add_var(&script_snippet, "verbose",
> >    	opts->flags & REBASE_VERBOSE ? "t" : "");
> >   	add_var(&script_snippet, "diffstat",
> > @@ -756,7 +758,7 @@ int cmd_rebase(int argc, const char **argv, const char
> > *prefix)
> >    struct rebase_options options = {
> >     .type = REBASE_UNSPECIFIED,
> >     .flags = REBASE_NO_QUIET,
> > -		.git_am_opt = STRBUF_INIT,
> > +		.git_am_opts = ARGV_ARRAY_INIT,
> >     .allow_rerere_autoupdate  = -1,
> >     .allow_empty_message = 1,
> >     .git_format_patch_opt = STRBUF_INIT,
> > @@ -777,12 +779,7 @@ int cmd_rebase(int argc, const char **argv, const
> > char *prefix)
> >     ACTION_EDIT_TODO,
> >     ACTION_SHOW_CURRENT_PATCH,
> >   	} action = NO_ACTION;
> > -	int committer_date_is_author_date = 0;
> > -	int ignore_date = 0;
> > -	int ignore_whitespace = 0;
> >   	const char *gpg_sign = NULL;
> > -	int opt_c = -1;
> > -	struct string_list whitespace = STRING_LIST_INIT_NODUP;
> >    struct string_list exec = STRING_LIST_INIT_NODUP;
> >    const char *rebase_merges = NULL;
> >    int fork_point = -1;
> > @@ -804,15 +801,20 @@ int cmd_rebase(int argc, const char **argv, const
> > char *prefix)
> >     {OPTION_NEGBIT, 'n', "no-stat", &options.flags, NULL,
> >      N_("do not show diffstat of what changed upstream"),
> >      PARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },
> > -		OPT_BOOL(0, "ignore-whitespace", &ignore_whitespace,
> > -			 N_("passed to 'git apply'")),
> >     OPT_BOOL(0, "signoff", &options.signoff,
> >   			 N_("add a Signed-off-by: line to each commit")),
> > -		OPT_BOOL(0, "committer-date-is-author-date",
> > -			 &committer_date_is_author_date,
> > -			 N_("passed to 'git am'")),
> > -		OPT_BOOL(0, "ignore-date", &ignore_date,
> > -			 N_("passed to 'git am'")),
> > +		OPT_PASSTHRU_ARGV(0, "ignore-whitespace",
> > &options.git_am_opts,
> > +				  NULL, N_("passed to 'git am'"),
> > +				  PARSE_OPT_NOARG),
> > +		OPT_PASSTHRU_ARGV(0, "committer-date-is-author-date",
> > +				  &options.git_am_opts, NULL,
> > +				  N_("passed to 'git am'"),
> > PARSE_OPT_NOARG),
> > +		OPT_PASSTHRU_ARGV(0, "ignore-date", &options.git_am_opts,
> > NULL,
> > +				  N_("passed to 'git am'"),
> > PARSE_OPT_NOARG),
> > +		OPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_("n"),
> > +				  N_("passed to 'git apply'"), 0),
> > +		OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts,
> > +				  N_("action"), N_("passed to 'git apply'"),
> > 0),
> >     OPT_BIT('f', "force-rebase", &options.flags,
> >      N_("cherry-pick all commits, even if unchanged"),
> >      REBASE_FORCE),
> > @@ -856,10 +858,6 @@ int cmd_rebase(int argc, const char **argv, const
> > char *prefix)
> >     { OPTION_STRING, 'S', "gpg-sign", &gpg_sign, N_("key-id"),
> >      N_("GPG-sign commits"),
> >      PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
> > -		OPT_STRING_LIST(0, "whitespace", &whitespace,
> > -				N_("whitespace"), N_("passed to 'git
> > apply'")),
> > -		OPT_SET_INT('C', NULL, &opt_c, N_("passed to 'git apply'"),
> > -			    REBASE_AM),
> >     OPT_BOOL(0, "autostash", &options.autostash,
> >     	 N_("automatically stash/stash pop before and after")),
> >   		OPT_STRING_LIST('x', "exec", &exec, N_("exec"),
> > @@ -884,6 +882,7 @@ int cmd_rebase(int argc, const char **argv, const char
> > *prefix)
> >     	 N_("rebase all reachable commits up to the root(s)")),
> >    	OPT_END(),
> >   	};
> > +	int i;
> >   
> >    /*
> >   	 * NEEDSWORK: Once the builtin rebase has been tested enough
> > @@ -1064,22 +1063,17 @@ int cmd_rebase(int argc, const char **argv, const
> > char *prefix)
> >    	    state_dir_base, cmd_live_rebase, buf.buf);
> >    }
> >   -	if (!(options.flags & REBASE_NO_QUIET))
> > -		strbuf_addstr(&options.git_am_opt, " -q");
> > -
> > -	if (committer_date_is_author_date) {
> > -		strbuf_addstr(&options.git_am_opt,
> > -			      " --committer-date-is-author-date");
> > -		options.flags |= REBASE_FORCE;
> > +	for (i = 0; i < options.git_am_opts.argc; i++) {
> > +		const char *option = options.git_am_opts.argv[i];
> > +		if (!strcmp(option, "--committer-date-is-author-date") ||
> > +		    !strcmp(option, "--ignore-date") ||
> > +		    !strcmp(option, "--whitespace=fix") ||
> > +		    !strcmp(option, "--whitespace=strip"))
> > +			options.flags |= REBASE_FORCE;
> >    }
> >   -	if (ignore_whitespace)
> > -		strbuf_addstr(&options.git_am_opt, " --ignore-whitespace");
> > -
> > -	if (ignore_date) {
> > -		strbuf_addstr(&options.git_am_opt, " --ignore-date");
> > -		options.flags |= REBASE_FORCE;
> > -	}
> > +	if (!(options.flags & REBASE_NO_QUIET))
> > +		argv_array_push(&options.git_am_opts, "-q");
> >   
> >    if (options.keep_empty)
> >   		imply_interactive(&options, "--keep-empty");
> > @@ -1089,23 +1083,6 @@ int cmd_rebase(int argc, const char **argv, const
> > char *prefix)
> >    	options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
> >    }
> >   -	if (opt_c >= 0)
> > -		strbuf_addf(&options.git_am_opt, " -C%d", opt_c);
> > -
> > -	if (whitespace.nr) {
> > -		int i;
> > -
> > -		for (i = 0; i < whitespace.nr; i++) {
> > -			const char *item = whitespace.items[i].string;
> > -
> > -			strbuf_addf(&options.git_am_opt, " --whitespace=%s",
> > -				    item);
> > -
> > -			if ((!strcmp(item, "fix")) || (!strcmp(item,
> > "strip")))
> > -				options.flags |= REBASE_FORCE;
> > -		}
> > -	}
> > -
> >    if (exec.nr) {
> >     int i;
> >   @@ -1181,23 +1158,18 @@ int cmd_rebase(int argc, const char **argv,
> > const char *prefix)
> >    	break;
> >    }
> >   -	if (options.git_am_opt.len) {
> > -		const char *p;
> > -
> > +	if (options.git_am_opts.argc) {
> >   		/* all am options except -q are compatible only with --am */
> > -		strbuf_reset(&buf);
> > -		strbuf_addbuf(&buf, &options.git_am_opt);
> > -		strbuf_addch(&buf, ' ');
> > -		while ((p = strstr(buf.buf, " -q ")))
> > -			strbuf_splice(&buf, p - buf.buf, 4, " ", 1);
> > -		strbuf_trim(&buf);
> > +		for (i = options.git_am_opts.argc - 1; i >= 0; i--)
> > +			if (strcmp(options.git_am_opts.argv[i], "-q"))
> > +				break;
> >   -		if (is_interactive(&options) && buf.len)
> > +		if (is_interactive(&options) && i >= 0)
> >      die(_("error: cannot combine interactive options "
> >            "(--interactive, --exec, --rebase-merges, "
> >            "--preserve-merges, --keep-empty, --root + "
> >            "--onto) with am options (%s)"), buf.buf);
> > -		if (options.type == REBASE_MERGE && buf.len)
> > +		if (options.type == REBASE_MERGE && i >= 0)
> >      die(_("error: cannot combine merge options (--merge, "
> >            "--strategy, --strategy-option) with am options "
> >            "(%s)"), buf.buf);
> > @@ -1207,7 +1179,7 @@ int cmd_rebase(int argc, const char **argv, const
> > char *prefix)
> >     if (options.type == REBASE_PRESERVE_MERGES)
> >      die("cannot combine '--signoff' with "
> >   			    "'--preserve-merges'");
> > -		strbuf_addstr(&options.git_am_opt, " --signoff");
> > +		argv_array_push(&options.git_am_opts, "--signoff");
> >    	options.flags |= REBASE_FORCE;
> >    }
> >   
> > 
> 
> 

```

## Phillip Wood, 2018-11-13 19:58

Subject: Re: [PATCH 1/1] rebase: really just passthru the `git am` options
Message-ID: <1b8461d1-6cb7-6622-94d2-44c27623236d@talktalk.net>
URL: https://gitlist.dev/e/1b8461d1-6cb7-6622-94d2-44c27623236d%40talktalk.net
In-Reply-To: <nycvar.QRO.7.76.6.1811132019440.39@tvgsbejvaqbjf.bet>

```
Hi Johannes

On 13/11/2018 19:21, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Tue, 13 Nov 2018, Phillip Wood wrote:
> 
>> Thanks for looking at this. Unfortunately using OPT_PASSTHRU_ARGV seems to
>> break the error reporting
>>
>> Running
>>    bin/wrappers/git rebase --onto @^^^^ @^^ -Cbad
>>
>> Gives
>>    git encountered an error while preparing the patches to replay
>>    these revisions:
>>
>>
>> 67f673aa4a580b9e407b1ca505abf1f50510ec47...7c3e01a708856885e60bf4051586970e65dd326c
>>
>>    As a result, git cannot rebase them.
>>

git 2.19.1 gives

First, rewinding head to replay your work on top of it...
Applying: Ninth batch for 2.20
error: switch `C' expects a numerical value

So it has a clear message as to what the error is, this patch regresses 
that. It would be better if rebase detected the error before starting 
though.

>> If I do
>>
>>    bin/wrappers/git rebase @^^ -Cbad
>>
>> I get no error, it just tells me that it does not need to rebase (which is
>> true)
> 
> Hmm. Isn't this the same behavior as with the scripted version?

Ah you're right the script does not check if the option argument is valid.

> Also: are
> we sure that we want to allow options to come *after* the `<upstream>`
> argument?

Maybe not but the scripted version does. I'm not sure if that is a good 
idea or not.

Best Wishes

Phillip

> Ciao,
> Dscho
> 
>> Best Wishes
>>
>> Phillip
>>
>>
>> On 13/11/2018 12:38, Johannes Schindelin via GitGitGadget wrote:
>>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>>
>>> Currently, we parse the options intended for `git am` as if we wanted to
>>> handle them in `git rebase`, and then reconstruct them painstakingly to
>>> define the `git_am_opt` variable.
>>>
>>> However, there is a much better way (that I was unaware of, at the time
>>> when I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.
>>> It is intended for exactly this use case, where command-line options
>>> want to be parsed into a separate `argv_array`.
>>>
>>> Let's use this feature.
>>>
>>> Incidentally, this also allows us to address a bug discovered by Phillip
>>> Wood, where the built-in rebase failed to understand that the `-C`
>>> option takes an optional argument.
>>>
>>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>>> ---
>>>    builtin/rebase.c | 98 +++++++++++++++++-------------------------------
>>>    1 file changed, 35 insertions(+), 63 deletions(-)
>>>
>>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>>> index 0ee06aa363..96ffa80b71 100644
>>> --- a/builtin/rebase.c
>>> +++ b/builtin/rebase.c
>>> @@ -87,7 +87,7 @@ struct rebase_options {
>>>      REBASE_FORCE = 1<<3,
>>>      REBASE_INTERACTIVE_EXPLICIT = 1<<4,
>>>    	} flags;
>>> -	struct strbuf git_am_opt;
>>> +	struct argv_array git_am_opts;
>>>     const char *action;
>>>     int signoff;
>>>     int allow_rerere_autoupdate;
>>> @@ -339,7 +339,7 @@ N_("Resolve all conflicts manually, mark them as
>>> resolved with\n"
>>>    static int run_specific_rebase(struct rebase_options *opts)
>>>    {
>>>    	const char *argv[] = { NULL, NULL };
>>> -	struct strbuf script_snippet = STRBUF_INIT;
>>> +	struct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;
>>>     int status;
>>>     const char *backend, *backend_func;
>>>    @@ -433,7 +433,9 @@ static int run_specific_rebase(struct rebase_options
>>> *opts)
>>>     	oid_to_hex(&opts->restrict_revision->object.oid) : NULL);
>>>     add_var(&script_snippet, "GIT_QUIET",
>>>    		opts->flags & REBASE_NO_QUIET ? "" : "t");
>>> -	add_var(&script_snippet, "git_am_opt", opts->git_am_opt.buf);
>>> +	sq_quote_argv_pretty(&buf, opts->git_am_opts.argv);
>>> +	add_var(&script_snippet, "git_am_opt", buf.buf);
>>> +	strbuf_release(&buf);
>>>     add_var(&script_snippet, "verbose",
>>>     	opts->flags & REBASE_VERBOSE ? "t" : "");
>>>    	add_var(&script_snippet, "diffstat",
>>> @@ -756,7 +758,7 @@ int cmd_rebase(int argc, const char **argv, const char
>>> *prefix)
>>>     struct rebase_options options = {
>>>      .type = REBASE_UNSPECIFIED,
>>>      .flags = REBASE_NO_QUIET,
>>> -		.git_am_opt = STRBUF_INIT,
>>> +		.git_am_opts = ARGV_ARRAY_INIT,
>>>      .allow_rerere_autoupdate  = -1,
>>>      .allow_empty_message = 1,
>>>      .git_format_patch_opt = STRBUF_INIT,
>>> @@ -777,12 +779,7 @@ int cmd_rebase(int argc, const char **argv, const
>>> char *prefix)
>>>      ACTION_EDIT_TODO,
>>>      ACTION_SHOW_CURRENT_PATCH,
>>>    	} action = NO_ACTION;
>>> -	int committer_date_is_author_date = 0;
>>> -	int ignore_date = 0;
>>> -	int ignore_whitespace = 0;
>>>    	const char *gpg_sign = NULL;
>>> -	int opt_c = -1;
>>> -	struct string_list whitespace = STRING_LIST_INIT_NODUP;
>>>     struct string_list exec = STRING_LIST_INIT_NODUP;
>>>     const char *rebase_merges = NULL;
>>>     int fork_point = -1;
>>> @@ -804,15 +801,20 @@ int cmd_rebase(int argc, const char **argv, const
>>> char *prefix)
>>>      {OPTION_NEGBIT, 'n', "no-stat", &options.flags, NULL,
>>>       N_("do not show diffstat of what changed upstream"),
>>>       PARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },
>>> -		OPT_BOOL(0, "ignore-whitespace", &ignore_whitespace,
>>> -			 N_("passed to 'git apply'")),
>>>      OPT_BOOL(0, "signoff", &options.signoff,
>>>    			 N_("add a Signed-off-by: line to each commit")),
>>> -		OPT_BOOL(0, "committer-date-is-author-date",
>>> -			 &committer_date_is_author_date,
>>> -			 N_("passed to 'git am'")),
>>> -		OPT_BOOL(0, "ignore-date", &ignore_date,
>>> -			 N_("passed to 'git am'")),
>>> +		OPT_PASSTHRU_ARGV(0, "ignore-whitespace",
>>> &options.git_am_opts,
>>> +				  NULL, N_("passed to 'git am'"),
>>> +				  PARSE_OPT_NOARG),
>>> +		OPT_PASSTHRU_ARGV(0, "committer-date-is-author-date",
>>> +				  &options.git_am_opts, NULL,
>>> +				  N_("passed to 'git am'"),
>>> PARSE_OPT_NOARG),
>>> +		OPT_PASSTHRU_ARGV(0, "ignore-date", &options.git_am_opts,
>>> NULL,
>>> +				  N_("passed to 'git am'"),
>>> PARSE_OPT_NOARG),
>>> +		OPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_("n"),
>>> +				  N_("passed to 'git apply'"), 0),
>>> +		OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts,
>>> +				  N_("action"), N_("passed to 'git apply'"),
>>> 0),
>>>      OPT_BIT('f', "force-rebase", &options.flags,
>>>       N_("cherry-pick all commits, even if unchanged"),
>>>       REBASE_FORCE),
>>> @@ -856,10 +858,6 @@ int cmd_rebase(int argc, const char **argv, const
>>> char *prefix)
>>>      { OPTION_STRING, 'S', "gpg-sign", &gpg_sign, N_("key-id"),
>>>       N_("GPG-sign commits"),
>>>       PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
>>> -		OPT_STRING_LIST(0, "whitespace", &whitespace,
>>> -				N_("whitespace"), N_("passed to 'git
>>> apply'")),
>>> -		OPT_SET_INT('C', NULL, &opt_c, N_("passed to 'git apply'"),
>>> -			    REBASE_AM),
>>>      OPT_BOOL(0, "autostash", &options.autostash,
>>>      	 N_("automatically stash/stash pop before and after")),
>>>    		OPT_STRING_LIST('x', "exec", &exec, N_("exec"),
>>> @@ -884,6 +882,7 @@ int cmd_rebase(int argc, const char **argv, const char
>>> *prefix)
>>>      	 N_("rebase all reachable commits up to the root(s)")),
>>>     	OPT_END(),
>>>    	};
>>> +	int i;
>>>    
>>>     /*
>>>    	 * NEEDSWORK: Once the builtin rebase has been tested enough
>>> @@ -1064,22 +1063,17 @@ int cmd_rebase(int argc, const char **argv, const
>>> char *prefix)
>>>     	    state_dir_base, cmd_live_rebase, buf.buf);
>>>     }
>>>    -	if (!(options.flags & REBASE_NO_QUIET))
>>> -		strbuf_addstr(&options.git_am_opt, " -q");
>>> -
>>> -	if (committer_date_is_author_date) {
>>> -		strbuf_addstr(&options.git_am_opt,
>>> -			      " --committer-date-is-author-date");
>>> -		options.flags |= REBASE_FORCE;
>>> +	for (i = 0; i < options.git_am_opts.argc; i++) {
>>> +		const char *option = options.git_am_opts.argv[i];
>>> +		if (!strcmp(option, "--committer-date-is-author-date") ||
>>> +		    !strcmp(option, "--ignore-date") ||
>>> +		    !strcmp(option, "--whitespace=fix") ||
>>> +		    !strcmp(option, "--whitespace=strip"))
>>> +			options.flags |= REBASE_FORCE;
>>>     }
>>>    -	if (ignore_whitespace)
>>> -		strbuf_addstr(&options.git_am_opt, " --ignore-whitespace");
>>> -
>>> -	if (ignore_date) {
>>> -		strbuf_addstr(&options.git_am_opt, " --ignore-date");
>>> -		options.flags |= REBASE_FORCE;
>>> -	}
>>> +	if (!(options.flags & REBASE_NO_QUIET))
>>> +		argv_array_push(&options.git_am_opts, "-q");
>>>    
>>>     if (options.keep_empty)
>>>    		imply_interactive(&options, "--keep-empty");
>>> @@ -1089,23 +1083,6 @@ int cmd_rebase(int argc, const char **argv, const
>>> char *prefix)
>>>     	options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
>>>     }
>>>    -	if (opt_c >= 0)
>>> -		strbuf_addf(&options.git_am_opt, " -C%d", opt_c);
>>> -
>>> -	if (whitespace.nr) {
>>> -		int i;
>>> -
>>> -		for (i = 0; i < whitespace.nr; i++) {
>>> -			const char *item = whitespace.items[i].string;
>>> -
>>> -			strbuf_addf(&options.git_am_opt, " --whitespace=%s",
>>> -				    item);
>>> -
>>> -			if ((!strcmp(item, "fix")) || (!strcmp(item,
>>> "strip")))
>>> -				options.flags |= REBASE_FORCE;
>>> -		}
>>> -	}
>>> -
>>>     if (exec.nr) {
>>>      int i;
>>>    @@ -1181,23 +1158,18 @@ int cmd_rebase(int argc, const char **argv,
>>> const char *prefix)
>>>     	break;
>>>     }
>>>    -	if (options.git_am_opt.len) {
>>> -		const char *p;
>>> -
>>> +	if (options.git_am_opts.argc) {
>>>    		/* all am options except -q are compatible only with --am */
>>> -		strbuf_reset(&buf);
>>> -		strbuf_addbuf(&buf, &options.git_am_opt);
>>> -		strbuf_addch(&buf, ' ');
>>> -		while ((p = strstr(buf.buf, " -q ")))
>>> -			strbuf_splice(&buf, p - buf.buf, 4, " ", 1);
>>> -		strbuf_trim(&buf);
>>> +		for (i = options.git_am_opts.argc - 1; i >= 0; i--)
>>> +			if (strcmp(options.git_am_opts.argv[i], "-q"))
>>> +				break;
>>>    -		if (is_interactive(&options) && buf.len)
>>> +		if (is_interactive(&options) && i >= 0)
>>>       die(_("error: cannot combine interactive options "
>>>             "(--interactive, --exec, --rebase-merges, "
>>>             "--preserve-merges, --keep-empty, --root + "
>>>             "--onto) with am options (%s)"), buf.buf);
>>> -		if (options.type == REBASE_MERGE && buf.len)
>>> +		if (options.type == REBASE_MERGE && i >= 0)
>>>       die(_("error: cannot combine merge options (--merge, "
>>>             "--strategy, --strategy-option) with am options "
>>>             "(%s)"), buf.buf);
>>> @@ -1207,7 +1179,7 @@ int cmd_rebase(int argc, const char **argv, const
>>> char *prefix)
>>>      if (options.type == REBASE_PRESERVE_MERGES)
>>>       die("cannot combine '--signoff' with "
>>>    			    "'--preserve-merges'");
>>> -		strbuf_addstr(&options.git_am_opt, " --signoff");
>>> +		argv_array_push(&options.git_am_opts, "--signoff");
>>>     	options.flags |= REBASE_FORCE;
>>>     }
>>>    
>>>
>>
>>


```

## Ævar Arnfjörð Bjarmason, 2018-11-13 21:50

Subject: rebase-in-C stability for 2.20
Message-ID: <87y39w1wc2.fsf@evledraar.gmail.com>
URL: https://gitlist.dev/e/87y39w1wc2.fsf%40evledraar.gmail.com
In-Reply-To: <1b8461d1-6cb7-6622-94d2-44c27623236d@talktalk.net>

```

On Tue, Nov 13 2018, Phillip Wood wrote:

> Hi Johannes
>
> On 13/11/2018 19:21, Johannes Schindelin wrote:
>> Hi Phillip,
>>
>> On Tue, 13 Nov 2018, Phillip Wood wrote:
>>
>>> Thanks for looking at this. Unfortunately using OPT_PASSTHRU_ARGV seems to
>>> break the error reporting
>>>
>>> Running
>>>    bin/wrappers/git rebase --onto @^^^^ @^^ -Cbad
>>>
>>> Gives
>>>    git encountered an error while preparing the patches to replay
>>>    these revisions:
>>>
>>>
>>> 67f673aa4a580b9e407b1ca505abf1f50510ec47...7c3e01a708856885e60bf4051586970e65dd326c
>>>
>>>    As a result, git cannot rebase them.
>>>
>
> git 2.19.1 gives
>
> First, rewinding head to replay your work on top of it...
> Applying: Ninth batch for 2.20
> error: switch `C' expects a numerical value
>
> So it has a clear message as to what the error is, this patch
> regresses that. It would be better if rebase detected the error before
> starting though.
>
>>> If I do
>>>
>>>    bin/wrappers/git rebase @^^ -Cbad
>>>
>>> I get no error, it just tells me that it does not need to rebase (which is
>>> true)
>>
>> Hmm. Isn't this the same behavior as with the scripted version?
>
> Ah you're right the script does not check if the option argument is valid.
>
>> Also: are
>> we sure that we want to allow options to come *after* the `<upstream>`
>> argument?
>
> Maybe not but the scripted version does. I'm not sure if that is a
> good idea or not.

According to Junio's calendar we're now 2 days from 2.20-rc0. We have
the js/rebase-autostash-detach-fix bug I reported sitting in "pu" still,
and then this.

I see we still have rebase.useBuiltin in the code as an escape hatch,
but it's undocumented.

Given that we're still finding regressions bugs in the rebase-in-C
version should we be considering reverting 5541bd5b8f ("rebase: default
to using the builtin rebase", 2018-08-08)?

I love the feature, but fear that the current list of known regressions
serve as a canary for a larger list which we'd discover if we held off
for another major release (and would re-enable rebase.useBuiltin=true in
master right after 2.20 is out the door).

But maybe I'm being overly paranoid. What do those more familiar with
this think?

```

## Stefan Beller, 2018-11-14 00:07

Subject: Re: rebase-in-C stability for 2.20
Message-ID: <CAGZ79kYX4vNMYgzitY1D8Bu7o=2oY6Aiq0d7KQdCTaC5eu=RYg@mail.gmail.com>
URL: https://gitlist.dev/e/CAGZ79kYX4vNMYgzitY1D8Bu7o%3D2oY6Aiq0d7KQdCTaC5eu%3DRYg%40mail.gmail.com
In-Reply-To: <87y39w1wc2.fsf@evledraar.gmail.com>

```
> But maybe I'm being overly paranoid. What do those more familiar with
> this think?

I am not too worried,
* as rebase is a main porcelain, that is even hard to use in a script.
  so any failures are not deep down in some automation,
  but when found exposed quickly (and hopefully reported).
* 5541bd5b8f was merged to next a month ago; internally we
   distribute the next branch to Googlers (on a weekly basis)
   and we have not had any bug reports regarding rebase.
   (Maybe our environment is too strict for the wide range
    of bugs reported)
* Johannes reported that the rebase is used in GfW
   https://public-inbox.org/git/nycvar.QRO.7.76.6.1808241320540.73@tvgsbejvaqbjf.bet/
   https://github.com/git-for-windows/git/pull/1800
   and from my cursory reading it is part of
   2.19.windows, which has a large user base.

> (and would re-enable rebase.useBuiltin=true in
> master right after 2.20 is out the door).

That would be fine with me as well, but I'd rather
document rebase.useBuiltin instead of flip-flopping
the switch around the release.

Have there been any fixes that are only in
the C version (has the shell version already bitrotted)?

Stefan

```

## Elijah Newren, 2018-11-14 00:36

Subject: Re: rebase-in-C stability for 2.20
Message-ID: <CABPp-BGOq2jAp79qU-XYkCv18WLBdw7huKsmBYTJcS2e_jzvFw@mail.gmail.com>
URL: https://gitlist.dev/e/CABPp-BGOq2jAp79qU-XYkCv18WLBdw7huKsmBYTJcS2e_jzvFw%40mail.gmail.com
In-Reply-To: <87y39w1wc2.fsf@evledraar.gmail.com>

```
On Tue, Nov 13, 2018 at 1:52 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> According to Junio's calendar we're now 2 days from 2.20-rc0. We have
> the js/rebase-autostash-detach-fix bug I reported sitting in "pu" still,
> and then this.
>
> I see we still have rebase.useBuiltin in the code as an escape hatch,
> but it's undocumented.
>
> Given that we're still finding regressions bugs in the rebase-in-C
> version should we be considering reverting 5541bd5b8f ("rebase: default
> to using the builtin rebase", 2018-08-08)?

That date feels slightly misleading; it has a commit date of Oct 11,
and only merged to master less than two weeks ago.  Given how big the
two different rebase in C series were, I'd expect a couple small
things to fall out after it hit master, which is what appears to be
happening.

> I love the feature, but fear that the current list of known regressions
> serve as a canary for a larger list which we'd discover if we held off
> for another major release (and would re-enable rebase.useBuiltin=true in
> master right after 2.20 is out the door).
>
> But maybe I'm being overly paranoid. What do those more familiar with
> this think?

I probably don't qualify as more familar, but giving my $0.02
anyway...  I'm happy that setting rebase.useBuiltin=false by default
exists an escape hatch if things don't settle down as we get closer to
the release, but I'd rather wait until further into the RC cycle
before going that route, personally.

```

## Junio C Hamano, 2018-11-14 03:39

Subject: Re: rebase-in-C stability for 2.20
Message-ID: <xmqq8t1wqqd7.fsf@gitster-ct.c.googlers.com>
URL: https://gitlist.dev/e/xmqq8t1wqqd7.fsf%40gitster-ct.c.googlers.com
In-Reply-To: <87y39w1wc2.fsf@evledraar.gmail.com>

```
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> According to Junio's calendar we're now 2 days from 2.20-rc0. We have
> the js/rebase-autostash-detach-fix bug I reported sitting in "pu" still,
> and then this.
>
> I see we still have rebase.useBuiltin in the code as an escape hatch,
> but it's undocumented.
>
> Given that we're still finding regressions bugs in the rebase-in-C
> version should we be considering reverting 5541bd5b8f ("rebase: default
> to using the builtin rebase", 2018-08-08)?
>
> I love the feature, but fear that the current list of known regressions
> serve as a canary for a larger list which we'd discover if we held off
> for another major release (and would re-enable rebase.useBuiltin=true in
> master right after 2.20 is out the door).
>
> But maybe I'm being overly paranoid. What do those more familiar with
> this think?

I was hoping that having it early in GfW 2.19 would have smoked out
all the remaining issues, but it seems that those building from the
source and testing have usage patterns different enough to find more
issues.  This is a normal part of the development process, and
hopefully the remaining bugs are minor and can be flushed out in the
-rc testing period---this kind of thing is the whole reason why we
code-freeze and test rcs.

It unfortunately is too late to depend on the rebase.useBuiltin as
an escape hatch, as 'master', 'next', or anywhere else had the
"rebase and rebase -i in C" series merged without it set to false
and used in any seriousness.  Quite honestly, I think we can trust
the rest of the "rebase and rebase -i in C" series much more than
its "use the scripted one even though we have built-in one" part.

So if we have to seriously consider holding back, we may be better
off ripping the whole thing out.  I do not offhand know how involved
such a reversion would be, though, and I am *NOT* looking forward to
having do such a major surgery right before the release.

```

## Jeff King, 2018-11-14 07:29

Subject: Re: [PATCH 0/1] rebase: understand -C again, refactor
Message-ID: <20181114072909.GD19904@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20181114072909.GD19904%40sigill.intra.peff.net
In-Reply-To: <pull.76.git.gitgitgadget@gmail.com>

```
On Tue, Nov 13, 2018 at 04:38:24AM -0800, Johannes Schindelin via GitGitGadget wrote:

> Phillip Wood reported a problem where the built-in rebase did not understand
> options like -C1, i.e. it did not expect the option argument.
> 
> While investigating how to address this best, I stumbled upon 
> OPT_PASSTHRU_ARGV (which I was so far happily unaware of).

I was unaware of it, too. Looking at the OPT_PASSTHRU and its ARGV
counterpart, I think the original intent was that you'd pass through
normal last-one-wins individual options with OPT_PASSTHRU, and then
list-like options with OPT_PASSTHRU_ARGV. But here you've used the
latter to pass sets of individual last-one-wins options.

That said, I think what you've done here is way simpler and more
readable than using a bunch of OPT_PASSTHRUs would have been. And even
if it was not the original intent of the ARGV variant, I can't see any
reason to avoid doing it this way.

-Peff

```

## Ævar Arnfjörð Bjarmason, 2018-11-14 09:01

Subject: [PATCH 0/2] rebase.useBuiltin doc & test mode
Message-ID: <20181114090144.31412-1-avarab@gmail.com>
URL: https://gitlist.dev/e/20181114090144.31412-1-avarab%40gmail.com
In-Reply-To: <CAGZ79kYX4vNMYgzitY1D8Bu7o=2oY6Aiq0d7KQdCTaC5eu=RYg@mail.gmail.com>

```
On Wed, Nov 14 2018, Stefan Beller wrote:

>> But maybe I'm being overly paranoid. What do those more familiar with
>> this think?
>
> I am not too worried,
> * as rebase is a main porcelain, that is even hard to use in a script.
>   so any failures are not deep down in some automation,
>   but when found exposed quickly (and hopefully reported).
> * 5541bd5b8f was merged to next a month ago; internally we
>    distribute the next branch to Googlers (on a weekly basis)
>    and we have not had any bug reports regarding rebase.
>    (Maybe our environment is too strict for the wide range
>     of bugs reported)

I do the same at Booking.com (although at a more ad-hoc schedule) and
got the report whose fix is now sitting in "pu" noted upthread.

I fear that these sorts of corporate environments, both Google's and
Booking's, end up testing a relatively narrow featureset. Most people
have similar enough workflows, e.g. just using "git pull --rebase",
I'd be surprised if we have more than 2-3 internal users who ever use
the --onto option for example.

> * Johannes reported that the rebase is used in GfW
>    https://public-inbox.org/git/nycvar.QRO.7.76.6.1808241320540.73@tvgsbejvaqbjf.bet/
>    https://github.com/git-for-windows/git/pull/1800
>    and from my cursory reading it is part of
>    2.19.windows, which has a large user base.
>
>> (and would re-enable rebase.useBuiltin=true in
>> master right after 2.20 is out the door).
>
> That would be fine with me as well, but I'd rather
> document rebase.useBuiltin instead of flip-flopping
> the switch around the release.
>
> Have there been any fixes that are only in
> the C version (has the shell version already bitrotted)?

That's a good question, one which I don't think we knew the answer to
before the following patches. As it turns out no, we still run the
tests without failures with GIT_TEST_REBASE_USE_BUILTIN=false.

Ævar Arnfjörð Bjarmason (2):
  rebase doc: document rebase.useBuiltin
  tests: add a special setup where rebase.useBuiltin is off

 Documentation/config/rebase.txt | 14 ++++++++++++++
 builtin/rebase.c                |  5 ++++-
 t/README                        |  3 +++
 3 files changed, 21 insertions(+), 1 deletion(-)

-- 
2.19.1.1182.g4ecb1133ce


```

## Ævar Arnfjörð Bjarmason, 2018-11-14 09:01

Subject: [PATCH 1/2] rebase doc: document rebase.useBuiltin
Message-ID: <20181114090144.31412-2-avarab@gmail.com>
URL: https://gitlist.dev/e/20181114090144.31412-2-avarab%40gmail.com
In-Reply-To: <CAGZ79kYX4vNMYgzitY1D8Bu7o=2oY6Aiq0d7KQdCTaC5eu=RYg@mail.gmail.com>

```
The rebase.useBuiltin variable introduced in 55071ea248 ("rebase:
start implementing it as a builtin", 2018-08-07) was turned on by
default in 5541bd5b8f ("rebase: default to using the builtin rebase",
2018-08-08), but had no documentation.

Let's document it so that users who run into any stability issues with
the C rewrite know there's an escape hatch[1], and make it clear that
needing to turn off builtin rebase means you've found a bug in git.

1. https://public-inbox.org/git/87y39w1wc2.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/rebase.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index 42e1ba7575..f079bf6b7e 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -1,3 +1,17 @@
+rebase.useBuiltin::
+	Set to `false` to use the legacy shellscript implementation of
+	linkgit:git-rebase[1]. Is `true` by default, which means use
+	the built-in rewrite of it in C.
++
+The C rewrite is first included with Git version 2.20. This option
+serves an an escape hatch to re-enable the legacy version in case any
+bugs are found in the rewrite. This option and the shellscript version
+of linkgit:git-rebase[1] will be removed in some future release.
++
+If you find some reason to set this option to `false` other than
+one-off testing you should report the behavior difference as a bug in
+git.
+
 rebase.stat::
 	Whether to show a diffstat of what changed upstream since the last
 	rebase. False by default.
-- 
2.19.1.1182.g4ecb1133ce


```

## Ævar Arnfjörð Bjarmason, 2018-11-14 09:01

Subject: [PATCH 2/2] tests: add a special setup where rebase.useBuiltin is off
Message-ID: <20181114090144.31412-3-avarab@gmail.com>
URL: https://gitlist.dev/e/20181114090144.31412-3-avarab%40gmail.com
In-Reply-To: <CAGZ79kYX4vNMYgzitY1D8Bu7o=2oY6Aiq0d7KQdCTaC5eu=RYg@mail.gmail.com>

```
Add a GIT_TEST_REBASE_USE_BUILTIN=false test mode which is equivalent
to running with rebase.useBuiltin=false. This is needed to spot that
we're not introducing any regressions in the legacy rebase version
while we're carrying both it and the new builtin version.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c | 5 ++++-
 t/README         | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ee06aa363..68ad8c1149 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -48,7 +48,10 @@ static int use_builtin_rebase(void)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf out = STRBUF_INIT;
-	int ret;
+	int ret, env = git_env_bool("GIT_TEST_REBASE_USE_BUILTIN", -1);
+
+	if (env != -1)
+		return env;
 
 	argv_array_pushl(&cp.args,
 			 "config", "--bool", "rebase.usebuiltin", NULL);
diff --git a/t/README b/t/README
index 242497455f..c719e08414 100644
--- a/t/README
+++ b/t/README
@@ -348,6 +348,9 @@ GIT_TEST_MULTI_PACK_INDEX=<boolean>, when true, forces the multi-pack-
 index to be written after every 'git repack' command, and overrides the
 'core.multiPackIndex' setting to true.
 
+GIT_TEST_REBASE_USE_BUILTIN<boolean>, when false, disables the builtin
+version of git-rebase. See 'rebase.useBuiltin' in git-config(1).
+
 Naming Tests
 ------------
 
-- 
2.19.1.1182.g4ecb1133ce


```

## Johannes Schindelin, 2018-11-14 14:07

Subject: Re: [PATCH 0/2] rebase.useBuiltin doc & test mode
Message-ID: <nycvar.QRO.7.76.6.1811141506100.39@tvgsbejvaqbjf.bet>
URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1811141506100.39%40tvgsbejvaqbjf.bet
In-Reply-To: <20181114090144.31412-1-avarab@gmail.com>

```
Hi Ævar,

On Wed, 14 Nov 2018, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Nov 14 2018, Stefan Beller wrote:
> 
> >> But maybe I'm being overly paranoid. What do those more familiar with
> >> this think?
> >
> > I am not too worried,
> > * as rebase is a main porcelain, that is even hard to use in a script.
> >   so any failures are not deep down in some automation,
> >   but when found exposed quickly (and hopefully reported).
> > * 5541bd5b8f was merged to next a month ago; internally we
> >    distribute the next branch to Googlers (on a weekly basis)
> >    and we have not had any bug reports regarding rebase.
> >    (Maybe our environment is too strict for the wide range
> >     of bugs reported)
> 
> I do the same at Booking.com (although at a more ad-hoc schedule) and
> got the report whose fix is now sitting in "pu" noted upthread.
> 
> I fear that these sorts of corporate environments, both Google's and
> Booking's, end up testing a relatively narrow featureset. Most people
> have similar enough workflows, e.g. just using "git pull --rebase",
> I'd be surprised if we have more than 2-3 internal users who ever use
> the --onto option for example.
> 
> > * Johannes reported that the rebase is used in GfW
> >    https://public-inbox.org/git/nycvar.QRO.7.76.6.1808241320540.73@tvgsbejvaqbjf.bet/
> >    https://github.com/git-for-windows/git/pull/1800
> >    and from my cursory reading it is part of
> >    2.19.windows, which has a large user base.
> >
> >> (and would re-enable rebase.useBuiltin=true in
> >> master right after 2.20 is out the door).
> >
> > That would be fine with me as well, but I'd rather
> > document rebase.useBuiltin instead of flip-flopping
> > the switch around the release.
> >
> > Have there been any fixes that are only in
> > the C version (has the shell version already bitrotted)?
> 
> That's a good question, one which I don't think we knew the answer to
> before the following patches.

I pay close attention to `git rebase` bug reports and patches (obviously),
and there have not been any changes going into the built-in rebase/rebase
-i that necessitated changes also in the scripted version.

Ciao,
Dscho

> As it turns out no, we still run the tests without failures with
> GIT_TEST_REBASE_USE_BUILTIN=false.
> 
> Ævar Arnfjörð Bjarmason (2):
>   rebase doc: document rebase.useBuiltin
>   tests: add a special setup where rebase.useBuiltin is off
> 
>  Documentation/config/rebase.txt | 14 ++++++++++++++
>  builtin/rebase.c                |  5 ++++-
>  t/README                        |  3 +++
>  3 files changed, 21 insertions(+), 1 deletion(-)
> 
> -- 
> 2.19.1.1182.g4ecb1133ce
> 
> 
```

## Johannes Schindelin, 2018-11-14 14:22

Subject: Re: [PATCH 1/1] rebase: really just passthru the `git am` options
Message-ID: <nycvar.QRO.7.76.6.1811141508130.39@tvgsbejvaqbjf.bet>
URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1811141508130.39%40tvgsbejvaqbjf.bet
In-Reply-To: <1b8461d1-6cb7-6622-94d2-44c27623236d@talktalk.net>

```
Hi Phillip,

On Tue, 13 Nov 2018, Phillip Wood wrote:

> On 13/11/2018 19:21, Johannes Schindelin wrote:
> > Hi Phillip,
> > 
> > On Tue, 13 Nov 2018, Phillip Wood wrote:
> > 
> > > Thanks for looking at this. Unfortunately using OPT_PASSTHRU_ARGV seems to
> > > break the error reporting
> > >
> > > Running
> > >    bin/wrappers/git rebase --onto @^^^^ @^^ -Cbad
> > >
> > > Gives
> > >    git encountered an error while preparing the patches to replay
> > >    these revisions:
> > >
> > >
> > > 67f673aa4a580b9e407b1ca505abf1f50510ec47...7c3e01a708856885e60bf4051586970e65dd326c
> > >
> > >    As a result, git cannot rebase them.
> > >
> 
> git 2.19.1 gives
> 
> First, rewinding head to replay your work on top of it...
> Applying: Ninth batch for 2.20
> error: switch `C' expects a numerical value
> 
> So it has a clear message as to what the error is, this patch regresses that.
> It would be better if rebase detected the error before starting though.

I agree that that would make most sense: why start something when you can
know that it will fail later...

Let me try to add that test case that Junio wanted, and some early error
handling.

> > > If I do
> > >
> > >    bin/wrappers/git rebase @^^ -Cbad
> > >
> > > I get no error, it just tells me that it does not need to rebase (which is
> > > true)
> > 
> > Hmm. Isn't this the same behavior as with the scripted version?
> 
> Ah you're right the script does not check if the option argument is valid.
> 
> > Also: are
> > we sure that we want to allow options to come *after* the `<upstream>`
> > argument?
> 
> Maybe not but the scripted version does. I'm not sure if that is a good idea
> or not.

That behavior was never documented, though, was it?

Ciao,
Dscho

> 
> Best Wishes
> 
> Phillip
> 
> > Ciao,
> > Dscho
> > 
> > > Best Wishes
> > >
> > > Phillip
> > >
> > >
> > > On 13/11/2018 12:38, Johannes Schindelin via GitGitGadget wrote:
> > > > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > > >
> > > > Currently, we parse the options intended for `git am` as if we wanted to
> > > > handle them in `git rebase`, and then reconstruct them painstakingly to
> > > > define the `git_am_opt` variable.
> > > >
> > > > However, there is a much better way (that I was unaware of, at the time
> > > > when I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.
> > > > It is intended for exactly this use case, where command-line options
> > > > want to be parsed into a separate `argv_array`.
> > > >
> > > > Let's use this feature.
> > > >
> > > > Incidentally, this also allows us to address a bug discovered by Phillip
> > > > Wood, where the built-in rebase failed to understand that the `-C`
> > > > option takes an optional argument.
> > > >
> > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > > > ---
> > > >    builtin/rebase.c | 98
> > > >    +++++++++++++++++-------------------------------
> > > >    1 file changed, 35 insertions(+), 63 deletions(-)
> > > >
> > > > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > > > index 0ee06aa363..96ffa80b71 100644
> > > > --- a/builtin/rebase.c
> > > > +++ b/builtin/rebase.c
> > > > @@ -87,7 +87,7 @@ struct rebase_options {
> > > >      REBASE_FORCE = 1<<3,
> > > >      REBASE_INTERACTIVE_EXPLICIT = 1<<4,
> > > >    	} flags;
> > > > -	struct strbuf git_am_opt;
> > > > +	struct argv_array git_am_opts;
> > > >     const char *action;
> > > >     int signoff;
> > > >     int allow_rerere_autoupdate;
> > > > @@ -339,7 +339,7 @@ N_("Resolve all conflicts manually, mark them as
> > > > resolved with\n"
> > > >    static int run_specific_rebase(struct rebase_options *opts)
> > > >    {
> > > >    	const char *argv[] = { NULL, NULL };
> > > > -	struct strbuf script_snippet = STRBUF_INIT;
> > > > +	struct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;
> > > >     int status;
> > > >     const char *backend, *backend_func;
> > > >    @@ -433,7 +433,9 @@ static int run_specific_rebase(struct
> > > > rebase_options
> > > > *opts)
> > > >     	oid_to_hex(&opts->restrict_revision->object.oid) : NULL);
> > > >     add_var(&script_snippet, "GIT_QUIET",
> > > >    		opts->flags & REBASE_NO_QUIET ? "" : "t");
> > > > -	add_var(&script_snippet, "git_am_opt", opts->git_am_opt.buf);
> > > > +	sq_quote_argv_pretty(&buf, opts->git_am_opts.argv);
> > > > +	add_var(&script_snippet, "git_am_opt", buf.buf);
> > > > +	strbuf_release(&buf);
> > > >     add_var(&script_snippet, "verbose",
> > > >     	opts->flags & REBASE_VERBOSE ? "t" : "");
> > > >    	add_var(&script_snippet, "diffstat",
> > > > @@ -756,7 +758,7 @@ int cmd_rebase(int argc, const char **argv, const
> > > > char
> > > > *prefix)
> > > >     struct rebase_options options = {
> > > >      .type = REBASE_UNSPECIFIED,
> > > >      .flags = REBASE_NO_QUIET,
> > > > -		.git_am_opt = STRBUF_INIT,
> > > > +		.git_am_opts = ARGV_ARRAY_INIT,
> > > >      .allow_rerere_autoupdate  = -1,
> > > >      .allow_empty_message = 1,
> > > >      .git_format_patch_opt = STRBUF_INIT,
> > > > @@ -777,12 +779,7 @@ int cmd_rebase(int argc, const char **argv, const
> > > > char *prefix)
> > > >      ACTION_EDIT_TODO,
> > > >      ACTION_SHOW_CURRENT_PATCH,
> > > >    	} action = NO_ACTION;
> > > > -	int committer_date_is_author_date = 0;
> > > > -	int ignore_date = 0;
> > > > -	int ignore_whitespace = 0;
> > > >    	const char *gpg_sign = NULL;
> > > > -	int opt_c = -1;
> > > > -	struct string_list whitespace = STRING_LIST_INIT_NODUP;
> > > >     struct string_list exec = STRING_LIST_INIT_NODUP;
> > > >     const char *rebase_merges = NULL;
> > > >     int fork_point = -1;
> > > > @@ -804,15 +801,20 @@ int cmd_rebase(int argc, const char **argv, const
> > > > char *prefix)
> > > >      {OPTION_NEGBIT, 'n', "no-stat", &options.flags, NULL,
> > > >       N_("do not show diffstat of what changed upstream"),
> > > >       PARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },
> > > > -		OPT_BOOL(0, "ignore-whitespace", &ignore_whitespace,
> > > > -			 N_("passed to 'git apply'")),
> > > >      OPT_BOOL(0, "signoff", &options.signoff,
> > > >    			 N_("add a Signed-off-by: line to each
> > > > commit")),
> > > > -		OPT_BOOL(0, "committer-date-is-author-date",
> > > > -			 &committer_date_is_author_date,
> > > > -			 N_("passed to 'git am'")),
> > > > -		OPT_BOOL(0, "ignore-date", &ignore_date,
> > > > -			 N_("passed to 'git am'")),
> > > > +		OPT_PASSTHRU_ARGV(0, "ignore-whitespace",
> > > > &options.git_am_opts,
> > > > +				  NULL, N_("passed to 'git am'"),
> > > > +				  PARSE_OPT_NOARG),
> > > > +		OPT_PASSTHRU_ARGV(0, "committer-date-is-author-date",
> > > > +				  &options.git_am_opts, NULL,
> > > > +				  N_("passed to 'git am'"),
> > > > PARSE_OPT_NOARG),
> > > > +		OPT_PASSTHRU_ARGV(0, "ignore-date", &options.git_am_opts,
> > > > NULL,
> > > > +				  N_("passed to 'git am'"),
> > > > PARSE_OPT_NOARG),
> > > > +		OPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_("n"),
> > > > +				  N_("passed to 'git apply'"), 0),
> > > > +		OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts,
> > > > +				  N_("action"), N_("passed to 'git apply'"),
> > > > 0),
> > > >      OPT_BIT('f', "force-rebase", &options.flags,
> > > >       N_("cherry-pick all commits, even if unchanged"),
> > > >       REBASE_FORCE),
> > > > @@ -856,10 +858,6 @@ int cmd_rebase(int argc, const char **argv, const
> > > > char *prefix)
> > > >      { OPTION_STRING, 'S', "gpg-sign", &gpg_sign, N_("key-id"),
> > > >       N_("GPG-sign commits"),
> > > >       PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
> > > > -		OPT_STRING_LIST(0, "whitespace", &whitespace,
> > > > -				N_("whitespace"), N_("passed to 'git
> > > > apply'")),
> > > > -		OPT_SET_INT('C', NULL, &opt_c, N_("passed to 'git apply'"),
> > > > -			    REBASE_AM),
> > > >      OPT_BOOL(0, "autostash", &options.autostash,
> > > >        	 N_("automatically stash/stash pop before and after")),
> > > >    		OPT_STRING_LIST('x', "exec", &exec, N_("exec"),
> > > > @@ -884,6 +882,7 @@ int cmd_rebase(int argc, const char **argv, const
> > > > char
> > > > *prefix)
> > > >      	 N_("rebase all reachable commits up to the root(s)")),
> > > >     	OPT_END(),
> > > >    	};
> > > > +	int i;
> > > >    
> > > >     /*
> > > >    	 * NEEDSWORK: Once the builtin rebase has been tested enough
> > > > @@ -1064,22 +1063,17 @@ int cmd_rebase(int argc, const char **argv,
> > > > const
> > > > char *prefix)
> > > >     	    state_dir_base, cmd_live_rebase, buf.buf);
> > > >    }
> > > >    -	if (!(options.flags & REBASE_NO_QUIET))
> > > > -		strbuf_addstr(&options.git_am_opt, " -q");
> > > > -
> > > > -	if (committer_date_is_author_date) {
> > > > -		strbuf_addstr(&options.git_am_opt,
> > > > -			      " --committer-date-is-author-date");
> > > > -		options.flags |= REBASE_FORCE;
> > > > +	for (i = 0; i < options.git_am_opts.argc; i++) {
> > > > +		const char *option = options.git_am_opts.argv[i];
> > > > +		if (!strcmp(option, "--committer-date-is-author-date") ||
> > > > +		    !strcmp(option, "--ignore-date") ||
> > > > +		    !strcmp(option, "--whitespace=fix") ||
> > > > +		    !strcmp(option, "--whitespace=strip"))
> > > > +			options.flags |= REBASE_FORCE;
> > > >    }
> > > >    -	if (ignore_whitespace)
> > > > -		strbuf_addstr(&options.git_am_opt, " --ignore-whitespace");
> > > > -
> > > > -	if (ignore_date) {
> > > > -		strbuf_addstr(&options.git_am_opt, " --ignore-date");
> > > > -		options.flags |= REBASE_FORCE;
> > > > -	}
> > > > +	if (!(options.flags & REBASE_NO_QUIET))
> > > > +		argv_array_push(&options.git_am_opts, "-q");
> > > >    
> > > >     if (options.keep_empty)
> > > >    		imply_interactive(&options, "--keep-empty");
> > > > @@ -1089,23 +1083,6 @@ int cmd_rebase(int argc, const char **argv, const
> > > > char *prefix)
> > > >     	options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
> > > >    }
> > > >    -	if (opt_c >= 0)
> > > > -		strbuf_addf(&options.git_am_opt, " -C%d", opt_c);
> > > > -
> > > > -	if (whitespace.nr) {
> > > > -		int i;
> > > > -
> > > > -		for (i = 0; i < whitespace.nr; i++) {
> > > > -			const char *item = whitespace.items[i].string;
> > > > -
> > > > -			strbuf_addf(&options.git_am_opt, " --whitespace=%s",
> > > > -				    item);
> > > > -
> > > > -			if ((!strcmp(item, "fix")) || (!strcmp(item,
> > > > "strip")))
> > > > -				options.flags |= REBASE_FORCE;
> > > > -		}
> > > > -	}
> > > > -
> > > >     if (exec.nr) {
> > > >      int i;
> > > >    @@ -1181,23 +1158,18 @@ int cmd_rebase(int argc, const char **argv,
> > > > const char *prefix)
> > > >     	break;
> > > >    }
> > > >    -	if (options.git_am_opt.len) {
> > > > -		const char *p;
> > > > -
> > > > +	if (options.git_am_opts.argc) {
> > > >    		/* all am options except -q are compatible only with
> > > > --am */
> > > > -		strbuf_reset(&buf);
> > > > -		strbuf_addbuf(&buf, &options.git_am_opt);
> > > > -		strbuf_addch(&buf, ' ');
> > > > -		while ((p = strstr(buf.buf, " -q ")))
> > > > -			strbuf_splice(&buf, p - buf.buf, 4, " ", 1);
> > > > -		strbuf_trim(&buf);
> > > > +		for (i = options.git_am_opts.argc - 1; i >= 0; i--)
> > > > +			if (strcmp(options.git_am_opts.argv[i], "-q"))
> > > > +				break;
> > > >    -		if (is_interactive(&options) && buf.len)
> > > > +		if (is_interactive(&options) && i >= 0)
> > > >       die(_("error: cannot combine interactive options "
> > > >             "(--interactive, --exec, --rebase-merges, "
> > > >             "--preserve-merges, --keep-empty, --root + "
> > > >             "--onto) with am options (%s)"), buf.buf);
> > > > -		if (options.type == REBASE_MERGE && buf.len)
> > > > +		if (options.type == REBASE_MERGE && i >= 0)
> > > >       die(_("error: cannot combine merge options (--merge, "
> > > >             "--strategy, --strategy-option) with am options "
> > > >             "(%s)"), buf.buf);
> > > > @@ -1207,7 +1179,7 @@ int cmd_rebase(int argc, const char **argv, const
> > > > char *prefix)
> > > >      if (options.type == REBASE_PRESERVE_MERGES)
> > > >       die("cannot combine '--signoff' with "
> > > >    			    "'--preserve-merges'");
> > > > -		strbuf_addstr(&options.git_am_opt, " --signoff");
> > > > +		argv_array_push(&options.git_am_opts, "--signoff");
> > > >     	options.flags |= REBASE_FORCE;
> > > >     }
> > > >    
> > > >
> > >
> > >
> 
> 
> 

```

## Johannes Schindelin, 2018-11-14 14:28

Subject: Re: [PATCH 0/1] rebase: understand -C again, refactor
Message-ID: <nycvar.QRO.7.76.6.1811141525080.39@tvgsbejvaqbjf.bet>
URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1811141525080.39%40tvgsbejvaqbjf.bet
In-Reply-To: <20181114072909.GD19904@sigill.intra.peff.net>

```
Hi Peff,

On Wed, 14 Nov 2018, Jeff King wrote:

> On Tue, Nov 13, 2018 at 04:38:24AM -0800, Johannes Schindelin via GitGitGadget wrote:
> 
> > Phillip Wood reported a problem where the built-in rebase did not understand
> > options like -C1, i.e. it did not expect the option argument.
> > 
> > While investigating how to address this best, I stumbled upon 
> > OPT_PASSTHRU_ARGV (which I was so far happily unaware of).
> 
> I was unaware of it, too.

Thanks, that makes me feel better ;-)

> Looking at the OPT_PASSTHRU and its ARGV counterpart, I think the
> original intent was that you'd pass through normal last-one-wins
> individual options with OPT_PASSTHRU, and then list-like options with
> OPT_PASSTHRU_ARGV. But here you've used the latter to pass sets of
> individual last-one-wins options.
> 
> That said, I think what you've done here is way simpler and more
> readable than using a bunch of OPT_PASSTHRUs would have been. And even
> if it was not the original intent of the ARGV variant, I can't see any
> reason to avoid doing it this way.

Thank you, that makes me feel *even* better ;-)

Together with the extra-early error checking for incorrect -C and
--whitespace options, I think we're in a much better place now.

Ciao,
Dscho

```

## Johannes Schindelin via GitGitGadget, 2018-11-14 16:25

Subject: [PATCH v2 0/2] rebase: understand -C again, refactor
Message-ID: <pull.76.v2.git.gitgitgadget@gmail.com>
URL: https://gitlist.dev/e/pull.76.v2.git.gitgitgadget%40gmail.com
In-Reply-To: <pull.76.git.gitgitgadget@gmail.com>

```
Phillip Wood reported a problem where the built-in rebase did not understand
options like -C1, i.e. it did not expect the option argument.

While investigating how to address this best, I stumbled upon 
OPT_PASSTHRU_ARGV (which I was so far happily unaware of).

Instead of just fixing the -C<n> bug, I decided to simply convert all of the
options intended for git am (or, eventually, for git apply). This happens to
fix that bug, and does so much more: it simplifies the entire logic (and
removes more lines than it adds).

Change since v1:

 * Introduce early parameter validation for the options passed through to 
   git am.

Johannes Schindelin (2):
  rebase: really just passthru the `git am` options
  rebase: validate -C<n> and --whitespace=<mode> parameters early

 builtin/rebase.c          | 108 ++++++++++++++++----------------------
 t/t3406-rebase-message.sh |   7 +++
 2 files changed, 52 insertions(+), 63 deletions(-)


base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-76%2Fdscho%2Frebase-Cn-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-76/dscho/rebase-Cn-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/76

Range-diff vs v1:

 1:  dc36a45068 = 1:  dc36a45068 rebase: really just passthru the `git am` options
 -:  ---------- > 2:  4c2ba52766 rebase: validate -C<n> and --whitespace=<mode> parameters early

-- 
gitgitgadget

```

## Johannes Schindelin via GitGitGadget, 2018-11-14 16:25

Subject: [PATCH v2 1/2] rebase: really just passthru the `git am` options
Message-ID: <dc36a450680b1854abbb9bb06180001ce68c3f3b.1542212726.git.gitgitgadget@gmail.com>
URL: https://gitlist.dev/e/dc36a450680b1854abbb9bb06180001ce68c3f3b.1542212726.git.gitgitgadget%40gmail.com
In-Reply-To: <pull.76.v2.git.gitgitgadget@gmail.com>

```
From: Johannes Schindelin <johannes.schindelin@gmx.de>

Currently, we parse the options intended for `git am` as if we wanted to
handle them in `git rebase`, and then reconstruct them painstakingly to
define the `git_am_opt` variable.

However, there is a much better way (that I was unaware of, at the time
when I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.
It is intended for exactly this use case, where command-line options
want to be parsed into a separate `argv_array`.

Let's use this feature.

Incidentally, this also allows us to address a bug discovered by Phillip
Wood, where the built-in rebase failed to understand that the `-C`
option takes an optional argument.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/rebase.c | 98 +++++++++++++++++-------------------------------
 1 file changed, 35 insertions(+), 63 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ee06aa363..96ffa80b71 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -87,7 +87,7 @@ struct rebase_options {
 		REBASE_FORCE = 1<<3,
 		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
 	} flags;
-	struct strbuf git_am_opt;
+	struct argv_array git_am_opts;
 	const char *action;
 	int signoff;
 	int allow_rerere_autoupdate;
@@ -339,7 +339,7 @@ N_("Resolve all conflicts manually, mark them as resolved with\n"
 static int run_specific_rebase(struct rebase_options *opts)
 {
 	const char *argv[] = { NULL, NULL };
-	struct strbuf script_snippet = STRBUF_INIT;
+	struct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;
 	int status;
 	const char *backend, *backend_func;
 
@@ -433,7 +433,9 @@ static int run_specific_rebase(struct rebase_options *opts)
 		oid_to_hex(&opts->restrict_revision->object.oid) : NULL);
 	add_var(&script_snippet, "GIT_QUIET",
 		opts->flags & REBASE_NO_QUIET ? "" : "t");
-	add_var(&script_snippet, "git_am_opt", opts->git_am_opt.buf);
+	sq_quote_argv_pretty(&buf, opts->git_am_opts.argv);
+	add_var(&script_snippet, "git_am_opt", buf.buf);
+	strbuf_release(&buf);
 	add_var(&script_snippet, "verbose",
 		opts->flags & REBASE_VERBOSE ? "t" : "");
 	add_var(&script_snippet, "diffstat",
@@ -756,7 +758,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct rebase_options options = {
 		.type = REBASE_UNSPECIFIED,
 		.flags = REBASE_NO_QUIET,
-		.git_am_opt = STRBUF_INIT,
+		.git_am_opts = ARGV_ARRAY_INIT,
 		.allow_rerere_autoupdate  = -1,
 		.allow_empty_message = 1,
 		.git_format_patch_opt = STRBUF_INIT,
@@ -777,12 +779,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		ACTION_EDIT_TODO,
 		ACTION_SHOW_CURRENT_PATCH,
 	} action = NO_ACTION;
-	int committer_date_is_author_date = 0;
-	int ignore_date = 0;
-	int ignore_whitespace = 0;
 	const char *gpg_sign = NULL;
-	int opt_c = -1;
-	struct string_list whitespace = STRING_LIST_INIT_NODUP;
 	struct string_list exec = STRING_LIST_INIT_NODUP;
 	const char *rebase_merges = NULL;
 	int fork_point = -1;
@@ -804,15 +801,20 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		{OPTION_NEGBIT, 'n', "no-stat", &options.flags, NULL,
 			N_("do not show diffstat of what changed upstream"),
 			PARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },
-		OPT_BOOL(0, "ignore-whitespace", &ignore_whitespace,
-			 N_("passed to 'git apply'")),
 		OPT_BOOL(0, "signoff", &options.signoff,
 			 N_("add a Signed-off-by: line to each commit")),
-		OPT_BOOL(0, "committer-date-is-author-date",
-			 &committer_date_is_author_date,
-			 N_("passed to 'git am'")),
-		OPT_BOOL(0, "ignore-date", &ignore_date,
-			 N_("passed to 'git am'")),
+		OPT_PASSTHRU_ARGV(0, "ignore-whitespace", &options.git_am_opts,
+				  NULL, N_("passed to 'git am'"),
+				  PARSE_OPT_NOARG),
+		OPT_PASSTHRU_ARGV(0, "committer-date-is-author-date",
+				  &options.git_am_opts, NULL,
+				  N_("passed to 'git am'"), PARSE_OPT_NOARG),
+		OPT_PASSTHRU_ARGV(0, "ignore-date", &options.git_am_opts, NULL,
+				  N_("passed to 'git am'"), PARSE_OPT_NOARG),
+		OPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_("n"),
+				  N_("passed to 'git apply'"), 0),
+		OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts,
+				  N_("action"), N_("passed to 'git apply'"), 0),
 		OPT_BIT('f', "force-rebase", &options.flags,
 			N_("cherry-pick all commits, even if unchanged"),
 			REBASE_FORCE),
@@ -856,10 +858,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		{ OPTION_STRING, 'S', "gpg-sign", &gpg_sign, N_("key-id"),
 			N_("GPG-sign commits"),
 			PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
-		OPT_STRING_LIST(0, "whitespace", &whitespace,
-				N_("whitespace"), N_("passed to 'git apply'")),
-		OPT_SET_INT('C', NULL, &opt_c, N_("passed to 'git apply'"),
-			    REBASE_AM),
 		OPT_BOOL(0, "autostash", &options.autostash,
 			 N_("automatically stash/stash pop before and after")),
 		OPT_STRING_LIST('x', "exec", &exec, N_("exec"),
@@ -884,6 +882,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			 N_("rebase all reachable commits up to the root(s)")),
 		OPT_END(),
 	};
+	int i;
 
 	/*
 	 * NEEDSWORK: Once the builtin rebase has been tested enough
@@ -1064,22 +1063,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		    state_dir_base, cmd_live_rebase, buf.buf);
 	}
 
-	if (!(options.flags & REBASE_NO_QUIET))
-		strbuf_addstr(&options.git_am_opt, " -q");
-
-	if (committer_date_is_author_date) {
-		strbuf_addstr(&options.git_am_opt,
-			      " --committer-date-is-author-date");
-		options.flags |= REBASE_FORCE;
+	for (i = 0; i < options.git_am_opts.argc; i++) {
+		const char *option = options.git_am_opts.argv[i];
+		if (!strcmp(option, "--committer-date-is-author-date") ||
+		    !strcmp(option, "--ignore-date") ||
+		    !strcmp(option, "--whitespace=fix") ||
+		    !strcmp(option, "--whitespace=strip"))
+			options.flags |= REBASE_FORCE;
 	}
 
-	if (ignore_whitespace)
-		strbuf_addstr(&options.git_am_opt, " --ignore-whitespace");
-
-	if (ignore_date) {
-		strbuf_addstr(&options.git_am_opt, " --ignore-date");
-		options.flags |= REBASE_FORCE;
-	}
+	if (!(options.flags & REBASE_NO_QUIET))
+		argv_array_push(&options.git_am_opts, "-q");
 
 	if (options.keep_empty)
 		imply_interactive(&options, "--keep-empty");
@@ -1089,23 +1083,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
 	}
 
-	if (opt_c >= 0)
-		strbuf_addf(&options.git_am_opt, " -C%d", opt_c);
-
-	if (whitespace.nr) {
-		int i;
-
-		for (i = 0; i < whitespace.nr; i++) {
-			const char *item = whitespace.items[i].string;
-
-			strbuf_addf(&options.git_am_opt, " --whitespace=%s",
-				    item);
-
-			if ((!strcmp(item, "fix")) || (!strcmp(item, "strip")))
-				options.flags |= REBASE_FORCE;
-		}
-	}
-
 	if (exec.nr) {
 		int i;
 
@@ -1181,23 +1158,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		break;
 	}
 
-	if (options.git_am_opt.len) {
-		const char *p;
-
+	if (options.git_am_opts.argc) {
 		/* all am options except -q are compatible only with --am */
-		strbuf_reset(&buf);
-		strbuf_addbuf(&buf, &options.git_am_opt);
-		strbuf_addch(&buf, ' ');
-		while ((p = strstr(buf.buf, " -q ")))
-			strbuf_splice(&buf, p - buf.buf, 4, " ", 1);
-		strbuf_trim(&buf);
+		for (i = options.git_am_opts.argc - 1; i >= 0; i--)
+			if (strcmp(options.git_am_opts.argv[i], "-q"))
+				break;
 
-		if (is_interactive(&options) && buf.len)
+		if (is_interactive(&options) && i >= 0)
 			die(_("error: cannot combine interactive options "
 			      "(--interactive, --exec, --rebase-merges, "
 			      "--preserve-merges, --keep-empty, --root + "
 			      "--onto) with am options (%s)"), buf.buf);
-		if (options.type == REBASE_MERGE && buf.len)
+		if (options.type == REBASE_MERGE && i >= 0)
 			die(_("error: cannot combine merge options (--merge, "
 			      "--strategy, --strategy-option) with am options "
 			      "(%s)"), buf.buf);
@@ -1207,7 +1179,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		if (options.type == REBASE_PRESERVE_MERGES)
 			die("cannot combine '--signoff' with "
 			    "'--preserve-merges'");
-		strbuf_addstr(&options.git_am_opt, " --signoff");
+		argv_array_push(&options.git_am_opts, "--signoff");
 		options.flags |= REBASE_FORCE;
 	}
 
-- 
gitgitgadget


```

## Johannes Schindelin via GitGitGadget, 2018-11-14 16:25

Subject: [PATCH v2 2/2] rebase: validate -C<n> and --whitespace=<mode> parameters early
Message-ID: <4c2ba5276636097a1c3bb2207c7537831e9079bc.1542212726.git.gitgitgadget@gmail.com>
URL: https://gitlist.dev/e/4c2ba5276636097a1c3bb2207c7537831e9079bc.1542212726.git.gitgitgadget%40gmail.com
In-Reply-To: <pull.76.v2.git.gitgitgadget@gmail.com>

```
From: Johannes Schindelin <johannes.schindelin@gmx.de>

It is a good idea to error out early upon seeing, say, `-Cbad`, rather
than starting the rebase only to have the `--am` backend complain later.

Let's do this.

The only options accepting parameters which we pass through to `git am`
(which may, or may not, forward them to `git apply`) are `-C` and
`--whitespace`. The other options we pass through do not accept
parameters, so we do not have to validate them here.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/rebase.c          | 12 +++++++++++-
 t/t3406-rebase-message.sh |  7 +++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 96ffa80b71..571cf899d5 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 
 	for (i = 0; i < options.git_am_opts.argc; i++) {
-		const char *option = options.git_am_opts.argv[i];
+		const char *option = options.git_am_opts.argv[i], *p;
 		if (!strcmp(option, "--committer-date-is-author-date") ||
 		    !strcmp(option, "--ignore-date") ||
 		    !strcmp(option, "--whitespace=fix") ||
 		    !strcmp(option, "--whitespace=strip"))
 			options.flags |= REBASE_FORCE;
+		else if (skip_prefix(option, "-C", &p)) {
+			while (*p)
+				if (!isdigit(*(p++)))
+					die(_("switch `C' expects a "
+					      "numerical value"));
+		} else if (skip_prefix(option, "--whitespace=", &p)) {
+			if (*p && strcmp(p, "warn") && strcmp(p, "nowarn") &&
+			    strcmp(p, "error") && strcmp(p, "error-all"))
+				die("Invalid whitespace option: '%s'", p);
+		}
 	}
 
 	if (!(options.flags & REBASE_NO_QUIET))
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 0392e36d23..2c79eed4fe 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the invalid ref' '
 	test_i18ngrep "invalid-ref" err
 '
 
+test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
+	test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
+	test_i18ngrep "numerical value" err &&
+	test_must_fail git rebase --whitespace=bad HEAD 2>err &&
+	test_i18ngrep "Invalid whitespace option" err
+'
+
 test_done
-- 
gitgitgadget

```

## Phillip Wood, 2018-11-14 16:37

Subject: Re: [PATCH v2 2/2] rebase: validate -C<n> and --whitespace=<mode> parameters early
Message-ID: <ce8e2f7c-cded-0c9d-1306-330711a29021@talktalk.net>
URL: https://gitlist.dev/e/ce8e2f7c-cded-0c9d-1306-330711a29021%40talktalk.net
In-Reply-To: <4c2ba5276636097a1c3bb2207c7537831e9079bc.1542212726.git.gitgitgadget@gmail.com>

```
Hi Johannes

Thanks for doing this, I think this patch is good. I've not checked the 
first patch as I think it is the same as before judging from the 
covering letter.

Best Wishes

Phillip

On 14/11/2018 16:25, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> It is a good idea to error out early upon seeing, say, `-Cbad`, rather
> than starting the rebase only to have the `--am` backend complain later.
> 
> Let's do this.
> 
> The only options accepting parameters which we pass through to `git am`
> (which may, or may not, forward them to `git apply`) are `-C` and
> `--whitespace`. The other options we pass through do not accept
> parameters, so we do not have to validate them here.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   builtin/rebase.c          | 12 +++++++++++-
>   t/t3406-rebase-message.sh |  7 +++++++
>   2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 96ffa80b71..571cf899d5 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	}
>   
>   	for (i = 0; i < options.git_am_opts.argc; i++) {
> -		const char *option = options.git_am_opts.argv[i];
> +		const char *option = options.git_am_opts.argv[i], *p;
>   		if (!strcmp(option, "--committer-date-is-author-date") ||
>   		    !strcmp(option, "--ignore-date") ||
>   		    !strcmp(option, "--whitespace=fix") ||
>   		    !strcmp(option, "--whitespace=strip"))
>   			options.flags |= REBASE_FORCE;
> +		else if (skip_prefix(option, "-C", &p)) {
> +			while (*p)
> +				if (!isdigit(*(p++)))
> +					die(_("switch `C' expects a "
> +					      "numerical value"));
> +		} else if (skip_prefix(option, "--whitespace=", &p)) {
> +			if (*p && strcmp(p, "warn") && strcmp(p, "nowarn") &&
> +			    strcmp(p, "error") && strcmp(p, "error-all"))
> +				die("Invalid whitespace option: '%s'", p);
> +		}
>   	}
>   
>   	if (!(options.flags & REBASE_NO_QUIET))
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index 0392e36d23..2c79eed4fe 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the invalid ref' '
>   	test_i18ngrep "invalid-ref" err
>   '
>   
> +test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
> +	test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
> +	test_i18ngrep "numerical value" err &&
> +	test_must_fail git rebase --whitespace=bad HEAD 2>err &&
> +	test_i18ngrep "Invalid whitespace option" err
> +'
> +
>   test_done
> 


```

## Johannes Schindelin, 2018-11-14 21:24

Subject: Re: [PATCH v2 2/2] rebase: validate -C<n> and --whitespace=<mode> parameters early
Message-ID: <nycvar.QRO.7.76.6.1811142224110.39@tvgsbejvaqbjf.bet>
URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1811142224110.39%40tvgsbejvaqbjf.bet
In-Reply-To: <ce8e2f7c-cded-0c9d-1306-330711a29021@talktalk.net>

```
Hi Phillip,

On Wed, 14 Nov 2018, Phillip Wood wrote:

> Thanks for doing this, I think this patch is good.

Thanks!

> I've not checked the first patch as I think it is the same as before
> judging from the covering letter.

Indeed, that's what the range-diff said. Sorry for not stating this
explicitly.

Thank you for your review,
Dscho

> 
> Best Wishes
> 
> Phillip
> 
> On 14/11/2018 16:25, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > 
> > It is a good idea to error out early upon seeing, say, `-Cbad`, rather
> > than starting the rebase only to have the `--am` backend complain later.
> > 
> > Let's do this.
> > 
> > The only options accepting parameters which we pass through to `git am`
> > (which may, or may not, forward them to `git apply`) are `-C` and
> > `--whitespace`. The other options we pass through do not accept
> > parameters, so we do not have to validate them here.
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >   builtin/rebase.c          | 12 +++++++++++-
> >   t/t3406-rebase-message.sh |  7 +++++++
> >   2 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 96ffa80b71..571cf899d5 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv,
> > const char *prefix)
> >    }
> >   
> >   	for (i = 0; i < options.git_am_opts.argc; i++) {
> > -		const char *option = options.git_am_opts.argv[i];
> > +		const char *option = options.git_am_opts.argv[i], *p;
> >     if (!strcmp(option, "--committer-date-is-author-date") ||
> > !strcmp(option, "--ignore-date") ||
> > !strcmp(option, "--whitespace=fix") ||
> > !strcmp(option, "--whitespace=strip"))
> >   			options.flags |= REBASE_FORCE;
> > +		else if (skip_prefix(option, "-C", &p)) {
> > +			while (*p)
> > +				if (!isdigit(*(p++)))
> > +					die(_("switch `C' expects a "
> > +					      "numerical value"));
> > +		} else if (skip_prefix(option, "--whitespace=", &p)) {
> > +			if (*p && strcmp(p, "warn") && strcmp(p, "nowarn")
> > &&
> > +			    strcmp(p, "error") && strcmp(p, "error-all"))
> > +				die("Invalid whitespace option: '%s'", p);
> > +		}
> >    }
> >   
> >   	if (!(options.flags & REBASE_NO_QUIET))
> > diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> > index 0392e36d23..2c79eed4fe 100755
> > --- a/t/t3406-rebase-message.sh
> > +++ b/t/t3406-rebase-message.sh
> > @@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the
> > invalid ref' '
> >   	test_i18ngrep "invalid-ref" err
> >   '
> >   
> > +test_expect_success 'error out early upon -C<n> or --whitespace=<bad>'
> > '
> > +	test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
> > +	test_i18ngrep "numerical value" err &&
> > +	test_must_fail git rebase --whitespace=bad HEAD 2>err &&
> > +	test_i18ngrep "Invalid whitespace option" err
> > +'
> > +
> >   test_done
> > 
> 
> 

```

## Ævar Arnfjörð Bjarmason, 2018-11-19 12:38

Subject: Re: [PATCH v2 2/2] rebase: validate -C<n> and --whitespace=<mode> parameters early
Message-ID: <874lcd1bub.fsf@evledraar.gmail.com>
URL: https://gitlist.dev/e/874lcd1bub.fsf%40evledraar.gmail.com
In-Reply-To: <4c2ba5276636097a1c3bb2207c7537831e9079bc.1542212726.git.gitgitgadget@gmail.com>

```

On Wed, Nov 14 2018, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> It is a good idea to error out early upon seeing, say, `-Cbad`, rather
> than starting the rebase only to have the `--am` backend complain later.
>
> Let's do this.
>
> The only options accepting parameters which we pass through to `git am`
> (which may, or may not, forward them to `git apply`) are `-C` and
> `--whitespace`. The other options we pass through do not accept
> parameters, so we do not have to validate them here.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/rebase.c          | 12 +++++++++++-
>  t/t3406-rebase-message.sh |  7 +++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 96ffa80b71..571cf899d5 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	}
>
>  	for (i = 0; i < options.git_am_opts.argc; i++) {
> -		const char *option = options.git_am_opts.argv[i];
> +		const char *option = options.git_am_opts.argv[i], *p;
>  		if (!strcmp(option, "--committer-date-is-author-date") ||
>  		    !strcmp(option, "--ignore-date") ||
>  		    !strcmp(option, "--whitespace=fix") ||
>  		    !strcmp(option, "--whitespace=strip"))
>  			options.flags |= REBASE_FORCE;
> +		else if (skip_prefix(option, "-C", &p)) {
> +			while (*p)
> +				if (!isdigit(*(p++)))
> +					die(_("switch `C' expects a "
> +					      "numerical value"));
> +		} else if (skip_prefix(option, "--whitespace=", &p)) {
> +			if (*p && strcmp(p, "warn") && strcmp(p, "nowarn") &&
> +			    strcmp(p, "error") && strcmp(p, "error-all"))
> +				die("Invalid whitespace option: '%s'", p);
> +		}
>  	}
>
>  	if (!(options.flags & REBASE_NO_QUIET))
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index 0392e36d23..2c79eed4fe 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the invalid ref' '
>  	test_i18ngrep "invalid-ref" err
>  '
>
> +test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
> +	test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
> +	test_i18ngrep "numerical value" err &&
> +	test_must_fail git rebase --whitespace=bad HEAD 2>err &&
> +	test_i18ngrep "Invalid whitespace option" err
> +'
> +

The combination of this gitster/js/rebase-am-options and my
gitster/ab/rebase-in-c-escape-hatch breaks tests under
GIT_TEST_REBASE_USE_BUILTIN=false for obvious reasons. The C version is
now more strict.

```

## Johannes Schindelin, 2018-11-20 10:58

Subject: Re: [PATCH v2 2/2] rebase: validate -C<n> and --whitespace=<mode> parameters early
Message-ID: <nycvar.QRO.7.76.6.1811201157170.41@tvgsbejvaqbjf.bet>
URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1811201157170.41%40tvgsbejvaqbjf.bet
In-Reply-To: <874lcd1bub.fsf@evledraar.gmail.com>

```
Hi Ævar,

On Mon, 19 Nov 2018, Ævar Arnfjörð Bjarmason wrote:

> 
> On Wed, Nov 14 2018, Johannes Schindelin via GitGitGadget wrote:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > It is a good idea to error out early upon seeing, say, `-Cbad`, rather
> > than starting the rebase only to have the `--am` backend complain later.
> >
> > Let's do this.
> >
> > The only options accepting parameters which we pass through to `git am`
> > (which may, or may not, forward them to `git apply`) are `-C` and
> > `--whitespace`. The other options we pass through do not accept
> > parameters, so we do not have to validate them here.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/rebase.c          | 12 +++++++++++-
> >  t/t3406-rebase-message.sh |  7 +++++++
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 96ffa80b71..571cf899d5 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >  	}
> >
> >  	for (i = 0; i < options.git_am_opts.argc; i++) {
> > -		const char *option = options.git_am_opts.argv[i];
> > +		const char *option = options.git_am_opts.argv[i], *p;
> >  		if (!strcmp(option, "--committer-date-is-author-date") ||
> >  		    !strcmp(option, "--ignore-date") ||
> >  		    !strcmp(option, "--whitespace=fix") ||
> >  		    !strcmp(option, "--whitespace=strip"))
> >  			options.flags |= REBASE_FORCE;
> > +		else if (skip_prefix(option, "-C", &p)) {
> > +			while (*p)
> > +				if (!isdigit(*(p++)))
> > +					die(_("switch `C' expects a "
> > +					      "numerical value"));
> > +		} else if (skip_prefix(option, "--whitespace=", &p)) {
> > +			if (*p && strcmp(p, "warn") && strcmp(p, "nowarn") &&
> > +			    strcmp(p, "error") && strcmp(p, "error-all"))
> > +				die("Invalid whitespace option: '%s'", p);
> > +		}
> >  	}
> >
> >  	if (!(options.flags & REBASE_NO_QUIET))
> > diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> > index 0392e36d23..2c79eed4fe 100755
> > --- a/t/t3406-rebase-message.sh
> > +++ b/t/t3406-rebase-message.sh
> > @@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the invalid ref' '
> >  	test_i18ngrep "invalid-ref" err
> >  '
> >
> > +test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
> > +	test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
> > +	test_i18ngrep "numerical value" err &&
> > +	test_must_fail git rebase --whitespace=bad HEAD 2>err &&
> > +	test_i18ngrep "Invalid whitespace option" err
> > +'
> > +
> 
> The combination of this gitster/js/rebase-am-options and my
> gitster/ab/rebase-in-c-escape-hatch breaks tests under
> GIT_TEST_REBASE_USE_BUILTIN=false for obvious reasons. The C version is
> now more strict.

Maybe you can concoct a prereq for this test? Something like

test_lazy_prereq BUILTIN_REBASE '
	test true = "${GIT_TEST_REBASE_USE_BUILTIN:-true}"
'

Ciao,
Dscho
```

## Ævar Arnfjörð Bjarmason, 2018-11-20 11:42

Subject: [PATCH] rebase: mark a test as failing with rebase.useBuiltin=false
Message-ID: <20181120114208.14251-1-avarab@gmail.com>
URL: https://gitlist.dev/e/20181120114208.14251-1-avarab%40gmail.com
In-Reply-To: <nycvar.QRO.7.76.6.1811201157170.41@tvgsbejvaqbjf.bet>

```
Mark a test added in 04519d7201 ("rebase: validate -C<n> and
--whitespace=<mode> parameters early", 2018-11-14) as only succeeding
with the builtin version of rebase. It would be nice if the
shellscript version had the same fix, but it's on its way out, and the
author is not interested in fixing it[1].

This makes the entire test suite pass again with the
GIT_TEST_REBASE_USE_BUILTIN=false mode added in my 62c23938fa ("tests:
add a special setup where rebase.useBuiltin is off", 2018-11-14).

1. https://public-inbox.org/git/nycvar.QRO.7.76.6.1811201157170.41@tvgsbejvaqbjf.bet/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Tue, Nov 20 2018, Johannes Schindelin wrote:

> [...]
> Maybe you can concoct a prereq for this test? Something like
>
> test_lazy_prereq BUILTIN_REBASE '
>       test true = "${GIT_TEST_REBASE_USE_BUILTIN:-true}"
> '

It's better to just mark the test as needing the prereq turned
off. E.g. this is what we do for the split index tests & now for the
gettext tests. That way we always run the test, but just indicate that
it relies on GIT_TEST_REBASE_USE_BUILTIN being unset.

 t/t3406-rebase-message.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 38bd876cab..77e5bbb3d5 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -84,7 +84,8 @@ test_expect_success 'rebase --onto outputs the invalid ref' '
 	test_i18ngrep "invalid-ref" err
 '
 
-test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
+test_expect_success 'builtin rebase: error out early upon -C<n> or --whitespace=<bad>' '
+	sane_unset GIT_TEST_REBASE_USE_BUILTIN &&
 	test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
 	test_i18ngrep "numerical value" err &&
 	test_must_fail git rebase --whitespace=bad HEAD 2>err &&
-- 
2.20.0.rc0.387.gc7a69e6b6c


```

## Johannes Schindelin, 2018-11-20 19:55

Subject: Re: [PATCH] rebase: mark a test as failing with rebase.useBuiltin=false
Message-ID: <nycvar.QRO.7.76.6.1811202029110.41@tvgsbejvaqbjf.bet>
URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1811202029110.41%40tvgsbejvaqbjf.bet
In-Reply-To: <20181120114208.14251-1-avarab@gmail.com>

```
Hi Ævar,

On Tue, 20 Nov 2018, Ævar Arnfjörð Bjarmason wrote:

> Mark a test added in 04519d7201 ("rebase: validate -C<n> and
> --whitespace=<mode> parameters early", 2018-11-14) as only succeeding
> with the builtin version of rebase. It would be nice if the
> shellscript version had the same fix, but it's on its way out, and the
> author is not interested in fixing it[1].

It's not that I am not interested in fixing it. It's more like I hoped
that I could work on Git for Windows v2.20.0-rc0 and rely on you to fix
this bug.

Now that Git for Windows v2.20.0-rc0 is out (see
https://github.com/git-for-windows/git/releases/tag/v2.20.0-rc0.windows.1),
expect a patch soon (see https://github.com/gitgitgadget/git/pull/86).

Ciao,
Dscho

> 
> This makes the entire test suite pass again with the
> GIT_TEST_REBASE_USE_BUILTIN=false mode added in my 62c23938fa ("tests:
> add a special setup where rebase.useBuiltin is off", 2018-11-14).
> 
> 1. https://public-inbox.org/git/nycvar.QRO.7.76.6.1811201157170.41@tvgsbejvaqbjf.bet/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> On Tue, Nov 20 2018, Johannes Schindelin wrote:
> 
> > [...]
> > Maybe you can concoct a prereq for this test? Something like
> >
> > test_lazy_prereq BUILTIN_REBASE '
> >       test true = "${GIT_TEST_REBASE_USE_BUILTIN:-true}"
> > '
> 
> It's better to just mark the test as needing the prereq turned
> off. E.g. this is what we do for the split index tests & now for the
> gettext tests. That way we always run the test, but just indicate that
> it relies on GIT_TEST_REBASE_USE_BUILTIN being unset.
> 
>  t/t3406-rebase-message.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index 38bd876cab..77e5bbb3d5 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -84,7 +84,8 @@ test_expect_success 'rebase --onto outputs the invalid ref' '
>  	test_i18ngrep "invalid-ref" err
>  '
>  
> -test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
> +test_expect_success 'builtin rebase: error out early upon -C<n> or --whitespace=<bad>' '
> +	sane_unset GIT_TEST_REBASE_USE_BUILTIN &&
>  	test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
>  	test_i18ngrep "numerical value" err &&
>  	test_must_fail git rebase --whitespace=bad HEAD 2>err &&
> -- 
> 2.20.0.rc0.387.gc7a69e6b6c
> 
> 
```