On 25-feb-2023 14:50:07, Junio C Hamano wrote: > Rubén Justo <rjusto@xxxxxxxxx> writes: > > > + test_when_finished " > > -+ git worktree remove wt1 && > > -+ git worktree remove wt2 && > > -+ git branch -d shared > > -+ git checkout - > > ++ git worktree remove wt1 ||: > > ++ git worktree remove wt2 ||: > > ++ git checkout - ||: > > ++ git branch -D shared ||: > > + " && > > Sorry, but I do not get the point of this construct. The > test_cleanup variable that accumulates test_when_finished scripts is > evaled without -e shopt set, so you can just remove all these ||: > and add a single "true" at the end, like > > ... > git checkout - > git branch -D shared > : > " && > > for exactly the same effect, no? Yes, of course. I agree that all of that '||:' can be confusing, but I'm not sure if a final ':' is much better. As I said in my response to Eric, I don't have a clear opinion here. test_when_finished() was introduced in 2bf78867 (test-lib: Let tests specify commands to be run at end of test, 2010-05-02) to allow indicating some actions to be executed after the test block finishes, even if the test fails, to leave a repository or overall situation healthy for the next tests. With that in mind, I ask myself, is it worth removing the worktrees here? Is it worth reporting a possible error in that removal? If, for example, in the future we deny the removal of a worktree under bisect, is it worth reporting the failure here? What about the "shared" branch? ... I Dunno. Thank you for reviewing this patch.
