You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <da...@elego.de> on 2013/07/01 13:33:06 UTC
[PATCH] default to expecting new FS backends to support \n in
filenames
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.
]]]
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));
For clarity, I'd be fine with adding FSX to the blacklist here.
Re: [PATCH] default to expecting new FS backends to support \n in
filenames
Posted by Daniel Shahaf <da...@elego.de>.
Stefan Sperling wrote on Mon, Jul 01, 2013 at 15:40:18 +0200:
> On Mon, Jul 01, 2013 at 04:05:54PM +0300, Daniel Shahaf wrote:
> > Daniel Shahaf wrote on Mon, Jul 01, 2013 at 15:52:32 +0300:
> > > 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.
>
> Fair enough, that's good enough for me.
>
> But please also check that log -v returns a good list of paths.
> With the FSFS \n problem, 'svn ls' was fine but 'log -v' was
> broken (dirent data vs changed paths data).
> Verify didn't detect the 'log -v' problem (see
> http://subversion.tigris.org/issues/show_bug.cgi?id=4343).
>
> Can you also rename allow_newlines to just 'is_fsfs', please?
>
> Thanks!
Done, and nominated for backport:
http://svn.apache.org/r1498489
Thanks,
Daniel
Re: [PATCH] default to expecting new FS backends to support \n in
filenames
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Jul 01, 2013 at 04:05:54PM +0300, Daniel Shahaf wrote:
> Daniel Shahaf wrote on Mon, Jul 01, 2013 at 15:52:32 +0300:
> > 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.
Fair enough, that's good enough for me.
But please also check that log -v returns a good list of paths.
With the FSFS \n problem, 'svn ls' was fine but 'log -v' was
broken (dirent data vs changed paths data).
Verify didn't detect the 'log -v' problem (see
http://subversion.tigris.org/issues/show_bug.cgi?id=4343).
Can you also rename allow_newlines to just 'is_fsfs', please?
Thanks!
> 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;
> }
>
Re: [PATCH] default to expecting new FS backends to support \n in
filenames
Posted by Daniel Shahaf <da...@elego.de>.
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;
}
Re: [PATCH] default to expecting new FS backends to support \n in
filenames
Posted by Daniel Shahaf <da...@elego.de>.
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.
Re: [PATCH] default to expecting new FS backends to support \n in
filenames
Posted by Stefan Sperling <st...@elego.de>.
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?
Re: [PATCH] default to expecting new FS backends to support \n in
filenames
Posted by Daniel Shahaf <da...@elego.de>.
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 I would strongly prefer keeping a whitelist.
Re: [PATCH] default to expecting new FS backends to support \n in
filenames
Posted by Stefan Sperling <st...@elego.de>.
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!
So I would strongly prefer keeping a whitelist.
Re: [PATCH] default to expecting new FS backends to support \n in filenames
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Jul 1, 2013 at 1:33 PM, Daniel Shahaf <da...@elego.de> 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.
> ]]]
>
> 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));
>
Looks good.
> For clarity, I'd be fine with adding FSX to the blacklist here.
>
FSX is supposed to eventually handle arbitrary filenames -
it's on the TODO list. So, don't blacklist it and simply let
the tests fail as a friendly reminder to the coder ...
-- Stefan^2