{"thread":{"id":"55015","subject":"[PATCH] rebase: add a config option for --no-fork-point","startedAt":"2021-01-20T04:50:55Z","lastAt":"2021-02-20T05:12:07Z","messageCount":9,"participants":["Alex Henrie","Junio C Hamano","Denton Liu"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"414773","messageId":"20210120044435.53509-1-alexhenrie24@gmail.com","threadId":"55015","inReplyTo":null,"subject":"[PATCH] rebase: add a config option for --no-fork-point","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2021-01-20T04:44:35Z","receivedAt":"2021-01-20T04:50:55Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Some users (myself included) would prefer to have this feature off by\ndefault because it can silently drop commits.\n\nSigned-off-by: Alex Henrie \n---\n Documentation/config/rebase.txt | 3 +++\n builtin/rebase.c | 21 ++++++++++++++-------\n t/t3431-rebase-fork-point.sh | 10 ++++++++++\n 3 files changed, 27 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt\nindex 7f7a07d22f..8531a4b3f7 100644\n--- a/Documentation/config/rebase.txt\n+++ b/Documentation/config/rebase.txt\n@@ -68,3 +68,6 @@ rebase.rescheduleFailedExec::\n \tAutomatically reschedule `exec` commands that failed. This only makes\n \tsense in interactive mode (or when an `--exec` option was provided).\n \tThis is the same as specifying the `--reschedule-failed-exec` option.\n+\n+rebase.forkPoint:\n+\tIf set to false set `--no-fork-point` option by default.\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 840dbd7eb7..1679acc649 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -102,6 +102,7 @@ struct rebase_options {\n \tint reschedule_failed_exec;\n \tint use_legacy_rebase;\n \tint reapply_cherry_picks;\n+\tint fork_point;\n };\n \n #define REBASE_OPTIONS_INIT {\t\t\t \t\\\n@@ -111,7 +112,8 @@ struct rebase_options {\n \t\t.default_backend = \"merge\",\t \t\\\n \t\t.flags = REBASE_NO_QUIET, \t\t\\\n \t\t.git_am_opts = STRVEC_INIT,\t\t\\\n-\t\t.git_format_patch_opt = STRBUF_INIT\t\\\n+\t\t.git_format_patch_opt = STRBUF_INIT,\t\\\n+\t\t.fork_point = -1,\t\t\t\\\n \t}\n \n static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n@@ -1095,6 +1097,12 @@ static int rebase_config(const char *var, const char *value, void *data)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"rebase.forkpoint\")) {\n+\t\tif (!git_config_bool(var, value))\n+\t\t\topts->fork_point = 0;\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"rebase.usebuiltin\")) {\n \t\topts->use_legacy_rebase = !git_config_bool(var, value);\n \t\treturn 0;\n@@ -1306,7 +1314,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tconst char *gpg_sign = NULL;\n \tstruct string_list exec = STRING_LIST_INIT_NODUP;\n \tconst char *rebase_merges = NULL;\n-\tint fork_point = -1;\n \tstruct string_list strategy_options = STRING_LIST_INIT_NODUP;\n \tstruct object_id squash_onto;\n \tchar *squash_onto_name = NULL;\n@@ -1406,7 +1413,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"mode\"),\n \t\t\tN_(\"try to rebase merges instead of skipping them\"),\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)\"\"},\n-\t\tOPT_BOOL(0, \"fork-point\", &fork_point,\n+\t\tOPT_BOOL(0, \"fork-point\", &options.fork_point,\n \t\t\t N_(\"use 'merge-base --fork-point' to refine upstream\")),\n \t\tOPT_STRING('s', \"strategy\", &options.strategy,\n \t\t\t N_(\"strategy\"), N_(\"use the given merge strategy\")),\n@@ -1494,7 +1501,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"cannot combine '--keep-base' with '--root'\"));\n \t}\n \n-\tif (options.root && fork_point > 0)\n+\tif (options.root && options.fork_point > 0)\n \t\tdie(_(\"cannot combine '--root' with '--fork-point'\"));\n \n \tif (action != ACTION_NONE && !in_progress)\n@@ -1840,8 +1847,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\t\t\t NULL);\n \t\t\tif (!options.upstream_name)\n \t\t\t\terror_on_missing_default_upstream();\n-\t\t\tif (fork_point < 0)\n-\t\t\t\tfork_point = 1;\n+\t\t\tif (options.fork_point < 0)\n+\t\t\t\toptions.fork_point = 1;\n \t\t} else {\n \t\t\toptions.upstream_name = argv[0];\n \t\t\targc--;\n@@ -1945,7 +1952,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t} else\n \t\tBUG(\"unexpected number of arguments left to parse\");\n \n-\tif (fork_point > 0) {\n+\tif (options.fork_point > 0) {\n \t\tstruct commit *head =\n \t\t\tlookup_commit_reference(the_repository,\n \t\t\t\t\t\t&options.orig_head);\ndiff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh\nindex 172562789e..e48c4014cf 100755\n--- a/t/t3431-rebase-fork-point.sh\n+++ b/t/t3431-rebase-fork-point.sh\n@@ -74,4 +74,14 @@ test_expect_success 'git rebase --fork-point with ambigous refname' '\n \ttest_must_fail git rebase --fork-point --onto D one\n '\n \n+test_expect_success '--fork-point and --root both given' '\n+\ttest_must_fail git rebase --fork-point --root 2>err &&\n+\ttest_i18ngrep \"cannot combine\" err\n+'\n+\n+test_expect_success 'rebase.forkPoint true and --root given' '\n+\ttest_config rebase.forkPoint true &&\n+\tgit rebase --root\n+'\n+\n test_done\n-- \n2.30.0\n\n"},{"id":"414869","messageId":"CAMMLpeQM7JCP0=TMqf0DxDCVJvkCGDEPSKN6vYTU_xc7+QaA1Q@mail.gmail.com","threadId":"55015","inReplyTo":"xmqqturbdxi2.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] rebase: add a config option for --no-fork-point","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2021-01-20T23:13:35Z","receivedAt":"2021-01-21T00:48:56Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Wed, Jan 20, 2021 at 4:00 PM Junio C Hamano wrote:\n>\n> Alex Henrie writes:\n>\n> > @@ -1095,6 +1097,12 @@ static int rebase_config(const char *var, const char *value, void *data)\n> > return 0;\n> > }\n> >\n> > + if (!strcmp(var, \"rebase.forkpoint\")) {\n> > + if (!git_config_bool(var, value))\n> > + opts->fork_point = 0;\n> > + return 0;\n> > + }\n>\n> It is curious that this code seems to deliberatly ignore\n>\n> [rebase] forkpoint = true\n>\n> but honors\n>\n> [rebase] forkpoint = false\n>\n> Intended? IOW, why isn't this just like this?\n>\n> if (!strcmp(var, \"rebase.forkpoint\")) {\n> opts->fork_point = git_config_bool(var, value);\n> return 0;\n> }\n>\n> > @@ -1306,7 +1314,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> > const char *gpg_sign = NULL;\n> > struct string_list exec = STRING_LIST_INIT_NODUP;\n> > const char *rebase_merges = NULL;\n> > - int fork_point = -1;\n> > struct string_list strategy_options = STRING_LIST_INIT_NODUP;\n> > struct object_id squash_onto;\n> > char *squash_onto_name = NULL;\n> > @@ -1406,7 +1413,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> > N_(\"mode\"),\n> > N_(\"try to rebase merges instead of skipping them\"),\n> > PARSE_OPT_OPTARG, NULL, (intptr_t)\"\"},\n> > - OPT_BOOL(0, \"fork-point\", &fork_point,\n> > + OPT_BOOL(0, \"fork-point\", &options.fork_point,\n> > N_(\"use 'merge-base --fork-point' to refine upstream\")),\n> > OPT_STRING('s', \"strategy\", &options.strategy,\n> > N_(\"strategy\"), N_(\"use the given merge strategy\")),\n> > @@ -1494,7 +1501,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> > die(_(\"cannot combine '--keep-base' with '--root'\"));\n> > }\n> >\n> > - if (options.root && fork_point > 0)\n> > + if (options.root && options.fork_point > 0)\n> > die(_(\"cannot combine '--root' with '--fork-point'\"));\n>\n> Is that because of this code?\n\nYes. I was trying to avoid annoying the user with errors when\nrebase.forkPoint is set to \"true\" and --root is given on the command\nline.\n\n> If so, perhaps the configuration parser should set the .fork_point\n> to (-1), so that \"[rebase] forkpoint = false\" that appears in your\n> ~/.gitconfig file can be countermanded with \"[rebase] forkpoint\"\n> that is placed in .git/config for one particular project that you do\n> not mind using the feature?\n\nThat's a great point. I will make that change and resubmit. Thanks!\n\n-Alex\n"},{"id":"414870","messageId":"xmqqturbdxi2.fsf@gitster.c.googlers.com","threadId":"55015","inReplyTo":"20210120044435.53509-1-alexhenrie24@gmail.com","subject":"Re: [PATCH] rebase: add a config option for --no-fork-point","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-20T23:00:53Z","receivedAt":"2021-01-21T00:48:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie writes:\n\n> @@ -1095,6 +1097,12 @@ static int rebase_config(const char *var, const char *value, void *data)\n> \t\treturn 0;\n> \t}\n> \n> +\tif (!strcmp(var, \"rebase.forkpoint\")) {\n> +\t\tif (!git_config_bool(var, value))\n> +\t\t\topts->fork_point = 0;\n> +\t\treturn 0;\n> +\t}\n\nIt is curious that this code seems to deliberatly ignore\n\n\t[rebase] forkpoint = true\n\nbut honors\n\n\t[rebase] forkpoint = false\n\nIntended? IOW, why isn't this just like this?\n\n\tif (!strcmp(var, \"rebase.forkpoint\")) {\n\t\topts->fork_point = git_config_bool(var, value);\n\t\treturn 0;\n\t}\n\n> @@ -1306,7 +1314,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \tconst char *gpg_sign = NULL;\n> \tstruct string_list exec = STRING_LIST_INIT_NODUP;\n> \tconst char *rebase_merges = NULL;\n> -\tint fork_point = -1;\n> \tstruct string_list strategy_options = STRING_LIST_INIT_NODUP;\n> \tstruct object_id squash_onto;\n> \tchar *squash_onto_name = NULL;\n> @@ -1406,7 +1413,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\tN_(\"mode\"),\n> \t\t\tN_(\"try to rebase merges instead of skipping them\"),\n> \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)\"\"},\n> -\t\tOPT_BOOL(0, \"fork-point\", &fork_point,\n> +\t\tOPT_BOOL(0, \"fork-point\", &options.fork_point,\n> \t\t\t N_(\"use 'merge-base --fork-point' to refine upstream\")),\n> \t\tOPT_STRING('s', \"strategy\", &options.strategy,\n> \t\t\t N_(\"strategy\"), N_(\"use the given merge strategy\")),\n> @@ -1494,7 +1501,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\tdie(_(\"cannot combine '--keep-base' with '--root'\"));\n> \t}\n> \n> -\tif (options.root && fork_point > 0)\n> +\tif (options.root && options.fork_point > 0)\n> \t\tdie(_(\"cannot combine '--root' with '--fork-point'\"));\n\nIs that because of this code?\n\nIf so, perhaps the configuration parser should set the .fork_point\nto (-1), so that \"[rebase] forkpoint = false\" that appears in your\n~/.gitconfig file can be countermanded with \"[rebase] forkpoint\"\nthat is placed in .git/config for one particular project that you do\nnot mind using the feature?\n\n> +test_expect_success '--fork-point and --root both given' '\n> +\ttest_must_fail git rebase --fork-point --root 2>err &&\n> +\ttest_i18ngrep \"cannot combine\" err\n> +'\n> +\n> +test_expect_success 'rebase.forkPoint true and --root given' '\n> +\ttest_config rebase.forkPoint true &&\n> +\tgit rebase --root\n> +'\n\ntest_expect_success 'rebase.forkPoint true overrides earlier false' '\n\ttest_config rebase.forkPoint false &&\n\ttest_config rebase.forkPoint true &&\n\t... check \"git rebase\" does use the fork-point feature here ...\n'\n\nThanks.\n"},{"id":"414933","messageId":"xmqqo8hiaptn.fsf@gitster.c.googlers.com","threadId":"55015","inReplyTo":"CAMMLpeQM7JCP0=TMqf0DxDCVJvkCGDEPSKN6vYTU_xc7+QaA1Q@mail.gmail.com","subject":"Re: [PATCH] rebase: add a config option for --no-fork-point","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-21T22:27:16Z","receivedAt":"2021-01-21T22:28:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie writes:\n\n>> > - if (options.root && fork_point > 0)\n>> > + if (options.root && options.fork_point > 0)\n>> > die(_(\"cannot combine '--root' with '--fork-point'\"));\n>>\n>> Is that because of this code?\n>\n> Yes. I was trying to avoid annoying the user with errors when\n> rebase.forkPoint is set to \"true\" and --root is given on the command\n> line.\n>\n>> If so, perhaps the configuration parser should set the .fork_point\n>> to (-1), so that \"[rebase] forkpoint = false\" that appears in your\n>> ~/.gitconfig file can be countermanded with \"[rebase] forkpoint\"\n>> that is placed in .git/config for one particular project that you do\n>> not mind using the feature?\n>\n> That's a great point. I will make that change and resubmit. Thanks!\n\nBy the way, is the feature itself something we can test easily? I\nthink I saw the \"we must interact with --root sensibly\" test, but\nthe actual feature not being tested include\n\n - rebase.forkpoint set to false and then to true; does it use the\n fork-point feature (your patch as-is would have failed this test);\n\n - rebase.forkpoint set to false and command line says --fork-point;\n\n - rebase.forkpoint set to true and command line says --no-fork-point;\n\nI offhand do not know if it is easy to observe it the fork-point\nfeature gets triggered, but it seems that the difference in\nbehaviour is big enough for you to care, so I am hoping you may\nthink of a way.\n\nThanks.\n"},{"id":"414937","messageId":"CAMMLpeQ5ga_e5-YeVuArYBrui+ps7HWzNjWNQgeJK3sEjo2zhA@mail.gmail.com","threadId":"55015","inReplyTo":"xmqqo8hiaptn.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] rebase: add a config option for --no-fork-point","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2021-01-21T23:25:25Z","receivedAt":"2021-01-21T23:26:47Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Thu, Jan 21, 2021 at 3:27 PM Junio C Hamano wrote:\n>\n> By the way, is the feature itself something we can test easily? I\n> think I saw the \"we must interact with --root sensibly\" test, but\n> the actual feature not being tested include\n>\n> - rebase.forkpoint set to false and then to true; does it use the\n> fork-point feature (your patch as-is would have failed this test);\n>\n> - rebase.forkpoint set to false and command line says --fork-point;\n>\n> - rebase.forkpoint set to true and command line says --no-fork-point;\n>\n> I offhand do not know if it is easy to observe it the fork-point\n> feature gets triggered, but it seems that the difference in\n> behaviour is big enough for you to care, so I am hoping you may\n> think of a way.\n\nThis blog post describes the same problem that I had with fork-point:\nhttps://commaok.xyz/post/fork-point/\n\nI didn't see any fork-point tests in the current codebase, but I can\nwork on adding some. Do you want them in a separate patch (that would\nmake the most sense to me), or squashed into the rebase.forkPoint\npatch?\n\n-Alex\n"},{"id":"414941","messageId":"YAoSIkCecZU98QD2@generichostname","threadId":"55015","inReplyTo":"CAMMLpeQ5ga_e5-YeVuArYBrui+ps7HWzNjWNQgeJK3sEjo2zhA@mail.gmail.com","subject":"Re: [PATCH] rebase: add a config option for --no-fork-point","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2021-01-21T23:45:38Z","receivedAt":"2021-01-21T23:46:26Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Alex,\n\nOn Thu, Jan 21, 2021 at 04:25:25PM -0700, Alex Henrie wrote:\n> I didn't see any fork-point tests in the current codebase, but I can\n> work on adding some. Do you want them in a separate patch (that would\n> make the most sense to me), or squashed into the rebase.forkPoint\n> patch?\n\nThe fork-point tests should be in t3431.\n\n-Denton\n"},{"id":"414943","messageId":"CAMMLpeTDNLUz9UVnL3aVK=J7X50Sq8L-OVGaoEsT5AY6EQON-g@mail.gmail.com","threadId":"55015","inReplyTo":"YAoSIkCecZU98QD2@generichostname","subject":"Re: [PATCH] rebase: add a config option for --no-fork-point","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2021-01-21T23:51:52Z","receivedAt":"2021-01-21T23:53:08Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Thu, Jan 21, 2021 at 4:45 PM Denton Liu wrote:\n>\n> On Thu, Jan 21, 2021 at 04:25:25PM -0700, Alex Henrie wrote:\n> > I didn't see any fork-point tests in the current codebase, but I can\n> > work on adding some. Do you want them in a separate patch (that would\n> > make the most sense to me), or squashed into the rebase.forkPoint\n> > patch?\n>\n> The fork-point tests should be in t3431.\n\nDerp! For some reason I thought that t3431 only tested --fork-point\nwith an ambiguous refname and that the other tests were unrelated.\nMaybe we should make copies of some of those tests that set\nrebase.forkPoint instead of passing --fork-point or --no-fork-point on\nthe command line.\n\n-Alex\n"},{"id":"417373","messageId":"xmqq4ki9c5u9.fsf@gitster.g","threadId":"55015","inReplyTo":"CAMMLpeTDNLUz9UVnL3aVK=J7X50Sq8L-OVGaoEsT5AY6EQON-g@mail.gmail.com","subject":"Re: [PATCH] rebase: add a config option for --no-fork-point","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-18T23:29:50Z","receivedAt":"2021-02-18T23:30:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie writes:\n\n> On Thu, Jan 21, 2021 at 4:45 PM Denton Liu wrote:\n>>\n>> On Thu, Jan 21, 2021 at 04:25:25PM -0700, Alex Henrie wrote:\n>> > I didn't see any fork-point tests in the current codebase, but I can\n>> > work on adding some. Do you want them in a separate patch (that would\n>> > make the most sense to me), or squashed into the rebase.forkPoint\n>> > patch?\n>>\n>> The fork-point tests should be in t3431.\n>\n> Derp! For some reason I thought that t3431 only tested --fork-point\n> with an ambiguous refname and that the other tests were unrelated.\n> Maybe we should make copies of some of those tests that set\n> rebase.forkPoint instead of passing --fork-point or --no-fork-point on\n> the command line.\n\nPing.\n\nAny progress on this topic? It's been almost a month without seeing\nany activity.\n\nThanks.\n"},{"id":"417433","messageId":"CAMMLpeQDKtNb-i5tQgphceAgS+V+wcf66_Xf+EfENn3CHNg-1w@mail.gmail.com","threadId":"55015","inReplyTo":"xmqq4ki9c5u9.fsf@gitster.g","subject":"Re: [PATCH] rebase: add a config option for --no-fork-point","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2021-02-20T05:10:42Z","receivedAt":"2021-02-20T05:12:07Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Thu, Feb 18, 2021 at 4:29 PM Junio C Hamano wrote:\n>\n> Any progress on this topic? It's been almost a month without seeing\n> any activity.\n\nSorry for letting this slip. I just sent a v2 of the patch with the\nspecific tests that you requested on January 21.\n\n-Alex\n"}]}