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

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

[Wesley Schwengle]

A couple of years ago I submitted d1e894c6d7 (Document `rebase.forkpoint` in
rebase man page, 2021-09-16) and during that discussion there was some talk
about the behaviour of `git rebase'[1]. During that time I found that the
documentation update was suffice. I wouldn't say it kept me awake at night but
I do think that `git rebase' with or without an upstream supplied should behave
the same in regards to forkpoints. This patch series addresses this behaviour
change. It introduces a warning so users will have to set `rebase.forkpoint' in
their configuration. In the future we can remove the warning and opt to pick
`--no-fork-point' as a default value for `git rebase'.

There is one point where I'm a little confused, the `test_cmp' function in the
testsuite doesn't like the output that is captured from STDERR, it seems that
there is a difference in regards to whitespace. My workaround is to use
`diff -wq`. I don't know if this is an accepted solution.

Another point of interest is that `git rebase' outputs `Successfully rebased
and updated refs/heads/foo.' on STDERR and when everything is up to date it
outputs `Current branch foo is up to date.' on STDOUT. I was a little confused
by this. Especially since the output on STDOUT can be compared with `test_cmp'.

[1] https://lore.kernel.org/git/xmqqmtocrxwq.fsf@gitster.g/

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

[Wesley Schwengle]

When commit d1e894c6d7 (Document `rebase.forkpoint` in rebase man page,
2021-09-16) was submitted there was a discussion on if the forkpoint
behaviour of `git rebase' was sane. In my experience this wasn't sane.
Git rebase doesn't work if you don't have an upstream branch configured
(or something that says `merge = refs/heads/master' in the git config).
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. 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. This can be seen with the following reproduction path:

    mkdir reproduction
    cd reproduction
    git init .
    echo "commit a" > file.txt
    git add file.txt
    git commit -m "First commit" file.txt
    echo "commit b" >> file.txt
    git commit -m "Second commit" file.txt

    git switch -c foo
    echo "commit c" >> file.txt"
    git commit -m "Third commit" file.txt
    git branch --set-upstream-to=master

    git status
    On branch foo
    Your branch is ahead of 'master' by 1 commit.

    git switch master
    git merge foo
    git reset --hard HEAD^
    git switch foo
    Switched to branch 'foo'
    Your branch is ahead of 'master' by 1 commit.

    git log --format='%C(yellow)%h%Creset %Cgreen%s%Creset'
    5f427e3 Third commit
    03ad791 Second commit
    411e6d4 First commit

    git rebase
    git status
    On branch foo
    Your branch is up to date with 'master'.

    git log --format='%C(yellow)%h%Creset %Cgreen%s%Creset'
    03ad791 Second commit
    411e6d4 First commit

