Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory

From: Jeff King <peff@peff.net>
Date: Dec 30, 2015, 07:37 UTC
Message-ID: <20151230073759.GA785@sigill.intra.peff.net>
In-Reply-To: <CAEtYS8S_ys3jT5ziWd7_u6Dn8b3LwnZYO7Pz6EegsmWpUM5riw@mail.gmail.com>

On Wed, Nov 04, 2015 at 02:08:21PM -0600, Doug Kelly wrote:

Show 26 quoted lines

> On Wed, Nov 4, 2015 at 2:02 PM, Jeff King <peff@peff.net> wrote:
> > Definitely cleaning up the .bitmap is sane and not racy (it's in the
> > same boat as the .idx, I think).
> >
> > .keep files are more tricky. I'd have to go over the receive-pack code
> > to confirm, but I think they _are_ racy. That is, receive-pack will
> > create them as a lockfile before moving the pack into place. That's OK,
> > though, if we use mtimes to give ourselves a grace period (I haven't
> > looked at your series yet).
> >
> > But moreover, .keep files can be created manually by the user. If the
> > pack they referenced goes away, they are not really serving any purpose.
> > But it's possible that the user would want to salvage the content of the
> > file, or know that it was there.
> >
> > So I'd argue we should leave them. Or at least leave ones that do not
> > have the generic "{receive,fetch}-pack $pid on $host comment in them,
> > which were clearly created as lockfiles.
> 
> Currently there's no mtime-guarding logic (I dug up that conversation
> earlier, though, but after I'd done the respin on this series)... OK,
> in that case, I'll create a separate patch that tests/cleans up
> .bitmap, but doesn't touch .keep.  This might be a small series since
> I think the logic for finding pack garbage doesn't know anything about
> .bitmap per-se, so it's looking like I'll extend that relevant code,
> before adding the handling in gc and appropriate tests.

I happened to be looking over your series again, and I noticed that we didn't end up with any mtime logic at all in what got merged.

I _think_ that is probably OK, because we always write the pack, followed by the .idx, followed by the .bitmap (if any). And we don't drop .keep files (though I think we would perhaps note them as possible cruft?).

So I don't think there are any races introduced here, but I wonder if we want to be a bit more conservative. Sorry to bring this up so much after the fact; I completely forgot about it when reviewing the patches.

These changes are slated for the v2.7 release. Like I said, I don't think it's buggy, so we don't necessarily need to address it before the release. We could add an mtime check in the next cycle as a belt-and-suspenders safety, rather than a fix.

-Peff