Re: [PATCH] Handle rebase fork-point options in pull --rebase

[Mr Bill <billc56196@gmail.com>]

On 05/15/2025 08:57 AM, Junio C Hamano wrote:
> Mr Bill <billc56196@gmail.com> writes:
>
> Welcome to the community and thanks for a patch.

Hi Junio,

Thank you for the review and suggestions. I'll look into making
a more complete patch; however, I'm working on a major system
cleanup and upgrade which may delay this until complete.

Thanks,
Bill

>
>>> Content-Type: text/plain; charset=utf-8; format=flowed
> That is a sign that the patch itself cannot be used mechanically, as
> format=flawed is known to corrupt patches.  But still, let's read on.
>
>> This is a patch to handle --fork-point and --no-fork-point in pull --rebase.
>>
>> I had a recent bug report about pull --rebase not working correctly...
>>
>> but it was working correctly, but not doing what I expected due to always
>>
>> using "merge-base --fork-point"
>>
>> This patch implements handling the --fork-point and --no-fork-point options,
>>
>> and also checks the config rebase.forkpoint value...
>>
>> and it works to resolve my prior bug report issue.
>>
>> If there are any questions or comments, let me know!
>>
>> Thanks to all for the help and comments on my prior bug report!
>>
>> -Bill
> The above is not quite usable as log message.
>
> The usual way to compose a log message of this project is to
>
>   - Give an observation on how the current system works in the present
>     tense (so no need to say "Currently X is Y", just "X is Y"), and
>     discuss what you perceive as a problem in it.
>
>   - Propose a solution (optional---often, problem description
>     trivially leads to an obvious solution in reader's minds).
>
>   - Give commands to the codebase to "become like so".
>
> in this order.  You may want to check a few examples in "git log
> --no-merges master..seen" to get yourself familialized to the style.
>
> Also check Documentation/SubmittingPatches; you'd need to sign off
> your patch with your real name, which should match the authorship
> identity (i.e. "From: " line of your message and "Signed-off-by: "
> trailer should both have your real name plus e-mail address).
>
>> diff --git a/builtin/pull.c b/builtin/pull.c
>> index a1ebc6a..f2d405f 100644
>> --- a/builtin/pull.c
>> +++ b/builtin/pull.c
>> @@ -117,6 +117,10 @@ static int opt_show_forced_updates = -1;
>>   static const char *set_upstream;
>>   static struct strvec opt_fetch = STRVEC_INIT;
>>
>> +/* options to include rebase fork-point preference */
>> +static int config_fork_point = -1;
>> +static int opt_fork_point = -1;
>> +
>>   static struct option pull_options[] = {
>>      /* Shared options */
>>      OPT__VERBOSITY(&opt_verbosity),
> As I already said, the patch part is all whitespace damaged and this
> patch will not be usable as-is, but let's see if the logic is sound.
>
>> @@ -253,6 +257,10 @@ static struct option pull_options[] = {
>>          N_("set upstream for git pull/fetch"),
>>          PARSE_OPT_NOARG),
>>
>> +   /* rebase option to use/not use merge-base --fork-point */
>> +   OPT_BOOL(0, "fork-point", &opt_fork_point,
>> +       N_("rebase with 'merge-base --fork-point' to refine upstream")),
>> +
>>      OPT_END()
>>   };
>>
>> @@ -366,6 +374,9 @@ static int git_pull_config(const char *var, const
>> char *value,
>>      if (!strcmp(var, "rebase.autostash")) {
>>          config_autostash = git_config_bool(var, value);
>>          return 0;
>> +   } else if (!strcmp(var, "rebase.forkpoint")) {
>> +       config_fork_point = git_config_bool(var, value) ? -1 : 0;
>> +       return 0;
> This is curious.  I would have expected it to return the value
> returned by git_config_bool() as-is, because "-1" is used as
> "unspecified" to initialize the config_fork_point variable.  With
> this code, configuring "[rebase] forkpoint = true" is a no-op, no?
>
> Assuming that it is fixed ...
>
>> @@ -1059,7 +1070,17 @@ int cmd_pull(int argc,
>>                  N_("pull with rebase"),
>>                  _("Please commit or stash them."), 1, 0);
>>
>> +       if (opt_fork_point == -1)
>> +           opt_fork_point = config_fork_point;
>> +       if (opt_fork_point < 0)
>> +           opt_fork_point = 1;
> ... this code looks reasonable.  opt_ and config_ are both
> initialized to "-1" (unspecified), so if there is no command line
> option given to affect the setting, we read from config_, and if
> neither specifies the settings, we enable the fork-point option.
>
>> +       fprintf_ln(stderr, _("rebasing %s fork-point"),
>> (opt_fork_point ? "with" : "without"));
> This looks like a debugging aid, not to be left for production.
>
> Besides, interpolating literal "with" in _("localizable string")
> does not make much sense.
>
>> +       /*
>> +        * If we're *not* using fork-point, or we don't find one in
>> get_rebase_fork_point(),
>> +        * clear the rebase_fork_point info.
>> +        */
>
>> -       if (get_rebase_fork_point(&rebase_fork_point, repo, *refspecs))
>> +       if (!opt_fork_point ||
>> get_rebase_fork_point(&rebase_fork_point, repo, *refspecs))
>>              oidclr(&rebase_fork_point, the_repository->hash_algo);
> Doubly besides, until we pass this point, we do not know if we are
> rebasing with fork-point.  The configuraiton, option, or the
> hardcoded default might have made opt_fork_point to true, but if
> get_rebase_fork_point() failed, we won't be rebasing with fork-point
> so the above debugging aid message we saw earlier, even if it were
> useful in production, is given way too early before we know enough
> to choose between "with" or "without".
>
> This new feature needs to have tests.
>
> The new command line option needs documentation.
>
> The new configuration variable needs documentation.
>
> Thanks.
>