On Mon, Jul 01, 2013 at 03:10:26PM +0300, Daniel Shahaf wrote:
> Stefan Sperling wrote on Mon, Jul 01, 2013 at 13:52:08 +0200:
> > On Mon, Jul 01, 2013 at 02:33:06PM +0300, Daniel Shahaf wrote:
> > > I don't remember whether I pointed this out when Stefan originally wrote
> > > that code:
> > >
> > > [[[
> > > * subversion/tests/libsvn_fs/fs-test.c
> > > (filename_trailing_newline): Switch from a blacklist approach to
> > > to a whitelist approach, for defining backends that don't implement
> > > the API correctly.
> > > ]]]
> >
> > When a new backend comes along that doesn't handle \n due to a bug
> > in the design/implementation, this test is supposed to FAIL, not PASS!
> >
>
> The current HEAD code will expect a new backend to return an error on
> \n. The code with my patch will expect a new backend to return
> SVN_NO_ERROR. It sounds like applying the patch will cause the code to
> do what you say you want it to do. What am I missing?
So you get SVN_NO_ERROR from a copy() fs call. That's doesn't tell you
anything about whether whoever implemented the backend did consider
the \n case. I don't think we should blindly trust SVN_NO_ERROR.
FSFS performed the copy and didn't return an error, and silently
created invalid revision files in the process.
The error the test expects is from a path verification function.
It's more reliable for this test to expect path name verification to fail,
rather than expecting the copy to succeed. The copy may well succeed and
silently corrupt the repos. The test forces implementors of new backends
to ensure that the \n case is properly handled, either by ensuring that
a PATH_SYNTAX error is raised because they cannot handle the \n, or add
their backend to the list of filesystems that will raise SVN_NO_ERROR
and are known good.
Perhaps we should add a comment to the test to make this clearer?
Received on 2013-07-01 14:46:31 CEST