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

> -----Original Message-----
> From: ivan_at_visualsvn.com [mailto:ivan_at_visualsvn.com] On Behalf Of Ivan
> Zhakov
> Sent: zaterdag 31 oktober 2015 18:10
> To: Bert Huijben <bert_at_qqmail.nl>
> Cc: dev_at_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
Received on 2015-11-02 13:22:36 CET