Re: [PATCH v2 0/2] Implement filtering repacks

From: Christian Couder <christian.couder@gmail.com>
Date: Nov 7, 2022, 09:29 UTC
Message-ID: <CAP8UFD2jjQxMsN7VzticiFkrOSLVK6f-f4R1fZU1wnEBpuHapQ@mail.gmail.com>
In-Reply-To: <Y1wzgOTiKdBcBba0@nand.local>

On Fri, Oct 28, 2022 at 9:54 PM Taylor Blau <me@ttaylorr.com> wrote:

Show 10 quoted lines

>
> On Tue, Oct 25, 2022 at 02:28:54PM +0200, Christian Couder wrote:
> > So there are only 2 patches now in this v2 series:
> >
> >   - Patch 1/2 is a preparatory patch.
> >
> >   - Patch 2/2 introduces the `--filter=<filter-spec>` option.
>
> One thing that I wasn't clear on in this or the previous round(s) was
> how we handle setting remote.<name>.promisor and partialclonefilter.

Yeah, I agree that it's an interesting question that I overlooked.

> If there is a single remote, then it's obvious that we should set
> promisor to "true" and partialCloneFilter to whatever value of
> `--filter` the user provided when repacking / GCing.

I would be Ok to setting remote.<name>.promisor to true in this case, but I am not sure we really need to do it.

Maybe the user is mostly interested in reducing the size of the repo for now and plans to set up a promisor remote afterwards.

Another perhaps better way to handle this would be to just die() if no remote.<name>.promisor is set to true. This way we can make sure that users will not forget to set up at least one promisor remote. This could also give users the opportunity to think about whether their configured remotes contain all the objects they are going to remove.

About remote.<name>.partialclonefilter I don't think we need to do anything. Maybe the user would be Ok with having different filters when fetching and when cleaning up.

> But what happens if there are multiple remotes? Which get the new
> configuration settings modified?

I agree that if we want this feature to modify settings, then there is no good and simple solution in this case.

> I wonder what breakage happens if we fail to do that (and why such
> breakage isn't yet noticed by CI).

If we want to avoid breakages as much as possible, then die()ing when no remote.<name>.promisor is set to true seems to be the best solution, instead of trying to modify settings.