On Thu, Aug 26, 2021 at 09:33:08AM -0700, Junio C Hamano wrote:
> > @@ -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
