On Thu, May 19, 2022 at 08:16:49AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> > Taylor Blau <me@xxxxxxxxxxxx> writes:
> >
> >> @@ -3870,6 +4034,20 @@ static int option_parse_unpack_unreachable(const struct option *opt,
> >> return 0;
> >> }
> >>
> >> +static int option_parse_cruft_expiration(const struct option *opt,
> >> + const char *arg, int unset)
> >> +{
> >> + if (unset) {
> >> + cruft = 0;
> >> + cruft_expiration = 0;
> >> + } else {
> >> + cruft = 1;
> >> + if (arg)
> >> + cruft_expiration = approxidate(arg);
> >> + }
> >> + return 0;
> >> +}
> >
> > It is somewhat sad that we have to invent this function, instead of
> > using parse_opt_expiry_date_cb().
>
> I failed to mention that this one does more than the bog-standard
> callback so the latter cannot be reused as-is, and that is what I
> meant by "somewhat sad". If we can find a way to reuse the
> parse_opt_expiry_date_cb() for the purpose of the user of this
> function that would be ideal, but only if we can do so without
> making the caller too unnatural. Having two separate values, "did
> we get --cruft-expiration option?" and "what's the value of it?",
> does benefit the current caller and we do not want to twist it just
> for not adding a similar callback---that's a tail wagging a dog.
I agree, though I'm not sure such a cleanup is possible: if the caller
specified `--cruft-expiration=never`, how would we distinguish that from
"the caller did not want to generate cruft packs"?
In that case, approxidate() would set our `cruft_expiration` variable to
`0` there, which makes "generate a cruft pack without expiring any
objects" indistinguishable from "do not generate a cruft pack" without
the additional bit of information stored in the "cruft" variable.
For now we're just recycling the pattern from the callback immediately
above this one: option_parse_unpack_unreachable().
Thanks,
Taylor
