Re: [RFC PATCH] add t3420-rebase-topology

From: Johannes Sixt <j.sixt@viscovery.net>
Date: Sep 18, 2012, 07:53 UTC
Message-ID: <50582873.603@viscovery.net>
In-Reply-To: <1347949878-12578-1-git-send-email-martinvonz@gmail.com>

Am 9/18/2012 8:31, schrieb Martin von Zweigbergk:

Show 56 quoted lines

> Add more test cases to check that the topology after a rebase is as
> expected. Conflicts are not considered, but patch-equivalence is.
> ---
> 
> Tests pass and fail as indicated by the suffix
> (_success/_failure). Your input especially appreciated on whether you
> agree with the intent of the test cases. For example, do you agree
> that 'rebase --onto does not re-apply patches in onto' is desirable?
> And if you do, then do you also agree that 'rebase --root --onto
> ignores patch in onto' is desirable? How about 'rebase --root is not a
> no-op'? One might think that --force would be necessary, but on the
> other hand, if that was the case, the only point (AFAICT) of "git
> rebase --root <branch>" without --force would be to linearize history,
> so I instead made the test case confirm that --root without --onto
> effectively behaves as if --force was also passed.
> 
> Feedback on the structure/setup and style is of course also
> appreciated.
> 
>  t/t3420-rebase-topology.sh | 348 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 348 insertions(+)
>  create mode 100755 t/t3420-rebase-topology.sh
> 
> diff --git a/t/t3420-rebase-topology.sh b/t/t3420-rebase-topology.sh
> new file mode 100755
> index 0000000..024a2b4
> --- /dev/null
> +++ b/t/t3420-rebase-topology.sh
> @@ -0,0 +1,348 @@
> +#!/bin/sh
> +
> +test_description='effect of rebase on topology'
> +. ./test-lib.sh
> +
> +
> +#       q---C---r
> +#      /
> +# a---b---c---d!--e---p
> +#      \
> +#       f---g!--h
> +#        \
> +#         j-------E---k
> +#          \       \
> +#           n---H---w
> +#
> +# x---y---B
> +#
> +#
> +# ! = empty
> +# uppercase = cherry-picked
> +# p = reverted e
> +#
> +# TODO:
> +# prune graph to what's needed
> +
> +empty () {

Is it "is_empty" or "make_empty"/"empty_commit"?

> +	git commit --allow-empty -m $1 &&
> +	git tag $1
> +}

Obviously the latter.

Show 49 quoted lines

> +
> +cherry_pick () {
> +	git cherry-pick -n $1 &&
> +	git commit -m $2 &&
> +	git tag $2
> +}
> +
> +revert () {
> +	git revert -n $1 &&
> +	git commit -m $2 &&
> +	git tag $2
> +}
> +
> +
> +test_expect_success 'setup' '
> +	test_commit a &&
> +	test_commit b &&
> +	test_commit c &&
> +	empty d &&
> +	test_commit e &&
> +	revert e p &&
> +	git checkout b &&
> +	test_commit f &&
> +	empty g &&
> +	test_commit h &&
> +	git checkout f &&
> +	test_commit j &&
> +	cherry_pick e E &&
> +	test_commit k &&
> +	git checkout j &&
> +	test_commit n &&
> +	cherry_pick h H &&
> +	git merge -m w E &&
> +	git tag w &&
> +	git checkout b &&
> +	test_commit q &&
> +	cherry_pick c C &&
> +	test_commit r &&
> +	git checkout --orphan disjoint &&
> +	git rm -rf . &&
> +	test_commit x &&
> +	test_commit y &&
> +	cherry_pick b B
> +'
> +
> +reset () {
> +	git rebase --abort
> +	git reset --hard
> +}

The 'rebase --abort' can fail, but there is no && chain. Good. Using this function instead of the individual commands in the tests does not break the && chain there. Good.

These following three functions should be and can be consistent with the order of their arguments: expected actual.

