Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint

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

Wesley Schwengle <wesleys@opperschaap.net> writes:

> The behaviour of `git rebase' was that if you supply an upstream on the
> command line that it behaves as if `--no-forkpoint' was supplied and if
> you don't supply an upstream, it behaves as if `--forkpoint' was
> supplied.

I actually think it is a reasonable, if a bit too clever (for my
taste at least), default for those who do not want to type the
"--fork-point" option from the command line and still want to use
that option when they are pulling from or rebasing on the source
they usually interact with, while still allowing them to be precise
when they do want to specify exactly what commit they want to base
it on.

And the way how you tell if they are using the "usual" source is to
see if they used the lazy "git rebase" (without arguments) form.  So
I do not think it is particularly a bad design to allow "git rebase
master" and "git rebase" to behave differently.  The latter may use
the "fork point computed using 'master' branch" (when the current
branch is configured to rebuild on top of 'master') while the former
may use "exactly the commit pointed at by the 'master' branch".

> This can result in a loss of commits if you don't know that
> and if you don't know about `git reflog' or have other copies of your
> changes.

Surely, but you would lose commits if you don't know these things
and explicitly gave the --fork-point option the same way.  So I am
not sure if switching of the default is warranted.

> -			if (options.fork_point < 0)
> +			if (options.fork_point < 0) {
> +				warning(_(
> +					"Rebasing without specifying a forkpoint is discouraged. You can squelch\n"
> +					"this message by running one of the following commands something before your\n"
> +					"next rebase:\n"
> +					"\n"
> +					"  git config rebase.forkpoint = false # This will become the new default\n"
> +					"  git config rebase.forkpoint = true  # This is the old default\n"
> +					"\n"

The message "Rebasing without specifying a forkpoint" reads as if
you are encouraging the use of forkpoint mode (which you are not, I
know), but then what the message advertises as a future default
stops not make sense.  "If we hate the forkpoint mode so much to
disable it by default, why so we discourage running the command
without specifying it?" would be the confused message the users will
read from it.

Your "git config" example command lines are not correct, are they?
There should be no '=' assignment operator.

I am also afraid that this is giving a way too broad an advice.

What you want to discourage is to rebase without specifying what to
rebase on and without saying if you want or you do not want the
forkpoint behaviour, which will opt the user into the more dangerous
forkpoint behaviour.  The above makes it sound as if we will
discourage even the more precise "git rebase <newbase>" form, but I
do not think it is the case.  We would and should not trigger the
folk-point behaviour if there is an explicit <upstream> and the user
does not say "--fork-point" from the command line.

Here is my attempt to rewrite the above:

    When 'git rebase' is run without specifying <upstream> on the
    command line, the current default is to use the fork-point
    heuristics, but this is expected to change in a future version
    of Git, and you will have to explicitly give "--fork-point" from
    the command line if you keep using the fork-point mode.  You can
    run "git config rebase.forkpoint false" to adopt the new default
    in advance and that will also squelch the message.

Note that the parsing of "rebase.forkpoint" is a bit peculiar in
that 

 - By leaving it unspecified, the .fork_point = -1 in
   REBASE_OPTIONS_INIT takes effect (which is unsurprising);

 - By setting it to false, .fork_point becomes 0; but

 - If you set the configuration variable to true, .fork_point
   becomes -1, not 1.

And this is very much deliberate if I understand it correctly [*1*].
By the time we get to this part of the code (i.e. .fork_point is
-1), the user may already have rebase.forkpoint set to true.  IOW,
setting it to 'true' is not a valid way to squelch this message.

I am not commenting on the tests, as the above code probably needs
to be corrected first so that folks who want to squelch the message
and want the "forkpoint behaviour by default when rebuilding on the
usual upstream" behaviour can do so by setting the variable to true.

And that obviously need to be tested, too.

Thanks.


[References]

*1* https://lore.kernel.org/git/xmqqturbdxi2.fsf@gitster.c.googlers.com/