[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [sparse-directories] Depth parameter for the editor API

From: Daniel Rall <dlr_at_collab.net>
Date: 2007-03-21 20:34:19 CET

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

This is an archived mail posted to the Subversion Dev mailing list.