Patchwork D6429: rust-discovery: avoid useless calls to addcommons/addmissings

login
register
mail settings
Submitter phabricator
Date May 22, 2019, 5 p.m.
Message ID <differential-rev-PHID-DREV-sbt4lqqtsk2l7nm5hajp-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/40189/
State Superseded
Headers show

Comments

phabricator - May 22, 2019, 5 p.m.
gracinet created this revision.
Herald added subscribers: mercurial-devel, kevincox, durin42.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Whatever the implementation in hg-core, it's simpler to test
  for this condition in the bindings because the core methods take
  `impl IntoIterator`, and don't have this `is_empty` at disposal.
  
  As of this writing the core `add_common_revisions` can take non
  trivial time on an empty list.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6429

AFFECTED FILES
  rust/hg-cpython/src/discovery.rs

CHANGE DETAILS




To: gracinet, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
phabricator - June 10, 2019, 12:53 p.m.
kevincox requested changes to this revision.
kevincox added a comment.
This revision now requires changes to proceed.


  Wouldn't it be better to make `add_missing_revisions` work properly on empty inputs? It seems like it is too error prone to try and catch every caller. If possible I would like to push this check down as far as possible.
  
  It seems easy enough to do in https://www.mercurial-scm.org/repo/hg/file/tip/rust/hg-core/src/discovery.rs#l56 by checking if `common` grew.  This has the added benefit that attempting to re-add bases will also skill the work.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6429

To: gracinet, #hg-reviewers, kevincox
Cc: durin42, kevincox, mercurial-devel

Patch

diff --git a/rust/hg-cpython/src/discovery.rs b/rust/hg-cpython/src/discovery.rs
--- a/rust/hg-cpython/src/discovery.rs
+++ b/rust/hg-cpython/src/discovery.rs
@@ -73,10 +73,14 @@ 
             }
         }
         let mut inner = self.inner(py).borrow_mut();
-        inner.add_common_revisions(common)
-            .map_err(|e| GraphError::pynew(py, e))?;
-        inner.add_missing_revisions(missing)
-            .map_err(|e| GraphError::pynew(py, e))?;
+        if !common.is_empty() {
+            inner.add_common_revisions(common)
+                .map_err(|e| GraphError::pynew(py, e))?;
+        }
+        if !missing.is_empty() {
+            inner.add_missing_revisions(missing)
+                .map_err(|e| GraphError::pynew(py, e))?;
+        }
         Ok(py.None())
     }