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

From: Jeff King <peff@peff.net>
Date: Jan 13, 2021, 13:25 UTC
Message-ID: <X/701Isbo8M9W2VP@coredump.intra.peff.net>
In-Reply-To: <20210112082159.2277214-2-chriscool@tuxfamily.org>

On Tue, Jan 12, 2021 at 09:21:59AM +0100, Christian Couder wrote:

Show 10 quoted lines

> Let's replace the 2 different pieces of code that write a
> promisor file in 'builtin/repack.c' and 'fetch-pack.c'
> with a new function called 'write_promisor_file()' in
> 'pack-write.c' and 'pack.h'.
> 
> This might also help us in the future, if we want to put
> back the ref names and associated hashes that were in
> the promisor files we are repacking in 'builtin/repack.c'
> as suggested by a NEEDSWORK comment just above the code
> we are refactoring.

I think the interface for the callers is much nicer. Not a new problem, but one thing I did notice in the implementation:

Show 10 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;

(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).

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.

-Peff