Fix std handle forwarding #1837
Conversation
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...
| CREATE_NO_WINDOW, // Process creation flags | ||
| NULL, // Process creation flags |
There was a problem hiding this comment.
I seriously wonder whether this flag is there on purpose, to avoid "flashing windows" when, say, running git.exe from Visual Studio.
There was a problem hiding this comment.
Good point, I'll check that.
There was a problem hiding this comment.
I observed the updated hook getting called in procmon from VS, but there was no flashing window that I could see. I would expect that VS is already launching its git command with CREATE_NO_WINDOW, so the hooks loader is inheriting that state and passing it on.
| 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.
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.