On Fri, Oct 15, 2010 at 10:27 AM, Bert Huijben <bert_at_qqmail.nl> wrote:
>
>
>> -----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)
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.
As an aside, it would be easier if people reviewed these changes on
the branch and commented in the STATUS file, rather than wait until
the merge actually happens. :/
-Hyrum
Received on 2010-10-16 19:54:43 CEST