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

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 29, 2024, 18:07 UTC
Message-ID: <ZyEkWyB/C/lGb36b@nand.local>
In-Reply-To: <20241028195404.4119175-1-jonathantanmy@google.com>

On Mon, Oct 28, 2024 at 12:54:04PM -0700, Jonathan Tan wrote:

Show 24 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > This new walk is incompatible with some features and is ignored by
> > others:
> >
> >  * Object filters are not currently integrated with the path-walk API,
> >    such as sparse-checkout or tree depth. A blobless packfile could be
> >    integrated easily, but that is deferred for later.
> >
> >  * Server-focused features such as delta islands, shallow packs, and
> >    using a bitmap index are incompatible with the path-walk API.
> >
> >  * The path walk API is only compatible with the --revs option, not
> >    taking object lists or pack lists over stdin. These alternative ways
> >    to specify the objects currently ignores the --path-walk option
> >    without even a warning.
>
> It might be better to declare --path-walk as "internal use only" and
> only supporting what send-pack.c (used by "git push") and "git repack"
> needs. (From this list, it seems that there is a lot of incompatibility,
> some of which can happen without a warning to the user, so it sounds
> better to be up-front and say that we only support what send-pack.c
> needs. This also makes reviewing easier, as we don't have to think about
> the possible interactions with every other rev-list feature - only what
> is used by send-pack.c.)

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

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.

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.

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