{"thread":{"id":"55191","subject":"[PATCH v3] rebase: add a config option for --no-fork-point","startedAt":"2021-02-23T07:23:26Z","lastAt":"2021-02-24T20:39:15Z","messageCount":7,"participants":["Alex Henrie","Denton Liu","Junio C Hamano","Martin Ågren"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"417542","messageId":"20210223071840.44267-1-alexhenrie24@gmail.com","threadId":"55191","inReplyTo":null,"subject":"[PATCH v3] rebase: add a config option for --no-fork-point","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2021-02-23T07:18:40Z","receivedAt":"2021-02-23T07:23:26Z","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 <alexhenrie24@gmail.com>\n---\nv3: Avoid calling test_expect_success from test_expect_success\n---\n Documentation/config/rebase.txt |  3 ++\n builtin/rebase.c                | 20 +++++++-----\n t/t3431-rebase-fork-point.sh    | 55 +++++++++++++++++++++++++++------\n 3 files changed, 61 insertions(+), 17 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..de400f9a19 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,11 @@ 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\topts->fork_point = git_config_bool(var, value) ? -1 : 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 +1313,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 +1412,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 +1500,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 +1846,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 +1951,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 2dab893c75..4c98d99e7e 100755\n--- a/t/t3431-rebase-fork-point.sh\n+++ b/t/t3431-rebase-fork-point.sh\n@@ -29,19 +29,23 @@ test_expect_success setup '\n \ttest_commit G\n '\n \n+do_test_rebase () {\n+\texpected=\"$1\" &&\n+\tshift &&\n+\tgit checkout main &&\n+\tgit reset --hard E &&\n+\tgit checkout side &&\n+\tgit reset --hard G &&\n+\tgit rebase $* &&\n+\ttest_write_lines $expected >expect &&\n+\tgit log --pretty=%s >actual &&\n+\ttest_cmp expect actual\n+}\n+\n test_rebase () {\n \texpected=\"$1\" &&\n \tshift &&\n-\ttest_expect_success \"git rebase $*\" \"\n-\t\tgit checkout main &&\n-\t\tgit reset --hard E &&\n-\t\tgit checkout side &&\n-\t\tgit reset --hard G &&\n-\t\tgit rebase $* &&\n-\t\ttest_write_lines $expected >expect &&\n-\t\tgit log --pretty=%s >actual &&\n-\t\ttest_cmp expect actual\n-\t\"\n+\ttest_expect_success \"git rebase $*\" \"do_test_rebase '$expected' $*\"\n }\n \n test_rebase 'G F E D B A'\n@@ -77,4 +81,35 @@ 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 set to false' '\n+\ttest_config rebase.forkPoint false &&\n+\tdo_test_rebase \"G F C E D B A\"\n+'\n+\n+test_expect_success 'rebase.forkPoint set to false and then to true' '\n+\ttest_config_global rebase.forkPoint false &&\n+\ttest_config rebase.forkPoint true &&\n+\tdo_test_rebase \"G F E D B A\"\n+'\n+\n+test_expect_success 'rebase.forkPoint set to false and command line says --fork-point' '\n+\ttest_config rebase.forkPoint false &&\n+\tdo_test_rebase \"G F E D B A\" --fork-point\n+'\n+\n+test_expect_success 'rebase.forkPoint set to true and command line says --no-fork-point' '\n+\ttest_config rebase.forkPoint true &&\n+\tdo_test_rebase \"G F C E D B A\" --no-fork-point\n+'\n+\n+test_expect_success 'rebase.forkPoint set to true and --root given' '\n+\ttest_config rebase.forkPoint true &&\n+\tgit rebase --root\n+'\n+\n test_done\n-- \n2.30.1\n\n"},{"id":"417544","messageId":"YDSvhEBh8YeCblpa@generichostname","threadId":"55191","inReplyTo":"20210223071840.44267-1-alexhenrie24@gmail.com","subject":"Re: [PATCH v3] rebase: add a config option for --no-fork-point","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2021-02-23T07:32:20Z","receivedAt":"2021-02-23T07:33:34Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Alex,\n\nOn Tue, Feb 23, 2021 at 12:18:40AM -0700, Alex Henrie wrote:\n> @@ -77,4 +81,35 @@ 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 set to false' '\n> +\ttest_config rebase.forkPoint false &&\n> +\tdo_test_rebase \"G F C E D B A\"\n> +'\n> +\n> +test_expect_success 'rebase.forkPoint set to false and then to true' '\n> +\ttest_config_global rebase.forkPoint false &&\n> +\ttest_config rebase.forkPoint true &&\n> +\tdo_test_rebase \"G F E D B A\"\n> +'\n\nI don't think this test is quite necessary. In other parts of the code,\nwe've already tested that local configs have priority over global\nconfigs. We can assume that config machinery works so we don't need to\ntest it here.\n\nThanks,\nDenton\n\n> +\n> +test_expect_success 'rebase.forkPoint set to false and command line says --fork-point' '\n> +\ttest_config rebase.forkPoint false &&\n> +\tdo_test_rebase \"G F E D B A\" --fork-point\n> +'\n> +\n> +test_expect_success 'rebase.forkPoint set to true and command line says --no-fork-point' '\n> +\ttest_config rebase.forkPoint true &&\n> +\tdo_test_rebase \"G F C E D B A\" --no-fork-point\n> +'\n> +\n> +test_expect_success 'rebase.forkPoint set to true and --root given' '\n> +\ttest_config rebase.forkPoint true &&\n> +\tgit rebase --root\n> +'\n> +\n>  test_done\n> -- \n> 2.30.1\n> \n"},{"id":"417550","messageId":"xmqqo8gb42kb.fsf@gitster.g","threadId":"55191","inReplyTo":"YDSvhEBh8YeCblpa@generichostname","subject":"Re: [PATCH v3] rebase: add a config option for --no-fork-point","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-23T08:21:24Z","receivedAt":"2021-02-23T08:22:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> I don't think this test is quite necessary. In other parts of the\n> code, we've already tested that local configs have priority over\n> global configs. We can assume that config machinery works so we\n> don't need to test it here.\n\nI am not so sure if we should place so much trust in the code of\n\"rebase\", or any other subsystem for that matter, that it uses the\nconfiguration API and parse-options API correctly.\n\nThe do_test_rebase helper introduced here would be useful clean-up\non its own, so I'd rather take it as-is.\n"},{"id":"417713","messageId":"CAN0heSqmAgt_hg0-kQpTK2LcuYPV9T9=1bowbuZ7Qah1q0EAtA@mail.gmail.com","threadId":"55191","inReplyTo":"20210223071840.44267-1-alexhenrie24@gmail.com","subject":"Re: [PATCH v3] rebase: add a config option for --no-fork-point","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-02-24T19:33:43Z","receivedAt":"2021-02-24T19:34:51Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 23 Feb 2021 at 08:24, Alex Henrie <alexhenrie24@gmail.com> wrote:\n> +rebase.forkPoint:\n> +       If set to false set `--no-fork-point` option by default.\n\nThis should be a double-colon at end of the line, not just a single\ncolon, in order to make it a \"description list separator\". When it's\njust \":\", it ends up being rendered literally, which isn't horrible, to\nbe sure, but which doesn't match this item's neighbours.\n\nMartin\n"},{"id":"417714","messageId":"CAN0heSp9ApYCr8RK5xF=_q2WJgE1RjUpZ3dexQuOwQFi8NLS9Q@mail.gmail.com","threadId":"55191","inReplyTo":"CAN0heSqmAgt_hg0-kQpTK2LcuYPV9T9=1bowbuZ7Qah1q0EAtA@mail.gmail.com","subject":"Re: [PATCH v3] rebase: add a config option for --no-fork-point","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-02-24T19:38:56Z","receivedAt":"2021-02-24T19:39:55Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 24 Feb 2021 at 20:33, Martin Ågren <martin.agren@gmail.com> wrote:\n\n> just \":\", it ends up being rendered literally, which isn't horrible, to\n\nHmm, I sort of take that back. In git-config.1, it looks not-too-bad,\nbut in git-rebase.1, this item runs into the next one (sequence.editor)\nand messes with it, like so:\n\n  rebase.forkPoint: If set to false set --no-fork-point option by\n  default. sequence.editor:: Text editor used by git rebase -i for\n  editing the rebase instruction file. The value is meant to be\n  interpreted by the shell when it is used. It can be overridden by the\n  GIT_SEQUENCE_EDITOR environment variable. When not configured the\n  default commit message editor is used instead.\n\nMartin\n"},{"id":"417716","messageId":"xmqqo8g9z1p2.fsf@gitster.g","threadId":"55191","inReplyTo":"CAN0heSqmAgt_hg0-kQpTK2LcuYPV9T9=1bowbuZ7Qah1q0EAtA@mail.gmail.com","subject":"Re: [PATCH v3] rebase: add a config option for --no-fork-point","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-24T19:48:57Z","receivedAt":"2021-02-24T19:49:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> On Tue, 23 Feb 2021 at 08:24, Alex Henrie <alexhenrie24@gmail.com> wrote:\n>> +rebase.forkPoint:\n>> +       If set to false set `--no-fork-point` option by default.\n>\n> This should be a double-colon at end of the line, not just a single\n> colon, in order to make it a \"description list separator\". When it's\n> just \":\", it ends up being rendered literally, which isn't horrible, to\n> be sure, but which doesn't match this item's neighbours.\n>\n> Martin\n\nThanks for your sharp eyes; will amend locally before merging it to\n'next'.\n"},{"id":"417759","messageId":"CAMMLpeT2uocoWQMoNYoVWa2-scSohwbvz-CFN3zupD7jEBEFew@mail.gmail.com","threadId":"55191","inReplyTo":"xmqqo8g9z1p2.fsf@gitster.g","subject":"Re: [PATCH v3] rebase: add a config option for --no-fork-point","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2021-02-24T20:38:15Z","receivedAt":"2021-02-24T20:39:15Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Wed, Feb 24, 2021 at 12:49 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Martin Ågren <martin.agren@gmail.com> writes:\n>\n> > On Tue, 23 Feb 2021 at 08:24, Alex Henrie <alexhenrie24@gmail.com> wrote:\n> >> +rebase.forkPoint:\n> >> +       If set to false set `--no-fork-point` option by default.\n> >\n> > This should be a double-colon at end of the line, not just a single\n> > colon, in order to make it a \"description list separator\". When it's\n> > just \":\", it ends up being rendered literally, which isn't horrible, to\n> > be sure, but which doesn't match this item's neighbours.\n> >\n> > Martin\n>\n> Thanks for your sharp eyes; will amend locally before merging it to\n> 'next'.\n\nAgreed, thank you!\n\n-Alex\n"}]}