Vlad, I think this patch is still outstanding -- were you planning to
post another iteration? (It sounded like you were, if not let me know.)
To answer your question:
> 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.
I'm pretty sure (b) is correct; if trunk needs a bugfix, then trunk
needs a bugfix, but let's not let that stand in the way of your patch.
-K
Vlad Georgescu <vgeorgescu@gmail.com> writes:
> 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,
>>> ®ular_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.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Apr 22 05:14:04 2007