[PATCH v3] rebase: add a config option for --no-fork-point

[PATCH v3] rebase: add a config option for --no-fork-point

[Alex Henrie]

Some users (myself included) would prefer to have this feature off by
default because it can silently drop commits.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
v3: Avoid calling test_expect_success from test_expect_success
---
 Documentation/config/rebase.txt |  3 ++
 builtin/rebase.c                | 20 +++++++-----
 t/t3431-rebase-fork-point.sh    | 55 +++++++++++++++++++++++++++------
 3 files changed, 61 insertions(+), 17 deletions(-)

diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index 7f7a07d22f..8531a4b3f7 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -68,3 +68,6 @@ rebase.rescheduleFailedExec::
 	Automatically reschedule `exec` commands that failed. This only makes
 	sense in interactive mode (or when an `--exec` option was provided).
 	This is the same as specifying the `--reschedule-failed-exec` option.
+
+rebase.forkPoint:
+	If set to false set `--no-fork-point` option by default.
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 840dbd7eb7..de400f9a19 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -102,6 +102,7 @@ struct rebase_options {
 	int reschedule_failed_exec;
 	int use_legacy_rebase;
 	int reapply_cherry_picks;
+	int fork_point;
 };
 
 #define REBASE_OPTIONS_INIT {			  	\
@@ -111,7 +112,8 @@ struct rebase_options {
 		.default_backend = "merge",	  	\
 		.flags = REBASE_NO_QUIET, 		\
 		.git_am_opts = STRVEC_INIT,		\
-		.git_format_patch_opt = STRBUF_INIT	\
+		.git_format_patch_opt = STRBUF_INIT,	\
+		.fork_point = -1,			\
 	}
 
 static struct replay_opts get_replay_opts(const struct rebase_options *opts)
@@ -1095,6 +1097,11 @@ static int rebase_config(const char *var, const char *value, void *data)
 		return 0;
 	}
 
+	if (!strcmp(var, "rebase.forkpoint")) {
+		opts->fork_point = git_config_bool(var, value) ? -1 : 0;
+		return 0;
+	}
+
 	if (!strcmp(var, "rebase.usebuiltin")) {
 		opts->use_legacy_rebase = !git_config_bool(var, value);
 		return 0;
@@ -1306,7 +1313,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	const char *gpg_sign = NULL;
 	struct string_list exec = STRING_LIST_INIT_NODUP;
 	const char *rebase_merges = NULL;
-	int fork_point = -1;
 	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
 	struct object_id squash_onto;
 	char *squash_onto_name = NULL;
@@ -1406,7 +1412,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			N_("mode"),
 			N_("try to rebase merges instead of skipping them"),
 			PARSE_OPT_OPTARG, NULL, (intptr_t)""},
-		OPT_BOOL(0, "fork-point", &fork_point,
+		OPT_BOOL(0, "fork-point", &options.fork_point,
 			 N_("use 'merge-base --fork-point' to refine upstream")),
 		OPT_STRING('s', "strategy", &options.strategy,
 			   N_("strategy"), N_("use the given merge strategy")),
@@ -1494,7 +1500,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			die(_("cannot combine '--keep-base' with '--root'"));
 	}
 
-	if (options.root && fork_point > 0)
+	if (options.root && options.fork_point > 0)
 		die(_("cannot combine '--root' with '--fork-point'"));
 
 	if (action != ACTION_NONE && !in_progress)
@@ -1840,8 +1846,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 								    NULL);
 			if (!options.upstream_name)
 				error_on_missing_default_upstream();
-			if (fork_point < 0)
-				fork_point = 1;
+			if (options.fork_point < 0)
+				options.fork_point = 1;
 		} else {
 			options.upstream_name = argv[0];
 			argc--;
@@ -1945,7 +1951,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	} else
 		BUG("unexpected number of arguments left to parse");
 
