Patchwork D8338: hook: move stdio redirection to context manager

login
register
mail settings
Submitter phabricator
Date March 29, 2020, 8:31 p.m.
Message ID <differential-rev-PHID-DREV-lhbdhiveqgbf7yjmj4qd-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/45938/
State Superseded
Headers show

Comments

phabricator - March 29, 2020, 8:31 p.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The old code was checking stdio redirection in a loop.
  This didn't make sense. The pattern is better expressed
  as a context manager IMO, so this commit refactors it
  to be one.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D8338

AFFECTED FILES
  mercurial/hook.py

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - March 29, 2020, 8:33 p.m.
indygreg added a comment.


  This commit isn't strictly required. I performed this refactoring anticipating needing to add `sys.std*` fixups as part of this function. But it turns out that the SSH protocol server handles I/O redirection via a different mechanism. There actually appear to be redundant mechanisms for intercepting stdio as part of the wire protocol. This is potentially an area that we could clean up. But I'm not inclined to do so at this time.
  
  Anyway, I feel the cleanup here improves readability, so I sent it along.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8338/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8338

To: indygreg, #hg-reviewers
Cc: mercurial-devel

Patch

diff --git a/mercurial/hook.py b/mercurial/hook.py
--- a/mercurial/hook.py
+++ b/mercurial/hook.py
@@ -7,6 +7,7 @@ 
 
 from __future__ import absolute_import
 
+import contextlib
 import os
 import sys
 
@@ -259,26 +260,45 @@ 
     return r
 
 
+@contextlib.contextmanager
+def redirect_stdio():
+    """Redirects stdout to stderr, if possible."""
+
+    oldstdout = -1
+    try:
+        if _redirect:
+            try:
+                stdoutno = procutil.stdout.fileno()
+                stderrno = procutil.stderr.fileno()
+                # temporarily redirect stdout to stderr, if possible
+                if stdoutno >= 0 and stderrno >= 0:
+                    procutil.stdout.flush()
+                    oldstdout = os.dup(stdoutno)
+                    os.dup2(stderrno, stdoutno)
+            except (OSError, AttributeError):
+                # files seem to be bogus, give up on redirecting (WSGI, etc)
+                pass
+
+        yield
+
+    finally:
+        # The stderr is fully buffered on Windows when connected to a pipe.
+        # A forcible flush is required to make small stderr data in the
+        # remote side available to the client immediately.
+        procutil.stderr.flush()
+
+        if _redirect and oldstdout >= 0:
+            procutil.stdout.flush()  # write hook output to stderr fd
+            os.dup2(oldstdout, stdoutno)
+            os.close(oldstdout)
+
+
 def runhooks(ui, repo, htype, hooks, throw=False, **args):
     args = pycompat.byteskwargs(args)
     res = {}
-    oldstdout = -1
 
-    try:
+    with redirect_stdio():
         for hname, cmd in hooks:
-            if oldstdout == -1 and _redirect:
-                try:
-                    stdoutno = procutil.stdout.fileno()
-                    stderrno = procutil.stderr.fileno()
-                    # temporarily redirect stdout to stderr, if possible
-                    if stdoutno >= 0 and stderrno >= 0:
-                        procutil.stdout.flush()
-                        oldstdout = os.dup(stdoutno)
-                        os.dup2(stderrno, stdoutno)
-                except (OSError, AttributeError):
-                    # files seem to be bogus, give up on redirecting (WSGI, etc)
-                    pass
-
             if cmd is _fromuntrusted:
                 if throw:
                     raise error.HookAbort(
@@ -312,15 +332,5 @@ 
                 raised = False
 
             res[hname] = r, raised
-    finally:
-        # The stderr is fully buffered on Windows when connected to a pipe.
-        # A forcible flush is required to make small stderr data in the
-        # remote side available to the client immediately.
-        procutil.stderr.flush()
-
-        if _redirect and oldstdout >= 0:
-            procutil.stdout.flush()  # write hook output to stderr fd
-            os.dup2(oldstdout, stdoutno)
-            os.close(oldstdout)
 
     return res