Re: svn commit: r1711582 - in /subversion/trunk/subversion: libsvn_fs_fs/tree.c tests/libsvn_fs/fs-test.c

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