Bert Huijben wrote on Thu, Mar 28, 2013 at 12:52:51 +0100: > > > > -----Original Message----- > > From: danie...@apache.org [mailto:danie...@apache.org] > > Sent: donderdag 28 maart 2013 12:37 > > To: comm...@subversion.apache.org > > Subject: svn commit: r1462054 - in /subversion/branches/verify-at- > > commit/subversion: include/svn_fs.h libsvn_fs/fs-loader.c libsvn_fs/fs- > > loader.h > > > > Author: danielsh > > Date: Thu Mar 28 11:36:50 2013 > > New Revision: 1462054 > > > > URL: http://svn.apache.org/r1462054 > > Log: > > On the verify-at-commit branch, add a backend-independent > > implementation: > > a db/fs.conf file. > > > > * subversion/include/svn_fs.h > > (SVN_FS_CONFIG_SECTION_MISC, > > (SVN_FS_CONFIG_OPTION_VERIFY_AT_COMMIT): > > New macros. > > > > * subversion/libsvn_fs/fs-loader.h > > (svn_fs_t.verify_at_commit): New struct member. > > > > * subversion/libsvn_fs/fs-loader.c > > (svn_config.h): Include. > > (CONFIG_FILENAME): New macro. > > (write_config): New helper, based on the eponymous helper in FSFS (rather > > than on write_fs_type()). > > (svn_fs_create): Call write_config(). > > (fs_new): Initialise new struct member. > > (svn_fs_open): Read config. > > (svn_fs_commit_txn): Use the config. > > Summarizing what was said on #svn-dev > > I think we should make this a fsfs only feature that uses the existing > fsfs.conf file. > > The option doesn't appear to make much sense for the now mostly > deprecated for new development BDB filesystem and the new work of > stefan2 might give new ideas. Besides if a filesystem needs a > verification step to be stable that should be part of the design. > Subversion shouldn't start assuming that filesystems are likely to > break down. (What would that tell us about the stability of previous > versions of Subversion?) >
Bugs happen, this is one way to catch them. Compare r1086222, which is basically a special-case of this work. > The reading of one file for each access to the repository is a more > than measurable slowdown when profiling operations. (Reading fsfs.conf > over and over again is one of the most expensive things apache worker > processes do when I profiled them. I think stefan2 optimized some of > this away) > OK, we can move this particular config knob to be provided at runtime by whoever creates the svn_fs_t. > Opening a file is expensive on Windows and probably on any other > system that always uses locking for file accesss and/or on-demand > virus scanners and also on network drives. > Don't virus-scan repository config files. (Why would you want to do that? Do you fear svn would try to execute the config option's value?) > > I don't think introducing yet another config file makes much sense. If > users want to turn on verifications at every commit they probably want > it for all their repositories (in which case the config option belongs > in the apache or svnserve config) or they are looking at specific fsfs > issues, in which case the option can be in fsfs.conf which is read > anyway. > See above, and yes if we end up opting for the backend-specific implementation then the new fs.conf file will go away. > > I wouldn't want to introduce yet another layer of configuration for > this verification helper. > > But then again I can see reasons that some users want the explicit > verification. fsfs.conf and/or a post commit hook are good enough > solutions without any performance/design implications. > post-commit isn't good enough, since it's post-. 'svnlook verify -t' in pre-commit is as good as the libsvn_fs implementation. That is nearly as good as the "just before bumping current" approach, althuogh that one would catch bugs in the rev file that are absent from the proto-rev file, too. > Bert >
