On Tue, Aug 12, 2014 at 10:13:03PM +0700, Duy Nguyen wrote: > On Tue, Aug 12, 2014 at 11:34 AM, Jeff King <peff@xxxxxxxx> wrote: > > 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. > > 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). 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. > 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). Yeah, that sounds like a good direction for the shallow part of it. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html