On 31 October 2015 at 17:01, Bert Huijben <[email protected]> wrote: >> -----Original Message----- >> From: [email protected] [mailto:[email protected]] >> Sent: zaterdag 31 oktober 2015 10:32 >> To: [email protected] >> 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