> +test_range () {
> +	test "$(git log --reverse --topo-order --format=%s "$1" | xargs)" = "$2"

	expected=$1
	set -- $(git log --reverse --topo-order --format=%s "$2")
	test "expected" = "$*"

Show 6 quoted lines

> +}
> +
> +test_revisions () {
> +	expected="$1"
> +	shift
> +	test "$(git log --format=%s --no-walk=unsorted "$@" | xargs)" = "$expected"

	set -- $(git log --format=%s --no-walk=unsorted "$@")
	test "expected" = "$*"

Show 5 quoted lines

> +}
> +
> +same_revision () {
> +	test "$(git rev-parse $1)" = "$(git rev-parse $2)"
> +}

'test_same_revision'?

Show 14 quoted lines

> +
> +# the following 5 (?) tests copy t3400 tests, but check the history rather than status code and/or stdout
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' c j &&
> +	same_revision HEAD~2 c &&
> +	test_range c.. "f j"
> +'
> +}
> +test_expect_success 'simple rebase' "$(run)"
> +test_expect_success 'simple rebase -m' "$(run -m)"
> +test_expect_success 'simple rebase -i' "$(run -i)"
> +test_expect_success 'simple rebase -p' "$(run -p)"

Since here and in the following tests the test cases and test descriptions vary in the same way, wouldn't it make sense to factor the description out as well?

test_run_rebase () {
	test_expect_success "simple rebase $*" "
		reset &&
		git rebase $* c j &&
		same_revision HEAD~2 c &&
		test_range c.. 'f j'
	"
}

test_run_rebase '' test_run_rebase -m test_run_rebase -i test_run_rebase -p

(Watch your quoting, though.)

Oh, I see: some tests expect failure. How about:

test_run_rebase () {
	result=$1
	shift
	test_expect_$result "simple rebase $*" "..."
}
test_run_rebase success ''
etc.

Show 12 quoted lines

> +
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' b j &&
> +	same_revision HEAD j
> +'
> +}
> +test_expect_success 'rebase is no-op if upstream is an ancestor' "$(run)"
> +test_expect_success 'rebase -m is no-op if upstream is an ancestor' "$(run -m)"
> +test_expect_success 'rebase -i is no-op if upstream is an ancestor' "$(run -i)"
> +test_expect_success 'rebase -p is no-op if upstream is an ancestor' "$(run -p)"

OK.

Show 13 quoted lines

> +
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' --force b j &&
> +	! same_revision HEAD j &&
> +	test_range b.. "f j"
> +'
> +}
> +test_expect_success 'rebase --force' "$(run)"
> +test_expect_success 'rebase -m --force' "$(run -m)"
> +test_expect_success 'rebase -i --force' "$(run -i)"
> +test_expect_failure 'rebase -p --force' "$(run -p)"

Do you mean "rebase --force rewrites even if upstream is an ancestor"? That would make sense.

Show 12 quoted lines

> +
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' j b &&
> +	same_revision HEAD j
> +'
> +}
> +test_expect_success 'rebase fast-forwards if an ancestor of upstream' "$(run)"
> +test_expect_success 'rebase -m fast-forwards if an ancestor of upstream' "$(run -m)"
> +test_expect_success 'rebase -i fast-forwards if an ancestor of upstream' "$(run -i)"
> +test_expect_success 'rebase -p fast-forwards if an ancestor of upstream' "$(run -p)"

OK.

Show 12 quoted lines

> +
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' p k &&
> +	test_range p.. "f j k"
> +'
> +}
> +test_expect_success 'rebase ignores patch in upstream' "$(run)"
> +test_expect_failure 'rebase -m ignores patch in upstream' "$(run -m)"
> +test_expect_success 'rebase -i ignores patch in upstream' "$(run -i)"
> +test_expect_success 'rebase -p ignores patch in upstream' "$(run -p)"

"drops" instead of "ignores"? OK.

Show 12 quoted lines

> +
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' c h &&
> +	test_range c.. "f h"
> +'
> +}
> +test_expect_success 'rebase ignores empty commit' "$(run)"
> +test_expect_success 'rebase -m ignores empty commit' "$(run -m)"
> +test_expect_success 'rebase -i ignores empty commit' "$(run -i)"
> +test_expect_success 'rebase -p ignores empty commit' "$(run -p)"

"drops" instead of "ignores"? OK.

Show 12 quoted lines

