[PATCH v2] Emit warning when rebasing without a forkpoint

From: Wesley Schwengle <wesleys@opperschaap.net>
Date: Sep 2, 2023, 22:16 UTC
Message-ID: <20230902221641.1399624-1-wesleys@opperschaap.net>
In-Reply-To: <xmqq1qfiubg5.fsf@gitster.g>

This is the second version of the patch series.

Patch 1: Be able to use rebase.forkpoint and --root Patch 2: Adding the warning + tests Patch 3: Update documenation

I think I have covered most of your concerns and feedback in this second version.

On 8/31/23 16:57, Junio C Hamano wrote:

Show 11 quoted lines

> Wesley Schwengle <wesleys@opperschaap.net> writes:
> 
> 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.

I agree. I'll change the text to your version.

Show 10 quoted lines

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

I changed this in patch 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.

So this works now with patch 2.

Show 5 quoted lines

> Another worrysome thing about rebase.forkpoint is that it will be
> inevitable for folks to start complaining that it does not work the
> way other configuration variables do.  Setting the variable to
> 'true' is not the same as passing '--fork-point=true' from the
> command line.

I think it is now with the current series.

Show 13 quoted lines

> I actually think it would be a lot larger behaviour change with a
> huge potential to be received as a regression if we start making the
> variable to mean the same thing as passing '--fork-point=true'.
> People may like the current "if you are rebuilding your branch on
> its usual upstream, pay attention to the rebase and rewind of the
> upstream itself, but if you are giving an explicit upstream from the
> command line, the tool does not second guess you with the fork-point
> heuristics" behaviour and prefer to set it to true.  We would be
> breaking them big time if suddenly the rebase.forkpoint=true they
> set previously starts triggering the fork-point heuristics when they
> run "git rebase upstream".  So that needs to be kept in mind when/if
> we fix the "setting the variable, even to 'true', will squelch the
> warning".

I get what you are saying. My solution is to make the --fork-point or --no-fork-point more explicit. People could use an alias for this?

It would mean a different approach to the problem and deprecating rebase.forkpoint as a boolean value. It could become one of three values: "true", "false" and "legacy". Where "legacy" can be "implicit" or "auto". Although you had some ideas on "auto" already. I'm not sure on how I would call it. "no-upstream"?

-- 
Wesley

Why not both?