threads / patch / 55191

v3rebase: add a config option for --no-fork-point

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

## tl;dr

7 messages between Feb 23, 2021 and Feb 24, 2021. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Alex Henrie· Feb 23, 2021, 07:18 UTC · lore

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(-)
Show changes to 3 files +61 −17

Documentation/config/rebase.txt, builtin/rebase.c, t/t3431-rebase-fork-point.sh

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
Denton Liu· Feb 23, 2021, 07:32 UTC · re: Alex Henrie · lore

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

Hi Alex,
On Tue, Feb 23, 2021 at 12:18:40AM -0700, Alex Henrie wrote:
Show 19 quoted lines
> @@ -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

Show 20 quoted lines
> +
> +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
> 
Junio C Hamano· Feb 23, 2021, 08:21 UTC · re: Denton Liu · lore

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

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.

Martin Ågren· Feb 24, 2021, 19:33 UTC · re: Alex Henrie · lore

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

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
Martin Ågren· Feb 24, 2021, 19:38 UTC · re: Martin Ågren · lore

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

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
Junio C Hamano· Feb 24, 2021, 19:48 UTC · re: Martin Ågren · lore

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

Martin Ågren <martin.agren@gmail.com> writes:
Show 10 quoted lines
> 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'.

Alex Henrie· Feb 24, 2021, 20:38 UTC · re: Junio C Hamano · lore

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

On Wed, Feb 24, 2021 at 12:49 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
>
> 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

← back to recent threads