On Mon, Dec 21, 2015 at 12:50:28PM -0800, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html