Philip Martin wrote:
> Julian Foad writes:
>> Evgeny Kotkov wrote:
>>> Philip, Julian, thank you for giving this patch a look.
>>>
>>> As I said in my previous e-mails, I think that the committed fix is a better
>>> choice here, as opposed to attempts to achieve the same behavior using the
>>> stream API. [...]
>>
>> That sounds good to me -- leaving the code using file APIs.
>
> It only fixes writing revprops during commit, it still leaves the
> problem that the pack operation doesn't flush. We need to add flush
> when writing the revision pack file, the packed indices, the revprop
> pack file and the revprop manifest file. That means replacing more, if
> not all, of the stream code in revprops.c.
Yes, like this, for a start, I suppose:
[[[
Index: subversion/libsvn_fs_fs/revprops.c
===================================================================
--- subversion/libsvn_fs_fs/revprops.c (revision 1682078)
+++ subversion/libsvn_fs_fs/revprops.c (working copy)
@@ -1170,7 +1170,6 @@ svn_fs_fs__copy_revprops(const char *pac
apr_file_t *pack_file;
svn_revnum_t rev;
apr_pool_t *iterpool = svn_pool_create(scratch_pool);
- svn_stream_t *stream;
/* create empty data buffer and a write stream on top of it */
svn_stringbuf_t *uncompressed
@@ -1194,6 +1193,7 @@ svn_fs_fs__copy_revprops(const char *pac
for (rev = start_rev; rev <= end_rev; rev++)
{
const char *path;
+ svn_stream_t *stream;
svn_pool_clear(iterpool);
@@ -1214,10 +1214,11 @@ svn_fs_fs__copy_revprops(const char *pac
/* compress the content (or just store it for COMPRESSION_LEVEL 0) */
SVN_ERR(svn__compress(uncompressed, compressed, compression_level));
- /* write the pack file content to disk */
- stream = svn_stream_from_aprfile2(pack_file, FALSE, scratch_pool);
- SVN_ERR(svn_stream_write(stream, compressed->data, &compressed->len));
- SVN_ERR(svn_stream_close(stream));
+ /* write and flush the pack file content to disk */
+ SVN_ERR(svn_io_file_write_full(pack_file, compressed->data, compressed->len,
+ NULL, scratch_pool));
+ SVN_ERR(svn_io_file_flush_to_disk(pack_file, scratch_pool));
+ SVN_ERR(svn_io_file_close(pack_file, scratch_pool));
svn_pool_destroy(iterpool);
@@ -1236,6 +1237,7 @@ svn_fs_fs__pack_revprops_shard(const cha
apr_pool_t *scratch_pool)
{
const char *manifest_file_path, *pack_filename = NULL;
+ apr_file_t *manifest_file;
svn_stream_t *manifest_stream;
svn_revnum_t start_rev, end_rev, rev;
apr_off_t total_size;
@@ -1252,8 +1254,13 @@ svn_fs_fs__pack_revprops_shard(const cha
/* Create the new directory and manifest file stream. */
SVN_ERR(svn_io_dir_make(pack_file_dir, APR_OS_DEFAULT, scratch_pool));
- SVN_ERR(svn_stream_open_writable(&manifest_stream, manifest_file_path,
- scratch_pool, scratch_pool));
+ SVN_ERR(svn_io_file_open(&manifest_file, manifest_file_path,
+ APR_WRITE
+ | APR_BUFFERED
+ | APR_CREATE
+ | APR_EXCL,
+ APR_OS_DEFAULT, scratch_pool));
+ manifest_stream = svn_stream_from_aprfile2(manifest_file, TRUE,
scratch_pool);
/* revisions to handle. Special case: revision 0 */
start_rev = (svn_revnum_t) (shard * max_files_per_dir);
@@ -1322,6 +1329,8 @@ svn_fs_fs__pack_revprops_shard(const cha
/* flush the manifest file and update permissions */
SVN_ERR(svn_stream_close(manifest_stream));
+ SVN_ERR(svn_io_file_flush_to_disk(manifest_file, scratch_pool));
+ SVN_ERR(svn_io_file_close(manifest_file, scratch_pool));
SVN_ERR(svn_io_copy_perms(shard_path, pack_file_dir, iterpool));
svn_pool_destroy(iterpool);
]]]
Here I just picked two functions that look like they needed flushes, I
didn't look for all places where it's required, as I just wanted to
check the patch would not be too difficult or ugly.
That looks good to me. Does it to you?
- Julian
