Possible git bug

Possible git bug

[Wesley Schwengle]

Hello,

I'm running into a weird issue with git at the moment and I'm wondering 
if this is a bug. I have a small reproduction path:

$ mkdir reproduction
$ cd reproduction
$ git init .
$ echo "commit a" > 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 master
$ 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

I do not expect to lose the commits from foo, it seems the ref of master 
doesn't get updated after a reset.

I didn't lose any work because I pushed my work to a remote and can 
still get it from there (and git reflog would also reach my code). It is 
rather surprising to see this kind of behavior.

I'm running:
$ git --version
git version 2.33.0.363.g4c719308ce

But I also got this behavior with the git shipped with Debian:
$ /usr/bin/git --version
git version 2.32.0

Cheers,
Wesley

-- 
Wesley Schwengle
E: wesley@schwengle.net

Re: Possible git bug

[Taylor Blau]

On Wed, Sep 15, 2021 at 11:29:14PM -0400, Wesley Schwengle wrote:
>
> Hello,
>
> I'm running into a weird issue with git at the moment and I'm wondering if
> this is a bug. I have a small reproduction path:

> $ git commit -m "First commit" file.txt

FWIW, I had to tweak this script a little, since file.txt is untracked
before it is added initially. (So a "git add file.txt" before this first
commit is required.)

But even after this, I got exactly what I expected from this script
(which was that your "foo" branch had three commits before and after).
Is there something else interesting going on with your setup that might
explain why I can't reproduce this?

Thanks,
Taylor

Re: Possible git bug

[Wesley Schwengle]

On 9/16/21 1:37 AM, Taylor Blau wrote:

> FWIW, I had to tweak this script a little, since file.txt is untracked
> before it is added initially. (So a "git add file.txt" before this first
> commit is required.)

Oh sorry! I overlooked that part.

> But even after this, I got exactly what I expected from this script
> (which was that your "foo" branch had three commits before and after).
> Is there something else interesting going on with your setup that might
> explain why I can't reproduce this?

Oh, I found it.. Replace `git rebase master' with `git rebase' in the 
reproduction path.

Disregard my post, it seems this is documented behavior in the rebase 
man-page. When you have an upstream configured and you don't specify it 
on the command line, --fork-point is used, while if you specify the 
upstream --no-fork-point is used. `git rebase master --fork-point' 
exhibits the same as I was seeing. Although I'm now completely confused 
by this behavior. It doesn't make sense to me.

This happens:

We are on a branch, we merge it into another branch.
We undo the merge because reasons.
Now we git rebase, without the upstream, because we've set it.
Fork-point is used now, because we haven't specified an upstream, but we 
did set it and git merge-base decides, oh, we had those commits in 
master but these where dropped so we drop them in this branch as well.

New question, is there a way to tell rebase to NOT use fork-point via 
git-config in this situation?

Cheers,
Wesley

-- 
Wesley Schwengle
E: wesley@schwengle.net

Re: Possible git bug

[wesley]

On 9/16/21 8:07 AM, Wesley Schwengle wrote:
 
> New question, is there a way to tell rebase to NOT use fork-point via 
> git-config in this situation?

I seem to have found the answer in the source code: rebase.forkpoint exists.

Would you accept the following patch that adds the following text to the
documentation?

Cheers,
Wesley

[PATCH] Document `rebase.forkpoint` in rebase man page

[wesley]

From: Wesley Schwengle <wesley@opperschaap.net>

The option exists and the rebase behaviour tricked me into thinking
there was a bug with git. This will tell people how they can tweak the
default behavior.

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

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 506345cb0e..8d2bee3365 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -446,7 +446,8 @@ When --fork-point is active, 'fork_point' will be used instead of
 ends up being empty, the <upstream> will be used as a fallback.
 +
 If <upstream> is given on the command line, then the default is
-`--no-fork-point`, otherwise the default is `--fork-point`.
+`--no-fork-point`, otherwise the default is `--fork-point`. You can override
+this default by setting the configuration option `rebase.forkpoint` to false.
 +
 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.33.0.364.gff7047fb76

Re: Possible git bug

[Junio C Hamano]

wesley@schwengle.net writes:

> On 9/16/21 8:07 AM, Wesley Schwengle wrote:
>  
>> New question, is there a way to tell rebase to NOT use fork-point via 
>> git-config in this situation?
>
> I seem to have found the answer in the source code: rebase.forkpoint exists.
>
> Would you accept the following patch that adds the following text to the
> documentation?

Not so fast.

Earlier you said:

> ... When you have an upstream configured and you don't specify
> it on the command line, --fork-point is used, while if you specify the 
> upstream --no-fork-point is used. `git rebase master --fork-point'
> exhibits the same as I was seeing. Although I'm now completely
> confused by this behavior. It doesn't make sense to me.
>
> This happens:
>
> We are on a branch, we merge it into another branch.
> We undo the merge because reasons.
> Now we git rebase, without the upstream, because we've set it.
> Fork-point is used now, because we haven't specified an upstream, but
> we did set it and git merge-base decides, oh, we had those commits in 
> master but these where dropped so we drop them in this branch as well.

