Daniel Rall wrote:
> On Wed, 21 Mar 2007, Karl Fogel wrote:
>
>> Daniel Rall <dlr@collab.net> writes:
>>> Karl, attached is a patch attempting to implement the outstanding
>>> suggestions from Peter's "Default depth in reporter?" review mail:
>>>
>>> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=124363
>>>
>>> I've dropped the DEPTH parameter from the svn_repos_begin_report3()
>>> API and its callers, in favor of writing the DEPTH passed to
>>> set_path() to the report, and subsequently using that depth during the
>>> editor drive. An unknown value passed to write_path_info() will
>>> default to report_baton_t->default_depth (a field which I renamed from
>>> depth). I've got svn_repos_begin_report3() setting
>>> report_baton_t->default_depth to svn_depth_infinity, but perhaps it
>>> should be smuggling in a depth of svn_depth_unknown?
>>>
>>> You'll notice that I had to change svn_wc_crawl_revisions3() to use
>>> the passed-in DEPTH parameter (when known) for the first call to
>>> set_path() (there's another call to set_path() near the beginning of
>>> this routine which may need similar treatment).
>> Dan, thanks for doing this! I will try to take it further tomorrow
>> (bad timing -- long doctor's appointment in the middle of the day),
>> and Thursday is fortunately more free. Sorry for the delay; I want to
>> get this merged too, but I think Malcolm and Peter are right about
>> fixing this bit of API lossage first.
>>
>> I've looked over the patch, it seems like the right idea. Very +1 on
>> the rename of "baton->depth" to "baton->default_depth" :-).
>
> Okay.
>
>> When you
>> wrote that you've "got svn_repos_begin_report3() setting
>> report_baton_t->default_depth to svn_depth_infinity, but perhaps it
>> should be smuggling in a depth of svn_depth_unknown?", I got a bit
>> confused, though.
>
> svn_repos_begin_report2() currently sets report_baton_t->default_depth
> to svn_depth_infinity. Malcolm and I was discussing the validity of
> this today on IRC -- since default_depth shouldn't really be used in
> conjunction with the svn_repos_begin_report2() API, perhaps
> svn_depth_unknown would be a better choice? In actuality,
> default_depth is used if svn_depth_unknown is passed to the new
> set_path() or link_path() reporter APIs; the fact that this occurs is
> an implementation detail of the svn_repos_begin_report() compatibility
> code. Malcolm and I were wondering if the new set_path()/link_path()
> APIs should actually error out if svn_depth_unknown is passed to them.
>
> Lastly, it *looks* like svn_depth_infinity and svn_depth_unknown are
> actually getting treated the same way inside libsvn_repos's reporter
> implementation (I haven't actually tested this). write_path_info(),
> which is called directly by set_path() and link_path(), falls into its
> default case:
>
> if (depth == svn_depth_empty)
> drep = "+0:";
> else if (depth == svn_depth_files)
> drep = "+1:";
> else if (depth == svn_depth_immediates)
> drep = "+2:";
> else /* svn_depth_infinity */
> drep = "-";
>
> ...and read_path_info() assumes infinity:
>
> if (c == '+')
> {
> ...
> }
> else
> {
> (*pi)->depth = svn_depth_infinity;
> }
>
>> Here's the situation in the patch as I understand it:
>>
>> There is an svn_repos_begin_report2() (no "3", right?),
>
> Yup.
>
>> and the
>> difference between it and svn_repos_begin_report() is that the newer
>> one takes neither recurse nor depth (because set_path will do it).
>> Right now, you're implementing svn_repos_begin_report2() as a wrapper
>> around svn_repos_begin_report() (in other words, backwards from the
>> usual way we do it), and you're passing TRUE for the 'recurse' flag to
>> svn_repos_begin_report(), which translates to svn_depth_infinity on
>> this [new] line in svn_repos_begin_report():
>>
>> b->default_depth = SVN_DEPTH_FROM_RECURSE(recurse);
>>
>> So your intention is that that's all placeholder code until we get
>> things figured out, and that we'll do the compatibility wrapping in
>> the usual direction in the final cut, right?
>
> Yeah, the patch was more concerned with finding the right approach.
>
> I've committed a modified version of the patch to the branch in
> r23967. Other than the issue I pointed out above with default_depth,
> I believe this task is complete.
What happens now if a pre-1.5 client tries to do a non-recursive
checkout? If I'm reading the code right, it looks like we set the
default depth to infinity, expecting the client to set the real depth
later via set_path(). However, old clients don't know about the new
set_path(), so it looks like they'd be stuck with a depth of infinity.
--
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Mar 24 15:14:34 2007