Patchwork chg: always wait for pager

login
register
mail settings
Submitter Jun Wu
Date April 12, 2017, 1:36 a.m.
Message ID <5d977772164deadb66b4.1491961017@x1c>
Download mbox | patch
Permalink /patch/20124/
State Accepted
Headers show

Comments

Jun Wu - April 12, 2017, 1:36 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1491960700 25200
#      Tue Apr 11 18:31:40 2017 -0700
# Node ID 5d977772164deadb66b4c9d27c2148fcadb03f0a
# Parent  f7b3677f66cd94fa01f345f6ab35229264aed179
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 5d977772164d
chg: always wait for pager

Previously, when runcommand raises, chg aborts with, and does not wait for
pager. The call stack is like:

  hgc_runcommand -> handleresponse -> readchannel -> debugmsg("failed to
  read channel") -> exit(255)

That means, chg returns to the shell, then both the pager and the shell will
read from the terminal at the same time, causing problems.

This patch fixes that by using "atexit" to register the pager cleanup
function so chg will always wait for pager even if runcommand raises.
Ryan McElroy - April 12, 2017, 11:56 a.m.
On 4/12/17 2:36 AM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1491960700 25200
> #      Tue Apr 11 18:31:40 2017 -0700
> # Node ID 5d977772164deadb66b4c9d27c2148fcadb03f0a
> # Parent  f7b3677f66cd94fa01f345f6ab35229264aed179
> chg: always wait for pager

Should this be on STABLE? It seems like a kind of bad issue.

>
> Previously, when runcommand raises, chg aborts with, and does not wait for
> pager. The call stack is like:
>
>    hgc_runcommand -> handleresponse -> readchannel -> debugmsg("failed to
>    read channel") -> exit(255)
>
> That means, chg returns to the shell, then both the pager and the shell will
> read from the terminal at the same time, causing problems.
>
> This patch fixes that by using "atexit" to register the pager cleanup
> function so chg will always wait for pager even if runcommand raises.
>
> diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
> --- a/contrib/chg/chg.c
> +++ b/contrib/chg/chg.c
> @@ -448,9 +448,9 @@ int main(int argc, const char *argv[], c
>   
>   	setupsignalhandler(hgc_peerpid(hgc), hgc_peerpgid(hgc));
> +	atexit(waitpager);
>   	int exitcode = hgc_runcommand(hgc, argv + 1, argc - 1);
>   	restoresignalhandler();
>   	hgc_close(hgc);
>   	freecmdserveropts(&opts);
> -	waitpager();
>   
>   	return exitcode;
> diff --git a/tests/test-chg.t b/tests/test-chg.t
> --- a/tests/test-chg.t
> +++ b/tests/test-chg.t
> @@ -103,4 +103,35 @@ pager should be enabled if the attached
>     0:1f7b0de80e11
>   
> +chg waits for pager if runcommand raises
> +
> +  $ cat > $TESTTMP/crash.py <<EOF
> +  > from mercurial import cmdutil
> +  > cmdtable = {}
> +  > command = cmdutil.command(cmdtable)
> +  > @command('crash')
> +  > def pagercrash(ui, repo, *pats, **opts):
> +  >     ui.write('going to crash\n')
> +  >     raise Exception('.')
> +  > EOF
> +
> +  $ cat > $TESTTMP/fakepager.py <<EOF
> +  > import sys, time
> +  > for line in iter(sys.stdin.readline, ''):
> +  >     if 'crash' in line: # only interested in lines containing 'crash'
> +  >         # if chg exits when pager is sleeping (incorrectly), the output
> +  >         # will be captured by the next test case
> +  >         time.sleep(1)
> +  >         sys.stdout.write('crash-pager: %s' % line)
> +  > EOF
> +
> +  $ cat >> .hg/hgrc <<EOF
> +  > [extensions]
> +  > crash = $TESTTMP/crash.py
> +  > EOF
> +
> +  $ chg crash --pager=on --config ui.formatted=True 2>/dev/null
> +  crash-pager: going to crash
> +  [255]
> +
>     $ cd ..
>   
>

The code change looks good to me. I'm in favor of taking this so I'll 
mark as pre-reviewed.
Augie Fackler - April 12, 2017, 8:13 p.m.
On Wed, Apr 12, 2017 at 12:56:28PM +0100, Ryan McElroy wrote:
> On 4/12/17 2:36 AM, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1491960700 25200
> > #      Tue Apr 11 18:31:40 2017 -0700
> > # Node ID 5d977772164deadb66b4c9d27c2148fcadb03f0a
> > # Parent  f7b3677f66cd94fa01f345f6ab35229264aed179
> > chg: always wait for pager
>
> Should this be on STABLE? It seems like a kind of bad issue.

chg is still semi-experimental (I think?) and we're close to a freeze. Queued for default.

>
> >
> > Previously, when runcommand raises, chg aborts with, and does not wait for
> > pager. The call stack is like:
> >
> >    hgc_runcommand -> handleresponse -> readchannel -> debugmsg("failed to
> >    read channel") -> exit(255)
> >
> > That means, chg returns to the shell, then both the pager and the shell will
> > read from the terminal at the same time, causing problems.
> >
> > This patch fixes that by using "atexit" to register the pager cleanup
> > function so chg will always wait for pager even if runcommand raises.
> >
> > diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
> > --- a/contrib/chg/chg.c
> > +++ b/contrib/chg/chg.c
> > @@ -448,9 +448,9 @@ int main(int argc, const char *argv[], c
> >     setupsignalhandler(hgc_peerpid(hgc), hgc_peerpgid(hgc));
> > +	atexit(waitpager);
> >     int exitcode = hgc_runcommand(hgc, argv + 1, argc - 1);
> >     restoresignalhandler();
> >     hgc_close(hgc);
> >     freecmdserveropts(&opts);
> > -	waitpager();
> >     return exitcode;
> > diff --git a/tests/test-chg.t b/tests/test-chg.t
> > --- a/tests/test-chg.t
> > +++ b/tests/test-chg.t
> > @@ -103,4 +103,35 @@ pager should be enabled if the attached
> >     0:1f7b0de80e11
> > +chg waits for pager if runcommand raises
> > +
> > +  $ cat > $TESTTMP/crash.py <<EOF
> > +  > from mercurial import cmdutil
> > +  > cmdtable = {}
> > +  > command = cmdutil.command(cmdtable)
> > +  > @command('crash')
> > +  > def pagercrash(ui, repo, *pats, **opts):
> > +  >     ui.write('going to crash\n')
> > +  >     raise Exception('.')
> > +  > EOF
> > +
> > +  $ cat > $TESTTMP/fakepager.py <<EOF
> > +  > import sys, time
> > +  > for line in iter(sys.stdin.readline, ''):
> > +  >     if 'crash' in line: # only interested in lines containing 'crash'
> > +  >         # if chg exits when pager is sleeping (incorrectly), the output
> > +  >         # will be captured by the next test case
> > +  >         time.sleep(1)
> > +  >         sys.stdout.write('crash-pager: %s' % line)
> > +  > EOF
> > +
> > +  $ cat >> .hg/hgrc <<EOF
> > +  > [extensions]
> > +  > crash = $TESTTMP/crash.py
> > +  > EOF
> > +
> > +  $ chg crash --pager=on --config ui.formatted=True 2>/dev/null
> > +  crash-pager: going to crash
> > +  [255]
> > +
> >     $ cd ..
> >
>
> The code change looks good to me. I'm in favor of taking this so I'll mark
> as pre-reviewed.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -448,9 +448,9 @@  int main(int argc, const char *argv[], c
 
 	setupsignalhandler(hgc_peerpid(hgc), hgc_peerpgid(hgc));
+	atexit(waitpager);
 	int exitcode = hgc_runcommand(hgc, argv + 1, argc - 1);
 	restoresignalhandler();
 	hgc_close(hgc);
 	freecmdserveropts(&opts);
-	waitpager();
 
 	return exitcode;
diff --git a/tests/test-chg.t b/tests/test-chg.t
--- a/tests/test-chg.t
+++ b/tests/test-chg.t
@@ -103,4 +103,35 @@  pager should be enabled if the attached 
   0:1f7b0de80e11
 
+chg waits for pager if runcommand raises
+
+  $ cat > $TESTTMP/crash.py <<EOF
+  > from mercurial import cmdutil
+  > cmdtable = {}
+  > command = cmdutil.command(cmdtable)
+  > @command('crash')
+  > def pagercrash(ui, repo, *pats, **opts):
+  >     ui.write('going to crash\n')
+  >     raise Exception('.')
+  > EOF
+
+  $ cat > $TESTTMP/fakepager.py <<EOF
+  > import sys, time
+  > for line in iter(sys.stdin.readline, ''):
+  >     if 'crash' in line: # only interested in lines containing 'crash'
+  >         # if chg exits when pager is sleeping (incorrectly), the output
+  >         # will be captured by the next test case
+  >         time.sleep(1)
+  >         sys.stdout.write('crash-pager: %s' % line)
+  > EOF
+
+  $ cat >> .hg/hgrc <<EOF
+  > [extensions]
+  > crash = $TESTTMP/crash.py
+  > EOF
+
+  $ chg crash --pager=on --config ui.formatted=True 2>/dev/null
+  crash-pager: going to crash
+  [255]
+
   $ cd ..