Re: What's cooking in git.git (Feb 2020, #01; Wed, 5)

From: Jeff King <peff@peff.net>
Date: Feb 7, 2020, 12:45 UTC
Message-ID: <20200207124552.GA4440@coredump.intra.peff.net>
In-Reply-To: <CAP8UFD1ascbutL5RGSGC1-eWvOJDpbcZ=edSG+WCd7aAkCmn8Q@mail.gmail.com>

On Fri, Feb 07, 2020 at 07:42:52AM +0100, Christian Couder wrote:

Show 31 quoted lines

> On Thu, Feb 6, 2020 at 12:36 AM Junio C Hamano <gitster@pobox.com> wrote:
> 
> > * jk/packfile-reuse-cleanup (2020-01-23) 12 commits
> >  - pack-bitmap: don't rely on bitmap_git->reuse_objects
> >  - pack-objects: add checks for duplicate objects
> >  - pack-objects: improve partial packfile reuse
> >  - builtin/pack-objects: introduce obj_is_packed()
> >  - pack-objects: introduce pack.allowPackReuse
> >  - csum-file: introduce hashfile_total()
> >  - pack-bitmap: simplify bitmap_has_oid_in_uninteresting()
> >  - pack-bitmap: uninteresting oid can be outside bitmapped packfile
> >  - pack-bitmap: introduce bitmap_walk_contains()
> >  - ewah/bitmap: introduce bitmap_word_alloc()
> >  - packfile: expose get_delta_base()
> >  - builtin/pack-objects: report reused packfile objects
> >
> >  The way "git pack-objects" reuses objects stored in existing pack
> >  to generate its result has been improved.
> >
> >  Will merge to 'next'?
> 
> I think that it would be indeed ok to merge to next if no one comments
> further following the v4 [1] and my last reply to Peff [2]
> 
> [1] https://lore.kernel.org/git/20191218112547.4974-1-chriscool@tuxfamily.org/
> [2] https://lore.kernel.org/git/CAP8UFD0h1TFVNqH7g823psaQzmEmzoz200CkZuDOV8GqNV7mrQ@mail.gmail.com/
> 
> According to Peff the code in this patch series has been running by
> GitHub for years. This patch series is also now used in a custom Git
> that has been running on gitlab.com for a few days. This custom Git is
> planned to be part of the GitLab 12.8 release later this month.

Yeah, I think the only lingering issue is that one I pointed out where the big "initial chunk" reuse doesn't carefully check for any REF_DELTA objects whose bases we're not including. But that exists in the current scheme, and in what we've been running at GitHub for a long time. And is probably OK by the reasoning I gave in the thread: these packs must have bitmaps, and therefore we wrote them as non-thin, and therefore they would have only OFS deltas. I think it would also be OK even if you tried repacking without --delta-base-offset, because we always put bases before their deltas (aside from --fix-thin, maybe?)

So I'd be OK to proceed with the series as-is, but it does give me a slight worry. Both that this assumption could somehow be violated, but also that the other parts of the code trying to handle REF_DELTAs are probably not getting exercised at all. It would be kind of interesting to cover this case with a test, but I'm not even sure there's a way that you could generate such a pack with `pack-objects` because of the bases-before-deltas ordering constraint.

-Peff