Re: [PATCH v4 0/6] pack-objects: freshen objects with multi-cruft packs

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 12, 2025, 19:52 UTC
Message-ID: <Z9Hl+UpnEq07qFeW@nand.local>
In-Reply-To: <xmqqa59qxc9b.fsf@gitster.g>

On Wed, Mar 12, 2025 at 12:46:56PM -0700, Junio C Hamano wrote:

Show 11 quoted lines

> Allowing the resulting pack to deliberately exceed 200MB ceiling is
> one possible way (I still do not think it is the best way among
> possible ways) to mark the pack "frozen-do not further touch".
>
> Now, if the essense of the problem is when to mark a pack that we
> stopped appending to due to max-size limit as "frozen and do not
> touch" to avoid this problem, how such a marking is done, and who
> uses it to avoid repeated work that is known (to us) to be futile,
> there should be better ways to do such a marking.  We have .keep
> mechanism which is ugly but proven to work and we know how to deal
> with, for example.

Yeah... I think a .keep-like mechanism would be sufficient here to mark packs which are close to the --max-cruft-size as frozen if we truly wanted to avoid this problem.

But if we have, say, packs that are 198MB, 1MB, 2MB, and 3MB in size, that we're going to accumulate in order of increasing size, so we would almost never repack the 198MB pack.

The .keep mechanism by itself wouldn't quite do the trick, though, because we'd have to distinguish between "marked as .kept by the user and so we absolutely must not delete it" and "marked as frozen by us, but we could delete it if we needed to (e.g., when pruning)".

So that would maybe suggest introducing a .frozen mechanism, but that is bolting further ugliness onto the .keep mechanism, which I think is the wrong direction to be going in.

Show 8 quoted lines

> I wonder if our pack header or trailer is extensible enough so that
> we can record "this pack was closed due to max-size limit of 200MB
> and appending even one more object in the queue would have busted
> the limit".
>
> No matter how such a marking is done, later repack that sees a pack
> marked as such, when it is limited to 200MB, should be able to tell
> that and act a bit more intelligently than what it currently does.

Perhaps in the longer term, but I think for the reasons above that the existing behavior (plus the new patch from v3, which we should still queue) is sufficient.

Thanks, Taylor