On Mon, Mar 18 2019, Jeff King wrote: > On Mon, Mar 18, 2019 at 05:14:58PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> This series is unrelated (and does not conflict with) my in-flight gc >> contention series >> (https://public-inbox.org/git/20190315155959.12390-1-avarab@xxxxxxxxx/), >> but the "git-gc" docs should be updated to discuss the >> core.filesRefLockTimeout option and how it impacts contention, see 8/8 >> in that series for context. I.e. "you may have contention, but >> core.filesRefLockTimeout can mitigate blah blah". >> >> I was going to do that, but then thought that we should also mention >> that on the server-side we mitigate most/all of the contention via the >> quarantine, see "QUARANTINE ENVIRONMENT" in >> git-receive-pack(1). I.e. we: >> >> 1. Get the temp pack >> 2. OK it (fsck, hooks etc.) >> 3. Move *complete* previously temp packs over >> 4. Update the refs >> >> I.e. we are immune from the "concurrently with another process" race, >> but of course something concurrently updating the "server" repo >> without a quarantine environment may be subject to that race. >> >> The only problem is that the last couple of paragraphs may be >> wrong. That's just my understanding from a brief reading of >> 722ff7f876c ("receive-pack: quarantine objects until pre-receive >> accepts", 2016-10-03) so I didn't want to include that in this >> series. Peff (or others), any comments? > > I don't think the quarantine stuff should impact contention at all. It's > only quarantining the objects, which are the least contentious part of > Git (because object content is idempotent, so we don't do any locking > there, and with two racing processes, one will just "win"). Without the quarantine, isn't there the race that the NOTES section talks about (unless I've misread it). I.e. we have some loose object "ABCD" not referrred to by anything for the last 2 weeks, as we're gc-ing a ref update comes in that makes it referenced again. We then delete "ABCD" (not used!) at the same time the ref update happens, and get corruption. Whereas the quarantine might work around since the client will have sent ABCD with no reference pointing to it to the server in the temp pack, which we then rename in-place and then update the ref, so we don't care if "ABCD" goes away. Unless that interacts racily with the receive.unpackLimit, but then I have no idea that section is trying to say... Also, surely the part where "NOTES" says something to the effect of "you are subject to races unless gc.auto=0" is wrong. To the extent that there's races it won't matter that you invoke "git gc" or "git gc --auto", it's the concurrency that matters. So if there's still races we should be saying the repo needs to be locked for writes for the duration of the "gc".
