Patchwork [4,of,4,RFC] clone: add a server-side option to disable full getbundles (pull-based clones)

login
register
mail settings
Submitter Siddharth Agarwal
Date May 9, 2017, 3:45 a.m.
Message ID <68ab508896918ac1d56a.1494301536@devvm028.frc2.facebook.com>
Download mbox | patch
Permalink /patch/20541/
State Superseded
Headers show

Comments

Siddharth Agarwal - May 9, 2017, 3:45 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1494301361 25200
#      Mon May 08 20:42:41 2017 -0700
# Node ID 68ab508896918ac1d56ae0d2681e39fbf623aa5c
# Parent  052bd5cfe3769b10c64a4a39d9734a2740d44e16
clone: add a server-side option to disable full getbundles (pull-based clones)

For large enough repositories, pull-based clones take too long, and an attempt
to use them indicates some sort of configuration or other issue or maybe an
outdated Mercurial. Add a config option to disable them.
Gregory Szorc - May 9, 2017, 6:18 a.m.
On Mon, May 8, 2017 at 8:45 PM, Siddharth Agarwal <sid0@fb.com> wrote:

> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1494301361 25200
> #      Mon May 08 20:42:41 2017 -0700
> # Node ID 68ab508896918ac1d56ae0d2681e39fbf623aa5c
> # Parent  052bd5cfe3769b10c64a4a39d9734a2740d44e16
> clone: add a server-side option to disable full getbundles (pull-based
> clones)
>
> For large enough repositories, pull-based clones take too long, and an
> attempt
> to use them indicates some sort of configuration or other issue or maybe an
> outdated Mercurial. Add a config option to disable them.
>

I think this whole series looks fine.

Parts 1 through 3 aren't contentious and seem like an obvious win.

I've wanted to implement full clone lock-out for a while and this patch is
pretty much how I'd do it.


>
> diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
> --- a/mercurial/help/config.txt
> +++ b/mercurial/help/config.txt
> @@ -1660,6 +1660,12 @@ Controls generic server settings.
>      When set, clients will try to use the uncompressed streaming
>      protocol. (default: False)
>
> +``disablefullbundle``
> +    When set, servers will refuse attempts to do pull-based clones.
> +    If this option is set, ``preferuncompressed`` and/or clone bundles
> +    are highly recommended. Partial clones will still be allowed.
> +    (default: False)
> +
>  ``validate``
>      Whether to validate the completeness of pushed changesets by
>      checking that all new file revisions specified in manifests are
> diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
> --- a/mercurial/wireproto.py
> +++ b/mercurial/wireproto.py
> @@ -16,6 +16,7 @@ from .i18n import _
>  from .node import (
>      bin,
>      hex,
> +    nullid,
>  )
>
>  from . import (
> @@ -841,6 +842,17 @@ def getbundle(repo, proto, others):
>                                hint=bundle2requiredhint)
>
>      try:
> +        if repo.ui.configbool('server', 'disablefullbundle', False):
> +            # Check to see if this is a full clone.
> +            clheads = set(repo.changelog.heads())
> +            heads = set(opts.get('heads', set()))
> +            common = set(opts.get('common', []))
>

The pattern here is the same but one uses set() for a default value and the
other [].


> +            common.discard(nullid)
> +            if heads == clheads and len(common) == 0:
>

No need to len() == 0 here. "not common" is the same. And if you want to
micro-optimize while you are here, make the set compare 2nd so it can be
skipped in the common case of an incremental pull.


