On Sun, Mar 29, 2015 at 9:44 AM, Daniel Shahaf <d...@daniel.shahaf.name> wrote:
> stef...@apache.org wrote on Sat, Mar 28, 2015 at 10:58:13 -0000: > > Author: stefan2 > > Date: Sat Mar 28 10:58:13 2015 > > New Revision: 1669743 > > > > URL: http://svn.apache.org/r1669743 > > Log: > > Follow-up to r1654934: > > Relax the size check in the FSFS representation sharing code to make it > as > > effective as in earlier releases. > > > > The size check has been to strict and would not allow two matching reps > to > > have a different base representation, e.g. mod on /trunk and add-without- > > history on a branch. Mainly depending on the merge policy (catch-up vs. > > cherry-pick), this resulted in detecting and eliding fewer instances of > > redundant representations. Hence, larger repositories. > > > > * subversion/libsvn_fs_fs/transaction.c > > (get_shared_rep): Care about the on-disk size only if the fulltext / > > expanded size is not known - which would be the case > > for e.g. PLAIN representations. > > > > Modified: > > subversion/trunk/subversion/libsvn_fs_fs/transaction.c > > > > Modified: subversion/trunk/subversion/libsvn_fs_fs/transaction.c > > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/transaction.c?rev=1669743&r1=1669742&r2=1669743&view=diff > > > ============================================================================== > > --- subversion/trunk/subversion/libsvn_fs_fs/transaction.c (original) > > +++ subversion/trunk/subversion/libsvn_fs_fs/transaction.c Sat Mar 28 > 10:58:13 2015 > > @@ -2214,10 +2214,11 @@ get_shared_rep(representation_t **old_re > > return SVN_NO_ERROR; > > > > /* We don't want 0-length PLAIN representations to replace > non-0-length > > - ones (see issue #4554). Also, this doubles as a simple guard > against > > - general rep-cache induced corruption. */ > > + ones (see issue #4554). Take into account that EXPANDED_SIZE may > be > > + 0 in which case we have to check the on-disk SIZE. Also, this > doubles > > + as a simple guard against general rep-cache induced corruption. */ > > if ( ((*old_rep)->expanded_size != rep->expanded_size) > > - || ((*old_rep)->size != rep->size)) > > + || (rep->expanded_size && ((*old_rep)->size != rep->size))) > > If two reps have EXPANDED_SIZE == 0 and different SIZE, the condition > will be FALSE. Shouldn't it be TRUE instead? > Yikes! Forgot the "!" in front of rep->expanded_size after updating the commentary. Fixed in r1669945. When comparing SIZE, shouldn't it also compare the rep kind (PLAIN or > DELTA) at the same time? Otherwise, two reps that have the same > EXPANDED_SIZE, different SIZE, and different kinds wouldn't be shared, > even though they could be. > The condition as it stands now on /trunk should be minimal and correct. Thanks for the review! -- Stefan^2.
