Patchwork chg: exec pager in child process

login
register
mail settings
Submitter Jun Wu
Date June 11, 2016, 7:27 p.m.
Message ID <1b3b0aae1d814f7b202d.1465673227@x1c>
Download mbox | patch
Permalink /patch/15469/
State Superseded
Commit bb3d5c20eaf664cca201fc9273c1df2402f5c6fa
Headers show

Comments

Jun Wu - June 11, 2016, 7:27 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1465673149 -3600
#      Sat Jun 11 20:25:49 2016 +0100
# Node ID 1b3b0aae1d814f7b202d7d1c52132ac7d0202989
# Parent  c27dc3c31222c7f74331221a3d25566146feecac
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 1b3b0aae1d81
chg: exec pager in child process

Before this patch, chg executes pager in the main process, which means the
exit code would be the pager's and the exit code of the command gets ignored.

This patch fixes the behavior by executing the pager in the child process,
and wait for it at the end of the main process.
Jun Wu - June 11, 2016, 7:37 p.m.
Maybe I should just move the pager part to test-pager.t

But there is a chg pager test already, I think it might be okay to
merge this one and have a follow up to move these 2 to test-pager.t.

Excerpts from Jun Wu's message of 2016-06-11 20:27:07 +0100:
> diff --git a/tests/test-chg.t b/tests/test-chg.t
> --- a/tests/test-chg.t
> +++ b/tests/test-chg.t
> @@ -15,6 +15,9 @@
>    hg: parse error at * (glob)
>    [255]
>  
> +pager
> +-----
> +
>  alias having an environment variable and set to use pager
>  
>    $ rm $HGRCPATH
> @@ -35,6 +38,29 @@
>    $ A=2 chg printa
>    P2
>  
> +pager should not override the exit code of commands
> +
> +  $ cat >> $TESTTMP/fortytwo.py <<'EOF'
> +  > from mercurial import cmdutil, commands
> +  > cmdtable = {}
> +  > command = cmdutil.command(cmdtable)
> +  > @command('fortytwo', [], 'fortytwo', norepo=True)
> +  > def fortytwo(ui, *opts):
> +  >     ui.write('42\n')
> +  >     return 42
> +  > EOF
> +
> +  $ cat >> $HGRCPATH <<'EOF'
> +  > [extensions]
> +  > fortytwo = $TESTTMP/fortytwo.py
> +  > EOF
> +
> +  $ chg fortytwo --pager=on
> +  P42
> +  [42]
> +
> +restore hgrc
> +
>    $ cp $HGRCPATH.orig $HGRCPATH
>    $ cd ..
>
Yuya Nishihara - June 12, 2016, 2:07 p.m.
On Sat, 11 Jun 2016 20:27:07 +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1465673149 -3600
> #      Sat Jun 11 20:25:49 2016 +0100
> # Node ID 1b3b0aae1d814f7b202d7d1c52132ac7d0202989
> # Parent  c27dc3c31222c7f74331221a3d25566146feecac
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 1b3b0aae1d81
> chg: exec pager in child process

Looks mostly good to me. A few nits.