> +                raise error.Abort(
> +                    _('server has pull-based clones disabled'),
> +                    hint=_('remove --pull if specified or upgrade
> Mercurial'))
> +
>

My only non-nit concern about this patch is the wording here. Does the
average user know what a "pull-based clone" is? But I'm struggling to come
up with something obviously better. Maybe "server has traditional clones
disabled?" But what does "traditional" even mean.

Also in case anyone was wondering, the capability for preferring stream
clones was added in Mercurial 2.2 (released May 2012).


>          chunks = exchange.getbundlechunks(repo, 'serve', **opts)
>      except error.Abort as exc:
>          # cleanly forward Abort error to the client
> diff --git a/tests/test-http-bundle1.t b/tests/test-http-bundle1.t
> --- a/tests/test-http-bundle1.t
> +++ b/tests/test-http-bundle1.t
> @@ -365,3 +365,41 @@ Check error reporting while pulling/clon
>    this is an exercise
>    [255]
>    $ cat error.log
> +
> +disable pull-based clones
> +
> +  $ hg -R test serve -p $HGPORT1 -d --pid-file=hg4.pid -E error.log
> --config server.disablefullbundle=True
> +  $ cat hg4.pid >> $DAEMON_PIDS
> +  $ hg clone http://localhost:$HGPORT1/ disable-pull-clone
> +  requesting all changes
> +  abort: remote error:
> +  server has pull-based clones disabled
> +  [255]
> +
> +... but keep stream clones working
> +
> +  $ hg clone --uncompressed --noupdate http://localhost:$HGPORT1/
> test-stream-clone
> +  streaming all changes
> +  * files to transfer, * of data (glob)
> +  transferred * in * seconds (* KB/sec) (glob)
> +  searching for changes
> +  no changes found
> +
> +... and also keep partial clones and pulls working
> +  $ hg clone http://localhost:$HGPORT1 --rev 0 test-partial-clone
> +  adding changesets
> +  adding manifests
> +  adding file changes
> +  added 1 changesets with 4 changes to 4 files
> +  updating to branch default
> +  4 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  $ hg pull -R test-partial-clone
> +  pulling from http://localhost:$HGPORT1/
> +  searching for changes
> +  adding changesets
> +  adding manifests
> +  adding file changes
> +  added 2 changesets with 3 changes to 3 files
> +  (run 'hg update' to get a working copy)
> +
> +  $ cat error.log
> diff --git a/tests/test-http.t b/tests/test-http.t
> --- a/tests/test-http.t
> +++ b/tests/test-http.t
> @@ -354,6 +354,44 @@ check abort error reporting while pullin
>    [255]
>    $ cat error.log
>
> +disable pull-based clones
> +
> +  $ hg -R test serve -p $HGPORT1 -d --pid-file=hg4.pid -E error.log
> --config server.disablefullbundle=True
> +  $ cat hg4.pid >> $DAEMON_PIDS
> +  $ hg clone http://localhost:$HGPORT1/ disable-pull-clone
> +  requesting all changes
> +  remote: abort: server has pull-based clones disabled
> +  abort: pull failed on remote
> +  (remove --pull if specified or upgrade Mercurial)
> +  [255]
> +
> +... but keep stream clones working
> +
> +  $ hg clone --uncompressed --noupdate http://localhost:$HGPORT1/
> test-stream-clone
> +  streaming all changes
> +  * files to transfer, * of data (glob)
> +  transferred * in * seconds (*/sec) (glob)
> +  searching for changes
> +  no changes found
> +  $ cat error.log
> +
> +... and also keep partial clones and pulls working
> +  $ hg clone http://localhost:$HGPORT1 --rev 0 test-partial-clone
> +  adding changesets
> +  adding manifests
> +  adding file changes
> +  added 1 changesets with 4 changes to 4 files
> +  updating to branch default
> +  4 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  $ hg pull -R test-partial-clone
> +  pulling from http://localhost:$HGPORT1/
> +  searching for changes
> +  adding changesets
> +  adding manifests
> +  adding file changes
> +  added 2 changesets with 3 changes to 3 files
> +  (run 'hg update' to get a working copy)
> +
>  corrupt cookies file should yield a warning
>
>    $ cat > $TESTTMP/cookies.txt << EOF
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Siddharth Agarwal - May 9, 2017, 6:13 p.m.
(I was looking for feedback on both the feature and the wording -- thanks!)

On 5/8/17 23:18, Gregory Szorc wrote:
> My only non-nit concern about this patch is the wording here. Does the 
> average user know what a "pull-based clone" is? But I'm struggling to 
> come up with something obviously better. Maybe "server has traditional 
> clones disabled?" But what does "traditional" even mean.

Yeah, I was really struggling with the wording here too. This was the 
best I could come up with after half an hour or so. Suggestions are welcome!

- Siddharth


>
> Also in case anyone was wondering, the capability for preferring 
> stream clones was added in Mercurial 2.2 (released May 2012).

Patch

diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
--- a/mercurial/help/config.txt
+++ b/mercurial/help/config.txt
@@ -1660,6 +1660,12 @@  Controls generic server settings.
     When set, clients will try to use the uncompressed streaming
     protocol. (default: False)
 
+``disablefullbundle``
+    When set, servers will refuse attempts to do pull-based clones.
+    If this option is set, ``preferuncompressed`` and/or clone bundles
+    are highly recommended. Partial clones will still be allowed.
+    (default: False)
+
 ``validate``
     Whether to validate the completeness of pushed changesets by
     checking that all new file revisions specified in manifests are
diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
--- a/mercurial/wireproto.py
+++ b/mercurial/wireproto.py
@@ -16,6 +16,7 @@  from .i18n import _
 from .node import (
     bin,
     hex,
+    nullid,
 )
 
 from . import (
@@ -841,6 +842,17 @@  def getbundle(repo, proto, others):
                               hint=bundle2requiredhint)
 
     try:
+        if repo.ui.configbool('server', 'disablefullbundle', False):
+            # Check to see if this is a full clone.
+            clheads = set(repo.changelog.heads())
+            heads = set(opts.get('heads', set()))
+            common = set(opts.get('common', []))
+            common.discard(nullid)
+            if heads == clheads and len(common) == 0:
+                raise error.Abort(
+                    _('server has pull-based clones disabled'),
+                    hint=_('remove --pull if specified or upgrade Mercurial'))
+
         chunks = exchange.getbundlechunks(repo, 'serve', **opts)
     except error.Abort as exc:
         # cleanly forward Abort error to the client
diff --git a/tests/test-http-bundle1.t b/tests/test-http-bundle1.t
--- a/tests/test-http-bundle1.t
+++ b/tests/test-http-bundle1.t
@@ -365,3 +365,41 @@  Check error reporting while pulling/clon
   this is an exercise
   [255]
   $ cat error.log
+
+disable pull-based clones
+
+  $ hg -R test serve -p $HGPORT1 -d --pid-file=hg4.pid -E error.log --config server.disablefullbundle=True
+  $ cat hg4.pid >> $DAEMON_PIDS
+  $ hg clone http://localhost:$HGPORT1/ disable-pull-clone
+  requesting all changes
+  abort: remote error:
+  server has pull-based clones disabled
+  [255]
+
+... but keep stream clones working
+
+  $ hg clone --uncompressed --noupdate http://localhost:$HGPORT1/ test-stream-clone
+  streaming all changes
+  * files to transfer, * of data (glob)
+  transferred * in * seconds (* KB/sec) (glob)
+  searching for changes
+  no changes found
+
+... and also keep partial clones and pulls working
+  $ hg clone http://localhost:$HGPORT1 --rev 0 test-partial-clone
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 4 changes to 4 files
+  updating to branch default
+  4 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg pull -R test-partial-clone
+  pulling from http://localhost:$HGPORT1/
+  searching for changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 2 changesets with 3 changes to 3 files
+  (run 'hg update' to get a working copy)
+
+  $ cat error.log
diff --git a/tests/test-http.t b/tests/test-http.t
--- a/tests/test-http.t
+++ b/tests/test-http.t
@@ -354,6 +354,44 @@  check abort error reporting while pullin
   [255]
   $ cat error.log
 
+disable pull-based clones
+
+  $ hg -R test serve -p $HGPORT1 -d --pid-file=hg4.pid -E error.log --config server.disablefullbundle=True
+  $ cat hg4.pid >> $DAEMON_PIDS
+  $ hg clone http://localhost:$HGPORT1/ disable-pull-clone
+  requesting all changes
+  remote: abort: server has pull-based clones disabled
+  abort: pull failed on remote
+  (remove --pull if specified or upgrade Mercurial)
+  [255]
+
+... but keep stream clones working
+
+  $ hg clone --uncompressed --noupdate http://localhost:$HGPORT1/ test-stream-clone
+  streaming all changes
+  * files to transfer, * of data (glob)
+  transferred * in * seconds (*/sec) (glob)
+  searching for changes
+  no changes found
+  $ cat error.log
+
+... and also keep partial clones and pulls working
+  $ hg clone http://localhost:$HGPORT1 --rev 0 test-partial-clone
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 4 changes to 4 files
+  updating to branch default
+  4 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg pull -R test-partial-clone
+  pulling from http://localhost:$HGPORT1/
+  searching for changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 2 changesets with 3 changes to 3 files
+  (run 'hg update' to get a working copy)
+
 corrupt cookies file should yield a warning
 
   $ cat > $TESTTMP/cookies.txt << EOF