[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1680819 - /subversion/trunk/subversion/libsvn_fs_fs/revprops.c

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Wed, 27 May 2015 18:19:15 +0100

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
Received on 2015-05-27 19:23:05 CEST

This is an archived mail posted to the Subversion Dev mailing list.