rebase flattens history when it shouldn't?

rebase flattens history when it shouldn't?

[Sergei Organov]

Hello,

$ git --version
git version 1.9.3

Please consider the following history:

     --C--
    /     \
   /   ----M topic,HEAD
  /   /
 A---B master

shouldn't

$ git rebase master

be a no-op here? According to my reading of the rebase manual page, it
should be a no-op, as 'topic' is a descendant of the 'master'. Instead,
"git rebase master" flattens the history to:

       ----C topic,HEAD
      /
 A---B master

I'd expect --force-rebase to be required for this to happen:

-f, --force-rebase
    Force the rebase even if the current branch is a descendant of the
    commit you are rebasing onto. Normally non-interactive rebase will
    exit with the message "Current branch is up to date" in such a
    situation. Incompatible with the --interactive option.

Also notice that:

$ git rebase --preserve-merges --verbose master

does perform the rebasing work, even though it does not change the
history in the end.

Here is use-case where it came from and where it gave me real surprise:

I have pull.rebase=true in configuration. Being on a remote tracking
branch, I've successfully pulled from the origin and had no any local
changes on this branch. Then I've successfully merged another branch to
the current one but didn't push the changes back upstream. A few hours
later I returned to the work and issued "git pull" that instead of doing
nothing (as it would be should pull.rebase be either "false" or
"preserve") created a surprising mess.

Do you think it's worth fixing?

Here are reproduction commands for the example history:

git init t
cd t
echo A > a
echo B > b
git add a b
git commit -m A -a
git checkout -b x
echo A >> a
git commit -m C -a
git checkout master
echo B >> b
git commit -m B -a
git checkout -b topic
git merge -m M x
git branch -d x
git rebase master

-- 
Sergey.

Re: rebase flattens history when it shouldn't?

[Jonathan Nieder]

Hi Sergei,

Sergei Organov wrote:

>      --C--
>     /     \
>    /   ----M topic,HEAD
>   /   /
>  A---B master
>
> shouldn't
>
> $ git rebase master
>
> be a no-op here?
[...]
> I'd expect --force-rebase to be required for this to happen:
>
> -f, --force-rebase
>     Force the rebase even if the current branch is a descendant of the
>     commit you are rebasing onto. Normally non-interactive rebase will
>     exit with the message "Current branch is up to date" in such a
>     situation.
[...]
> Do you think it's worth fixing?

Thanks for a clear report.

After a successful 'git rebase master', the current branch is always a
linear string of patches on top of 'master'.  The "already up to date"
behavior when -f is not passed is in a certain sense an optimization
--- it is about git noticing that 'git rebase' wouldn't have anything
to do (except for touching timestamps) and therefore doing nothing.

So I don't think requiring -f for this case would be an improvement.

I do agree that the documentation is misleading.  Any ideas for
wording that could make it clearer?

Hope that helps,
Jonathan

Re: rebase flattens history when it shouldn't?

[Sergei Organov]

Jonathan Nieder <jrnieder@gmail.com> writes:
> Hi Sergei,
>
> Sergei Organov wrote:
>
>>      --C--
>>     /     \
>>    /   ----M topic,HEAD
>>   /   /
>>  A---B master
>>
>> shouldn't
>>
>> $ git rebase master
>>
>> be a no-op here?
> [...]
>> I'd expect --force-rebase to be required for this to happen:
>>
>> -f, --force-rebase
>>     Force the rebase even if the current branch is a descendant of the
>>     commit you are rebasing onto. Normally non-interactive rebase will
>>     exit with the message "Current branch is up to date" in such a
>>     situation.
> [...]
>> Do you think it's worth fixing?
>
> Thanks for a clear report.
>
> After a successful 'git rebase master', the current branch is always a
> linear string of patches on top of 'master'.  The "already up to date"
> behavior when -f is not passed is in a certain sense an optimization
> --- it is about git noticing that 'git rebase' wouldn't have anything
> to do (except for touching timestamps) and therefore doing nothing.
>
> So I don't think requiring -f for this case would be an improvement.

What actually bothers me is the unfortunate consequence that "git pull"
is not always a no-op when nothing was changed at the origin since the
last "git pull". THIS is really surprising and probably should better be
fixed. Requiring -f is just one (obvious) way to fix this.

> I do agree that the documentation is misleading.  Any ideas for
> wording that could make it clearer?

