I (Julian Foad) wrote:
> Bert Huijben wrote:
>> I also see new test failures on the two new tests for ra_serf.
>
> It's another real bug in svnrdump. It calls
> commit_editor->change_dir_prop() twice for the same property name, which is
> documented as not allowed. The other RA layers don't mind, but RA-serf does.
RA-serf accepts the calls but commits an unexpected result because it processes
the non-deleting prop changes first and then the prop deletes.
For example (as in svnrdump_tests.py 52), to change properties from (p=v, q=v)
to (q=v), svnrdump calls:
set_node_property("p", NULL)
set_node_property("q", NULL)
set_node_property("q", "v")
which results in libsvn_ra_serf/commit.c sending:
PROPPATCH: pname="q", pval="v"
PROPPATCH: pname="p", pval=NULL
PROPPATCH: pname="q", pval=NULL
resulting in no properties.
> I should be able to test for this bug independently of issue #4551.
r1654186 adds such a regression test. I'm keeping it within issue #4551, but
it's independent of *copying* a node which was where that issue showed up until
now.
Bert's commit http://svn.apache.org/r1654194 "Simplify the ra_serf proppatch
code ..." changes RA-serf to work the same way as the other RA layers and thus
avoids the problem here. The tests for this issue now pass, although
svndumpfilter is still violating the editor rules.
Why haven't we found this before? Because we haven't tested it, is the easy
answer. But we already had a test that exercises this kind of scenario
(svnrdump_tests.py 49 -- but it's only looking to see if the commit fails, not
checking the results). If we had generic editor call checking in place then
maybe we would have found this driving order violation earlier.
Maybe we should introduce an editor validator, even at this late stage. Note
that Ev2 attempts to provide one -- see ENABLE_ORDERING_CHECK in
libsvn_delta/editor.c.
I also noticed that configuring with '--enable-ev2-shims' creates the same
driving order violation, so that several tests would fail against RA-serf, but
since r1654194 they no longer fail.
Wondering what to do next... I think we should still fix the driving order in
svndumpfilter.
We should definitely backport the issue #4551 fixes when they're ready.
- Julian
