Den mån 5 feb. 2024 kl 07:38 skrev Branko Čibej <br...@apache.org>:
> On 04.02.2024 00:31, Evgeny Kotkov via dev wrote: > > Daniel Sahlberg <daniel.l.sahlb...@gmail.com> <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). > > > I agree. If it's worth solving, it should be done in a way that's specific > to revert, not here. > > -- Brane > Thanks for the feedback and sorry for the late reply. I've been swamped with other tasks lately and have not yet found the time to look at this but it is for sure on my todo list. Kind regards, Daniel
