Patchwork [V2] chgserver: set sys.argv to improve compatibility

login
register
mail settings
Submitter Jun Wu
Date May 4, 2016, 9:24 p.m.
Message ID <6889dd5f104d2870f74a.1462397052@x1c>
Download mbox | patch
Permalink /patch/14887/
State Rejected
Delegated to: Augie Fackler
Headers show

Comments

Jun Wu - May 4, 2016, 9:24 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1459730303 -3600
#      Mon Apr 04 01:38:23 2016 +0100
# Node ID 6889dd5f104d2870f74ae1ef54eb7c2450bb6ea5
# Parent  906a1c8a75fd8a18e43e8545eedcbe5222f84647
chgserver: set sys.argv to improve compatibility

Before this patch, extensions reading sys.argv will behave incorrectly when
running with chg, because chg has a different sys.argv which is how the
chgserver started.

The chgserver argv is meaningless for extensions (and for chg itself). So
let's replace it with what the user actually uses passed from the chg client.

Therefore the extensions reading sys.argv even in reposetup will have correct
behaviors.
Jun Wu - May 4, 2016, 9:31 p.m.
This is a resend since Pierre-Yves replied it was pushed but seems it's not
there. And we recently found revlog uses sys.argv as well.

That said, I noticed the code is better living in commandserver.py's
runcommand just after sending this. I will send a V3 moving it to
commandserver.py if the idea is okay. Sorry for the inconvenience.

On 05/04/2016 10:24 PM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1459730303 -3600
> #      Mon Apr 04 01:38:23 2016 +0100
> # Node ID 6889dd5f104d2870f74ae1ef54eb7c2450bb6ea5
> # Parent  906a1c8a75fd8a18e43e8545eedcbe5222f84647
> chgserver: set sys.argv to improve compatibility
Augie Fackler - May 5, 2016, 3:27 a.m.
On Wed, May 04, 2016 at 10:24:12PM +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1459730303 -3600
> #      Mon Apr 04 01:38:23 2016 +0100
> # Node ID 6889dd5f104d2870f74ae1ef54eb7c2450bb6ea5
> # Parent  906a1c8a75fd8a18e43e8545eedcbe5222f84647
> chgserver: set sys.argv to improve compatibility
>
> Before this patch, extensions reading sys.argv will behave incorrectly when
> running with chg, because chg has a different sys.argv which is how the
> chgserver started.
>
> The chgserver argv is meaningless for extensions (and for chg itself). So
> let's replace it with what the user actually uses passed from the chg client.
>
> Therefore the extensions reading sys.argv even in reposetup will have correct
> behaviors.

What extensions read sys.argv? I'm having trouble figuring out why one
would even do that...


> diff --git a/hgext/chgserver.py b/hgext/chgserver.py
> --- a/hgext/chgserver.py
> +++ b/hgext/chgserver.py
> @@ -448,6 +448,11 @@
>          the instructions.
>          """
>          args = self._readlist()
> +        # some extensions read sys.argv in reposetup or their commands. we
> +        # don't have an easy way to get command args from an extension so
> +        # just change sys.argv to support them. this won't affect sys.argv
> +        # of the main server process since we forked.
> +        sys.argv = args
>          try:
>              self.ui, lui = _loadnewui(self.ui, args)
>          except error.ParseError as inst:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - May 5, 2016, 3:35 a.m.
On Wed, 4 May 2016 22:31:54 +0100, Jun Wu wrote:
> This is a resend since Pierre-Yves replied it was pushed but seems it's not
> there. And we recently found revlog uses sys.argv as well.
> 
> That said, I noticed the code is better living in commandserver.py's
> runcommand just after sending this. I will send a V3 moving it to
> commandserver.py if the idea is okay. Sorry for the inconvenience.

No. commandserver tries not to modify the globals, and it isn't even forked.
chgserver does such ugly things, but I want to eliminate them later.
Jun Wu - May 5, 2016, 12:42 p.m.
On 05/05/2016 04:27 AM, Augie Fackler wrote:
> What extensions read sys.argv? I'm having trouble figuring out why one
> would even do that...

Currently the known extensions are:

1. mutable-history/hgext/evolve.py (special handling 'debugobsconvert')
2. fb-hgext/reflog.py (writing command line to reflog)

It's painful because the extension has no way to read args from ui or repo
object. Ideally we will have some API to expose dispatch.request. Otherwise
extensions have to hook dispatch or ui.log to get the information. I will fix
fb-hgext/reflog.py in this way.

At the sprint, Yuya mentioned the idea to pass down full arguments from
dispatch.py. It's a part of a very complex plan to avoid constructing repo
object during chgserver startup. The whole plan is too complex and does not
worth the effort imo.

If we have to solve the sys.argv issue, I would prefer a straightforward and
simple way like putting the information directly in the ui object.
Jun Wu - May 5, 2016, 12:44 p.m.
On 05/05/2016 04:35 AM, Yuya Nishihara wrote:
> No. commandserver tries not to modify the globals, and it isn't even forked.
> chgserver does such ugly things, but I want to eliminate them later.

Good point. I think it's better to drop this patch then.
Pierre-Yves David - May 5, 2016, 3:49 p.m.
On 05/05/2016 02:44 PM, Jun Wu wrote:
> On 05/05/2016 04:35 AM, Yuya Nishihara wrote:
>> No. commandserver tries not to modify the globals, and it isn't even 
>> forked.
>> chgserver does such ugly things, but I want to eliminate them later.
>
> Good point. I think it's better to drop this patch then.

I've dropped it from patchwork. Feel free to submit a patch with another 
(hacky) approach for the evolve usecase.

Patch

diff --git a/hgext/chgserver.py b/hgext/chgserver.py
--- a/hgext/chgserver.py
+++ b/hgext/chgserver.py
@@ -448,6 +448,11 @@ 
         the instructions.
         """
         args = self._readlist()
+        # some extensions read sys.argv in reposetup or their commands. we
+        # don't have an easy way to get command args from an extension so
+        # just change sys.argv to support them. this won't affect sys.argv
+        # of the main server process since we forked.
+        sys.argv = args
         try:
             self.ui, lui = _loadnewui(self.ui, args)
         except error.ParseError as inst: