[1/1] rebase: really just passthru the `git am` options

Commit Message

Linus Arver via GitGitGadget Nov. 13, 2018, 12:38 p.m. UTC

From: Johannes Schindelin <[e-mail]>

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 <[e-mail]>
---
 builtin/rebase.c | 98 +++++++++++++++++-------------------------------
 1 file changed, 35 insertions(+), 63 deletions(-)

Comments

Junio C Hamano Nov. 13, 2018, 1:05 p.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
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 Nov. 13, 2018, 3:05 p.m. UTC | #2

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 <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>   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 Nov. 13, 2018, 7:21 p.m. UTC | #3

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 <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> >   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 Nov. 13, 2018, 7:58 p.m. UTC | #4

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 <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> ---
>>>    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 Nov. 13, 2018, 9:50 p.m. UTC | #5

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 Nov. 14, 2018, 12:07 a.m. UTC | #6

> 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/[e-mail]/
   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 Nov. 14, 2018, 12:36 a.m. UTC | #7

On Tue, Nov 13, 2018 at 1:52 PM Ævar Arnfjörð Bjarmason
<[e-mail]> 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 Nov. 14, 2018, 3:39 a.m. UTC | #8

Ævar Arnfjörð Bjarmason <[e-mail]> 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.

Johannes Schindelin Nov. 14, 2018, 2:22 p.m. UTC | #9

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 <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > ---
> > > >    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 Nov. 24, 2018, 8:54 p.m. UTC | #10

On Wed, Nov 21 2018, Junio C Hamano wrote:

>  * "git rebase" and "git rebase -i" have been reimplemented in C.

Here's another regression in the C version (and rc1), note: the
sha1collisiondetection is just a stand in for "some repo":

    (
        rm -rf /tmp/repo &&
        git init /tmp/repo &&
        cd /tmp/repo &&
        for c in 1 2
        do
            touch $c &&
            git add $c &&
            git commit -m"add $c"
        done &&
        git remote add origin https://github.com/cr-marcstevens/sha1collisiondetection.git &&
        git fetch &&
        git branch --set-upstream-to origin/master &&
        git rebase -i
    )

The C version will die with "fatal: unable to read tree
0000000000000000000000000000000000000000". Running this with
rebase.useBuiltin=false does the right thing and rebases as of the merge
base of the two (which here is the root of the history).

