From: Johannes Sixt <j6t@kdbg.org>
Date: Thu, 22 Dec 2016 20:26:30 GMT
Subject: Re: [PATCH v2 3/3] mingw: replace isatty() hack
Message-ID: <e9f8a015-4106-fa88-082a-9e8c06ff61f3@kdbg.org>
In-Reply-To: <5e3c505a206a735e6ba0bfaf4c73965e95a928eb.1482426497.git.johannes.schindelin@gmx.de>

I've only one request for clarification below. Otherwise, the patch 
looks good.

(lines removed by the patch trimmed)

Am 22.12.2016 um 18:09 schrieb Johannes Schindelin:
> +static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
> +{
> +	/*
> +	 * Create a copy of the original handle associated with fd
> +	 * because the original will get closed when we dup2().
> +	 */
> +	HANDLE handle = (HANDLE)_get_osfhandle(fd);
> +	HANDLE duplicate = duplicate_handle(handle);
>
> +	/* Create a temp fd associated with the already open "new_handle". */
> +	int new_fd = _open_osfhandle((intptr_t)new_handle, O_BINARY);
>
> +	assert((fd == 1) || (fd == 2));
>
> +	/*
> +	 * Use stock dup2() to re-bind fd to the new handle.  Note that
> +	 * this will implicitly close(1) and close both fd=1 and the
> +	 * originally associated handle.  It will open a new fd=1 and
> +	 * call DuplicateHandle() on the handle associated with new_fd.
> +	 * It is because of this implicit close() that we created the
> +	 * copy of the original.
> +	 *
> +	 * Note that the OS can recycle HANDLE (numbers) just like it
> +	 * recycles fd (numbers), so we must update the cached value
> +	 * of "console".  You can use GetFileType() to see that
> +	 * handle and _get_osfhandle(fd) may have the same number
> +	 * value, but they refer to different actual files now.

Certainly, the OS does not recycle handle values that are in use (open). 
Then I do not quite get the point of this paragraph. See...

> +	 *
> +	 * Note that dup2() when given target := {0,1,2} will also
> +	 * call SetStdHandle(), so we don't need to worry about that.
> +	 */
> +	dup2(new_fd, fd);
> +	if (console == handle)
> +		console = duplicate;

... This is where "the cached value of console is updated", right? If 
console == handle, then this is not because a handle value was recycled, 
but because fd *was* console. Since the old value of console has been 
closed by the dup2(), we must refer to the back-up value in the future. 
Am I missing something?

> +	handle = INVALID_HANDLE_VALUE;
>
> +	/* Close the temp fd.  This explicitly closes "new_handle"
> +	 * (because it has been associated with it).
> +	 */
> +	close(new_fd);
>
> +	fd_is_interactive[fd] |= FD_SWAPPED;
>
> +	return duplicate;
>  }