You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by iv...@apache.org on 2015/10/31 10:31:46 UTC
svn commit: r1711582 - in /subversion/trunk/subversion: libsvn_fs_fs/tree.c
tests/libsvn_fs/fs-test.c
Author: ivan
Date: Sat Oct 31 09:31:46 2015
New Revision: 1711582
URL: http://svn.apache.org/viewvc?rev=1711582&view=rev
Log:
Avoid double DAG lookup in FSFS implementation of svn_fs_contents_changed()
and svn_fs_contents_different().
It also slightly changes error message when these invoked functions invoked
for non-existent path: before this change error message was "'/non-existent'
is not a file" now it will be "File not found: revision 1, path
'/non-existent'"
* subversion/libsvn_fs_fs/tree.c
(fs_contents_changed): Use svn_fs_fs__dag_node_kind() to get node kind of
already obtained dag_node_t instead of calling to svn_fs_fs__check_path().
* subversion/tests/libsvn_fs/fs-test.c
(compare_contents): Extend test to test behavior of
svn_fs_contents_changed() and svn_fs_contents_different() with directories
and non-existent paths.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/tree.c
subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1711582&r1=1711581&r2=1711582&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Sat Oct 31 09:31:46 2015
@@ -3235,23 +3235,18 @@ fs_contents_changed(svn_boolean_t *chang
(SVN_ERR_FS_GENERAL, NULL,
_("Cannot compare file contents between two different filesystems"));
- /* Check that both paths are files. */
- {
- svn_node_kind_t kind;
-
- SVN_ERR(svn_fs_fs__check_path(&kind, root1, path1, pool));
- if (kind != svn_node_file)
- return svn_error_createf
- (SVN_ERR_FS_GENERAL, NULL, _("'%s' is not a file"), path1);
-
- SVN_ERR(svn_fs_fs__check_path(&kind, root2, path2, pool));
- if (kind != svn_node_file)
- return svn_error_createf
- (SVN_ERR_FS_GENERAL, NULL, _("'%s' is not a file"), path2);
- }
-
SVN_ERR(get_dag(&node1, root1, path1, pool));
+ /* Make sure that path is file. */
+ if (svn_fs_fs__dag_node_kind(node1) != svn_node_file)
+ return svn_error_createf
+ (SVN_ERR_FS_GENERAL, NULL, _("'%s' is not a file"), path1);
+
SVN_ERR(get_dag(&node2, root2, path2, pool));
+ /* Make sure that path is file. */
+ if (svn_fs_fs__dag_node_kind(node2) != svn_node_file)
+ return svn_error_createf
+ (SVN_ERR_FS_GENERAL, NULL, _("'%s' is not a file"), path2);
+
return svn_fs_fs__dag_things_different(NULL, changed_p,
node1, node2, strict, pool);
}
Modified: subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c?rev=1711582&r1=1711581&r2=1711582&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_fs/fs-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_fs/fs-test.c Sat Oct 31 09:31:46 2015
@@ -5971,6 +5971,7 @@ compare_contents(const svn_test_opts_t *
svn_revnum_t rev;
int i;
apr_pool_t *iterpool = svn_pool_create(pool);
+ svn_boolean_t changed;
/* Two similar but different texts that yield the same MD5 digest. */
const char *evil_text1
@@ -6153,6 +6154,21 @@ compare_contents(const svn_test_opts_t *
}
}
+ /* Check how svn_fs_contents_different() and svn_fs_contents_changed()
+ handles invalid path.*/
+ SVN_ERR(svn_fs_revision_root(&root1, fs, 1, iterpool));
+ SVN_TEST_ASSERT_ANY_ERROR(
+ svn_fs_contents_changed(&changed, root1, "/", root1, "/", iterpool));
+ SVN_TEST_ASSERT_ANY_ERROR(
+ svn_fs_contents_different(&changed, root1, "/", root1, "/", iterpool));
+
+ SVN_TEST_ASSERT_ANY_ERROR(
+ svn_fs_contents_changed(&changed, root1, "/non-existent", root1,
+ "/non-existent", iterpool));
+ SVN_TEST_ASSERT_ANY_ERROR(
+ svn_fs_contents_different(&changed, root1, "/non-existent", root1,
+ "/non-existent", iterpool));
+
svn_pool_destroy(iterpool);
return SVN_NO_ERROR;
RE: svn commit: r1711582 - in /subversion/trunk/subversion: libsvn_fs_fs/tree.c tests/libsvn_fs/fs-test.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: ivan@visualsvn.com [mailto:ivan@visualsvn.com] On Behalf Of Ivan
> Zhakov
> Sent: zaterdag 31 oktober 2015 18:10
> To: Bert Huijben <be...@qqmail.nl>
> Cc: dev@subversion.apache.org
> Subject: Re: svn commit: r1711582 - in /subversion/trunk/subversion:
> libsvn_fs_fs/tree.c tests/libsvn_fs/fs-test.c
> > I don't think you want to backport this, but returning
> SVN_ERR_FS_NOT_FILE
> > would make more sense here, than returning SVN_ERR_FS_GENERAL.
> >
> I agree that SVN_ERR_FS_NOT_FILE makes more sense. I'll do it.
>
> > And I would like to see the regression test checking that for all filesystem
> > layers for 1.10... but that is a bigger change.
> >
> We do not document this behavior, so I think it would be incorrect to
> require specific error code for
> svn_fs_contents_changed()/svn_fs_contents_different().
No, but as we go we can make our own implementations more strictly defined than before.
(Our regression tests are for our implementations, aren't they?... Or should we limit what we test based on theoretical third party filesystems?)
One of the reasons for not strictly documenting things before, might be that we want to improve things later. Not documenting things gives more freedom to change things for different reasons. (The whole C standard is based on explicitly keeping things undefined).
The old code might have relied on falling back to error handling on a different layer, which might have given a different (or more generic) error. But as we already documented these two functions to only apply to files, there are good reasons to test implementations on this... in fact you just added the test.
Once all implementations match a stricter standard we can improve the documentation and implementation by producing a more stable and guaranteed error.
(I'm hoping BDB already produces something sane)
Bert
Re: svn commit: r1711582 - in /subversion/trunk/subversion:
libsvn_fs_fs/tree.c tests/libsvn_fs/fs-test.c
Posted by Ivan Zhakov <iv...@apache.org>.
On 31 October 2015 at 17:01, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: ivan@apache.org [mailto:ivan@apache.org]
>> Sent: zaterdag 31 oktober 2015 10:32
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1711582 - in /subversion/trunk/subversion:
>> libsvn_fs_fs/tree.c tests/libsvn_fs/fs-test.c
>>
>> Author: ivan
>> Date: Sat Oct 31 09:31:46 2015
>> New Revision: 1711582
>>
>> URL: http://svn.apache.org/viewvc?rev=1711582&view=rev
>> Log:
>> Avoid double DAG lookup in FSFS implementation of
>> svn_fs_contents_changed()
>> and svn_fs_contents_different().
>>
>> It also slightly changes error message when these invoked functions invoked
>> for non-existent path: before this change error message was "'/non-existent'
>> is not a file" now it will be "File not found: revision 1, path
>> '/non-existent'"
>>
>> * subversion/libsvn_fs_fs/tree.c
>> (fs_contents_changed): Use svn_fs_fs__dag_node_kind() to get node kind
>> of
>> already obtained dag_node_t instead of calling to
>> svn_fs_fs__check_path().
>>
>> * subversion/tests/libsvn_fs/fs-test.c
>> (compare_contents): Extend test to test behavior of
>> svn_fs_contents_changed() and svn_fs_contents_different() with
>> directories
>> and non-existent paths.
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_fs_fs/tree.c
>> subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>>
>> Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tr
>> ee.c?rev=1711582&r1=1711581&r2=1711582&view=diff
>> ==========================================================
>> ====================
>> --- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
>> +++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Sat Oct 31 09:31:46
>> 2015
>> @@ -3235,23 +3235,18 @@ fs_contents_changed(svn_boolean_t *chang
>> (SVN_ERR_FS_GENERAL, NULL,
>> _("Cannot compare file contents between two different filesystems"));
>>
>> - /* Check that both paths are files. */
>> - {
>> - svn_node_kind_t kind;
>> -
>> - SVN_ERR(svn_fs_fs__check_path(&kind, root1, path1, pool));
>> - if (kind != svn_node_file)
>> - return svn_error_createf
>> - (SVN_ERR_FS_GENERAL, NULL, _("'%s' is not a file"), path1);
>> -
>> - SVN_ERR(svn_fs_fs__check_path(&kind, root2, path2, pool));
>> - if (kind != svn_node_file)
>> - return svn_error_createf
>> - (SVN_ERR_FS_GENERAL, NULL, _("'%s' is not a file"), path2);
>> - }
>> -
>> SVN_ERR(get_dag(&node1, root1, path1, pool));
>> + /* Make sure that path is file. */
>> + if (svn_fs_fs__dag_node_kind(node1) != svn_node_file)
>> + return svn_error_createf
>> + (SVN_ERR_FS_GENERAL, NULL, _("'%s' is not a file"), path1);
>> +
>> SVN_ERR(get_dag(&node2, root2, path2, pool));
>> + /* Make sure that path is file. */
>> + if (svn_fs_fs__dag_node_kind(node2) != svn_node_file)
>> + return svn_error_createf
>> + (SVN_ERR_FS_GENERAL, NULL, _("'%s' is not a file"), path2);
>> +
>
> I don't think you want to backport this, but returning SVN_ERR_FS_NOT_FILE
> would make more sense here, than returning SVN_ERR_FS_GENERAL.
>
I agree that SVN_ERR_FS_NOT_FILE makes more sense. I'll do it.
> And I would like to see the regression test checking that for all filesystem
> layers for 1.10... but that is a bigger change.
>
We do not document this behavior, so I think it would be incorrect to
require specific error code for
svn_fs_contents_changed()/svn_fs_contents_different().
--
Ivan Zhakov
RE: svn commit: r1711582 - in /subversion/trunk/subversion: libsvn_fs_fs/tree.c tests/libsvn_fs/fs-test.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: ivan@apache.org [mailto:ivan@apache.org]
> Sent: zaterdag 31 oktober 2015 10:32
> To: commits@subversion.apache.org
> Subject: svn commit: r1711582 - in /subversion/trunk/subversion:
> libsvn_fs_fs/tree.c tests/libsvn_fs/fs-test.c
>
> Author: ivan
> Date: Sat Oct 31 09:31:46 2015
> New Revision: 1711582
>
> URL: http://svn.apache.org/viewvc?rev=1711582&view=rev
> Log:
> Avoid double DAG lookup in FSFS implementation of
> svn_fs_contents_changed()
> and svn_fs_contents_different().
>
> It also slightly changes error message when these invoked functions invoked
> for non-existent path: before this change error message was "'/non-existent'
> is not a file" now it will be "File not found: revision 1, path
> '/non-existent'"
>
> * subversion/libsvn_fs_fs/tree.c
> (fs_contents_changed): Use svn_fs_fs__dag_node_kind() to get node kind
> of
> already obtained dag_node_t instead of calling to
> svn_fs_fs__check_path().
>
> * subversion/tests/libsvn_fs/fs-test.c
> (compare_contents): Extend test to test behavior of
> svn_fs_contents_changed() and svn_fs_contents_different() with
> directories
> and non-existent paths.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/tree.c
> subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tr
> ee.c?rev=1711582&r1=1711581&r2=1711582&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Sat Oct 31 09:31:46
> 2015
> @@ -3235,23 +3235,18 @@ fs_contents_changed(svn_boolean_t *chang
> (SVN_ERR_FS_GENERAL, NULL,
> _("Cannot compare file contents between two different filesystems"));
>
> - /* Check that both paths are files. */
> - {
> - svn_node_kind_t kind;
> -
> - SVN_ERR(svn_fs_fs__check_path(&kind, root1, path1, pool));
> - if (kind != svn_node_file)
> - return svn_error_createf
> - (SVN_ERR_FS_GENERAL, NULL, _("'%s' is not a file"), path1);
> -
> - SVN_ERR(svn_fs_fs__check_path(&kind, root2, path2, pool));
> - if (kind != svn_node_file)
> - return svn_error_createf
> - (SVN_ERR_FS_GENERAL, NULL, _("'%s' is not a file"), path2);
> - }
> -
> SVN_ERR(get_dag(&node1, root1, path1, pool));
> + /* Make sure that path is file. */
> + if (svn_fs_fs__dag_node_kind(node1) != svn_node_file)
> + return svn_error_createf
> + (SVN_ERR_FS_GENERAL, NULL, _("'%s' is not a file"), path1);
> +
> SVN_ERR(get_dag(&node2, root2, path2, pool));
> + /* Make sure that path is file. */
> + if (svn_fs_fs__dag_node_kind(node2) != svn_node_file)
> + return svn_error_createf
> + (SVN_ERR_FS_GENERAL, NULL, _("'%s' is not a file"), path2);
> +
I don't think you want to backport this, but returning SVN_ERR_FS_NOT_FILE would make more sense here, than returning SVN_ERR_FS_GENERAL.
And I would like to see the regression test checking that for all filesystem layers for 1.10... but that is a bigger change.
Bert