Warnings in gc.log can prevent gc --auto from running

Warnings in gc.log can prevent gc --auto from running

[Gregory Szorc]

I think I've found some undesirable behavior with regards to the
behavior of `git gc --auto`. The tl;dr is that a warning message written
to gc.log can result in `git gc --auto` effectively disabling itself for
gc.logExpiry. The problem is easier to trigger in 2.22 as a result of
enabling bitmap indices for bare repositories by default and the
behavior can easily result in performance degradation, especially on
servers.

`git gc --auto` will stop itself from running if a gc.log file newer
than gc.logExpiry (1 day by default) exists. The intention of this
behavior seems reasonable enough. However, it is relatively easy for a
relatively harmless gc.log file to exist and for that relatively
harmless gc.log file to effectively disable `git gc --auto`.

For example, if bitmap indices are being produced (this is the default
behavior for bare repositories in Git 2.22) and the user has taken any
action that would result in a `git gc` producing multiple packfiles
(setting gc.bigPackThreshold, setting pack.packSizeLimit, annotating a
packfile with a .keep file, etc) then a message like "warning: disabling
bitmap writing, as some objects are not being packed" or "warning:
disabling bitmap writing, packs are split due to pack.packSizeLimit" may
be written to gc.log. This warning message will result in the presence
of a gc.log file, which will cause `git gc --auto` to stop doing
meaningful work until gc.logExpiry has passed or the gc.log is cleaned
up out-of-band.

The practical impact of this behavior is that an environment having only
made minor tweaks to tweak packfile behavior may end up inadvertently
disabling `git gc --auto` and having excessive amounts of packfiles and
loose object files accumulate since `git gc --auto` isn't running. This
can result in performance degradation, especially for repositories
receiving hundreds or thousands of pushes a day - ask me how I know :)

