Re: [PATCH 0/4] Always release pack files before calling gc --auto

From: Jeff King <peff@peff.net>
Date: Jan 13, 2016, 17:52 UTC
Message-ID: <20160113175237.GA13139@sigill.intra.peff.net>
In-Reply-To: <cover.1452705584.git.johannes.schindelin@gmx.de>

On Wed, Jan 13, 2016 at 06:20:02PM +0100, Johannes Schindelin wrote:

Show 9 quoted lines

> These patches are companions to 786b150 (clone --dissociate: avoid
> locking pack files, 2015-10-06), triggered by a bug report to the Git
> for Windows project.
> 
> Johannes Schindelin (4):
>   fetch: release pack files before garbage-collecting
>   am: release pack files before garbage-collecting
>   merge: release pack files before garbage-collecting
>   receive-pack: release pack files before garbage-collecting

I think this is fine, but I noticed an almost-problem that I think is worth mentioning.

Closing packs can open up race conditions in some cases. I.e., we think we have an object in a particular pack, do some work on that basis, and then later find the pack to be inaccessible (due to somebody else running gc). If we keep the fd open, there's no race, but if we close it, we can't reopen it.

I think all of the cases here are fine, for two reasons:

  1. The new closing is right before the program would exit anyway, so
     there's no interesting work left to do (and obviously that's where
     we generally want to call "gc --auto", too)

  2. This type of race might only be an issue for pack-objects (or at least that's
     the only place I've seen it).  Most normal read_sha1_file() callers
     will happily re-scan the pack directory for the new packs upon
     finding that the old pack went away. But pack-objects peeks in the
     packfiles a little more intimately, and will record the actual pack
     and offset (for delta reuse).

     It _might_ also be possible to trigger this race using bitmaps,
     which are also pretty intimate with the packfile code. I'm not
     sure.

So I think this series is OK.

-Peff