Re: [PATCH 2/2] fetch-pack: refactor writing promisor file

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 13, 2021, 15:20 UTC
Message-ID: <X/8PtpCRJ4w19zGs@nand.local>
In-Reply-To: <X/701Isbo8M9W2VP@coredump.intra.peff.net>

On Wed, Jan 13, 2021 at 08:25:40AM -0500, Jeff King wrote:

Show 20 quoted lines

> > +void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)
> > +{
> > +	int i;
> > +	FILE *output = xfopen(promisor_name, "w");
> > +
> > +	for (i = 0; i < nr_sought; i++)
> > +		fprintf(output, "%s %s\n", oid_to_hex(&sought[i]->old_oid),
> > +			sought[i]->name);
> > +	fclose(output);
> > +}
>
> We check errors on open via xfopen(), which is good. But we would not
> notice any problems writing via fprintf or fclose. Is it worth doing
> something like:
>
>   err = ferror(output);
>   err |= fclose(output);
>   return err ? -1 : 0;
>
> ?

I agree below that *not* doing this isn't a regression against the current code, since it doesn't check either, but this could be done relatively easily. It is appropriate for both callers of write_promisor_file() to immediately die() if they get an error, so I think that this is potentially worth doing.

> (As an aside, this ferror/fclose dance is awkward enough and has caused
> us enough questions in the past that I wonder if it is worth
> encapsulating into a wrapper).

From a quick grep through uses of ferror, there are a reasonable handful of spots that I think could be improved if there was a ferror+fclose helper, perhaps: xfclose().

> The existing callers behave the same way (checking open, but not the
> writes), so definitely not a regression. But the helper function may
> provide an opportunity to make things more robust without adding a lot
> of duplicated code.

Yep.

> -Peff

Thanks, Taylor