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

[Jeff King <peff@peff.net>]

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

> 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