If you feel "It doesn't make sense to me", either

 - the behaviour does not make sense because it is simply buggy, in
   which case, adding a sentence to the documentation and explaining
   how not to use it is missing the point---don't you rather want it
   to behave in a way that makes sense to you instead?

or

 - it appears as nonsense to you only because your understanding of
   the behaviour is faulty but the feature is working correctly and
   is not a bug, in which case, adding a sentence to the
   documentation and explaining how not to use it is missing the
   point---don't you rather want the existing documentation extended
   to help you and other users to understand the behaviour better
   first?

Between "buggy behaviour" and "bad documentation of a well-designed
behaviour", I offhand do not know which side "--fork-point" is for
this particular case, but I've always felt that it is a bad
heuristic that should be used with care, and my gut feeling is it
might be the third possibility: "bad heuristic that sometimes
misbehave badly and that is unfixable".  If that is the case,
perhaps the documentation should tell readers the unreliable nature
of the option and warn them to double check the result before
teaching them how to turn it off permanently.

Thanks.

Re: [PATCH] Document `rebase.forkpoint` in rebase man page

[Junio C Hamano]

wesley@schwengle.net writes:

> From: Wesley Schwengle <wesley@opperschaap.net>
>
> The option exists and the rebase behaviour tricked me into thinking
> there was a bug with git. This will tell people how they can tweak the
> default behavior.

This tells readers about almost nothing but your frustration.

We, or anybody who will be reading "git log" in 6 months to improve
the system, will not need to hear it.  Instead we need to understand
what the real problem is, what was wrong in the behaviour, or what
the expected behaviour was and why the use of the feature was
inappropriate in the particular case, without which it is impossible
to understand why this sentence was added when a future developer
and documenter tries to improve upon this text.

> Signed-off-by: Wesley Schwengle <wesley@opperschaap.net>
> ---
>  Documentation/git-rebase.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 506345cb0e..8d2bee3365 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -446,7 +446,8 @@ When --fork-point is active, 'fork_point' will be used instead of
>  ends up being empty, the <upstream> will be used as a fallback.
>  +
>  If <upstream> is given on the command line, then the default is
> -`--no-fork-point`, otherwise the default is `--fork-point`.
> +`--no-fork-point`, otherwise the default is `--fork-point`. You can override
> +this default by setting the configuration option `rebase.forkpoint` to false.

We often do:

  "See also `rebase.forkpoint` in linkgit:git-config[1]."

(for example, there is a reference to linkgit:githooks[5] in the
same page).

Thanks.

Re: Possible git bug

[Wesley Schwengle]

On 9/16/21 11:33 AM, Junio C Hamano wrote:

>> We are on a branch, we merge it into another branch.
>> We undo the merge because reasons.
>> Now we git rebase, without the upstream, because we've set it.
>> Fork-point is used now, because we haven't specified an upstream, but
>> we did set it and git merge-base decides, oh, we had those commits in
>> master but these where dropped so we drop them in this branch as well.
> 
> If you feel "It doesn't make sense to me", either
> 
>   - the behaviour does not make sense because it is simply buggy, in
>     which case, adding a sentence to the documentation and explaining
>     how not to use it is missing the point---don't you rather want it
>     to behave in a way that makes sense to you instead?
> 
> or
> 
>   - it appears as nonsense to you only because your understanding of
>     the behaviour is faulty but the feature is working correctly and
>     is not a bug, in which case, adding a sentence to the
>     documentation and explaining how not to use it is missing the
>     point---don't you rather want the existing documentation extended
>     to help you and other users to understand the behaviour better
>     first?
> 
> Between "buggy behaviour" and "bad documentation of a well-designed
> behaviour", I offhand do not know which side "--fork-point" is for
> this particular case, but I've always felt that it is a bad
> heuristic that should be used with care, and my gut feeling is it
> might be the third possibility: "bad heuristic that sometimes
> misbehave badly and that is unfixable".  If that is the case,
> perhaps the documentation should tell readers the unreliable nature
> of the option and warn them to double check the result before
> teaching them how to turn it off permanently.