I can't suggest anything as I don't see why -f is there in the first
place. What are use cases?

-- 
Sergey.

Re: rebase flattens history when it shouldn't?

[Holger Hellmuth]

On 23.07.2014 21:33, Sergei Organov wrote:
> What actually bothers me is the unfortunate consequence that "git pull"
> is not always a no-op when nothing was changed at the origin since the
> last "git pull". THIS is really surprising and probably should better be
> fixed. Requiring -f is just one (obvious) way to fix this.

That would invalidate the simple rule that "git pull" is equivalent to 
"git fetch" + "git rebase".

git rebase depends on both branches it operates on, not just one. The 
same goes for "git merge", I assume it is just a coincidence that git 
merge does have this characteristic you now expect both to have.

Re: rebase flattens history when it shouldn't?

[Sergey Organov]

Holger Hellmuth <hellmuth@ira.uka.de> writes:

> On 23.07.2014 21:33, Sergei Organov wrote:
>> What actually bothers me is the unfortunate consequence that "git pull"
>> is not always a no-op when nothing was changed at the origin since the
>> last "git pull". THIS is really surprising and probably should better be
>> fixed. Requiring -f is just one (obvious) way to fix this.
>
> That would invalidate the simple rule that "git pull" is equivalent to
> "git fetch" + "git rebase".

Sorry, I don't see how it would invalidate this. My suggestion even
won't change git-pull source code at all, only git-rebase code.

> git rebase depends on both branches it operates on, not just one. The
> same goes for "git merge", I assume it is just a coincidence that git
> merge does have this characteristic you now expect both to have.

git pull --reabse=false
git pull --rebase=preserve

both have this property.

git pull --rebase=true

almost always has this property, unless there are local merge commits to 
be rebased.

So, I'd rather say it's likely behavior of "git pull --rebase=true" that
is a coincidence.

-- 
Sergey.

Re: rebase flattens history when it shouldn't?

[Sergey Organov]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Hi Sergei,
>
> Sergei Organov wrote:
>
>>      --C--
>>     /     \
>>    /   ----M topic,HEAD
>>   /   /
>>  A---B master
>>
>> shouldn't
>>
>> $ git rebase master
>>
>> be a no-op here?
> [...]
>> I'd expect --force-rebase to be required for this to happen:
>>
>> -f, --force-rebase
>>     Force the rebase even if the current branch is a descendant of the
>>     commit you are rebasing onto. Normally non-interactive rebase will
>>     exit with the message "Current branch is up to date" in such a
>>     situation.
> [...]
>> Do you think it's worth fixing?
>
> Thanks for a clear report.
>
> After a successful 'git rebase master', the current branch is always a
> linear string of patches on top of 'master'.

Is this documented? Except implicitly by the: 

-p, --preserve-merges
           Instead of ignoring merges, try to recreate them.

??

Anyway, why such a requirement, and is it actually enforced by tests?

> The "already up to date" behavior when -f is not passed is in a
> certain sense an optimization --- it is about git noticing that 'git
> rebase' wouldn't have anything to do (except for touching timestamps)
> and therefore doing nothing.

Maybe, but I'd argue it's rather sane behavior to do no rebase when new
rebase point is the same as original in general. I.e., when "current
branch is a descendant of the commit you are rebasing onto", as
documentation says.

> So I don't think requiring -f for this case would be an improvement.

I still do, as it will match documentation, that in turn looks
reasonable.

> I do agree that the documentation is misleading.

If the problem is in documentation, it's not only misleading, it's
formally wrong.

> Any ideas for wording that could make it clearer?

Well, if it's indeed documentation, how about this:

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 2a93c64..62dac31 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -316,10 +316,9 @@ which makes little sense.
 
 -f::
 --force-rebase::
-	Force the rebase even if the current branch is a descendant
-	of the commit you are rebasing onto.  Normally non-interactive rebase will
-	exit with the message "Current branch is up to date" in such a
-	situation.
+	Force the rebase even if the result will only change commit
+	timestamps. Normally non-interactive rebase will exit with the
+	message "Current branch is up to date" in such a situation.
 	Incompatible with the --interactive option.
 +
 You may find this (or --no-ff with an interactive rebase) helpful after

BTW, why "Incompatible with the --interactive option."? Isn't "force"
assumed by --interactive, functionally?

-- 
Sergey.