Odd rebase behavior

Odd rebase behavior

[David A. Greene]

Hi,

The attached tests do not do what I expected them to do.  I commented
out the tests involving the new rebase empty commit behavior I just
sent.  The uncommented tests show the strange behavior.

According to the rebase man page, rebase gathers commits as in "git log
<upstream>..HEAD."  However, that is not what happens in the tests
below.  Some of the commits disappear.

The test basically does this:

- Setup a master project and a subproject, merged via a subtree-like
  merge (this is how git-subtree does it).

- Add some commits to the subproject directory after the subtree merge,
  to create some history not in the original subproject.

- filter-branch --subdirectory-filter to extract commits from the
  subproject directory.

- Rebase those commits back on to the original subproject repository.

The above loses all commits made after the subproject is merged into
the main project.

Note that the rebase is a little wonky.  filter-branch creates a
disconnected graph and the rebase is invoked with <upstream>=master.
I'm not sure how rebase is supposed to operate in this case (if it is
supported at all) but it definitely is not doing the master..HEAD thing.

Replacing "master" with "--root" causes rebase to do the right thing.

This seems like a bug to me, even with the strange <upstream> on a
disconnected graph.  At the very least git should not silently lose
commits.

I can think of two ways this could be resolved:

- Forbid this kind of operation and error our with a message (when
  <upstream> and HEAD do not share ancestry)

- Make it work as if --root were specified

Thoughts?

                         -David

--->8---

#!/bin/sh

test_description='git rebase tests for empty commits

This test runs git rebase and tests handling of empty commits.
'
. ./test-lib.sh

addfile() {
    name=$1
    echo $(basename ${name}) > ${name}
    ${git} add ${name}
    ${git} commit -m "Add $(basename ${name})"
}

check_equal()
{
	test_debug 'echo'
	test_debug "echo \"check a:\" \"{$1}\""
	test_debug "echo \"      b:\" \"{$2}\""
	if [ "$1" = "$2" ]; then
		return 0
	else
		return 1
	fi
}

last_commit_message()
{
	git log --pretty=format:%s -1
}

test_expect_success 'setup' '
	test_commit README &&
	mkdir files &&
	cd files &&
	git init &&
	test_commit master1 &&
	test_commit master2 &&
	test_commit master3 &&
	cd .. &&
	test_debug "echo Add project master to master" &&
	git fetch files master &&
	git branch files-master FETCH_HEAD &&
	test_debug "echo Add subtree master to master via subtree" &&
	git read-tree --prefix=files_subtree files-master &&
	git checkout -- files_subtree &&
	tree=$(git write-tree) &&
	head=$(git rev-parse HEAD) &&
	rev=$(git rev-parse --verify files-master^0) &&
	commit=$(git commit-tree -p ${head} -p ${rev} -m "Add subproject master" ${tree}) &&
	git reset ${commit} &&
	cd files_subtree &&
	test_commit master4 &&
	cd .. &&
	test_commit files_subtree/master5
'

# Does not preserve master4 and master5.
test_expect_success 'Rebase default' '
	git checkout -b rebase-default master &&
	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
	git commit -m "Empty commit" --allow-empty &&
	git rebase -Xsubtree=files_subtree  --preserve-merges --onto files-master master &&
	check_equal "$(last_commit_message)" "files_subtree/master5"
'

# Does not preserve master4, master5 and empty.
test_expect_success 'Rebase --keep-empty' '
	git checkout -b rebase-keep-empty master &&
	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
	git commit -m "Empty commit" --allow-empty &&
	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
	check_equal "$(last_commit_message)" "Empty commit"
'


# Does not preserve master4 and master5.
#test_expect_success 'Rebase --keep-redundant' '
#	git checkout -b rebase-keep-redundant master &&
#	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
#	git commit -m "Empty commit" --allow-empty &&
#	git rebase -Xsubtree=files_subtree --keep-redundant --preserve-merges --onto files-master master &&
#	check_equal "$(last_commit_message)" "files_subtree/master5"
#'


# Does not preserve master4, master5 and empty.
#test_expect_success 'Rebase --keep-empty --keep-redundant' '
#	git checkout -b rebase-keep-empty-keep-redundant master &&
#	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
#	git commit -m "Empty commit" --allow-empty &&
#	git rebase -Xsubtree=files_subtree --keep-empty --keep-redundant --preserve-merges --onto files-master master &&
#	check_equal "$(last_commit_message)" "Empty commit"
#'


