Re: [PATCH v3 10/15] rebase: add an --am option

[Phillip Wood <phillip.wood123@xxxxxxxxx>]

Hi Elijah

On 24/12/2019 19:54, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@xxxxxxxxx>
>
> Currently, this option doesn't do anything except error out if any
> options requiring the interactive-backend are also passed.  However,
> when we make the default backend configurable later in this series, this
> flag will provide a way to override the config setting.

I wonder if we really want to add a new option that we know we don't 
want to keep in the long term. Could we not just default to the 
interactive backend and fallback to the am backend if the user passes 
any of the am-based options on the commandline? i.e. reverse the current 
situation of defaulting to the am backend and selecting the interactive 
backend when the user passes certain options.

Best Wishes

Phillip


> Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
> ---
>   Documentation/git-rebase.txt | 11 ++++++++++-
>   builtin/rebase.c             | 18 +++++++++++++++++-
>   2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index f1ace07c38..cf1ac2e359 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -258,6 +258,13 @@ See also INCOMPATIBLE OPTIONS below.
>   	original branch. The index and working tree are also left
>   	unchanged as a result.
>   
> +--am:
> +	Use git-am internally to rebase.  This option may become a
> +	no-op in the future once the interactive backend handles
> +	everything the am one does.
> ++
> +See also INCOMPATIBLE OPTIONS below.
> +
>   --empty={drop,keep,ask}::
>   	How to handle commits that become empty (because they contain a
>   	subset of already upstream changes) or start empty.  With drop
> @@ -372,7 +379,7 @@ See also INCOMPATIBLE OPTIONS below.
>   	Ensure at least <n> lines of surrounding context match before
>   	and after each change.  When fewer lines of surrounding
>   	context exist they all must match.  By default no context is
> -	ever ignored.
> +	ever ignored.  Implies --am.
>   +
>   See also INCOMPATIBLE OPTIONS below.
>   
> @@ -417,6 +424,7 @@ with `--keep-base` in order to drop those commits from your branch.
>   --whitespace=<option>::
>   	This flag is passed to the 'git apply' program
>   	(see linkgit:git-apply[1]) that applies the patch.
> +	Implies --am.
>   +
>   See also INCOMPATIBLE OPTIONS below.
>   
> @@ -567,6 +575,7 @@ INCOMPATIBLE OPTIONS
>   
>   The following options:
>   
> + * --am
>    * --whitespace
>    * -C
>   
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index d2b99e9908..b7915fc0cb 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1361,6 +1361,18 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
>   	return res && is_linear_history(onto, head);
>   }
>   
> +static int parse_opt_am(const struct option *opt, const char *arg, int unset)
> +{
> +	struct rebase_options *opts = opt->value;
> +
> +	BUG_ON_OPT_NEG(unset);
> +	BUG_ON_OPT_ARG(arg);
> +
> +	opts->type = REBASE_AM;
> +
> +	return 0;
> +}
> +
>   /* -i followed by -m is still -i */
>   static int parse_opt_merge(const struct option *opt, const char *arg, int unset)
>   {
> @@ -1546,6 +1558,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		OPT_CMDMODE(0, "show-current-patch", &action,
>   			    N_("show the patch file being applied or merged"),
>   			    ACTION_SHOW_CURRENT_PATCH),
> +		{ OPTION_CALLBACK, 0, "am", &options, NULL,
> +			N_("use apply-mail strategies to rebase"),
> +			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
> +			parse_opt_am },
>   		{ OPTION_CALLBACK, 'm', "merge", &options, NULL,
>   			N_("use merging strategies to rebase"),
>   			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
> @@ -1906,7 +1922,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	if (isatty(2) && options.flags & REBASE_NO_QUIET)
>   		strbuf_addstr(&options.git_format_patch_opt, " --progress");
>   
> -	if (options.git_am_opts.argc) {
> +	if (options.git_am_opts.argc || options.type == REBASE_AM) {
>   		/* all am options except -q are compatible only with --am */
>   		for (i = options.git_am_opts.argc - 1; i >= 0; i--)
>   			if (strcmp(options.git_am_opts.argv[i], "-q"))