Re: [PATCH v3 13/23] log_ref_setup(): pass the open file descriptor back to the caller

On Sat, Dec 31, 2016 at 04:12:53AM +0100, Michael Haggerty wrote:

This function will most often be called by log_ref_write_1(), which
wants to append to the reflog file. In that case, it is silly to close
the file only for the caller to reopen it immediately. So, in the case
that the file was opened, pass the open file descriptor back to the
caller.

Sounds like a much saner interface.

 /*
- * Create a reflog for a ref.  If force_create = 0, the reflog will
- * only be created for certain refs (those for which
- * should_autocreate_reflog returns non-zero.  Otherwise, create it
- * regardless of the ref name.  Fill in *err and return -1 on failure.
+ * Create a reflog for a ref. Store its path to *logfile. If
+ * force_create = 0, only create the reflog for certain refs (those
+ * for which should_autocreate_reflog returns non-zero). Otherwise,
+ * create it regardless of the reference name. If the logfile already
+ * existed or was created, return 0 and set *logfd to the file
+ * descriptor opened for appending to the file. If no logfile exists
+ * and we decided not to create one, return 0 and set *logfd to -1. On
+ * failure, fill in *err, set *logfd to -1, and return -1.
  */
-static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)
+static int log_ref_setup(const char *refname,
+			 struct strbuf *logfile, int *logfd,
+			 struct strbuf *err, int force_create)

The return value is always "0" or "-1". It seems like it would be
simpler to just return the descriptor instead of 0.

I guess that makes it hard to identify the case when we chose not to
create a descriptor. I wonder if more "normal" semantics would be:

  1. ret >= 0: file existed or was created, and ret is the descriptor

  2. ret < 0, err is empty: we chose not to create

  3. ret < 0, err is non-empty: a real error

I dunno. This may just be bikeshedding, and I can live with it either
way (especially because you documented it!).

-Peff