threads / patch / 55015

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

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

## tl;dr

9 messages between Jan 20, 2021 and Feb 20, 2021. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Alex Henrie· Jan 20, 2021, 04:44 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>
---
 Documentation/config/rebase.txt |  3 +++
 builtin/rebase.c                | 21 ++++++++++++++-------
 t/t3431-rebase-fork-point.sh    | 10 ++++++++++
 3 files changed, 27 insertions(+), 7 deletions(-)
Show changes to 3 files +27 −7

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..1679acc649 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,12 @@ static int rebase_config(const char *var, const char *value, void *data)
 		return 0;
 	}
 
+	if (!strcmp(var, "rebase.forkpoint")) {
+		if (!git_config_bool(var, value))
+			opts->fork_point = 0;
+		return 0;
+	}
+
 	if (!strcmp(var, "rebase.usebuiltin")) {
 		opts->use_legacy_rebase = !git_config_bool(var, value);
 		return 0;
@@ -1306,7 +1314,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 +1413,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 +1501,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 +1847,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 +1952,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 172562789e..e48c4014cf 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -74,4 +74,14 @@ 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 true and --root given' '
+	test_config rebase.forkPoint true &&
+	git rebase --root
+'
+
 test_done
-- 
2.30.0
Junio C Hamano· Jan 20, 2021, 23:00 UTC · re: Alex Henrie · lore

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

Alex Henrie <alexhenrie24@gmail.com> writes:
Show 9 quoted lines
> @@ -1095,6 +1097,12 @@ static int rebase_config(const char *var, const char *value, void *data)
>  		return 0;
>  	}
>  
> +	if (!strcmp(var, "rebase.forkpoint")) {
> +		if (!git_config_bool(var, value))
> +			opts->fork_point = 0;
> +		return 0;
> +	}
It is curious that this code seems to deliberatly ignore
	[rebase] forkpoint = true
but honors
	[rebase] forkpoint = false
Intended?  IOW, why isn't this just like this?
	if (!strcmp(var, "rebase.forkpoint")) {
		opts->fork_point = git_config_bool(var, value);
		return 0;
	}
Show 24 quoted lines
> @@ -1306,7 +1314,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 +1413,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 +1501,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'"));
Is that because of this code?

If so, perhaps the configuration parser should set the .fork_point to (-1), so that "[rebase] forkpoint = false" that appears in your ~/.gitconfig file can be countermanded with "[rebase] forkpoint" that is placed in .git/config for one particular project that you do not mind using the feature?

Show 9 quoted lines
> +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 true and --root given' '
> +	test_config rebase.forkPoint true &&
> +	git rebase --root
> +'
test_expect_success 'rebase.forkPoint true overrides earlier false' '
	test_config rebase.forkPoint false &&
	test_config rebase.forkPoint true &&
	... check "git rebase" does use the fork-point feature here ...
'
Thanks.
Alex Henrie· Jan 20, 2021, 23:13 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 20, 2021 at 4:00 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 54 quoted lines
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > @@ -1095,6 +1097,12 @@ static int rebase_config(const char *var, const char *value, void *data)
> >               return 0;
> >       }
> >
> > +     if (!strcmp(var, "rebase.forkpoint")) {
> > +             if (!git_config_bool(var, value))
> > +                     opts->fork_point = 0;
> > +             return 0;
> > +     }
>
> It is curious that this code seems to deliberatly ignore
>
>         [rebase] forkpoint = true
>
> but honors
>
>         [rebase] forkpoint = false
>
> Intended?  IOW, why isn't this just like this?
>
>         if (!strcmp(var, "rebase.forkpoint")) {
>                 opts->fork_point = git_config_bool(var, value);
>                 return 0;
>         }
>
> > @@ -1306,7 +1314,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 +1413,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 +1501,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'"));
>
> Is that because of this code?

Yes. I was trying to avoid annoying the user with errors when rebase.forkPoint is set to "true" and --root is given on the command line.

Show 5 quoted lines
> If so, perhaps the configuration parser should set the .fork_point
> to (-1), so that "[rebase] forkpoint = false" that appears in your
> ~/.gitconfig file can be countermanded with "[rebase] forkpoint"
> that is placed in .git/config for one particular project that you do
> not mind using the feature?
That's a great point. I will make that change and resubmit. Thanks!
-Alex
Junio C Hamano· Jan 21, 2021, 22:27 UTC · re: Alex Henrie · lore

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

