Patchwork [5,of,5] chgserver: make _renewui load repo and command line configs

login
register
mail settings
Submitter Jun Wu
Date Feb. 24, 2016, 10:30 p.m.
Message ID <d8bb763c7fe0628ce293.1456353016@x1c>
Download mbox | patch
Permalink /patch/13378/
State Superseded
Commit 3682e201cce6e4c7ffe5c20d0517912eab39451c
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - Feb. 24, 2016, 10:30 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1456352811 0
#      Wed Feb 24 22:26:51 2016 +0000
# Node ID d8bb763c7fe0628ce293de5f914947b5e9e3dfc3
# Parent  a987a1eaf022e94dc62eba75e05c6e04b1e03714
chgserver: make _renewui load repo and command line configs

Before this patch, there is no way to load repo config in chgserver. This
patch revised _renewui to let it load repo config and take command line
flags passed into consideration.

The _renewui logic is not ideal at present because it's very tricky to know
what should copy and what should not from the old ui object to the new one.
This is partially because the current ui and config object design is not
ideal. In the future, we may want to avoid all ui or config copies.
Sean Farley - Feb. 24, 2016, 11:13 p.m.
Jun Wu <quark@fb.com> writes:

> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1456352811 0
> #      Wed Feb 24 22:26:51 2016 +0000
> # Node ID d8bb763c7fe0628ce293de5f914947b5e9e3dfc3
> # Parent  a987a1eaf022e94dc62eba75e05c6e04b1e03714
> chgserver: make _renewui load repo and command line configs
>
> Before this patch, there is no way to load repo config in chgserver. This
> patch revised _renewui to let it load repo config and take command line
> flags passed into consideration.
>
> The _renewui logic is not ideal at present because it's very tricky to know
> what should copy and what should not from the old ui object to the new one.
> This is partially because the current ui and config object design is not
> ideal. In the future, we may want to avoid all ui or config copies.

These patches are a bit much to digest. I like the direction but I think
Yuya needs to take a look.
Jun Wu - Feb. 24, 2016, 11:31 p.m.
On 02/24/2016 11:13 PM, Sean Farley wrote:
> These patches are a bit much to digest. I like the direction but I
> think Yuya needs to take a look.

Haha, Yuya has previewed these so I guess it's not too hard for him to
review this time. Pointers to behind the scenes:

https://bitbucket.org/yuja/chg/pull-requests/5 [1]
https://bitbucket.org/yuja/chg/pull-requests/4 [2]

[1] Basically a list of major feature patches I am sending and will send
(but with much lower quality of commit messages). Do not include some
latest fixes for tests though.
[2] The long discussion, where you can see how we abandoned a lot of
ideas and finally come to the current approach that:
  - covers a lot of cases with relatively simple logic
  - dodges filesystem race conditions
  - neither too many processes, nor restart frequently
Sean Farley - Feb. 24, 2016, 11:32 p.m.
Jun Wu <quark@fb.com> writes:

> On 02/24/2016 11:13 PM, Sean Farley wrote:
>> These patches are a bit much to digest. I like the direction but I
>> think Yuya needs to take a look.
>
> Haha, Yuya has previewed these so I guess it's not too hard for him to
> review this time. Pointers to behind the scenes:
>
> https://bitbucket.org/yuja/chg/pull-requests/5 [1]
> https://bitbucket.org/yuja/chg/pull-requests/4 [2]
>
> [1] Basically a list of major feature patches I am sending and will send
> (but with much lower quality of commit messages). Do not include some
> latest fixes for tests though.
> [2] The long discussion, where you can see how we abandoned a lot of
> ideas and finally come to the current approach that:
>   - covers a lot of cases with relatively simple logic
>   - dodges filesystem race conditions
>   - neither too many processes, nor restart frequently

Ah, then sounds good to me :-)
Yuya Nishihara - Feb. 25, 2016, 2:48 p.m.
On Wed, 24 Feb 2016 22:30:16 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1456352811 0
> #      Wed Feb 24 22:26:51 2016 +0000
> # Node ID d8bb763c7fe0628ce293de5f914947b5e9e3dfc3
> # Parent  a987a1eaf022e94dc62eba75e05c6e04b1e03714
> chgserver: make _renewui load repo and command line configs
> 
> Before this patch, there is no way to load repo config in chgserver. This
> patch revised _renewui to let it load repo config and take command line
> flags passed into consideration.
> 
> The _renewui logic is not ideal at present because it's very tricky to know
> what should copy and what should not from the old ui object to the new one.
> This is partially because the current ui and config object design is not
> ideal. In the future, we may want to avoid all ui or config copies.
> 
> diff --git a/hgext/chgserver.py b/hgext/chgserver.py
> --- a/hgext/chgserver.py
> +++ b/hgext/chgserver.py
> @@ -232,17 +232,42 @@
>  
>      return chgui(srcui)
>  
> -def _renewui(srcui):
> +def _renewui(srcui, args=None):
> +    if not args:
> +        args = []
> +
>      newui = srcui.__class__()
>      for a in ['fin', 'fout', 'ferr', 'environ']:
>          setattr(newui, a, getattr(srcui, a))
>      if util.safehasattr(srcui, '_csystem'):
>          newui._csystem = srcui._csystem
> +
> +    # load repo or local config, copied from dispatch.py
> +    cwd = dispatch._earlygetopt(['--cwd'], args)
> +    oldwd = os.getcwd()
> +    if cwd:
> +        os.chdir(cwd[-1])
> +    rpath = dispatch._earlygetopt(["-R", "--repository", "--repo"], args)
> +    path, newui = dispatch._getlocal(newui, rpath)
> +
> +    # go back to old wd otherwise it will cause error if --cwd is relative
> +    # since chdir will be called again in runcommand -> dispatch
> +    if cwd:
> +        os.chdir(oldwd)

