RE: [PATCH v3 2/4] terminal: don't assume stdin is /dev/tty

From: <rsbecker@nexbridge.com>
To: "'Junio C Hamano'" <gitster@pobox.com>,
	"'Phillip Wood'" <phillip.wood123@gmail.com>
Cc: "'Git Mailing List'" <git@vger.kernel.org>,
	"'Phillip Wood'" <phillip.wood@dunelm.org.uk>,
	"'Ævar Arnfjörð Bjarmason'" <avarab@gmail.com>,
	"'Carlo Arenas'" <carenas@gmail.com>,
	"'Johannes Schindelin'" <Johannes.Schindelin@gmx.de>,
	"'Ramsay Jones'" <ramsay@ramsayjones.plus.com>
Subject: RE: [PATCH v3 2/4] terminal: don't assume stdin is /dev/tty
Date: Tue, 15 Mar 2022 14:01:46 -0400	[thread overview]
Message-ID: <02fd01d83896$c0261240$407236c0$@nexbridge.com> (raw)
In-Reply-To: <xmqqzglrdsd8.fsf@gitster.g>

On March 15, 2022 1:43 PM, Junio C Hamano wrote:
>Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> diff --git a/compat/terminal.c b/compat/terminal.c index
>> da2f788137..bfbde3c1af 100644
>> --- a/compat/terminal.c
>> +++ b/compat/terminal.c
>> @@ -23,21 +23,28 @@ static void restore_term_on_signal(int sig)
>> static int term_fd = -1;
>
>The variable can be -1 to signal "no valid file descriptor".
>
>>  static struct termios old_term;
>>
>> +static void close_term_fd(void)
>> +{
>> +	if (term_fd)
>> +		close(term_fd);
>> +	term_fd = -1;
>> +}
>> +
>
>And we use that negative value after closing it.
>
>The check before closing it is wrong.  It should be
>
>	if (0 <= term_fd)

Should this expression succeed if term_fd == stdin? I might be missing the point here.
--Randall