From: Mr Bill Date: Fri, 16 May 2025 20:20:32 GMT Subject: Re: [PATCH] Handle rebase fork-point options in pull --rebase Message-ID: <106d9ce5-2c2f-9724-792a-b7ea46641c8b@gmail.com> In-Reply-To: On 05/15/2025 08:57 AM, Junio C Hamano wrote: > Mr Bill 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. >