1.8 suboptimal rep-cache deduplication

Philip Martin <philip_at_codematters.co.uk> writes:

> My reproduction doesn't trigger the bug in 1.8 but that seems to be
> because 1.8 has some other problem and the rep-cache deduplication
> doesn't trigger. The third commit restoring the content in the first
> commit simply stores a new delta.

Comparing 1.8 and 1.9 the reason that 1.8 doesn't do deduplication is
this code, fs_fs.c:get_shared_rep:7757

  /* 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. */
  if ( ((*old_rep)->expanded_size != rep->expanded_size)
      || ((*old_rep)->size != rep->size))
    {
      *old_rep = NULL;
    }
  else

In 1.9 the corresponding code is transaction.c:get_shared_rep:2234

  if ( ((*old_rep)->expanded_size != rep->expanded_size)
      || ((rep->expanded_size == 0) && ((*old_rep)->size != rep->size)))
    {
      *old_rep = NULL;
    }
  else

Since 1.8 doesn't have the expanded_size check it sets *old_rep to NULL
and therefore doesn't do deduplication in some cases where it would
otherwise be possible. If I patch 1.8

Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 1826211)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -7754,7 +7754,7 @@ get_shared_rep(representation_t **old_rep,
      ones (see issue #4554). 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 == 0) && (*old_rep)->size != rep->size))
     {
       *old_rep = NULL;
     }

the deduplication happens (and the checksum failure does not occur
because the checksum bug does not affect 1.8).

-- 
Philip