Patchwork [3,of,3] chg: detect chg started by chg

login
register
mail settings
Submitter Jun Wu
Date Feb. 24, 2016, 2:31 p.m.
Message ID <2c07d50c16da4f859b3c.1456324285@x1c>
Download mbox | patch
Permalink /patch/13353/
State Superseded
Commit 2ab59ac06b762e4c598cbff41dfdfa27c82b849e
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - Feb. 24, 2016, 2:31 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1456324196 0
#      Wed Feb 24 14:29:56 2016 +0000
# Node ID 2c07d50c16da4f859b3cb002d9c5a04554b7478f
# Parent  b7c36ebb96f5492f3dd90bacbb8fee65c6eabb59
chg: detect chg started by chg

Sometimes people may create a symbol link from hg to chg, or write a wrapper
script named hg calling chg. Without $HG and $CHGHG set, this will lead to
chg executes itself causing deadlock. The user will notice chg hangs for some
time and aborts with a timed out message, without knowing the root cause and
how to solve it.

This patch sets a dummy environment variable CHGMARK to detect this situation,
and print a fatal message with some possible solutions.
Sean Farley - Feb. 24, 2016, 6:26 p.m.
Jun Wu <quark@fb.com> writes:

> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1456324196 0
> #      Wed Feb 24 14:29:56 2016 +0000
> # Node ID 2c07d50c16da4f859b3cb002d9c5a04554b7478f
> # Parent  b7c36ebb96f5492f3dd90bacbb8fee65c6eabb59
> chg: detect chg started by chg
>
> Sometimes people may create a symbol link from hg to chg, or write a wrapper
> script named hg calling chg. Without $HG and $CHGHG set, this will lead to
> chg executes itself causing deadlock. The user will notice chg hangs for some
> time and aborts with a timed out message, without knowing the root cause and
> how to solve it.
>
> This patch sets a dummy environment variable CHGMARK to detect this situation,
> and print a fatal message with some possible solutions.

Except for my question on the first patch, I like these.
Yuya Nishihara - Feb. 25, 2016, 1:41 p.m.
On Wed, 24 Feb 2016 14:31:25 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1456324196 0
> #      Wed Feb 24 14:29:56 2016 +0000
> # Node ID 2c07d50c16da4f859b3cb002d9c5a04554b7478f
> # Parent  b7c36ebb96f5492f3dd90bacbb8fee65c6eabb59
> chg: detect chg started by chg
> 
> Sometimes people may create a symbol link from hg to chg, or write a wrapper
> script named hg calling chg. Without $HG and $CHGHG set, this will lead to
> chg executes itself causing deadlock. The user will notice chg hangs for some
> time and aborts with a timed out message, without knowing the root cause and
> how to solve it.
> 
> This patch sets a dummy environment variable CHGMARK to detect this situation,
> and print a fatal message with some possible solutions.
> 
> diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
> --- a/contrib/chg/chg.c
> +++ b/contrib/chg/chg.c
> @@ -479,6 +479,13 @@
>  	if (getenv("CHGDEBUG"))
>  		enabledebugmsg();
>  
> +	if (getenv("CHGMARK"))
> +		abortmsg("chg started by chg detected.\n"
> +			 "Please make sure ${HG:-hg} is not a symlink or "
> +			 "wrapper to chg. Alternatively, set $CHGHG to the "
> +			 "path of real hg.");
> +	setenv("CHGMARK", "", 1);

Can this message be merged with the timeout error? Since we have a lock file,
the result is dead lock, which is much safer than executing chg recursively.

I want to avoid future headache caused by introducing an environment variable
that is always set.
Jun Wu - Feb. 25, 2016, 2:07 p.m.
On 02/25/2016 01:41 PM, Yuya Nishihara wrote:
> Can this message be merged with the timeout error? Since we have a
> lock file, the result is dead lock, which is much safer than
> executing chg recursively.
>
> I want to avoid future headache caused by introducing an environment
> variable that is always set.

I think this is better because:

1. time out can be caused by other issues like bad extensions or config
2. people can get feedback without waiting

I'm not sure how the environment variable can cause issues. Could you
give an example?
Yuya Nishihara - Feb. 25, 2016, 3:22 p.m.
On Thu, 25 Feb 2016 14:07:41 +0000, Jun Wu wrote:
> On 02/25/2016 01:41 PM, Yuya Nishihara wrote:
> > Can this message be merged with the timeout error? Since we have a
> > lock file, the result is dead lock, which is much safer than
> > executing chg recursively.
> >
> > I want to avoid future headache caused by introducing an environment
> > variable that is always set.
> 
> I think this is better because:
> 
> 1. time out can be caused by other issues like bad extensions or config
> 2. people can get feedback without waiting
> 
> I'm not sure how the environment variable can cause issues. Could you
> give an example?

Yep, it's better, but I can't say it's worth polluting environment variables.
My concern is the CHGMARK variable is visible to child processes. It might
be problem when simulating the hg behavior.

Silly example:

  $ hg --config='alias.x=!echo $CHGMARK' x
Jun Wu - Feb. 25, 2016, 4:05 p.m.
On 02/25/2016 03:22 PM, Yuya Nishihara wrote:
> Yep, it's better, but I can't say it's worth polluting environment variables.
> My concern is the CHGMARK variable is visible to child processes. It might
> be problem when simulating the hg behavior.
>
> Silly example:
>
>    $ hg --config='alias.x=!echo $CHGMARK' x

How about dropping it server side?
Yuya Nishihara - Feb. 26, 2016, 1:21 p.m.
On Thu, 25 Feb 2016 16:05:44 +0000, Jun Wu wrote:
> On 02/25/2016 03:22 PM, Yuya Nishihara wrote:
> > Yep, it's better, but I can't say it's worth polluting environment variables.
> > My concern is the CHGMARK variable is visible to child processes. It might
> > be problem when simulating the hg behavior.
> >
> > Silly example:
> >
> >    $ hg --config='alias.x=!echo $CHGMARK' x
> 
> How about dropping it server side?

and the client can't overwrite CHGMARK because it may be set by user.
Jun Wu - Feb. 26, 2016, 2:08 p.m.
On 02/26/2016 01:21 PM, Yuya Nishihara wrote:
> and the client can't overwrite CHGMARK because it may be set by
> user.

If the user sets it, chg client will just abortmsg. I will make the name
longer and look more internal.

Patch

diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -479,6 +479,13 @@ 
 	if (getenv("CHGDEBUG"))
 		enabledebugmsg();
 
+	if (getenv("CHGMARK"))
+		abortmsg("chg started by chg detected.\n"
+			 "Please make sure ${HG:-hg} is not a symlink or "
+			 "wrapper to chg. Alternatively, set $CHGHG to the "
+			 "path of real hg.");
+	setenv("CHGMARK", "", 1);
+
 	if (isunsupported(argc, argv))
 		execoriginalhg(argv);