Bert Huijben wrote on Thu, Mar 28, 2013 at 12:52:51 +0100:
>
>
> > -----Original Message-----
> > From: danielsh_at_apache.org [mailto:danielsh_at_apache.org]
> > Sent: donderdag 28 maart 2013 12:37
> > To: commits_at_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
>
Received on 2013-03-28 13:23:02 CET