I feel like it is a bad default, it caught me by surprise. Especially 
because in the reproduction path I wanted to explicit in my rebase 
action and this caused different behavior. After this was pointed out I 
read the man page because I thought `git rebase' and `git rebase master' 
was the same thing if that was configured as an upstream. It took me a 
while to figure this out, because I kept typing `git rebase' instead of 
`git rebase master' when quickly trying to find out why it wasn't 
behaving like it did earlier.

I'm clueless about "buggy behavior", "bad documentation of a well 
designed feature" or "bad heuristic that sometimes misbehave badly and 
that is unfixable". To me `git rebase' with a configured upstream should 
behave the same as `git rebase @{u}'. Only when adding --fork-point it 
should behave as it does currently. I'm not sure when I would want to 
use it, but I'm thinking people want it, otherwise it wouldn't be a default.

As for the patch. The reason why --fork-point is default I do not know, 
but how to disable it isn't documented and I think it should. It is 
hidden in the source code and the release notes of 2.31.0. It should be 
more visible. Which is the reason I submitted the patch.

Cheers,
Wesley

-- 
Wesley Schwengle
E: wesley@schwengle.net

Re: [PATCH] Document `rebase.forkpoint` in rebase man page

[Junio C Hamano]

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

> wesley@schwengle.net writes:
>
>> From: Wesley Schwengle <wesley@opperschaap.net>
>>
>> The option exists and the rebase behaviour tricked me into thinking
>> there was a bug with git. This will tell people how they can tweak the
>> default behavior.
>
> This tells readers about almost nothing but your frustration.
>
> We, or anybody who will be reading "git log" in 6 months to improve
> the system, will not need to hear it.  Instead we need to understand
>
> what the real problem is, what was wrong in the behaviour, or what
> the expected behaviour was and why the use of the feature was
> inappropriate in the particular case, without which it is impossible
> to understand why this sentence was added when a future developer
> and documenter tries to improve upon this text.

I misspke a bit here.  While hearing only your frustration and
nothing else won't help us much, we do need to understand what
caused your frustration, to avoid frustrating the next user the same
way.  All of the "Instead we need to understand ..." are about that.

> We often do:
>
>   "See also `rebase.forkpoint` in linkgit:git-config[1]."
>
> (for example, there is a reference to linkgit:githooks[5] in the
> same page).

One reason why you didn't find how to tweak the forkpoint feature,
other than giving a command line option to countermand it, is
because this link pointing at the list of configurations, where the
variable is already described, was missing in the doucmentation for
the "rebase" command.

Thanks.

Re: Possible git bug

[Junio C Hamano]

Wesley Schwengle <wesley@schwengle.net> writes:

> I feel like it is a bad default, it caught me by surprise.

I tend to agree.  It seems that d44e7126 (pull: support rebased
upstream + fetch + pull --rebase, 2009-07-19) started it, probably
by mistake, which was partially corrected by ad8261d2 (rebase: use
reflog to find common base with upstream, 2013-12-09).

The thread that contains

  https://lore.kernel.org/git/xmqq7gbdzsvt.fsf@gitster.dls.corp.google.com/

seems to have resulted in the design of the current behaviour, where 
the discussion refers to an even older discussion thread:

https://lore.kernel.org/git/d8e9f102609ee4502f579cb4ce872e0a40756204.1381949622.git.john@keeping.me.uk/

	Side note: I am kind-of surprised that I contributed the
	core computation of the fork-point logic, even though I
	wasn't buying it is a good feature back then.

In any case, updating the documentation to refer to the configuraion
variable that tweaks the default for --fork-point would be a good
near-term thing to do, but in the longer term, I think it may make
sense to fix this "surprise" and transition the default over time,
i.e.

 (1) when "git rebase" is run without --[no-]fork-point from the
     command line, and without rebase.forkpoint configuration
     variable in effect, give a warning that says we'll change the
     default to 'false' and the users who want to can use the
     configuration variable to force it to 'true'.  Update the
     documentation to say the special casing of "If <upstream> is
     not specified, --fork-point option is assumed" will be changed
     in the future.

     Ship such a version of Git and wait for several development
     cycles.


 (2) flip the default and remove the warning.  Update the
     documentation.

> As for the patch. The reason why --fork-point is default I do not
> know, but how to disable it isn't documented and I think it should. It
> is hidden in the source code and the release notes of 2.31.0. It
> should be more visible. Which is the reason I submitted the patch.

Certainly.

"git config --help" is the only end-user facing place the reference
from the configuration variable to the command line option is found.
We should also have a backreference from the command line option to
the configuration variable.

Re: Possible git bug

[Wesley Schwengle]

On 9/16/21 5:52 PM, Junio C Hamano wrote:
> Wesley Schwengle <wesley@schwengle.net> writes:

>> As for the patch. The reason why --fork-point is default I do not
>> know, but how to disable it isn't documented and I think it should. It
>> is hidden in the source code and the release notes of 2.31.0. It
>> should be more visible. Which is the reason I submitted the patch.
> 
> Certainly.
> 
 > In any case, updating the documentation to refer to the configuraion
 > variable that tweaks the default for --fork-point would be a good
 > near-term thing to do, but in the longer term, I think it may make
 > sense to fix this "surprise" and transition the default over time,
 > i.e.
 >
 > "git config --help" is the only end-user facing place the reference
 > from the configuration variable to the command line option is found.
 > We should also have a backreference from the command line option to
 > the configuration variable.

A simple reference in the documentation would be a good first step and 
the change of the default can than happen over multiple iterations/versions.

Cheers,
Wesley

-- 
Wesley Schwengle
E: wesley@schwengle.net

Re: Possible git bug

[wesley]

I've updated the patch and the commit message. I hope this is sufficient
to be included.

Cheers,
Wesley

[PATCH] Document `rebase.forkpoint` in rebase man page

[wesley]

From: Wesley Schwengle <wesley@opperschaap.net>

The option exists and the rebase behaviour tricked me into thinking
there was a bug with git. This will tell people how they can tweak the
default behavior.

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

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 506345cb0e..8d2bee3365 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -446,7 +446,8 @@ When --fork-point is active, 'fork_point' will be used instead of
 ends up being empty, the <upstream> will be used as a fallback.
 +
 If <upstream> is given on the command line, then the default is
-`--no-fork-point`, otherwise the default is `--fork-point`.
+`--no-fork-point`, otherwise the default is `--fork-point`. You can override
+this default by setting the configuration option `rebase.forkpoint` to false.
 +
 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.33.0.364.gff7047fb76

Re: Possible git bug

[wesley]

I failed to include the correct format-patch, 3rd time is a charm :)

Cheers,
Wesley

[PATCH] Document `rebase.forkpoint` in rebase man page

[wesley]

From: Wesley Schwengle <wesley@opperschaap.net>

The configuration option `rebase.forkpoint' is only mentioned in the man
page of git-config(1). Since it is a configuration for rebase, mention
it in the documentation of rebase at the --fork-point/--no-fork-point
section. This will help users set a preferred default for their
workflow.

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

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 506345cb0e..c116dbf4bb 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -446,7 +446,8 @@ When --fork-point is active, 'fork_point' will be used instead of
 ends up being empty, the <upstream> will be used as a fallback.
 +
 If <upstream> is given on the command line, then the default is
