Bert wrote: > I don't think we should fix this with a 'revision+1' to explicitly allow > many bad ranges,
Why? An empty range isn't inherently "bad". Allowing an empty range isn't bad. Being able to specify an empty range in many different ways isn't bad. Changing a released API to make a previously no-op case now throw an assertion failure breaks our compatibility guarantees. That is bad. I don't like having undocumented behaviours and having to decide whether they're to be supported or just accidental, but that's what we have. In this case I think the behaviour is consistent and logical, and is demonstrably used by a client (our own 'svn'). In this case, we should decide that it is supported (and we should document it from now on). I explained why I think we *should* fix this. Please will you explain why you think we shouldn't. > but I do think we should specifically fix the r0 case for > empty repositories. Note that, as per my follow-up email, it's not specific to r0, but applies to any sync when the mirror is already up to date. I think we should change the caller to avoid calling replay_range with an empty range, but mainly for aesthetic reasons within the caller (it already has an early return but it is off by one), not because there's anything wrong with making such a call. (I think we should also add a regression test that does call replay_range with an empty range, to verify that it continues to work.) - Julian
