This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5944

rust: stop putting NULL_REVISION in MissingAncestors.bases
ClosedPublic

Authored by gracinet on Feb 12 2019, 7:02 AM.

Details

Reviewers
kevincox
Group Reviewers
hg-reviewers
Commits
rHG977432970080: rust: stop putting NULL_REVISION in MissingAncestors.bases
Summary

As noted in initial review of MissingAncestors, adding
NULL_REVISION in constructor in case the given bases is
empty wasn't really useful, yet it's been kept for identity
with the Python implementation

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

gracinet created this revision.Feb 12 2019, 7:02 AM
kevincox accepted this revision.Feb 12 2019, 9:54 AM
kevincox added inline comments.
rust/hg-core/src/ancestors.rs
245

I think it would be more clear if you filtered the NULL_REVISION out before extending the set.

let new_bases = new_bases.into_iter().filter(|&rev| rev != NULL_REVISION);
self.bases.extend(new_bases);
gracinet updated this revision to Diff 14056.Feb 13 2019, 6:55 AM
Closed by commit rHG977432970080: rust: stop putting NULL_REVISION in MissingAncestors.bases (authored by gracinet). · Explain WhyFeb 15 2019, 10:34 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/ancestors.rs (19 lines)

Diff 14126

rust/hg-core/src/ancestors.rs

self.inclusive, self.inclusive,
) )
.unwrap() .unwrap()
} }
} }
impl<G: Graph> MissingAncestors<G> { impl<G: Graph> MissingAncestors<G> {
pub fn new(graph: G, bases: impl IntoIterator<Item = Revision>) -> Self { pub fn new(graph: G, bases: impl IntoIterator<Item = Revision>) -> Self {
let mut bases: HashSet<Revision> = bases.into_iter().collect(); MissingAncestors { graph: graph, bases: bases.into_iter().collect() }
if bases.is_empty() {
bases.insert(NULL_REVISION);
}
MissingAncestors { graph, bases }
} }
pub fn has_bases(&self) -> bool { pub fn has_bases(&self) -> bool {
self.bases.iter().any(|&b| b != NULL_REVISION) !self.bases.is_empty()
} }
/// Return a reference to current bases. /// Return a reference to current bases.
/// ///
/// This is useful in unit tests, but also setdiscovery.py does /// This is useful in unit tests, but also setdiscovery.py does
/// read the bases attribute of a ancestor.missingancestors instance. /// read the bases attribute of a ancestor.missingancestors instance.
pub fn get_bases<'a>(&'a self) -> &'a HashSet<Revision> { pub fn get_bases<'a>(&'a self) -> &'a HashSet<Revision> {
&self.bases &self.bases
dagops::retain_heads(&self.graph, &mut self.bases)?; dagops::retain_heads(&self.graph, &mut self.bases)?;
Ok(self.bases) Ok(self.bases)
} }
pub fn add_bases( pub fn add_bases(
&mut self, &mut self,
new_bases: impl IntoIterator<Item = Revision>, new_bases: impl IntoIterator<Item = Revision>,
) { ) {
self.bases.extend(new_bases); self.bases
.extend(new_bases.into_iter().filter(|&rev| rev != NULL_REVISION));
kevincoxUnsubmitted
Not Done

I think it would be more clear if you filtered the NULL_REVISION out before extending the set.

let new_bases = new_bases.into_iter().filter(|&rev| rev != NULL_REVISION);
self.bases.extend(new_bases);
kevincox: I think it would be more clear if you filtered the NULL_REVISION out before extending the set.
} }
/// Remove all ancestors of self.bases from the revs set (in place) /// Remove all ancestors of self.bases from the revs set (in place)
pub fn remove_ancestors_from( pub fn remove_ancestors_from(
&mut self, &mut self,
revs: &mut HashSet<Revision>, revs: &mut HashSet<Revision>,
) -> Result<(), GraphError> { ) -> Result<(), GraphError> {
revs.retain(|r| !self.bases.contains(r)); revs.retain(|r| !self.bases.contains(r));
// the null revision is always an ancestor // the null revision is always an ancestor. Logically speaking
// it's debatable in case bases is empty, but the Python
// implementation always adds NULL_REVISION to bases, making it
// unconditionnally true.
revs.remove(&NULL_REVISION); revs.remove(&NULL_REVISION);
if revs.is_empty() { if revs.is_empty() {
return Ok(()); return Ok(());
} }
// anything in revs > start is definitely not an ancestor of bases // anything in revs > start is definitely not an ancestor of bases
// revs <= start need to be investigated // revs <= start need to be investigated
// TODO optim: if a missingancestors is to be used several times, // TODO optim: if a missingancestors is to be used several times,
// we shouldn't need to iterate each time on bases // we shouldn't need to iterate each time on bases
let start = match self.bases.iter().cloned().max() { let start = match self.bases.iter().cloned().max() {
Some(m) => m, Some(m) => m,
None => { None => { // self.bases is empty
// bases is empty (shouldn't happen, but let's be safe)
return Ok(()); return Ok(());
} }
}; };
// whatever happens, we'll keep at least keepcount of them // whatever happens, we'll keep at least keepcount of them
// knowing this gives us a earlier stop condition than // knowing this gives us a earlier stop condition than
// going all the way to the root // going all the way to the root
let keepcount = revs.iter().filter(|r| **r > start).count(); let keepcount = revs.iter().filter(|r| **r > start).count();