Re: [PATCH 4/4] create_symref: use existing ref-lock code

From: Jeff King <peff@peff.net>
Date: Dec 22, 2015, 00:58 UTC
Message-ID: <20151222005824.GB6901@sigill.intra.peff.net>
In-Reply-To: <xmqq8u4nfrvv.fsf@gitster.mtv.corp.google.com>

On Mon, Dec 21, 2015 at 12:50:28PM -0800, Junio C Hamano wrote:

Show 21 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> >  #ifndef NO_SYMLINK_HEAD
> > -	if (prefer_symlink_refs) {
> > -		unlink(ref_path);
> > -		if (!symlink(target, ref_path))
> > -			goto done;
> 
> I see that the original was sloppy (most certainly my bad) ...
> 
> > +	char *ref_path = get_locked_file_path(lock->lk);
> > +	unlink(ref_path);
> 
> ... and you inherited that.  I see a few seemingly related helpers
> in wrapper.c, but none looks useful in this context X-<.
> 
>     if (unlink_or_warn(ref_path))
>     	return -1;
> 
> is close enough, but it still lets the caller fallback to textual
> symref.

I don't think the original is _wrong_; it's meant to be "unlink if needed", and the symlink call is what we really care about (if our unlink failed for anything but ENOENT, the symlink will, too). But I agree unlink_or_warn should do that and give us a nice warning (and cover up ENOENT). To fall through, I think we just want (in the original):

  if (!unlink_or_warn(ref_path) && !symlink(target, ref_path))
	goto done;

and in mine that becomes:

  int ret = -1;
  ...
  if (!unlink_or_warn(ref_path) && !symlink(target, ref_path))
	ret = 0;

I must confess I considered a follow-on patch to drop the prefer_symlink_refs code path completely. I'm surprised it all still works with packed-refs, etc, but resolve_ref does take special care to use readlink().

-Peff