RE: [PATCH v2 0/6] Force pipes to flush immediately on NonStop platform

From: Randall S. Becker <rsbecker@nexbridge.com>
Date: Jan 22, 2018, 22:43 UTC
Message-ID: <001401d393d2$73458ef0$59d0acd0$@nexbridge.com>
In-Reply-To: <xmqqshaxjzcc.fsf@gitster.mtv.corp.google.com>

On January 22, 2018 5:36 PM, Junio C Hamano wrote:

Show 55 quoted lines

> Torsten Bögershausen <tboegi@web.de> writes:
> 
> > 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.uname
> 
> I 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.

v4 might be a little better. The issue seems to be specific to NonStop that it's PIPE mechanism needs to have setbuf(pipe,NULL) called for git to be happy. The default behaviour appears to be different on NonStop from other platforms from our testing. We get hung up waiting on pipes unless this is done. At the moment, this is platform-specific. Other parts of the discussion led to the conclusion that we should make this available to any platform using a new configuration option, but my objective is to get the NonStop port integrated with the main git code base and when my $DAYJOB permits it, spend the time adding the option. Note: __TANDEM is #define automatically emitted by the NonStop compilers.

Does that help?

Sincerely, Randall