I wasn't trying to stress test rebase. I was just wanting to rebase a
history I was about to force-push after cleaning it up, hardly an
obscure use-case. So [repeat last transmission in
https://public-inbox.org/git/[e-mail]/ ]

Junio C Hamano Nov. 25, 2018, 1 a.m. UTC | #11

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

>>  * "git rebase" and "git rebase -i" have been reimplemented in C.
>
> Here's another regression in the C version (and rc1),...
> I wasn't trying to stress test rebase. I was just wanting to rebase a
> history I was about to force-push after cleaning it up, hardly an
> obscure use-case. So [repeat last transmission in
> https://public-inbox.org/git/[e-mail]/ ]

which, to those who are reading from sidelines:

    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).

I am fine with the proposed flip, but I'll have to see the extent of
damage this late in the game so that I won't miss anything.  In
addition to the one-liner below, we'd need to update the quoted
release notes entry, and possibly adjust some tests (even though the
"reimplementation" ought to be bug-to-bug compatible, it may not be).

diff --git b/builtin/rebase.c a/builtin/rebase.c
index 9dc8475cd3..60e357c735 100644
--- b/builtin/rebase.c
+++ a/builtin/rebase.c
@@ -54,7 +54,7 @@ static int use_builtin_rebase(void)
 	cp.git_cmd = 1;
 	if (capture_command(&cp, &out, 6)) {
 		strbuf_release(&out);
-		return 1;
+		return 0;
 	}
 
 	strbuf_trim(&out);

Johannes Schindelin Nov. 26, 2018, 10:52 p.m. UTC | #12

Hi Ævar,

On Sat, 24 Nov 2018, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Nov 21 2018, Junio C Hamano wrote:
> 
> >  * "git rebase" and "git rebase -i" have been reimplemented in C.
> 
> Here's another regression in the C version (and rc1), note: the
> sha1collisiondetection is just a stand in for "some repo":
> 
>     (
>         rm -rf /tmp/repo &&
>         git init /tmp/repo &&
>         cd /tmp/repo &&
>         for c in 1 2
>         do
>             touch $c &&
>             git add $c &&
>             git commit -m"add $c"
>         done &&
>         git remote add origin https://github.com/cr-marcstevens/sha1collisiondetection.git &&
>         git fetch &&
>         git branch --set-upstream-to origin/master &&
>         git rebase -i
>     )
> 
> The C version will die with "fatal: unable to read tree
> 0000000000000000000000000000000000000000". Running this with
> rebase.useBuiltin=false does the right thing and rebases as of the merge
> base of the two (which here is the root of the history).

Sorry, this bug does not reproduce here:

$ git rebase -i
Successfully rebased and updated refs/heads/master.

> I wasn't trying to stress test rebase. I was just wanting to rebase a
> history I was about to force-push after cleaning it up, hardly an
> obscure use-case. So [repeat last transmission in
> https://public-inbox.org/git/[e-mail]/ ]

Maybe you can give me the full details so that I can verify that this is
indeed a bug in the builtin C and not just a regression caused by some
random branches being merged together?

In short: please provide me with the exact URL and branch of your git.git
fork to test. Then please make sure to specify the precise revision of the
sha1collisiondetection/master rev, just in case that it matters.

Ideally, you would reduce the problem to a proper test case, say, for
t3412 (it seems that you try to rebase onto an unrelated history, so it is
*vaguely* related to "rebase-root").

Ciao,
Dscho

Johannes Schindelin Nov. 26, 2018, 11:47 p.m. UTC | #13

Hi Ævar,

On Mon, 26 Nov 2018, Johannes Schindelin wrote:

> On Sat, 24 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> 
> > On Wed, Nov 21 2018, Junio C Hamano wrote:
> > 
> > >  * "git rebase" and "git rebase -i" have been reimplemented in C.
> > 
> > Here's another regression in the C version (and rc1), note: the
> > sha1collisiondetection is just a stand in for "some repo":
> > 
> >     (
> >         rm -rf /tmp/repo &&
> >         git init /tmp/repo &&
> >         cd /tmp/repo &&
> >         for c in 1 2
> >         do
> >             touch $c &&
> >             git add $c &&
> >             git commit -m"add $c"
> >         done &&
> >         git remote add origin https://github.com/cr-marcstevens/sha1collisiondetection.git &&
> >         git fetch &&
> >         git branch --set-upstream-to origin/master &&
> >         git rebase -i
> >     )
> > 
> > The C version will die with "fatal: unable to read tree
> > 0000000000000000000000000000000000000000". Running this with
> > rebase.useBuiltin=false does the right thing and rebases as of the merge
> > base of the two (which here is the root of the history).
> 
> Sorry, this bug does not reproduce here:
> 
> $ git rebase -i
> Successfully rebased and updated refs/heads/master.
> 
> > I wasn't trying to stress test rebase. I was just wanting to rebase a
> > history I was about to force-push after cleaning it up, hardly an
> > obscure use-case. So [repeat last transmission in
> > https://public-inbox.org/git/[e-mail]/ ]
> 
> Maybe you can give me the full details so that I can verify that this is
> indeed a bug in the builtin C and not just a regression caused by some
> random branches being merged together?
> 
> In short: please provide me with the exact URL and branch of your git.git
> fork to test. Then please make sure to specify the precise revision of the
> sha1collisiondetection/master rev, just in case that it matters.
> 
> Ideally, you would reduce the problem to a proper test case, say, for
> t3412 (it seems that you try to rebase onto an unrelated history, so it is
> *vaguely* related to "rebase-root").

So I was getting spooked enough by your half-complete bug report that I
did more digging (it is really quite a bit frustrating to have so little
hard evidence to go on, a wild goose chase is definitely not what I was
looking forward to after a day of fighting other fires, but you know,
built-in rebase is dear to me).

The error message you copied clearly comes from tree-walk.c, from
`fill_tree_descriptor()` (the other "unable to read tree" messages enclose
the hash in parentheses).

There are exactly 3 calls to said function in the built-in rebase/rebase
-i in the current `master`, a1598010f775 (Merge branch
'nd/per-worktree-ref-iteration', 2018-11-26):

$ git grep fill_tree_descriptor -- builtin/rebase*.c sequencer.[ch] rebase-interactive.[ch]
builtin/rebase.c:       if (!reset_hard && !fill_tree_descriptor(&desc[nr++], &head_oid)) {
builtin/rebase.c:       if (!fill_tree_descriptor(&desc[nr++], oid)) {
sequencer.c:    if (!fill_tree_descriptor(&desc, &oid)) {

The last one of these is in `do_reset()`, i.e. handling a `reset` command
which you did not ask for, as you passed `-i` to `git rebase`, not `-ir`.

The first two *both* are in `reset_head()`. The first of them uses
`head_oid`, which is read directly via `get_oid("HEAD", &head_oid)`, so if
this is all zeroes for you, then it's not rebase's fault.

The second one uses the parameter `oid` passed into `reset_head()`. The
only calls to that function that do not pass `NULL` as `oid` (which would
trigger `oid` to be replaced by `&head_oid`, i.e again not all zeroes
unless your setup is broken) are:

- in the `--abort` code path
- in the `--autostash` code path
- in the fast-forwarding code path
- just after the "First, rewinding head" message in the *non*-interactive
  rebase

None of these apply to your script snippet.

Under the assumption that you might have forgotten to talk about
rebase.autostash=true and some dirty file, I tried to augment the script
snippet accordingly, but the built-in rebase as of current `master` still
works for me, plus: reading the autostash code path, it is hard to imagine
that the `lookup_commit_reference()` would return a pointer to a commit
object whose oid is all zeroes.

In short, even a thorough study of the code (keeping in mind the few
tidbits of information provided by you) leaves me really wondering which
code you run, because it sure does not look like current `master` to me.

And if it is not `master`, then I have to ask why you keep suggesting to
turn off the built-in rebase by default in `master`.

Ciao,
Johannes

P.S.: Maybe you have a hook you forgot to mention?

Junio C Hamano Nov. 28, 2018, 4:07 a.m. UTC | #14

Johannes Schindelin <[e-mail]> writes:

> ...
> In short, even a thorough study of the code (keeping in mind the few
> tidbits of information provided by you) leaves me really wondering which
> code you run, because it sure does not look like current `master` to me.
>
> And if it is not `master`, then I have to ask why you keep suggesting to
> turn off the built-in rebase by default in `master`.
>
> Ciao,
> Johannes
>
> P.S.: Maybe you have a hook you forgot to mention?

Any response?  Or can I retract jc/postpone-rebase-in-c that was
prepared as a reaction to this?

Johannes Schindelin Nov. 28, 2018, 9:30 a.m. UTC | #15

Hi Junio,

On Wed, 28 Nov 2018, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
> 
> > ...
> > In short, even a thorough study of the code (keeping in mind the few
> > tidbits of information provided by you) leaves me really wondering which
> > code you run, because it sure does not look like current `master` to me.
> >
> > And if it is not `master`, then I have to ask why you keep suggesting to
> > turn off the built-in rebase by default in `master`.
> >
> > Ciao,
> > Johannes
> >
> > P.S.: Maybe you have a hook you forgot to mention?
> 
> Any response?  Or can I retract jc/postpone-rebase-in-c that was
> prepared as a reaction to this?

I worked with Ævar via IRC and we figured out the root cause and I
submitted https://public-inbox.org/git/[e-mail]/
to fix it.

Given that this is a really obscure edge case (`git rebase --stat -v -i`
onto an unrelated commit history, if you take away one of these, the bug
does not trigger), and that it was only discovered to be a bug *because*
of the built-in rebase (the scripted version had the same bug, but simply
forgot to do proper error checking), I would not think that the reported
bug is a strong argument in favor of turning off the built-in rebase by
defauly.

In other words, after understanding the bug I am even more confident than
before that the built-in rebase is actually in a pretty good shape.

I do not expect any major regressions, and if any happen: we do have that
escape hatch for corner cases while I fix those bugs.

Ciao,
Dscho

Patch

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;
 	}