Re: [PATCH] repack: add `repack.honorpackkeep` config var

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 28, 2014, 18:45 UTC
Message-ID: <xmqqob1ruld8.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140228085546.GA11709@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 29 quoted lines

> On Thu, Feb 27, 2014 at 10:04:44AM -0800, Junio C Hamano wrote:
>
>> I wonder if it makes sense to link it with "pack.writebitmaps" more
>> tightly, without even exposing it as a seemingly orthogonal knob
>> that can be tweaked, though.
>> 
>> I think that is because I do not fully understand the ", because ..."
>> part of the below:
>> 
>> >> This patch introduces an option to disable the
>> >> `--honor-pack-keep` option.  It is not triggered by default,
>> >> even when pack.writeBitmaps is turned on, because its use
>> >> depends on your overall packing strategy and use of .keep
>> >> files.
>> 
>> If you ask --write-bitmap-index (or have pack.writeBitmaps on), you
>> do want the bitmap-index to be written, and unless you tell
>> pack-objects to ignore the .keep marker, it cannot do so, no?
>> 
>> Does the ", because ..." part above mean "you may have an overall
>> packing strategy to use .keep file to not ever repack some subset of
>> the objects, so we will not silently explode the kept objects into a
>> new pack"?
>
> Exactly. The two features (bitmaps and .keep) are not compatible with
> each other, so you have to prioritize one. If you are using static .keep
> files, you might want them to continue being respected at the expense of
> using bitmaps for that repo. So I think you want a separate option from
> --write-bitmap-index to allow the appropriate flexibility.

What is "the appropriate flexibility", though? If the user wants to use bitmap, we would need to drop .keep, no? Doesn't always having two copies in two packs degrade performance unnecessarily (without even talking about wasted diskspace)? An explicit .keep exists in the repository because it is expensive and undesirable to duplicate what is in there in the first place, so it feels to me that either

 - Disable with warning, or outright refuse, the "-b" option if
   there is .keep (if we want to give precedence to .keep); or

 - Remove .keep with warning when "-b" option is given (if we want
   to give precedence to "-b").

and nothing else would be a reasonable option. Unfortunately, we can do neither automatically because there could be a transient .keep file in an active repository.

So I think I agree with this...

Show 5 quoted lines

> The default is another matter.  I think most people using .bitmaps on a
> server would probably want to set repack.packKeptObjects.  They would
> want to repack often to take advantage of the .bitmaps anyway, so they
> probably don't care about .keep files (any they see are due to races
> with incoming pushes).

... which makes me think that repack.packKeptObjects is merely a distraction---it should be enough to just pass "--pack-kept-objects" when "-b" is asked, without giving any extra configurability, no?

> So we could do something like falling back to turning the option on if
> --write-bitmap-index is on _and_ the user didn't specify
> --pack-kept-objects.

If you mean "didn't specify --no-pack-kept-objects", then I think that is sensible. I still do not know why we would want the configuration variable, though.