Nathan Manceaux-PanotMay 11

Whoa: just found a crashing bug in Retcon, that I wrote *four years* ago. It’s easy to trigger, too: just ⌘↓ a commit that’s right above the very first commit in the history.

(it’s an array indexing bug: the commit has no parent now, so the lookup fails. these days I use `safeIndex:` everywhere.)

Show threadRob NapierMay 11

@Cykelero safeIndex patterns point to your problem. If you’re not sure that the index actually comes from the collection, then you have to become sure, not add a partial fallback.

If your collection can change independently of your stored index, then the index is invalid. “Safe”Index only does anything if it happens to also be out of range. It could just as easily be valid but point to the wrong item.

It is generally not safe to store indexes of mutable collections.

Show threadNathan Manceaux-PanotMay 11
@cocoaphony I see what you mean. Things here are all good: the `selectedViewCommitIndices` value we’re reading is a computed property; it reads `tableView.selectedRowIndexes`, and converts them to `cachedViewCommits` indices, by performing lookups in this same collection. So it’s by definition always up to date!
Show threadNathan Manceaux-PanotMay 11

@cocoaphony Here, the expected value really is nil. Having a nil parent means that a commit is root; when moving a commit down, if it becomes the first commit in the history (i.e. the root), then it should be assigned a nil parent.

This code has been around for a while, and I’m really not one to write things I don’t (think that I) understand. I very much appreciate the vigilance though!

And, maybe that was also your point, an explicit `idx == endIndex` check would lock things down even better

Show threadRob Napier
@Cykelero I don't think you want to add yet another index check. You want to treat indexes as opaque. Even that +2 is a bit risky. You mentioned that you are "converting" table view indexes into cachedViewCommits indexes, and that step is probably wrong due to a race condition. If you have a a cache, it's probably wiser to manage it as a dictionary and look things up by an identifier rather than trying to maintain parallel arrays. Parallel arrays are really hard to keep right in concurrent code.
May 11 at 1:25pmWeb