Vlad Georgescu <vgeorgescu@gmail.com> writes:
>> I expected that parameter to be...
>>
>> info ? DEPTH_BELOW_HERE(info->depth)
>> : DEPTH_BELOW_HERE(wc_depth),
>>
>> ...but maybe I'm missing something?
>
> info->depth is the depth of the entry. wc_depth is the depth of the
> parent of the entry.
Ah, right. Thanks.
>> else if (depth != svn_depth_empty)
>> {
>> /* Recursively crawl ROOT_DIRECTORY and report differing
>> revisions. */
>> err = report_revisions_and_depths(adm_access,
>> "",
>> base_rev,
>> reporter, report_baton,
>> notify_func, notify_baton,
>> restore_files, depth,
>> entry->incomplete,
>> use_commit_times,
>> traversal_info,
>> pool);
>> if (err)
>> goto abort_report;
>> }
>> }
>>
>> That seems wrong. It's used for the conditional and as a parameter to
>> report_revisions_and_depths(). But if 'svn_depth_unknown' was passed
>> in as the requested depth parameter to svn_wc_crawl_revisions3(),
>> shouldn't it just be passed along to report_revisions_and_depths()?
>>
>> Everything else about this patch seems exactly right to me, except
>> this one thing, but it's an important thing :-). If svn_depth_unknown
>> was requested, then that's what we should report to the repository.
>
> The requested depth was already sent to the server by the time we call
> svn_wc_crawl_revisions3() (we send it in svn_ra_do_update2()). When
> reporting wc paths, both svn_depth_unknown and svn_depth_infinity mean
> "report everything inside the working copy". The server will, of
> course, treat svn_depth_unknown and svn_depth_infinity differently, but
> when reporting the paths, it's simpler to just use infinity (it
> simplifies the depth-handling logic inside report_revisions_and_depths).
Okay, I understand, and after your explanation, the situation seems a
bit less severe than I thought. Also, I've now looked more deeply
into report_revisions_and_depths() and see that we never again use the
so-called requested depth for reporting via set_path() or link_path().
But IMHO, it would still be better, in principle, to not toss away the
information of whether a depth was explicitly requested. Right now,
based on the doc strings, when someone sees that 'depth' parameter in
adm_crawler.c, they will assume it represents the depth the user
actually requested -- yet it might not.
So either we should change the doc strings, or change the recursion
logic to handle svn_depth_unknown like svn_depth_infinity. I prefer
the latter, because it would maintain the requested-depth information
in the most intuitive way, even though it would mean adding an extra
clause to a few conditionals.
However, I'm not asking you to make this change. If you commit your
patch, I'll be happy to follow up right away with the above-described
change.
>> Maybe that means we'll have to change some of the crawler code to
>> *treat* svn_depth_unknown like svn_depth_infinity, when recursing into
>> subdirs, but that's okay. The important thing is not to lie to the
>> server about what was requested.
>
> We don't, AFAICT.
You're right -- when I looked across all of adm_crawler.c, it's clear
always report the entry->depth, we just use 'depth' for governing
recursion behavior.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jun 4 21:38:06 2007