-`--no-fork-point`, otherwise the default is `--fork-point`.
+`--no-fork-point`, otherwise the default is `--fork-point`. See also
+`rebase.forkpoint` in linkgit:git-config[1].
 +
 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.33.0.364.gff7047fb76

Re: [PATCH] Document `rebase.forkpoint` in rebase man page

[Junio C Hamano]

wesley@schwengle.net writes:

> From: Wesley Schwengle <wesley@opperschaap.net>
>
> The option exists and the rebase behaviour tricked me into thinking
> there was a bug with git. This will tell people how they can tweak the
> default behavior.

So this still does not explain what rebase behaviour tricked you
into thinking so.  That leaves the readers of "git log" frustrated,
much more than a log message based only on a simple statement of
fact, e.g.

    "git config --help" describes rebase.forkpoint as a way to give
    the default value for --[no-]forkpoint option, but "git rebase
    --help" does not mention it.  

    Help people who visits the documentation of "rebase" to find the
    variable.

or something like that.

> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 506345cb0e..8d2bee3365 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -446,7 +446,8 @@ When --fork-point is active, 'fork_point' will be used instead of
>  ends up being empty, the <upstream> will be used as a fallback.
>  +
>  If <upstream> is given on the command line, then the default is
> -`--no-fork-point`, otherwise the default is `--fork-point`.
> +`--no-fork-point`, otherwise the default is `--fork-point`. You can override
> +this default by setting the configuration option `rebase.forkpoint` to false.

