Re: [PATCH v2 29/33] refs: resolve symbolic refs first

From: Jeff King <peff@peff.net>
Date: May 13, 2016, 12:51 UTC
Message-ID: <20160513125155.GA24031@sigill.intra.peff.net>
In-Reply-To: <5735C990.8080502@alum.mit.edu>

On Fri, May 13, 2016 at 02:33:20PM +0200, Michael Haggerty wrote:

Show 7 quoted lines

> The problem in this case is a misguided call to
> verify_refname_available_dir() in the case that read_raw_ref() fails
> with ENOENT. In that case it is not possible for there to be a conflict
> with another loose reference, because (1) we already hold the lock, so
> the containing directory must exist, and (2) we got ENOENT, so there
> can't be a loose reference in a subdirectory named after the reference
> that we are trying to create.

Right, that makes perfect sense.

Show 6 quoted lines

> As Peff explained, the call of verify_refname_available_dir() was
> forcing the loose tags to be loaded, which is expensive in this test
> because there are 100000 of them being created one at a time. (If they
> were created in a single ref_transaction instead, the "available" tests
> would all be done together, before any changes are committed, so the
> loose ref cache would not have to be invalidated each time.)

Yeah, I noticed that, too, and had to wonder whether we should make "fetch" do a single ref transaction. I think the thing blocking that (besides the obvious refactoring needed in fetch) is that transactions are currently all-or-nothing. And fetch right now will do its best to update whatever refs it can.

Show 6 quoted lines

> Please note that there are still some calls of
> verify_refname_available_dir() against the loose reference cache in this
> function. If we wanted to give up a little bit on the quality of our
> error messages, I think we could make those paths faster, too. But they
> are all in failure paths, so I don't think that they are performance
> critical, so I won't make that change.

That's probably fine. I think the most pathological case becomes a fetch a fetch where every other incoming ref is bogus. So, "refs/tags/2/nope", "refs/tags/2-ok". And there are 100K of those (or whatever large number you choose).

The bogus refs cause us to load the loose ref cache, and the successful ones cause us to discard it. The result is still quadratic (it's just n/2 squared). I have a hard time believing anybody would run into this situation in practice, but I do wonder if somebody could cause denial-of-service mischief. Probably not any more than they could cause by other means (like sending a nasty diff case).

-Peff