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

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.

---
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

index 238b502..b59f5d8 100644
@@ -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);

new file mode 100755
index 0000000..872a95d
@@ -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

It's getting too subtle (is_repository_shallow fails to return 1). register_shallow() is used elsewhere too, luckily pack bitmap's use is still limited in pack-objects (I think).I prefer (in future) to teach is_repository_shallow about register_shallow and move it to right before get_object_list_from_bitmap() is called, and some sort of mechanism to say "hey I'm all set, never change shallow repo status again from now on, or just die if you have to do it" to protect us from similar bugs. But for now your fix is good (and simple).

Duy

It is, though I have some patches in the works to use it in more places.

I was tempted to make a check in prepare_bitmap_walk() to just return -1 (the same as if there are no bitmaps at all) if any commit grafts are in use. That would also catch new callers. But the graft (and replace) rules are not always the same. We should not respect those features when packing or pruning (though I think pruning _does_ currently respect grafts, which seems like an accident waiting to happen).

I think this is a good minimal fix for now, but I'll revisit the replace/graft/shallow issues when I add more bitmap users outside of pack-objects.

Yeah, that sounds like a good direction for the shallow part of it.

-Peff