[PATCH] pack-objects: turn off bitmaps when we see --shallow lines

[Jeff King <peff@peff.net>]

Reachability bitmaps do not work with shallow operations,
because they cache a view of the object reachability that
represents the true objects. Whereas a shallow repository
(or a shallow operation in a repository) is inherently
cutting off the object graph with a graft.

We explicitly disallow the use of bitmaps in shallow
repositories by checking is_repository_shallow(), and we
should continue to do that. However, we also want to
disallow bitmaps when we are serving a fetch to a shallow
client, since we momentarily take on their grafted view of
the world.

It used to be enough to call is_repository_shallow at the
start of pack-objects.  Upload-pack wrote the other side's
shallow state to a temporary file and pointed the whole
pack-objects process at this state with "git --shallow-file",
and from the perspective of pack-objects, we really were
in a shallow repo.  But since b790e0f (upload-pack: send
shallow info over stdin to pack-objects, 2014-03-11), we do
it differently: we send --shallow lines to pack-objects over
stdin, and it registers them itself.

This means that our is_repository_shallow check is way too
early (we have not been told about the shallowness yet), and
that it is insufficient (calling is_repository_shallow is
not enough, as the shallow grafts we register do not change
its return value). Instead, we can just turn off bitmaps
explicitly when we see these lines.

Signed-off-by: Jeff King <peff@peff.net>
---
Sorry not to catch this earlier. The bug is in v2.0, and I only noticed
the regression because a very small percentage of shallow fetches from
GitHub started failing after we deployed v2.0 a few weeks ago. It took
me a while to figure out the reproduction recipe below. :)

Arguably is_repository_shallow should return 1 if anybody has registered
a shallow graft, but that wouldn't be enough to fix this (we'd still
need to check it again _after_ reading the --shallow lines). So I think
this fix is fine here. I don't know if any other parts of the code would
care, though.

 builtin/pack-objects.c          |  1 +
 t/t5311-pack-bitmaps-shallow.sh | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)
 create mode 100755 t/t5311-pack-bitmaps-shallow.sh

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 238b502..b59f5d8 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2494,6 +2494,7 @@ static void get_object_list(int ac, const char **av)
 				if (get_sha1_hex(line + 10, sha1))
 					die("not an SHA-1 '%s'", line + 10);
 				register_shallow(sha1);
+				use_bitmap_index = 0;
 				continue;
 			}
 			die("not a rev '%s'", line);
diff --git a/t/t5311-pack-bitmaps-shallow.sh b/t/t5311-pack-bitmaps-shallow.sh
new file mode 100755
index 0000000..872a95d
--- /dev/null
+++ b/t/t5311-pack-bitmaps-shallow.sh
@@ -0,0 +1,39 @@
+#!/bin/sh
+
+test_description='check bitmap operation with shallow repositories'
+. ./test-lib.sh
+
+# We want to create a situation where the shallow, grafted
+# view of reachability does not match reality in a way that
+# might cause us to send insufficient objects.
+#
+# We do this with a history that repeats a state, like:
+#
+#      A    --   B    --   C
+#    file=1    file=2    file=1
+#
+# and then create a shallow clone to the second commit, B.
+# In a non-shallow clone, that would mean we already have
+# the tree for A. But in a shallow one, we've grafted away
+# A, and fetching A to B requires that the other side send
+# us the tree for file=1.
+test_expect_success 'setup shallow repo' '
+	echo 1 >file &&
+	git add file &&
+	git commit -m orig &&
+	echo 2 >file &&
+	git commit -a -m update &&
+	git clone --no-local --bare --depth=1 . shallow.git &&
+	echo 1 >file &&
+	git commit -a -m repeat
+'
+
+test_expect_success 'turn on bitmaps in the parent' '
+	git repack -adb
+'
+
+test_expect_success 'shallow fetch from bitmapped repo' '
+	(cd shallow.git && git fetch)
+'
+
+test_done
-- 
2.1.0.rc0.286.g5c67d74