* [PATCH] rebase: add a config option for --no-fork-point
@ 2021-01-20 4:44 Alex Henrie
2021-01-20 23:00 ` Junio C Hamano
0 siblings, 1 reply; 9+ messages in thread
From: Alex Henrie @ 2021-01-20 4:44 UTC (permalink / raw)
To: git; +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>
---
Documentation/config/rebase.txt | 3 +++
builtin/rebase.c | 21 ++++++++++++++-------
t/t3431-rebase-fork-point.sh | 10 ++++++++++
3 files changed, 27 insertions(+), 7 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..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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] rebase: add a config option for --no-fork-point
2021-01-20 4:44 [PATCH] rebase: add a config option for --no-fork-point Alex Henrie
@ 2021-01-20 23:00 ` Junio C Hamano
2021-01-20 23:13 ` Alex Henrie
0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2021-01-20 23:00 UTC (permalink / raw)
To: Alex Henrie; +Cc: git
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?
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?
> +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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] rebase: add a config option for --no-fork-point
2021-01-20 23:00 ` Junio C Hamano
@ 2021-01-20 23:13 ` Alex Henrie
2021-01-21 22:27 ` Junio C Hamano
0 siblings, 1 reply; 9+ messages in thread
From: Alex Henrie @ 2021-01-20 23:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git mailing list
On Wed, Jan 20, 2021 at 4:00 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] rebase: add a config option for --no-fork-point
2021-01-20 23:13 ` Alex Henrie
@ 2021-01-21 22:27 ` Junio C Hamano
2021-01-21 23:25 ` Alex Henrie
0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2021-01-21 22:27 UTC (permalink / raw)
To: Alex Henrie; +Cc: Git mailing list
Alex Henrie <alexhenrie24@gmail.com> writes:
>> > - 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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] rebase: add a config option for --no-fork-point
2021-01-21 22:27 ` Junio C Hamano
@ 2021-01-21 23:25 ` Alex Henrie
2021-01-21 23:45 ` Denton Liu
0 siblings, 1 reply; 9+ messages in thread
From: Alex Henrie @ 2021-01-21 23:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git mailing list
On Thu, Jan 21, 2021 at 3:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] rebase: add a config option for --no-fork-point
2021-01-21 23:25 ` Alex Henrie
@ 2021-01-21 23:45 ` Denton Liu
2021-01-21 23:51 ` Alex Henrie
0 siblings, 1 reply; 9+ messages in thread
From: Denton Liu @ 2021-01-21 23:45 UTC (permalink / raw)
To: Alex Henrie; +Cc: Junio C Hamano, Git mailing list
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] rebase: add a config option for --no-fork-point
2021-01-21 23:45 ` Denton Liu
@ 2021-01-21 23:51 ` Alex Henrie
2021-02-18 23:29 ` Junio C Hamano
0 siblings, 1 reply; 9+ messages in thread
From: Alex Henrie @ 2021-01-21 23:51 UTC (permalink / raw)
To: Denton Liu; +Cc: Junio C Hamano, Git mailing list
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.
-Alex
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] rebase: add a config option for --no-fork-point
2021-01-21 23:51 ` Alex Henrie
@ 2021-02-18 23:29 ` Junio C Hamano
2021-02-20 5:10 ` Alex Henrie
0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2021-02-18 23:29 UTC (permalink / raw)
To: Alex Henrie; +Cc: Denton Liu, Git mailing list
Alex Henrie <alexhenrie24@gmail.com> writes:
> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-02-20 5:14 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-20 4:44 [PATCH] rebase: add a config option for --no-fork-point Alex Henrie
2021-01-20 23:00 ` Junio C Hamano
2021-01-20 23:13 ` Alex Henrie
2021-01-21 22:27 ` Junio C Hamano
2021-01-21 23:25 ` Alex Henrie
2021-01-21 23:45 ` Denton Liu
2021-01-21 23:51 ` Alex Henrie
2021-02-18 23:29 ` Junio C Hamano
2021-02-20 5:10 ` Alex Henrie
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).