Vlad Georgescu <vgeorgescu@gmail.com> writes:
> This patch makes checkouts and updates at svn_depth_empty and
> svn_depth_immediates work with servers that do not understand the new
> reporter changes, by making the update editor ignore edits on paths below
> the edit depth.
>
> With this patch, I can successfully run the depth tests against a 1.4
> svnserve. As this is my first attempt at sparse-directories related work,
> I'm posting the patch here for review.
Great! (Sanity check: is there definitely a test in there that tests
the behavior we want?)
> [[[
> Make updates and checkouts at 'empty' or 'immediates' depth work with old
> servers.
>
> * subversion/include/svn_types.h
> (svn_depth_t): Remove comment about svn_depth_exclude being potentially
> unnecessary.
>
> * subversion/libsvn_ra_svn/client.c
> (ra_svn_update): Don't use the SVN_DEPTH_TO_RECURSE macro, because we
> don't want svn_depth_immediates mapped to FALSE in this case. Use a
> custom check instead.
>
> * subversion/libsvn_wc/update_editor.c
> (dir_baton): New member 'depth'.
> (file_baton.skipped): Update comment to reflect new usage.
> (prep_directory): Remove 'depth' parameter, it is stored in the dir_baton
> now. Use db->depth instead of depth when creating the administrative area.
> (open_root): Initialize the returned dir_baton's depth to the default
> depth of the edit.
> (delete_entry): For svn_depth_empty and svn_depth_files, ignore deletions
> of children that haven't been explicitly brought in.
> (add_directory): If the directory is the target of the edit, set its
> depth to that of the edit. Otherwise, initialize the returned dir_baton's
> depth based on the depth of the parent. Remove the old depth calculation
> code, and update the call to prep_directory.
> (open_directory): If the directory is the target of the edit, set its
> depth to that of the edit. Otherwise, initialize the returned dir_baton's
> depth based on the depth of the parent. If the entry exists on disk, allow
> its depth to override svn_depth_infinity.
> (change_dir_prop, close_directory): Don't do anything if the directory is
> at svn_depth_exclude.
> (absent_file_or_dir): Don't do anything if the parent is empty or excluded,
> or if it is at svn_depth_files and the absent entry is a directory.
> (add_or_open_file): If parent is at svn_depth_empty or svn_depth_exclude,
> skip this file.
> ]]]
*nod* This all looks like the right approach to me.
> Index: subversion/libsvn_ra_svn/client.c
> ===================================================================
> --- subversion/libsvn_ra_svn/client.c (revision 24077)
> +++ subversion/libsvn_ra_svn/client.c (working copy)
> @@ -1079,8 +1079,14 @@
> {
> svn_ra_svn__session_baton_t *sess_baton = session->priv;
> svn_ra_svn_conn_t *conn = sess_baton->conn;
> - svn_boolean_t recurse = SVN_DEPTH_TO_RECURSE(depth);
> + svn_boolean_t recurse;
>
> + /* Old servers know "recursive" but not "depth"; help them DTRT. */
> + if (depth == svn_depth_empty || depth == svn_depth_files)
> + recurse = FALSE;
> + else
> + recurse = TRUE;
> +
> /* Tell the server we want to start an update. */
> SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "update", "(?r)cbw", rev, target,
> recurse, svn_depth_to_word(depth)));
This raises an interesting question: should SVN_DEPTH_TO_RECURSE()
itself change to the behavior you have above? I surveyed its callers:
it looks like approximately half could use (or could easily be changed
to use) the behavior you wrote above, and about half need the current
behavior of SVN_DEPTH_TO_RECURSE().
I'm not sure what to recommend here, I just wanted to raise this
issue. If you have time to take a look at that as a separate change,
even better.
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 24077)
> +++ subversion/libsvn_wc/update_editor.c (working copy)
> @@ -1135,6 +1157,26 @@
> || ((! copyfrom_path) && (SVN_IS_VALID_REVNUM(copyfrom_revision))))
> abort();
>
> + *child_baton = db;
> +
> + /* If we have a non-empty target and the parent of the newly added
> + directory is the root of the edit, then the new directory *is* the
> + target and we should set its depth accordingly. */
> + if (*eb->target && ! pb->parent_baton)
> + db->depth = eb->depth;
> + else if (pb->depth == svn_depth_empty
> + || pb->depth == svn_depth_exclude
> + || pb->depth == svn_depth_files)
> + {
> + db->depth = svn_depth_exclude;
> + db->bump_info->skipped = TRUE;
> + return SVN_NO_ERROR;
> + }
> + else if (pb->depth == svn_depth_immediates)
> + db->depth = svn_depth_empty;
> + else
> + db->depth = svn_depth_infinity;
> +
> SVN_ERR(svn_io_check_path(db->path, &kind, db->pool));
I feel silly saying this, but if one clause has braces, we usually
make sure they all do. I personally don't mind either way, but it
helps make the code more readable for a lot of people.
The logic here looks good to me: it's exactly the depth-shortening
effect I'd expected to see as we walk down the directory hierarchy.
> @@ -1350,6 +1379,26 @@
>
> /* Skip this directory if it has property conflicts. */
> SVN_ERR(svn_wc_entry(&entry, db->path, eb->adm_access, FALSE, pool));
> +
> + /* If we have a non-empty target and the parent of the newly added
> + directory is the root of the edit, then the new directory *is* the
> + target and we should set its depth accordingly. */
> + if (*eb->target && ! pb->parent_baton)
> + db->depth = eb->depth;
> + else if (pb->depth == svn_depth_empty
> + || pb->depth == svn_depth_files
> + || pb->depth == svn_depth_exclude)
> + {
> + db->depth = svn_depth_exclude;
> + db->bump_info->skipped = TRUE;
> + return SVN_NO_ERROR;
> + }
> + else if (pb->depth == svn_depth_immediates)
> + db->depth = svn_depth_empty;
> + else if (entry)
> + /* Allow the entry's actual depth to override svn_depth_infinity. */
> + db->depth = entry->depth;
> +
Ah, okay, I think I understand: if the operation's requested depth is
not svn_depth_infinity, then an entry->depth can't override it --
after all, if this were a new server, it wouldn't give us the
information we'd need to descend anyway. It's only because of the
old-server case that we have to ignore content coming down the pipe.
But if svn_depth_infinity, then we're getting exactly what we want
from the pipe, and the question is: what depth is the wc using?
Is that a fair summary of the reasoning here?
(Just trying to make sure I understand the code.)
> @@ -1451,6 +1500,12 @@
> apr_array_header_t *entry_props, *wc_props, *regular_props;
> svn_wc_adm_access_t *adm_access;
>
> + if (db->depth == svn_depth_exclude)
> + {
> + SVN_ERR(maybe_bump_dir_info(db->edit_baton, db->bump_info, db->pool));
> + return SVN_NO_ERROR;
> + }
> +
> SVN_ERR(svn_categorize_props(db->propchanges, &entry_props, &wc_props,
> ®ular_props, pool));
>
Hmmm. This is in close_directory(). Are you sure you didn't mean
svn_depth_empty intead of svn_depth_exclude?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Mar 28 09:00:59 2007