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

[Mr Bill <billc56196@xxxxxxxxx>]

On 05/15/2025 08:57 AM, Junio C Hamano wrote:
> Mr Bill <billc56196@xxxxxxxxx> 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.