| Message ID | cac51a949eed0fa593247a593aae2b100be6f4f2.1647546828.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: update HEAD when is an oid | expand |
"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.
Junio C Hamano <gitster@pobox.com> writes: >> + 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" Scratch that. No, we do not want HEAD to be pointing at 'main' after rebase, so the above is totally wrong. What you have at the end of the series is the right version, as I said in my review of the [PATCH 3/3]. Thanks for working on this topic.
Hi Junio, On 17 Mar 2022, at 17:10, Junio C Hamano wrote: > "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. I was attemping to follow the advice to "show" vs "tell" in [1]. All this make sense to me however. > > 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. 1. https://lore.kernel.org/git/220317.86r170d6zs.gmgdl@evledraar.gmail.com/ > >> +' > > Thanks.
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 71b1735e1dd..5c4073f06d6 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -399,6 +399,15 @@ test_expect_success 'switch to branch not checked out' ' git rebase main other ' +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 +' + test_expect_success 'refuse to switch to branch checked out elsewhere' ' git checkout main && git worktree add wt &&