This workaround is hacky, but it's lightweight and non-intrusive, and it's probably good to have it even if we fix the bug properly since the search logic is complex and there's no reason why it should wrap more than once in any case.

I've opened PR #1371 with a real fix for the bug... still probably a good idea to merge both.

Looks fine. If you're confident that the other fix covers all cases you could BUG_ON instead.

IMO the comment could still be shortened considering how blunt the existing codebase tends to be.

BUG_ON instead

I did that in my first iteration, but I'm really not all that confident the search isn't subtly broken somewhere else (for an unrelated reason)... it took me long enough to understand the tree search enough to fix it.

I figure that if this gets hit, we shouldn't crash cmus since we don't have to, it takes a while to get bugfixes out, and if it's broken to the point of not finding something, someone will eventually open an issue and we'll see this in the logs.

IMO the comment could still be shortened considering how blunt the existing codebase tends to be.

The length of comments I write tends to scale with how absurd the code it's commenting is, and how long it took me to figure out what led to me writing it. I do think cmus needs some more comments in general.

Fair point, though. @flyingmutant?

Regarding BUG_ON – not a fan of this kind of testing in production in any software.

As for the comment – I would agree in general that the codebase can be improved with tasteful sprinkling of comments (very hard to have hard rules here, it is purely question of experience/taste). However, if we are discussing this specific one – I think it can be shortened without losing anything useful.

Something like Workaround possible double-wrap bug in ..., see ... would be my take, but this is purely a matter of preference.

I edited the comment.