Julian Foad | 26 May 23:15 2015

Queries about rep cache: get_shared_rep()

Index: subversion/libsvn_fs_fs/transaction.c
--- subversion/libsvn_fs_fs/transaction.c    (revision 1681856)
+++ subversion/libsvn_fs_fs/transaction.c    (working copy)
 <at>  <at>  -2243,12 +2243,14  <at>  <at>  (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,
 <at>  <at>  -2262,14 +2264,20  <at>  <at>  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.
(Continue reading)

Evgeny Kotkov | 21 May 17:23 2015

Possible incompatibility of svn_repos_verify_fs2() in 1.9.0-rc1

Subversion 1.9.0-rc1 introduced a new svnadmin verify --keep-going mode [1].
In order to achieve this, we added a svn_repos_verify_fs3() API function and
deprecated its predecessor, svn_repos_verify_fs2(), that now calls the newer
function with keep_going argument set to FALSE.

However, svn_repos_verify_fs2() behaves differently in 1.9.0-rc1 and 1.8.13
when it comes to error reporting.  As an example, the following code ...

  SVN_ERR(svn_repos_verify_fs2(repos, 1, 5, NULL, NULL, NULL, NULL, pool));

...would return two different errors depending on the binaries being used,
assuming that one of the revisions in the [r1:r5] range is corrupted:

 (With 1.8.13)  E160004: Checksum mismatch in item at offset 0 of
    length 59 bytes in file path/asf/db/revs/0/2

 (With 1.9.0-rc1)  E165011: Repository 'path/asf' failed to verify

Please note that the second error is generic, and that the actual information
about the error is lost.  Existing API users of svn_repos_verify_fs2() are
going to lose an important bit of information about the root cause of the
corruption unless they update the code.  Furthermore, even if an API caller
subscribes to notifications with a notify_func / notify_baton pair, it would
still be necessary to update the code to handle svn_repos_notify_failure that
did not exist before 1.9.

I did not find any discussion on the matter or the corresponding entry in
/notes/api-errata [2] that would describe this behavior change, so I am
inclined to think that this is accidental and, probably, undesirable.

(Continue reading)

Marc Strapetz | 19 May 15:59 2015

JavaHL: ClientNotifyCallback reports unexpected kind "file" for symlinks

When recursively adding a directory "test" which contains another 
directory "sub" and a symlink "sub.link" pointing to "sub", "sub.link" 
is reported with kind=file where I would expect to receive kind=symlink. 
The problem can be reproduced by following code snippet, using quite 
recent 1.9 binaries:

final File root = <path-to-a-test-working-copy>;
final File dir = new File(root, "test");

final File sub = new File(dir, "sub");

final File subLink = new File(dir, "sub.link");
Runtime.getRuntime().exec("ln -s " + sub.getAbsolutePath() +
   " " + subLink.getAbsolutePath());

final ISVNClient client = new SVNClient();
// client.revert(dir.getAbsolutePath(), Depth.infinity,
//   new ArrayList<String>());

client.notification2(new ClientNotifyCallback() {
    <at> Override
   public void onNotify(ClientNotifyInformation cni) {
     System.out.println("[" + cni.getKind() + "] " + cni.getPath() +
                        " " + cni.getAction());
client.add(dir.getAbsolutePath(), Depth.infinity, false, false, false);

(Continue reading)

Philip Martin | 19 May 12:24 2015

reversible fingerprint comment in membuffer code

Is this comment in cache-membuffer.c:combine_key correct?

      /* scramble key DATA.  All of this must be reversible to prevent key
       * collisions.  So, we limit ourselves to xor and permutations. */
      data[1] = (data[1] << 27) | (data[1] >> 37);
      data[1] ^= data[0] & 0xffff;
      data[0] ^= data[1] & APR_UINT64_C(0xffffffffffff0000);

I don't see why this needs to be reversible, and it's not clear it is

The comment was added in r1458643 on the cache-server branch


Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*

Lorenz | 18 May 15:54 2015

getting desperate because of assumed regression after 1.8.11 (was: Can someone please verfiy this "svn -v st" behaviour with file externals?)

Hi all,

I'm getting desperate because I think I've spotted a bug/regression in
svn since 1.8.11 and in spite of several post about the topic (both to
users and dev) noone seem interested in verifying or rejecting my

>can someone please check if this realy is a regression, or if it is
>just me doing something wrong.
>I have a problem with "svn -v status" showing an unexpected result for
>file externals.
>This seems to be a regression that happended with svn 1.8.12/13
>I've tried this with the binaries bundled with TSVN 1.8.11 (svn
>1.8.13) and TSVN trunk nightly (build against
>I've also tried with the 1.8.13 windows builds from
>http://sourceforge.net/projects/win32svn/ and
>http://www.visualsvn.com/downloads/ with the same results.
>Using svn 1.8.11 binaries (bundled with TSVN 1.8.10) the is effect is
>not reproducible.
>After freshly checking out a working copy (containing file externals)
>at first everything is ok.
(Continue reading)

Daniel Shahaf | 18 May 01:20 2015

fsfs7 structure-indexes - questions

Good morning Stefan,

A couple of questions on the fsfs7 indexes doc:

--- subversion/libsvn_fs_fs/structure-indexes
+++ subversion/libsvn_fs_fs/structure-indexes
 <at>  <at>  -18,6 +18,9  <at>  <at>  a simple concatenation of runtime structs and as such, an implementation
 detail subject to change.  A proto index basically aggregates all the
 information that must later be transformed into the final index.

+### "Subject to change?"  The format must be stable & documented since
+### "begin txn, modify txn, reboot, upgrade svn, commit txn" should work.

 General design concerns
 <at>  <at>  -346,7 +349,10  <at>  <at>  For performance reasons we use a modified version:
 * combine the big endian representation of these checksums plus the
   remnant of the original stream into a 12 to 15 byte long intermediate

   [i0 .. iK], 12 <= K+1 <= 15

+### "combine" is unclear.  Combine how?  Concatenate?  Interleave?  Add?  Multiply?
 * FNV checksum = fnv_1a([i0 .. iK]) in big endian representation

+### Why do we checksum the output of a checksum algorithm?  (Bytes i0..iK are themselves FNV output)

(Continue reading)

Daniel Shahaf | 18 May 01:16 2015

[PATCH] fsfs7 index offset validation


How about the following patch to sanity check the rev file footer?

fsfs7: Validate index offsets in rev file footer.

* subversion/libsvn_fs_fs/low_level.c
  (svn_fs_fs__parse_footer): Take FILESIZE argument, use it for validation.

* subversion/libsvn_fs_fs/low_level.h
  (svn_fs_fs__parse_footer): Take FILESIZE argument.

* subversion/libsvn_fs_fs/rev_file.c
  (svn_fs_fs__auto_read_footer): Pass FILESIZE.

Index: subversion/libsvn_fs_fs/low_level.c
--- subversion/libsvn_fs_fs/low_level.c	(revision 1679906)
+++ subversion/libsvn_fs_fs/low_level.c	(working copy)
 <at>  <at>  -196,9 +196,10  <at>  <at>  svn_fs_fs__parse_footer(apr_off_t *l2p_offset,
                         svn_checksum_t **p2l_checksum,
                         svn_stringbuf_t *footer,
                         svn_revnum_t rev,
+                        svn_filesize_t filesize,
                         apr_pool_t *result_pool)
-  apr_int64_t val;
(Continue reading)

Bruce Bye | 15 May 13:38 2015

Subversion configure script broken for serf if sysroot path contains "-D"


If you attempt to cross-compile subversion with a sysroot path containing "-D", blame.c will fail to compile, failing to find serf.h.

The root cause is this line in serf.m4:
          SVN_SERF_INCLUDES=[`$PKG_CONFIG $serf_major --cflags | $SED -e 's/-D[^ ]*//g'`]

I assume the intention is simply to strip any pre-compile defines from the cflags returned by pkg-config, but the regex is too aggressive.

This was observed in subversion 1.8.9, doing a Yocto build on Linux, but a code inspection of 1.8.13 suggests the issue is still there and it doesn't look especially platform sensitive.

Repro steps:
Set-up a Yocto environment in a path such as '/home/user/Dev-Demo'. Then run "bitbake subversion-native".

Ivan Zhakov | 13 May 19:04 2015

[VOTE] Merging 1.9-cache-improvements to /trunk

[adjusting subject to make it valid vote thread]

On 13 May 2015 at 19:23, Stefan Fuhrmann <stefan.fuhrmann <at> wandisco.com> wrote:
> Hi there,
> Ivan has reviewed my recent membuffer cache
> key handling changes, corrected and backported
> them on the 1.9-cache-improvements branch.
> I reviewed it and I'm +1 on merging it to /trunk -
> hoping we may even get it into 1.9. Since this
> touches a sensitive part of the server code, I'd
> like to see 2 more +1s for the branch->/trunk
> merge.

PS: I think detailed log message will be useful for reviewers. I'll
make it tomorrow if you didn't outstrip me.


Ivan Zhakov

Stefan Fuhrmann | 13 May 18:23 2015

Merging 1.9-cache-improvements to /trunk

Hi there,

Ivan has reviewed my recent membuffer cache
key handling changes, corrected and backported
them on the 1.9-cache-improvements branch.

I reviewed it and I'm +1 on merging it to /trunk -
hoping we may even get it into 1.9. Since this
touches a sensitive part of the server code, I'd
like to see 2 more +1s for the branch->/trunk

-- Stefan^2.
Stefan Fuhrmann | 13 May 08:21 2015

Recursive operations and authz

Hi devs,

At WANdisco, people have been playing with the new
wildcard support for authz (see authz-performance branch)
and ran into an interesting problem.

Today, recursive operations (COPY, DELETE and MOVE)
require recursive access rights on the respective trees.
For instance, COPY requires recursive read rights on the
source - which is fine b/c we don't want the copy to expose
previously protected data.

The problem with authz, however, is that we don't perform
a full tree crawl (and don't intend to) but check the authz
file for rules that *may* affect the respective sub-tree. A
rule like [:glob:/**/yeti] will *always* match whether there
are "yeti"s in your repo or not. Without wildcard support
the problem exists as well but is less prevalent because
tend to write explicit rules for paths that don't exist. For
wildcard rules, OTOH, it is almost the whole point to cover
existing and not-yet existing, future paths was well.

My suggestion is to relax the requirements as follows:
COPY & MOVE still require recursive read access on
the source but only non-recursive write access on the
destination. Thus it becomes possible to protect data
in branches and tags right from their inception without
relaxing read access requirements to existing data. The
attached patch achieves just that.

I have 3 questions:

(1) Is there something fundamentally wrong with this
   approach, e.g. braking major use-cases?
(2) Should we require write access to target and target's
   parent folder (as done by the patch) or just to the target's
   parent folder?
(3) Should we (optionally?) reduce the access rights reqs
   for DELETE similarly such that no recursive write access
   is needed? That seems risky but would be symmetrical
   to creating copies with r/o or n/a sub-paths. MOVE would
   be updated accordingly (always the same reqs as a COPY
   + DELETE).

-- Stefan^2.