Re: [PATCH] rebase: add a config option for --no-fork-point

Alex Henrie [off-list ref] writes:

quoted

quoted

-     if (options.root && fork_point > 0)
+     if (options.root && options.fork_point > 0)
              die(_("cannot combine '--root' with '--fork-point'"));

Is that because of this code?

Yes. I was trying to avoid annoying the user with errors when
rebase.forkPoint is set to "true" and --root is given on the command
line.

quoted

If so, perhaps the configuration parser should set the .fork_point
to (-1), so that "[rebase] forkpoint = false" that appears in your
~/.gitconfig file can be countermanded with "[rebase] forkpoint"
that is placed in .git/config for one particular project that you do
not mind using the feature?

That's a great point. I will make that change and resubmit. Thanks!

By the way, is the feature itself something we can test easily?  I
think I saw the "we must interact with --root sensibly" test, but
the actual feature not being tested include

 - rebase.forkpoint set to false and then to true; does it use the
   fork-point feature (your patch as-is would have failed this test);

 - rebase.forkpoint set to false and command line says --fork-point;

 - rebase.forkpoint set to true and command line says --no-fork-point;

I offhand do not know if it is easy to observe it the fork-point
feature gets triggered, but it seems that the difference in
behaviour is big enough for you to care, so I am hoping you may
think of a way.

Thanks.

On Thu, Jan 21, 2021 at 3:27 PM Junio C Hamano [off-list ref] wrote:

By the way, is the feature itself something we can test easily?  I
think I saw the "we must interact with --root sensibly" test, but
the actual feature not being tested include

 - rebase.forkpoint set to false and then to true; does it use the
   fork-point feature (your patch as-is would have failed this test);

 - rebase.forkpoint set to false and command line says --fork-point;

 - rebase.forkpoint set to true and command line says --no-fork-point;

I offhand do not know if it is easy to observe it the fork-point
feature gets triggered, but it seems that the difference in
behaviour is big enough for you to care, so I am hoping you may
think of a way.

This blog post describes the same problem that I had with fork-point:
https://commaok.xyz/post/fork-point/

I didn't see any fork-point tests in the current codebase, but I can
work on adding some. Do you want them in a separate patch (that would
make the most sense to me), or squashed into the rebase.forkPoint
patch?

-Alex

Hi Alex,

On Thu, Jan 21, 2021 at 04:25:25PM -0700, Alex Henrie wrote:

I didn't see any fork-point tests in the current codebase, but I can
work on adding some. Do you want them in a separate patch (that would
make the most sense to me), or squashed into the rebase.forkPoint
patch?

The fork-point tests should be in t3431.

-Denton

On Thu, Jan 21, 2021 at 4:45 PM Denton Liu [off-list ref] wrote:

On Thu, Jan 21, 2021 at 04:25:25PM -0700, Alex Henrie wrote:

quoted

I didn't see any fork-point tests in the current codebase, but I can
work on adding some. Do you want them in a separate patch (that would
make the most sense to me), or squashed into the rebase.forkPoint
patch?

The fork-point tests should be in t3431.

Derp! For some reason I thought that t3431 only tested --fork-point
with an ambiguous refname and that the other tests were unrelated.
Maybe we should make copies of some of those tests that set
rebase.forkPoint instead of passing --fork-point or --no-fork-point on
the command line.

-Alex