> +
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' --keep-empty c h &&
> +	test_range c.. "f g h"
> +'
> +}
> +test_expect_success 'rebase --keep-empty' "$(run)"
> +test_expect_failure 'rebase -m --keep-empty' "$(run -m)"
> +test_expect_success 'rebase -i --keep-empty' "$(run -i)"
> +test_expect_failure 'rebase -p --keep-empty' "$(run -p)"

OK.

Show 12 quoted lines

> +
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' --keep-empty p h &&
> +	test_range p.. "f g h"
> +'
> +}
> +test_expect_success 'rebase --keep-empty keeps empty even if already in upstream' "$(run)"
> +test_expect_failure 'rebase -m --keep-empty keeps empty even if already in upstream' "$(run -m)"
> +test_expect_failure 'rebase -i --keep-empty keeps empty even if already in upstream' "$(run -i)"
> +test_expect_failure 'rebase -p --keep-empty keeps empty even if already in upstream' "$(run -p)"

"is in upstream" is decided by the patch text. If an empty commit is already in upstream, this adds another one with the same or a different commit message and authorship information. Dubious, but since it is opt-in, it should be OK.

Show 11 quoted lines

> +
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' E w &&
> +	test_range E.. "n H"
> +'
> +}
> +test_expect_success 'rebase after merge' "$(run)"
> +test_expect_success 'rebase -m after merge' "$(run -m)"
> +test_expect_success 'rebase -i after merge' "$(run -i)"

OK.

Show 13 quoted lines

> +
> +test_expect_success 'rebase -p is no-op in history with merges' '
> +	reset &&
> +	git rebase -p j w &&
> +	same_revision HEAD w
> +'
> +
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' j w &&
> +	test_range j.. "E n H" || test_range j.. "n H E"
> +'

Chaining tests with || is dangerous: you do not know whether the first failed because the condition is not satisfied or because of some other failure.

Why is this needed in the first place? Shouldn't the history be deterministic, provided that the commit timestamps are all distinct?

> +}
> +test_expect_success 'rebase of history with merges is linearized' "$(run)"
> +test_expect_success 'rebase -m of history with merges is linearized' "$(run -m)"
> +test_expect_success 'rebase -i of history with merges is linearized' "$(run -i)"

OK.

Show 12 quoted lines

> +
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' --onto p h k &&
> +	test_range p.. "j k"
> +'
> +}
> +test_expect_failure 'rebase --onto does not re-apply patches in onto' "$(run)"
> +test_expect_failure 'rebase -m --onto does not re-apply patches in onto' "$(run -m)"
> +test_expect_failure 'rebase -i --onto does not re-apply patches in onto' "$(run -i)"
> +test_expect_failure 'rebase -p --onto does not re-apply patches in onto' "$(run -p)"

Makes sense.

Show 13 quoted lines

> +
> +
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' --onto f d r &&
> +	test_range f.. "q C r"
> +'
> +}
> +test_expect_failure 'rebase --onto does not lose patches in upstream' "$(run)"
> +test_expect_success 'rebase -m --onto does not lose patches in upstream' "$(run -m)"
> +test_expect_failure 'rebase -i --onto does not lose patches in upstream' "$(run -i)"
> +test_expect_failure 'rebase -p --onto does not lose patches in upstream' "$(run -p)"

Makes sense.

Show 13 quoted lines

> +
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' --root c &&
> +	! same_revision HEAD c &&
> +	test_range c "a b c"
> +'
> +}
> +test_expect_success 'rebase --root is not a no-op' "$(run)"
> +test_expect_success 'rebase -m --root is not a no-op' "$(run -m)"
> +test_expect_success 'rebase -i --root is not a no-op' "$(run -i)"
> +test_expect_success 'rebase -p --root is not a no-op' "$(run -p)"

Why? Is it more like "--root implies --force"?

Show 12 quoted lines

> +
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' --root --onto e y &&
> +	test_range e.. "x y"
> +'
> +}
> +test_expect_success 'rebase --root --onto' "$(run)"
> +test_expect_failure 'rebase -m --root --onto' "$(run -m)"
> +test_expect_success 'rebase -i --root --onto' "$(run -i)"
> +test_expect_success 'rebase -p --root --onto' "$(run -p)"

