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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 17, 2022, 21:10 UTC
Message-ID: <xmqqo824e145.fsf@gitster.g>
In-Reply-To: <cac51a949eed0fa593247a593aae2b100be6f4f2.1647546828.git.gitgitgadget@gmail.com>

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

Show 13 quoted lines

> 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.