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