Re: [PATCH 08/17] pack-objects: add --path-walk option

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 29, 2024, 21:36 UTC
Message-ID: <20241029213657.1303762-1-jonathantanmy@google.com>
In-Reply-To: <ZyEkWyB/C/lGb36b@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

> Is the thinking there that we care mostly about 'git push' and 'git
> repack' on the client-side?

I would go further - for the initial patch set, we should only care about "git push" on the client side. Stolee said [1] that the "primary motivation for this feature is its use to shrink the packfile created by 'git push' when there are many name-hash collisions", and in thinking about how to reduce the patch set for easier review, I thought that to be a good scope.

Subsequent patch set(s) can implement "git repack", useful for both client and server.

[1] https://lore.kernel.org/git/pull.1813.v2.git.1729431810.gitgitgadget@gmail.com/

Show 5 quoted lines

> I don't think it's unreasonable necessarily, but I would add that
> client-side users definitely do use bitmaps (though not delta islands),
> either when working in a bare repository (where bitmaps are the default)
> or when using 'git gc' (and/or through 'git maintenance') when
> 'repack.writeBitmaps' is enabled.

I was thinking that a typical use case would be to create the commits (using the tool Stolee mentioned, "beachball") and then immediately push them. In this case, I don't think there would be much opportunity for a bitmap write to be triggered, meaning that the pushed commits are not covered by bitmaps.

But in any case, this was motivated by a desire to reduce the patch set
- I don't have a fundamental objection to including support for bitmaps
in the first patch set.

> So I think the approach here would be to limit it to some cases of
> client side behavior, but we should keep in mind that it will not cover
> all cases.

Yeah, that was my approach too.

Show 6 quoted lines

> My feeling is that it would be nice to pull on the incompatibility
> string a little more and see if we can't make the two work together
> without too much effort.
> 
> Thanks,
> Taylor

By incompatibility, do you mean the incompatibility between bitmaps and the overall --path-walk feature as implemented collectively by the patches in Stolee's patch set? If so, I suspect that we will need a parallel code path that takes in the "want" and "uninteresting" commits and emits the list of objects (possibly before sorting the objects by path hash), much like in builtin/pack-objects.c, so I think there will be some effort involved in making the two work together.