On Fri, Jan 31, 2025 at 1:20 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Olga Pilipenco <olga.pilipenco@xxxxxxxxxxx> writes: > > >> Perhaps the logic is clear to those who diagnosed the problem, wrote > >> the patch, and reviewed it, in which case there is no reason to > >> reroll. Perhaps it was just me to whom it was not obvious that > >> the purpose of "is_current" check was not about "are we looking at > >> the main worktree" but was about "if we are not in the main worktree, > >> we need this extra check". > >> > >> Thanks. > > > > You did a great job figuring it out and I agree it's confusing at > > first, but we tried our best to make it less confusing. > > `is_current` check is actually not necessary there, but having it there saves > > extra unnecessary calculations, also describes & fixes the exact scenario > > that didn't work (not being able to see main worktree as bare from a > > secondary worktree). > > If I had to do a great job there, then the code does deserve to be > explained a bit better for later developers who wonder why it is > written in the way it is, perhaps we a single-liner comment? I have 2 versions for comment: 1. Since is_main_worktree_bare explains quite well what it does we can have a shorter explanation of `!worktree->is_current` part, something like: /* Additional checks are needed if main worktree is not current (checking from secondary worktree) */ (!worktree->is_current && is_main_worktree_bare(the_repository)); 2. Or a bit longer inline explanation that partially repeats the explanation of is_main_worktree_bare + adds explanation about efficiency: /* * When in a secondary worktree we have to also verify if the main worktree * is bare in $commondir/config.worktree. * This check is unnecessary if we're currently in the main worktree, * as prior checks already consulted all configs of the current worktree. */ (!worktree->is_current && is_main_worktree_bare(the_repository)); Let me know if any of these work. Thanks. > Thanks.
