Thanks for the review, David! I won't be able to respond in as much
depth -- uh, please pardon the expression -- as I'd like to, because
of limited online time (am at the Subversion Conference in Germany
right now, subconf.com). But I'll give some initial responses, and
take a more detailed look as soon as I can.
"David Glasser" <glasser@davidglasser.net> writes:
> Three issues. First off, the first big block is essentially calling
> svn_wc_entry and deciding to "exclude" if it throws
> SVN_ERR_ENTRY_NOT_FOUND... but svn_wc_entry doesn't throw errors for
> entry not found, it just returns null, and here we're responding to a
> null entry by setting ambient_depth to unknown.
When I debugged the original issue, it was throwing an error, and
adding this code fixed it. But you raise a good point. It could be
that my fix is working for some other reason; the thing to do is run
the reproduction recipe in GDB and trace through that code block.
> Secondly, after all that effort to get ambient depth stuff from the
> parent, we go into this next set of conditionals, which as far as I
> can tell totally overwrites whatever was calculated above. Should
> only one of these pieces of logic be there or something? And what
> does the "For opened directories, we'll get the real depth later"
> refer to?
That comment is referring to this line in open_directory():
db->ambient_depth = entry->depth;
Here's what's going on:
Formerly, make_dir_baton() did not normally fetch the entry, and
rather than have make_dir_baton() do so unconditionally,
open_directory() would fill in dir_baton's ambient depth later (since
open_directory() needed to fetch the entry for other reasons anyway).
After r27031, make_dir_baton() becomes somewhat more likely to fetch
the entry, although it still isn't guaranteed to do so. In the case
where we do fetch it now, though, I thought "Well, as long as we have
the ambient depth at hand, and we haven't returned already, we might
as well set d->ambient_depth."
So the line in open_directory() is still correct, but in some cases it
may be redundant, because db->ambient_depth will have already been
set. Thus the code as it stands is a bit confusing. One solution
would be to make that line in open_directory() conditional: if the
construction of db hasn't set a specific depth already, then set it
now. Another solution would be to not set depth in make_dir_baton()
if we're just going to set it to the entry->ambient_depth anyway,
since open_directory() will do it for us later.
I think the first solution is probably cleaner, since it creates a
sensible communications channel between make_dir_baton() and
open_directory(), basically saying that the latter should only do
something if the former hasn't done it yet. Thoughts?
(I remember noticing this disagreement between make_dir_baton() and
open_directory() at the time of r27031, but I got distracted by
something and forgot to go all the way and clean it up. Mea culpa.)
> Third, I'm not really sure what the logic inside the "if (added)" is
> really trying to figure out. Like, what if path is deeper than
> eb->target (so we don't take the first clause), but requested_depth is
> svn_depth_files or svn_depth_empty? Won't we take the third clause
> and set d->ambient_depth to svn_depth_infinity? Or is this impossible
> because the depth filter delta editor won't let us get such an open in
> the first place?
I think we wouldn't get such an open in the first place, for the
reasons you stated. But if we *did* -- that is, if we don't take the
first clause, but eb->requested_depth is svn_depth_files or
svn_depth_empty, and we're adding a directory at all -- then we should
add it at svn_depth_infinity.
I agree with you, though: those two conditional blocks should be
better coordinated with each other. Feel free to hack away at it,
else I will when I get back.
> Also, make_file_baton has the same issue about expecting an error from
> svn_wc_entry.
*nod*
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Oct 16 08:31:04 2007