Can you clean up this a little more? It can at least avoid chdir().

> +    # internal config: extensions.chgserver
> +    # copy it. it can only be overrided from command line.
> +    newui.setconfig('extensions', 'chgserver',
> +                    srcui.config('extensions', 'chgserver'))

Nit: configsource is dropped here.
Jun Wu - Feb. 25, 2016, 4:42 p.m.
Thanks for all the comments! Replied some below. Non-replied are "will fix".


-- PATCH 1 --

> Let's start with ['extensions']. The other sections would need
> detailed comments. It's easy to add sections, but hard to remove
> them from the list.

I think it's easy to remove them too, we are free to change the hash
function.

> Missing '$' at the end? re.match() passes if a string starts with
> the pattern, but you seem to expect an exact match.

Thanks for pointing out! The Python behavior here seems a bit strange to
me though. I'm much more familiar with Ruby than Python.

> configitems() have a stable order. IIRC, we can't be 100% sure that
> the loading order of extensions never change the behavior.

I see it's util.sortdict, like Ruby's Hash. Will remove sorted.

But extensions should behavior the same no matter of the loading order,
right? I thought otherwise they are required to touch extensions._order
or _aftercallbacks.

-- PATCH 2 --

> hg should run without __version__.py. So it would be wrong to import
>  __version__ mandatory.

Guess I should switch to use util.gethgcmd() then.

-- PATCH 5 --

> Can you clean up this a little more? It can at least avoid chdir().

dispatch._getlocal() depends on os.getcwd(). I will clean it up then.
Yuya Nishihara - Feb. 26, 2016, 1:46 p.m.
On Thu, 25 Feb 2016 16:42:17 +0000, Jun Wu wrote:
> -- PATCH 1 --
> > Let's start with ['extensions']. The other sections would need
> > detailed comments. It's easy to add sections, but hard to remove
> > them from the list.
> 
> I think it's easy to remove them too, we are free to change the hash
> function.

I meant we would have to be more careful not to break things. Imagine that
you have to clean up demandimport.ignore list, can you be confident?

> > Missing '$' at the end? re.match() passes if a string starts with
> > the pattern, but you seem to expect an exact match.
> 
> Thanks for pointing out! The Python behavior here seems a bit strange to
> me though. I'm much more familiar with Ruby than Python.

IIRC, Ruby has no function equivalent to re.match(), but has re.search().

> > configitems() have a stable order. IIRC, we can't be 100% sure that
> > the loading order of extensions never change the behavior.
> 
> I see it's util.sortdict, like Ruby's Hash. Will remove sorted.
> 
> But extensions should behavior the same no matter of the loading order,
> right? I thought otherwise they are required to touch extensions._order
> or _aftercallbacks.

Yep. There were a couple of bugs in old versions.

Patch

diff --git a/hgext/chgserver.py b/hgext/chgserver.py
--- a/hgext/chgserver.py
+++ b/hgext/chgserver.py
@@ -232,17 +232,42 @@ 
 
     return chgui(srcui)
 
-def _renewui(srcui):
+def _renewui(srcui, args=None):
+    if not args:
+        args = []
+
     newui = srcui.__class__()
     for a in ['fin', 'fout', 'ferr', 'environ']:
         setattr(newui, a, getattr(srcui, a))
     if util.safehasattr(srcui, '_csystem'):
         newui._csystem = srcui._csystem
+
+    # load repo or local config, copied from dispatch.py
+    cwd = dispatch._earlygetopt(['--cwd'], args)
+    oldwd = os.getcwd()
+    if cwd:
+        os.chdir(cwd[-1])
+    rpath = dispatch._earlygetopt(["-R", "--repository", "--repo"], args)
+    path, newui = dispatch._getlocal(newui, rpath)
+
+    # go back to old wd otherwise it will cause error if --cwd is relative
+    # since chdir will be called again in runcommand -> dispatch
+    if cwd:
+        os.chdir(oldwd)
+
+    # internal config: extensions.chgserver
+    # copy it. it can only be overrided from command line.
+    newui.setconfig('extensions', 'chgserver',
+                    srcui.config('extensions', 'chgserver'))
+
+    # command line args
+    dispatch._parseconfig(newui, dispatch._earlygetopt(['--config'], args))
+
     # stolen from tortoisehg.util.copydynamicconfig()
     for section, name, value in srcui.walkconfig():
         source = srcui.configsource(section, name)
-        if ':' in source:
-            # path:line
+        if ':' in source or source == '--config':
+            # path:line or command line
             continue
         if source == 'none':
             # ui.configsource returns 'none' by default