Re: [PATCH v5 20/20] rebase: rename the two primary rebase backends

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Elijah Newren wrote:
> On Thu, Mar 12, 2020 at 11:46 AM Jonathan Nieder <jrnieder@gmail.com> wrote:

>> Sorry for the lack of clarity.  I mean allowing
>>
>>         [rebase]
>>                 backend = am
>>                 backend = apply
>>                 backend = futuristic
>>
>> with behavior
>>
>> - on "git" that understands am but not apply or futuristic, use the am
>>   backend
>> - on "git" that understands apply but not am or futuristic, use the
>>   apply backend
>> - on "git" that understands apply and futuristic, use the futuristic
>>   backend
>>
>> That way, a single config file is usable on all three versions of Git.
>
> Ah, gotcha, that makes sense though we'd need to make the thing
> multi-valued which is a bit late for 2.26.  But we could at least
> extend the logic in that way for 2.27.

Here's a patch implementing that.  I'm not convinced it's worth the
complexity, mostly because I'm not convinced that rebase is going to
have to select between additional new backends in the future.  But if
you think it will, then I think this would be a reasonable thing to do
(maybe even without the documentation part of the patch).

Thoughts?

Thanks,
Jonathan

-- >8 --
Subject: rebase: allow specifying unrecognized rebase.backend with a fallback

In 8295ed690bf (rebase: make the backend configurable via config
setting, 2020-02-15), Git learned a new rebase.backend setting that
can be used to specify which implementation should be used for
non-interactive rebases: "am" (now called "apply"), which uses "git
am", or "merge", which uses the three-way merge machinery.

Most likely those are the only two backends that rebase will ever need
to learn, so this level of configurability would be sufficient.  At
some point the "apply" backend would be retired, and the setting would
be removed altogether.

Suppose, though, that rebase learns another backend --- e.g. "faster".
In that case, a user might set configuration to request it:

	[rebase]
		backend = faster

If their configuration is shared between multiple versions of Git
(think "home directory on NFS shared between machines"), this would
produce errors when read by older versions of Git:

	fatal: Unknown rebase backend: faster

On the other hand, if we ignore unrecognized rebase backend settings,
then Git would fail to realize that

	[rebase]
		backend = appply

is a typo, producing a confusing user experience.  Let's do something
in between: when a rebase backend setting is unrecognized, fall back
to the last earlier recognized value, but if no value was recognized,
print an error message allowing the user to catch their typo.

Reported-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/config/rebase.txt |  5 ++
 builtin/rebase.c                | 52 +++++++++++++++---
 t/t3435-rebase-backend.sh       | 97 +++++++++++++++++++++++++++++++++
 3 files changed, 146 insertions(+), 8 deletions(-)
 create mode 100755 t/t3435-rebase-backend.sh

diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index 7f7a07d22f8..c92adbdcc69 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -10,6 +10,11 @@ rebase.backend::
 	'apply' or 'merge'.  In the future, if the merge backend gains
 	all remaining capabilities of the apply backend, this setting
 	may become unused.
++
+If set multiple times, the last value corresponding to a recognized
+backend is used. This is for forward compatibility, as it allows
+specifying a rebase backend that Git does not know about yet along
+with a backend known today as a fallback.
 
 rebase.stat::
 	Whether to show a diffstat of what changed upstream since the last
diff --git a/builtin/rebase.c b/builtin/rebase.c
index ffa467aad52..5b0fab9741f 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -56,10 +56,18 @@ enum empty_type {
 	EMPTY_ASK
 };
 
+enum rebase_backend {
+	BACKEND_UNSPECIFIED = 0,
+	BACKEND_UNRECOGNIZED,
+	BACKEND_APPLY,
+	BACKEND_MERGE,
+};
+
 struct rebase_options {
 	enum rebase_type type;
 	enum empty_type empty;
-	const char *default_backend;
+	enum rebase_backend configured_backend;
+	const char *last_specified_backend;
 	const char *state_dir;
 	struct commit *upstream;
 	const char *upstream_name;
@@ -100,7 +108,6 @@ struct rebase_options {
 #define REBASE_OPTIONS_INIT {			  	\
 		.type = REBASE_UNSPECIFIED,	  	\
 		.empty = EMPTY_UNSPECIFIED,	  	\
-		.default_backend = "merge",	  	\
 		.flags = REBASE_NO_QUIET, 		\
 		.git_am_opts = ARGV_ARRAY_INIT,		\
 		.git_format_patch_opt = STRBUF_INIT	\
@@ -1224,6 +1231,15 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)
 	return status ? -1 : 0;
 }
 
