Re: "disabling bitmap writing, as some objects are not being packed"?

David Turner [off-list ref] writes:

I'm a bit confused by the message "disabling bitmap writing, as some
objects are not being packed".  I see it the my gc.log file on my git
server.

1. Its presence in the gc.log file prevents future automatic garbage
collection.  This seems bad.  I understand the desire to avoid making
things worse if a past gc has run into issues.  But this warning is
non-fatal; the only consequence is that many operations get slower.  But
a lack of gc when there are too many packs causes that consequence too
(often a much worse slowdown than would be caused by the missing
bitmap).

So I wonder if it would be better for auto gc to grep gc.log for fatal
errors (as opposed to warnings) and only skip running if any are found.
Alternately, we could simply put warnings into gc.log.warning and
reserve gc.log for fatal errors. I'm not sure which would be simpler.  

I am not sure if string matching is really a good idea, as I'd
assume that these messages are eligible for i18n.

329e6e8794 ("gc: save log from daemonized gc --auto and print it
next time", 2015-09-19) wanted to notice that auto-gc is not
making progress and used the presense of error messages as a cue.
In your case, I think the auto-gc _is_ making progress, reducing
number of loose objects in the repository or consolidating many
packfiles into one, and the message is only about the fact that
packing is punting and not producing a bitmap as you asked, which
is different from not making any progress.  I do not think log vs
warn is a good criteria to tell them apart, either.

In any case, as the error message asks the user to do, the user
eventually wants to correct the root cause before removing the
gc.log; I am not sure report_last_gc_error() is the place to correct
this in the first place.

2. I don't understand what would cause that message.  That is, what bad
thing am I doing that I should stop doing?  I've briefly skimmed the
code and commit message, but the answer isn't leaping out at me.

Enabling bitmap generation for incremental packing that does not
cram everything into a single pack is triggering it, I would
presume.  Perhaps we should ignore -b option in most of the cases
and enable it only for "repack -a -d -f" codepath?  Or detect that
we are being run from "gc --auto" and automatically disable -b?  I
have a feeling that an approach along that line is closer to the
real solution than tweaking report_last_gc_error() and trying to
deduce if we are making any progress.

On Fri, Dec 16, 2016 at 01:28:00PM -0800, Junio C Hamano wrote:

quoted

2. I don't understand what would cause that message.  That is, what bad
thing am I doing that I should stop doing?  I've briefly skimmed the
code and commit message, but the answer isn't leaping out at me.

Enabling bitmap generation for incremental packing that does not
cram everything into a single pack is triggering it, I would
presume.  Perhaps we should ignore -b option in most of the cases
and enable it only for "repack -a -d -f" codepath?  Or detect that
we are being run from "gc --auto" and automatically disable -b?  I
have a feeling that an approach along that line is closer to the
real solution than tweaking report_last_gc_error() and trying to
deduce if we are making any progress.

Ah, indeed. I was thinking in my other response that "git gc" would
always kick off an all-into-one repack. But "gc --auto" will not in
certain cases. And yes, in those cases you definitely would want
--no-write-bitmap-index. I think it would be reasonable for "git repack"
to disable bitmap-writing automatically when not doing an all-into-one
repack.

-Peff

On Fri, 2016-12-16 at 16:32 -0500, Jeff King wrote:

On Fri, Dec 16, 2016 at 01:28:00PM -0800, Junio C Hamano wrote:

quoted

quoted

2. I don't understand what would cause that message.  That is, what bad
thing am I doing that I should stop doing?  I've briefly skimmed the
code and commit message, but the answer isn't leaping out at me.

Enabling bitmap generation for incremental packing that does not
cram everything into a single pack is triggering it, I would
presume.  Perhaps we should ignore -b option in most of the cases
and enable it only for "repack -a -d -f" codepath?  Or detect that
we are being run from "gc --auto" and automatically disable -b?  I
have a feeling that an approach along that line is closer to the
real solution than tweaking report_last_gc_error() and trying to
deduce if we are making any progress.

Ah, indeed. I was thinking in my other response that "git gc" would
always kick off an all-into-one repack. But "gc --auto" will not in
certain cases. And yes, in those cases you definitely would want
--no-write-bitmap-index. I think it would be reasonable for "git repack"
to disable bitmap-writing automatically when not doing an all-into-one
repack.

I do not have alternates and am not using --local.  Nor do I have .keep
packs.

I would assume, based on the documentation, that auto gc would be doing
an all-into-one repack:
"If the number of packs exceeds the value of gc.autopacklimit, then
 existing packs (except those marked with a .keep file) are
 consolidated into a single pack by using the -A option of git
 repack."

I don't have any settings that limit the size of packs, either.  And a
manual git repack -a -d creates only a single pack.  Its loneliness
doesn't last long, because pretty soon a new pack is created by an
incoming push.

Unless this just means that some objects are being kept loose (perhaps
because they are unreferenced)? 

On Fri, Dec 16, 2016 at 04:40:16PM -0500, David Turner wrote:

I would assume, based on the documentation, that auto gc would be doing
an all-into-one repack:
"If the number of packs exceeds the value of gc.autopacklimit, then
 existing packs (except those marked with a .keep file) are
 consolidated into a single pack by using the -A option of git
 repack."

I don't have any settings that limit the size of packs, either.  And a
manual git repack -a -d creates only a single pack.  Its loneliness
doesn't last long, because pretty soon a new pack is created by an
incoming push.

The interesting code is in need_to_gc():

        /*
         * If there are too many loose objects, but not too many
         * packs, we run "repack -d -l".  If there are too many packs,
         * we run "repack -A -d -l".  Otherwise we tell the caller
         * there is no need.
         */
        if (too_many_packs())
                add_repack_all_option();
        else if (!too_many_loose_objects())
                return 0;

So if you have (say) 10 packs and 10,000 objects, we'll incrementally
pack those objects into a single new pack.

I never noticed this myself because we do not use auto-gc at GitHub at
all. We only ever do a big all-into-one repack.

Unless this just means that some objects are being kept loose (perhaps
because they are unreferenced)? 

If they're unreferenced, they won't be part of the new pack. You might
accumulate loose objects that are ejected from previous packs, which
could trigger auto-gc to do an incremental pack (even though it wouldn't
be productive, because they're unreferenced!). You may also get them
from pushes (small pushes will be exploded into loose objects by
default).

-Peff

When running git pack-objects --incremental, we do not expect to be
able to write a bitmap; it is very likely that objects in the new pack
will have references to objects outside of the pack.  So we don't need
to warn the user about it.

This warning was making its way into gc.log because auto-gc will do an
incremental repack when there are too many loose objects but not too
many packs.  When the gc.log was present, future auto gc runs would
refuse to run.

Signed-off-by: David Turner <redacted>
---
 builtin/pack-objects.c  |  3 ++-
 t/t5310-pack-bitmaps.sh | 12 ++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 0fd52bd..96de213 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c

@@ -1083,7 +1083,8 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,
 	if (!want_object_in_pack(sha1, 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 (!incremental)
+				warning(_(no_closure_warning));
 			write_bitmap_index = 0;
 		}
 		return 0;

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index b4c7a6f..d81636e 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh

@@ -247,6 +247,18 @@ test_expect_success 'pack-objects respects --incremental' '
 	test_cmp 4.objects objects
 '
 
+test_expect_success 'incremental repack does not create bitmaps' '
+	test_commit 11 &&
+	ls .git/objects/pack/ | grep bitmap >existing_bitmaps &&
+	ls .git/objects/pack/ | grep -v bitmap >existing_packs &&
+	git repack -d 2>err &&
+	test_line_count = 0 err &&
+	ls .git/objects/pack/ | grep bitmap >output &&
+	ls .git/objects/pack/ | grep -v bitmap >post_packs &&
+	test_cmp existing_bitmaps output &&
+	! test_cmp existing_packs post_packs
+'
+
 test_expect_success 'pack with missing blob' '
 	rm $(objpath $blob) &&
 	git pack-objects --stdout --revs <revs >/dev/null

-- 
2.8.0.rc4.22.g8ae061a

On Fri, Dec 16, 2016 at 06:59:35PM -0500, David Turner wrote:

I agree that the user doesn't need to be warned about it when running
"gc --auto", but I wonder if somebody invoking "pack-objects
--incremental --write-bitmap-index" ought to be.

In other words, your patch is detecting at a low level that we've been
given a nonsense combination of options, but should we perhaps stop
passing nonsense in the first place?

Either at the repack level, with something like:

diff --git a/builtin/repack.c b/builtin/repack.c
index 80dd06b4a2..6608a902b1 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c

@@ -231,8 +231,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		argv_array_pushf(&cmd.args, "--no-reuse-delta");
 	if (no_reuse_object)
 		argv_array_pushf(&cmd.args, "--no-reuse-object");
-	if (write_bitmaps)
-		argv_array_push(&cmd.args, "--write-bitmap-index");
 
 	if (pack_everything & ALL_INTO_ONE) {
 		get_non_kept_pack_filenames(&existing_packs);

@@ -256,8 +254,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	} else {
 		argv_array_push(&cmd.args, "--unpacked");
 		argv_array_push(&cmd.args, "--incremental");
+		write_bitmap_index = 0;
 	}
 
+	if (write_bitmaps)
+		argv_array_push(&cmd.args, "--write-bitmap-index");
 	if (local)
 		argv_array_push(&cmd.args,  "--local");
 	if (quiet)

Though that still means we do not warn on:

  git repack --write-bitmap-index

which is nonsense (it is asking for an incremental repack with bitmaps).

So maybe do it at the gc level, like:

diff --git a/builtin/gc.c b/builtin/gc.c
index 069950d0b4..d3c978c765 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c

@@ -191,6 +191,11 @@ static void add_repack_all_option(void)
 	}
 }
 
+static void add_repack_incremental_option(void)
+{
+	argv_array_push(&repack, "--no-write-bitmap-index");
+}
+
 static int need_to_gc(void)
 {
 	/*

@@ -208,7 +213,9 @@ static int need_to_gc(void)
 	 */
 	if (too_many_packs())
 		add_repack_all_option();
-	else if (!too_many_loose_objects())
+	else if (too_many_loose_objects())
+		add_repack_incremental_option();
+	else
 		return 0;
 
 	if (run_hook_le(NULL, "pre-auto-gc", NULL))

-Peff

On Sat, Dec 17, 2016 at 4:28 AM, Junio C Hamano [off-list ref] wrote:

David Turner [off-list ref] writes:

quoted

I'm a bit confused by the message "disabling bitmap writing, as some
objects are not being packed".  I see it the my gc.log file on my git
server.

quoted

1. Its presence in the gc.log file prevents future automatic garbage
collection.  This seems bad.  I understand the desire to avoid making
things worse if a past gc has run into issues.  But this warning is
non-fatal; the only consequence is that many operations get slower.  But
a lack of gc when there are too many packs causes that consequence too
(often a much worse slowdown than would be caused by the missing
bitmap).

So I wonder if it would be better for auto gc to grep gc.log for fatal
errors (as opposed to warnings) and only skip running if any are found.
Alternately, we could simply put warnings into gc.log.warning and
reserve gc.log for fatal errors. I'm not sure which would be simpler.

I am not sure if string matching is really a good idea, as I'd
assume that these messages are eligible for i18n.

And we can't grep for fatal errors anyway. The problem that led to
329e6e8794 was this line

    warning: There are too many unreachable loose objects; run 'git
prune' to remove them.

which is not fatal.

329e6e8794 ("gc: save log from daemonized gc --auto and print it
next time", 2015-09-19) wanted to notice that auto-gc is not
making progress and used the presense of error messages as a cue.
In your case, I think the auto-gc _is_ making progress, reducing
number of loose objects in the repository or consolidating many
packfiles into one

Yeah the key point is making progress, and to reliably detect that we
need some way for all the commands that git-gc executes to tell it
about that, git-repack in this particular case but...

and the message is only about the fact that
packing is punting and not producing a bitmap as you asked, which
is different from not making any progress.  I do not think log vs
warn is a good criteria to tell them apart, either.

In any case, as the error message asks the user to do, the user
eventually wants to correct the root cause before removing the
gc.log; I am not sure report_last_gc_error() is the place to correct
this in the first place.

quoted

2. I don't understand what would cause that message.  That is, what bad
thing am I doing that I should stop doing?  I've briefly skimmed the
code and commit message, but the answer isn't leaping out at me.

Enabling bitmap generation for incremental packing that does not
cram everything into a single pack is triggering it, I would
presume.  Perhaps we should ignore -b option in most of the cases
and enable it only for "repack -a -d -f" codepath?  Or detect that
we are being run from "gc --auto" and automatically disable -b?

... since we have to change down in git-repack for that, perhaps doing
this is better. We can pass --auto (or something) to repack to tell it
about this special caller, so it only prints something to stderr in
serious cases.

Or we detect cases where background gc'ing won't work well and always
do it in foreground (e.g. when bitmap generation is enabled).

I have a feeling that an approach along that line is closer to the
real solution than tweaking report_last_gc_error() and trying to
deduce if we are making any progress.

-- 
Duy

Sure, that's fine.

On Sat, 2016-12-17 at 14:50 +0700, Duy Nguyen wrote:

And we can't grep for fatal errors anyway. The problem that led to
329e6e8794 was this line

    warning: There are too many unreachable loose objects; run 'git
prune' to remove them.

which is not fatal.

So, speaking of that message, I noticed that our git servers were
getting slow again and found that message in gc.log.

I propose to make auto gc not write that message either. Any objections?

On Wed, Feb 8, 2017 at 8:03 AM, David Turner [off-list ref] wrote:

On Sat, 2016-12-17 at 14:50 +0700, Duy Nguyen wrote:

quoted

And we can't grep for fatal errors anyway. The problem that led to
329e6e8794 was this line

    warning: There are too many unreachable loose objects; run 'git
prune' to remove them.

which is not fatal.

So, speaking of that message, I noticed that our git servers were
getting slow again and found that message in gc.log.

I propose to make auto gc not write that message either. Any objections?

Does that really help? auto gc would run more often, but unreachable
loose objects are still present and potentially make your servers
slow? Should these servers run periodic and explicit gc/prune?
-- 
Duy

On Wed, 2017-02-08 at 13:45 +0700, Duy Nguyen wrote:

On Wed, Feb 8, 2017 at 8:03 AM, David Turner [off-list ref] wrote:

quoted

On Sat, 2016-12-17 at 14:50 +0700, Duy Nguyen wrote:

quoted

And we can't grep for fatal errors anyway. The problem that led to
329e6e8794 was this line

    warning: There are too many unreachable loose objects; run 'git
prune' to remove them.

which is not fatal.

So, speaking of that message, I noticed that our git servers were
getting slow again and found that message in gc.log.

I propose to make auto gc not write that message either. Any objections?

Does that really help? auto gc would run more often, but unreachable
loose objects are still present and potentially make your servers
slow? Should these servers run periodic and explicit gc/prune?

At least pack files wouldn't accumulate.  This is the major cause of
slowdown, since each pack file must be checked for each object.

(And, also, maybe those unreachable loose objects are too new to get
gc'd, but if we retry next week, we'll gc them).

On Wed, Feb 8, 2017 at 3:24 PM, David Turner [off-list ref] wrote:

On Wed, 2017-02-08 at 13:45 +0700, Duy Nguyen wrote:

quoted

On Wed, Feb 8, 2017 at 8:03 AM, David Turner [off-list ref] wrote:

quoted

On Sat, 2016-12-17 at 14:50 +0700, Duy Nguyen wrote:

quoted

And we can't grep for fatal errors anyway. The problem that led to
329e6e8794 was this line

    warning: There are too many unreachable loose objects; run 'git
prune' to remove them.

which is not fatal.

So, speaking of that message, I noticed that our git servers were
getting slow again and found that message in gc.log.

I propose to make auto gc not write that message either. Any objections?

Does that really help? auto gc would run more often, but unreachable
loose objects are still present and potentially make your servers
slow? Should these servers run periodic and explicit gc/prune?

At least pack files wouldn't accumulate.  This is the major cause of
slowdown, since each pack file must be checked for each object.

(And, also, maybe those unreachable loose objects are too new to get
gc'd, but if we retry next week, we'll gc them).

I was about to suggest a config option that lets you run auto gc
unconditionally, which, I think, is better than suppressing the
message. Then I found gc.autoDetach. If you set it to false globally,
I think you'll get the behavior you want.

On second thought, perhaps gc.autoDetach should default to false if
there's no tty, since its main point it to stop breaking interactive
usage. That would make the server side happy (no tty there).
-- 
Duy