Re: svnsync crash on empty repo

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
Received on 2015-09-05 15:00:33 CEST