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 2014/06/21 17:15:31 UTC
svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/
tools/server-side/svnfsfs/
Author: stefan2
Date: Sat Jun 21 15:15:30 2014
New Revision: 1604421
URL: http://svn.apache.org/r1604421
Log:
Append index data to the rev data file instead of using separate files.
This gives unpacked FSFS format 7 similar I/O characteristics and disk
space usage as earlier formats. It also eliminates the need for retries
after a potential background pack run because each file is now always
consistent with itself (earlier, data or index files might already have
been deleted while the respective other was still valid). Overall,
most of this patch is removing code necessary to handle separate files.
The new file format is simple:
<rep data><l2p index><p2l index><footer><footer length>
with the first three being identical what we had before. <footer length>
is a single byte giving the length of the preceding footer, so it's
easier to extract than the pre-f7 rev trailer and there is only one
per pack / rev file.
The footer itself is just two decimal file offsets marking the start
of the index sections, separated by a single space char.
* subversion/libsvn_fs_fs/low_level.h
(svn_fs_fs__parse_footer,
svn_fs_fs__unparse_footer): Declare new parser / generator API for the
f7 footer, similar to f6- revision trailer
but slightly simplified.
* subversion/libsvn_fs_fs/low_level.c
(svn_fs_fs__parse_footer,
svn_fs_fs__unparse_footer): Implement.
* subversion/libsvn_fs_fs/rev_file.h
(svn_fs_fs__revision_file_t): Add members for footer contents access
block size.
(svn_fs_fs__reopen_revision_file): Drop. Retry is no longer necessary.
(svn_fs_fs__auto_read_footer): Declare new function that reads the footer
if we didn't yet.
* subversion/libsvn_fs_fs/rev_file.c
(svn_fs_fs__reopen_revision_file): Drop.
(svn_fs_fs__auto_read_footer): Implement.
(svn_fs_fs__close_revision_file): Index streams don't carry any releasable
resources anymore.
* subversion/libsvn_fs_fs/index.h
(svn_fs_fs__packed_stream_close): Drop. There are no independent file
resources linked to the index data
streams anymore.
(svn_fs_fs__l2p_index_create,
svn_fs_fs__p2l_index_create): Rename to ...
(svn_fs_fs__l2p_index_append
svn_fs_fs__p2l_index_create): ... these and accept an open file instead
of a file name to be created.
* subversion/libsvn_fs_fs/index.c
(packed_stream_open): Instead of opening our own index file, accept an
already open one and use the specified offset range.
(svn_fs_fs__packed_stream_close): Drop.
(index_create): Drop. The files already exist.
(svn_fs_fs__l2p_index_create,
svn_fs_fs__p2l_index_create): Rename to ...
(svn_fs_fs__l2p_index_append
svn_fs_fs__p2l_index_create): ... these and use the already open file
instead of creating new ones.
(retry_open_l2p_index,
retry_open_p2l_index): No longer necessary.
(svn_fs_fs__item_offset,
svn_fs_fs__p2l_get_max_offset): No lookup retry necessary anymore.
(auto_open_l2p_index,
auto_open_p2l_index): Make sure we know the index location and then
open the stream based on the existing file.
* subversion/libsvn_fs_fs/transaction.h
(svn_fs_fs__add_index_data): New private API that combines index data
with the rep data file and finalizes it.
* subversion/libsvn_fs_fs/transaction.c
(svn_fs_fs__add_index_data): Implement.
(commit_body): Use the above.
* subversion/libsvn_fs_fs/fs_fs.c
(write_revision_zero): Update r0 template by simply appending the
index contents and a final footer.
* subversion/libsvn_fs_fs/cached_data.c
(block_read): Remove the now unnecessary retry-on-pack code.
* subversion/libsvn_fs_fs/hotcopy.c
(hotcopy_copy_shard_file): Remove index file copy code.
(hotcopy_copy_packed_shard,
hotcopy_revisions): Update callers.
* subversion/libsvn_fs_fs/pack.c
(close_pack_context): Use the new transaction.h API to finalize the
index data.
(pack_range): Instead of ending with the file, revision content now ends
where L2P index data starts.
* subversion/libsvn_fs_fs/util.h
(svn_fs_fs__path_l2p_index,
svn_fs_fs__path_p2l_index): Drop. These are no longer separate files.
* subversion/libsvn_fs_fs/util.c
(svn_fs_fs__path_l2p_index,
svn_fs_fs__path_p2l_index): Drop.
* subversion/libsvn_fs_fs/verify.c
(compare_p2l_to_rev): Rev contents now ends where the L2P index begins.
* subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c
(pack_filesystem): There are no separate index files anymore.
* subversion/tests/cmdline/svnadmin_tests.py
(read_l2p): Use contents of the rev file instead of a separate index file.
(set_changed_path_list): Parse footer and splice data correctly retaining
the index contents as-is.
* tools/server-side/svnfsfs/load-index-cmd.c
(write_p2l_index,
write_l2p_index): Instead of creating the final index files, create
the proto-index files only.
(load_index): Trim the old index from the pack / rev file and append
the new ones using our new transaction.h API.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/cached_data.c
subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
subversion/trunk/subversion/libsvn_fs_fs/hotcopy.c
subversion/trunk/subversion/libsvn_fs_fs/index.c
subversion/trunk/subversion/libsvn_fs_fs/index.h
subversion/trunk/subversion/libsvn_fs_fs/low_level.c
subversion/trunk/subversion/libsvn_fs_fs/low_level.h
subversion/trunk/subversion/libsvn_fs_fs/pack.c
subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
subversion/trunk/subversion/libsvn_fs_fs/rev_file.h
subversion/trunk/subversion/libsvn_fs_fs/transaction.c
subversion/trunk/subversion/libsvn_fs_fs/transaction.h
subversion/trunk/subversion/libsvn_fs_fs/util.c
subversion/trunk/subversion/libsvn_fs_fs/util.h
subversion/trunk/subversion/libsvn_fs_fs/verify.c
subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py
subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c
subversion/trunk/tools/server-side/svnfsfs/load-index-cmd.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/cached_data.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/cached_data.c?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/cached_data.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/cached_data.c Sat Jun 21 15:15:30 2014
@@ -3162,37 +3162,12 @@ block_read(void **result,
*/
do
{
- svn_error_t *err;
-
/* fetch list of items in the block surrounding OFFSET */
block_start = offset - (offset % ffd->block_size);
- err = svn_fs_fs__p2l_index_lookup(&entries, fs, revision_file,
- revision, block_start,
- ffd->block_size, scratch_pool);
-
- /* if the revision got packed in the meantime and we still need
- * to actually read some item, we retry the whole process */
- if (err)
- {
- /* We failed for the first time. Refresh cache & retry. */
- SVN_ERR(svn_fs_fs__update_min_unpacked_rev(fs, scratch_pool));
- if ( revision_file->is_packed
- != svn_fs_fs__is_packed_rev(fs, revision))
- {
- if (result && !*result)
- {
- SVN_ERR(svn_fs_fs__reopen_revision_file(revision_file, fs,
- revision));
- SVN_ERR(block_read(result, fs, revision, item_index,
- revision_file, result_pool,
- scratch_pool));
- }
-
- return SVN_NO_ERROR;
- }
- }
+ SVN_ERR(svn_fs_fs__p2l_index_lookup(&entries, fs, revision_file,
+ revision, block_start,
+ ffd->block_size, scratch_pool));
- SVN_ERR(err);
SVN_ERR(aligned_seek(fs, revision_file->file, &block_start, offset,
iterpool));
Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21 15:15:30 2014
@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
/* Write out a rev file for revision 0. */
if (svn_fs_fs__use_log_addressing(fs, 0))
- SVN_ERR(svn_io_file_create(path_revision_zero,
- "PLAIN\nEND\nENDREP\n"
- "id: 0.0.r0/2\n"
- "type: dir\n"
- "count: 0\n"
- "text: 0 3 4 4 "
- "2d2977d1c96f487abe4a1e202dd03b4e\n"
- "cpath: /\n"
- "\n\n", fs->pool));
+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
+ "PLAIN\nEND\nENDREP\n"
+ "id: 0.0.r0/2\n"
+ "type: dir\n"
+ "count: 0\n"
+ "text: 0 3 4 4 "
+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
+ "cpath: /\n"
+ "\n\n"
+
+ /* L2P index */
+ "\0\x80\x40" /* rev 0, 8k entries per page */
+ "\1\1\1" /* 1 rev, 1 page, 1 page in 1st rev */
+ "\6\4" /* page size: bytes, count */
+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
+
+ /* P2L index */
+ "\0\x6b" /* start rev, rev file size */
+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29 bytes */
+ "\0" /* offset entry 0 page 1 */
+ /* len, item & type, rev, checksum */
+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k page */
+
+ /* Footer */
+ "107 121\7",
+ 107 + 14 + 38 + 7 + 1, fs->pool));
else
SVN_ERR(svn_io_file_create(path_revision_zero,
"PLAIN\nEND\nENDREP\n"
@@ -1471,35 +1491,6 @@ write_revision_zero(svn_fs_t *fs)
SVN_ERR(svn_io_set_file_read_only(path_revision_zero, FALSE, fs->pool));
- if (svn_fs_fs__use_log_addressing(fs, 0))
- {
- const char *path = svn_fs_fs__path_l2p_index(fs, 0, FALSE, fs->pool);
- SVN_ERR(svn_io_file_create_binary
- (path,
- "\0\x80\x40" /* rev 0, 8k entries per page */
- "\1\1\1" /* 1 rev, 1 page, 1 page in 1st rev */
- "\6\4" /* page size: bytes, count */
- "\0\xd6\1\xb1\1\x21", /* phys offsets + 1 */
- 14,
- fs->pool));
- SVN_ERR(svn_io_set_file_read_only(path, FALSE, fs->pool));
-
- path = svn_fs_fs__path_p2l_index(fs, 0, FALSE, fs->pool);
- SVN_ERR(svn_io_file_create_binary
- (path,
- "\0\x6b" /* start rev, rev file size */
- "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29 bytes */
- "\0" /* offset entry 0 page 1 */
- /* len, item & type, rev, checksum */
- "\x11\x34\0\xf5\xd6\x8c\x81\x06"
- "\x59\x09\0\xc8\xfc\xf6\x81\x04"
- "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
- "\x95\xff\3\x1b\0\0", /* last entry fills up 64k page */
- 38,
- fs->pool));
- SVN_ERR(svn_io_set_file_read_only(path, FALSE, fs->pool));
- }
-
/* Set a date on revision 0. */
date.data = svn_time_to_cstring(apr_time_now(), fs->pool);
date.len = strlen(date.data);
Modified: subversion/trunk/subversion/libsvn_fs_fs/hotcopy.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/hotcopy.c?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/hotcopy.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/hotcopy.c Sat Jun 21 15:15:30 2014
@@ -228,14 +228,12 @@ hotcopy_io_copy_dir_recursively(const ch
/* Copy an un-packed revision or revprop file for revision REV from SRC_SUBDIR
* to DST_SUBDIR. Assume a sharding layout based on MAX_FILES_PER_DIR.
- * If INCLUDE_INDEXES is set, copy rev index files as well.
* Use SCRATCH_POOL for temporary allocations. */
static svn_error_t *
hotcopy_copy_shard_file(const char *src_subdir,
const char *dst_subdir,
svn_revnum_t rev,
int max_files_per_dir,
- svn_boolean_t include_indexes,
apr_pool_t *scratch_pool)
{
const char *src_subdir_shard = src_subdir,
@@ -260,18 +258,6 @@ hotcopy_copy_shard_file(const char *src_
apr_psprintf(scratch_pool, "%ld", rev),
scratch_pool));
- if (include_indexes)
- {
- SVN_ERR(hotcopy_io_dir_file_copy(src_subdir_shard, dst_subdir_shard,
- apr_psprintf(scratch_pool, "%ld.l2p",
- rev),
- scratch_pool));
- SVN_ERR(hotcopy_io_dir_file_copy(src_subdir_shard, dst_subdir_shard,
- apr_psprintf(scratch_pool, "%ld.p2l",
- rev),
- scratch_pool));
- }
-
return SVN_NO_ERROR;
}
@@ -327,7 +313,7 @@ hotcopy_copy_packed_shard(svn_revnum_t *
SVN_ERR(hotcopy_copy_shard_file(src_subdir, dst_subdir,
revprop_rev, max_files_per_dir,
- FALSE, iterpool));
+ iterpool));
}
svn_pool_destroy(iterpool);
}
@@ -336,7 +322,7 @@ hotcopy_copy_packed_shard(svn_revnum_t *
/* revprop for revision 0 will never be packed */
if (rev == 0)
SVN_ERR(hotcopy_copy_shard_file(src_subdir, dst_subdir,
- 0, max_files_per_dir, FALSE,
+ 0, max_files_per_dir,
scratch_pool));
/* packed revprops folder */
@@ -717,7 +703,6 @@ hotcopy_revisions(svn_revnum_t *dst_youn
/* Copy the rev file. */
err = hotcopy_copy_shard_file(src_revs_dir, dst_revs_dir,
rev, max_files_per_dir,
- svn_fs_fs__use_log_addressing(src_fs, rev),
iterpool);
if (err)
{
@@ -768,7 +753,7 @@ hotcopy_revisions(svn_revnum_t *dst_youn
/* Copy the revprop file. */
SVN_ERR(hotcopy_copy_shard_file(src_revprops_dir,
dst_revprops_dir,
- rev, max_files_per_dir, FALSE,
+ rev, max_files_per_dir,
iterpool));
/* After completing a full shard, update 'current'. */
Modified: subversion/trunk/subversion/libsvn_fs_fs/index.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/index.c?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/index.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/index.c Sat Jun 21 15:15:30 2014
@@ -313,28 +313,25 @@ packed_stream_read(svn_fs_fs__packed_num
return SVN_NO_ERROR;
}
-/* Create and open a packed number stream reading from FILE_NAME and
- * return it in *STREAM. Access the file in chunks of BLOCK_SIZE bytes.
- * Use POOL for allocations.
+/* Create and open a packed number stream reading from offsets START to
+ * END in FILE and return it in *STREAM. Access the file in chunks of
+ * BLOCK_SIZE bytes. Use POOL for allocations.
*/
static svn_error_t *
packed_stream_open(svn_fs_fs__packed_number_stream_t **stream,
- const char *file_name,
+ apr_file_t *file,
+ apr_off_t start,
+ apr_off_t end,
apr_size_t block_size,
apr_pool_t *pool)
{
svn_fs_fs__packed_number_stream_t *result
= apr_palloc(pool, sizeof(*result));
- result->pool = svn_pool_create(pool);
- SVN_ERR(svn_io_file_open(&result->file, file_name,
- APR_READ | APR_BUFFERED, APR_OS_DEFAULT,
- result->pool));
-
- result->stream_start = 0;
- result->stream_end = 0;
- SVN_ERR(svn_io_file_seek(result->file, APR_END, &result->stream_end,
- result->pool));
+ result->pool = pool;
+ result->file = file;
+ result->stream_start = start;
+ result->stream_end = end;
result->used = 0;
result->current = 0;
@@ -343,20 +340,6 @@ packed_stream_open(svn_fs_fs__packed_num
result->block_size = block_size;
*stream = result;
-
- return SVN_NO_ERROR;
-}
-
-/* Close STREAM which may be NULL.
- */
-svn_error_t *
-svn_fs_fs__packed_stream_close(svn_fs_fs__packed_number_stream_t *stream)
-{
- if (stream)
- {
- SVN_ERR(svn_io_file_close(stream->file, stream->pool));
- svn_pool_destroy(stream->pool);
- }
return SVN_NO_ERROR;
}
@@ -560,25 +543,9 @@ svn_fs_fs__l2p_proto_index_add_entry(apr
pool));
}
-static svn_error_t *
-index_create(apr_file_t **index_file, const char *file_name, apr_pool_t *pool)
-{
- /* remove any old index file
- * (it would probably be r/o and simply re-writing it would fail) */
- SVN_ERR(svn_io_remove_file2(file_name, TRUE, pool));
-
- /* We most likely own the write lock to the repo, so this should
- * either just work or fail indicating a serious problem. */
- SVN_ERR(svn_io_file_open(index_file, file_name,
- APR_WRITE | APR_CREATE | APR_BUFFERED,
- APR_OS_DEFAULT, pool));
-
- return SVN_NO_ERROR;
-}
-
svn_error_t *
-svn_fs_fs__l2p_index_create(svn_fs_t *fs,
- const char *file_name,
+svn_fs_fs__l2p_index_append(svn_fs_t *fs,
+ apr_file_t *index_file,
const char *proto_file_name,
svn_revnum_t revision,
apr_pool_t *pool)
@@ -588,7 +555,6 @@ svn_fs_fs__l2p_index_create(svn_fs_t *fs
int i;
apr_uint64_t entry;
svn_boolean_t eof = FALSE;
- apr_file_t *index_file;
unsigned char encoded[ENCODED_INT_LENGTH];
int last_page_count = 0; /* total page count at the start of
@@ -695,9 +661,6 @@ svn_fs_fs__l2p_index_create(svn_fs_t *fs
/* close the source file */
SVN_ERR(svn_io_file_close(proto_index, local_pool));
- /* create the target file */
- SVN_ERR(index_create(&index_file, file_name, local_pool));
-
/* Paranoia check that makes later casting to int32 safe.
* The current implementation is limited to 2G pages per index. */
if (page_counts->nelts > APR_INT32_MAX)
@@ -748,45 +711,13 @@ svn_fs_fs__l2p_index_create(svn_fs_t *fs
local_pool),
NULL, NULL, local_pool));
- /* finalize the index file */
- SVN_ERR(svn_io_file_close(index_file, local_pool));
- SVN_ERR(svn_io_set_file_read_only(file_name, FALSE, local_pool));
-
svn_pool_destroy(local_pool);
return SVN_NO_ERROR;
}
-/* Reopen the rev / pack file in REV_FILE and attempt to open the L2P
- * index for REVISION in FS.
- */
-static svn_error_t *
-retry_open_l2p_index(svn_fs_fs__revision_file_t *rev_file,
- svn_fs_t *fs,
- svn_revnum_t revision)
-{
- fs_fs_data_t *ffd = fs->fsap_data;
-
- /* reopen rep file if necessary */
- if (rev_file->file)
- SVN_ERR(svn_fs_fs__reopen_revision_file(rev_file, fs, revision));
- else
- rev_file->is_packed = svn_fs_fs__is_packed_rev(fs, revision);
-
- /* re-try opening this index (keep the p2l closed) */
- SVN_ERR(packed_stream_open(&rev_file->l2p_stream,
- svn_fs_fs__path_l2p_index(fs, revision,
- rev_file->is_packed,
- rev_file->pool),
- ffd->block_size,
- rev_file->pool));
-
- return SVN_NO_ERROR;
-}
-
/* If REV_FILE->L2P_STREAM is NULL, create a new stream for the log-to-phys
- * index for REVISION in FS and return it in REV_FILE. May fail if pack
- * status changed.
+ * index for REVISION in FS and return it in REV_FILE.
*/
static svn_error_t *
auto_open_l2p_index(svn_fs_fs__revision_file_t *rev_file,
@@ -796,10 +727,12 @@ auto_open_l2p_index(svn_fs_fs__revision_
if (rev_file->l2p_stream == NULL)
{
fs_fs_data_t *ffd = fs->fsap_data;
+
+ SVN_ERR(svn_fs_fs__auto_read_footer(rev_file));
SVN_ERR(packed_stream_open(&rev_file->l2p_stream,
- svn_fs_fs__path_l2p_index(fs, revision,
- rev_file->is_packed,
- rev_file->pool),
+ rev_file->file,
+ rev_file->l2p_offset,
+ rev_file->p2l_offset,
ffd->block_size,
rev_file->pool));
}
@@ -1571,26 +1504,8 @@ svn_fs_fs__item_offset(apr_off_t *absolu
else if (svn_fs_fs__use_log_addressing(fs, revision))
{
/* ordinary index lookup */
- err = l2p_index_lookup(absolute_position, fs, rev_file, revision,
- item_index, pool);
-
- if (err && !rev_file->is_packed)
- {
- /* REVISION might have been packed in the meantime.
- Refresh cache & retry. */
- err = svn_error_compose_create(err,
- svn_fs_fs__update_min_unpacked_rev
- (fs, pool));
-
- /* retry upon intermittent pack */
- if (svn_fs_fs__is_packed_rev(fs, revision) != rev_file->is_packed)
- {
- svn_error_clear(err);
- SVN_ERR(retry_open_l2p_index(rev_file, fs, revision));
- err = l2p_index_lookup(absolute_position, fs, rev_file,
- revision, item_index, pool);
- }
- }
+ SVN_ERR(l2p_index_lookup(absolute_position, fs, rev_file, revision,
+ item_index, pool));
}
else if (rev_file->is_packed)
{
@@ -1670,8 +1585,8 @@ svn_fs_fs__p2l_proto_index_next_offset(a
}
svn_error_t *
-svn_fs_fs__p2l_index_create(svn_fs_t *fs,
- const char *file_name,
+svn_fs_fs__p2l_index_append(svn_fs_t *fs,
+ apr_file_t *index_file,
const char *proto_file_name,
svn_revnum_t revision,
apr_pool_t *pool)
@@ -1681,7 +1596,6 @@ svn_fs_fs__p2l_index_create(svn_fs_t *fs
apr_file_t *proto_index = NULL;
int i;
svn_boolean_t eof = FALSE;
- apr_file_t *index_file;
unsigned char encoded[ENCODED_INT_LENGTH];
svn_revnum_t last_revision = revision;
apr_uint64_t last_compound = 0;
@@ -1802,9 +1716,6 @@ svn_fs_fs__p2l_index_create(svn_fs_t *fs
APR_ARRAY_PUSH(table_sizes, apr_uint64_t)
= svn_spillbuf__get_size(buffer) - last_buffer_size;
- /* create the target file */
- SVN_ERR(index_create(&index_file, file_name, local_pool));
-
/* write the start revision, file size and page size */
SVN_ERR(svn_io_file_write_full(index_file, encoded,
encode_uint(encoded, revision),
@@ -1834,44 +1745,14 @@ svn_fs_fs__p2l_index_create(svn_fs_t *fs
local_pool),
NULL, NULL, local_pool));
- /* finalize the index file */
- SVN_ERR(svn_io_file_close(index_file, local_pool));
- SVN_ERR(svn_io_set_file_read_only(file_name, FALSE, local_pool));
-
svn_pool_destroy(iter_pool);
svn_pool_destroy(local_pool);
return SVN_NO_ERROR;
}
-/* Reopen the rev / pack file in REV_FILE (if previously opened) using
- * the current pack status of REVISION in FS and open the suitable
- * P2L index. Close the L2P index.
- */
-static svn_error_t *
-retry_open_p2l_index(svn_fs_fs__revision_file_t *rev_file,
- svn_fs_t *fs,
- svn_revnum_t revision)
-{
- fs_fs_data_t *ffd = fs->fsap_data;
-
- /* reopen rep file if necessary */
- if (rev_file->file)
- SVN_ERR(svn_fs_fs__reopen_revision_file(rev_file, fs, revision));
-
- /* re-try opening this index (keep the l2p closed) */
- SVN_ERR(packed_stream_open(&rev_file->p2l_stream,
- svn_fs_fs__path_p2l_index(fs, revision,
- rev_file->is_packed,
- rev_file->pool),
- ffd->block_size,
- rev_file->pool));
-
- return SVN_NO_ERROR;
-}
-
/* If REV_FILE->P2L_STREAM is NULL, create a new stream for the phys-to-log
- * index for REVISION in FS using the rev / pack status provided by REV_FILE.
+ * index for REVISION in FS using the rev / pack file provided by REV_FILE.
*/
static svn_error_t *
auto_open_p2l_index(svn_fs_fs__revision_file_t *rev_file,
@@ -1881,10 +1762,12 @@ auto_open_p2l_index(svn_fs_fs__revision_
if (rev_file->p2l_stream == NULL)
{
fs_fs_data_t *ffd = fs->fsap_data;
+
+ SVN_ERR(svn_fs_fs__auto_read_footer(rev_file));
SVN_ERR(packed_stream_open(&rev_file->p2l_stream,
- svn_fs_fs__path_p2l_index(fs, revision,
- rev_file->is_packed,
- rev_file->pool),
+ rev_file->file,
+ rev_file->p2l_offset,
+ rev_file->footer_offset,
ffd->block_size, rev_file->pool));
}
@@ -2724,17 +2607,8 @@ svn_fs_fs__p2l_get_max_offset(apr_off_t
svn_revnum_t revision,
apr_pool_t *pool)
{
- svn_error_t *err = p2l_get_max_offset(offset, fs, rev_file, revision, pool);
-
- /* retry upon intermittent pack */
- if (err && svn_fs_fs__is_packed_rev(fs, revision) != rev_file->is_packed)
- {
- svn_error_clear(err);
- SVN_ERR(retry_open_p2l_index(rev_file, fs, revision));
- err = p2l_get_max_offset(offset, fs, rev_file, revision, pool);
- }
-
- return svn_error_trace(err);
+ return svn_error_trace(p2l_get_max_offset(offset, fs, rev_file, revision,
+ pool));
}
/*
Modified: subversion/trunk/subversion/libsvn_fs_fs/index.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/index.h?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/index.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/index.h Sat Jun 21 15:15:30 2014
@@ -70,10 +70,6 @@ typedef struct svn_fs_fs__p2l_entry_t
svn_fs_fs__id_part_t item;
} svn_fs_fs__p2l_entry_t;
-/* Close the index file STREAM and underlying file handle. */
-svn_error_t *
-svn_fs_fs__packed_stream_close(svn_fs_fs__packed_number_stream_t *stream);
-
/* Open / create a log-to-phys index file with the full file path name
* FILE_NAME. Return the open file in *PROTO_INDEX and use POOL for
* allocations.
@@ -105,14 +101,14 @@ svn_fs_fs__l2p_proto_index_add_entry(apr
apr_uint64_t item_index,
apr_pool_t *pool);
-/* Use the proto index file stored at PROTO_FILE_NAME and construct the
- * final log-to-phys index file at FILE_NAME. The first revision will
+/* Use the proto index file stored at PROTO_FILE_NAME, construct the final
+ * log-to-phys index and append it to INDEX_FILE. The first revision will
* be REVISION, entries to the next revision will be assigned to REVISION+1
* and so forth. Use POOL for allocations.
*/
svn_error_t *
-svn_fs_fs__l2p_index_create(svn_fs_t *fs,
- const char *file_name,
+svn_fs_fs__l2p_index_append(svn_fs_t *fs,
+ apr_file_t *index_file,
const char *proto_file_name,
svn_revnum_t revision,
apr_pool_t *pool);
@@ -145,14 +141,14 @@ svn_fs_fs__p2l_proto_index_next_offset(a
apr_file_t *proto_index,
apr_pool_t *pool);
-/* Use the proto index file stored at PROTO_FILE_NAME and construct the
- * final phys-to-log index file at FILE_NAME. Entries without a valid
+/* Use the proto index file stored at PROTO_FILE_NAME, construct the final
+ * phys-to-log index and append it to INDEX_FILE. Entries without a valid
* revision will be assigned to the REVISION given here.
* Use POOL for allocations.
*/
svn_error_t *
-svn_fs_fs__p2l_index_create(svn_fs_t *fs,
- const char *file_name,
+svn_fs_fs__p2l_index_append(svn_fs_t *fs,
+ apr_file_t *index_file,
const char *proto_file_name,
svn_revnum_t revision,
apr_pool_t *pool);
Modified: subversion/trunk/subversion/libsvn_fs_fs/low_level.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/low_level.c?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/low_level.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/low_level.c Sat Jun 21 15:15:30 2014
@@ -158,6 +158,41 @@ svn_fs_fs__unparse_revision_trailer(apr_
changes_offset);
}
+svn_error_t *
+svn_fs_fs__parse_footer(apr_off_t *l2p_offset,
+ apr_off_t *p2l_offset,
+ svn_stringbuf_t *footer,
+ svn_revnum_t rev)
+{
+ apr_int64_t val;
+
+ /* Split the footer into the 2 number strings. */
+ char *seperator = strchr(footer->data, ' ');
+ if (!seperator)
+ return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,
+ _("Revision file (r%ld) has corrupt footer"),
+ rev);
+ *seperator = '\0';
+
+ /* Convert offset values. */
+ SVN_ERR(svn_cstring_atoi64(&val, footer->data));
+ *l2p_offset = (apr_off_t)val;
+ SVN_ERR(svn_cstring_atoi64(&val, seperator + 1));
+ *p2l_offset = (apr_off_t)val;
+
+ return SVN_NO_ERROR;
+}
+
+svn_stringbuf_t *
+svn_fs_fs__unparse_footer(apr_off_t l2p_offset,
+ apr_off_t p2l_offset,
+ apr_pool_t *pool)
+{
+ return svn_stringbuf_createf(pool,
+ "%" APR_OFF_T_FMT " %" APR_OFF_T_FMT,
+ l2p_offset,
+ p2l_offset);
+}
/* Read the next entry in the changes record from file FILE and store
the resulting change in *CHANGE_P. If there is no next record,
Modified: subversion/trunk/subversion/libsvn_fs_fs/low_level.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/low_level.h?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/low_level.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/low_level.h Sat Jun 21 15:15:30 2014
@@ -61,6 +61,28 @@ svn_fs_fs__unparse_revision_trailer(apr_
apr_off_t changes_offset,
apr_pool_t *pool);
+/* Given the format 7+ revision / pack FOOTER, parse it destructively
+ * and return the start offsets of the index data in *L2P_OFFSET and
+ * *P2L_OFFSET, respectively.
+ *
+ * Note that REV is only used to construct nicer error objects that
+ * mention this revision.
+ */
+svn_error_t *
+svn_fs_fs__parse_footer(apr_off_t *l2p_offset,
+ apr_off_t *p2l_offset,
+ svn_stringbuf_t *footer,
+ svn_revnum_t rev);
+
+/* Given the offset of the L2P index data in L2P_OFFSET and the offset of
+ * the P2L index data in P2L_OFFSET, return the corresponding format 7+
+ * revision / pack file footer. Allocate it in POOL.
+ */
+svn_stringbuf_t *
+svn_fs_fs__unparse_footer(apr_off_t l2p_offset,
+ apr_off_t p2l_offset,
+ apr_pool_t *pool);
+
/* Read all the changes from STREAM and store them in *CHANGES. Do all
allocations in POOL. */
svn_error_t *
Modified: subversion/trunk/subversion/libsvn_fs_fs/pack.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/pack.c?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/pack.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/pack.c Sat Jun 21 15:15:30 2014
@@ -360,12 +360,6 @@ static svn_error_t *
close_pack_context(pack_context_t *context,
apr_pool_t *pool)
{
- const char *l2p_index_path
- = apr_pstrcat(pool, context->pack_file_path, PATH_EXT_L2P_INDEX,
- SVN_VA_NULL);
- const char *p2l_index_path
- = apr_pstrcat(pool, context->pack_file_path, PATH_EXT_P2L_INDEX,
- SVN_VA_NULL);
const char *proto_l2p_index_path;
const char *proto_p2l_index_path;
@@ -374,18 +368,17 @@ close_pack_context(pack_context_t *conte
context->proto_l2p_index, pool));
SVN_ERR(svn_io_file_name_get(&proto_p2l_index_path,
context->proto_p2l_index, pool));
-
+
/* finalize proto index files */
SVN_ERR(svn_io_file_close(context->proto_l2p_index, pool));
SVN_ERR(svn_io_file_close(context->proto_p2l_index, pool));
- /* Create the actual index files*/
- SVN_ERR(svn_fs_fs__l2p_index_create(context->fs, l2p_index_path,
- proto_l2p_index_path,
- context->shard_rev, pool));
- SVN_ERR(svn_fs_fs__p2l_index_create(context->fs, p2l_index_path,
- proto_p2l_index_path,
- context->shard_rev, pool));
+ /* Append the actual index data to the pack file. */
+ SVN_ERR(svn_fs_fs__add_index_data(context->fs, context->pack_file,
+ proto_l2p_index_path,
+ proto_p2l_index_path,
+ context->shard_rev,
+ pool));
/* remove proto index files */
SVN_ERR(svn_io_remove_file2(proto_l2p_index_path, FALSE, pool));
@@ -1294,26 +1287,21 @@ pack_range(pack_context_t *context,
for (revision = context->start_rev; revision < context->end_rev; ++revision)
{
apr_off_t offset = 0;
- apr_finfo_t finfo;
svn_fs_fs__revision_file_t *rev_file;
- const char *path;
svn_pool_clear(revpool);
- /* Get the size of the file. */
- path = svn_dirent_join(context->shard_dir,
- apr_psprintf(revpool, "%ld", revision),
- revpool);
- SVN_ERR(svn_io_stat(&finfo, path, APR_FINFO_SIZE, revpool));
+ /* Get the rev file dimensions (mainly index locations). */
SVN_ERR(svn_fs_fs__open_pack_or_rev_file(&rev_file, context->fs,
revision, revpool));
+ SVN_ERR(svn_fs_fs__auto_read_footer(rev_file));
/* store the indirect array index */
APR_ARRAY_PUSH(context->rev_offsets, int) = context->reps->nelts;
/* read the phys-to-log index file until we covered the whole rev file.
* That index contains enough info to build both target indexes from it. */
- while (offset < finfo.size)
+ while (offset < rev_file->l2p_offset)
{
/* read one cluster */
int i;
@@ -1339,7 +1327,7 @@ pack_range(pack_context_t *context,
/* process entry while inside the rev file */
offset = entry->offset;
- if (offset < finfo.size)
+ if (offset < rev_file->l2p_offset)
{
SVN_ERR(svn_io_file_seek(rev_file->file, SEEK_SET, &offset,
iterpool2));
Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21 15:15:30 2014
@@ -23,6 +23,7 @@
#include "rev_file.h"
#include "fs_fs.h"
#include "index.h"
+#include "low_level.h"
#include "util.h"
#include "../libsvn_fs/fs-loader.h"
@@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
file->stream = NULL;
file->p2l_stream = NULL;
file->l2p_stream = NULL;
+ file->block_size = ffd->block_size;
+ file->l2p_offset = -1;
+ file->p2l_offset = -1;
+ file->footer_offset = -1;
file->pool = pool;
}
@@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
}
svn_error_t *
-svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
- svn_fs_t *fs,
- svn_revnum_t rev)
+svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
{
- if (file->file)
- svn_fs_fs__close_revision_file(file);
+ if (file->l2p_offset == -1)
+ {
+ apr_off_t filesize = 0;
+ unsigned char footer_length;
+ svn_stringbuf_t *footer;
+
+ /* Determine file size. */
+ SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize, file->pool));
+
+ /* Read last byte (containing the length of the footer). */
+ SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size, NULL,
+ filesize - 1, file->pool));
+ SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
+ sizeof(footer_length), NULL, NULL,
+ file->pool));
+
+ /* Read footer. */
+ footer = svn_stringbuf_create_ensure(footer_length, file->pool);
+ SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size, NULL,
+ filesize - 1 - footer_length,
+ file->pool));
+ SVN_ERR(svn_io_file_read_full2(file->file, footer->data, footer_length,
+ &footer->len, NULL, file->pool));
+ footer->data[footer->len] = '\0';
+
+ /* Extract index locations. */
+ SVN_ERR(svn_fs_fs__parse_footer(&file->l2p_offset, &file->p2l_offset,
+ footer, file->start_revision));
+ file->footer_offset = filesize - footer_length - 1;
+ }
- return svn_error_trace(open_pack_or_rev_file(file, fs, rev, file->pool));
+ return SVN_NO_ERROR;
}
svn_error_t *
@@ -165,9 +196,6 @@ svn_fs_fs__close_revision_file(svn_fs_fs
if (file->file)
SVN_ERR(svn_io_file_close(file->file, file->pool));
- SVN_ERR(svn_fs_fs__packed_stream_close(file->l2p_stream));
- SVN_ERR(svn_fs_fs__packed_stream_close(file->p2l_stream));
-
file->file = NULL;
file->stream = NULL;
file->l2p_stream = NULL;
Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.h?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/rev_file.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.h Sat Jun 21 15:15:30 2014
@@ -38,7 +38,9 @@
typedef struct svn_fs_fs__packed_number_stream_t
svn_fs_fs__packed_number_stream_t;
-/* All files and associated properties for START_REVISION.
+/* Data file, including indexes data, and associated properties for
+ * START_REVISION. As the FILE is kept open, background pack operations
+ * will not cause access to this file to fail.
*/
typedef struct svn_fs_fs__revision_file_t
{
@@ -49,18 +51,37 @@ typedef struct svn_fs_fs__revision_file_
/* the revision was packed when the first file / stream got opened */
svn_boolean_t is_packed;
- /* rev / pack file or NULL if not opened, yet */
+ /* rev / pack file */
apr_file_t *file;
/* stream based on FILE and not NULL exactly when FILE is not NULL */
svn_stream_t *stream;
- /* the opened P2L index or NULL. Always NULL for txns. */
+ /* the opened P2L index stream or NULL. Always NULL for txns. */
svn_fs_fs__packed_number_stream_t *p2l_stream;
- /* the opened L2P index or NULL. Always NULL for txns. */
+ /* the opened L2P index stream or NULL. Always NULL for txns. */
svn_fs_fs__packed_number_stream_t *l2p_stream;
+ /* Copied from FS->FFD->BLOCK_SIZE upon creation. It allows us to
+ * use aligned seek() without having the FS handy. */
+ apr_off_t block_size;
+
+ /* Offset within FILE at which the rev data ends and the L2P index
+ * data starts. Less than P2L_OFFSET. -1 if svn_fs_fs__auto_read_footer
+ * has not been called, yet. */
+ apr_off_t l2p_offset;
+
+ /* Offset within FILE at which the L2P index ends and the P2L index
+ * data starts. Greater than L2P_OFFSET. -1 if svn_fs_fs__auto_read_footer
+ * has not been called, yet. */
+ apr_off_t p2l_offset;
+
+ /* Offset within FILE at which the P2L index ends and the footer starts.
+ * Greater than P2L_OFFSET. -1 if svn_fs_fs__auto_read_footer has not
+ * been called, yet. */
+ apr_off_t footer_offset;
+
/* pool containing this object */
apr_pool_t *pool;
} svn_fs_fs__revision_file_t;
@@ -75,15 +96,13 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
svn_revnum_t rev,
apr_pool_t *pool);
-/* Close previous files as well as streams in FILE (if open) and open the
- * rev / pack file for REVISION in FS. This is useful when a pack operation
- * made the current files outdated or no longer available and the caller
- * wants to keep the same revision file data structure.
+/* If the footer data in FILE has not been read, yet, do so now.
+ * Index locations will only be read upon request as we assume they get
+ * cached and the FILE is usually used for REP data access only.
+ * Hence, the separate step.
*/
svn_error_t *
-svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
- svn_fs_t *fs,
- svn_revnum_t revision);
+svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file);
/* Open the proto-rev file of transaction TXN_ID in FS and return it in *FILE.
* Use POOL for allocations. */
Modified: subversion/trunk/subversion/libsvn_fs_fs/transaction.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/transaction.c?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/transaction.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/transaction.c Sat Jun 21 15:15:30 2014
@@ -3540,6 +3540,42 @@ write_final_revprop(const char **path,
return SVN_NO_ERROR;
}
+svn_error_t *
+svn_fs_fs__add_index_data(svn_fs_t *fs,
+ apr_file_t *file,
+ const char *l2p_proto_index,
+ const char *p2l_proto_index,
+ svn_revnum_t revision,
+ apr_pool_t *pool)
+{
+ apr_off_t l2p_offset;
+ apr_off_t p2l_offset;
+ svn_stringbuf_t *footer;
+ unsigned char footer_length;
+
+ /* Append the actual index data to the pack file. */
+ l2p_offset = 0;
+ SVN_ERR(svn_io_file_seek(file, APR_CUR, &l2p_offset, pool));
+ SVN_ERR(svn_fs_fs__l2p_index_append(fs, file, l2p_proto_index, revision,
+ pool));
+
+ p2l_offset = 0;
+ SVN_ERR(svn_io_file_seek(file, APR_CUR, &p2l_offset, pool));
+ SVN_ERR(svn_fs_fs__p2l_index_append(fs, file, p2l_proto_index, revision,
+ pool));
+
+ /* Append footer. */
+ footer = svn_fs_fs__unparse_footer(l2p_offset, p2l_offset, pool);
+ SVN_ERR(svn_io_file_write_full(file, footer->data, footer->len, NULL,
+ pool));
+
+ footer_length = footer->len;
+ SVN_ERR_ASSERT(footer_length == footer->len);
+ SVN_ERR(svn_io_file_write_full(file, &footer_length, 1, NULL, pool));
+
+ return SVN_NO_ERROR;
+}
+
/* Baton used for commit_body below. */
struct commit_baton {
svn_revnum_t *new_rev_p;
@@ -3638,6 +3674,15 @@ commit_body(void *baton, apr_pool_t *poo
SVN_ERR(svn_io_file_write_full(proto_file, trailer->data, trailer->len,
NULL, pool));
}
+ else
+ {
+ /* Append the index data to the rev file. */
+ SVN_ERR(svn_fs_fs__add_index_data(cb->fs, proto_file,
+ svn_fs_fs__path_l2p_proto_index(cb->fs, txn_id, pool),
+ svn_fs_fs__path_p2l_proto_index(cb->fs, txn_id, pool),
+ new_rev, pool));
+ }
+
SVN_ERR(svn_io_file_flush_to_disk(proto_file, pool));
SVN_ERR(svn_io_file_close(proto_file, pool));
@@ -3683,20 +3728,6 @@ commit_body(void *baton, apr_pool_t *poo
}
}
- if (svn_fs_fs__use_log_addressing(cb->fs, new_rev))
- {
- /* Convert the index files from the proto format into their form
- in their final location */
- SVN_ERR(svn_fs_fs__l2p_index_create(cb->fs,
- svn_fs_fs__path_l2p_index(cb->fs, new_rev, FALSE, pool),
- svn_fs_fs__path_l2p_proto_index(cb->fs, txn_id, pool),
- new_rev, pool));
- SVN_ERR(svn_fs_fs__p2l_index_create(cb->fs,
- svn_fs_fs__path_p2l_index(cb->fs, new_rev, FALSE, pool),
- svn_fs_fs__path_p2l_proto_index(cb->fs, txn_id, pool),
- new_rev, pool));
- }
-
/* Move the finished rev file into place.
### This "breaks" the transaction by removing the protorev file
Modified: subversion/trunk/subversion/libsvn_fs_fs/transaction.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/transaction.h?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/transaction.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/transaction.h Sat Jun 21 15:15:30 2014
@@ -198,6 +198,18 @@ svn_fs_fs__set_proplist(svn_fs_t *fs,
apr_hash_t *proplist,
apr_pool_t *pool);
+/* Append the L2P and P2L indexes given by their proto index file names
+ * L2P_PROTO_INDEX and P2L_PROTO_INDEX to the revision / pack FILE.
+ * The latter contains revision(s) starting at REVISION in FS.
+ * Use POOL for temporary allocations. */
+svn_error_t *
+svn_fs_fs__add_index_data(svn_fs_t *fs,
+ apr_file_t *file,
+ const char *l2p_proto_index,
+ const char *p2l_proto_index,
+ svn_revnum_t revision,
+ apr_pool_t *pool);
+
/* Commit the transaction TXN in filesystem FS and return its new
revision number in *REV. If the transaction is out of date, return
the error SVN_ERR_FS_TXN_OUT_OF_DATE. Use POOL for temporary
Modified: subversion/trunk/subversion/libsvn_fs_fs/util.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/util.c?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/util.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/util.c Sat Jun 21 15:15:30 2014
@@ -154,26 +154,6 @@ path_rev_absolute_internal(svn_fs_t *fs,
}
const char *
-svn_fs_fs__path_l2p_index(svn_fs_t *fs,
- svn_revnum_t rev,
- svn_boolean_t packed,
- apr_pool_t *pool)
-{
- return apr_psprintf(pool, "%s" PATH_EXT_L2P_INDEX,
- path_rev_absolute_internal(fs, rev, packed, pool));
-}
-
-const char *
-svn_fs_fs__path_p2l_index(svn_fs_t *fs,
- svn_revnum_t rev,
- svn_boolean_t packed,
- apr_pool_t *pool)
-{
- return apr_psprintf(pool, "%s" PATH_EXT_P2L_INDEX,
- path_rev_absolute_internal(fs, rev, packed, pool));
-}
-
-const char *
svn_fs_fs__path_rev_absolute(svn_fs_t *fs,
svn_revnum_t rev,
apr_pool_t *pool)
Modified: subversion/trunk/subversion/libsvn_fs_fs/util.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/util.h?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/util.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/util.h Sat Jun 21 15:15:30 2014
@@ -176,24 +176,6 @@ svn_fs_fs__path_revprops(svn_fs_t *fs,
svn_revnum_t rev,
apr_pool_t *pool);
-/* Return the path of the file containing the log-to-phys index for the
- * file containing revision REV in FS. The result will be allocated in POOL.
- */
-const char *
-svn_fs_fs__path_l2p_index(svn_fs_t *fs,
- svn_revnum_t rev,
- svn_boolean_t packed,
- apr_pool_t *pool);
-
-/* Return the path of the file containing the phys-to-log index for the
- * file containing revision REV in FS. The result will be allocated in POOL.
- */
-const char *
-svn_fs_fs__path_p2l_index(svn_fs_t *fs,
- svn_revnum_t rev,
- svn_boolean_t packed,
- apr_pool_t *pool);
-
/* Return the path of the file storing the oldest non-packed revision in FS.
* The result will be allocated in POOL.
*/
Modified: subversion/trunk/subversion/libsvn_fs_fs/verify.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/verify.c?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/verify.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/verify.c Sat Jun 21 15:15:30 2014
@@ -503,15 +503,15 @@ compare_p2l_to_rev(svn_fs_t *fs,
SVN_ERR(svn_fs_fs__open_pack_or_rev_file(&rev_file, fs, start, pool));
/* check file size vs. range covered by index */
- SVN_ERR(svn_io_file_seek(rev_file->file, APR_END, &offset, pool));
+ SVN_ERR(svn_fs_fs__auto_read_footer(rev_file));
SVN_ERR(svn_fs_fs__p2l_get_max_offset(&max_offset, fs, rev_file, start,
pool));
- if (offset != max_offset)
+ if (rev_file->l2p_offset != max_offset)
return svn_error_createf(SVN_ERR_FS_ITEM_INDEX_INCONSISTENT, NULL,
_("File size of %s for revision r%ld does "
"not match p2l index size of %s"),
- apr_off_t_toa(pool, offset), start,
+ apr_off_t_toa(pool, rev_file->l2p_offset), start,
apr_off_t_toa(pool, max_offset));
SVN_ERR(svn_io_file_aligned_seek(rev_file->file, ffd->block_size, NULL, 0,
Modified: subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py Sat Jun 21 15:15:30 2014
@@ -262,16 +262,9 @@ def load_dumpstream(sbox, dump, *varargs
return load_and_verify_dumpstream(sbox, None, None, None, False, dump,
*varargs)
-def read_l2p(sbox, revision, item):
- """ For the format 7+ repository in SBOX, return the physical offset
- of ITEM in REVISION. This code supports only small, nonpacked revs. """
-
- filename = fsfs_file(sbox.repo_dir, 'revs', str(revision) + ".l2p")
-
- fp = open(filename, 'rb')
- contents = fp.read()
- length = len(contents)
- fp.close()
+def read_l2p(contents, item):
+ """ For the format 7+ revision file CONTENTS, return the physical offset
+ of ITEM. This code supports only small, nonpacked revs. """
# decode numbers
numbers = []
@@ -320,10 +313,10 @@ def set_changed_path_list(sbox, revision
# read full file
fp = open(fsfs_file(sbox.repo_dir, 'revs', str(revision)), 'r+b')
contents = fp.read()
+ length = len(contents)
if repo_format(sbox) < 7:
# replace the changed paths list
- length = len(contents)
header = contents[contents.rfind('\n', length - 64, length - 1):]
body_len = long(header.split(' ')[1])
@@ -331,8 +324,24 @@ def set_changed_path_list(sbox, revision
# we will invalidate the l2p index but that's ok for the
# kind of tests we run here. The p2l index remains valid
# because the offset of the last item does not change
- body_len = read_l2p(sbox, revision, 1)
- header = '\n'
+
+ # read & parse revision file footer
+ footer_length = ord(contents[length-1]);
+ footer = contents[length - footer_length - 1:length-1]
+ l2p_offset = long(footer.split(' ')[0])
+ p2l_offset = long(footer.split(' ')[1])
+
+ # split file contents
+ body_len = read_l2p(contents[l2p_offset:], 1)
+ indexes = contents[l2p_offset:length - footer_length - 1]
+
+ # construct new footer, include indexes as are
+ file_len = body_len + len(changes) + 1
+ p2l_offset += file_len - l2p_offset
+
+ header = str(file_len) + ' ' + str(p2l_offset)
+ header += chr(len(header))
+ header = '\n' + indexes + header
contents = contents[:body_len] + changes + header
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=1604421&r1=1604420&r2=1604421&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 Sat Jun 21 15:15:30 2014
@@ -347,26 +347,6 @@ pack_filesystem(const svn_test_opts_t *o
"Expected manifest file '%s' not found",
path);
}
- else
- {
- path = svn_dirent_join_many(pool, REPO_NAME, "revs",
- apr_psprintf(pool, "%d.pack", i / SHARD_SIZE),
- "pack.l2p", SVN_VA_NULL);
- SVN_ERR(svn_io_check_path(path, &kind, pool));
- if (kind != svn_node_file)
- return svn_error_createf(SVN_ERR_FS_GENERAL, NULL,
- "Expected log-to-phys index file '%s' not found",
- path);
-
- path = svn_dirent_join_many(pool, REPO_NAME, "revs",
- apr_psprintf(pool, "%d.pack", i / SHARD_SIZE),
- "pack.p2l", NULL);
- SVN_ERR(svn_io_check_path(path, &kind, pool));
- if (kind != svn_node_file)
- return svn_error_createf(SVN_ERR_FS_GENERAL, NULL,
- "Expected phys-to-log index file '%s' not found",
- path);
- }
/* This directory should not exist. */
path = svn_dirent_join_many(pool, REPO_NAME, "revs",
Modified: subversion/trunk/tools/server-side/svnfsfs/load-index-cmd.c
URL: http://svn.apache.org/viewvc/subversion/trunk/tools/server-side/svnfsfs/load-index-cmd.c?rev=1604421&r1=1604420&r2=1604421&view=diff
==============================================================================
--- subversion/trunk/tools/server-side/svnfsfs/load-index-cmd.c (original)
+++ subversion/trunk/tools/server-side/svnfsfs/load-index-cmd.c Sat Jun 21 15:15:30 2014
@@ -28,6 +28,7 @@
#include "private/svn_sorts_private.h"
#include "../../../subversion/libsvn_fs_fs/index.h"
+#include "../../../subversion/libsvn_fs_fs/transaction.h"
#include "../../../subversion/libsvn_fs_fs/util.h"
#include "svn_private_config.h"
@@ -77,35 +78,33 @@ calc_fnv1(svn_fs_fs__p2l_entry_t *entry,
return SVN_NO_ERROR;
}
-/* In FS, (re-)write the P2L index for the revision file starting at
- * REVISION. All entries to write are of type svn_fs_fs__p2l_entry_t* and
- * given in ENTRIES. The FVN1 checksums are not taken from ENTRIES but are
- * begin calculated as we go. Use POOL for temporary allocations.
+/* For FS, create a new P2L auto-deleting proto index file in POOL and return
+ * its name in *INDEX_NAME. All entries to write are given in ENTRIES and
+ * of type svn_fs_fs__p2l_entry_t*. The FVN1 checksums are not taken from
+ * ENTRIES but are begin calculated from the current contents of REV_FILE
+ * as we go. Use POOL for allocations.
*/
static svn_error_t *
-write_p2l_index(svn_fs_t *fs,
- svn_revnum_t revision,
+write_p2l_index(const char **index_name,
+ svn_fs_t *fs,
+ svn_fs_fs__revision_file_t *rev_file,
apr_array_header_t *entries,
apr_pool_t *pool)
{
apr_file_t *proto_index;
- const char *protoname, *filename;
/* Use a subpool for immediate temp file cleanup at the end of this
* function. */
- apr_pool_t *subpool = svn_pool_create(pool);
apr_pool_t *iterpool = svn_pool_create(pool);
- svn_fs_fs__revision_file_t *rev_file;
int i;
/* Create a proto-index file. */
- SVN_ERR(svn_io_open_unique_file3(NULL, &protoname, NULL,
+ SVN_ERR(svn_io_open_unique_file3(NULL, index_name, NULL,
svn_io_file_del_on_pool_cleanup,
- subpool, subpool));
- SVN_ERR(svn_fs_fs__p2l_proto_index_open(&proto_index, protoname, subpool));
+ pool, iterpool));
+ SVN_ERR(svn_fs_fs__p2l_proto_index_open(&proto_index, *index_name, pool));
/* Write ENTRIES to proto-index file and calculate checksums as we go. */
- SVN_ERR(svn_fs_fs__open_pack_or_rev_file(&rev_file, fs, revision, pool));
for (i = 0; i < entries->nelts; ++i)
{
svn_fs_fs__p2l_entry_t *entry
@@ -121,15 +120,10 @@ write_p2l_index(svn_fs_t *fs,
* Note that REV_FILE contains the start revision of the shard file if it
* has been packed while REVISION may be somewhere in the middle. For
* non-packed shards, they will have identical values. */
- SVN_ERR(svn_io_file_flush(proto_index, iterpool));
- filename = svn_fs_fs__path_p2l_index(fs, rev_file->start_revision,
- rev_file->is_packed, subpool);
- SVN_ERR(svn_fs_fs__p2l_index_create(fs, filename, protoname,
- rev_file->start_revision, subpool));
+ SVN_ERR(svn_io_file_close(proto_index, iterpool));
/* Temp file cleanup. */
svn_pool_destroy(iterpool);
- svn_pool_destroy(subpool);
return SVN_NO_ERROR;
}
@@ -151,22 +145,22 @@ compare_p2l_entry_revision(const void *l
return lhs_entry->item.revision == rhs_entry->item.revision ? 0 : 1;
}
-/* In FS, (re-)write the L2P index with all entries taken from ENTRIES.
- * The revisions found in ENTRIES define the index file to write. The
- * entries are given as svn_fs_fs__p2l_entry_t* (sic!) and the ENTRIES
- * array will be reordered. Use POOL for temporary allocations.
+/* For FS, create a new L2P auto-deleting proto index file in POOL and return
+ * its name in *INDEX_NAME. All entries to write are given in ENTRIES and
+ * entries are of type svn_fs_fs__p2l_entry_t* (sic!). The ENTRIES array
+ * will be reordered. Use POOL for allocations.
*/
static svn_error_t *
-write_l2p_index(svn_fs_t *fs,
+write_l2p_index(const char **index_name,
+ svn_fs_t *fs,
apr_array_header_t *entries,
apr_pool_t *pool)
{
apr_file_t *proto_index;
- const char *protoname, *filename;
+ const char *filename;
/* Use a subpool for immediate temp file cleanup at the end of this
* function. */
- apr_pool_t *subpool = svn_pool_create(pool);
apr_pool_t *iterpool = svn_pool_create(pool);
int i;
svn_revnum_t last_revision = SVN_INVALID_REVNUM;
@@ -183,10 +177,11 @@ write_l2p_index(svn_fs_t *fs,
->item.revision;
/* Create the temporary proto-rev file. */
- SVN_ERR(svn_io_open_unique_file3(NULL, &protoname, NULL,
+ SVN_ERR(svn_io_open_unique_file3(NULL, index_name, NULL,
svn_io_file_del_on_pool_cleanup,
- subpool, subpool));
- SVN_ERR(svn_fs_fs__l2p_proto_index_open(&proto_index, protoname, subpool));
+ pool, iterpool));
+ SVN_ERR(svn_fs_fs__l2p_proto_index_open(&proto_index, *index_name,
+ iterpool));
/* Write all entries. */
for (i = 0; i < entries->nelts; ++i)
@@ -211,16 +206,10 @@ write_l2p_index(svn_fs_t *fs,
}
/* Convert proto-index into final index and move it into position. */
- SVN_ERR(svn_io_file_flush(proto_index, iterpool));
- filename = svn_fs_fs__path_l2p_index(fs, revision,
- svn_fs_fs__is_packed_rev(fs, revision),
- subpool);
- SVN_ERR(svn_fs_fs__l2p_index_create(fs, filename, protoname, revision,
- subpool));
+ SVN_ERR(svn_io_file_close(proto_index, iterpool));
/* Temp file cleanup. */
svn_pool_destroy(iterpool);
- svn_pool_destroy(subpool);
return SVN_NO_ERROR;
}
@@ -371,8 +360,26 @@ load_index(const char *path,
/* Treat an empty array as a no-op instead error. */
if (entries->nelts != 0)
{
- SVN_ERR(write_p2l_index(fs, revision, entries, pool));
- SVN_ERR(write_l2p_index(fs, entries, pool));
+ const char *l2p_proto_index;
+ const char *p2l_proto_index;
+ svn_fs_fs__revision_file_t *rev_file;
+
+ /* Open rev / pack file & trim indexes + footer off it. */
+ SVN_ERR(svn_fs_fs__open_pack_or_rev_file(&rev_file, fs, revision,
+ iterpool));
+ SVN_ERR(svn_fs_fs__auto_read_footer(rev_file));
+ SVN_ERR(svn_io_file_trunc(rev_file->file, rev_file->l2p_offset,
+ iterpool));
+
+ /* Create proto index files for the new index data
+ * (will be cleaned up automatically with iterpool). */
+ SVN_ERR(write_p2l_index(&p2l_proto_index, fs, rev_file, entries,
+ iterpool));
+ SVN_ERR(write_l2p_index(&l2p_proto_index, fs, entries, iterpool));
+
+ /* Combine rev data with new index data. */
+ SVN_ERR(svn_fs_fs__add_index_data(fs, rev_file->file, l2p_proto_index,
+ p2l_proto_index, revision, iterpool));
}
svn_pool_destroy(iterpool);
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/ subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Julian Foad <ju...@btopenworld.com>.
Hi Stefan.
Compared with the extent of what you have achieved, this is a minor thing, so I don't want to go on about it much.
However the point about readable code is not just whether it will need to be changed within the lifetime of "format 7" deployments, but about whether other software engineers have the reasonable ability to read the code, take it apart and put it together "in new and interesting ways". That's the part that's difficult here: if I want to experiment with "format 8" changes here, I would have a hard time putting this section of code back together.
Instructions how to run some external tool(s) to generate this constant would be sufficient to address my concern.
FWIW I think your logical addressing work is excellent and I am really looking forward to having it available.
- Julian
Stefan Fuhrmann wrote:
> What Ivan is certainly aware of as he claims to know FSFS
> very well but you may not is, that this particular constant
> describes a template with API-like guarantees. I.e. once
> released, it must never, ever change. If it had to change
> for some future extension, an independent variant must
> be created and both currently available once be retained.
[...]
>So, Ivan made two claims:
>
>
>1. Unreadable.
> I tried to give a rough "translation" or "orientation"
> in the comments. The details can be found in the
> FSFS spec. If you want to see "plain" numbers, run
> 'svnfsfs dump-index'.
>
>
>2. Unmaintainable & error prone.
>
> That makes only sense if we are talking change
> scenarios (maybe as part of a bug fix). The only
>
> way there could be a change to that template is
>
> if it so broken that we cannot use it under any
>
> circumstance. Otherwise, once released, it is set
>
> in stone.
>
>
> I wrote the template by hand and changed it by hand,
> not fun but doable. What it learned is that even the
> slightest mistake will cause the test suite to fail and
> in many case just terminate the server tests.
>
>
>As this is the "empty revision 0", there is little gray
>area between "all contents (the root node) can be
>read plus verified" and "virtually nothing works". So,
>the only change scenario that I can see here is a
>change in the index structure before release.
>
>
>Ivan seems to have other scenarios in mind to (potentially)
>change that code that I must obviously miss. Therefore:
>"How?"
>
>
>Among everything you've written do you really have no code that could help to generate and write the indexes without having to spell it all out bit-for-bit by hand? I won't believe it if you say "no".
>>
>
>
>There is svnfsfs now.
>
>
>
>>[...]
>>
>>
>>> Changing the rev 0 template has never been a fun
>>> activity and wont become one in the foreseeable
>>> future.
>>
>> In 1.8.x it looked like this:
>>
>> SVN_ERR(svn_io_file_create(path_revision_zero,
>> "PLAIN\nEND\nENDREP\n"
>> "id: 0.0.r0/17\n"
>> "type: dir\n"
>> "count: 0\n"
>> "text: 0 0 4 4 "
>> "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> "cpath: /\n"
>> "\n\n17 107\n", fs->pool));
>>
>>
>> Yes it contained a few magic constants, some of which probably should have been
>> generated, but still it was *much* simpler.
>
> It is still there and has been unchanged since its
> original release in 1.1. And that is my point.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Jun 25, 2014 at 6:19 PM, Julian Foad <ju...@btopenworld.com>
wrote:
> Stefan Fuhrmann wrote:
>
> > Ivan Zhakov wrote:
> >> @@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >>>+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >>>+ "PLAIN\nEND\nENDREP\n"
> >>>+ "id: 0.0.r0/2\n"
> >>>+ "type: dir\n"
> >>>+ "count: 0\n"
> >>>+ "text: 0 3 4 4 "
> >>>+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >>>+ "cpath: /\n"
> >>>+ "\n\n"
> >>>+
> >>>+ /* L2P index */
> >>>+ "\0\x80\x40" /* rev 0, 8k entries per page
> */
> >>>+ "\1\1\1" /* 1 rev, 1 page, 1 page in
> 1st rev */
> >>>+ "\6\4" /* page size: bytes, count */
> >>>+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >>>+
> >>>+ /* P2L index */
> >>>+ "\0\x6b" /* start rev, rev file size */
> >>>+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
> bytes */
> >>>+ "\0" /* offset entry 0 page 1 */
> >>>+ /* len, item & type, rev,
> checksum */
> >>>+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >>>+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >>>+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >>>+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
> page */
> >>>+
> >>>+ /* Footer */
> >>>+ "107 121\7",
> >>>+ 107 + 14 + 38 + 7 + 1, fs->pool));
> >>
> >> This constant makes code unreadable, unmaintainable and very error
> prone.
> >
> > How?
>
> "How?" Really? REALLY? I don't know if Ivan was referring to the numeric
> constant or the string constant or both, but I take issue with the string
> constant.
In fact, "really" ;).
What Ivan is certainly aware of as he claims to know FSFS
very well but you may not is, that this particular constant
describes a template with API-like guarantees. I.e. once
released, it must never, ever change. If it had to change
for some future extension, an independent variant must
be created and both currently available once be retained.
So, Ivan made two claims:
1. Unreadable.
I tried to give a rough "translation" or "orientation"
in the comments. The details can be found in the
FSFS spec. If you want to see "plain" numbers, run
'svnfsfs dump-index'.
2. Unmaintainable & error prone.
That makes only sense if we are talking change
scenarios (maybe as part of a bug fix). The only
way there could be a change to that template is
if it so broken that we cannot use it under any
circumstance. Otherwise, once released, it is set
in stone.
I wrote the template by hand and changed it by hand,
not fun but doable. What it learned is that even the
slightest mistake will cause the test suite to fail and
in many case just terminate the server tests.
As this is the "empty revision 0", there is little gray
area between "all contents (the root node) can be
read plus verified" and "virtually nothing works". So,
the only change scenario that I can see here is a
change in the index structure before release.
Ivan seems to have other scenarios in mind to (potentially)
change that code that I must obviously miss. Therefore:
"How?"
> Among everything you've written do you really have no code that could help
> to generate and write the indexes without having to spell it all out
> bit-for-bit by hand? I won't believe it if you say "no".
>
There is svnfsfs now.
>
> [...]
>
> > Changing the rev 0 template has never been a fun
> > activity and wont become one in the foreseeable
> > future.
>
> In 1.8.x it looked like this:
>
> SVN_ERR(svn_io_file_create(path_revision_zero,
> "PLAIN\nEND\nENDREP\n"
> "id: 0.0.r0/17\n"
> "type: dir\n"
> "count: 0\n"
> "text: 0 0 4 4 "
> "2d2977d1c96f487abe4a1e202dd03b4e\n"
> "cpath: /\n"
> "\n\n17 107\n", fs->pool));
>
>
> Yes it contained a few magic constants, some of which probably should have
> been generated, but still it was *much* simpler.
>
It is still there and has been unchanged since its
original release in 1.1. And that is my point.
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/ subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Fuhrmann wrote:
> Ivan Zhakov wrote:
>> @@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>>>+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>>>+ "PLAIN\nEND\nENDREP\n"
>>>+ "id: 0.0.r0/2\n"
>>>+ "type: dir\n"
>>>+ "count: 0\n"
>>>+ "text: 0 3 4 4 "
>>>+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>>>+ "cpath: /\n"
>>>+ "\n\n"
>>>+
>>>+ /* L2P index */
>>>+ "\0\x80\x40" /* rev 0, 8k entries per page */
>>>+ "\1\1\1" /* 1 rev, 1 page, 1 page in 1st rev */
>>>+ "\6\4" /* page size: bytes, count */
>>>+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>>>+
>>>+ /* P2L index */
>>>+ "\0\x6b" /* start rev, rev file size */
>>>+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29 bytes */
>>>+ "\0" /* offset entry 0 page 1 */
>>>+ /* len, item & type, rev, checksum */
>>>+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>>>+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>>>+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>>>+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k page */
>>>+
>>>+ /* Footer */
>>>+ "107 121\7",
>>>+ 107 + 14 + 38 + 7 + 1, fs->pool));
>>
>> This constant makes code unreadable, unmaintainable and very error prone.
>
> How?
"How?" Really? REALLY? I don't know if Ivan was referring to the numeric constant or the string constant or both, but I take issue with the string constant. Among everything you've written do you really have no code that could help to generate and write the indexes without having to spell it all out bit-for-bit by hand? I won't believe it if you say "no".
[...]
> Changing the rev 0 template has never been a fun
> activity and wont become one in the foreseeable
> future.
In 1.8.x it looked like this:
SVN_ERR(svn_io_file_create(path_revision_zero,
"PLAIN\nEND\nENDREP\n"
"id: 0.0.r0/17\n"
"type: dir\n"
"count: 0\n"
"text: 0 0 4 4 "
"2d2977d1c96f487abe4a1e202dd03b4e\n"
"cpath: /\n"
"\n\n17 107\n", fs->pool));
Yes it contained a few magic constants, some of which probably should have been generated, but still it was *much* simpler.
- Julian
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Jul 4, 2014 at 6:29 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 30 June 2014 14:19, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> >
> > On Fri, Jun 27, 2014 at 12:55 PM, Ivan Zhakov <iv...@visualsvn.com>
> wrote:
> >>
> >> On 26 June 2014 19:08, Stefan Fuhrmann <st...@wandisco.com>
> >> wrote:
> >> >
> >> > Hi Ivan,
> >> >
> >> > I see three alternative ways to code that function
> >> >
> >> > 1. As hard coded string / byte sequence (current implementation).
> >> > Cons:
> >> > * Hard to write, hard to review by just looking at it (applies to time
> >> > until initial release only).
> >> > Pros:
> >> > * Explicitly coded as constant, deterring people from changing it.
> >> > * Independent of other code, i.e. unintended changes to the format /
> >> > encoding generated by the normal code usually become apparent
> >> > when running the test suite.
> >> >
> >> > 2. Use our txn code to write r0. This should be simple and might
> >> > at most require some special ID handling.
> >> > Cons:
> >> > * Generating incompatible r0s is likely not caught by our test suite
> >> > (assuming that reader and writer functions are in sync). Basically
> >> > all the risk that comes with dynamically generating a "constant".
> >> > * Test cases must make sure our backward compat repo creation
> >> > options create repos that can actually be used by old releases.
> >> > (we might want explicit test for that anyway, though)
> >> > Pros:
> >> > * No or very little special code for r0 (not sure, yet).
> >> > * Format / encoding changes don't require new r0 templates.
> >> >
> >> > 3. Write code to "stitch" r0 together, e.g. string_add(md5("END\n")).
> >> > Cons:
> >> > * No more robust than 1. but requires much more code.
> >> > * May _look_ easy to understand but an actual offline review is still
> >> > hard.
> >> > Pros:
> >> > * Widely independent of other code, although not as much as 1.
> >> >
> >> > Do you generally agree with the range of options and their assessment?
> >> Yes, I generally agree with range of options. The only other I have is
> >> do not implement log addressing in first place.
> >>
> >> > Which one would you pick and why?
> >> >
> >> It's hard to pick option without looking to code, but I would start
> >> with leaving string constant for revision body and then appending
> >> indexes data using API. I.e. somewhat modified (2).
> >
> >
> > r1606554 generates the index data dynamically now.
> >
> > It makes repo creation slightly more expensive but that
> > does not seem to affect our test suite in any significant
> > way. So, I think that is not an issue.
> >
> > Are you o.k. with the code as it stands now?
> >
> Now code is much better, but still not good as it could be. The
> semantic and implementation of function
> svn_fs_fs__open_pack_or_rev_file_writable() is really bad: it makes
> file writable and then changes it back to read only on pool cleanup.
>
What part of that is bad? As a courtesy to the user,
we mark all repository files as r/o in a hope that it
may help prevent accidental modification. But nothing
requires us to do so.
If we want to modify the revision files, e.g. to rewrite
the index data by tools like svn-fsfs, we need to reset
the r/o flag before we can open the file for writing.
Restoring the r/o flag upon pool cleanup is probably
the safest way wrt making sure it eventually happens.
One might discuss whether svn_fs_fs__close_revision_file
should reset the flag explicitly as well.
IOW, this function is already required for features not
linked to repo creation and has well-defined semantics.
The repo creation simply uses existing functionality.
> The code your committed works like this:
> 1. Open zero revision file for writing
> 2. Write revision content
> 3. Close revision file
> 4. Check that revision file doesn't have read only attribute
> 5. Make it writeable if needed and register pool cleanup handler
> 6. Open revision file for writing
> 7. Calculate required checksums
> 8. Append indexes to revision file
> 9. Close revision file.
> 10. Make revision file readonly
>
> But this could be implemented much better:
> 1. Open zero revision file for writing
> 2. Write revision content
> 3. Calculate required data without closing revision file
> 4. Append indexes to revision file
> 5. Close revision file
> 6. Make revision file readonly
>
I agree that this does less work, i.e. faster but it comes at
a price: we need to switch *one* of the index interfaces
from revision_file_t to apr_file_t, creating an asymmetry
and we have to update all callers (which are not all
covered by your patch).
More importantly, the patch does not make the code any
easier to review - which was the point of the exercise.
So, we are left with some code churn and worse interfaces
for the sake of a micro-optimization of a rare operation.
Something to which you have expressed utmost
opposition in the past.
Therefore, thank you for the feedback but I will not apply
that patch as is.
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Jul 4, 2014 at 6:29 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 30 June 2014 14:19, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> >
> > On Fri, Jun 27, 2014 at 12:55 PM, Ivan Zhakov <iv...@visualsvn.com>
> wrote:
> >>
> >> On 26 June 2014 19:08, Stefan Fuhrmann <st...@wandisco.com>
> >> wrote:
> >> >
> >> > Hi Ivan,
> >> >
> >> > I see three alternative ways to code that function
> >> >
> >> > 1. As hard coded string / byte sequence (current implementation).
> >> > Cons:
> >> > * Hard to write, hard to review by just looking at it (applies to time
> >> > until initial release only).
> >> > Pros:
> >> > * Explicitly coded as constant, deterring people from changing it.
> >> > * Independent of other code, i.e. unintended changes to the format /
> >> > encoding generated by the normal code usually become apparent
> >> > when running the test suite.
> >> >
> >> > 2. Use our txn code to write r0. This should be simple and might
> >> > at most require some special ID handling.
> >> > Cons:
> >> > * Generating incompatible r0s is likely not caught by our test suite
> >> > (assuming that reader and writer functions are in sync). Basically
> >> > all the risk that comes with dynamically generating a "constant".
> >> > * Test cases must make sure our backward compat repo creation
> >> > options create repos that can actually be used by old releases.
> >> > (we might want explicit test for that anyway, though)
> >> > Pros:
> >> > * No or very little special code for r0 (not sure, yet).
> >> > * Format / encoding changes don't require new r0 templates.
> >> >
> >> > 3. Write code to "stitch" r0 together, e.g. string_add(md5("END\n")).
> >> > Cons:
> >> > * No more robust than 1. but requires much more code.
> >> > * May _look_ easy to understand but an actual offline review is still
> >> > hard.
> >> > Pros:
> >> > * Widely independent of other code, although not as much as 1.
> >> >
> >> > Do you generally agree with the range of options and their assessment?
> >> Yes, I generally agree with range of options. The only other I have is
> >> do not implement log addressing in first place.
> >>
> >> > Which one would you pick and why?
> >> >
> >> It's hard to pick option without looking to code, but I would start
> >> with leaving string constant for revision body and then appending
> >> indexes data using API. I.e. somewhat modified (2).
> >
> >
> > r1606554 generates the index data dynamically now.
> >
> > It makes repo creation slightly more expensive but that
> > does not seem to affect our test suite in any significant
> > way. So, I think that is not an issue.
> >
> > Are you o.k. with the code as it stands now?
> >
> Now code is much better, but still not good as it could be. The
> semantic and implementation of function
> svn_fs_fs__open_pack_or_rev_file_writable() is really bad: it makes
> file writable and then changes it back to read only on pool cleanup.
>
What part of that is bad? As a courtesy to the user,
we mark all repository files as r/o in a hope that it
may help prevent accidental modification. But nothing
requires us to do so.
If we want to modify the revision files, e.g. to rewrite
the index data by tools like svn-fsfs, we need to reset
the r/o flag before we can open the file for writing.
Restoring the r/o flag upon pool cleanup is probably
the safest way wrt making sure it eventually happens.
One might discuss whether svn_fs_fs__close_revision_file
should reset the flag explicitly as well.
IOW, this function is already required for features not
linked to repo creation and has well-defined semantics.
The repo creation simply uses existing functionality.
> The code your committed works like this:
> 1. Open zero revision file for writing
> 2. Write revision content
> 3. Close revision file
> 4. Check that revision file doesn't have read only attribute
> 5. Make it writeable if needed and register pool cleanup handler
> 6. Open revision file for writing
> 7. Calculate required checksums
> 8. Append indexes to revision file
> 9. Close revision file.
> 10. Make revision file readonly
>
> But this could be implemented much better:
> 1. Open zero revision file for writing
> 2. Write revision content
> 3. Calculate required data without closing revision file
> 4. Append indexes to revision file
> 5. Close revision file
> 6. Make revision file readonly
>
I agree that this does less work, i.e. faster but it comes at
a price: we need to switch *one* of the index interfaces
from revision_file_t to apr_file_t, creating an asymmetry
and we have to update all callers (which are not all
covered by your patch).
More importantly, the patch does not make the code any
easier to review - which was the point of the exercise.
So, we are left with some code churn and worse interfaces
for the sake of a micro-optimization of a rare operation.
Something to which you have expressed utmost
opposition in the past.
Therefore, thank you for the feedback but I will not apply
that patch as is.
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 30 June 2014 14:19, Stefan Fuhrmann <st...@wandisco.com> wrote:
>
> On Fri, Jun 27, 2014 at 12:55 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 26 June 2014 19:08, Stefan Fuhrmann <st...@wandisco.com>
>> wrote:
>> >
>> > Hi Ivan,
>> >
>> > I see three alternative ways to code that function
>> >
>> > 1. As hard coded string / byte sequence (current implementation).
>> > Cons:
>> > * Hard to write, hard to review by just looking at it (applies to time
>> > until initial release only).
>> > Pros:
>> > * Explicitly coded as constant, deterring people from changing it.
>> > * Independent of other code, i.e. unintended changes to the format /
>> > encoding generated by the normal code usually become apparent
>> > when running the test suite.
>> >
>> > 2. Use our txn code to write r0. This should be simple and might
>> > at most require some special ID handling.
>> > Cons:
>> > * Generating incompatible r0s is likely not caught by our test suite
>> > (assuming that reader and writer functions are in sync). Basically
>> > all the risk that comes with dynamically generating a "constant".
>> > * Test cases must make sure our backward compat repo creation
>> > options create repos that can actually be used by old releases.
>> > (we might want explicit test for that anyway, though)
>> > Pros:
>> > * No or very little special code for r0 (not sure, yet).
>> > * Format / encoding changes don't require new r0 templates.
>> >
>> > 3. Write code to "stitch" r0 together, e.g. string_add(md5("END\n")).
>> > Cons:
>> > * No more robust than 1. but requires much more code.
>> > * May _look_ easy to understand but an actual offline review is still
>> > hard.
>> > Pros:
>> > * Widely independent of other code, although not as much as 1.
>> >
>> > Do you generally agree with the range of options and their assessment?
>> Yes, I generally agree with range of options. The only other I have is
>> do not implement log addressing in first place.
>>
>> > Which one would you pick and why?
>> >
>> It's hard to pick option without looking to code, but I would start
>> with leaving string constant for revision body and then appending
>> indexes data using API. I.e. somewhat modified (2).
>
>
> r1606554 generates the index data dynamically now.
>
> It makes repo creation slightly more expensive but that
> does not seem to affect our test suite in any significant
> way. So, I think that is not an issue.
>
> Are you o.k. with the code as it stands now?
>
Now code is much better, but still not good as it could be. The
semantic and implementation of function
svn_fs_fs__open_pack_or_rev_file_writable() is really bad: it makes
file writable and then changes it back to read only on pool cleanup.
The code your committed works like this:
1. Open zero revision file for writing
2. Write revision content
3. Close revision file
4. Check that revision file doesn't have read only attribute
5. Make it writeable if needed and register pool cleanup handler
6. Open revision file for writing
7. Calculate required checksums
8. Append indexes to revision file
9. Close revision file.
10. Make revision file readonly
But this could be implemented much better:
1. Open zero revision file for writing
2. Write revision content
3. Calculate required data without closing revision file
4. Append indexes to revision file
5. Close revision file
6. Make revision file readonly
Proof of concept patch is attached.
---
Ivan Zhakov
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 30 June 2014 14:19, Stefan Fuhrmann <st...@wandisco.com> wrote:
>
> On Fri, Jun 27, 2014 at 12:55 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 26 June 2014 19:08, Stefan Fuhrmann <st...@wandisco.com>
>> wrote:
>> >
>> > Hi Ivan,
>> >
>> > I see three alternative ways to code that function
>> >
>> > 1. As hard coded string / byte sequence (current implementation).
>> > Cons:
>> > * Hard to write, hard to review by just looking at it (applies to time
>> > until initial release only).
>> > Pros:
>> > * Explicitly coded as constant, deterring people from changing it.
>> > * Independent of other code, i.e. unintended changes to the format /
>> > encoding generated by the normal code usually become apparent
>> > when running the test suite.
>> >
>> > 2. Use our txn code to write r0. This should be simple and might
>> > at most require some special ID handling.
>> > Cons:
>> > * Generating incompatible r0s is likely not caught by our test suite
>> > (assuming that reader and writer functions are in sync). Basically
>> > all the risk that comes with dynamically generating a "constant".
>> > * Test cases must make sure our backward compat repo creation
>> > options create repos that can actually be used by old releases.
>> > (we might want explicit test for that anyway, though)
>> > Pros:
>> > * No or very little special code for r0 (not sure, yet).
>> > * Format / encoding changes don't require new r0 templates.
>> >
>> > 3. Write code to "stitch" r0 together, e.g. string_add(md5("END\n")).
>> > Cons:
>> > * No more robust than 1. but requires much more code.
>> > * May _look_ easy to understand but an actual offline review is still
>> > hard.
>> > Pros:
>> > * Widely independent of other code, although not as much as 1.
>> >
>> > Do you generally agree with the range of options and their assessment?
>> Yes, I generally agree with range of options. The only other I have is
>> do not implement log addressing in first place.
>>
>> > Which one would you pick and why?
>> >
>> It's hard to pick option without looking to code, but I would start
>> with leaving string constant for revision body and then appending
>> indexes data using API. I.e. somewhat modified (2).
>
>
> r1606554 generates the index data dynamically now.
>
> It makes repo creation slightly more expensive but that
> does not seem to affect our test suite in any significant
> way. So, I think that is not an issue.
>
> Are you o.k. with the code as it stands now?
>
Now code is much better, but still not good as it could be. The
semantic and implementation of function
svn_fs_fs__open_pack_or_rev_file_writable() is really bad: it makes
file writable and then changes it back to read only on pool cleanup.
The code your committed works like this:
1. Open zero revision file for writing
2. Write revision content
3. Close revision file
4. Check that revision file doesn't have read only attribute
5. Make it writeable if needed and register pool cleanup handler
6. Open revision file for writing
7. Calculate required checksums
8. Append indexes to revision file
9. Close revision file.
10. Make revision file readonly
But this could be implemented much better:
1. Open zero revision file for writing
2. Write revision content
3. Calculate required data without closing revision file
4. Append indexes to revision file
5. Close revision file
6. Make revision file readonly
Proof of concept patch is attached.
---
Ivan Zhakov
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Jun 30, 2014 at 1:38 PM, Julian Foad <ju...@btopenworld.com>
wrote:
> Stefan Fuhrmann wrote:
> > r1606554 generates the index data dynamically now.
>
> That looks *much* better to my eyes. Now it only has a very few magic
> "offset" and "length" constants which are on a par with those we already
> had in the r0 header in the previous format. I don't much mind those,
> although I'd be even happier if they weren't there either.
>
> > + /* If the config file has not been initialized, yet, set some
> defaults
> > + here for r0. r0 is so small we could do with any non-zero
> values. */
> > + if (ffd->l2p_page_size == 0)
> > + ffd->l2p_page_size = 0x2000;
> > + if (ffd->p2l_page_size == 0)
> > + ffd->p2l_page_size = 0x10000;
>
> It's not clear why it's acceptable to initialize just those two parameters
> -- it looks like an implementation detail of the particular calls you make.
> It would be better if the caller set up the config before calling this.
> There is only one caller and it sets up the default config immediately
> after calling this. Why not swap the order of calls so that it sets the
> config first and then calls this write_revision_zero()? It would make sense
> from a logical point of view that the configuration is needed to tell the
> system how to write a revision, whereas r0 doesn't need to exist in order
> to create a default config.
>
r1606744 now delays the creation of r0 until after the config
structures have been fully initialized. The only downside is
that we now use slightly different values than in the original
template. But apart from r0 getting 1 or 2 bytes larger, that
has zero effect further down the path.
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/ subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Fuhrmann wrote:
> r1606554 generates the index data dynamically now.
That looks *much* better to my eyes. Now it only has a very few magic "offset" and "length" constants which are on a par with those we already had in the r0 header in the previous format. I don't much mind those, although I'd be even happier if they weren't there either.
> + /* If the config file has not been initialized, yet, set some defaults
> + here for r0. r0 is so small we could do with any non-zero values. */
> + if (ffd->l2p_page_size == 0)
> + ffd->l2p_page_size = 0x2000;
> + if (ffd->p2l_page_size == 0)
> + ffd->p2l_page_size = 0x10000;
It's not clear why it's acceptable to initialize just those two parameters -- it looks like an implementation detail of the particular calls you make. It would be better if the caller set up the config before calling this. There is only one caller and it sets up the default config immediately after calling this. Why not swap the order of calls so that it sets the config first and then calls this write_revision_zero()? It would make sense from a logical point of view that the configuration is needed to tell the system how to write a revision, whereas r0 doesn't need to exist in order to create a default config.
- Julian
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Jun 27, 2014 at 12:55 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 26 June 2014 19:08, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> >
> > Hi Ivan,
> >
> > I see three alternative ways to code that function
> >
> > 1. As hard coded string / byte sequence (current implementation).
> > Cons:
> > * Hard to write, hard to review by just looking at it (applies to time
> > until initial release only).
> > Pros:
> > * Explicitly coded as constant, deterring people from changing it.
> > * Independent of other code, i.e. unintended changes to the format /
> > encoding generated by the normal code usually become apparent
> > when running the test suite.
> >
> > 2. Use our txn code to write r0. This should be simple and might
> > at most require some special ID handling.
> > Cons:
> > * Generating incompatible r0s is likely not caught by our test suite
> > (assuming that reader and writer functions are in sync). Basically
> > all the risk that comes with dynamically generating a "constant".
> > * Test cases must make sure our backward compat repo creation
> > options create repos that can actually be used by old releases.
> > (we might want explicit test for that anyway, though)
> > Pros:
> > * No or very little special code for r0 (not sure, yet).
> > * Format / encoding changes don't require new r0 templates.
> >
> > 3. Write code to "stitch" r0 together, e.g. string_add(md5("END\n")).
> > Cons:
> > * No more robust than 1. but requires much more code.
> > * May _look_ easy to understand but an actual offline review is still
> hard.
> > Pros:
> > * Widely independent of other code, although not as much as 1.
> >
> > Do you generally agree with the range of options and their assessment?
> Yes, I generally agree with range of options. The only other I have is
> do not implement log addressing in first place.
>
> > Which one would you pick and why?
> >
> It's hard to pick option without looking to code, but I would start
> with leaving string constant for revision body and then appending
> indexes data using API. I.e. somewhat modified (2).
>
r1606554 generates the index data dynamically now.
It makes repo creation slightly more expensive but that
does not seem to affect our test suite in any significant
way. So, I think that is not an issue.
Are you o.k. with the code as it stands now?
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Jun 27, 2014 at 12:55 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 26 June 2014 19:08, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> >
> > Hi Ivan,
> >
> > I see three alternative ways to code that function
> >
> > 1. As hard coded string / byte sequence (current implementation).
> > Cons:
> > * Hard to write, hard to review by just looking at it (applies to time
> > until initial release only).
> > Pros:
> > * Explicitly coded as constant, deterring people from changing it.
> > * Independent of other code, i.e. unintended changes to the format /
> > encoding generated by the normal code usually become apparent
> > when running the test suite.
> >
> > 2. Use our txn code to write r0. This should be simple and might
> > at most require some special ID handling.
> > Cons:
> > * Generating incompatible r0s is likely not caught by our test suite
> > (assuming that reader and writer functions are in sync). Basically
> > all the risk that comes with dynamically generating a "constant".
> > * Test cases must make sure our backward compat repo creation
> > options create repos that can actually be used by old releases.
> > (we might want explicit test for that anyway, though)
> > Pros:
> > * No or very little special code for r0 (not sure, yet).
> > * Format / encoding changes don't require new r0 templates.
> >
> > 3. Write code to "stitch" r0 together, e.g. string_add(md5("END\n")).
> > Cons:
> > * No more robust than 1. but requires much more code.
> > * May _look_ easy to understand but an actual offline review is still
> hard.
> > Pros:
> > * Widely independent of other code, although not as much as 1.
> >
> > Do you generally agree with the range of options and their assessment?
> Yes, I generally agree with range of options. The only other I have is
> do not implement log addressing in first place.
>
> > Which one would you pick and why?
> >
> It's hard to pick option without looking to code, but I would start
> with leaving string constant for revision body and then appending
> indexes data using API. I.e. somewhat modified (2).
>
r1606554 generates the index data dynamically now.
It makes repo creation slightly more expensive but that
does not seem to affect our test suite in any significant
way. So, I think that is not an issue.
Are you o.k. with the code as it stands now?
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 26 June 2014 19:08, Stefan Fuhrmann <st...@wandisco.com> wrote:
>
>
> On Thu, Jun 26, 2014 at 11:52 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 25 June 2014 22:11, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>> > Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
>> >> On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com>
>> >> wrote:
>> >> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com>
>> >> > wrote:
>> >> >>
>> >> >> On 25 June 2014 19:24, Stefan Fuhrmann
>> >> >> <st...@wandisco.com>
>> >> >> wrote:
>> >> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
>> >> >> >> > Author: stefan2
>> >> >> >> > Date: Sat Jun 21 15:15:30 2014
>> >> >> >> > New Revision: 1604421
>> >> >> >> >
>> >> >> >> > URL: http://svn.apache.org/r1604421
>> >> >> >> > Log:
>> >> >> >> > Append index data to the rev data file instead of using
>> >> >> >> > separate
>> >> >> >> > files.
>
>
>>
>> >> >> >> > >
>> >> >> >> > > > >==============================================================================
>>
>> >> >> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >> >> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
>> >> >> >> > 15:15:30
>> >> >> >> > 2014
>> >> >> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >> >> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >> >> >> >+ "PLAIN\nEND\nENDREP\n"
>> >> >> >> >+ "id: 0.0.r0/2\n"
>> >> >> >> >+ "type: dir\n"
>> >> >> >> >+ "count: 0\n"
>> >> >> >> >+ "text: 0 3 4 4 "
>> >> >> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >> >> >> >+ "cpath: /\n"
>> >> >> >> >+ "\n\n"
>> >> >> >> >+
>> >> >> >> >+ /* L2P index */
>> >> >> >> >+ "\0\x80\x40" /* rev 0, 8k entries
>> >> >> >> > per page
>> >> >> >> > */
>> >> >> >> >+ "\1\1\1" /* 1 rev, 1 page, 1
>> >> >> >> > page in
>> >> >> >> > 1st
>> >> >> >> > rev */
>> >> >> >> >+ "\6\4" /* page size: bytes,
>> >> >> >> > count */
>> >> >> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >> >> >> >+
>> >> >> >> >+ /* P2L index */
>> >> >> >> >+ "\0\x6b" /* start rev, rev file
>> >> >> >> > size
>> >> >> >> > */
>> >> >> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page
>> >> >> >> > using 29
>> >> >> >> > bytes */
>> >> >> >> >+ "\0" /* offset entry 0 page
>> >> >> >> > 1 */
>> >> >> >> >+ /* len, item & type,
>> >> >> >> > rev,
>> >> >> >> > checksum */
>> >> >> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >> >> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >> >> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >> >> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up
>> >> >> >> > 64k
>> >> >> >> > page
>> >> >> >> > */
>> >> >> >> >+
>> >> >> >> >+ /* Footer */
>> >> >> >> >+ "107 121\7",
>> >> >> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> >> >> >> This constant makes code unreadable, unmaintainable and very
>> >> >> >> error
>> >> >> >> prone.
>> > ...
>> >> I saw it, but it doesn't make code easier to maintain.
>> >
>> > Ivan, that's the third time in as many emails that you say "the new code
>> > is hard to maintain". Could you please explain *how* you find it hard
>> > to maintain?
>> >
>> > Anyway, that's just me guessing. Could you please clarify what exactly
>> > makes the code unmaintainable in your opinion?
>> >
>> In my opinion 'code maintainability' is how easy or hard to make
>> change and review the code. This magic constant with a lot 7b encoded
>> numbers are very hard to review and modify if needed in future. Even
>> Stefan2 mentioned that he made slight mistakes when changing that
>> constant that was caught by test suite. But I don't assume that code
>> that passes all test suite doesn't have bugs, while Stefan2 seems to
>> assume. I bet that nobody reviewed "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> constant in the code above. Even I didn't, because I think it's a
>> waste of time and proper code should be committed or change reverted.
>
>
> Hi Ivan,
>
> I see three alternative ways to code that function
>
> 1. As hard coded string / byte sequence (current implementation).
> Cons:
> * Hard to write, hard to review by just looking at it (applies to time
> until initial release only).
> Pros:
> * Explicitly coded as constant, deterring people from changing it.
> * Independent of other code, i.e. unintended changes to the format /
> encoding generated by the normal code usually become apparent
> when running the test suite.
>
> 2. Use our txn code to write r0. This should be simple and might
> at most require some special ID handling.
> Cons:
> * Generating incompatible r0s is likely not caught by our test suite
> (assuming that reader and writer functions are in sync). Basically
> all the risk that comes with dynamically generating a "constant".
> * Test cases must make sure our backward compat repo creation
> options create repos that can actually be used by old releases.
> (we might want explicit test for that anyway, though)
> Pros:
> * No or very little special code for r0 (not sure, yet).
> * Format / encoding changes don't require new r0 templates.
>
> 3. Write code to "stitch" r0 together, e.g. string_add(md5("END\n")).
> Cons:
> * No more robust than 1. but requires much more code.
> * May _look_ easy to understand but an actual offline review is still hard.
> Pros:
> * Widely independent of other code, although not as much as 1.
>
> Do you generally agree with the range of options and their assessment?
Yes, I generally agree with range of options. The only other I have is
do not implement log addressing in first place.
> Which one would you pick and why?
>
It's hard to pick option without looking to code, but I would start
with leaving string constant for revision body and then appending
indexes data using API. I.e. somewhat modified (2).
--
Ivan Zhakov
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Branko Čibej <br...@wandisco.com>.
On 26.06.2014 17:45, Stefan Sperling wrote:
> On Thu, Jun 26, 2014 at 05:08:38PM +0200, Stefan Fuhrmann wrote:
>> I'd be fine with switching to option 2 as long as everyone understands
>> the implications.
> How about we write option 3 code to generate option 1 code, then hardcode
> the generated option 1 code but put the option 3 code in a comment near
> the option 1 code as a reference?
I don't see how requiring people to review the generator /and/ the
generated constant, independently of each other, and prove that the
former produces the latter, will make the code more maintainable and
understandable.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1604421 - in /subversion/trunk:
subversion/libsvn_fs_fs/ subversion/tests/cmdline/
subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jun 26, 2014 at 05:08:38PM +0200, Stefan Fuhrmann wrote:
> I see three alternative ways to code that function
>
> 1. As hard coded string / byte sequence (current implementation).
> Cons:
> * Hard to write, hard to review by just looking at it (applies to time
> until initial release only).
> Pros:
> * Explicitly coded as constant, deterring people from changing it.
> * Independent of other code, i.e. unintended changes to the format /
> encoding generated by the normal code usually become apparent
> when running the test suite.
>
> 2. Use our txn code to write r0. This should be simple and might
> at most require some special ID handling.
> Cons:
> * Generating incompatible r0s is likely not caught by our test suite
> (assuming that reader and writer functions are in sync). Basically
> all the risk that comes with dynamically generating a "constant".
> * Test cases must make sure our backward compat repo creation
> options create repos that can actually be used by old releases.
> (we might want explicit test for that anyway, though)
> Pros:
> * No or very little special code for r0 (not sure, yet).
> * Format / encoding changes don't require new r0 templates.
>
> 3. Write code to "stitch" r0 together, e.g. string_add(md5("END\n")).
> Cons:
> * No more robust than 1. but requires much more code.
> * May _look_ easy to understand but an actual offline review is still hard.
> Pros:
> * Widely independent of other code, although not as much as 1.
>
> Do you generally agree with the range of options and their assessment?
> Which one would you pick and why?
>
> I'd be fine with switching to option 2 as long as everyone understands
> the implications.
How about we write option 3 code to generate option 1 code, then hardcode
the generated option 1 code but put the option 3 code in a comment near
the option 1 code as a reference?
Re: svn commit: r1604421 - in /subversion/trunk:
subversion/libsvn_fs_fs/ subversion/tests/cmdline/
subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jun 26, 2014 at 05:08:38PM +0200, Stefan Fuhrmann wrote:
> I see three alternative ways to code that function
>
> 1. As hard coded string / byte sequence (current implementation).
> Cons:
> * Hard to write, hard to review by just looking at it (applies to time
> until initial release only).
> Pros:
> * Explicitly coded as constant, deterring people from changing it.
> * Independent of other code, i.e. unintended changes to the format /
> encoding generated by the normal code usually become apparent
> when running the test suite.
>
> 2. Use our txn code to write r0. This should be simple and might
> at most require some special ID handling.
> Cons:
> * Generating incompatible r0s is likely not caught by our test suite
> (assuming that reader and writer functions are in sync). Basically
> all the risk that comes with dynamically generating a "constant".
> * Test cases must make sure our backward compat repo creation
> options create repos that can actually be used by old releases.
> (we might want explicit test for that anyway, though)
> Pros:
> * No or very little special code for r0 (not sure, yet).
> * Format / encoding changes don't require new r0 templates.
>
> 3. Write code to "stitch" r0 together, e.g. string_add(md5("END\n")).
> Cons:
> * No more robust than 1. but requires much more code.
> * May _look_ easy to understand but an actual offline review is still hard.
> Pros:
> * Widely independent of other code, although not as much as 1.
>
> Do you generally agree with the range of options and their assessment?
> Which one would you pick and why?
>
> I'd be fine with switching to option 2 as long as everyone understands
> the implications.
How about we write option 3 code to generate option 1 code, then hardcode
the generated option 1 code but put the option 3 code in a comment near
the option 1 code as a reference?
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 26 June 2014 19:08, Stefan Fuhrmann <st...@wandisco.com> wrote:
>
>
> On Thu, Jun 26, 2014 at 11:52 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 25 June 2014 22:11, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>> > Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
>> >> On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com>
>> >> wrote:
>> >> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com>
>> >> > wrote:
>> >> >>
>> >> >> On 25 June 2014 19:24, Stefan Fuhrmann
>> >> >> <st...@wandisco.com>
>> >> >> wrote:
>> >> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
>> >> >> >> > Author: stefan2
>> >> >> >> > Date: Sat Jun 21 15:15:30 2014
>> >> >> >> > New Revision: 1604421
>> >> >> >> >
>> >> >> >> > URL: http://svn.apache.org/r1604421
>> >> >> >> > Log:
>> >> >> >> > Append index data to the rev data file instead of using
>> >> >> >> > separate
>> >> >> >> > files.
>
>
>>
>> >> >> >> > >
>> >> >> >> > > > >==============================================================================
>>
>> >> >> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >> >> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
>> >> >> >> > 15:15:30
>> >> >> >> > 2014
>> >> >> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >> >> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >> >> >> >+ "PLAIN\nEND\nENDREP\n"
>> >> >> >> >+ "id: 0.0.r0/2\n"
>> >> >> >> >+ "type: dir\n"
>> >> >> >> >+ "count: 0\n"
>> >> >> >> >+ "text: 0 3 4 4 "
>> >> >> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >> >> >> >+ "cpath: /\n"
>> >> >> >> >+ "\n\n"
>> >> >> >> >+
>> >> >> >> >+ /* L2P index */
>> >> >> >> >+ "\0\x80\x40" /* rev 0, 8k entries
>> >> >> >> > per page
>> >> >> >> > */
>> >> >> >> >+ "\1\1\1" /* 1 rev, 1 page, 1
>> >> >> >> > page in
>> >> >> >> > 1st
>> >> >> >> > rev */
>> >> >> >> >+ "\6\4" /* page size: bytes,
>> >> >> >> > count */
>> >> >> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >> >> >> >+
>> >> >> >> >+ /* P2L index */
>> >> >> >> >+ "\0\x6b" /* start rev, rev file
>> >> >> >> > size
>> >> >> >> > */
>> >> >> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page
>> >> >> >> > using 29
>> >> >> >> > bytes */
>> >> >> >> >+ "\0" /* offset entry 0 page
>> >> >> >> > 1 */
>> >> >> >> >+ /* len, item & type,
>> >> >> >> > rev,
>> >> >> >> > checksum */
>> >> >> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >> >> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >> >> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >> >> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up
>> >> >> >> > 64k
>> >> >> >> > page
>> >> >> >> > */
>> >> >> >> >+
>> >> >> >> >+ /* Footer */
>> >> >> >> >+ "107 121\7",
>> >> >> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> >> >> >> This constant makes code unreadable, unmaintainable and very
>> >> >> >> error
>> >> >> >> prone.
>> > ...
>> >> I saw it, but it doesn't make code easier to maintain.
>> >
>> > Ivan, that's the third time in as many emails that you say "the new code
>> > is hard to maintain". Could you please explain *how* you find it hard
>> > to maintain?
>> >
>> > Anyway, that's just me guessing. Could you please clarify what exactly
>> > makes the code unmaintainable in your opinion?
>> >
>> In my opinion 'code maintainability' is how easy or hard to make
>> change and review the code. This magic constant with a lot 7b encoded
>> numbers are very hard to review and modify if needed in future. Even
>> Stefan2 mentioned that he made slight mistakes when changing that
>> constant that was caught by test suite. But I don't assume that code
>> that passes all test suite doesn't have bugs, while Stefan2 seems to
>> assume. I bet that nobody reviewed "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> constant in the code above. Even I didn't, because I think it's a
>> waste of time and proper code should be committed or change reverted.
>
>
> Hi Ivan,
>
> I see three alternative ways to code that function
>
> 1. As hard coded string / byte sequence (current implementation).
> Cons:
> * Hard to write, hard to review by just looking at it (applies to time
> until initial release only).
> Pros:
> * Explicitly coded as constant, deterring people from changing it.
> * Independent of other code, i.e. unintended changes to the format /
> encoding generated by the normal code usually become apparent
> when running the test suite.
>
> 2. Use our txn code to write r0. This should be simple and might
> at most require some special ID handling.
> Cons:
> * Generating incompatible r0s is likely not caught by our test suite
> (assuming that reader and writer functions are in sync). Basically
> all the risk that comes with dynamically generating a "constant".
> * Test cases must make sure our backward compat repo creation
> options create repos that can actually be used by old releases.
> (we might want explicit test for that anyway, though)
> Pros:
> * No or very little special code for r0 (not sure, yet).
> * Format / encoding changes don't require new r0 templates.
>
> 3. Write code to "stitch" r0 together, e.g. string_add(md5("END\n")).
> Cons:
> * No more robust than 1. but requires much more code.
> * May _look_ easy to understand but an actual offline review is still hard.
> Pros:
> * Widely independent of other code, although not as much as 1.
>
> Do you generally agree with the range of options and their assessment?
Yes, I generally agree with range of options. The only other I have is
do not implement log addressing in first place.
> Which one would you pick and why?
>
It's hard to pick option without looking to code, but I would start
with leaving string constant for revision body and then appending
indexes data using API. I.e. somewhat modified (2).
--
Ivan Zhakov
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/ subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Fuhrmann wrote:
> I see three alternative ways to code that function
>
> 1. As hard coded string / byte sequence (current implementation).
>
> Cons:
> * Hard to write, hard to review by just looking at it (applies to time
> until initial release only).
>
> Pros:
> * Explicitly coded as constant, deterring people from changing it.
There's no need to obfuscate code in order to deter people from changing it. We have review, regression testing, social responsibility, and such like.
> * Independent of other code, i.e. unintended changes to the format /
> encoding generated by the normal code usually become apparent
> when running the test suite.
Ack.
> 2. Use our txn code to write r0. This should be simple and might
> at most require some special ID handling.
>
> Cons:
> * Generating incompatible r0s is likely not caught by our test suite
> (assuming that reader and writer functions are in sync). Basically
> all the risk that comes with dynamically generating a "constant".
>
> * Test cases must make sure our backward compat repo creation
> options create repos that can actually be used by old releases.
> (we might want explicit test for that anyway, though)
So it sounds like you're comfortable that explicit test cases would easily cover this.
I simply don't understand why you think writing r0 is special, compared with writing everything else in a repository. Everything that's written has to be backwards compatible with its claimed format, not just the r0 record.
Nor do I understand why you think it is especially "constant" -- why are we not allowed to change the order of a couple of records in it or introduce a leading zero or a space or some such change that would otherwise be backward-compatible but would break the idea that this file is strictly "constant"?
> Pros:
> * No or very little special code for r0 (not sure, yet).
>
> * Format / encoding changes don't require new r0 templates.
And:
* Software engineers can take the code apart and put it back together in new and interesting ways, such as to develop format 8, without unreasonable extra effort.
> 3. Write code to "stitch" r0 together, e.g. string_add(md5("END\n")).
>
> Cons:
> * No more robust than 1. but requires much more code.
>
> * May _look_ easy to understand but an actual offline review is still hard.
>
> Pros:
> * Widely independent of other code, although not as much as 1.
>
> Do you generally agree with the range of options and their assessment?
>
> Which one would you pick and why?
>
> I'd be fine with switching to option 2 as long as everyone understands
> the implications.
Sounds good to me.
- Julian
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Thu, Jun 26, 2014 at 11:52 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 25 June 2014 22:11, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
> >> On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> >> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com>
> wrote:
> >> >>
> >> >> On 25 June 2014 19:24, Stefan Fuhrmann <stefan.fuhrmann@wandisco.com
> >
> >> >> wrote:
> >> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com>
> wrote:
> >> >> >>
> >> >> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
> >> >> >> > Author: stefan2
> >> >> >> > Date: Sat Jun 21 15:15:30 2014
> >> >> >> > New Revision: 1604421
> >> >> >> >
> >> >> >> > URL: http://svn.apache.org/r1604421
> >> >> >> > Log:
> >> >> >> > Append index data to the rev data file instead of using separate
> >> >> >> > files.
>
> >> >> >> > >
> >==============================================================================
> >> >> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >> >> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
> >> >> >> > 15:15:30
> >> >> >> > 2014
> >> >> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >> >> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >> >> >> >+ "PLAIN\nEND\nENDREP\n"
> >> >> >> >+ "id: 0.0.r0/2\n"
> >> >> >> >+ "type: dir\n"
> >> >> >> >+ "count: 0\n"
> >> >> >> >+ "text: 0 3 4 4 "
> >> >> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >> >> >> >+ "cpath: /\n"
> >> >> >> >+ "\n\n"
> >> >> >> >+
> >> >> >> >+ /* L2P index */
> >> >> >> >+ "\0\x80\x40" /* rev 0, 8k entries
> per page
> >> >> >> > */
> >> >> >> >+ "\1\1\1" /* 1 rev, 1 page, 1
> page in
> >> >> >> > 1st
> >> >> >> > rev */
> >> >> >> >+ "\6\4" /* page size: bytes,
> count */
> >> >> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >> >> >> >+
> >> >> >> >+ /* P2L index */
> >> >> >> >+ "\0\x6b" /* start rev, rev file
> size
> >> >> >> > */
> >> >> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page
> using 29
> >> >> >> > bytes */
> >> >> >> >+ "\0" /* offset entry 0 page
> 1 */
> >> >> >> >+ /* len, item & type,
> rev,
> >> >> >> > checksum */
> >> >> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >> >> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >> >> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >> >> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up
> 64k
> >> >> >> > page
> >> >> >> > */
> >> >> >> >+
> >> >> >> >+ /* Footer */
> >> >> >> >+ "107 121\7",
> >> >> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
> >> >> >> This constant makes code unreadable, unmaintainable and very error
> >> >> >> prone.
> > ...
> >> I saw it, but it doesn't make code easier to maintain.
> >
> > Ivan, that's the third time in as many emails that you say "the new code
> > is hard to maintain". Could you please explain *how* you find it hard
> > to maintain?
> >
> > Anyway, that's just me guessing. Could you please clarify what exactly
> > makes the code unmaintainable in your opinion?
> >
> In my opinion 'code maintainability' is how easy or hard to make
> change and review the code. This magic constant with a lot 7b encoded
> numbers are very hard to review and modify if needed in future. Even
> Stefan2 mentioned that he made slight mistakes when changing that
> constant that was caught by test suite. But I don't assume that code
> that passes all test suite doesn't have bugs, while Stefan2 seems to
> assume. I bet that nobody reviewed "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> constant in the code above. Even I didn't, because I think it's a
> waste of time and proper code should be committed or change reverted.
>
Hi Ivan,
I see three alternative ways to code that function
1. As hard coded string / byte sequence (current implementation).
Cons:
* Hard to write, hard to review by just looking at it (applies to time
until initial release only).
Pros:
* Explicitly coded as constant, deterring people from changing it.
* Independent of other code, i.e. unintended changes to the format /
encoding generated by the normal code usually become apparent
when running the test suite.
2. Use our txn code to write r0. This should be simple and might
at most require some special ID handling.
Cons:
* Generating incompatible r0s is likely not caught by our test suite
(assuming that reader and writer functions are in sync). Basically
all the risk that comes with dynamically generating a "constant".
* Test cases must make sure our backward compat repo creation
options create repos that can actually be used by old releases.
(we might want explicit test for that anyway, though)
Pros:
* No or very little special code for r0 (not sure, yet).
* Format / encoding changes don't require new r0 templates.
3. Write code to "stitch" r0 together, e.g. string_add(md5("END\n")).
Cons:
* No more robust than 1. but requires much more code.
* May _look_ easy to understand but an actual offline review is still hard.
Pros:
* Widely independent of other code, although not as much as 1.
Do you generally agree with the range of options and their assessment?
Which one would you pick and why?
I'd be fine with switching to option 2 as long as everyone understands
the implications.
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Thu, Jun 26, 2014 at 11:52 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 25 June 2014 22:11, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
> >> On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> >> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com>
> wrote:
> >> >>
> >> >> On 25 June 2014 19:24, Stefan Fuhrmann <stefan.fuhrmann@wandisco.com
> >
> >> >> wrote:
> >> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com>
> wrote:
> >> >> >>
> >> >> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
> >> >> >> > Author: stefan2
> >> >> >> > Date: Sat Jun 21 15:15:30 2014
> >> >> >> > New Revision: 1604421
> >> >> >> >
> >> >> >> > URL: http://svn.apache.org/r1604421
> >> >> >> > Log:
> >> >> >> > Append index data to the rev data file instead of using separate
> >> >> >> > files.
>
> >> >> >> > >
> >==============================================================================
> >> >> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >> >> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
> >> >> >> > 15:15:30
> >> >> >> > 2014
> >> >> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >> >> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >> >> >> >+ "PLAIN\nEND\nENDREP\n"
> >> >> >> >+ "id: 0.0.r0/2\n"
> >> >> >> >+ "type: dir\n"
> >> >> >> >+ "count: 0\n"
> >> >> >> >+ "text: 0 3 4 4 "
> >> >> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >> >> >> >+ "cpath: /\n"
> >> >> >> >+ "\n\n"
> >> >> >> >+
> >> >> >> >+ /* L2P index */
> >> >> >> >+ "\0\x80\x40" /* rev 0, 8k entries
> per page
> >> >> >> > */
> >> >> >> >+ "\1\1\1" /* 1 rev, 1 page, 1
> page in
> >> >> >> > 1st
> >> >> >> > rev */
> >> >> >> >+ "\6\4" /* page size: bytes,
> count */
> >> >> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >> >> >> >+
> >> >> >> >+ /* P2L index */
> >> >> >> >+ "\0\x6b" /* start rev, rev file
> size
> >> >> >> > */
> >> >> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page
> using 29
> >> >> >> > bytes */
> >> >> >> >+ "\0" /* offset entry 0 page
> 1 */
> >> >> >> >+ /* len, item & type,
> rev,
> >> >> >> > checksum */
> >> >> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >> >> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >> >> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >> >> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up
> 64k
> >> >> >> > page
> >> >> >> > */
> >> >> >> >+
> >> >> >> >+ /* Footer */
> >> >> >> >+ "107 121\7",
> >> >> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
> >> >> >> This constant makes code unreadable, unmaintainable and very error
> >> >> >> prone.
> > ...
> >> I saw it, but it doesn't make code easier to maintain.
> >
> > Ivan, that's the third time in as many emails that you say "the new code
> > is hard to maintain". Could you please explain *how* you find it hard
> > to maintain?
> >
> > Anyway, that's just me guessing. Could you please clarify what exactly
> > makes the code unmaintainable in your opinion?
> >
> In my opinion 'code maintainability' is how easy or hard to make
> change and review the code. This magic constant with a lot 7b encoded
> numbers are very hard to review and modify if needed in future. Even
> Stefan2 mentioned that he made slight mistakes when changing that
> constant that was caught by test suite. But I don't assume that code
> that passes all test suite doesn't have bugs, while Stefan2 seems to
> assume. I bet that nobody reviewed "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> constant in the code above. Even I didn't, because I think it's a
> waste of time and proper code should be committed or change reverted.
>
Hi Ivan,
I see three alternative ways to code that function
1. As hard coded string / byte sequence (current implementation).
Cons:
* Hard to write, hard to review by just looking at it (applies to time
until initial release only).
Pros:
* Explicitly coded as constant, deterring people from changing it.
* Independent of other code, i.e. unintended changes to the format /
encoding generated by the normal code usually become apparent
when running the test suite.
2. Use our txn code to write r0. This should be simple and might
at most require some special ID handling.
Cons:
* Generating incompatible r0s is likely not caught by our test suite
(assuming that reader and writer functions are in sync). Basically
all the risk that comes with dynamically generating a "constant".
* Test cases must make sure our backward compat repo creation
options create repos that can actually be used by old releases.
(we might want explicit test for that anyway, though)
Pros:
* No or very little special code for r0 (not sure, yet).
* Format / encoding changes don't require new r0 templates.
3. Write code to "stitch" r0 together, e.g. string_add(md5("END\n")).
Cons:
* No more robust than 1. but requires much more code.
* May _look_ easy to understand but an actual offline review is still hard.
Pros:
* Widely independent of other code, although not as much as 1.
Do you generally agree with the range of options and their assessment?
Which one would you pick and why?
I'd be fine with switching to option 2 as long as everyone understands
the implications.
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 25 June 2014 22:11, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
>> On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com> wrote:
>> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >>
>> >> On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com>
>> >> wrote:
>> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >> >>
>> >> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
>> >> >> > Author: stefan2
>> >> >> > Date: Sat Jun 21 15:15:30 2014
>> >> >> > New Revision: 1604421
>> >> >> >
>> >> >> > URL: http://svn.apache.org/r1604421
>> >> >> > Log:
>> >> >> > Append index data to the rev data file instead of using separate
>> >> >> > files.
>> >> >> >
>> >> >> > This gives unpacked FSFS format 7 similar I/O characteristics and
>> >> >> > disk
>> >> >> > space usage as earlier formats. It also eliminates the need for
>> >> >> > retries
>> >> >> > after a potential background pack run because each file is now always
>> >> >> > consistent with itself (earlier, data or index files might already
>> >> >> > have
>> >> >> > been deleted while the respective other was still valid). Overall,
>> >> >> > most of this patch is removing code necessary to handle separate
>> >> >> > files.
>> >> >> >
>> >> >> > The new file format is simple:
>> >> >> >
>> >> >> > <rep data><l2p index><p2l index><footer><footer length>
>> >> >> >
>> >> >> > with the first three being identical what we had before. <footer
>> >> >> > length>
>> >> >> > is a single byte giving the length of the preceding footer, so it's
>> >> >> > easier to extract than the pre-f7 rev trailer and there is only one
>> >> >> > per pack / rev file.
>> >> >> >
>> >> >> [..]
>> >> >>
>> >> >>
>> >> >> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>> >> >> >URL:
>> >> >> >
>> >> >> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>> >> >>
>> >> >> >
>> >> >> > > >==============================================================================
>> >> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
>> >> >> > 15:15:30
>> >> >> > 2014
>> >> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >> >> >+ "PLAIN\nEND\nENDREP\n"
>> >> >> >+ "id: 0.0.r0/2\n"
>> >> >> >+ "type: dir\n"
>> >> >> >+ "count: 0\n"
>> >> >> >+ "text: 0 3 4 4 "
>> >> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >> >> >+ "cpath: /\n"
>> >> >> >+ "\n\n"
>> >> >> >+
>> >> >> >+ /* L2P index */
>> >> >> >+ "\0\x80\x40" /* rev 0, 8k entries per page
>> >> >> > */
>> >> >> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in
>> >> >> > 1st
>> >> >> > rev */
>> >> >> >+ "\6\4" /* page size: bytes, count */
>> >> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >> >> >+
>> >> >> >+ /* P2L index */
>> >> >> >+ "\0\x6b" /* start rev, rev file size
>> >> >> > */
>> >> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
>> >> >> > bytes */
>> >> >> >+ "\0" /* offset entry 0 page 1 */
>> >> >> >+ /* len, item & type, rev,
>> >> >> > checksum */
>> >> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
>> >> >> > page
>> >> >> > */
>> >> >> >+
>> >> >> >+ /* Footer */
>> >> >> >+ "107 121\7",
>> >> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> >> >> This constant makes code unreadable, unmaintainable and very error
>> >> >> prone.
> ...
>> I saw it, but it doesn't make code easier to maintain.
>
> Ivan, that's the third time in as many emails that you say "the new code
> is hard to maintain". Could you please explain *how* you find it hard
> to maintain?
>
> Anyway, that's just me guessing. Could you please clarify what exactly
> makes the code unmaintainable in your opinion?
>
In my opinion 'code maintainability' is how easy or hard to make
change and review the code. This magic constant with a lot 7b encoded
numbers are very hard to review and modify if needed in future. Even
Stefan2 mentioned that he made slight mistakes when changing that
constant that was caught by test suite. But I don't assume that code
that passes all test suite doesn't have bugs, while Stefan2 seems to
assume. I bet that nobody reviewed "\x11\x34\0\xf5\xd6\x8c\x81\x06"
constant in the code above. Even I didn't, because I think it's a
waste of time and proper code should be committed or change reverted.
>
> I assume you dislike the magic numbers and would prefer to see sizeof()
> uesd instead, e.g.,
>
>
> #define REV0_FILE_PART_1 "foo"
> #define REV0_FILE_PART_2 "bar"
>
> svn_file_create_binary(REV0_FILE_PART_1 REV0_FILE_PART_2,
> sizeof(REV0_FILE_PART_1)-1
> + sizeof(REV0_FILE_PART_2)-1);
>
This will make code a little bit better, but not so much. Because the
problem in magic hex numbers above.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 25 June 2014 22:11, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
>> On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com> wrote:
>> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >>
>> >> On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com>
>> >> wrote:
>> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >> >>
>> >> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
>> >> >> > Author: stefan2
>> >> >> > Date: Sat Jun 21 15:15:30 2014
>> >> >> > New Revision: 1604421
>> >> >> >
>> >> >> > URL: http://svn.apache.org/r1604421
>> >> >> > Log:
>> >> >> > Append index data to the rev data file instead of using separate
>> >> >> > files.
>> >> >> >
>> >> >> > This gives unpacked FSFS format 7 similar I/O characteristics and
>> >> >> > disk
>> >> >> > space usage as earlier formats. It also eliminates the need for
>> >> >> > retries
>> >> >> > after a potential background pack run because each file is now always
>> >> >> > consistent with itself (earlier, data or index files might already
>> >> >> > have
>> >> >> > been deleted while the respective other was still valid). Overall,
>> >> >> > most of this patch is removing code necessary to handle separate
>> >> >> > files.
>> >> >> >
>> >> >> > The new file format is simple:
>> >> >> >
>> >> >> > <rep data><l2p index><p2l index><footer><footer length>
>> >> >> >
>> >> >> > with the first three being identical what we had before. <footer
>> >> >> > length>
>> >> >> > is a single byte giving the length of the preceding footer, so it's
>> >> >> > easier to extract than the pre-f7 rev trailer and there is only one
>> >> >> > per pack / rev file.
>> >> >> >
>> >> >> [..]
>> >> >>
>> >> >>
>> >> >> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>> >> >> >URL:
>> >> >> >
>> >> >> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>> >> >>
>> >> >> >
>> >> >> > > >==============================================================================
>> >> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
>> >> >> > 15:15:30
>> >> >> > 2014
>> >> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >> >> >+ "PLAIN\nEND\nENDREP\n"
>> >> >> >+ "id: 0.0.r0/2\n"
>> >> >> >+ "type: dir\n"
>> >> >> >+ "count: 0\n"
>> >> >> >+ "text: 0 3 4 4 "
>> >> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >> >> >+ "cpath: /\n"
>> >> >> >+ "\n\n"
>> >> >> >+
>> >> >> >+ /* L2P index */
>> >> >> >+ "\0\x80\x40" /* rev 0, 8k entries per page
>> >> >> > */
>> >> >> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in
>> >> >> > 1st
>> >> >> > rev */
>> >> >> >+ "\6\4" /* page size: bytes, count */
>> >> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >> >> >+
>> >> >> >+ /* P2L index */
>> >> >> >+ "\0\x6b" /* start rev, rev file size
>> >> >> > */
>> >> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
>> >> >> > bytes */
>> >> >> >+ "\0" /* offset entry 0 page 1 */
>> >> >> >+ /* len, item & type, rev,
>> >> >> > checksum */
>> >> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
>> >> >> > page
>> >> >> > */
>> >> >> >+
>> >> >> >+ /* Footer */
>> >> >> >+ "107 121\7",
>> >> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> >> >> This constant makes code unreadable, unmaintainable and very error
>> >> >> prone.
> ...
>> I saw it, but it doesn't make code easier to maintain.
>
> Ivan, that's the third time in as many emails that you say "the new code
> is hard to maintain". Could you please explain *how* you find it hard
> to maintain?
>
> Anyway, that's just me guessing. Could you please clarify what exactly
> makes the code unmaintainable in your opinion?
>
In my opinion 'code maintainability' is how easy or hard to make
change and review the code. This magic constant with a lot 7b encoded
numbers are very hard to review and modify if needed in future. Even
Stefan2 mentioned that he made slight mistakes when changing that
constant that was caught by test suite. But I don't assume that code
that passes all test suite doesn't have bugs, while Stefan2 seems to
assume. I bet that nobody reviewed "\x11\x34\0\xf5\xd6\x8c\x81\x06"
constant in the code above. Even I didn't, because I think it's a
waste of time and proper code should be committed or change reverted.
>
> I assume you dislike the magic numbers and would prefer to see sizeof()
> uesd instead, e.g.,
>
>
> #define REV0_FILE_PART_1 "foo"
> #define REV0_FILE_PART_2 "bar"
>
> svn_file_create_binary(REV0_FILE_PART_1 REV0_FILE_PART_2,
> sizeof(REV0_FILE_PART_1)-1
> + sizeof(REV0_FILE_PART_2)-1);
>
This will make code a little bit better, but not so much. Because the
problem in magic hex numbers above.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1604421 - in /subversion/trunk:
subversion/libsvn_fs_fs/ subversion/tests/cmdline/
subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
> On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com> wrote:
> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>
> >> On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com>
> >> wrote:
> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >> >>
> >> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
> >> >> > Author: stefan2
> >> >> > Date: Sat Jun 21 15:15:30 2014
> >> >> > New Revision: 1604421
> >> >> >
> >> >> > URL: http://svn.apache.org/r1604421
> >> >> > Log:
> >> >> > Append index data to the rev data file instead of using separate
> >> >> > files.
> >> >> >
> >> >> > This gives unpacked FSFS format 7 similar I/O characteristics and
> >> >> > disk
> >> >> > space usage as earlier formats. It also eliminates the need for
> >> >> > retries
> >> >> > after a potential background pack run because each file is now always
> >> >> > consistent with itself (earlier, data or index files might already
> >> >> > have
> >> >> > been deleted while the respective other was still valid). Overall,
> >> >> > most of this patch is removing code necessary to handle separate
> >> >> > files.
> >> >> >
> >> >> > The new file format is simple:
> >> >> >
> >> >> > <rep data><l2p index><p2l index><footer><footer length>
> >> >> >
> >> >> > with the first three being identical what we had before. <footer
> >> >> > length>
> >> >> > is a single byte giving the length of the preceding footer, so it's
> >> >> > easier to extract than the pre-f7 rev trailer and there is only one
> >> >> > per pack / rev file.
> >> >> >
> >> >> [..]
> >> >>
> >> >>
> >> >> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >> >> >URL:
> >> >> >
> >> >> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> >> >>
> >> >> >
> >> >> > > >==============================================================================
> >> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
> >> >> > 15:15:30
> >> >> > 2014
> >> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >> >> >+ "PLAIN\nEND\nENDREP\n"
> >> >> >+ "id: 0.0.r0/2\n"
> >> >> >+ "type: dir\n"
> >> >> >+ "count: 0\n"
> >> >> >+ "text: 0 3 4 4 "
> >> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >> >> >+ "cpath: /\n"
> >> >> >+ "\n\n"
> >> >> >+
> >> >> >+ /* L2P index */
> >> >> >+ "\0\x80\x40" /* rev 0, 8k entries per page
> >> >> > */
> >> >> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in
> >> >> > 1st
> >> >> > rev */
> >> >> >+ "\6\4" /* page size: bytes, count */
> >> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >> >> >+
> >> >> >+ /* P2L index */
> >> >> >+ "\0\x6b" /* start rev, rev file size
> >> >> > */
> >> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
> >> >> > bytes */
> >> >> >+ "\0" /* offset entry 0 page 1 */
> >> >> >+ /* len, item & type, rev,
> >> >> > checksum */
> >> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
> >> >> > page
> >> >> > */
> >> >> >+
> >> >> >+ /* Footer */
> >> >> >+ "107 121\7",
> >> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
> >> >> This constant makes code unreadable, unmaintainable and very error
> >> >> prone.
...
> I saw it, but it doesn't make code easier to maintain.
Ivan, that's the third time in as many emails that you say "the new code
is hard to maintain". Could you please explain *how* you find it hard
to maintain?
I assume you dislike the magic numbers and would prefer to see sizeof()
uesd instead, e.g.,
#define REV0_FILE_PART_1 "foo"
#define REV0_FILE_PART_2 "bar"
svn_file_create_binary(REV0_FILE_PART_1 REV0_FILE_PART_2,
sizeof(REV0_FILE_PART_1)-1
+ sizeof(REV0_FILE_PART_2)-1);
Anyway, that's just me guessing. Could you please clarify what exactly
makes the code unmaintainable in your opinion?
Thanks.
Re: svn commit: r1604421 - in /subversion/trunk:
subversion/libsvn_fs_fs/ subversion/tests/cmdline/
subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
> On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com> wrote:
> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>
> >> On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com>
> >> wrote:
> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >> >>
> >> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
> >> >> > Author: stefan2
> >> >> > Date: Sat Jun 21 15:15:30 2014
> >> >> > New Revision: 1604421
> >> >> >
> >> >> > URL: http://svn.apache.org/r1604421
> >> >> > Log:
> >> >> > Append index data to the rev data file instead of using separate
> >> >> > files.
> >> >> >
> >> >> > This gives unpacked FSFS format 7 similar I/O characteristics and
> >> >> > disk
> >> >> > space usage as earlier formats. It also eliminates the need for
> >> >> > retries
> >> >> > after a potential background pack run because each file is now always
> >> >> > consistent with itself (earlier, data or index files might already
> >> >> > have
> >> >> > been deleted while the respective other was still valid). Overall,
> >> >> > most of this patch is removing code necessary to handle separate
> >> >> > files.
> >> >> >
> >> >> > The new file format is simple:
> >> >> >
> >> >> > <rep data><l2p index><p2l index><footer><footer length>
> >> >> >
> >> >> > with the first three being identical what we had before. <footer
> >> >> > length>
> >> >> > is a single byte giving the length of the preceding footer, so it's
> >> >> > easier to extract than the pre-f7 rev trailer and there is only one
> >> >> > per pack / rev file.
> >> >> >
> >> >> [..]
> >> >>
> >> >>
> >> >> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >> >> >URL:
> >> >> >
> >> >> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> >> >>
> >> >> >
> >> >> > > >==============================================================================
> >> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
> >> >> > 15:15:30
> >> >> > 2014
> >> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >> >> >+ "PLAIN\nEND\nENDREP\n"
> >> >> >+ "id: 0.0.r0/2\n"
> >> >> >+ "type: dir\n"
> >> >> >+ "count: 0\n"
> >> >> >+ "text: 0 3 4 4 "
> >> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >> >> >+ "cpath: /\n"
> >> >> >+ "\n\n"
> >> >> >+
> >> >> >+ /* L2P index */
> >> >> >+ "\0\x80\x40" /* rev 0, 8k entries per page
> >> >> > */
> >> >> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in
> >> >> > 1st
> >> >> > rev */
> >> >> >+ "\6\4" /* page size: bytes, count */
> >> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >> >> >+
> >> >> >+ /* P2L index */
> >> >> >+ "\0\x6b" /* start rev, rev file size
> >> >> > */
> >> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
> >> >> > bytes */
> >> >> >+ "\0" /* offset entry 0 page 1 */
> >> >> >+ /* len, item & type, rev,
> >> >> > checksum */
> >> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
> >> >> > page
> >> >> > */
> >> >> >+
> >> >> >+ /* Footer */
> >> >> >+ "107 121\7",
> >> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
> >> >> This constant makes code unreadable, unmaintainable and very error
> >> >> prone.
...
> I saw it, but it doesn't make code easier to maintain.
Ivan, that's the third time in as many emails that you say "the new code
is hard to maintain". Could you please explain *how* you find it hard
to maintain?
I assume you dislike the magic numbers and would prefer to see sizeof()
uesd instead, e.g.,
#define REV0_FILE_PART_1 "foo"
#define REV0_FILE_PART_2 "bar"
svn_file_create_binary(REV0_FILE_PART_1 REV0_FILE_PART_2,
sizeof(REV0_FILE_PART_1)-1
+ sizeof(REV0_FILE_PART_2)-1);
Anyway, that's just me guessing. Could you please clarify what exactly
makes the code unmaintainable in your opinion?
Thanks.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com> wrote:
> On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com>
>> wrote:
>> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >>
>> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
>> >> > Author: stefan2
>> >> > Date: Sat Jun 21 15:15:30 2014
>> >> > New Revision: 1604421
>> >> >
>> >> > URL: http://svn.apache.org/r1604421
>> >> > Log:
>> >> > Append index data to the rev data file instead of using separate
>> >> > files.
>> >> >
>> >> > This gives unpacked FSFS format 7 similar I/O characteristics and
>> >> > disk
>> >> > space usage as earlier formats. It also eliminates the need for
>> >> > retries
>> >> > after a potential background pack run because each file is now always
>> >> > consistent with itself (earlier, data or index files might already
>> >> > have
>> >> > been deleted while the respective other was still valid). Overall,
>> >> > most of this patch is removing code necessary to handle separate
>> >> > files.
>> >> >
>> >> > The new file format is simple:
>> >> >
>> >> > <rep data><l2p index><p2l index><footer><footer length>
>> >> >
>> >> > with the first three being identical what we had before. <footer
>> >> > length>
>> >> > is a single byte giving the length of the preceding footer, so it's
>> >> > easier to extract than the pre-f7 rev trailer and there is only one
>> >> > per pack / rev file.
>> >> >
>> >> [..]
>> >>
>> >>
>> >> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>> >> >URL:
>> >> >
>> >> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>> >>
>> >> >
>> >> > > >==============================================================================
>> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
>> >> > 15:15:30
>> >> > 2014
>> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >> >+ "PLAIN\nEND\nENDREP\n"
>> >> >+ "id: 0.0.r0/2\n"
>> >> >+ "type: dir\n"
>> >> >+ "count: 0\n"
>> >> >+ "text: 0 3 4 4 "
>> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >> >+ "cpath: /\n"
>> >> >+ "\n\n"
>> >> >+
>> >> >+ /* L2P index */
>> >> >+ "\0\x80\x40" /* rev 0, 8k entries per page
>> >> > */
>> >> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in
>> >> > 1st
>> >> > rev */
>> >> >+ "\6\4" /* page size: bytes, count */
>> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >> >+
>> >> >+ /* P2L index */
>> >> >+ "\0\x6b" /* start rev, rev file size
>> >> > */
>> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
>> >> > bytes */
>> >> >+ "\0" /* offset entry 0 page 1 */
>> >> >+ /* len, item & type, rev,
>> >> > checksum */
>> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
>> >> > page
>> >> > */
>> >> >+
>> >> >+ /* Footer */
>> >> >+ "107 121\7",
>> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> >> This constant makes code unreadable, unmaintainable and very error
>> >> prone.
>> >
>> >
>> > How?
>> >
>> > To make it more obvious that I intend to follow
>> > the svn_io_file_create_binary interface, I added
>> > some commentary explaining where the numbers
>> > come from. But even just placing the sum (without
>> > its elements) in there would be fine already.
>> >
>> > Changing the rev 0 template has never been a fun
>> > activity and wont become one in the foreseeable
>> > future.
>> >
>> I believe that the committer should be responsible forcommitting
>> readable and easy to maintain code, not the reviewer. So please fix or
>> revert.
>
>
> Sorry, I should have referenced r1605444 in my post.
>
I saw it, but it doesn't make code easier to maintain.
--
Ivan Zhakov
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com> wrote:
> On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com>
>> wrote:
>> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >>
>> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
>> >> > Author: stefan2
>> >> > Date: Sat Jun 21 15:15:30 2014
>> >> > New Revision: 1604421
>> >> >
>> >> > URL: http://svn.apache.org/r1604421
>> >> > Log:
>> >> > Append index data to the rev data file instead of using separate
>> >> > files.
>> >> >
>> >> > This gives unpacked FSFS format 7 similar I/O characteristics and
>> >> > disk
>> >> > space usage as earlier formats. It also eliminates the need for
>> >> > retries
>> >> > after a potential background pack run because each file is now always
>> >> > consistent with itself (earlier, data or index files might already
>> >> > have
>> >> > been deleted while the respective other was still valid). Overall,
>> >> > most of this patch is removing code necessary to handle separate
>> >> > files.
>> >> >
>> >> > The new file format is simple:
>> >> >
>> >> > <rep data><l2p index><p2l index><footer><footer length>
>> >> >
>> >> > with the first three being identical what we had before. <footer
>> >> > length>
>> >> > is a single byte giving the length of the preceding footer, so it's
>> >> > easier to extract than the pre-f7 rev trailer and there is only one
>> >> > per pack / rev file.
>> >> >
>> >> [..]
>> >>
>> >>
>> >> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>> >> >URL:
>> >> >
>> >> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>> >>
>> >> >
>> >> > > >==============================================================================
>> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
>> >> > 15:15:30
>> >> > 2014
>> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >> >+ "PLAIN\nEND\nENDREP\n"
>> >> >+ "id: 0.0.r0/2\n"
>> >> >+ "type: dir\n"
>> >> >+ "count: 0\n"
>> >> >+ "text: 0 3 4 4 "
>> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >> >+ "cpath: /\n"
>> >> >+ "\n\n"
>> >> >+
>> >> >+ /* L2P index */
>> >> >+ "\0\x80\x40" /* rev 0, 8k entries per page
>> >> > */
>> >> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in
>> >> > 1st
>> >> > rev */
>> >> >+ "\6\4" /* page size: bytes, count */
>> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >> >+
>> >> >+ /* P2L index */
>> >> >+ "\0\x6b" /* start rev, rev file size
>> >> > */
>> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
>> >> > bytes */
>> >> >+ "\0" /* offset entry 0 page 1 */
>> >> >+ /* len, item & type, rev,
>> >> > checksum */
>> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
>> >> > page
>> >> > */
>> >> >+
>> >> >+ /* Footer */
>> >> >+ "107 121\7",
>> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> >> This constant makes code unreadable, unmaintainable and very error
>> >> prone.
>> >
>> >
>> > How?
>> >
>> > To make it more obvious that I intend to follow
>> > the svn_io_file_create_binary interface, I added
>> > some commentary explaining where the numbers
>> > come from. But even just placing the sum (without
>> > its elements) in there would be fine already.
>> >
>> > Changing the rev 0 template has never been a fun
>> > activity and wont become one in the foreseeable
>> > future.
>> >
>> I believe that the committer should be responsible forcommitting
>> readable and easy to maintain code, not the reviewer. So please fix or
>> revert.
>
>
> Sorry, I should have referenced r1605444 in my post.
>
I saw it, but it doesn't make code easier to maintain.
--
Ivan Zhakov
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>
> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
> >> > Author: stefan2
> >> > Date: Sat Jun 21 15:15:30 2014
> >> > New Revision: 1604421
> >> >
> >> > URL: http://svn.apache.org/r1604421
> >> > Log:
> >> > Append index data to the rev data file instead of using separate
> files.
> >> >
> >> > This gives unpacked FSFS format 7 similar I/O characteristics and disk
> >> > space usage as earlier formats. It also eliminates the need for
> retries
> >> > after a potential background pack run because each file is now always
> >> > consistent with itself (earlier, data or index files might already
> have
> >> > been deleted while the respective other was still valid). Overall,
> >> > most of this patch is removing code necessary to handle separate
> files.
> >> >
> >> > The new file format is simple:
> >> >
> >> > <rep data><l2p index><p2l index><footer><footer length>
> >> >
> >> > with the first three being identical what we had before. <footer
> length>
> >> > is a single byte giving the length of the preceding footer, so it's
> >> > easier to extract than the pre-f7 rev trailer and there is only one
> >> > per pack / rev file.
> >> >
> >> [..]
> >>
> >>
> >> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >> >URL:
> >> >
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> >>
> >> >
> >==============================================================================
> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
> 15:15:30
> >> > 2014
> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >> >+ "PLAIN\nEND\nENDREP\n"
> >> >+ "id: 0.0.r0/2\n"
> >> >+ "type: dir\n"
> >> >+ "count: 0\n"
> >> >+ "text: 0 3 4 4 "
> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >> >+ "cpath: /\n"
> >> >+ "\n\n"
> >> >+
> >> >+ /* L2P index */
> >> >+ "\0\x80\x40" /* rev 0, 8k entries per page
> */
> >> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in
> 1st
> >> > rev */
> >> >+ "\6\4" /* page size: bytes, count */
> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >> >+
> >> >+ /* P2L index */
> >> >+ "\0\x6b" /* start rev, rev file size */
> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
> >> > bytes */
> >> >+ "\0" /* offset entry 0 page 1 */
> >> >+ /* len, item & type, rev,
> >> > checksum */
> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
> page
> >> > */
> >> >+
> >> >+ /* Footer */
> >> >+ "107 121\7",
> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
> >> This constant makes code unreadable, unmaintainable and very error
> prone.
> >
> >
> > How?
> >
> > To make it more obvious that I intend to follow
> > the svn_io_file_create_binary interface, I added
> > some commentary explaining where the numbers
> > come from. But even just placing the sum (without
> > its elements) in there would be fine already.
> >
> > Changing the rev 0 template has never been a fun
> > activity and wont become one in the foreseeable
> > future.
> >
> I believe that the committer should be responsible forcommitting
> readable and easy to maintain code, not the reviewer. So please fix or
> revert.
>
Sorry, I should have referenced r1605444 in my post.
> >> > Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
> >> > URL:
> >> >
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> >> >
> >> >
> ==============================================================================
> >> > --- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
> >> > +++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21
> >> > 15:15:30 2014
> >> > @@ -23,6 +23,7 @@
> >> > #include "rev_file.h"
> >> > #include "fs_fs.h"
> >> > #include "index.h"
> >> > +#include "low_level.h"
> >> > #include "util.h"
> >> >
> >> > #include "../libsvn_fs/fs-loader.h"
> >> > @@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
> >> > file->stream = NULL;
> >> > file->p2l_stream = NULL;
> >> > file->l2p_stream = NULL;
> >> > + file->block_size = ffd->block_size;
> >> > + file->l2p_offset = -1;
> >> > + file->p2l_offset = -1;
> >> > + file->footer_offset = -1;
> >> > file->pool = pool;
> >> > }
> >> >
> >> > @@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
> >> > }
> >> >
> >> > svn_error_t *
> >> > -svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
> >> > - svn_fs_t *fs,
> >> > - svn_revnum_t rev)
> >> > +svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
> >> > {
> >> > - if (file->file)
> >> > - svn_fs_fs__close_revision_file(file);
> >> > + if (file->l2p_offset == -1)
> >> > + {
> >> > + apr_off_t filesize = 0;
> >> > + unsigned char footer_length;
> >> > + svn_stringbuf_t *footer;
> >> > +
> >> > + /* Determine file size. */
> >> > + SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize,
> >> > file->pool));
> >> > +
> >> > + /* Read last byte (containing the length of the footer). */
> >> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
> >> > NULL,
> >> > + filesize - 1, file->pool));
> >> > + SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
> >> > + sizeof(footer_length), NULL,
> NULL,
> >> > + file->pool));
> >> > +
> >> > + /* Read footer. */
> >> > + footer = svn_stringbuf_create_ensure(footer_length,
> file->pool);
> >> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
> >> > NULL,
> >> > + filesize - 1 - footer_length,
> >> > + file->pool));
> >> > + SVN_ERR(svn_io_file_read_full2(file->file, footer->data,
> >> > footer_length,
> >> > + &footer->len, NULL,
> file->pool));
> >> You're passing pointer to possible 32-bit value to function accepting
> >> pointer to 64-bit value. This will lead unpredictable results.
> >
> >
> > I assume you are referring to the &footer->len?
> > In that case, I think I'm passing an apr_size_t*
> > to an interface expecting an apr_size_t*.
> > What am I missing?
> >
> You're right. I confused svn_io_file_read_full2() with
> svn_io_file_seek(). The code above doesn't have problem I mentioned.
>
ack.
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>
> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
> >> > Author: stefan2
> >> > Date: Sat Jun 21 15:15:30 2014
> >> > New Revision: 1604421
> >> >
> >> > URL: http://svn.apache.org/r1604421
> >> > Log:
> >> > Append index data to the rev data file instead of using separate
> files.
> >> >
> >> > This gives unpacked FSFS format 7 similar I/O characteristics and disk
> >> > space usage as earlier formats. It also eliminates the need for
> retries
> >> > after a potential background pack run because each file is now always
> >> > consistent with itself (earlier, data or index files might already
> have
> >> > been deleted while the respective other was still valid). Overall,
> >> > most of this patch is removing code necessary to handle separate
> files.
> >> >
> >> > The new file format is simple:
> >> >
> >> > <rep data><l2p index><p2l index><footer><footer length>
> >> >
> >> > with the first three being identical what we had before. <footer
> length>
> >> > is a single byte giving the length of the preceding footer, so it's
> >> > easier to extract than the pre-f7 rev trailer and there is only one
> >> > per pack / rev file.
> >> >
> >> [..]
> >>
> >>
> >> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >> >URL:
> >> >
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> >>
> >> >
> >==============================================================================
> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
> 15:15:30
> >> > 2014
> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >> >+ "PLAIN\nEND\nENDREP\n"
> >> >+ "id: 0.0.r0/2\n"
> >> >+ "type: dir\n"
> >> >+ "count: 0\n"
> >> >+ "text: 0 3 4 4 "
> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >> >+ "cpath: /\n"
> >> >+ "\n\n"
> >> >+
> >> >+ /* L2P index */
> >> >+ "\0\x80\x40" /* rev 0, 8k entries per page
> */
> >> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in
> 1st
> >> > rev */
> >> >+ "\6\4" /* page size: bytes, count */
> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >> >+
> >> >+ /* P2L index */
> >> >+ "\0\x6b" /* start rev, rev file size */
> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
> >> > bytes */
> >> >+ "\0" /* offset entry 0 page 1 */
> >> >+ /* len, item & type, rev,
> >> > checksum */
> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
> page
> >> > */
> >> >+
> >> >+ /* Footer */
> >> >+ "107 121\7",
> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
> >> This constant makes code unreadable, unmaintainable and very error
> prone.
> >
> >
> > How?
> >
> > To make it more obvious that I intend to follow
> > the svn_io_file_create_binary interface, I added
> > some commentary explaining where the numbers
> > come from. But even just placing the sum (without
> > its elements) in there would be fine already.
> >
> > Changing the rev 0 template has never been a fun
> > activity and wont become one in the foreseeable
> > future.
> >
> I believe that the committer should be responsible forcommitting
> readable and easy to maintain code, not the reviewer. So please fix or
> revert.
>
Sorry, I should have referenced r1605444 in my post.
> >> > Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
> >> > URL:
> >> >
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> >> >
> >> >
> ==============================================================================
> >> > --- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
> >> > +++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21
> >> > 15:15:30 2014
> >> > @@ -23,6 +23,7 @@
> >> > #include "rev_file.h"
> >> > #include "fs_fs.h"
> >> > #include "index.h"
> >> > +#include "low_level.h"
> >> > #include "util.h"
> >> >
> >> > #include "../libsvn_fs/fs-loader.h"
> >> > @@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
> >> > file->stream = NULL;
> >> > file->p2l_stream = NULL;
> >> > file->l2p_stream = NULL;
> >> > + file->block_size = ffd->block_size;
> >> > + file->l2p_offset = -1;
> >> > + file->p2l_offset = -1;
> >> > + file->footer_offset = -1;
> >> > file->pool = pool;
> >> > }
> >> >
> >> > @@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
> >> > }
> >> >
> >> > svn_error_t *
> >> > -svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
> >> > - svn_fs_t *fs,
> >> > - svn_revnum_t rev)
> >> > +svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
> >> > {
> >> > - if (file->file)
> >> > - svn_fs_fs__close_revision_file(file);
> >> > + if (file->l2p_offset == -1)
> >> > + {
> >> > + apr_off_t filesize = 0;
> >> > + unsigned char footer_length;
> >> > + svn_stringbuf_t *footer;
> >> > +
> >> > + /* Determine file size. */
> >> > + SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize,
> >> > file->pool));
> >> > +
> >> > + /* Read last byte (containing the length of the footer). */
> >> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
> >> > NULL,
> >> > + filesize - 1, file->pool));
> >> > + SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
> >> > + sizeof(footer_length), NULL,
> NULL,
> >> > + file->pool));
> >> > +
> >> > + /* Read footer. */
> >> > + footer = svn_stringbuf_create_ensure(footer_length,
> file->pool);
> >> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
> >> > NULL,
> >> > + filesize - 1 - footer_length,
> >> > + file->pool));
> >> > + SVN_ERR(svn_io_file_read_full2(file->file, footer->data,
> >> > footer_length,
> >> > + &footer->len, NULL,
> file->pool));
> >> You're passing pointer to possible 32-bit value to function accepting
> >> pointer to 64-bit value. This will lead unpredictable results.
> >
> >
> > I assume you are referring to the &footer->len?
> > In that case, I think I'm passing an apr_size_t*
> > to an interface expecting an apr_size_t*.
> > What am I missing?
> >
> You're right. I confused svn_io_file_read_full2() with
> svn_io_file_seek(). The code above doesn't have problem I mentioned.
>
ack.
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com> wrote:
> On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 21 June 2014 17:15, <st...@apache.org> wrote:
>> > Author: stefan2
>> > Date: Sat Jun 21 15:15:30 2014
>> > New Revision: 1604421
>> >
>> > URL: http://svn.apache.org/r1604421
>> > Log:
>> > Append index data to the rev data file instead of using separate files.
>> >
>> > This gives unpacked FSFS format 7 similar I/O characteristics and disk
>> > space usage as earlier formats. It also eliminates the need for retries
>> > after a potential background pack run because each file is now always
>> > consistent with itself (earlier, data or index files might already have
>> > been deleted while the respective other was still valid). Overall,
>> > most of this patch is removing code necessary to handle separate files.
>> >
>> > The new file format is simple:
>> >
>> > <rep data><l2p index><p2l index><footer><footer length>
>> >
>> > with the first three being identical what we had before. <footer length>
>> > is a single byte giving the length of the preceding footer, so it's
>> > easier to extract than the pre-f7 rev trailer and there is only one
>> > per pack / rev file.
>> >
>> [..]
>>
>>
>> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>> >URL:
>> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>>
>> > >==============================================================================
>> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21 15:15:30
>> > 2014
>> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >+ "PLAIN\nEND\nENDREP\n"
>> >+ "id: 0.0.r0/2\n"
>> >+ "type: dir\n"
>> >+ "count: 0\n"
>> >+ "text: 0 3 4 4 "
>> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >+ "cpath: /\n"
>> >+ "\n\n"
>> >+
>> >+ /* L2P index */
>> >+ "\0\x80\x40" /* rev 0, 8k entries per page */
>> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in 1st
>> > rev */
>> >+ "\6\4" /* page size: bytes, count */
>> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >+
>> >+ /* P2L index */
>> >+ "\0\x6b" /* start rev, rev file size */
>> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
>> > bytes */
>> >+ "\0" /* offset entry 0 page 1 */
>> >+ /* len, item & type, rev,
>> > checksum */
>> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k page
>> > */
>> >+
>> >+ /* Footer */
>> >+ "107 121\7",
>> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> This constant makes code unreadable, unmaintainable and very error prone.
>
>
> How?
>
> To make it more obvious that I intend to follow
> the svn_io_file_create_binary interface, I added
> some commentary explaining where the numbers
> come from. But even just placing the sum (without
> its elements) in there would be fine already.
>
> Changing the rev 0 template has never been a fun
> activity and wont become one in the foreseeable
> future.
>
I believe that the committer should be responsible forcommitting
readable and easy to maintain code, not the reviewer. So please fix or
revert.
>>
>>
>> > Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
>> > URL:
>> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>> >
>> > ==============================================================================
>> > --- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
>> > +++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21
>> > 15:15:30 2014
>> > @@ -23,6 +23,7 @@
>> > #include "rev_file.h"
>> > #include "fs_fs.h"
>> > #include "index.h"
>> > +#include "low_level.h"
>> > #include "util.h"
>> >
>> > #include "../libsvn_fs/fs-loader.h"
>> > @@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
>> > file->stream = NULL;
>> > file->p2l_stream = NULL;
>> > file->l2p_stream = NULL;
>> > + file->block_size = ffd->block_size;
>> > + file->l2p_offset = -1;
>> > + file->p2l_offset = -1;
>> > + file->footer_offset = -1;
>> > file->pool = pool;
>> > }
>> >
>> > @@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
>> > }
>> >
>> > svn_error_t *
>> > -svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
>> > - svn_fs_t *fs,
>> > - svn_revnum_t rev)
>> > +svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
>> > {
>> > - if (file->file)
>> > - svn_fs_fs__close_revision_file(file);
>> > + if (file->l2p_offset == -1)
>> > + {
>> > + apr_off_t filesize = 0;
>> > + unsigned char footer_length;
>> > + svn_stringbuf_t *footer;
>> > +
>> > + /* Determine file size. */
>> > + SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize,
>> > file->pool));
>> > +
>> > + /* Read last byte (containing the length of the footer). */
>> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
>> > NULL,
>> > + filesize - 1, file->pool));
>> > + SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
>> > + sizeof(footer_length), NULL, NULL,
>> > + file->pool));
>> > +
>> > + /* Read footer. */
>> > + footer = svn_stringbuf_create_ensure(footer_length, file->pool);
>> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
>> > NULL,
>> > + filesize - 1 - footer_length,
>> > + file->pool));
>> > + SVN_ERR(svn_io_file_read_full2(file->file, footer->data,
>> > footer_length,
>> > + &footer->len, NULL, file->pool));
>> You're passing pointer to possible 32-bit value to function accepting
>> pointer to 64-bit value. This will lead unpredictable results.
>
>
> I assume you are referring to the &footer->len?
> In that case, I think I'm passing an apr_size_t*
> to an interface expecting an apr_size_t*.
> What am I missing?
>
You're right. I confused svn_io_file_read_full2() with
svn_io_file_seek(). The code above doesn't have problem I mentioned.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com> wrote:
> On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 21 June 2014 17:15, <st...@apache.org> wrote:
>> > Author: stefan2
>> > Date: Sat Jun 21 15:15:30 2014
>> > New Revision: 1604421
>> >
>> > URL: http://svn.apache.org/r1604421
>> > Log:
>> > Append index data to the rev data file instead of using separate files.
>> >
>> > This gives unpacked FSFS format 7 similar I/O characteristics and disk
>> > space usage as earlier formats. It also eliminates the need for retries
>> > after a potential background pack run because each file is now always
>> > consistent with itself (earlier, data or index files might already have
>> > been deleted while the respective other was still valid). Overall,
>> > most of this patch is removing code necessary to handle separate files.
>> >
>> > The new file format is simple:
>> >
>> > <rep data><l2p index><p2l index><footer><footer length>
>> >
>> > with the first three being identical what we had before. <footer length>
>> > is a single byte giving the length of the preceding footer, so it's
>> > easier to extract than the pre-f7 rev trailer and there is only one
>> > per pack / rev file.
>> >
>> [..]
>>
>>
>> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>> >URL:
>> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>>
>> > >==============================================================================
>> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21 15:15:30
>> > 2014
>> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >+ "PLAIN\nEND\nENDREP\n"
>> >+ "id: 0.0.r0/2\n"
>> >+ "type: dir\n"
>> >+ "count: 0\n"
>> >+ "text: 0 3 4 4 "
>> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >+ "cpath: /\n"
>> >+ "\n\n"
>> >+
>> >+ /* L2P index */
>> >+ "\0\x80\x40" /* rev 0, 8k entries per page */
>> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in 1st
>> > rev */
>> >+ "\6\4" /* page size: bytes, count */
>> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >+
>> >+ /* P2L index */
>> >+ "\0\x6b" /* start rev, rev file size */
>> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
>> > bytes */
>> >+ "\0" /* offset entry 0 page 1 */
>> >+ /* len, item & type, rev,
>> > checksum */
>> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k page
>> > */
>> >+
>> >+ /* Footer */
>> >+ "107 121\7",
>> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> This constant makes code unreadable, unmaintainable and very error prone.
>
>
> How?
>
> To make it more obvious that I intend to follow
> the svn_io_file_create_binary interface, I added
> some commentary explaining where the numbers
> come from. But even just placing the sum (without
> its elements) in there would be fine already.
>
> Changing the rev 0 template has never been a fun
> activity and wont become one in the foreseeable
> future.
>
I believe that the committer should be responsible forcommitting
readable and easy to maintain code, not the reviewer. So please fix or
revert.
>>
>>
>> > Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
>> > URL:
>> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>> >
>> > ==============================================================================
>> > --- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
>> > +++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21
>> > 15:15:30 2014
>> > @@ -23,6 +23,7 @@
>> > #include "rev_file.h"
>> > #include "fs_fs.h"
>> > #include "index.h"
>> > +#include "low_level.h"
>> > #include "util.h"
>> >
>> > #include "../libsvn_fs/fs-loader.h"
>> > @@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
>> > file->stream = NULL;
>> > file->p2l_stream = NULL;
>> > file->l2p_stream = NULL;
>> > + file->block_size = ffd->block_size;
>> > + file->l2p_offset = -1;
>> > + file->p2l_offset = -1;
>> > + file->footer_offset = -1;
>> > file->pool = pool;
>> > }
>> >
>> > @@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
>> > }
>> >
>> > svn_error_t *
>> > -svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
>> > - svn_fs_t *fs,
>> > - svn_revnum_t rev)
>> > +svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
>> > {
>> > - if (file->file)
>> > - svn_fs_fs__close_revision_file(file);
>> > + if (file->l2p_offset == -1)
>> > + {
>> > + apr_off_t filesize = 0;
>> > + unsigned char footer_length;
>> > + svn_stringbuf_t *footer;
>> > +
>> > + /* Determine file size. */
>> > + SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize,
>> > file->pool));
>> > +
>> > + /* Read last byte (containing the length of the footer). */
>> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
>> > NULL,
>> > + filesize - 1, file->pool));
>> > + SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
>> > + sizeof(footer_length), NULL, NULL,
>> > + file->pool));
>> > +
>> > + /* Read footer. */
>> > + footer = svn_stringbuf_create_ensure(footer_length, file->pool);
>> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
>> > NULL,
>> > + filesize - 1 - footer_length,
>> > + file->pool));
>> > + SVN_ERR(svn_io_file_read_full2(file->file, footer->data,
>> > footer_length,
>> > + &footer->len, NULL, file->pool));
>> You're passing pointer to possible 32-bit value to function accepting
>> pointer to 64-bit value. This will lead unpredictable results.
>
>
> I assume you are referring to the &footer->len?
> In that case, I think I'm passing an apr_size_t*
> to an interface expecting an apr_size_t*.
> What am I missing?
>
You're right. I confused svn_io_file_read_full2() with
svn_io_file_seek(). The code above doesn't have problem I mentioned.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 21 June 2014 17:15, <st...@apache.org> wrote:
> > Author: stefan2
> > Date: Sat Jun 21 15:15:30 2014
> > New Revision: 1604421
> >
> > URL: http://svn.apache.org/r1604421
> > Log:
> > Append index data to the rev data file instead of using separate files.
> >
> > This gives unpacked FSFS format 7 similar I/O characteristics and disk
> > space usage as earlier formats. It also eliminates the need for retries
> > after a potential background pack run because each file is now always
> > consistent with itself (earlier, data or index files might already have
> > been deleted while the respective other was still valid). Overall,
> > most of this patch is removing code necessary to handle separate files.
> >
> > The new file format is simple:
> >
> > <rep data><l2p index><p2l index><footer><footer length>
> >
> > with the first three being identical what we had before. <footer length>
> > is a single byte giving the length of the preceding footer, so it's
> > easier to extract than the pre-f7 rev trailer and there is only one
> > per pack / rev file.
> >
> [..]
>
>
> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>
> >==============================================================================
> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21 15:15:30
> 2014
> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >+ "PLAIN\nEND\nENDREP\n"
> >+ "id: 0.0.r0/2\n"
> >+ "type: dir\n"
> >+ "count: 0\n"
> >+ "text: 0 3 4 4 "
> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >+ "cpath: /\n"
> >+ "\n\n"
> >+
> >+ /* L2P index */
> >+ "\0\x80\x40" /* rev 0, 8k entries per page */
> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in 1st
> rev */
> >+ "\6\4" /* page size: bytes, count */
> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >+
> >+ /* P2L index */
> >+ "\0\x6b" /* start rev, rev file size */
> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
> bytes */
> >+ "\0" /* offset entry 0 page 1 */
> >+ /* len, item & type, rev,
> checksum */
> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k page
> */
> >+
> >+ /* Footer */
> >+ "107 121\7",
> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
> This constant makes code unreadable, unmaintainable and very error prone.
>
How?
To make it more obvious that I intend to follow
the svn_io_file_create_binary interface, I added
some commentary explaining where the numbers
come from. But even just placing the sum (without
its elements) in there would be fine already.
Changing the rev 0 template has never been a fun
activity and wont become one in the foreseeable
future.
>
> > Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
> > +++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21
> 15:15:30 2014
> > @@ -23,6 +23,7 @@
> > #include "rev_file.h"
> > #include "fs_fs.h"
> > #include "index.h"
> > +#include "low_level.h"
> > #include "util.h"
> >
> > #include "../libsvn_fs/fs-loader.h"
> > @@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
> > file->stream = NULL;
> > file->p2l_stream = NULL;
> > file->l2p_stream = NULL;
> > + file->block_size = ffd->block_size;
> > + file->l2p_offset = -1;
> > + file->p2l_offset = -1;
> > + file->footer_offset = -1;
> > file->pool = pool;
> > }
> >
> > @@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
> > }
> >
> > svn_error_t *
> > -svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
> > - svn_fs_t *fs,
> > - svn_revnum_t rev)
> > +svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
> > {
> > - if (file->file)
> > - svn_fs_fs__close_revision_file(file);
> > + if (file->l2p_offset == -1)
> > + {
> > + apr_off_t filesize = 0;
> > + unsigned char footer_length;
> > + svn_stringbuf_t *footer;
> > +
> > + /* Determine file size. */
> > + SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize,
> file->pool));
> > +
> > + /* Read last byte (containing the length of the footer). */
> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
> NULL,
> > + filesize - 1, file->pool));
> > + SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
> > + sizeof(footer_length), NULL, NULL,
> > + file->pool));
> > +
> > + /* Read footer. */
> > + footer = svn_stringbuf_create_ensure(footer_length, file->pool);
> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
> NULL,
> > + filesize - 1 - footer_length,
> > + file->pool));
> > + SVN_ERR(svn_io_file_read_full2(file->file, footer->data,
> footer_length,
> > + &footer->len, NULL, file->pool));
> You're passing pointer to possible 32-bit value to function accepting
> pointer to 64-bit value. This will lead unpredictable results.
>
I assume you are referring to the &footer->len?
In that case, I think I'm passing an apr_size_t*
to an interface expecting an apr_size_t*.
What am I missing?
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 21 June 2014 17:15, <st...@apache.org> wrote:
> > Author: stefan2
> > Date: Sat Jun 21 15:15:30 2014
> > New Revision: 1604421
> >
> > URL: http://svn.apache.org/r1604421
> > Log:
> > Append index data to the rev data file instead of using separate files.
> >
> > This gives unpacked FSFS format 7 similar I/O characteristics and disk
> > space usage as earlier formats. It also eliminates the need for retries
> > after a potential background pack run because each file is now always
> > consistent with itself (earlier, data or index files might already have
> > been deleted while the respective other was still valid). Overall,
> > most of this patch is removing code necessary to handle separate files.
> >
> > The new file format is simple:
> >
> > <rep data><l2p index><p2l index><footer><footer length>
> >
> > with the first three being identical what we had before. <footer length>
> > is a single byte giving the length of the preceding footer, so it's
> > easier to extract than the pre-f7 rev trailer and there is only one
> > per pack / rev file.
> >
> [..]
>
>
> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>
> >==============================================================================
> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21 15:15:30
> 2014
> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >+ "PLAIN\nEND\nENDREP\n"
> >+ "id: 0.0.r0/2\n"
> >+ "type: dir\n"
> >+ "count: 0\n"
> >+ "text: 0 3 4 4 "
> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >+ "cpath: /\n"
> >+ "\n\n"
> >+
> >+ /* L2P index */
> >+ "\0\x80\x40" /* rev 0, 8k entries per page */
> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in 1st
> rev */
> >+ "\6\4" /* page size: bytes, count */
> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >+
> >+ /* P2L index */
> >+ "\0\x6b" /* start rev, rev file size */
> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
> bytes */
> >+ "\0" /* offset entry 0 page 1 */
> >+ /* len, item & type, rev,
> checksum */
> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k page
> */
> >+
> >+ /* Footer */
> >+ "107 121\7",
> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
> This constant makes code unreadable, unmaintainable and very error prone.
>
How?
To make it more obvious that I intend to follow
the svn_io_file_create_binary interface, I added
some commentary explaining where the numbers
come from. But even just placing the sum (without
its elements) in there would be fine already.
Changing the rev 0 template has never been a fun
activity and wont become one in the foreseeable
future.
>
> > Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
> > +++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21
> 15:15:30 2014
> > @@ -23,6 +23,7 @@
> > #include "rev_file.h"
> > #include "fs_fs.h"
> > #include "index.h"
> > +#include "low_level.h"
> > #include "util.h"
> >
> > #include "../libsvn_fs/fs-loader.h"
> > @@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
> > file->stream = NULL;
> > file->p2l_stream = NULL;
> > file->l2p_stream = NULL;
> > + file->block_size = ffd->block_size;
> > + file->l2p_offset = -1;
> > + file->p2l_offset = -1;
> > + file->footer_offset = -1;
> > file->pool = pool;
> > }
> >
> > @@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
> > }
> >
> > svn_error_t *
> > -svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
> > - svn_fs_t *fs,
> > - svn_revnum_t rev)
> > +svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
> > {
> > - if (file->file)
> > - svn_fs_fs__close_revision_file(file);
> > + if (file->l2p_offset == -1)
> > + {
> > + apr_off_t filesize = 0;
> > + unsigned char footer_length;
> > + svn_stringbuf_t *footer;
> > +
> > + /* Determine file size. */
> > + SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize,
> file->pool));
> > +
> > + /* Read last byte (containing the length of the footer). */
> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
> NULL,
> > + filesize - 1, file->pool));
> > + SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
> > + sizeof(footer_length), NULL, NULL,
> > + file->pool));
> > +
> > + /* Read footer. */
> > + footer = svn_stringbuf_create_ensure(footer_length, file->pool);
> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
> NULL,
> > + filesize - 1 - footer_length,
> > + file->pool));
> > + SVN_ERR(svn_io_file_read_full2(file->file, footer->data,
> footer_length,
> > + &footer->len, NULL, file->pool));
> You're passing pointer to possible 32-bit value to function accepting
> pointer to 64-bit value. This will lead unpredictable results.
>
I assume you are referring to the &footer->len?
In that case, I think I'm passing an apr_size_t*
to an interface expecting an apr_size_t*.
What am I missing?
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 21 June 2014 17:15, <st...@apache.org> wrote:
> Author: stefan2
> Date: Sat Jun 21 15:15:30 2014
> New Revision: 1604421
>
> URL: http://svn.apache.org/r1604421
> Log:
> Append index data to the rev data file instead of using separate files.
>
> This gives unpacked FSFS format 7 similar I/O characteristics and disk
> space usage as earlier formats. It also eliminates the need for retries
> after a potential background pack run because each file is now always
> consistent with itself (earlier, data or index files might already have
> been deleted while the respective other was still valid). Overall,
> most of this patch is removing code necessary to handle separate files.
>
> The new file format is simple:
>
> <rep data><l2p index><p2l index><footer><footer length>
>
> with the first three being identical what we had before. <footer length>
> is a single byte giving the length of the preceding footer, so it's
> easier to extract than the pre-f7 rev trailer and there is only one
> per pack / rev file.
>
[..]
>Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>==============================================================================
>--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21 15:15:30 2014
>@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>+ "PLAIN\nEND\nENDREP\n"
>+ "id: 0.0.r0/2\n"
>+ "type: dir\n"
>+ "count: 0\n"
>+ "text: 0 3 4 4 "
>+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>+ "cpath: /\n"
>+ "\n\n"
>+
>+ /* L2P index */
>+ "\0\x80\x40" /* rev 0, 8k entries per page */
>+ "\1\1\1" /* 1 rev, 1 page, 1 page in 1st rev */
>+ "\6\4" /* page size: bytes, count */
>+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>+
>+ /* P2L index */
>+ "\0\x6b" /* start rev, rev file size */
>+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29 bytes */
>+ "\0" /* offset entry 0 page 1 */
>+ /* len, item & type, rev, checksum */
>+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k page */
>+
>+ /* Footer */
>+ "107 121\7",
>+ 107 + 14 + 38 + 7 + 1, fs->pool));
This constant makes code unreadable, unmaintainable and very error prone.
> Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21 15:15:30 2014
> @@ -23,6 +23,7 @@
> #include "rev_file.h"
> #include "fs_fs.h"
> #include "index.h"
> +#include "low_level.h"
> #include "util.h"
>
> #include "../libsvn_fs/fs-loader.h"
> @@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
> file->stream = NULL;
> file->p2l_stream = NULL;
> file->l2p_stream = NULL;
> + file->block_size = ffd->block_size;
> + file->l2p_offset = -1;
> + file->p2l_offset = -1;
> + file->footer_offset = -1;
> file->pool = pool;
> }
>
> @@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
> }
>
> svn_error_t *
> -svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
> - svn_fs_t *fs,
> - svn_revnum_t rev)
> +svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
> {
> - if (file->file)
> - svn_fs_fs__close_revision_file(file);
> + if (file->l2p_offset == -1)
> + {
> + apr_off_t filesize = 0;
> + unsigned char footer_length;
> + svn_stringbuf_t *footer;
> +
> + /* Determine file size. */
> + SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize, file->pool));
> +
> + /* Read last byte (containing the length of the footer). */
> + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size, NULL,
> + filesize - 1, file->pool));
> + SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
> + sizeof(footer_length), NULL, NULL,
> + file->pool));
> +
> + /* Read footer. */
> + footer = svn_stringbuf_create_ensure(footer_length, file->pool);
> + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size, NULL,
> + filesize - 1 - footer_length,
> + file->pool));
> + SVN_ERR(svn_io_file_read_full2(file->file, footer->data, footer_length,
> + &footer->len, NULL, file->pool));
You're passing pointer to possible 32-bit value to function accepting
pointer to 64-bit value. This will lead unpredictable results.
> + footer->data[footer->len] = '\0';
> +
> + /* Extract index locations. */
> + SVN_ERR(svn_fs_fs__parse_footer(&file->l2p_offset, &file->p2l_offset,
> + footer, file->start_revision));
> + file->footer_offset = filesize - footer_length - 1;
> + }
>
> - return svn_error_trace(open_pack_or_rev_file(file, fs, rev, file->pool));
> + return SVN_NO_ERROR;
> }
>
> svn_error_t *
> @@ -165,9 +196,6 @@ svn_fs_fs__close_revision_file(svn_fs_fs
> if (file->file)
> SVN_ERR(svn_io_file_close(file->file, file->pool));
>
> - SVN_ERR(svn_fs_fs__packed_stream_close(file->l2p_stream));
> - SVN_ERR(svn_fs_fs__packed_stream_close(file->p2l_stream));
> -
> file->file = NULL;
> file->stream = NULL;
> file->l2p_stream = NULL;
>
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 21 June 2014 17:15, <st...@apache.org> wrote:
> Author: stefan2
> Date: Sat Jun 21 15:15:30 2014
> New Revision: 1604421
>
> URL: http://svn.apache.org/r1604421
> Log:
> Append index data to the rev data file instead of using separate files.
>
> This gives unpacked FSFS format 7 similar I/O characteristics and disk
> space usage as earlier formats. It also eliminates the need for retries
> after a potential background pack run because each file is now always
> consistent with itself (earlier, data or index files might already have
> been deleted while the respective other was still valid). Overall,
> most of this patch is removing code necessary to handle separate files.
>
> The new file format is simple:
>
> <rep data><l2p index><p2l index><footer><footer length>
>
> with the first three being identical what we had before. <footer length>
> is a single byte giving the length of the preceding footer, so it's
> easier to extract than the pre-f7 rev trailer and there is only one
> per pack / rev file.
>
[..]
>Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>==============================================================================
>--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21 15:15:30 2014
>@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>+ "PLAIN\nEND\nENDREP\n"
>+ "id: 0.0.r0/2\n"
>+ "type: dir\n"
>+ "count: 0\n"
>+ "text: 0 3 4 4 "
>+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>+ "cpath: /\n"
>+ "\n\n"
>+
>+ /* L2P index */
>+ "\0\x80\x40" /* rev 0, 8k entries per page */
>+ "\1\1\1" /* 1 rev, 1 page, 1 page in 1st rev */
>+ "\6\4" /* page size: bytes, count */
>+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>+
>+ /* P2L index */
>+ "\0\x6b" /* start rev, rev file size */
>+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29 bytes */
>+ "\0" /* offset entry 0 page 1 */
>+ /* len, item & type, rev, checksum */
>+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k page */
>+
>+ /* Footer */
>+ "107 121\7",
>+ 107 + 14 + 38 + 7 + 1, fs->pool));
This constant makes code unreadable, unmaintainable and very error prone.
> Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21 15:15:30 2014
> @@ -23,6 +23,7 @@
> #include "rev_file.h"
> #include "fs_fs.h"
> #include "index.h"
> +#include "low_level.h"
> #include "util.h"
>
> #include "../libsvn_fs/fs-loader.h"
> @@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
> file->stream = NULL;
> file->p2l_stream = NULL;
> file->l2p_stream = NULL;
> + file->block_size = ffd->block_size;
> + file->l2p_offset = -1;
> + file->p2l_offset = -1;
> + file->footer_offset = -1;
> file->pool = pool;
> }
>
> @@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
> }
>
> svn_error_t *
> -svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
> - svn_fs_t *fs,
> - svn_revnum_t rev)
> +svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
> {
> - if (file->file)
> - svn_fs_fs__close_revision_file(file);
> + if (file->l2p_offset == -1)
> + {
> + apr_off_t filesize = 0;
> + unsigned char footer_length;
> + svn_stringbuf_t *footer;
> +
> + /* Determine file size. */
> + SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize, file->pool));
> +
> + /* Read last byte (containing the length of the footer). */
> + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size, NULL,
> + filesize - 1, file->pool));
> + SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
> + sizeof(footer_length), NULL, NULL,
> + file->pool));
> +
> + /* Read footer. */
> + footer = svn_stringbuf_create_ensure(footer_length, file->pool);
> + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size, NULL,
> + filesize - 1 - footer_length,
> + file->pool));
> + SVN_ERR(svn_io_file_read_full2(file->file, footer->data, footer_length,
> + &footer->len, NULL, file->pool));
You're passing pointer to possible 32-bit value to function accepting
pointer to 64-bit value. This will lead unpredictable results.
> + footer->data[footer->len] = '\0';
> +
> + /* Extract index locations. */
> + SVN_ERR(svn_fs_fs__parse_footer(&file->l2p_offset, &file->p2l_offset,
> + footer, file->start_revision));
> + file->footer_offset = filesize - footer_length - 1;
> + }
>
> - return svn_error_trace(open_pack_or_rev_file(file, fs, rev, file->pool));
> + return SVN_NO_ERROR;
> }
>
> svn_error_t *
> @@ -165,9 +196,6 @@ svn_fs_fs__close_revision_file(svn_fs_fs
> if (file->file)
> SVN_ERR(svn_io_file_close(file->file, file->pool));
>
> - SVN_ERR(svn_fs_fs__packed_stream_close(file->l2p_stream));
> - SVN_ERR(svn_fs_fs__packed_stream_close(file->p2l_stream));
> -
> file->file = NULL;
> file->stream = NULL;
> file->l2p_stream = NULL;
>
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com