On Thu, 2016-01-14 at 04:26 -0500, Jeff King wrote: > On Tue, Jan 12, 2016 at 04:22:09PM -0800, Junio C Hamano wrote: > > > David Turner <dturner@xxxxxxxxxxxxxxxx> writes: > > > > > This version incorporates many changes suggested by Michael > > > Haggerty, > > > Junio, Jonathan Nieder, Eric Sunshine, and Jeff King. I think I > > > have > > > addressed of the comments that were sent to me. Those that I > > > chose > > > not to incorporate, I responded to on the mailing list. > > > > > > Thanks for all of the feed back so far. > > > > Unfortunately this did not compile for me X-< and with a trivial > > fix-up, I found that this overlaps with Peff's recent fixes to the > > locking of symbolic refs. So for today's integration run, I > > punted. > > > > I still will push out this topic to the broken-out repository I > > keep > > here: > > > > https://github.com/gitster/git > > > > It's just 'pu' will not have this latest incarnation, but has the > > older one. > > 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. > 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. > So sorry, I don't have a quick resolution to this. I'm hoping David > can > make more sense of it than I did. 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). 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. -- 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