Re: svn: E000005: Can't check path '/net/.svn': Input/output error
On 11/06/2013 08:05 PM, Ben Reser wrote: On 11/6/13 6:21 PM, Blair Zajac wrote: I'm wondering if its a regression with older versions of Subversion as it walks up the tree to find a .svn directory. Well the error is coming from io_check_path() in subversion/libsvn_subr/io.c. Specifically we run apr_stat() on the path and are expecting a valid status or something that is true when passed to APR_STATUS_IS_ENOENT() or SVN__APR_STATUS_IS_ENOTDIR(). apr_stat of course is just a thing wrapper around lstat() or stat(). In your case based on the error message it's returning 5 which according to /usr/include/sys/errno.h is EIO. And of course Subversion is saying there's an Input/output error consistent with the error code it received. Unfortunately the documentation for this is hardly clear (see man auto_master under Executable Map). It says that you should exit with a non zero exit code and no output if there is an error. Their example even shows it exiting with `exit 1` when the Open Directory query returned no results. Based on some simple experimentation I added the following to my /etc/auto_master file: /nxx/etc/auto.nxx Added a /etc/auto.nxx file containing [[[ #!/bin/sh exit 1 ]]] Made /etc/auto.nxx executable and ran `automount -vc` to activate it. Then I ran this: $ ls -l /nxx/x ls: /nxx/x: Input/output error Changing the exit code to 0 in /etc/auto.nxx I then repeated the ls command and got: $ ls -l /nxx/x ls: /nxx/x: No such file or directory So it seems to me that unless the commands to query LDAP fail that your script should exit with zero exit code and no output to indicate there is no such mount point. So like I said before it seems to me that this is a bug in your /etc/auto.spi Changing 'exit 1' to 'exit 0' and having the script send nothing to stdout gets it to work. Thanks for taking the time to track this down. Blair
svn: E000005: Can't check path '/net/.svn': Input/output error
Using a MacPorts build of svn 1.8.4 on a 10.7.5 MacBook Air: $ uname -a Darwin foo.example.com 11.4.2 Darwin Kernel Version 11.4.2: Thu Aug 23 16:25:48 PDT 2012; root:xnu-1699.32.7~1/RELEASE_X86_64 x86_64 i386 MacBookAir4,2 Darwin $ cd /net/yum/blair $ mkdir tmp $ svn checkout -N http://svn.apache.org/repos/asf/subversion/trunk svn: E05: Can't check path '/net/.svn': Input/output error This looks related to NFS, as the MacBook Air will mount our NFS volumes: $ cat /etc/auto_master # # Automounter master map # /net/etc/auto.spi /hosts /etc/auto.net /etc/auto.spi is a long shell script that does an LDAP lookup on the proper server to mount and mount options to use. It looks like /etc/auto.spi does an 'exit 1' when the LDAP lookup for '.svn' fails. $ /etc/auto.spi yum -intr,soft,nodev,vers=3,tcp,bg\ foobar:/foo/vol0/yum $ echo $? 0 $ /etc/auto.spi .svn $ echo $? 1 Any ideas on how to best resolve this? Blair
Re: svn: E000005: Can't check path '/net/.svn': Input/output error
On 11/06/2013 05:54 PM, Ben Reser wrote: On 11/6/13 5:15 PM, Blair Zajac wrote: Using a MacPorts build of svn 1.8.4 on a 10.7.5 MacBook Air: $ uname -a Darwin foo.example.com 11.4.2 Darwin Kernel Version 11.4.2: Thu Aug 23 16:25:48 PDT 2012; root:xnu-1699.32.7~1/RELEASE_X86_64 x86_64 i386 MacBookAir4,2 Darwin $ cd /net/yum/blair $ mkdir tmp $ svn checkout -N http://svn.apache.org/repos/asf/subversion/trunk svn: E05: Can't check path '/net/.svn': Input/output error This looks related to NFS, as the MacBook Air will mount our NFS volumes: $ cat /etc/auto_master # # Automounter master map # /net/etc/auto.spi /hosts/etc/auto.net /etc/auto.spi is a long shell script that does an LDAP lookup on the proper server to mount and mount options to use. It looks like /etc/auto.spi does an 'exit 1' when the LDAP lookup for '.svn' fails. $ /etc/auto.spi yum -intr,soft,nodev,vers=3,tcp,bg\ foobar:/foo/vol0/yum $ echo $? 0 $ /etc/auto.spi .svn $ echo $? 1 Any ideas on how to best resolve this? I think you should talk to whoever wrote auto.spi. For what it's worth I use working copies on NFS as well (also on a MacBook Air with 10.7.5), however I mount them with the automatic NFS mounts feature of the Disk Utility application. I haven't had any issues doing this. We have large numbers of different volumes with mount points coming and going and hundreds of MacBook Airs, so the LDAP approach lets us centralize this. It's not clear if you think there is a Subversion specific issue here but I don't see what it is if you do. I'm wondering if its a regression with older versions of Subversion as it walks up the tree to find a .svn directory. Blair
Re: svn commit: r1519275 - /subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
On 09/01/2013 09:20 AM, Branko Čibej wrote:
On 01.09.2013 16:40, danie...@apache.org wrote:
Author: danielsh
Date: Sun Sep 1 14:40:55 2013
New Revision: 1519275
URL: http://svn.apache.org/r1519275
Log:
Fix a compiler warning. No functional change.
* subversion/libsvn_subr/cache-membuffer.c
(ensure_data_insertable_l1): Satisfy the -Wparentheses nanny.
Perhaps we should just disable that particular warning flag...
Modified:
subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
Modified: subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/cache-membuffer.c?rev=1519275r1=1519274r2=1519275view=diff
==
--- subversion/trunk/subversion/libsvn_subr/cache-membuffer.c (original)
+++ subversion/trunk/subversion/libsvn_subr/cache-membuffer.c Sun Sep 1
14:40:55 2013
@@ -1360,10 +1360,12 @@ ensure_data_insertable_l1(svn_membuffer_
/* We might have touched the group that contains ENTRY. Recheck. */
if (entry_index == cache-l1.next)
-if (keep)
- promote_entry(cache, entry);
-else
- drop_entry(cache, entry);
+{
+ if (keep)
+promote_entry(cache, entry);
+ else
+drop_entry(cache, entry);
+}
}
}
This is exactly the kind of situation where the extra braces make the
code much clearer. I'd say the warning should stay.
+1.
Blair
otool -L and svn --version --verbose inconsistency
On my MacPorts svn 1.8.1 build on PPC otool -L reports only MacPorts' sqlite: $ otool -L /opt/local/bin/svn | grep sqlite /opt/local/lib/libsqlite3.0.dylib (compatibility version 9.0.0, current version 9.6.0) However, svn --version --verbose also reports the system's sqlite: $ /opt/local/bin/svn --version --verbose | grep sqlite - /opt/local/lib/libsqlite3.0.dylib (PowerPC) - /usr/lib/libsqlite3.0.dylib (PowerPC) Is this anything to be concerned of? Blair
Re: otool -L and svn --version --verbose inconsistency
On 08/16/2013 12:43 PM, Ben Reser wrote: On Fri Aug 16 12:39:10 2013, Ben Reser wrote: dyld is loading both for some reason. Even with the amalgamation I'm seeing: $ svn-trunk --version --verbose | grep -i sqlite - SQLite 3.7.12 (static) - /usr/lib/libsqlite3.dylib (Intel 64-bit) But I think I know why. If you're using the APR-Util that comes with OS X then it linked to sqlite: $ otool -L /usr/lib/libaprutil-1.0.dylib /usr/lib/libaprutil-1.0.dylib: /usr/lib/libaprutil-1.0.dylib (compatibility version 4.0.0, current version 4.10.0) /usr/lib/libexpat.1.dylib (compatibility version 7.0.0, current version 7.2.0) /usr/lib/libiconv.2.dylib (compatibility version 7.0.0, current version 7.0.0) /usr/lib/libpq.5.dylib (compatibility version 5.0.0, current version 5.3.0) /usr/lib/libsqlite3.dylib (compatibility version 9.0.0, current version 9.6.0) /System/Library/Frameworks/LDAP.framework/Versions/A/LDAP (compatibility version 1.0.0, current version 2.2.0) /usr/lib/libresolv.9.dylib (compatibility version 1.0.0, current version 46.0.0) /usr/lib/libapr-1.0.dylib (compatibility version 5.0.0, current version 5.2.0) /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 159.0.0) MacPorts uses nothing from the OS if it can avoid it. Here's the complete otool output. $ otool -L /opt/local/bin/svn /opt/local/bin/svn: /opt/local/lib/libsvn_client-1.0.dylib (compatibility version 1.0.0, current version 1.0.0) /opt/local/lib/libsvn_wc-1.0.dylib (compatibility version 1.0.0, current version 1.0.0) /opt/local/lib/libsvn_ra-1.0.dylib (compatibility version 1.0.0, current version 1.0.0) /opt/local/lib/libsvn_diff-1.0.dylib (compatibility version 1.0.0, current version 1.0.0) /opt/local/lib/libsvn_ra_local-1.0.dylib (compatibility version 1.0.0, current version 1.0.0) /opt/local/lib/libsvn_repos-1.0.dylib (compatibility version 1.0.0, current version 1.0.0) /opt/local/lib/libsvn_fs-1.0.dylib (compatibility version 1.0.0, current version 1.0.0) /opt/local/lib/libsvn_fs_fs-1.0.dylib (compatibility version 1.0.0, current version 1.0.0) /opt/local/lib/libsvn_fs_base-1.0.dylib (compatibility version 1.0.0, current version 1.0.0) /opt/local/lib/db46/libdb-4.6.dylib (compatibility version 0.0.0, current version 0.0.0) /opt/local/lib/libsvn_fs_util-1.0.dylib (compatibility version 1.0.0, current version 1.0.0) /opt/local/lib/libsvn_ra_svn-1.0.dylib (compatibility version 1.0.0, current version 1.0.0) /opt/local/lib/libsasl2.2.dylib (compatibility version 3.0.0, current version 3.25.0) /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 111.1.7) /usr/lib/libresolv.9.dylib (compatibility version 1.0.0, current version 25.0.2) /usr/lib/libpam.1.dylib (compatibility version 1.0.0, current version 1.0.0) /opt/local/lib/libsvn_ra_serf-1.0.dylib (compatibility version 1.0.0, current version 1.0.0) /opt/local/lib/libserf-1.dylib (compatibility version 4.0.0, current version 4.0.0) /opt/local/lib/libsvn_delta-1.0.dylib (compatibility version 1.0.0, current version 1.0.0) /opt/local/lib/libsvn_subr-1.0.dylib (compatibility version 1.0.0, current version 1.0.0) /opt/local/lib/libexpat.1.dylib (compatibility version 8.0.0, current version 8.0.0) /opt/local/lib/libsqlite3.0.dylib (compatibility version 9.0.0, current version 9.6.0) /opt/local/lib/libmagic.1.dylib (compatibility version 2.0.0, current version 2.0.0) /opt/local/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.8) /opt/local/lib/libaprutil-1.0.dylib (compatibility version 6.0.0, current version 6.2.0) /opt/local/lib/libapr-1.0.dylib (compatibility version 5.0.0, current version 5.8.0) /opt/local/lib/libintl.8.dylib (compatibility version 10.0.0, current version 10.2.0) /opt/local/lib/libiconv.2.dylib (compatibility version 8.0.0, current version 8.1.0) /System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices (compatibility version 1.0.0, current version 32.0.0) /System/Library/Frameworks/Security.framework/Versions/A/Security (compatibility version 1.0.0, current version 26935.0.0) /usr/lib/libgcc_s.1.dylib (compatibility version 1.0.0, current version 1.0.0)
Re: otool -L and svn --version --verbose inconsistency
On 08/16/2013 02:19 PM, Ben Reser wrote: On Fri Aug 16 13:08:59 2013, Blair Zajac wrote: MacPorts uses nothing from the OS if it can avoid it. Here's the Check to see if the apr-util you're using isn't linked against the system sqlite. Could also be some other dependency but as far as I recall apr-util is the only dependency that might be linked against sqlite. It's not: $ otool -L /opt/local/lib/libaprutil-1.dylib /opt/local/lib/libaprutil-1.dylib: /opt/local/lib/libaprutil-1.0.dylib (compatibility version 6.0.0, current version 6.2.0) /opt/local/lib/libexpat.1.dylib (compatibility version 8.0.0, current version 8.0.0) /opt/local/lib/libiconv.2.dylib (compatibility version 8.0.0, current version 8.1.0) /opt/local/lib/libapr-1.0.dylib (compatibility version 5.0.0, current version 5.8.0) /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 111.1.7) /usr/lib/libgcc_s.1.dylib (compatibility version 1.0.0, current version 1.0.0) It's dynamically loaded module isn't: $ otool -L /opt/local/lib/apr-util-1/apr_dbd_sqlite3.so /opt/local/lib/apr-util-1/apr_dbd_sqlite3.so: /opt/local/lib/libsqlite3.0.dylib (compatibility version 9.0.0, current version 9.6.0) /usr/lib/libgcc_s.1.dylib (compatibility version 1.0.0, current version 1.0.0) /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 111.1.7)
Re: Semantics of Move
On 08/15/2013 08:01 AM, Julian Foad wrote: I propose the following logical semantics of the versioned move operation that is the basis of move tracking, independent of any implementation. A versioned move of the node with node id “N”, with respect to two revisions rX and rY (X Y), shall mean: * Same node id. A node with node id N exists in rX and in rY. It is “the same node”. It therefore has the same node kind. It may have content modifications. If the contents change then it should get a new node-id. I have a vested stake in this since our asset management system backed by svn caches in memcached the result of many svn_fs_*() functions by node-id. Our system does three cache lookups: time - revision (revision, path) - node_id (node_id, function_name) - result Doing this allows me to take a lot of load off the svn backend RPC server. Blair
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c util.c
On 06/23/2013 07:47 PM, Greg Stein wrote: On Sun, Jun 23, 2013 at 3:56 PM, Blair Zajac bl...@orcaware.com wrote: On 6/22/13 1:22 AM, Lieven Govaerts wrote: Even better if we could cache this info somewhere automatically. So that the first time svn connects to a server ever it will use this 'slow' procedure, and then persists the results http/1.0 vs http/1.1 and chunked-encoding supported somewhere. Like we already do for passwords and ssl certificate validation. Will this work if you change where you are connecting from, e.g. at home one day and then from a Starbucks or maybe your work where you have different proxies in the path? Well... I don't think that we're talking (yet) about a cache. Just a config and an extra detection step. In your scenario, you'll suffer a slightly slower start cost (the extra OPTIONS), and in the other, a somewhat degraded experience (switching to C-L for all requests). But it should auto-switch as you move about. (assuming we don't try to cache, at this point) Sounds good. Sounded like there was an idea of caching, which seemed like it could be problematic. Regards, Blair
Re: svn commit: r1495419 - in /subversion/trunk/subversion/libsvn_ra_serf: options.c ra_serf.h serf.c util.c
On 6/22/13 1:22 AM, Lieven Govaerts wrote: Even better if we could cache this info somewhere automatically. So that the first time svn connects to a server ever it will use this 'slow' procedure, and then persists the results http/1.0 vs http/1.1 and chunked-encoding supported somewhere. Like we already do for passwords and ssl certificate validation. Will this work if you change where you are connecting from, e.g. at home one day and then from a Starbucks or maybe your work where you have different proxies in the path? Blair
Re: svn commit: r1495204 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c
On 06/20/2013 02:40 PM, stef...@apache.org wrote: Author: stefan2 Date: Thu Jun 20 21:40:50 2013 New Revision: 1495204 URL: http://svn.apache.org/r1495204 Log: Follow-up to r1495063: integer overflows result in an inefficient hash function and reduce cache effectiveness. * subversion/libsvn_fs_fs/tree.c (cache_lookup): prevent unnecessary integer overflows in hash function Hi Stefan, I've been reading up on hash functions and am curious about this how overflow can reduce cache effectiveness. Do you have any links or can offer an explanation? Thanks, Blair
Re: [JavaHL] Reported incompatibility
On 06/12/2013 07:07 AM, Branko Čibej wrote: On 12.06.2013 14:42, Vladimir Berezniker wrote: Hi All, I came across a bug in one of the Atlassian tools that suggests that there was an API compatibility break in the JavaHL between versions 1.6 and 1.7. I am not sure when/if I get time to look more into it, therefore I am sharing with the group meanwhile. The following comment is from the issue: https://jira.atlassian.com/browse/FE-4679 I would argue that this counts as a bug fix, since property values are in general not strings. Even if its a bug fix, the normal way to fix it would be to add a new ProplistCallback2 interface with the proper signature, but since we have org.apache.subversion.javahl.callback.ProplistCallback, there isn't a point to do that, but to keep the original signature. However, given this is released code and 1.7 has been out there a while and the new signature is correct and people may have compiled against that, it seems we're in a tough spot. Blair
Re: AW: C++ thoughts for Berlin
On 06/02/2013 10:53 PM, Markus Schaber wrote: Hi, Blair, Von: Blair Zajac [mailto:bl...@orcaware.com] [Discussion whether to use (part of) C++ for SVN development] I agree it's not worth going to C++. Where I'm coming from is a frustration on the number of times I've seen pool lifetime bugs get fixed and it would be great to be in a language where one doesn't need to worry about that, or as much. I fully understand your frustration. :-) C can be frustrating. But believe me, as I used to program C++ for my day job for almost five years, it is not the solution. Frustration just shifts from some problem areas to others, and memory management is not really better. I'm not sure where all the negativity for C++ memory comes from. A colleague and myself at work wrote a multithreaded-safe C++ library that provides an asset API for visual effects (textures, plates, rigs, etc) to all the apps at work; it's either explicitly linked to or pulled in via a plugin everywhere, including third party apps (Maya, Nuke, Katana, etc). Under the hood it uses a multithreaded C++ RPC library to talk to our middle-tier servers to get the assets. The middle-tier talks to a C++ server that uses svn_fs and svn_repos. The client library handles the parent process calling fork() and properly handling locked mutexes in the parent process that cannot be unlocked in the child process. So I feel that its a pretty complicated library, especially when the parent process can do anything it wants. The library uses shared pointers mostly everywhere, but this made it easy for us to code and easy for our users because we hand them a shared pointer, so there's no thought required on who owns the memory. Maybe this design came from too much Java/Scala or Python coding ;) The only place we had memory issues was in our C++ server using the svn_fs and svn_repos API, where in a C++ class, if we listed a data member that used pool memory before the AprPool data member that provided that memory, then at destruction time we could get core dump. The fix was to reorder the AprPool data member before all other data members. So I don't get where the dislike comes from. Maybe it's not cool to use shared pointers everywhere, but it was easier and got the job done and our users are happy. Blair
Re: C++ thoughts for Berlin
On 5/29/13 6:17 PM, Justin Erenkrantz wrote: On Wed, May 29, 2013 at 6:03 PM, Blair Zajac bl...@orcaware.com mailto:bl...@orcaware.com wrote: Yup, I've had lots of issues with this. Putting C++ pool wrappers in C++ classes and having them destroy in the correct order can be tricky to get right (lots of core dumps in our internal RPC server). One of the nice things about moving to C++, assuming we don't use pools, is that one could stop thinking about memory management. I don't see how you can have a clean C++ implementation as we expose pools in the C API. I just think we're going to bring a world of hurt upon us all if we try to do a wholesale rewrite of C++ and try to shove the pool model into it. (The batons and callbacks all require pools with a certain lifetime - so there are third-party code that is relying upon the pool hierarchy being correct.) If we really wanted to go that route, the far saner (if you can call it that) approach is to just call it Subversion 2.0 and have a pool-free external API...from where I sit, I don't see much value going that far - how many years do we want to dedicate to 2.0? Are things ultimately so broken that we simply can't make Subversion better unless we go to C++? It's not a zero-cost item. I agree it's not worth going to C++. Where I'm coming from is a frustration on the number of times I've seen pool lifetime bugs get fixed and it would be great to be in a language where one doesn't need to worry about that, or as much. Blair
C++ thoughts for Berlin
Given I won't be at the Berlin hackathon, here's my thought on the C++ topic [1]. I'm generally in favor of a move to C++, it would be nice to get features that we work around now in C. Questions/issues: 1) How old g++ do we maintain? We have RHEL 6 boxes that have 4.4.6, so we wouldn't be able to use nice C++ 11 features. 2) Which libraries can we leverage? I use Boost and have no issues pulling that in, but I except some people do, given what I hear about Boost (although I'm a fan). RHEL 6 comes with Boost 1.41.0. 3) Do we target an older distro then RHEL 6, maybe RHEL 5? 4) We use the Google C++ style guide [2] at work for all our C++ coding, dropping the ban on exceptions. Would we use that? Maybe we want to keep our code exception free, if we can? I guess if we go Boost, exceptions come included? I haven't checked if you can? If we use the distro's Boost, does that imply we cannot disable exceptions? Blair [1] http://wiki.apache.org/subversion/Berlin2013 [2] http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml
Re: C++ thoughts for Berlin
On 05/29/2013 05:48 PM, Justin Erenkrantz wrote: On Wed, May 29, 2013 at 12:05 PM, Blair Zajac bl...@orcaware.com mailto:bl...@orcaware.com wrote: I'm generally in favor of a move to C++, it would be nice to get features that we work around now in C. Rewriting even some of our core libraries to use C++ (even if it we kept the existing C API) just doesn't seem to address any real problems that we have. We'd likely be having to write off support for a lot of platforms due to the inconsistent nature of many C++ compilers on platforms we have supported since 1.0. I do not think this is a good thing. With regards to libraries, I have had nothing but horrible developer experiences with Boost - it's pretty counter-intuitive in a lot of places; and C++11 isn't anywhere near widely supported to be considered if we want to keep broad platform support. Yeah, I've only had good experiences with Boost, but I mainly use scoped and shared pointers and some of the filesystem APIs. As trying to use APR in a C++-based memory management model is fraught with paradigm conflicts, we'd quite likely need to write a new portability layer and new HTTP networking layer. Fun! (Not.) Yup, I've had lots of issues with this. Putting C++ pool wrappers in C++ classes and having them destroy in the correct order can be tricky to get right (lots of core dumps in our internal RPC server). One of the nice things about moving to C++, assuming we don't use pools, is that one could stop thinking about memory management. BTW, I believe that GCC is special - due to its bootstrapping methodologies, it's only really meant to be compiled by itself - this doesn't apply to Subversion, so I think that analogy is a bit of red herring. If we really switched to having core libraries written in C++, I would forcefully argue that it has to be SVN 2 (regardless if we kept the C API identical)...and I'd probably say we should just rename the project to something else - it's not Subversion at that point, but something else. -- justin Well, I wouldn't go that far ;) For most people, if it talks to the repo and wc, it's still Subversion. Blair
Re: svn commit: r1485499 - /subversion/trunk/subversion/libsvn_ra_svn/marshal.c
On 05/22/2013 04:22 PM, stef...@apache.org wrote:
Author: stefan2
Date: Wed May 22 23:22:22 2013
New Revision: 1485499
URL: http://svn.apache.org/r1485499
Log:
Reduce ra_svn protocol parsing overhead.
* subversion/libsvn_ra_svn/marshal.c
(vparse_tuple): test for the most frequent item types first
Modified:
subversion/trunk/subversion/libsvn_ra_svn/marshal.c
Modified: subversion/trunk/subversion/libsvn_ra_svn/marshal.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/marshal.c?rev=1485499r1=1485498r2=1485499view=diff
==
--- subversion/trunk/subversion/libsvn_ra_svn/marshal.c (original)
+++ subversion/trunk/subversion/libsvn_ra_svn/marshal.c Wed May 22 23:22:22 2013
@@ -1202,14 +1202,15 @@ static svn_error_t *vparse_tuple(const a
if (**fmt == '?')
(*fmt)++;
elt = APR_ARRAY_IDX(items, count, svn_ra_svn_item_t);
- if (**fmt == 'n' elt-kind == SVN_RA_SVN_NUMBER)
-*va_arg(*ap, apr_uint64_t *) = elt-u.number;
- else if (**fmt == 'r' elt-kind == SVN_RA_SVN_NUMBER)
-*va_arg(*ap, svn_revnum_t *) = (svn_revnum_t) elt-u.number;
- else if (**fmt == 's' elt-kind == SVN_RA_SVN_STRING)
-*va_arg(*ap, svn_string_t **) = elt-u.string;
+ if (**fmt == '(' elt-kind == SVN_RA_SVN_LIST)
+{
+ (*fmt)++;
+ SVN_ERR(vparse_tuple(elt-u.list, pool, fmt, ap));
+}
Out of curiosity, how much cost is there in the double dereferencing of
fmt? Could you save it in a local variable and do the if/else on that
instead? Also, would a switch block be more efficient?
Blair
Re: svn commit: r1469520 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c
On 4/18/13 11:53 AM, Philip Martin wrote:
bl...@apache.org writes:
Author: blair
Date: Thu Apr 18 18:41:55 2013
New Revision: 1469520
URL: http://svn.apache.org/r1469520
Log:
open_path(): silence compiler warning.
subversion/libsvn_fs_fs/tree.c: In function 'open_path':
subversion/libsvn_fs_fs/tree.c:916: warning: 'directory' may be used
uninitialized in this function
* subversion/libsvn_fs_fs/tree.c
(open_path):
Set directory to NULL to silence a compiler warning.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/tree.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1469520r1=1469519r2=1469520view=diff
==
--- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Thu Apr 18 18:41:55 2013
@@ -913,7 +913,7 @@ open_path(parent_path_t **parent_path_p,
a sibling of PATH has been presently accessed. Try to start the lookup
directly at the parent node, if the caller did not requested the full
parent chain. */
- const char *directory;
+ const char *directory = NULL;
assert(svn_fs__is_canonical_abspath(path));
if (flags open_path_node_only)
{
In the past we have not been adding these sorts of initialisations. A
build without warnings is nice but unnecessary initialisation can reduce
the effectiveness of memory checking tools. In this particular case the
code is:
I just saw this now when looking through STATUS.
I would say that we have been keeping our build warning free though, if it takes
a initialization.
if (here)
{
path_so_far = directory;
rest = path + strlen(directory) + 1;
}
Passing NULL to strlen is not guaranteed to work but will work on some
platforms. Without the initialisation a memory checking tool like
valgrind will trigger if strlen were to access directory. The
initialisation means there is no longer a guarantee that valgrind will
trigger.
I would say though that I would prefer keeping the runtime in better shape, say
reading from NULL and getting a core dumo, than random memory location.
I did assume that the code was good though and just did this to silence a
warning.
Blair
Re: svn commit: r1469520 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c
On 4/19/13 8:04 AM, Philip Martin wrote: Daniel Shahaf danie...@elego.de writes: What would a better fix be? It's hard to say which is probably why these warnings have not been addressed so far. The warning is a 'maybe': ../src/subversion/libsvn_fs_fs/tree.c:929:27: warning: 'directory' may be used uninitialized in this function [-Wmaybe-uninitialized] so what will make it go away probably depends on various compiler details. A better compiler optimiser might make the warning go away. Yes, this is a pretty old gcc 4.4 on RedHat 6.3 The following is a first sketch, it should resolve the compiler warning but won't solve the actual semantic bug (if there is one) here. I believe it's a false positive and there is no bug, other than the warning, as 'directory' has to be non-NULL if 'here' is non-NULL. Agreed.
Nominating your r1481848 and r1481870
Hi Brane, Just saw the veto on my compiler warning commits for 1.8.x. I nominated your two commits on top of my commits (to avoid merge conflicts), which should resolve the veto, but I left it in the vetoed section to be safe. Pinging you here so you can take a look. Blair
Re: Issue 4359 fix not merged to 1.8.x
I think adding a check like that would be good. I just want to know what did everybody review ;) Blair On Apr 24, 2013, at 6:06 AM, Daniel Shahaf danie...@elego.de wrote: Should backport.pl attempt to catch such a situation? I'm inclined to say No, since the entry should not have been moved below the Approved header and given three +1 votes if it had a revnum typo. (But it should be easy to inject an 'svn log -qvr | wc -l 4' type check, if people prefer that.) Philip Martin wrote on Wed, Apr 24, 2013 at 09:02:14 +0100: Blair Zajac bl...@orcaware.com writes: It looks like the wrong revision number was given in STATUS, as it's a tomcat commit: http://svn.apache.org/viewvc?view=revisionrevision=r1471029 r1471028 is the revision that should have been proposed/voted/merged. -- Certified Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
Issue 4359 fix not merged to 1.8.x (was svn commit: r1471243 - in /subversion/branches/1.8.x: ./ STATUS)
It looks like the wrong revision number was given in STATUS, as it's a tomcat commit: http://svn.apache.org/viewvc?view=revisionrevision=r1471029 Blair ---BeginMessage--- Author: svn-role Date: Wed Apr 24 04:01:30 2013 New Revision: 1471243 URL: http://svn.apache.org/r1471243 Log: Merge r1471029 from trunk: * r1471029 Fix issue 4359, incorrect/missing pack notification. Justification: Regression from 1.6. Votes: +1: philip, rhuijben, cmpilato Modified: subversion/branches/1.8.x/ (props changed) subversion/branches/1.8.x/STATUS Propchange: subversion/branches/1.8.x/ -- Merged /subversion/trunk:r1471029 Modified: subversion/branches/1.8.x/STATUS URL: http://svn.apache.org/viewvc/subversion/branches/1.8.x/STATUS?rev=1471243r1=1471242r2=1471243view=diff == --- subversion/branches/1.8.x/STATUS (original) +++ subversion/branches/1.8.x/STATUS Wed Apr 24 04:01:30 2013 @@ -118,10 +118,3 @@ Veto-blocked changes: Approved changes: = - - * r1471029 - Fix issue 4359, incorrect/missing pack notification. - Justification: - Regression from 1.6. - Votes: - +1: philip, rhuijben, cmpilato ---End Message---
Caching svn_fs_t and friends
Hi guys, Saw my name mentioned in IRC regarding caching svn_*_t. In our custom svn RPC server that exposes svn_repos_t and svn_fs_t, we cache svn_repos_t, svn_fs_root_t (for both revisions and txns). I pulled some stats from our live server, here is the svn_repos_t cache: Max=238 N=238 InUse=0 Lookups=209959446 CacheHits=209958192 CacheMisses=1254 CacheHitRatio=0.94 Evictions=1016 ConstructionAge( N=238 min=04:22:19.316771 25%=1d07:05:44.297955 50%=1d22:38:44.728577 90%=6d05:40:09.023494 95%=6d05:47:27.417303 99%=7d05:34:32.500303 max=7d06:48:08.218891) LastUseAge( N=238 min=0.000316 25%=38:54.439219 50%=38:54.843007 90%=1d22:08:15.423188 95%=1d22:09:46.925353 99%=1d22:09:46.925165 max=1d22:10:05.678702) NumberServantsPerId( N=119 min=1 25%=1 50%=1 90%=2 95%=3 99%=10 max=76) So our oldest svn_repos_t is 7 days and 6 hours old, which is basically as old as the process, so we can cache them indefinitely. Actually, I size the cache to 2*number repos. Blair
Re: 1.8 new public API review (mostly) complete.
On 4/1/13 10:25 AM, C. Michael Pilato wrote: On 03/31/2013 12:31 AM, Blair Zajac wrote: Question on svn_repos_load_fs4() and svn_repos_get_fs_build_parser4() (maybe other functions have a similar text): * @note If @a start_rev and @a end_rev are valid revisions, this * function presumes the revisions as numbered in @a dumpstream only * increase from the beginning of the stream to the end. Gaps in the * number sequence are ignored, but upon finding a revision number * younger than the specified range, this function may stop loading * new revisions regardless of their number. What does 'may stop' mean? Does it flips a coin ;) Seriously, will it or will it not stop, or under which conditions. It won't stop. I've removed this @note from both APIs in r1463213. Great, thanks! Blair
Re: 1.8 new public API review (mostly) complete.
On 03/29/2013 01:17 PM, C. Michael Pilato wrote: Devs, I've just completed my review of the new-in-1.8 public APIs, minus the bits that Philip reviewed (thanks!) and the new merge-related stuff which, if I understand from recent threads correctly, is still subject to some churn. The results of my review revealed overwhelmingly positive results which, in my approximation, are non-contentious. I had (for some definition of had) to touch up quite a few docstring in the process, but by and large those were stylistic nits with the occasionally overlooked item. Question on svn_repos_load_fs4() and svn_repos_get_fs_build_parser4() (maybe other functions have a similar text): * @note If @a start_rev and @a end_rev are valid revisions, this * function presumes the revisions as numbered in @a dumpstream only * increase from the beginning of the stream to the end. Gaps in the * number sequence are ignored, but upon finding a revision number * younger than the specified range, this function may stop loading * new revisions regardless of their number. What does 'may stop' mean? Does it flips a coin ;) Seriously, will it or will it not stop, or under which conditions. Blair
Re: svn commit: r1442071 - in /subversion/trunk/subversion/libsvn_fs_fs: dag.c temp_serializer.c
On 02/04/2013 02:38 AM, stef...@apache.org wrote: Author: stefan2 Date: Mon Feb 4 10:38:45 2013 New Revision: 1442071 URL: http://svn.apache.org/viewvc?rev=1442071view=rev Log: Speed up serialization of DAG and noderev structures for our caches. Turns out that 503 bytes is often not sufficient for noderevs with longer path names and even less so for DAG nodes. Up that to 1007 bytes (+1 0-terminator +16 bytes memory management overhead = 1kB). What happens with paths that are longer than 1007 bytes? If two paths differ, say at the suffix, past that length, then will they have the same cache key? Blair
Re: packages tree
On 01/29/2013 10:04 AM, Ben Reser wrote: Should we remove the packages tree? The packages are almost certainly terribly out of date. There's been 1 functional commit (not including things like URL and license changes from the ASF changeover) since 2009. To give an example the RedHat rpm packages don't have a rhel-6 package, the documentation points for rhel-5 points you to a URL that is dead and the dev list on tigris. Back in the day it made a lot of sense to try and help provide some packaging support. But at this point we are well packaged by a variety of 3rd parties. So I'm not sure we're really gaining anything by having these around anymore. If we're not going to keep these things up to date then I think we should just remove them. +1. Blair
Re: svn commit: r1405517 - in /subversion/trunk/subversion/bindings/javahl/native: CopySources.cpp CreateJ.cpp SVNClient.cpp StringArray.cpp Targets.cpp
On Nov 4, 2012, at 2:14 AM, stef...@apache.org wrote: Author: stefan2 Date: Sun Nov 4 10:14:56 2012 New Revision: 1405517 URL: http://svn.apache.org/viewvc?rev=1405517view=rev Log: Silence integer size conversion warnings in JavaHL under Win64 by casting the values explicitly. We assume that argument counts and property sizes are all well below the 2G limit. Hi Stefan, Since you're in C++ here you could switch to using static_cast or reinterpret_cast instead, that would be better for self documentation, although that doesn't seem to be the style in the code. Blair
Re: svn commit: r1405539 - /subversion/trunk/subversion/bindings/javahl/native/
On 11/04/2012 04:48 AM, stef...@apache.org wrote: Author: stefan2 Date: Sun Nov 4 12:47:59 2012 New Revision: 1405539 URL: http://svn.apache.org/viewvc?rev=1405539view=rev Log: Make all explicit casts, i.e. those not inside some APR macro, use the C++ cast operators static_cast, const_cast and reinterpret_cast. Cool, thanks! Blair
Re: svn commit: r1403982 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
On 10/30/12 9:59 PM, bre...@apache.org wrote: Author: breser Date: Wed Oct 31 04:59:42 2012 New Revision: 1403982 URL: http://svn.apache.org/viewvc?rev=1403982view=rev Log: Fix a compile warning and a memory leak in rep_write_cleanup. * subversion/libsvn_fs_fs/fs_fs.c (rep_write_cleanup): txn_id shouldn't be a const and need to clear the err since we don't return it. Looking at the diff, do you mean 'should be a const'? Blair
Re: svn commit: r1391641 - /subversion/trunk/subversion/libsvn_subr/cache-memcache.c
On 09/28/2012 01:34 PM, cmpil...@apache.org wrote: Author: cmpilato Date: Fri Sep 28 20:34:50 2012 New Revision: 1391641 URL: http://svn.apache.org/viewvc?rev=1391641view=rev Log: * subversion/libsvn_subr/cache-memcache.c (add_memcache_server): Convert time-to-live value from seconds to the expected microseconds. == --- subversion/trunk/subversion/libsvn_subr/cache-memcache.c (original) +++ subversion/trunk/subversion/libsvn_subr/cache-memcache.c Fri Sep 28 20:34:50 2012 @@ -470,7 +470,7 @@ add_memcache_server(const char *name, 0, /* min connections */ 5, /* soft max connections */ 10, /* hard max connections */ - 50, /* connection time to live (secs) */ + apr_time_from_sec(50), /* ttl (ms) */ Hi Mike, ms is for milliseconds and µs for microseconds, I suggest changing 'ms' to 'microseconds', as 'us' would be confusing. Blair
Re: svn commit: r1388816 - in /subversion/branches/10Gb/subversion: include/private/svn_pseudo_md5.h libsvn_subr/cache-membuffer.c libsvn_subr/pseudo_md5.c tests/libsvn_subr/checksum-test.c
On 09/22/2012 08:14 AM, stef...@apache.org wrote: Author: stefan2 Date: Sat Sep 22 15:14:25 2012 New Revision: 1388816 URL: http://svn.apache.org/viewvc?rev=1388816view=rev Log: On the 10Gb branch: Introduce MD5-based hash functions optimized for short input lengths. Use these to speed up membuffer access. How much faster is it than a plain MD5? If we only need it for hashing, did you look at using a more well known hashing function, e.g. FNV [1] or murmur [2]? Also, can you include URLs where you downloaded the code from in the log message and code. Blair [1] http://isthe.com/chongo/tech/comp/fnv/ [2] https://sites.google.com/site/murmurhash/
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test
On 08/21/2012 10:29 AM, cmpil...@apache.org wrote: Author: cmpilato Date: Tue Aug 21 17:29:40 2012 New Revision: 1375675 URL: http://svn.apache.org/viewvc?rev=1375675view=rev Log: Introduce a new 'init-commit' hook script which runs immediately after the commit txn is created and populated with initial txnprops. I'm curious where this is used. This is just after the txn is begun but before any modifications are made in it? Calling this init isn't self documenting, it's not clear where in the commit steps it occurs. It doesn't fit in the pre- or post- convention where it's clear where it runs in the order. Calling this post-create, post-txn-create would be a clearer name. Given this hook is infrequently used, I would prefer the later. Blair
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test
On 08/21/2012 11:09 AM, C. Michael Pilato wrote: On 08/21/2012 01:54 PM, Blair Zajac wrote: On 08/21/2012 10:29 AM, cmpil...@apache.org wrote: Author: cmpilato Date: Tue Aug 21 17:29:40 2012 New Revision: 1375675 URL: http://svn.apache.org/viewvc?rev=1375675view=rev Log: Introduce a new 'init-commit' hook script which runs immediately after the commit txn is created and populated with initial txnprops. I'm curious where this is used. This is just after the txn is begun but before any modifications are made in it? That's correct. Well, not before *any* modifications are made. The libsvn_repos-level code which creates commit transactions also sets a bunch of txnprops (author, date, plus any user-supplied ones such as log messages or custom revprops) on the newly created transaction. This hook runs immediately after that initial batch of txnprop changes is made. Is the use case it to check the txn props that aren't available in start-commit? Calling this init isn't self documenting, it's not clear where in the commit steps it occurs. It doesn't fit in the pre- or post- convention where it's clear where it runs in the order. I agree that the name isn't ideal. Calling this post-create, post-txn-create would be a clearer name. Given this hook is infrequently used, I would prefer the later. I actually considered using post-create-txn and renaming start-commit to pre-create-txn (with code to run start-commit iff not pre-create-txn hook exists, for compat purposes). +1. I always have to remember which comes first, start-commit or pre-commit, so this renaming helps. Blair Any thoughts on that?
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test
On 08/21/2012 11:45 AM, Philip Martin wrote: Blair Zajac bl...@orcaware.com writes: On 08/21/2012 11:09 AM, C. Michael Pilato wrote: I actually considered using post-create-txn and renaming start-commit to pre-create-txn (with code to run start-commit iff not pre-create-txn hook exists, for compat purposes). +1. I always have to remember which comes first, start-commit or pre-commit, so this renaming helps. Suppose both pre-create-txn and start-commit exist. Is it an error? If not which one is run? I don't think it's an error, run both of them. I don't have any thoughts on the ordering, perhaps start-commit first, since it's always been there? We have already bumped the FSFS format in 1.8 but we have not yet bumped the repos format. Perhaps we could bump and have an upgrade that renames the hook? That would be make repos maintenance easier in the long run. Blair
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test
On 08/21/2012 11:56 AM, C. Michael Pilato wrote: On 08/21/2012 02:45 PM, Philip Martin wrote: Blair Zajac bl...@orcaware.com writes: On 08/21/2012 11:09 AM, C. Michael Pilato wrote: I actually considered using post-create-txn and renaming start-commit to pre-create-txn (with code to run start-commit iff not pre-create-txn hook exists, for compat purposes). +1. I always have to remember which comes first, start-commit or pre-commit, so this renaming helps. Suppose both pre-create-txn and start-commit exist. Is it an error? If not which one is run? We have already bumped the FSFS format in 1.8 but we have not yet bumped the repos format. Perhaps we could bump and have an upgrade that renames the hook? Are we comfortable with renaming the hook, which -- strictly speaking -- is user-managed data, not Subversion managed data? What if the hook itself is kept under version control (which is pretty common)? I lean against doing so. And because no change of administrative behavior is required (we'll still happily run start-commit if there's no pre-txn-create), I see no need for the format bump. True, good point. Moving anything there isn't save. In my setup, we have symlinks to a common area, which wouldn't break with a rename, but I would be surprised. As to whether to flag an error if both start-commit and pre-txn-create exist: this makes sense. I see value in warning *someone* that the repository configuration is non-ideal, similarly to the error we return from a missing pre-revprop-change hook script (ask the administrator to [fix this problem]). A missing pre-revprop-change script will warn to users doing a propedit, which is infrequent. But for a non-ideal hooks, do we warn all committers? That would be annoying, but probably result in a prompt fix by the admin ;) Blair
Re: Please move mod_dontdothat back from tools/server-side/mod_dontdothat to subversion/mod_dontdothat
On 08/19/2012 11:02 AM, Branko Čibej wrote: On 18.08.2012 16:53, Nico Kadel-Garcia wrote: mod_dontdothat is not a core module. It's effectively a debugging tool. Whereas mod_dav_svn and mod_authz_svn are part of the core Subversion functionality. The different locations aren't accidental. The main problem with moving mod_dontdothat was that the way it was done broke Unix builds without Apache HTTPd. That's fixed on trunk now, and could probably be backported to 1.7.x. There's some downstream maintainers I know that would appreciate that ;) Which revisions need backporting so I can review and vote on them? Blair
Re: svn commit: r1372665 - /subversion/branches/master-passphrase/subversion/libsvn_auth_kwallet/kwallet.cpp
On 08/13/2012 04:24 PM, phi...@apache.org wrote:
Author: philip
Date: Mon Aug 13 23:24:06 2012
New Revision: 1372665
URL: http://svn.apache.org/viewvc?rev=1372665view=rev
Log:
Fix C++ compile errors.
* subversion/libsvn_auth_kwallet/kwallet.cpp
(): Include svn_base64.h.
(kwallet_master_passphrase_first_creds,
kwallet_master_passphrase_save_creds): Cast from void *.
Modified:
subversion/branches/master-passphrase/subversion/libsvn_auth_kwallet/kwallet.cpp
Modified:
subversion/branches/master-passphrase/subversion/libsvn_auth_kwallet/kwallet.cpp
URL:
http://svn.apache.org/viewvc/subversion/branches/master-passphrase/subversion/libsvn_auth_kwallet/kwallet.cpp?rev=1372665r1=1372664r2=1372665view=diff
==
---
subversion/branches/master-passphrase/subversion/libsvn_auth_kwallet/kwallet.cpp
(original)
+++
subversion/branches/master-passphrase/subversion/libsvn_auth_kwallet/kwallet.cpp
Mon Aug 13 23:24:06 2012
@@ -40,6 +40,7 @@
#include svn_pools.h
#include svn_string.h
#include svn_version.h
+#include svn_base64.h
#include private/svn_auth_private.h
@@ -483,7 +484,8 @@ kwallet_master_passphrase_first_creds(vo
if (done passphrase)
{
svn_auth_cred_master_passphrase_t *creds;
- creds = apr_pcalloc(pool, sizeof(*creds));
+ creds = (svn_auth_cred_master_passphrase_t *)apr_pcalloc(pool,
+ sizeof(*creds));
Hi Philip,
I think it's better to use C++ style casts in a C++ body than C style
cases. I'll reference the Google C++ style guide, even though we
haven't said we use it:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Casting
Blair
Re: svn commit: r1366209 - in /subversion/trunk/subversion: libsvn_wc/externals.c tests/libsvn_client/client-test.c
On 07/26/2012 03:04 PM, rhuij...@apache.org wrote: Author: rhuijben Date: Thu Jul 26 22:04:03 2012 New Revision: 1366209 URL: http://svn.apache.org/viewvc?rev=1366209view=rev Log: * subversion/libsvn_wc/externals.c (svn_wc__resolve_relative_external_url): Deny /../ syntax in urls in externals. Stepping over the root of a server is not possible. The first sentence sounds like no /../ are allowed in any external URLs which isn't the case, it appears with your change they are not allowed anywhere for any scheme or server root relative path, just just past the first two characters. Blair
Re: svnmerge.py (Was: Re: mergeinfo ignorance)
On 07/23/2012 08:55 AM, Jim Jagielski wrote: Is this still useful: svnmerge.py ? http://www.orcaware.com/svn/wiki/Svnmerge.py It still gets occasional usage and people occasionally ask questions about it (such as today) but I recommend people switch to svn's builtin merge tracking if they are running svn 1.6 or later. Blair
Re: svn commit: r1362480 - /subversion/trunk/subversion/libsvn_fs/fs-loader.c
On 7/17/12 6:17 AM, phi...@apache.org wrote: Author: philip Date: Tue Jul 17 13:17:34 2012 New Revision: 1362480 URL: http://svn.apache.org/viewvc?rev=1362480view=rev Log: * subversion/libsvn_fs/fs-loader.c (load_module): Only allow alphanumeric characters in name. apr_status_t status; +apr_size_t i; + +/* Demand a simple alphanumeric name so that the generated DSO + name is sensible. */ +for (i = 0; i strlen(name); ++i) + if (!svn_ctype_isalnum(name[i])) +return svn_error_createf(SVN_ERR_FS_UNKNOWN_FS_TYPE, NULL, + _(Invalid name for FS type '%s'), + name); This code doesn't probably get run often, but generally it's better to iterate through name using pointers instead of strlen() since the later scans the string twice. Blair
Re: Revprop packing implemented
On 7/6/12 6:27 AM, C. Michael Pilato wrote: On 07/06/2012 04:32 AM, Stefan Fuhrmann wrote: Hi devs, This week I had one of my how hard can it be? moments and finally implemented revprop packing (did that mainly offline). It passes all tests and seems to work pretty well. Cool! [...] Since the new code will not be used unless you create a format 6 repo, I'd like to commit everything directly to /trunk for review instead of creating a new branch or overwriting the existing one. This is the order in which I want to commit the changes: * refactor existing code * update the design file * add the revprop pack support * add tests; write more tests Should you transpose these last two? Put the tests in place first, then the code? Or are these tests of the particular low-level feature behavior that mean nothing when the feature code isn't in place? * bump the FSFS format Any objections? I have none, so long as trunk remains stable to the degree we've defined it. +1. This is great. My 50,000,000 million revisions thank you! Blair
Re: svn commit: r1358322 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c
On 7/6/12 11:04 AM, stef...@apache.org wrote:
Author: stefan2
Date: Fri Jul 6 18:04:09 2012
New Revision: 1358322
URL: http://svn.apache.org/viewvc?rev=1358322view=rev
Log:
Relax overly strict consistency check that is not covered by the FSFS
format specification.
* subversion/libsvn_fs_fs/tree.c
(svn_fs_fs__verify_root): fix test and add commentary
Modified:
subversion/trunk/subversion/libsvn_fs_fs/tree.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1358322r1=1358321r2=1358322view=diff
==
--- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Fri Jul 6 18:04:09 2012
@@ -3881,6 +3881,7 @@ svn_fs_fs__verify_root(svn_fs_root_t *ro
const svn_fs_id_t *pred_id;
dag_node_t *pred;
svn_revnum_t pred_rev;
+svn_revnum_t delta;
/* Only r0 should have no predecessor. */
SVN_ERR(svn_fs_fs__dag_get_predecessor_id(pred_id, frd-root_dir));
@@ -3898,8 +3899,23 @@ svn_fs_fs__verify_root(svn_fs_root_t *ro
{
SVN_ERR(svn_fs_fs__dag_get_node(pred, root-fs, pred_id, pool));
SVN_ERR(svn_fs_fs__dag_get_revision(pred_rev, pred, pool));
-if (pred_rev+1 != root-rev)
- /* Issue #4129. */
+
+/* Issue #4129: bogus predecessors. */
+/* Check 1: predecessor must be an earlier revision.
+ */
+if (pred_rev = root-rev)
+ return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,
+ r%ld's root node's predecessor is r%ld,
+ root-rev, pred_rev);
Suggest describing in the error message the condition that failed.
+
+/* Check 2: distances must be a power of 2.
+ * Note that this condition is not defined by the FSFS format but
+ * merely a byproduct of the current implementation. Therefore,
+ * it may help to spot corruptions for the time being but might
+ * need to be removed / relaxed in later versions.
+ */
+delta = root-rev - pred_rev;
+if (delta (delta - 1))
return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,
r%ld's root node's predecessor is r%ld,
root-rev, pred_rev);
Same here. This will make it easier for people who are not familiar with fsfs's
internals to see which condition failed.
Blair
Re: svn commit: r1352400 - in /subversion/branches/javahl-ra/subversion/bindings/javahl: native/SVNBase.cpp native/SVNBase.h src/org/apache/subversion/javahl/JNIObject.java
On 06/20/2012 08:34 PM, v...@apache.org wrote:
Author: vmpn
Date: Thu Jun 21 03:34:05 2012
New Revision: 1352400
URL: http://svn.apache.org/viewvc?rev=1352400view=rev
Log:
On the javahl-ra branch:
JavaHL: New method for creating java objects linked to their C++ counterpart
[ in subversion/bindings/javahl/native ]
* SVNBase.cpp,
SVNBase.h
(createCppBoundObject): New method for creating java objects linked to their
C++ counterpart
[ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]
* JNIObject.java: Base class for JNI linked java objects
Added:
subversion/branches/javahl-ra/subversion/bindings/javahl/src/org/apache/subversion/javahl/JNIObject.java
Modified:
subversion/branches/javahl-ra/subversion/bindings/javahl/native/SVNBase.cpp
subversion/branches/javahl-ra/subversion/bindings/javahl/native/SVNBase.h
Modified:
subversion/branches/javahl-ra/subversion/bindings/javahl/native/SVNBase.cpp
URL:
http://svn.apache.org/viewvc/subversion/branches/javahl-ra/subversion/bindings/javahl/native/SVNBase.cpp?rev=1352400r1=1352399r2=1352400view=diff
==
--- subversion/branches/javahl-ra/subversion/bindings/javahl/native/SVNBase.cpp
(original)
+++ subversion/branches/javahl-ra/subversion/bindings/javahl/native/SVNBase.cpp
Thu Jun 21 03:34:05 2012
@@ -97,3 +97,29 @@ inline void SVNBase::findCppAddrFieldID(
}
}
}
+
+jobject SVNBase::createCppBoundObject(const char *clazzName)
+{
+ JNIEnv *env = JNIUtil::getEnv();
+
+ // Create java session object
+ jclass clazz = env-FindClass(clazzName);
+ if (JNIUtil::isJavaExceptionThrown())
+ return NULL;
+
+ static jmethodID ctor = 0;
+ if (ctor == 0)
+ {
The { are indented two spaces:
http://subversion.apache.org/docs/community-guide/conventions.html
There's other indentation fixes also needed.
+package org.apache.subversion.javahl;
+
+/**
+ * This class is used internally by the JavaHL implementation and not
+ * considered part part of the public API.
+ */
+public abstract class JNIObject
+{
+/**
+ * slot for the address of the native peer.
+ * The JNI code controls this field. If it is set to 0 then
+ * underlying JNI object has been freed
+ */
+protected long cppAddr;
Can this be final?
Blair
How high are the revision numbers in your repo?
Hi Hyrum, Just got home and saw your question on IRC. Here are the statistics on our svn repositories: N is 105 sum is 51557790 meanis 491026.571428571 SD Nis 1609852.27973539 SD N-1 is 1617573.43808371 min is 1 25% is 5 50% is 3028 75% is 35981 90% is 714058 95% is 2597937 99% is 6047746 max is 10362457 \d+% is the distribution of repository sizes. There are 11 repositories with over a million commits each. Blair
Re: Serf 1.1.0 has been released
On 06/07/2012 01:40 PM, Greg Stein wrote: Hello all, I'm pleased to announce the serf 1.1.0 release! MacPorts has been updated. port -v sync port -v upgrade --enforce-variants outdated Blair
Re: svn commit: r1340253 - /subversion/trunk/COMMITTERS
On 05/21/2012 04:18 AM, Vladimir Berezniker wrote: Hyrum, 4. Use runtime rather than checked exceptions. I strongly dislike checked exceptions in code paths where there is no expected recovery logic that can be applied. This just forces people to either write a lot of try catch blocks that don't have any useful logic, propagate the exceptions through all the methods, or catch and wrap the exception in a RuntimeException derived class. I don't know if any of the other JavaHL coders saw this note, so I suggest sending a separate email with a descriptive subject Switching to unchecked Java exceptions since it is a significant change in the API and some other people may want to have a say. I do think it's a good idea when there is no action that the caller can do to recover. I cannot think of any drawbacks right now, and in our own non-svn Scala code we effectively use unchecked exceptions (because Scala doesn't do compile time checking if an exception is handled). You're proposing keeping checked exceptions when the caller can do something? Blair
Re: subversion pull request: port to new style classes - http://stackoverflow.c...
On 05/24/2012 01:32 AM, Greg Stein wrote: Hey all, I moderated this through, in order to start a discussion. My feeling is: this is not how we want to accept patches. This approach is completely disconnected from our community. How do we talk to the person providing the change? How do we ask for modifications? How to interact? As someone who spends more time coding and deploying three-tier systems dependent upon a large number of different projects (Scala, Guava, Spring, Boost, Python, Ice, etc) than working on svn (except when we run into an issue with svn itself ;) ) I appreciate the ease to quickly get a patch into a project I'm dependent upon. I prefer to submit patches than maintain private copies of upstream code. So I like the idea that people can do a small amount of work and get it into the project, even if it's as tiny as documentation patches when I'm reading the project's docs. I care to try to follow the project's coding conventions and log messages, but that's because I've been trained in this project and know that projects can be sensitive to that. So I can see this being an issue for drive-by patches. But even larger: our goal is to get people *involved* in our community. There isn't any obvious way to get 'techtonik' brought into our community unless they come to the dev@ list. I think that's a good goal, but people don't want to be turned off by a lot of process in accepting a patch either. That said... we *do* accept patches via the issue tracker on subversion.tigris.org. Are we ready to accept patches through a separate channel? Personally, I'm not ready to say hey, any channel on the planet is fine. please... feel free! devise new channels! we are willing to review 100 channels for incoming patches! I like GitHub. It is a very, very well-done site. But I'm not ready to say that it is a viable mechanism for people to deliver patches. My preference is for those to arrive here on dev@, where we can interact with the person. Not as some drive-by, fait accompli. I think that's too hard. I would rather accept work from somebody who doesn't have the goal of joining Subversion then not getting the work at all. Many of the projects I've contributed to was work to get done what I needed for my projects, but that still helped the open-source project that I was using over all. I don't have the time to join every project I use. Blair
Re: Trunk checkout fails with serf but not neon
On 05/16/2012 11:15 AM, Justin Erenkrantz wrote: On Wed, May 16, 2012 at 11:10 AM, Mark Phippardmarkp...@gmail.com wrote: I found another tool that worked ( I think I just needed to run wireshark as root), however this tool and tcpdump both do not give what I want. I am going through a VPN and it looks like all the packets are encrypted. The protocol for all packets is ESP. Philip's trick to redirect the traffic to local port 8080 via socat should let you see what your client is sending. But, a VPN could certainly be mucking with something...have you tried the HTTP/1.0 force yet? You'd need serf 1.1.x which I don't think MacPorts has yet... -- justin I'm the MacPorts serf maintainer. I don't see anything newer than 1.0.3 on http://code.google.com/p/serf/downloads/list, otherwise I probably would have updated it shortly after its release. Blair
Deleting svn-fast-backup
[cc'ing previous svn-fast-backup authors or patchers.] Reviewing my svn training deck I noticed I still recommend using svn- fast-backup, but I don't think I trust it any more, given it hasn't been updated with recent fsfs repository changes. There's been no substantial commits since it was added to our repo in 2005. If nobody minds and if no commits for 1.7 support show up, I'll delete it. Blair
Re: svn commit: r1329083 - in /subversion/trunk/subversion: include/svn_fs.h libsvn_fs/editor.c
On Apr 23, 2012, at 1:01 PM, Greg Stein wrote: On Apr 23, 2012 8:00 AM, Blair Zajac bl...@orcaware.com wrote: On Apr 23, 2012, at 2:45 AM, gst...@apache.org wrote: Author: gstein Date: Mon Apr 23 06:45:46 2012 New Revision: 1329083 URL: http://svn.apache.org/viewvc?rev=1329083view=rev Log: Add a flag that we'll need. mod_dav_svn wants to keep the txn alive, and will manually commit it. libsvn_repos never keeps a txn alive beyond the editor drive. This new flag enables the two behaviors. * subversion/include/svn_fs.h: (SVN_FS_TXN_NO_AUTOCOMMIT): new flag How about renaming this to SVN_FS_TXN_EDITOR_NO_AUTOCOMMIT to make it clear in the name that it's specific to an editor and not to txn's as a whole. (make_editor): set the new flag, to be used by complete_cb later. Modified: subversion/trunk/subversion/include/svn_fs.h subversion/trunk/subversion/libsvn_fs/editor.c Modified: subversion/trunk/subversion/include/svn_fs.h URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_fs.h?rev=1329083r1=1329082r2=1329083view=diff = = = = = = = = == --- subversion/trunk/subversion/include/svn_fs.h (original) +++ subversion/trunk/subversion/include/svn_fs.h Mon Apr 23 06:45:46 2012 @@ -808,6 +808,11 @@ typedef struct svn_fs_txn_t svn_fs_txn_t * if a caller tries to edit a locked item without having rights to the lock. */ #define SVN_FS_TXN_CHECK_LOCKS 0x2 + +/** Do not auto-commit the txn when its associated editor is marked + * as completed. + */ +#define SVN_FS_TXN_NO_AUTOCOMMIT 0x4 /** @} */ The new #define doesn't belong in this block of defines as they are for svn_fs_begin_txn2(): But it does... these three flags can be passed as the FLAGS param to svn_fs_editor_create, and are used to control the txn created as part of that. It's not controlling the underlying txn object though, just the editor. I could pass SVN_FS_TXN_NO_AUTOCOMMIT to svn_fs_begin_txn2() but it doesn't do anything with it, so it's not appropriate. It can ignore it and never use it, but it's not as clear. (so I'm also not keen on inserting another word into the symbol) I see what you're getting at in saving an argument to svn_fs_editor_create, but conceptually it's mixing two distinct sets of flags(). svn_error_t * svn_fs_editor_create(svn_editor_t **editor, const char **txn_name, svn_fs_t *fs, svn_revnum_t revision, apr_uint32_t txn_flags, apr_uint32_t editor_flags, ... or svn_error_t * svn_fs_editor_create(svn_editor_t **editor, const char **txn_name, svn_fs_t *fs, svn_revnum_t revision, apr_uint32_t txn_flags, svn_boolean_t no_autocommit, ... and hence be named SVN_FS_EDITOR_TXN_NO_AUTOCOMMIT. Blair
Re: svn commit: r1328939 - /subversion/trunk/subversion/libsvn_subr/io.c
On Apr 22, 2012, at 3:14 PM, stef...@apache.org wrote: Author: stefan2 Date: Sun Apr 22 19:14:50 2012 New Revision: 1328939 URL: http://svn.apache.org/viewvc?rev=1328939view=rev Log: * subversion/libsvn_subr/io.c (svn_io_lock_open_file, svn_io_unlock_open_file): fix handling of APR errors I guess getting the filename should never fail, but I would rather try to do the lock or unlock operation and then fail trying to get the hostname if it needs it. Blair
Re: svn commit: r1328939 - /subversion/trunk/subversion/libsvn_subr/io.c
On 4/22/12 11:52 PM, Blair Zajac wrote: On Apr 22, 2012, at 3:14 PM, stef...@apache.org wrote: Author: stefan2 Date: Sun Apr 22 19:14:50 2012 New Revision: 1328939 URL: http://svn.apache.org/viewvc?rev=1328939view=rev Log: * subversion/libsvn_subr/io.c (svn_io_lock_open_file, svn_io_unlock_open_file): fix handling of APR errors I guess getting the filename should never fail, but I would rather try to do the lock or unlock operation and then fail trying to get the hostname if it needs it. I meant filename. Blair
sqlite3 3.7.11 breaks svnversion: E200030: sqlite: callback requested query abort
I just got this on my MacPorts svn, the revision number output is missing: $ svnversion svn: E200030: sqlite: callback requested query abort svn: E200030: sqlite: callback requested query abort $ echo $? 1 I've seen a number of other reports of this issue, including freebsd: https://www.google.com/search?q=svn%3A+E200030%3A+sqlite%3A+callback+requested+query+abort This error occurs when sqlite is upgraded to 3.7.11, it works fine with 3.7.10. Personally, I've only seen this in upgraded wc's, fresh checkouts haven't failed, but I've only tried it in one checkout. Blair
Re: sqlite3 3.7.11 breaks svnversion: E200030: sqlite: callback requested query abort
On Apr 20, 2012, at 5:22 PM, Hyrum K Wright wrote: On Fri, Apr 20, 2012 at 6:19 PM, Blair Zajac bl...@orcaware.com wrote: I just got this on my MacPorts svn, the revision number output is missing: $ svnversion svn: E200030: sqlite: callback requested query abort svn: E200030: sqlite: callback requested query abort $ echo $? 1 I've seen a number of other reports of this issue, including freebsd: https://www.google.com/search?q=svn%3A+E200030%3A+sqlite%3A+callback+requested+query+abort This error occurs when sqlite is upgraded to 3.7.11, it works fine with 3.7.10. Personally, I've only seen this in upgraded wc's, fresh checkouts haven't failed, but I've only tried it in one checkout. We use the RELEASE statement, which has a bug in 3.7.11, scheduled to be fixed in 3.7.12. From http://www.sqlite.org/draft/releaselog/3_7_12.html: Bug fix: Fix the RELEASE command so that it does not cancel pending queries. This repairs a problem introduced in 3.7.11 On the surface, this sounds like what your problem is. Hi Hyrum, Thanks for the info. We'll wait for 3.7.12. Blair
Re: Changing svn_checksum__from_digest()'s signature
On 04/18/2012 12:15 AM, Julian Foad wrote: Blair Zajac wrote: In case of an illegal svn_checksum_kind_t being passed to svn_checksum__from_digest(), I want to change it from svn_checksum_t * svn_checksum__from_digest(const unsigned char *digest, svn_checksum_kind_t kind, apr_pool_t *result_pool); to svn_error_t * svn_checksum__from_digest(svn_checksum_t **checksum, const unsigned char *digest, svn_checksum_kind_t kind, apr_pool_t *result_pool); Wouldn't a better API be: svn_checksum_t * svn_checksum__from_digest_md5(digest, result_pool); svn_checksum_t * svn_checksum__from_digest_sha1(digest, result_pool); since all current callers want just one specific kind (apart from internal calls from the implementations of svn_checksum_dup and svn_checksum_empty_checksum)? Notice that svn_checksum_empty_checksum() returns NULL if the kind is not recognized, while svn_checksum_dup() does no such check and would pass the unknown kind into ...from_digest(). Therefore it appears that _dup() is the only caller that could have been passing a bad kind -- at least in current trunk code; it may be different in whatever version you're running. And so, to fix the crash, you will still need to decide how _dup() should handle a bogus checksum struct. You could have _dup() return NULL, but unless the callers check for null that would just defer the crash. It would be a little better to call SVN_ERR_MALFUNCTION_NO_RETURN(), to give the client program a little more awareness of the abnormal termination. That's what I'm seeing, _dup() is calling __from_digest(). It appears to be a lifetime issue where it's trying to dup a svn_checksum_t * pointing to bad data. It would be nice if _dup() returned a svn_error_t * instead of causing a core dump later or calling SVN_ERR_MALFUNCTION_NO_RETURN(), but that would be a messier API and not consistent with all our other _dup()'s. Too bad we're not working in C++ where we could throw an exception. This is to prevent a core dump we've observed with our RPC server. The function is in a public .h file but shown as private. Do I need to add svn_checksum__from_digest2() or can I just change it? We're allowed to just change it, as I understand it. (The closest thing I can find in 'hacking' ishttp://subversion.apache.org/docs/community-guide/conventions.html#interface-visibility. But that doesn't seem to mention the nuances of ABI vs. API and the issue of, IIRC, all symbols in a shared library getting into a Windows DLL ABI even if they're supposed to be private, or whatever exactly that issue was.) Thanks. I'm just deleting it following this discussion. Blair
Re: svn commit: r1327695 - /subversion/trunk/subversion/libsvn_subr/checksum.c
On 04/18/2012 02:54 PM, Greg Stein wrote:
On Wed, Apr 18, 2012 at 17:52,bl...@apache.org wrote:
...
+++ subversion/trunk/subversion/libsvn_subr/checksum.c Wed Apr 18 21:52:01 2012
@@ -56,25 +56,43 @@ validate_kind(svn_checksum_kind_t kind)
return svn_error_create(SVN_ERR_BAD_CHECKSUM_KIND, NULL, NULL);
}
+/* Create a svn_checksum_t with everything but the contents of the
+ digest populated. */
+static svn_checksum_t *
+checksum_create(svn_checksum_kind_t kind,
+apr_size_t digest_size,
+apr_pool_t *pool)
+{
+ // Use apr_palloc() instead of apr_pcalloc() so that the digest
+ // contents are only set once by the caller.
No C++ style comments, please...
Oops, thanks. Been coding in C++ too long. Fixed in r1327708.
Blair
Re: svn commit: r1327695 - /subversion/trunk/subversion/libsvn_subr/checksum.c
On 04/18/2012 02:57 PM, Greg Stein wrote: On Wed, Apr 18, 2012 at 17:52,bl...@apache.org wrote: Author: blair Date: Wed Apr 18 21:52:01 2012 New Revision: 1327695 URL: http://svn.apache.org/viewvc?rev=1327695view=rev Log: Refactor svn_checksum_create() to avoid two switch statements. * subversion/libsvn_subr/checksum.c (checksum_create): New static function that creates a svn_checksum_t * with everything set but the contents of the digest. (svn_checksum_create): Use the existing switch statement to get the digest size without needing DIGESTSIZE(). Use checksum_create(). Couldn't you just lose the switch in favor of: digest_size = DIGESTSIZE(kind) ?? That seemed micro-optimization wasteful, as it's checking the kind if it's a known checksum once and then checking it again on the size. Blair
Re: svn commit: r1327703 - in /subversion/trunk/subversion: include/private/svn_subr_private.h libsvn_fs_base/util/fs_skels.c libsvn_subr/checksum.c
On 04/18/2012 03:35 PM, Greg Stein wrote: On Wed, Apr 18, 2012 at 18:11,bl...@apache.org wrote: Author: blair Date: Wed Apr 18 22:11:43 2012 New Revision: 1327703 URL: http://svn.apache.org/viewvc?rev=1327703view=rev Log: Add and use svn_checksum__from_digest_sha1(). This replaces svn_checksum__from_digest() taking a svn_checksum_sha1 argument with a new svn_checksum_t constructor function. * subversion/include/private/svn_subr_private.h, * subversion/libsvn_subr/checksum.c (svn_checksum__from_digest_sha1): New private function. * subversion/libsvn_fs_base/util/fs_skels.c (svn_fs_base__parse_representation_skel): Replace svn_checksum__from_digest(digest, svn_checksum_sha1, pool); with svn_checksum__from_digest_sha1(digest, pool); Shouldn't we be using svn_checksum_serialize() and _deserialize() rather than raw digests? (iow, we shouldn't need from_digest_sha1) They don't contain the same bytes. I wasn't aware of svn_checksum_serialize() till now, but those prepend a checksum type string to the hex representation, as these are raw digest bytes. This work was to get rid of svn_checksum__from_digest(), which is now almost complete, per the other thread. Blair
Re: svn commit: r1327703 - in /subversion/trunk/subversion: include/private/svn_subr_private.h libsvn_fs_base/util/fs_skels.c libsvn_subr/checksum.c
On 04/18/2012 03:45 PM, Greg Stein wrote: On Wed, Apr 18, 2012 at 18:42, Blair Zajacbl...@orcaware.com wrote: On 04/18/2012 03:35 PM, Greg Stein wrote: On Wed, Apr 18, 2012 at 18:11,bl...@apache.orgwrote: Author: blair Date: Wed Apr 18 22:11:43 2012 New Revision: 1327703 URL: http://svn.apache.org/viewvc?rev=1327703view=rev Log: Add and use svn_checksum__from_digest_sha1(). This replaces svn_checksum__from_digest() taking a svn_checksum_sha1 argument with a new svn_checksum_t constructor function. * subversion/include/private/svn_subr_private.h, * subversion/libsvn_subr/checksum.c (svn_checksum__from_digest_sha1): New private function. * subversion/libsvn_fs_base/util/fs_skels.c (svn_fs_base__parse_representation_skel): Replace svn_checksum__from_digest(digest, svn_checksum_sha1, pool); with svn_checksum__from_digest_sha1(digest, pool); Shouldn't we be using svn_checksum_serialize() and _deserialize() rather than raw digests? (iow, we shouldn't need from_digest_sha1) They don't contain the same bytes. I wasn't aware of svn_checksum_serialize() till now, but those prepend a checksum type string to the hex representation, as these are raw digest bytes. This work was to get rid of svn_checksum__from_digest(), which is now almost complete, per the other thread. Yup, I understood. Sorry that I wasn't clear: I meant in our serialization code, shouldn't we use the proper functions rather than raw sha1 digests? Is there any way to switch to them at a higher/semantic level? I haven't looked at that stuff, but I'm going to guess repositories now exist with raw sha1 digests. Is there a format type in there? Can we start writing csum and svn_checksum_serialize() into the skel? And then read raw md5, raw sha1, or a serialized checksum? I'm not familiar with this part of the code either (not having looked at the svn_skel.h before) but that make sense. The code could look for a raw digest or a $name$ and then use that. It doesn't look like it would be too hard. Because the skel has a len, if it's equal to APR_SHA1_DIGESTSIZE then you would use the raw digest, otherwise use svn_checksum_deserialize(). Sounds like a 1.8 repository upgrade though, since once you wrote a $sha1$ style string, older Subversion's wouldn't parse it directly. I won't be taking this on, I've still got a memory lifetime issue I'm debugging and then a svn commit thread pool to write to support 4 commits/sec from remote clients that are consuming all threads from our RPC thread pool and killing readers. Blair
Re: fs_fs core dumps in checksum code
On 04/16/2012 02:16 AM, Julian Foad wrote:
Blair Zajac wrote:
On 04/13/2012 12:45 AM, Julian Foad wrote:
Blair Zajac wrote:
Having the empty files, such as changes, is that odd? Could that be a
hint?
No, that's not interesting, that's just the result of crashing out
at the point where it did -- in the middle of doing a commit.
The 'changes' is created during the commit process and not building the
transaction? If so, then having an empty changes file is odd and probably only
possible through the RPCS API we wrote that wraps svn_fs.h and svn_repos.h, in
which case, could there be a bug with trying to commit empty transactions in a
multithreaded environment?
Or maybe the commit just got as far as creating the 'changes' file but crashed
out before it got around to writing the list of changes into that file and
flushing/closing it.
It appears that the changes file is only modified upon any modification
to the txn and only read during the actual commit of the txn:
#!/usr/bin/python
import os
import sys
import time
import svn.fs
import svn.repos
try:
repo = svn.repos.create('repo', None, None, {}, {})
except Exception:
repo = svn.repos.open('repo')
fs = svn.repos.fs(repo)
txn = svn.fs.begin_txn2(fs, svn.fs.youngest_rev(fs), 0)
fs_root = svn.fs.txn_root(txn)
txn_name = svn.fs.txn_name(txn)
changes_path = os.path.join('repo', 'db', 'transactions', '%s.txn' %
txn_name,
'changes')
print 1, os.stat(changes_path)[6], svn.fs.paths_changed(fs_root)
svn.fs.make_dir(fs_root, '%s' % (1000*1000*time.time()))
print 2, os.stat(changes_path)[6], svn.fs.paths_changed(fs_root)
os.system(strace -p %s -o strace.out % os.getpid())
print svn.repos.fs_commit_txn(repo, txn)
Changing svn_checksum__from_digest()'s signature
In case of an illegal svn_checksum_kind_t being passed to svn_checksum__from_digest(), I want to change it from svn_checksum_t * svn_checksum__from_digest(const unsigned char *digest, svn_checksum_kind_t kind, apr_pool_t *result_pool); to svn_error_t * svn_checksum__from_digest(svn_checksum_t **checksum, const unsigned char *digest, svn_checksum_kind_t kind, apr_pool_t *result_pool); This is to prevent a core dump we've observed with our RPC server. The function is in a public .h file but shown as private. Do I need to add svn_checksum__from_digest2() or can I just change it? Blair
Re: svn commit: r1298587 - in /subversion/branches/revprop-cache: build.conf subversion/include/private/svn_named_atomic.h subversion/include/svn_error_codes.h subversion/libsvn_subr/svn_named_atomic.
On 03/08/2012 01:18 PM, stef...@apache.org wrote:
Author: stefan2
Date: Thu Mar 8 21:18:45 2012
New Revision: 1298587
URL: http://svn.apache.org/viewvc?rev=1298587view=rev
Log:
Define and implement svn_named_atomic__t and associated API.
* subversion/include/private/svn_named_atomic.h
new file, containing the new svn_named_atomic API
* subversion/libsvn_subr/svn_named_atomic.c
new file, implements the new API
* subversion/include/svn_error_codes.h
(SVN_ERR_BAD_ATOMIC): new error code used throughout the new API
* build.conf
(libsvn_subr): add new header to exports
+static apr_int64_t
+synched_add(volatile apr_int64_t *mem, apr_int64_t delta)
+{
+ return *mem += delta;
+}
Is this thread safe? Could the thread after reading *mem be context
switched out, another thread calls synced_add(), then the first thread
does its add and writes the *mem?
Blair
Re: Use 1.7.7 for next release
On 04/15/2012 07:05 AM, Ashod Nakashian wrote: - Original Message - From: Daniel Shahafd...@daniel.shahaf.name To: Ashod Nakashianashodnakash...@yahoo.com; Stefan Küngtortoise...@gmail.com; dev@subversion.apache.orgdev@subversion.apache.org Cc: Sent: Sunday, April 15, 2012 2:06 PM Subject: Re: Use 1.7.7 for next release On Sun, Apr 15, 2012, at 02:36, Ashod Nakashian wrote: And surely at 1.8.0 you'll sync back, so not only this will auto- correct, but as the third number is the patch number, I personally care little whether or not the patch is a TSVN one or mainline SVN - I care about the major.minor, and that's perfectly matching. It's not auto-correcting, what if TSVN 1.8.0 links against SVN 1.8.0 and then TSVN needs to make another release? I meant the two project versions won't diverge indefinitely. Sure each patch can result in a mismatch (assuming TSVN continues to use the 3rd number for patches) but as long as both projects use the same major.minor numbers, they realign again at every significant release. Which is my point; patch numbers aren't all that relevant. What a user typically cares about are the major.minor version of SVN (which dictates features and compatibility etc.) and that they have the latest patch for *that* major.minor. In other words, if I'm using TSVN and I care to use version 1.7 of SVN features, I'll make sure I have the latest patch for 1.7 of TSVN. If that's 1.7.9, then that's the best that TSVN provides, regardless what the patch number of SVN stands at. If I care to know the exact SVN patch TSVN is linked against, I have ample places to look for this information. (and if I know of an SVN patch that TSVN doesn't yet provide, then I'll either wait or switch to a different 3rd party SVN product.) Taking care of the second concern also resolves the first concern. I've always found it a little more work than I would think is necessary to see quickly see which version of Subversion TortoiseSVN is built on. Not talking about a lot, but it's a minor annoyance. Blair
Re: svn commit: r1326307 - in /subversion/trunk: ./ build/ac-macros/ subversion/include/ subversion/include/private/ subversion/libsvn_fs_fs/ subversion/libsvn_subr/ subversion/mod_dav_svn/ subversion
On 04/15/2012 04:20 AM, stef...@apache.org wrote:
Author: stefan2
Date: Sun Apr 15 11:20:58 2012
New Revision: 1326307
URL: http://svn.apache.org/viewvc?rev=1326307view=rev
Log:
Merge all changes (-r1298521-1326293) from branches/revprop-cache to trunk
and resolve minor conflicts.
+/* Test whether revprop cache and necessary infrastructure are
+ available in FS. */
+static svn_boolean_t
+has_revprop_cache(svn_fs_t *fs)
+{
+ fs_fs_data_t *ffd = fs-fsap_data;
+ svn_error_t *error;
+
+ /* is the cache (still) enabled? */
+ if (ffd-revprop_cache == NULL)
+return FALSE;
+
+ /* try to access our SHM-backed infrastructure */
+ error = ensure_revprop_generation(fs);
+ if (error)
+{
+ /* failure - disable revprop cache for good */
+
+ svn_error_clear(error);
Should we log why it was disabled instead of silently doing so? I would
want a way to determine why it was disabled.
How about returning the error to the caller so they can log it? They
can check the svn_error_t == SVN_NO_ERROR to see if it's enabled or not.
+/* Read the current revprop generation and return it in *GENERATION.
+ Also, detect aborted / crashed writers and recover from that.
+ Use the access object in FS to set the shared mem values. */
+static svn_error_t *
+read_revprop_generation(svn_fs_t *fs,
+apr_int64_t *generation)
Out parameters are normally listed first.
Blair
Re: svn commit: r1326354 - in /subversion/trunk/subversion: include/private/svn_named_atomic.h libsvn_fs_fs/fs_fs.c libsvn_subr/svn_named_atomic.c
On 04/15/2012 07:49 AM, stef...@apache.org wrote: Author: stefan2 Date: Sun Apr 15 14:49:21 2012 New Revision: 1326354 URL: http://svn.apache.org/viewvc?rev=1326354view=rev Log: Disable the revprop cache if our sync. mechanisms are too expensive. * subversion/include/private/svn_named_atomic.h (svn_named_atomic__is_efficient): declare new API function * subversion/libsvn_subr/svn_named_atomic.c (SYNCHRONIZE_IS_FAST): new constant, set depending on system / compiler capabilities (svn_named_atomic__is_efficient): implement new API function * subversion/libsvn_fs_fs/fs_fs.c (has_revprop_cache): disable cache if sync is slow Modified: subversion/trunk/subversion/include/private/svn_named_atomic.h subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c subversion/trunk/subversion/libsvn_subr/svn_named_atomic.c +/** Returns #TRUE on platforms that don't need expensive synchronization + * objects to serialize access to named atomics. If this returns #FALSE, + * reading from or modifying a #svn_named_atomic__t may be as expensive + * as a file system operation. That is assuming it's a local file system? It still may be faster than getting a file from NFS, which people will host repositories on. Blair
svn_checksum__from_digest() core dump on invalid input
In tracking down spurious coredumps we have in our custom RPC server, I found
one in svn_checksum__from_digest() due to bad input:
svn_checksum_t *
svn_checksum__from_digest(const unsigned char *digest,
svn_checksum_kind_t kind,
apr_pool_t *result_pool)
{
svn_checksum_t *checksum = svn_checksum_create(kind, result_pool);
memcpy((unsigned char *)checksum-digest, digest, DIGESTSIZE(kind));
return checksum;
}
If kind is invalid, then svn_checksum_create() returns NULL, which then
coredumps in memcpy().
Normally, I would change svn_checksum__from_digest() to
svn_error_t *
svn_checksum__from_digest(svn_checksum_t **checksum,
const unsigned char *digest,
svn_checksum_kind_t kind,
apr_pool_t *result_pool);
but I'm wondering how the checksum code needs to handle newer checksums in older
Subversion servers? Say 1.9 adds sha224 and then we read the repos in 1.7, how
should svn handle this? Should it throw a SVN_ERR_BAD_CHECKSUM_KIND or silently
map an unknown checksum to NULL? Do we want to conditionally support checksums
depending upon repository version?
Frankly, I'm happy I got a core dump in our code since it prevented bad data
from being used. My feeling is that the bad data is coming from a cache entry
that is released but something has a pointer to it still.
Blair
Re: Use 1.7.7 for next release (was: Re: 1.7.5 in one/two weeks?)
On 4/14/12 12:24 PM, Konstantin Kolinko wrote: 2012/4/12 Daniel Shahafdanie...@apache.org: We released 1.6.18 today and 1.7.4 just over a month ago. There are a few useful items merged already, and STATUS has a truckload of pending changes. Shall we roll 1.7.5 in two weeks from today? If we can clear STATUS and roll next Thursday that's fine too, but I don't think we're in a hurry. Hi! I have a proposal: Skip several numbers and name the next release as 1.7.7. Justification: to align with TortoiseSVN, which is 1.7.6 now. There is a lot of Subversion exception! threads on users@ where TortoiseSVN version is visible. For example [1]. I think skipping those already used numbers will lessen confusion. Since Subversion is the base project, I would rather see TortoiseSVN change it's versioning to match ours than the other way. TortoiseSVN could add an additional version number after Subversion's, e.g. 1.7.4-tsvn1 for the first TortoiseSVN release based on 1.7.4, 1.7.4-tsvn2 for the second, etc. Blair
Re: fs_fs core dumps in checksum code
On 04/13/2012 12:45 AM, Julian Foad wrote: Blair Zajac wrote: Since we discussed this, we moved the Subversion server to a new box and from RAID to FusionIO storage and we're still getting the core dumps with the same stack trace, so I don't think its memory corruption. I meant I suspect corruption of this process's state by any mechanism, which could be buffer overflows, bad multi-threading, and so on. You wrote before, I'll run our our backend severs on the dev cluster in valgrind and see if we pick up anything there. Were you able to try that? Or just load the core dump files into GDB and see what you can see? We didn't do valgrind, the box is in production and it would be too slow. Maybe I can set up a dev process on the production server to test. To do valgrind well, do I need to recompile APR with specific flags to enable pool debugging? Yesterday, we got two core dumps within 30 minutes of each other. Would looking at the txn files in progress tell us anything? [...] Having the empty files, such as changes, is that odd? Could that be a hint? No, that's not interesting, that's just the result of crashing out at the point where it did -- in the middle of doing a commit. The 'changes' is created during the commit process and not building the transaction? If so, then having an empty changes file is odd and probably only possible through the RPCS API we wrote that wraps svn_fs.h and svn_repos.h, in which case, could there be a bug with trying to commit empty transactions in a multithreaded environment? Blair
Re: fs_fs core dumps in checksum code
On 04/13/2012 06:33 PM, Daniel Shahaf wrote: Blair Zajac wrote on Fri, Apr 13, 2012 at 13:08:12 -0700: To do valgrind well, do I need to recompile APR with specific flags to enable pool debugging? Other than --enable-pool-debugging=N (aka -DAPR_POOL_DEBUG=N) (where N is placeholder for a number) ? Yes, besides that? Besides valgrind and enabling pool debugging, does anybody have any recommendations for memory debugging besides valgrind in a production environment, something like using a library that you LD_PRELOAD to intercept malloc(), free(), etc? These core dumps occur infrequently so if there's a way to leave some debugging in that doesn't cost too much of a hit, that would be great. Blair
Re: fs_fs core dumps in checksum code
On 08/04/2010 09:32 AM, Blair Zajac wrote: On 08/04/2010 05:38 AM, Julian Foad wrote: Again due to in-lining, I presume, fs_fs.c:2859 calls svn_checksum_dup() but the debugger shows, in this case, only its subroutine svn_checksum__from_digest(). #0 0x2ac15f8bd28c in svn_checksum__from_digest ( digest=0x646c6f663a706e76Address 0x646c6f663a706e76 out of bounds, kind=1949987428, result_pool=value optimized out) at subversion/libsvn_subr/checksum.c:77 Here, digest is ASCII dmof:pnv and kind is hex 743a7264 is ASCII t:rd... 77 memcpy((unsigned char *)checksum-digest, digest, DIGESTSIZE(kind)); #1 0x2ac15f057f87 in read_representation (contents_p=0x2aaab92d7c50, fs=0x2aab80d53590, rep=0x2aabbd57e748, pool=0x2aab1c15a0e8) at subversion/libsvn_fs_fs/fs_fs.c:2859 ... meaning that 'rep-md5_checksum' points to readable memory but not to a valid checksum object. Thus, in both cases, it looks like that pointer was either uninitialized or subsequently corrupted. I searched in 1.6.5 code for creation of a representation_t structure and found nowhere that leaves it uninitialized. At least one place allocates the structure with apr_pcalloc (zero-initialized) and doesn't subsequently fill in -md5_checksum, but that's not what you're seeing. I searched for creation of node_revision_t structures and found no places where the data_rep member is uninitialized. Thus I can only suggest looking for memory corruption. Hi Julian, Thanks for taking your time to look at this, I appreciate it. I haven't followed the code path in detail, but my gut agrees with you on memory corruption. Hi Julian, Resurrecting this thread [1] from 1.5 years ago ;) Since we discussed this, we moved the Subversion server to a new box and from RAID to FusionIO storage and we're still getting the core dumps with the same stack trace, so I don't think its memory corruption. Yesterday, we got two core dumps within 30 minutes of each other. Would looking at the txn files in progress tell us anything? bash-3.2# ls -l transactions/5653610-3f4jm.txn/ total 16 -rw-r--r-- 1 tomcat games0 Apr 11 18:40 changes -rw-r--r-- 1 tomcat games4 Apr 11 18:41 next-ids -rw-r--r-- 1 tomcat games 156 Apr 11 18:41 node.0.0 -rw-r--r-- 1 tomcat games 2035 Apr 11 18:41 node.0.0.children -rw-r--r-- 1 tomcat games 2366 Apr 11 18:41 props bash-3.2# ls -l txn-protorevs/5653610-3f4jm.rev* -rw-r--r-- 1 tomcat games 0 Apr 11 18:40 txn-protorevs/5653610-3f4jm.rev -rw-r--r-- 1 tomcat games 0 Apr 11 18:40 txn-protorevs/5653610-3f4jm.rev-lock Having the empty files, such as changes, is that odd? Could that be a hint? Blair [1] http://mail-archives.apache.org/mod_mbox/subversion-dev/201008.mbox/%3c4c59960c.90...@orcaware.com%3E
Re: 1.6.18 tarballs up for testing/signing
On 03/29/2012 06:10 AM, Stefan Sperling wrote: On Fri, Mar 23, 2012 at 03:54:13PM +0100, Stefan Sperling wrote: Subversion-1.6.18 tarballs are now available for testing/signing by committers. To obtain them please check out a working copy from https://dist.apache.org/repos/dist/dev/subversion Please add your signatures to the .asc files there. Just a reminder: We are still short on 2 *nix and 2 windows sigs for 1.6.18. Working on a Unix signature. The tar and zip deps are different, zip appears to contain apr 1.4.5 and the tar contains apr 1.4.6. I suggest unpacking both and doing a diff -rwu, there's some other differences also, such as a missing apr-iconv in the zip. Blair
Re: 1.6.18 tarballs up for testing/signing
On 03/29/2012 11:44 AM, Blair Zajac wrote: On 03/29/2012 06:10 AM, Stefan Sperling wrote: On Fri, Mar 23, 2012 at 03:54:13PM +0100, Stefan Sperling wrote: Subversion-1.6.18 tarballs are now available for testing/signing by committers. To obtain them please check out a working copy from https://dist.apache.org/repos/dist/dev/subversion Please add your signatures to the .asc files there. Just a reminder: We are still short on 2 *nix and 2 windows sigs for 1.6.18. Working on a Unix signature. The tar and zip deps are different, zip appears to contain apr 1.4.5 and the tar contains apr 1.4.6. I suggest unpacking both and doing a diff -rwu, there's some other differences also, such as a missing apr-iconv in the zip. +1 for release. Tested on the following OSes: == Centos 4.4 i386 and x86_64 Dependencies (most are *not* from the OS): APR 1.3.8 APR-util 1.3.9 Berkeley DB 4.6.21 SQLite 3.6.20 SWIG 1.3.25 Neon 0.28.3 Java 6u26 Perl 5.8.5 Python 2.4.1 Ruby 1.8.5.2 Verified: (local, svn) x (bdb, fsfs) + py + pl + rb + javahl All tests pass == Oracle Linux 5.5 Dependencies (most are *not* from the OS): APR 1.3.12 APR-util 1.3.10 Berkeley DB 4.6.21 SQLite 3.6.20 SWIG 1.3.25 Neon 0.28.4 Java 6u30 Perl 5.8.8 Python 2.4.3 Ruby 1.8.5.2 Verified: (local, svn) x (bdb, fsfs) + py + pl + rb + javahl All tests pass == Oracle Linux 6.2 Dependencies (most are from the OS): APR 1.3.9 APR-util 1.3.9 SQLite 3.6.20 SWIG 1.3.40 Neon 0.29.3 Java 1.5.0 Perl 5.10.1 Python 2.6.6 Ruby 1.8.7 Verified: (local) x (fsfs) + py + pl All tests pass Blair
Re: 1.6.18 tarballs up for testing/signing
On 03/24/2012 03:13 AM, Stefan Sperling wrote: On Fri, Mar 23, 2012 at 03:54:13PM +0100, Stefan Sperling wrote: Since this is a 1.6.x release there are -dep tarballs which should also be signed by those who test them. I've run a test build with these dependencies on a Debian Linux system. I made a slight mistake by putting APR 1.4.6 into the -deps tarball while the 1.6.x test suite isn't prepared to deal with the APR hash ordering changes. Should we re-roll the deps tarball with APR 1.4.5? I'd call this a packaging bug and re-use the existing version number. Nobody but myself has signed the -deps tarball yet. Don't we have a hard and fast policy on this? Version numbers are cheap. Also, do we need to have matching the svn and deps tarball version numbers, so we could bump the deps but leave svn alone? BTW, any chance of getting the new svnadmin verify work into 1.6.x? I was thinking it may be worth cutting .19 just to get that in as people may want to easily know if they have an issue with their repositories. Blair
Re: 1.6.18 tarballs up for testing/signing
On 03/24/2012 01:40 PM, Daniel Shahaf wrote: Blair Zajac wrote on Sat, Mar 24, 2012 at 13:17:17 -0700: BTW, any chance of getting the new svnadmin verify work into 1.6.x? I was thinking it may be worth cutting .19 just to get that in as people may want to easily know if they have an issue with their repositories. That work hasn't been backported even to 1.7 because it uses a new public API, svn_fs_verify(). We can rename this function and make it private for 1.6 and 1.7. Blair
Re: [RFC] bump min serf version, or degrade? (was: svn commit: r1302682 ...)
On 03/19/2012 05:18 PM, Hyrum K Wright wrote: On Mon, Mar 19, 2012 at 5:11 PM, Greg Steingst...@gmail.com wrote: Hey all, With the change below, we can send all requests using Content-Length rather than chunking. This is the core work for fixing issue 3979. My question: should we simply bump the minimum serf to 1.1, or should we just omit the Content-Length functionality? If the latter, then users may run into 3979 if they have an older serf. I'm thinking: bump our requirement. It will require packagers/distros to get serf 1.1 packaged soonish. (and no, it hasn't been released yet) Thoughts? Bump the required version: folks are going to have to repackage Subversion as it is. If you want a newer Subversion, you need to be prepared for newer deps. Yes, if this is just for 1.8, then go to serf 1.1. Blair
Re: serf 1.0.3 released
On 03/20/2012 01:00 PM, Greg Stein wrote: Hi all, I've just released serf version 1.0.3. This contains a small fix from Bert to map more OpenSSL error codes into serf's UNKNOWNCA code. This allows Subversion's certificate verification prompt to provide the (p) permanent acceptance option. Please visit http://code.google.com/p/serf/downloads/list to download the release. Updated in MacPorts. Blair
Re: svn commit: r1296604 - in /subversion/trunk/subversion/libsvn_fs_fs: caching.c fs.h fs_fs.c
On 03/05/2012 01:57 PM, Stefan Fuhrmann wrote: On 05.03.2012 15:20, Daniel Shahaf wrote: Philip Martin wrote on Mon, Mar 05, 2012 at 13:58:18 +: Daniel Shahafdanie...@elego.de writes: Philip Martin wrote on Mon, Mar 05, 2012 at 12:23:40 +: Daniel Shahafdanie...@elego.de writes: You're right, I misread the code: I mistakenly thought line 2867 will always re-read the revprop-gen file from disk. How about: Index: subversion/libsvn_fs_fs/fs_fs.c === --- subversion/libsvn_fs_fs/fs_fs.c (revision 1297002) +++ subversion/libsvn_fs_fs/fs_fs.c (working copy) @@ -583,8 +583,9 @@ with_some_lock_file(svn_fs_t *fs, fs_fs_data_t *ffd = fs-fsap_data; if (ffd-format= SVN_FS_FS__MIN_PACKED_FORMAT) SVN_ERR(update_min_unpacked_rev(fs, pool)); SVN_ERR(get_youngest(ffd-youngest_rev_cache, fs-path, pool)); + SVN_ERR(read_revprop_generation(fs, pool)); err = body(baton, subpool); } That looks like it works. But it only works if all writers update the generation file so this whole caching scheme requires an FSFS format bump to exclude 1.7 and earlier. +1 (It was pointed on IRC, too.) If we were to clear the cache after taking the write lock, either unconditionally or if the revprop generation has changed, then we would become compatible with pre-1.8 style writers. Of course this does highlight a change in svn_fs_t behaviour. A long lived svn_fs_t may never see revprop updates as the cache never expires. The cached values might get pushed out if the process (thead?) reads other revisions, to get reasonably up-to-date values the caller must not hold svn_fs_t objects too long. Which is a regression --- even a process that does proplist() in a loop should eventually see changes. Hm. I would expect the live of a fs_t to be limited by the life of the RA session. At least for our CL client, this would not be a problem. For TSVN dialogs like the repository browser that keep client contexts open for a long time, it all hinges on how long the RA session is kept open by the SVN libs and whether the client has any control over it. Can anyone comment on that? We (Sony Imageworks) have a custom built Subversion server that does 2,000 RPCs/sec, so we LRU cache svn_fs_t, svn_repos_t, and svn_fs_root_t to avoid filesystem IO. Blair
Re: svn commit: r1294455 - /subversion/trunk/tools/buildbot/slaves/ubuntu-x64/svnbuild.sh
On 02/28/2012 05:46 AM, Mark Phippard wrote: On Tue, Feb 28, 2012 at 8:36 AM, Hyrum K Wright hyrum.wri...@wandisco.com wrote: I'm not sure I understand what you mean here. People will be using Java 7 to attempt to build the JavaHL bindings, so I think it makes sense to use them on our tests. In fact, this entire thing was motivated by the fact that I upgraded the slave to the latest Ubuntu Beta, and discovered it is now shipping Java 7 by default. I assume he was asking how do we make sure we do not add features in the source code that would only compile with JDK7. Yes, that's correct. It's like moving from Python 2.X to 2.X+1, e.g. the with keywords. Java 7 picked up some nice new features from Project Coin [1] From what I recall, we specify the --source=1.5 option when compiling. So that should take care of it in terms of language features. I think we could technically still use new methods from the runtime libraries without being flagged with an error, but that is really unlikely in our codebase. Using --source=1.5 should be sufficient. Blair [1] https://blogs.oracle.com/abuckley/resource/QConSF2011-JavaSE78.pdf
Re: svn commit: r1294455 - /subversion/trunk/tools/buildbot/slaves/ubuntu-x64/svnbuild.sh
On 2/27/12 8:32 PM, hwri...@apache.org wrote: Author: hwright Date: Tue Feb 28 04:32:21 2012 New Revision: 1294455 URL: http://svn.apache.org/viewvc?rev=1294455view=rev Log: * tools/buildbot/slaves/ubuntu-x64/svnbuild.sh: Update configure args to reflect new java version. Modified: subversion/trunk/tools/buildbot/slaves/ubuntu-x64/svnbuild.sh Modified: subversion/trunk/tools/buildbot/slaves/ubuntu-x64/svnbuild.sh URL: http://svn.apache.org/viewvc/subversion/trunk/tools/buildbot/slaves/ubuntu-x64/svnbuild.sh?rev=1294455r1=1294454r2=1294455view=diff == --- subversion/trunk/tools/buildbot/slaves/ubuntu-x64/svnbuild.sh (original) +++ subversion/trunk/tools/buildbot/slaves/ubuntu-x64/svnbuild.sh Tue Feb 28 04:32:21 2012 @@ -30,7 +30,7 @@ echo = autogen.sh echo = configure ./configure --enable-javahl --enable-maintainer-mode \ --without-berkeley-db \ ---with-jdk=/usr/lib/jvm/java-6-openjdk/ \ +--with-jdk=/usr/lib/jvm/java-7-openjdk-amd64/ \ Don't we want to stay on JDK 6 so we don't inadvertently pick up new Java 7 features? Blair
Re: svn commit: r1294455 - /subversion/trunk/tools/buildbot/slaves/ubuntu-x64/svnbuild.sh
On 2/27/12 8:34 PM, Blair Zajac wrote: On 2/27/12 8:32 PM, hwri...@apache.org wrote: Author: hwright Date: Tue Feb 28 04:32:21 2012 New Revision: 1294455 URL: http://svn.apache.org/viewvc?rev=1294455view=rev Log: * tools/buildbot/slaves/ubuntu-x64/svnbuild.sh: Update configure args to reflect new java version. Modified: subversion/trunk/tools/buildbot/slaves/ubuntu-x64/svnbuild.sh Modified: subversion/trunk/tools/buildbot/slaves/ubuntu-x64/svnbuild.sh URL: http://svn.apache.org/viewvc/subversion/trunk/tools/buildbot/slaves/ubuntu-x64/svnbuild.sh?rev=1294455r1=1294454r2=1294455view=diff == --- subversion/trunk/tools/buildbot/slaves/ubuntu-x64/svnbuild.sh (original) +++ subversion/trunk/tools/buildbot/slaves/ubuntu-x64/svnbuild.sh Tue Feb 28 04:32:21 2012 @@ -30,7 +30,7 @@ echo = autogen.sh echo = configure ./configure --enable-javahl --enable-maintainer-mode \ --without-berkeley-db \ - --with-jdk=/usr/lib/jvm/java-6-openjdk/ \ + --with-jdk=/usr/lib/jvm/java-7-openjdk-amd64/ \ Don't we want to stay on JDK 6 so we don't inadvertently pick up new Java 7 features? I saw the build failures on the build box. Do we have other build boxes with JDK 6? Blair
Re: APR hash order
On 2/21/12 5:00 AM, Daniel Shahaf wrote: Philip Martin wrote on Tue, Feb 21, 2012 at 12:32:43 +: The dumpfile order is more interesting. Although we don't specify the dumpfile order until now it has been repeatable, at least when using the same executable/libraries. I can see that this repeatability is useful to an administrator. Rather than fixing the testsuite to ignore dumpfile order changes perhaps we should remove the random behaviour and continue to provide repeatable dumpfiles? This would involve using apr_hash_make_custom rather than apr_hash_make. I don't know whether Instead of apr_hash_make_custom(), couldn't we dump entries in directory order instead? That makes the order a function of the repository mirror dumped, rather than of the software version used. After using git for a while, I've become used to it alphabetically sorting all all output. It's useful when reading a long diff to know how far into it you are, for writing log messages when you want the files listed alphabetically. I'd be more for a solution that puts sorting everywhere. Blair
Time for 1.6.18?
With a fsfs corruption bug found and fixed this week [1], should we cut a 1.6.18? 1.6.17 was tagged 8 months ago. If we don't cut a new release, then we should probably let downstream distributions know about the issue so they can patch their builds. Blair [1] http://svn.apache.org/viewvc?view=revisionamp;revision=1240752
Re: svn commit: r1240755 - /subversion/branches/1.7.x/STATUS
On 02/05/2012 08:08 AM, Daniel Shahaf wrote: stef...@apache.org wrote on Sun, Feb 05, 2012 at 16:03:51 -: Author: stefan2 Date: Sun Feb 5 16:03:51 2012 New Revision: 1240755 URL: http://svn.apache.org/viewvc?rev=1240755view=rev Log: * STATUS: Add r1240752 and vote for it. Modified: subversion/branches/1.7.x/STATUS Modified: subversion/branches/1.7.x/STATUS URL: http://svn.apache.org/viewvc/subversion/branches/1.7.x/STATUS?rev=1240755r1=1240754r2=1240755view=diff == --- subversion/branches/1.7.x/STATUS (original) +++ subversion/branches/1.7.x/STATUS Sun Feb 5 16:03:51 2012 @@ -84,6 +84,18 @@ Candidate changes: Votes: +1: rhuijben, philip + * r1240752 + Workround for a faulty APR truncate() implementation. When rep sharing A bit more info please? What APR platforms/versions are affected? It looks like this was fixed in APR in December 10, 2010 for unix platforms: http://svn.apache.org/viewvc?view=revisionrevision=r100 But it's not in any released version of APR, not even 1.4.5: http://svn.apache.org/repos/asf/apr/apr/tags/1.4.5/file_io/unix/seek.c Will an 'svnadmin verify' or 'svnadmin dump' find this corruption? If one has it, will the standard fsfs repair tool(s) fix it? Blair
Re: svn commit: r1239926 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
On 02/02/2012 03:29 PM, s...@apache.org wrote: Author: stsp Date: Thu Feb 2 23:29:11 2012 New Revision: 1239926 URL: http://svn.apache.org/viewvc?rev=1239926view=rev Log: Add a public libsvn_repos API to set the environment of hook scripts. Modified: subversion/trunk/subversion/include/svn_repos.h subversion/trunk/subversion/libsvn_repos/hooks.c subversion/trunk/subversion/libsvn_repos/repos.c subversion/trunk/subversion/libsvn_repos/repos.h Modified: subversion/trunk/subversion/include/svn_repos.h URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_repos.h?rev=1239926r1=1239925r2=1239926view=diff == --- subversion/trunk/subversion/include/svn_repos.h (original) +++ subversion/trunk/subversion/include/svn_repos.h Thu Feb 2 23:29:11 2012 @@ -764,6 +764,13 @@ const char * svn_repos_post_unlock_hook(svn_repos_t *repos, apr_pool_t *pool); +/** Set the environment that @ repos's hooks will inherit. + * If this function is not called, hooks will run in an empty environment. + * @since New in 1.8. */ Is NULL OK? How is the environment formatted? It's not of the same form as execve(), which I was surprised by. It doesn't look like it's multithreaded safe to set, but should it be? Blair
Re: svn commit: r1239926 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
On 02/02/2012 05:07 PM, Stefan Sperling wrote: On Thu, Feb 02, 2012 at 03:41:48PM -0800, Blair Zajac wrote: It doesn't look like it's multithreaded safe to set, but should it be? I am not sure. Note that this isn't changing the environment of the currently running process. It takes effect in the child process that runs a hook. The intended use is to set the hook environment once, after opening the repository with svn_repos_open(). I've just committed some code that uses this from mod_dav_svn in r1239966. Maybe this example will make the intented use clear, and we can document behaviour WRT thread-safety or make the code thread-safe? I'm reading code too fast today and only noticed after I asked the question that it's being set on the svn_repos_t instead of globally, so documenting it seems fine. I don't imagine people having multiple threads set this. Blair
New warnings from Xcode 4.2's clang
I upgraded my Xcode to 4.2 following the iOS 5 release and then compiled svn 1.7.0 for MacPorts, which updated this weekend. The new clang compiler generates many more, higher level warnings then I've seen before, which is pretty cool. Definitely worth upgrading your Xcode and switching to clang. Here's the warnings in the 1.7.0 build, not including deprecation warnings. subversion/libsvn_delta/compose_delta.c:707:31: warning: comparison of unsigned expression = 0 is always true [-Wtautological-compare] if (ptn_overlap = 0) ~~~ ^ ~ There's a comment for this in our code: /* ### FIXME: ptn_overlap is unsigned, so the if() condition below is always true! Either it should be ' 0', or the code block should be unconditional. See also r842362. */ subversion/libsvn_client/merge.c:4565:9: warning: if statement has empty body [- Wempty-body] ; ^ This is this snippet: if (nbr_skips notify_b-nbr_notifications) /* ### Use RANGELIST as the mergeinfo for all children of ### this path which were not also explicitly ### skipped? */ ; subversion/libsvn_wc/copy.c:618:13: warning: 6 enumeration values not handled in switch: 'svn_wc__db_status_normal', 'svn_wc__db_status_added', 'svn_wc__db_stat us_moved_here'... [-Wswitch-enum] switch (src_status) ^ subversion/libsvn_diff/diff_file.c:2138:3: warning: data argument not used by format string [-Wformat-extra-args] SVN_ERR(svn_stream_printf(btn-output_stream, btn-pool, ^~~~ ./subversion/include/svn_error.h:303:35: note: instantiated from: svn_error_t *svn_err__temp = (expr);\ ^ subversion/libsvn_diff/diff_file.c:2143:49: note: instantiated from: modified_start + 1, modified_length)); ^ This looks ok: SVN_ERR(svn_stream_printf(btn-output_stream, btn-pool, (modified_length == 1 ? %s (% APR_OFF_T_FMT ) : %s (% APR_OFF_T_FMT ,% APR_OFF_T_FMT )), btn-conflict_modified, modified_start + 1, modified_length)); There's a bunch more of these warnings. So all in all, svn 1.7.0 comes out pretty good. None of the above warnings are a bug, even the switch one from a cursory reading. It's nice to see a compiler warn about these. Blair
Re: svn commit: r1176416 - /subversion/trunk/subversion/bindings/javahl/native/JNIUtil.cpp
On Sep 27, 2011, at 7:56 AM, hwri...@apache.org wrote: Author: hwright Date: Tue Sep 27 14:56:56 2011 New Revision: 1176416 Modified: subversion/trunk/subversion/bindings/javahl/native/JNIUtil.cpp URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/JNIUtil.cpp?rev=1176416r1=1176415r2=1176416view=diff == --- subversion/trunk/subversion/bindings/javahl/native/JNIUtil.cpp (original) +++ subversion/trunk/subversion/bindings/javahl/native/JNIUtil.cpp Tue Sep 27 14:56:56 2011 @@ -474,9 +474,9 @@ void JNIUtil::handleSVNError(svn_error_t if (isJavaExceptionThrown()) POP_AND_RETURN_NOTHING(); } - Array stackTraceArray((jobjectArray) env-CallObjectMethod(nativeException, - mid_gst)); - std::vectorjobject oldStackTrace = stackTraceArray.vector(); + Array *stackTraceArray = +new Array((jobjectArray) env-CallObjectMethod(nativeException, mid_gst)); + std::vectorjobject oldStackTrace = stackTraceArray-vector(); // Build the new stack trace elements from the chained errors. std::vectorjobject newStackTrace; @@ -506,6 +506,8 @@ void JNIUtil::handleSVNError(svn_error_t ++i; } + delete stackTraceArray; Hi Hyrum, Out of curiosity, since I'm not familiar with JNI coding, is there a chance of memory leak here if an exception is thrown before the delete? Or is using something like std::auto_ptr a good idea? Blair
Re: svn commit: r1157172 - in /subversion/trunk/subversion: libsvn_client/revert.c tests/cmdline/copy_tests.py
On Aug 12, 2011, at 9:05 AM, s...@apache.org wrote: Author: stsp Date: Fri Aug 12 16:05:15 2011 New Revision: 1157172 URL: http://svn.apache.org/viewvc?rev=1157172view=rev Log: Make 'svn revert' error out if only one side of a move is reverted. I actually find this useful sometimes, to revert one part of the move. For now, 'revert' and always refuses to incompletely revert a move. This will later be extended so that both sides of a move are reverted if no other local mods affect the moved nodes. Later is by 1.8? Blair
Re: Problems compiling 1.7.0 on redhat el4 64bit
On Aug 11, 2011, at 8:32 AM, michael_rytt...@agilent.com michael_rytt...@agilent.com wrote: No luck with that option, still crashing. Try running the process under valgrind to see if it finds anything. Blair
Re: [VOTE]: Default http-client for 1.7 Serf or Neon
On Aug 4, 2011, at 7:52 PM, C. Michael Pilato wrote: On 08/04/2011 09:47 PM, Blair Zajac wrote: On the veto issue, it's odd that Greg is veto a revert of a commit change that originally occurred on trunk and is now sitting on the branch. It does seem odd one can veto the move back to the original default implementation. Just wondering, couldn't we veto the commit that made serf the default? Eh... sounds like politickin'... let's not go there, please. Yeah, I was more wondering than anything else. Blair
Re: svn commit: r1150368 - in /subversion/trunk/subversion/libsvn_client: commit.c commit_util.c
On Jul 24, 2011, at 6:50 AM, rhuij...@apache.org wrote: Author: rhuijben Date: Sun Jul 24 13:50:44 2011 New Revision: 1150368 URL: http://svn.apache.org/viewvc?rev=1150368view=rev Log: Clear iterpools in two error conditions during commit to avoid deleting files with open handles when aborting fs transactions. The txdelta part of this patch looks like a small regression introduced by switching from subpools to dual pool handling. * subversion/libsvn_client/commit.c (svn_client_commit5): Clear iterpool before aborting the edit to flush the most commonly passed scratch pool to the commit processing. This to avoid possible similar problems as that caught in svn_client__do_commit. * subversion/libsvn_client/commit_util.c (svn_client__do_commit): Clear iterpool when svn_wc_transmit_text_deltas3 fails. The txdelta infrastructure doesn't have a well defined abort procedure so the current implementations rely on pool cleanup. But when iterpool isn't cleared here we call the editor's abort before this cleanup runs. Do you mean destroy here, as that's what the commit does? Blair
Re: serf 1.0.0 released
On Jul 15, 2011, at 3:16 PM, Greg Stein wrote: On Fri, Jul 15, 2011 at 18:06, Blair Zajac bl...@orcaware.com wrote: On Jul 15, 2011, at 3:03 PM, Greg Stein wrote: Hi all, I'm pleased to finally announce the 1.0.0 release of serf! The serf 1.0.x series is intended to support Apache Subversion 1.6.x and the upcoming 1.7.x for their supported lifetimes. Nice! So any version of Subversion 1.6.x, assuming it is recompiled, will work with serf 1.0.x? Or do we need to wait for 1.6.18? I honestly don't know. I haven't investigated that, although I believe others have. Just tried, svn 1.6.x doesn't compile against serf 1.0.0 due to configure looking for serf-0 instead of serf-1. If I s/serf-0/serf-1/ g in configure, than it doesn't compile due to the removal of serf_bucket_snapshot(). Given that serf-0 and serf-1 can be simultaneously installed, it doesn't appear necessary for us to make 1.6.x work with serf 1.0.x, but it may be nice for some distributions. For MacPorts, it looks like we'll just provide svn 1.7.x and let Mac OS X provide 1.6.x One other thing. With 0.7.2, make check passed all tests, but 1.0.0 fails 7 tests on Mac OS X 10.6 against MacPorts' provided apr: == Running test_all == FFF.. There were 7 failures: 1) test_serf_connection_request_create: test/test_context.c:166: expected 0 but was 47 2) test_serf_connection_priority_request_create: test/test_context.c: 265: expected 0 but was 47 3) test_serf_closed_connection: test/test_context.c:404: expected 0 but was 47 4) test_serf_setup_proxy: test/test_context.c:505: expected 0 but was 47 5) test_keepalive_limit_one_by_one: test/test_context.c:656: expected 0 but was 47 6) test_keepalive_limit_one_by_one_and_burst: test/test_context.c:810: expected 0 but was 47 7) test_serf_progress_callback: test/test_context.c:932: expected 0 but was 47 Is there anything different I should do, or should the unit test just work? Blair
Re: serf 1.0.0 released
On Jul 15, 2011, at 3:03 PM, Greg Stein wrote: Hi all, I'm pleased to finally announce the 1.0.0 release of serf! The serf 1.0.x series is intended to support Apache Subversion 1.6.x and the upcoming 1.7.x for their supported lifetimes. Nice! So any version of Subversion 1.6.x, assuming it is recompiled, will work with serf 1.0.x? Or do we need to wait for 1.6.18? Blair
Re: svn commit: r1146513 - /subversion/branches/1.7.x/STATUS
On Jul 13, 2011, at 4:22 PM, danie...@apache.org wrote: Author: danielsh Date: Wed Jul 13 23:22:45 2011 New Revision: 1146513 URL: http://svn.apache.org/viewvc?rev=1146513view=rev Log: Withdraw this before I set a record on the number of vetoes. [ If anyone would be +1 on a backport for the whole feature, feel free to indicate as much in reply to this email. ] I'm +1 on the concept if you can get it done before it's too late. Blair
Re: [PROPOSAL] Create the 1.7.x branch. Like, now.
On Jul 12, 2011, at 7:42 AM, Hyrum K Wright wrote: One question that I have is regarding housekeeping. Do we have any actions which should be done on trunk (file renames, whitespace cleaning, mergeinfo pruning, etc), which will improve our experience when merging to the branch? I took my OCD pass through the code :) Blair
Re: svn commit: r1145716 - in /subversion/trunk/subversion/include: svn_fs.h svn_repos.h
On Jul 12, 2011, at 12:05 PM, Johan Corveleyn wrote: On Tue, Jul 12, 2011 at 8:42 PM, bl...@apache.org wrote: * * Invoke svn_repos_node_from_baton() on @a edit_baton to obtain the root - * node afterwards. + * node afterwords. sp? (not that I have such a high OCD level to make me review every spelling-changing commit in detail, but for some reason I scrolled down here :-)) Thanks, fixed in r1145773. Blair
Re: svn commit: r1144530 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
On Jul 8, 2011, at 3:44 PM, danie...@apache.org wrote: Author: danielsh Date: Fri Jul 8 22:44:52 2011 New Revision: 1144530 URL: http://svn.apache.org/viewvc?rev=1144530view=rev Log: Repeat r1143899 elsewhere in the same function. * subversion/libsvn_fs_fs/fs_fs.c (commit_body): Creating a revprops shard has always been orthogonal of packing, since we have not implemented packing of non-completed shards or just-committed revisions. Accordingly, replace an if() with an assert(). Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1144530r1=1144529r2=1144530view=diff == --- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original) +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Fri Jul 8 22:44:52 2011 @@ -6509,8 +6509,7 @@ commit_body(void *baton, apr_pool_t *poo new_dir, pool)); } - if (ffd-format SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT || - new_rev = ffd-min_unpacked_revprop) + assert(! is_packed_revprop(cb-fs, new_rev)); Wouldn't an SVN_ERR_ASSERT() be better here? Blair
Re: svn commit: r1144530 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
On 7/8/11 4:34 PM, Daniel Shahaf wrote: Blair Zajac wrote on Fri, Jul 08, 2011 at 15:50:17 -0700: On Jul 8, 2011, at 3:44 PM, danie...@apache.org wrote: +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Fri Jul 8 22:44:52 2011 @@ -6509,8 +6509,7 @@ commit_body(void *baton, apr_pool_t *poo new_dir, pool)); } - if (ffd-format SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT || - new_rev= ffd-min_unpacked_revprop) + assert(! is_packed_revprop(cb-fs, new_rev)); Wouldn't an SVN_ERR_ASSERT() be better here? Works for me. What about those other instances --- in write_change_entry() and recover_find_max_ids()? +1. Blair
Re: svn commit: r1142026 - in /subversion/branches/svn_mutex/subversion: include/private/svn_mutex.h libsvn_subr/svn_mutex.c
On Jul 1, 2011, at 12:04 PM, stef...@apache.org wrote:
Author: stefan2
Date: Fri Jul 1 19:04:29 2011
New Revision: 1142026
URL: http://svn.apache.org/viewvc?rev=1142026view=rev
Log:
Introduce the svn_mutex API and implement it
* subversion/include/private/svn_mutex.h
new private API header
* subversion/libsvn_subr/svn_mutex.c
implement the new API
Added:
subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h
subversion/branches/svn_mutex/subversion/libsvn_subr/svn_mutex.c
Added: subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h
URL:
http://svn.apache.org/viewvc/subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h?rev=1142026view=auto
==
--- subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h
(added)
+++ subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h Fri
Jul 1 19:04:29 2011
@@ -0,0 +1,82 @@
+/**
+ * @copyright
+ *
+ *Licensed to the Apache Software Foundation (ASF) under one
+ *or more contributor license agreements. See the NOTICE file
+ *distributed with this work for additional information
+ *regarding copyright ownership. The ASF licenses this file
+ *to you under the Apache License, Version 2.0 (the
+ *License); you may not use this file except in compliance
+ *with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *Unless required by applicable law or agreed to in writing,
+ *software distributed under the License is distributed on an
+ *AS IS BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *KIND, either express or implied. See the License for the
+ *specific language governing permissions and limitations
+ *under the License.
+ *
+ * @endcopyright
+ *
+ * @file svn_mutex.h
+ * @brief Strutures and functions for mutual exclusion
+ */
+
+#ifndef SVN_MUTEX_H
+#define SVN_MUTEX_H
+
+#include apr_thread_mutex.h
+#include svn_error.h
+
+#ifdef __cplusplus
+extern C {
+#endif /* __cplusplus */
+
+/**
+ * This is a simple wrapper around @c apr_thread_mutex_t and will be a
+ * valid identifier even if APR does not support threading.
+ */
+typedef struct svn_mutex__t
+{
+#if APR_HAS_THREADS
+
+ /** A mutex for synchronization between threads. It may be NULL, in
+ * which case no synchronization will take place. The latter is useful,
+ * if implement some functionality where synchronization is optional.
+ */
+ apr_thread_mutex_t *mutex;
+
+#endif
+} svn_mutex__t;
+
+/** Initialize the @a *mutex with a lifetime defined by @a pool, if
+ * @a enable_mutex is TRUE and with @c NULL otherwise. If @a enable_mutex
+ * is set but threading is not supported by APR, this function returns an
+ * @c APR_ENOTIMPL error.
The NULL part is a little confusing, it took a couple of reads to see the NULL
was associated with *mutex. Suggest changing it so something like
/** If @a enable_mutex is TRUE, initialize the @a *mutex with a lifetime
* defined by @a pool, otherwise initialize it with @c NULL.
How would we use enable_mutex? Seems like a compile time constant, not a
runtime one.
+ */
+svn_error_t *
+svn_mutex__init(svn_mutex__t *mutex,
+svn_boolean_t enable_mutex,
+apr_pool_t *pool);
+
+/** Acquire the @a mutex, if that has been enabled in @ref svn_mutex__init.
+ * Make sure to call @ref svn_mutex__unlock some time later in the same
+ * thread to release the mutex again. Recursive locking are not supported.
+ */
+svn_error_t *
+svn_mutex__lock(svn_mutex__t mutex);
+
+/** Release the @a mutex, previously acquired using @ref svn_mutex__lock
+ * that has been enabled in @ref svn_mutex__init.
+ */
+svn_error_t *
+svn_mutex__unlock(svn_mutex__t mutex,
+ svn_error_t *err);
What is the input err for?
Blair
