Re: Race in svn_atomic_namespace__create
On Tue, Oct 30, 2012 at 12:11 AM, Philip Martin philip.mar...@wandisco.comwrote: Stefan Fuhrmann stefan.fuhrm...@wandisco.com writes: On Mon, Oct 29, 2012 at 10:46 PM, Philip Martin philip.mar...@wandisco.comwrote: Philip Martin philip.mar...@wandisco.com writes: Philip Martin philip.mar...@wandisco.com writes: I can't see any order in which we can do attach/create that doesn't have a similar race. I think the best solution is a short loop trying attach-create a few times before giving up. I've committed a loop in r1403463. That doesn't fix the race but it is now very unlikely to fail. The creation code is protected by a repo-global lock/unlock pair. So, in theory, there should be no race condition. Which lock and where? Does this lock out other processes? Lines 266 to 292 implement the lock. It first takes out the process-local and and then the global lock (a repo-local file lock). L416 acquires the lock in svn_atomic_namespace__create and L453 releases it. I've just observed the same failure with the looping code. I'm not sure what is wrong. I suppose there is a window during the creation process where the file exists, so the create fails, but the memory is not yet ready, so the attach also fails. If one process is in this state another process might loop around 10 times and have both create and attach fail. Perhaps a short and/or random delay would help? It's on my TODO list to identify the root cause of this issue. I think it must be the window between apr_file_open( APR_EXCL ) and mmap( MAP_SHARED ) in apr_shm_create. During that period any other process will see both apr_shm_create and apr_shm_attach fail. But that would imply that your process lock isn't working. It is well possible that the locking logic is faulty. Maybe, there should be a regression test that tries concurrent initialization. -- Stefan^2. -- Certified Supported Apache Subversion Downloads: * http://www.wandisco.com/subversion/download *
RE: svn commit: r1403588 - in /subversion/trunk/subversion: mod_dav_svn/reports/update.c tests/cmdline/update_tests.py
-Original Message- From: cmpil...@apache.org [mailto:cmpil...@apache.org] Sent: dinsdag 30 oktober 2012 01:24 To: comm...@subversion.apache.org Subject: svn commit: r1403588 - in /subversion/trunk/subversion: mod_dav_svn/reports/update.c tests/cmdline/update_tests.py Author: cmpilato Date: Tue Oct 30 00:23:38 2012 New Revision: 1403588 URL: http://svn.apache.org/viewvc?rev=1403588view=rev Log: Tweak the server-side logic which validates update report source and target revision values so that they always get checked for validity, not only when doing non-client-pegged updates. * subversion/mod_dav_svn/reports/update.c (validate_input_revision): New helper function. (dav_svn__update_report): Always query the youngest FS revision, and use it (via validate_input_revision()) to raise errors when the client requests an update to a revision younger than is available or reports that it has objects whose revision is greater than the youngest. How hard would it be to port this one back to 1.7? snip Modified: subversion/trunk/subversion/tests/cmdline/update_tests.py URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/ update_tests.py?rev=1403588r1=1403587r2=1403588view=diff == --- subversion/trunk/subversion/tests/cmdline/update_tests.py (original) +++ subversion/trunk/subversion/tests/cmdline/update_tests.py Tue Oct 30 00:23:38 2012 @@ -5516,7 +5516,7 @@ def update_to_HEAD_plus_1(sbox): svntest.actions.run_and_verify_update(wc_dir, None, None, None, -.*No such revision, +.*E160006, If possible, please also keep some part(s) of an error message in the regex, to make the testsuite verify that we keep failing for the same reason. (In this case it checks for SVN_ERR_FS_NO_SUCH_REVISION, which already tells almost the whole story, but I don't see that in the test suite code) Bert None, None, None, None, None, wc_dir, '-r', '2')
Re: svn commit: r1403588 - in /subversion/trunk/subversion: mod_dav_svn/reports/update.c tests/cmdline/update_tests.py
On 10/30/2012 06:46 AM, Bert Huijben wrote: -Original Message- From: cmpil...@apache.org [mailto:cmpil...@apache.org] Sent: dinsdag 30 oktober 2012 01:24 To: comm...@subversion.apache.org Subject: svn commit: r1403588 - in /subversion/trunk/subversion: mod_dav_svn/reports/update.c tests/cmdline/update_tests.py Author: cmpilato Date: Tue Oct 30 00:23:38 2012 New Revision: 1403588 URL: http://svn.apache.org/viewvc?rev=1403588view=rev Log: Tweak the server-side logic which validates update report source and target revision values so that they always get checked for validity, not only when doing non-client-pegged updates. * subversion/mod_dav_svn/reports/update.c (validate_input_revision): New helper function. (dav_svn__update_report): Always query the youngest FS revision, and use it (via validate_input_revision()) to raise errors when the client requests an update to a revision younger than is available or reports that it has objects whose revision is greater than the youngest. How hard would it be to port this one back to 1.7? I don't think it would be hard at all. I certainly plan to make the attempt today. Modified: subversion/trunk/subversion/tests/cmdline/update_tests.py URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/ update_tests.py?rev=1403588r1=1403587r2=1403588view=diff == --- subversion/trunk/subversion/tests/cmdline/update_tests.py (original) +++ subversion/trunk/subversion/tests/cmdline/update_tests.py Tue Oct 30 00:23:38 2012 @@ -5516,7 +5516,7 @@ def update_to_HEAD_plus_1(sbox): svntest.actions.run_and_verify_update(wc_dir, None, None, None, -.*No such revision, +.*E160006, If possible, please also keep some part(s) of an error message in the regex, to make the testsuite verify that we keep failing for the same reason. (In this case it checks for SVN_ERR_FS_NO_SUCH_REVISION, which already tells almost the whole story, but I don't see that in the test suite code) Good suggestion. The No such revision ... is now (for DAV servers) No such [target|reported] revision Would .*No such.*revision serve your desires here? -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Enterprise Cloud Development signature.asc Description: OpenPGP digital signature
RE: svn commit: r1403588 - in /subversion/trunk/subversion: mod_dav_svn/reports/update.c tests/cmdline/update_tests.py
-Original Message- From: C. Michael Pilato [mailto:cmpil...@collab.net] Sent: dinsdag 30 oktober 2012 13:01 To: Bert Huijben Cc: dev@subversion.apache.org Subject: Re: svn commit: r1403588 - in /subversion/trunk/subversion: mod_dav_svn/reports/update.c tests/cmdline/update_tests.py On 10/30/2012 06:46 AM, Bert Huijben wrote: -Original Message- From: cmpil...@apache.org [mailto:cmpil...@apache.org] Sent: dinsdag 30 oktober 2012 01:24 To: comm...@subversion.apache.org Subject: svn commit: r1403588 - in /subversion/trunk/subversion: mod_dav_svn/reports/update.c tests/cmdline/update_tests.py Author: cmpilato Date: Tue Oct 30 00:23:38 2012 New Revision: 1403588 URL: http://svn.apache.org/viewvc?rev=1403588view=rev Log: Tweak the server-side logic which validates update report source and target revision values so that they always get checked for validity, not only when doing non-client-pegged updates. * subversion/mod_dav_svn/reports/update.c (validate_input_revision): New helper function. (dav_svn__update_report): Always query the youngest FS revision, and use it (via validate_input_revision()) to raise errors when the client requests an update to a revision younger than is available or reports that it has objects whose revision is greater than the youngest. How hard would it be to port this one back to 1.7? I don't think it would be hard at all. I certainly plan to make the attempt today. One of the reasons I was suggesting is that I was thinking that this code might also relate to the errors reported by temporarily out of sync proxies, but I now think those originate in the reporter handling. Modified: subversion/trunk/subversion/tests/cmdline/update_tests.py URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/ update_tests.py?rev=1403588r1=1403587r2=1403588view=diff == --- subversion/trunk/subversion/tests/cmdline/update_tests.py (original) +++ subversion/trunk/subversion/tests/cmdline/update_tests.py Tue Oct 30 00:23:38 2012 @@ -5516,7 +5516,7 @@ def update_to_HEAD_plus_1(sbox): svntest.actions.run_and_verify_update(wc_dir, None, None, None, -.*No such revision, +.*E160006, If possible, please also keep some part(s) of an error message in the regex, to make the testsuite verify that we keep failing for the same reason. (In this case it checks for SVN_ERR_FS_NO_SUCH_REVISION, which already tells almost the whole story, but I don't see that in the test suite code) Good suggestion. The No such revision ... is now (for DAV servers) No such [target|reported] revision Would .*No such.*revision serve your desires here? Yes great. I think you could use E160006.*No such.*revision. (If I remember correctly we don't need a .* at the start.) Bert
Re: Race in svn_atomic_namespace__create
Stefan Fuhrmann stefan.fuhrm...@wandisco.com writes: Maybe, there should be a regression test that tries concurrent initialization. There are two objects here: the named file and the shared memory segment. The usual sequence for a single process is: create file, create segment, delete segment, delete file. There are two windows when the file exists and the segment does not and at that point a second process can neither attach nor create. The create window is protected by the inter-process lock but I think the delete window is not protected as it is triggered by a pool cleanup. We need to find a way to either take the inter-process lock or introduce some sort of delay/retry. However I think there is a bigger problem: I don't think this use of APR named memory allows processes to start and stop in an unordered fashion. I've built APR 1.4.x and configure chooses: apr.h:#define APR_USE_SHMEM_MMAP_TMP 0 apr.h:#define APR_USE_SHMEM_MMAP_SHM 0 apr.h:#define APR_USE_SHMEM_MMAP_ZERO0 apr.h:#define APR_USE_SHMEM_SHMGET_ANON 0 apr.h:#define APR_USE_SHMEM_SHMGET 1 apr.h:#define APR_USE_SHMEM_MMAP_ANON1 apr.h:#define APR_USE_SHMEM_BEOS 0 Start a process, svnadmin in my case, and apr_shm_attach fails as this is the first process so apr_shm_create is run. This creates the named file and the shared memory segment. The apr call installs a cleanup handler shm_cleanup_owner. At this point I see the shared memory segment: $ ipcs -m | grep 7340039 0x0101e244 7340039pm 60065536 1 and the process using it: $ lsof | grep 7340039 lt-svnadm 24614 pm DEL REG0,4 7340039 /SYSV0101e244 Start a second process, this time the apr_shm_attach works and the two processes use the same shared memory segment. At this point I see two processes using the shared memory segment: $ lsof | grep 7340039 lt-svnadm 24614 pm DEL REG0,4 7340039 /SYSV0101e244 lt-svnadm 24623 pm DEL REG0,4 7340039 /SYSV0101e244 Allow the first process to exit. The cleanup handler detaches from the shared memory removes the named file. The second process still uses the segment but the named file is deleted. At this point the shared memory segment key has changed: $ ipcs -m | grep 7340039 0x 7340039pm 60065536 1 dest but the second process is still using it: lt-svnadm 24623 pm DEL REG0,4 7340039 /SYSV0101e244 Start a third process, the apr_shm_attach fails because the file is removed. So this process creates a new named file and a second shared memory segment. At this point I see two shared memory segments: $ ipcs -m | egrep '(7340039|7372808)' 0x 7340039pm 60065536 1 dest 0x0101e245 7372808pm 60065536 1 and the second and third processes are using different segments: $ lsof | egrep '(7340039|7372808)' lt-svnadm 24623 pm DEL REG0,4 7340039 /SYSV0101e244 lt-svnadm 24637 pm DEL REG0,4 7372808 /SYSV0101e245 That's a serious problem, the two processes are not longer using the same shared memory segment to keep in sync. Changes made by one process won't be visible to another. -- Certified Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
Re: [PATCH] Implement svnadmin verify --force
Thanks to Stefan and Daniel. Attaching a new patch addressing the suggestions given by Stefan and Daniel. Hope this is good :) Edited the log message also. Thanks and regards Prabhu On 10/29/2012 11:53 PM, Stefan Sperling wrote: On Mon, Oct 29, 2012 at 10:45:19PM +0530, Prabhu Gnana Sundar wrote: Thank you so much Stefan for your patience in reviewing and guiding me through. I have addressed your points in the patch attached in this mail. I hope I addressed all the suggestions given by you :) Please share your views. Sure, see below. I don't think we're quite done yet but we're getting close :) Index: subversion/svnadmin/main.c === --- subversion/svnadmin/main.c (revision 1402414) +++ subversion/svnadmin/main.c (working copy) @@ -738,6 +743,11 @@ notify-warning_str)); return; +case svn_repos_notify_failure: + svn_handle_error2(notify-err, stderr, FALSE /* non-fatal */, +svnadmin: ); + return; + Running this on a repository with 5 revisions, where r3 is corrupt, the output looks as follows. $ svnadmin verify --keep-going repos * Verified revision 0. * Verified revision 1. * Verified revision 2. svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787) svnadmin: E140001: zlib (uncompress): corrupt data: Decompression of svndiff data failed * Verified revision 5. $ I've removed error tracing output as shown by a maintainer-mode build to match what it would look like in a release build. What I don't like is that there is no visual indication at all that tells me which revision an error corresponds to. The first error message is from r3, which is obvious only because the error message text itself happens to mention the revision number. The second error message is from r4, but I know that only because I already know what the corruption is because I've deliberately corrupted the repository myself :) So I'd suggest printing the revision number as part of the error message. Something like this should do: $ svnadmin verify --keep-going repos * Verified revision 0. * Verified revision 1. * Verified revision 2. svnadmin: Error verifying revision 3: E160004: Missing node-id in node-rev at r3 (offset 787) svnadmin: Error verifying revision 4: E140001: zlib (uncompress): corrupt data: Decompression of svndiff data failed * Verified revision 5. $ And while we're here, it would be great to do the same if the user did not pass the --keep-going option: $ svnadmin verify repos * Verified revision 0. * Verified revision 1. * Verified revision 2. svnadmin: Error verifying revision 3: E160004: Missing node-id in node-rev at r3 (offset 787) $ Do you agree that this is better? If so, please amend the patch accordingly. You can use the existing 'revision' field in svn_repos_notify_t to communicate a revision number to the notification handler. Index: subversion/libsvn_repos/dump.c === --- subversion/libsvn_repos/dump.c (revision 1402414) +++ subversion/libsvn_repos/dump.c (working copy) @@ -1360,9 +1360,10 @@ } svn_error_t * -svn_repos_verify_fs2(svn_repos_t *repos, +svn_repos_verify_fs3(svn_repos_t *repos, svn_revnum_t start_rev, svn_revnum_t end_rev, + svn_boolean_t keep_going, svn_repos_notify_func_t notify_func, void *notify_baton, svn_cancel_func_t cancel_func, @@ -1374,6 +1375,7 @@ svn_revnum_t rev; apr_pool_t *iterpool = svn_pool_create(pool); svn_repos_notify_t *notify; + svn_error_t *error; We usually call these variables 'err', not 'error'. Please follow this stylistic pattern. /* Determine the current youngest revision of the filesystem. */ SVN_ERR(svn_fs_youngest_rev(youngest, fs, pool)); @@ -1397,9 +1399,17 @@ end_rev, youngest); /* Verify global/auxiliary data and backend-specific data first. */ - SVN_ERR(svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton, -start_rev, end_rev, pool)); + error = svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton, +start_rev, end_rev, pool); + if(error !keep_going) Please always add a space between 'if' and the opening brace, like this: if (err !keep_going) We do the same for 'while', 'for', and a bunch of other C keywords. The goal here is consistency, there's no point in arguing which style is better. We're just following the style that the project has agreed on using. +SVN_ERR(error); If an error is known to be non-NULL (i.e. it is not SVN_NO_ERROR), you can use svn_error_trace() instead of SVN_ERR(): return svn_error_trace(err); + /* We don't have to notify this failure when keep_going is TRUE since + * that is taken care
Re: [PATCH] Implement svnadmin verify --force
Prabhu Gnana Sundar wrote on Tue, Oct 30, 2012 at 19:22:31 +0530:
Thanks to Stefan and Daniel.
Attaching a new patch addressing the suggestions given by Stefan and
Daniel. Hope this is good :)
Edited the log message also.
Index: subversion/libsvn_repos/dump.c
===
--- subversion/libsvn_repos/dump.c(revision 1402414)
+++ subversion/libsvn_repos/dump.c(working copy)
@@ -1360,9 +1360,10 @@
}
svn_error_t *
-svn_repos_verify_fs2(svn_repos_t *repos,
+svn_repos_verify_fs3(svn_repos_t *repos,
svn_revnum_t start_rev,
svn_revnum_t end_rev,
+ svn_boolean_t keep_going,
svn_repos_notify_func_t notify_func,
void *notify_baton,
svn_cancel_func_t cancel_func,
@@ -1374,6 +1375,7 @@
svn_revnum_t rev;
apr_pool_t *iterpool = svn_pool_create(pool);
svn_repos_notify_t *notify;
+ svn_error_t *err;
/* Determine the current youngest revision of the filesystem. */
SVN_ERR(svn_fs_youngest_rev(youngest, fs, pool));
@@ -1397,8 +1399,19 @@
end_rev, youngest);
/* Verify global/auxiliary data and backend-specific data first. */
- SVN_ERR(svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton,
-start_rev, end_rev, pool));
+ err = svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton,
+ start_rev, end_rev, pool);
+ if (err keep_going)
+{
+ svn_repos_notify_t *notify_failure;
+ notify_failure = svn_repos_notify_create(svn_repos_notify_failure,
+ iterpool);
+ notify_failure-err = err;
+ notify_func(notify_baton, notify_failure, iterpool);
+ svn_error_clear(err);
+}
+ else
+return svn_error_trace(err);
This pattern repeats three times, maybe introduce a macro (like SVN_ERR,
SVN_INT_ERR, etc) to improve readability?
Oh, and NOTIFY_FUNC may be NULL. (if that happens, trying to call it
will segfault) I think the docstring needs to document what happens
when NOTIFY_FUNC is NULL and KEEP_GOING is TRUE.
* subversion/libsvn_repos/dump.c
(svn_repos_verify_fs3): Handle keep-going. If keep-going is TRUE, the
error message is notified and verification process continues.
Normally I mention svn_repos_verify_fs2() too here, with a text
description Deprecate.. (in the imperative, not past)
* subversion/libsvn_repos/deprecated.c
(svn_repos_verify_fs2): Deprecated. Call svn_repos_verify_fs3 with
keep_going as FALSE by default to keep the old default implementation.
* subversion/libsvn_repos/notify.c
(svn_repos_notify_create): Initialise the error chain err to SVN_NO_ERROR.
Patch by: Prabhu Gnana Sundar prabhugs{_AT_}collab.net
Reviewed by: Stefan Sperling stsp{_AT_}elego.de
Looks good otherwise.
Re: svn commit: r1403588 - in /subversion/trunk/subversion: mod_dav_svn/reports/update.c tests/cmdline/update_tests.py
On 10/30/2012 08:58 AM, Bert Huijben wrote: How hard would it be to port this one back to 1.7? I don't think it would be hard at all. I certainly plan to make the attempt today. One of the reasons I was suggesting is that I was thinking that this code might also relate to the errors reported by temporarily out of sync proxies, but I now think those originate in the reporter handling. My changes to do affect temporarily out-of-sync proxies. Yes great. I think you could use E160006.*No such.*revision. (If I remember correctly we don't need a .* at the start.) Done. (And proposed alongside r1403588 for backport.) -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Enterprise Cloud Development signature.asc Description: OpenPGP digital signature
Re: [PATCH] Implement svnadmin verify --force
On Tue, Oct 30, 2012 at 07:22:31PM +0530, Prabhu Gnana Sundar wrote: Index: subversion/svnadmin/main.c === --- subversion/svnadmin/main.c(revision 1402414) +++ subversion/svnadmin/main.c(working copy) @@ -738,6 +743,14 @@ notify-warning_str)); return; +case svn_repos_notify_failure: + svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool, +_(svnadmin: Error verifying revision %ld. ), +notify-revision)); + svn_handle_error2(notify-err, stderr, FALSE /* non-fatal */, +(svnadmin: )); No brackets needed around svnadmin: . It look as if you meant to write _(svnadmin: ), marking the string for translation using _(). But the program name should not be translated. + return; + case svn_repos_notify_dump_rev_end: svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool, _(* Dumped revision %ld.\n), On my system your patch produces the following output: $ svnadmin verify --keep-going repos svnadmin: Error verifying revision 0. subversion/libsvn_fs/fs-loader.c:500: (apr_err=160004) subversion/libsvn_fs_fs/fs_fs.c:10167: (apr_err=160004) subversion/libsvn_fs_fs/tree.c:608: (apr_err=160004) subversion/libsvn_fs_fs/dag.c:611: (apr_err=160004) subversion/libsvn_fs_fs/fs_fs.c:3024: (apr_err=160004) subversion/libsvn_fs_fs/fs_fs.c:2775: (apr_err=160004) svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787) * Verified revision 0. * Verified revision 1. * Verified revision 2. svnadmin: Error verifying revision 3. subversion/libsvn_repos/dump.c:983: (apr_err=160004) subversion/libsvn_fs/fs-loader.c:858: (apr_err=160004) subversion/libsvn_fs_fs/tree.c:608: (apr_err=160004) subversion/libsvn_fs_fs/dag.c:611: (apr_err=160004) subversion/libsvn_fs_fs/fs_fs.c:3024: (apr_err=160004) subversion/libsvn_fs_fs/fs_fs.c:2775: (apr_err=160004) svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787) svnadmin: Error verifying revision 4. subversion/libsvn_delta/path_driver.c:263: (apr_err=140001) subversion/libsvn_repos/replay.c:664: (apr_err=140001) subversion/libsvn_delta/cancel.c:197: (apr_err=140001) subversion/libsvn_repos/dump.c:829: (apr_err=140001) subversion/libsvn_repos/dump.c:628: (apr_err=140001) subversion/libsvn_subr/stream.c:143: (apr_err=140001) subversion/libsvn_fs_fs/fs_fs.c:5058: (apr_err=140001) subversion/libsvn_fs_fs/fs_fs.c:5033: (apr_err=140001) subversion/libsvn_fs_fs/fs_fs.c:4886: (apr_err=140001) subversion/libsvn_fs_fs/fs_fs.c:4849: (apr_err=140001) subversion/libsvn_delta/svndiff.c:726: (apr_err=140001) subversion/libsvn_delta/svndiff.c:563: (apr_err=140001) subversion/libsvn_repos/dump.c:983: (apr_err=140001) svnadmin: E140001: zlib (uncompress): corrupt data: Decompression of svndiff data failed * Verified revision 5. $ The first error seems to come from the fs-layer verification function. This error also appears without your patch. Apparently fs-layer verification finds corruption in r3. But fs-layer verification errors shouldn't be associated with a particular revision in the output. And I'd prefer to have each error message appear on a single line, with prefixes separated by colons instead of full-stops. So I'd expect the output to look as follows: $ svnadmin verify --keep-going repos subversion/libsvn_fs_fs/fs_fs.c:10167: (apr_err=160004) subversion/libsvn_fs_fs/tree.c:608: (apr_err=160004) subversion/libsvn_fs_fs/dag.c:611: (apr_err=160004) subversion/libsvn_fs_fs/fs_fs.c:3024: (apr_err=160004) subversion/libsvn_fs_fs/fs_fs.c:2775: (apr_err=160004) svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787) * Verified revision 0. * Verified revision 1. * Verified revision 2. subversion/libsvn_fs/fs-loader.c:858: (apr_err=160004) subversion/libsvn_fs_fs/tree.c:608: (apr_err=160004) subversion/libsvn_fs_fs/dag.c:611: (apr_err=160004) subversion/libsvn_fs_fs/fs_fs.c:3024: (apr_err=160004) subversion/libsvn_fs_fs/fs_fs.c:2775: (apr_err=160004) svnadmin: Error verifying revision 3: E160004: Missing node-id in node-rev at r3 (offset 787) subversion/libsvn_repos/replay.c:664: (apr_err=140001) subversion/libsvn_delta/cancel.c:197: (apr_err=140001) subversion/libsvn_repos/dump.c:829: (apr_err=140001) subversion/libsvn_repos/dump.c:628: (apr_err=140001) subversion/libsvn_subr/stream.c:143: (apr_err=140001) subversion/libsvn_fs_fs/fs_fs.c:5058: (apr_err=140001) subversion/libsvn_fs_fs/fs_fs.c:5033: (apr_err=140001) subversion/libsvn_fs_fs/fs_fs.c:4886: (apr_err=140001) subversion/libsvn_fs_fs/fs_fs.c:4849: (apr_err=140001) subversion/libsvn_delta/svndiff.c:726: (apr_err=140001) subversion/libsvn_delta/svndiff.c:563: (apr_err=140001) subversion/libsvn_repos/dump.c:983: (apr_err=140001) svnadmin: Error verifying revision 4: E140001:
Re: [PATCH] Implement svnadmin verify --force
On Tue, Oct 30, 2012 at 04:07:49PM +0200, Daniel Shahaf wrote:
Prabhu Gnana Sundar wrote on Tue, Oct 30, 2012 at 19:22:31 +0530:
+ if (err keep_going)
+{
+ svn_repos_notify_t *notify_failure;
+ notify_failure = svn_repos_notify_create(svn_repos_notify_failure,
+ iterpool);
+ notify_failure-err = err;
+ notify_func(notify_baton, notify_failure, iterpool);
+ svn_error_clear(err);
+}
+ else
+return svn_error_trace(err);
This pattern repeats three times, maybe introduce a macro (like SVN_ERR,
SVN_INT_ERR, etc) to improve readability?
I'd prefer a new helper function instead of a new macro.
Perhaps something like:
if (err keep_going)
notify_verification_error(err, rev, pool);
else
SVN_ERR(err);
[PATCH] Add platform information to user agent string
Hi, I think it will be useful to add client platform information to user agent string, to use it Apache HTTP Server configuration or Subversion hooks. Within my patch applied user agent will be like this: SVN/1.8.0 (Windows) serf/1.1.1 TortoiseSVN/1.8.0 SVN/1.8.0 (Macintosh) serf/1.1.1 SVN/1.8.0 (FreeBSD) serf/1.1.1 [[[ * subversion/libsvn_ra_serf/ra_serf.h (USER_AGENT_PLATFORM): New. (USER_AGENT): Add platform information. ]]] Any objections? I've tested it only on Windows, so more testing would be useful. [1] http://sourceforge.net/p/predef/wiki/OperatingSystems/ -- Ivan Zhakov Index: subversion/libsvn_ra_serf/ra_serf.h === --- subversion/libsvn_ra_serf/ra_serf.h (revision 1403730) +++ subversion/libsvn_ra_serf/ra_serf.h (working copy) @@ -56,8 +56,27 @@ /** Use this to silence compiler warnings about unused parameters. */ #define UNUSED_CTX(x) ((void)(x)) -/** Our User-Agent string. */ -#define USER_AGENT SVN/ SVN_VER_NUMBER serf/ \ +#if defined(_WIN32) +#define USER_AGENT_PLATFORM Windows +#elif __APPLE__ +#define USER_AGENT_PLATFORM Macintosh +#elif __linux__ +#define USER_AGENT_PLATFORM Linux +#elif __FreeBSD__ +#define USER_AGENT_PLATFORM FreeBSD +#elif __NetBSD__ +#define USER_AGENT_PLATFORM NetBSD +#elif __OpenBSD__ +#define USER_AGENT_PLATFORM OpenBSD +#elif __unix__ +#define USER_AGENT_PLATFORM Unix +#else +#define USER_AGENT_PLATFORM Unknown +#endif + +/** Our User-Agent string. */ +#define USER_AGENT SVN/ SVN_VER_NUMBER ( USER_AGENT_PLATFORM ) \ +serf/ \ APR_STRINGIFY(SERF_MAJOR_VERSION) . \ APR_STRINGIFY(SERF_MINOR_VERSION) . \ APR_STRINGIFY(SERF_PATCH_VERSION)
Re: [PATCH] Add platform information to user agent string
On Tue, Oct 30, 2012 at 07:28:27PM +0400, Ivan Zhakov wrote: Hi, I think it will be useful to add client platform information to user agent string, to use it Apache HTTP Server configuration or Subversion hooks. Within my patch applied user agent will be like this: SVN/1.8.0 (Windows) serf/1.1.1 TortoiseSVN/1.8.0 SVN/1.8.0 (Macintosh) serf/1.1.1 SVN/1.8.0 (FreeBSD) serf/1.1.1 [[[ * subversion/libsvn_ra_serf/ra_serf.h (USER_AGENT_PLATFORM): New. (USER_AGENT): Add platform information. ]]] Any objections? Some users might not want to broadcast this information. Should we provide a way for users to override this? Maybe from the client configuration file? -/** Our User-Agent string. */ -#define USER_AGENT SVN/ SVN_VER_NUMBER serf/ \ +#if defined(_WIN32) +#define USER_AGENT_PLATFORM Windows +#elif __APPLE__ +#define USER_AGENT_PLATFORM Macintosh +#elif __linux__ +#define USER_AGENT_PLATFORM Linux +#elif __FreeBSD__ +#define USER_AGENT_PLATFORM FreeBSD +#elif __NetBSD__ +#define USER_AGENT_PLATFORM NetBSD +#elif __OpenBSD__ +#define USER_AGENT_PLATFORM OpenBSD +#elif __unix__ +#define USER_AGENT_PLATFORM Unix +#else +#define USER_AGENT_PLATFORM Unknown +#endif A hard-coded list like this will need to be maintained over time. Note that a trunk client prints the OS name with this command: svn --verbose --version I'd suggest getting the OS name the same way in libsvn_ra_serf.
Re: [PATCH] Add platform information to user agent string
On Tue, Oct 30, 2012 at 7:49 PM, Stefan Sperling s...@elego.de wrote: On Tue, Oct 30, 2012 at 07:28:27PM +0400, Ivan Zhakov wrote: Hi, I think it will be useful to add client platform information to user agent string, to use it Apache HTTP Server configuration or Subversion hooks. Within my patch applied user agent will be like this: SVN/1.8.0 (Windows) serf/1.1.1 TortoiseSVN/1.8.0 SVN/1.8.0 (Macintosh) serf/1.1.1 SVN/1.8.0 (FreeBSD) serf/1.1.1 [[[ * subversion/libsvn_ra_serf/ra_serf.h (USER_AGENT_PLATFORM): New. (USER_AGENT): Add platform information. ]]] Any objections? Some users might not want to broadcast this information. Should we provide a way for users to override this? Maybe from the client configuration file? You already broadcast this information when browsing the Internet :) Also we already broadcast SVN version information. So I don't see big issue here. -- Ivan Zhakov
Re: [PATCH] Add platform information to user agent string
On Tue, Oct 30, 2012 at 8:28 AM, Ivan Zhakov i...@visualsvn.com wrote: I think it will be useful to add client platform information to user agent string, to use it Apache HTTP Server configuration or Subversion hooks. Within my patch applied user agent will be like this: SVN/1.8.0 (Windows) serf/1.1.1 TortoiseSVN/1.8.0 SVN/1.8.0 (Macintosh) serf/1.1.1 SVN/1.8.0 (FreeBSD) serf/1.1.1 Useful how?
Re: [PATCH] Add platform information to user agent string
On Tue, Oct 30, 2012 at 08:00:00PM +0400, Ivan Zhakov wrote: You already broadcast this information when browsing the Internet :) Not with a custom user agent string a browser allows me to set :) Also we already broadcast SVN version information. So I don't see big issue here. Of course it's not a big issue. I just wanted to point this out. The ability to override the user agent string can be added later. My concerns about the hard-coded list of OS names are more important to me.
Re: [PATCH] Add platform information to user agent string
On Tue, Oct 30, 2012 at 8:07 PM, Ben Reser b...@reser.org wrote: On Tue, Oct 30, 2012 at 8:28 AM, Ivan Zhakov i...@visualsvn.com wrote: I think it will be useful to add client platform information to user agent string, to use it Apache HTTP Server configuration or Subversion hooks. Within my patch applied user agent will be like this: SVN/1.8.0 (Windows) serf/1.1.1 TortoiseSVN/1.8.0 SVN/1.8.0 (Macintosh) serf/1.1.1 SVN/1.8.0 (FreeBSD) serf/1.1.1 Useful how? For example to disable plain-text basic authentication on Windows. Or disable commits from Linux using pre-commit hook. -- Ivan Zhakov
Re: svn commit: r1403733 - /subversion/trunk/subversion/libsvn_ra_serf/serf.c
i...@apache.org wrote on Tue, Oct 30, 2012 at 15:09:57 -: Author: ivan Date: Tue Oct 30 15:09:56 2012 New Revision: 1403733 URL: http://svn.apache.org/viewvc?rev=1403733view=rev Log: * subversion/libsvn_ra_serf/serf.c (svn_ra_serf__open): Use whitespace instead of '/' as delimiter for custom user-agent string. Modified: subversion/trunk/subversion/libsvn_ra_serf/serf.c Modified: subversion/trunk/subversion/libsvn_ra_serf/serf.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/serf.c?rev=1403733r1=1403732r2=1403733view=diff == --- subversion/trunk/subversion/libsvn_ra_serf/serf.c (original) +++ subversion/trunk/subversion/libsvn_ra_serf/serf.c Tue Oct 30 15:09:56 2012 @@ -417,7 +417,7 @@ svn_ra_serf__open(svn_ra_session_t *sess callbacks-get_client_string(callback_baton, client_string, pool); if (client_string) -serf_sess-useragent = apr_pstrcat(pool, USER_AGENT, /, +serf_sess-useragent = apr_pstrcat(pool, USER_AGENT, , Change libsvn_ra_svn/client.c:open_session too in the same way? client_string, (char *)NULL); else serf_sess-useragent = USER_AGENT;
Re: Race in svn_atomic_namespace__create
Philip Martin philip.mar...@wandisco.com writes: That's a serious problem, the two processes are not longer using the same shared memory segment to keep in sync. Changes made by one process won't be visible to another. I don't see how to fix this with the current APR code. The process that creates the named shared memory will always delete the associated file at pool cleanup and that stops further processes attaching. I guess this is designed for a server where the parent process passes shared memory to child processes but it doesn't really fit our model of independent processes. I think we would have to change APR to get this to work. An alternative approach would use anonymous shared memory and have Subversion itself manage the ID outside APR (storing it in the lock file perhaps) but APR doesn't support that either: APR doesn't make the ID available or support attaching to anonymous shared memory. Again, we would have to add support to APR and then require the new APR version. Another approach would be to create the shared memory created from some other, long-lived, process. The user would have to run this process to enable caching. To handle a large number of repositories this would probably have to be some sort of daemon. Perhaps we could use apr_mmap_create instead? APR doesn't specify whether the mmap is SHARED or PRIVATE but the current implementation on Unix is shared. I don't know enough about Windows to understand how that implementation behaves. -- Certified Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
Re: [PATCH] Add platform information to user agent string
Ivan Zhakov wrote: On Tue, Oct 30, 2012 at 8:07 PM, Ben Reser wrote: On Tue, Oct 30, 2012 at 8:28 AM, Ivan Zhakov wrote: I think it will be useful to add client platform information to user agent string, to use it Apache HTTP Server configuration or Subversion hooks. Within my patch applied user agent will be like this: SVN/1.8.0 (Windows) serf/1.1.1 TortoiseSVN/1.8.0 SVN/1.8.0 (Macintosh) serf/1.1.1 SVN/1.8.0 (FreeBSD) serf/1.1.1 Useful how? For example to disable plain-text basic authentication on Windows. Or disable commits from Linux using pre-commit hook. (1) Brane has recently added platform identification to the 'svn --version' command; is there any reason to use different code here producing a different set of platform identifiers? It would seem much better to share that code -- whether using just the 'platform' string or the whole 'platform-cpu-vendor' (or whatever it is) tuple -- so that the two sets of client identifiers cannot get out of sync. (2) C-Mike Pilato (?) has recently added some sort of ability for pre-commit hooks to see more information (via txn-rev-props), which AFAIK was partly intended to let the hook see more info about the client platform. I don't know exactly what it does, but isn't that a more suitable place to put any new extensions of this kind of functionality? How does what you're proposing relate to those two developments? - Julian
Re: Race in svn_atomic_namespace__create
On Tue, Oct 30, 2012 at 05:58:20PM +, Philip Martin wrote: Another approach would be to create the shared memory created from some other, long-lived, process. The user would have to run this process to enable caching. To handle a large number of repositories this would probably have to be some sort of daemon. Can we require an auxilliary process such as memcached to hold cached revprop data instead of caching it in-process? Or would that perform worse or equal to no caching at all?
Re: [PATCH] Add platform information to user agent string
On 10/30/2012 02:15 PM, Julian Foad wrote: Ivan Zhakov wrote: On Tue, Oct 30, 2012 at 8:07 PM, Ben Reser wrote: On Tue, Oct 30, 2012 at 8:28 AM, Ivan Zhakov wrote: I think it will be useful to add client platform information to user agent string, to use it Apache HTTP Server configuration or Subversion hooks. Within my patch applied user agent will be like this: SVN/1.8.0 (Windows) serf/1.1.1 TortoiseSVN/1.8.0 SVN/1.8.0 (Macintosh) serf/1.1.1 SVN/1.8.0 (FreeBSD) serf/1.1.1 Useful how? For example to disable plain-text basic authentication on Windows. Or disable commits from Linux using pre-commit hook. (1) Brane has recently added platform identification to the 'svn --version' command; is there any reason to use different code here producing a different set of platform identifiers? It would seem much better to share that code -- whether using just the 'platform' string or the whole 'platform-cpu-vendor' (or whatever it is) tuple -- so that the two sets of client identifiers cannot get out of sync. Agreed that there should be a single canonical source of build information, and that the work Brane has done seems the reasonable best candidate of such. (2) C-Mike Pilato (?) has recently added some sort of ability for pre-commit hooks to see more information (via txn-rev-props), which AFAIK was partly intended to let the hook see more info about the client platform. I don't know exactly what it does, but isn't that a more suitable place to put any new extensions of this kind of functionality? The stuff I did bypasses the logs altogether and is more useful for letting hook scripts make decisions based on client *versions* (feature compatability). My changes don't take client platform into account at all, but of course could be expanded to do so. One benefit of Ivan's changes are that the platform information will get logged in the Apache logs in the same fashion that a browser's User-Agent string would. -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Enterprise Cloud Development signature.asc Description: OpenPGP digital signature
Re: [PATCH] Add platform information to user agent string
On 10/30/2012 02:15 PM, Julian Foad wrote: Ivan Zhakov wrote: On Tue, Oct 30, 2012 at 8:07 PM, Ben Reser wrote: On Tue, Oct 30, 2012 at 8:28 AM, Ivan Zhakov wrote: I think it will be useful to add client platform information to user agent string, to use it Apache HTTP Server configuration or Subversion hooks. Within my patch applied user agent will be like this: SVN/1.8.0 (Windows) serf/1.1.1 TortoiseSVN/1.8.0 SVN/1.8.0 (Macintosh) serf/1.1.1 SVN/1.8.0 (FreeBSD) serf/1.1.1 Useful how? For example to disable plain-text basic authentication on Windows. Or disable commits from Linux using pre-commit hook. (1) Brane has recently added platform identification to the 'svn --version' command; is there any reason to use different code here producing a different set of platform identifiers? It would seem much better to share that code -- whether using just the 'platform' string or the whole 'platform-cpu-vendor' (or whatever it is) tuple -- so that the two sets of client identifiers cannot get out of sync. Agreed that there should be a single canonical source of build information, and that the work Brane has done seems the reasonable best candidate of such. (2) C-Mike Pilato (?) has recently added some sort of ability for pre-commit hooks to see more information (via txn-rev-props), which AFAIK was partly intended to let the hook see more info about the client platform. I don't know exactly what it does, but isn't that a more suitable place to put any new extensions of this kind of functionality? By modifying the user-agent string in this fashion, Ivan is effectively making use of my enhancements. And because of how he chose to do so, the additional platform information will also show up in Apache log (like a browser's User-Agent string does), too. So, I've no complaints about the approach he used to make the changes. I echo, though, the concern expressed here and elsethread about the static list of platforms. -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Enterprise Cloud Development signature.asc Description: OpenPGP digital signature
Re: [PATCH] Add platform information to user agent string
[ THIS WAS AN EARLIER DRAFT OF MY INTENDED MAIL. NOT SURE ] [ HOW IT HIT THE LIST. PLEASE IGNORE. ] On 10/30/2012 02:39 PM, C. Michael Pilato wrote: On 10/30/2012 02:15 PM, Julian Foad wrote: Ivan Zhakov wrote: On Tue, Oct 30, 2012 at 8:07 PM, Ben Reser wrote: On Tue, Oct 30, 2012 at 8:28 AM, Ivan Zhakov wrote: I think it will be useful to add client platform information to user agent string, to use it Apache HTTP Server configuration or Subversion hooks. Within my patch applied user agent will be like this: SVN/1.8.0 (Windows) serf/1.1.1 TortoiseSVN/1.8.0 SVN/1.8.0 (Macintosh) serf/1.1.1 SVN/1.8.0 (FreeBSD) serf/1.1.1 Useful how? For example to disable plain-text basic authentication on Windows. Or disable commits from Linux using pre-commit hook. (1) Brane has recently added platform identification to the 'svn --version' command; is there any reason to use different code here producing a different set of platform identifiers? It would seem much better to share that code -- whether using just the 'platform' string or the whole 'platform-cpu-vendor' (or whatever it is) tuple -- so that the two sets of client identifiers cannot get out of sync. Agreed that there should be a single canonical source of build information, and that the work Brane has done seems the reasonable best candidate of such. (2) C-Mike Pilato (?) has recently added some sort of ability for pre-commit hooks to see more information (via txn-rev-props), which AFAIK was partly intended to let the hook see more info about the client platform. I don't know exactly what it does, but isn't that a more suitable place to put any new extensions of this kind of functionality? The stuff I did bypasses the logs altogether and is more useful for letting hook scripts make decisions based on client *versions* (feature compatability). My changes don't take client platform into account at all, but of course could be expanded to do so. One benefit of Ivan's changes are that the platform information will get logged in the Apache logs in the same fashion that a browser's User-Agent string would. -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Enterprise Cloud Development signature.asc Description: OpenPGP digital signature
Re: [Bug] [Subversion 1.7] svn blame doesn't work for locally modified files
On Fri, Oct 14, 2011 at 6:49 AM, Daniel Shahaf d...@daniel.shahaf.name wrote: [ switching list, please drop users@ from further replies ] Konstantin Kolinko wrote on Fri, Oct 14, 2011 at 11:43:14 +0400: 2011/10/14 Andrey Paramonov para...@acdlabs.ru: On 13.10.2011 22:31, Ryan Schmidt wrote: On Oct 13, 2011, at 06:51, Andrey Paramonov wrote: I believe Subversion can automagically translate line ending sequences when transferring data to and from server. I use Windows, so I have CRLF sequences at the ends of the lines in my working copy. The question is: what is the EOL sequence on the server? On Oct 13, 2011, at 07:06, Andrey Paramonov wrote: On 13.10.2011 15:54, Daniel Shahaf wrote: What properties are set on your file? svn proplist returns nothing. If the svn:eol-style property is not set on the file, then Subversion does not modify the file in any way as it's stored in the repository. The EOL sequence (and every other byte in the file) is the same on the server as it is on the client. If, on the other hand, svn:eol-style is set to any valid value, then Subversion stores the file in the repository with LF line endings, and on checkout or export, the client translates the line endings to the style requested in the svn:eol-style property. By looking through the code in libsvn_client/blame.c I see that in svn_client_blame5 working copy version of the file is unconditionally normalized to have \n EOLs (by call to svn_subst_stream_translated). However, it seems that svn_ra_get_file_revs2 doesn't do EOL normalization, at least in my case (absent svn:eol-style). Confirming that the issue reproduces for me on Windows using a file that does not have svn:eol-style property. The svn blame filename@BASE, as mentioned earlier, works correctly, blaming unmodified base version of the file. If a file has svn:eol-style=native the issue does not reproduce and blame works correctly. @Both, thanks for the analysis. Could someone file a bug report and/or send a patch, too? If you have a build environment, does the patch below fix the problem? Hi All, Mike Pilato is tying off loose ends for the upcoming 1.8 release and asked me to check this patch for issue #4034 on my Windows machine. The patch doesn't appear to make any difference: With three files, with three different eols as per their names: trunk@1403813svn st -v 99 pburba . 99 pburba file-with-CR-eols 99 pburba file-with-CRLF-eols 99 pburba file-with-LF-eols All three files have the same contents but for eols: trunk@1403813type file-with-CRLF-eols A B C D E F Blame works fine with no local mods as expected: trunk@1403813svn blame file-with-CR-eols 9 pburba A 9 pburba B 9 pburba C 9 pburba D 9 pburba E 9 pburba F trunk@1403813svn blame file-with-CRLF-eols 9 pburba A 9 pburba B 9 pburba C 9 pburba D 9 pburba E 9 pburba F trunk@1403813svn blame file-with-LF-eols 9 pburba A 9 pburba B 9 pburba C 9 pburba D 9 pburba E 9 pburba F I edited line #3 in each file like this: trunk@1403813svn diff file-with-CRLF-eols Index: file-with-CRLF-eols === --- file-with-CRLF-eols (revision 9) +++ file-with-CRLF-eols (working copy) @@ -1,6 +1,6 @@ A B -C +C-mod D E F Without Daniels patch we see the bug described previously in the thread: trunk@1403813svn blame file-with-CR-eols - - A - - B - - C-mod - - D - - E - - F trunk@1403813svn blame file-with-CRLF-eols - - A - - B - - C-mod - - D - - E - - F trunk@1403813svn blame file-with-LF-eols 9 pburba A 9 pburba B - - C-mod 9 pburba D 9 pburba E 9 pburba F But with Daniel's patch I see no difference in the blame output: trunk@1403813svn blame file-with-CR-eols - - A - - B - - C-mod - - D - - E - - F trunk@1403813svn blame file-with-CRLF-eols - - A - - B - - C-mod - - D - - E - - F trunk@1403813svn blame file-with-LF-eols 9 pburba A 9 pburba B - - C-mod 9 pburba D 9 pburba E 9 pburba F -- Paul T. Burba CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development Skype: ptburba [[[ Index: subversion/libsvn_client/blame.c === ---
Re: [PATCH] Add platform information to user agent string
On Tue, Oct 30, 2012 at 10:42 PM, C. Michael Pilato cmpil...@collab.net wrote: On 10/30/2012 02:15 PM, Julian Foad wrote: Ivan Zhakov wrote: On Tue, Oct 30, 2012 at 8:07 PM, Ben Reser wrote: On Tue, Oct 30, 2012 at 8:28 AM, Ivan Zhakov wrote: I think it will be useful to add client platform information to user agent string, to use it Apache HTTP Server configuration or Subversion hooks. Within my patch applied user agent will be like this: SVN/1.8.0 (Windows) serf/1.1.1 TortoiseSVN/1.8.0 SVN/1.8.0 (Macintosh) serf/1.1.1 SVN/1.8.0 (FreeBSD) serf/1.1.1 Useful how? For example to disable plain-text basic authentication on Windows. Or disable commits from Linux using pre-commit hook. (1) Brane has recently added platform identification to the 'svn --version' command; is there any reason to use different code here producing a different set of platform identifiers? It would seem much better to share that code -- whether using just the 'platform' string or the whole 'platform-cpu-vendor' (or whatever it is) tuple -- so that the two sets of client identifiers cannot get out of sync. Agreed that there should be a single canonical source of build information, and that the work Brane has done seems the reasonable best candidate of such. I completely agree that it would be great to have one code to get platform information. But current svn_sysinfo* implementation provides detailed information which very expensive to obtain. In some codepath we have to execute external commands. This is fine for separate command, but I think that's does makes sense for every Subversion library call. That's why I suggest use static build time information in user-agent string. (2) C-Mike Pilato (?) has recently added some sort of ability for pre-commit hooks to see more information (via txn-rev-props), which AFAIK was partly intended to let the hook see more info about the client platform. I don't know exactly what it does, but isn't that a more suitable place to put any new extensions of this kind of functionality? By modifying the user-agent string in this fashion, Ivan is effectively making use of my enhancements. And because of how he chose to do so, the additional platform information will also show up in Apache log (like a browser's User-Agent string does), too. Exactly. That was my idea. -- Ivan Zhakov
Re: [PATCH] Add platform information to user agent string
On Wed, Oct 31, 2012 at 12:25:43AM +0400, Ivan Zhakov wrote: I completely agree that it would be great to have one code to get platform information. But current svn_sysinfo* implementation provides detailed information which very expensive to obtain. In some codepath we have to execute external commands. This is fine for separate command, but I think that's does makes sense for every Subversion library call. That's why I suggest use static build time information in user-agent string. Running uname is too expensive? Can you show some benchmark numbers to back this up? :) If it is prohibitively expensive perhaps you could use a global variable to store the OS name and set it up at startup with svn_atomic__init_once()?
Re: [PATCH] Add platform information to user agent string
On 30.10.2012 21:25, Ivan Zhakov wrote: On Tue, Oct 30, 2012 at 10:42 PM, C. Michael Pilato cmpil...@collab.net wrote: On 10/30/2012 02:15 PM, Julian Foad wrote: Ivan Zhakov wrote: On Tue, Oct 30, 2012 at 8:07 PM, Ben Reser wrote: On Tue, Oct 30, 2012 at 8:28 AM, Ivan Zhakov wrote: I think it will be useful to add client platform information to user agent string, to use it Apache HTTP Server configuration or Subversion hooks. Within my patch applied user agent will be like this: SVN/1.8.0 (Windows) serf/1.1.1 TortoiseSVN/1.8.0 SVN/1.8.0 (Macintosh) serf/1.1.1 SVN/1.8.0 (FreeBSD) serf/1.1.1 Useful how? For example to disable plain-text basic authentication on Windows. Or disable commits from Linux using pre-commit hook. (1) Brane has recently added platform identification to the 'svn --version' command; is there any reason to use different code here producing a different set of platform identifiers? It would seem much better to share that code -- whether using just the 'platform' string or the whole 'platform-cpu-vendor' (or whatever it is) tuple -- so that the two sets of client identifiers cannot get out of sync. Agreed that there should be a single canonical source of build information, and that the work Brane has done seems the reasonable best candidate of such. I completely agree that it would be great to have one code to get platform information. But current svn_sysinfo* implementation provides detailed information which very expensive to obtain. In some codepath we have to execute external commands. This is fine for separate command, but I think that's does makes sense for every Subversion library call. That's why I suggest use static build time information in user-agent string. The sysinfo bits have static (build-time) info and dynamic (runtime) info. Presumably the only difference will be noticing when you're running the program on a different size of OS, e.g., running 32-bit code on a 64-bit OS (hopefully in some compatibility mode). For the purpose of user agent strings, the host triplet exposed in the #define in svn_private_config.h should be more than good enough. -- Brane
Re: [Bug] [Subversion 1.7] svn blame doesn't work for locally modified files
Paul Burba wrote: Mike Pilato is tying off loose ends for the upcoming 1.8 release and asked me to check this patch for issue #4034 on my Windows machine. The patch doesn't appear to make any difference: With three files, with three different eols as per their names: [...] You don't appear to mention anything about setting svn:eol-style properties for these files. I assume that would be important. (For this reply I've removed users@ and all other persons from the CC, leaving just you and dev@.) - Julian
Re: [Bug] [Subversion 1.7] svn blame doesn't work for locally modified files
On Tue, Oct 30, 2012 at 6:08 PM, Julian Foad julianf...@btopenworld.com wrote: Paul Burba wrote: Mike Pilato is tying off loose ends for the upcoming 1.8 release and asked me to check this patch for issue #4034 on my Windows machine. The patch doesn't appear to make any difference: With three files, with three different eols as per their names: [...] You don't appear to mention anything about setting svn:eol-style properties for these files. I assume that would be important. Sorry, I neglected to mention that each file had svn:eol-style set as per its name: trunk@1403813svn pl -vR Properties on 'file-with-CR-eols': svn:eol-style CR Properties on 'file-with-CRLF-eols': svn:eol-style CRLF Properties on 'file-with-LF-eols': svn:eol-style LF -- Paul T. Burba CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development Skype: ptburba (For this reply I've removed users@ and all other persons from the CC, leaving just you and dev@.) - Julian
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: r1403982 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
bre...@apache.org wrote on Wed, Oct 31, 2012 at 04:59:43 -:
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.
Found by: danielsh
(danielsh found the leak of the errors, I found the const)
I did think the lack of const on that TXN_ID parameter was funny...
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=1403982r1=1403981r2=1403982view=diff
==
--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Wed Oct 31 04:59:42 2012
@@ -7086,21 +7086,33 @@ static apr_status_t
rep_write_cleanup(void *data)
{
struct rep_write_baton *b = data;
- char *txn_id = svn_fs_fs__id_txn_id(b-noderev-id);
+ const char *txn_id = svn_fs_fs__id_txn_id(b-noderev-id);
svn_error_t *err;
Re: svn commit: r1403982 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
On Tue, Oct 30, 2012 at 10:02 PM, Blair Zajac bl...@orcaware.com wrote: Looking at the diff, do you mean 'should be a const'? Thanks, log fixed.
