Thanks for the useful tips!

Queued this, thanks.

+ / Add rev's parents to self.bases
+ #[inline]
+ fn add_parents(&mut self, rev: Revision) -> Result<(), GraphError> {
+ No need to bother the set with inserting NULL_REVISION over and
+ // over
+ for p in self.graph.parents(rev)?.iter().cloned() {
+ if p != NULL_REVISION {
+ self.bases.insert(p);
+ }
+ }
+ Ok(())
+ }

The comment sounds like we don't care the NULL_REVISION (i.e. the bases could
be extend()-ed), but we do explicitly avoid doing that.

+ let mut missing: Vec<Revision> = Vec::new();
+ for curr in (0..=start).map(|i| start - i) {

Nit: can be (0..=start).rev() ?

+ if revs_visit.is_empty() {
+ break;
+ }
+ if both_visit.contains(&curr) {
+ curr's parents might have made it into revs_visit through
+ another path
+ TODO optim: Rust's HashSet.remove returns a boolean telling
+ if it happened. This will spare us one set lookup
+ both_visit.remove(&curr);
+ for p in self.graph.parents(curr)?.iter().cloned() {
+ if p == NULL_REVISION {
+ continue;
+ }
+ revs_visit.remove(&p);
+ bases_visit.insert(p);
+ both_visit.insert(p);
+ }
+ continue;
+ }
+ in Rust, one can't just use mutable variables assignation
+ to be more straightforward. Instead of Python's
+ thisvisit and othervisit, we'll differentiate with a boolean
+ let this_visit_is_revs = {
+ if revs_visit.remove(&curr) {
+ missing.push(curr);
+ true
+ } else if bases_visit.contains(&curr) {
+ false
+ } else {
+ not an ancestor of revs or bases: ignore
+ continue;
+ }
+ };

Maybe this could be extracted to a function
visit_parents(&mut revs_visit, &mut bases_visit, select_this, select_other)
where select_this and select_other are FnMut(&mut, &mut) -> &mut, but
I don't know if that will improve the readability.

>  +            // in Rust, one can't just use mutable variables assignation
>  +            // to be more straightforward. Instead of Python's
>  +            // thisvisit and othervisit, we'll differentiate with a boolean
>  +            let this_visit_is_revs = {
>  +                if revs_visit.remove(&curr) {
>  +                    missing.push(curr);
>  +                    true
>  +                } else if bases_visit.contains(&curr) {
>  +                    false
>  +                } else {
>  +                    // not an ancestor of revs or bases: ignore
>  +                    continue;
>  +                }
>  +            };
Maybe this could be extracted to a function
`visit_parents(&mut revs_visit, &mut bases_visit, select_this, select_other)`
where `select_this` and `select_other` are `FnMut(&mut, &mut) -> &mut`, but
I don't know if that will improve the readability.

Actually it looks better if I copy-paste the visit loop. I'll send follow-up
patches.

if both_visit.contains(&curr) {
    for p in ...
} else if revs_visit.remove(&curr) {
    missing.push(curr);
    for p in ...
} else if bases_visit.contains(&curr) {
    for p in ...
}