hook: stdout/stderr corruption with redirection (e.g. ssh repo)

[Matt Mackall]

On Mon, 2011-11-14 at 13:25 +0100, Thomas De Schampheleire wrote:
> Hi,
> 
> I have been experiencing cloning problems in particular situations,
> reproducibly. The problem only occurs when cloning over ssh (even from
> localhost), with particular repositories/revisions, when any
> non-python outgoing hook was present.
> 
> Comparison of data sent over the connection with and without outgoing
> hook, showed that in the bad case (with hook) there was source code
> appearing on stderr. This source code was supposed to be sent on
> stdout, as it is part of the clone data.
> 
> The problem manifests itself as:
> 
> requesting all changes
> adding changesets
> adding manifests
> adding file changes
> transaction abort!
> rollback completed
> abort: consistency error in delta!
> 
> This is consistent with our stdout/stderr observation, as part of the
> clone data would be missing from stdout due to a bad redirection to
> stderr.
> 
> Looking at the relevant code from mercurial/hook.py:
> 
>     oldstdout = -1
>     if _redirect:
>         try:
>             stdoutno = sys.__stdout__.fileno()
>             stderrno = sys.__stderr__.fileno()
>             # temporarily redirect stdout to stderr, if possible
>             if stdoutno >= 0 and stderrno >= 0:
>                 oldstdout = os.dup(stdoutno)
>                 os.dup2(stderrno, stdoutno)
>         except AttributeError:
>             # __stdout/err__ doesn't have fileno(), it's not a real file
>             pass
> 
> The stdout descriptor is redirected to stderr, but no flushing of
> stdout is attempted.
> If we add flushing of stdout, as in following patch, the problem disappears.
> 
> diff --git a/mercurial/hook.py b/mercurial/hook.py
> --- a/mercurial/hook.py
> +++ b/mercurial/hook.py
> @@ -139,6 +139,7 @@ def hook(ui, repo, name, throw=False, **
>              stderrno = sys.__stderr__.fileno()
>              # temporarily redirect stdout to stderr, if possible
>              if stdoutno >= 0 and stderrno >= 0:
> +                sys.__stdout__.flush()
>                  oldstdout = os.dup(stdoutno)
>                  os.dup2(stderrno, stdoutno)
>          except AttributeError:
> 
> I'm not sure whether this is airtight though. I can not assess if
> there are other situations in which flushing would be required, or
> maybe if there is a better solution.
> If you agree that the above change is the right one, I can send a
> proper patch if you like (or feel free to just apply it).

Please do! See http://mercurial.selenic.com/wiki/ContributingChanges

> The problem is still present in 2.0.
> 
> Best regards,
> Thomas
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel


-- 
Mathematics is the supreme nostalgia of our time.