Re: [PATCH v2 00/21] refs backend reroll

On Thu, Jan 14, 2016 at 11:25:52AM -0500, David Turner wrote:

quoted

I took a look at David's changes. The conflicts come from "refs:
resolve symbolic refs first". I'm not sure I fully understand all
that
is going on in that patch, but it looks like after it, we are less
likely to handle ENOTDIR and d/f conflicts for symrefs, as we skip
that
whole code path for REF_ISSYMREF.

We only get into the symref part of that codepath if there's already a
symref present, meaning that d/f conflicts can't happen.

Ah, right, that makes sense.

quoted

The rest of the conflicts are related to the fact that all of the
initial resolution is pulled out of lock_ref_sha1_basic(), and the
caller is supposed to do it. So I think if create_symref() is going
to
call lock_ref_sha1_basic(), as in my series, when combined with
David's
it should also be calling dereference_symrefs(). That uses a
ref_transaction, which we don't have in create_symref() right now,
but
it makes sense that we would ultimately want to push symref updates
through the same transaction/backend system.

I don't think that's quite true.  create_symref *always* creates
symrefs, and never creates underlying refs.  So it calls
lock_ref_sha1_basic(), but since type_p is NULL, it doesn't go into the
resolved-symlinks path; instead, we get into the original codepath.

I'm not sure in which version of the code you mean here. I guess in the
merged one, because in your original create_symref is still
pre-lock_ref_sha1_basic. But I think...

I was totally convinced that we were doomed, but I think the stupid
resolution basically works, with some minor tweaks.  I'm going to re
-review that patch and resend the series (then go out of town until
Tuesday).

...the best thing for me to do is wait and see your revised patch. :)

Thanks for looking into it.

We will need to apply your new d/f conflict check to the LMDB backend's
symref code (presently, it fails your new test), but I'm going to punt
on that for now since d/f conflicts don't cause problems for the LMDB
backend and this is a relatively minor case.  I've added a TODO to the
code.

Makes sense.

-Peff