One of the comments we are deleting here said:
// Comparing just the hash is not enough; we need to compare both the
// action and the hash, as the hash could appear multiple times (e.g. in a
// pick and later in a merge).
I don't remember what I was thinking when I wrote this code, but it's nonsense
of course. Maybe I was thinking that the hash that appears in a "merge" todo
would be the hash of the commit that is being merged in (which would then
actually appear in an earlier pick), but it isn't, it's the hash of the merge
commit itself (so that the rebase can reuse its commit message). Which means
that hashes are unique, no need to compare the action.
So far it didn't have to handle the case where one hash is empty and the other
isn't, but in the next commit we need that, so let's handle that case correctly.
There's enough logic in the function now that it's worth covering it with tests.
Sometimes it takes a while to get PRs accepted upstream, and this blocks our
progress. Since I'm pretty much the only one making changes there anyway, it
makes sense to point to my fork directly.
The assert package is already very good at displaying errors, including printing
a diff of expected and actual value, so there's no point in printing the same
information again ourselves.
