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.
Received on 2017-05-16 20:25:04 CEST