Re: svn commit: r1653988 - in /subversion/trunk/subversion: svnrdump/load_editor.c tests/cmdline/svnrdump_tests.py

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
Received on 2015-01-23 15:54:31 CET