| 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
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".
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.
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
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.
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".
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)