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

[Junio C Hamano <gitster@pobox.com>]

Mr Bill <billc56196@gmail.com> writes:

Welcome to the community and thanks for a patch.

>> 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.