Confused by the ambient depth logic in the update editor

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