threads / patch / 16132

patchWindows: Make OpenSSH properly detect tty detachment.

Subject: [PATCH (MINGW) Resend] Windows: Make OpenSSH properly detect tty detachment.

## tl;dr

4 messages between Nov 2, 2008 and Nov 6, 2008. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Alexander Gavrilov· Nov 2, 2008, 17:11 UTC · lore

Apparently, CREATE_NO_WINDOW makes the OS tell the process that it has a console, but without actually creating the window. As a result, when git is started from GUI, ssh tries to ask its questions on the invisible console.

This patch uses DETACHED_PROCESS instead, which clearly means that the process should be left without a console. The downside is that if the process manually calls AllocConsole, the window will appear. A similar thing might occur if it calls another console executable.

Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
Acked-by: Johannes Sixt <johannes.sixt@telecom.at>
---
	This patch appears to have been overlooked, so I	resend
	it just in case. It fixes a long standing problem in msysgit.
	-- Alexander
 compat/mingw.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)
Show changes to compat/mingw.c +6 −2
diff --git a/compat/mingw.c b/compat/mingw.c
index 1e29b88..b6fcf69 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -586,12 +586,16 @@ static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
 		 * would normally create a console window. But
 		 * since we'll be redirecting std streams, we do
 		 * not need the console.
+		 * It is necessary to use DETACHED_PROCESS
+		 * instead of CREATE_NO_WINDOW to make ssh
+		 * recognize that it has no console.
 		 */
-		flags = CREATE_NO_WINDOW;
+		flags = DETACHED_PROCESS;
 	} else {
 		/* There is already a console. If we specified
-		 * CREATE_NO_WINDOW here, too, Windows would
+		 * DETACHED_PROCESS here, too, Windows would
 		 * disassociate the child from the console.
+		 * The same is true for CREATE_NO_WINDOW.
 		 * Go figure!
 		 */
 		flags = 0;

-- 
1.6.0.2.1256.ga12f0
Johannes Sixt· Nov 6, 2008, 07:35 UTC · re: Alexander Gavrilov · lore

Re: [PATCH (MINGW) Resend] Windows: Make OpenSSH properly detect tty detachment.

Alexander Gavrilov schrieb:
Show 10 quoted lines
> Apparently, CREATE_NO_WINDOW makes the OS tell the process
> that it has a console, but without actually creating the
> window. As a result, when git is started from GUI, ssh
> tries to ask its questions on the invisible console.
> 
> This patch uses DETACHED_PROCESS instead, which clearly
> means that the process should be left without a console.
> The downside is that if the process manually calls
> AllocConsole, the window will appear. A similar thing
> might occur if it calls another console executable.

The latest "What's cooking in git.git" report still doesn't mention this patch. The reason might be that you mention some scary downsides in this paragraph. I think you should either not mention the downsides, or describe further why they do not make a difference in our case:

- We never call AllocConsole(). If some third-party program calls it, then
it gets what it requested. (But, yes, the behavior would indeed be
different from what it used to be.)
- There are two possible circumstances when another console executable is
called:
  * A program that has no console - e.g. git-gui - runs a git process.
Then we are entering the "then" branch, and the new process is started
with DETACH_PROCESS. No new console window appears.
  * A program that already has a console runs a git process. Then we are
entering the "else" branch, and we do not change the process flags nor are
we requesting a new console. No new console window appears.
I'm using this patch since weeks without noticing any downsides.
-- Hannes
Show 36 quoted lines
> 
> Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
> Acked-by: Johannes Sixt <johannes.sixt@telecom.at>
> ---
> 
> 	This patch appears to have been overlooked, so I	resend
> 	it just in case. It fixes a long standing problem in msysgit.
> 
> 	-- Alexander
> 
>  compat/mingw.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 1e29b88..b6fcf69 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -586,12 +586,16 @@ static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
>  		 * would normally create a console window. But
>  		 * since we'll be redirecting std streams, we do
>  		 * not need the console.
> +		 * It is necessary to use DETACHED_PROCESS
> +		 * instead of CREATE_NO_WINDOW to make ssh
> +		 * recognize that it has no console.
>  		 */
> -		flags = CREATE_NO_WINDOW;
> +		flags = DETACHED_PROCESS;
>  	} else {
>  		/* There is already a console. If we specified
> -		 * CREATE_NO_WINDOW here, too, Windows would
> +		 * DETACHED_PROCESS here, too, Windows would
>  		 * disassociate the child from the console.
> +		 * The same is true for CREATE_NO_WINDOW.
>  		 * Go figure!
>  		 */
>  		flags = 0;
Junio C Hamano· Nov 6, 2008, 16:55 UTC · re: Johannes Sixt · lore

Re: [PATCH (MINGW) Resend] Windows: Make OpenSSH properly detect tty detachment.

Johannes Sixt <j.sixt@viscovery.net> writes:
> The latest "What's cooking in git.git" report still doesn't mention this
> patch. The reason might be ...
Sorry, my mistake.
Show 5 quoted lines
>> Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
>> Acked-by: Johannes Sixt <johannes.sixt@telecom.at>
>> ---
>> 
>> 	This patch appears to have been overlooked,

Indeed. It was lost in the noise because it was marked MinGW and I failed to see your Ack.

Do people build and test MinGW port out of my 'next'? If so, I'll queue this on its own topic branch and have it go through the usual "cook then graduate" cycle. Otherwise, I'll apply this directly on top of 'master'.

Thanks.
Johannes Sixt· Nov 6, 2008, 17:21 UTC · re: Junio C Hamano · lore

Re: [PATCH (MINGW) Resend] Windows: Make OpenSSH properly detect tty detachment.

Junio C Hamano schrieb:
> Do people build and test MinGW port out of my 'next'?  If so, I'll queue
> this on its own topic branch and have it go through the usual "cook then
> graduate" cycle.  Otherwise, I'll apply this directly on top of 'master'.

My own build is on top of your 'master', and I've cooked this patch to death there already, so I don't think it needs another cycle. ;)

-- Hannes

← back to recent threads