| Submitter | Simon Farnsworth |
|---|---|
| Date | Jan. 19, 2017, 7:02 p.m. |
| Message ID | <76123ae2e0ccaa58db3d.1484852527@devvm022.lla2.facebook.com> |
| Download | mbox | patch |
| Permalink | /patch/18264/ |
| State | Deferred |
| Headers | show |
Comments
On Thu, Jan 19, 2017 at 11:02 AM, Simon Farnsworth <simonfar@fb.com> wrote: > # HG changeset patch > # User Simon Farnsworth <simonfar@fb.com> > # Date 1484835774 28800 > # Thu Jan 19 06:22:54 2017 -0800 > # Node ID 76123ae2e0ccaa58db3d4fc26b75b7251e13ad16 > # Parent 036c37bd3ec189480647ff568cee9e0b43a5bc81 > pager: stdout is line buffered by default We're in a code freeze and accept patches for the stable branch only, so please resend this after the freeze is over (~ Feb 3).
On Thu, 19 Jan 2017 11:02:07 -0800, Simon Farnsworth wrote: > # HG changeset patch > # User Simon Farnsworth <simonfar@fb.com> > # Date 1484835774 28800 > # Thu Jan 19 06:22:54 2017 -0800 > # Node ID 76123ae2e0ccaa58db3d4fc26b75b7251e13ad16 > # Parent 036c37bd3ec189480647ff568cee9e0b43a5bc81 > pager: stdout is line buffered by default > > pager only starts when ui.formatted() is true. In normal operation, > when ui.formatted() is true, stdout is line buffered anyway. Unfortunately that isn't always true. The buffering mode of FILE object is decided on the first fwrite() (or perhaps fflush()) as far as on glibc. We generally attach the pager fd before writing anything, which makes the stdout fully-buffered. > The code here doesn't actually do what the commit message in changeset > 62c5e937 claims it will do; sys.stdout is not yet pager.stdin when we > reopen, so we affect the original sys.stdout's buffering mode. It's the dup2 > that puts pager.stdin's fd into sys.stdout's fd, and that doesn't affect > buffering. No. AFAIK, the buffering mode is defined per FILE object, not per file descriptor.
On 20/01/2017 12:56, Yuya Nishihara wrote: > On Thu, 19 Jan 2017 11:02:07 -0800, Simon Farnsworth wrote: >> # HG changeset patch >> # User Simon Farnsworth <simonfar@fb.com> >> # Date 1484835774 28800 >> # Thu Jan 19 06:22:54 2017 -0800 >> # Node ID 76123ae2e0ccaa58db3d4fc26b75b7251e13ad16 >> # Parent 036c37bd3ec189480647ff568cee9e0b43a5bc81 >> pager: stdout is line buffered by default >> >> pager only starts when ui.formatted() is true. In normal operation, >> when ui.formatted() is true, stdout is line buffered anyway. > > Unfortunately that isn't always true. The buffering mode of FILE object is > decided on the first fwrite() (or perhaps fflush()) as far as on glibc. We > generally attach the pager fd before writing anything, which makes the stdout > fully-buffered. > >> The code here doesn't actually do what the commit message in changeset >> 62c5e937 claims it will do; sys.stdout is not yet pager.stdin when we >> reopen, so we affect the original sys.stdout's buffering mode. It's the dup2 >> that puts pager.stdin's fd into sys.stdout's fd, and that doesn't affect >> buffering. > > No. AFAIK, the buffering mode is defined per FILE object, not per file > descriptor. > Argh. I've written a test program, and this is true of glibc (thanks for the unclear documentation, glibc guys) - I'll improve the comment when I resubmit after the freeze.
Patch
diff --git a/hgext/pager.py b/hgext/pager.py --- a/hgext/pager.py +++ b/hgext/pager.py @@ -87,14 +87,10 @@ close_fds=util.closefds, stdin=subprocess.PIPE, stdout=util.stdout, stderr=util.stderr) - # back up original file objects and descriptors - olduifout = ui.fout - oldstdout = util.stdout + # back up original file descriptors stdoutfd = os.dup(util.stdout.fileno()) stderrfd = os.dup(util.stderr.fileno()) - # create new line-buffered stdout so that output can show up immediately - ui.fout = util.stdout = newstdout = os.fdopen(util.stdout.fileno(), 'wb', 1) os.dup2(pager.stdin.fileno(), util.stdout.fileno()) if ui._isatty(util.stderr): os.dup2(pager.stdin.fileno(), util.stderr.fileno()) @@ -103,15 +99,12 @@ def killpager(): if util.safehasattr(signal, "SIGINT"): signal.signal(signal.SIGINT, signal.SIG_IGN) - pager.stdin.close() - ui.fout = olduifout - util.stdout = oldstdout - # close new stdout while it's associated with pager; otherwise stdout - # fd would be closed when newstdout is deleted - newstdout.close() - # restore original fds: stdout is open again + # restore original fds os.dup2(stdoutfd, util.stdout.fileno()) os.dup2(stderrfd, util.stderr.fileno()) + # close pager's stdin so that it knows we're not going to send any more + # data + pager.stdin.close() pager.wait() def uisetup(ui):