On Wed, Jul 31, 2019 at 01:26:12PM -0700, Junio C Hamano wrote:
> Jeff King <[email protected]> writes:
>
> > @@ -3313,8 +3319,13 @@ int cmd_pack_objects(int argc, const char **argv,
> > const char *prefix)
> > N_("do not hide commits by grafts"), 0),
> > OPT_BOOL(0, "use-bitmap-index", &use_bitmap_index,
> > N_("use a bitmap index if available to speed up
> > counting objects")),
> > - OPT_BOOL(0, "write-bitmap-index", &write_bitmap_index,
> > - N_("write a bitmap index together with the pack
> > index")),
> > + OPT_SET_INT(0, "write-bitmap-index", &write_bitmap_index,
> > + N_("write a bitmap index together with the pack
> > index"),
> > + WRITE_BITMAP_TRUE),
> > + OPT_SET_INT_F(0, "write-bitmap-index-quiet",
> > + &write_bitmap_index,
> > + N_("write a bitmap index if possible"),
> > + WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),
>
> The receiving end of this communication is pretty easy to follow.
> I'd have named an option to trigger "if possible" behaviour after
> that "if possible" phrase and not "quiet", but this is entirely
> internal that it does not matter.
Heh, that was actually the part of this series that I struggled the most
with. I didn't like "if possible" because that is already how we behave
(we continue without bitmaps if we can't make them, even if the user
asked for them explicitly). I had "if convenient" at one point, but it
seemed too vague and too long. ;)
So I'm happy to change it, but it would require somebody coming up with
a better name.
> > if (write_bitmaps < 0) {
> > - write_bitmaps = (pack_everything & ALL_INTO_ONE) &&
> > - is_bare_repository() &&
> > - keep_pack_list.nr == 0 &&
> > - !has_pack_keep_file();
> > + if (!(pack_everything & ALL_INTO_ONE) ||
> > + !is_bare_repository() ||
> > + keep_pack_list.nr != 0 ||
> > + has_pack_keep_file())
> > + write_bitmaps = 0;
>
> This side of communication is a bit harder to follow, but not
> impossible ;-) We leave it "negative" to signal "the user did not
> specify, but we enabled it by default" here.
Yeah, I also noticed that was a bit subtle. Maybe a comment:
/* leave as -1 to indicate "auto bitmaps" */
or something would help. I also thought about writing it as:
if (write_bitmaps < 0) {
if ((pack_everything & ALL_INTO_ONE) &&
is_bare_repository() &&
keep_pack_list.nr == 0 &&
!hash_pack_keep_file()) {
write_bitmaps = -1; /* indicates auto-enabled */
} else {
write_bitmaps = 0;
}
}
Better?
-Peff
