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

From

Jonathan Nieder <jrnieder@gmail.com>

Date

Mar 17, 2020, 02:58 UTC

Message-ID

<20200317025808.GB31380@google.com>

In-Reply-To

<CABPp-BHSAbJzWEsPSTM5Q6MPdmu4VSuOx-=6-MJkHUovg3_1=g@mail.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.

Thoughts?

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

	[rebase]
		backend = faster

	fatal: Unknown rebase backend: faster

	[rebase]
		backend = appply

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