Re: [PATCH] refs: write packed_refs file using stdio

[Jeff King <peff@peff.net>]

On Wed, Sep 10, 2014 at 01:21:27PM +0200, Michael Haggerty wrote:

> > +	if (fclose(out))
> > +		die_errno("write error");
> > +	packed_ref_cache->lock->fd = -1;
> 
> It might be a minuscule bit safer to set `lock->fd = -1` *before*
> calling `fclose()`.

Yeah, I considered that. The worst case is a signal coming in between
the two calls (or a somebody calling die() between the two :) ). In that
case the lockfile code will close() the fd again, which should be a noop
(since nobody will have opened it in the interim...right?).

That "since" assumption is the dangerous part. But on the other hand, if
we unset the fd first, then the lockfile code may fail to close it if it
is called before the fclose() (definitely by a signal in this case). I
think there are platforms where that would cause us to fail to remove
the file, which is annoying.

So I dunno. We cannot be atomic here. I could go either way.

> TBH, it makes me uncomfortable having code outside of `lockfile.c`
> having this level of intimacy with lockfile objects.

I kind of agree.

> I think it would be better to have a
> 
>     FILE *fopen_lock_file(struct *lock_file, const char *mode);
> 
> that records the `FILE *` inside the `lockfile` instance, and to teach
> `commit_lock_file()` and its friends to call `fclose()` if the `FILE *`
> was created. I think that such a feature would encourage other lockfile
> users to use the more convenient and readable stdio API.

I was tempted by that. We could also do:

  if (fflush(out))
	die_errno("write error");
  if (commit_lock_file(...))
     ...

which sidesteps the issue. We do then have to _later_ call fclose(out)
to free the handle memory, which is going to want to close the fd again.
Putting us back in the "let's hope nobody opened it in the meantime"
case from above.

-Peff