Patchwork D6267: incoming: detect if server send partial replies

login
register
mail settings
Submitter phabricator
Date April 17, 2019, 9:36 p.m.
Message ID <differential-rev-PHID-DREV-b4rl45ucwx5ybzceq3rj-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/39709/
State New
Headers show

Comments

phabricator - April 17, 2019, 9:36 p.m.
joerg.sonnenberger created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  incoming is not using the normal exchange logic and therefore doesn't
  know how to tell with pullbundles. Fixing that is involved and it is
  currently not sure if the current incoming code will survive, so apply a
  band aid for now.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/bundlerepo.py
  tests/test-pull-bundle.t

CHANGE DETAILS




To: joerg.sonnenberger, #hg-reviewers
Cc: mercurial-devel
phabricator - April 17, 2019, 9:56 p.m.
indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


  +1 for feature. Needs some minor work to get review.

INLINE COMMENTS

> bundlerepo.py:639
>  
> +        if rheads and any(x not in localrepo for x in rheads):
> +            ui.warn(_("warning: partial reply from server\n"))

`localrepo.__contains__` can be slow in tight loops because it has to resolve `self.changelog`. Also, `localrepo.__getitem__` (which `__contains__` calls) accepts a myriad of different values (integers, hex and binary hashes, etc). We want to user a lower-level API to perform lookups. I think `localrepo.changelog.hasnode()` is what we want. Please alias that to a local to avoid the `localrepo.changelog` property resolution in a loop.

> bundlerepo.py:640
> +        if rheads and any(x not in localrepo for x in rheads):
> +            ui.warn(_("warning: partial reply from server\n"))
> +            rheads = [x for x in rheads if x in localrepo]

+1 for this feature. But the warning message is a bit ambiguous to me. How about something like `warning: server sent partial data; not all remote changesets are available`. This still isn't great. But I'm struggling to find a way to better state the issue here.

> bundlerepo.py:641
> +            ui.warn(_("warning: partial reply from server\n"))
> +            rheads = [x for x in rheads if x in localrepo]
>      csets = localrepo.changelog.findmissing(common, rheads)

Same comment as above regarding the `x in localrepo` issues.

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - April 19, 2019, 3:07 p.m.
indygreg requested changes to this revision.
indygreg added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> bundlerepo.py:642
> +            ui.warn(_("warning: the server did not send all changes; "
> +                      "the follow list is incomplete\n"))
> +            rheads = [x for x in rheads if hasnode(x)]

Something I didn't notice before is that this function doesn't do any UI presentation. So it feels like a bug to print a message in this function assuming that fetched changesets will be displayed.

I suppose that means we'll need to include the "was partial reply" state in the return value and format it to a warning elsewhere.

Sorry for not catching this on first review. I intend to queue this patch for stable so it gets in the 5.0 release, as I think the improved UI is good to have.

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel

Patch

diff --git a/tests/test-pull-bundle.t b/tests/test-pull-bundle.t
--- a/tests/test-pull-bundle.t
+++ b/tests/test-pull-bundle.t
@@ -136,20 +136,21 @@ 
   updating to branch default
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ cd repo.pullbundle2a
-  $ hg incoming -r ed1b79f46b9a
+  $ hg incoming
   comparing with http://localhost:$HGPORT2/
   searching for changes
-  changeset:   1:ed1b79f46b9a
+  warning: partial reply from server
+  changeset:   1:effea6de0384
   tag:         tip
   user:        test
   date:        Thu Jan 01 00:00:00 1970 +0000
-  summary:     change foo
+  summary:     add bar
   
   $ cd ..
   $ killdaemons.py
   $ grep 'sending pullbundle ' repo/.hg/blackbox.log
   * sending pullbundle "0.hg" (glob)
-  * sending pullbundle "1.hg" (glob)
+  * sending pullbundle "2.hg" (glob)
   $ rm repo/.hg/blackbox.log
 
 Test recovery from misconfigured server sending no new data
diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
--- a/mercurial/bundlerepo.py
+++ b/mercurial/bundlerepo.py
@@ -636,6 +636,9 @@ 
         # The discovery process probably need cleanup to avoid that
         localrepo = localrepo.unfiltered()
 
+        if rheads and any(x not in localrepo for x in rheads):
+            ui.warn(_("warning: partial reply from server\n"))
+            rheads = [x for x in rheads if x in localrepo]
     csets = localrepo.changelog.findmissing(common, rheads)
 
     if bundlerepo: