Patchwork dispatch: do not construct repo for chgserver

login
register
mail settings
Submitter Jun Wu
Date Feb. 19, 2016, 2:01 p.m.
Message ID <4668ead485adb2d83dc4.1455890509@x1c>
Download mbox | patch
Permalink /patch/13263/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - Feb. 19, 2016, 2:01 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1455890329 0
#      Fri Feb 19 13:58:49 2016 +0000
# Node ID 4668ead485adb2d83dc4399d4e2ce619e70751c7
# Parent  6c3ab048228f9b1760bf0b6baa307a4ec361c638
dispatch: do not construct repo for chgserver

chgserver is designed to be able to serve multiple repos from one server
process. It does not need a repo object but still needs to read repo config
to get the final [extensions] config to know what extensions it should load.

While constructing a repo object (running reposetup) should not have bad side
effects, it hurts performance a bit and there are some bad extensions having
global side effects like wrapping ui in reposetup.

This patch adds special handling logic in dispatch.py to detect the chgserver
case and skip repo object construction. It is not ideal but relatively
reasonable comparing to other hacks like a global flag or monkey patching.
Yuya Nishihara - Feb. 19, 2016, 2:27 p.m.
On Fri, 19 Feb 2016 14:01:49 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1455890329 0
> #      Fri Feb 19 13:58:49 2016 +0000
> # Node ID 4668ead485adb2d83dc4399d4e2ce619e70751c7
> # Parent  6c3ab048228f9b1760bf0b6baa307a4ec361c638
> dispatch: do not construct repo for chgserver
> 
> chgserver is designed to be able to serve multiple repos from one server
> process. It does not need a repo object but still needs to read repo config
> to get the final [extensions] config to know what extensions it should load.
> 
> While constructing a repo object (running reposetup) should not have bad side
> effects, it hurts performance a bit and there are some bad extensions having
> global side effects like wrapping ui in reposetup.
> 
> This patch adds special handling logic in dispatch.py to detect the chgserver
> case and skip repo object construction. It is not ideal but relatively
> reasonable comparing to other hacks like a global flag or monkey patching.
> 
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -873,9 +873,17 @@
>      elif not cmd:
>          return commands.help_(ui, 'shortlist')
>  
> +    norepocmd = cmd in commands.norepo.split()
> +    norepo = norepocmd
> +    cmdserver = _earlygetopt(['--cmdserver'], fullargs)
> +    # chgserver does not need repo object. one chgserver is designed to serve
> +    # multiple repos with same sensitive configs ([extensions], etc.)
> +    if cmdserver[:1] == ['chgunix'] and fullargs[:1] == ['serve']:
> +        norepo = True
> +
>      repo = None
>      cmdpats = args[:]
> -    if cmd not in commands.norepo.split():
> +    if not norepo:
>          # use the repo from the request only if we don't have -R
>          if not rpath and not cwd:
>              repo = req.repo
> @@ -914,7 +922,7 @@
>              if options['hidden']:
>                  repo = repo.unfiltered()
>          args.insert(0, repo)
> -    elif rpath:
> +    elif rpath and norepocmd:
>          ui.warn(_("warning: --repository ignored\n"))

Uh. If this is the only way to go, I would rather add a copy of "serve"
command, say "debugchgserve", and set it to "norepo" and (new) "nowarnrepo".
Jun Wu - Feb. 19, 2016, 2:46 p.m.
On 02/19/2016 02:27 PM, Yuya Nishihara wrote:
> Uh. If this is the only way to go, I would rather add a copy of
> "serve" command, say "debugchgserve", and set it to "norepo" and
> (new) "nowarnrepo".

That's "alternative 2" in my previous mail ;)

Is it worth adding a new top level command? From the users' perspective
the commands are not that different.

Since this is internal implementation detail, I would hide it from end
users. i.e. it's impossible for an end user to abuse it.

And I think it's valid for a power user to start a chg server manually.
So probably just make "hg serve" work as expected.
Yuya Nishihara - Feb. 19, 2016, 3:10 p.m.
On Fri, 19 Feb 2016 14:46:14 +0000, Jun Wu wrote:
> On 02/19/2016 02:27 PM, Yuya Nishihara wrote:
> > Uh. If this is the only way to go, I would rather add a copy of
> > "serve" command, say "debugchgserve", and set it to "norepo" and
> > (new) "nowarnrepo".
> 
> That's "alternative 2" in my previous mail ;)

Yes.

> Is it worth adding a new top level command? From the users' perspective
> the commands are not that different.

Well, my opinion is that it isn't worth messing up _dispatch().

> Since this is internal implementation detail, I would hide it from end
> users. i.e. it's impossible for an end user to abuse it.
> 
> And I think it's valid for a power user to start a chg server manually.
> So probably just make "hg serve" work as expected.

Maybe we could change 'norepo' to a callable.

I have patches that will replace 'dispatch.norepo' list by 'func.norepo'
boolean. It could be

norepo =
   | boolean
   | callable(ui, *args, **opts) -> boolean

or

needsrepo =
   | no
   | yes
   | optional
   | callable(ui, *args, **opts) -> no|yes|optional
Jun Wu - Feb. 20, 2016, 10:53 p.m.
On 02/19/2016 03:10 PM, Yuya Nishihara wrote:
> I have patches that will replace 'dispatch.norepo' list by 'func.norepo'
> boolean. It could be

I like this direction.

> norepo =
>     | boolean
>     | callable(ui, *args, **opts) -> boolean

This feels simpler.


> needsrepo =
>     | no
>     | yes
>     | optional
>     | callable(ui, *args, **opts) -> no|yes|optional

This may require additional refactoring on "optionalrepo".

I guess we cannot deprecate things easily. Adding a new keyword will
make the interface more cumbersome (especially for new developers).
I prefer the first approach.
Do you have patches ready? Otherwise I can work on this.
Jun Wu - Feb. 21, 2016, 5:46 p.m.
On 02/21/2016 04:25 AM, Yuya Nishihara wrote:
>> I guess we cannot deprecate things easily.
> We can change the internal API, but for this part, we shouldn't
> change too often.

I mean "norepo", "optionalrepo" are probably already used in external
extensions. The idea to use one "needrepo" to rule them all is good but:

- If we remove "norepo" and "optionalrepo", we may break something.
- If we keep them, new developers may get confused which one to use.

>> Do you have patches ready? Otherwise I can work on this.
> Yes, I have them except for the "callable" extension.

Looking forward to it!

Guess I may want to modify "Reply All" to make it "To" the list instead
of "CC".
Yuya Nishihara - Feb. 22, 2016, 1:32 p.m.
On Sun, 21 Feb 2016 17:46:38 +0000, Jun Wu wrote:
> On 02/21/2016 04:25 AM, Yuya Nishihara wrote:
> >> I guess we cannot deprecate things easily.
> > We can change the internal API, but for this part, we shouldn't
> > change too often.
> 
> I mean "norepo", "optionalrepo" are probably already used in external
> extensions. The idea to use one "needrepo" to rule them all is good but:
> 
> - If we remove "norepo" and "optionalrepo", we may break something.
> - If we keep them, new developers may get confused which one to use.

I meant we can theoretically break the third-party extensions. But I agree
that wouldn't be worth in this case.

> Guess I may want to modify "Reply All" to make it "To" the list instead
> of "CC".

Oops, I didn't notice I hit the wrong "Reply" button.

Patch

diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -873,9 +873,17 @@ 
     elif not cmd:
         return commands.help_(ui, 'shortlist')
 
+    norepocmd = cmd in commands.norepo.split()
+    norepo = norepocmd
+    cmdserver = _earlygetopt(['--cmdserver'], fullargs)
+    # chgserver does not need repo object. one chgserver is designed to serve
+    # multiple repos with same sensitive configs ([extensions], etc.)
+    if cmdserver[:1] == ['chgunix'] and fullargs[:1] == ['serve']:
+        norepo = True
+
     repo = None
     cmdpats = args[:]
-    if cmd not in commands.norepo.split():
+    if not norepo:
         # use the repo from the request only if we don't have -R
         if not rpath and not cwd:
             repo = req.repo
@@ -914,7 +922,7 @@ 
             if options['hidden']:
                 repo = repo.unfiltered()
         args.insert(0, repo)
-    elif rpath:
+    elif rpath and norepocmd:
         ui.warn(_("warning: --repository ignored\n"))
 
     msg = ' '.join(' ' in a and repr(a) or a for a in fullargs)