> -/* This implementation is based on hgext/pager.py (pre 369741ef7253) */
> -static void setuppager(hgclient_t *hgc, const char *const args[],
> +/* This implementation is based on hgext/pager.py (pre 369741ef7253)
> + * Return 0 if pager is not started, or pid of the pager */

So this is basically the post-369741ef7253 pager. Can you remove (or update)
the outdated comment?

And yes, test-pager.t is better place to write the test.

> +static void waitpager(pid_t pid)
> +{
> +	/* close output streams to notify the pager its input ends */
> +	fclose(stdout);
> +	fclose(stderr);
> +	while (1) {
> +		int stat = 0;
> +		pid_t ret = waitpid(pid, &stat, 0);
> +		if (ret == -1) {
> +			if (errno == EINTR)
> +				continue;
> +			break;
> +		}
> +		if (WIFEXITED(stat) || WIFSIGNALED(stat))
> +			break;

waitpid() does not return on stopped/continued by default, so we won't need
to check the status.
Jun Wu - June 12, 2016, 2:58 p.m.
Excerpts from Yuya Nishihara's message of 2016-06-12 23:07:01 +0900:
> So this is basically the post-369741ef7253 pager. Can you remove (or
> update) the outdated comment?

Sorry I didn't notice we have a bad pager.py before 369741ef7253, it
should have provided a test case tho.

> waitpid() does not return on stopped/continued by default, so we won't
> need to check the status.

Good point. I double checked it is the behavior of major systems.

I will follow up with first moving the existing test to test-pager.py then
the fix.

Patch

diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -417,21 +417,22 @@ 
 	abortmsgerrno("failed to set up signal handlers");
 }
 
-/* This implementation is based on hgext/pager.py (pre 369741ef7253) */
-static void setuppager(hgclient_t *hgc, const char *const args[],
+/* This implementation is based on hgext/pager.py (pre 369741ef7253)
+ * Return 0 if pager is not started, or pid of the pager */
+static pid_t setuppager(hgclient_t *hgc, const char *const args[],
 		       size_t argsize)
 {
 	const char *pagercmd = hgc_getpager(hgc, args, argsize);
 	if (!pagercmd)
-		return;
+		return 0;
 
 	int pipefds[2];
 	if (pipe(pipefds) < 0)
-		return;
+		return 0;
 	pid_t pid = fork();
 	if (pid < 0)
 		goto error;
-	if (pid == 0) {
+	if (pid > 0) {
 		close(pipefds[0]);
 		if (dup2(pipefds[1], fileno(stdout)) < 0)
 			goto error;
@@ -441,7 +442,7 @@ 
 		}
 		close(pipefds[1]);
 		hgc_attachio(hgc);  /* reattach to pager */
-		return;
+		return pid;
 	} else {
 		dup2(pipefds[0], fileno(stdin));
 		close(pipefds[0]);
@@ -451,13 +452,32 @@ 
 		if (r < 0) {
 			abortmsgerrno("cannot start pager '%s'", pagercmd);
 		}
-		return;
+		return 0;
 	}
 
 error:
 	close(pipefds[0]);
 	close(pipefds[1]);
 	abortmsgerrno("failed to prepare pager");
+	return 0;
+}
+
+static void waitpager(pid_t pid)
+{
+	/* close output streams to notify the pager its input ends */
+	fclose(stdout);
+	fclose(stderr);
+	while (1) {
+		int stat = 0;
+		pid_t ret = waitpid(pid, &stat, 0);
+		if (ret == -1) {
+			if (errno == EINTR)
+				continue;
+			break;
+		}
+		if (WIFEXITED(stat) || WIFSIGNALED(stat))
+			break;
+	}
 }
 
 /* Run instructions sent from the server like unlink and set redirect path
@@ -585,9 +605,12 @@ 
 	}
 
 	setupsignalhandler(hgc_peerpid(hgc));
-	setuppager(hgc, argv + 1, argc - 1);
+	pid_t pagerpid = setuppager(hgc, argv + 1, argc - 1);
 	int exitcode = hgc_runcommand(hgc, argv + 1, argc - 1);
 	hgc_close(hgc);
 	freecmdserveropts(&opts);
+	if (pagerpid)
+		waitpager(pagerpid);
+
 	return exitcode;
 }
diff --git a/tests/test-chg.t b/tests/test-chg.t
--- a/tests/test-chg.t
+++ b/tests/test-chg.t
@@ -15,6 +15,9 @@ 
   hg: parse error at * (glob)
   [255]
 
+pager
+-----
+
 alias having an environment variable and set to use pager
 
   $ rm $HGRCPATH
@@ -35,6 +38,29 @@ 
   $ A=2 chg printa
   P2
 
+pager should not override the exit code of commands
+
+  $ cat >> $TESTTMP/fortytwo.py <<'EOF'
+  > from mercurial import cmdutil, commands
+  > cmdtable = {}
+  > command = cmdutil.command(cmdtable)
+  > @command('fortytwo', [], 'fortytwo', norepo=True)
+  > def fortytwo(ui, *opts):
+  >     ui.write('42\n')
+  >     return 42
+  > EOF
+
+  $ cat >> $HGRCPATH <<'EOF'
+  > [extensions]
+  > fortytwo = $TESTTMP/fortytwo.py
+  > EOF
+
+  $ chg fortytwo --pager=on
+  P42
+  [42]
+
+restore hgrc
+
   $ cp $HGRCPATH.orig $HGRCPATH
   $ cd ..