Re: [PATCH v4 1/3] rebase: test showing bug in rebase with non-branch

[Junio C Hamano <gitster@pobox.com>]

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: John Cai <johncai86@gmail.com>
>
> Currently when rebase is used with a non branch, and <oid> is up to
> date with base:
>
> git rebase base <oid>
>
> It will update the ref that HEAD is pointing at to <oid>, and leave HEAD
> unmodified.
>
> This is a bug. The expected behavior is that the branch HEAD points at
> remains unmodified while HEAD is updated to point to <oid> in detached
> HEAD mode.

Never do tests this way.

The primary reason why we do not want to write our tests the way
this patch does is because we do not _care_ how it is broken in the
behaviour of the original code.  'main' moving out of $old_main is
the bug we care about.  It is still buggy if it did not move to
Second, but some other commit.  Yet this patch insists that 'main'
to move to Second and nothing else.  What we want is 'main' to stay
at $old_main in the end anyway, and we should directly test that
condition.

If you insist to have two commits (which I strongly recommend
against in this case), you write a test that makes sure that 'main'
stays at $old_main, but mark the test with test_expect_failure.  And
then later in the step that fixes the code, flip "expect_failure" to
"expect_success".

But it is not ideal, either.  Imagine what you see in "git show"
output of the commit that fixed the problem.  Most of the test that
shows the behaviour that the commit _cares_ about will be outside
post-context of the hunk that flips test_expect_failure to
test_expect_success.

The best and the simplest way, for a simple case like this, to write
test is to add the test to expect what we want to see in the end,
and do so in the same commit as the one that corrects the behaviour
of the code.  If somebody wants to see what the breakage looks like,
it is easy to

 (1) checkout the commit that fixes the code and adds such a test,

 (2) tentatively revert everything outside t/, and

 (3) run the test with "-i -v" options.

Then test_expect_success that wants to see 'main' to stay at
$old_main will show that 'main' moved by a test failure.  Working
from a patch is the same way, i.e. you can apply only the parts
inside t/ and run the current code to see the breakage, and then
apply the rest to see the fix.

> +test_expect_success 'switch to non-branch changes branch HEAD points to' '
> +	git checkout main &&
> +	old_main=$(git rev-parse HEAD) &&
> +	git rebase First Second^0 &&

> +	test_cmp_rev HEAD main &&
> +	test_cmp_rev main $(git rev-parse Second) &&
> +	git symbolic-ref HEAD

I already said that the second one should expect main to be at
$old_main, but the "HEAD and main are the same" and "HEAD is a
symolic-ref" test can be replaced with a single test that is "HEAD
is a symbolic-ref to 'main'", which would be more strict.  I.e.

	test "$(git symbolic-ref HEAD)" = refs/heads/main &&
	test_cmp_rev main "$old_main"

And such a test that expects the correct behaviour we want to have
in the end should be added in [PATCH 3/3] when the code is fixed,
not here in a separate commit.

> +'

Thanks.