Fix std handle forwarding #1837
Conversation
dscho
left a comment
dscho
left a comment
There was a problem hiding this comment.
I do not think that stdin is worth bothering about, given that Git specifically prevents Git hooks from accepting interactive user input.
Also, I am quite puzzled that the redirection should not work.
The only thing I vaguely remember causing problems with the usage pattern of the code touched by this PR is that the handles need to be duplicated lest they are closed by the child process...
| si.hStdOutput = GetStdHandle(STD_OUTPUT_HANDLE); | ||
| si.hStdError = GetStdHandle(STD_ERROR_HANDLE); | ||
| si.dwFlags = STARTF_USESTDHANDLES; |
There was a problem hiding this comment.
So what you're saying is that this explicit redirection of stdout/stderr currently does not work? This is puzzling because the GitHooksLoader.exe process should have the correct handles, and this way of creating the child process should work...
There was a problem hiding this comment.
Yes, it does not work. For example running git status on an unmounted gvfs repo displays only the standard fatal: pre-command hook aborted command message, and not the specific message about needing to mount.
The documentation for CREATE_NO_WINDOW says The process is a console application that is being run without a console window. Therefore, the console handle for the application is not set.
There was a problem hiding this comment.
I have updated this to more closely match how git-wrapper in microsoft/git works - checking explicitly for a console with CONOUT$, and if found then use default behavior (ie don't set CREATE_NO_WINDOW, don't explicitly forward handles as they are forwarded by default), falling back to the current behavior if CONOUT$ is not found (set CREATE_NO_WINDOW and explicitly forward handles)
There was a problem hiding this comment.
Excellent, thank you! Did you test this a bit, including running git status on an unmounted GVFS repo?
There was a problem hiding this comment.
Yes, it now displays The repo does not appear to be mounted. Use 'gvfs status' to check. fatal: pre-command hook aborted command
instead of just the fatal line.
tyrielv
force-pushed
the
hooks-stdout
branch
from
49d1d10 to
7e6cd6e
Compare
tyrielv
force-pushed
the
hooks-stdout
branch
from
7e6cd6e to
bf4be0f
Compare
dscho
left a comment
dscho
left a comment
There was a problem hiding this comment.
Thank you for accommodating my feedback!
| si.hStdOutput = GetStdHandle(STD_OUTPUT_HANDLE); | ||
| si.hStdError = GetStdHandle(STD_ERROR_HANDLE); | ||
| si.dwFlags = STARTF_USESTDHANDLES; |
There was a problem hiding this comment.
Excellent, thank you! Did you test this a bit, including running git status on an unmounted GVFS repo?
3 participants
Currently, GitHooksLoader.exe does not forward any output from the hooks that it is configured to run. This is different from the expected git hooks behavior, and it also does not appear to be intentional - stdout and stderr are explicitly configured to be forwarded, but the CREATE_NO_WINDOW flag prevents that from happening.
There are also outputs in GVFS.Hooks.exe (which in most cases is the only hook called by this) that are clearly expected to be visible to the user, but are not. For example, running a git command when GVFS is not mounted has suppressed output
The repo does not appear to be mounted. Use 'gvfs status' to check.This PR removes CREATE_NO_WINDOW so that standard handles are forwarded. It also removes the explicit forwarding of stdout and stderr, which has the effect that stdin is also forwarded. I'm not aware of any reasons we wouldn't want to forward stdin - without it, if a configured hook expects user input then it will just hang.