On 22.05.2015 00:09, Philip Martin wrote:
> Ivan Zhakov <i...@visualsvn.com> writes:
>
>> We considered adding FLUSH_TO_DISK flag to svn_stream_from_apr_file(),
>> but decided to commit simple fix for now:
>> 1. The current FSFS code uses explicit flush and this commit makes
>> code consistent with other parts
>> 2. Adding flag to svn_stream_from_apr_file() will require revving API
>> etc, while this fix should be backported to 1.8.x and 1.9.x
The API argument only applies to 1.8.x, which would need a completely
different fix; not to 1.9.x, because it's not released yet. The benefit
of revising the API (which I tentatively support, see [1] below)
outweighs the pain of having to write a different fix for 1.8.x.
>> 3. It's unclear how FLUSH_TO_DISK flag should interact with DISOWN=TRUE.
>>
>> Taking in account all above we decided to commit simple fix for now
>> and possibly implement FLUSH_TO_DISK flag later applying it to all
>> FSFS code where needed.
> Is it simple? It's possible that r1680819 is more complicated than
> adding flush to the the stream.
>
> Exactly what the API should look like is bit unclear. Is there any need
> to support disown=TRUE and flush=TRUE? When would Subversion use it?
> If we create svn_stream_from_aprfile3 that takes two booleans then
> perhaps we change the return type to allow an error for the impossible
> disown=TRUE and flush=TRUE.
Yup. There's no reason to try to support mutually conflicting arguments.
> All the flushing code goes in libsvn_subr
Just one nit here: the flushing code would indeed be localized in
libsvn_subr, however, the API change would propagate to some 100 places
in the code [1], including lots of places in libsvn_fs_fs. That's unless
we decide /not/ to deprecate svn_stream_from_aprfile2 in the 1.9.x
series and use it in parallel with the prospective svn_stream_from_aprfile3.
FWIW, I think in this case revving the API without deprecating the old
one is justified. Another option would be to invent a different API name
for the flushing stream, e.g., svn_stream_from_aprfile_flushed or some
such. This way we'd also avoid the dilemma about disown by simply not
having that flag in the new function.
> and the code change in FSFS is really small, something like:
>
> Index: subversion/libsvn_fs_fs/revprops.c
> ===================================================================
> --- subversion/libsvn_fs_fs/revprops.c (revision 1680818)
> +++ subversion/libsvn_fs_fs/revprops.c (working copy)
> @@ -874,7 +874,7 @@
> new_filename->data,
> pool),
> APR_WRITE | APR_CREATE, APR_OS_DEFAULT, pool));
> - *stream = svn_stream_from_aprfile2(file, FALSE, pool);
> + SVN_ERR(svn_stream_from_aprfile3(stream, file, FALSE, TRUE, pool));
>
> return SVN_NO_ERROR;
> }
> @@ -1205,7 +1205,8 @@
> 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_from_aprfile3(&stream, pack_file, FALSE, TRUE,
> + scratch_pool));
> SVN_ERR(svn_stream_write(stream, compressed->data, &compressed->len));
> SVN_ERR(svn_stream_close(stream));
>
> A patch like that is possibly easier to review than r1680819.
Quite a bit easier, yes.
-- Brane
[1]
$ grep -r svn_stream_from_aprfile2 subversion | wc -l
101
$ grep -r svn_stream_from_aprfile2 subversion/libsvn_fs_fs | wc -l
21
