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

From: Jeff King <peff@peff.net>
Date: Jan 14, 2016, 20:12 UTC
Message-ID: <20160114201201.GA1985@sigill.intra.peff.net>
In-Reply-To: <1452788752.16226.17.camel@twopensource.com>

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

Show 10 quoted lines

> > 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.

Show 16 quoted lines

> > 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.

Show 5 quoted lines

> 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