Re: [PATCH 2/1] repack: warn if bitmaps are explicitly enabled with keep files

From: Jeff King <peff@peff.net>
Date: Jul 3, 2019, 17:38 UTC
Message-ID: <20190703173814.GA29348@sigill.intra.peff.net>
In-Reply-To: <xmqq7e917h9x.fsf@gitster-ct.c.googlers.com>

On Mon, Jul 01, 2019 at 11:15:38AM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> >> Maybe that's tricky with the gc.log functionality, but I think we should
> >> at least document this before the next guy shows up with "sometimes my
> >> .bitmap files aren't generated...".
> >
> > I'm not sure if the warning should be present by default;
> > because we'll silently stop using bitmaps, now.  But warning
> > if '-b' is specified seems right:
> 
> Hmph...  write_bitmaps can come either from the command line or from
> the configuration.  When repack.writebitmaps configuration is set,
> and some .keep files exist, the user probably is not even aware of
> doing something that is unsupported.

I think one tricky thing here is that we do not know if the .keep files are meant to be there, or if they are simply transient locks.

The whole point of the current behavior is that we should be ignoring the transient locks, and if you are explicitly using bitmaps you understand the tradeoff you are making.

I think the important case to cover is the one where the user didn't explicitly ask for them, and the initial patch (to disable them when there's a .keep around) covers that.

A much more robust solution would be to stop conflating user-provided permanent .keep files with temporary locks. I think that was a mistaken design added many years ago. We probably could introduce a different filename for the temporary locks (though I am not entirely convinced they are necessary in the first place, as gc expiration-times would generally save a racily-written packfile anyway).

Or perhaps we could differentiate our temporary locks from "real" .keep files by looking at the content; I think our locks always say something like "(receive|receive)-pack \d+ on .*", and it wouldn't be too onerous to commit to that, I think (or even adjust it to something even more unambiguous).

It does muddy the meaning of packed_git.pack_keep a bit. Some callers would want to consider it kept in either case (i.e., for purposes of pruning, we delete neither) and some would want it kept only for non-locks (for packing, duplicating the objects is OK). So I think we'd end up with two bits there, and callers would have to use one or the other as appropriate.

-Peff