Patchwork [04,of,10] chg: drop --cwd /

login
register
mail settings
Submitter Jun Wu
Date March 2, 2016, 10:44 a.m.
Message ID <9cd173eff72962ada960.1456915446@x1c>
Download mbox | patch
Permalink /patch/13524/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - March 2, 2016, 10:44 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1456746319 0
#      Mon Feb 29 11:45:19 2016 +0000
# Node ID 9cd173eff72962ada9601a368efdb8f87e2eed43
# Parent  855e65accbb4519f7140828efe6800b0b92fd5f4
chg: drop --cwd /

We intensionally want chgserver to load repo config to be able to:
- find out what extensions it should load
- detect config changes correctly
Therefore the --cwd flag is dropped.
Yuya Nishihara - March 3, 2016, 2:44 p.m.
On Wed, 2 Mar 2016 10:44:06 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1456746319 0
> #      Mon Feb 29 11:45:19 2016 +0000
> # Node ID 9cd173eff72962ada9601a368efdb8f87e2eed43
> # Parent  855e65accbb4519f7140828efe6800b0b92fd5f4
> chg: drop --cwd /
> 
> We intensionally want chgserver to load repo config to be able to:
> - find out what extensions it should load
> - detect config changes correctly
> Therefore the --cwd flag is dropped.


> 
> diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
> --- a/contrib/chg/chg.c
> +++ b/contrib/chg/chg.c
> @@ -210,7 +210,6 @@
>  	const char *baseargv[] = {
>  		hgcmd,
>  		"serve",
> -		"--cwd", "/",
>  		"--cmdserver", "chgunix",
>  		"--address", opts->sockname,
>  		"--daemon-postexec", "none",

I don't like the implicit chdir() introduced by the next patch. Probably
we can do "--daemon-postexec chdir:/" instead. It's common for a daemon process
to chdir to / once its boot-up sequence is completed.
Jun Wu - March 4, 2016, 11:37 a.m.
On 03/03/2016 02:44 PM, Yuya Nishihara wrote:
> I don't like the implicit chdir() introduced by the next patch. Probably we
> can do "--daemon-postexec chdir:/" instead. It's common for a daemon process
> to chdir to / once its boot-up sequence is completed.

Sorry for the late response but I am the London oncall this week and was busy
handling internal stuffs.

I think --daemon-postexec chdir:/ works but is not a good fit because:
- For chgserver, chdir / should be always performed, regardless of what the
   client says.
- It needs extra work to make multiple "postexec" actions (say you want to do
   both chdir and unlink). Turning --daemon-postexec into a multiple flag will
   make "--daemon-postexec none" look weird.

I will skip this one temporarily for now.
Yuya Nishihara - March 4, 2016, 1:30 p.m.
On Fri, 4 Mar 2016 11:37:18 +0000, Jun Wu wrote:
> On 03/03/2016 02:44 PM, Yuya Nishihara wrote:
> > I don't like the implicit chdir() introduced by the next patch. Probably we
> > can do "--daemon-postexec chdir:/" instead. It's common for a daemon process
> > to chdir to / once its boot-up sequence is completed.
> 
> Sorry for the late response but I am the London oncall this week and was busy
> handling internal stuffs.
> 
> I think --daemon-postexec chdir:/ works but is not a good fit because:

> - For chgserver, chdir / should be always performed, regardless of what the
>    client says.

No, chdir('/') isn't a requisite. We need it because chg (client) starts a
server at arbitrary locations, and the server is daemonized.

> - It needs extra work to make multiple "postexec" actions (say you want to do
>    both chdir and unlink). Turning --daemon-postexec into a multiple flag will
>    make "--daemon-postexec none" look weird.

Yep, "none" looks weird.

Patch

diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -210,7 +210,6 @@ 
 	const char *baseargv[] = {
 		hgcmd,
 		"serve",
-		"--cwd", "/",
 		"--cmdserver", "chgunix",
 		"--address", opts->sockname,
 		"--daemon-postexec", "none",