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.