test_done

Re: Odd rebase behavior

[John Keeping]

On Tue, Dec 15, 2015 at 09:17:30PM -0600, David A. Greene wrote:
> According to the rebase man page, rebase gathers commits as in "git log
> <upstream>..HEAD."  However, that is not what happens in the tests
> below.  Some of the commits disappear.
> 
> The test basically does this:
> 
> - Setup a master project and a subproject, merged via a subtree-like
>   merge (this is how git-subtree does it).
> 
> - Add some commits to the subproject directory after the subtree merge,
>   to create some history not in the original subproject.
> 
> - filter-branch --subdirectory-filter to extract commits from the
>   subproject directory.
> 
> - Rebase those commits back on to the original subproject repository.
> 
> The above loses all commits made after the subproject is merged into
> the main project.
[snip]
> # Does not preserve master4 and master5.
> test_expect_success 'Rebase default' '
> 	git checkout -b rebase-default master &&
> 	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
> 	git commit -m "Empty commit" --allow-empty &&
> 	git rebase -Xsubtree=files_subtree  --preserve-merges --onto files-master master &&

It seems that the problem is introduces by --preserve-merges (and
-Xsubtree causes something interesting to happen as well).  I see the
following behaviour:

git rebase --onto files-master master

	Works (master4 and master5 preserved).

git rebase --preserve-merges --onto files-master master

	Behaves as described above (master4 and master5 are lost).

git rebase -Xsubtree=files_subtree --onto files-master master

	fatal: Could not parse object 'b15c4133fc3146e1330c84159886f0f7a09fbf43^'
	Unknown exit code (128) from command: git-merge-recursive b15c4133fc3146e1330c84159886f0f7a09fbf43^ -- HEAD b15c4133fc3146e1330c84159886f0f7a09fbf43 

git rebase -Xsubtree=files_subtree --preserve-merges --onto files-master master

	Same as the version with only --preserve-merges.

Re: Odd rebase behavior

[David A. Greene]

John Keeping <john@keeping.me.uk> writes:

> It seems that the problem is introduces by --preserve-merges (and
> -Xsubtree causes something interesting to happen as well).  I see the
> following behaviour:

Thanks for narrowing this down!  Is it possible this is actually a
cherry-pick problem since --preserve-merges forces rebase to use
cherry-pick?

> git rebase -Xsubtree=files_subtree --onto files-master master
>
> 	fatal: Could not parse object 'b15c4133fc3146e1330c84159886f0f7a09fbf43^'
> 	Unknown exit code (128) from command: git-merge-recursive
> b15c4133fc3146e1330c84159886f0f7a09fbf43^ -- HEAD
> b15c4133fc3146e1330c84159886f0f7a09fbf43

Ah, good!  I had seen this behavior as well but couldn't remember what I
did to trigger it.

I don't think I have the expertise to fix rebase and/or cherry-pick.
What's the process for adding these tests to the testbase and marking
them so the appropriate person can fix them?  I see a lot of TODO tests.
Should I mark these similarly and propose a patch to the testbase?

                             -David

Re: Odd rebase behavior

[John Keeping]

On Fri, Dec 18, 2015 at 11:43:16AM -0600, David A. Greene wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > It seems that the problem is introduces by --preserve-merges (and
> > -Xsubtree causes something interesting to happen as well).  I see the
> > following behaviour:
> 
> Thanks for narrowing this down!  Is it possible this is actually a
> cherry-pick problem since --preserve-merges forces rebase to use
> cherry-pick?

I'm pretty sure this a result of the code in git-rebase--interactive.sh
just below the comment "Watch for commits that have been dropped by
cherry-pick", which filters out certain commits.  However, I'm not at
all familiar with the --preserve-merges code in git-rebase so I could be
completely wrong.