+static enum rebase_backend parse_rebase_backend(const char *value)
+{
+	if (!strcmp(value, "apply"))
+		return BACKEND_APPLY;
+	if (!strcmp(value, "merge"))
+		return BACKEND_MERGE;
+	return BACKEND_UNRECOGNIZED;
+}
+
 static int rebase_config(const char *var, const char *value, void *data)
 {
 	struct rebase_options *opts = data;
@@ -1264,7 +1280,18 @@ static int rebase_config(const char *var, const char *value, void *data)
 	}
 
 	if (!strcmp(var, "rebase.backend")) {
-		return git_config_string(&opts->default_backend, var, value);
+		enum rebase_backend val;
+		if (!value)
+			return config_error_nonbool(var);
+		val = parse_rebase_backend(value);
+		if (opts->configured_backend == BACKEND_UNSPECIFIED)
+			opts->configured_backend = val;
+		else if (val == BACKEND_UNRECOGNIZED)
+			; /* Unrecognized rebase backend. Ignore it. */
+		else
+			opts->configured_backend = val;
+		opts->last_specified_backend = value;
+		return 0;
 	}
 
 	return git_default_config(var, value, data);
@@ -1903,14 +1930,23 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	if (options.configured_backend == BACKEND_UNRECOGNIZED)
+		die(_("unknown rebase backend: %s"),
+		    options.last_specified_backend);
+
 	if (options.type == REBASE_UNSPECIFIED) {
-		if (!strcmp(options.default_backend, "merge"))
+		switch (options.configured_backend) {
+		case BACKEND_UNSPECIFIED:
+		case BACKEND_MERGE:
 			imply_merge(&options, "--merge");
-		else if (!strcmp(options.default_backend, "apply"))
+			break;
+		case BACKEND_APPLY:
 			options.type = REBASE_APPLY;
-		else
-			die(_("Unknown rebase backend: %s"),
-			    options.default_backend);
+			break;
+		default:
+			BUG("unexpected backend %d",
+			    (int) options.configured_backend);
+		}
 	}
 
 	switch (options.type) {
diff --git a/t/t3435-rebase-backend.sh b/t/t3435-rebase-backend.sh
new file mode 100755
index 00000000000..8b9ba6f1894
--- /dev/null
+++ b/t/t3435-rebase-backend.sh
@@ -0,0 +1,97 @@
+#!/bin/sh
+
+test_description='rebase.backend tests
+
+Checks of config parsing for the [rebase] backend setting.  We detect
+which backend was used by checking which directory was created to hold
+state.'
+
+. ./test-lib.sh
+
+# usage: test_backend_choice <expectation> <command>
+#
+# Tests that the chosen backend for rebase command <command>
+# is <expectation> ("merge" or "apply").
+test_backend_choice () {
+	expect=$1 &&
+	shift &&
+
+	test_must_fail git "$@" master topic &&
+	case $expect in
+	apply)
+		test_path_is_dir .git/rebase-apply &&
+		test_path_is_missing .git/rebase-merge
+		;;
+	merge)
+		test_path_is_dir .git/rebase-merge &&
+		test_path_is_missing .git/rebase-apply
+		;;
+	*)
+		error "unrecognized expectation $expect"
+	esac
+}
+
+test_expect_success 'setup' '
+	test_commit base &&
+	test_commit sidea conflict.txt myway &&
+	git checkout -b topic base &&
+	test_commit sideb conflict.txt thehighway
+'
+
+test_expect_success '--apply uses apply backend' '
+	test_when_finished "git rebase --abort" &&
+	test_backend_choice apply rebase --apply
+'
+
+test_expect_success '--merge uses merge backend' '
+	test_when_finished "git rebase --abort" &&
+	test_backend_choice merge rebase --merge
+'
+
+test_expect_success 'default to merge backend' '
+	test_when_finished "git rebase --abort" &&
+	test_backend_choice merge rebase
+'
+
+test_expect_success 'config overrides default' '
+	test_when_finished "git rebase --abort" &&
+	test_backend_choice apply -c rebase.backend=apply rebase
+'
+
+test_expect_success 'option overrides config' '
+	test_when_finished "git rebase --abort" &&
+	test_backend_choice merge -c rebase.backend=apply rebase --merge
+'
+
+test_expect_success 'last config value wins' '
+	test_when_finished "git rebase --abort" &&
+	test_backend_choice merge \
+		-c rebase.backend=apply \
+		-c rebase.backend=merge \
+		rebase
+'
+
+test_expect_success 'last config value wins' '
+	test_when_finished "git rebase --abort" &&
+	test_backend_choice merge \
+		-c rebase.backend=apply \
+		-c rebase.backend=merge \
+		rebase
+'
+
+test_expect_success 'misspelled backend without fallback is diagnosed' '
+	test_must_fail \
+		git -c rebase.backend=appply rebase master topic 2>message &&
+	test_i18ngrep "unknown rebase backend" message &&
+	grep appply message
+'
+
+test_expect_success 'forward compatibility by skipping unrecognized values' '
+	test_when_finished "git rebase --abort" &&
+	test_backend_choice apply \
+		-c rebase.backend=apply \
+		-c rebase.backend=futuristic \
+		rebase
+'
+
+test_done
-- 
2.25.1.481.gfbce0eb801