From: David Turner Date: Thu, 14 Jan 2016 16:25:52 GMT Subject: Re: [PATCH v2 00/21] refs backend reroll Message-ID: <1452788752.16226.17.camel@twopensource.com> In-Reply-To: <20160114092614.GA8533@sigill.intra.peff.net> 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 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.