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
Received on 2017-03-16 15:26:45 CET