Ramkumar Ramachandra wrote:
> I would like to get r985477 merged into trunk. I've applied and used
> it successfully and checked that all tests pass.
>
> Warning: I have no background knowledge. I'm just reviewing the patch
> as-it-is, because Stefan asked me to.
>
> > [[[
> > r985477 | stefan2 | 2010-08-14 18:02:04 +0530 (Sat, 14 Aug 2010) | 9 lines
> >
> > Eliminate all file system access in get_default_file_perms() for all but the first execution.
> >
> > The only downside is that we don't detect FS permission changes made while the
> > (client) process runs. Because such changes would cause race conditions and file I/O
> > errors anyway, we are not make things worse by omitting those tests.
Looks good to me.
I wondered if it is safe in a long-running Subversion process, like
TortoiseSvn or a Linux equivalent.
It seems to me that it won't really matter much in practice. If someone
changes their umask and finds that Subversion carries on creating files
with the 'old' permissions that were in effect when Subversion was
started... that's fine, as far as I'm concerned.
To make it easier for others to see, here's the patch, ignoring
indentation changes:
[[[
$ svn diff -x -upw -c985477 http://svn.apache.org/repos/asf/subversion
Index: branches/performance/subversion/libsvn_subr/io.c
===================================================================
--- branches/performance/subversion/libsvn_subr/io.c (revision 985476)
+++ branches/performance/subversion/libsvn_subr/io.c (revision 985477)
@@ -1297,6 +1297,16 @@
static svn_error_t *
get_default_file_perms(apr_fileperms_t *perms, apr_pool_t *scratch_pool)
{
+ /* the default permissions as read from the temp folder */
+ static apr_fileperms_t default_perms = 0;
+
+ /* Technically, this "racy": Multiple threads may use enter here and
+ try to figure out the default permisission concurrently. That's fine
+ since they will end up with the same results. Even more technical,
+ apr_fileperms_t is an atomic type on 32+ bit machines.
+ */
+ if (default_perms == 0)
+ {
apr_finfo_t finfo;
apr_file_t *fd;
@@ -1321,6 +1331,13 @@ get_default_file_perms(apr_fileperms_t *
SVN_ERR(svn_io_file_close(fd, scratch_pool));
*perms = finfo.protection;
+ default_perms = finfo.protection;
+ }
+ else
+ {
+ *perms = default_perms;
+ }
+
return SVN_NO_ERROR;
}
]]]
- Julian
Received on 2010-10-01 17:59:35 CEST