Daniel Sahlberg <daniel.l.sahlb...@gmail.com> writes: >> Index: subversion/libsvn_subr/io.c >> =================================================================== >> --- subversion/libsvn_subr/io.c (revision 1915064) >> +++ subversion/libsvn_subr/io.c (working copy) >> @@ -2535,7 +2535,14 @@ svn_io__is_finfo_read_only(svn_boolean_t *read_onl >> apr_uid_t uid; >> apr_gid_t gid; >> >> - *read_only = FALSE; >> + *read_only = (access(file_info->fname, W_OK) != 0);
There are a few problems, because this approach adds a I/O syscall (that checks access to a file by its name) into a fairly low-level function. Previously this function was analyzing the file information from the passed-in apr_finfo_t structure. The new version, however, performs an I/O call, and that leads to a couple of problems: 1) Potential performance regression, not limited to just the revert. For example, all calls to open_pack_or_rev_file() in libsvn_fs_fs would now start to make additional access() calls, thus slowing down the repository operations. 2) This adds an opportunity for a desynchronization/race between the information in apr_finfo_t and the result of access(), because access() works by a filename. For example, in a case where a file is recreated between the two calls, its result will correspond to a completely different file, compared to the one that produced the apr_finfo_t. Overall, I have a feeling that solving this specific edge-case might not be worth introducing these regressions (which look significant, at least to me). Thanks, Evgeny Kotkov
