D8338: hook: move stdio redirection to context manager

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

D8338: hook: move stdio redirection to context manager

valentin.gatienbaron (Valentin Gatien-Baron)
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

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



To: indygreg, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D8338: hook: move stdio redirection to context manager

valentin.gatienbaron (Valentin Gatien-Baron)
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
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D8338: hook: move stdio redirection to context manager

valentin.gatienbaron (Valentin Gatien-Baron)
In reply to this post by valentin.gatienbaron (Valentin Gatien-Baron)
Closed by commit rHG3cbbfd0bfc17: hook: move stdio redirection to context manager (authored by indygreg).
This revision was automatically updated to reflect the committed changes.
This revision was not accepted when it landed; it landed in state "Needs Review".

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D8338?vs=20911&id=20949

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

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

AFFECTED FILES
  mercurial/hook.py

CHANGE DETAILS

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



To: indygreg, #hg-reviewers, Alphare
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel