On Sat, Oct 16, 2010 at 12:54 PM, Hyrum K. Wright <hyrum_wri...@mail.utexas.edu> wrote: > On Fri, Oct 15, 2010 at 10:27 AM, Bert Huijben <b...@qqmail.nl> wrote: >> >> >>> -----Original Message----- >>> From: Stefan Sperling [mailto:s...@elego.de] >>> Sent: donderdag 14 oktober 2010 23:39 >>> To: dev@subversion.apache.org >>> Subject: Re: svn commit: r1022707 - in /subversion/trunk: ./ >>> subversion/libsvn_subr/io.c >>> >>> On Thu, Oct 14, 2010 at 09:00:44PM -0000, hwri...@apache.org wrote: >>> > Author: hwright >>> > Date: Thu Oct 14 21:00:43 2010 >>> > New Revision: 1022707 >>> > >>> > URL: http://svn.apache.org/viewvc?rev=1022707&view=rev >>> > Log: >>> > Merge r985472 from the performance branch, which see for more >>> information. >>> > >>> > This revision randomizes the numerical suffix of temporary files, in an >>> effort >>> > to make finding one easier. >> >> >>> > @@ -384,17 +393,35 @@ svn_io_open_uniquely_named(apr_file_t ** >>> > This is good, since "1" would misleadingly imply that >>> > the second attempt was actually the first... and if someone's >>> > got conflicts on their conflicts, we probably don't want to >>> > - add to their confusion :-). */ >>> > + add to their confusion :-). >>> > + >>> > + Also, the randomization used to minimize the number of re-try >>> > + cycles will interfere with certain tests that compare working >>> > + copies etc. >>> > + */ >>> > if (i == 1) >>> > unique_name = apr_psprintf(scratch_pool, "%s%s", path, suffix); >>> > else >>> > - unique_name = apr_psprintf(scratch_pool, "%s.%u%s", path, i, >> suffix); >>> > + unique_name = apr_psprintf(scratch_pool, "%s.%u_%x%s", path, i, >>> rand(), suffix); >>> >>> -1 on using rand() here, for several reasons: >>> >>> The intent of svn_io_open_uniquely_named() is to provide user-facing >>> temporary files. The order of numbers actually carry meaning in some >> cases. >>> With files named svn-commit.tmp, svn-commit.2.tmp, svn-commit.3.tmp, >>> users >>> can easily tell which one is the newest by looking at the number. >>> Names with random numbers don't provide this information. >>> >>> We already have a different function to create truely randomly-named >>> temporary files, svn_io_open_unique_file3(). It should be used instead >>> of svn_io_open_uniquely_named() wherever doing so doesn't hurt user >>> convenience (i.e. for any tempfile a user will never directly work with). >>> >>> Also, rand() isn't thread-safe, and this is library code. >> >> +1 on this reasoning and the -1. >> This merge should be reverted and performance critical code should use >> svn_io_open_unique_file3() instead of this function. >> (I think we already did this in most cases. In 1.6 this function was a >> severe performance problem) > > There are parts to the merge which are still useful, so I propose that > instead of reverting the merge, we make the appropriate reversions on > the performance branch, and then merge that revision to trunk.
Change made on the branch in r1025660, and merged to trunk in r1025668. -Hyrum
