Subject: Re: [PATCH 12/17] builtin/repack.c: support generating a cruft pack

From: Junio C Hamano <[email-masked]>
Date: Sun, 05 Dec 2021 12:46:19 -0800

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"));
	}

The conditionals are a bit unpleasant to read and maintain, but I
guess we cannot help it?

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.

Thanks.