Re: [PATCH v4 0/4] fix die_if_checked_out() when ignore_current_worktree
- From
Rubén Justo <rjusto@gmail.com>
- Date
- Feb 27, 2023, 00:00 UTC
- Message-ID
- <7a8dc2ee-a8a3-b59e-f2ea-a7720320fb7c@gmail.com>
- In-Reply-To
- <xmqqbklhuzq8.fsf@gitster.g>
On 25-feb-2023 14:50:07, Junio C Hamano wrote:
Show 25 quoted lines
> Rubén Justo <rjusto@gmail.com> 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.