[PATCH] chg: exec pager in child process

[Jun Wu]

# HG changeset patch
# User Jun Wu <quark at 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.

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 ..