Confused by the ambient depth logic in the update editor

[David Glasser <gl...@davidglasser.net>]

(I guess this is review for kfogel's r27031.)

So I'm looking through the ambient depth logic in
libsvn_wc/update_editor.c, and I'm confused by a few things.  First
off, let's look at make_dir_baton:

<<<<
  if (eb->requested_depth == svn_depth_unknown
      && pb
      && (pb->ambient_depth == svn_depth_empty
          || pb->ambient_depth == svn_depth_files))
    {
      /* This is not a depth upgrade, and the parent directory is
         depth==empty or depth==files.  So if the parent doesn't
         already have an entry for the new dir, then the parent
         doesn't want the new dir at all, thus we should initialize
         it at svn_depth_exclude. */
      svn_error_t *err;
      const svn_wc_entry_t *entry;
      svn_wc_adm_access_t *adm_access;

      err = svn_wc_adm_retrieve(&adm_access, eb->adm_access, d->path, pool);
      if (err &&
          (err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND
           || err->apr_err == SVN_ERR_WC_NOT_LOCKED))
        {
          svn_error_clear(err);
          d->ambient_depth = svn_depth_exclude;
          *d_p = d;
          return SVN_NO_ERROR;
        }

      err = svn_wc_entry(&entry, d->path, adm_access, FALSE, pool);
      if (err && err->apr_err == SVN_ERR_ENTRY_NOT_FOUND)
        {
          svn_error_clear(err);
          d->ambient_depth = svn_depth_exclude;
          *d_p = d;
          return SVN_NO_ERROR;
        }
      else if (err)
        return err;
      else if (entry)
        d->ambient_depth = entry->depth;
      else
        d->ambient_depth = svn_depth_unknown;
    }

  if (added)
    {
      if (strcmp(eb->target, path) == 0)
        {
          /* The target of the edit is being added, give it the requested
             depth of the edit (but convert svn_depth_unknown to
             svn_depth_infinity). */
          d->ambient_depth = (eb->requested_depth == svn_depth_unknown)
            ? svn_depth_infinity : eb->requested_depth;
        }
      else if (eb->requested_depth == svn_depth_immediates
               || (eb->requested_depth == svn_depth_unknown
                   && pb->ambient_depth == svn_depth_immediates))
        {
          d->ambient_depth = svn_depth_empty;
        }
      else
        {
          d->ambient_depth = svn_depth_infinity;
        }
    }
  else
    {
      /* For opened directories, we'll get the real depth later. */
      d->ambient_depth = svn_depth_unknown;
    }
>>>>

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.

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?

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?

Also, make_file_baton has the same issue about expecting an error from
svn_wc_entry.

On the bright side, only one failing depth test left with svnserve 1.4
:)

--dave

-- 
David Glasser | glasser@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

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

[David Glasser <gl...@davidglasser.net>]

On 10/15/07, Karl Fogel <kf...@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" <gl...@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@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

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

[Karl Fogel <kf...@red-bean.com>]

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