{"thread":{"id":"63458","subject":"[PATCH] Handle rebase fork-point options in pull --rebase","startedAt":"2025-05-14T17:37:30Z","lastAt":"2025-05-16T20:20:35Z","messageCount":3,"participants":["Mr Bill","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"518059","messageId":"06beff46-cdaf-91c8-e6a3-6557694af618@gmail.com","threadId":"63458","inReplyTo":null,"subject":"[PATCH] Handle rebase fork-point options in pull --rebase","fromName":"Mr Bill","fromEmail":"billc56196@gmail.com","sentAt":"2025-05-14T17:37:26Z","receivedAt":"2025-05-14T17:37:30Z","isPatch":true,"sender":{"key":"billc56196@gmail.com","avatar":null},"body":"\nThis is a patch to handle --fork-point and --no-fork-point in pull --rebase.\n\nI had a recent bug report about pull --rebase not working correctly...\n\nbut it was working correctly, but not doing what I expected due to always\n\nusing \"merge-base --fork-point\"\n\nThis patch implements handling the --fork-point and --no-fork-point options,\n\nand also checks the config rebase.forkpoint value...\n\nand it works to resolve my prior bug report issue.\n\nIf there are any questions or comments, let me know!\n\nThanks to all for the help and comments on my prior bug report!\n\n-Bill\n\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex a1ebc6a..f2d405f 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -117,6 +117,10 @@ static int opt_show_forced_updates = -1;\n  static const char *set_upstream;\n  static struct strvec opt_fetch = STRVEC_INIT;\n\n+/* options to include rebase fork-point preference */\n+static int config_fork_point = -1;\n+static int opt_fork_point = -1;\n+\n  static struct option pull_options[] = {\n     /* Shared options */\n     OPT__VERBOSITY(&opt_verbosity),\n@@ -253,6 +257,10 @@ static struct option pull_options[] = {\n         N_(\"set upstream for git pull/fetch\"),\n         PARSE_OPT_NOARG),\n\n+   /* rebase option to use/not use merge-base --fork-point */\n+   OPT_BOOL(0, \"fork-point\", &opt_fork_point,\n+       N_(\"rebase with 'merge-base --fork-point' to refine upstream\")),\n+\n     OPT_END()\n  };\n\n@@ -366,6 +374,9 @@ static int git_pull_config(const char *var, const \nchar *value,\n     if (!strcmp(var, \"rebase.autostash\")) {\n         config_autostash = git_config_bool(var, value);\n         return 0;\n+   } else if (!strcmp(var, \"rebase.forkpoint\")) {\n+       config_fork_point = git_config_bool(var, value) ? -1 : 0;\n+       return 0;\n     } else if (!strcmp(var, \"submodule.recurse\")) {\n         recurse_submodules = git_config_bool(var, value) ?\n             RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;\n@@ -1059,7 +1070,17 @@ int cmd_pull(int argc,\n                 N_(\"pull with rebase\"),\n                 _(\"Please commit or stash them.\"), 1, 0);\n\n-       if (get_rebase_fork_point(&rebase_fork_point, repo, *refspecs))\n+       if (opt_fork_point == -1)\n+           opt_fork_point = config_fork_point;\n+       if (opt_fork_point < 0)\n+           opt_fork_point = 1;\n+       fprintf_ln(stderr, _(\"rebasing %s fork-point\"), (opt_fork_point \n? \"with\" : \"without\"));\n+\n+       /*\n+        * If we're *not* using fork-point, or we don't find one in \nget_rebase_fork_point(),\n+        * clear the rebase_fork_point info.\n+        */\n+       if (!opt_fork_point || get_rebase_fork_point(&rebase_fork_point, \nrepo, *refspecs))\n             oidclr(&rebase_fork_point, the_repository->hash_algo);\n     }\n\n\n"},{"id":"518138","messageId":"xmqq5xi23riw.fsf@gitster.g","threadId":"63458","inReplyTo":"06beff46-cdaf-91c8-e6a3-6557694af618@gmail.com","subject":"Re: [PATCH] Handle rebase fork-point options in pull --rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-15T13:57:59Z","receivedAt":"2025-05-15T13:58:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mr Bill <billc56196@gmail.com> writes:\n\nWelcome to the community and thanks for a patch.\n\n>> Content-Type: text/plain; charset=utf-8; format=flowed\n\nThat is a sign that the patch itself cannot be used mechanically, as\nformat=flawed is known to corrupt patches.  But still, let's read on.\n\n> This is a patch to handle --fork-point and --no-fork-point in pull --rebase.\n>\n> I had a recent bug report about pull --rebase not working correctly...\n>\n> but it was working correctly, but not doing what I expected due to always\n>\n> using \"merge-base --fork-point\"\n>\n> This patch implements handling the --fork-point and --no-fork-point options,\n>\n> and also checks the config rebase.forkpoint value...\n>\n> and it works to resolve my prior bug report issue.\n>\n> If there are any questions or comments, let me know!\n>\n> Thanks to all for the help and comments on my prior bug report!\n>\n> -Bill\n\nThe above is not quite usable as log message.\n\nThe usual way to compose a log message of this project is to\n\n - Give an observation on how the current system works in the present\n   tense (so no need to say \"Currently X is Y\", just \"X is Y\"), and\n   discuss what you perceive as a problem in it.\n\n - Propose a solution (optional---often, problem description\n   trivially leads to an obvious solution in reader's minds).\n\n - Give commands to the codebase to \"become like so\".\n\nin this order.  You may want to check a few examples in \"git log\n--no-merges master..seen\" to get yourself familialized to the style.\n\nAlso check Documentation/SubmittingPatches; you'd need to sign off\nyour patch with your real name, which should match the authorship\nidentity (i.e. \"From: \" line of your message and \"Signed-off-by: \"\ntrailer should both have your real name plus e-mail address).\n\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index a1ebc6a..f2d405f 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -117,6 +117,10 @@ static int opt_show_forced_updates = -1;\n>  static const char *set_upstream;\n>  static struct strvec opt_fetch = STRVEC_INIT;\n>\n> +/* options to include rebase fork-point preference */\n> +static int config_fork_point = -1;\n> +static int opt_fork_point = -1;\n> +\n>  static struct option pull_options[] = {\n>     /* Shared options */\n>     OPT__VERBOSITY(&opt_verbosity),\n\nAs I already said, the patch part is all whitespace damaged and this\npatch will not be usable as-is, but let's see if the logic is sound.\n\n> @@ -253,6 +257,10 @@ static struct option pull_options[] = {\n>         N_(\"set upstream for git pull/fetch\"),\n>         PARSE_OPT_NOARG),\n>\n> +   /* rebase option to use/not use merge-base --fork-point */\n> +   OPT_BOOL(0, \"fork-point\", &opt_fork_point,\n> +       N_(\"rebase with 'merge-base --fork-point' to refine upstream\")),\n> +\n>     OPT_END()\n>  };\n>\n> @@ -366,6 +374,9 @@ static int git_pull_config(const char *var, const\n> char *value,\n>     if (!strcmp(var, \"rebase.autostash\")) {\n>         config_autostash = git_config_bool(var, value);\n>         return 0;\n> +   } else if (!strcmp(var, \"rebase.forkpoint\")) {\n> +       config_fork_point = git_config_bool(var, value) ? -1 : 0;\n> +       return 0;\n\nThis is curious.  I would have expected it to return the value\nreturned by git_config_bool() as-is, because \"-1\" is used as\n\"unspecified\" to initialize the config_fork_point variable.  With\nthis code, configuring \"[rebase] forkpoint = true\" is a no-op, no?\n\nAssuming that it is fixed ...\n\n> @@ -1059,7 +1070,17 @@ int cmd_pull(int argc,\n>                 N_(\"pull with rebase\"),\n>                 _(\"Please commit or stash them.\"), 1, 0);\n>\n> +       if (opt_fork_point == -1)\n> +           opt_fork_point = config_fork_point;\n> +       if (opt_fork_point < 0)\n> +           opt_fork_point = 1;\n\n... this code looks reasonable.  opt_ and config_ are both\ninitialized to \"-1\" (unspecified), so if there is no command line\noption given to affect the setting, we read from config_, and if\nneither specifies the settings, we enable the fork-point option.\n\n> +       fprintf_ln(stderr, _(\"rebasing %s fork-point\"),\n> (opt_fork_point ? \"with\" : \"without\"));\n\nThis looks like a debugging aid, not to be left for production.\n\nBesides, interpolating literal \"with\" in _(\"localizable string\") \ndoes not make much sense.\n\n> +       /*\n> +        * If we're *not* using fork-point, or we don't find one in\n> get_rebase_fork_point(),\n> +        * clear the rebase_fork_point info.\n> +        */\n\n\n> -       if (get_rebase_fork_point(&rebase_fork_point, repo, *refspecs))\n> +       if (!opt_fork_point ||\n> get_rebase_fork_point(&rebase_fork_point, repo, *refspecs))\n>             oidclr(&rebase_fork_point, the_repository->hash_algo);\n\nDoubly besides, until we pass this point, we do not know if we are\nrebasing with fork-point.  The configuraiton, option, or the\nhardcoded default might have made opt_fork_point to true, but if\nget_rebase_fork_point() failed, we won't be rebasing with fork-point\nso the above debugging aid message we saw earlier, even if it were\nuseful in production, is given way too early before we know enough\nto choose between \"with\" or \"without\".\n\nThis new feature needs to have tests.\n\nThe new command line option needs documentation.\n\nThe new configuration variable needs documentation.\n\nThanks.\n"},{"id":"518309","messageId":"106d9ce5-2c2f-9724-792a-b7ea46641c8b@gmail.com","threadId":"63458","inReplyTo":"xmqq5xi23riw.fsf@gitster.g","subject":"Re: [PATCH] Handle rebase fork-point options in pull --rebase","fromName":"Mr Bill","fromEmail":"billc56196@gmail.com","sentAt":"2025-05-16T20:20:32Z","receivedAt":"2025-05-16T20:20:35Z","isPatch":true,"sender":{"key":"billc56196@gmail.com","avatar":null},"body":"On 05/15/2025 08:57 AM, Junio C Hamano wrote:\n> Mr Bill <billc56196@gmail.com> writes:\n>\n> Welcome to the community and thanks for a patch.\n\nHi Junio,\n\nThank you for the review and suggestions. I'll look into making\na more complete patch; however, I'm working on a major system\ncleanup and upgrade which may delay this until complete.\n\nThanks,\nBill\n\n>\n>>> Content-Type: text/plain; charset=utf-8; format=flowed\n> That is a sign that the patch itself cannot be used mechanically, as\n> format=flawed is known to corrupt patches.  But still, let's read on.\n>\n>> This is a patch to handle --fork-point and --no-fork-point in pull --rebase.\n>>\n>> I had a recent bug report about pull --rebase not working correctly...\n>>\n>> but it was working correctly, but not doing what I expected due to always\n>>\n>> using \"merge-base --fork-point\"\n>>\n>> This patch implements handling the --fork-point and --no-fork-point options,\n>>\n>> and also checks the config rebase.forkpoint value...\n>>\n>> and it works to resolve my prior bug report issue.\n>>\n>> If there are any questions or comments, let me know!\n>>\n>> Thanks to all for the help and comments on my prior bug report!\n>>\n>> -Bill\n> The above is not quite usable as log message.\n>\n> The usual way to compose a log message of this project is to\n>\n>   - Give an observation on how the current system works in the present\n>     tense (so no need to say \"Currently X is Y\", just \"X is Y\"), and\n>     discuss what you perceive as a problem in it.\n>\n>   - Propose a solution (optional---often, problem description\n>     trivially leads to an obvious solution in reader's minds).\n>\n>   - Give commands to the codebase to \"become like so\".\n>\n> in this order.  You may want to check a few examples in \"git log\n> --no-merges master..seen\" to get yourself familialized to the style.\n>\n> Also check Documentation/SubmittingPatches; you'd need to sign off\n> your patch with your real name, which should match the authorship\n> identity (i.e. \"From: \" line of your message and \"Signed-off-by: \"\n> trailer should both have your real name plus e-mail address).\n>\n>> diff --git a/builtin/pull.c b/builtin/pull.c\n>> index a1ebc6a..f2d405f 100644\n>> --- a/builtin/pull.c\n>> +++ b/builtin/pull.c\n>> @@ -117,6 +117,10 @@ static int opt_show_forced_updates = -1;\n>>   static const char *set_upstream;\n>>   static struct strvec opt_fetch = STRVEC_INIT;\n>>\n>> +/* options to include rebase fork-point preference */\n>> +static int config_fork_point = -1;\n>> +static int opt_fork_point = -1;\n>> +\n>>   static struct option pull_options[] = {\n>>      /* Shared options */\n>>      OPT__VERBOSITY(&opt_verbosity),\n> As I already said, the patch part is all whitespace damaged and this\n> patch will not be usable as-is, but let's see if the logic is sound.\n>\n>> @@ -253,6 +257,10 @@ static struct option pull_options[] = {\n>>          N_(\"set upstream for git pull/fetch\"),\n>>          PARSE_OPT_NOARG),\n>>\n>> +   /* rebase option to use/not use merge-base --fork-point */\n>> +   OPT_BOOL(0, \"fork-point\", &opt_fork_point,\n>> +       N_(\"rebase with 'merge-base --fork-point' to refine upstream\")),\n>> +\n>>      OPT_END()\n>>   };\n>>\n>> @@ -366,6 +374,9 @@ static int git_pull_config(const char *var, const\n>> char *value,\n>>      if (!strcmp(var, \"rebase.autostash\")) {\n>>          config_autostash = git_config_bool(var, value);\n>>          return 0;\n>> +   } else if (!strcmp(var, \"rebase.forkpoint\")) {\n>> +       config_fork_point = git_config_bool(var, value) ? -1 : 0;\n>> +       return 0;\n> This is curious.  I would have expected it to return the value\n> returned by git_config_bool() as-is, because \"-1\" is used as\n> \"unspecified\" to initialize the config_fork_point variable.  With\n> this code, configuring \"[rebase] forkpoint = true\" is a no-op, no?\n>\n> Assuming that it is fixed ...\n>\n>> @@ -1059,7 +1070,17 @@ int cmd_pull(int argc,\n>>                  N_(\"pull with rebase\"),\n>>                  _(\"Please commit or stash them.\"), 1, 0);\n>>\n>> +       if (opt_fork_point == -1)\n>> +           opt_fork_point = config_fork_point;\n>> +       if (opt_fork_point < 0)\n>> +           opt_fork_point = 1;\n> ... this code looks reasonable.  opt_ and config_ are both\n> initialized to \"-1\" (unspecified), so if there is no command line\n> option given to affect the setting, we read from config_, and if\n> neither specifies the settings, we enable the fork-point option.\n>\n>> +       fprintf_ln(stderr, _(\"rebasing %s fork-point\"),\n>> (opt_fork_point ? \"with\" : \"without\"));\n> This looks like a debugging aid, not to be left for production.\n>\n> Besides, interpolating literal \"with\" in _(\"localizable string\")\n> does not make much sense.\n>\n>> +       /*\n>> +        * If we're *not* using fork-point, or we don't find one in\n>> get_rebase_fork_point(),\n>> +        * clear the rebase_fork_point info.\n>> +        */\n>\n>> -       if (get_rebase_fork_point(&rebase_fork_point, repo, *refspecs))\n>> +       if (!opt_fork_point ||\n>> get_rebase_fork_point(&rebase_fork_point, repo, *refspecs))\n>>              oidclr(&rebase_fork_point, the_repository->hash_algo);\n> Doubly besides, until we pass this point, we do not know if we are\n> rebasing with fork-point.  The configuraiton, option, or the\n> hardcoded default might have made opt_fork_point to true, but if\n> get_rebase_fork_point() failed, we won't be rebasing with fork-point\n> so the above debugging aid message we saw earlier, even if it were\n> useful in production, is given way too early before we know enough\n> to choose between \"with\" or \"without\".\n>\n> This new feature needs to have tests.\n>\n> The new command line option needs documentation.\n>\n> The new configuration variable needs documentation.\n>\n> Thanks.\n>\n\n"}]}