Re: svn commit: r1463721 - in /subversion/trunk/subversion/libsvn_client: checkout.c client.h commit.c copy.c export.c externals.c revert.c switch.c update.c

--
Join WANdisco's free daily demo sessions on Scaling Subversion for the Enterprise
<http://www.wandisco.com/training/webinars>
----- Original Message -----
> From: Philip Martin <philip.martin_at_wandisco.com>
> To: dev_at_subversion.apache.org
> Cc: 
> Sent: Thursday, 11 April 2013, 6:27
> Subject: Re: svn commit: r1463721 - in /subversion/trunk/subversion/libsvn_client: checkout.c client.h commit.c copy.c export.c externals.c revert.c switch.c update.c
> 
> julianfoad_at_apache.org writes:
> 
>>  Author: julianfoad
>>  Date: Tue Apr  2 19:53:43 2013
>>  New Revision: 1463721
>> 
>>  URL: http://svn.apache.org/r1463721
>>  Log:
>>  Fix some sleep-for-timestamps code: avoid missing some cases where we need
>>  to sleep -- especially error cases -- and avoid sleeping in some cases 
> where
>>  we don't need to.  Simplify the code by using a consistent pattern: 
> make the
>>  decision of whether to sleep at the lowest level possible (ideally in the 
> WC
>>  layer, but not yet), and do the sleep at the highest level possible
>>  (immediately below the libsvn_client API).
> 
>>  * subversion/libsvn_client/copy.c
>>    (do_wc_to_wc_copies_with_write_lock,
>>     do_wc_to_wc_moves,
>>     repos_to_wc_copy_single): Return a flag instead of sleeping here. A
>>       multi-target repos-to-WC copy will now only sleep once, not once per
>>       target.
>>    (do_wc_to_wc_copies,
>>     repos_to_wc_copy_locked,
>>     repos_to_wc_copy,
>>     try_copy): Pass the flag through.
>>    (svn_client_copy6,
>>     svn_client_move7): Handle the sleep here.
> 
>>  --- subversion/trunk/subversion/libsvn_client/copy.c (original)
>>  +++ subversion/trunk/subversion/libsvn_client/copy.c Tue Apr  2 19:53:43 
> 2013
>>  @@ -212,7 +214,6 @@ do_wc_to_wc_copies_with_write_lock(const
>>       }
>>     svn_pool_destroy(iterpool);
>>   
>>  -  svn_io_sleep_for_timestamps(dst_parent, scratch_pool);
>>     SVN_ERR(err);
>>     return SVN_NO_ERROR;
>>   }
> 
>>  @@ -2292,6 +2312,9 @@ svn_client_copy6(const apr_array_header_
>>                        subpool);
>>       }
>>   
>>  +  if (timestamp_sleep)
>>  +    svn_io_sleep_for_timestamps(NULL, subpool);
>>  +
>>     svn_pool_destroy(subpool);
>>     return svn_error_trace(err);
>>   }
> 
> 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.
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.
- Julian