I was able to work around this in a server environment by removing
gc.log if the contents were "harmless" warning messages, unblocking `git
gc --auto`. However, the solution is a bit brittle. As an end-user of
Git, I would prefer a `git gc --auto` execution mode that was less
sensitive to the presence of non-fatal messages in gc.log. Lowering the
value of gc.logExpiry is also a somewhat reasonable solution. I /think/
you could even make the value "now" to effectively disable the gc.log
check, but I haven't tested this. I don't feel great about that
workaround though, as if there is an actual gc/repack error, I'd like to
know about it instead of sweeping it under the rug by continuously
deleting the gc.log file. I'm also not keen on triggering `git gc --auto
--force` because --force will ignore lock files and I like respecting
lock files.

I don't prescribe to know the best way to solve this problem. I just
know it is a footgun sitting in the default Git configuration. And the
footgun became a lot easier to fire with the introduction of warning
messages related to bitmap indices and again when bitmap indices were
enabled by default for bare repositories in Git 2.22.

Gregory Szorc
gregory.szorc@gmail.com

Re: Warnings in gc.log can prevent gc --auto from running

[Jeff King]

On Thu, Jul 25, 2019 at 07:18:57PM -0700, Gregory Szorc wrote:

> I think I've found some undesirable behavior with regards to the
> behavior of `git gc --auto`. The tl;dr is that a warning message written
> to gc.log can result in `git gc --auto` effectively disabling itself for
> gc.logExpiry. The problem is easier to trigger in 2.22 as a result of
> enabling bitmap indices for bare repositories by default and the
> behavior can easily result in performance degradation, especially on
> servers.

Yuck, thanks for reporting this.

As you note, this is a special case of a much larger problem. The other
common case is the "oops, you still have a lot of loose objects after
repacking" warning. There's more discussion and some patches here:

  https://public-inbox.org/git/20180716172717.237373-1-jonathantanmy@google.com/

though I don't think any of the work that came out of that fundamentally
solves the issue.

> I don't prescribe to know the best way to solve this problem. I just
> know it is a footgun sitting in the default Git configuration. And the
> footgun became a lot easier to fire with the introduction of warning
> messages related to bitmap indices and again when bitmap indices were
> enabled by default for bare repositories in Git 2.22.

IMHO one way to mitigate this is to simply warn less. In particular, if
we are auto-enabling bitmaps, then it doesn't necessarily make sense for
us to warn about them being disabled.

In the case of .keep files, we've already got 7328482253 (repack:
disable bitmaps-by-default if .keep files exist, 2019-06-29), which
should be in the next released version of Git. But I suspect that's
racy with respect to somebody creating .keep files, and as you note
there are other config options that might prevent us from generating
bitmaps.

Instead, it may make sense to turn the --write-bitmap-index option of
pack-objects into a tri-state: true/false/auto. Then pack-objects would
know that we are in best-effort mode, and would avoid warning in that
case. That would also let git-repack express its intentions better to
git-pack-objects, so we could replace 7328482253, and keep more of the
logic in pack-objects, which is ultimately what has to make the decision
about whether it can generate bitmaps.

-Peff

Re: Warnings in gc.log can prevent gc --auto from running

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 29 2019, Jeff King wrote:

> On Thu, Jul 25, 2019 at 07:18:57PM -0700, Gregory Szorc wrote:
>
>> I think I've found some undesirable behavior with regards to the
>> behavior of `git gc --auto`. The tl;dr is that a warning message written
>> to gc.log can result in `git gc --auto` effectively disabling itself for
>> gc.logExpiry. The problem is easier to trigger in 2.22 as a result of
>> enabling bitmap indices for bare repositories by default and the
>> behavior can easily result in performance degradation, especially on
>> servers.
>
> Yuck, thanks for reporting this.
>
> As you note, this is a special case of a much larger problem. The other
> common case is the "oops, you still have a lot of loose objects after
> repacking" warning. There's more discussion and some patches here:
>
>   https://public-inbox.org/git/20180716172717.237373-1-jonathantanmy@google.com/
>
> though I don't think any of the work that came out of that fundamentally
> solves the issue.

To add to that Gregory probably finds these two old reports of mine
interesting. The former is pretty much his report (but for a different
root cause, the loose object issue):
https://public-inbox.org/git/87inc89j38.fsf@evledraar.gmail.com/ &
https://public-inbox.org/git/87fu6bmr0j.fsf@evledraar.gmail.com/

>> I don't prescribe to know the best way to solve this problem. I just
>> know it is a footgun sitting in the default Git configuration. And the
>> footgun became a lot easier to fire with the introduction of warning
>> messages related to bitmap indices and again when bitmap indices were
>> enabled by default for bare repositories in Git 2.22.
>
> IMHO one way to mitigate this is to simply warn less. In particular, if
> we are auto-enabling bitmaps, then it doesn't necessarily make sense for
> us to warn about them being disabled.
>
> In the case of .keep files, we've already got 7328482253 (repack:
> disable bitmaps-by-default if .keep files exist, 2019-06-29), which
> should be in the next released version of Git. But I suspect that's
> racy with respect to somebody creating .keep files, and as you note
> there are other config options that might prevent us from generating
> bitmaps.
>
> Instead, it may make sense to turn the --write-bitmap-index option of
> pack-objects into a tri-state: true/false/auto. Then pack-objects would
> know that we are in best-effort mode, and would avoid warning in that
> case. That would also let git-repack express its intentions better to
> git-pack-objects, so we could replace 7328482253, and keep more of the
> logic in pack-objects, which is ultimately what has to make the decision
> about whether it can generate bitmaps.

Sounds like pentastate to me :) (penta = 5, had to look it up). I.e. in
most cases of "auto" we pick a true/false at the outset, whereas this is
true/true-but-dont-care-much/false/false-but-dont-care-much with "auto"
picking the "-but-dont-care-much" versions of a "soft" true/false.

On this general topic a *soft* poke about relying to
https://public-inbox.org/git/8736lnxlig.fsf@evledraar.gmail.com/ if you
have time. I think a "loose pack" might be a way forward for the loose
object proliferation, but maybe I'm wrong.

More generally we're really straining the gc.log pass-along-a-message
facility.

Re: Warnings in gc.log can prevent gc --auto from running

[Jeff King]

On Mon, Jul 29, 2019 at 02:50:56PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > Instead, it may make sense to turn the --write-bitmap-index option of
> > pack-objects into a tri-state: true/false/auto. Then pack-objects would
> > know that we are in best-effort mode, and would avoid warning in that
> > case. That would also let git-repack express its intentions better to
> > git-pack-objects, so we could replace 7328482253, and keep more of the
> > logic in pack-objects, which is ultimately what has to make the decision
> > about whether it can generate bitmaps.
> 
> Sounds like pentastate to me :) (penta = 5, had to look it up). I.e. in
> most cases of "auto" we pick a true/false at the outset, whereas this is
> true/true-but-dont-care-much/false/false-but-dont-care-much with "auto"
> picking the "-but-dont-care-much" versions of a "soft" true/false.

I don't think we care about false-but-dont-care-much. Pack-objects just
needs to know whether the bitmaps are the user's expressed intention, or
just something that it should do if it's convenient.

I'll see if I can work up a patch to demonstrate.

> On this general topic a *soft* poke about relying to
> https://public-inbox.org/git/8736lnxlig.fsf@evledraar.gmail.com/ if you
> have time. I think a "loose pack" might be a way forward for the loose
> object proliferation, but maybe I'm wrong.

I just left a reply, though I think most of the discussion there is
about the actual pruning-corruption race. I'm totally on board with the
idea of an "unreachable pack", but AFAIK nobody has produced any
patches yet.

> More generally we're really straining the gc.log pass-along-a-message
> facility.

I definitely agree with that. :)

-Peff

[PATCH 0/3] handling warnings due to auto-enabled bitmaps

[Jeff King]

On Wed, Jul 31, 2019 at 12:28:07AM -0400, Jeff King wrote:

> On Mon, Jul 29, 2019 at 02:50:56PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> > > Instead, it may make sense to turn the --write-bitmap-index option of
> > > pack-objects into a tri-state: true/false/auto. Then pack-objects would
> > > know that we are in best-effort mode, and would avoid warning in that
> > > case. That would also let git-repack express its intentions better to
> > > git-pack-objects, so we could replace 7328482253, and keep more of the
> > > logic in pack-objects, which is ultimately what has to make the decision
> > > about whether it can generate bitmaps.
> > 
> > Sounds like pentastate to me :) (penta = 5, had to look it up). I.e. in
> > most cases of "auto" we pick a true/false at the outset, whereas this is
> > true/true-but-dont-care-much/false/false-but-dont-care-much with "auto"
> > picking the "-but-dont-care-much" versions of a "soft" true/false.
> 
> I don't think we care about false-but-dont-care-much. Pack-objects just
> needs to know whether the bitmaps are the user's expressed intention, or
> just something that it should do if it's convenient.
> 
> I'll see if I can work up a patch to demonstrate.

This actually turned out pretty well, I think. I wish I had thought of
it when were initially looking at the .keep stuff. :) It was not too
hard to clean that up in the third patch, though.

  [1/3]: t7700: clean up .keep file in bitmap-writing test
  [2/3]: repack: silence warnings when auto-enabled bitmaps cannot be built
  [3/3]: repack: simplify handling of auto-bitmaps and .keep files

 builtin/pack-objects.c | 21 ++++++++++++++++-----
 builtin/repack.c       | 24 +++++++-----------------
 t/t7700-repack.sh      | 15 ++++++++++++++-
 3 files changed, 37 insertions(+), 23 deletions(-)

[PATCH 1/3] t7700: clean up .keep file in bitmap-writing test

[Jeff King]

After our test snippet finishes, the .keep file is left in place, making
it hard to do further tests of the auto-bitmap-writing code (since it
suppresses the feature completely). Let's clean it up.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7700-repack.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 0e9af832c9..8d9a358df8 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -243,6 +243,7 @@ test_expect_success 'no bitmaps created if .keep files present' '
 	pack=$(ls bare.git/objects/pack/*.pack) &&
 	test_path_is_file "$pack" &&
 	keep=${pack%.pack}.keep &&
+	test_when_finished "rm -f \"\$keep\"" &&
 	>"$keep" &&
 	git -C bare.git repack -ad &&
 	find bare.git/objects/pack/ -type f -name "*.bitmap" >actual &&
-- 
2.23.0.rc0.426.gbdee707ba7

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

[Jeff King]

Depending on various config options, a full repack may not be able to
build a reachability bitmap index (e.g., if pack.packSizeLimit forces us
to write multiple packs). In these cases pack-objects may write a
warning to stderr.

Since 36eba0323d (repack: enable bitmaps by default on bare repos,
2019-03-14), we may generate these warnings even when the user did not
explicitly ask for bitmaps. This has two downsides:

  - it can be confusing, if they don't know what bitmaps are

  - a daemonized auto-gc will write this to its log file, and the
    presence of the warning may suppress further auto-gc (until
    gc.logExpiry has elapsed)

Let's have repack communicate to pack-objects that the choice to turn on
bitmaps was not made explicitly by the user, which in turn allows
pack-objects to suppress these warnings.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/pack-objects.c | 21 ++++++++++++++++-----
 builtin/repack.c       | 15 +++++++++------
 t/t7700-repack.sh      | 11 +++++++++++
 3 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 267c562b1f..76ce906946 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -96,7 +96,11 @@ static off_t reuse_packfile_offset;
 
 static int use_bitmap_index_default = 1;
 static int use_bitmap_index = -1;
-static int write_bitmap_index;
+static enum {
+	WRITE_BITMAP_FALSE = 0,
+	WRITE_BITMAP_QUIET,
+	WRITE_BITMAP_TRUE,
+} write_bitmap_index;
 static uint16_t write_bitmap_options = BITMAP_OPT_HASH_CACHE;
 
 static int exclude_promisor_objects;
@@ -892,7 +896,8 @@ static void write_pack_file(void)
 						 nr_written, oid.hash, offset);
 			close(fd);
 			if (write_bitmap_index) {
-				warning(_(no_split_warning));
+				if (write_bitmap_index != WRITE_BITMAP_QUIET)
+					warning(_(no_split_warning));
 				write_bitmap_index = 0;
 			}
 		}
@@ -1176,7 +1181,8 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,
 	if (!want_object_in_pack(oid, exclude, &found_pack, &found_offset)) {
 		/* The pack is missing an object, so it will not have closure */
 		if (write_bitmap_index) {
-			warning(_(no_closure_warning));
+			if (write_bitmap_index != WRITE_BITMAP_QUIET)
+				warning(_(no_closure_warning));
 			write_bitmap_index = 0;
 		}
 		return 0;
@@ -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),
 		OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
 		{ OPTION_CALLBACK, 0, "missing", NULL, N_("action"),
 		  N_("handling for missing objects"), PARSE_OPT_NONEG,
diff --git a/builtin/repack.c b/builtin/repack.c
index 30982ed2a2..db93ca3660 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -345,13 +345,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		die(_("--keep-unreachable and -A are incompatible"));
 
 	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;
 	}
 	if (pack_kept_objects < 0)
-		pack_kept_objects = write_bitmaps;
+		pack_kept_objects = !!write_bitmaps;
 
 	if (write_bitmaps && !(pack_everything & ALL_INTO_ONE))
 		die(_(incremental_bitmap_conflict_error));
@@ -375,8 +376,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	argv_array_push(&cmd.args, "--indexed-objects");
 	if (repository_format_partial_clone)
 		argv_array_push(&cmd.args, "--exclude-promisor-objects");
-	if (write_bitmaps)
+	if (write_bitmaps > 0)
 		argv_array_push(&cmd.args, "--write-bitmap-index");
+	else if (write_bitmaps < 0)
+		argv_array_push(&cmd.args, "--write-bitmap-index-quiet");
 	if (use_delta_islands)
 		argv_array_push(&cmd.args, "--delta-islands");
 
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 8d9a358df8..54f815b8b9 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -250,4 +250,15 @@ test_expect_success 'no bitmaps created if .keep files present' '
 	test_must_be_empty actual
 '
 
+test_expect_success 'auto-bitmaps do not complain if unavailable' '
+	test_config -C bare.git pack.packSizeLimit 1M &&
+	blob=$(test-tool genrandom big $((1024*1024)) |
+	       git -C bare.git hash-object -w --stdin) &&
+	git -C bare.git update-ref refs/tags/big $blob &&
+	git -C bare.git repack -ad 2>stderr &&
+	test_must_be_empty stderr &&
+	find bare.git/objects/pack -type f -name "*.bitmap" >actual &&
+	test_must_be_empty actual
+'
+
 test_done
-- 
2.23.0.rc0.426.gbdee707ba7

[PATCH 3/3] repack: simplify handling of auto-bitmaps and .keep files

[Jeff King]

Commit 7328482253 (repack: disable bitmaps-by-default if .keep files
exist, 2019-06-29) taught repack to prefer disabling bitmaps to
duplicating objects (unless bitmaps were asked for explicitly).

But there's an easier way to do this: if we keep passing the
--honor-pack-keep flag to pack-objects when auto-enabling bitmaps, then
pack-objects already makes the same decision (it will disable bitmaps
rather than duplicate). Better still, pack-objects can actually decide
to do so based not just on the presence of a .keep file, but on whether
that .keep file actually impacts the new pack we're making (so if we're
racing with a push or fetch, for example, their temporary .keep file
will not block us from generating bitmaps if they haven't yet updated
their refs).

And because repack uses the --write-bitmap-index-quiet flag, we don't
have to worry about pack-objects generating confusing warnings when it
does see a .keep file. We can confirm this by tweaking the .keep test to
check repack's stderr.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/repack.c  | 17 ++---------------
 t/t7700-repack.sh |  3 ++-
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index db93ca3660..632c0c0a79 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -89,17 +89,6 @@ static void remove_pack_on_signal(int signo)
 	raise(signo);
 }
 
-static int has_pack_keep_file(void)
-{
-	struct packed_git *p;
-
-	for (p = get_all_packs(the_repository); p; p = p->next) {
-		if (p->pack_keep)
-			return 1;
-	}
-	return 0;
-}
-
 /*
  * Adds all packs hex strings to the fname list, which do not
  * have a corresponding .keep file. These packs are not to
@@ -346,13 +335,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	if (write_bitmaps < 0) {
 		if (!(pack_everything & ALL_INTO_ONE) ||
-		    !is_bare_repository() ||
-		    keep_pack_list.nr != 0 ||
-		    has_pack_keep_file())
+		    !is_bare_repository())
 			write_bitmaps = 0;
 	}
 	if (pack_kept_objects < 0)
-		pack_kept_objects = !!write_bitmaps;
+		pack_kept_objects = write_bitmaps > 0;
 
 	if (write_bitmaps && !(pack_everything & ALL_INTO_ONE))
 		die(_(incremental_bitmap_conflict_error));
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 54f815b8b9..4e855bc21b 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -245,7 +245,8 @@ test_expect_success 'no bitmaps created if .keep files present' '
 	keep=${pack%.pack}.keep &&
 	test_when_finished "rm -f \"\$keep\"" &&
 	>"$keep" &&
-	git -C bare.git repack -ad &&
+	git -C bare.git repack -ad 2>stderr &&
+	test_must_be_empty stderr &&
 	find bare.git/objects/pack/ -type f -name "*.bitmap" >actual &&
 	test_must_be_empty actual
 '
-- 
2.23.0.rc0.426.gbdee707ba7

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

[Junio C Hamano]

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.

> diff --git a/builtin/repack.c b/builtin/repack.c
> index 30982ed2a2..db93ca3660 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -345,13 +345,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  		die(_("--keep-unreachable and -A are incompatible"));
>  
>  	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.

>  	}
>  	if (pack_kept_objects < 0)
> -		pack_kept_objects = write_bitmaps;
> +		pack_kept_objects = !!write_bitmaps;

And non-zero write_bitmaps replaces "true" in the older world.

>  	if (write_bitmaps && !(pack_everything & ALL_INTO_ONE))

So this does not have to change.

>  		die(_(incremental_bitmap_conflict_error));
> @@ -375,8 +376,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	argv_array_push(&cmd.args, "--indexed-objects");
>  	if (repository_format_partial_clone)
>  		argv_array_push(&cmd.args, "--exclude-promisor-objects");
> -	if (write_bitmaps)
> +	if (write_bitmaps > 0)
>  		argv_array_push(&cmd.args, "--write-bitmap-index");
> +	else if (write_bitmaps < 0)
> +		argv_array_push(&cmd.args, "--write-bitmap-index-quiet");

And "enabled by user" and "enabled by default" are mapped to the two
options.

All makes sense.

Thanks.

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

[Jeff King]

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

Re: [PATCH 3/3] repack: simplify handling of auto-bitmaps and .keep files

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Commit 7328482253 (repack: disable bitmaps-by-default if .keep files
> exist, 2019-06-29) taught repack to prefer disabling bitmaps to
> duplicating objects (unless bitmaps were asked for explicitly).
>
> But there's an easier way to do this: if we keep passing the
> --honor-pack-keep flag to pack-objects when auto-enabling bitmaps, then
> pack-objects already makes the same decision (it will disable bitmaps
> rather than duplicate). Better still, pack-objects can actually decide
> to do so based not just on the presence of a .keep file, but on whether
> that .keep file actually impacts the new pack we're making (so if we're
> racing with a push or fetch, for example, their temporary .keep file
> will not block us from generating bitmaps if they haven't yet updated
> their refs).
>
> And because repack uses the --write-bitmap-index-quiet flag, we don't
> have to worry about pack-objects generating confusing warnings when it
> does see a .keep file. We can confirm this by tweaking the .keep test to
> check repack's stderr.

This change is a bit too dense so I'll need to think about it a bit
longer, but in the meantime it is queued alongside the other two.

Thanks.

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/repack.c  | 17 ++---------------
>  t/t7700-repack.sh |  3 ++-
>  2 files changed, 4 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index db93ca3660..632c0c0a79 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -89,17 +89,6 @@ static void remove_pack_on_signal(int signo)
>  	raise(signo);
>  }
>  
> -static int has_pack_keep_file(void)
> -{
> -	struct packed_git *p;
> -
> -	for (p = get_all_packs(the_repository); p; p = p->next) {
> -		if (p->pack_keep)
> -			return 1;
> -	}
> -	return 0;
> -}
> -
>  /*
>   * Adds all packs hex strings to the fname list, which do not
>   * have a corresponding .keep file. These packs are not to
> @@ -346,13 +335,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  
>  	if (write_bitmaps < 0) {
>  		if (!(pack_everything & ALL_INTO_ONE) ||
> -		    !is_bare_repository() ||
> -		    keep_pack_list.nr != 0 ||
> -		    has_pack_keep_file())
> +		    !is_bare_repository())
>  			write_bitmaps = 0;
>  	}
>  	if (pack_kept_objects < 0)
> -		pack_kept_objects = !!write_bitmaps;
> +		pack_kept_objects = write_bitmaps > 0;
>  
>  	if (write_bitmaps && !(pack_everything & ALL_INTO_ONE))
>  		die(_(incremental_bitmap_conflict_error));
> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index 54f815b8b9..4e855bc21b 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -245,7 +245,8 @@ test_expect_success 'no bitmaps created if .keep files present' '
>  	keep=${pack%.pack}.keep &&
>  	test_when_finished "rm -f \"\$keep\"" &&
>  	>"$keep" &&
> -	git -C bare.git repack -ad &&
> +	git -C bare.git repack -ad 2>stderr &&
> +	test_must_be_empty stderr &&
>  	find bare.git/objects/pack/ -type f -name "*.bitmap" >actual &&
>  	test_must_be_empty actual
>  '