RE: svn commit: r1580406 - /subversion/trunk/subversion/libsvn_fs_fs/cached_data.c

[Bert Huijben <be...@qqmail.nl>]

> -----Original Message-----
> From: stefan2@apache.org [mailto:stefan2@apache.org]
> Sent: zondag 23 maart 2014 02:17
> To: commits@subversion.apache.org
> Subject: svn commit: r1580406 -
> /subversion/trunk/subversion/libsvn_fs_fs/cached_data.c
> 
> Author: stefan2
> Date: Sun Mar 23 01:16:31 2014
> New Revision: 1580406
> 
> URL: http://svn.apache.org/r1580406
> Log:
> * subversion/libsvn_fs_fs/cached_data.c
>   (svn_fs_fs__get_file_delta_stream): Use the plain delta windows only
>                                       if we don't want and can provide
>                                       simple plaintext.
> 
> Modified:
>     subversion/trunk/subversion/libsvn_fs_fs/cached_data.c
> 
> Modified: subversion/trunk/subversion/libsvn_fs_fs/cached_data.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/ca
> ched_data.c?rev=1580406&r1=1580405&r2=1580406&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_fs/cached_data.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/cached_data.c Sun Mar 23
> 01:16:31 2014
> @@ -1883,10 +1883,11 @@ svn_fs_fs__get_file_delta_stream(svn_txd
>    svn_stream_t *source_stream, *target_stream;
>    rep_state_t *rep_state;
>    svn_fs_fs__rep_header_t *rep_header;
> +  fs_fs_data_t *ffd = fs->fsap_data;
> 
>    /* Try a shortcut: if the target is stored as a delta against the
>       source, then just use that delta. */
> -  if (target->data_rep)
> +  if (target->data_rep && (source || ! ffd->fulltext_cache))
>      {
>        /* Read target's base rep if any. */
>        SVN_ERR(create_rep_state(&rep_state, &rep_header, NULL,

With just your log message I can't really see what you change here, but I would guess the comment above the check needs an update now.

Is the final result still a delta stream against the source in the cases where it was before?

(Or are there cases where we now get a delta against the empty stream where we didn't... or the other way around?)

	Bert

Re: svn commit: r1580406 - /subversion/trunk/subversion/libsvn_fs_fs/cached_data.c

[Stefan Fuhrmann <st...@wandisco.com>]

On Sun, Mar 23, 2014 at 2:38 AM, Bert Huijben <be...@qqmail.nl> wrote:

>
> > -----Original Message-----
> > From: stefan2@apache.org [mailto:stefan2@apache.org]
> > Sent: zondag 23 maart 2014 02:17
> > To: commits@subversion.apache.org
> > Subject: svn commit: r1580406 -
> > /subversion/trunk/subversion/libsvn_fs_fs/cached_data.c
> >
> > Author: stefan2
> > Date: Sun Mar 23 01:16:31 2014
> > New Revision: 1580406
> >
> > URL: http://svn.apache.org/r1580406
> > Log:
> > * subversion/libsvn_fs_fs/cached_data.c
> >   (svn_fs_fs__get_file_delta_stream): Use the plain delta windows only
> >                                       if we don't want and can provide
> >                                       simple plaintext.
>

I updated the log message.


> >
> > Modified:
> >     subversion/trunk/subversion/libsvn_fs_fs/cached_data.c
> >
> > Modified: subversion/trunk/subversion/libsvn_fs_fs/cached_data.c
> > URL:
> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/ca
> > ched_data.c?rev=1580406&r1=1580405&r2=1580406&view=diff
> > ==========================================================
> > ====================
> > --- subversion/trunk/subversion/libsvn_fs_fs/cached_data.c (original)
> > +++ subversion/trunk/subversion/libsvn_fs_fs/cached_data.c Sun Mar 23
> > 01:16:31 2014
> > @@ -1883,10 +1883,11 @@ svn_fs_fs__get_file_delta_stream(svn_txd
> >    svn_stream_t *source_stream, *target_stream;
> >    rep_state_t *rep_state;
> >    svn_fs_fs__rep_header_t *rep_header;
> > +  fs_fs_data_t *ffd = fs->fsap_data;
> >
> >    /* Try a shortcut: if the target is stored as a delta against the
> >       source, then just use that delta. */
> > -  if (target->data_rep)
> > +  if (target->data_rep && (source || ! ffd->fulltext_cache))
> >      {
> >        /* Read target's base rep if any. */
> >        SVN_ERR(create_rep_state(&rep_state, &rep_header, NULL,
>
> With just your log message I can't really see what you change here, but I
> would guess the comment above the check needs an update now.
>

Thanks, you are right. Fixed in r1580629.

Is the final result still a delta stream against the source in the cases
> where it was before?
>
> (Or are there cases where we now get a delta against the empty stream
> where we didn't... or the other way around?)
>

The gist of this is that the "shortcut" section is only a
reasonable optimization and the full functionality is
still available when only the code after that if() block
is being executed.

If we want just plaintext (e.g. a delta against a empty
source during checkout), it is better to query and
populate the fulltext cache when available instead
of going for the delta cache. My change added a
check for that latter case: If there is no source rep
and we support fulltext caching, fall back to standard
code. This will access and populate the fulltext cache.

-- Stefan^2.