| Message ID | 5b0d5b6e-5055-6323-1b6c-fe98137e81f6@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 279f42fa27b4ffd0b3ac7c9378043eeb413f0f5a |
| Headers | show |
| Series | [v3,1/4] worktree: introduce is_shared_symref() | expand |
On Sun, Feb 05 2023, Rubén Justo wrote: > In b5cabb4a9 (rebase: refuse to switch to branch already checked out > elsewhere, 2020-02-23) we add a condition to prevent a rebase operation > involving a switch to a branch that is already checked out in another > worktree. > > A bug has recently been fixed that caused this to not work as expected. Presumably in 1/4 and 2/4, if so saying "in the preceding commit(s)" would be better. > Let's add a test to notice if this changes in the future. I for one would find this series much easier to follow if you started with this test, possibly with a test_expect_failure, and as we fix the relevant code flip them (both this and the subsequent one) to run successfully, and include them as part of the commit that fixes the bug). Maybe there's reasons for why that's tricky to do in this case, so please ignore this if so.
On 06-feb-2023 17:59:11, Ævar Arnfjörð Bjarmason wrote: > > Let's add a test to notice if this changes in the future. > > I for one would find this series much easier to follow if you started > with this test, possibly with a test_expect_failure, and as we fix the > relevant code flip them (both this and the subsequent one) to run > successfully, and include them as part of the commit that fixes the > bug). > > Maybe there's reasons for why that's tricky to do in this case, so > please ignore this if so. I'll give it try, I like the idea. Thanks.
Hi Rubén On 06/02/2023 23:16, Rubén Justo wrote: > On 06-feb-2023 17:59:11, Ævar Arnfjörð Bjarmason wrote: > >>> Let's add a test to notice if this changes in the future. >> >> I for one would find this series much easier to follow if you started >> with this test, possibly with a test_expect_failure, and as we fix the >> relevant code flip them (both this and the subsequent one) to run >> successfully, and include them as part of the commit that fixes the >> bug). >> >> Maybe there's reasons for why that's tricky to do in this case, so >> please ignore this if so. > > I'll give it try, I like the idea. Thanks. Squashing the last three commits together so that the tests are introduced in the same commit as the fix as Junio suggested in his comments on the previous round would be very welcome. Best Wishes Phillip
On 07-feb-2023 10:52:39, Phillip Wood wrote: > Hi Rubén > > On 06/02/2023 23:16, Rubén Justo wrote: > > On 06-feb-2023 17:59:11, Ævar Arnfjörð Bjarmason wrote: > > > > > > Let's add a test to notice if this changes in the future. > > > > > > I for one would find this series much easier to follow if you started > > > with this test, possibly with a test_expect_failure, and as we fix the > > > relevant code flip them (both this and the subsequent one) to run > > > successfully, and include them as part of the commit that fixes the > > > bug). > > > > > > Maybe there's reasons for why that's tricky to do in this case, so > > > please ignore this if so. > > > > I'll give it try, I like the idea. Thanks. > > Squashing the last three commits together so that the tests are introduced > in the same commit as the fix as Junio suggested in his comments on the > previous round would be very welcome. Yes, I considered that, but I think that keeping the tests in their own commit is reasonable. The tests protect against a malfunction that we did not notice when the implementation was done. The commits reference (and use similar subjects) to the original commit where we should have introduced the tests. If the tests fail, I think it will be easier and less confusing to reach the original commit where the implementation was done if we keep them separated, rather than combining all three commits. I'm going to reorder the commits and change to use test_expect_failure(). This way the commit with the fix will also be linked.
Rubén Justo <rjusto@gmail.com> writes: > ... If the tests fail, I think it will be easier and > less confusing to reach the original commit where the implementation was > done if we keep them separated, rather than combining all three commits. No, if the test were in the same commit as the implementation, then upon a future test breakage it is easier to see what code the test was meant to protect, exactly because it is part of the same commit. > I'm going to reorder the commits and change to use test_expect_failure(). Do not do that. Adding a failing test and flipping the expect_failure to expect_success is a sure way to make the series harder to review.
On 07-feb-2023 21:19:33, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > > ... If the tests fail, I think it will be easier and > > less confusing to reach the original commit where the implementation was > > done if we keep them separated, rather than combining all three commits. > > No, if the test were in the same commit as the implementation, then > upon a future test breakage it is easier to see what code the test > was meant to protect, exactly because it is part of the same commit. Yes, but my reasoning was that those tests protect what we did in 8d9fdd7 (worktree.c: check whether branch is rebased in another worktree, 2016-04-22) and in b5cabb4a9 (rebase: refuse to switch to branch already checked out elsewhere, 2020-02-23), not die_if_checked_out(). > > I'm going to reorder the commits and change to use test_expect_failure(). > > Do not do that. Adding a failing test and flipping the > expect_failure to expect_success is a sure way to make the series > harder to review. Hmm
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index d5a8ee39fc..3ce918fdb8 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -388,6 +388,20 @@ test_expect_success 'switch to branch checked out here' ' git rebase main main ' +test_expect_success 'switch to branch checked out elsewhere fails' ' + test_when_finished " + git worktree remove wt1 && + git worktree remove wt2 && + git branch -d shared + " && + git worktree add wt1 -b shared && + git worktree add wt2 -f shared && + # we test in both worktrees to ensure that works + # as expected with "first" and "next" worktrees + test_must_fail git -C wt1 rebase shared shared && + test_must_fail git -C wt2 rebase shared shared +' + test_expect_success 'switch to branch not checked out' ' git checkout main && git branch other &&
In b5cabb4a9 (rebase: refuse to switch to branch already checked out elsewhere, 2020-02-23) we add a condition to prevent a rebase operation involving a switch to a branch that is already checked out in another worktree. A bug has recently been fixed that caused this to not work as expected. Let's add a test to notice if this changes in the future. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- t/t3400-rebase.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+)