On 04.02.2024 00:31, Evgeny Kotkov via dev wrote:
Daniel Sahlberg<[email protected]> 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).
I agree. If it's worth solving, it should be done in a way that's specific to revert, not here.
-- Brane
