Re: [RFC] git gc "--prune=now" semantics considered harmful

From: Jeff King <peff@peff.net>
Date: Jun 1, 2018, 07:04 UTC
Message-ID: <20180601070456.GB15578@sigill.intra.peff.net>
In-Reply-To: <xmqqd0xim1tp.fsf@gitster-ct.c.googlers.com>

On Sun, May 27, 2018 at 08:31:14AM +0900, Junio C Hamano wrote:

Show 17 quoted lines

> > So I actually would much prefer that foir git gc, "--prune=now" means
> >
> >  (a) "now"
> >
> >  (b) now at the _start_ of the "git gc" operation, not the time at
> >      the _end_ of the operation when we've already spent a minute or
> >      two doing repacking and are now doing the final pruning.
> >
> > anyway, with that explanation in mind, I'm appending a patch that is 
> > pretty small and does that. It's a bit hacky, but I think it still makes 
> > sense.
> >
> > Comments?
> 
> Closing the possiblity of racing a running "gc" and new object
> creation like the above generally makes sense, I would think,
> whether the creation is due to 'pull/fetch', 'add', or even 'push'.

I think Linus's suggestion is an obvious improvement. It does shorten the window for confusing things to happen, and I think it makes things much easier to reason about if all parts of the gc are using the same timestamp.

Regarding the implementation:

Show 8 quoted lines

> > -	if (prune_expire && parse_expiry_date(prune_expire, &dummy))
> > -		die(_("failed to parse prune expiry value %s"), prune_expire);
> > +	if (prune_expire) {
> > +		if (!strcmp(prune_expire, "now"))
> > +			prune_expire = show_date(time(NULL), 0, DATE_MODE(ISO8601));
> > +		if (parse_expiry_date(prune_expire, &dummy))
> > +			die(_("failed to parse prune expiry value %s"), prune_expire);
> > +	}

We'd also accept relative times like "5.minutes.ago" (in fact, the default is a relative 2.weeks.ago, though it's long enough that the difference between "2 weeks" and "2 weeks plus 5 minutes" may not matter much). So we probably ought to just normalize _everything_ without even bothering to match "now". It's a noop for non-relative times, but that's OK.

Show 6 quoted lines

> I however have to wonder if there are opposite "oops" end-user
> operation we also need to worry about, i.e. we are doing a large-ish
> fetch, and get bored and run a gc fron another terminal.  Perhaps
> *that* is a bit too stupid to worry about?  Auto-gc deliberately
> does not use 'now' because it wants to leave a grace period to avoid
> exactly that kind of race.

There are still possibilities for a race, even with the grace period. You can have an unreferenced 2-week-old object sitting on disk, and somebody can choose to reference it at the same time as we are pruning it. My freshness patches from a few years ago made things a bit better:

  - when we optimize out the write of an existing object, we now at
    least update its timestamp

  - we consider non-fresh objects reachable from fresh ones to also be
    fresh

But fundamentally none of this is atomic. You can have an old tree, and while you're pruning somebody writes a new commit referencing it and sticks that in a ref. It's more common if your grace period is "now", but it can still happen with any grace period.

-Peff