Re: [PATCH 2/2] upload-pack: use stdio in send_ref callbacks

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 26, 2021, 22:35 UTC
Message-ID: <YSgXKgHgPwo/8aUW@nand.local>
In-Reply-To: <xmqqpmu0f9ob.fsf@gitster.g>

On Thu, Aug 26, 2021 at 09:33:08AM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> > @@ -171,6 +171,9 @@ int ls_refs(struct repository *r, struct strvec *keys,
> >  		strvec_push(&data.prefixes, "");
> >  	for_each_fullref_in_prefixes(get_git_namespace(), data.prefixes.v,
> >  				     send_ref, &data, 0);
> > +	/* Call fflush because send_ref uses stdio. */
> > +	if (fflush(stdout))
> > +		die_errno(_("write failure on standard output"));
>
> There is maybe_flush_or_die() helper that does a bit too much (I do
> not think this code path needs to worry about GIT_FLUSH) but does
> call check_pipe(errno) like packet_write_fmt() does upon seeing a
> write failure.
>
> >  	packet_flush(1);

I was somewhat surprised to see the fflush call here and not in a companion to the existing packet_flush (when working with stdio instead of file descriptors, of course).

What Jacob wrote is not wrong, of course, but I think having packet_fflush() or similar would be less error-prone.

> OK.  This step looks quite sensible, other than the same "do we want
> check_pipe() before dying upon fflush() failure?" we see in the last
> hunk below.  I didn't mention this in the review of 1/2, but the new
> fwrite_or_die() helper function may also have the same issue.

Agreed.

Thanks, Taylor