You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ivan Zhakov <iv...@visualsvn.com> on 2013/09/25 10:02:50 UTC
Re: svn commit: r1526057 - in /subversion/trunk/subversion:
include/svn_fs.h libsvn_fs_fs/fs.h libsvn_fs_fs/tree.c libsvn_fs_fs/util.c libsvn_fs_fs/util.h
On 25 September 2013 04:09, <st...@apache.org> wrote:
> Author: stefan2
> Date: Wed Sep 25 00:09:06 2013
> New Revision: 1526057
>
> URL: http://svn.apache.org/r1526057
> Log:
> Make native svn_fs_move support in FSFS dependent on a format bump
> (tweak the conditional manually for testing). Fall back to ordinary
> copy-with-history for older format.
>
> Also, relax the condition on svn_fs_move source revision. If a rev
> older than the txn's base revision is specified, there must have been
> no changes at all to that node in meantime and neither the node nor
> nor any of the parents been deleted (and later restored).
>
[...]
Hi, Stefan. See my comment inline.
> Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1526057&r1=1526056&r2=1526057&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Wed Sep 25 00:09:06 2013
> @@ -60,6 +60,7 @@
> #include "pack.h"
> #include "temp_serializer.h"
> #include "transaction.h"
> +#include "util.h"
>
> #include "private/svn_mergeinfo_private.h"
> #include "private/svn_subr_private.h"
> @@ -2399,6 +2400,55 @@ typedef enum copy_type_t
> copy_type_move
> } copy_type_t;
>
> +/* Set CHANGES to TRUE if PATH in ROOT is unchanged in REVISION if the
> + same files system. If the content is identical, parent path copies and
> + deletions still count as changes. Use POOL for temporary allocations.
> + Not that we will return an error if PATH does not exist in ROOT or
> + REVISION- */
> +static svn_error_t *
> +is_changed_node(svn_boolean_t *changed,
> + svn_fs_root_t *root,
> + const char *path,
> + svn_revnum_t revision,
> + apr_pool_t *pool)
> +{
> + dag_node_t *node, *rev_node;
> + svn_fs_root_t *rev_root;
> + svn_fs_root_t *copy_from_root1, *copy_from_root2;
> + const char *copy_from_path1, *copy_from_path2;
> +
> + *changed = TRUE;
> +
> + SVN_ERR(svn_fs_fs__revision_root(&rev_root, root->fs, revision, pool));
> +
> + /* Get the NODE for FROM_PATH in FROM_ROOT.*/
> + SVN_ERR(get_dag(&node, root, path, TRUE, pool));
> + SVN_ERR(get_dag(&rev_node, rev_root, path, TRUE, pool));
> +
> + /* different ID -> got changed*/
> + if (!svn_fs_fs__id_eq(svn_fs_fs__dag_get_id(node),
> + svn_fs_fs__dag_get_id(rev_node)))
> + return SVN_NO_ERROR;
> +
> + /* some node. might still be a lazy copy */
> + SVN_ERR(svn_fs_closest_copy(©_from_root1, ©_from_path1, root,
> + path, pool));
> + SVN_ERR(svn_fs_closest_copy(©_from_root2, ©_from_path2, rev_root,
> + path, pool));
> +
> + if ((copy_from_root1 == NULL) != (copy_from_root2 == NULL))
> + return SVN_NO_ERROR;
> +
> + if (copy_from_root1)
> + if ( copy_from_root1->rev != copy_from_root2->rev
> + || strcmp(copy_from_path1, copy_from_path2))
> + return SVN_NO_ERROR;
> +
> + *changed = FALSE;
> + return SVN_NO_ERROR;
> +}
I didn't check that is_changed_node() function doing right thing, but
I suggest to refactor is_changed_node() a bit (see attached patch).
Final code will be:
[[[[
static svn_error_t *
is_changed_node(svn_boolean_t *changed,
svn_fs_root_t *root,
const char *path,
svn_revnum_t revision,
apr_pool_t *pool)
{
dag_node_t *node, *rev_node;
svn_fs_root_t *rev_root;
svn_fs_root_t *copy_from_root1, *copy_from_root2;
const char *copy_from_path1, *copy_from_path2;
SVN_ERR(svn_fs_fs__revision_root(&rev_root, root->fs, revision, pool));
/* Get the NODE for FROM_PATH in FROM_ROOT.*/
SVN_ERR(get_dag(&node, root, path, TRUE, pool));
SVN_ERR(get_dag(&rev_node, rev_root, path, TRUE, pool));
/* different ID -> got changed*/
if (!svn_fs_fs__id_eq(svn_fs_fs__dag_get_id(node),
svn_fs_fs__dag_get_id(rev_node)))
{
*changed = TRUE;
return SVN_NO_ERROR;
}
/* some node. might still be a lazy copy */
SVN_ERR(fs_closest_copy(©_from_root1, ©_from_path1, root,
path, pool));
SVN_ERR(fs_closest_copy(©_from_root2, ©_from_path2, rev_root,
path, pool));
if (copy_from_root1 == NULL && copy_from_root2 == NULL)
{
*changed = FALSE;
return SVN_NO_ERROR;
}
else if (copy_from_root1 != NULL && copy_from_root2 != NULL)
{
*changed = (copy_from_root1->rev != copy_from_root2->rev)
|| strcmp(copy_from_path1, copy_from_path2);
return SVN_NO_ERROR;
}
else
{
*changed = TRUE;
return SVN_NO_ERROR;
}
}
]]]
I think it makes behavior more clear. I cannot commit it because there
is no tests for this functionality. Could you please add them.
> +
> +
> /* Copy the node at FROM_PATH under FROM_ROOT to TO_PATH under
> TO_ROOT. COPY_TYPE determines whether then the copy is recorded in
> the copies table and whether it is being marked as a move.
> @@ -2436,10 +2486,33 @@ copy_helper(svn_fs_root_t *from_root,
> (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> _("Copy immutable tree not supported"));
>
> - if (copy_type == copy_type_move && from_root->rev != txn_id->revision)
> - return svn_error_create
> - (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> - _("Move from non-HEAD revision not currently supported"));
> + /* move support comes with a number of conditions ... */
> + if (copy_type == copy_type_move)
> + {
> + /* if we don't copy from the TXN's base rev, check that the path has
> + not been touched in that revision range */
> + if (from_root->rev != txn_id->revision)
> + {
> + svn_boolean_t changed = TRUE;
> + svn_error_t *err = is_changed_node(&changed, from_root,
> + from_path, txn_id->revision,
> + pool);
> + if (err || changed)
> + {
> + svn_error_clear(err);
Converting *any* error to SVN_ERR_FS_OUT_OF_DATE looks wrong for me.
For example SVN_FS_CORRUPTED error returned from is_changed_node()
will be also cleared and converted SVN_ERR_FS_OUT_OF_DATE. I think
this should be changed.
> + return svn_error_create(SVN_ERR_FS_OUT_OF_DATE, NULL,
> + _("Move-from node is out-of-date"));
> + }
> +
> + /* always move from the txn's base rev */
> + SVN_ERR(svn_fs_fs__revision_root(&from_root, from_root->fs,
> + txn_id->revision, pool));
> + }
> +
> + /* does the FS support moves at all? */
> + if (!svn_fs_fs__supports_move(to_root->fs))
> + copy_type = copy_type_add_with_history;
> + }
>
> /* Get the NODE for FROM_PATH in FROM_ROOT.*/
> SVN_ERR(get_dag(&from_node, from_root, from_path, TRUE, pool));
>
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1526057 - in /subversion/trunk/subversion:
include/svn_fs.h libsvn_fs_fs/fs.h libsvn_fs_fs/tree.c libsvn_fs_fs/util.c libsvn_fs_fs/util.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Sep 25, 2013 at 10:02 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 25 September 2013 04:09, <st...@apache.org> wrote:
> > Author: stefan2
> > Date: Wed Sep 25 00:09:06 2013
> > New Revision: 1526057
> >
> > URL: http://svn.apache.org/r1526057
> > Log:
> > Make native svn_fs_move support in FSFS dependent on a format bump
> > (tweak the conditional manually for testing). Fall back to ordinary
> > copy-with-history for older format.
> >
> > Also, relax the condition on svn_fs_move source revision. If a rev
> > older than the txn's base revision is specified, there must have been
> > no changes at all to that node in meantime and neither the node nor
> > nor any of the parents been deleted (and later restored).
> >
> [...]
> Hi, Stefan. See my comment inline.
>
> > Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1526057&r1=1526056&r2=1526057&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
> > +++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Wed Sep 25 00:09:06
> 2013
> > @@ -60,6 +60,7 @@
> > #include "pack.h"
> > #include "temp_serializer.h"
> > #include "transaction.h"
> > +#include "util.h"
> >
> > #include "private/svn_mergeinfo_private.h"
> > #include "private/svn_subr_private.h"
> > @@ -2399,6 +2400,55 @@ typedef enum copy_type_t
> > copy_type_move
> > } copy_type_t;
> >
> > +/* Set CHANGES to TRUE if PATH in ROOT is unchanged in REVISION if the
> > + same files system. If the content is identical, parent path copies
> and
> > + deletions still count as changes. Use POOL for temporary
> allocations.
> > + Not that we will return an error if PATH does not exist in ROOT or
> > + REVISION- */
> > +static svn_error_t *
> > +is_changed_node(svn_boolean_t *changed,
> > + svn_fs_root_t *root,
> > + const char *path,
> > + svn_revnum_t revision,
> > + apr_pool_t *pool)
> > +{
> > + dag_node_t *node, *rev_node;
> > + svn_fs_root_t *rev_root;
> > + svn_fs_root_t *copy_from_root1, *copy_from_root2;
> > + const char *copy_from_path1, *copy_from_path2;
> > +
> > + *changed = TRUE;
> > +
> > + SVN_ERR(svn_fs_fs__revision_root(&rev_root, root->fs, revision,
> pool));
> > +
> > + /* Get the NODE for FROM_PATH in FROM_ROOT.*/
> > + SVN_ERR(get_dag(&node, root, path, TRUE, pool));
> > + SVN_ERR(get_dag(&rev_node, rev_root, path, TRUE, pool));
> > +
> > + /* different ID -> got changed*/
> > + if (!svn_fs_fs__id_eq(svn_fs_fs__dag_get_id(node),
> > + svn_fs_fs__dag_get_id(rev_node)))
> > + return SVN_NO_ERROR;
> > +
> > + /* some node. might still be a lazy copy */
> > + SVN_ERR(svn_fs_closest_copy(©_from_root1, ©_from_path1, root,
> > + path, pool));
> > + SVN_ERR(svn_fs_closest_copy(©_from_root2, ©_from_path2,
> rev_root,
> > + path, pool));
> > +
> > + if ((copy_from_root1 == NULL) != (copy_from_root2 == NULL))
> > + return SVN_NO_ERROR;
> > +
> > + if (copy_from_root1)
> > + if ( copy_from_root1->rev != copy_from_root2->rev
> > + || strcmp(copy_from_path1, copy_from_path2))
> > + return SVN_NO_ERROR;
> > +
> > + *changed = FALSE;
> > + return SVN_NO_ERROR;
> > +}
> I didn't check that is_changed_node() function doing right thing, but
> I suggest to refactor is_changed_node() a bit (see attached patch).
> Final code will be:
> [[[[
> static svn_error_t *
> is_changed_node(svn_boolean_t *changed,
> svn_fs_root_t *root,
> const char *path,
> svn_revnum_t revision,
> apr_pool_t *pool)
> {
> dag_node_t *node, *rev_node;
> svn_fs_root_t *rev_root;
> svn_fs_root_t *copy_from_root1, *copy_from_root2;
> const char *copy_from_path1, *copy_from_path2;
>
> SVN_ERR(svn_fs_fs__revision_root(&rev_root, root->fs, revision, pool));
>
> /* Get the NODE for FROM_PATH in FROM_ROOT.*/
> SVN_ERR(get_dag(&node, root, path, TRUE, pool));
> SVN_ERR(get_dag(&rev_node, rev_root, path, TRUE, pool));
>
> /* different ID -> got changed*/
> if (!svn_fs_fs__id_eq(svn_fs_fs__dag_get_id(node),
> svn_fs_fs__dag_get_id(rev_node)))
> {
> *changed = TRUE;
> return SVN_NO_ERROR;
> }
>
> /* some node. might still be a lazy copy */
> SVN_ERR(fs_closest_copy(©_from_root1, ©_from_path1, root,
> path, pool));
> SVN_ERR(fs_closest_copy(©_from_root2, ©_from_path2, rev_root,
> path, pool));
>
> if (copy_from_root1 == NULL && copy_from_root2 == NULL)
> {
> *changed = FALSE;
> return SVN_NO_ERROR;
> }
> else if (copy_from_root1 != NULL && copy_from_root2 != NULL)
> {
> *changed = (copy_from_root1->rev != copy_from_root2->rev)
> || strcmp(copy_from_path1, copy_from_path2);
> return SVN_NO_ERROR;
> }
> else
> {
> *changed = TRUE;
> return SVN_NO_ERROR;
> }
> }
> ]]]
> I think it makes behavior more clear. I cannot commit it because there
> is no tests for this functionality. Could you please add them.
>
With some more improvements by me: r1527079.
> > +
> > +
> > /* Copy the node at FROM_PATH under FROM_ROOT to TO_PATH under
> > TO_ROOT. COPY_TYPE determines whether then the copy is recorded in
> > the copies table and whether it is being marked as a move.
> > @@ -2436,10 +2486,33 @@ copy_helper(svn_fs_root_t *from_root,
> > (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> > _("Copy immutable tree not supported"));
> >
> > - if (copy_type == copy_type_move && from_root->rev != txn_id->revision)
> > - return svn_error_create
> > - (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> > - _("Move from non-HEAD revision not currently supported"));
> > + /* move support comes with a number of conditions ... */
> > + if (copy_type == copy_type_move)
> > + {
> > + /* if we don't copy from the TXN's base rev, check that the path
> has
> > + not been touched in that revision range */
> > + if (from_root->rev != txn_id->revision)
> > + {
> > + svn_boolean_t changed = TRUE;
> > + svn_error_t *err = is_changed_node(&changed, from_root,
> > + from_path,
> txn_id->revision,
> > + pool);
> > + if (err || changed)
> > + {
> > + svn_error_clear(err);
> Converting *any* error to SVN_ERR_FS_OUT_OF_DATE looks wrong for me.
> For example SVN_FS_CORRUPTED error returned from is_changed_node()
> will be also cleared and converted SVN_ERR_FS_OUT_OF_DATE. I think
> this should be changed.
>
You are right. Fixed in r1527084.
>
> > + return svn_error_create(SVN_ERR_FS_OUT_OF_DATE, NULL,
> > + _("Move-from node is
> out-of-date"));
> > + }
> > +
> > + /* always move from the txn's base rev */
> > + SVN_ERR(svn_fs_fs__revision_root(&from_root, from_root->fs,
> > + txn_id->revision, pool));
> > + }
> > +
> > + /* does the FS support moves at all? */
> > + if (!svn_fs_fs__supports_move(to_root->fs))
> > + copy_type = copy_type_add_with_history;
> > + }
> >
> > /* Get the NODE for FROM_PATH in FROM_ROOT.*/
> > SVN_ERR(get_dag(&from_node, from_root, from_path, TRUE, pool));
> >
>
Thanks for the feedback!
-- Stefan^2.