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

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

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:

+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