On Sun, Dec 05, 2021 at 12:46:19PM -0800, Junio C Hamano wrote:
> 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...
> 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
