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

[Jeff King <peff@peff.net>]

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

> >> 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