You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bill Tutt <ra...@lyra.org> on 2002/11/24 02:17:31 UTC
[PATCH] Fix for 1003, and URL stability
For a long time now libsvn_fs/tree.c could not correctly resolve out the
laziness inherent in our O(1) copy model. That is no longer true. This
patch allows mod_dav_svn to report the correct "NodeRevision first
visible on this path at revision number X". This is accomplished by the
following changes:
* Record non-root bubble up changes in the changes table.
* svn_fs_node_created_rev now reports the "first visible on this path"
created revision number.
* svn_fs_dir_entries() now includes the new created_rev in the
svn_fs_dirent_t structure.
* svn_fs_revisions_changed() now stops immediately if cross_copy_history
is false.
There are a couple of follow on tweaks that make the code more efficient
(by making the expensive lazy detection optional to open_path callers),
but I'd like to get this first chunk of changes in first.
All of the C FS tests (including the three new ones) pass as of 3870
with this patch applied.
* include/svn_fs.h
(svn_fs_dirent_t) Add created_rev that sees through lazy copies and
is set to the first revision that a lazily copied node revision
appeared on the requested path.
* libsvn_fs/tree.c
(add_change): Move earlier in the file, so that more functions can
call it.
(parent_path_t): Add members last_branch_id, in_lazy_land, and
last_path.
(make_parent_path): Add new parameters for new parent_path_t
members.
(get_id_path): Move earlier in the file, so that more functions can
call it.
(is_child_lazily_copied): New function see through the laziness
of the tree.
(open_path): Keep track of last_branch_id so that it can be used in
parent_paths.
(choose_copy_id): Replace usage of svn_fs_compare_ids with a check
that is more restrictive then svn_fs_check_related.
(make_path_mutable): Ensure bubble up changes make it into the
changes table by calling add_change.
(txn_body_node_created_rev): Make use of information stored in
open_path's parent_path to return the correct created revision.
(txn_body_dir_entries): Fixup the DAG directory entries with the
correct created revision.
(revision_changed_args): Remove id and fs, and replace it with
path and root so txn_body_revisions_changed can call open_path.
(txn_body_revisions_changed): Make use of information from open_path
to correctly stop immediately if cross_copy_history is false
and the path in question only exists because of a copy.
(svn_fs_revisions_changed): Update with respect to
revision_changed_args changes.
* tests/libsvn_fs/fs-test.c:
(branch_test): Add validation code.
(lazy_copies_created_rev): New test case for svn_fs_node_created_rev
that tests the new ability of created_rev to differ based on
copies.
(lazy_copies_dir_entries): New test case for svn_fs_dir_entries
that tests the new ability of created_rev to differ based on
copies.
(lazy_copies_rev_changed): New test case for new !cross_copy_history
behavior in svn_fs_revisions_changed.
(test_funcs): Add new test cases: lazy_copies_created_rev,
lazy_copies_dir_entries, lazy_copies_rev_changed.
Bill
----
Do you want a dangerous fugitive staying in your flat?
No.
Well, don't upset him and he'll be a nice fugitive staying in your flat.
Re: [PATCH] Fix for 1003, and URL stability
Posted by cm...@collab.net.
"Bill Tutt" <ra...@lyra.org> writes:
> For a long time now libsvn_fs/tree.c could not correctly resolve out the
> laziness inherent in our O(1) copy model. That is no longer true. This
> patch allows mod_dav_svn to report the correct "NodeRevision first
> visible on this path at revision number X". This is accomplished by the
> following changes:
> * Record non-root bubble up changes in the changes table.
> * svn_fs_node_created_rev now reports the "first visible on this path"
> created revision number.
> * svn_fs_dir_entries() now includes the new created_rev in the
> svn_fs_dirent_t structure.
> * svn_fs_revisions_changed() now stops immediately if cross_copy_history
> is false.
Hmm... a couple of initial formatting annoyances with this patch. You
are using "foo()" instead of "foo ()", which is inconsistent with the
rest of tree.c. Also, your indentation differs from the rest of the
code in places.
Another concern was mentioned by Ben, and I think I echo the same
concern. Namely, are the additional `changes' table items for
bubble-up directories a necessity or an optimization? If they are an
optimization, I'd like to discuss whether that optimization outweighs
the costs: a one-time dump/load cycle requirement, but then a
tremendous bloat in the size of the changes table itself.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for 1003, and URL stability
Posted by cm...@collab.net.
"Bill Tutt" <ra...@lyra.org> writes:
> For a long time now libsvn_fs/tree.c could not correctly resolve out the
> laziness inherent in our O(1) copy model. That is no longer true. This
> patch allows mod_dav_svn to report the correct "NodeRevision first
> visible on this path at revision number X". This is accomplished by the
> following changes:
> * Record non-root bubble up changes in the changes table.
> * svn_fs_node_created_rev now reports the "first visible on this path"
> created revision number.
> * svn_fs_dir_entries() now includes the new created_rev in the
> svn_fs_dirent_t structure.
> * svn_fs_revisions_changed() now stops immediately if cross_copy_history
> is false.
I'll set about reviewing this patch today. As this change touches
some fairly elemental filesystem functions, and will necessitate a
dump/load of the repository, I really want to test the heck out of it.
Thanks for the submission, Bill.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for 1003, and URL stability
Posted by cm...@collab.net.
Karl Fogel <kf...@newton.ch.collab.net> writes:
> cmpilato@collab.net writes:
> > I think this change addresses the following old problem:
> >
> > Given a greek tree at revision 1, I move A (and all its younguns) to Z
> > (making revision 2). Mod_dav_svn talks about paths and revisions in
> > all its communications. If you ask the filesystem for the created
> > revision of the path /Z/D/G/rho, it will say "1" because /Z/D/G/rho
> > was a subtree item of a copied dir, and was not itself touched as part
> > of the copy. Of course, if you now refer to the path/rev tuple
> > (/Z/D/G/rho, 1), the filesystem will tell you that no such path
> > exists. That's wrong.
>
> Oh! Yah, that's a bug :-). I had thought this was how
> svn_fs_node_created_rev() behaved right now, but looking at its
> implementation, it clearly doesn't handle this case. Furthermore,
> it's not clear that its behavior should change. That is, in the
> scenario you describe above, we'd sometimes want created_rev() to
> return 1, and sometimes 2, depending on whether one is asking about
> the node itself or path by which it is accessed.
I think Bill's change simply makes 'created_rev' mean something more
like "path_created_rev" than "node_created_rev". I'm not sure if
anyone really does care about the "node_created_rev" ... that's part
of what I'm investigating while reviewing this patch.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for 1003, and URL stability
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
cmpilato@collab.net writes:
> I think this change addresses the following old problem:
>
> Given a greek tree at revision 1, I move A (and all its younguns) to Z
> (making revision 2). Mod_dav_svn talks about paths and revisions in
> all its communications. If you ask the filesystem for the created
> revision of the path /Z/D/G/rho, it will say "1" because /Z/D/G/rho
> was a subtree item of a copied dir, and was not itself touched as part
> of the copy. Of course, if you now refer to the path/rev tuple
> (/Z/D/G/rho, 1), the filesystem will tell you that no such path
> exists. That's wrong.
Oh! Yah, that's a bug :-). I had thought this was how
svn_fs_node_created_rev() behaved right now, but looking at its
implementation, it clearly doesn't handle this case. Furthermore,
it's not clear that its behavior should change. That is, in the
scenario you describe above, we'd sometimes want created_rev() to
return 1, and sometimes 2, depending on whether one is asking about
the node itself or path by which it is accessed.
Hmmm.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for 1003, and URL stability
Posted by cm...@collab.net.
Karl Fogel <kf...@newton.ch.collab.net> writes:
> "Bill Tutt" <ra...@lyra.org> writes:
> > For a long time now libsvn_fs/tree.c could not correctly resolve out the
> > laziness inherent in our O(1) copy model. That is no longer true. This
> > patch allows mod_dav_svn to report the correct "NodeRevision first
> > visible on this path at revision number X". This is accomplished by the
> > following changes:
> > * Record non-root bubble up changes in the changes table.
> > * svn_fs_node_created_rev now reports the "first visible on this path"
> > created revision number.
> > * svn_fs_dir_entries() now includes the new created_rev in the
> > svn_fs_dirent_t structure.
> > * svn_fs_revisions_changed() now stops immediately if cross_copy_history
> > is false.
>
> Bill, could you describe a concrete example scenario(s) affected by
> these changes? I'm having trouble understanding the motivation.
> Obviously, cmpilato understands it, but then, he was born in the
> filesystem :-).
I think this change addresses the following old problem:
Given a greek tree at revision 1, I move A (and all its younguns) to Z
(making revision 2). Mod_dav_svn talks about paths and revisions in
all its communications. If you ask the filesystem for the created
revision of the path /Z/D/G/rho, it will say "1" because /Z/D/G/rho
was a subtree item of a copied dir, and was not itself touched as part
of the copy. Of course, if you now refer to the path/rev tuple
(/Z/D/G/rho, 1), the filesystem will tell you that no such path
exists. That's wrong.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for 1003, and URL stability
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
"Bill Tutt" <ra...@lyra.org> writes:
> For a long time now libsvn_fs/tree.c could not correctly resolve out the
> laziness inherent in our O(1) copy model. That is no longer true. This
> patch allows mod_dav_svn to report the correct "NodeRevision first
> visible on this path at revision number X". This is accomplished by the
> following changes:
> * Record non-root bubble up changes in the changes table.
> * svn_fs_node_created_rev now reports the "first visible on this path"
> created revision number.
> * svn_fs_dir_entries() now includes the new created_rev in the
> svn_fs_dirent_t structure.
> * svn_fs_revisions_changed() now stops immediately if cross_copy_history
> is false.
Bill, could you describe a concrete example scenario(s) affected by
these changes? I'm having trouble understanding the motivation.
Obviously, cmpilato understands it, but then, he was born in the
filesystem :-).
Thanks,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org