Jeff King <peff@xxxxxxxx> writes: > It does raise an interesting question, though. I had imagined we would > limit the depth of all delta chains here, not just ref-deltas. But it is > true that ofs deltas can't cycle. Without cycles, neither type can go on > indefinitely (they are limited by the number of entries in the > packfile). I could see arguments going either way: > > - ofs deltas cannot cycle, so we do not need a counter that limits > them (and which _could_ find a false positive). So we should not > limit them. > > - a counter is preventing us from following cycles indefinitely, but > also hardening us against misbehavior due to bugs or insanely large > delta chains (intentional or not). So we should include ofs deltas > in our limit. A chain can have both types, so I am fuzzy how the counting would go. We just do not count ofs_delta at all and only count ref_delta we've seen during the recursion? > A related point is that delta chains might be composed of both types. If > we don't differentiate between the two types, then the limit is clearly > total chain length. If we do, then is the limit the total number of > ref-deltas found in the current lookup, or is it the number of > consecutive ref-deltas? I guess it would have to be the former if our > goal is to catch cycles (since a cycle could include an ofs-delta, as > long as a ref-delta is the part that forms the loop). Ah, OK, you've thought about it already. I wonder we can just count both and limit the chain length to the total number of objects in the pack we are currently looking at? It guarantees to catch any cycle as long as pack is not thin, but is that too lenient and likely to bust the stack while counting? On the other side of the coin, we saw 10000 as a hard-coded limit in the patch, but do we know 10000 is low enough that most boxes have no trouble recursing that deep? Thanks.
