On Wed, Jun 21, 2023 at 02:08:38PM +0200, Christian Couder wrote:
> > > @@ -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