It is not wrong per-se, but sounds overly verbose and seems to give
only half an advice.  You can also set it to 'true' to override the
default --no-fork-point given when you give <upstream> on the
command line, no?  So perhaps only a single line addition (plus
downcasing "If" to "if"), i.e.

+   If `rebase.forkpoint` is set, that gives the default.  Otherwise,
    if <upstream is given on the command line, the default is ...

would be a better rewrite?

Thanks.

Re: [PATCH] Document `rebase.forkpoint` in rebase man page

[Wesley Schwengle]

On 9/16/21 6:47 PM, Junio C Hamano wrote:
> wesley@schwengle.net writes:
> 
>> From: Wesley Schwengle <wesley@opperschaap.net>
>>
>> The option exists and the rebase behaviour tricked me into thinking
>> there was a bug with git. This will tell people how they can tweak the
>> default behavior.
> 
> So this still does not explain what rebase behaviour tricked you
> into thinking so.  That leaves the readers of "git log" frustrated,
> much more than a log message based only on a simple statement of
> fact, e.g.
> 
>      "git config --help" describes rebase.forkpoint as a way to give
>      the default value for --[no-]forkpoint option, but "git rebase
>      --help" does not mention it.
> 
>      Help people who visits the documentation of "rebase" to find the
>      variable.
> 
> or something like that.

I made a booboo. I did not run format-patch with the updated commit 
message and commit. I saw it too late. You should have a new one by the 
time you read this :)

Cheers,
Wesley


-- 
Wesley Schwengle
E: wesley@schwengle.net

Re: [PATCH] Document `rebase.forkpoint` in rebase man page

[Junio C Hamano]

Wesley Schwengle <wesley@schwengle.net> writes:

> I made a booboo. I did not run format-patch with the updated commit
> message and commit. I saw it too late. You should have a new one by

That is OK.  Mistakes happen.

I'll slow down to avoid further confusion from mails crossing, but I
have a feeling that you either forgot to read, or sent an updated
patch before reading, the latter half of the message you are
responding to.

Thanks.

Re: [PATCH] Document `rebase.forkpoint` in rebase man page

[Wesley Schwengle]

On 9/16/21 6:53 PM, Junio C Hamano wrote:
> Wesley Schwengle <wesley@schwengle.net> writes:
> 
>> I made a booboo. I did not run format-patch with the updated commit
>> message and commit. I saw it too late. You should have a new one by
> 
> That is OK.  Mistakes happen.
> 
> I'll slow down to avoid further confusion from mails crossing, but I
> have a feeling that you either forgot to read, or sent an updated
> patch before reading, the latter half of the message you are
> responding to.

I did the same thing, the patch is present in the thread at 
https://public-inbox.org/git/20210916224603.2912887-1-wesley@schwengle.net/

Cheers,
Wesley

-- 
Wesley Schwengle
E: wesley@schwengle.net

Re: [PATCH] Document `rebase.forkpoint` in rebase man page

[Junio C Hamano]

wesley@schwengle.net writes:

> From: Wesley Schwengle <wesley@opperschaap.net>
>
> The configuration option `rebase.forkpoint' is only mentioned in the man
> page of git-config(1). Since it is a configuration for rebase, mention
> it in the documentation of rebase at the --fork-point/--no-fork-point
> section. This will help users set a preferred default for their
> workflow.
>
> Signed-off-by: Wesley Schwengle <wesley@opperschaap.net>
> ---
>  Documentation/git-rebase.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 506345cb0e..c116dbf4bb 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -446,7 +446,8 @@ When --fork-point is active, 'fork_point' will be used instead of
>  ends up being empty, the <upstream> will be used as a fallback.
>  +
>  If <upstream> is given on the command line, then the default is
> -`--no-fork-point`, otherwise the default is `--fork-point`.
> +`--no-fork-point`, otherwise the default is `--fork-point`. See also
> +`rebase.forkpoint` in linkgit:git-config[1].
>  +
>  If your branch was based on <upstream> but <upstream> was rewound and
>  your branch contains commits which were dropped, this option can be used

I still think "the variable gives the default, otherwise the
presence of <upstream> on the command line affects which one is used
as the default" presentation order is better, but the above is a
strict improvement over the current message, so let's take it.

Thanks.