Re: [PATCH v2 0/6] Force pipes to flush immediately on NonStop platform
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 22, 2018, 22:36 UTC
- Message-ID
- <xmqqshaxjzcc.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20180120111012.GA26459@tor.lan>
Torsten Bögershausen <tboegi@web.de> writes:
Show 31 quoted lines
> On Fri, Jan 19, 2018 at 12:33:59PM -0500, randall.s.becker@rogers.com wrote:
>> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>>
>> * wrapper.c: called setbuf(stream,0) to force pipe flushes not enabled by
>> default on the NonStop platform.
>>
>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> ---
>> wrapper.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/wrapper.c b/wrapper.c
>> index d20356a77..671cbb4b4 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -403,6 +403,9 @@ FILE *xfdopen(int fd, const char *mode)
>> FILE *stream = fdopen(fd, mode);
>> if (stream == NULL)
>> die_errno("Out of memory? fdopen failed");
>> +#ifdef __TANDEM
>> + setbuf(stream,0);
>> +#endif
>
> Reading the commit message, I would have expected someting similar to
>
> #ifdef FORCE_PIPE_FLUSHES
> setbuf(stream,0);
> #endif
>
> (Because other systems may need the tweak as well, some day)
> Of course you need to change that in the Makefile and config.mak.unameI actually wouldn't have expected anything like that after reading the commit message.
First I thought it was describing only what it does (i.e. "let's use setbuf() to set the stream unbuffered on TANDEM"), which is a useless description that only says what it does which we can read from the diff, but "NonStop by default creates pipe that does not flush" is a potentially useful information the log message adds. But it is just "potentially"---we cannot read what exact problem the change is trying to address. Making a pipe totally unbuffered is a heavy hammer that may not be an appropriate solution---it could be that we are missing calls to fflush() where we need and have been lucky because most of the systems we deal with do line-buffered by default, or something silly/implausible like that, and if that is the case, a more proper fix may be to add these missing fflush() to right places.
IOW, I do not see it explained clearly why this change is needed on any single platform---so "that issue may be shared by others, too" is a bit premature thing for me to listen to and understand, as "that issue" is quite unclear to me.