Odd rebase behavior

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

On Tue, Dec 15, 2015 at 09:17:30PM -0600, David A. Greene wrote:

[snip]

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.

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

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

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

On Fri, Dec 18, 2015 at 11:43:16AM -0600, David A. Greene wrote:

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 think marking them with test_expect_failure (instead of test_expect_success) is enough.

Am 18.12.2015 um 19:05 schrieb John Keeping:

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

-- Hannes

On Fri, Dec 18, 2015 at 06:05:49PM +0000, John Keeping wrote:

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.