I claim that it is impossible to hit the `continue` which is removed in
this commit. Setup:

- `events` is a list of all membership events in the range
  `since < time <= now`.
- `non_joins` is the list of `e in events` with `e.membership` not equal
  to`"join"`.
- `events` is a nonempty list by construction of
  `mem_change_events_by_room_id`.

Rationale:

- We hit the deleted code only if `non_joins` is empty.
- If so, `events` consists only of `join` membership events.
- `events` is non_empty, so there was at least one join during the
  sync period.
- Therefore the room_id will belong to
  `sync_result_builder.joined_room_ids`. But this means we will have
 `continue`d in the branch above.
  - I'm assuming here that `joined_room_ids` and `events` are both using
   the same `now_token.room_key`.

Doing so makes it possible we'll repeat a DB query. In the next commit,
I'll demonstrate that we always call this exactly once.

I claim this is identical to the existing behaviour. Proof: consider the
boolean `room_id in sync_result_builder.joined_room_ids or has_join`.
If this is true, we make the first call to `_fetch_membership_event_at`.
Otherwise:

- `room_id not in sync_result_builder.joined_room_ids` and `not has_join`.
- The former means we continue on to inspect `events[-1].membership`.
- This is not `"join"`, or else `room_id in
  sync_result_builder.joined_room_ids` would be true.
- `has_join` is False, so we hit the `else` branch and make the second
  call to `_fetch_membership_event_at`.

So, assuming we continue beyond the first `continue`, we always call
fetch the old membership event exactly once. Do it up front to make the
reader's life easier.

I claim this check is redundant, because said membership cannot be join.
If not, we'd have `continue`d above because `room_id` would be in
`sync_result_builder.joined_room_ids`.

I prefer `if .. elif` where possible, because I know for sure that at
most one branch will execute.

Co-authored-by: Dan Callahan <[email protected]>