> I (Julian Foad) wrote:
>> Philip Martin wrote:
>>> julianfoad_at_apache.org writes:
>>>> URL: http://svn.apache.org/r1463721
> [...]
>>>> --- subversion/trunk/subversion/libsvn_client/copy.c (original)
>>>> +++ subversion/trunk/subversion/libsvn_client/copy.c Tue Apr 2
>>>> @@ -212,7 +214,6 @@ do_wc_to_wc_copies_with_write_lock(const
>>>>
>>>> - svn_io_sleep_for_timestamps(dst_parent, scratch_pool);
>>>>
>>>> @@ -2292,6 +2312,9 @@ svn_client_copy6(const apr_array_header_
>>>>
>>>> + if (timestamp_sleep)
>>>> + svn_io_sleep_for_timestamps(NULL, subpool);
>>>> +
>>>
>>> Is there any reason you are passing NULL to svn_io_sleep_for_timestamps
>>> rather than dst_path?
>>
>> Good catch.
>>
>> I suppose I was thinking I couldn't be sure that the same path was used in
>> each possible case. Looking at it again, if dst_path is not a URL then it
>> would be reasonable to assume that all relevant changes are on the same
>> filesystem as dst_path. Even more so if we use the child of dst_path
>> instead, in the copy_as_child case; then the path used would be, I think,
>> the same as it was when the sleep_for_timestamps calls were distributed
>> among the leaf functions before my patch.
Philip Martin wrote:
> The path is used to estimate the timestamp resolution, it dosen't matter
> which path in the destination is used.
The child path will usually be created on the same filesystem as the
parent, but I'm not sure that's *always* the case because I haven't
studied the details of how the child path is created by the copy code.
If the child path in some cases is "created" by overwriting an existing
path, then perhaps it can be on a different FS. If it is created from
scratch and the operating system allows something like a "mount point"
to be defined without a directory already existing at that path, then
too perhaps the child can be created on a different FS.
Rather than try to investigate all the possibilities, I went for the
simpler solution of passing the actual child path, which is the same as
what it was doing before my changes.
That still doesn't eliminate the possibility of a grandchild path being
on a different FS, but we can't fix that without a complete overhaul of
the sleep-for-timestamps infrastructure.
> The other thing to consider is a move between working copies: the source
> might be on a filesystem with 1s sleeps while the destination is on a
> filesystem with 1ms sleeps. Even then it should be OK to use dst_path
> because the source is only getting the delete so there are no timestamps
> of concern on that filesystem.
Yes, I considered that, and agree that's OK because the deletes are of no
concern.
>> And when dst_path is a URL, timestamp_sleep shouldn't be true, but I also
>> notice I failed to initialize 'timestamp_sleep'. The standard pattern
>> is that the called functions don't evert set it false, they only ever set
>> it true, so the top level function needs to initialize it.
>>
>> Will fix.
>
> Committed r1466876.
- Julian
Received on 2013-04-11 16:07:33 CEST