-	if (fork_point > 0) {
+	if (options.fork_point > 0) {
 		struct commit *head =
 			lookup_commit_reference(the_repository,
 						&options.orig_head);
diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
index 2dab893c75..4c98d99e7e 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -29,19 +29,23 @@ test_expect_success setup '
 	test_commit G
 '
 
+do_test_rebase () {
+	expected="$1" &&
+	shift &&
+	git checkout main &&
+	git reset --hard E &&
+	git checkout side &&
+	git reset --hard G &&
+	git rebase $* &&
+	test_write_lines $expected >expect &&
+	git log --pretty=%s >actual &&
+	test_cmp expect actual
+}
+
 test_rebase () {
 	expected="$1" &&
 	shift &&
-	test_expect_success "git rebase $*" "
-		git checkout main &&
-		git reset --hard E &&
-		git checkout side &&
-		git reset --hard G &&
-		git rebase $* &&
-		test_write_lines $expected >expect &&
-		git log --pretty=%s >actual &&
-		test_cmp expect actual
-	"
+	test_expect_success "git rebase $*" "do_test_rebase '$expected' $*"
 }
 
 test_rebase 'G F E D B A'
@@ -77,4 +81,35 @@ test_expect_success 'git rebase --fork-point with ambigous refname' '
 	test_must_fail git rebase --fork-point --onto D one
 '
 
+test_expect_success '--fork-point and --root both given' '
+	test_must_fail git rebase --fork-point --root 2>err &&
+	test_i18ngrep "cannot combine" err
+'
+
+test_expect_success 'rebase.forkPoint set to false' '
+	test_config rebase.forkPoint false &&
+	do_test_rebase "G F C E D B A"
+'
+
+test_expect_success 'rebase.forkPoint set to false and then to true' '
+	test_config_global rebase.forkPoint false &&
+	test_config rebase.forkPoint true &&
+	do_test_rebase "G F E D B A"
+'
+
+test_expect_success 'rebase.forkPoint set to false and command line says --fork-point' '
+	test_config rebase.forkPoint false &&
+	do_test_rebase "G F E D B A" --fork-point
+'
+
+test_expect_success 'rebase.forkPoint set to true and command line says --no-fork-point' '
+	test_config rebase.forkPoint true &&
+	do_test_rebase "G F C E D B A" --no-fork-point
+'
+
+test_expect_success 'rebase.forkPoint set to true and --root given' '
+	test_config rebase.forkPoint true &&
+	git rebase --root
+'
+
 test_done
-- 
2.30.1

Re: [PATCH v3] rebase: add a config option for --no-fork-point

[Denton Liu]

Hi Alex,

On Tue, Feb 23, 2021 at 12:18:40AM -0700, Alex Henrie wrote:
> @@ -77,4 +81,35 @@ test_expect_success 'git rebase --fork-point with ambigous refname' '
>  	test_must_fail git rebase --fork-point --onto D one
>  '
>  
> +test_expect_success '--fork-point and --root both given' '
> +	test_must_fail git rebase --fork-point --root 2>err &&
> +	test_i18ngrep "cannot combine" err
> +'
> +
> +test_expect_success 'rebase.forkPoint set to false' '
> +	test_config rebase.forkPoint false &&
> +	do_test_rebase "G F C E D B A"
> +'
> +
> +test_expect_success 'rebase.forkPoint set to false and then to true' '
> +	test_config_global rebase.forkPoint false &&
> +	test_config rebase.forkPoint true &&
> +	do_test_rebase "G F E D B A"
> +'

I don't think this test is quite necessary. In other parts of the code,
we've already tested that local configs have priority over global
configs. We can assume that config machinery works so we don't need to
test it here.

Thanks,
Denton

> +
> +test_expect_success 'rebase.forkPoint set to false and command line says --fork-point' '
> +	test_config rebase.forkPoint false &&
> +	do_test_rebase "G F E D B A" --fork-point
> +'
> +
> +test_expect_success 'rebase.forkPoint set to true and command line says --no-fork-point' '
> +	test_config rebase.forkPoint true &&
> +	do_test_rebase "G F C E D B A" --no-fork-point
> +'
> +
> +test_expect_success 'rebase.forkPoint set to true and --root given' '
> +	test_config rebase.forkPoint true &&
> +	git rebase --root
> +'
> +
>  test_done
> -- 
> 2.30.1
> 

Re: [PATCH v3] rebase: add a config option for --no-fork-point

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> I don't think this test is quite necessary. In other parts of the
> code, we've already tested that local configs have priority over
> global configs. We can assume that config machinery works so we
> don't need to test it here.

I am not so sure if we should place so much trust in the code of
"rebase", or any other subsystem for that matter, that it uses the
configuration API and parse-options API correctly.

The do_test_rebase helper introduced here would be useful clean-up
on its own, so I'd rather take it as-is.

Re: [PATCH v3] rebase: add a config option for --no-fork-point

[Martin Ågren]

On Tue, 23 Feb 2021 at 08:24, Alex Henrie <alexhenrie24@gmail.com> wrote:
> +rebase.forkPoint:
> +       If set to false set `--no-fork-point` option by default.

This should be a double-colon at end of the line, not just a single
colon, in order to make it a "description list separator". When it's
just ":", it ends up being rendered literally, which isn't horrible, to
be sure, but which doesn't match this item's neighbours.

Martin

Re: [PATCH v3] rebase: add a config option for --no-fork-point

[Martin Ågren]

On Wed, 24 Feb 2021 at 20:33, Martin Ågren <martin.agren@gmail.com> wrote:

> just ":", it ends up being rendered literally, which isn't horrible, to

Hmm, I sort of take that back. In git-config.1, it looks not-too-bad,
but in git-rebase.1, this item runs into the next one (sequence.editor)
and messes with it, like so:

  rebase.forkPoint: If set to false set --no-fork-point option by
  default. sequence.editor:: Text editor used by git rebase -i for
  editing the rebase instruction file. The value is meant to be
  interpreted by the shell when it is used. It can be overridden by the
  GIT_SEQUENCE_EDITOR environment variable. When not configured the
  default commit message editor is used instead.

Martin

Re: [PATCH v3] rebase: add a config option for --no-fork-point

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> On Tue, 23 Feb 2021 at 08:24, Alex Henrie <alexhenrie24@gmail.com> wrote:
>> +rebase.forkPoint:
>> +       If set to false set `--no-fork-point` option by default.
>
> This should be a double-colon at end of the line, not just a single
> colon, in order to make it a "description list separator". When it's
> just ":", it ends up being rendered literally, which isn't horrible, to
> be sure, but which doesn't match this item's neighbours.
>
> Martin

Thanks for your sharp eyes; will amend locally before merging it to
'next'.

Re: [PATCH v3] rebase: add a config option for --no-fork-point

[Alex Henrie]

On Wed, Feb 24, 2021 at 12:49 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> > On Tue, 23 Feb 2021 at 08:24, Alex Henrie <alexhenrie24@gmail.com> wrote:
> >> +rebase.forkPoint:
> >> +       If set to false set `--no-fork-point` option by default.
> >
> > This should be a double-colon at end of the line, not just a single
> > colon, in order to make it a "description list separator". When it's
> > just ":", it ends up being rendered literally, which isn't horrible, to
> > be sure, but which doesn't match this item's neighbours.
> >
> > Martin
>
> Thanks for your sharp eyes; will amend locally before merging it to
> 'next'.

Agreed, thank you!

-Alex