Re: [PATCH 8/9] repack: implement `--filter-to` for storing filtered out objects

From: Christian Couder <christian.couder@gmail.com>
Date: Jun 21, 2023, 12:08 UTC
Message-ID: <CAP8UFD388DOTqueok7fRF+=-+s_PNria1oB18AEed3gjdRrqJA@mail.gmail.com>
In-Reply-To: <ZJLjvhnh0lSRCVzG@nand.local>

On Wed, Jun 21, 2023 at 1:49 PM Taylor Blau <me@ttaylorr.com> wrote:

>
> On Wed, Jun 14, 2023 at 09:25:40PM +0200, Christian Couder wrote:

Show 23 quoted lines

> > +--filter-to=<dir>::
> > +     Write the pack containing filtered out objects to the
> > +     directory `<dir>`. This can be used for putting the pack on a
> > +     separate object directory that is accessed through the Git
> > +     alternates mechanism. Only useful with `--filter`.
>
> Here you say "only useful with --filter", but...
>
> > @@ -1073,8 +1077,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> >               strvec_push(&cmd.args, "--incremental");
> >       }
> >
> > -     if (po_args.filter)
> > -             prepare_pack_filtered_cmd(&pack_filtered_cmd, &po_args, packtmp);
> > +     if (po_args.filter) {
> > +             if (!filter_to)
> > +                     filter_to = packtmp;
> > +             prepare_pack_filtered_cmd(&pack_filtered_cmd, &po_args, filter_to);
> > +     }
>
> Would you want an "} else if (filter_to)" here to die and show the usage
> message, since --filter-to needs --filter? Or maybe it should imply
> --filter-to.

In the doc for --expire-to=<dir> there is "Only useful with `--cruft -d`" and I don't think there is a check to see if --cruft and -d have been passed when --expire-to is passed. So I am not sure if it's better to be consistent with --expire-to or not.