On Thu, Jan 14, 2016 at 11:25:52AM -0500, David Turner wrote: > > 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. > > 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 -- 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