Blair Zajac <blair@orcaware.com> writes:
> Hi Karl,
Heh, I think everyone thinks I wrote the depth code and am the only
one who understands it... but it's not true :-). Vlad knows it as
well as I do now; others may as well.
(Not that I mind being asked, of course, I just want to point out that
I'm not the only expert here.)
> In r23994, the comment for svn_client__checkout_internal says this in
> libsvn_client/client.h:
>
> DEPTH must be a definite depth, not (e.g.) svn_depth_unknown.
>
> but in the code itself, it does this:
>
> SVN_ERR(svn_wc_check_wc(path, &wc_format, pool));
> if (! wc_format)
> {
> initialize_area:
>
> if (depth == svn_depth_unknown)
> depth = svn_depth_infinity;
>
> Not that this a big deal, but it would be nice to have it be consistent.
Thanks! You've found a deeper inconsistency, I think.
The only caller who could possibly pass 'svn_depth_unknown' appears to
be svn_client_checkout3(), whose doc string says:
* If @a depth is @c svn_depth_unknown, then behave as if for
* @c svn_depth_infinity, except in the case of resuming a previous
* checkout of @a path (i.e., updating), in which case use the depth
* of the existing working copy.
That's essentially the "else" case of the 'if (! wc_format) {...}'
above. As you might expect, the core of that "else" case is to call
svn_client__update_internal(..., depth, ...)
using the depth originally passed to svn_client__checkout_internal().
We don't check to see if it's 'svn_depth_unknown' or whatever, we just
pass it straight along.
Assume, for a moment, that it is 'svn_depth_unknown'. Here's what
svn_client__update_internal()'s doc string says about that:
* If DEPTH is svn_depth_unknown, then use whatever depth is already
* set for PATH, or @c svn_depth_infinity if PATH does not exist.
Well, does svn_client__update_internal() do what it promises?
Apparently not. Near the top of its body, it has this:
/* ### TODO(sd): Ah, the irony. We'd like to base our adm_open depth on
### the depth we're going to use for the update. But that
### may depend on the depth in the working copy, which we can't
### discover without calling adm_open. We could expend an extra
### call, with adm_open_depth=0, to get the real depth (but only
### if we need to) and then make the real call... but it's not
### worth the complexity right now. Locking the entire tree when
### we didn't need to is a performance hit, but (except for
### access contention) not a correctness problem. */
if (depth == svn_depth_empty
|| depth == svn_depth_files)
adm_open_depth = 0;
else
adm_open_depth = -1;
But even though it claims it might grok the depth from the working
copy, it never actually does, as far as I can tell.
Conclusion:
Tentatively, I think svn_client__checkout_internal() *should* accept
'svn_depth_unknown', and match svn_client_checkout3()'s behavior:
* If @a depth is @c svn_depth_unknown, then behave as if for
* @c svn_depth_infinity, except in the case of resuming a previous
* checkout of @a path (i.e., updating), in which case use the depth
* of the existing working copy.
However, first svn_client__update_internal() needs to actually grok
the existing working copy depth, for the resuming-a-checkout case.
Here's why: if someone resumes a checkout, they might assume that the
partial working copy will remember the depth the tree is supposed to
have -- and if they don't pass an explicit depth, that's equivalent to
'svn_depth_unknown'. svn_client__update_internal() is what gets
invoked from the checkout code when resuming an interrupted checkout,
so it needs to DTRT with 'svn_depth_unknown' and grok the depth from
the working copy.
I'm writing this long message because I'm deep in the commit code and
probably will not start on this until I'm done with that. However,
maybe someone will read this message and try to fix this. I left out
some details, but they would be quickly encountered from walking
through the code.
(r23994 is just the big merge of the sparse-directories branch, by the
way; the relevant commits happened earlier, but I'm not sure it's
worth tracing them down now, we might as well just deal with what's
before us.)
Thanks for pointing out the inconsistency,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jul 16 09:54:19 2007