Re: [PATCH 1/3] pack-objects: fix handling of multiple --filter options

From: Jeff King <peff@peff.net>
Date: Nov 13, 2022, 16:44 UTC
Message-ID: <Y3Ee1o82JS6Lxr71@coredump.intra.peff.net>
In-Reply-To: <Y3B6PxNaZvmm5OD2@nand.local>

On Sun, Nov 13, 2022 at 12:01:51AM -0500, Taylor Blau wrote:

Show 12 quoted lines

> > Yikes. I wondered how we managed to miss this in the tests. Surely we
> > have some examples that use multiple filters?
> >
> > I think the answer is that we do in t5616, but in practice they didn't
> > hit this bug because of the way filter-specs are treated server-sid3.
> > Even if we say "clone --filter=foo --filter=bar" on the client, the
> > server upload-pack coalesces that into a single "combine:foo+bar"
> > string, and that's what its pack-objects sees.
> 
> ...Or in other words, clients aren't broken here because we always send
> "combine" filters when multiple `--filter` arguments are passed to `git
> clone`, `git fetch`, etc.

Right.

> Is that always the case? I *think* so, but it would be nice to know if
> there was a way that clients would not send the `combine` filter with >1
> filter.

I think it's always the case at least with Git's code, because the client-side send_filter() sends from list_objects_filter_spec(), which takes the single-string spec of the top-level combine filter. It would have to explicitly iterate over the sub-filters to come up with anything else, and I don't think we even have a function for that (we do when evaluating them, of course, but not for generically iterating or producing a multi-string set of specs).

And on the server side, even if a client did send multiple "filter" lines, I think we'd hit list_objects_filter_die_if_populated(), so we'd complain.

I don't know of any other code paths that could hit this (can you filter on an outgoing push?). But in the end I'm not sure it even matters. We should be fixing the bug anyway, and this is all just understanding how far-reaching its effects are (and the answer seems to be: not very).

-Peff