Re: [PATCH 2/3] repack: silence warnings when auto-enabled bitmaps cannot be built

[Jeff King <peff@peff.net>]

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

> Jeff King <peff@peff.net> 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