Karl Fogel wrote:
> Vlad Georgescu <vgeorgescu@gmail.com> writes:
>> I finished implementing depth upgrades (with 1.5 servers) in the
>> attached patch. I can get the depth upgrade tests to work without
>> breaking anything else (tested over ra_svn and ra_local).
>>
>> I'm posting it here for review.
> [...]
(Snipped various suggestions about improving comments and clarity.
These have been noted and will be fixed in the next version of the
patch, or before I commit).
>> + if (requested_depth != svn_depth_files
>> + || ((! t_entry || t_entry->kind != svn_node_dir)
>> + && (! s_entry || s_entry->kind != svn_node_dir)))
>> + SVN_ERR(update_entry(b, s_rev, s_fullpath, s_entry, t_fullpath,
>> + t_entry, dir_baton, e_fullpath, info,
>> + info ? info->depth
>> + : DEPTH_BELOW_HERE(wc_depth),
>> + DEPTH_BELOW_HERE(requested_depth), subpool));
>
> 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.
>> + /* svn_depth_unknown is just like svn_depth_infinity, as far as
>> + reporting is concerned. */
>> + if (depth == svn_depth_unknown)
>> + depth = svn_depth_infinity;
>> +
>> if ((! entry) || ((entry->schedule == svn_wc_schedule_add)
>> && (entry->kind == svn_node_dir)))
>> {
>
> Okay, this is the one thing that still bothers me.
>
> The depth passed in to svn_wc_crawl_revisions3() is the *requested*
> depth, right? (We haven't yet documented that function's 'depth'
> parameter yet, unfortunately, but I'm pretty sure it's requested
> depth.)
Yup, it's the requested depth.
> About thirteen lines after the above, we pass that depth to
> set_path() as what seems to be a wc depth:
>
> SVN_ERR(reporter->set_path(report_baton, "", base_rev, depth,
> entry ? entry->incomplete : TRUE,
> NULL, pool));
>
> Now in this particular call to set_path(), it's not a problem, because
> there's no versioned path to crawl anyway.
Yes, there's no versioned path, so the depth at which it is brought in
should be the requested depth.
> [...]
> So even though it's a little confusing to someone reading the code,
> maybe it's okay that we passed a requested depth (instead of a wc
> depth) to set_path(), since there's no wc depth available. But let's
> look at the other place 'depth' is used in svn_wc_crawl_revisions3():
>
> if (entry->kind == svn_node_dir)
> {
> if (missing)
> {
> /* Always report directories as missing; we can't recreate
> them locally. */
> err = reporter->delete_path(report_baton, "", pool);
> if (err)
> goto abort_report;
> }
> 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).
> 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.
--
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jun 4 09:52:29 2007