You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@wandisco.com> on 2010/03/10 12:32:02 UTC
Re: svn commit: r920023 -
/subversion/trunk/subversion/libsvn_wc/update_editor.c
On Sun, 2010-03-07, rhuijben@apache.org wrote:
> Author: rhuijben
> Date: Sun Mar 7 16:10:25 2010
> New Revision: 920023
>
> URL: http://svn.apache.org/viewvc?rev=920023&view=rev
> Log:
> Prepare the update editor for more direct WC-DB usage by making it use
> repository relative paths for its calculations.
I reviewed all of this change and it looks good. Just one question...
> * subversion/libsvn_wc/update_editor.c
> (edit_baton): Rename variable.
> (dir_baton): Rename variable.
> (node_get_url_ignore_errors): Rename to ...
> (node_get_relpath_ignore_errors): ... this and use some wc_db apis
> instead of svn_wc__node helpers.
> (make_dir_baton): Calculate relative path instead of full uri.
> (file_baton): Rename variable.
> (make_file_baton): Calculate relative path instead of full uri.
> (open_root): Fetch full uri for entry operation.
> (check_tree_conflict): Take relative path instead of url.
> (do_entry_deletion): Accept relative path and calculate full url
> if re-adding a node via its entry.
> (delete_entry): Construct relative path.
> (add_directory, open_directory, add_file,
> open_file): Create full urls for error messages and update calls
> to functions that changed to relative paths.
> (loggy_tweak_base_node): Add repos_root argument.
> (merge_file): Update caller.
> (close_edit): Construct full url.
> (make_editor): Assert on repository_root and construct switch
> relative path.
>
> Modified:
> subversion/trunk/subversion/libsvn_wc/update_editor.c
>
> Modified: subversion/trunk/subversion/libsvn_wc/update_editor.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/update_editor.c?rev=920023&r1=920022&r2=920023&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/update_editor.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/update_editor.c Sun Mar 7 16:10:25 2010
[...]
> @@ -2887,10 +2913,11 @@
>
> tmp_entry.revision = *(eb->target_revision);
>
> - if (eb->switch_url)
> + if (eb->switch_relpath)
> {
> - tmp_entry.url = svn_path_url_add_component2(eb->switch_url,
> - db->name, pool);
> + tmp_entry.url = svn_path_url_add_component2(eb->repos,
> + db->new_relpath,
> + pool);
> modify_flags |= SVN_WC__ENTRY_MODIFY_URL;
> }
Is that definitely the same as the straightforward conversion which
would be to join (eb->repos, eb->switch_relpath, db->name)?
I can't figure out whether the code that generates db->new_relpath (in
make_dir_baton()) will always have the same result. I'm trying it out
by constructing both and comparing them with an assertion, but I can't
be sure that all the switch scenarios actually get tested in the test
suite.
On the other hand, maybe the new "join(repos, new_relpath)" is more
correct than what was there before.
- Julian
Re: svn commit: r920023 -
/subversion/trunk/subversion/libsvn_wc/update_editor.c
Posted by Julian Foad <ju...@wandisco.com>.
Greg Stein wrote:
> On Wed, Mar 10, 2010 at 07:32, Julian Foad <ju...@wandisco.com> wrote:
> >...
> >> @@ -2887,10 +2913,11 @@
> >>
> >> tmp_entry.revision = *(eb->target_revision);
> >>
> >> - if (eb->switch_url)
> >> + if (eb->switch_relpath)
> >> {
> >> - tmp_entry.url = svn_path_url_add_component2(eb->switch_url,
> >> - db->name, pool);
> >> + tmp_entry.url = svn_path_url_add_component2(eb->repos,
> >> + db->new_relpath,
> >> + pool);
> >> modify_flags |= SVN_WC__ENTRY_MODIFY_URL;
> >> }
> >
> > Is that definitely the same as the straightforward conversion which
> > would be to join (eb->repos, eb->switch_relpath, db->name)?
> >
> > I can't figure out whether the code that generates db->new_relpath (in
> > make_dir_baton()) will always have the same result. I'm trying it out
> > by constructing both and comparing them with an assertion, but I can't
> > be sure that all the switch scenarios actually get tested in the test
> > suite.
> >
> > On the other hand, maybe the new "join(repos, new_relpath)" is more
> > correct than what was there before.
>
> Conceptually, yes it is better. We always want to identify items in a
> repository with a <root, relpath> tuple. Consistently.
And correctness-wise, I'm happy now that we've had Bert's and your eyes
on it, and I successfully ran the test suite while asserting that it
produces the same result as join(eb->repos, eb->switch_relpath,
db->name).
> Within wc_db (and hopefully percolating out to libsvn_wc and further),
> we also carry around the repository's UUID, so you'll see lots of
> 3-tuples of <root, relpath, uuid>. And in some cases, we need the
> revision (like with original_*), so you get a 4-tuple.
>
> Even though we do not allow variant repositories within the "same"
> working copy, the wc_db allows for it and should be coded that way. We
> should continue to push those concepts further out. Today, working
> copy subtrees from a separate repository are *distinct* working
> copies. The subtree is labeled as a "working copy root" (per
> svn_wc__check_wc_root). In the future, we can stitch these together
> into one big working copy.
OK.
- Julian
Re: svn commit: r920023 - /subversion/trunk/subversion/libsvn_wc/update_editor.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Mar 10, 2010 at 07:32, Julian Foad <ju...@wandisco.com> wrote:
>...
>> @@ -2887,10 +2913,11 @@
>>
>> tmp_entry.revision = *(eb->target_revision);
>>
>> - if (eb->switch_url)
>> + if (eb->switch_relpath)
>> {
>> - tmp_entry.url = svn_path_url_add_component2(eb->switch_url,
>> - db->name, pool);
>> + tmp_entry.url = svn_path_url_add_component2(eb->repos,
>> + db->new_relpath,
>> + pool);
>> modify_flags |= SVN_WC__ENTRY_MODIFY_URL;
>> }
>
> Is that definitely the same as the straightforward conversion which
> would be to join (eb->repos, eb->switch_relpath, db->name)?
>
> I can't figure out whether the code that generates db->new_relpath (in
> make_dir_baton()) will always have the same result. I'm trying it out
> by constructing both and comparing them with an assertion, but I can't
> be sure that all the switch scenarios actually get tested in the test
> suite.
>
> On the other hand, maybe the new "join(repos, new_relpath)" is more
> correct than what was there before.
Conceptually, yes it is better. We always want to identify items in a
repository with a <root, relpath> tuple. Consistently.
Within wc_db (and hopefully percolating out to libsvn_wc and further),
we also carry around the repository's UUID, so you'll see lots of
3-tuples of <root, relpath, uuid>. And in some cases, we need the
revision (like with original_*), so you get a 4-tuple.
Even though we do not allow variant repositories within the "same"
working copy, the wc_db allows for it and should be coded that way. We
should continue to push those concepts further out. Today, working
copy subtrees from a separate repository are *distinct* working
copies. The subtree is labeled as a "working copy root" (per
svn_wc__check_wc_root). In the future, we can stitch these together
into one big working copy.
Cheers,
-g