This patch adds a warning where it will indicate that `rebase.forkpoint'
must be set in the git configuration and/or that you can supply a
`--forkpoint' or `--no-forkpoint' command line option to your `git
rebase' invocation.

Signed-off-by: Wesley Schwengle <wesleys@opperschaap.net>
---
 builtin/rebase.c             | 15 ++++++++++-
 t/t3431-rebase-fork-point.sh | 50 ++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 50cb85751f..41dd9b6256 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1604,8 +1604,21 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 								    NULL);
 			if (!options.upstream_name)
 				error_on_missing_default_upstream();
-			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"
+					"You can replace \"git config\" with \"git config --global\" to set a default\n"
+					"preference for all repositories. You can also pass --no-fork-point, --fork-point\n"
+					"on the command line to override the configured default per invocation.\n"
+				));
 				options.fork_point = 1;
+			}
 		} else {
 			options.upstream_name = argv[0];
 			argc--;
diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
index 4bfc779bb8..a583ca6228 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -113,4 +113,54 @@ test_expect_success 'rebase.forkPoint set to true and --root given' '
 	git rebase --root
 '
 
+# The use of the diff -qw is because there is some kind of whitespace character
+# magic going on which probably has to do with the tabs. It only occurs when we
+# check STDERR
+test_expect_success 'rebase without forkpoint' '
+	git init rebase-forkpoint &&
+	cd rebase-forkpoint &&
+	git status >/tmp/foo &&
+	echo "commit a" > file.txt &&
+	git add file.txt &&
+	git commit -m "First commit" file.txt &&
+	echo "commit b" >> file.txt &&
+	git commit -m "Second commit" file.txt &&
+	git switch -c foo &&
+	echo "commit c" >> file.txt &&
+	git commit -m "Third commit" file.txt &&
+	git branch --set-upstream-to=main &&
+	git switch main &&
+	git merge foo &&
+	git reset --hard HEAD^ &&
+	git switch foo &&
+	commit=$(git log -n1 --format="%h") &&
+	git rebase >out 2>err &&
+	test_must_be_empty out &&
+	cat <<-\OEF > expect &&
+	warning: Rebasing without specifying a forkpoint is discouraged. You can squelch
+	this message by running one of the following commands something before your
+	next rebase:
+
+	  git config rebase.forkpoint = false # This will become the new default
+	  git config rebase.forkpoint = true  # This is the old default
+
+	You can replace "git config" with "git config --global" to set a default
+	preference for all repositories. You can also pass --no-fork-point, --fork-point
+	on the command line to override the configured default per invocation.
+
+	Successfully rebased and updated refs/heads/foo.
+	OEF
+	diff -qw expect err &&
+	git reset --hard $commit &&
+	git rebase --fork-point >out 2>err &&
+	test_must_be_empty out &&
+	echo "Successfully rebased and updated refs/heads/foo." > expect &&
+	diff -qw expect err &&
+	git reset --hard $commit &&
+	git rebase --no-fork-point >out 2>err &&
+	test_must_be_empty err &&
+	echo "Current branch foo is up to date." > expect &&
+	test_cmp out expect
+'
+
 test_done
-- 
2.42.0.rc2.7.gf9972720e9.dirty

[PATCH 2/2] git-rebase.txt: Add deprecation notice to the --fork-point options

[Wesley Schwengle]

Signed-off-by: Wesley Schwengle <wesleys@opperschaap.net>
---
 Documentation/git-rebase.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index e7b39ad244..e47b58bec2 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -462,7 +462,9 @@ ends up being empty, the `<upstream>` will be used as a fallback.
 +
 If `<upstream>` or `--keep-base` is given on the command line, then
 the default is `--no-fork-point`, otherwise the default is
-`--fork-point`. See also `rebase.forkpoint` in linkgit:git-config[1].
+`--fork-point`. See also `rebase.forkpoint` in linkgit:git-config[1]. This
+behaviour will be changed in an upcoming release of git. It is advised that you
+set `rebase.forkpoint` in your config or supply the command line switches.
 +
 If your branch was based on `<upstream>` but `<upstream>` was rewound and
 your branch contains commits which were dropped, this option can be used
-- 
2.42.0.rc2.7.gf9972720e9.dirty

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

[Wesley Schwengle]

> [snip]

I submitted this earlier this month. I think it wasn't noticed because of the
release of v4.24.0. If someone could have a look at this, that would be
appreciated. 

Cheers,
Wesley

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

[Junio C Hamano]

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/

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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

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

[Phillip Wood]

Hi Wesley

On 19/08/2023 21:34, Wesley Schwengle wrote:
> When commit d1e894c6d7 (Document `rebase.forkpoint` in rebase man page,
> 2021-09-16) was submitted there was a discussion on if the forkpoint
> behaviour of `git rebase' was sane. In my experience this wasn't sane.
> Git rebase doesn't work if you don't have an upstream branch configured
> (or something that says `merge = refs/heads/master' in the git config).
> 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. 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. This can be seen with the following reproduction path:
> 
>      mkdir reproduction
>      cd reproduction
>      git init .
>      echo "commit a" > file.txt
>      git add file.txt
>      git commit -m "First commit" file.txt
>      echo "commit b" >> file.txt
>      git commit -m "Second commit" file.txt
> 
>      git switch -c foo
>      echo "commit c" >> file.txt"
>      git commit -m "Third commit" file.txt
>      git branch --set-upstream-to=master
> 
>      git status
>      On branch foo
>      Your branch is ahead of 'master' by 1 commit.
> 
>      git switch master
>      git merge foo

Here "git merge" fast-forwards I think, if instead it created a merge 
commit there would be no problem as the tip of branch "foo" would not 
end up in master's reflog.

>      git reset --hard HEAD^
>      git switch foo
>      Switched to branch 'foo'
>      Your branch is ahead of 'master' by 1 commit.
> 
>      git log --format='%C(yellow)%h%Creset %Cgreen%s%Creset'

For a reproduction recipe I think "git log --oneline" would suffice.

>      5f427e3 Third commit
>      03ad791 Second commit
>      411e6d4 First commit
> 
>      git rebase
>      git status
>      On branch foo
>      Your branch is up to date with 'master'.
> 
>      git log --format='%C(yellow)%h%Creset %Cgreen%s%Creset'
>      03ad791 Second commit
>      411e6d4 First commit

Thanks for the detailed reproduction recipe, I think it would be helpful 
to summarize what's happening in the commit message, especially as it 
seems to depend on "git merge" fast-forwarding. Do you often merge a 
branch into it's upstream and then reset the upstream branch?

I tend to agree with Junio that the current default is pretty 
reasonable. Looking through the links from the cover letter it seems 
that the current behavior came from a desire for

	git fetch && git rebase

to behave like

	git pull --rebase

I think the commit message for any change to the default should address 
why that is undesirable. Also we should consider what problems may arise 
from not defaulting to --fork-point when rebasing on an upstream branch 
that has itself been rebased or rewound.

Best Wishes

Phillip

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

[Phillip Wood]

On 31/08/2023 22:52, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> 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.
> 
> 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.

It does seem strange, it looks like the variable was really added as a 
way to turn off the current default. If we do change the default to 
--no-fork-point when no upstream is given on the commandline then I 
think we should consider allowing "auto" for rebase.forkpoint with the 
some meaning as "true" and recommend that instead.

Best Wishes

Phillip

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

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> It does seem strange, it looks like the variable was really added as a
> way to turn off the current default. If we do change the default to
> --no-fork-point when no upstream is given on the commandline then I
> think we should consider allowing "auto" for rebase.forkpoint with the
> some meaning as "true" and recommend that instead.

Perhaps.  The current and existing users do not need to change
anything and 'true' should keep working fine, but given that we are
discouraging the use of fork-point heuristics, it is not clear if it
makes sense to entice new users with a new 'auto' synonym, so, I
dunno

Thanks.
.

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

[Wesley]

Hello Phillip,


On 9/1/23 09:19, Phillip Wood wrote:
> Hi Wesley
> 
> On 19/08/2023 21:34, Wesley Schwengle wrote:
>> When commit d1e894c6d7 (Document `rebase.forkpoint` in rebase man page,
>> 2021-09-16) was submitted there was a discussion on if the forkpoint
>> behaviour of `git rebase' was sane. In my experience this wasn't sane.
>> Git rebase doesn't work if you don't have an upstream branch configured
>> (or something that says `merge = refs/heads/master' in the git config).
>> 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. 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. This can be seen with the following reproduction path:
>>
>>      mkdir reproduction
>>      cd reproduction
>>      git init .
>>      echo "commit a" > file.txt
>>      git add file.txt
>>      git commit -m "First commit" file.txt
>>      echo "commit b" >> file.txt
>>      git commit -m "Second commit" file.txt
>>
>>      git switch -c foo
>>      echo "commit c" >> file.txt"
>>      git commit -m "Third commit" file.txt
>>      git branch --set-upstream-to=master
>>
>>      git status
>>      On branch foo
>>      Your branch is ahead of 'master' by 1 commit.
>>
>>      git switch master
>>      git merge foo
> 
> Here "git merge" fast-forwards I think, if instead it created a merge 
> commit there would be no problem as the tip of branch "foo" would not 
> end up in master's reflog.

If you do

git merge foo --no-ff
git reset --hard HEAD^
git switch foo
git rebase

You'll end up with just the commits that are in master. You'll lose all 
commits from foo.

>>      git log --format='%C(yellow)%h%Creset %Cgreen%s%Creset'
> 
> For a reproduction recipe I think "git log --oneline" would suffice.

Will update, thanks.

 >> [snip]
> 
> Thanks for the detailed reproduction recipe, I think it would be helpful 
> to summarize what's happening in the commit message, especially as it 
> seems to depend on "git merge" fast-forwarding. Do you often merge a 
> branch into it's upstream and then reset the upstream branch?

Tricky question. When I encountered this behavior I was working on an 
epic/topic branch that I had locally. And I made a commit that I thought 
should have been in another branch. I moved the commit to another branch 
and than later on rebased it.

I didn't reply to Juno yet, but he refers to the discussion about 
--fork-point and --root command line options. This discussion links to a 
blogpost [*1*] where the same behavior is experienced.

The quirk is this: --fork-point looks at the reflog and reflog is local. 
Meaning, having an remote upstream branch will make --fork-point a noop. 
Only where you have an upstream which is local and your reflog has seen 
dropped commits it does something. In all other cases (including 
supplying the upstream) it behaves as if --no-fork-point was set. If you 
do the same action in two different clones, you get a different result, 
depending on what is in your reflog. I find this very tricky behavior 
for a default. I've set it to false myself, to get a more consistent 
behavior.

I usually have a remote upstream (gitlab/github) and work with that, so 
the --fork-point behaviour isn't present because there is no reflog for 
that, so it behaves as --no-fork-point.

Cheers,
Wesley


[1]: https://commaok.xyz/post/fork-point/
-- 
Wesley

Why not both?

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

[Junio C Hamano]

Wesley <wesleys@opperschaap.net> writes:

> The quirk is this: --fork-point looks at the reflog and reflog is
> local. Meaning, having an remote upstream branch will make
> --fork-point a noop. Only where you have an upstream which is local
> and your reflog has seen dropped commits it does something.

Why do you lack reflog on your remote-tracking branches in the first
place?  

The fork-point heuristics, as far as I understand it, was invented
exactly to protect you from your upstream repository rewinding and
rebuilding the branch you have been building on top of.  The default
fetch refspec +refs/heads/*:refs/remotes/origin/* has the "force"
option "+" in front exactly because the fetching repository is
expected to keep the reflog for remote-tracking branches to help
recovering from such a rewind & rebuild.

Puzzled.  

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

[Wesley]

On 9/1/23 14:10, Junio C Hamano wrote:
> Wesley <wesleys@opperschaap.net> writes:
> 
>> The quirk is this: --fork-point looks at the reflog and reflog is
>> local. Meaning, having an remote upstream branch will make
>> --fork-point a noop. Only where you have an upstream which is local
>> and your reflog has seen dropped commits it does something.
> 
> Why do you lack reflog on your remote-tracking branches in the first
> place?

I do not know? I tested with a bare repo and two clones. And I also 
tested it with just a remote upstream in another branch.

When in repo-1 I do the reset --hard HEAD^, and push the results, and 
pull them in in repo-2 the behavior doesn't replicate. The git reflog 
command doesn't show the reset.
However, if I delete the reflog entry for removal of the reset HEAD^, 
git rebase exposes the fork-point behavior.

> The fork-point heuristics, as far as I understand it, was invented
> exactly to protect you from your upstream repository rewinding and
> rebuilding the branch you have been building on top of.  The default
> fetch refspec +refs/heads/*:refs/remotes/origin/* has the "force"
> option "+" in front exactly because the fetching repository is
> expected to keep the reflog for remote-tracking branches to help
> recovering from such a rewind & rebuild.

I haven't force pushed anything btw, maybe that could explain things?

Cheers,
Wesley

-- 
Wesley

Why not both?

[PATCH v2] Emit warning when rebasing without a forkpoint

[Wesley Schwengle]

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

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

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

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

[PATCH v2 1/3] rebase.c: Make a distiction between rebase.forkpoint and --fork-point arguments

[Wesley Schwengle]

When you call `git rebase --root' we are not interested in the
rebase.forkpoint configuration. The two options are not to be combined.

Because the implementation checks if the configured value for using a
forkpoint > 0 I've opted to give the configured forkpoint the value 2.
If the user supplies --fork-point on the command line this has a value
of 1. Now we can make a distinction between user input and the configured
value of rebase.forkpoint.

Signed-off-by: Wesley Schwengle <wesleys@opperschaap.net>
---
 builtin/rebase.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 50cb85751f..2108001600 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -824,7 +824,7 @@ static int rebase_config(const char *var, const char *value,
 	}
 
 	if (!strcmp(var, "rebase.forkpoint")) {
-		opts->fork_point = git_config_bool(var, value) ? -1 : 0;
+		opts->fork_point = git_config_bool(var, value) ? 2 : 0;
 		return 0;
 	}
 
@@ -1264,8 +1264,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		if (options.fork_point < 0)
 			options.fork_point = 0;
 	}
-	if (options.root && options.fork_point > 0)
+	if (options.root && options.fork_point == 1) {
 		die(_("options '%s' and '%s' cannot be used together"), "--root", "--fork-point");
+	} else if (options.root && options.fork_point > 1) {
+	    options.fork_point = 0;
+	}
+
 
 	if (options.action != ACTION_NONE && !in_progress)
 		die(_("No rebase in progress?"));
-- 
2.42.0.103.g5622fd1409.dirty

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

[Wesley Schwengle]

This patch adds a warning where it will indicate that `rebase.forkpoint'
must be set in the git configuration and/or that you can supply a
`--fork-point' or `--no-fork-point' command line option to your `git
rebase' invocation.

When commit d1e894c6d7 (Document `rebase.forkpoint` in rebase man page,
2021-09-16) was submitted there was a discussion on if the forkpoint
behaviour of `git rebase' was sane. In my experience this wasn't sane.

Git rebase doesn't work if you don't have an upstream branch configured
(or something that says `merge = refs/heads/master' in the git config).
You would than need to use `git rebase <upstream>' to rebase. If you
configure an upstream it would seem logical to be able to run `git
rebase' without arguments. However doing so would trigger a different
kind of behavior.  `git rebase <upstream>' behaves as if
`--no-fork-point' was supplied and without it behaves as if
`--fork-point' was supplied. This behavior can result in a loss of
commits and can surprise users. The following reproduction path exposes
this behavior:

    git init reproduction
    cd reproduction
    echo "commit a" > file.txt
    git add file.txt
    git commit -m "First commit" file.txt
    echo "commit b" >> file.txt
    git commit -m "Second commit" file.txt

    git switch -c foo
    echo "commit c" >> file.txt"
    git commit -m "Third commit" file.txt
    git branch --set-upstream-to=master

    git status
    On branch foo
    Your branch is ahead of 'master' by 1 commit.

    git switch master
    git merge foo
    git reset --hard HEAD^
    git switch foo
    Switched to branch 'foo'
    Your branch is ahead of 'master' by 1 commit.

    git log --oneline
    5f427e3 Third commit
    03ad791 Second commit
    411e6d4 First commit

    git rebase
    git status
    On branch foo
    Your branch is up to date with 'master'.

    git log --oneline
    03ad791 Second commit
    411e6d4 First commit

Signed-off-by: Wesley Schwengle <wesleys@opperschaap.net>
---
 builtin/rebase.c             | 16 +++++++++-
 t/t3431-rebase-fork-point.sh | 62 ++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 2108001600..ee7db9ba0c 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1608,8 +1608,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 								    NULL);
 			if (!options.upstream_name)
 				error_on_missing_default_upstream();
-			if (options.fork_point < 0)
+			if (options.fork_point < 0) {
+				warning(_(
+					"When \"git rebase\" is run without specifying <upstream> on the\n"
+					"command line, the current default is to use the fork-point\n"
+					"heuristics. This is expected to change in a future version\n"
+					"of Git, and you will have to explicitly give \"--fork-point\" from\n"
+					"the command line if you keep using the fork-point mode.  You can\n"
+					"run \"git config rebase.forkpoint false\" to adopt the new default\n"
+					"in advance and that will also squelch the message.\n"
+					"\n"
+					"You can replace \"git config\" with \"git config --global\" to set a default\n"
+					"preference for all repositories. You can also pass --no-fork-point, --fork-point\n"
+					"on the command line to override the configured default per invocation.\n"
+				));
 				options.fork_point = 1;
+			}
 		} else {
 			options.upstream_name = argv[0];
 			argc--;
diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
index 4bfc779bb8..908867ae0f 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -113,4 +113,66 @@ test_expect_success 'rebase.forkPoint set to true and --root given' '
 	git rebase --root
 '
 
+# The use of the diff -qw is because there is some kind of whitespace character
+# magic going on which probably has to do with the tabs. It only occurs when we
+# check STDERR
+test_expect_success 'rebase without rebase.forkpoint' '
+	git init rebase-forkpoint &&
+	cd rebase-forkpoint &&
+	git status >/tmp/foo &&
+	echo "commit a" > file.txt &&
+	git add file.txt &&
+	git commit -m "First commit" file.txt &&
+	echo "commit b" >> file.txt &&
+	git commit -m "Second commit" file.txt &&
+	git switch -c foo &&
+	echo "commit c" >> file.txt &&
+	git commit -m "Third commit" file.txt &&
+	git branch --set-upstream-to=main &&
+	git switch main &&
+	git merge foo &&
+	git reset --hard HEAD^ &&
+	git switch foo &&
+	commit=$(git log -n1 --format="%h") &&
+	git rebase >out 2>err &&
+	test_must_be_empty out &&
+	cat <<-\OEF > expect &&
+	warning: When "git rebase" is run without specifying <upstream> on the
+	command line, the current default is to use the fork-point
+	heuristics. 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.
+
+	You can replace "git config" with "git config --global" to set a default
+	preference for all repositories. You can also pass --no-fork-point, --fork-point
+	on the command line to override the configured default per invocation.
+
+	Successfully rebased and updated refs/heads/foo.
+	OEF
+	diff -qw expect err &&
+	git reset --hard $commit &&
+	git rebase --fork-point >out 2>err &&
+	test_must_be_empty out &&
+	echo "Successfully rebased and updated refs/heads/foo." > expect &&
+	diff -qw expect err &&
+	git reset --hard $commit &&
+	git rebase --no-fork-point >out 2>err &&
+	test_must_be_empty err &&
+	echo "Current branch foo is up to date." > expect &&
+	test_cmp out expect &&
+	git config --add rebase.forkpoint true &&
+	git rebase >out 2>err &&
+	test_must_be_empty out &&
+	echo "Successfully rebased and updated refs/heads/foo." > expect &&
+	diff -qw expect err &&
+	git reset --hard $commit &&
+	git config --replace-all rebase.forkpoint false &&
+	git rebase >out 2>err &&
+	test_must_be_empty err &&
+	echo "Current branch foo is up to date." > expect &&
+	test_cmp out expect
+'
+
 test_done
-- 
2.42.0.103.g5622fd1409.dirty

[PATCH v2 3/3] git-rebase.txt: Add deprecation notice to the --fork-point options

[Wesley Schwengle]

Signed-off-by: Wesley Schwengle <wesleys@opperschaap.net>
---
 Documentation/git-rebase.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index e7b39ad244..e47b58bec2 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -462,7 +462,9 @@ ends up being empty, the `<upstream>` will be used as a fallback.
 +
 If `<upstream>` or `--keep-base` is given on the command line, then
 the default is `--no-fork-point`, otherwise the default is
-`--fork-point`. See also `rebase.forkpoint` in linkgit:git-config[1].
+`--fork-point`. See also `rebase.forkpoint` in linkgit:git-config[1]. This
+behaviour will be changed in an upcoming release of git. It is advised that you
+set `rebase.forkpoint` in your config or supply the command line switches.
 +
 If your branch was based on `<upstream>` but `<upstream>` was rewound and
 your branch contains commits which were dropped, this option can be used
-- 
2.42.0.103.g5622fd1409.dirty

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

[Junio C Hamano]

Wesley <wesleys@opperschaap.net> writes:

> On 9/1/23 14:10, Junio C Hamano wrote:
>> Wesley <wesleys@opperschaap.net> writes:
>> 
>>> The quirk is this: --fork-point looks at the reflog and reflog is
>>> local. Meaning, having an remote upstream branch will make
>>> --fork-point a noop. Only where you have an upstream which is local
>>> and your reflog has seen dropped commits it does something.
>> Why do you lack reflog on your remote-tracking branches in the first
>> place?
>
> I do not know? I tested with a bare repo and two clones. And I also
> tested it with just a remote upstream in another branch.

IIRC, a non-bare repository (i.e. with working tree) should get
core.logallrefupdates set to true by default, so all your refs, not
just local and remote-tracking branches, should have records.

> I haven't force pushed anything btw, maybe that could explain things?

If your "remote" is never force-pushed, then the movements of refs
at the remote (which you will observe whenever you fetch from it)
will always fast-forward, and the remote-tracking branches in your
local repository that keeps track of the movement will also record
the fast-forwarding movement in the reflog.  But then there is no
need for the fork-point heurisitics to trigger, and even if it
triggered the heuristics would not change the outcome, when rebasing
against such a remote branch, as their tip will always a decendant
of all commits that ever sat at the tip of that remote branch.

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

[Junio C Hamano]

Wesley Schwengle <wesleys@opperschaap.net> writes:

> Subject: Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint

(applies to all three patches) downcase "Emit" after the <area>: prefix.

> This patch adds a warning where it will indicate that `rebase.forkpoint'
> must be set in the git configuration and/or that you can supply a
> `--fork-point' or `--no-fork-point' command line option to your `git
> rebase' invocation.
>
> When commit d1e894c6d7 (Document `rebase.forkpoint` in rebase man page,
> 2021-09-16) was submitted there was a discussion on if the forkpoint
> behaviour of `git rebase' was sane. In my experience this wasn't sane.

I already said that the above is not true, so I will not repeat myself.

> Git rebase doesn't work if you don't have an upstream branch configured

"git rebase foo" works just fine, so this statement needs a lot of
tightening.

> (or something that says `merge = refs/heads/master' in the git config).
> You would than need to use `git rebase <upstream>' to rebase. If you
> configure an upstream it would seem logical to be able to run `git
> rebase' without arguments. However doing so would trigger a different
> kind of behavior.  `git rebase <upstream>' behaves as if
> `--no-fork-point' was supplied and without it behaves as if
> `--fork-point' was supplied. This behavior can result in a loss of
> commits and can surprise users.

No, what is causing the loss in this particular case is allowing to
use the fork-point heuristics.  If you do not want it, you can
either explicitly give --no-fork-point or <upstream> (or both if you
feel that you need to absolutely be clear).  Or you can set the
configuration to "false" to disable this "auto" behaviour.

> The following reproduction path exposes
> this behavior:

I actually do not think having this example in the proposed log
message adds more value than it distracts readers from the real
point of this change.

If you rewind to lose commits from the branch you are (re)building
against, and what was rewound and discarded was part of the work you
are building, whether it is on a local branch or on a remote branch
that contains what you have already pushed, they will be discarded,
it is by design, and it is a known deficiency with the fork-point
heuristics.  How the fork-point heuristics breaks down is rather
well known and it is pretty much orthogonal to the point of this
patch, which is to make it harder to trigger by folks who are not
familiar with "git rebase" and yet try to be lazy by not specifying
the <upstream> from the command line.

By the way, while I do agree with the need to make users _aware_ of
the "auto" behaviour [*1*], I am not yet convinced that there is a
need to change the default in the future.

	Side note: It allows those who originally advocated the
	fork-point heuristics to be extra lazy and allow fork-point
	heuristics to be used when they rebuild on top of what they
	usually rebuild on (and the "usually" part is signalled by
	using "git rebase" without saying what to build on from the
	command line).  The default allows them not to worry about
	the heuristics to kick in when they explicitly say on which
	exact commit they want to rebuild on.

And when we do not know if the default will change, the new warning
message will lose value.  Many of those who see the message are
already familiar with when the forkpoint heuristics will kick in,
and those who weren't familiar with will not know what the default
change is about, without consulting the documentation.

It might be better to extend the documentation instead, which will
not distract those who are using the tool just fine already.

> diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
> index 4bfc779bb8..908867ae0f 100755
> --- a/t/t3431-rebase-fork-point.sh
> +++ b/t/t3431-rebase-fork-point.sh
> @@ -113,4 +113,66 @@ test_expect_success 'rebase.forkPoint set to true and --root given' '
>  	git rebase --root
>  '
>  
> +# The use of the diff -qw is because there is some kind of whitespace character
> +# magic going on which probably has to do with the tabs. It only occurs when we
> +# check STDERR
> +test_expect_success 'rebase without rebase.forkpoint' '
> +	git init rebase-forkpoint &&
> +	cd rebase-forkpoint &&
> +	git status >/tmp/foo &&
> +	echo "commit a" > file.txt &&

Style???

> +	git add file.txt &&
> +	git commit -m "First commit" file.txt &&
> +	echo "commit b" >> file.txt &&
> +	git commit -m "Second commit" file.txt &&
> +	git switch -c foo &&
> +	echo "commit c" >> file.txt &&
> +	git commit -m "Third commit" file.txt &&
> +	git branch --set-upstream-to=main &&
> +	git switch main &&
> +	git merge foo &&
> +	git reset --hard HEAD^ &&
> +	git switch foo &&
> +	commit=$(git log -n1 --format="%h") &&
> +	git rebase >out 2>err &&
> +	test_must_be_empty out &&
> +	cat <<-\OEF > expect &&

Why does this have to be orgiinal in such a strange way?  When
everybody else uses string "EOF" as the end-of-here-doc-marker, and
if there is no downside to use the same string here, we should just
use the same "EOF" to avoid distracting readers.

> +	warning: When "git rebase" is run without specifying <upstream> on the
> +	command line, the current default is to use the fork-point
> +	heuristics. 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.
> +
> +	You can replace "git config" with "git config --global" to set a default
> +	preference for all repositories. You can also pass --no-fork-point, --fork-point
> +	on the command line to override the configured default per invocation.
> +
> +	Successfully rebased and updated refs/heads/foo.
> +	OEF
> +	diff -qw expect err &&

Why not "test_cmp expect actual" like everybody else?

> +...
> +	echo "Current branch foo is up to date." > expect &&
> +	test_cmp out expect
> +'
> +
>  test_done

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

[Wesley]

On 9/2/23 19:37, Junio C Hamano wrote:
> Wesley Schwengle <wesleys@opperschaap.net> writes:

Thanks for the feedback. I won't continue the patch series because some 
of the feedback you've given below.

>> However doing so would trigger a different
>> kind of behavior.  `git rebase <upstream>' behaves as if
>> `--no-fork-point' was supplied and without it behaves as if
>> `--fork-point' was supplied. This behavior can result in a loss of
>> commits and can surprise users.
> 
> No, what is causing the loss in this particular case is allowing to
> use the fork-point heuristics.  If you do not want it, you can
> either explicitly give --no-fork-point or <upstream> (or both if you
> feel that you need to absolutely be clear).  Or you can set the
> configuration to "false" to disable this "auto" behaviour.

Isn't that what I'm saying? At least I'm trying to say what you are saying.

> By the way, while I do agree with the need to make users _aware_ of
> the "auto" behaviour [*1*], I am not yet convinced that there is a
> need to change the default in the future.

In that case, I'll abort this patch series. I don't agree with the `git 
rebase' in the lazy form and `git rebase <upstream>' acting differently, 
but I already have the rebase.forkpoint set to false to counter it.

> It might be better to extend the documentation instead, which will
> not distract those who are using the tool just fine already.

That is with the current viewpoints the best option I think.

>> +	diff -qw expect err &&
> 
> Why not "test_cmp expect actual" like everybody else?

As said in the initial patch series and the comment above the tests:

> There is one point where I'm a little confused, the `test_cmp' function in the
> testsuite doesn't like the output that is captured from STDERR, it seems that
> there is a difference in regards to whitespace. My workaround is to use
> `diff -wq`. I don't know if this is an accepted solution.

That's why.

Cheers,
Wesley

-- 
Wesley

Why not both?

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> If you rewind to lose commits from the branch you are (re)building
> against, and what was rewound and discarded was part of the work you
> are building, whether it is on a local branch or on a remote branch
> that contains what you have already pushed, they will be discarded,
> it is by design, and it is a known deficiency with the fork-point
> heuristics.  How the fork-point heuristics breaks down is rather
> well known ...

Another tangent, this time very closely related to this topic, is
that it may be worth warning when the fork-point heuristics chooses
the base commit that is different from the original upstream,
regardless of how we ended up using fork-point heuristics.

Experienced users may not be confused when the heuristics kicks in
and when it does not (e.g. because they configured, because they
used the "lazy" form, or because they gave "--fork-point" from the
command line explicitly), but they still may get surprising results
if a reflog entry chosen to be used as the base by the heuristics is
not what they expected to be used, and can lose their work that way.
Imagine that you pushed your work to the remote that is a shared
repository, and then continued building on top of it, while others
rewound the remote branch to eject your work, and your "git fetch"
updated the remote-tracking branch.  You'll be pretty much in the
same situation you had in your reproduction recipe that rewound your
own local branch that you used to build your derived work on and
would lose your work the same way, if you do not notice that the
remote branch has been rewound (and the fork-point heuristics chose
a "wrong" commit from the reflog of your remote-tracking branch.

Perhaps something along the lines of this (not even compile tested,
though)...  It might even be useful to show a shortlog between the
.restrict_revision and .upstream, which is the list of commits that
is potentially lost, but that might turn out to be excessively loud
and noisy in the workflow of those who do benefit from the
fork-point heuristics because their project rewinds branches too
often and too wildly for them to manually keep track of.  I dunno.


 builtin/rebase.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git c/builtin/rebase.c w/builtin/rebase.c
index 50cb85751f..432a97e205 100644
--- c/builtin/rebase.c
+++ w/builtin/rebase.c
@@ -1721,9 +1721,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (keep_base && options.reapply_cherry_picks)
 		options.upstream = options.onto;
 
-	if (options.fork_point > 0)
+	if (options.fork_point > 0) {
 		options.restrict_revision =
 			get_fork_point(options.upstream_name, options.orig_head);
+		if (options.restrict_revision &&
+		    options.restrict_revision != options.upstream)
+			warning(_("fork-point heuristics using %s from the reflog of %s"),
+				oid_to_hex(&options.restrict_revision->object.oid),
+				options.upstream_name);
+	}
 
 	if (repo_read_index(the_repository) < 0)
 		die(_("could not read index"));

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

[Wesley Schwengle]

On 9/3/23 00:50, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> If you rewind to lose commits from the branch you are (re)building
>> against, and what was rewound and discarded was part of the work you
>> are building, whether it is on a local branch or on a remote branch
>> that contains what you have already pushed, they will be discarded,
>> it is by design, and it is a known deficiency with the fork-point
>> heuristics.  How the fork-point heuristics breaks down is rather
>> well known ...
> 
> Another tangent, this time very closely related to this topic, is
> that it may be worth warning when the fork-point heuristics chooses
> the base commit that is different from the original upstream,
> regardless of how we ended up using fork-point heuristics.
> 
> [snip]
> 
> Perhaps something along the lines of this (not even compile tested,
> though)...  It might even be useful to show a shortlog between the
> .restrict_revision and .upstream, which is the list of commits that
> is potentially lost, but that might turn out to be excessively loud
> and noisy in the workflow of those who do benefit from the
> fork-point heuristics because their project rewinds branches too
> often and too wildly for them to manually keep track of.  I dunno.

I like the idea of the warning, but it could be loud indeed and you'll 
want to turn it off in that case.

-- 
Wesley

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

[Phillip Wood]

On 03/09/2023 05:50, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> If you rewind to lose commits from the branch you are (re)building
>> against, and what was rewound and discarded was part of the work you
>> are building, whether it is on a local branch or on a remote branch
>> that contains what you have already pushed, they will be discarded,
>> it is by design, and it is a known deficiency with the fork-point
>> heuristics.  How the fork-point heuristics breaks down is rather
>> well known ...
> 
> Another tangent, this time very closely related to this topic, is
> that it may be worth warning when the fork-point heuristics chooses
> the base commit that is different from the original upstream,
> regardless of how we ended up using fork-point heuristics.

I think that is a good idea and would help to mitigate the surprise that 
some users have expressed when --fork-point kicks and they didn't know 
about it. I think we may want to compare "branch_base" which holds the 
merge-base of HEAD and upstream with "restrict_revision" to decide when 
to warn.

Best Wishes

Phillip

> Experienced users may not be confused when the heuristics kicks in
> and when it does not (e.g. because they configured, because they
> used the "lazy" form, or because they gave "--fork-point" from the
> command line explicitly), but they still may get surprising results
> if a reflog entry chosen to be used as the base by the heuristics is
> not what they expected to be used, and can lose their work that way.
> Imagine that you pushed your work to the remote that is a shared
> repository, and then continued building on top of it, while others
> rewound the remote branch to eject your work, and your "git fetch"
> updated the remote-tracking branch.  You'll be pretty much in the
> same situation you had in your reproduction recipe that rewound your
> own local branch that you used to build your derived work on and
> would lose your work the same way, if you do not notice that the
> remote branch has been rewound (and the fork-point heuristics chose
> a "wrong" commit from the reflog of your remote-tracking branch.
> 
> Perhaps something along the lines of this (not even compile tested,
> though)...  It might even be useful to show a shortlog between the
> .restrict_revision and .upstream, which is the list of commits that
> is potentially lost, but that might turn out to be excessively loud
> and noisy in the workflow of those who do benefit from the
> fork-point heuristics because their project rewinds branches too
> often and too wildly for them to manually keep track of.  I dunno.
> 
> 
>   builtin/rebase.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git c/builtin/rebase.c w/builtin/rebase.c
> index 50cb85751f..432a97e205 100644
> --- c/builtin/rebase.c
> +++ w/builtin/rebase.c
> @@ -1721,9 +1721,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	if (keep_base && options.reapply_cherry_picks)
>   		options.upstream = options.onto;
>   
> -	if (options.fork_point > 0)
> +	if (options.fork_point > 0) {
>   		options.restrict_revision =
>   			get_fork_point(options.upstream_name, options.orig_head);
> +		if (options.restrict_revision &&
> +		    options.restrict_revision != options.upstream)
> +			warning(_("fork-point heuristics using %s from the reflog of %s"),
> +				oid_to_hex(&options.restrict_revision->object.oid),
> +				options.upstream_name);
> +	}
>   
>   	if (repo_read_index(the_repository) < 0)
>   		die(_("could not read index"));
> 

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

[Junio C Hamano]

Wesley Schwengle <wesleys@opperschaap.net> writes:

> I like the idea of the warning, but it could be loud indeed and you'll
> want to turn it off in that case.

I tend to think that a single-liner warning would not be too
intrusive (it might actually be too subtle to be noticed),
especially given that it is issued only when the fork-point does
move the target commit from what was given.

Giving a shortlog of what is lost in the history does sound like a
bit too loud, I am afraind, though.