> > git rebase -Xsubtree=files_subtree --onto files-master master
> >
> > 	fatal: Could not parse object 'b15c4133fc3146e1330c84159886f0f7a09fbf43^'
> > 	Unknown exit code (128) from command: git-merge-recursive
> > b15c4133fc3146e1330c84159886f0f7a09fbf43^ -- HEAD
> > b15c4133fc3146e1330c84159886f0f7a09fbf43
> 
> Ah, good!  I had seen this behavior as well but couldn't remember what I
> did to trigger it.
> 
> I don't think I have the expertise to fix rebase and/or cherry-pick.
> What's the process for adding these tests to the testbase and marking
> them so the appropriate person can fix them?  I see a lot of TODO tests.
> Should I mark these similarly and propose a patch to the testbase?

I think marking them with test_expect_failure (instead of
test_expect_success) is enough.

Re: Odd rebase behavior

[Johannes Sixt]

Am 18.12.2015 um 19:05 schrieb John Keeping:
> On Fri, Dec 18, 2015 at 11:43:16AM -0600, David A. Greene wrote:
>> John Keeping <john@keeping.me.uk> writes:
>>
>>> It seems that the problem is introduces by --preserve-merges (and
>>> -Xsubtree causes something interesting to happen as well).  I see the
>>> following behaviour:
>>
>> Thanks for narrowing this down!  Is it possible this is actually a
>> cherry-pick problem since --preserve-merges forces rebase to use
>> cherry-pick?
>
> I'm pretty sure this a result of the code in git-rebase--interactive.sh
> just below the comment "Watch for commits that have been dropped by
> cherry-pick", which filters out certain commits.  However, I'm not at
> all familiar with the --preserve-merges code in git-rebase so I could be
> completely wrong.
>
>>> git rebase -Xsubtree=files_subtree --onto files-master master
>>>
>>> 	fatal: Could not parse object 'b15c4133fc3146e1330c84159886f0f7a09fbf43^'
>>> 	Unknown exit code (128) from command: git-merge-recursive
>>> b15c4133fc3146e1330c84159886f0f7a09fbf43^ -- HEAD
>>> b15c4133fc3146e1330c84159886f0f7a09fbf43
>>
>> Ah, good!  I had seen this behavior as well but couldn't remember what I
>> did to trigger it.
>>
>> I don't think I have the expertise to fix rebase and/or cherry-pick.
>> What's the process for adding these tests to the testbase and marking
>> them so the appropriate person can fix them?  I see a lot of TODO tests.
>> Should I mark these similarly and propose a patch to the testbase?
>
> I think marking them with test_expect_failure (instead of
> test_expect_success) is enough.
>

There are a few known breakages recorded in 
t3512-cherry-pick-submodule.sh. Perhaps the one observed here is already 
among them?

-- Hannes

Re: Odd rebase behavior

[John Keeping]

On Fri, Dec 18, 2015 at 06:05:49PM +0000, John Keeping wrote:
> On Fri, Dec 18, 2015 at 11:43:16AM -0600, David A. Greene wrote:
> > John Keeping <john@keeping.me.uk> writes:
> > 
> > > It seems that the problem is introduces by --preserve-merges (and
> > > -Xsubtree causes something interesting to happen as well).  I see the
> > > following behaviour:
> > 
> > Thanks for narrowing this down!  Is it possible this is actually a
> > cherry-pick problem since --preserve-merges forces rebase to use
> > cherry-pick?
> 
> I'm pretty sure this a result of the code in git-rebase--interactive.sh
> just below the comment "Watch for commits that have been dropped by
> cherry-pick", which filters out certain commits.  However, I'm not at
> all familiar with the --preserve-merges code in git-rebase so I could be
> completely wrong.

I've traced through git-rebase--interactive.sh and I can see what's
happening here now.  The problematic code is around the handling of the
"rewritten" directory.

In --preserve-merges mode, we write the SHA1 of the "onto" commit into
rewritten and then add any commits descended from it along the
first-parent path that we have identified as candidates for being
rebased.  This allows us to identify commits that have been merged in
and remove them from the rebase instruction list.

Because the right-hand commit in this case is disjoint from "onto", we
end up dropping everything at this point.  The --root option fixes this
because instead of preserving just "onto", it adds all of the commits
given by:

	git merge-base --all $orig_head $upstream

Since the disjoint history causes the root commit to be rewritten, I
think requiring --root for this case to work is reasonable.  However,
I'm not sure we should add it automatically.  It may be better to detect
that there is no common history and die if --root has not been given.