Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
MIME-Version: 1.0
From: Julian Foad <julianfoad@gmail.com>
Date: Tue, 26 May 2015 22:15:21 +0100
Message-ID: 
 <CAEcU=1aoCRNNA1A9axvgwZGefaxFzp0TE7U-0ZqrgaBNtxBZhA@mail.gmail.com>
Subject: Queries about rep cache: get_shared_rep()
To: dev <dev@subversion.apache.org>
Content-Type: text/plain; charset=UTF-8

Index: subversion/libsvn_fs_fs/transaction.c
===================================================================
--- subversion/libsvn_fs_fs/transaction.c    (revision 1681856)
+++ subversion/libsvn_fs_fs/transaction.c    (working copy)
@@ -2243,12 +2243,14 @@ (representation_t **old_re
       const char *file_name
         = path_txn_sha1(fs, &rep->txn_id, rep->sha1_digest, scratch_pool);

       /* in our txn, is there a rep file named with the wanted SHA1?
          If so, read it and use that rep.
        */
+      /* ### Would it be faster (and so better) to just try reading it,
+             and handle ENOENT, instead of first checking for presence? */
       SVN_ERR(svn_io_check_path(file_name, &kind, scratch_pool));
       if (kind == svn_node_file)
         {
           svn_stringbuf_t *rep_string;
           SVN_ERR(svn_stringbuf_from_file2(&rep_string, file_name,
                                            scratch_pool));
@@ -2262,14 +2264,20 @@ get_shared_rep(representation_t **old_re

   /* A simple guard against general rep-cache induced corruption. */
   if ((*old_rep)->expanded_size != rep->expanded_size)
     {
       /* Make the problem show up in the server log.

-         Because not sharing reps is always a save option,
+         Because not sharing reps is always a safe option,
          terminating the request would be inappropriate.
+
+         ### [JAF] Why should we assume the cache is corrupt rather than the
+                   new rep is corrupt? Is this really the logic we want?
+
+                   Should we do something more forceful -- flag the cache as
+                   unusable, perhaps -- rather than just logging a warning?
        */
       svn_checksum_t checksum;
       checksum.digest = rep->sha1_digest;
       checksum.kind = svn_checksum_sha1;

       err = svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,


- Julian