Re: svn commit: r1057088 - in /subversion/trunk/subversion: libsvn_fs_base/bdb/env.c libsvn_ra_svn/cyrus_auth.c libsvn_subr/io.c libsvn_subr/sqlite.c
danie...@apache.org writes: Author: danielsh Date: Mon Jan 10 06:03:30 2011 New Revision: 1057088 URL: http://svn.apache.org/viewvc?rev=1057088view=rev Log: Initialize svn_atomic_t's to zero, per svn_atomic__init_once(). That is redundant, strict speaking, because static variables are initialised to zero if not explicitly initialised to something else. Modified: subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c?rev=1057088r1=1057087r2=1057088view=diff == --- subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c (original) +++ subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c Mon Jan 10 06:03:30 2011 @@ -54,8 +54,9 @@ * in atexit processing, at which point we are already running in * single threaded mode. */ -volatile svn_atomic_t svn_ra_svn__sasl_status; +volatile svn_atomic_t svn_ra_svn__sasl_status = 0; That is a static variable, explicitly marking it static might be more useful than explicitly marking it 0. +/* Initialized by svn_ra_svn__sasl_common_init(). */ static volatile svn_atomic_t sasl_ctx_count; That comment is wrong, sasl_ctx_count is static and so is initialised to 0. -- Philip
Re: SQLite and callbacks
On Tue, Nov 30, 2010 at 12:33:38PM +, Philip Martin wrote: Hyrum K. Wright hyrum_wri...@mail.utexas.edu writes: Stefan's patch to make a recursive proplist much more performant highlights the great benefit that our sqlite-backed storage can have. However, he reverted it due to concerns about the potential for database contention. The theory was that the callback might try and call additional wc functions to get more information, and such nested statements weren't healthy for sqlite. We talked about it for a bit in IRC this morning, and the picture raised by this issue was quite dire. It's not the nested statements that are the problem, it's extending the duration of the select. The callback is to the application so it could be updating a GUI, interacting with the user, etc. and so could take a much greater length of time than other selects. An SQLite read blocks SQlite writes by other threads/processes, so the long lived select may cause writes to fail that would otherwise succeed. (We have a busy timeout of 10 seconds so that's the sort of time that becomes a problem.) It's related to the problem of querying the working copy while it is locked. In 1.6 it was possible to run status, info, propget, etc. while an update was in progress. In 1.7 that's no longer possible, all the read operations fail. If we decide that this change in behaviour is acceptable then the long-lived select isn't really a problem, it simply stops the operations like update from starting. If we want the 1.6 behaviour then the long-lived select becomes more of a problem, as it makes update much more likely to fail part way through. The callback is not necessarily wrong, it depends what sort of behaviour we are trying to provide. I think we should review our requirements for callbacks again. recap In 1.7 we have two layers of locking. One is the sqlite locking, which guarantees meta data consistency in wc.db. The other is working copy write locks (svn_wc__db_wclock_obtain()) which are obtained at the start of long-running operations which modify the working copy (e.g. commit, update, merge, patch, ...). The problem we're facing is that we want to allow read-only operations (e.g. status, proplist, ...) to run concurrently with N other read-only operations and at most one write operation. For instance, the user might have TortoiseSVN and some IDE SVN plugin running while working with the CLI client on the same working copy. The CLI client should be able to perform write operations while the other clients periodically poll the WC status to update their icons. The WC write lock will not pose a problem here since it is only taken by writers of which there must at most be one at a time (for a given subtree of the WC). Readers won't attempt to acquire this lock. The sqlite locks on the other hand creates a problem during wc.db queries. A writer blocks all readers, and any active reader will block the writer (see http://sqlite.org/atomiccommit.html). Our solution to this problem is to keep the queries short, so that sqlite locking will not cause long-running read or write operations to stall or fail. /recap The problem with callbacks is that they can in theory take a long amount of time to complete. So when running callbacks while an sqlite query is running, the sqlite query could take a long time to complete. We can solve this problem by never calling back to the client while an sqlite transaction is in progress. However this has an impact on performance. Even if retrieving the desired information is possible with a single query, we have to run multiple queries to retrieve the information in chunks (e.g. per directory) so we can pass it to the callback. The other option is to place restrictions on what a client callback is allowed to do, depending on which locking context the callback runs in. A locking context implies restrictions on what the callback is allowed to do. E.g. we could say that the proplist callback runs within a wc.db locking context. This would be a highly restrictive context, because an sqlite transaction is in progress. The only action allowed is to consume the data passed in (e.g. print it or store it to a temporary file or a buffer for later processing). The callback should complete its task as quickly as possible to avoid disturbing other SVN clients. No calls to any function in libsvn_client or libsvn_wc would be allowed from this context. If the callback doesn't comply with this restriction, there is a risk of undefined behaviour of the entire system. This means that a badly behaving client can disturb user experience. But it also allows us to be maximally efficient. If backwards compatibility is a concern, we could allow callbacks to be called in this context only for API calls that exist in 1.7 and newer. Backwards-compat code would use a different implementation with a less restrictive locking context for these callbacks. This would allow existing
Re: [l10n] Translation status report for trunk r1055292
Hi, Hyrum K Wright writes: I'm working on getting a web-based translation system installed on ASF hardware for use by Subversion, and possibly other projects, and will report progress here as it happens. I am in the unfortunate position of being a monoglot, and hence unable to actually do the translation, but I would like to lower the barrier for other people to contribute. I believe Transifex has been quite successful in this regard. I'm considering using it for translations in git.git too. -- Ram
Re: [l10n] Translation status report for trunk r1055292
On Wed, Jan 12, 2011 at 7:50 AM, Ramkumar Ramachandra artag...@gmail.com wrote: Hi, Hyrum K Wright writes: I'm working on getting a web-based translation system installed on ASF hardware for use by Subversion, and possibly other projects, and will report progress here as it happens. I am in the unfortunate position of being a monoglot, and hence unable to actually do the translation, but I would like to lower the barrier for other people to contribute. I believe Transifex has been quite successful in this regard. I'm considering using it for translations in git.git too. Yes, that's the system that Stefan Küng recommended to me, as they are using it for TortoiseSVN. I've been tempted to just register Subversion at the hosted Tranifex site and be done with it, but I'd first like to experiment with an ASF-hosted version, which could hopefully integrate with the ASF LDAP for automatic commits and such. We'll see how far that gets. :) -Hyrum
headers in the body Re: [l10n] Translation status report for trunk r1057984
SVN DEV wrote on Wed, Jan 12, 2011 at 04:21:52 +: X-Mailer: l10n-report.py r1055713 Reply-To: dev@subversion.apache.org Mail-Followup-To: dev@subversion.apache.org Auto-Submitted: auto-generated Translation status report for tr...@r1057984 Looks like something inserts a spurious blank link before the X-Mailer header. However, when I run it locally it doesn't do that. Hyrum, any ideas? Do you have any local mods in your live script instance?
Re: headers in the body Re: [l10n] Translation status report for trunk r1057984
On 12.01.2011 15:01, Daniel Shahaf wrote: SVN DEV wrote on Wed, Jan 12, 2011 at 04:21:52 +: X-Mailer: l10n-report.py r1055713 Reply-To: dev@subversion.apache.org Mail-Followup-To: dev@subversion.apache.org Auto-Submitted: auto-generated Translation status report for tr...@r1057984 Looks like something inserts a spurious blank link before the X-Mailer header. However, when I run it locally it doesn't do that. Hyrum, any ideas? Do you have any local mods in your live script instance? Wherever that blank is inserted, it surely stems from constructing the message directly instead of using the email module. -- Brane
Re: svn commit: r1057088 - in /subversion/trunk/subversion: libsvn_fs_base/bdb/env.c libsvn_ra_svn/cyrus_auth.c libsvn_subr/io.c libsvn_subr/sqlite.c
Philip Martin wrote on Wed, Jan 12, 2011 at 11:01:12 +: danie...@apache.org writes: Author: danielsh Date: Mon Jan 10 06:03:30 2011 New Revision: 1057088 URL: http://svn.apache.org/viewvc?rev=1057088view=rev Log: Initialize svn_atomic_t's to zero, per svn_atomic__init_once(). That is redundant, strict speaking, because static variables are initialised to zero if not explicitly initialised to something else. I didn't remember that. I don't mind to revert the change if people are comfortable with our coding style relying on remembering which kinds of variables C does or doesn't initialize to zero when they're not explicitly initialized... (i.e., defensive coding) Modified: subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c?rev=1057088r1=1057087r2=1057088view=diff == --- subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c (original) +++ subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c Mon Jan 10 06:03:30 2011 @@ -54,8 +54,9 @@ * in atexit processing, at which point we are already running in * single threaded mode. */ -volatile svn_atomic_t svn_ra_svn__sasl_status; +volatile svn_atomic_t svn_ra_svn__sasl_status = 0; That is a static variable, explicitly marking it static might be more useful than explicitly marking it 0. Actually, it's called from svnserve's cyrus_init(). Feel free to go ahead and make any change you like --- I made a horizontal change and I don't claim to be expertly familiar with each of the variables I touched. +/* Initialized by svn_ra_svn__sasl_common_init(). */ static volatile svn_atomic_t sasl_ctx_count; That comment is wrong, sasl_ctx_count is static and so is initialised to 0. Well, s/Initialized/Set to non-zero/ then? You know what I meant, right? :) -- Philip
Re: SQLite and callbacks
Nothing to add to the immediate discussion at this time. Just wanted to note that we have essentially the same problems to deal with in the BDB backend. The solution we've taken there is don't drive public API callbacks from inside BDB transactions. It's not always the best for performance, but the FS API is such a high profile API for third-party consumers (very unlike the WC API in that respect, I'd imagine) that simplicity of the API rules was valued. signature.asc Description: OpenPGP digital signature
Re: svn commit: r1057088 - in /subversion/trunk/subversion: libsvn_fs_base/bdb/env.c libsvn_ra_svn/cyrus_auth.c libsvn_subr/io.c libsvn_subr/sqlite.c
On 12.01.2011 15:12, Daniel Shahaf wrote: Philip Martin wrote on Wed, Jan 12, 2011 at 11:01:12 +: danie...@apache.org writes: Author: danielsh Date: Mon Jan 10 06:03:30 2011 New Revision: 1057088 URL: http://svn.apache.org/viewvc?rev=1057088view=rev Log: Initialize svn_atomic_t's to zero, per svn_atomic__init_once(). That is redundant, strict speaking, because static variables are initialised to zero if not explicitly initialised to something else. I didn't remember that. I don't mind to revert the change if people are comfortable with our coding style relying on remembering which kinds of variables C does or doesn't initialize to zero when they're not explicitly initialized... (i.e., defensive coding) 's true that static (and in fact all global) storage is default-inited to 0 ... but there's no harm in putting the initializer there. IMO it's better to have it, if only for the sake of clarity. -- Brane
Re: svn commit: r1057088 - in /subversion/trunk/subversion: libsvn_fs_base/bdb/env.c libsvn_ra_svn/cyrus_auth.c libsvn_subr/io.c libsvn_subr/sqlite.c
On 01/12/2011 09:54 AM, Branko Čibej wrote: 's true that static (and in fact all global) storage is default-inited to 0 ... but there's no harm in putting the initializer there. IMO it's better to have it, if only for the sake of clarity. You took the words right outta my ... Outbox. I take a similar approach to the use of parentheses around conditionals. Maybe you don't *need* them due to order of operations rules, but the additional clarity is worth it to me. -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Distributed Development On Demand signature.asc Description: OpenPGP digital signature
Re: headers in the body Re: [l10n] Translation status report for trunk r1057984
On Wed, Jan 12, 2011 at 8:14 AM, Branko Čibej br...@xbc.nu wrote: On 12.01.2011 15:01, Daniel Shahaf wrote: SVN DEV wrote on Wed, Jan 12, 2011 at 04:21:52 +: X-Mailer: l10n-report.py r1055713 Reply-To: dev@subversion.apache.org Mail-Followup-To: dev@subversion.apache.org Auto-Submitted: auto-generated Translation status report for tr...@r1057984 Looks like something inserts a spurious blank link before the X-Mailer header. However, when I run it locally it doesn't do that. Hyrum, any ideas? Do you have any local mods in your live script instance? I don't have any local mods to the script. The cron job just pulls whatever is in trunk, and runs that. Wherever that blank is inserted, it surely stems from constructing the message directly instead of using the email module. Heh. A good suggestion for improvement, indeed. -Hyrum
Re: [PATCH] Force scratch pool usage in the calls to svn_uri_condense_targets/svn_dirent_condense_targets
On Mon, Dec 27, 2010 at 04:05:02PM +0530, vijay wrote: Hi, We can use SCRATCH_POOL in the calls to svn_uri_condense_targets/svn_dirent_condense_targets wherever it is applicable. It will save few bits of memory. The attached patch forces SCRATCH_POOL usage in few places. Thanks Regards, Vijayaguru Thank you, committed in r1058237. Stefan
Re: headers in the body Re: [l10n] Translation status report for trunk r1057984
On 12.01.2011 16:47, Hyrum K Wright wrote: On Wed, Jan 12, 2011 at 8:14 AM, Branko Čibej br...@xbc.nu wrote: On 12.01.2011 15:01, Daniel Shahaf wrote: SVN DEV wrote on Wed, Jan 12, 2011 at 04:21:52 +: X-Mailer: l10n-report.py r1055713 Reply-To: dev@subversion.apache.org Mail-Followup-To: dev@subversion.apache.org Auto-Submitted: auto-generated Translation status report for tr...@r1057984 Looks like something inserts a spurious blank link before the X-Mailer header. However, when I run it locally it doesn't do that. Hyrum, any ideas? Do you have any local mods in your live script instance? I don't have any local mods to the script. The cron job just pulls whatever is in trunk, and runs that. Wherever that blank is inserted, it surely stems from constructing the message directly instead of using the email module. Heh. A good suggestion for improvement, indeed. Doing that now, and I found the reason for that extra line -- it comes from the output of svnversion. Could be a difference between 1.6 and 1.7, I suppose. -- Brane
Re: headers in the body Re: [l10n] Translation status report for trunk r1057984
On 12.01.2011 18:43, Branko Čibej wrote: On 12.01.2011 16:47, Hyrum K Wright wrote: On Wed, Jan 12, 2011 at 8:14 AM, Branko Čibej br...@xbc.nu wrote: On 12.01.2011 15:01, Daniel Shahaf wrote: SVN DEV wrote on Wed, Jan 12, 2011 at 04:21:52 +: X-Mailer: l10n-report.py r1055713 Reply-To: dev@subversion.apache.org Mail-Followup-To: dev@subversion.apache.org Auto-Submitted: auto-generated Translation status report for tr...@r1057984 Looks like something inserts a spurious blank link before the X-Mailer header. However, when I run it locally it doesn't do that. Hyrum, any ideas? Do you have any local mods in your live script instance? I don't have any local mods to the script. The cron job just pulls whatever is in trunk, and runs that. Wherever that blank is inserted, it surely stems from constructing the message directly instead of using the email module. Heh. A good suggestion for improvement, indeed. Doing that now, and I found the reason for that extra line -- it comes from the output of svnversion. Could be a difference between 1.6 and 1.7, I suppose. r1058257, local testing seems OK, but please double-check. -- Brane
Re: headers in the body Re: [l10n] Translation status report for trunk r1057984
On Wed, Jan 12, 2011 at 12:01 PM, Branko Čibej br...@xbc.nu wrote: On 12.01.2011 18:43, Branko Čibej wrote: On 12.01.2011 16:47, Hyrum K Wright wrote: On Wed, Jan 12, 2011 at 8:14 AM, Branko Čibej br...@xbc.nu wrote: On 12.01.2011 15:01, Daniel Shahaf wrote: SVN DEV wrote on Wed, Jan 12, 2011 at 04:21:52 +: X-Mailer: l10n-report.py r1055713 Reply-To: dev@subversion.apache.org Mail-Followup-To: dev@subversion.apache.org Auto-Submitted: auto-generated Translation status report for tr...@r1057984 Looks like something inserts a spurious blank link before the X-Mailer header. However, when I run it locally it doesn't do that. Hyrum, any ideas? Do you have any local mods in your live script instance? I don't have any local mods to the script. The cron job just pulls whatever is in trunk, and runs that. Wherever that blank is inserted, it surely stems from constructing the message directly instead of using the email module. Heh. A good suggestion for improvement, indeed. Doing that now, and I found the reason for that extra line -- it comes from the output of svnversion. Could be a difference between 1.6 and 1.7, I suppose. r1058257, local testing seems OK, but please double-check. Testing on people.apache.org (the cron host) looks good. Thanks! -Hyrum
Re: issue 3486 should be reopened?
On Mon, Nov 29, 2010 at 09:14:09AM +1100, Gavin Beau Baumanis wrote: On 27/11/2010, at 12:46 AM, Stefan Sperling wrote: On Fri, Nov 26, 2010 at 02:45:07PM +0100, Stefan Sperling wrote: On Fri, Nov 26, 2010 at 02:40:07PM +0100, Peter Parker wrote: On 13.11.2010 16:07, Daniel Shahaf wrote: Daniel Shahaf wrote on Sat, Nov 13, 2010 at 14:35:13 +0200: Attached the non-windows version of your script I used for testing. [ SNIP ] So as Daniel has same error on trunk, is it possible to reopen issue 3486 ? Best regards, Peter Yes please. That assertion should never be hit. Thanks for reporting this. Stefan PS: Is your name really Peter Parker? That's awesome! Hmmm.. My spidey-senses are telling me There's a bug in there, somehwere. The bug has been fixed and the fix has been nominated for backport to 1.6.16. See http://subversion.tigris.org/issues/show_bug.cgi?id=3486#desc7 Thanks for reporting this, Peter! Stefan
[PATCH] Add include of svn_dirent_uri.h to mod_authz_svn.c
This is a follow up to a previously submitted patch which was applied, see http://mail-archives.apache.org/mod_mbox/subversion-dev/201011.mbox/%3c87fwvigwgr@stat.home.lan%3e and r1030536 r1030540. It seems that patch was missing: #include svn_dirent_uri.h Without this, we get a warning: subversion/mod_authz_svn/mod_authz_svn.c:161: warning: assignment makes pointer from integer without a cast and subsequent breakage on 64-bit platforms where the return pointer is truncated. I'm sorry for this oversight in the original patch. Patch against trunk: Index: subversion/mod_authz_svn/mod_authz_svn.c === --- subversion/mod_authz_svn/mod_authz_svn.c(revision 1058279) +++ subversion/mod_authz_svn/mod_authz_svn.c(working copy) @@ -42,6 +42,7 @@ #include svn_config.h #include svn_string.h #include svn_repos.h +#include svn_dirent_uri.h extern module AP_MODULE_DECLARE_DATA authz_svn_module; -- Nick Piper MEng MIET RHCE| #define Joint Lead Architect 250 Brook Drive, Green Park, Reading RG2 6UA | United Kingdom nick.pi...@logica.com | www.logica.com Logica UK Limited, registered in England Wales (registered number 947968) Registered Office: 250 Brook Drive, Green Park, Reading RG2 6UA, United Kingdom Think green - keep it on the screen. This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.
Re: issue 3486 should be reopened?
Thanks Stefan, I will remove it from my watch list. On 13/01/2011, at 6:07 AM, Stefan Sperling wrote: On Mon, Nov 29, 2010 at 09:14:09AM +1100, Gavin Beau Baumanis wrote: On 27/11/2010, at 12:46 AM, Stefan Sperling wrote: On Fri, Nov 26, 2010 at 02:45:07PM +0100, Stefan Sperling wrote: On Fri, Nov 26, 2010 at 02:40:07PM +0100, Peter Parker wrote: On 13.11.2010 16:07, Daniel Shahaf wrote: Daniel Shahaf wrote on Sat, Nov 13, 2010 at 14:35:13 +0200: Attached the non-windows version of your script I used for testing. [ SNIP ] So as Daniel has same error on trunk, is it possible to reopen issue 3486 ? Best regards, Peter Yes please. That assertion should never be hit. Thanks for reporting this. Stefan PS: Is your name really Peter Parker? That's awesome! Hmmm.. My spidey-senses are telling me There's a bug in there, somehwere. The bug has been fixed and the fix has been nominated for backport to 1.6.16. See http://subversion.tigris.org/issues/show_bug.cgi?id=3486#desc7 Thanks for reporting this, Peter! Stefan
Re: svn commit: r1058308 - /subversion/site/publish/docs/release-notes/1.7.html
s...@apache.org wrote on Wed, Jan 12, 2011 at 20:36:34 -: Author: stsp Date: Wed Jan 12 20:36:33 2011 New Revision: 1058308 URL: http://svn.apache.org/viewvc?rev=1058308view=rev Log: * publish/docs/release-notes/1.7.html (atomic-revprops): Mention that this is also a client-side fix (suggested by danielsh). Provide a link to the pre-revprop-change hook script by philip which fixes the issue #3546 race even for pre-1.7 svnsync clients. Two comments: * The entire locking discussion applies unmodified to svnrdump, since it shares the locking logic with svnsync: namely, 'svnrdump load' is susceptible to the same race condition with ≤1.6 servers. That logic is now svn_ra__get_operational_lock(). (Its docstring doesn't currently mention the race condition with old servers; I'll get to it.) * The difference that's supposed to cause Philip's script to work as advertised is that svn_repos_fs_change_rev_prop4() uses the same propvalue to compute the ACTION parameter and to pass as the old propvalue to the FS for atomicity. I'm too sleepy right now to determine whether Philip's script will actually work as advertised given this server-side change, so just throwing it out there for now. Modified: subversion/site/publish/docs/release-notes/1.7.html Modified: subversion/site/publish/docs/release-notes/1.7.html URL: http://svn.apache.org/viewvc/subversion/site/publish/docs/release-notes/1.7.html?rev=1058308r1=1058307r2=1058308view=diff == --- subversion/site/publish/docs/release-notes/1.7.html (original) +++ subversion/site/publish/docs/release-notes/1.7.html Wed Jan 12 20:36:33 2011 @@ -551,16 +551,21 @@ a more useful error message on temporary /div !-- http-redirects -- div class=h3 id=atomic-revprops -h3Atomic revprop changes (emserver/em) +h3Atomic revprop changes (emclient/em and emserver/em) a class=sectionlink href=#atomic-revprops title=Link to this sectionpara;/a /h3 -pRevprop changes are now handled atomically on the server. +pRevprop changes are now handled atomically. This fixes a known race condition in the locking algorithm used by codesvnsync/code (see a href=http://subversion.tigris.org/issues/show_bug.cgi?id=3546; - issue #3546/a.)/p + issue #3546/a.)./p +pWith a 1.7 server, it is possible to fix the svnsync race condition +even for pre-1.7 svnsync clients by installing the pre-revprop-change hook +script given in +a href=http://subversion.tigris.org/issues/show_bug.cgi?id=3546#desc12; + comment 12 of issue #3546/a./p /div !-- atomic-revprops --
