[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: Branko ─îibej <brane_at_wandisco.com>
Date: Fri, 22 May 2015 08:40:59 +0200

On 22.05.2015 00:09, Philip Martin wrote:
> Ivan Zhakov <ivan_at_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),
> - *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


    $ grep -r svn_stream_from_aprfile2 subversion | wc -l
    $ grep -r svn_stream_from_aprfile2 subversion/libsvn_fs_fs | wc -l
Received on 2015-05-22 08:42:04 CEST

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