Julian Foad wrote on Thu, Mar 16, 2017 at 11:50:34 +0000: > What sort of testing do you think this needs? A first level could be to test > that the verification code runs when the option is explicitly enabled and > doesn't when disabled, and preferably test the default state too. A second > level could be to test that the verification actually picks up some > (injected) corruption and prevents the commit and exits cleanly. This second > level is not directly related to making the feature optional, but it is > related to providing the feature at all in a release-mode build.
Another thing we could test: 'make check' with the knob enabled. I'd lean to having that use 'svnserve -T' or a threaded MPM, in order to increase the chances of catching possible bugs related to process-global state being inappropriately modified by the "Time travel" svn_fs_t handle. > Daniel Shahaf wrote: > >By the way, are you planning to enable > >verify_as_revision_before_current_plus_plus() > >in alpha3? I would suggest enabling it unconditionally before alpha3, > >then reverting it [or adding the knob under discussion] at some point > >before branching 1.10.x. > > Sounds good. To keep track of the need to change the default back again, > would a bold coloured box in the release notes would be a suitable place for > such a note? I was thinking of an issue with milestone 1.10.0, but anything we'll run into before cutting rc1 would be fine. > >[We should probably give that function a shorter name... ;)] > > Could do. verify_before_commit? Sure. > @@ -1017,12 +1028,19 @@ write_config(svn_fs_t *fs, > +"" > NL > +"[" CONFIG_SECTION_DEBUG "]" > NL > +"###" > NL > +"### Whether to verify each new revision immediately before finalizing" > NL > +"### the commit. The default is false in release-mode builds, and true" > NL > +"### in debug-mode builds." > NL > +"# " CONFIG_OPTION_VERIFY_BEFORE_COMMIT " = false" > NL The phrase "debug-mode builds" isn't quite accurate: on Unix, SVN_DEBUG is enabled by --enable-maintainer-mode but not by --enable-debug. (On windows, there's no distinction between maintainer and debug modes, and SVN_DEBUG is defined in that case.) I'm not sure whether we should change the comment to match the code, or vice-versa. Cheers, Daniel
