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

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

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.

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.

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