Where does this rebase start? Ah, --root stands in for the "upstream" argument, hence, y is the tip to rebase. Right? Then it makes sense.

Show 13 quoted lines

> +
> +
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' --root --onto e B &&
> +	test_range e.. "x y"
> +'
> +}
> +test_expect_success 'rebase --root --onto ignores patch in onto' "$(run)"
> +test_expect_failure 'rebase -m --root --onto ignores patch in onto' "$(run -m)"
> +test_expect_success 'rebase -i --root --onto ignores patch in onto' "$(run -i)"
> +test_expect_success 'rebase -p --root --onto ignores patch in onto' "$(run -p)"

Makes sense.

Show 13 quoted lines

> +
> +
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' e y &&
> +	test_range e.. "x y"
> +'
> +}
> +test_expect_success 'rebase without --root works on disjoint history' "$(run)"
> +test_expect_failure 'rebase -m without --root works on disjoint history' "$(run -m)"
> +test_expect_success 'rebase -i without --root works on disjoint history' "$(run -i)"
> +test_expect_failure 'rebase -p without --root works on disjoint history' "$(run -p)"

OK.

Show 13 quoted lines

> +
> +
> +run () {
> +echo '
> +	reset &&
> +	git rebase '"$@"' --root --onto p k &&
> +	test_range p.. "f j k"
> +'
> +}
> +test_expect_success 'rebase --root --onto with merge-base ignores --root' "$(run)"
> +test_expect_failure 'rebase -m --root --onto with merge-base ignores --root' "$(run -m)"
> +test_expect_success 'rebase -i --root --onto with merge-base ignores --root' "$(run -i)"
> +test_expect_success 'rebase -p --root --onto with merge-base ignores --root' "$(run -p)"

Makes sense.

Show 7 quoted lines

> +
> +test_expect_success 'rebase -p re-creates merge from upstream' '
> +	reset &&
> +	git rebase -p k w &&
> +	same_revision HEAD^ H &&
> +	same_revision HEAD^2 k
> +'

IMO, this tests the wrong thing. You have this history:

 ---j-------E---k
     \       \
      n---H---w

where E is the second parent of w. What does it mean to rebase w onto k? IMO, it is a meaningless operation, and the outcome is irrelevant.

It would make sense to test that this history results after the upstream at H moved forward:

 ---j-------E---k
     \       \
      n---H   \
           \   \
            z---w'

That is, w began a topic by mergeing the sidebranch E; then upstream advanced to z, and now you rebase the topic to the new upstream.

Show 5 quoted lines

> +
> +test_expect_success 'rebase -p re-creates internal merge' '
> +	reset &&
> +	git rebase -p c w &&
> +	test_revisions "f j n E H w" HEAD~4 HEAD~3 HEAD~2 HEAD^2 HEAD^ HEAD

You must also test for c; otherwise the test would succeed if rebase did nothing at all.

This comment applies to all other tests as well, even the "regular" rebase tests above. (But I noticed only when I read this test.)

Show 25 quoted lines

> +'
> +
> +test_expect_success 'rebase -p rebuilds history around dropped commit matching upstream' '
> +	reset &&
> +	git rebase -p h w &&
> +	test_revisions "j E n w" HEAD~2 HEAD^2 HEAD^ HEAD
> +'
> +
> +test_expect_success 'rebase -p drops merge commit when one entire side is dropped' '
> +	reset &&
> +	git rebase -p p w &&
> +	test_range p.. "f j n H"
> +'
> +
> +test_expect_failure 'rebase -p --onto drops commit in <onto>' '
> +	reset &&
> +	git rebase -p --onto p f w &&
> +	test_range p.. "j n H"
> +'
> +
> +test_expect_success 'rebase -p with two paths to $from' '
> +	reset &&
> +	git rebase -p --onto c j w &&
> +	test_revisions "c n E H w" HEAD~3 HEAD~2 HEAD^2 HEAD^ HEAD
> +'

I didn't look at these remaining -p tests because I ran out of time.

After this plethora of tests, can we get rid of some or many from other test scripts? (t34* tests are the ones that take the longest on Windows to run.)

A nice summary of the rebase behavior. I like it.

-- Hannes