Re: [PATCH 12/17] builtin/repack.c: support generating a cruft pack
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Mar 1, 2022, 02:00 UTC
- Message-ID
- <Yh1+WQcs71rQYPEg@nand.local>
- In-Reply-To
- <xmqqk0gikcf8.fsf@gitster.g>
On Sun, Dec 05, 2021 at 12:46:19PM -0800, Junio C Hamano wrote:
Show 25 quoted lines
> Various thoughts on just this part, as the hunk got my attention
> while merging with other topics in 'seen'.
>
> > + if (pack_everything & PACK_CRUFT && delete_redundant) {
> > + if (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE))
> > + die(_("--cruft and -A are incompatible"));
> > + if (keep_unreachable)
> > + die(_("--cruft and -k are incompatible"));
> > + if (!(pack_everything & ALL_INTO_ONE))
> > + die(_("--cruft must be combined with all-into-one"));
> > + }
>
> The "reuse similar messages for i18n" topic will encourage us to
> turn this part into:
>
> if (pack_everything & PACK_CRUFT && delete_redundant) {
> if (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE))
> die(_("%s and %s are mutually exclusive"),
> "--cruft", "-A");
> if (keep_unreachable)
> die(_("%s and %s are mutually exclusive"),
> "--cruft", "-k");
> if (!(pack_everything & ALL_INTO_ONE))
> die(_("--cruft must be combined with all-into-one"));
> }Thanks, done.
> The conditionals are a bit unpleasant to read and maintain, but I > guess we cannot help it?
I don't know that I find them unpleasant to read, but perhaps they are a hassle to maintain (as we add new, mutually-exclusive options). But I can't seem to think of a better alternative...
Show 13 quoted lines
> Saying ALL_INTO_ONE is a bit unfriendly to the end user, who would > probably not know that it is the name the code gave to the bit that > is turned on when given an option externally known under a different > name (is that "-a"?). > > If "--cruft" must be used with "all into one", I wonder if it makes > sense to make it imply that? Not in the sense that OPT_BIT() > initially flips the ALL_INTO_ONE bit on upon seeing "--cruft", but > after parse_options() returns, we check PACK_CRUFT and if it is on > turn ALL_INTO_ONE also on (so even if '-a' gains '--all-into-one' > option, the user won't break us by giving "--no-all-into-one" after > they gave us "--cruft")? I didn't think about this part thoroughly > enough, though.
Yes, `--cruft` must be used with an option that sets ALL_INTO_ONE. Since we don't have any automatic '--no-' versions of single character options, I think that this conditional is currently redundant, but I agree that this code would break if we (a) removed the conditional you're talking about and (b) allowed passing something like `--no-all-into-one` which unsets the ALL_INTO_ONE bit.
So setting ALL_INTO_ONE ourselves _after_ option parsing is done makes sense to me, thanks.
Thanks, Taylor