Alex Henrie <alexhenrie24@gmail.com> writes:
Show 17 quoted lines
>> > -     if (options.root && fork_point > 0)
>> > +     if (options.root && options.fork_point > 0)
>> >               die(_("cannot combine '--root' with '--fork-point'"));
>>
>> Is that because of this code?
>
> Yes. I was trying to avoid annoying the user with errors when
> rebase.forkPoint is set to "true" and --root is given on the command
> line.
>
>> If so, perhaps the configuration parser should set the .fork_point
>> to (-1), so that "[rebase] forkpoint = false" that appears in your
>> ~/.gitconfig file can be countermanded with "[rebase] forkpoint"
>> that is placed in .git/config for one particular project that you do
>> not mind using the feature?
>
> That's a great point. I will make that change and resubmit. Thanks!

By the way, is the feature itself something we can test easily? I think I saw the "we must interact with --root sensibly" test, but the actual feature not being tested include

 - rebase.forkpoint set to false and then to true; does it use the
   fork-point feature (your patch as-is would have failed this test);
 - rebase.forkpoint set to false and command line says --fork-point;
 - rebase.forkpoint set to true and command line says --no-fork-point;

I offhand do not know if it is easy to observe it the fork-point feature gets triggered, but it seems that the difference in behaviour is big enough for you to care, so I am hoping you may think of a way.

Thanks.
Alex Henrie· Jan 21, 2021, 23:25 UTC · re: Junio C Hamano · lore

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

On Thu, Jan 21, 2021 at 3:27 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
>
> By the way, is the feature itself something we can test easily?  I
> think I saw the "we must interact with --root sensibly" test, but
> the actual feature not being tested include
>
>  - rebase.forkpoint set to false and then to true; does it use the
>    fork-point feature (your patch as-is would have failed this test);
>
>  - rebase.forkpoint set to false and command line says --fork-point;
>
>  - rebase.forkpoint set to true and command line says --no-fork-point;
>
> I offhand do not know if it is easy to observe it the fork-point
> feature gets triggered, but it seems that the difference in
> behaviour is big enough for you to care, so I am hoping you may
> think of a way.

This blog post describes the same problem that I had with fork-point: https://commaok.xyz/post/fork-point/

I didn't see any fork-point tests in the current codebase, but I can work on adding some. Do you want them in a separate patch (that would make the most sense to me), or squashed into the rebase.forkPoint patch?

-Alex
Denton Liu· Jan 21, 2021, 23:45 UTC · re: Alex Henrie · lore

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

Hi Alex,
On Thu, Jan 21, 2021 at 04:25:25PM -0700, Alex Henrie wrote:
> I didn't see any fork-point tests in the current codebase, but I can
> work on adding some. Do you want them in a separate patch (that would
> make the most sense to me), or squashed into the rebase.forkPoint
> patch?
The fork-point tests should be in t3431.
-Denton
Alex Henrie· Jan 21, 2021, 23:51 UTC · re: Denton Liu · lore

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

On Thu, Jan 21, 2021 at 4:45 PM Denton Liu <liu.denton@gmail.com> wrote:
Show 8 quoted lines
>
> On Thu, Jan 21, 2021 at 04:25:25PM -0700, Alex Henrie wrote:
> > I didn't see any fork-point tests in the current codebase, but I can
> > work on adding some. Do you want them in a separate patch (that would
> > make the most sense to me), or squashed into the rebase.forkPoint
> > patch?
>
> The fork-point tests should be in t3431.

Derp! For some reason I thought that t3431 only tested --fork-point with an ambiguous refname and that the other tests were unrelated. Maybe we should make copies of some of those tests that set rebase.forkPoint instead of passing --fork-point or --no-fork-point on the command line.

-Alex
Junio C Hamano· Feb 18, 2021, 23:29 UTC · re: Alex Henrie · lore

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

Alex Henrie <alexhenrie24@gmail.com> writes:
Show 15 quoted lines
> On Thu, Jan 21, 2021 at 4:45 PM Denton Liu <liu.denton@gmail.com> wrote:
>>
>> On Thu, Jan 21, 2021 at 04:25:25PM -0700, Alex Henrie wrote:
>> > I didn't see any fork-point tests in the current codebase, but I can
>> > work on adding some. Do you want them in a separate patch (that would
>> > make the most sense to me), or squashed into the rebase.forkPoint
>> > patch?
>>
>> The fork-point tests should be in t3431.
>
> Derp! For some reason I thought that t3431 only tested --fork-point
> with an ambiguous refname and that the other tests were unrelated.
> Maybe we should make copies of some of those tests that set
> rebase.forkPoint instead of passing --fork-point or --no-fork-point on
> the command line.
Ping.

Any progress on this topic? It's been almost a month without seeing any activity.

Thanks.
Alex Henrie· Feb 20, 2021, 05:10 UTC · re: Junio C Hamano · lore

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

On Thu, Feb 18, 2021 at 4:29 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Any progress on this topic?  It's been almost a month without seeing
> any activity.

Sorry for letting this slip. I just sent a v2 of the patch with the specific tests that you requested on January 21.

-Alex

← back to recent threads