On Wed, Jul 31, 2019 at 01:26:12PM -0700, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

@@ -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.

quoted

 	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