WIth git-next, writing bitmaps fails when keep files are present

Running git-next, writing bitmap indexes fails if a keep file is present 
from an earlier pack.

With git at b139ac2, the following commands demonstrate the problem:

git init test
cd test
touch a
git add a
git commit -m "a"

git repack -ad  # generate a pack file
for f in .git/objects/pack/*.pack; touch ${f/%pack/keep}  # mark it as 
to keep

touch b
git add b
git commit -m "b"
git repack -adb

This fails at the bitmap writing stage with something like:

Counting objects: 2, done.
Delta compression using up to 24 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (2/2), done.
fatal: Failed to write bitmap index. Packfile doesn't have full closure 
(object 7388a015938147155b600eaacc59af6e78c75e5a is missing)

In our case we have .keep files lying around from ages ago (possibly due 
to kill -9s run on the server). It also means that running repack -a 
with bitmap writing enabled on a repo becomes problematic if a fetch is 
run concurrently.

Even if we practice good .keep hygiene, this seems like a bug in git 
that should be fixed.

On 01/22/2014 06:38 PM, Siddharth Agarwal wrote:

In our case we have .keep files lying around from ages ago (possibly 
due to kill -9s run on the server). It also means that running repack 
-a with bitmap writing enabled on a repo becomes problematic if a 
fetch is run concurrently.

We briefly discussed locking our repos while the repack was run, but the 
repo that would benefit the most from repacks cannot be locked to pushes 
for even a tenth of the time that repack takes on it.

On Wed, Jan 22, 2014 at 06:38:57PM -0800, Siddharth Agarwal wrote:

Running git-next, writing bitmap indexes fails if a keep file is
present from an earlier pack.

Right, that's expected.

The bitmap format cannot represent objects that are not present in the
pack. So we cannot write a bitmap index if any object reachable from a
packed commit is omitted from the pack.

We could be nicer and downgrade it to a warning, though. The patch below
does that.

In our case we have .keep files lying around from ages ago (possibly
due to kill -9s run on the server).

We ran into that problem at GitHub, too. We just turn off
`--honor-pack-keep` during our repacks, as we never want them on anyway
(and we would prefer to ignore the .keep than to abort the bitmap).

It also means that running repack -a with bitmap writing enabled on a
repo becomes problematic if a fetch is run concurrently.

For the most part, no. The .keep file should generally only be set
during the period between indexing the pack and updating the refs (so
while checking connectivity and running hooks). But pack-objects starts
from the ref tips and walks backwards. Until they are updated, it will
not try to pack the objects in the .keep files, as nobody references
them. There are two loopholes, though:

  1. In some instances, a remote may send an object we already have
     (e.g., because it is a blob referenced in an old commit, but newly
     referenced again due to a revert; we do not do a full object
     difference during the protocol negotiation, for reasons of
     efficiency). If that is the case, we may omit it if pack-objects
     starts during the period that the .pack and .keep files exist.

  2. Once the fetch updates the refs, it removes the .keep file. But
     this isn't atomic. A repack which starts between the two may pick
     up the new ref values, but also see the .keep file.

These are both unlikely, but possible on a very busy repository. The
patch below will downgrade each to a warning, rather than aborting the
repack.

So this should just work out of the box with this patch.  But if bitmaps
are important to you (say, you are running a very busy site and want
to make sure you always have bitmaps turned on) and you do not otherwise
care about .keep files, you may want to disable them, too.

-Peff

-- >8 --
Subject: pack-objects: turn off bitmaps when skipping objects

The pack bitmap format requires that we have a single bit
for each object in the pack, and that each object's bitmap
represents its complete set of reachable objects. Therefore
we have no way to represent the bitmap of an object which
references objects outside the pack.

We notice this problem while generating the bitmaps, as we
try to find the offset of a particular object and realize
that we do not have it. In this case we die, and neither the
bitmap nor the pack is generated. This is correct, but
perhaps a little unfriendly. If you have bitmaps turned on
in the config, many repacks will fail which would otherwise
succeed. E.g., incremental repacks, repacks with "-l" when
you have alternates, ".keep" files.

Instead, this patch notices early that we are omitting some
objects from the pack and turns off bitmaps (with a
warning). Note that this is not strictly correct, as it's
possible that the object being omitted is not reachable from
any other object in the pack. In practice, this is almost
never the case, and there are two advantages to doing it
this way:

  1. The code is much simpler, as we do not have to cleanly
     abort the bitmap-generation process midway through.

  2. We do not waste time partially generating bitmaps only
     to find out that some object deep in the history is not
     being packed.

Signed-off-by: Jeff King <redacted>
---
I tried to keep the warning to an 80-character line without making it
too confusing. Suggestions welcome if it doesn't make sense to people.

 builtin/pack-objects.c  | 12 +++++++++++-
 t/t5310-pack-bitmaps.sh |  5 ++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 8364fbd..76831d9 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c

@@ -1000,6 +1000,10 @@ static void create_object_entry(const unsigned char *sha1,
 	entry->no_try_delta = no_try_delta;
 }
 
+static const char no_closure_warning[] = N_(
+"disabling bitmap writing, as some objects are not being packed"
+);
+
 static int add_object_entry(const unsigned char *sha1, enum object_type type,
 			    const char *name, int exclude)
 {

@@ -1010,8 +1014,14 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,
 	if (have_duplicate_entry(sha1, exclude, &index_pos))
 		return 0;
 
-	if (!want_object_in_pack(sha1, exclude, &found_pack, &found_offset))
+	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));
+			write_bitmap_index = 0;
+		}
 		return 0;
+	}
 
 	create_object_entry(sha1, type, pack_name_hash(name),
 			    exclude, name && no_try_delta(name),

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index d3a3afa..f13525c 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh

@@ -91,7 +91,10 @@ test_expect_success 'fetch (partial bitmap)' '
 
 test_expect_success 'incremental repack cannot create bitmaps' '
 	test_commit more-1 &&
-	test_must_fail git repack -d
+	find .git/objects/pack -name "*.bitmap" >expect &&
+	git repack -d &&
+	find .git/objects/pack -name "*.bitmap" >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'incremental repack can disable bitmaps' '

-- 
1.8.5.2.500.g8060133

On 01/23/2014 02:52 PM, Jeff King wrote:

Right, that's expected.

The bitmap format cannot represent objects that are not present in the
pack. So we cannot write a bitmap index if any object reachable from a
packed commit is omitted from the pack.

We could be nicer and downgrade it to a warning, though. The patch below
does that.

This makes sense.

quoted

In our case we have .keep files lying around from ages ago (possibly
due to kill -9s run on the server).

We ran into that problem at GitHub, too. We just turn off
`--honor-pack-keep` during our repacks, as we never want them on anyway
(and we would prefer to ignore the .keep than to abort the bitmap).

Yes, we'd prefer to do that too. How do you actually do this, though? I 
don't see a way to pass `--honor-pack-keep` (shouldn't I pass in its 
inverse?) down to `git-pack-objects`.

quoted

It also means that running repack -a with bitmap writing enabled on a
repo becomes problematic if a fetch is run concurrently.

For the most part, no. The .keep file should generally only be set
during the period between indexing the pack and updating the refs (so
while checking connectivity and running hooks). But pack-objects starts
from the ref tips and walks backwards. Until they are updated, it will
not try to pack the objects in the .keep files, as nobody references
them.

The worry is less certain objects not being packed and more the old 
packs being deleted by git repack, isn't it? From the man page for 
git-index-pack:

--keep
Before moving the index into its final destination create an empty .keep 
file for the associated pack file. This option is usually necessary with
--stdin to prevent a simultaneous git repack process from deleting the 
newly constructed pack and index before refs can be updated to use 
objects contained in the pack.

I could be misunderstanding things here, though. From the description in 
the man page it's not clear what the actual failure mode here is.

There are two loopholes, though:

   1. In some instances, a remote may send an object we already have
      (e.g., because it is a blob referenced in an old commit, but newly
      referenced again due to a revert; we do not do a full object
      difference during the protocol negotiation, for reasons of
      efficiency). If that is the case, we may omit it if pack-objects
      starts during the period that the .pack and .keep files exist.

   2. Once the fetch updates the refs, it removes the .keep file. But
      this isn't atomic. A repack which starts between the two may pick
      up the new ref values, but also see the .keep file.

These are both unlikely, but possible on a very busy repository. The
patch below will downgrade each to a warning, rather than aborting the
repack.

So this should just work out of the box with this patch.  But if bitmaps
are important to you (say, you are running a very busy site and want
to make sure you always have bitmaps turned on) and you do not otherwise
care about .keep files, you may want to disable them, too.

We need to make sure bitmaps are always turned on, but we need to be 
even more certain that pushes don't fail due to races.

On 01/23/2014 03:45 PM, Siddharth Agarwal wrote:

The worry is less certain objects not being packed and more the old 
packs being deleted by git repack, isn't it? From the man page for 
git-index-pack:

This should probably be "new pack" and not "old packs", I guess. Not 
knowing much about how this actually works, I'm assuming the scenario 
here is something like:

(1) git receive-pack receives a pack P.pack and writes it to disk
(2) git index-pack runs on P.pack
(3) git repack runs separately, finds pack P.pack with no refs pointing 
to it, and deletes it
(4) everything goes wrong

With a keep file, this would be averted because

(1) git receive-pack receives a pack P.pack and writes it to disk
(2) git index-pack writes a keep file for P.pack, called P.keep
(3) git repack runs separately, finds pack P.pack with a keep file, 
doesn't touch it
(4) git index-pack finishes, and something updates refs to point to 
P.pack and deletes P.keep

On Fri, Jan 24, 2014 at 12:45 AM, Siddharth Agarwal [off-list ref] wrote:

Yes, we'd prefer to do that too. How do you actually do this, though? I
don't see a way to pass `--honor-pack-keep` (shouldn't I pass in its
inverse?) down to `git-pack-objects`.

We run with this patch in production, it may be of use to you:
https://gist.github.com/vmg/8589317

In fact, it may be worth upstreaming too. I'll kindly ask peff to do
it when he has a moment.

Apologies for not attaching the patch inline, the GMail web UI doesn't
mix well with patch workflow.

Cheers,
vmg

On Fri, Jan 24, 2014 at 12:56:17AM +0100, Vicent Martí wrote:

On Fri, Jan 24, 2014 at 12:45 AM, Siddharth Agarwal [off-list ref] wrote:

quoted

Yes, we'd prefer to do that too. How do you actually do this, though? I
don't see a way to pass `--honor-pack-keep` (shouldn't I pass in its
inverse?) down to `git-pack-objects`.

We run with this patch in production, it may be of use to you:
https://gist.github.com/vmg/8589317

In fact, it may be worth upstreaming too. I'll kindly ask peff to do
it when he has a moment.

I was actually looking at it earlier when I sent this message. The
tricky thing about the patch is that it turns off --honor-pack-keep, but
does _not_ teach git-repack to clean up the .keep file.

Which I think is the right and safe thing to do, as otherwise you might
blow away a pack with .keep, even though you did not just pack its
objects (i.e., because it was written by a fetch or push which did not
yet update the refs). So the safe thing is to actually duplicate those
objects, leave the .keep pack around, and then assume it will get
cleaned up on the next repack.

If you _do_ have a stale .keep file, though, then that stale pack will
hang around forever (presumably with its objects duplicated in the
"real" pack).

So I think the patch is doing the right thing, but I was still figuring
out how to explain it (and I hope I just did). I'll post it with a full
commit message tomorrow.

-Peff

On Thu, Jan 23, 2014 at 03:53:28PM -0800, Siddharth Agarwal wrote:

On 01/23/2014 03:45 PM, Siddharth Agarwal wrote:

quoted

The worry is less certain objects not being packed and more the old
packs being deleted by git repack, isn't it? From the man page for
git-index-pack:

This should probably be "new pack" and not "old packs", I guess. Not
knowing much about how this actually works, I'm assuming the scenario
here is something like:

(1) git receive-pack receives a pack P.pack and writes it to disk
(2) git index-pack runs on P.pack
(3) git repack runs separately, finds pack P.pack with no refs
pointing to it, and deletes it
(4) everything goes wrong

With a keep file, this would be averted because

(1) git receive-pack receives a pack P.pack and writes it to disk
(2) git index-pack writes a keep file for P.pack, called P.keep
(3) git repack runs separately, finds pack P.pack with a keep file,
doesn't touch it
(4) git index-pack finishes, and something updates refs to point to
P.pack and deletes P.keep

I think your understanding is accurate here. So we want repack to
respect keep files for deletion, but we _not_ necessarily want
pack-objects to avoid packing an object just because it's in a pack
marked by .keep (see my other email).

-Peff

On 01/23/2014 06:28 PM, Jeff King wrote:

I think your understanding is accurate here. So we want repack to
respect keep files for deletion, but we _not_ necessarily want
pack-objects to avoid packing an object just because it's in a pack
marked by .keep (see my other email).

Yes, that makes sense and sounds pretty safe.

So the right solution for us probably is to apply the patch Vicent 
posted, set repack.honorpackkeep to false, and also have a cron job that 
cleans up stale .keep files so that subsequent repacks clean it up.

On Thu, Jan 23, 2014 at 06:44:43PM -0800, Siddharth Agarwal wrote:

On 01/23/2014 06:28 PM, Jeff King wrote:

quoted

I think your understanding is accurate here. So we want repack to
respect keep files for deletion, but we _not_ necessarily want
pack-objects to avoid packing an object just because it's in a pack
marked by .keep (see my other email).

Yes, that makes sense and sounds pretty safe.

So the right solution for us probably is to apply the patch Vicent
posted, set repack.honorpackkeep to false, and also have a cron job
that cleans up stale .keep files so that subsequent repacks clean it
up.

Yes, that matches what we do at GitHub.

Here's Vicent's patch, with documentation and an expanded commit
message. I think it should be suitable for upstream git.

-- >8 --
From: Vicent Marti <redacted>
Subject: repack: add `repack.honorpackkeep` config var

The git-repack command always passes `--honor-pack-keep`
to pack-objects. This has traditionally been a good thing,
as we do not want to duplicate those objects in a new pack,
and we are not going to delete the old pack.

However, when bitmaps are in use, it is important for a full
repack to include all reachable objects, even if they may be
duplicated in a .keep pack. Otherwise, we cannot generate
the bitmaps, as the on-disk format requires the set of
objects in the pack to be fully closed.

Even if the repository does not generally have .keep files,
a simultaneous push could cause a race condition in which a
.keep file exists at the moment of a repack. The repack may
try to include those objects in one of two situations:

  1. The pushed .keep pack contains objects that were
     already in the repository (e.g., blobs due to a revert of
     an old commit).

  2. Receive-pack updates the refs, making the objects
     reachable, but before it removes the .keep file, the
     repack runs.

In either case, we may prefer to duplicate some objects in
the new, full pack, and let the next repack (after the .keep
file is cleaned up) take care of removing them.

This patch introduces an option to disable the
`--honor-pack-keep` option.  It is not triggered by default,
even when pack.writeBitmaps is turned on, because its use
depends on your overall packing strategy and use of .keep
files.

Note that this option just disables the pack-objects
behavior. We still leave packs with a .keep in place, as we
do not necessarily know that we have duplicated all of their
objects.

Signed-off-by: Jeff King <redacted>
---
Intended for the jk/pack-bitmap topic.

 Documentation/config.txt | 8 ++++++++
 builtin/repack.c         | 8 +++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 947e6f8..5036a10 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -2128,6 +2128,14 @@ repack.usedeltabaseoffset::
 	"false" and repack. Access from old Git versions over the
 	native protocol are unaffected by this option.
 
+repack.honorPackKeep::
+	If set to false, include objects in `.keep` files when repacking
+	via `git repack`. Note that we still do not delete `.keep` packs
+	after `pack-objects` finishes. This means that we may duplicate
+	objects, but this makes the option safe to use when there are
+	concurrent pushes or fetches. This option is generally only
+	useful if you have set `pack.writeBitmaps`. Defaults to true.
+
 rerere.autoupdate::
 	When set to true, `git-rerere` updates the index with the
 	resulting contents after it cleanly resolves conflicts using

diff --git a/builtin/repack.c b/builtin/repack.c
index a9c4593..585c41d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c

@@ -9,6 +9,7 @@
 #include "argv-array.h"
 
 static int delta_base_offset = 1;
+static int honor_pack_keep = 1;
 static char *packdir, *packtmp;
 
 static const char *const git_repack_usage[] = {

@@ -22,6 +23,10 @@ static int repack_config(const char *var, const char *value, void *cb)
 		delta_base_offset = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "repack.honorpackkeep")) {
+		honor_pack_keep = git_config_bool(var, value);
+		return 0;
+	}
 	return git_default_config(var, value, cb);
 }
 

@@ -190,10 +195,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	argv_array_push(&cmd_args, "pack-objects");
 	argv_array_push(&cmd_args, "--keep-true-parents");
-	argv_array_push(&cmd_args, "--honor-pack-keep");
 	argv_array_push(&cmd_args, "--non-empty");
 	argv_array_push(&cmd_args, "--all");
 	argv_array_push(&cmd_args, "--reflog");
+	if (honor_pack_keep)
+		argv_array_push(&cmd_args, "--honor-pack-keep");
 	if (window)
 		argv_array_pushf(&cmd_args, "--window=%u", window);
 	if (window_memory)

-- 
1.8.5.2.500.g8060133