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

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 21, 2023, 12:25 UTC
Message-ID: <ZJLsMK73ae8Htec4@nand.local>
In-Reply-To: <CAP8UFD388DOTqueok7fRF+=-+s_PNria1oB18AEed3gjdRrqJA@mail.gmail.com>

On Wed, Jun 21, 2023 at 02:08:38PM +0200, Christian Couder wrote:

Show 20 quoted lines

> > > @@ -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.

TBH, I don't think that my decision at the time to silently accept --expire-to without --cruft was the right one. It should at least require --cruft, or imply it. It doesn't make a ton of sense to use without -d, but doing so is OK, so I wouldn't consider that a failing condition.

In other words, I would be fine with something like:

--- 8< ---

diff --git a/builtin/repack.c b/builtin/repack.c
index 0541c3ce15..1890f283ee 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -866,6 +866,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	    (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE)))
 		die(_("options '%s' and '%s' cannot be used together"), "--keep-unreachable", "-A");

+	/* --expire-to implies cruft */
+	if (expire_to)
+		pack_everything |= PACK_CRUFT;
+
 	if (pack_everything & PACK_CRUFT) {
 		pack_everything |= ALL_INTO_ONE;

--- >8 ---

But that sounds like a good candidate for some #leftoverbits.

In the meantime, I would be absolutely fine with deviating from the
existing behavior of --expire-to w.r.t --cruft.

Thanks,
Taylor