> -----Original Message-----
> From: Stefan Sperling [mailto:stsp_at_elego.de]
> Sent: donderdag 14 oktober 2010 23:39
> To: dev_at_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, hwright_at_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)
Bert
Received on 2010-10-15 17:29:03 CEST