Re: Confused by the ambient depth logic in the update editor

On 10/15/07, Karl Fogel <kfogel@red-bean.com> wrote:
> 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.

Definitely, thanks. I have a bit of non-Subversion stuff on my plate
this week (gvn, specifically), but should get to this before the end
of SubConf, as well as trying to move the ambient stuff to a wrapper
to reuse it for diff and status.

> "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.

I'll take a look. I also think that the code
      else if (entry)
        d->ambient_depth = entry->depth;
      else
        d->ambient_depth = svn_depth_unknown;
is redundant (it gets wiped out a few lines later).

> > 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;

Aha, open_directory! Gotcha.

> 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?

Seems reasonable, I'll give it a shot.

> (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*

Thanks for the explanation. I should be able to take this on.

--dave

-- 
David Glasser | glasser_at_davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org