Re: [PATCH] Make sparse-directories checkouts/updates work with old servers

Karl Fogel wrote:
...
>> 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.

We could:
a) introduce a new macro, or
b) update all APIs which currently take a boolean recurse parameter, and
which depend on the current behavior of SVN_DEPTH_TO_RECURSE, by making
them take a svn_depth_t parameter. After that, we could change
SVN_DEPTH_TO_RECURSE to the new behavior.

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

Sure, I'll make these changes in the next iteration of the patch, or
before I commit.

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

I wanted to get the same behavior from old servers that we get from new
servers. From my (probably inaccurate) understanding of the code, I
thought that if we have a svn_depth_infinity update and we hit a
directory that is at a non-infinity depth, we don't recurse any further.
 Is this what currently happens?

For example, what happens (or should happen) if we do a depth=infinity
update on a depth=empty wc? Do we:

a) do a depth=empty update
b) bring in everything that's missing into the wc, effectively making it
depth=infinity
c) update only those files and directories in the wc that have been
brought in explicitly

notes/sparse-directories.txt says b), but I don't think that's what the
code in trunk/ currently does.

>
>> @@ -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,
>> &regular_props, pool));
>>
>
> Hmmm. This is in close_directory(). Are you sure you didn't mean
> svn_depth_empty intead of svn_depth_exclude?

My intention was to make close_directory() a no-op for excluded (i.e
ignored) directories (the call to maybe_bump_dir_info is still necessary
to update a reference count). Empty directories still need the full
treatment.

-- 
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org