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

From: David Turner <[email-masked]>
Date: Thu, 14 Jan 2016 11:25:52 -0500

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 <[email-masked]> 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.