subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Branko ─îibej <br...@wandisco.com>
Subject Re: svn commit: r1680819 - /subversion/trunk/subversion/libsvn_fs_fs/revprops.c
Date Fri, 22 May 2015 06:40:59 GMT
On 22.05.2015 00:09, Philip Martin wrote:
> Ivan Zhakov <ivan@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



Mime
View raw message