Daniel Shahaf wrote on Mon, Jul 01, 2013 at 15:52:32 +0300:
> Stefan Sperling wrote on Mon, Jul 01, 2013 at 14:45:42 +0200:
> > 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?
>
> No, we should extend the test to run 'svn ls' or 'svnadmin verify' to
> ensure that creating the path with \n in it didn't break anything.
Here's the patch implementing that. If I don't hear otherwise, I'll
commit and nominate for backport to 1.8.1.
Daniel
Index: subversion/tests/libsvn_fs/fs-test.c
===================================================================
--- subversion/tests/libsvn_fs/fs-test.c (revision 1498385)
+++ subversion/tests/libsvn_fs/fs-test.c (working copy)
@@ -4959,10 +4959,11 @@ filename_trailing_newline(const svn_test_opts_t *o
svn_error_t *err;
svn_boolean_t allow_newlines;
- /* Some filesystem implementations can handle newlines in filenames
- * and can be white-listed here.
- * Currently, only BDB supports \n in filenames. */
- allow_newlines = (strcmp(opts->fs_type, "bdb") == 0);
+ /* The FS API wants \n to be permitted, but FSFS never implemented that,
+ * so for FSFS we expect errors rather than successes in some of the commits.
+ * Use a blacklist approach so that new FSes default to implementing the API
+ * as originally defined. */
+ allow_newlines = (!!strcmp(opts->fs_type, SVN_FS_TYPE_FSFS));
SVN_ERR(svn_test__create_fs(&fs, "test-repo-filename-trailing-newline",
opts, pool));
@@ -4992,6 +4993,22 @@ filename_trailing_newline(const svn_test_opts_t *o
else
SVN_TEST_ASSERT_ERROR(err, SVN_ERR_FS_PATH_SYNTAX);
+ if (allow_newlines)
+ {
+ static svn_test__tree_entry_t expected_entries[] = {
+ { "foo", NULL },
+ { "bar\n", NULL },
+ { "foo/baz\n", "" },
+ };
+ svn_revnum_t after_rev;
+
+ SVN_ERR(svn_fs_commit_txn(NULL, &after_rev, txn, subpool));
+ SVN_TEST_ASSERT(SVN_IS_VALID_REVNUM(after_rev));
+
+ SVN_ERR(svn_fs_revision_root(&root, fs, after_rev, pool));
+ SVN_ERR(svn_test__validate_tree(root, expected_entries, 3, pool));
+ }
+
return SVN_NO_ERROR;
}
