[PATCH] Re: verify_as_revision_before_current_plus_plus() on a production repo?

[Julian Foad <ju...@apache.org>]

Daniel Shahaf wrote:
>> should our policy be to provide administrative choice?
>
> A knob sounds good to me [...]

And Bert said "+1" on IRC too.

The attached patch is a basic implementation. The fsfs.conf option is 
spelled

   [debug]
   verify-before-commit = true

I put it under [debug] because that section name already exists and 
holds a "pack after commit" option (not documented in the file template 
comments) which seems conceptually similar.

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.


> If we enable this outside maintainer mode, we should look into the
> points raised upthread about ensuring that 'verify' is read-only.
> We don't have to fix everything, but we should at least add roadsign
> comments in the code to reduce the chance that future changes to verify
> will interact badly with the unusual callsite in verify_as_revision_before_current_plus_plus().

Ack.

> 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?

> [We should probably give that function a shorter name... ;)]

Could do. verify_before_commit?

- Julian

Re: [PATCH] Re: verify_as_revision_before_current_plus_plus() on a production repo?

[Stefan Fuhrmann <st...@alice-dsl.de>]

On 16.03.2017 12:50, Julian Foad wrote:
> Daniel Shahaf wrote:
>>> should our policy be to provide administrative choice?
>>
>> A knob sounds good to me [...]
>
> And Bert said "+1" on IRC too.
>
> The attached patch is a basic implementation. The fsfs.conf option is 
> spelled
>
>   [debug]
>   verify-before-commit = true
>
> I put it under [debug] because that section name already exists and 
> holds a "pack after commit" option (not documented in the file 
> template comments) which seems conceptually similar.
>
> 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.
>
>
>> If we enable this outside maintainer mode, we should look into the
>> points raised upthread about ensuring that 'verify' is read-only.
>> We don't have to fix everything, but we should at least add roadsign
>> comments in the code to reduce the chance that future changes to verify
>> will interact badly with the unusual callsite in 
>> verify_as_revision_before_current_plus_plus().
>
> Ack.
>
>> 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?
>
>> [We should probably give that function a shorter name... ;)]
>
> Could do. verify_before_commit?

I applied the patch, including the rename, in r1795351.

It adds 50% to our test suite (2:10 instead of 1:26).
If we were to enable it by default in release mode,
we need to add a feature to disable it for mass ops
like 'svnadmin import'.

-- Stefan^2.

Re: [PATCH] Re: verify_as_revision_before_current_plus_plus() on a production repo?

[Daniel Shahaf <d....@daniel.shahaf.name>]

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