git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [PATCH v3] rebase: add a config option for --no-fork-point
@ 2021-02-23  7:18 Alex Henrie
  2021-02-23  7:32 ` Denton Liu
  2021-02-24 19:33 ` Martin Ågren
  0 siblings, 2 replies; 7+ messages in thread
From: Alex Henrie @ 2021-02-23  7:18 UTC (permalink / raw)
  To: git, gitster, liu.denton; +Cc: 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] rebase: add a config option for --no-fork-point
  2021-02-23  7:18 [PATCH v3] rebase: add a config option for --no-fork-point Alex Henrie
@ 2021-02-23  7:32 ` Denton Liu
  2021-02-23  8:21   ` Junio C Hamano
  2021-02-24 19:33 ` Martin Ågren
  1 sibling, 1 reply; 7+ messages in thread
From: Denton Liu @ 2021-02-23  7:32 UTC (permalink / raw)
  To: Alex Henrie; +Cc: git, gitster

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
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] rebase: add a config option for --no-fork-point
  2021-02-23  7:32 ` Denton Liu
@ 2021-02-23  8:21   ` Junio C Hamano
  0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2021-02-23  8:21 UTC (permalink / raw)
  To: Denton Liu; +Cc: Alex Henrie, git

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] rebase: add a config option for --no-fork-point
  2021-02-23  7:18 [PATCH v3] rebase: add a config option for --no-fork-point Alex Henrie
  2021-02-23  7:32 ` Denton Liu
@ 2021-02-24 19:33 ` Martin Ågren
  2021-02-24 19:38   ` Martin Ågren
  2021-02-24 19:48   ` Junio C Hamano
  1 sibling, 2 replies; 7+ messages in thread
From: Martin Ågren @ 2021-02-24 19:33 UTC (permalink / raw)
  To: Alex Henrie; +Cc: Git Mailing List, Junio C Hamano, Denton Liu

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] rebase: add a config option for --no-fork-point
  2021-02-24 19:33 ` Martin Ågren
@ 2021-02-24 19:38   ` Martin Ågren
  2021-02-24 19:48   ` Junio C Hamano
  1 sibling, 0 replies; 7+ messages in thread
From: Martin Ågren @ 2021-02-24 19:38 UTC (permalink / raw)
  To: Alex Henrie; +Cc: Git Mailing List, Junio C Hamano, Denton Liu

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] rebase: add a config option for --no-fork-point
  2021-02-24 19:33 ` Martin Ågren
  2021-02-24 19:38   ` Martin Ågren
@ 2021-02-24 19:48   ` Junio C Hamano
  2021-02-24 20:38     ` Alex Henrie
  1 sibling, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2021-02-24 19:48 UTC (permalink / raw)
  To: Martin Ågren; +Cc: Alex Henrie, Git Mailing List, Denton Liu

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'.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] rebase: add a config option for --no-fork-point
  2021-02-24 19:48   ` Junio C Hamano
@ 2021-02-24 20:38     ` Alex Henrie
  0 siblings, 0 replies; 7+ messages in thread
From: Alex Henrie @ 2021-02-24 20:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Martin Ågren, Git Mailing List, Denton Liu

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-02-24 20:41 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-23  7:18 [PATCH v3] rebase: add a config option for --no-fork-point Alex Henrie
2021-02-23  7:32 ` Denton Liu
2021-02-23  8:21   ` Junio C Hamano
2021-02-24 19:33 ` Martin Ågren
2021-02-24 19:38   ` Martin Ågren
2021-02-24 19:48   ` Junio C Hamano
2021-02-24 20:38     ` Alex Henrie

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git