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.
- application/pgp-signature attachment: stored
Received on Wed Mar 21 20:34:41 2007