You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2015/01/27 02:48:08 UTC
svn commit: r1654933 -
/subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c
Author: stefan2
Date: Tue Jan 27 01:48:08 2015
New Revision: 1654933
URL: http://svn.apache.org/r1654933
Log:
Add a test demonstrating issue #4554.
* subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c
(receive_index,
stringbuf_find,
get_line): New, test-specific utility functions.
(plain_0_length): The new test case.
(test_funcs): Register new test case.
Modified:
subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c
Modified: subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c?rev=1654933&r1=1654932&r2=1654933&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c Tue Jan 27 01:48:08 2015
@@ -25,8 +25,10 @@
#include <apr_pools.h>
#include "../svn_test.h"
+#include "../../libsvn_fs/fs-loader.h"
#include "../../libsvn_fs_fs/fs.h"
#include "../../libsvn_fs_fs/fs_fs.h"
+#include "../../libsvn_fs_fs/util.h"
#include "svn_hash.h"
#include "svn_pools.h"
@@ -1243,6 +1245,134 @@ id_parser_test(const svn_test_opts_t *op
#undef REPO_NAME
+/* ------------------------------------------------------------------------ */
+
+#define REPO_NAME "test-repo-plain_0_length"
+
+static svn_error_t *
+receive_index(const svn_fs_fs__p2l_entry_t *entry,
+ void *baton,
+ apr_pool_t *scratch_pool)
+{
+ apr_array_header_t *entries = baton;
+ APR_ARRAY_PUSH(entries, svn_fs_fs__p2l_entry_t *)
+ = apr_pmemdup(entries->pool, entry, sizeof(*entry));
+
+ return SVN_NO_ERROR;
+}
+
+static char *
+stringbuf_find(svn_stringbuf_t *rev_contents,
+ const char *substring)
+{
+ apr_size_t i;
+ apr_size_t len = strlen(substring);
+
+ for (i = 0; i < rev_contents->len - len + 1; ++i)
+ if (!memcmp(rev_contents->data + i, substring, len))
+ return rev_contents->data + i;
+
+ return NULL;
+}
+
+
+static svn_stringbuf_t *
+get_line(svn_stringbuf_t *rev_contents,
+ const char *prefix,
+ apr_pool_t *pool)
+{
+ char *end, *start = stringbuf_find(rev_contents, prefix);
+ if (start == NULL)
+ return svn_stringbuf_create_empty(pool);
+
+ end = strchr(start, '\n');
+ if (end == NULL)
+ return svn_stringbuf_create_empty(pool);
+
+ return svn_stringbuf_ncreate(start, end - start, pool);
+}
+
+static svn_error_t *
+plain_0_length(const svn_test_opts_t *opts,
+ apr_pool_t *pool)
+{
+ svn_fs_t *fs;
+ fs_fs_data_t *ffd;
+ svn_fs_txn_t *txn;
+ svn_fs_root_t *root;
+ svn_revnum_t rev;
+ const char *rev_path;
+ svn_stringbuf_t *rev_contents, *props_line;
+ char *text;
+ apr_hash_t *fs_config;
+ svn_filesize_t file_length;
+
+ if (strcmp(opts->fs_type, "fsfs") != 0)
+ return svn_error_create(SVN_ERR_TEST_SKIPPED, NULL, NULL);
+
+ /* Create a repo that does not deltify properties and does not share reps
+ on its own - makes it easier to do that later by hand. */
+ SVN_ERR(svn_test__create_fs(&fs, REPO_NAME, opts, pool));
+ ffd = fs->fsap_data;
+ ffd->deltify_properties = FALSE;
+ ffd->rep_sharing_allowed = FALSE;
+
+ /* Create one file node with matching contents and property reps. */
+ SVN_ERR(svn_fs_begin_txn(&txn, fs, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&root, txn, pool));
+ SVN_ERR(svn_fs_make_file(root, "foo", pool));
+ SVN_ERR(svn_test__set_file_contents(root, "foo", "END\n", pool));
+ SVN_ERR(svn_fs_change_node_prop(root, "foo", "x", NULL, pool));
+ SVN_ERR(svn_fs_commit_txn(NULL, &rev, txn, pool));
+
+ /* Redirect text rep to props rep. */
+ rev_path = svn_fs_fs__path_rev_absolute(fs, rev, pool);
+ SVN_ERR(svn_stringbuf_from_file2(&rev_contents, rev_path, pool));
+
+ props_line = get_line(rev_contents, "props: ", pool);
+ text = stringbuf_find(rev_contents, "text: ");
+
+ if (text)
+ {
+ /* Explicitly set the last number before the MD5 to 0 */
+ strstr(props_line->data, " 2d29")[-1] = '0';
+
+ /* Add a padding space - in case we shorten the number in TEXT */
+ svn_stringbuf_appendbyte(props_line, ' ');
+
+ /* Make text point to the PLAIN data rep with no expanded size info. */
+ memcpy(text + 6, props_line->data + 7, props_line->len - 7);
+ }
+
+ SVN_ERR(svn_io_write_atomic(rev_path, rev_contents->data,
+ rev_contents->len, NULL, pool));
+
+ if (svn_fs_fs__use_log_addressing(fs))
+ {
+ /* Refresh index data (checksums). */
+ apr_array_header_t *entries = apr_array_make(pool, 4, sizeof(void *));
+ SVN_ERR(svn_fs_fs__dump_index(fs, rev, receive_index, entries,
+ NULL, NULL, pool));
+ SVN_ERR(svn_fs_fs__load_index(fs, rev, entries, pool));
+ }
+
+ /* Create an independent FS instances with separate caches etc. */
+ fs_config = apr_hash_make(pool);
+ svn_hash_sets(fs_config, SVN_FS_CONFIG_FSFS_CACHE_NS,
+ svn_uuid_generate(pool));
+ SVN_ERR(svn_fs_open2(&fs, REPO_NAME, fs_config, pool, pool));
+
+ /* Now, check that we get the correct file length. */
+ SVN_ERR(svn_fs_revision_root(&root, fs, rev, pool));
+ SVN_ERR(svn_fs_file_length(&file_length, root, "foo", pool));
+
+ SVN_TEST_ASSERT(file_length == 4);
+
+ return SVN_NO_ERROR;
+}
+
+#undef REPO_NAME
+
/* The test table. */
@@ -1285,6 +1415,8 @@ static struct svn_test_descriptor_t test
"change revprops with enabled and disabled caching"),
SVN_TEST_OPTS_PASS(id_parser_test,
"id parser test"),
+ SVN_TEST_OPTS_PASS(plain_0_length,
+ "file with 0 expanded-length, issue #4554"),
SVN_TEST_NULL
};
Re: svn commit: r1654933 - /subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Jan 27, 2015 at 11:19 AM, Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> Good morning Stefan,
>
> stefan2@apache.org wrote on Tue, Jan 27, 2015 at 01:48:08 -0000:
> > + /* Redirect text rep to props rep. */
> > + rev_path = svn_fs_fs__path_rev_absolute(fs, rev, pool);
> > + SVN_ERR(svn_stringbuf_from_file2(&rev_contents, rev_path, pool));
> > +
> > + props_line = get_line(rev_contents, "props: ", pool);
> > + text = stringbuf_find(rev_contents, "text: ");
> > +
> > + if (text)
> > + {
> > + /* Explicitly set the last number before the MD5 to 0 */
> > + strstr(props_line->data, " 2d29")[-1] = '0';
> > +
> > + /* Add a padding space - in case we shorten the number in TEXT */
> > + svn_stringbuf_appendbyte(props_line, ' ');
> > +
> > + /* Make text point to the PLAIN data rep with no expanded size
> info. */
> > + memcpy(text + 6, props_line->data + 7, props_line->len - 7);
> > + }
>
> The above bit appears brittle: it heavily depends on the exact current
> layout of rev files, down to the number of whitespace bytes between the
> <expanded_size> and <md5> fields in a noderev header line and the length
> of the uniquifier field.
>
> I realize that it works now, and that if the original bytes ever are
> different the test may corrupt the filesystem and thereby at least fail
> noisily, but perhaps there is a more robust way to achieve the declared
> end of redirecting the text rep to the props rep?
>
You are right. After thinking about it, I remembered that /trunk
has nice parsers and serializers for such tasks, see r1655008.
The downside is that only the original implementation can be
backported.
It is still somewhat brittle as the numbers must not expand,
which they won't. If that was to happen one day, we would
have to add quite a bit more code.
> For example, perhaps enable rep-cache.db (after creating the first revision
> with it disabled) and insert a false record for the text rep, or use
> svn_cstring_split() to get at the individual tokens more robustly.
>
Rep caching is only available for format 4+ while older formats
are particularly likely to use "PLAIN" representations.
> Cheers,
>
> Daniel
>
> P.S. The test changes the number of bytes in the noderev header, without
> changing any of of the physical offsets in the rev file. Why does that
> work with the formats ≤ 6? I expected it to invalidate the
> final changed-paths-offset, but the test runs and passes and the offset is
> correct when it finishes.
>
The length of rev_contents did not change. New data got
memcpy'ed over whatever older text was there.
-- Stefan^2.
Re: svn commit: r1654933 - /subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Jan 27, 2015 at 11:19 AM, Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> Good morning Stefan,
>
> stefan2@apache.org wrote on Tue, Jan 27, 2015 at 01:48:08 -0000:
> > + /* Redirect text rep to props rep. */
> > + rev_path = svn_fs_fs__path_rev_absolute(fs, rev, pool);
> > + SVN_ERR(svn_stringbuf_from_file2(&rev_contents, rev_path, pool));
> > +
> > + props_line = get_line(rev_contents, "props: ", pool);
> > + text = stringbuf_find(rev_contents, "text: ");
> > +
> > + if (text)
> > + {
> > + /* Explicitly set the last number before the MD5 to 0 */
> > + strstr(props_line->data, " 2d29")[-1] = '0';
> > +
> > + /* Add a padding space - in case we shorten the number in TEXT */
> > + svn_stringbuf_appendbyte(props_line, ' ');
> > +
> > + /* Make text point to the PLAIN data rep with no expanded size
> info. */
> > + memcpy(text + 6, props_line->data + 7, props_line->len - 7);
> > + }
>
> The above bit appears brittle: it heavily depends on the exact current
> layout of rev files, down to the number of whitespace bytes between the
> <expanded_size> and <md5> fields in a noderev header line and the length
> of the uniquifier field.
>
> I realize that it works now, and that if the original bytes ever are
> different the test may corrupt the filesystem and thereby at least fail
> noisily, but perhaps there is a more robust way to achieve the declared
> end of redirecting the text rep to the props rep?
>
You are right. After thinking about it, I remembered that /trunk
has nice parsers and serializers for such tasks, see r1655008.
The downside is that only the original implementation can be
backported.
It is still somewhat brittle as the numbers must not expand,
which they won't. If that was to happen one day, we would
have to add quite a bit more code.
> For example, perhaps enable rep-cache.db (after creating the first revision
> with it disabled) and insert a false record for the text rep, or use
> svn_cstring_split() to get at the individual tokens more robustly.
>
Rep caching is only available for format 4+ while older formats
are particularly likely to use "PLAIN" representations.
> Cheers,
>
> Daniel
>
> P.S. The test changes the number of bytes in the noderev header, without
> changing any of of the physical offsets in the rev file. Why does that
> work with the formats ≤ 6? I expected it to invalidate the
> final changed-paths-offset, but the test runs and passes and the offset is
> correct when it finishes.
>
The length of rev_contents did not change. New data got
memcpy'ed over whatever older text was there.
-- Stefan^2.
Re: svn commit: r1654933 -
/subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Good morning Stefan,
stefan2@apache.org wrote on Tue, Jan 27, 2015 at 01:48:08 -0000:
> + /* Redirect text rep to props rep. */
> + rev_path = svn_fs_fs__path_rev_absolute(fs, rev, pool);
> + SVN_ERR(svn_stringbuf_from_file2(&rev_contents, rev_path, pool));
> +
> + props_line = get_line(rev_contents, "props: ", pool);
> + text = stringbuf_find(rev_contents, "text: ");
> +
> + if (text)
> + {
> + /* Explicitly set the last number before the MD5 to 0 */
> + strstr(props_line->data, " 2d29")[-1] = '0';
> +
> + /* Add a padding space - in case we shorten the number in TEXT */
> + svn_stringbuf_appendbyte(props_line, ' ');
> +
> + /* Make text point to the PLAIN data rep with no expanded size info. */
> + memcpy(text + 6, props_line->data + 7, props_line->len - 7);
> + }
The above bit appears brittle: it heavily depends on the exact current
layout of rev files, down to the number of whitespace bytes between the
<expanded_size> and <md5> fields in a noderev header line and the length
of the uniquifier field.
I realize that it works now, and that if the original bytes ever are
different the test may corrupt the filesystem and thereby at least fail
noisily, but perhaps there is a more robust way to achieve the declared
end of redirecting the text rep to the props rep?
For example, perhaps enable rep-cache.db (after creating the first revision
with it disabled) and insert a false record for the text rep, or use
svn_cstring_split() to get at the individual tokens more robustly.
Cheers,
Daniel
P.S. The test changes the number of bytes in the noderev header, without
changing any of of the physical offsets in the rev file. Why does that
work with the formats ≤ 6? I expected it to invalidate the
final changed-paths-offset, but the test runs and passes and the offset is
correct when it finishes.
Re: svn commit: r1654933 -
/subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Good morning Stefan,
stefan2@apache.org wrote on Tue, Jan 27, 2015 at 01:48:08 -0000:
> + /* Redirect text rep to props rep. */
> + rev_path = svn_fs_fs__path_rev_absolute(fs, rev, pool);
> + SVN_ERR(svn_stringbuf_from_file2(&rev_contents, rev_path, pool));
> +
> + props_line = get_line(rev_contents, "props: ", pool);
> + text = stringbuf_find(rev_contents, "text: ");
> +
> + if (text)
> + {
> + /* Explicitly set the last number before the MD5 to 0 */
> + strstr(props_line->data, " 2d29")[-1] = '0';
> +
> + /* Add a padding space - in case we shorten the number in TEXT */
> + svn_stringbuf_appendbyte(props_line, ' ');
> +
> + /* Make text point to the PLAIN data rep with no expanded size info. */
> + memcpy(text + 6, props_line->data + 7, props_line->len - 7);
> + }
The above bit appears brittle: it heavily depends on the exact current
layout of rev files, down to the number of whitespace bytes between the
<expanded_size> and <md5> fields in a noderev header line and the length
of the uniquifier field.
I realize that it works now, and that if the original bytes ever are
different the test may corrupt the filesystem and thereby at least fail
noisily, but perhaps there is a more robust way to achieve the declared
end of redirecting the text rep to the props rep?
For example, perhaps enable rep-cache.db (after creating the first revision
with it disabled) and insert a false record for the text rep, or use
svn_cstring_split() to get at the individual tokens more robustly.
Cheers,
Daniel
P.S. The test changes the number of bytes in the noderev header, without
changing any of of the physical offsets in the rev file. Why does that
work with the formats ≤ 6? I expected it to invalidate the
final changed-paths-offset, but the test runs and passes and the offset is
correct when it finishes.